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

FuzailBrcm · 2019-08-27T11:13:40Z

- 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)

…ugins

jleveque

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

device/accton/x86_64-accton_as7726_32x-r0/pddf/pd-plugin.json

…ass in PR#62

…4-spaces

FuzailBrcm · 2019-09-26T12:19:23Z

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 · 2019-09-27T04:27:18Z

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 · 2019-10-14T19:47:38Z

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 ?

srideepDell · 2019-10-14T19:54:21Z

device/common/pddf/plugins/eeprom.py

+        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)


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

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.

srideepDell · 2019-10-14T19:58:24Z

device/common/pddf/plugins/sfputil.py

+        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


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.

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

srideepDell · 2019-10-14T20:00:04Z

device/common/pddf/plugins/sfputil.py

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


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

srideepDell · 2019-10-14T20:01:38Z

device/common/pddf/plugins/sfputil.py

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


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.

srideepDell · 2019-10-14T20:03:12Z

device/common/pddf/plugins/sysstatutil.py

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


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

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

srideepDell · 2019-10-14T20:20:22Z

platform/pddf/i2c/modules/mux/pddf_mux_module.c

+ * 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


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

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

srideepDell · 2019-10-14T20:22:07Z

platform/pddf/i2c/modules/psu/driver/pddf_psu_driver.c

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


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

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

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.

srideepDell · 2019-10-14T20:24:02Z

platform/pddf/i2c/modules/sysstatus/pddf_sysstatus_module.c

+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);


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

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

srideepDell · 2019-10-14T20:33:10Z

platform/pddf/i2c/utils/pddf_util.py

+    return True
+
+kos = [
+'modprobe i2c-ismt',


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 ?

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 · 2019-10-19T04:19:21Z

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

lguohan · 2019-12-06T21:28:34Z

platform/pddf/i2c/modules/xcvr/pddf_xcvr_module.c

@@ -0,0 +1,291 @@
+/*
+ * Copyright 2019 Broadcom. All rights reserved.
+ * The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.


need gpl license

taken care in
@fk410167

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

lguohan · 2019-12-06T21:31:57Z

platform/pddf/i2c/modules/psu/pddf_psu_module.c

+
+	ret = sysfs_create_group(i2c_kobj, &pddf_psu_client_data_group);
+	if (ret)
+    {


the spacing is not consistent.

This is already taken care in commit: a654dc7.

lguohan · 2019-12-06T21:33:36Z

device/common/pddf/plugins/fanutil.py

+
+
+
+#if __name__== "__main__":


remove these testing code.

taken care of in ,
@fk410167

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

…per the latest review comments

FuzailBrcm

Made changes are per the review comments

lguohan · 2019-12-10T16:21:29Z

platform/pddf/i2c/modules/psu/driver/Makefile

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


remove commented code?

lguohan · 2019-12-10T16:21:41Z

platform/pddf/i2c/modules/psu/driver/Makefile

+obj-m := $(TARGET).o 
+
+$(TARGET)-objs := pddf_psu_api.o pddf_psu_driver.o 
+#GCOV_PROFILE := y


remove commented code?

lguohan · 2019-12-10T16:23:36Z

platform/pddf/i2c/modules/cpld/driver/pddf_cpld_driver.c

+    struct cpld_client_node *cpld_node = NULL;
+    int ret = -EPERM;
+
+    //hw_preaccess_func_cpld_mux_default((uint32_t)cpld_addr, NULL);


remove commented code.

lguohan · 2019-12-10T16:23:48Z

platform/pddf/i2c/modules/cpld/Makefile

+	#$(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 ;


remove commented code.

lguohan · 2019-12-10T16:25:07Z

platform/pddf/i2c/modules/client/Makefile

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


remove commented.

lguohan · 2019-12-10T16:35:56Z

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

lguohan · 2019-12-10T16:36:50Z

platform/pddf/i2c/modules/client/pddf_client_module.c

+            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);*/


remove commented code.

FuzailBrcm · 2019-12-11T06:51:14Z

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,

None of the BSP code or, HW devices related code is present outside of the 'platform' folder,
PDDF code is kept in a separate 'platform/pddf' folder and not under any specific chip vendor,
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 · 2020-01-07T18:16:38Z

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

All the related code in other submodules have been checked-in
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 · 2020-01-08T03:42:42Z

retest vsimage please

FuzailBrcm · 2020-01-08T11:59:30Z

Retest this please

FuzailBrcm · 2020-01-08T16:33:51Z

Retest vsimage please

FuzailBrcm · 2020-01-09T04:10:31Z

Retest vsimage please

FuzailBrcm · 2020-01-09T07:15:56Z

Retest this please

FuzailBrcm · 2020-01-09T18:04:33Z

retest vsimage please

FuzailBrcm · 2020-01-10T04:30:57Z

Retest this please

FuzailBrcm · 2020-01-12T16:01:53Z

retest this please

jleveque

Please fix conflicts

Platform Driver Developement Framework (PDDF): Generic drivers and Pl…

d3ad4d8

…ugins

jleveque suggested changes Sep 18, 2019

View reviewed changes

platform/pddf/i2c/modules/include/pddf_fan_defs.h Outdated Show resolved Hide resolved

device/accton/x86_64-accton_as7726_32x-r0/pddf/pd-plugin.json Outdated Show resolved Hide resolved

FuzailBrcm and others added 3 commits September 19, 2019 14:57

Merge branch 'master' into pddf_pr

25745f5

Changes in common pddf psu plugin based on the changes in psu_base cl…

a654dc7

…ass in PR#62

Changes incorporating feedback comments in PR#3387. Chaning tab into …

ed922f1

…4-spaces

srideepDell reviewed Oct 14, 2019

View reviewed changes

Changes in psu driver module as per the review comments

f0b4dc2

srideepDell previously approved these changes Dec 3, 2019

View reviewed changes

lguohan reviewed Dec 6, 2019

View reviewed changes

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

8e8cf2f

…per the latest review comments

FuzailBrcm commented Dec 7, 2019

View reviewed changes

lguohan reviewed Dec 10, 2019

View reviewed changes

Removing commented code as per the latest review comments

33ab9b7

Merge branch 'master' into pddf_pr

c8e2821

FuzailBrcm dismissed srideepDell’s stale review via c8e2821 January 7, 2020 18:11

FuzailBrcm mentioned this pull request Jun 11, 2020

Platform Driver Developement Framework (PDDF) #4756

Merged

jleveque suggested changes Jul 7, 2020

View reviewed changes

lguohan force-pushed the master branch from 3690c1a to 512eb6b Compare December 25, 2020 18:34

FuzailBrcm closed this Dec 28, 2020



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

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

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

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

Conversation

FuzailBrcm commented Aug 27, 2019

jleveque left a comment

Choose a reason for hiding this comment

FuzailBrcm commented Sep 26, 2019

FuzailBrcm commented Sep 27, 2019

srideepDell commented Oct 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FuzailBrcm commented Oct 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FuzailBrcm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lguohan commented Dec 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FuzailBrcm commented Dec 11, 2019

FuzailBrcm commented Jan 7, 2020

FuzailBrcm commented Jan 8, 2020

FuzailBrcm commented Jan 8, 2020

FuzailBrcm commented Jan 8, 2020

FuzailBrcm commented Jan 9, 2020

FuzailBrcm commented Jan 9, 2020

FuzailBrcm commented Jan 9, 2020

FuzailBrcm commented Jan 10, 2020

FuzailBrcm commented Jan 12, 2020

jleveque left a comment

Choose a reason for hiding this comment

srideepDell commented Oct 14, 2019 •

edited

Loading