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

[gearbox] Support setting tx taps on gearbox ports #2158

Merged
merged 3 commits into from
Dec 8, 2022

Conversation

byu343
Copy link
Contributor

@byu343 byu343 commented Feb 24, 2022

What I did
This change adds support for setting tx tap or tuning values on gearbox ports. It uses the SAI attributes such as SAI_PORT_SERDES_ATTR_TX_FIR_PRE1 to communicate with SAI-based gearbox drivers. For the values, they are provided in the format like "system_tx_fir_pre2": [1,1] for an interface from gearbox_config.json.

Why I did it

How I verified it
We verified that values provided in sonic-net/sonic-buildimage#10084 are set to the chip with this change.

Added test to tests/test_gearbox.py. The added test will not pass until the following two changes (which should be merged first) are merged:
Support SAI_PORT_ATTR_PORT_SERDES_ID on vs gearbox: sonic-net/sonic-sairedis#1082
Add gearbox taps to vs gearbox_config.json: sonic-net/sonic-buildimage#11480

Details if related

@prgeor
Copy link
Contributor

prgeor commented Feb 25, 2022

@byu343 do ensure that during warm-reboot we are not setting these taps because it can cause the port to flap?

@prgeor
Copy link
Contributor

prgeor commented Feb 25, 2022

What I did This change adds support for setting tx tap or tuning values on gearbox ports. It uses the SAI attributes such as SAI_PORT_SERDES_ATTR_TX_FIR_PRE1 to communicate with SAI-based gearbox drivers. For the values, they are provided in the format like "system_tx_fir_pre2": [1,1] for an interface from gearbox_config.json.

Why I did it

How I verified it We verified that values provided in Azure/sonic-buildimage#10084 are set to the chip with this change.

Details if related

@byu343 is this "system_tx_fir_pre2": [1,1]" format captured in HLD? can you point me the HLD?

@prgeor prgeor added the gearbox label Feb 25, 2022
@prgeor
Copy link
Contributor

prgeor commented Feb 25, 2022

@Pterosaur @jimmyzhai please review

@byu343
Copy link
Contributor Author

byu343 commented Feb 26, 2022

@byu343 is this "system_tx_fir_pre2": [1,1]" format captured in HLD? can you point me the HLD?

No, this is no captured by HLD. I followed the existing format for system_lanes, such as: "system_lanes": [6,7]. And each value in system_tx_fir_pre2 matches the corresponding lane.

@prgeor
Copy link
Contributor

prgeor commented Apr 13, 2022

@byu343 could you fix the merge conflicts?

@byu343
Copy link
Contributor Author

byu343 commented Apr 15, 2022

@byu343 could you fix the merge conflicts?

Done

@byu343 byu343 closed this Apr 15, 2022
@byu343
Copy link
Contributor Author

byu343 commented Apr 15, 2022

@byu343 could you fix the merge conflicts?

Done

Reopen it

@byu343 byu343 reopened this Apr 15, 2022
@Pterosaur Pterosaur requested a review from jimmyzhai April 15, 2022 01:57
@kenneth-arista
Copy link
Contributor

Tagging @arlakshm for awareness

@byu343
Copy link
Contributor Author

byu343 commented May 10, 2022

@byu343 do ensure that during warm-reboot we are not setting these taps because it can cause the port to flap?

For the credo gearbox phys, I think warm-reboot support is not considered for now.

@arlakshm
Copy link
Contributor

/Azp run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@byu343
Copy link
Contributor Author

byu343 commented Jul 19, 2022

@Pterosaur @jimmyzhai Could please help to review this? Thanks.

orchagent/portsorch.cpp Outdated Show resolved Hide resolved
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
orchagent/portsorch.cpp Outdated Show resolved Hide resolved
tests/test_gearbox.py Show resolved Hide resolved
@byu343 byu343 force-pushed the gearbox-taps branch 2 times, most recently from 01fc353 to ad1d17b Compare August 8, 2022 23:00
@abdosi
Copy link
Contributor

abdosi commented Sep 20, 2022

