Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage

[Marco Felsch <m.felsch@pengutronix.de>]

On 19-12-02 15:30, Stefan Riedmüller wrote:
> Hi Marco,
> 
> On 02.12.19 15:14, Marco Felsch wrote:
> > Hi Stefan,
> > 
> > On 19-12-02 14:55, Stefan Riedmüller wrote:
> > > Hi Marco,
> > > 
> > > On 02.12.19 13:42, Marco Felsch wrote:
> > > > Hi Stefan,
> > > > 
> > > > On 19-12-02 11:09, Stefan Riedmüller wrote:
> > > > > Hi Marco,
> > > > > 
> > > > > your proposed setting is only valid for the LDO enabled case but not for the
> > > > > case where the LDO's are in bypass mode. Is that intended? In bypass mode it
> > > > > actually needs to be 0.925 V min for ARM and 1.15 V min for SOC.
> > > > 
> > > > The case is that the driver doesn't support the bypass mode currently so
> > > > yes it was intended.
> > > 
> > > Ok, I see.
> > > 
> > > > 
> > > > > Did you experience an issue with the current settings or is this just a
> > > > > cosmetic change?
> > > > 
> > > > There is currently no issue because the internally LDO's don't try to
> > > > apply such a low voltage value. But I think it isn't a cosmetic change
> > > > because this value is wrong. We need to specify the valid voltage range.
> > > 
> > > Please correct me if I'm wrong, but isn't the regulator-min and
> > > regulator-max values supposed to reflect the min and max values this
> > > regulator can deliver?
> > 
> > Nope, the constraints are hard coded within the driver e.g. da9062: >
> > 8<-----------------------------------------------------------------
> > /* Regulator operations */
> > 
> > /* Current limits array (in uA)
> >   * - DA9061_ID_[BUCK1|BUCK3]
> >   * - DA9062_ID_[BUCK1|BUCK2|BUCK4]
> >   * Entry indexes corresponds to register values.
> >   */
> > static const unsigned int da9062_buck_a_limits[] = {
> > 	500000,  600000,  700000,  800000,  900000, 1000000,
> > 	1100000, 1200000,
> > 	1300000, 1400000, 1500000, 1600000, 1700000,
> > 	1800000, 1900000, 2000000
> > };
> > 
> > /* Current limits array (in uA)
> >   * - DA9061_ID_BUCK2
> >   * - DA9062_ID_BUCK3
> >   * Entry indexes corresponds to register values.
> >   */
> > static const unsigned int
> > da9062_buck_b_limits[] = {
> > 	1500000, 1600000, 1700000, 1800000,
> > 	1900000, 2000000, 2100000, 2200000,
> > 	2300000, 2400000, 2500000,
> > 	2600000, 2700000, 2800000,
> > 	2900000, 3000000
> > };
> > 
> > 8<-----------------------------------------------------------------
> > 
> 
> These are the available current limits for the buck regulators. I don't see
> where they correspond to the min/max settable output voltage. Maybe I missed
> something?

Please check the following structs:

 - static const struct da9062_regulator_info local_da9061_regulator_info[]
 - static const struct da9062_regulator_info local_da9062_regulator_info[]

There you have the min_uV, uV_step, n_voltages so the core can validate
if the dt-value is within the range.

> The regulator bindings state:
> - regulator-min-microvolt: smallest voltage consumers may set
> 
> - regulator-max-microvolt: largest voltage consumers may set

Yes and according the datasheet I mentionied the current values aren't
correct.

> For me that is device depended and not design depended.
> 
> What is the scenario you're thinking about which would cause the SOC, as a
> consumer, to request a lower voltage as it needs?

The thing is that the DT abstracts the HW and these values are not
correct. As mentioned in my commit message the values should meet
the datasheet restrictions and this isn't the case yet.

Regards,
  Marco 

> Regards,
> Stefan
> 
> > So you have to specify the min/max voltage for your design.
> > 
> > Regards,
> >    Marco
> > 
> > > Maybe your change is better placed in the anatop regulators. Btw they also
> > > have a 0.725 V minimum voltage:
> > > 
> > >  From arch/arm/boot/dts/imx6qdl.dtsi:
> > > 
> > >                                  reg_arm: regulator-vddcore {
> > > 
> > >                                          compatible = "fsl,anatop-regulator";
> > >                                          regulator-name = "vddarm";
> > > 
> > >                                          regulator-min-microvolt = <725000>;
> > > 
> > >                                          regulator-max-microvolt = <1450000>;
> > >                                          regulator-always-on;
> > > 
> > >                                          anatop-reg-offset = <0x140>;
> > > 
> > >                                          anatop-vol-bit-shift = <0>;
> > > 
> > >                                          anatop-vol-bit-width = <5>;
> > > 
> > >                                          anatop-delay-reg-offset = <0x170>;
> > > 
> > >                                          anatop-delay-bit-shift = <24>;
> > > 
> > >                                          anatop-delay-bit-width = <2>;
> > > 
> > >                                          anatop-min-bit-val = <1>;
> > > 
> > >                                          anatop-min-voltage = <725000>;
> > > 
> > >                                          anatop-max-voltage = <1450000>;
> > > 
> > >                                  };
> > > 
> > > 
> > > Regards,
> > > Stefan
> > > 
> > > > 
> > > > Regards,
> > > >     Marco
> > > > 
> > > > > Regards,
> > > > > Stefan
> > > > > 
> > > > > 
> > > > > On 29.11.19 17:48, Marco Felsch wrote:
> > > > > > The current set minimum voltage of 730000mV seems to be wrong. I don't
> > > > > > know the document which specifies that but the imx6qdl datasheets says
> > > > > > that the minimum voltage should be 1.05V for VDD_ARM (LDO enabled, lowest
> > > > > > opp) and 1.275V for VDD_SOC (LDO enabled, lowest opp).
> > > > > > 
> > > > > > Fixes: ddec5d1c0047 ("ARM: dts: imx6: Add initial support for phyCORE-i.MX 6 SOM")
> > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > > > ---
> > > > > >     arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi | 4 ++--
> > > > > >     1 file changed, 2 insertions(+), 2 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> > > > > > index 6486df3e2942..46d4953c5588 100644
> > > > > > --- a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> > > > > > +++ b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> > > > > > @@ -107,14 +107,14 @@
> > > > > >     		regulators {
> > > > > >     			vdd_arm: buck1 {
> > > > > >     				regulator-name = "vdd_arm";
> > > > > > -				regulator-min-microvolt = <730000>;
> > > > > > +				regulator-min-microvolt = <1050000>;
> > > > > >     				regulator-max-microvolt = <1380000>;
> > > > > >     				regulator-always-on;
> > > > > >     			};
> > > > > >     			vdd_soc: buck2 {
> > > > > >     				regulator-name = "vdd_soc";
> > > > > > -				regulator-min-microvolt = <730000>;
> > > > > > +				regulator-min-microvolt = <1275000>;
> > > > > >     				regulator-max-microvolt = <1380000>;
> > > > > >     				regulator-always-on;
> > > > > >     			};
> > > > > > 
> > > > > 
> > > > 
> > > 
> > 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel