Add complete bank support for CMIS transceivers by bobby-nexthop · Pull Request #640 · sonic-net/sonic-platform-common

bobby-nexthop · 2026-03-24T21:55:56Z

Description

This PR extends the bank parameter support introduced in PR #632 to fully enable multi-bank CMIS transceiver support throughout the stack.

Changes include:

Memory Map Bank Awareness:
- Updated CmisFlatMemMap, CmisMemMap, and CCmisMemMap to accept bank parameter
- Implemented bank-aware getaddr() method in CmisFlatMemMap that correctly calculates linear offsets using the formula from sonic-linux-kernel PR
- Added bank property to CmisFlatMemMap for read-only access
- Added constants: CMIS_EEPROM_PAGE_SIZE, CMIS_NUM_NON_BANKED_PAGES, CMIS_NUM_BANKED_PAGES, CMIS_ARCH_PAGES
- Uses unified formula: linear_offset = (bank * CMIS_ARCH_PAGES + page) * page_size + offset
- Each bank is treated as a full 256-page (32KB) architectural block for proper alignment
API Factory Updates:
- Added bank parameter to create_xcvr_api() and _create_cmis_api()
- Updated CMIS API instantiation to pass bank parameter to memory maps
- Modified id_mapping to pass bank parameter for all CMIS module types (0x18, 0x19, 0x1b, 0x1e)
SfpBase Integration:
- Updated refresh_xcvr_api() to pass bank parameter when creating xcvr_api
Field Definitions:
- Added BANKS_SUPPORTED_FIELD constant to consts.py
- Added BanksSupported field to MODULE_CHAR_ADVT in CmisMemMap

This enables proper support for CMIS modules with multiple memory banks, allowing correct EEPROM access across all banks as defined in the CMIS specification. The offset calculation formula matches the optoe kernel driver implementation in sonic-linux-kernel PR #473.

Motivation and Context

We are implementing banking support as allowed by the CMIS spec. Our HLD is available at sonic-net/SONiC#2183

How Has This Been Tested?

I don't have banked hardware yet to test on, have confirmed that these changes are backwards compatible and don't break existing systems

Additional Information (Optional)

mssonicbld · 2026-03-24T21:56:05Z

/azp run

azure-pipelines · 2026-03-24T21:56:16Z

Azure Pipelines successfully started running 1 pipeline(s).

mssonicbld · 2026-03-25T16:51:18Z

/azp run

azure-pipelines · 2026-03-25T16:51:27Z

Azure Pipelines successfully started running 1 pipeline(s).

This PR extends the bank parameter support introduced in PR sonic-net#632 to fully enable multi-bank CMIS transceiver support throughout the stack. Changes include: 1. **Memory Map Bank Awareness**: - Updated CmisFlatMemMap, CmisMemMap, and CCmisMemMap to accept bank parameter - Implemented bank-aware getaddr() method in CmisFlatMemMap that correctly calculates linear offsets using the formula from sonic-linux-kernel PR sonic-net#473 - Added bank property to CmisFlatMemMap for read-only access - Added constants: CMIS_EEPROM_PAGE_SIZE, CMIS_NUM_NON_BANKED_PAGES, CMIS_NUM_BANKED_PAGES, CMIS_ARCH_PAGES - Uses unified formula: linear_offset = (bank * CMIS_ARCH_PAGES + page) * page_size + offset - Each bank is treated as a full 256-page (32KB) architectural block for proper alignment 2. **API Factory Updates**: - Added bank parameter to create_xcvr_api() and _create_cmis_api() - Updated CMIS API instantiation to pass bank parameter to memory maps - Modified id_mapping to pass bank parameter for all CMIS module types (0x18, 0x19, 0x1b, 0x1e) 3. **SfpBase Integration**: - Updated refresh_xcvr_api() to pass bank parameter when creating xcvr_api 4. **Field Definitions**: - Added BANKS_SUPPORTED_FIELD constant to consts.py - Added BanksSupported field to MODULE_CHAR_ADVT in CmisMemMap This enables proper support for CMIS modules with multiple memory banks, allowing correct EEPROM access across all banks as defined in the CMIS specification. The offset calculation formula matches the optoe kernel driver implementation in sonic-linux-kernel PR sonic-net#473. Signed-off-by: Bobby McGonigle <bobby@nexthop.ai>

mssonicbld · 2026-03-30T21:28:02Z

/azp run

azure-pipelines · 2026-03-30T21:28:12Z

Azure Pipelines successfully started running 1 pipeline(s).

Copilot

Pull request overview

This PR extends CMIS transceiver support to be bank-aware end-to-end (SfpBase → API factory → CMIS mem maps) so EEPROM access can be targeted to a selected CMIS memory bank.

Changes:

Added bank parameter plumbing into SfpBase.refresh_xcvr_api() and XcvrApiFactory.create_xcvr_api() / _create_cmis_api().
Updated CMIS memory map classes to accept a bank and implemented a bank-aware getaddr() offset calculation.
Introduced a BanksSupported field definition and corresponding constant.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
sonic_platform_base/sonic_xcvr/xcvr_api_factory.py	Threads `bank` through CMIS API creation paths.
sonic_platform_base/sonic_xcvr/mem_maps/public/cmis.py	Adds bank-aware addressing to CMIS mem maps and defines a new `BanksSupported` field.
sonic_platform_base/sonic_xcvr/mem_maps/public/c_cmis.py	Passes `bank` through to the coherent CMIS mem map.
sonic_platform_base/sonic_xcvr/fields/consts.py	Adds the `BANKS_SUPPORTED_FIELD` constant.
sonic_platform_base/sfp_base.py	Ensures bank is provided when refreshing the cached transceiver API.

Copilot · 2026-04-20T04:41:50Z

+        For lower memory (page 0, offset < 128):
+            linear_offset = offset
+
+        For paged memory:
+            offset_in_paged_area = (page * page_size + offset) - 128
+            bytes_per_bank = CMIS_ARCH_PAGES * page_size  (256 * 128 = 32KB)
+            linear_offset = 128 + (bank * bytes_per_bank) + offset_in_paged_area
+
+        Simplified:
+            linear_offset = (bank * CMIS_ARCH_PAGES + page) * page_size + offset
+
+        Note: Each bank is treated as a full 256-page (32KB) architectural block,
+        even though only pages 10h-FFh (240 pages) are actually banked. This ensures
+        proper alignment and matches the kernel driver behavior.
+        """
+        if page == 0 and offset < 128:
+            # Lower memory - not affected by banking
+            return offset
+
+        # For all paged memory (including bank 0), use the unified formula
+        # that treats each bank as a 256-page (32KB) block


getaddr() only exempts the lower 128 bytes (page 0, offset < 128) from banking, but the constants/comments state pages 00h–0Fh are non-banked. With the current formula, upper page 00h (offset 128–255) and pages 01h–0Fh will be shifted by bank * 32KB, which is likely incorrect if those pages are meant to be shared across banks. If pages 00h–0Fh are truly non-banked, special-case them to always use bank 0 (or otherwise document why they must be bank-offset).

Suggested change

For lower memory (page 0, offset < 128):

linear_offset = offset

For paged memory:

offset_in_paged_area = (page * page_size + offset) - 128

bytes_per_bank = CMIS_ARCH_PAGES * page_size (256 * 128 = 32KB)

linear_offset = 128 + (bank * bytes_per_bank) + offset_in_paged_area

Simplified:

linear_offset = (bank * CMIS_ARCH_PAGES + page) * page_size + offset

Note: Each bank is treated as a full 256-page (32KB) architectural block,

even though only pages 10h-FFh (240 pages) are actually banked. This ensures

proper alignment and matches the kernel driver behavior.

"""

if page == 0 and offset < 128:

# Lower memory - not affected by banking

return offset

# For all paged memory (including bank 0), use the unified formula

# that treats each bank as a 256-page (32KB) block

Pages 00h-0Fh are architecturally non-banked and must always map to the

shared address space regardless of the selected bank:

linear_offset = page * page_size + offset

For banked pages 10h-FFh:

linear_offset = (bank * CMIS_ARCH_PAGES + page) * page_size + offset

Note: Each bank is still treated as a full 256-page (32KB)

architectural block for banked pages to preserve alignment and match

the kernel driver behavior.

"""

if page < CMIS_NUM_NON_BANKED_PAGES:

# Pages 00h-0Fh are not affected by banking

return page * page_size + offset

# Banked pages 10h-FFh use the unified formula that treats each bank

# as a 256-page (32KB) architectural block.

This suggestion is invalid, since the addressing approach of treating all upper memory as if it were banked is intentional.

I've actually addressed this by clamping the bank parameter to 0 for non-banked pages. This will result in us writing 0 to the bank select register when accessing non-banked pages.

Natgerbi · 2026-05-10T11:32:08Z

@bobby-nexthop, is this PR still relevant? I see that you also have PR sonic-net/SONiC#2183, which seems to be more maintained and includes the paging part that does not exist here.

prgeor · 2026-05-13T16:38:31Z


+# Constants matching optoe driver
+CMIS_EEPROM_PAGE_SIZE = 128
+CMIS_NUM_NON_BANKED_PAGES = 16   # pages 00h-0Fh


@bobby-nexthop can we add a unit test to cover a scenario to ensure that NONE of the EEPROM memory map field mistakenly does not set bank >=1 for Unbanked Pages.

I have changed getaddr() to clamp bank to 0 when accessing unbanked pages.

I have added unit-tests that ensure we calculate the same offset regardless of what bank is set to.

prgeor · 2026-05-13T17:07:51Z

+            linear_offset = 128 + (bank * bytes_per_bank) + offset_in_paged_area
+
+        Simplified:
+            linear_offset = (bank * CMIS_ARCH_PAGES + page) * page_size + offset


@bobby-nexthop will be good to put an ASSERT here to make sure none of the memmap fields mistakenly access NON-banked pages. And in the unit test we can exercise this. This way we ensure this requirement at single place.

I think the new unit-tests provide coverage to prevent incorrect offset calculation. Let me know if I am missing anything.

Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

…ith a banked register Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

abelamit · 2026-05-14T06:35:18Z

@bobby-nexthop, is this PR still relevant? I see that you also have PR sonic-net/SONiC#2183, which seems to be more maintained and includes the paging part that does not exist here.

hi @bobby-nexthop - can you please comment if you plan to rebase this commit on top of the other commit proposed? It seems like they are not correlated.

mssonicbld · 2026-05-14T16:31:14Z

/azp run

azure-pipelines · 2026-05-14T16:31:24Z

Azure Pipelines successfully started running 1 pipeline(s).

prgeor · 2026-05-14T20:19:57Z

@bobby-nexthop, is this PR still relevant? I see that you also have PR sonic-net/SONiC#2183, which seems to be more maintained and includes the paging part that does not exist here.

@Natgerbi can you be more specific? feel free to quote the ref section or snapshot here

…n-banked pages Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

mssonicbld · 2026-05-14T20:24:16Z

/azp run

azure-pipelines · 2026-05-14T20:24:26Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

mssonicbld · 2026-05-14T20:38:20Z

/azp run

azure-pipelines · 2026-05-14T20:38:27Z

Azure Pipelines will not run the associated pipelines, because the pull request was updated after the run command was issued. Review the pull request again and issue a new run command.

…nk != 0 Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

mssonicbld · 2026-05-14T20:54:47Z

/azp run

azure-pipelines · 2026-05-14T20:54:57Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

…bank_size Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

mssonicbld · 2026-05-14T22:33:42Z

/azp run

azure-pipelines · 2026-05-14T22:33:51Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

mssonicbld · 2026-05-14T22:43:04Z

/azp run

azure-pipelines · 2026-05-14T22:43:14Z

Azure Pipelines successfully started running 1 pipeline(s).

prgeor · 2026-05-15T04:27:05Z

+
+        # If we are accessing a non-banked page, there is no reason to set the bank
+        # to a non-zero value. 
+        bank = 0 if page < CMIS_NUM_NON_BANKED_PAGES else self.bank


@Natgerbi @abelamit please note this TODO for your platform.

For reference, this is the relevant part of the CMIS spec this TODO is referring to (2 CDB instances are possible to support, accessible via bank selection).

micas-net · 2026-05-15T06:25:39Z

        except (OSError, IOError):
            pass

+    def set_optoe_max_bank_size(self, max_bank_size):


why bank size is set in optoe, we should move it into chassis. the scope of sfp is within one bank, not the whole eeprom

Yes, however, the API and the ability to access the EEPROM and detect module removal/insertion is at a per-bank SFP object level. We do ensure read-before-write semantics to ensure we don't write max_bank_size if it is already correct to prevent repeated writes. Do you have any recommendations on where we could put it in the chassis and how we could set it and also handle setting it upon transceiver removal/insertion?

While each SFP object is associated with a single bank, I chose this approach for the following reasons:

the write needs to be triggered again in the event of a new transceiver being plugged in with a different number of banks than the old one. Housing this logic in refresh_xcvr_api gives us that behaviour for free. If it was in the Chassis code, this would not be the case.

writing to max_bank_size is an implementation detail specific to the optoe driver, so it is nice to keep this logic in optoe-specific code.

The change has been implemented with read-before-write idempotency, so the following sequence of events would occur if a Chassis created 4 Sfp objects for a 4-banked transceiver:

Sfp object with bank=0 would do nothing (this is so unbanked transceivers continue to operate as they previously did before banking support was introduced)

Sfp object with bank=1 will perform the write to max_bank_size.

Sfp object with bank=2 will see the value has already been written and do nothing.

Sfp object with bank=3 will see the value has already been written and do nothing.

We will ultimately only write to the max_bank_size sysfs path once, while still keeping this optoe specific quirk internal to SfpOptoeBase.

micas-net · 2026-05-15T06:26:35Z

+        except (OSError, IOError):
+            pass
+
+    def _read_optoe_max_bank_size(self):


should also move to chassis if necessary

Same comment as above.

…OptoeBase Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

mssonicbld · 2026-05-15T17:19:39Z

/azp run

azure-pipelines · 2026-05-15T17:19:49Z

Azure Pipelines successfully started running 1 pipeline(s).

Natgerbi · 2026-05-17T07:13:20Z

see that you also have PR sonic-net/SONiC#2183, which seems to be more maintained and includes the paging part that does not exist

What I meant is that section 7.5 of the HLD, “Memory Map Abstraction Changes (cmis.py),” already aligns with the new abstraction layer based on memory-map pages. However, I think the actual implementation of the page abstraction can be added later in a separate PR.

micas-net

Seems good to me!

prgeor · 2026-05-18T06:25:43Z

+
+    def _read_optoe_max_bank_size(self):
+        """Determine optoe max_bank_size from the module's CMIS BanksSupported advertisement, or None if non-CMIS, flat-memory, or unreadable."""
+        id_byte = self.read_eeprom(0, 1)


