-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Conversation
Proposal to apply host defined SI parameters to module in the CMIS state machine.
@AnoopKamath please accept the EasyCLA and that is required before you can commit code to SONiC repos. |
Address review Comments
@AnoopKamath can you please add the code PRs to this HLD by referring to #806 ? Thanks. |
community review recording https://zoom.us/rec/share/PXb9n4lWMc097r2880NvKq06TKjEUWYnTGW_vtOPLa_mwbCfaEotmYj2ZXRZQmU.Dt2FZmQ3eZph39wA |
@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
Can you please provide an example of what needs to referred from #806 ? |
Taken care |
There was a problem hiding this 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.
@prgeor Sure. will take care. Thanks |
Address review comments
Fix Json file attributes
}, | ||
"PORT_MEDIA_SETTINGS": { | ||
"31": { | ||
"100G_SPEED": { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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", |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 Taken care |
…appropriate sections having this data
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 |
@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. |
There was a problem hiding this comment.
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.?
There was a problem hiding this comment.
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
@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 |
Proposal to apply host defined SI parameters to CMIS supported modules.