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

Platform Driver Development Framework (PDDF): Generic drivers and plugins #3387

Closed
wants to merge 8 commits into from

Conversation

FuzailBrcm
Copy link
Contributor

- What I did
This change introduces PDDF which is described here,
sonic-net/SONiC#406

Most of the platform bring up effort goes in developing the platform device drivers, SONiC plugins and validating them. Typically each platform vendor writes their own drivers and plugins which is very tailor made to that platform. This involves writing code, building, installing it on the target platform devices and testing. Many of the details of the platform are hard coded into these drivers, from the HW spec. They go through this cycle repetitively till everything works fine, and is validated before upstreaming the code.
PDDF aims to make this platform driver and plugin development process much simpler by providing a data driven development framework. This is enabled by:

  • JSON descriptor files for platform data
  • Generic data-driven drivers for various devices
  • Generic SONiC plugins
  • Vendor specific extensions for customisation and extensibility

- How I did it
PDDF generic platform drivers and utils are provided under
sonic-buildimage/platform/pddf

PDDF generic user space plugins are added here,
sonic-buildimage/device/common/pddf

Rest of the changes are added to enable the framework.

- How to verify it
New platforms may use this framework for their development. Currently, Accton AS7712-32X and AS7726-32X platforms support PDDF mode as an alternate to the usual bringup. I have provided the scripts to switch between PDDF and non-PDDF modes.

To switch to PDDF mode,
# sudo /usr/local/bin/pddf_util.py switch-pddf

Under PDDF mode, pddf utils can be verified,
# sudo pddf_psuutil <options>
# sudo pddf_fanutil <options>
# sudo pddf_thermalutil <options>
# sudo pddf_ledutil <options>

To switch back to the non-PDDF mode,
# sudo /usr/local/bin/pddf_util.py switch-nonpddf

- Description for the changelog
Platform Driver Development Framework (PDDF): Generic kernel device drivers and user plugins

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

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

As comments.

Also, when do you plan on integrating the generic platform 2.0 API into PDDF? Most vendors are currently working on transitioning to the new API. The old plugins should no longer be used.

platform/pddf/i2c/modules/include/pddf_fan_defs.h Outdated Show resolved Hide resolved
@FuzailBrcm
Copy link
Contributor Author

This PR has dependency on sonic-net/sonic-platform-common#62 which is approved. I have taken care of the comments related to tab-width etc.

@FuzailBrcm
Copy link
Contributor Author

This PR has dependency over

1- sonic-net/sonic-platform-common#62 , and
2- sonic-net/sonic-utilities#624

Out of these '1' is approved as well.

@srideepDell
Copy link
Contributor

srideepDell commented Oct 14, 2019

When is the plan to support thermal devices behind BMC chip also media behind a FPGA chip ?
Will the current utilities enhance for the new BMC and FPGA devices or a new design would be implemented ?

pddf_obj = pddfparse.PddfParse()
# system EEPROM always has device name EEPROM1
self.eeprom_path = pddf_obj.get_path("EEPROM1", "eeprom")
super(board, self).__init__(self.eeprom_path, 0, '', True)
Copy link
Contributor

Choose a reason for hiding this comment

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

is it linux dev name EEPROM1 created by the PPDF framework driver ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EEPROM1 is just the object name mentioned in the PDDF device json file. In PDDF, we call the system EEPROM with the name EEPROM1. For AS7712 and SA7726, we have used '1t24' driver for this device.

for port_num in range(self._port_start, self._port_end):
device = "PORT" + "%d"%(port_num+1)
port_eeprom_path = pddf_obj.get_path(device,"eeprom")
self._port_to_eeprom_mapping[port_num] = port_eeprom_path
Copy link
Contributor

Choose a reason for hiding this comment

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

how do we handle platforms which does not have to create eeprom for the ports example ( 1G ports) platforms ?
We need to device a eeprom mapping as none to skip the loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In such case, we wouldn't mention the 'PORT<>-EEPROM' objects in pddf device json files. That way, get_path() would return None.

self._qsfp_ports.append(port)
elif self._port_to_type_mapping[port] == 'SFP':
self._sfp_ports.append(port)

Copy link
Contributor

Choose a reason for hiding this comment

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

These port types are supposed to added by ODM when a new platform support is created example SPF28 or DDQSFP ?

