Re: [PATCH 01/19] dt-bindings: clock: tegra124-dfll: Update DFLL binding for PWM regulator

Joseph Lo
2018-12-11T01:28:50Z

On 12/10/18 5:44 PM, Jon Hunter wrote:
>
> On 10/12/2018 09:31, Joseph Lo wrote:
>> On 12/10/18 4:59 PM, Jon Hunter wrote:
>>>
>>> On 10/12/2018 08:49, Joseph Lo wrote:
>>>> Hi Jon,
>>>>
>>>> Thanks for reviewing this series.
>>>>
>>>> On 12/7/18 9:41 PM, Jon Hunter wrote:
>>>>>
>>>>> On 04/12/2018 09:25, Joseph Lo wrote:
>>>>>> From: Peter De Schrijver <pdeschrijver@nvidia.com>
>>>>>>
>>>>>> Add new properties to configure the DFLL PWM regulator support. Also
>>>>>> add an example and make the I2C clock only required when I2C
>>>>>> support is
>>>>>> used.
>>>>>>
>>>>>> Cc: devicetree@vger.kernel.org
>>>>>> Signed-off-by: Peter De Schrijver <pdeschrijver@nvidia.com>
>>>>>> Signed-off-by: Joseph Lo <josephl@nvidia.com>
>>>>>> ---
>>>>>>    .../bindings/clock/nvidia,tegra124-dfll.txt   | 73
>>>>>> ++++++++++++++++++-
>>>>>>    1 file changed, 71 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
>>>>>> b/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
>>>>>> index dff236f524a7..8c97600d2bad 100644
>>>>>> --- a/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
>>>>>> +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra124-dfll.txt
>>>>>> @@ -8,7 +8,6 @@ the fast CPU cluster. It consists of a free-running
>>>>>> voltage controlled
>>>>>>    oscillator connected to the CPU voltage rail (VDD_CPU), and a
>>>>>> closed loop
>>>>>>    control module that will automatically adjust the VDD_CPU
>>>>>> voltage by
>>>>>>    communicating with an off-chip PMIC either via an I2C bus or via
>>>>>> PWM signals.
>>>>>> -Currently only the I2C mode is supported by these bindings.
>>>>>>      Required properties:
>>>>>>    - compatible : should be "nvidia,tegra124-dfll"
>>>>>> @@ -45,10 +44,28 @@ Required properties for the control loop
>>>>>> parameters:
>>>>>>    Optional properties for the control loop parameters:
>>>>>>    - nvidia,cg-scale: Boolean value, see the field
>>>>>> DFLL_PARAMS_CG_SCALE in the TRM.
>>>>>>    +Optional properties for mode selection:
>>>>>> +- nvidia,pwm-to-pmic: Use PWM to control regulator rather then I2C.
>>>>>> +
>>>>>>    Required properties for I2C mode:
>>>>>>    - nvidia,i2c-fs-rate: I2C transfer rate, if using full speed mode.
>>>>>>    -Example:
>>>>>> +Required properties for PWM mode:
>>>>>> +- nvidia,pwm-period: period of PWM square wave in microseconds.
>>>>>> +- nvidia,init-uv: Regulator voltage in micro volts when PWM control
>>>>>> is disabled.
>>>>>
>>>>> Maybe consider 'pwm-inactive-voltage-microvolt'.
>>>> Ah, I think I need to refine the description here. It should be
>>>> something like below.
>>>>   - nvidia,pwm-init-microvolt : Regulator voltage in micro volts when
>>>> PWM
>>>> control is initialized
>>>>
>>>> This is the initial voltage that when we just initialize the DFLL
>>>> hardware for PWM output. And before we switch the CPU clock from PLLX to
>>>> DFLL, we will enable DFLL hardware in closed loop mode which will aplly
>>>> the DVFS table that was calculated from CVB table.
>>>>
>>>> The original description maybe make you think that it's the working
>>>> voltage when it's under open-loop mode. But it's not. Sorry.
>>>>
>>>> When we working on open-loop mode which will switch to low voltage range
>>>> which also follows the DVFS table. Not this one.
>>>
>>> OK, but I am still not sure what this voltage is. I mean that I
>>> understand it is the initial voltage, but how exactly do we define this
>>> number? Where does it come from, how is this determined?
>> IIRC, this is the same output voltage that bootloader configured with
>> proper PLLX rate. So before DFLL handling the CPU clock, that is the
>> output voltage. We configure the same when DFLL HW is just initialized.
>>
>> 1 volt here is pretty safe when CPU clock switched from bootloader to
>> kernel at 6.912MHz@pllx.
>
> OK, now I understand. Seems a little fragile, but maybe there is no
> better alternative here.
>
> Please describe what this voltage is in the binding doc, so if anyone
> happened to modify their bootloader to run at a different speed that it
> is stated that this voltage should be high enough to support initial
> boot frequency.
>
>>>
>>>>>
>>>>>> +- nvidia,align-offset-uv: Regulator voltage in micro volts when PWM
>>>>>> control is
>>>>>> +              enabled and PWM output is low.
>>>>>
>>>>> Would this be considered the minimum pwm active voltage?
>>>> This would be used for minimum voltage for LUT table, which is the table
>>>> that PMIC can output. The real minimum voltage in PWM mode still depends
>>>> on the CVB table.
>>>>
>>>> So maybe change this one to 'nvidia,pwm-offset-uv'.
>>>
>>> So is this the min supported by the PMIC? Maybe the name should reflect
>>> that because the above name does not reflect this. Furthermore, if this
>>> is a min then maybe the name should use 'min' as opposed to 'offset'.
>>> for example, 'nvidia,pwm-pmic-min-microvolts'.
>>>
>>> Does this need to be described in DT, can it not be queried from the
>>> PMIC?
>>>
>> Yes, either way needs to go with DT. The very initial patchset for
>> DFLL-PWM support, which introduced a redundant pwm-dfll driver. And with
>> the driver, we can describe the PWM vdd-cpu regulator in DT with the
>> voltage table that the PMIC can output. But NAKed by reviewer.
>>
>> Okay, 'nvidia,pwm-pmic-min-microvolts' looks fine. Thanks.
>>
>> [1]: https://patchwork.ozlabs.org/patch/613524/
>
> Thierry, what are your thoughts here? I guess you had asked initially
> why we needed a phandle to the PMIC. Seems we need to comprehend the min
> voltage supported by the PMIC for creating the LUT.
>

Jon,

Notice that this is not a real "PWM" driver it just provides an
interface that makes us can provide a PWM regulator node with voltage
table. Then you can get LUT from that. But the PWM driver itself is a
fake pwm driver as the DFLL-PWM is hardware driven not SW.

And as you saw in [1], the DFLL control code should remain in DFLL
driver, not in the PWM driver. So basically, I still prefer the way we
presented in this series.

Thanks,
Joseph

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org