Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
@ 2019-11-29 16:48 Marco Felsch
  2019-11-29 16:48 ` [PATCH 2/3] ARM: dts: imx6: phycore-som: fix emmc supply Marco Felsch
                   ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Marco Felsch @ 2019-11-29 16:48 UTC (permalink / raw)
  To: robh+dt, shawnguo, festevam, linux-imx, c.hemp, s.christ,
	chf.fritz, s.riedmueller
  Cc: kernel, linux-arm-kernel

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;
 			};
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 2/3] ARM: dts: imx6: phycore-som: fix emmc supply
  2019-11-29 16:48 [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage Marco Felsch
@ 2019-11-29 16:48 ` Marco Felsch
  2019-12-05 12:00   ` Stefan Riedmüller
  2019-11-29 16:48 ` [PATCH 3/3] ARM: dts: imx6: phycore-som: add pmic onkey device Marco Felsch
  2019-12-02 10:09 ` [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage Stefan Riedmüller
  2 siblings, 1 reply; 16+ messages in thread
From: Marco Felsch @ 2019-11-29 16:48 UTC (permalink / raw)
  To: robh+dt, shawnguo, festevam, linux-imx, c.hemp, s.christ,
	chf.fritz, s.riedmueller
  Cc: kernel, linux-arm-kernel

Currently the vmmc is supplied by the 1.8V pmic rail but this is wrong.
The 1.8V pmic rail is connected to the emmc vccq (vqmmc).

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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
index 46d4953c5588..44e333848b4d 100644
--- a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
@@ -183,7 +183,7 @@
 	pinctrl-0 = <&pinctrl_usdhc4>;
 	bus-width = <8>;
 	non-removable;
-	vmmc-supply = <&vdd_emmc_1p8>;
+	vqmmc-supply = <&vdd_emmc_1p8>;
 	status = "disabled";
 };
 
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* [PATCH 3/3] ARM: dts: imx6: phycore-som: add pmic onkey device
  2019-11-29 16:48 [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage Marco Felsch
  2019-11-29 16:48 ` [PATCH 2/3] ARM: dts: imx6: phycore-som: fix emmc supply Marco Felsch
@ 2019-11-29 16:48 ` Marco Felsch
  2019-12-02 10:09 ` [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage Stefan Riedmüller
  2 siblings, 0 replies; 16+ messages in thread
From: Marco Felsch @ 2019-11-29 16:48 UTC (permalink / raw)
  To: robh+dt, shawnguo, festevam, linux-imx, c.hemp, s.christ,
	chf.fritz, s.riedmueller
  Cc: kernel, linux-arm-kernel

Without the onkey device it isn't possible to power off the system using
the X_PMIC_nONKEY signal which is routed to the SoM pin header.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
index 44e333848b4d..cb6789807de9 100644
--- a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
@@ -100,6 +100,10 @@
 			compatible = "dlg,da9062-rtc";
 		};
 
+		da9062_onkey: onkey {
+			compatible = "dlg,da9062-onkey";
+		};
+
 		watchdog {
 			compatible = "dlg,da9062-watchdog";
 		};
-- 
2.20.1


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-11-29 16:48 [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage Marco Felsch
  2019-11-29 16:48 ` [PATCH 2/3] ARM: dts: imx6: phycore-som: fix emmc supply Marco Felsch
  2019-11-29 16:48 ` [PATCH 3/3] ARM: dts: imx6: phycore-som: add pmic onkey device Marco Felsch
@ 2019-12-02 10:09 ` Stefan Riedmüller
  2019-12-02 12:42   ` Marco Felsch
  2 siblings, 1 reply; 16+ messages in thread
From: Stefan Riedmüller @ 2019-12-02 10:09 UTC (permalink / raw)
  To: Marco Felsch
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

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.

Did you experience an issue with the current settings or is this just a 
cosmetic change?

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;
>   			};
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-02 10:09 ` [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage Stefan Riedmüller
@ 2019-12-02 12:42   ` Marco Felsch
  2019-12-02 13:55     ` Stefan Riedmüller
  0 siblings, 1 reply; 16+ messages in thread
From: Marco Felsch @ 2019-12-02 12:42 UTC (permalink / raw)
  To: Stefan Riedmüller
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

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.

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

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-02 12:42   ` Marco Felsch
@ 2019-12-02 13:55     ` Stefan Riedmüller
  2019-12-02 14:14       ` Marco Felsch
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Riedmüller @ 2019-12-02 13:55 UTC (permalink / raw)
  To: Marco Felsch
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

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?

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;
>>>    			};
>>>
>>
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-02 13:55     ` Stefan Riedmüller
@ 2019-12-02 14:14       ` Marco Felsch
  2019-12-02 14:30         ` Stefan Riedmüller
  0 siblings, 1 reply; 16+ messages in thread
From: Marco Felsch @ 2019-12-02 14:14 UTC (permalink / raw)
  To: Stefan Riedmüller
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

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<-----------------------------------------------------------------

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-02 14:14       ` Marco Felsch
@ 2019-12-02 14:30         ` Stefan Riedmüller
  2019-12-02 14:53           ` Marco Felsch
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Riedmüller @ 2019-12-02 14:30 UTC (permalink / raw)
  To: Marco Felsch
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

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?

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

- regulator-max-microvolt: largest voltage consumers may set

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?

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;
>>>>>     			};
>>>>>
>>>>
>>>
>>
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-02 14:30         ` Stefan Riedmüller
@ 2019-12-02 14:53           ` Marco Felsch
  2019-12-03  8:11             ` Stefan Riedmüller
  0 siblings, 1 reply; 16+ messages in thread
From: Marco Felsch @ 2019-12-02 14:53 UTC (permalink / raw)
  To: Stefan Riedmüller
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-02 14:53           ` Marco Felsch
@ 2019-12-03  8:11             ` Stefan Riedmüller
  2019-12-03  8:33               ` Marco Felsch
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Riedmüller @ 2019-12-03  8:11 UTC (permalink / raw)
  To: Marco Felsch
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

Hi Marco,

On 02.12.19 15:53, Marco Felsch wrote:
> 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.

Thanks, that makes more sense.

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

I don't agree. The datasheet you mention is the i.MX 6 datasheet and thus 
the limitation should reside in the i.MX 6 regulators and not in the PMIC's. 
This limitation is not just valid in combination with that PMIC but for all 
i.MX 6.

If I have this wrong and the maintainers agree with you could you please 
make sure to account for the bypass mode as well since these values from the 
datasheet are valid too?

Regards,
Stefan

> 
> 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;
>>>>>>>      			};
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-03  8:11             ` Stefan Riedmüller
@ 2019-12-03  8:33               ` Marco Felsch
  2019-12-03  9:07                 ` Stefan Riedmüller
  2019-12-03 11:44                 ` Lucas Stach
  0 siblings, 2 replies; 16+ messages in thread
From: Marco Felsch @ 2019-12-03  8:33 UTC (permalink / raw)
  To: Stefan Riedmüller
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

Hi Stefan,

On 19-12-03 09:11, Stefan Riedmüller wrote:
> Hi Marco,
> 
> On 02.12.19 15:53, Marco Felsch wrote:
> > 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.
> 
> Thanks, that makes more sense.
> 
> > 
> > > 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.
> 
> I don't agree. The datasheet you mention is the i.MX 6 datasheet and thus
> the limitation should reside in the i.MX 6 regulators and not in the PMIC's.
> This limitation is not just valid in combination with that PMIC but for all
> i.MX 6.

The datasheet tells you which voltage should be applied to the imx6 and
so you have to set this here. What happens if the internally ldo
request a voltage value below 0.9V? Then the value will be applied
because we specified 0.73V and the system don't work anymore or did you
verified that case?

> If I have this wrong and the maintainers agree with you could you please
> make sure to account for the bypass mode as well since these values from the
> datasheet are valid too?

As I said, the bypass mode isn't supported by the driver and all imx6
based devicetrees follow that case. So we don't have to take that into
account. Also we can't meet both contraints with one dt and futhermore
the bypass mode decrease your imx6 lifetime due the the increased ripple
on the arm-core supply. So I think no one wants this setup in the near
future.

Regards,
  Marco

> Regards,
> Stefan
> 
> > 
> > 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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-03  8:33               ` Marco Felsch
@ 2019-12-03  9:07                 ` Stefan Riedmüller
  2019-12-03 11:44                 ` Lucas Stach
  1 sibling, 0 replies; 16+ messages in thread
From: Stefan Riedmüller @ 2019-12-03  9:07 UTC (permalink / raw)
  To: Marco Felsch
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

Hi Marco,

On 03.12.19 09:33, Marco Felsch wrote:
> Hi Stefan,
> 
> On 19-12-03 09:11, Stefan Riedmüller wrote:
>> Hi Marco,
>>
>> On 02.12.19 15:53, Marco Felsch wrote:
>>> 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.
>>
>> Thanks, that makes more sense.
>>
>>>
>>>> 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.
>>
>> I don't agree. The datasheet you mention is the i.MX 6 datasheet and thus
>> the limitation should reside in the i.MX 6 regulators and not in the PMIC's.
>> This limitation is not just valid in combination with that PMIC but for all
>> i.MX 6.
> 
> The datasheet tells you which voltage should be applied to the imx6 and
> so you have to set this here. What happens if the internally ldo
> request a voltage value below 0.9V? Then the value will be applied
> because we specified 0.73V and the system don't work anymore or did you
> verified that case?

The LDO should not be able to request a too low voltage since the values 
from the i.MX 6 datasheet need to be applied to the LDO regulator and thus 
preventing the LDO from requesting this too low voltage. Why just fix this 
for this particular case with this one PMIC when it actually could be fixed 
for all i.MX 6 PMIC combinations? These limitations are fixed limitations 
from the i.MX 6 datasheet and do not result from the PMIC + i.MX 6 combination.

> 
>> If I have this wrong and the maintainers agree with you could you please
>> make sure to account for the bypass mode as well since these values from the
>> datasheet are valid too?
> 
> As I said, the bypass mode isn't supported by the driver and all imx6
> based devicetrees follow that case. So we don't have to take that into
> account. Also we can't meet both contraints with one dt and futhermore
> the bypass mode decrease your imx6 lifetime due the the increased ripple
> on the arm-core supply. So I think no one wants this setup in the near
> future.

Ok.

Regards,
Stefan

> 
> Regards,
>    Marco
> 
>> Regards,
>> Stefan
>>
>>>
>>> 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;
>>>>>>>>>       			};
>>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-03  8:33               ` Marco Felsch
  2019-12-03  9:07                 ` Stefan Riedmüller
@ 2019-12-03 11:44                 ` Lucas Stach
  2019-12-03 12:37                   ` Stefan Riedmüller
  1 sibling, 1 reply; 16+ messages in thread
From: Lucas Stach @ 2019-12-03 11:44 UTC (permalink / raw)
  To: Marco Felsch, Stefan Riedmüller
  Cc: festevam, shawnguo, chf.fritz, robh+dt, linux-imx, kernel,
	c.hemp, s.christ, linux-arm-kernel

On Di, 2019-12-03 at 09:33 +0100, Marco Felsch wrote:
[...]
> > > 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.
> > 
> > Thanks, that makes more sense.
> > 
> > > > 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.
> > 
> > I don't agree. The datasheet you mention is the i.MX 6 datasheet and thus
> > the limitation should reside in the i.MX 6 regulators and not in the PMIC's.
> > This limitation is not just valid in combination with that PMIC but for all
> > i.MX 6.
> 
> The datasheet tells you which voltage should be applied to the imx6 and
> so you have to set this here. What happens if the internally ldo
> request a voltage value below 0.9V? Then the value will be applied
> because we specified 0.73V and the system don't work anymore or did you
> verified that case?

The DT constraints are supposed to reflect absolute maximum ratings of
the board design. The regulator driver already knows the limits of the
PMIC chip, so there is no point in duplicating this information in the
DT.

The DT constraints are there to make sure the regulator core can
constrain the voltage setting to something safe for the specific
design. A consumer driver bug must never be able to set a voltage that
is outside of the absolute maximum ratings of _all_ consumers of this
specific power rail. I know that a lot of DTs get this detail wrong,
but we shouldn't block patches to fix this. :)

> > If I have this wrong and the maintainers agree with you could you please
> > make sure to account for the bypass mode as well since these values from the
> > datasheet are valid too?
> 
> As I said, the bypass mode isn't supported by the driver and all imx6
> based devicetrees follow that case. So we don't have to take that into
> account. Also we can't meet both contraints with one dt and futhermore
> the bypass mode decrease your imx6 lifetime due the the increased ripple
> on the arm-core supply. So I think no one wants this setup in the near
> future.

As a violation of the minimum voltage setting is very unlikely to cause
any permanent damage to the design (expect if you got reverse voltage
flows somewhere) I think it is safe to include the LDO bypass supply
limits as the lower bound in the DT constraints, even if this mode
isn't currently used anywhere.

Regards,
Lucas


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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-03 11:44                 ` Lucas Stach
@ 2019-12-03 12:37                   ` Stefan Riedmüller
  2019-12-05  7:19                     ` Marco Felsch
  0 siblings, 1 reply; 16+ messages in thread
