On Mon, Jan 06, 2020 at 04:57:32PM +0000, Vadim Pasternak wrote: > > > > -----Original Message----- > > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > > Sent: Monday, January 06, 2020 4:53 PM > > To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>; robh+dt@xxxxxxxxxx; > > vijaykhemka@xxxxxx > > Cc: linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx > > Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) Extend > > device list supported by driver > > > > On 1/6/20 4:16 AM, Vadim Pasternak wrote: > > > > > > > > >> -----Original Message----- > > >> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > > >> Sent: Sunday, January 05, 2020 8:35 PM > > >> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>; robh+dt@xxxxxxxxxx; > > >> vijaykhemka@xxxxxx > > >> Cc: linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx > > >> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) > > >> Extend device list supported by driver > > >> > > >> On 1/5/20 8:44 AM, Vadim Pasternak wrote: > > >>> > > >>> > > >>>> -----Original Message----- > > >>>> From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck > > >>>> Sent: Sunday, January 05, 2020 6:04 PM > > >>>> To: Vadim Pasternak <vadimp@xxxxxxxxxxxx>; robh+dt@xxxxxxxxxx; > > >>>> vijaykhemka@xxxxxx > > >>>> Cc: linux-hwmon@xxxxxxxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx > > >>>> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) > > >>>> Extend device list supported by driver > > >>>> > > >>>> On 1/5/20 2:58 AM, Vadim Pasternak wrote: > > >>>>> Extends driver with support of the additional devices: > > >>>>> Texas Instruments Dual channel DCAP+ multiphase controllers: > > >>>>> TPS53688, SN1906016. > > >>>>> Infineon Multi-phase Digital VR Controller Sierra devices > > >>>>> XDPE12286C, XDPE12284C, XDPE12283C, XDPE12254C and XDPE12250C. > > >>>>> > > >>>>> Extend Kconfig with added devices. > > >>>>> > > >>>>> Signed-off-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx> > > >>>>> --- > > >>>>> drivers/hwmon/pmbus/Kconfig | 5 +++-- > > >>>>> drivers/hwmon/pmbus/tps53679.c | 14 ++++++++++++++ > > >>>>> 2 files changed, 17 insertions(+), 2 deletions(-) > > >>>>> > > >>>>> diff --git a/drivers/hwmon/pmbus/Kconfig > > >>>>> b/drivers/hwmon/pmbus/Kconfig index 59859979571d..9e3d197d5322 > > >>>>> 100644 > > >>>>> --- a/drivers/hwmon/pmbus/Kconfig > > >>>>> +++ b/drivers/hwmon/pmbus/Kconfig > > >>>>> @@ -200,10 +200,11 @@ config SENSORS_TPS40422 > > >>>>> be called tps40422. > > >>>>> > > >>>>> config SENSORS_TPS53679 > > >>>>> - tristate "TI TPS53679" > > >>>>> + tristate "TI TPS53679, TPS53688, SN1906016, Infineon XDPE122xxx > > >>>> family" > > >>>>> help > > >>>>> If you say yes here you get hardware monitoring support for TI > > >>>>> - TPS53679. > > >>>>> + TPS53679, PS53688, SN1906016 and Infineon XDPE12286C, > > >>>> XDPE12284C, > > >>>> > > >>>> TPS53688. For the others, for some I can't even determine if they > > >>>> exist in the first place (eg SN1906016, XPDE12250C) or how they > > >>>> would differ from other variants (eg XPDE12284C vs. XPDE12284A). > > >>>> And why would they all use the same bit map in the VOUT_MODE > > >>>> register, the same number of PMBus pages (phases), and the same > > >>>> attributes > > >> in each page ? > > >>> > > >>> Hi Guenter, > > >>> > > >>> Thank you for reply. > > >>> > > >>> On our new system we have device XPDE12284C equipped. > > >>> I tested this device. > > >>> > > >> Sounds good, but did you also make sure that all chips have the same > > >> number of pages (phases), the same set of commands as the TI chip, > > >> and support the same bit settings in VOUT_MODE ? It seems a bit > > >> unlikely that TI's register definitions would make it into an Infineon chip. > > >> > > >> Also, what about the SN1906016 ? I don't find that anywhere, except > > >> in one place where it is listed as MCU from TI. > > > > > > I'll drop SN1906016. > > > Datasheet has a title Dual channel DCAP+ multiphase controllers: > > > TPS53688, SN1906016. > > > But maybe it's some custom device (anyway I'll try to check it with TI). > > > > > > > Or maybe SN1906016 means something else. Unless we have explicit > > confirmation that the chip exists (or will exist) we should not add it to the list. > > > > >> > > >>> Infineon datasheet refers all these device as XDPE122xxC family and > > >>> it doesn't specify any differences in register map between these devices. > > >> > > >> That is a bit vague, especially when it includes devices which return > > >> zero results with Google searches. > > >> > > >> "A" vs. "C" may distinguish automotive vs. commercial; the "A" device > > >> is listed under automotive. If the command set is the same, I don't > > >> really want the "c" in the id. > > > > > > Got feedback from Infineon guys. > > > No need 'C' at the end, as you wrote. > > > All XDPE12250, XDPE12254, XDPE12283, XDPE12284, XDPE12286 are treated > > > in the same way: > > > same pages, same VOUT_MODE, VOUT_READ, etcetera. > > > > > > > And same as TI, including VOUT_MODE ? Also, did they confirm that the > > unpublished chips do or will actually exist ? > > Hi Gunther, > > According to the input I got from Infineon guys, these device are already > available for the customers, like XPDE12284, which is equipped on new > Mellanox 400Gx32 Ethernet system, on which we are working now. > > But I'll re-check if all these devices are available today to be on the safe > Side. > > Regarding VOUT modes: > TPS53679 uses modes - 0x01, 0x02, 0x04, 0x05, 0x07 > TPS53688 uses modes - 0x04, 0x07 > XDPE122xxx uses modes - 0x01, 0x02, 0x03 and additionally 0x10, which is > for 6.25mV VID table (AMD application). > The problem is that PMBus does not define VID mode values. If it did, we could add vrm version detection detection to the pmbus core. 0x01 for TPS53679 _may_ be the same as 0x01 for XDPE122xxx, or maybe not. There is no way to be sure without datasheets. > I didn't add support for mode 0x10 in the patch. > > The VID table for the AMD application is different from the > Intel VID tables. > > A value of 0x0 corresponds to the highest output voltage of 1.55V. > The voltage is reduced in 6.25mV steps down to the value 0xd8, > which corresponds to 0.2V. > > The formula for the calculation of the output voltage would be: > > case DON’T_NOW_HOW_TO_CALL_IT: Doesn't the datasheet have something to say ? > if (val >= 0x00 && val <= 0xd8) > rv = 1550 – (val *6.25); > > I doubled check it. > > Do you think it should added as well? > I am quite neutral on that. I am much more concerned with the assumption that the mode values have the same meaning for chips from different vendors. In this case, what do we do if TI starts shipping a chip in the TPS53xxx series which uses mode 0x10 for something else ? Overall I'd rather play safe and add a separate driver for the Infineon chips. Thanks, Guenter
