Skip to content

High power class enabling for SFF-8636 modules#521

Merged
prgeor merged 4 commits intosonic-net:masterfrom
longhuan-cisco:high_power_class_handling
Mar 16, 2025
Merged

High power class enabling for SFF-8636 modules#521
prgeor merged 4 commits intosonic-net:masterfrom
longhuan-cisco:high_power_class_handling

Conversation

@longhuan-cisco
Copy link
Contributor

@longhuan-cisco longhuan-cisco commented Dec 13, 2024

Description

Add platform common API support for enabling high power class if module's power class is greater or equal to 5 (Refer to SFF-8636 spec 6.2.6 Control Functions (Page 00h, Bytes 86-99) for byte 93 definition)

Motivation and Context

Without enabling high power class for those modules, the module won't operate properly with power output and link won't come up.

How Has This Been Tested?

Verified the module operate with proper power output.

Additional Information (Optional)

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@longhuan-cisco
Copy link
Contributor Author

/azp run

@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 521 in repo sonic-net/sonic-platform-common

@longhuan-cisco
Copy link
Contributor Author

/azpw run Azure.sonic-platform-common

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-platform-common

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mihirpat1
Copy link
Contributor

@longhuan-cisco Please help in fixing the built failure

@longhuan-cisco
Copy link
Contributor Author

/azpw run Azure.sonic-platform-common

@mssonicbld
Copy link
Collaborator

/AzurePipelines run Azure.sonic-platform-common

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@longhuan-cisco
Copy link
Contributor Author

@longhuan-cisco Please help in fixing the built failure

@mihirpat1 It was due to issue in CICD infra. Issue was gone after rerun.

@prgeor prgeor requested review from Copilot and mihirpat1 February 15, 2025 21:48
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment on lines +442 to +448
if power_class < 5:
return ret
elif power_class >= 8:
ret = self.xcvr_eeprom.write(consts.HIGH_POWER_CLASS_ENABLE_CLASS_8, enable)
else: # Power class 5, 6, 7
ret = self.xcvr_eeprom.write(consts.HIGH_POWER_CLASS_ENABLE_CLASS_5_TO_7, enable)

Copy link
Collaborator

Choose a reason for hiding this comment

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

@longhuan-cisco you also need to make sure that the SW is in control for driving the power class. The way to do that is by doing a write of 1 to Byte 93 bit 0 i.e power override. Let do that in sff manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

According to sff-8636, no matter whether power_override bit is set or not, enabled power class behaves the same.

Today, override bit is handled separately at the time of turning off lpmode.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@longhuan-cisco the point you are missing here is if you dont set the power override bit to 1 then LPmode must be de-asserted which is an extra thing to ensure. In SW control, where B93 bit 0 = 1, LPmode is don't care. Anyways, we should discuss this in your SFF manager PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this discussion to sff_mgr PR at sonic-net/sonic-platform-daemons#574 (comment)

@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Comment on lines +443 to +448
if power_class < 5:
return ret
elif power_class >= 8:
ret = self.xcvr_eeprom.write(consts.HIGH_POWER_CLASS_ENABLE_CLASS_8, enable)
else: # Power class 5, 6, 7
ret = self.xcvr_eeprom.write(consts.HIGH_POWER_CLASS_ENABLE_CLASS_5_TO_7, enable)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@longhuan-cisco can we simply this as follows
if power_class >= 8:

else if power_class >= 5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated it.

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
After I updated the code to address this comment. The approval gets dismissed.
could you please re-approve this PR. Thanks

prgeor
prgeor previously approved these changes Mar 9, 2025
@mssonicbld
Copy link
Collaborator

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@prgeor prgeor merged commit e5aedb6 into sonic-net:master Mar 16, 2025
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants