Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[sfp-refactoring] Initial support for CMIS application initialization #219

Merged
merged 9 commits into from
Dec 7, 2021

Conversation

ds952811
Copy link
Contributor

@ds952811 ds952811 commented Oct 4, 2021

Signed-off-by: Dante Su dante.su@broadcom.com

Why I did it

Add initial support for CMIS application initialization

How I did it

  1. sonic-xcvr/*/cmis.py: Add additional register access for CMIS application advertisement and initialization
  2. sonic_platform_base/sfp_base.py: Add stub routines for CMIS application initialization and port/cage type to help the xcvrd determine if the CMIS manager service is ready necessary on the target platform

How to verify it

  1. Bring up Quanta IX9 with default configuration and CMIS 4.0/5.0 QSFPDD modules
  2. Issue 'show logging xcvrd' to check if the application initialization succeeded
  3. Issue 'show interfaces transceiver eeprom --dom' to verify both the static and DOM information parsers
  4. Issue 'sudo sfputil show eeprom --dom' to verify both the static and DOM information parsers

Which release branch to backport (provide reason below if selected)

  • 201811
  • 201911
  • 202006
  • 202012
  • 202106

Description for the changelog

Repo PR title State
SONiC [sfp-refactoring] Initial support for CMIS application initialization

A picture of a cute animal (not mandatory but encouraged)

@lgtm-com
Copy link

lgtm-com bot commented Oct 4, 2021

This pull request introduces 5 alerts when merging 45b21d6 into 4598d40 - view on LGTM.com

new alerts:

  • 4 for Except block handles 'BaseException'
  • 1 for Variable defined multiple times

@keboliu
Copy link
Collaborator

keboliu commented Oct 6, 2021

If I am not wrong there are activities ongoing to refactor the SFP base, maybe can consolidate with those PRs

@ds952811 ds952811 changed the title sfp: add initial support for CMIS [sfp-refactoring] add initial support for CMIS application initialization Nov 3, 2021
@ds952811 ds952811 changed the title [sfp-refactoring] add initial support for CMIS application initialization [sfp-refactoring] sonic-xcvr: add initial support for CMIS application initialization Nov 3, 2021
@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2021

This pull request introduces 2 alerts when merging 7a810bd into 26c8346 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2021

This pull request introduces 2 alerts when merging cb8e6dc into 26c8346 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'
  • 1 for Unused import

for i in range(10):
time.sleep(0.5)
val = self.xcvr_eeprom.read(consts.MODULE_LEVEL_CTRL_RESET_FIELD)
if (val is None) or val:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the expected return type of val here?. why do we need 10 iterations?. In the worst case scenario, this would take 5 seconds to read SW reset status.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a self-clear 'Software Reset' bit, writing '1' to this field will trigger the reset, and the value will be automatically translated to a Boolean value by the new sfp-refactoring software stack.
i.e.

  1. The return value is a Boolean
  2. During the reset, the TWI could be malfunctioned and leads to I2C read failure, and in turn, None will be returned.
  3. To deal with this error condition gracefully, we'll treat TWI read failure as a flag of RESET In-Progress.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for a record, this code snippet is already removed

sonic_platform_base/sonic_xcvr/api/public/cmis.py Outdated Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2021

This pull request introduces 2 alerts when merging 146d861 into 26c8346 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'
  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2021

This pull request introduces 2 alerts when merging 001e7bb into 26c8346 - view on LGTM.com

new alerts:

  • 1 for Except block handles 'BaseException'
  • 1 for Unused import

@ds952811 ds952811 changed the title [sfp-refactoring] sonic-xcvr: add initial support for CMIS application initialization [sfp-refactoring] Initial support for CMIS application initialization Nov 8, 2021
@lgtm-com
Copy link

lgtm-com bot commented Nov 8, 2021

This pull request introduces 2 alerts when merging 21824db into 26c8346 - view on LGTM.com

new alerts:

  • 2 for Module-level cyclic import

@@ -17,6 +17,12 @@ class SfpBase(device_base.DeviceBase):
# Device type definition. Note, this is a constant.
DEVICE_TYPE = "sfp"

# SFP port type. Please note this is the cage type rather than transceiver type.
SFP_PORT_TYPE_UNSPECIFIED = "UNSPECIFIED"
SFP_PORT_TYPE_SFP = "SFP"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about other cage types (OSFP, etc)? Is "SFP" the same as "SFP+"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the cage/form-factor type, while SFP/SFP+/SFP28 shares the same cage/form-factor type, I do not know much about OSFP, hence it's not declared here.

speed = 1000
return speed

def get_cmis_application_selected(self):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to assume that the first lane's apsel field is representative of the entire module. I do not think this is a safe assumption. Modules may support mixed channelizations/speeds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the PR, we support only synchronized lane allocation, which is all the lanes will be reconfigured to use the same application mode.
While it's possible to support the mixed application modes from the CMIS SPEC. perspective, none of such samples is available as of now.
e.g.
In the theory, it's possible to support 2x100PAM4 + 1x100NRZ in a single QSFPDD module, but there is no such module/cable in the field, and it's also not a supported port mode in the Broadocm BlackHawk core.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"none of such samples is available as of now" I'm assuming that this is what you have available? Certainly modules that support mixed speeds should be generally available (400G-SR8 for example).

I suspect you'd find that mixed channelizations like 100G NRZ (4x25G) + 200G PAM4 (4x50G) or 2x100G PAM4 and 1x200G PAM4 are supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

None of these modules is available at my site, and please note mixed port mode requires both MAC and PHY support, in the case of Broadcom blackhawk core (TH3 and TD4), the mixed port mode may not be supported.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, the HLD has been updated to clearly specify the scope for dynamic port-breakout only, which have exactly the same application on all the lanes. We could deal with this later once both MAC and PHY are available for this

if self.is_flat_memory():
return False

# Only applicable to CMIS v4.0 onwards

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CMIS3 support would certianly be desirable from my perspective

Copy link
Contributor Author

@ds952811 ds952811 Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of now, all the CMIS3 samples we have are working fine without updating application modes upon dynamic-port-breakout operations, while the CMIS3 SPEC. seems to declare the application init needs to be done.
Hence, we don't do CMIS application initialization for CMIS3 modules in this PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That certainly won't be true generically for CMIS3 devices.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My original target is CMIS 4 onwards, I'm expecting the CMIS 3 modules needs to be having firmware updated.
But we could surely add the CMIS3 support if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for a record, the CMIS3 is now supported in the latest commit

if self.xcvr_eeprom.read(consts.CMIS_REVISION_FIELD) < 0x40:
return False

# Check for application selection updates

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I appreciate that this is an optimization, feedback I've gotten from optics folks is that we may want to perform the initialization sequence even if the module is already configured correctly to ensure the module locks properly.

Copy link
Contributor Author

@ds952811 ds952811 Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm not following you. Did you mean a re-initialization is necessary even when the module/lane is in default application mode (i.e. Both ConfigError and DataPathInitState are 0x00)? If that's the case, we could update the checker NOT to skip this operation.
But it does not seem to be necessary to do re-initialization if the module/lane is already correctly configured and passing traffic while no port mode updates.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic will only ever be triggered on port speed or breakout change (ie: traffic is going to be disrupted anyway). Performing re-initialization ensures that the module adapation is done after the ASIC configuration has been completed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The the latest commit, only default application (ApSel = 1) is supported, we'll deal with port breakout support once the synchronization mechanism between syncd and xcvrd is in place and ready.
  2. In the latest code change, in the case that CMIS is initialized by the hardware init sequence, the ConfigError will be 0 (No status), and it will trigger the CMIS software init sequence. In other words, the CMIS will always be re-initialized with the software init sequence.

if ready:
break

# Reset the module

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should not be done. This will disrupt the entire module on speed change for a channel of the module, and will result in the ApSel being cleared for the other lanes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main purpose of this PR is the CMIS application initialization for dynamic-port-breakout, hence all the lanes will need to be reconfigured, and a reset is to make sure the module will be placed in a known state even if it hits a datapath init failure or a config error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. It's intended to have the entire module reset upon the application initialization, but we could remove it if necessary, this is nothing but a fail-proof procedure.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd strongly suggest removing it. For single-channel speed change without rechannelization it will cause incorrect behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As of now, we support only default applications, which means all lanes will be configured to use exactly the same application, and we'll deal with the per-lane application initialization once the he synchronization mechanism between syncd and xcvrd is in place and ready

self.reset()

# Deinitialize DataPath
self.xcvr_eeprom.write(consts.DATAPATH_DEINIT_FIELD, 0xff)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only the lanes being configured should be de-initialized (though a knob may be required for older modules to apply to all lanes)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After datapath deinit, should poll for all affected lanes to be de-initialized

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the latest code update, it's now using a state-based init sequence, and it will poll both ConfigError and DPState before transitioning to next state

self.xcvr_eeprom.write(consts.MODULE_LEVEL_CTRL_LPSW_FIELD, 0)
self.xcvr_eeprom.write(consts.MODULE_LEVEL_CTRL_LPHW_FIELD, 0)

for lane in range(1, 9):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

make this range 0,8 then skip the subtract 1 below?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, while it's 0-based indexing when deriving the lane allocation, it's 1-based indexing in the memory map of sonic-xcvr, and the existing code of cmis.py already used 1-based indexing for the DOM registers, it's better not to break this due to a cosmetic change.
Hence, we'll not change the range here.


return (ap_code & 0xf)

def set_cmis_application(self, host_speed, host_lanes):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how are different speeds on different lane blocks configured?

Copy link
Contributor Author

@ds952811 ds952811 Nov 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not a supported port mode in the Broadocm, hence we can't validate it, and having a different port mode (lane count, speed) on a QSFPDD requires SERDES support on both the MAC and PHY, and it is outside the scope of this PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you say "This is not a supported port mode in the Broadocm", what level support are you talking about? Is this Broadcom SONiC? I believe this should be supported at the SDK/ASIC layer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I was referring to the CMIS transceivers available to me, none of them are able to support different application codes on lanes of the single optic, and yes, this is also not a supported port mode on Broadcom SONIC switches, hence I can't do the verification.
Since we support only default application as of now, we'll deal with it later when the synchronization mechanism between syncd and xcvrd is in place and ready

# Update the 1st lane of each datapath
if ap_new > 1:
val |= (int((lane - 1) / host_lanes) * host_lanes) << 1
# Toggle the BIT0(i.e. application-defined)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the CMIS4 optics are using faulty firmware design, the BIT0 need to be set to activate the selected application, while some are expecting BIT0 to be cleared.

# Initialize DataPath
self.xcvr_eeprom.write(consts.DATAPATH_DEINIT_FIELD, 0x00)

# Allow 20 seconds for status verification

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The module advertises how long the datapath initialization may take in page01 byte 144. Should that be used? (may be overkill?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, the code will later be updated

@prgeor
Copy link
Collaborator

prgeor commented Dec 1, 2021

@andywongarista could you please review

@prgeor
Copy link
Collaborator

prgeor commented Dec 1, 2021

@ds952811 could you rebase with master to resolve merge conflict

Copy link
Collaborator

@prgeor prgeor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ds952811 could you address review comment?

Comment on lines 594 to 611
def get_lpmode(self):
if self.is_flat_memory() or not self.get_lpmode_support():
return False
return self.get_module_state() == CmisCodes.MODULE_STATE[1]

def set_lpmode(self, lpmode):
if self.is_flat_memory() or not self.get_lpmode_support():
return False
byte = self.xcvr_eeprom.read(consts.MODULE_LEVEL_CONTROL)
# Clear LowPwrAllowRequestHW (BIT6, CMIS v4 onwards)
byte &= 0xbf
# Set/Clear LowPwrRequestHW (BIT4, CMIS v3 onwards)
if lpmode:
byte |= 0x10
else:
byte &= 0xef
return self.xcvr_eeprom.write(consts.MODULE_LEVEL_CONTROL, byte)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please rebase your change with latest master, you will find this already implemented.

# TODO
"application_advertisement": "N/A",
"application_advertisement": admin_info.get(consts.APPLS_ADVT_FIELD, "N/A"),
"memory_type": "flat" if self.is_flat_memory() else "paged"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new field will fail sonic-mgmt test. Please run the test

@@ -1665,4 +1685,178 @@ def get_transceiver_loopback(self):
trans_loopback['host_input_loopback_lane7'] = host_input_loopback_status[6]
trans_loopback['host_input_loopback_lane8'] = host_input_loopback_status[7]
return trans_loopback

def set_datapath_deinit(self, lanes, enable):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you split this into two APIs? set_datapath_init() and set_datapath_deinit() ? or add description?

Comment on lines 36 to 46
type = sff8024.type_of_media_interface[code]
if type == "nm_850_media_interface":
media_if_dict = sff8024.nm_850_media_interface
elif type == "sm_media_interface":
media_if_dict = sff8024.sm_media_interface
elif type == "passive_copper_media_interface":
media_if_dict = sff8024.passive_copper_media_interface
elif type == "active_cable_media_interface":
media_if_dict = sff8024.active_cable_media_interface
elif type == "base_t_media_interface":
media_if_dict = sff8024.base_t_media_interface
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please reuse CmisMemMap.MEDIA_TYPE_FIELD

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refer to the comment below, and I'll replace sff8024 with the new codes defined in sonic_xcvr/codes/public/sff8024.py

@@ -28,6 +29,7 @@ def __init__(self, codes):
)

self.ADMIN_INFO = RegGroupField(consts.ADMIN_INFO_FIELD,
ApplicationAdvertField(consts.APPLS_ADVT_FIELD, self.getaddr(0x0, 85), size=33),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please rebase your code with master. We already decode these bytes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPLS_ADVT_FIELD != MEDIA_TYPE_FIELD, while MEDIA_TYPE_FIELD decodes only byte 85, the APPLS_ADVT_FIELD decodes the advertising applications from byte 85 - 117

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e. The following existing code snippet supports only application 1 (default application), while the APPLS_ADVT_FIELD tried to decode all the applications defined in byte 85 - 117.
......
CodeRegField(consts.HOST_ELECTRICAL_INTERFACE, self.getaddr(0x0, 86), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField(consts.MEDIA_TYPE_FIELD, self.getaddr(0x0, 85), self.codes.MODULE_MEDIA_TYPE),
CodeRegField(consts.MODULE_MEDIA_INTERFACE_850NM, self.getaddr(0x0, 87), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField(consts.MODULE_MEDIA_INTERFACE_SM, self.getaddr(0x0, 87), self.codes.SM_MEDIA_INTERFACE),
CodeRegField(consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, self.getaddr(0x0, 87), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField(consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, self.getaddr(0x0, 87), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField(consts.MODULE_MEDIA_INTERFACE_BASE_T, self.getaddr(0x0, 87), self.codes.BASE_T_MEDIA_INTERFACE),
NumberRegField(consts.MEDIA_LANE_COUNT, self.getaddr(0x0, 88),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField(consts.HOST_LANE_COUNT, self.getaddr(0x0, 88),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField(consts.HOST_LANE_ASSIGNMENT_OPTION, self.getaddr(0x0, 89), format="B", size=1),
......

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, this PR scope is to support default application only right? then why parse other applications. if you still want to search for other applications please add the following fields for EACH application (notice the '0' appended for field name to indicate its for application 0)

            CodeRegField(consts.HOST_ELECTRICAL_INTERFACE 0, self.getaddr(0x0, 86), self.codes.HOST_ELECTRICAL_INTERFACE),
            CodeRegField(consts.MEDIA_TYPE_FIELD0, self.getaddr(0x0, 85), self.codes.MODULE_MEDIA_TYPE),
            CodeRegField(consts.MODULE_MEDIA_INTERFACE_850NM0, self.getaddr(0x0, 87), self.codes.NM_850_MEDIA_INTERFACE),
            CodeRegField(consts.MODULE_MEDIA_INTERFACE_SM0, self.getaddr(0x0, 87), self.codes.SM_MEDIA_INTERFACE),
            CodeRegField(consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, self.getaddr(0x0, 87), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
            CodeRegField(consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, self.getaddr(0x0, 87), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
            CodeRegField(consts.MODULE_MEDIA_INTERFACE_BASE_T, self.getaddr(0x0, 87), self.codes.BASE_T_MEDIA_INTERFACE),
            NumberRegField(consts.MEDIA_LANE_COUNT0, self.getaddr(0x0, 88),
                *(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
            ),
            NumberRegField(consts.HOST_LANE_COUNT0, self.getaddr(0x0, 88),
                *(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
            ),
            NumberRegField(consts.HOST_LANE_ASSIGNMENT_OPTION0, self.getaddr(0x0, 89), format="B", size=1),

This is to avoid doing the raw decoding outside mem_map/public/cmis.py and all decoding logic for the eeprom content is at one place: mem_map/public/cmis.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, and it looks like we'll need to use Python3 for the pytest, otherwise it will fail

continue
if self.get_host_speed(d.get('host_electrical_interface_id')) != host_speed:
continue
ap_code = c
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the no of lanes and speed has matched i.e the application has been found, why not break out of the for-loop now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, you're right, will break the loop

self.set_lpmode(False)
return True

def set_cmis_application_apsel(self, host_lanes, appl_code):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please make this api to select datapath ID and explicit control with default values so that we can reuse this API in future when we support non-default

Copy link
Contributor Author

@ds952811 ds952811 Dec 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selected datapath ID (i.e. line_lane_id) should be derived from the host_lanes, which is a list of 0-based lane ids on the system-side. Please note in a different application, each datapath could require different lane counts from the system side, hence directly passing datapath/line_lane_id is not a good idea, if we did that, the parameters of this routine will also need to be updated from (self, host_lanes, appl_code) to (self, line_lane_id, appl_code, host_lane_cout), and the actuate system lane id list will also need to be derived in this routine.
Hence I'll suggest sticking with the current implementation
e.g.
Assuming there are 8 lanes available on the CMIS optic
host[0,1] --> Lane 0,1 of the system side of the optic will be allocated for this application, and it's DataPath 0 (line_lane_0)
host[4,5] --> Lane 4,5 of the system side of the optic will be allocated for this application, and it's DataPath 2 (line_lane_2)

mask = 0
for lane in host_lanes:
addr = "{}_{}".format(consts.STAGED_CTRL0_APSEL_FIELD, lane + 1)
data = appl_code << 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should not hardcode datapath id and explicit control as ZERO. see previous comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will update the memory map for stage controls

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you assume, the datapath ID will be always ZERO and EXPLICIT control will be ZERO?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. In the case of AVAGO Optic, the EXPLICIT needs to be ZERO for 4x100G
  2. In the case of INNOLIGHT#T-DP4CNT-N00, the EXPLICIT needs to be 1 for 4x100G
  3. The first lane mark is now added to the latest commit


def set_cmis_application_txon(self, host_lanes):
"""
(Stage 4) Initialize the new data path
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment - "Turn on the laser" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks

api = self.get_xcvr_api()
return api.set_lpmode(lpmode) if api is not None else False

def get_module_state(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

new API, please add description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

get_module_state() is already merged from someone else in the community

Signed-off-by: Dante Su <dante.su@broadcom.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2021

This pull request introduces 2 alerts when merging 2a8bfa6 into 2dff8cd - view on LGTM.com

new alerts:

  • 2 for Variable defined multiple times

@@ -26,7 +26,7 @@ def __init__(self, xcvr_eeprom):
super(CmisCdbApi, self).__init__(xcvr_eeprom)
self.cdb_instance_supported = self.xcvr_eeprom.read(consts.CDB_SUPPORT)
self.failed_status_dict = self.xcvr_eeprom.mem_map.codes.CDB_FAIL_STATUS
assert self.cdb_instance_supported != 0
#assert self.cdb_instance_supported != 0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This leads to a crash on my 400G DR4 optic (CMIS v4, AVAGO#AFCT-93DRPHZ-AZ2)
i.e.
admin@sonic:$ sudo sfputil show eeprom -p Ethernet0
Traceback (most recent call last):
File "/usr/local/bin/sfputil", line 8, in
sys.exit(cli())
File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 764, in call
return self.main(*args, **kwargs)
File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 717, in main
rv = self.invoke(ctx)
File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 1137, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 1137, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 956, in invoke
return ctx.invoke(self.callback, **ctx.params)
File "/usr/local/lib/python3.7/dist-packages/click/core.py", line 555, in invoke
return callback(*args, **kwargs)
File "/usr/local/lib/python3.7/dist-packages/sfputil/main.py", line 562, in eeprom
xcvr_info = platform_chassis.get_sfp(physical_port).get_transceiver_info()
File "/usr/local/lib/python3.7/dist-packages/sonic_platform_base/sonic_xcvr/sfp_optoe_base.py", line 23, in get_transceiver_info
api = self.get_xcvr_api()
File "/usr/local/lib/python3.7/dist-packages/sonic_platform_base/sfp_base.py", line 451, in get_xcvr_api
self.refresh_xcvr_api()
File "/usr/local/lib/python3.7/dist-packages/sonic_platform_base/sfp_base.py", line 441, in refresh_xcvr_api
self._xcvr_api = self._xcvr_api_factory.create_xcvr_api()
File "/usr/local/lib/python3.7/dist-packages/sonic_platform_base/sonic_xcvr/xcvr_api_factory.py", line 47, in create_xcvr_api
api = CmisApi(xcvr_eeprom)
File "/usr/local/lib/python3.7/dist-packages/sonic_platform_base/sonic_xcvr/api/public/cmis.py", line 29, in init
self.cdb = CmisCdbApi(xcvr_eeprom)
File "/usr/local/lib/python3.7/dist-packages/sonic_platform_base/sonic_xcvr/api/public/cmisCDB.py", line 29, in init
assert self.cdb_instance_supported != 0
AssertionError
admin@sonic:
$

Signed-off-by: Dante Su <dante.su@broadcom.com>
@lgtm-com
Copy link

lgtm-com bot commented Dec 2, 2021

This pull request introduces 2 alerts when merging a1a01cd into 2dff8cd - view on LGTM.com

new alerts:

  • 2 for Variable defined multiple times

@prgeor
Copy link
Collaborator

prgeor commented Dec 2, 2021

This pull request introduces 2 alerts when merging a1a01cd into 2dff8cd - view on LGTM.com

new alerts:

  • 2 for Variable defined multiple times

@ds952811 can you fix the LGTM warnings

Signed-off-by: Dante Su <dante.su@broadcom.com>
@ds952811
Copy link
Contributor Author

ds952811 commented Dec 2, 2021

This pull request introduces 2 alerts when merging a1a01cd into 2dff8cd - view on LGTM.com
new alerts:

  • 2 for Variable defined multiple times

@ds952811 can you fix the LGTM warnings

Sure, and all the LGTM warnings are addressed

@@ -365,6 +368,17 @@ def __init__(self, codes):
NumberRegField(consts.CDB_RPL_LENGTH, self.getaddr(0x9f, 134), size=1, ro=False),
NumberRegField(consts.CDB_RPL_CHKCODE, self.getaddr(0x9f, 135), size=1, ro=False),
)

self.STAGED_CTRL = RegGroupField(consts.STAGED_CTRL_FIELD,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you append '0' to specify this is staged control set 0 and not 1

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do

Comment on lines 230 to 295
def set_cmis_datapath_init(self, host_lanemask):
"""
Put the CMIS datapath into the initialized state

Args:
host_lanemask: Integer, a bitmask of the lanes on the system/host side
e.g. 0x5 for lane 0 and lane 2.

Returns:
Boolean, true if success otherwise false
"""
api = self.get_xcvr_api()
return api.set_cmis_datapath_init(host_lanemask) if api is not None else False

def set_cmis_datapath_deinit(self, host_lanemask):
"""
Put the CMIS datapath into the de-initialized state

Args:
host_lanemask: Integer, a bitmask of the lanes on the system/host side
e.g. 0x5 for lane 0 and lane 2.

Returns:
Boolean, true if success otherwise false
"""
api = self.get_xcvr_api()
return api.set_cmis_datapath_deinit(host_lanemask) if api is not None else False

def has_cmis_application_update(self, host_speed, host_lanemask):
"""
Check for CMIS application update and retrieve the new application code

Args:
host_speed:
Integer, the port speed of the host interface
host_lanemask:
Integer, a bitmask of the lanes on the host side
e.g. 0x5 for lane 0 and lane 2.

Returns:
(has_update, new_appl)
"""
api = self.get_xcvr_api()
if api is not None:
return api.has_cmis_application_update(host_speed, host_lanemask)
return (False, 1)

def set_cmis_application_apsel(self, host_lanemask, appl_code):
"""
Update the selected application code to the specified host lanes

Args:
host_lanemask:
Integer, a bitmask of the lanes on the host side
e.g. 0x5 for lane 0 and lane 2.
appl_code:
Integer, the desired application code

Returns:
Boolean, true if success otherwise false
"""
ret = False
api = self.get_xcvr_api()
if api is not None:
ret = api.set_cmis_application_apsel(host_lanemask, appl_code)
return ret
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you please move these CMIS specific APIs to Xcvrd where its going to be used? SfpOpetoeBase class is common for transceivers NOT following CMIS

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and I probably get your points now, I guess you're referring to use 'sfp.get_xcvr_api()' inside the CmisManagerTask of Xcvrd instead of adding these CMIS stuff into the SfpOptoeBase, correct?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done


def reset(self):
"""
Reset SFP and return all user module settings to their default srate.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type 'srate'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks

@@ -876,6 +877,22 @@ def reset_module(self, reset = False):
else:
return True

def reset(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

else:
appl['module_media_interface_id'] = 'Unknown'

appl['host_lane_count'] = raw_data[pos+2] >> 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes please reuse the existing code, instead of adding more duplicate code.

@@ -1745,4 +1765,246 @@ def get_transceiver_loopback(self):
for lane in range(1, self.NUM_CHANNELS+1):
trans_loopback['host_input_loopback_lane%d' % lane] = 'N/A'
return trans_loopback

def set_cmis_datapath_init(self, host_lanemask):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dont we need API to know if Datapath has transitioned to "DPActivated" state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could leverage get_datapath_state() for this

return (False, 1)

app_new = self.get_cmis_application_matched(host_speed, host_lanemask)
if app_new != 1 or host_lanemask != (1 << self.NUM_CHANNELS) - 1:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you think the default application should ALWAYS support 8 lanes - this is not realistic assumption

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, this additional checker is removed

mask = 0
for lane in host_lanes:
addr = "{}_{}".format(consts.STAGED_CTRL0_APSEL_FIELD, lane + 1)
data = appl_code << 4
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do you assume, the datapath ID will be always ZERO and EXPLICIT control will be ZERO?


return (appl_code & 0xf)

def has_cmis_application_update(self, host_speed, host_lanemask):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be rename to "is_cmis_application_update_required()" ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it's moved to Xcvrd


app_old = 0
for lane in range(self.NUM_CHANNELS):
if (1 << lane) & host_lanemask == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add extra bracket?

if ((1 << lane) & host_lanemask) == 0:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

A boolean, True if successful, False if not
"""
mask = (1 << 3)
byte = self.xcvr_eeprom.read(consts.MODULE_LEVEL_CONTROL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

read call can also return None.
can you add a condition to check for None here?.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if host_speed == 0 or host_lanemask == 0:
return 0

host_lane_count = 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that we have already have a API to get host lane count. can you please check?.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

def get_error_description(self):
dp_state = self.get_datapath_state()
conf_state = self.get_config_datapath_hostlane_status()
for lane in range(self.NUM_CHANNELS):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you define a function (get_cmis_num_channels()) to get the count?. Optics like DR4 has only 4 channels. we can't assume 8 as default number of channels.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, but please note these are host lanes connected to the MAC/ASIC, hence it's 8 lanes in DR4, as to QSFP+ CMIS, it's most likely 4 lanes.

e.g. Here is an example of INNOLIGHT 400G DR4, the ConfigError reside at 0x94a, while DPState are at 0x900

root@sonic:~# hexdump -C -n 256 -s 0x880 /sys/bus/i2c/devices/40-0050/eeprom
00000880  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
00000890  00 21 21 25 25 29 29 2d  2d ff 00 00 33 33 33 33  |.!!%%))--...3333|
000008a0  ff ff 22 22 22 22 00 00  00 00 22 22 22 22 00 00  |..""""....""""..|
000008b0  00 00 00 00 10 10 10 10  10 10 10 10 ff 00 00 33  |...............3|
000008c0  33 33 33 ff ff 22 22 22  22 00 00 00 00 22 22 22  |333..""""...."""|
000008d0  22 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |"...............|
000008e0  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  |................|
*
00000900  44 44 44 44 00 00 00 00  00 00 00 00 00 00 00 00  |DDDD............|
00000910  00 00 00 00 00 00 00 00  00 00 3e db 3e 51 3c ca  |..........>.>Q<.|
00000920  3d 09 00 00 00 00 00 00  00 00 9c 40 9c 40 9c 40  |=..........@.@.@|
00000930  9c 40 00 00 00 00 00 00  00 00 3a 7c 56 23 31 23  |.@........:|V#1#|
00000940  32 7e 00 00 00 00 00 00  00 00 11 11 11 11 21 21  |2~............!!|
00000950  25 25 29 29 2d 2d ff 00  00 33 33 33 33 ff ff 22  |%%))--...3333.."|
00000960  22 22 22 00 00 00 00 22  22 22 22 00 00 00 00 00  |"""...."""".....|
00000970  11 12 13 14 00 00 00 00  11 12 13 14 00 00 00 00  |................|

Signed-off-by: Dante Su <dante.su@broadcom.com>
@@ -51,6 +51,54 @@ def __init__(self, codes):
),

CodeRegField(consts.CONNECTOR_FIELD, self.getaddr(0x0, 203), self.codes.CONNECTORS),

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of Python2, these will cause Python errors in the pytest.
Please be sure to use python3 -m pytest -sx

@lgtm-com
Copy link

lgtm-com bot commented Dec 3, 2021

This pull request introduces 1 alert when merging dd0de8d into 2dff8cd - view on LGTM.com

new alerts:

  • 1 for Unused import

Signed-off-by: Dante Su <dante.su@broadcom.com>
Signed-off-by: Dante Su <dante.su@broadcom.com>
@aravindmani-1
Copy link
Contributor

could you please check build failure?.

Signed-off-by: Dante Su <dante.su@broadcom.com>
@ds952811
Copy link
Contributor Author

ds952811 commented Dec 6, 2021

could you please check build failure?.

Done, but there is still something wrong in the coverage checker, do you know what's up over there?

@prgeor
Copy link
Collaborator

prgeor commented Dec 6, 2021

could you please check build failure?.

Done, but there is still something wrong in the coverage checker, do you know what's up over there?

Looks like, code coverage should be at least 50%

Total: 117 lines
Missing: 87 lines
Coverage: 25% <<<
Threshold: 50% <<<

# Apply DataPathInit
return self.xcvr_eeprom.write("%s_%d" % (consts.STAGED_CTRL_APPLY_DPINIT_FIELD, 0), channel)

def get_num_channels(self):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i believe this is trying to get the number of host lanes(NOT media lanes), but the actual no of host lanes depends upon the active application. For eg, if the default application is 100G PSM4, then the no of host lanes is 4 but that is not the case here.

Copy link
Contributor Author

@ds952811 ds952811 Dec 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. The get_num_channels() is from C-CMIS PR, and it seems to fetch the lane count of the host side from a module perspective, hence it's correct
  2. When a host lane is unused (i.e. No datapath associated with it), its corresponding ApSel code in the Staged Control will be 0, hence we could improve the checker here if necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait... get_num_channels() is from my end! I'll drop it later

@@ -1132,6 +1132,8 @@ def test_get_transceiver_info(self, mock_response, expected):
self.api.get_module_media_type.return_value = mock_response[13]
self.api.get_module_hardware_revision = MagicMock()
self.api.get_module_hardware_revision.return_value = '0.0'
self.api.is_flat_memory = MagicMock()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add more test case so that code coverage is better.?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I got it now, the code coverage here is not talking about the GCOV? Instead, it's the sanity test in the test_cmis.py? I'm not sure how the ratios are calculated, I'll do a couple of more tests to figure out the way to improve the code coverage

"""
if self.reset_module(True):
for retries in range(5):
time.sleep(1)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After reset the management init time required is 2 seconds

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, will do

Signed-off-by: Dante Su <dante.su@broadcom.com>
time.sleep(1)
if self.get_module_state() != 'Unknown':
state = self.get_module_state()
if state in ['ModuleReady']:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

module won't transition to 'ModuleReady' state from 'ModuleLowPwr' unless LowPwrS is FALSE so this check will fail on those cases where LowPwrS is TRUE. so we should expect the module to be in one of the steady state i.e either ModuleReady or ModuleLowPwr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The trick is, the reset_module() always clear LowPwrS, but yes, I could improve the reset() accordingly

Comment on lines +1927 to +1930
name = "{}_{}_{}".format(consts.STAGED_CTRL_APSEL_FIELD, 0, lane + 1)
appl = self.xcvr_eeprom.read(name)
if (appl is None) or ((appl >> 4) == 0):
continue
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this api should return error state of datapath and module any time it is called. but looks like that is not the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does return the failure in the current application init sequence.

Comment on lines +244 to +258
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 1), self.getaddr(0x0, 89 + 4 * (1 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 2), self.getaddr(0x0, 89 + 4 * (2 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 3), self.getaddr(0x0, 89 + 4 * (3 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 4), self.getaddr(0x0, 89 + 4 * (4 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 5), self.getaddr(0x0, 89 + 4 * (5 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 6), self.getaddr(0x0, 89 + 4 * (6 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 7), self.getaddr(0x0, 89 + 4 * (7 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 8), self.getaddr(0x0, 89 + 4 * (8 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 9), self.getaddr(0x1, 226 + 4 * (9 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 10), self.getaddr(0x1, 226 + 4 * (10 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 11), self.getaddr(0x1, 226 + 4 * (11 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 12), self.getaddr(0x1, 226 + 4 * (12 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 13), self.getaddr(0x1, 226 + 4 * (13 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 14), self.getaddr(0x1, 226 + 4 * (14 - 1)), format="B", size=1),
NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, 15), self.getaddr(0x1, 226 + 4 * (15 - 1)), format="B", size=1),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we condense the HOST_LANE_ASSIGNMENT_OPTION into single line as done for MEDIA_LANE_ASSIGNMENT_OPTION ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, please see the comments below

Comment on lines +198 to +242
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 1), self.getaddr(0x0, 88 + 4 * (1 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 2), self.getaddr(0x0, 88 + 4 * (2 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 3), self.getaddr(0x0, 88 + 4 * (3 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 4), self.getaddr(0x0, 88 + 4 * (4 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 5), self.getaddr(0x0, 88 + 4 * (5 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 6), self.getaddr(0x0, 88 + 4 * (6 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 7), self.getaddr(0x0, 88 + 4 * (7 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 8), self.getaddr(0x0, 88 + 4 * (8 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 9), self.getaddr(0x1, 225 + 4 * (9 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 10), self.getaddr(0x1, 225 + 4 * (10 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 11), self.getaddr(0x1, 225 + 4 * (11 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 12), self.getaddr(0x1, 225 + 4 * (12 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 13), self.getaddr(0x1, 225 + 4 * (13 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 14), self.getaddr(0x1, 225 + 4 * (14 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, 15), self.getaddr(0x1, 225 + 4 * (15 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8))
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we condense HOST_LANE_COUNT similar to ACTIVE_APSEL_CODE field?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, please see the comments below

Comment on lines +152 to +197
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 1), self.getaddr(0x0, 88 + 4 * (1 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 2), self.getaddr(0x0, 88 + 4 * (2 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 3), self.getaddr(0x0, 88 + 4 * (3 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 4), self.getaddr(0x0, 88 + 4 * (4 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 5), self.getaddr(0x0, 88 + 4 * (5 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 6), self.getaddr(0x0, 88 + 4 * (6 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 7), self.getaddr(0x0, 88 + 4 * (7 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 8), self.getaddr(0x0, 88 + 4 * (8 - 1)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 9), self.getaddr(0x1, 225 + 4 * (9 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 10), self.getaddr(0x1, 225 + 4 * (10 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 11), self.getaddr(0x1, 225 + 4 * (11 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 12), self.getaddr(0x1, 225 + 4 * (12 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 13), self.getaddr(0x1, 225 + 4 * (13 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 14), self.getaddr(0x1, 225 + 4 * (14 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),
NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, 15), self.getaddr(0x1, 225 + 4 * (15 - 9)),
*(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4))
),

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we condense MEDIA_LANE_COUNT fields similar to ACTIVE_APSEL_CODE ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't, it hits the Python2 generator expression issue, so we can;t do it, unless the online checker is migrated from Python2 to Python3

Comment on lines +56 to +150
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 1), self.getaddr(0x0, 86 + 4 * (1 - 1)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 2), self.getaddr(0x0, 86 + 4 * (2 - 1)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 3), self.getaddr(0x0, 86 + 4 * (3 - 1)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 4), self.getaddr(0x0, 86 + 4 * (4 - 1)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 5), self.getaddr(0x0, 86 + 4 * (5 - 1)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 6), self.getaddr(0x0, 86 + 4 * (6 - 1)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 7), self.getaddr(0x0, 86 + 4 * (7 - 1)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 8), self.getaddr(0x0, 86 + 4 * (8 - 1)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 9), self.getaddr(0x1, 223 + 4 * (9 - 9)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 10), self.getaddr(0x1, 223 + 4 * (10 - 9)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 11), self.getaddr(0x1, 223 + 4 * (11 - 9)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 12), self.getaddr(0x1, 223 + 4 * (12 - 9)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 13), self.getaddr(0x1, 223 + 4 * (13 - 9)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 14), self.getaddr(0x1, 223 + 4 * (14 - 9)), self.codes.HOST_ELECTRICAL_INTERFACE),
CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, 15), self.getaddr(0x1, 223 + 4 * (15 - 9)), self.codes.HOST_ELECTRICAL_INTERFACE),

CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 1), self.getaddr(0x0, 87 + 4 * (1 - 1)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 2), self.getaddr(0x0, 87 + 4 * (2 - 1)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 3), self.getaddr(0x0, 87 + 4 * (3 - 1)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 4), self.getaddr(0x0, 87 + 4 * (4 - 1)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 5), self.getaddr(0x0, 87 + 4 * (5 - 1)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 6), self.getaddr(0x0, 87 + 4 * (6 - 1)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 7), self.getaddr(0x0, 87 + 4 * (7 - 1)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 8), self.getaddr(0x0, 87 + 4 * (8 - 1)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 9), self.getaddr(0x1, 224 + 4 * (9 - 9)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 10), self.getaddr(0x1, 224 + 4 * (10 - 9)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 11), self.getaddr(0x1, 224 + 4 * (11 - 9)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 12), self.getaddr(0x1, 224 + 4 * (12 - 9)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 13), self.getaddr(0x1, 224 + 4 * (13 - 9)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 14), self.getaddr(0x1, 224 + 4 * (14 - 9)), self.codes.NM_850_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, 15), self.getaddr(0x1, 224 + 4 * (15 - 9)), self.codes.NM_850_MEDIA_INTERFACE),

CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 1), self.getaddr(0x0, 87 + 4 * (1 - 1)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 2), self.getaddr(0x0, 87 + 4 * (2 - 1)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 3), self.getaddr(0x0, 87 + 4 * (3 - 1)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 4), self.getaddr(0x0, 87 + 4 * (4 - 1)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 5), self.getaddr(0x0, 87 + 4 * (5 - 1)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 6), self.getaddr(0x0, 87 + 4 * (6 - 1)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 7), self.getaddr(0x0, 87 + 4 * (7 - 1)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 8), self.getaddr(0x0, 87 + 4 * (8 - 1)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 9), self.getaddr(0x1, 224 + 4 * (9 - 9)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 10), self.getaddr(0x1, 224 + 4 * (10 - 9)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 11), self.getaddr(0x1, 224 + 4 * (11 - 9)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 12), self.getaddr(0x1, 224 + 4 * (12 - 9)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 13), self.getaddr(0x1, 224 + 4 * (13 - 9)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 14), self.getaddr(0x1, 224 + 4 * (14 - 9)), self.codes.SM_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, 15), self.getaddr(0x1, 224 + 4 * (15 - 9)), self.codes.SM_MEDIA_INTERFACE),

CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 1), self.getaddr(0x0, 87 + 4 * (1 - 1)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 2), self.getaddr(0x0, 87 + 4 * (2 - 1)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 3), self.getaddr(0x0, 87 + 4 * (3 - 1)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 4), self.getaddr(0x0, 87 + 4 * (4 - 1)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 5), self.getaddr(0x0, 87 + 4 * (5 - 1)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 6), self.getaddr(0x0, 87 + 4 * (6 - 1)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 7), self.getaddr(0x0, 87 + 4 * (7 - 1)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 8), self.getaddr(0x0, 87 + 4 * (8 - 1)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 9), self.getaddr(0x1, 224 + 4 * (9 - 9)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 10), self.getaddr(0x1, 224 + 4 * (10 - 9)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 11), self.getaddr(0x1, 224 + 4 * (11 - 9)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 12), self.getaddr(0x1, 224 + 4 * (12 - 9)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 13), self.getaddr(0x1, 224 + 4 * (13 - 9)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 14), self.getaddr(0x1, 224 + 4 * (14 - 9)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, 15), self.getaddr(0x1, 224 + 4 * (15 - 9)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE),

CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 1), self.getaddr(0x0, 87 + 4 * (1 - 1)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 2), self.getaddr(0x0, 87 + 4 * (2 - 1)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 3), self.getaddr(0x0, 87 + 4 * (3 - 1)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 4), self.getaddr(0x0, 87 + 4 * (4 - 1)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 5), self.getaddr(0x0, 87 + 4 * (5 - 1)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 6), self.getaddr(0x0, 87 + 4 * (6 - 1)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 7), self.getaddr(0x0, 87 + 4 * (7 - 1)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 8), self.getaddr(0x0, 87 + 4 * (8 - 1)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 9), self.getaddr(0x1, 224 + 4 * (9 - 9)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 10), self.getaddr(0x1, 224 + 4 * (10 - 9)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 11), self.getaddr(0x1, 224 + 4 * (11 - 9)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 12), self.getaddr(0x1, 224 + 4 * (12 - 9)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 13), self.getaddr(0x1, 224 + 4 * (13 - 9)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 14), self.getaddr(0x1, 224 + 4 * (14 - 9)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, 15), self.getaddr(0x1, 224 + 4 * (15 - 9)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE),

CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 1), self.getaddr(0x0, 87 + 4 * (1 - 1)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 2), self.getaddr(0x0, 87 + 4 * (2 - 1)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 3), self.getaddr(0x0, 87 + 4 * (3 - 1)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 4), self.getaddr(0x0, 87 + 4 * (4 - 1)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 5), self.getaddr(0x0, 87 + 4 * (5 - 1)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 6), self.getaddr(0x0, 87 + 4 * (6 - 1)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 7), self.getaddr(0x0, 87 + 4 * (7 - 1)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 8), self.getaddr(0x0, 87 + 4 * (8 - 1)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 9), self.getaddr(0x1, 224 + 4 * (9 - 9)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 10), self.getaddr(0x1, 224 + 4 * (10 - 9)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 11), self.getaddr(0x1, 224 + 4 * (11 - 9)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 12), self.getaddr(0x1, 224 + 4 * (12 - 9)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 13), self.getaddr(0x1, 224 + 4 * (13 - 9)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 14), self.getaddr(0x1, 224 + 4 * (14 - 9)), self.codes.BASE_T_MEDIA_INTERFACE),
CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, 15), self.getaddr(0x1, 224 + 4 * (15 - 9)), self.codes.BASE_T_MEDIA_INTERFACE),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need to declare each field, can we condense it in a for loop similar to MEDIA_LANE_ASSIGNMENT_OPTION

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using multiple generator expressions will cause failures in Python2, unfortunately, the online checker of this PR still sticks with Phton2, hence it was reworked in this way.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is the example:
CODE:

            RegGroupField(consts.APPLS_ADVT_FIELD,
                *(CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, app), self.getaddr(0x0, 86 + 4 * (app - 1)), self.codes.HOST_ELECTRICAL_INTERFACE)
                  for app in range(1, 9)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, app), self.getaddr(0x0, 87 + 4 * (app - 1)), self.codes.NM_850_MEDIA_INTERFACE)
                  for app in range(1, 9)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, app), self.getaddr(0x0, 87 + 4 * (app - 1)), self.codes.SM_MEDIA_INTERFACE)
                  for app in range(1, 9)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, app), self.getaddr(0x0, 87 + 4 * (app - 1)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE)
                  for app in range(1, 9)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, app), self.getaddr(0x0, 87 + 4 * (app - 1)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE)
                  for app in range(1, 9)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, app), self.getaddr(0x0, 87 + 4 * (app - 1)), self.codes.BASE_T_MEDIA_INTERFACE)
                  for app in range(1, 9)),
                *(NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, app), self.getaddr(0x0, 88 + 4 * (app - 1)),
                      *(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4)))
                  for app in range(1, 9)),
                *(NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, app), self.getaddr(0x0, 88 + 4 * (app - 1)),
                      *(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8)))
                  for app in range(1, 9)),
                *(NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, app), self.getaddr(0x0, 89 + 4 * (app - 1)), format="B", size=1)
                  for app in range(1, 9)),

                *(NumberRegField("%s_%d" % (consts.MEDIA_LANE_ASSIGNMENT_OPTION, app), self.getaddr(0x1, 176 + (app - 1)), format="B", size=1)
                  for app in range(1, 16)),

                *(CodeRegField("%s_%d" % (consts.HOST_ELECTRICAL_INTERFACE, app), self.getaddr(0x1, 223 + 4 * (app - 9)), self.codes.HOST_ELECTRICAL_INTERFACE)
                  for app in range(9, 16)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, app), self.getaddr(0x1, 224 + 4 * (app - 9)), self.codes.NM_850_MEDIA_INTERFACE)
                  for app in range(9, 16)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_SM, app), self.getaddr(0x1, 224 + 4 * (app - 9)), self.codes.SM_MEDIA_INTERFACE)
                  for app in range(9, 16)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_PASSIVE_COPPER, app), self.getaddr(0x1, 224 + 4 * (app - 9)), self.codes.PASSIVE_COPPER_MEDIA_INTERFACE)
                  for app in range(9, 16)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_ACTIVE_CABLE, app), self.getaddr(0x1, 224 + 4 * (app - 9)), self.codes.ACTIVE_CABLE_MEDIA_INTERFACE)
                  for app in range(9, 16)),
                *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_BASE_T, app), self.getaddr(0x1, 224 + 4 * (app - 9)), self.codes.BASE_T_MEDIA_INTERFACE)
                  for app in range(9, 16)),
                *(NumberRegField("%s_%d" % (consts.MEDIA_LANE_COUNT, app), self.getaddr(0x1, 225 + 4 * (app - 9)),
                      *(RegBitField("Bit%d" % (bit), bit) for bit in range (0, 4)))
                  for app in range(9, 16)),
                *(NumberRegField("%s_%d" % (consts.HOST_LANE_COUNT, app), self.getaddr(0x1, 225 + 4 * (app - 9)),
                      *(RegBitField("Bit%d" % (bit), bit) for bit in range (4, 8)))
                  for app in range(9, 16)),
                *(NumberRegField("%s_%d" % (consts.HOST_LANE_ASSIGNMENT_OPTION, app), self.getaddr(0x1, 226 + 4 * (app - 9)), format="B", size=1)
                  for app in range(9, 16)),
            ),

Pytest Failure (Python2 specific, and it's working fine with Python3)

============================= test session starts ==============================
platform linux2 -- Python 2.7.16, pytest-3.10.1, py-1.7.0, pluggy-0.8.0 -- /usr/bin/python2
cachedir: .pytest_cache
rootdir: /projects/fastpath/ds952811/community/sonic-platform-common, inifile: pytest.ini
plugins: cov-2.6.0
collecting ...
==================================== ERRORS ====================================
_________________ ERROR collecting tests/sfputilhelper_test.py _________________
/usr/lib/python2.7/dist-packages/_pytest/python.py:450: in _importtestmodule
    mod = self.fspath.pyimport(ensuresyspath=importmode)
/usr/lib/python2.7/dist-packages/py/_path/local.py:668: in pyimport
    __import__(modname)
/usr/lib/python2.7/dist-packages/_pytest/assertion/rewrite.py:294: in load_module
    six.exec_(co, mod.__dict__)
/usr/lib/python2.7/dist-packages/six.py:709: in exec_
    exec("""exec _code_ in _globs_, _locs_""")
<string>:1: in <module>
    ???
tests/sfputilhelper_test.py:6: in <module>
    from sonic_platform_base.sonic_sfp import sfputilhelper
sonic_platform_base/__init__.py:9: in <module>
    from . import sfp_base
sonic_platform_base/sfp_base.py:11: in <module>
    from .sonic_xcvr.xcvr_api_factory import XcvrApiFactory
sonic_platform_base/sonic_xcvr/xcvr_api_factory.py:13: in <module>
    from .mem_maps.public.cmis import CmisMemMap
E     File "/projects/fastpath/ds952811/community/sonic-platform-common/sonic_platform_base/sonic_xcvr/mem_maps/public/cmis.py", line 58
E       *(CodeRegField("%s_%d" % (consts.MODULE_MEDIA_INTERFACE_850NM, app), self.getaddr(0x0, 87 + 4 * (app - 1)), self.codes.NM_850_MEDIA_INTERFACE)
E       ^
E   SyntaxError: invalid syntax
 generated xml file: /projects/fastpath/ds952811/community/sonic-platform-common/test-results.xml

Signed-off-by: Dante Su <dante.su@broadcom.com>
@prgeor prgeor merged commit 94919de into sonic-net:master Dec 7, 2021
oleksandrivantsiv pushed a commit to oleksandrivantsiv/sonic-platform-common that referenced this pull request Oct 25, 2024
…vrd (sonic-net#219)

* [xcvrd] suuport for integrating y cable within xcvrd

This PR separates the logic of Y-Cable from xcvrd. Before this change we were utilizing xcvrd daemon to control all aspects of Y-Cable right from initialization to processing requests from other entities like orch,linkmgr.
Now we would have another daemon ycabled which will serve this purpose.
Logically everything still remains the same from the perspective of other daemons.
it also take care aspects like init/delete daemon from Y-Cable perspective.

dependent-startup                EXITED    Jan 18 05:40 AM

xcvrd                            RUNNING   pid 33, uptime 20:02:58
ycabled                          RUNNING   pid 218, uptime 0:22:12
Motivation and Context
Required for separating the logic of Y-Cable from xcvrd. This is to ensure that the daemon always works and responds to linkmgr to address its requests

How Has This Been Tested?
Built an image with the changes and ran dualtor specific tests on the change on a 7050cx3 testbed.
Signed-off-by: vaibhav-dahiya <vdahiya@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants