RE: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) Extend device list supported by driver

[Vadim Pasternak <vadimp@mellanox.com>]

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Tuesday, January 14, 2020 4:19 PM
> To: Vadim Pasternak <vadimp@mellanox.com>
> Cc: robh+dt@kernel.org; vijaykhemka@fb.com; linux-hwmon@vger.kernel.org;
> devicetree@vger.kernel.org; Michael Shych <michaelsh@mellanox.com>; Ofer
> Levy <oferl@mellanox.com>
> 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@gmail.com> On Behalf Of Guenter Roeck
> >> Sent: Monday, January 13, 2020 10:56 PM
> >> To: Vadim Pasternak <vadimp@mellanox.com>
> >> Cc: robh+dt@kernel.org; vijaykhemka@fb.com;
> >> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org; Michael
> >> Shych <michaelsh@mellanox.com>; Ofer Levy <oferl@mellanox.com>
> >> 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@gmail.com> On Behalf Of Guenter Roeck
> >>>> Sent: Wednesday, January 08, 2020 6:48 PM
> >>>> To: Vadim Pasternak <vadimp@mellanox.com>
> >>>> Cc: robh+dt@kernel.org; vijaykhemka@fb.com;
> >>>> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org; Michael
> >>>> Shych <michaelsh@mellanox.com>
> >>>> 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