Xcvrd changes to support 400G ZR configuration by prgeor · Pull Request #270 · sonic-net/sonic-platform-daemons

prgeor · 2022-07-01T19:49:00Z

Description

400G ZR configuration support

Motivation and Context

400G ZR needs configuration changes for laser frequency and Tx target output power

How Has This Been Tested?

Xcvrd honors the configuration changes for both Tx power and laser frequency:

When Xcvrd is running
When Xcvrd comes up

root@sonic:~# sfputil show eeprom -d -p Ethernet0
Ethernet0: SFP EEPROM detected
        Active App Selection Host Lane 1: 1
        Active App Selection Host Lane 2: 1
        Active App Selection Host Lane 3: 1
        Active App Selection Host Lane 4: 1
        Active App Selection Host Lane 5: 1
        Active App Selection Host Lane 6: 1
        Active App Selection Host Lane 7: 1
        Active App Selection Host Lane 8: 1
        Active Firmware Version: 61.20
        Application Advertisement: {1: {'host_electrical_interface_id': '400GAUI-8 C2M (Annex 120E)', 'module_media_interface_id': '400ZR, DWDM, amplified', 'media_lane_count': 1, 'host_lane_count': 8, 'host_lane_assignment_options': 1}, 2: {'host_electrical_interface_id': '400GAUI-8 C2M (Annex 120E)', 'module_media_interface_id': '400ZR, Single Wavelength, Unamplified', 'media_lane_count': 1, 'host_lane_count': 8, 'host_lane_assignment_options': 1}, 3: {'host_electrical_interface_id': '100GAUI-2 C2M (Annex 135G)', 'module_media_interface_id': '400ZR, DWDM, amplified', 'media_lane_count': 1, 'host_lane_count': 2, 'host_lane_assignment_options': 85}}
        CMIS Revision: 4.1
        Connector: LC
        Encoding: N/A
        Extended Identifier: Power Class 8 (20.0W Max)
        Extended RateSelect Compliance: N/A
        Hardware Revision: 1.1
        Host Electrical Interface: 400GAUI-8 C2M (Annex 120E)
        Host Lane Assignment Options: 1
        Host Lane Count: 8
        Identifier: QSFP-DD Double Density 8X Pluggable Transceiver
        Inactive Firmware Version: 161.10
        Length Cable Assembly(m): 0.0
        Media Interface Code: 400ZR, DWDM, amplified
        Media Interface Technology: 1550 nm DFB
        Media Lane Assignment Options: 1
        Media Lane Count: 1
        Nominal Bit Rate(100Mbs): 0
        Specification compliance: sm_media_interface
        Supported Max Laser Frequency: 196100GHz
        Supported Max TX Power: 4.0dBm
        Supported Min Laser Frequency: 191300GHz
        Supported Min TX Power: -22.9dBm
        Vendor Date Code(YYYY-MM-DD Lot): 2021-11-19   
        Vendor Name: Acacia Comm Inc.
        Vendor OUI: 7c-b2-5c
        Vendor PN: DP04QSDD-E20-001
        Vendor Rev: A 
        Vendor SN: 214455197       
        ChannelMonitorValues:
                RX1Power: -11.884249941294067dBm
                RX2Power: -infdBm
                RX3Power: -infdBm
                RX4Power: -infdBm
                RX5Power: -infdBm
                RX6Power: -infdBm
                RX7Power: -infdBm
                RX8Power: -infdBm
                TX1Bias: 241.496mA
                TX1Power: -13.585258894959004dBm
                TX2Bias: 0.0mA
                TX2Power: -infdBm
                TX3Bias: 0.0mA
                TX3Power: -infdBm
                TX4Bias: 0.0mA
                TX4Power: -infdBm
                TX5Bias: 0.0mA
                TX5Power: -infdBm
                TX6Bias: 0.0mA
                TX6Power: -infdBm
                TX7Bias: 0.0mA
                TX7Power: -infdBm
                TX8Bias: 0.0mA
                TX8Power: -infdBm
        ChannelThresholdValues:
                RxPowerHighAlarm  : 2.0dBm
                RxPowerHighWarning: 0.0dBm
                RxPowerLowAlarm   : -23.01dBm
                RxPowerLowWarning : -20.0dBm
                TxBiasHighAlarm   : 0.0mA
                TxBiasHighWarning : 0.0mA
                TxBiasLowAlarm    : 0.0mA
                TxBiasLowWarning  : 0.0mA
                TxPowerHighAlarm  : 0.0dBm
                TxPowerHighWarning: -2.0dBm
                TxPowerLowAlarm   : -18.013dBm
                TxPowerLowWarning : -16.003dBm
        ModuleMonitorValues:
                Temperature: 66.0C
                Vcc: 3.321Volts
        ModuleThresholdValues:
                TempHighAlarm  : 80.0C
                TempHighWarning: 75.0C
                TempLowAlarm   : -5.0C
                TempLowWarning : 15.0C
                VccHighAlarm   : 3.465Volts
                VccHighWarning : 3.432Volts
                VccLowAlarm    : 3.135Volts
                VccLowWarning  : 3.168Volts


root@sonic:~#

Configured frequency

root@sonic:/# python3
Python 3.7.3 (default, Jan 22 2021, 20:04:44) 
[GCC 8.3.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from sonic_platform.chassis import Chassis
>>> c = Chassis()
>>> sfp = c.get_sfp(1)
>>> api = sfp.get_xcvr_api()
>>> api.get_current_laser_freq()
193175.0
>>>

Additional Information (Optional)

prgeor · 2022-07-07T03:38:49Z

                except AttributeError:
                    # Skip if these essential routines are not available
                    self.port_dict[lport]['cmis_state'] = self.CMIS_STATE_READY
+                    assert 1 == 0


remove this

prgeor · 2022-07-12T18:24:09Z

@snider-nokia can you review?

prgeor · 2022-07-12T18:26:52Z

@jaganbal-a @shyam77git please review

prgeor · 2022-07-12T18:28:17Z

@qinchuanares please review

jaganbal-a

Please add the unit test log.

qinchuanares · 2022-07-21T17:59:09Z

+
+        found, port_info = port_tbl.get(lport)
+        if found and 'laser_freq' in dict(port_info):
+            freq = dict(port_info)['laser_freq']


what if found is True but no frequency is configured in port_info? It's likely to happen when a ZR module is plugged but freq config is missing. Do we see the port to be err-disabled?

the port_info{} dictionary represents the PORT table in CONFIG_DB. If laser frequency is NOT configured, then we move on with the module's default frequency.

freq init value == 0 in this function. Does it mean when we see freq == 0, it will set module frequency to the default frequency? Can we test this feature with a ZR module by erasing the port's frequency config and see how the module will be configured.

yes module comes with default frequency if not configured

qinchuanares · 2022-07-21T18:00:25Z

+
+        found, port_info = port_tbl.get(lport)
+        if found and 'tx_power' in dict(port_info):
+            power = dict(port_info)['tx_power']


Similarly. What if found is True but no tx power is configured on the port. Should we enable a default power setting?

The default is the module's default. This API gets the value explicitly configured by the user

Similar to the previous comment, Can we test this feature with a ZR module by erasing the port's tx power config and see how the module will be configured.

If tx power is not configured, module comes with module's default Tx power.

qinchuanares · 2022-07-21T18:09:11Z

+                        if api.is_coherent_module():
+                           tx_power = self.port_dict[lport]['tx_power']
+                           # Prevent configuring same tx power multiple times
+                           if 0 != tx_power and tx_power != api.get_tx_config_power():


what does it mean when tx_power == 0?

it means no 'tx_power' missing in the PORT table of CONFIG_DB. See get_configured_tx_power()

qinchuanares · 2022-07-21T18:10:09Z

+                           freq = self.port_dict[lport]['laser_freq']
+                           # If user requested frequency is NOT the same as configured on the module
+                           # force datapath re-initialization
+                           if 0 != freq and freq != api.get_laser_config_freq():


what does it mean when freq == 0?

It means 'laser_freq' is missing in PORT table of CONFIG_DB. see get_configured_laser_freq()

Understood. It means the no laser_freq is configured in config_db.

qinchuanares · 2022-07-21T18:14:05Z


+
+    @patch('xcvrd.xcvrd.XcvrTableHelper')
+    def test_CmisManagerTask_get_configured_freq(self, mock_table_helper):


Can we add failure case? For instance, 1. when config_freq is out of allowed freq range. 2. when config_freq does not fall on 75GHz grid.

This API tests get_configured_laser_freq() which obtains the configured value from the PORT table of CONFIG_DB. Invalid frequency is validated here sonic-net/sonic-utilities#2197

Am I looking at the correct place https://github.com/prgeor/sonic-utilities/blob/ba58d985b2483d24fb68821f11c6507dd08de567/scripts/portconfig#L337? seems like it's only validating the case when freq is <= 0? Does it verify the provisioned frequency is within the min and max advertised supported freq values?

qinchuanares · 2022-07-21T18:15:11Z

+        assert task.get_configured_laser_freq('Ethernet0') == 193100
+
+    @patch('xcvrd.xcvrd.XcvrTableHelper')
+    def test_CmisManagerTask_get_configured_tx_power(self, mock_table_helper):


Similarly can we add failure case? 1. when configured tx power is out of range.

This API simply get the configured tx_power from the CONFIG_DB. The user input for TX power is validated here sonic-net/sonic-utilities#2197. Please do note, tx_power can be configured even if transceiver is NOT plugged in, so we cannot validate tx_power in that case.

Is this the right place that it validates tx power https://github.com/prgeor/sonic-utilities/blob/ba58d985b2483d24fb68821f11c6507dd08de567/scripts/portconfig#L332? Does it mean it only allows one digit after decimal point? Does it verify the configured tx power is within the min and max advertised supported tx power values?

jaganbal-a · 2022-07-25T21:48:14Z

+                        if api.is_coherent_module():
+                           tx_power = self.port_dict[lport]['tx_power']
+                           # Prevent configuring same tx power multiple times
+                           if 0 != tx_power and tx_power != api.get_tx_config_power():


this check helps preventing reprogramming during xcvrd restart?

this check helps preventing reprogramming during xcvrd restart?

@jaganbal-a YES

jaganbal-a · 2022-07-25T21:50:46Z

+                       if 'tx_power' not in self.port_dict[lport]:
+                           self.port_dict[lport]['tx_power'] = self.get_configured_tx_power(lport)
+                       if 'laser_freq' not in self.port_dict[lport]:
+                           self.port_dict[lport]['laser_freq'] = self.get_configured_laser_freq(lport)


I think get_configured_tx_power_from_dict() naming is appropriate as it reading from dict and not from the device. similar for get_configured_laser_freq().

@jaganbal-a the configured frequency is read from the DB. see comment inside get_configured_tx_power()

I see the get_configured_tx_power() read from DB, but the naming sounds like it read from the device.
So I would suggest the naming get_configured_tx_power_from_dict()

@jaganbal-a please check latest changes

jaganbal-a · 2022-07-25T21:53:44Z

+                self.port_dict[lport]['laser_freq'] = int(port_change_event.port_dict['laser_freq'])
+            if 'tx_power' in port_change_event.port_dict:
+                self.port_dict[lport]['tx_power'] = float(port_change_event.port_dict['tx_power'])
+


With CMIS state reset to INSERTED upon any event for the port, the coherent module will be read continuously for Tx power and frequency.

port_dict[] is updated by reading the configuration values from the DB, not by reading the module

It is not about port_dict[].
When the CMIS state is reset, the state changes to INSERTED. Once the CMIS state is INSERTED, the below piece of code executed continuously in the CMIS mgr task worker. api.get_tx_config_power() and api.get_laser_config_freq() query the device with that.

Configure the target output power if ZR module

if api.is_coherent_module(): tx_power = self.port_dict[lport]['tx_power'] # Prevent configuring same tx power multiple times if 0 != tx_power and tx_power != api.get_tx_config_power():

as discussed offline, this is not specific to tx_power or laser_freq. Currently don't have a subscription mechanism to listen only to the interested fields. This is the git issue to improve on this #259

I agree and hopefully #259 converges soon.
Note: currently there is no overhead due to this issue. But with the this PR changes the optical module will be read for both freq and tx-power register offsets in continuous fashion.

prgeor · 2022-07-27T03:00:31Z

ZR-log.txt
@jaganbal-a added the log

qinchuanares · 2022-07-27T05:47:51Z

@prgeor I checked the test log. For TX power configuration, we would not want data path to be reinitialized. It should be done when data path is always activated and tx is enabled.

jaganbal-a · 2022-07-27T15:11:26Z

ZR-log.txt @jaganbal-a added the log

Can you please capture show interface transceiver eeprom -d for the interfaces having ZR with tx and freq configuration?

jaganbal-a

Added response to the existing comments.

prgeor · 2022-07-27T18:49:05Z

@prgeor I checked the test log. For TX power configuration, we would not want data path to be reinitialized. It should be done when data path is always activated and tx is enabled.

@qinchuanares can you point me to the CMIS reference where you find this requirement. Here is what I in section 7.5.2. There is no strict requirement to program TX power in datapath activated state. At CMIS_STATE_INSERTED, module is either in Low power mode or in High power mode.

prgeor · 2022-07-27T19:30:16Z

ZR-log.txt @jaganbal-a added the log

Can you please capture show interface transceiver eeprom -d for the interfaces having ZR with tx and freq configuration?

Added the log in the description. please check

jaganbal-a

Looks good to me.

jaganbal-a · 2022-07-28T01:48:22Z

Frequency should be displayed as part of show int transceiver CLI. Please raise a git issue.

prgeor · 2022-07-28T21:22:41Z

@rlhui @lguohan please merge

lizhuhuams

Merge for Prince.

Update sonic-platform-daemons submodule pointer to include the following: * Xcvrd changes to support 400G ZR configuration ([sonic-net#270](sonic-net/sonic-platform-daemons#270)) * [ycabled] add secure channel support for grpc dualtor active-active connectivity ([sonic-net#275](sonic-net/sonic-platform-daemons#275)) Signed-off-by: dprital <[email protected]>

* Xcvrd changes to support 400G ZR configuration * Fix test failures * Improve code coverage * Addressed review comments

…#270) Description The code that decodes the content of the ONIE syseeprom includes a flag to enable/disable displaying the content of the vendor extension TLV. This flag is currently set to 'disable'. Hence the 'show platform syseeprom' command shows the presence and size of the vendor extension TLV but does not show its content. This commit sets the flag to 'enable' so that the vendor extension TLV content is displayed. Motivation and Context The 'show platform syseeprom' command shows that the Vendor Extension TLV is present but does not show its content. Here's what that looks like on an example platform.

prgeor added 2 commits July 1, 2022 19:46

Xcvrd changes to support 400G ZR configuration

20906d7

Fix test failures

2bf2f47

prgeor commented Jul 7, 2022

View reviewed changes

Improve code coverage

491e21f

rlhui added the Chassis label Jul 12, 2022

rlhui requested a review from judyjoseph July 12, 2022 09:35

jaganbal-a reviewed Jul 18, 2022

View reviewed changes

qinchuanares reviewed Jul 21, 2022

View reviewed changes

jaganbal-a reviewed Jul 25, 2022

View reviewed changes

prgeor added the CMIS label Jul 27, 2022

jaganbal-a reviewed Jul 27, 2022

View reviewed changes

jaganbal-a previously approved these changes Jul 28, 2022

View reviewed changes

qinchuanares previously approved these changes Jul 28, 2022

View reviewed changes

vdahiya12 previously approved these changes Jul 28, 2022

View reviewed changes

lizhuhuams reviewed Jul 28, 2022

View reviewed changes

lizhuhuams previously approved these changes Jul 28, 2022

View reviewed changes

judyjoseph previously approved these changes Jul 29, 2022

View reviewed changes

Addressed review comments

0e31af1

prgeor dismissed stale reviews from judyjoseph, lizhuhuams, vdahiya12, qinchuanares, and jaganbal-a via 0e31af1 July 29, 2022 16:58

prgeor force-pushed the 400G-ZR branch from 9356a2a to 0e31af1 Compare July 29, 2022 16:58

lguohan approved these changes Aug 1, 2022

View reviewed changes

prgeor merged commit cc56367 into sonic-net:master Aug 2, 2022

dprital mentioned this pull request Aug 9, 2022

[submodule] Advance sonic-platform-daemons pointer sonic-net/sonic-buildimage#11656

Merged

7 tasks

prgeor added the Request for 202205 Branch label Aug 11, 2022

yxieca pushed a commit that referenced this pull request Aug 17, 2022

Xcvrd changes to support 400G ZR configuration (#270)

5a68073

* Xcvrd changes to support 400G ZR configuration * Fix test failures * Improve code coverage * Addressed review comments

yxieca added the Included in 202205 Branch label Aug 17, 2022

This was referenced Aug 26, 2022

Add CMIS_and_C-CMIS_support_for_ZR.md sonic-net/SONiC#769

Closed

Add CMIS_and_C-CMIS_support_for_ZR.md sonic-net/SONiC#1076

Merged



		@patch('xcvrd.xcvrd.XcvrTableHelper')
		def test_CmisManagerTask_get_configured_freq(self, mock_table_helper):

Conversation

prgeor commented Jul 1, 2022 • 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!

Choose a reason for hiding this comment

Uh oh!

prgeor commented Jul 12, 2022

Uh oh!

prgeor commented Jul 12, 2022

Uh oh!

prgeor commented Jul 12, 2022

Uh oh!

jaganbal-a left a comment

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!

qinchuanares Jul 23, 2022 • 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!

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!

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!

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!

qinchuanares Jul 23, 2022 • 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!

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!

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!

jaganbal-a Jul 27, 2022 • 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!

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

prgeor commented Jul 1, 2022 •

edited

Loading

qinchuanares Jul 23, 2022 •

edited

Loading

qinchuanares Jul 23, 2022 •

edited

Loading

jaganbal-a Jul 27, 2022 •

edited

Loading