@anamehra: Please tag Shyam/Arpit.

@anamehra
Copy link

@anamehra: Please tag Shyam/Arpit.

@shyam77git

@arpp93
Copy link

arpp93 commented Sep 20, 2022

Couple of questions on this:

  1. Is adding the taps to the gearbox_config files mandatory or optional?
  2. Do you expect the SAI calls to be implemented in the platform mandatorily for these or are they optional?

Regards,
Arpit

@byu343
Copy link
Contributor Author

byu343 commented Sep 20, 2022

Couple of questions on this:

  1. Is adding the taps to the gearbox_config files mandatory or optional?
  2. Do you expect the SAI calls to be implemented in the platform mandatorily for these or are they optional?

Regards, Arpit

Hi Arpit, whether to provide those values in gearbox_config.json for any platform is optional. If orchagent finds the values from the gearbox_config.json, it will calls SAI implementation with the values; if it cannot, there is no calls to SAI for setting the tuning/tap values, so the tuning/tap support is not mandatory for the phy driver.

arlakshm pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Oct 10, 2022
Why I did it
For the change to support gearbox taps by gearbox_config.json (sonic-net/sonic-swss#2158), I need to add tests to sonic-swss/tests/test_gearbox.py to satisfy the test coverage of the change. The existing code in test_gearbox.py has already used brcm_gearbox_vs and here is to add some gearbox tap value to its gearbox_config.json, for the added tests in sonic-swss/tests/test_gearbox.py.

How I did it
How to verify it
This change itself will not affect existing code.
@arlakshm
Copy link
Contributor

/Azp run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@arlakshm
Copy link
Contributor

Hi @arpp93, @prgeor, can you approve this PR is no other comments?

@byu343
Copy link
Contributor Author

byu343 commented Oct 25, 2022

/Azp run Azure.sonic-swss

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 2158 in repo sonic-net/sonic-swss

@byu343
Copy link
Contributor Author

byu343 commented Oct 28, 2022

/Azpw run Azure.sonic-swss

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-swss

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@byu343 byu343 requested review from jimmyzhai and removed request for prsunny and Pterosaur November 15, 2022 18:01
@byu343
Copy link
Contributor Author

byu343 commented Nov 15, 2022

Hi @jimmyzhai @arpp93 @prgeor, could you please review this again? Thanks.

@jimmyzhai jimmyzhai merged commit 6695113 into sonic-net:master Dec 8, 2022
dgsudharsan pushed a commit to dgsudharsan/sonic-swss that referenced this pull request Dec 9, 2022
What I did
This change adds support for setting tx tap or tuning values on gearbox ports. It uses the SAI attributes such as SAI_PORT_SERDES_ATTR_TX_FIR_PRE1 to communicate with SAI-based gearbox drivers. For the values, they are provided in the format like "system_tx_fir_pre2": [1,1] for an interface from gearbox_config.json.

Why I did it

How I verified it
We verified that values provided in sonic-net/sonic-buildimage#10084 are set to the chip with this change.

Added test to tests/test_gearbox.py. The added test will not pass until the following two changes (which should be merged first) are merged:
Support SAI_PORT_ATTR_PORT_SERDES_ID on vs gearbox: sonic-net/sonic-sairedis#1082
Add gearbox taps to vs gearbox_config.json: sonic-net/sonic-buildimage#11480

Updated handling of VRF_VNI mapping and VLAN_VNI mapping for same VNI ID

fixed compile issues

Updated code for the flow where VRF VNI mapping is processed first followed by VLAN VNI mapping
lguohan pushed a commit to sonic-net/sonic-buildimage that referenced this pull request Feb 15, 2023
Why I did it
This change specifies the tuning values for each lane of the B52 phy chips. These values can be different for different ports. The values being set are under the assumption of optical transceivers. This change depends on the change to sonic-swss: sonic-net/sonic-swss#2158.

How to verify it
We verified the values are correctly set on the B52 chips of Arista 7280cr3, by reading them from the debug cli of the B52 driver.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants