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

Pddf_support_dell_platforms: Add PDDF support for s52xx dell platforms. #18849

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

arun1355492
Copy link

@arun1355492 arun1355492 commented May 2, 2024

Why I did it

Add PDDF support for the dell s52xx platforms.

How I did it

Add PDDF configuration files, scripts and python files.

How to verify it

To switch to PDDF mode run the below command and wait for the system to be ready.
pddf_util.py -d switch-pddf

Run the PDDF commnads
systemctl | grep -i pddf
pddf_fanutil direction
pddf_fanutil getspeed
pddf_fanutil numfans
pddf_fanutil status
pddf_psuutil mfrinfo
pddf_psuutil numpsus
pddf_psuutil seninfo
pddf_psuutil status
pddf_thermalutil gettemp
pddf_thermalutil numthermals
pddf_ledutil getstatusled SYS_LED
pddf_ledutil getstatusled FAN_LED
pddf_ledutil getstatusled FANTRAY1_LED
pddf_ledutil getstatusled FANTRAY2_LED
pddf_ledutil getstatusled FANTRAY3_LED
pddf_ledutil getstatusled FANTRAY4_LED

Note: To switch back to non PDDF mode check help on pddf_util.py -help

  • 202305

Unit Test results:
s5232f_pddf_logs.txt
s5248f_pddf_logs.txt
s5296_pddf_logs.txt
s5212f_pddf_logs.txt
s5224f_pddf_logs.txt

A picture of a cute animal (not mandatory but encouraged)

@arun1355492 arun1355492 requested a review from lguohan as a code owner May 2, 2024 09:16
Copy link

linux-foundation-easycla bot commented May 2, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@arun1355492 arun1355492 marked this pull request as draft May 2, 2024 09:17
@arun1355492 arun1355492 changed the title [DELL] pddf_support_dell_platforms: Add PDDF support for s5212f, s5224f, s5232f and s5248f dell platforms. Pddf_support_dell_platforms: Add PDDF support for s5212f, s5224f, s5232f and s5248f dell platforms. May 2, 2024
@arun1355492 arun1355492 changed the title Pddf_support_dell_platforms: Add PDDF support for s5212f, s5224f, s5232f and s5248f dell platforms. Pddf_support_dell_platforms: Add PDDF support for s52xx dell platforms. May 4, 2024
@arun1355492 arun1355492 marked this pull request as ready for review May 7, 2024 18:39
@arun1355492
Copy link
Author

@srideepDell Please review the PR.

@jeff-yin
Copy link
Collaborator

jeff-yin commented May 8, 2024

@lguohan @zhangyanzhao -- please assign @srideepDell , @rvasanthm , @thaj-deen , @vpsubramaniam , @arunlk-dell to review this Dell platform PR. Also, @FuzailBrcm and @leeprecy from Broadcom, who have expertise in PDDF.

Thanks!

@lguohan
Copy link
Collaborator

lguohan commented May 12, 2024

@jeff-yin , no need to assign. please have reviewer to sign off, then we can merge.

Copy link
Contributor

@aravindmani-1 aravindmani-1 left a comment

Choose a reason for hiding this comment

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

From the UT logs, Hardware Revision is displayed as "N/A". Could you please check it?.
Also, please attach sonic-mgmt test results.

@FuzailBrcm
Copy link
Contributor

@leeprecy @geans-pin
Please review these changes

Copy link
Contributor

@aravindmani-1 aravindmani-1 left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

why switch_board_sfp/ switch_board_qsfp is not deinitialized like it is done for other platforms?.

Copy link
Author

Choose a reason for hiding this comment

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

They are done as part of s52xxf_platform.sh under deinitialized.

Copy link
Contributor

@aravindmani-1 aravindmani-1 Jul 13, 2024

Choose a reason for hiding this comment

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

media_down is not used in platform init file. Check the same for all the platform init files.

Copy link
Author

Choose a reason for hiding this comment

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

Implemented changes similar to other platform init files.

@@ -128,8 +128,7 @@ switch_board_led_default() {
install_python_api_package() {
device="/usr/share/sonic/device"
platform=$(/usr/local/bin/sonic-cfggen -H -v DEVICE_METADATA.localhost.platform)

pip3 install $device/$platform/sonic_platform-1.0-py3-none-any.whl
rv=$(pip3 install $device/$platform/sonic_platform-1.0-py3-none-any.whl --force-reinstall -q --root-user-action=ignore)
Copy link
Contributor

Choose a reason for hiding this comment

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

why --force-reinstall option and --root-user-action rules added here?.

Copy link
Contributor

@aravindmani-1 aravindmani-1 Jul 13, 2024

Choose a reason for hiding this comment

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

return value rv is unused here.

@@ -0,0 +1,125 @@
#!/usr/bin/env python

Choose a reason for hiding this comment

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

should it be python or python3 ?

Copy link
Author

Choose a reason for hiding this comment

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

Should be Python, ensures that the script is executed with the Python interpreter found in the user’s environment, regardless of where Python is installed on the system.


return True

def main():
Copy link

@rohinikumart rohinikumart Jul 15, 2024

Choose a reason for hiding this comment

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

I see main is dummy. is that fine ?

Copy link
Author

Choose a reason for hiding this comment

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

Fine and serving as a placeholder.

@@ -31,40 +31,75 @@ override_dh_auto_build:
python3 -m build --wheel --no-isolation --outdir $(MOD_SRC_DIR)/$${mod}/modules; \
cd $(MOD_SRC_DIR); \
elif [ $$mod = "z9264f" ]; then \
cp $(COMMON_DIR)/dell_fpga_ocores.c $(MOD_SRC_DIR)/$${mod}/modules/dell_z9264f_fpga_ocores.c; \

Choose a reason for hiding this comment

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

What is the change here ?

Copy link
Author

Choose a reason for hiding this comment

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

What is the change here ?

No change.

Choose a reason for hiding this comment

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

can you remove this from diff then ?

Copy link
Author

Choose a reason for hiding this comment

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

sure

@arun1355492
Copy link
Author

could you please use https://github.com/sonic-net/sonic-buildimage/blob/master/platform/broadcom/sonic-platform-modules-dell/common/ipmihelper.py instead of redefining ipmihelper in each platform.

Changed to use from the common folder.

@arun1355492
Copy link
Author

arun1355492 commented Sep 30, 2024

From the UT logs, Hardware Revision is displayed as "N/A". Could you please check it?. Also, please attach sonic-mgmt test results.

I have ran sonic-mgmt test for the dell platforms in pddf mode and added required function as part of sonic-mgmt test. Also addressed review comments.

Attaching latest UT logs and sonic-mgmt test case report. Some of the test cases are failed due to name mismatch from the platform.json which will resolved in the future enhancements whenever newer pddf updates.

Platforms s5232f and s5248f does not have platform.json. s5248f will have platform.json from https://github.com/sonic-net/sonic-buildimage/pull/20287/files and s5232f will have platform.json in future updates.

s5212f_logs.txt
s5212f_pddf_logs.txt
s5224f_logs.txt
s5224f_pddf_logs.txt
s5232f_logs.txt
s5232f_pddf_logs.txt
s5248f_logs.txt
s5248f_pddf_logs.txt
s5296f_logs.txt
s5296f_pddf_logs.txt
sonic_mgmt_test_report.xlsx

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

Successfully merging this pull request may close these issues.

7 participants