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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [LM Sensors]     [Linux Sound]     [ALSA Users]     [ALSA Devel]     [Linux Audio Users]     [Linux Media]     [Kernel]     [Gimp]     [Yosemite News]     [Linux Media]

  Powered by Linux