From: Stefan Riedmüller @ 2019-12-03 12:37 UTC (permalink / raw)
  To: Lucas Stach, Marco Felsch
  Cc: festevam, shawnguo, chf.fritz, robh+dt, linux-imx, kernel,
	c.hemp, s.christ, linux-arm-kernel

Hi Lucas, hi Marco,

On 03.12.19 12:44, Lucas Stach wrote:
> On Di, 2019-12-03 at 09:33 +0100, Marco Felsch wrote:
> [...]
>>>> 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.
>>>
>>> Thanks, that makes more sense.
>>>
>>>>> 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.
>>>
>>> I don't agree. The datasheet you mention is the i.MX 6 datasheet and thus
>>> the limitation should reside in the i.MX 6 regulators and not in the PMIC's.
>>> This limitation is not just valid in combination with that PMIC but for all
>>> i.MX 6.
>>
>> The datasheet tells you which voltage should be applied to the imx6 and
>> so you have to set this here. What happens if the internally ldo
>> request a voltage value below 0.9V? Then the value will be applied
>> because we specified 0.73V and the system don't work anymore or did you
>> verified that case?
> 
> The DT constraints are supposed to reflect absolute maximum ratings of
> the board design. The regulator driver already knows the limits of the
> PMIC chip, so there is no point in duplicating this information in the
> DT.
> 
> The DT constraints are there to make sure the regulator core can
> constrain the voltage setting to something safe for the specific
> design. A consumer driver bug must never be able to set a voltage that
> is outside of the absolute maximum ratings of _all_ consumers of this
> specific power rail. I know that a lot of DTs get this detail wrong,
> but we shouldn't block patches to fix this. :)

Thanks for the clarification and the example. I got it now and will keep it 
in mind for the future.

> 
>>> If I have this wrong and the maintainers agree with you could you please
>>> make sure to account for the bypass mode as well since these values from the
>>> datasheet are valid too?
>>
>> As I said, the bypass mode isn't supported by the driver and all imx6
>> based devicetrees follow that case. So we don't have to take that into
>> account. Also we can't meet both contraints with one dt and futhermore
>> the bypass mode decrease your imx6 lifetime due the the increased ripple
>> on the arm-core supply. So I think no one wants this setup in the near
>> future.
> 
> As a violation of the minimum voltage setting is very unlikely to cause
> any permanent damage to the design (expect if you got reverse voltage
> flows somewhere) I think it is safe to include the LDO bypass supply
> limits as the lower bound in the DT constraints, even if this mode
> isn't currently used anywhere.

Sounds good to me.

Regards,
Stefan

> 
> Regards,
> Lucas
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage
  2019-12-03 12:37                   ` Stefan Riedmüller
@ 2019-12-05  7:19                     ` Marco Felsch
  0 siblings, 0 replies; 16+ messages in thread
From: Marco Felsch @ 2019-12-05  7:19 UTC (permalink / raw)
  To: Stefan Riedmüller
  Cc: festevam, shawnguo, chf.fritz, robh+dt, linux-imx, kernel,
	c.hemp, s.christ, linux-arm-kernel, Lucas Stach

Hi Stefan,

On 19-12-03 13:37, Stefan Riedmüller wrote:
> Hi Lucas, hi Marco,
> 
> On 03.12.19 12:44, Lucas Stach wrote:

...

