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

[Guenter Roeck <linux@roeck-us.net>]

On 1/6/20 10:06 PM, Vadim Pasternak wrote:
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>> Sent: Tuesday, January 07, 2020 3:29 AM
>> To: Vadim Pasternak <vadimp@mellanox.com>
>> Cc: robh+dt@kernel.org; vijaykhemka@fb.com; linux-hwmon@vger.kernel.org;
>> devicetree@vger.kernel.org
>> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) Extend
>> device list supported by driver
>>
>> On 1/6/20 2:29 PM, Vadim Pasternak wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>>>> Sent: Monday, January 06, 2020 11:01 PM
>>>> To: Vadim Pasternak <vadimp@mellanox.com>
>>>> Cc: robh+dt@kernel.org; vijaykhemka@fb.com;
>>>> linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org
>>>> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679)
>>>> Extend device list supported by driver
>>>>
>>>> On Mon, Jan 06, 2020 at 04:57:32PM +0000, Vadim Pasternak wrote:
>>>>>
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
>>>>>> Sent: Monday, January 06, 2020 4:53 PM
>>>>>> To: Vadim Pasternak <vadimp@mellanox.com>; robh+dt@kernel.org;
>>>>>> vijaykhemka@fb.com
>>>>>> Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org
>>>>>> 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@gmail.com> On Behalf Of Guenter
>>>>>>>> Roeck
>>>>>>>> Sent: Sunday, January 05, 2020 8:35 PM
>>>>>>>> To: Vadim Pasternak <vadimp@mellanox.com>; robh+dt@kernel.org;
>>>>>>>> vijaykhemka@fb.com
>>>>>>>> Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org
>>>>>>>> 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@gmail.com> On Behalf Of Guenter
>>>>>>>>>> Roeck
>>>>>>>>>> Sent: Sunday, January 05, 2020 6:04 PM
>>>>>>>>>> To: Vadim Pasternak <vadimp@mellanox.com>; robh+dt@kernel.org;
>>>>>>>>>> vijaykhemka@fb.com
>>>>>>>>>> Cc: linux-hwmon@vger.kernel.org; devicetree@vger.kernel.org
>>>>>>>>>> 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@mellanox.com>
>>>>>>>>>>> ---
>>>>>>>>>>>       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 ?
>>>
>>> It just specifies VID table format as
>>> 0 = 10mV VID table
>>> 1 = 5mv VID table
>>> 2 = 6.25mv VID table
>>> 3 = 10mV VID table (200mV offset)
>>> calculation as:
>>> Range: 0 = Off, 1 (250mV) to 255 (1520mV); vid_table=0 (10mV)
>>> Range: 0 = Off, 1 (500mV) to 255 (3040mV); vid_table=1 (5mV)
>>> Range: 0 = Off, 1 (200mV) to 255 (2740mV); vid_table=3 (10mV)
>>> Range: 0 = (1550mV) to 247 (6.25mV); 248~255 = Off; vid_table=2
>>> (6.25mV)
>>>
>>> And VOUT_MODE[4:0] as:
>>> 00001 = 5mV VID table (VR12)
>>> 00010 = 10mV VID table (VR12.5 or VR13)
>>> 00011 = 10mV VID table (IMVP9)
>>> 10000 = 6.25mV VID table (AMD application) others = illegal setting -
>>> PMBus write is acked, but no write occurs
>>>
>>>>
>>>>> 		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.
>>>
>>> I see.
>>>
>>> We actually waned to have ability of transparent replacement of TI and
>>> Infineon devices within the same type of system.
>>>
>>
>> That should not be a problem as long as you instantiate them differently.
>> After all, the relevant information _should_ be available in ACPI tables.
>> Otherwise you'd have to claim that a chip is, say, tps53688, while it is really an
>> Infineon chip. And that is never a good idea.
>>
>>> Maybe it's possible to have 0x01, 0x02, 0x03, 0x04, 0x05, 0x07 as a
>>> basic set and support for example 0x10 according to specific device id?
>>>
>>
>> Unfortunately not, because there is no standard defining those. TI might at
>> some point decide to sell a new chip where 0x03 means something completely
>> different. On top of that, I already know that at least some of the TI chips of this
>> series have more than two pages. Unfortunately, the information I have is vague
>> (no datasheet again). That is another reason for avoiding pollution of the tps
>> driver with non-TI chip support.
> 
> OK.
> So, I think to modify the patch as following:
> 
> Add sperate driver  xdpe122xx with support this Infineon family (after final
> checking with Infineon, which of the are available).
> It will support 0x01, 0x02, 0x03, 0x10.
> 
> Add tps53688 to tps53679 driver.
> 
> Add two new vrf versions imvp9, amd625mv (I only don't know what is the
> best naming for these new modes).
> 
> And these new modes will be handled as:
> 	case imvp9:
> 		if (val >= 0x01)
> 			rv = 200 + (val - 1) * 10;
> 		break;
> 	case amd625mv:
> 		if (val >= 0x0 && val <= 0xd8)
> 			rv = DIV_ROUND_CLOSEST(155000 - val * 625, 100);
> 		break;
> 
> What it be fine?
> 

Yes, sounds good.

Thanks,
Guenter

> If yes, I'll make v1 patch version.
> 
> Thanks,
> Vadim.
> 
>>
>> Thanks,
>> Guenter