> -----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? Thanks, Vadim. > > Thanks, > Guenter
