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: 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?

Thanks,
Vadim.

> 
> Thanks,
> Guenter