> > 
> > The DT constraints are supposed to reflect absolute maximum ratings of
> > the board design. The regulator driver already knows the limits of the
> > PMIC chip, so there is no point in duplicating this information in the
> > DT.
> > 
> > The DT constraints are there to make sure the regulator core can
> > constrain the voltage setting to something safe for the specific
> > design. A consumer driver bug must never be able to set a voltage that
> > is outside of the absolute maximum ratings of _all_ consumers of this
> > specific power rail. I know that a lot of DTs get this detail wrong,
> > but we shouldn't block patches to fix this. :)
> 
> Thanks for the clarification and the example. I got it now and will keep it
> in mind for the future.
> 
> > 
> > > > If I have this wrong and the maintainers agree with you could you please
> > > > make sure to account for the bypass mode as well since these values from the
> > > > datasheet are valid too?
> > > 
> > > As I said, the bypass mode isn't supported by the driver and all imx6
> > > based devicetrees follow that case. So we don't have to take that into
> > > account. Also we can't meet both contraints with one dt and futhermore
> > > the bypass mode decrease your imx6 lifetime due the the increased ripple
> > > on the arm-core supply. So I think no one wants this setup in the near
> > > future.
> > 
> > As a violation of the minimum voltage setting is very unlikely to cause
> > any permanent damage to the design (expect if you got reverse voltage
> > flows somewhere) I think it is safe to include the LDO bypass supply
> > limits as the lower bound in the DT constraints, even if this mode
> > isn't currently used anywhere.
> 
> Sounds good to me.

Okay, I will use the bypass value and add a comment. Stefan can you take
a look at the other patches as well?

Regards,
  Marco

> Regards,
> Stefan
> 
> > 
> > Regards,
> > Lucas
> > 
> 

-- 
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

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH 2/3] ARM: dts: imx6: phycore-som: fix emmc supply
  2019-11-29 16:48 ` [PATCH 2/3] ARM: dts: imx6: phycore-som: fix emmc supply Marco Felsch
@ 2019-12-05 12:00   ` Stefan Riedmüller
  0 siblings, 0 replies; 16+ messages in thread
From: Stefan Riedmüller @ 2019-12-05 12:00 UTC (permalink / raw)
  To: Marco Felsch
  Cc: s.christ, chf.fritz, robh+dt, linux-imx, kernel, c.hemp,
	shawnguo, festevam, linux-arm-kernel

Hi Marco,

On 29.11.19 17:48, Marco Felsch wrote:
> Currently the vmmc is supplied by the 1.8V pmic rail but this is wrong.
> The 1.8V pmic rail is connected to the emmc vccq (vqmmc).

I just checked the schematics again and actually both VCC and VCCQ are 
connected to the 3.3V power rail. VCCQ can be connected to the 1.8V PMIC 
power rail by solder jumper but default is 3.3V.

So I think either both should be connected to a fixed 3.3V regulator or 
removed, since the default system does not support switching these voltages.

Regards,
Stefan

> 
> 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 | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> index 46d4953c5588..44e333848b4d 100644
> --- a/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> +++ b/arch/arm/boot/dts/imx6qdl-phytec-phycore-som.dtsi
> @@ -183,7 +183,7 @@
>   	pinctrl-0 = <&pinctrl_usdhc4>;
>   	bus-width = <8>;
>   	non-removable;
> -	vmmc-supply = <&vdd_emmc_1p8>;
> +	vqmmc-supply = <&vdd_emmc_1p8>;
>   	status = "disabled";
>   };
>   
> 

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

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-29 16:48 [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage Marco Felsch
2019-11-29 16:48 ` [PATCH 2/3] ARM: dts: imx6: phycore-som: fix emmc supply Marco Felsch
2019-12-05 12:00   ` Stefan Riedmüller
2019-11-29 16:48 ` [PATCH 3/3] ARM: dts: imx6: phycore-som: add pmic onkey device Marco Felsch
2019-12-02 10:09 ` [PATCH 1/3] ARM: dts: imx6: phycore-som: fix arm and soc minimum voltage Stefan Riedmüller
2019-12-02 12:42   ` Marco Felsch
2019-12-02 13:55     ` Stefan Riedmüller
2019-12-02 14:14       ` Marco Felsch
2019-12-02 14:30         ` Stefan Riedmüller
2019-12-02 14:53           ` Marco Felsch
2019-12-03  8:11             ` Stefan Riedmüller
2019-12-03  8:33               ` Marco Felsch
2019-12-03  9:07                 ` Stefan Riedmüller
2019-12-03 11:44                 ` Lucas Stach
2019-12-03 12:37                   ` Stefan Riedmüller
2019-12-05  7:19                     ` Marco Felsch

Linux-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git