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

[Joseph Lo <josephl@nvidia.com>]

On 12/11/18 7:52 PM, Jon Hunter wrote:
> 
> On 11/12/2018 09:15, Peter De Schrijver wrote:
>> On Fri, Dec 07, 2018 at 01:41:57PM +0000, 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'.
>>>
>>
>> Inactive is not very accurate. The OVR regulator will output
>> nvidia,align-offset-uv when the PWM input is driven low but will output
>> nvidia,init-uv when the PWM input is in tristate mode.
> 
> Maybe but I really don't find 'nvidia,align-offset-uv' and
> 'nvidia,init-uv' very descriptive either. We need to make sure that the
> names and description make it clear what these are and where they come
> from to anyone reading that documentation that has never laid eyes on
> this before.
> 
> Sounds like the align-offset-uv is the minimum voltage when PWM is
> active/enabled and init-uv is the default voltage the regulator outputs
> when PWM control is disabled. Would the following be any better ...
> 
> - nvidia,pwm-tristate-microvolts: Regulator voltage in micro volts when
>    PWM control is disabled and the PWM output is tristated. Note that
>    this voltage is configured in hardware, typically via a resistor
>    divider.
> - nvidia,pwm-min-microvolts: Regulator voltage in micro volts when PWM
>    control is enabled and PWM output is low. Hence, this is the minimum
>    output voltage that the regulator supports when PWM control is
>    enabled.

Thanks, Jon and Peter.

That's more clear indeed. In the discussion, that also comes up with one 
more reason that we shouldn't handle vdd-cpu rail with regulator API in 
cpufreq driver that showed in other patches in this patchset. With 
PWM-based vdd-cpu regulator, we are not able to control the output in 
software.