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

Create CMIS-custom-SI-settings.md #1334

Merged
merged 10 commits into from
Jul 14, 2023
Merged

Conversation

AnoopKamath
Copy link
Contributor

@AnoopKamath AnoopKamath commented May 8, 2023

Proposal to apply host defined SI parameters to CMIS supported modules.

Repo PR Title State
sonic-platform-daemons [CMIS][sonic-platform-daemon]Apply custom SI settings Review
sonic-platform-common [CMIS][sonic-platform-common]Apply custom SI settings Review

Proposal to apply host defined SI parameters to module in the CMIS state machine.
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented May 8, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@zhangyanzhao
Copy link
Collaborator

@AnoopKamath please accept the EasyCLA and that is required before you can commit code to SONiC repos.

doc/sfp-cmis/CMIS-custom-SI-settings.md Outdated Show resolved Hide resolved
doc/sfp-cmis/CMIS-custom-SI-settings.md Outdated Show resolved Hide resolved
doc/sfp-cmis/CMIS-custom-SI-settings.md Outdated Show resolved Hide resolved
doc/sfp-cmis/CMIS-custom-SI-settings.md Outdated Show resolved Hide resolved
@zhangyanzhao
Copy link
Collaborator

@AnoopKamath can you please add the code PRs to this HLD by referring to #806 ? Thanks.

@zhangyanzhao
Copy link
Collaborator

@zhangyanzhao
Copy link
Collaborator

@AnoopKamath can you please help to update the HLD with this template https://github.com/sonic-net/SONiC/blob/master/doc/hld_template.md? Thanks.

Address review comments
@AnoopKamath
Copy link
Contributor Author

@AnoopKamath can you please add the code PRs to this HLD by referring to #806 ? Thanks.

Can you please provide an example of what needs to referred from #806 ?

@AnoopKamath
Copy link
Contributor Author

@AnoopKamath can you please help to update the HLD with this template https://github.com/sonic-net/SONiC/blob/master/doc/hld_template.md? Thanks.

Taken care

Copy link
Contributor

@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.

@AnoopKamath In the code please don't forget to check for the advertised capabilities for many of these SI settings are supported or not.

@AnoopKamath
Copy link
Contributor Author

@AnoopKamath In the code please don't forget to check for the advertised capabilities for many of these SI settings are supported or not.

@prgeor Sure. will take care. Thanks

},
"PORT_MEDIA_SETTINGS": {
"31": {
"100G_SPEED": {
Copy link
Contributor

@tshalvi tshalvi Jun 27, 2023

Choose a reason for hiding this comment

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

Can you please clarify how are you going to get this lane speed to use it as part of the key? Where is it stored?

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 lane speed in JSON will be compared with lane_speed generated in CMIS FSM using module speed divided by host lane count. This lane speed will be passed to the api which we set module SI settings

```
{
"TX_SETTING": {
"EQ_FIXED": "False",
Copy link
Contributor

Choose a reason for hiding this comment

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

@AnoopKamath why do you assume the TX Eq will be fixed? Should we not check module advt. on Page 01h, Byte 161 bit 3?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed TX/RX/EQ_FIXED details from JSON. I had added TX EQ details in JSON file to start with. I am checking this in code before applying. TX/RX details can be added directly in the JSON

prgeor
prgeor previously approved these changes Jun 27, 2023
@AnoopKamath
Copy link
Contributor Author

@AnoopKamath In the code please don't forget to check for the advertised capabilities for many of these SI settings are supported or not.

@prgeor Taken care

@eddyk-nvidia
Copy link

@AnoopKamath In the code please don't forget to check for the advertised capabilities for many of these SI settings are supported or not.

@prgeor Taken care

Shouldn't new values (from the config file) be validated against both advertised (1.162) and values (1.153-154) ?

@AnoopKamath
Copy link
Contributor Author

@AnoopKamath In the code please don't forget to check for the advertised capabilities for many of these SI settings are supported or not.

@prgeor Taken care

Shouldn't new values (from the config file) be validated against both advertised (1.162) and values (1.153-154) ?

@eddyk-nvidia, Thank you for pointing this out. I have taken care of validating the config against 1.161 and 1.162 advertised in the code. I have add logic to validate against 1.153-154 values also

@prgeor
Copy link
Contributor

prgeor commented Jul 14, 2023

@AnoopKamath can you update the PR description with your supporting code change PR like #1076

@AnoopKamath
Copy link
Contributor Author

@AnoopKamath can you update the PR description with your supporting code change PR like #1076

@prgeor Done. Thanks

2.2. Using this module key (2.1), the search begins for the detected port in the GLOBAL_MEDIA_SETTINGS section. If the module key matches any entries or any search that matches for (2.3) to (2.5) sections, then SI attributes from this section are copied to the SI param attribute list (2.7).
2.3. If no match happens in 2.2, reduce the search to module default + speed of the detected port in GLOBAL_MEDIA_SETTINGS
2.4. If no match happens in 2.3, reduce the search to the speed of the detected port in GLOBAL_MEDIA_SETTINGS
2.5. If no match happens in the GLOBAL_MEDIA_SETTINGS block, the search now begins in the PORT_MEDIA_SETTINGS block for the detected port. If no match happens in the PORT_MEDIA_SETTINGS block, the final search for the default block is done.
Copy link
Contributor

Choose a reason for hiding this comment

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

@AnoopKamath in the PORT_MEDIA_SETTINGS, I guess the search will be more specific like check for vendor specific block and if not then reduce to default.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prgeor : Yes correct. Same as GLOBAL_MEDIA_SETTINGS block: speed -> vendor ->default. May be I can add it clearly to the HLD if there will be any other changes required to the HLD in the future

@prgeor prgeor merged commit 9b7c6b1 into sonic-net:master Jul 14, 2023
@skg-net
Copy link
Member

skg-net commented Feb 6, 2024

@AnoopKamath Can you please update the Quality Metric (Alpha/Beta/GA) for the feature either in this PR comments or in HLD itself based on https://github.com/sonic-net/SONiC/blob/master/doc/SONiC%20feature%20quality%20definition.md
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants