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

Commit Message

Vadim Pasternak Jan. 5, 2020, 10:58 a.m. UTC

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 <[e-mail]>
---
 drivers/hwmon/pmbus/Kconfig    |  5 +++--
 drivers/hwmon/pmbus/tps53679.c | 14 ++++++++++++++
 2 files changed, 17 insertions(+), 2 deletions(-)

Comments

Guenter Roeck Jan. 5, 2020, 4:03 p.m. UTC | #1

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 <[e-mail]>
> ---
>   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 ?

Thanks,
Guenter

> +	  XDPE12283C, XDPE12254C, XDPE12250C devices.
>   
>   	  This driver can also be built as a module. If so, the module will
>   	  be called tps53679.
> diff --git a/drivers/hwmon/pmbus/tps53679.c b/drivers/hwmon/pmbus/tps53679.c
> index 7ce2fca4acde..f38eb116735b 100644
> --- a/drivers/hwmon/pmbus/tps53679.c
> +++ b/drivers/hwmon/pmbus/tps53679.c
> @@ -89,6 +89,13 @@ static int tps53679_probe(struct i2c_client *client,
>   
>   static const struct i2c_device_id tps53679_id[] = {
>   	{"tps53679", 0},
> +	{"tps53688", 0},
> +	{"sn1906016", 0},
> +	{"xdpe12283c", 0},
> +	{"xdpe12250c", 0},
> +	{"xdpe12254c", 0},
> +	{"xdpe12284c", 0},
> +	{"xdpe12286c", 0},

Alphabetic order, please.

>   	{}
>   };
>   
> @@ -96,6 +103,13 @@ MODULE_DEVICE_TABLE(i2c, tps53679_id);
>   
>   static const struct of_device_id __maybe_unused tps53679_of_match[] = {
>   	{.compatible = "ti,tps53679"},
> +	{.compatible = "ti,tps53688"},
> +	{.compatible = "ti,sn1906016"},
> +	{.compatible = "infineon,xdpe12283c"},
> +	{.compatible = "infineon,xdpe12250c"},
> +	{.compatible = "infineon,xdpe12254c"},
> +	{.compatible = "infineon,xdpe12284c"},
> +	{.compatible = "infineon,xdpe12286c"},
>   	{}
>   };
>   MODULE_DEVICE_TABLE(of, tps53679_of_match);
>

Vadim Pasternak Jan. 5, 2020, 4:44 p.m. UTC | #2

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Sunday, January 05, 2020 6:04 PM
> To: Vadim Pasternak <[e-mail]>; [e-mail];
> [e-mail]
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]>
> > ---
> >   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.

Infineon datasheet refers all these device as XDPE122xxC
family and it doesn't specify any differences in register map
between these devices.
Tomorrow we'll have guys from Infineon in our lab and I'll
verify if there is any difference.
If yes, I'll leave only XPDE12284C.

> 
> Thanks,
> Guenter
> 
> > +	  XDPE12283C, XDPE12254C, XDPE12250C devices.
> >
> >   	  This driver can also be built as a module. If so, the module will
> >   	  be called tps53679.
> > diff --git a/drivers/hwmon/pmbus/tps53679.c
> > b/drivers/hwmon/pmbus/tps53679.c index 7ce2fca4acde..f38eb116735b
> > 100644
> > --- a/drivers/hwmon/pmbus/tps53679.c
> > +++ b/drivers/hwmon/pmbus/tps53679.c
> > @@ -89,6 +89,13 @@ static int tps53679_probe(struct i2c_client
> > *client,
> >
> >   static const struct i2c_device_id tps53679_id[] = {
> >   	{"tps53679", 0},
> > +	{"tps53688", 0},
> > +	{"sn1906016", 0},
> > +	{"xdpe12283c", 0},
> > +	{"xdpe12250c", 0},
> > +	{"xdpe12254c", 0},
> > +	{"xdpe12284c", 0},
> > +	{"xdpe12286c", 0},
> 
> Alphabetic order, please.
> 
> >   	{}
> >   };
> >
> > @@ -96,6 +103,13 @@ MODULE_DEVICE_TABLE(i2c, tps53679_id);
> >
> >   static const struct of_device_id __maybe_unused tps53679_of_match[] = {
> >   	{.compatible = "ti,tps53679"},
> > +	{.compatible = "ti,tps53688"},
> > +	{.compatible = "ti,sn1906016"},
> > +	{.compatible = "infineon,xdpe12283c"},
> > +	{.compatible = "infineon,xdpe12250c"},
> > +	{.compatible = "infineon,xdpe12254c"},
> > +	{.compatible = "infineon,xdpe12284c"},
> > +	{.compatible = "infineon,xdpe12286c"},
> >   	{}
> >   };
> >   MODULE_DEVICE_TABLE(of, tps53679_of_match);
> >

Guenter Roeck Jan. 5, 2020, 6:34 p.m. UTC | #3

On 1/5/20 8:44 AM, Vadim Pasternak wrote:
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
>> Sent: Sunday, January 05, 2020 6:04 PM
>> To: Vadim Pasternak <[e-mail]>; [e-mail];
>> [e-mail]
>> Cc: [e-mail]; [e-mail]
>> 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 <[e-mail]>
>>> ---
>>>    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.

> 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.

> Tomorrow we'll have guys from Infineon in our lab and I'll
> verify if there is any difference.

Tell them that it isn't really helpful to keep their datasheets
under wrap. Unfortunately, TI started doing the same, which isn't
helpful either.

Thanks,
Guenter

> If yes, I'll leave only XPDE12284C.
> 
>>
>> Thanks,
>> Guenter
>>
>>> +	  XDPE12283C, XDPE12254C, XDPE12250C devices.
>>>
>>>    	  This driver can also be built as a module. If so, the module will
>>>    	  be called tps53679.
>>> diff --git a/drivers/hwmon/pmbus/tps53679.c
>>> b/drivers/hwmon/pmbus/tps53679.c index 7ce2fca4acde..f38eb116735b
>>> 100644
>>> --- a/drivers/hwmon/pmbus/tps53679.c
>>> +++ b/drivers/hwmon/pmbus/tps53679.c
>>> @@ -89,6 +89,13 @@ static int tps53679_probe(struct i2c_client
>>> *client,
>>>
>>>    static const struct i2c_device_id tps53679_id[] = {
>>>    	{"tps53679", 0},
>>> +	{"tps53688", 0},
>>> +	{"sn1906016", 0},
>>> +	{"xdpe12283c", 0},
>>> +	{"xdpe12250c", 0},
>>> +	{"xdpe12254c", 0},
>>> +	{"xdpe12284c", 0},
>>> +	{"xdpe12286c", 0},
>>
>> Alphabetic order, please.
>>
>>>    	{}
>>>    };
>>>
>>> @@ -96,6 +103,13 @@ MODULE_DEVICE_TABLE(i2c, tps53679_id);
>>>
>>>    static const struct of_device_id __maybe_unused tps53679_of_match[] = {
>>>    	{.compatible = "ti,tps53679"},
>>> +	{.compatible = "ti,tps53688"},
>>> +	{.compatible = "ti,sn1906016"},
>>> +	{.compatible = "infineon,xdpe12283c"},
>>> +	{.compatible = "infineon,xdpe12250c"},
>>> +	{.compatible = "infineon,xdpe12254c"},
>>> +	{.compatible = "infineon,xdpe12284c"},
>>> +	{.compatible = "infineon,xdpe12286c"},
>>>    	{}
>>>    };
>>>    MODULE_DEVICE_TABLE(of, tps53679_of_match);
>>>
>

Vadim Pasternak Jan. 6, 2020, 12:16 p.m. UTC | #4

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Sunday, January 05, 2020 8:35 PM
> To: Vadim Pasternak <[e-mail]>; [e-mail];
> [e-mail]
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]> On Behalf Of Guenter Roeck
> >> Sent: Sunday, January 05, 2020 6:04 PM
> >> To: Vadim Pasternak <[e-mail]>; [e-mail];
> >> [e-mail]
> >> Cc: [e-mail]; [e-mail]
> >> 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 <[e-mail]>
> >>> ---
> >>>    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).

> 
> > 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.

> 
> > Tomorrow we'll have guys from Infineon in our lab and I'll verify if
> > there is any difference.
> 
> Tell them that it isn't really helpful to keep their datasheets under wrap.
> Unfortunately, TI started doing the same, which isn't helpful either.

Told them about datasheets availability - got :)

> 
> Thanks,
> Guenter
> 
> > If yes, I'll leave only XPDE12284C.
> >
> >>
> >> Thanks,
> >> Guenter
> >>
> >>> +	  XDPE12283C, XDPE12254C, XDPE12250C devices.
> >>>
> >>>    	  This driver can also be built as a module. If so, the module will
> >>>    	  be called tps53679.
> >>> diff --git a/drivers/hwmon/pmbus/tps53679.c
> >>> b/drivers/hwmon/pmbus/tps53679.c index 7ce2fca4acde..f38eb116735b
> >>> 100644
> >>> --- a/drivers/hwmon/pmbus/tps53679.c
> >>> +++ b/drivers/hwmon/pmbus/tps53679.c
> >>> @@ -89,6 +89,13 @@ static int tps53679_probe(struct i2c_client
> >>> *client,
> >>>
> >>>    static const struct i2c_device_id tps53679_id[] = {
> >>>    	{"tps53679", 0},
> >>> +	{"tps53688", 0},
> >>> +	{"sn1906016", 0},
> >>> +	{"xdpe12283c", 0},
> >>> +	{"xdpe12250c", 0},
> >>> +	{"xdpe12254c", 0},
> >>> +	{"xdpe12284c", 0},
> >>> +	{"xdpe12286c", 0},
> >>
> >> Alphabetic order, please.
> >>
> >>>    	{}
> >>>    };
> >>>
> >>> @@ -96,6 +103,13 @@ MODULE_DEVICE_TABLE(i2c, tps53679_id);
> >>>
> >>>    static const struct of_device_id __maybe_unused tps53679_of_match[] =
> {
> >>>    	{.compatible = "ti,tps53679"},
> >>> +	{.compatible = "ti,tps53688"},
> >>> +	{.compatible = "ti,sn1906016"},
> >>> +	{.compatible = "infineon,xdpe12283c"},
> >>> +	{.compatible = "infineon,xdpe12250c"},
> >>> +	{.compatible = "infineon,xdpe12254c"},
> >>> +	{.compatible = "infineon,xdpe12284c"},
> >>> +	{.compatible = "infineon,xdpe12286c"},
> >>>    	{}
> >>>    };
> >>>    MODULE_DEVICE_TABLE(of, tps53679_of_match);
> >>>
> >

Guenter Roeck Jan. 6, 2020, 2:52 p.m. UTC | #5

On 1/6/20 4:16 AM, Vadim Pasternak wrote:
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
>> Sent: Sunday, January 05, 2020 8:35 PM
>> To: Vadim Pasternak <[e-mail]>; [e-mail];
>> [e-mail]
>> Cc: [e-mail]; [e-mail]
>> 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 <[e-mail]> On Behalf Of Guenter Roeck
>>>> Sent: Sunday, January 05, 2020 6:04 PM
>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
>>>> [e-mail]
>>>> Cc: [e-mail]; [e-mail]
>>>> 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 <[e-mail]>
>>>>> ---
>>>>>     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 ?

Sorry, to be persistent, but give my thanks to Infineon.

>>
>>> Tomorrow we'll have guys from Infineon in our lab and I'll verify if
>>> there is any difference.
>>
>> Tell them that it isn't really helpful to keep their datasheets under wrap.
>> Unfortunately, TI started doing the same, which isn't helpful either.
> 
> Told them about datasheets availability - got :)
> 

Surprise.

Thanks,
Guenter

Vadim Pasternak Jan. 6, 2020, 4:57 p.m. UTC | #6

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Monday, January 06, 2020 4:53 PM
> To: Vadim Pasternak <[e-mail]>; [e-mail];
> [e-mail]
> Cc: [e-mail]; [e-mail]
> 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 <[e-mail]> On Behalf Of Guenter Roeck
> >> Sent: Sunday, January 05, 2020 8:35 PM
> >> To: Vadim Pasternak <[e-mail]>; [e-mail];
> >> [e-mail]
> >> Cc: [e-mail]; [e-mail]
> >> 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 <[e-mail]> On Behalf Of Guenter Roeck
> >>>> Sent: Sunday, January 05, 2020 6:04 PM
> >>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
> >>>> [e-mail]
> >>>> Cc: [e-mail]; [e-mail]
> >>>> 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 <[e-mail]>
> >>>>> ---
> >>>>>     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).

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:
		if (val >= 0x00 && val <= 0xd8)
              			rv = 1550 – (val *6.25);

I doubled check it.

Do you think it should added as well?

Thanks,
Vadim.

> 
> Sorry, to be persistent, but give my thanks to Infineon.
> 
> >>
> >>> Tomorrow we'll have guys from Infineon in our lab and I'll verify if
> >>> there is any difference.
> >>
> >> Tell them that it isn't really helpful to keep their datasheets under wrap.
> >> Unfortunately, TI started doing the same, which isn't helpful either.
> >
> > Told them about datasheets availability - got :)
> >
> 
> Surprise.
> 
> Thanks,
> Guenter

Guenter Roeck Jan. 6, 2020, 9:01 p.m. UTC | #7

On Mon, Jan 06, 2020 at 04:57:32PM +0000, Vadim Pasternak wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> > Sent: Monday, January 06, 2020 4:53 PM
> > To: Vadim Pasternak <[e-mail]>; [e-mail];
> > [e-mail]
> > Cc: [e-mail]; [e-mail]
> > 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 <[e-mail]> On Behalf Of Guenter Roeck
> > >> Sent: Sunday, January 05, 2020 8:35 PM
> > >> To: Vadim Pasternak <[e-mail]>; [e-mail];
> > >> [e-mail]
> > >> Cc: [e-mail]; [e-mail]
> > >> 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 <[e-mail]> On Behalf Of Guenter Roeck
> > >>>> Sent: Sunday, January 05, 2020 6:04 PM
> > >>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
> > >>>> [e-mail]
> > >>>> Cc: [e-mail]; [e-mail]
> > >>>> 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 <[e-mail]>
> > >>>>> ---
> > >>>>>     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

Vadim Pasternak Jan. 6, 2020, 10:29 p.m. UTC | #8

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Monday, January 06, 2020 11:01 PM
> To: Vadim Pasternak <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> 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 <[e-mail]> On Behalf Of Guenter Roeck
> > > Sent: Monday, January 06, 2020 4:53 PM
> > > To: Vadim Pasternak <[e-mail]>; [e-mail];
> > > [e-mail]
> > > Cc: [e-mail]; [e-mail]
> > > 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 <[e-mail]> On Behalf Of Guenter
> > > >> Roeck
> > > >> Sent: Sunday, January 05, 2020 8:35 PM
> > > >> To: Vadim Pasternak <[e-mail]>; [e-mail];
> > > >> [e-mail]
> > > >> Cc: [e-mail]; [e-mail]
> > > >> 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 <[e-mail]> On Behalf Of Guenter
> > > >>>> Roeck
> > > >>>> Sent: Sunday, January 05, 2020 6:04 PM
> > > >>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
> > > >>>> [e-mail]
> > > >>>> Cc: [e-mail]; [e-mail]
> > > >>>> 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 <[e-mail]>
> > > >>>>> ---
> > > >>>>>     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.

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?  


> 
> Thanks,
> Guenter

Guenter Roeck Jan. 7, 2020, 1:28 a.m. UTC | #9

On 1/6/20 2:29 PM, Vadim Pasternak wrote:
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
>> Sent: Monday, January 06, 2020 11:01 PM
>> To: Vadim Pasternak <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]
>> 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 <[e-mail]> On Behalf Of Guenter Roeck
>>>> Sent: Monday, January 06, 2020 4:53 PM
>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
>>>> [e-mail]
>>>> Cc: [e-mail]; [e-mail]
>>>> 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 <[e-mail]> On Behalf Of Guenter
>>>>>> Roeck
>>>>>> Sent: Sunday, January 05, 2020 8:35 PM
>>>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
>>>>>> [e-mail]
>>>>>> Cc: [e-mail]; [e-mail]
>>>>>> 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 <[e-mail]> On Behalf Of Guenter
>>>>>>>> Roeck
>>>>>>>> Sent: Sunday, January 05, 2020 6:04 PM
>>>>>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
>>>>>>>> [e-mail]
>>>>>>>> Cc: [e-mail]; [e-mail]
>>>>>>>> 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 <[e-mail]>
>>>>>>>>> ---
>>>>>>>>>      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.

Thanks,
Guenter

Vadim Pasternak Jan. 7, 2020, 6:06 a.m. UTC | #10

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Tuesday, January 07, 2020 3:29 AM
> To: Vadim Pasternak <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> 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 <[e-mail]> On Behalf Of Guenter Roeck
> >> Sent: Monday, January 06, 2020 11:01 PM
> >> To: Vadim Pasternak <[e-mail]>
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> 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 <[e-mail]> On Behalf Of Guenter Roeck
> >>>> Sent: Monday, January 06, 2020 4:53 PM
> >>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
> >>>> [e-mail]
> >>>> Cc: [e-mail]; [e-mail]
> >>>> 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 <[e-mail]> On Behalf Of Guenter
> >>>>>> Roeck
> >>>>>> Sent: Sunday, January 05, 2020 8:35 PM
> >>>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
> >>>>>> [e-mail]
> >>>>>> Cc: [e-mail]; [e-mail]
> >>>>>> 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 <[e-mail]> On Behalf Of Guenter
> >>>>>>>> Roeck
> >>>>>>>> Sent: Sunday, January 05, 2020 6:04 PM
> >>>>>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
> >>>>>>>> [e-mail]
> >>>>>>>> Cc: [e-mail]; [e-mail]
> >>>>>>>> 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 <[e-mail]>
> >>>>>>>>> ---
> >>>>>>>>>      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?

If yes, I'll make v1 patch version.

Thanks,
Vadim.

> 
> Thanks,
> Guenter

Guenter Roeck Jan. 7, 2020, 12:14 p.m. UTC | #11

On 1/6/20 10:06 PM, Vadim Pasternak wrote:
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
>> Sent: Tuesday, January 07, 2020 3:29 AM
>> To: Vadim Pasternak <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]
>> 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 <[e-mail]> On Behalf Of Guenter Roeck
>>>> Sent: Monday, January 06, 2020 11:01 PM
>>>> To: Vadim Pasternak <[e-mail]>
>>>> Cc: [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]
>>>> 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 <[e-mail]> On Behalf Of Guenter Roeck
>>>>>> Sent: Monday, January 06, 2020 4:53 PM
>>>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
>>>>>> [e-mail]
>>>>>> Cc: [e-mail]; [e-mail]
>>>>>> 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 <[e-mail]> On Behalf Of Guenter
>>>>>>>> Roeck
>>>>>>>> Sent: Sunday, January 05, 2020 8:35 PM
>>>>>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
>>>>>>>> [e-mail]
>>>>>>>> Cc: [e-mail]; [e-mail]
>>>>>>>> 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 <[e-mail]> On Behalf Of Guenter
>>>>>>>>>> Roeck
>>>>>>>>>> Sent: Sunday, January 05, 2020 6:04 PM
>>>>>>>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
>>>>>>>>>> [e-mail]
>>>>>>>>>> Cc: [e-mail]; [e-mail]
>>>>>>>>>> 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 <[e-mail]>
>>>>>>>>>>> ---
>>>>>>>>>>>       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

Vadim Pasternak Jan. 8, 2020, 2:10 p.m. UTC | #12

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Tuesday, January 07, 2020 2:14 PM
> To: Vadim Pasternak <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]
> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) Extend
> device list supported by driver
> 
> On 1/6/20 10:06 PM, Vadim Pasternak wrote:
> >
> >
> >> -----Original Message-----
> >> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> >> Sent: Tuesday, January 07, 2020 3:29 AM
> >> To: Vadim Pasternak <[e-mail]>
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]
> >> 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 <[e-mail]> On Behalf Of Guenter Roeck
> >>>> Sent: Monday, January 06, 2020 11:01 PM
> >>>> To: Vadim Pasternak <[e-mail]>
> >>>> Cc: [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]
> >>>> 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 <[e-mail]> On Behalf Of Guenter
> >>>>>> Roeck
> >>>>>> Sent: Monday, January 06, 2020 4:53 PM
> >>>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
> >>>>>> [e-mail]
> >>>>>> Cc: [e-mail]; [e-mail]
> >>>>>> 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 <[e-mail]> On Behalf Of Guenter
> >>>>>>>> Roeck
> >>>>>>>> Sent: Sunday, January 05, 2020 8:35 PM
> >>>>>>>> To: Vadim Pasternak <[e-mail]>; [e-mail];
> >>>>>>>> [e-mail]
> >>>>>>>> Cc: [e-mail]; [e-mail]
> >>>>>>>> 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 <[e-mail]> On Behalf Of Guenter
> >>>>>>>>>> Roeck
> >>>>>>>>>> Sent: Sunday, January 05, 2020 6:04 PM
> >>>>>>>>>> To: Vadim Pasternak <[e-mail]>;
> >>>>>>>>>> [e-mail]; [e-mail]
> >>>>>>>>>> Cc: [e-mail]; [e-mail]
> >>>>>>>>>> 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 <[e-mail]>
> >>>>>>>>>>> ---
> >>>>>>>>>>>       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.

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.

What do you think about such approach?

Thanks,
Vadim.

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

Guenter Roeck Jan. 8, 2020, 4:47 p.m. UTC | #13

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 ?

Thanks,
Guenter

Vadim Pasternak Jan. 13, 2020, 12:25 p.m. UTC | #14

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Wednesday, January 08, 2020 6:48 PM
> To: Vadim Pasternak <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Michael Shych <[e-mail]>
> 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?

Thanks,
Vadim.

> 
> Thanks,
> Guenter

Guenter Roeck Jan. 13, 2020, 8:56 p.m. UTC | #15

Hi Vadim,

On Mon, Jan 13, 2020 at 12:25:44PM +0000, Vadim Pasternak wrote:
> 
> 
> > -----Original Message-----
> > From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> > Sent: Wednesday, January 08, 2020 6:48 PM
> > To: Vadim Pasternak <[e-mail]>
> > Cc: [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; Michael Shych <[e-mail]>
> > 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.

Thanks,
Guenter

Vadim Pasternak Jan. 14, 2020, 6:54 a.m. UTC | #16

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Monday, January 13, 2020 10:56 PM
> To: Vadim Pasternak <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Michael Shych <[e-mail]>; Ofer
> Levy <[e-mail]>
> 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 <[e-mail]> On Behalf Of Guenter Roeck
> > > Sent: Wednesday, January 08, 2020 6:48 PM
> > > To: Vadim Pasternak <[e-mail]>
> > > Cc: [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; Michael
> > > Shych <[e-mail]>
> > > 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

Guenter Roeck Jan. 14, 2020, 2:19 p.m. UTC | #17

On 1/13/20 10:54 PM, Vadim Pasternak wrote:
> 
> 
>> -----Original Message-----
>> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
>> Sent: Monday, January 13, 2020 10:56 PM
>> To: Vadim Pasternak <[e-mail]>
>> Cc: [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; Michael Shych <[e-mail]>; Ofer
>> Levy <[e-mail]>
>> 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 <[e-mail]> On Behalf Of Guenter Roeck
>>>> Sent: Wednesday, January 08, 2020 6:48 PM
>>>> To: Vadim Pasternak <[e-mail]>
>>>> Cc: [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; Michael
>>>> Shych <[e-mail]>
>>>> 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?
> 

Correct. We'll need a bindings document, though, to make it official.

The binding would look something like "vrm-mode = <number>", where
<number> is well defined (possibly in an include file). There are many
bindings include files which you can use as examples.
We'll need to get DT maintainer approval for the exact binding name;
maybe it has to be "pmbus,vrm-mode" or something like that.

Thanks,
Guenter

Vadim Pasternak Jan. 16, 2020, 7:54 p.m. UTC | #18

> -----Original Message-----
> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> Sent: Tuesday, January 14, 2020 4:19 PM
> To: Vadim Pasternak <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; Michael Shych <[e-mail]>; Ofer
> Levy <[e-mail]>
> Subject: Re: [RFC PATCH hwmon-next v1 5/5] hwmon: (pmbus/tps53679) Extend
> device list supported by driver
> 
> On 1/13/20 10:54 PM, Vadim Pasternak wrote:
> >
> >
> >> -----Original Message-----
> >> From: Guenter Roeck <[e-mail]> On Behalf Of Guenter Roeck
> >> Sent: Monday, January 13, 2020 10:56 PM
> >> To: Vadim Pasternak <[e-mail]>
> >> Cc: [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; Michael
> >> Shych <[e-mail]>; Ofer Levy <[e-mail]>
> >> 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 <[e-mail]> On Behalf Of Guenter Roeck
> >>>> Sent: Wednesday, January 08, 2020 6:48 PM
> >>>> To: Vadim Pasternak <[e-mail]>
> >>>> Cc: [e-mail]; [e-mail];
> >>>> [e-mail]; [e-mail]; Michael
> >>>> Shych <[e-mail]>
> >>>> 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?
> >
> 
> Correct. We'll need a bindings document, though, to make it official.
> 
> The binding would look something like "vrm-mode = <number>", where
> <number> is well defined (possibly in an include file). There are many bindings
> include files which you can use as examples.
> We'll need to get DT maintainer approval for the exact binding name; maybe it
> has to be "pmbus,vrm-mode" or something like that.

Great.
I suppose we'll try to make it for v5.7.

Thank you very much for all your inputs.
 
Thanks,
Vadim.

> 
> Thanks,
> Guenter

Patch

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,
+	  XDPE12283C, XDPE12254C, XDPE12250C devices.
 
 	  This driver can also be built as a module. If so, the module will
 	  be called tps53679.
diff --git a/drivers/hwmon/pmbus/tps53679.c b/drivers/hwmon/pmbus/tps53679.c
index 7ce2fca4acde..f38eb116735b 100644
--- a/drivers/hwmon/pmbus/tps53679.c
+++ b/drivers/hwmon/pmbus/tps53679.c
@@ -89,6 +89,13 @@  static int tps53679_probe(struct i2c_client *client,
 
 static const struct i2c_device_id tps53679_id[] = {
 	{"tps53679", 0},
+	{"tps53688", 0},
+	{"sn1906016", 0},
+	{"xdpe12283c", 0},
+	{"xdpe12250c", 0},
+	{"xdpe12254c", 0},
+	{"xdpe12284c", 0},
+	{"xdpe12286c", 0},
 	{}
 };
 
@@ -96,6 +103,13 @@  MODULE_DEVICE_TABLE(i2c, tps53679_id);
 
 static const struct of_device_id __maybe_unused tps53679_of_match[] = {
 	{.compatible = "ti,tps53679"},
+	{.compatible = "ti,tps53688"},
+	{.compatible = "ti,sn1906016"},
+	{.compatible = "infineon,xdpe12283c"},
+	{.compatible = "infineon,xdpe12250c"},
+	{.compatible = "infineon,xdpe12254c"},
+	{.compatible = "infineon,xdpe12284c"},
+	{.compatible = "infineon,xdpe12286c"},
 	{}
 };
 MODULE_DEVICE_TABLE(of, tps53679_of_match);