[PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Naresh Solanki]

From: Patrick Rudolph <patrick.rudolph@9elements.com>

The TDA38640 chip has different output control mechanisms depending on
its mode of operation. When the chip is in SVID mode, only
hardware-based output control is supported via ENABLE pin. However, when
it operates in PMBus mode, software control works perfectly.

To enable software control as a workaround in SVID mode, add the DT
property 'infineon,en-svid-control'. This property will enable the
workaround, which utilizes ENABLE pin polarity flipping for output when
the chip is in SVID mode.

Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
---
 .../hwmon/pmbus/infineon,tda38640.yaml        | 51 +++++++++++++++++++
 .../devicetree/bindings/trivial-devices.yaml  |  2 -
 2 files changed, 51 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
new file mode 100644
index 000000000000..c5924ddf1b47
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
@@ -0,0 +1,51 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/hwmon/pmbus/infineon,tda38640.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Infineon TDA38640 Synchronous Buck Regulator with SVID and I2C
+
+maintainers:
+  - Naresh Solanki <naresh.solanki@9elements.com>
+
+description: |
+  The Infineon TDA38640 is a 40A Single-voltage Synchronous Buck
+  Regulator with SVID and I2C designed for Industrial use.
+
+  Datasheet: https://www.infineon.com/dgdl/Infineon-TDA38640-0000-DataSheet-v02_04-EN.pdf?fileId=8ac78c8c80027ecd018042f2337f00c9
+
+properties:
+  compatible:
+    enum:
+      - infineon,tda38640
+
+  reg:
+    maxItems: 1
+
+  infineon,en-svid-control:
+    description: |
+      When enabled, it allows the chip to utilize workaround for
+      software control of output when operating in SVID mode where
+      hardware-based output control is the default behavior.
+    type: boolean
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    i2c {
+        #address-cells = <1>;
+        #size-cells = <0>;
+
+        tda38640@40 {
+            compatible = "infineon,tda38640";
+            reg = <0x40>;
+        };
+    };
+
diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
index 40bc475ee7e1..86c7d34f63bf 100644
--- a/Documentation/devicetree/bindings/trivial-devices.yaml
+++ b/Documentation/devicetree/bindings/trivial-devices.yaml
@@ -151,8 +151,6 @@ properties:
           - infineon,slb9645tt
             # Infineon SLB9673 I2C TPM 2.0
           - infineon,slb9673
-            # Infineon TDA38640 Voltage Regulator
-          - infineon,tda38640
             # Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
           - infineon,tlv493d-a1b6
             # Infineon Multi-phase Digital VR Controller xdpe11280

base-commit: cb7022b8976e3c4d12cea2e7bb820a2944e2fd7b
-- 
2.41.0

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 3738 bytes --]

On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> 
> The TDA38640 chip has different output control mechanisms depending on
> its mode of operation. When the chip is in SVID mode, only
> hardware-based output control is supported via ENABLE pin. However, when
> it operates in PMBus mode, software control works perfectly.
> 
> To enable software control as a workaround in SVID mode, add the DT
> property 'infineon,en-svid-control'. This property will enable the
> workaround, which utilizes ENABLE pin polarity flipping for output when
> the chip is in SVID mode.

Why do you need a custom property for this? How come it is not possible
to determine what bus you are on?

Thanks,
Conor.

> 
> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
> ---
>  .../hwmon/pmbus/infineon,tda38640.yaml        | 51 +++++++++++++++++++
>  .../devicetree/bindings/trivial-devices.yaml  |  2 -
>  2 files changed, 51 insertions(+), 2 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> new file mode 100644
> index 000000000000..c5924ddf1b47
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
> @@ -0,0 +1,51 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/hwmon/pmbus/infineon,tda38640.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Infineon TDA38640 Synchronous Buck Regulator with SVID and I2C
> +
> +maintainers:
> +  - Naresh Solanki <naresh.solanki@9elements.com>
> +
> +description: |
> +  The Infineon TDA38640 is a 40A Single-voltage Synchronous Buck
> +  Regulator with SVID and I2C designed for Industrial use.
> +
> +  Datasheet: https://www.infineon.com/dgdl/Infineon-TDA38640-0000-DataSheet-v02_04-EN.pdf?fileId=8ac78c8c80027ecd018042f2337f00c9
> +
> +properties:
> +  compatible:
> +    enum:
> +      - infineon,tda38640
> +
> +  reg:
> +    maxItems: 1
> +
> +  infineon,en-svid-control:
> +    description: |
> +      When enabled, it allows the chip to utilize workaround for
> +      software control of output when operating in SVID mode where
> +      hardware-based output control is the default behavior.
> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        tda38640@40 {
> +            compatible = "infineon,tda38640";
> +            reg = <0x40>;
> +        };
> +    };
> +
> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
> index 40bc475ee7e1..86c7d34f63bf 100644
> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
> @@ -151,8 +151,6 @@ properties:
>            - infineon,slb9645tt
>              # Infineon SLB9673 I2C TPM 2.0
>            - infineon,slb9673
> -            # Infineon TDA38640 Voltage Regulator
> -          - infineon,tda38640
>              # Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
>            - infineon,tlv493d-a1b6
>              # Infineon Multi-phase Digital VR Controller xdpe11280
> 
> base-commit: cb7022b8976e3c4d12cea2e7bb820a2944e2fd7b
> -- 
> 2.41.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Guenter Roeck]

On 8/8/23 04:46, Conor Dooley wrote:
> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>
>> The TDA38640 chip has different output control mechanisms depending on
>> its mode of operation. When the chip is in SVID mode, only
>> hardware-based output control is supported via ENABLE pin. However, when
>> it operates in PMBus mode, software control works perfectly.
>>
>> To enable software control as a workaround in SVID mode, add the DT
>> property 'infineon,en-svid-control'. This property will enable the
>> workaround, which utilizes ENABLE pin polarity flipping for output when
>> the chip is in SVID mode.
> 
> Why do you need a custom property for this? How come it is not possible
> to determine what bus you are on?
> 

That is not the point. Yes, it can be detected if the control method is
PMBus or SVID. However, in SVID mode, SVID is supposed to control the
output, not PMBUs. This is bypassed by controlling the polarity of the
(physical) output enable signal. We do _not_ want this enabled automatically
in SVID mode. Its side effects on random boards using this chip are unknown.
Thus, this needs a property which specifically enables this functionality
for users who _really_ need to use it and (hopefully) know what they are
doing.

Guenter


> Thanks,
> Conor.
> 
>>
>> Signed-off-by: Patrick Rudolph <patrick.rudolph@9elements.com>
>> Signed-off-by: Naresh Solanki <Naresh.Solanki@9elements.com>
>> ---
>>   .../hwmon/pmbus/infineon,tda38640.yaml        | 51 +++++++++++++++++++
>>   .../devicetree/bindings/trivial-devices.yaml  |  2 -
>>   2 files changed, 51 insertions(+), 2 deletions(-)
>>   create mode 100644 Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
>> new file mode 100644
>> index 000000000000..c5924ddf1b47
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/hwmon/pmbus/infineon,tda38640.yaml
>> @@ -0,0 +1,51 @@
>> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +
>> +$id: http://devicetree.org/schemas/hwmon/pmbus/infineon,tda38640.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Infineon TDA38640 Synchronous Buck Regulator with SVID and I2C
>> +
>> +maintainers:
>> +  - Naresh Solanki <naresh.solanki@9elements.com>
>> +
>> +description: |
>> +  The Infineon TDA38640 is a 40A Single-voltage Synchronous Buck
>> +  Regulator with SVID and I2C designed for Industrial use.
>> +
>> +  Datasheet: https://www.infineon.com/dgdl/Infineon-TDA38640-0000-DataSheet-v02_04-EN.pdf?fileId=8ac78c8c80027ecd018042f2337f00c9
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - infineon,tda38640
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  infineon,en-svid-control:
>> +    description: |
>> +      When enabled, it allows the chip to utilize workaround for
>> +      software control of output when operating in SVID mode where
>> +      hardware-based output control is the default behavior.
>> +    type: boolean
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    i2c {
>> +        #address-cells = <1>;
>> +        #size-cells = <0>;
>> +
>> +        tda38640@40 {
>> +            compatible = "infineon,tda38640";
>> +            reg = <0x40>;
>> +        };
>> +    };
>> +
>> diff --git a/Documentation/devicetree/bindings/trivial-devices.yaml b/Documentation/devicetree/bindings/trivial-devices.yaml
>> index 40bc475ee7e1..86c7d34f63bf 100644
>> --- a/Documentation/devicetree/bindings/trivial-devices.yaml
>> +++ b/Documentation/devicetree/bindings/trivial-devices.yaml
>> @@ -151,8 +151,6 @@ properties:
>>             - infineon,slb9645tt
>>               # Infineon SLB9673 I2C TPM 2.0
>>             - infineon,slb9673
>> -            # Infineon TDA38640 Voltage Regulator
>> -          - infineon,tda38640
>>               # Infineon TLV493D-A1B6 I2C 3D Magnetic Sensor
>>             - infineon,tlv493d-a1b6
>>               # Infineon Multi-phase Digital VR Controller xdpe11280
>>
>> base-commit: cb7022b8976e3c4d12cea2e7bb820a2944e2fd7b
>> -- 
>> 2.41.0
>>

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1869 bytes --]

On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
> On 8/8/23 04:46, Conor Dooley wrote:
> > On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > 
> > > The TDA38640 chip has different output control mechanisms depending on
> > > its mode of operation. When the chip is in SVID mode, only
> > > hardware-based output control is supported via ENABLE pin. However, when
> > > it operates in PMBus mode, software control works perfectly.
> > > 
> > > To enable software control as a workaround in SVID mode, add the DT
> > > property 'infineon,en-svid-control'. This property will enable the
> > > workaround, which utilizes ENABLE pin polarity flipping for output when
> > > the chip is in SVID mode.
> > 
> > Why do you need a custom property for this? How come it is not possible
> > to determine what bus you are on?
> > 
> 
> That is not the point. Yes, it can be detected if the control method is
> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
> output, not PMBUs. This is bypassed by controlling the polarity of the
> (physical) output enable signal. We do _not_ want this enabled automatically
> in SVID mode. Its side effects on random boards using this chip are unknown.
> Thus, this needs a property which specifically enables this functionality
> for users who _really_ need to use it and (hopefully) know what they are
> doing.

Hmm, reading this it makes a lot more sense why this is a property - I
guess I just struggled to understand the commit message here,
particularly what the benefit of using the workaround is. I'm still
having difficulty parsing the commit & property text though - its
unclear to me when you would need to use it - so I will stay out
of the way & let Rob or Krzysztof handle things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Naresh Solanki]

Hi,

On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
>
> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
> > On 8/8/23 04:46, Conor Dooley wrote:
> > > On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> > > > From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > >
> > > > The TDA38640 chip has different output control mechanisms depending on
> > > > its mode of operation. When the chip is in SVID mode, only
> > > > hardware-based output control is supported via ENABLE pin. However, when
> > > > it operates in PMBus mode, software control works perfectly.
> > > >
> > > > To enable software control as a workaround in SVID mode, add the DT
> > > > property 'infineon,en-svid-control'. This property will enable the
> > > > workaround, which utilizes ENABLE pin polarity flipping for output when
> > > > the chip is in SVID mode.
> > >
> > > Why do you need a custom property for this? How come it is not possible
> > > to determine what bus you are on?
> > >
> >
> > That is not the point. Yes, it can be detected if the control method is
> > PMBus or SVID. However, in SVID mode, SVID is supposed to control the
> > output, not PMBUs. This is bypassed by controlling the polarity of the
> > (physical) output enable signal. We do _not_ want this enabled automatically
> > in SVID mode. Its side effects on random boards using this chip are unknown.
> > Thus, this needs a property which specifically enables this functionality
> > for users who _really_ need to use it and (hopefully) know what they are
> > doing.
>
> Hmm, reading this it makes a lot more sense why this is a property - I
> guess I just struggled to understand the commit message here,
> particularly what the benefit of using the workaround is. I'm still
> having difficulty parsing the commit & property text though - its
> unclear to me when you would need to use it - so I will stay out
> of the way & let Rob or Krzysztof handle things.

To provide context, my system employs a unique power sequence
strategy utilizing a BMC (Baseboard Management Controller),
rendering the reliance on the ENABLE pin unnecessary.
In this configuration, the ENABLE pin is grounded in the hardware.
While most regulators facilitate PMBus Operation for output control,
the TDA38640 chip, when in SVID mode, is constrained by the
ENABLE pin to align with Intel specifications.
My communication with Infineon confirmed that the recommended
approach is to invert the Enable Pin for my use case.

Since this is not typically the use case for most setup & hence DT property
is must for enabling the special case.

For further insight into my setup's power sequence strategy, you can
refer to the following link: https://github.com/9elements/pwrseqd

I trust this clarifies any questions or uncertainties you may have had.

Best Regards,
Naresh

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Krzysztof Kozlowski]

On 11/08/2023 18:00, Naresh Solanki wrote:
> Hi,
> 
> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
>>
>> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
>>> On 8/8/23 04:46, Conor Dooley wrote:
>>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
>>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>>>
>>>>> The TDA38640 chip has different output control mechanisms depending on
>>>>> its mode of operation. When the chip is in SVID mode, only
>>>>> hardware-based output control is supported via ENABLE pin. However, when
>>>>> it operates in PMBus mode, software control works perfectly.
>>>>>
>>>>> To enable software control as a workaround in SVID mode, add the DT
>>>>> property 'infineon,en-svid-control'. This property will enable the
>>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
>>>>> the chip is in SVID mode.
>>>>
>>>> Why do you need a custom property for this? How come it is not possible
>>>> to determine what bus you are on?
>>>>
>>>
>>> That is not the point. Yes, it can be detected if the control method is
>>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
>>> output, not PMBUs. This is bypassed by controlling the polarity of the
>>> (physical) output enable signal. We do _not_ want this enabled automatically
>>> in SVID mode. Its side effects on random boards using this chip are unknown.
>>> Thus, this needs a property which specifically enables this functionality
>>> for users who _really_ need to use it and (hopefully) know what they are
>>> doing.
>>
>> Hmm, reading this it makes a lot more sense why this is a property - I
>> guess I just struggled to understand the commit message here,
>> particularly what the benefit of using the workaround is. I'm still
>> having difficulty parsing the commit & property text though - its
>> unclear to me when you would need to use it - so I will stay out
>> of the way & let Rob or Krzysztof handle things.
> 
> To provide context, my system employs a unique power sequence
> strategy utilizing a BMC (Baseboard Management Controller),
> rendering the reliance on the ENABLE pin unnecessary.
> In this configuration, the ENABLE pin is grounded in the hardware.
> While most regulators facilitate PMBus Operation for output control,
> the TDA38640 chip, when in SVID mode, is constrained by the
> ENABLE pin to align with Intel specifications.
> My communication with Infineon confirmed that the recommended
> approach is to invert the Enable Pin for my use case.
> 
> Since this is not typically the use case for most setup & hence DT property
> is must for enabling the special case.
> 
> For further insight into my setup's power sequence strategy, you can
> refer to the following link: https://github.com/9elements/pwrseqd
> 

This justifies to me the property, but still you described desired
driver behavior, not the hardware characteristic. Don't describe what
you want to control, but describe the entire system.

Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Naresh Solanki]

Hi Krzysztof,

On Tue, 15 Aug 2023 at 01:02, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 11/08/2023 18:00, Naresh Solanki wrote:
> > Hi,
> >
> > On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
> >>
> >> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
> >>> On 8/8/23 04:46, Conor Dooley wrote:
> >>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> >>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >>>>>
> >>>>> The TDA38640 chip has different output control mechanisms depending on
> >>>>> its mode of operation. When the chip is in SVID mode, only
> >>>>> hardware-based output control is supported via ENABLE pin. However, when
> >>>>> it operates in PMBus mode, software control works perfectly.
> >>>>>
> >>>>> To enable software control as a workaround in SVID mode, add the DT
> >>>>> property 'infineon,en-svid-control'. This property will enable the
> >>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
> >>>>> the chip is in SVID mode.
> >>>>
> >>>> Why do you need a custom property for this? How come it is not possible
> >>>> to determine what bus you are on?
> >>>>
> >>>
> >>> That is not the point. Yes, it can be detected if the control method is
> >>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
> >>> output, not PMBUs. This is bypassed by controlling the polarity of the
> >>> (physical) output enable signal. We do _not_ want this enabled automatically
> >>> in SVID mode. Its side effects on random boards using this chip are unknown.
> >>> Thus, this needs a property which specifically enables this functionality
> >>> for users who _really_ need to use it and (hopefully) know what they are
> >>> doing.
> >>
> >> Hmm, reading this it makes a lot more sense why this is a property - I
> >> guess I just struggled to understand the commit message here,
> >> particularly what the benefit of using the workaround is. I'm still
> >> having difficulty parsing the commit & property text though - its
> >> unclear to me when you would need to use it - so I will stay out
> >> of the way & let Rob or Krzysztof handle things.
> >
> > To provide context, my system employs a unique power sequence
> > strategy utilizing a BMC (Baseboard Management Controller),
> > rendering the reliance on the ENABLE pin unnecessary.
> > In this configuration, the ENABLE pin is grounded in the hardware.
> > While most regulators facilitate PMBus Operation for output control,
> > the TDA38640 chip, when in SVID mode, is constrained by the
> > ENABLE pin to align with Intel specifications.
> > My communication with Infineon confirmed that the recommended
> > approach is to invert the Enable Pin for my use case.
> >
> > Since this is not typically the use case for most setup & hence DT property
> > is must for enabling the special case.
> >
> > For further insight into my setup's power sequence strategy, you can
> > refer to the following link: https://github.com/9elements/pwrseqd
> >
>
> This justifies to me the property, but still you described desired
> driver behavior, not the hardware characteristic. Don't describe what
> you want to control, but describe the entire system.
I guess by entire system you mean how the regulators(including
TDA38640) connected & operated in our setup ?

Regards,
Naresh
>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Krzysztof Kozlowski]

On 16/08/2023 10:51, Naresh Solanki wrote:
> Hi Krzysztof,
> 
> On Tue, 15 Aug 2023 at 01:02, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
>>
>> On 11/08/2023 18:00, Naresh Solanki wrote:
>>> Hi,
>>>
>>> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
>>>>
>>>> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
>>>>> On 8/8/23 04:46, Conor Dooley wrote:
>>>>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
>>>>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
>>>>>>>
>>>>>>> The TDA38640 chip has different output control mechanisms depending on
>>>>>>> its mode of operation. When the chip is in SVID mode, only
>>>>>>> hardware-based output control is supported via ENABLE pin. However, when
>>>>>>> it operates in PMBus mode, software control works perfectly.
>>>>>>>
>>>>>>> To enable software control as a workaround in SVID mode, add the DT
>>>>>>> property 'infineon,en-svid-control'. This property will enable the
>>>>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
>>>>>>> the chip is in SVID mode.
>>>>>>
>>>>>> Why do you need a custom property for this? How come it is not possible
>>>>>> to determine what bus you are on?
>>>>>>
>>>>>
>>>>> That is not the point. Yes, it can be detected if the control method is
>>>>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
>>>>> output, not PMBUs. This is bypassed by controlling the polarity of the
>>>>> (physical) output enable signal. We do _not_ want this enabled automatically
>>>>> in SVID mode. Its side effects on random boards using this chip are unknown.
>>>>> Thus, this needs a property which specifically enables this functionality
>>>>> for users who _really_ need to use it and (hopefully) know what they are
>>>>> doing.
>>>>
>>>> Hmm, reading this it makes a lot more sense why this is a property - I
>>>> guess I just struggled to understand the commit message here,
>>>> particularly what the benefit of using the workaround is. I'm still
>>>> having difficulty parsing the commit & property text though - its
>>>> unclear to me when you would need to use it - so I will stay out
>>>> of the way & let Rob or Krzysztof handle things.
>>>
>>> To provide context, my system employs a unique power sequence
>>> strategy utilizing a BMC (Baseboard Management Controller),
>>> rendering the reliance on the ENABLE pin unnecessary.
>>> In this configuration, the ENABLE pin is grounded in the hardware.
>>> While most regulators facilitate PMBus Operation for output control,
>>> the TDA38640 chip, when in SVID mode, is constrained by the
>>> ENABLE pin to align with Intel specifications.
>>> My communication with Infineon confirmed that the recommended
>>> approach is to invert the Enable Pin for my use case.
>>>
>>> Since this is not typically the use case for most setup & hence DT property
>>> is must for enabling the special case.
>>>
>>> For further insight into my setup's power sequence strategy, you can
>>> refer to the following link: https://github.com/9elements/pwrseqd
>>>
>>
>> This justifies to me the property, but still you described desired
>> driver behavior, not the hardware characteristic. Don't describe what
>> you want to control, but describe the entire system.
> I guess by entire system you mean how the regulators(including
> TDA38640) connected & operated in our setup ?

I mean, property name and description should say what is the
characteristic of the hardware/firmware/entire system.


Best regards,
Krzysztof

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Naresh Solanki]

Hi

On Fri, 18 Aug 2023 at 14:53, Krzysztof Kozlowski
<krzysztof.kozlowski@linaro.org> wrote:
>
> On 16/08/2023 10:51, Naresh Solanki wrote:
> > Hi Krzysztof,
> >
> > On Tue, 15 Aug 2023 at 01:02, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> >>
> >> On 11/08/2023 18:00, Naresh Solanki wrote:
> >>> Hi,
> >>>
> >>> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
> >>>>
> >>>> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
> >>>>> On 8/8/23 04:46, Conor Dooley wrote:
> >>>>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> >>>>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> >>>>>>>
> >>>>>>> The TDA38640 chip has different output control mechanisms depending on
> >>>>>>> its mode of operation. When the chip is in SVID mode, only
> >>>>>>> hardware-based output control is supported via ENABLE pin. However, when
> >>>>>>> it operates in PMBus mode, software control works perfectly.
> >>>>>>>
> >>>>>>> To enable software control as a workaround in SVID mode, add the DT
> >>>>>>> property 'infineon,en-svid-control'. This property will enable the
> >>>>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
> >>>>>>> the chip is in SVID mode.
> >>>>>>
> >>>>>> Why do you need a custom property for this? How come it is not possible
> >>>>>> to determine what bus you are on?
> >>>>>>
> >>>>>
> >>>>> That is not the point. Yes, it can be detected if the control method is
> >>>>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
> >>>>> output, not PMBUs. This is bypassed by controlling the polarity of the
> >>>>> (physical) output enable signal. We do _not_ want this enabled automatically
> >>>>> in SVID mode. Its side effects on random boards using this chip are unknown.
> >>>>> Thus, this needs a property which specifically enables this functionality
> >>>>> for users who _really_ need to use it and (hopefully) know what they are
> >>>>> doing.
> >>>>
> >>>> Hmm, reading this it makes a lot more sense why this is a property - I
> >>>> guess I just struggled to understand the commit message here,
> >>>> particularly what the benefit of using the workaround is. I'm still
> >>>> having difficulty parsing the commit & property text though - its
> >>>> unclear to me when you would need to use it - so I will stay out
> >>>> of the way & let Rob or Krzysztof handle things.
> >>>
> >>> To provide context, my system employs a unique power sequence
> >>> strategy utilizing a BMC (Baseboard Management Controller),
> >>> rendering the reliance on the ENABLE pin unnecessary.
> >>> In this configuration, the ENABLE pin is grounded in the hardware.
> >>> While most regulators facilitate PMBus Operation for output control,
> >>> the TDA38640 chip, when in SVID mode, is constrained by the
> >>> ENABLE pin to align with Intel specifications.
> >>> My communication with Infineon confirmed that the recommended
> >>> approach is to invert the Enable Pin for my use case.
> >>>
> >>> Since this is not typically the use case for most setup & hence DT property
> >>> is must for enabling the special case.
> >>>
> >>> For further insight into my setup's power sequence strategy, you can
> >>> refer to the following link: https://github.com/9elements/pwrseqd
> >>>
> >>
> >> This justifies to me the property, but still you described desired
> >> driver behavior, not the hardware characteristic. Don't describe what
> >> you want to control, but describe the entire system.
> > I guess by entire system you mean how the regulators(including
> > TDA38640) connected & operated in our setup ?
>
> I mean, property name and description should say what is the
> characteristic of the hardware/firmware/entire system.
Based on your feedback, will update to below:
infineon,fixed-level-en-pin:
    description: |
      Indicate the ENABLE pin is set at fixed level or left
      unconnected(has internal pull-up).
    type: boolean
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Guenter Roeck]

On Tue, Aug 22, 2023 at 02:32:31PM +0530, Naresh Solanki wrote:
> Hi
> 
> On Fri, 18 Aug 2023 at 14:53, Krzysztof Kozlowski
> <krzysztof.kozlowski@linaro.org> wrote:
> >
> > On 16/08/2023 10:51, Naresh Solanki wrote:
> > > Hi Krzysztof,
> > >
> > > On Tue, 15 Aug 2023 at 01:02, Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > >>
> > >> On 11/08/2023 18:00, Naresh Solanki wrote:
> > >>> Hi,
> > >>>
> > >>> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
> > >>>>
> > >>>> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
> > >>>>> On 8/8/23 04:46, Conor Dooley wrote:
> > >>>>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> > >>>>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > >>>>>>>
> > >>>>>>> The TDA38640 chip has different output control mechanisms depending on
> > >>>>>>> its mode of operation. When the chip is in SVID mode, only
> > >>>>>>> hardware-based output control is supported via ENABLE pin. However, when
> > >>>>>>> it operates in PMBus mode, software control works perfectly.
> > >>>>>>>
> > >>>>>>> To enable software control as a workaround in SVID mode, add the DT
> > >>>>>>> property 'infineon,en-svid-control'. This property will enable the
> > >>>>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
> > >>>>>>> the chip is in SVID mode.
> > >>>>>>
> > >>>>>> Why do you need a custom property for this? How come it is not possible
> > >>>>>> to determine what bus you are on?
> > >>>>>>
> > >>>>>
> > >>>>> That is not the point. Yes, it can be detected if the control method is
> > >>>>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
> > >>>>> output, not PMBUs. This is bypassed by controlling the polarity of the
> > >>>>> (physical) output enable signal. We do _not_ want this enabled automatically
> > >>>>> in SVID mode. Its side effects on random boards using this chip are unknown.
> > >>>>> Thus, this needs a property which specifically enables this functionality
> > >>>>> for users who _really_ need to use it and (hopefully) know what they are
> > >>>>> doing.
> > >>>>
> > >>>> Hmm, reading this it makes a lot more sense why this is a property - I
> > >>>> guess I just struggled to understand the commit message here,
> > >>>> particularly what the benefit of using the workaround is. I'm still
> > >>>> having difficulty parsing the commit & property text though - its
> > >>>> unclear to me when you would need to use it - so I will stay out
> > >>>> of the way & let Rob or Krzysztof handle things.
> > >>>
> > >>> To provide context, my system employs a unique power sequence
> > >>> strategy utilizing a BMC (Baseboard Management Controller),
> > >>> rendering the reliance on the ENABLE pin unnecessary.
> > >>> In this configuration, the ENABLE pin is grounded in the hardware.
> > >>> While most regulators facilitate PMBus Operation for output control,
> > >>> the TDA38640 chip, when in SVID mode, is constrained by the
> > >>> ENABLE pin to align with Intel specifications.
> > >>> My communication with Infineon confirmed that the recommended
> > >>> approach is to invert the Enable Pin for my use case.
> > >>>
> > >>> Since this is not typically the use case for most setup & hence DT property
> > >>> is must for enabling the special case.
> > >>>
> > >>> For further insight into my setup's power sequence strategy, you can
> > >>> refer to the following link: https://github.com/9elements/pwrseqd
> > >>>
> > >>
> > >> This justifies to me the property, but still you described desired
> > >> driver behavior, not the hardware characteristic. Don't describe what
> > >> you want to control, but describe the entire system.
> > > I guess by entire system you mean how the regulators(including
> > > TDA38640) connected & operated in our setup ?
> >
> > I mean, property name and description should say what is the
> > characteristic of the hardware/firmware/entire system.
> Based on your feedback, will update to below:
> infineon,fixed-level-en-pin:
>     description: |
>       Indicate the ENABLE pin is set at fixed level or left
>       unconnected(has internal pull-up).
>     type: boolean

Messy, because while it reflects physical connectivity, it doesn't reflect
its use in the system at all. The pin may be at fixed level or left
unconnected, but the system vendor doesn't want to give users the
means to control output power (which would be the normal situation).

But then, if that is the only way to get a property accepted, so be it.

Guenter

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Naresh Solanki]

Hi,

On Tue, 22 Aug 2023 at 18:47, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Aug 22, 2023 at 02:32:31PM +0530, Naresh Solanki wrote:
> > Hi
> >
> > On Fri, 18 Aug 2023 at 14:53, Krzysztof Kozlowski
> > <krzysztof.kozlowski@linaro.org> wrote:
> > >
> > > On 16/08/2023 10:51, Naresh Solanki wrote:
> > > > Hi Krzysztof,
> > > >
> > > > On Tue, 15 Aug 2023 at 01:02, Krzysztof Kozlowski
> > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > >>
> > > >> On 11/08/2023 18:00, Naresh Solanki wrote:
> > > >>> Hi,
> > > >>>
> > > >>> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
> > > >>>>
> > > >>>> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
> > > >>>>> On 8/8/23 04:46, Conor Dooley wrote:
> > > >>>>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> > > >>>>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > >>>>>>>
> > > >>>>>>> The TDA38640 chip has different output control mechanisms depending on
> > > >>>>>>> its mode of operation. When the chip is in SVID mode, only
> > > >>>>>>> hardware-based output control is supported via ENABLE pin. However, when
> > > >>>>>>> it operates in PMBus mode, software control works perfectly.
> > > >>>>>>>
> > > >>>>>>> To enable software control as a workaround in SVID mode, add the DT
> > > >>>>>>> property 'infineon,en-svid-control'. This property will enable the
> > > >>>>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
> > > >>>>>>> the chip is in SVID mode.
> > > >>>>>>
> > > >>>>>> Why do you need a custom property for this? How come it is not possible
> > > >>>>>> to determine what bus you are on?
> > > >>>>>>
> > > >>>>>
> > > >>>>> That is not the point. Yes, it can be detected if the control method is
> > > >>>>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
> > > >>>>> output, not PMBUs. This is bypassed by controlling the polarity of the
> > > >>>>> (physical) output enable signal. We do _not_ want this enabled automatically
> > > >>>>> in SVID mode. Its side effects on random boards using this chip are unknown.
> > > >>>>> Thus, this needs a property which specifically enables this functionality
> > > >>>>> for users who _really_ need to use it and (hopefully) know what they are
> > > >>>>> doing.
> > > >>>>
> > > >>>> Hmm, reading this it makes a lot more sense why this is a property - I
> > > >>>> guess I just struggled to understand the commit message here,
> > > >>>> particularly what the benefit of using the workaround is. I'm still
> > > >>>> having difficulty parsing the commit & property text though - its
> > > >>>> unclear to me when you would need to use it - so I will stay out
> > > >>>> of the way & let Rob or Krzysztof handle things.
> > > >>>
> > > >>> To provide context, my system employs a unique power sequence
> > > >>> strategy utilizing a BMC (Baseboard Management Controller),
> > > >>> rendering the reliance on the ENABLE pin unnecessary.
> > > >>> In this configuration, the ENABLE pin is grounded in the hardware.
> > > >>> While most regulators facilitate PMBus Operation for output control,
> > > >>> the TDA38640 chip, when in SVID mode, is constrained by the
> > > >>> ENABLE pin to align with Intel specifications.
> > > >>> My communication with Infineon confirmed that the recommended
> > > >>> approach is to invert the Enable Pin for my use case.
> > > >>>
> > > >>> Since this is not typically the use case for most setup & hence DT property
> > > >>> is must for enabling the special case.
> > > >>>
> > > >>> For further insight into my setup's power sequence strategy, you can
> > > >>> refer to the following link: https://github.com/9elements/pwrseqd
> > > >>>
> > > >>
> > > >> This justifies to me the property, but still you described desired
> > > >> driver behavior, not the hardware characteristic. Don't describe what
> > > >> you want to control, but describe the entire system.
> > > > I guess by entire system you mean how the regulators(including
> > > > TDA38640) connected & operated in our setup ?
> > >
> > > I mean, property name and description should say what is the
> > > characteristic of the hardware/firmware/entire system.
> > Based on your feedback, will update to below:
> > infineon,fixed-level-en-pin:
> >     description: |
> >       Indicate the ENABLE pin is set at fixed level or left
> >       unconnected(has internal pull-up).
> >     type: boolean
>
> Messy, because while it reflects physical connectivity, it doesn't reflect
> its use in the system at all. The pin may be at fixed level or left
> unconnected, but the system vendor doesn't want to give users the
> means to control output power (which would be the normal situation).
Maybe this would be better ?
infineon,svid-mode-fixed-en-pin
  description: |
    Indicate the ENABLE pin is set at fixed level or left
    unconnected(has internal pull-up) which chip in
    SVID mode.

