Message-ID: <CABqG17iKVEZ6+f8UctpK1A6VeRL9u+yYyfV7we=QsQ8vEi4+cA@mail.gmail.com>
Date:   Tue, 29 Aug 2023 19:40:45 +0530
From:   Naresh Solanki <naresh.solanki@...ements.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
        Conor Dooley <conor@...nel.org>,
        Jean Delvare <jdelvare@...e.com>,
        krzysztof.kozlowski+dt@...aro.org,
        Rob Herring <robh+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>,
        linux-hwmon@...r.kernel.org,
        Patrick Rudolph <patrick.rudolph@...ements.com>,
        Rob Herring <robh@...nel.org>, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/3] dt-bindings: hwmon: Add Infineon TDA38640

Hi


On Tue, 22 Aug 2023 at 23:08, Naresh Solanki
<naresh.solanki@...ements.com> wrote:
>
> Hi.
>
> On Tue, 22 Aug 2023 at 22:20, Guenter Roeck <linux@...ck-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@...ck-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@...aro.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@...aro.org> wrote:
> > > > > > >>
> > > > > > >> On 11/08/2023 18:00, Naresh Solanki wrote:
> > > > > > >>> Hi,
> > > > > > >>>
> > > > > > >>> On Tue, 8 Aug 2023 at 19:58, Conor Dooley <conor@...nel.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@...ements.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

Powered by blists - more mailing lists