@bgallagher-nexthop the optoe API here assumes its accessing CMIS memory map even before API is refreshed.
TODO: Refresh the API and then set the max bank if the API supports it.

bobby-nexthop marked this pull request as ready for review March 27, 2026 22:18

bobby-nexthop force-pushed the bank-support-cmis branch from 1fd9148 to fe10336 Compare March 30, 2026 21:27

prgeor requested review from Copilot and mihirpat1 April 20, 2026 04:35

Copilot AI reviewed Apr 20, 2026

View reviewed changes

prgeor reviewed May 13, 2026

View reviewed changes

bgallagher-nexthop added 2 commits May 13, 2026 21:54

Add unit-tests for the getaddr function

5d5a3cc

Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

Add logic to write to optoe's max bank size sysfs path when dealing w…

292acf5

…ith a banked register Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

Change address calculation to ignore bank parameter when accessing no…

64e89d3

…n-banked pages Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

Plumb bank parameter to other CMIS variant API creation callsites

d7bb6de

Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

bgallagher-nexthop added 2 commits May 14, 2026 20:50

Only perform the sysfs write to max_bank_size in SfpOptoeBase when ba…

22cfee6

…nk != 0 Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

Merge branch 'master' into bank-support-cmis

2ee9ba6

bgallagher-nexthop added 2 commits May 14, 2026 20:59

Write 0 to max_bank_size when banking is disabled for extra clarity

32a5745

Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

Cleanup + add a guard to ignore flat memory modules when setting max_…

e182296

…bank_size Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

fix test failures

d864631

Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

prgeor reviewed May 15, 2026

View reviewed changes

micas-net reviewed May 15, 2026

View reviewed changes

Add read-before-write idempotency to max_bank_size write logic in Sfp…

4a72dcd

…OptoeBase Signed-off-by: Brian Gallagher <bgallagher@nexthop.ai>

micas-net approved these changes May 18, 2026

View reviewed changes

prgeor reviewed May 18, 2026

View reviewed changes

prgeor approved these changes May 18, 2026

View reviewed changes

prgeor merged commit 39c3bd6 into sonic-net:master May 18, 2026
6 checks passed

Conversation

bobby-nexthop commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Additional Information (Optional)

Uh oh!

mssonicbld commented Mar 24, 2026

Uh oh!

azure-pipelines Bot commented Mar 24, 2026

Uh oh!

mssonicbld commented Mar 25, 2026

Uh oh!

azure-pipelines Bot commented Mar 25, 2026

Uh oh!

mssonicbld commented Mar 30, 2026

Uh oh!

azure-pipelines Bot commented Mar 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Apr 20, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgallagher-nexthop May 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Natgerbi commented May 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abelamit commented May 14, 2026

Uh oh!

mssonicbld commented May 14, 2026

Uh oh!

azure-pipelines Bot commented May 14, 2026

Uh oh!

prgeor commented May 14, 2026

Uh oh!

mssonicbld commented May 14, 2026

Uh oh!

azure-pipelines Bot commented May 14, 2026

Uh oh!

mssonicbld commented May 14, 2026

Uh oh!

azure-pipelines Bot commented May 14, 2026

Uh oh!

mssonicbld commented May 14, 2026

Uh oh!

azure-pipelines Bot commented May 14, 2026

Uh oh!

mssonicbld commented May 14, 2026

Uh oh!

azure-pipelines Bot commented May 14, 2026

Uh oh!

mssonicbld commented May 14, 2026

Uh oh!

azure-pipelines Bot commented May 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

bobby-nexthop commented Mar 24, 2026 •

edited

Loading

bgallagher-nexthop May 14, 2026 •

edited

Loading