Regards,
Naresh
>
> But then, if that is the only way to get a property accepted, so be it.
>
> Guenter

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Guenter Roeck]

On Tue, Aug 22, 2023 at 09:41:48PM +0530, Naresh Solanki wrote:
> Hi,
> 
> On Tue, 22 Aug 2023 at 18:47, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Tue, Aug 22, 2023 at 02:32:31PM +0530, Naresh Solanki wrote:
> > > Hi
> > >
> > > On Fri, 18 Aug 2023 at 14:53, Krzysztof Kozlowski
> > > <krzysztof.kozlowski@linaro.org> wrote:
> > > >
> > > > On 16/08/2023 10:51, Naresh Solanki wrote:
> > > > > Hi Krzysztof,
> > > > >
> > > > > On Tue, 15 Aug 2023 at 01:02, Krzysztof Kozlowski
> > > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > > >>
> > > > >> On 11/08/2023 18:00, Naresh Solanki wrote:
> > > > >>> Hi,
> > > > >>>
> > > > >>> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
> > > > >>>>
> > > > >>>> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
> > > > >>>>> On 8/8/23 04:46, Conor Dooley wrote:
> > > > >>>>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> > > > >>>>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > >>>>>>>
> > > > >>>>>>> The TDA38640 chip has different output control mechanisms depending on
> > > > >>>>>>> its mode of operation. When the chip is in SVID mode, only
> > > > >>>>>>> hardware-based output control is supported via ENABLE pin. However, when
> > > > >>>>>>> it operates in PMBus mode, software control works perfectly.
> > > > >>>>>>>
> > > > >>>>>>> To enable software control as a workaround in SVID mode, add the DT
> > > > >>>>>>> property 'infineon,en-svid-control'. This property will enable the
> > > > >>>>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
> > > > >>>>>>> the chip is in SVID mode.
> > > > >>>>>>
> > > > >>>>>> Why do you need a custom property for this? How come it is not possible
> > > > >>>>>> to determine what bus you are on?
> > > > >>>>>>
> > > > >>>>>
> > > > >>>>> That is not the point. Yes, it can be detected if the control method is
> > > > >>>>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
> > > > >>>>> output, not PMBUs. This is bypassed by controlling the polarity of the
> > > > >>>>> (physical) output enable signal. We do _not_ want this enabled automatically
> > > > >>>>> in SVID mode. Its side effects on random boards using this chip are unknown.
> > > > >>>>> Thus, this needs a property which specifically enables this functionality
> > > > >>>>> for users who _really_ need to use it and (hopefully) know what they are
> > > > >>>>> doing.
> > > > >>>>
> > > > >>>> Hmm, reading this it makes a lot more sense why this is a property - I
> > > > >>>> guess I just struggled to understand the commit message here,
> > > > >>>> particularly what the benefit of using the workaround is. I'm still
> > > > >>>> having difficulty parsing the commit & property text though - its
> > > > >>>> unclear to me when you would need to use it - so I will stay out
> > > > >>>> of the way & let Rob or Krzysztof handle things.
> > > > >>>
> > > > >>> To provide context, my system employs a unique power sequence
> > > > >>> strategy utilizing a BMC (Baseboard Management Controller),
> > > > >>> rendering the reliance on the ENABLE pin unnecessary.
> > > > >>> In this configuration, the ENABLE pin is grounded in the hardware.
> > > > >>> While most regulators facilitate PMBus Operation for output control,
> > > > >>> the TDA38640 chip, when in SVID mode, is constrained by the
> > > > >>> ENABLE pin to align with Intel specifications.
> > > > >>> My communication with Infineon confirmed that the recommended
> > > > >>> approach is to invert the Enable Pin for my use case.
> > > > >>>
> > > > >>> Since this is not typically the use case for most setup & hence DT property
> > > > >>> is must for enabling the special case.
> > > > >>>
> > > > >>> For further insight into my setup's power sequence strategy, you can
> > > > >>> refer to the following link: https://github.com/9elements/pwrseqd
> > > > >>>
> > > > >>
> > > > >> This justifies to me the property, but still you described desired
> > > > >> driver behavior, not the hardware characteristic. Don't describe what
> > > > >> you want to control, but describe the entire system.
> > > > > I guess by entire system you mean how the regulators(including
> > > > > TDA38640) connected & operated in our setup ?
> > > >
> > > > I mean, property name and description should say what is the
> > > > characteristic of the hardware/firmware/entire system.
> > > Based on your feedback, will update to below:
> > > infineon,fixed-level-en-pin:
> > >     description: |
> > >       Indicate the ENABLE pin is set at fixed level or left
> > >       unconnected(has internal pull-up).
> > >     type: boolean
> >
> > Messy, because while it reflects physical connectivity, it doesn't reflect
> > its use in the system at all. The pin may be at fixed level or left
> > unconnected, but the system vendor doesn't want to give users the
> > means to control output power (which would be the normal situation).
> Maybe this would be better ?
> infineon,svid-mode-fixed-en-pin
>   description: |
>     Indicate the ENABLE pin is set at fixed level or left
>     unconnected(has internal pull-up) which chip in
>     SVID mode.

which chip ? Do you mean "with the chip" ?

I don't think that makes a difference. It still doesn't describe
your use case (which is something like "the chip is in SVID mode,
its enable pin is set to fixed level, and we need to manipulate its
interpretation by the chip so we can control the enable status from
software). I have no idea how to express that in a way that would be
acceptable as devicetree property.

It doesn't seem to me that we are making much progress here.
I know it isn't supposed to be done, and I don't really like it,
but could you use a module parameter in your system ? I'd be
open to accept that to make progress.

Thanks,
Guenter

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Naresh Solanki]

Hi.

On Tue, 22 Aug 2023 at 22:20, Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Tue, Aug 22, 2023 at 09:41:48PM +0530, Naresh Solanki wrote:
> > Hi,
> >
> > On Tue, 22 Aug 2023 at 18:47, Guenter Roeck <linux@roeck-us.net> wrote:
> > >
> > > On Tue, Aug 22, 2023 at 02:32:31PM +0530, Naresh Solanki wrote:
> > > > Hi
> > > >
> > > > On Fri, 18 Aug 2023 at 14:53, Krzysztof Kozlowski
> > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > > >
> > > > > On 16/08/2023 10:51, Naresh Solanki wrote:
> > > > > > Hi Krzysztof,
> > > > > >
> > > > > > On Tue, 15 Aug 2023 at 01:02, Krzysztof Kozlowski
> > > > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > > > >>
> > > > > >> On 11/08/2023 18:00, Naresh Solanki wrote:
> > > > > >>> Hi,
> > > > > >>>
> > > > > >>> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
> > > > > >>>>
> > > > > >>>> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
> > > > > >>>>> On 8/8/23 04:46, Conor Dooley wrote:
> > > > > >>>>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> > > > > >>>>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > >>>>>>>
> > > > > >>>>>>> The TDA38640 chip has different output control mechanisms depending on
> > > > > >>>>>>> its mode of operation. When the chip is in SVID mode, only
> > > > > >>>>>>> hardware-based output control is supported via ENABLE pin. However, when
> > > > > >>>>>>> it operates in PMBus mode, software control works perfectly.
> > > > > >>>>>>>
> > > > > >>>>>>> To enable software control as a workaround in SVID mode, add the DT
> > > > > >>>>>>> property 'infineon,en-svid-control'. This property will enable the
> > > > > >>>>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
> > > > > >>>>>>> the chip is in SVID mode.
> > > > > >>>>>>
> > > > > >>>>>> Why do you need a custom property for this? How come it is not possible
> > > > > >>>>>> to determine what bus you are on?
> > > > > >>>>>>
> > > > > >>>>>
> > > > > >>>>> That is not the point. Yes, it can be detected if the control method is
> > > > > >>>>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
> > > > > >>>>> output, not PMBUs. This is bypassed by controlling the polarity of the
> > > > > >>>>> (physical) output enable signal. We do _not_ want this enabled automatically
> > > > > >>>>> in SVID mode. Its side effects on random boards using this chip are unknown.
> > > > > >>>>> Thus, this needs a property which specifically enables this functionality
> > > > > >>>>> for users who _really_ need to use it and (hopefully) know what they are
> > > > > >>>>> doing.
> > > > > >>>>
> > > > > >>>> Hmm, reading this it makes a lot more sense why this is a property - I
> > > > > >>>> guess I just struggled to understand the commit message here,
> > > > > >>>> particularly what the benefit of using the workaround is. I'm still
> > > > > >>>> having difficulty parsing the commit & property text though - its
> > > > > >>>> unclear to me when you would need to use it - so I will stay out
> > > > > >>>> of the way & let Rob or Krzysztof handle things.
> > > > > >>>
> > > > > >>> To provide context, my system employs a unique power sequence
> > > > > >>> strategy utilizing a BMC (Baseboard Management Controller),
> > > > > >>> rendering the reliance on the ENABLE pin unnecessary.
> > > > > >>> In this configuration, the ENABLE pin is grounded in the hardware.
> > > > > >>> While most regulators facilitate PMBus Operation for output control,
> > > > > >>> the TDA38640 chip, when in SVID mode, is constrained by the
> > > > > >>> ENABLE pin to align with Intel specifications.
> > > > > >>> My communication with Infineon confirmed that the recommended
> > > > > >>> approach is to invert the Enable Pin for my use case.
> > > > > >>>
> > > > > >>> Since this is not typically the use case for most setup & hence DT property
> > > > > >>> is must for enabling the special case.
> > > > > >>>
> > > > > >>> For further insight into my setup's power sequence strategy, you can
> > > > > >>> refer to the following link: https://github.com/9elements/pwrseqd
> > > > > >>>
> > > > > >>
> > > > > >> This justifies to me the property, but still you described desired
> > > > > >> driver behavior, not the hardware characteristic. Don't describe what
> > > > > >> you want to control, but describe the entire system.
> > > > > > I guess by entire system you mean how the regulators(including
> > > > > > TDA38640) connected & operated in our setup ?
> > > > >
> > > > > I mean, property name and description should say what is the
> > > > > characteristic of the hardware/firmware/entire system.
> > > > Based on your feedback, will update to below:
> > > > infineon,fixed-level-en-pin:
> > > >     description: |
> > > >       Indicate the ENABLE pin is set at fixed level or left
> > > >       unconnected(has internal pull-up).
> > > >     type: boolean
> > >
> > > Messy, because while it reflects physical connectivity, it doesn't reflect
> > > its use in the system at all. The pin may be at fixed level or left
> > > unconnected, but the system vendor doesn't want to give users the
> > > means to control output power (which would be the normal situation).
> > Maybe this would be better ?
> > infineon,svid-mode-fixed-en-pin
> >   description: |
> >     Indicate the ENABLE pin is set at fixed level or left
> >     unconnected(has internal pull-up) which chip in
> >     SVID mode.
>
> which chip ? Do you mean "with the chip" ?
Yep. sorry for the typo.
>
> I don't think that makes a difference. It still doesn't describe
> your use case (which is something like "the chip is in SVID mode,
> its enable pin is set to fixed level, and we need to manipulate its
> interpretation by the chip so we can control the enable status from
> software). I have no idea how to express that in a way that would be
> acceptable as devicetree property.
In typical use case when chip is in SVID mode ,the ENABLE pin
isn't fixed i.e., either connected power good or FPGA pin.
If this property is explicitly specified then its clear in the hardware that
1. chip is in SVID mode,
2. ENABLE pin is fixed.
3. Intent to use the workaround.

@Krzysztof , Is this acceptable ? Can you please recommend a way
forward please.

Regards,
Naresh
>
> It doesn't seem to me that we are making much progress here.
> I know it isn't supposed to be done, and I don't really like it,
> but could you use a module parameter in your system ? I'd be
> open to accept that to make progress.
>
> Thanks,
> Guenter

Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

[Naresh Solanki]

Hi


On Tue, 22 Aug 2023 at 23:08, Naresh Solanki
<naresh.solanki@9elements.com> wrote:
>
> Hi.
>
> On Tue, 22 Aug 2023 at 22:20, Guenter Roeck <linux@roeck-us.net> wrote:
> >
> > On Tue, Aug 22, 2023 at 09:41:48PM +0530, Naresh Solanki wrote:
> > > Hi,
> > >
> > > On Tue, 22 Aug 2023 at 18:47, Guenter Roeck <linux@roeck-us.net> wrote:
> > > >
> > > > On Tue, Aug 22, 2023 at 02:32:31PM +0530, Naresh Solanki wrote:
> > > > > Hi
> > > > >
> > > > > On Fri, 18 Aug 2023 at 14:53, Krzysztof Kozlowski
> > > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > > > >
> > > > > > On 16/08/2023 10:51, Naresh Solanki wrote:
> > > > > > > Hi Krzysztof,
> > > > > > >
> > > > > > > On Tue, 15 Aug 2023 at 01:02, Krzysztof Kozlowski
> > > > > > > <krzysztof.kozlowski@linaro.org> wrote:
> > > > > > >>
> > > > > > >> On 11/08/2023 18:00, Naresh Solanki wrote:
> > > > > > >>> Hi,
> > > > > > >>>
> > > > > > >>> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@kernel.org> wrote:
> > > > > > >>>>
> > > > > > >>>> On Tue, Aug 08, 2023 at 07:10:08AM -0700, Guenter Roeck wrote:
> > > > > > >>>>> On 8/8/23 04:46, Conor Dooley wrote:
> > > > > > >>>>>> On Wed, Aug 02, 2023 at 09:31:51PM +0200, Naresh Solanki wrote:
> > > > > > >>>>>>> From: Patrick Rudolph <patrick.rudolph@9elements.com>
> > > > > > >>>>>>>
> > > > > > >>>>>>> The TDA38640 chip has different output control mechanisms depending on
> > > > > > >>>>>>> its mode of operation. When the chip is in SVID mode, only
> > > > > > >>>>>>> hardware-based output control is supported via ENABLE pin. However, when
> > > > > > >>>>>>> it operates in PMBus mode, software control works perfectly.
> > > > > > >>>>>>>
> > > > > > >>>>>>> To enable software control as a workaround in SVID mode, add the DT
> > > > > > >>>>>>> property 'infineon,en-svid-control'. This property will enable the
> > > > > > >>>>>>> workaround, which utilizes ENABLE pin polarity flipping for output when
> > > > > > >>>>>>> the chip is in SVID mode.
> > > > > > >>>>>>
> > > > > > >>>>>> Why do you need a custom property for this? How come it is not possible
> > > > > > >>>>>> to determine what bus you are on?
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>> That is not the point. Yes, it can be detected if the control method is
> > > > > > >>>>> PMBus or SVID. However, in SVID mode, SVID is supposed to control the
> > > > > > >>>>> output, not PMBUs. This is bypassed by controlling the polarity of the
> > > > > > >>>>> (physical) output enable signal. We do _not_ want this enabled automatically
> > > > > > >>>>> in SVID mode. Its side effects on random boards using this chip are unknown.
> > > > > > >>>>> Thus, this needs a property which specifically enables this functionality
> > > > > > >>>>> for users who _really_ need to use it and (hopefully) know what they are
> > > > > > >>>>> doing.
> > > > > > >>>>
> > > > > > >>>> Hmm, reading this it makes a lot more sense why this is a property - I
> > > > > > >>>> guess I just struggled to understand the commit message here,
> > > > > > >>>> particularly what the benefit of using the workaround is. I'm still
> > > > > > >>>> having difficulty parsing the commit & property text though - its
> > > > > > >>>> unclear to me when you would need to use it - so I will stay out
> > > > > > >>>> of the way & let Rob or Krzysztof handle things.
> > > > > > >>>
> > > > > > >>> To provide context, my system employs a unique power sequence
> > > > > > >>> strategy utilizing a BMC (Baseboard Management Controller),
> > > > > > >>> rendering the reliance on the ENABLE pin unnecessary.
> > > > > > >>> In this configuration, the ENABLE pin is grounded in the hardware.
> > > > > > >>> While most regulators facilitate PMBus Operation for output control,
> > > > > > >>> the TDA38640 chip, when in SVID mode, is constrained by the
> > > > > > >>> ENABLE pin to align with Intel specifications.
> > > > > > >>> My communication with Infineon confirmed that the recommended
> > > > > > >>> approach is to invert the Enable Pin for my use case.
> > > > > > >>>
> > > > > > >>> Since this is not typically the use case for most setup & hence DT property
> > > > > > >>> is must for enabling the special case.
> > > > > > >>>
> > > > > > >>> For further insight into my setup's power sequence strategy, you can
> > > > > > >>> refer to the following link: https://github.com/9elements/pwrseqd
> > > > > > >>>
> > > > > > >>
> > > > > > >> This justifies to me the property, but still you described desired
> > > > > > >> driver behavior, not the hardware characteristic. Don't describe what
> > > > > > >> you want to control, but describe the entire system.
> > > > > > > I guess by entire system you mean how the regulators(including
> > > > > > > TDA38640) connected & operated in our setup ?
> > > > > >
> > > > > > I mean, property name and description should say what is the
> > > > > > characteristic of the hardware/firmware/entire system.
> > > > > Based on your feedback, will update to below:
> > > > > infineon,fixed-level-en-pin:
> > > > >     description: |
> > > > >       Indicate the ENABLE pin is set at fixed level or left
> > > > >       unconnected(has internal pull-up).
> > > > >     type: boolean
> > > >
> > > > Messy, because while it reflects physical connectivity, it doesn't reflect
> > > > its use in the system at all. The pin may be at fixed level or left
> > > > unconnected, but the system vendor doesn't want to give users the
> > > > means to control output power (which would be the normal situation).
> > > Maybe this would be better ?
> > > infineon,svid-mode-fixed-en-pin
> > >   description: |
> > >     Indicate the ENABLE pin is set at fixed level or left
> > >     unconnected(has internal pull-up) which chip in
> > >     SVID mode.
> >
> > which chip ? Do you mean "with the chip" ?
> Yep. sorry for the typo.
> >
> > I don't think that makes a difference. It still doesn't describe
> > your use case (which is something like "the chip is in SVID mode,
> > its enable pin is set to fixed level, and we need to manipulate its
> > interpretation by the chip so we can control the enable status from
> > software). I have no idea how to express that in a way that would be
> > acceptable as devicetree property.
> In typical use case when chip is in SVID mode ,the ENABLE pin
> isn't fixed i.e., either connected power good or FPGA pin.
> If this property is explicitly specified then its clear in the hardware that
> 1. chip is in SVID mode,
> 2. ENABLE pin is fixed.
> 3. Intent to use the workaround.
>
> @Krzysztof , Is this acceptable ? Can you please recommend a way
> forward please.

I guess the below change in property should be fine. Will push V4 based on it.
infineon,svid-mode-fixed-en-pin
  description: |
    Indicate the ENABLE pin is set at fixed level or left
    unconnected(has internal pull-up) with the chip in
    SVID mode.

Regards,
Naresh
>
> Regards,
> Naresh
> >
> > It doesn't seem to me that we are making much progress here.
> > I know it isn't supposed to be done, and I don't really like it,
> > but could you use a module parameter in your system ? I'd be
> > open to accept that to make progress.
> >
> > Thanks,
> > Guenter