TODO: This function need to be implemented
when decide to support monitoring SFP(Xcvrd)
on this platform.
"""
Copy link
Contributor

Choose a reason for hiding this comment

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

When is the plan to implement this. So once PDDF is adapted change of events will be critical for run-time plug-in/out of media.

@@ -0,0 +1,94 @@
#!/usr/bin/env python
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we have comments of what each file in the plugins does ? It helps in easy understanding also it could be incremented with comment any new api's added

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sysstatusutil.py is not used as of now. Sure we will add the comments.

* Copyright 2019 Broadcom. All rights reserved.
* The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
*
* A pddf kernel module to create I2C client for pca954x type of multiplexer
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this driver needed when native linux pca954x driver can create devices in sysfs/devfs as clients?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are also using the same PCA954x driver. However, There are some fields we have modified using the "pddf_mux_module". e.g base_bus which should be taken form device json object field 'parent_bus' which is a user-provided field. Hence we used pddf_mux_module. It works sort of wrapper on top of PCA954x module



static unsigned short normal_i2c[] = {0x50, 0x58, 0x51, 0x59, 0x53, 0x5b, I2C_CLIENT_END};

Copy link
Contributor

Choose a reason for hiding this comment

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

these addresses vary from platform to platform . how could we make it simple ? and keep it generic
Just parsing as arguments during insmodule ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Normal i2c should be made normal_i2c[] = {I2C_CLIENT_END } ... Will make that change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already parse the dev_addr from the JSON file descriptor. This normal_i2c for for quick scan. The device gets initialised even if the addresses is not mentioned in normal_i2c array.

PDDF_DATA_ATTR(cpld2_version, S_IWUSR|S_IRUGO, show_sysstatus_data, NULL, PDDF_UINT32, sizeof(uint32_t), NULL, NULL);
PDDF_DATA_ATTR(cpld3_version, S_IWUSR|S_IRUGO, show_sysstatus_data, NULL, PDDF_UINT32, sizeof(uint32_t), NULL, NULL);
PDDF_DATA_ATTR(power_module_status, S_IWUSR|S_IRUGO, show_sysstatus_data, NULL, PDDF_UINT32, sizeof(uint32_t), NULL, NULL);
PDDF_DATA_ATTR(system_reset1, S_IWUSR|S_IRUGO, show_sysstatus_data, NULL, PDDF_UINT32, sizeof(uint32_t), NULL, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

What does different types of reset means ? how will be common for all platforms ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generally SYSSTATUS object stores the miscellaneous information. The fields mentioned here were in relation to the platforms where PDDF is supported.

Different resets were, hard_reset, soft_reset, MAC reset etc.

rules/config Show resolved Hide resolved
return True

kos = [
'modprobe i2c-ismt',
Copy link
Contributor

Choose a reason for hiding this comment

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

This lis of ko's include linux specific dirvers and also pddf specific drivers. can we not keep it separate ?
Also allow users to add in /etc/module.d/generic.conf and /etc/module.d/pddf.conf ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separating the pddf drivers and linux drivers (which might differ based on platform) is already in plan for PDDF 2.0, where we will be moving to 2.0 Platform APIs implementation as well.

@FuzailBrcm
Copy link
Contributor Author

Changes in psu driver module as per the review comments
... commit id: f0b4dc2

srideepDell
srideepDell previously approved these changes Dec 3, 2019
@@ -0,0 +1,291 @@
/*
* Copyright 2019 Broadcom. All rights reserved.
* The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
Copy link
Collaborator

Choose a reason for hiding this comment

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

need gpl license

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taken care in
@fk410167

  1. GPL license disclaimer inclusion 2) Removal of commented code, as … …
    8e8cf2f


ret = sysfs_create_group(i2c_kobj, &pddf_psu_client_data_group);
if (ret)
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

the spacing is not consistent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is already taken care in commit: a654dc7.




#if __name__== "__main__":
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove these testing code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

taken care of in ,
@fk410167

  1. GPL license disclaimer inclusion 2) Removal of commented code, as … …
    8e8cf2f

Copy link
Contributor Author

@FuzailBrcm FuzailBrcm left a comment

Choose a reason for hiding this comment

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

Made changes are per the review comments

#GCOV_PROFILE := y

ccflags-y := -I$(M)/modules/include
#ccflags-y += -I$(M)/modules/psu
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

obj-m := $(TARGET).o

$(TARGET)-objs := pddf_psu_api.o pddf_psu_driver.o
#GCOV_PROFILE := y
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

struct cpld_client_node *cpld_node = NULL;
int ret = -EPERM;

//hw_preaccess_func_cpld_mux_default((uint32_t)cpld_addr, NULL);
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

#$(MAKE) -C /projects/slxos_dev/fk410167/18.2.01X/slxos_main/build/swbd4000/linux_4_9/ M=$(PWD) modules ;

#clean:
#$(MAKE) -C /projects/slxos_dev/fk410167/18.2.01X/slxos_main/build/swbd4000/linux_4_9/ M=$(PWD) clean ;
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -0,0 +1,4 @@
obj-m:= pddf_client_module.o
#GCOV_PROFILE := y
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lguohan
Copy link
Collaborator

lguohan commented Dec 10, 2019

since pddf is a driver framework, I am wondering if it should be in the src folder instead of platform folder.

ret = kstrtoint(buf,16,&num);
if (ret==0)
*(uint32_t *)(ptr->addr) = (uint32_t)num;
/*pddf_dbg(KERN_ERR "Stored value: 0x%x, num: 0x%x\n", *(int*)(ptr->addr), num);*/
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove commented code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@FuzailBrcm
Copy link
Contributor Author

Took care of the latest review comments in

Removing commented code as per the latest review comments 33ab9b7

Regarding movement of the code to 'src' folder,

  1. None of the BSP code or, HW devices related code is present outside of the 'platform' folder,
  2. PDDF code is kept in a separate 'platform/pddf' folder and not under any specific chip vendor,
  3. Inclusion of the PDDF code in the build is subjected to the config SONIC_USE_PDDF_FRAMEWORK.

Hence in my view it is better to keep PDDF kernel module code under 'platform'.

@FuzailBrcm
Copy link
Contributor Author

We need to push the PDDF v1.0 first. The reason is

  1. All the related code in other submodules have been checked-in
  2. PDDF v2.0 changes are on top of v1.0 code. It includes not only 2.0 platform API support but also some extra components support e.g. GPIO, BMC etc.

So, below comment needs to be modified.
"We do have plan to move PDDF generic plugins to 2.0 APIs. Tentatively, we will complete movement to 2.0 by November 2019."

We have PDDF v2.0 code changes ready and we are waiting for v1.0 changes to get in.

@FuzailBrcm
Copy link
Contributor Author

retest vsimage please

@FuzailBrcm
Copy link
Contributor Author

Retest this please

@FuzailBrcm
Copy link
Contributor Author

Retest vsimage please

1 similar comment
@FuzailBrcm
Copy link
Contributor Author

Retest vsimage please

@FuzailBrcm
Copy link
Contributor Author

Retest this please

@FuzailBrcm
Copy link
Contributor Author

retest vsimage please

@FuzailBrcm
Copy link
Contributor Author

Retest this please

@FuzailBrcm
Copy link
Contributor Author

retest this please

Copy link
Contributor

@jleveque jleveque left a comment

Choose a reason for hiding this comment

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

Please fix conflicts

@FuzailBrcm FuzailBrcm closed this Dec 28, 2020
mssonicbld added a commit that referenced this pull request Dec 4, 2024
…lly (#21012)

#### Why I did it
src/sonic-swss
```
* 42e9feaa - (HEAD -> 202405, origin/202405) [buffer] indicate shared pool by putting a very small shared buffer pool size (#3387) (61 minutes ago) [Junchao-Mellanox]
```
#### How I did it
#### How to verify it
#### Description for the changelog
mssonicbld added a commit that referenced this pull request Dec 10, 2024
…atically (#20573)

#### Why I did it
src/sonic-utilities
```
* 32e6de7f - (HEAD -> master, origin/master, origin/HEAD) Update DB migrator script to next branch 202505 (#3664) (3 days ago) [Kumaresh Perumal]
* 8d00aba7 - (origin/test_202411_20, origin/test_202411_100, origin/kperumal_202411, origin/202411) Fix save_file command in generate_dump (#3647) (4 days ago) [DavidZagury]
* 038aaa5f - Revert "Speed up route_check script" (#3653) (4 days ago) [xwjiang-ms]
* 032a0e03 - [config]Restrict YANG validation to Golden Config (#3656) (4 days ago) [jingwenxie]
* 47b79bea - Update gcu_field_operation_validators.conf.json with Arista-7260CX3-D108C10 (#3650) (4 days ago) [Dev Ojha]
* 845f46ef - Update PR checker pipeline to point to bookworm (#3651) (4 days ago) [bingwang-ms]
* 28e645f4 - [show_tech] Filter out a list of commands in generate_dump script which are not applicable to Supervisor (#3646) (5 days ago) [Marty Y. Lok]
* b767cb86 - [cisco|express-boot]: Add support for cisco express boot in sonic-utilities (#3056) (6 days ago) [jhli-cisco]
* f6b9912c - Fix show command column change issue (#3645) (8 days ago) [Feng-msft]
* 9cd6c8de - Debug dump utility dash objects update (#3387) (8 days ago) [Gagan Punathil Ellath]
* f705c456 - Validate interface name length in CLI (#3580) (8 days ago) [Stepan Blyshchak]
* 3d78cb16 - [Mellanox] Update the save_file command in generate_dump to handle folders (#3631) (13 days ago) [DavidZagury]
* 5086a43b - Fix counterpoll show for ENI Counters (#3618) (13 days ago) [Vivek]
* 70f81397 - Extra delay between running firmware and updating firmware version (#3610) (2 weeks ago) [Stephen Sun]
* 636f7f1d - Add port FEC BER show changes (#3607) (2 weeks ago) [vincentpcng]
* 5e60031b - Supports FRR-VRRP configuration (#2949) (3 weeks ago) [Philo]
* 9bd37dc6 - [ARM64][Warm-Reboot]: Get boot arguments from uboot environment (#3613) (3 weeks ago) [Parthiv Shah]
* 171b16c2 - platform/innovium renaming to platform/marvell-teralynx (#3474) (3 weeks ago) [krismarvell]
* 2cbfcc91 - [Mellanox] Add Mellanox-SN5600-C224O8 to gcu_field.json (#3612) (4 weeks ago) [noaOrMlnx]
* a8f7bde0 - PVST Feature commit (#3567) (4 weeks ago) [Divya Kumaran Chandralekha]
* 0e188076 - Refresh dump file modify timestamp before cleanup call (#3595) (4 weeks ago) [anamehra]
* 8f3c5ef5 - [warm-reboot] Add support to pass extra kernel cmdline parameters (#3192) (4 weeks ago) [Pavan Naregundi]
* 96774477 - Run yang validation in db migrator (#3102) (4 weeks ago) [ganglv]
* 9708f52b - [Mellanox] Update C256X1 SKU to be C256S1 in gcu_field.json (#3611) (4 weeks ago) [noaOrMlnx]
* 093ed4aa - [SPM] Add logic to disable the feature before stopping it and enabling it before starting (#3344) (4 weeks ago) [Vadym Hlushko]
* 7d013dff - Fix slash in path. (#3573) (5 weeks ago) [Xincun Li]
* 0af4386e - Consolidate the get running config way. (#3585) (5 weeks ago) [Xincun Li]
* 964b4895 - Fix for integer overflow of counter value if its too large (#3596) (5 weeks ago) [harjotsinghpawra]
* 3354d08a - [config] Bypass standard input for reload (#3597) (5 weeks ago) [jingwenxie]
* 7cbcfda5 - Speed up route_check script (#3544) (5 weeks ago) [Deepak Singhal]
* 329fc223 - Add support of the pensando-dpu platform to generate-dump utility. (#3557) (6 weeks ago) [BHUKYA SIDDHU]
* 0ae2ec1e - Add CLI for bmp configdb entity Enable/Disable (#3286) (6 weeks ago) [Feng-msft]
* 5b37ee6c - Vnet_route_check TCP socket for DB connection. (#3578) (6 weeks ago) [siqbal1986]
* d64a90a0 - Adds logic to get default disk and check disk type (#3399) (6 weeks ago) [Ashwin Srinivasan]
* b2b97340 - [Banner] Added CLI commands to configure Banner and display current configuration (#3021) (6 weeks ago) [Sviatoslav Boichuk]
* d0aa94a1 - Revert "Record and warn tables which not covered by YANG (#3583)" (#3588) (7 weeks ago) [jingwenxie]
* aeda86a1 - Record and warn tables which not covered by YANG (#3583) (7 weeks ago) [jingwenxie]
* dd34d7c4 - Revert "Skip default lanes dup check (#3489)" (#3572) (7 weeks ago) [Xincun Li]
* 6c8007fa - [Mellanox]Remove deprecated sdk sniffer cli and collect sdk dump in show techsupport (#3491) (7 weeks ago) [Yuanzhe]
* 89bb87ad - Add YANG validation for config reload if file is given (#3576) (7 weeks ago) [jingwenxie]
```
#### How I did it
#### How to verify it
#### Description for the changelog
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.

4 participants