[link-training] initial support #925

ds952811 · 2022-01-13T09:20:27Z

Why I did this?

In the case of DAC, static pre-calibrated pre-emphasis is rarely available on SONIC, as most of the ODM are expecting this to be done dynamically at runtime via link-training, hence we'll need this feature to improve the link quality

Related PRs:

Repo	PR title	State
sonic-buildimage	sonic-port.yang: add link training support
sonic-sairedis	vslib: add support for read-only port capabilities
sonic-swss	portsorch: initial support for link-training
sonic-swss-common	selectabletimer: add mutex to start() and stop()
sonic-utilities	sonic-utils: initial support for link-training

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

mikeberesford · 2022-02-07T22:24:35Z

doc/port_link_training/port-link-training-design.md

+
+#### PMON xcvrd Considerations
+
+Given that port link training shall not be enabled on certain transceivers. For example, a chip-to-module transceiver. It's better to have pmon#xcvrd provide a hint to the swss#orchagent for the advanced link training controls. Hence, **xcvr_capabilities** is introduced into APPL_DB.  


What will be the criteria for determining if a module supports link training?

LT is not going to work if there is a retimer in-between. For examples:

DR transceiver

LR transceiver

chip-to-module transceiver

Sorry, perhaps I wasn't clear. Is there a list of specific module types you're planning to have that explicitly does (or doesn't) support LT somewhere? If so, how is it defined?

CR modules only, the details will later be updated to the HLD

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

dgsudharsan · 2022-02-09T04:41:47Z

doc/port_link_training/port-link-training-design.md

+
+Vendor-specific SAI implementation is not in the scope of this document, but there are some common requirements for SAI:
+
+1. SAI implementation must return error code if any of the above attributes is not supported, swss and syncd must not crash.


Please ensure that SAI supports sai_query_attribute_capability and use it before calling link training set

Agree. Maybe we could query the link training SAI capability and put it into STATE DB, so that CLI can use it and tell user if the platform does not support it.

Thanks for the comments, in the current implementation (i.e. PR for sonic-utilities) it resues the xcvr_capabilities in APPL_DB for this, and we could also duplicate this info to STATE_DB, and the xcvr_capabilities will later be reused in AutoNeg for the AN and speed capability, and the speed capability may changed per port breakout operation, hence it's better to create a new 'TABLE TRANSCEIVER_CAPS' for these dynamic values, if we want to make a copy in STATE_DB. Please feel free to share your thoughts, thank you

@ds952811 I believe the SAI capabilities should determine whether xcvrd should publish the capabilities. Please ensure that xcvrd publishes the capabilities only when orchagent has updated STATE DB from SAI that link training is supported by ASIC.

Thanks for the comments, and based on the conclusion of the earlier meeting, the changes against xcvrd will be dropped, and the config command should directly reference the TRANSCEIVER_INFO of STATE_DB for the ability checks.

Agree. Maybe we could query the link training SAI capability and put it into STATE DB, so that CLI can use it and tell user if the platform does not support it.

Yep, maybe we could create a new table for the capabilities, will add this later

Agree. Maybe we could query the link training SAI capability and put it into STATE DB, so that CLI can use it and tell user if the platform does not support it.

Yep, maybe we could create a new table for the capabilities, will add this later

Unfortunately, there is a conflict on this from the comment in AutoNeg PR, which is for the timing constraint in the warm-reboot scenario. Hence the capability will only be initialized when there is a AN or LT config update, that being said the capabilities list will not be initiated during the system startup.

i.e. We're not going to add this into STATE_DB for now

dgsudharsan · 2022-02-09T04:44:28Z

doc/port_link_training/port-link-training-design.md

+
+ | Mode |     Description                                                 |
+ |:----:|:---------------------------------------------------------------:|
+ | auto | Enable link-training if applicable to the transceiver attached  |


Please remove the auto mode as discussed

Sure, will do, thanks for the reminder

dgsudharsan · 2022-02-09T04:45:13Z

doc/port_link_training/port-link-training-design.md

+	key                     = PORT_TABLE:port_name ; configuration of the port
+	; field                 = value
+	...
+	link_training           = STRING               ; operational link training config


Please rename the variable to reflect if it is admin or oper status.

link_training is the admin, and all the operational status will be integrated as enum like string into link_training_status

dgsudharsan · 2022-02-09T04:49:37Z

doc/port_link_training/port-link-training-design.md

+fvp = swsscommon.FieldValuePairs([
+    ('type', 'link_training')
+])
+port_np.send('port_state_refresh', port, fvp)


Please add the sequence diagram. While portsorch is the consumer of this notification who is going to be the producer for this PORT_CTRL?

Considering the further integration with CLIs (REST, KLISH...etc.) and avoid possible confusions, it's better to periodically poll the status when LT is enabled, hence this will be updated to a periodical polling

dgsudharsan · 2022-02-09T04:50:34Z

doc/port_link_training/port-link-training-design.md

+
+When **link_training=auto**, swss#orchagent should request syncd to enable port link training only if **LT** is specified in **xcvr_capabilities**.
+
+### Warmboot Design Impact


Please add considerations for fast reboot. Currently pmon starts after syncd and with link training in place how will this be impacted?

If I'm right about it, the fast-reboot is a case that kernel will be restarted from loading a new ELF file, without BIOS intervention, and this will be a COLD-REBOOT scenario, from the switch silicon, swss and pmon perspective, hence none extra care will be necessary.
In the case of warm-reboot, the APPL_DB and STATE_DB will be restored, and the switch silicon hardware state will not be changed and seamless resumed, all we need is to make sure static pre-emphasis request will not be applied in this scenario. Of course, a system test could provide more details, and I'll update the HLD if any issues are observed.

When I meant in case of fast-boot, the requirement is to have downtime less than 30 seconds. In normal scenario this wouldn't be a problem as syncd can come up and initialize the ports whereas in this case the xcvrd should publish the capabilities which will be required for link to be up. So the link downtime may be greater than 30 seconds. Please verify this scenario

Thanks for the comments, and based on the conclusion of the earlier meeting, the changes against xcvrd will be dropped, and the config command should directly reference the TRANSCEIVER_INFO of STATE_DB for the ability checks.

dgsudharsan · 2022-02-09T04:52:20Z

doc/port_link_training/port-link-training-design.md

+String value, the operational port link training failure status. "none" indicates no error is detected, otherwise any of the link training failure status defined in SAI. For example: "none", "lock", "snr" and "timeout".
+- link_training_rxstatus:  
+String value, the operational port link training rx status. any of the link training rx status defined in SAI. For example: "trained", "not-trained".
+- xcvr_capabilities:


What are all the list of values that can be in xcr_capabilities apart from LT. Please detail it in the HLD.

As of now, only 'LT' will be present, and yes, I'll update the HLD for this, thanks for the comment

dgsudharsan · 2022-02-09T04:55:39Z

doc/port_link_training/port-link-training-design.md

+
+#### PMON xcvrd Considerations
+
+Given that port link training shall not be enabled on certain transceivers. For example, a chip-to-module transceiver. It's better to have pmon#xcvrd provide a hint to the swss#orchagent for the advanced link training controls. Hence, **xcvr_capabilities** is introduced into APPL_DB.  


If you can capture the types of transceivers which you plan to incorporate in code that can support LT vs that cannot it would be great.
What is going to be the action when user enables LT on a transceiver that doesn't support? If we emit error log, capture it here.

The xcvrd will inspect the 'specification_compliance' and set 'LT' to xcvr_capability if it's a CR module (SR may also works, but some of the transceivers may inappropriately claims itself a SR module, while it's a CuSFP with RJ45.

In the swss#orchagent, a error/warning will be logged for this, but the operation will still proceed

In the config command (sonic-utilitie), the users will be blocked by an error indicate this is an unsupported module, and user could always use '-f' to force to run the operation

Junchao-Mellanox · 2022-02-10T02:21:29Z

doc/port_link_training/port-link-training-design.md

+
+Arguments:
+  interface_name: name of the interface to be configured. e.g: Ethernet0
+  mode: link training mode, can be either "auto", "on" or "off"


If the mode is auto, what is the value of SAI_PORT_ATTR_LINK_TRAINING_ENABLE?

Per the discussion in the last meeting, the auto mode will be dropped

Junchao-Mellanox · 2022-02-10T02:47:53Z

doc/port_link_training/port-link-training-design.md

+
+Vendor-specific SAI implementation is not in the scope of this document, but there are some common requirements for SAI:
+
+1. SAI implementation must return error code if any of the above attributes is not supported, swss and syncd must not crash.


Agree. Maybe we could query the link training SAI capability and put it into STATE DB, so that CLI can use it and tell user if the platform does not support it.

Junchao-Mellanox · 2022-02-10T02:48:53Z

doc/port_link_training/port-link-training-design.md

+To minimize system overhead, instead of periodically fetching the operational link training status, the corresponding fields in the APPL_DB will only be updated in the following events
+
+- Per-Port link status changes  
+In this case, only the **link_training_status** will be updated.


How does it get link training status? By which SAI attribute?

The operation state could be fetch by the following SAI attributes

SAI_PORT_ATTR_LINK_TRAINING_FAILURE_STATUS

SAI_PORT_ATTR_LINK_TRAINING_RX_STATUS
And 'link_training_status' will be updated to display only one of the best appropriate description for the current state.
e.g. 'not_trained' will only be presented when none of failure is reported by SAI_PORT_ATTR_LINK_TRAINING_FAILURE_STATUS, and the LT hardware engine failed to get a fine-tuned pre-emphasis

The HLD will later be updated to have this information, it should be done later today

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

dgsudharsan · 2022-03-02T03:54:59Z

@ds952811 What will be the behavior if AN and LT are turned on together? Are there any CLI level blocking planned for it?

ds952811 · 2022-03-02T04:13:26Z

@ds952811 What will be the behavior if AN and LT are turned on together? Are there any CLI level blocking planned for it?

From the Intel FPGA spec., the AN and LT could be enabled/disabled separately
https://www.intel.com/content/www/us/en/docs/programmable/683100/21-1-19-2-0/auto-negotiation-and-link-training.html
https://www.intel.com/content/www/us/en/docs/programmable/683100/21-1-19-2-0/auto-negotiation-and-link-training-registers.html
In the current Broadcom SDK-6.5.21 implementation, the LT is always enabled when AN is activated and it can't be disabled.

Conclusion:
None of CLI is necessary to block a user from such an AN/LT combination, and swss#orchagent should log an error if the 'link_training=off' + 'autoneg=on' while it's invalid to the vendor-specific SAI implementation. The HLD will later be updated.

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

dgsudharsan · 2022-03-10T06:39:20Z

doc/port_link_training/port-link-training-design.md

+  - empty upon success
+```
+
+For deployment considerations, when the link-training is enabled on a transceiver without LT capabilities, this config command should report an error and get aborted, unless '-f' option is specified.


Since we have removed pmon related sections, I believe this line becomes irrelevant. Can you please remove?

HLD updated

dgsudharsan · 2022-03-10T06:41:00Z

doc/port_link_training/port-link-training-design.md

+if autoneg changed:
+    setPortAutoNeg(port, autoneg)
+
+if link_training changed and "LT" in xcvr_capabilities:


Is this relevant since pmon sections are removed?

HLD updated

dgsudharsan · 2022-03-10T06:43:03Z

doc/port_link_training/port-link-training-design.md

+admin@sonic:~$ show interfaces link-training status
+  Interface      LT Oper    LT Admin    Oper    Admin        Error Description
+-----------  -----------  ----------  ------  -------  -----------------------
+  Ethernet0      trained          on      up       up  unsupported transceiver


I believe unsupported transceiver will not be relevant since we removed pmon related sections.

HLD updated

dgsudharsan · 2022-03-10T06:43:57Z

doc/port_link_training/port-link-training-design.md

+Arguments:
+  interface_name: [mandatory] name of the interface to be configured. e.g: Ethernet0
+  mode: [mandatory] link training mode, can be either "on" or "off"
+  -f: [optional] forcing link-training configuration on an unsupported transceiver


Can we remove the -f option since we don't perform transceiver checking?

HLD updated

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

lguohan · 2022-03-15T20:43:41Z

doc/port_link_training/port-link-training-design.md

+
+#### System Test cases
+
+Will leverage sonic-mgmt to test this feature.


can you be more specific?

will there be new test cases added for this new feature?

Thanks for the comment, and the HLD is now updated, please check it out and see if it works

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

Junchao-Mellanox · 2022-04-21T02:22:49Z

Will this comment be considered? #925 (comment) ?

This is part of HLD sonic-net/SONiC#925 #### Why I did it Add link-training support #### How I did it Update SONiC YANG for port link-training support #### Description for the changelog Add "link_training" to sonic-port.yang #### Link to config_db schema for YANG module changes https://github.com/sonic-net/SONiC/wiki/Configuration#port

HLD: sonic-net/SONiC#925 #### What I did Add CLI support for link-trainig #### How I did it 1. portconfig: initial support for link-training config 2. config/main.py: initial support for link-training config 3. intfutil: initial support for link-training show command 4. show/interfaces/__init__.py: initial support for link-training show command #### How to verify it 1. Manual test 2. Ran the Unit-tests to the corresponding changes #### Previous command output (if the output of a command-line utility has changed) #### New command output (if the output of a command-line utility has changed) ``` admin@sonic:~$ sudo config interface link-training Ethernet0 on admin@sonic:~$ sudo config interface link-training Ethernet8 on admin@sonic:~$ sudo config interface link-training Ethernet16 off admin@sonic:~$ sudo config interface link-training Ethernet24 on admin@sonic:~$ sudo config interface link-training Ethernet32 off admin@sonic:~$ show interfaces link-training status Interface LT Oper LT Admin Oper Admin ----------- ----------- ---------- ------ ------- Ethernet0 trained on up up Ethernet8 trained on up up Ethernet16 off off down up Ethernet24 not trained on down up Ethernet32 off off down up Ethernet40 off - down up Ethernet48 off - down up Ethernet56 off - down up Ethernet64 off - down up Ethernet72 off - down up Ethernet80 off - down up Ethernet88 off - down up Ethernet96 off - down up Ethernet104 off - down up Ethernet112 off - down up Ethernet120 off - down up Ethernet128 off - down up Ethernet136 off - down up Ethernet144 off - down up Ethernet152 off - down up Ethernet160 off - down up Ethernet168 off - down up Ethernet176 off - down up Ethernet184 off - down up Ethernet192 off - down up Ethernet200 off - down up Ethernet208 off - down up Ethernet216 off - down up Ethernet224 off - down up Ethernet232 off - down up Ethernet240 off - down up Ethernet248 off - down up admin@sonic:~$ ```

ds952811 force-pushed the link-training branch 5 times, most recently from 0c4b84a to a029855 Compare January 30, 2022 02:39

mikeberesford reviewed Feb 8, 2022

View reviewed changes

link-training: initial commit

d86199f

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

ds952811 force-pushed the link-training branch from a029855 to d86199f Compare February 8, 2022 11:55

dgsudharsan suggested changes Feb 9, 2022

View reviewed changes

Junchao-Mellanox suggested changes Feb 10, 2022

View reviewed changes

ds952811 added 2 commits February 18, 2022 14:54

Address review comments

5d1fb59

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

cosmetic updates

5055860

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

prgeor self-assigned this Mar 2, 2022

ds952811 mentioned this pull request Mar 9, 2022

saiport: add support for link training ability query opencomputeproject/SAI#1434

Merged

drop xcvrd changes, add support for LT ability query

ec399de

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

dgsudharsan suggested changes Mar 10, 2022

View reviewed changes

address review comments

caf78d8

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

dgsudharsan previously approved these changes Mar 10, 2022

View reviewed changes

lguohan reviewed Mar 15, 2022

View reviewed changes

add test cases to sonic-mgmt

9809574

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

ds952811 dismissed dgsudharsan’s stale review via 9809574 March 17, 2022 02:03

This was referenced Mar 25, 2022

schema.h: add STATE_LINK_TRAINING_TABLE_NAME sonic-net/sonic-swss-common#592

Closed

portsorch: initial support for link-training and remote advertisement sonic-net/sonic-swss#2149

Closed

yxieca force-pushed the master branch 2 times, most recently from 8498931 to 8837dc2 Compare April 15, 2022 16:51

ds952811 added 2 commits April 18, 2022 08:50

add flow diagrams and pre-emphasis restoration

75208f5

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

fix the typos in filename of flow diagrams

5b8291a

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

dgsudharsan approved these changes Apr 21, 2022

View reviewed changes

Junchao-Mellanox approved these changes Apr 21, 2022

View reviewed changes

lguohan merged commit c64b087 into sonic-net:master Apr 27, 2022

ds952811 mentioned this pull request May 16, 2022

sonic-utils: initial support for link-training sonic-net/sonic-utilities#2071

Merged

ds952811 mentioned this pull request Jun 28, 2022

portsorch: initial support for link-training sonic-net/sonic-swss#2359

Merged


		#### PMON xcvrd Considerations

		Given that port link training shall not be enabled on certain transceivers. For example, a chip-to-module transceiver. It's better to have pmon#xcvrd provide a hint to the swss#orchagent for the advanced link training controls. Hence, xcvr_capabilities is introduced into APPL_DB.


		Vendor-specific SAI implementation is not in the scope of this document, but there are some common requirements for SAI:

		1. SAI implementation must return error code if any of the above attributes is not supported, swss and syncd must not crash.


		When link_training=auto, swss#orchagent should request syncd to enable port link training only if LT is specified in xcvr_capabilities.

		### Warmboot Design Impact


		#### System Test cases

		Will leverage sonic-mgmt to test this feature.

[link-training] initial support #925

[link-training] initial support #925

Conversation

ds952811 commented Jan 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dgsudharsan commented Mar 2, 2022

ds952811 commented Mar 2, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Junchao-Mellanox commented Apr 21, 2022

ds952811 commented Jan 13, 2022 •

edited

Loading