> -----Original Message----- > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > Sent: Tuesday, January 14, 2020 4:19 PM > To: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > Cc: robh+dt@xxxxxxxxxx; vijaykhemka@xxxxxx; linux-hwmon@xxxxxxxxxxxxxxx; > devicetree@xxxxxxxxxxxxxxx; Michael Shych <michaelsh@xxxxxxxxxxxx>; Ofer > Levy <oferl@xxxxxxxxxxxx> > Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) Extend > device list supported by driver > > On 1/13/20 10:54 PM, Vadim Pasternak wrote: > > > > > >> -----Original Message----- > >> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > >> Sent: Monday, January 13, 2020 10:56 PM > >> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > >> Cc: robh+dt@xxxxxxxxxx; vijaykhemka@xxxxxx; > >> linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Michael > >> Shych <michaelsh@xxxxxxxxxxxx>; Ofer Levy <oferl@xxxxxxxxxxxx> > >> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) > >> Extend device list supported by driver > >> > >> Hi Vadim, > >> > >> On Mon, Jan 13, 2020 at 12:25:44PM +0000, Vadim Pasternak wrote: > >>> > >>> > >>>> -----Original Message----- > >>>> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > >>>> Sent: Wednesday, January 08, 2020 6:48 PM > >>>> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > >>>> Cc: robh+dt@xxxxxxxxxx; vijaykhemka@xxxxxx; > >>>> linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; Michael > >>>> Shych <michaelsh@xxxxxxxxxxxx> > >>>> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) > >>>> Extend device list supported by driver > >>>> > >>>> On Wed, Jan 08, 2020 at 02:10:50PM +0000, Vadim Pasternak wrote: > >>>>> > >>>>> Hi Guenter, > >>>>> > >>>>> We are looking for possibility to provide some kind of flexible > >>>>> driver to handle different devices from different vendors, but > >>>>> which have common nature, like support of two pages for telemetry > >>>>> with same set of functions and same formats. (Actually driver > >>>>> could be flexible regarding the > >>>> number of pages). > >>>>> > >>>>> While the difference only in VID codes mapping. > >>>>> > >>>>> The motivation for that is to give free hand to HW design to > >>>>> choose which particular device to use on the same system type. > >>>>> There are two main reasons for such requirement: > >>>>> - Quality of device (we already had a serios problems with ucd9224 and > >>>>> tps53679, caused system meltdown). In such case the intention is to > move > >>>>> to fallback devices in the next batches. > >>>>> - Device availability in stock. Sometimes vendors can't supply in time the > >>>>> necessary quantity. > >>>>> > >>>>> Currently there are the devices from three vendor: TI tps536xx, > >>>>> Infineon > >>>>> xdpe122 and MPS mp2975. > >>>>> All have different mapping of VID codes. > >>>>> > >>>>> It could be also few additional devices, which are supposed to be > >>>>> used as fallback devices. > >>>>> > >>>>> We have several very big customers ordering now huge quantity of > >>>>> our systems, which are very conservative regarding image upgrade. > >>>>> Means we can provide now support for tps536xx, xdpe122xx and > >>>>> mp2975 but in case new devices are coming soon, we will be able to > >>>>> support it in kernel for their image after year or even more. > >>>>> > >>>>> We can provide ACPI pmbus driver, which will read VID mapping from > >>>>> ACPI DSDT table. This mapping table will contain the names of > >>>>> available modes and specific vendor code for this mode. Like: > >>>>> PMBVR11=1 > >>>>> PMBVR12=2 > >>>>> PMBVR13=5 > >>>>> PMBIMVP9=10 > >>>>> And driver will set info->vrm_version[i] according to this mapping. > >>>>> > >>>> > >>>> The DSDT would have to provide all properties, not just the VID modes. > >>>> At the very least that would have to be the number of pages, data > >>>> formats, vrm version, and the supported attributes per page. It > >>>> should be possible to also add m/b/R information for each of the > >>>> sensor classes, but I guess the actual implementation could be > >>>> postponed > >> until it is needed. > >>>> > >>>> This could all be done through the existing generic driver > >>>> (pmbus.c); it would not really require a new driver. ACPI/DSDT > >>>> could provide firmware properties, and pmbus.c could read those > >>>> using device_property API functions (eg > >>>> device_property_read_u32(dev, "vrm- > >> mode")). Would that work for you ? > >>> > >>> Hi Guenter, > >>> > >>> We thought that it's possible to provide just modified DSDT with the > >>> specific configuration as an external table, which is loaded during system > boot. > >>> > >>> It should not be integrated into BIOS image. > >>> We want to avoid releasing of new BIOS or new each time system > >>> configuration is changed. > >>> New BIOS is released only when new CPU type should be supported. > >>> Also BIOS overwriting is not an option, sine we have to support secured > BIOS. > >>> > >>> It should not be located in initrd. > >>> The new system batch is released with BIOS, SMBIOS and with > >>> customer's OS or with install environment like ONIE. > >>> Means no kernel changes. > >>> Also not all our customers use initrd. > >>> > >>> So, it seems there is no place, when we can locate modified DSDT and > >>> load it dynamically. > >>> But we should think more about possible methods for doing it. > >>> > >>> Today all the static devices are instantiated from the user space. > >>> It's supposed to work for us while we have to support some > >>> pre-defined set of devices, for which we can detect the specific > >>> configuration through DMI tables (SKU in particular). > >>> But it'll not work for some new coming devices. > >>> > >>> We have a possibility to provide VID mapping info through CPLD device. > >>> But in this case we'll have to implement Mellanox specific PMBus > >>> driver aware of CPLD register map. > >>> Not sure if such approach is accepted? > >>> > >> > >> How about a platform driver which loads the parameters into device > >> properties from whatever location and instantiates the pmbus driver ? > >> The PMBus driver would then read the device attributes and > >> instantiate itself accordingly. > >> > >> If the other PMBus attributes can be auto-detected, it might actually > >> be sufficient to have a per-page vrm-mode property and otherwise use > >> the auto- detect mechanism of pmbus.c. > > > > Hi Guenter, > > > > I didn't think about such possibility. > > It sounds promising. > > > > So, we can add our platform driver to "drivers/platform/mellanox", > > which can: > > - fetch "vrm" mapping per each relevant device. > > - for each allocate device node, create properties table with vrm mode > > mapping like "vrm-mode-vr11", "vrm-mode-vr12", "vrm-mode-vr13", > > "vrm-mode-imvp9", "vrm-mode-amd625mv", coded accordingly to > > "enum vrm_version". > > - attach this node to "i2c_board_info" and instantiate it with > > i2c_new_device() for "pmbus" type. > > > > And i"pmbus" will get these properties like > > device_property_read_8(dev, "vrm-mode-vr11")) etcetera. > > > > Did I get your suggestion correctly? > > > > Correct. We'll need a bindings document, though, to make it official. > > The binding would look something like "vrm-mode = <number>", where > <number> is well defined (possibly in an include file). There are many bindings > include files which you can use as examples. > We'll need to get DT maintainer approval for the exact binding name; maybe it > has to be "pmbus,vrm-mode" or something like that. Great. I suppose we'll try to make it for v5.7. Thank you very much for all your inputs. Thanks, Vadim. > > Thanks, > Guenter
