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

[Joseph Lo <josephl@nvidia.com>]

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.

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

> 
>> +- nvidia,align-step-uv: Voltage increase in micro volts corresponding to a
>> +			1/33th increase in duty cycle. Eg the voltage for 2/33th
>> +			duty cycle would be:
> 
> Maybe consider 'pwm-voltage-step-microvolt'.
Okay.

> 
>> +			nvidia,align-offset-uv + nvidia,align-step-uv * 2.
>> +- pinctrl-0: I/O pad configuration when PWM control is enabled.
>> +- pinctrl-1: I/O pad configuration when PWM control is disabled.
>> +- pinctrl-names: must include the following entries:
>> +  - dvfs_pwm_enable: I/O pad configuration when PWM control is enabled.
>> +  - dvfs_pwm_disable: I/O pad configuration when PWM control is disabled.
> 
> Please see Rob's feedback on the above [0].
Yes, I did refer that comments in this patch. And fixed that in 
description but missed for bindings ...

Thanks,
Joseph

> 
> Cheers
> Jon
> 
> [0] https://lore.kernel.org/patchwork/patch/885328/
>