linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6
@ 2020-04-28 14:26 Clément Péron
  2020-04-28 14:30 ` Clément Péron
                   ` (2 more replies)
  0 siblings, 3 replies; 9+ messages in thread
From: Clément Péron @ 2020-04-28 14:26 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Rob Herring
  Cc: devicetree, linux-kernel, linux-sunxi, Clément Péron,
	Piotr Oniszczuk, linux-arm-kernel

Tanix TX6 has a fixed regulator. As DVFS is instructed to change
voltage to meet OPP table, the DVFS is not working as expected.

Avoid to introduce a new dedicated OPP Table where voltage are
equals to the fixed regulator as it will only duplicate all the OPPs.
Instead remove the fixed regulator so the DVFS framework will create
dummy regulator and will have the same behavior.

Add some comments to explain this in the device-tree.

Reported-by: Piotr Oniszczuk <warpme@o2.pl>
Fixes: add1e27fb703 ("arm64: dts: allwinner: h6: Enable CPU opp tables for Tanix TX6")
Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 .../boot/dts/allwinner/sun50i-h6-tanix-tx6.dts | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
index be81330db14f..3e96fcb317ea 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
@@ -48,7 +48,15 @@
 };
 
 &cpu0 {
-	cpu-supply = <&reg_vdd_cpu_gpu>;
+	/*
+	 * Don't specify the CPU regulator, as it's a fixed
+	 * regulator DVFS will not work as it is intructed
+	 * to reach a voltage which can't be reached.
+	 * Not specifying a regulator will create a dummy
+	 * regulator allowing all OPPs.
+	 *
+	 * cpu-supply = <&reg_vdd_cpu_gpu>;
+	 */
 };
 
 &de {
@@ -68,7 +76,13 @@
 };
 
 &gpu {
-	mali-supply = <&reg_vdd_cpu_gpu>;
+	/*
+	 * Don't specify the GPU regulator, see comment
+	 * above for the CPU supply.
+	 *
+	 * mali-supply = <&reg_vdd_cpu_gpu>;
+	 */
+
 	status = "okay";
 };
 
-- 
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 related	[flat|nested] 9+ messages in thread

* Re: [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6
  2020-04-28 14:26 [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6 Clément Péron
@ 2020-04-28 14:30 ` Clément Péron
  2020-04-28 15:21 ` Robin Murphy
  2020-05-04 12:27 ` [linux-sunxi] " Ondřej Jirman
  2 siblings, 0 replies; 9+ messages in thread
From: Clément Péron @ 2020-04-28 14:30 UTC (permalink / raw)
  To: Maxime Ripard, Chen-Yu Tsai, Rob Herring
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Piotr Oniszczuk

Hi Maxime, Warpme,

On Tue, 28 Apr 2020 at 16:26, Clément Péron <peron.clem@gmail.com> wrote:
>
> Tanix TX6 has a fixed regulator. As DVFS is instructed to change
> voltage to meet OPP table, the DVFS is not working as expected.
>
> Avoid to introduce a new dedicated OPP Table where voltage are
> equals to the fixed regulator as it will only duplicate all the OPPs.
> Instead remove the fixed regulator so the DVFS framework will create
> dummy regulator and will have the same behavior.
>
> Add some comments to explain this in the device-tree.

Changes since v1:
I have change this patch to use dummy regulator and add comment about
this choice.
I think it's a bit better than just dropping the regulator.

@Piotr Oniszczuk:
Please add your tested-by tag, to be sure this is working as expected !

Thanks & Regards
Clement

>
> Reported-by: Piotr Oniszczuk <warpme@o2.pl>
> Fixes: add1e27fb703 ("arm64: dts: allwinner: h6: Enable CPU opp tables for Tanix TX6")
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  .../boot/dts/allwinner/sun50i-h6-tanix-tx6.dts | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> index be81330db14f..3e96fcb317ea 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> @@ -48,7 +48,15 @@
>  };
>
>  &cpu0 {
> -       cpu-supply = <&reg_vdd_cpu_gpu>;
> +       /*
> +        * Don't specify the CPU regulator, as it's a fixed
> +        * regulator DVFS will not work as it is intructed
> +        * to reach a voltage which can't be reached.
> +        * Not specifying a regulator will create a dummy
> +        * regulator allowing all OPPs.
> +        *
> +        * cpu-supply = <&reg_vdd_cpu_gpu>;
> +        */
>  };
>
>  &de {
> @@ -68,7 +76,13 @@
>  };
>
>  &gpu {
> -       mali-supply = <&reg_vdd_cpu_gpu>;
> +       /*
> +        * Don't specify the GPU regulator, see comment
> +        * above for the CPU supply.
> +        *
> +        * mali-supply = <&reg_vdd_cpu_gpu>;
> +        */
> +
>         status = "okay";
>  };
>
> --
> 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] 9+ messages in thread

* Re: [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6
  2020-04-28 14:26 [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6 Clément Péron
  2020-04-28 14:30 ` Clément Péron
@ 2020-04-28 15:21 ` Robin Murphy
  2020-04-28 16:23   ` [linux-sunxi] " Clément Péron
  2020-05-04 12:27 ` [linux-sunxi] " Ondřej Jirman
  2 siblings, 1 reply; 9+ messages in thread
From: Robin Murphy @ 2020-04-28 15:21 UTC (permalink / raw)
  To: Clément Péron, Maxime Ripard, Chen-Yu Tsai, Rob Herring
  Cc: devicetree, linux-sunxi, linux-kernel, linux-arm-kernel, Piotr Oniszczuk

On 2020-04-28 3:26 pm, Clément Péron wrote:
> Tanix TX6 has a fixed regulator. As DVFS is instructed to change
> voltage to meet OPP table, the DVFS is not working as expected.

Hmm, isn't that really a bug in the DVFS code? I guess it's just blindly 
propagating -EINVAL from the fixed regulators not implementing 
set_voltage, but AFAICS it has no real excuse not to be cleverer and 
still allow switching frequency as long as the voltage *is* high enough 
for the given OPP. I wonder how well it works if the regulator is 
programmable but shared with other consumers... that case probably can't 
be hacked around in DT.

Robin.

> Avoid to introduce a new dedicated OPP Table where voltage are
> equals to the fixed regulator as it will only duplicate all the OPPs.
> Instead remove the fixed regulator so the DVFS framework will create
> dummy regulator and will have the same behavior.
> 
> Add some comments to explain this in the device-tree.
> 
> Reported-by: Piotr Oniszczuk <warpme@o2.pl>
> Fixes: add1e27fb703 ("arm64: dts: allwinner: h6: Enable CPU opp tables for Tanix TX6")
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>   .../boot/dts/allwinner/sun50i-h6-tanix-tx6.dts | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> index be81330db14f..3e96fcb317ea 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> @@ -48,7 +48,15 @@
>   };
>   
>   &cpu0 {
> -	cpu-supply = <&reg_vdd_cpu_gpu>;
> +	/*
> +	 * Don't specify the CPU regulator, as it's a fixed
> +	 * regulator DVFS will not work as it is intructed
> +	 * to reach a voltage which can't be reached.
> +	 * Not specifying a regulator will create a dummy
> +	 * regulator allowing all OPPs.
> +	 *
> +	 * cpu-supply = <&reg_vdd_cpu_gpu>;
> +	 */
>   };
>   
>   &de {
> @@ -68,7 +76,13 @@
>   };
>   
>   &gpu {
> -	mali-supply = <&reg_vdd_cpu_gpu>;
> +	/*
> +	 * Don't specify the GPU regulator, see comment
> +	 * above for the CPU supply.
> +	 *
> +	 * mali-supply = <&reg_vdd_cpu_gpu>;
> +	 */
> +
>   	status = "okay";
>   };
>   
> 

_______________________________________________
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] 9+ messages in thread

* Re: [linux-sunxi] Re: [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6
  2020-04-28 15:21 ` Robin Murphy
@ 2020-04-28 16:23   ` Clément Péron
  2020-04-28 16:45     ` Maxime Ripard
  0 siblings, 1 reply; 9+ messages in thread
From: Clément Péron @ 2020-04-28 16:23 UTC (permalink / raw)
  To: Robin Murphy
  Cc: devicetree, Chen-Yu Tsai, Maxime Ripard, linux-kernel,
	linux-sunxi, Rob Herring, Piotr Oniszczuk, linux-arm-kernel

Hi Robin,

On Tue, 28 Apr 2020 at 17:21, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-04-28 3:26 pm, Clément Péron wrote:
> > Tanix TX6 has a fixed regulator. As DVFS is instructed to change
> > voltage to meet OPP table, the DVFS is not working as expected.
>
> Hmm, isn't that really a bug in the DVFS code? I guess it's just blindly
> propagating -EINVAL from the fixed regulators not implementing
> set_voltage, but AFAICS it has no real excuse not to be cleverer and
> still allow switching frequency as long as the voltage *is* high enough
> for the given OPP. I wonder how well it works if the regulator is
> programmable but shared with other consumers... that case probably can't
> be hacked around in DT.

Like you, I thought that the DVFS was clever enough to understand this
but guess not..

Maybe they are some cases where you don't want to leave the voltage high and
reduce the frequency. But I don't know such case.

Regards,
Clement




>
> Robin.
>
> > Avoid to introduce a new dedicated OPP Table where voltage are
> > equals to the fixed regulator as it will only duplicate all the OPPs.
> > Instead remove the fixed regulator so the DVFS framework will create
> > dummy regulator and will have the same behavior.
> >
> > Add some comments to explain this in the device-tree.
> >
> > Reported-by: Piotr Oniszczuk <warpme@o2.pl>
> > Fixes: add1e27fb703 ("arm64: dts: allwinner: h6: Enable CPU opp tables for Tanix TX6")
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >   .../boot/dts/allwinner/sun50i-h6-tanix-tx6.dts | 18 ++++++++++++++++--
> >   1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> > index be81330db14f..3e96fcb317ea 100644
> > --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> > +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> > @@ -48,7 +48,15 @@
> >   };
> >
> >   &cpu0 {
> > -     cpu-supply = <&reg_vdd_cpu_gpu>;
> > +     /*
> > +      * Don't specify the CPU regulator, as it's a fixed
> > +      * regulator DVFS will not work as it is intructed
> > +      * to reach a voltage which can't be reached.
> > +      * Not specifying a regulator will create a dummy
> > +      * regulator allowing all OPPs.
> > +      *
> > +      * cpu-supply = <&reg_vdd_cpu_gpu>;
> > +      */
> >   };
> >
> >   &de {
> > @@ -68,7 +76,13 @@
> >   };
> >
> >   &gpu {
> > -     mali-supply = <&reg_vdd_cpu_gpu>;
> > +     /*
> > +      * Don't specify the GPU regulator, see comment
> > +      * above for the CPU supply.
> > +      *
> > +      * mali-supply = <&reg_vdd_cpu_gpu>;
> > +      */
> > +
> >       status = "okay";
> >   };
> >
> >
>
> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/98246e5d-ebef-bcb5-f0b8-d74b3834b835%40arm.com.

_______________________________________________
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] 9+ messages in thread

* Re: [linux-sunxi] Re: [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6
  2020-04-28 16:23   ` [linux-sunxi] " Clément Péron
@ 2020-04-28 16:45     ` Maxime Ripard
  2020-04-30 13:48       ` Clément Péron
  0 siblings, 1 reply; 9+ messages in thread
From: Maxime Ripard @ 2020-04-28 16:45 UTC (permalink / raw)
  To: Clément Péron
  Cc: devicetree, linux-sunxi, linux-kernel, Chen-Yu Tsai, Rob Herring,
	Piotr Oniszczuk, Robin Murphy, linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1309 bytes --]

On Tue, Apr 28, 2020 at 06:23:35PM +0200, Clément Péron wrote:
> Hi Robin,
> 
> On Tue, 28 Apr 2020 at 17:21, Robin Murphy <robin.murphy@arm.com> wrote:
> >
> > On 2020-04-28 3:26 pm, Clément Péron wrote:
> > > Tanix TX6 has a fixed regulator. As DVFS is instructed to change
> > > voltage to meet OPP table, the DVFS is not working as expected.
> >
> > Hmm, isn't that really a bug in the DVFS code? I guess it's just blindly
> > propagating -EINVAL from the fixed regulators not implementing
> > set_voltage, but AFAICS it has no real excuse not to be cleverer and
> > still allow switching frequency as long as the voltage *is* high enough
> > for the given OPP. I wonder how well it works if the regulator is
> > programmable but shared with other consumers... that case probably can't
> > be hacked around in DT.
> 
> Like you, I thought that the DVFS was clever enough to understand this
> but guess not..
> 
> Maybe they are some cases where you don't want to leave the voltage high and
> reduce the frequency. But I don't know such case.

I assume the intent was to prevent a regulator driver to overshoot and end up
over-volting the CPU which would be pretty bad.

I guess we could check that the voltage is in the range opp < actual voltage <
max opp voltage ?

Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
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] 9+ messages in thread

* Re: [linux-sunxi] Re: [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6
  2020-04-28 16:45     ` Maxime Ripard
@ 2020-04-30 13:48       ` Clément Péron
  2020-05-04 16:40         ` Maxime Ripard
  0 siblings, 1 reply; 9+ messages in thread
From: Clément Péron @ 2020-04-30 13:48 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: devicetree, linux-sunxi, linux-kernel, Chen-Yu Tsai, Rob Herring,
	Piotr Oniszczuk, Robin Murphy, linux-arm-kernel

Hi Maxime,

On Tue, 28 Apr 2020 at 18:45, Maxime Ripard <maxime@cerno.tech> wrote:
>
> On Tue, Apr 28, 2020 at 06:23:35PM +0200, Clément Péron wrote:
> > Hi Robin,
> >
> > On Tue, 28 Apr 2020 at 17:21, Robin Murphy <robin.murphy@arm.com> wrote:
> > >
> > > On 2020-04-28 3:26 pm, Clément Péron wrote:
> > > > Tanix TX6 has a fixed regulator. As DVFS is instructed to change
> > > > voltage to meet OPP table, the DVFS is not working as expected.
> > >
> > > Hmm, isn't that really a bug in the DVFS code? I guess it's just blindly
> > > propagating -EINVAL from the fixed regulators not implementing
> > > set_voltage, but AFAICS it has no real excuse not to be cleverer and
> > > still allow switching frequency as long as the voltage *is* high enough
> > > for the given OPP. I wonder how well it works if the regulator is
> > > programmable but shared with other consumers... that case probably can't
> > > be hacked around in DT.
> >
> > Like you, I thought that the DVFS was clever enough to understand this
> > but guess not..
> >
> > Maybe they are some cases where you don't want to leave the voltage high and
> > reduce the frequency. But I don't know such case.
>
> I assume the intent was to prevent a regulator driver to overshoot and end up
> over-volting the CPU which would be pretty bad.
>
> I guess we could check that the voltage is in the range opp < actual voltage <
> max opp voltage ?

As this could take more time than expected,

Could you drop the commit :
add1e27fb703f65f33191ccc70dd9d811254387c
arm64: dts: allwinner: h6: Enable CPU opp tables for Tanix TX6

Thanks,
Clement

>
> Maxime

_______________________________________________
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] 9+ messages in thread

* Re: [linux-sunxi] [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6
  2020-04-28 14:26 [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6 Clément Péron
  2020-04-28 14:30 ` Clément Péron
  2020-04-28 15:21 ` Robin Murphy
@ 2020-05-04 12:27 ` Ondřej Jirman
  2020-05-04 19:34   ` Clément Péron
  2 siblings, 1 reply; 9+ messages in thread
From: Ondřej Jirman @ 2020-05-04 12:27 UTC (permalink / raw)
  To: Clément Péron
  Cc: devicetree, linux-sunxi, linux-kernel, Maxime Ripard,
	Chen-Yu Tsai, Rob Herring, Piotr Oniszczuk, linux-arm-kernel

Hi Clément,

On Tue, Apr 28, 2020 at 04:26:29PM +0200, Clément Péron wrote:
> Tanix TX6 has a fixed regulator. As DVFS is instructed to change
> voltage to meet OPP table, the DVFS is not working as expected.
> 
> Avoid to introduce a new dedicated OPP Table where voltage are
> equals to the fixed regulator as it will only duplicate all the OPPs.
> Instead remove the fixed regulator so the DVFS framework will create
> dummy regulator and will have the same behavior.
> 
> Add some comments to explain this in the device-tree.
> 
> Reported-by: Piotr Oniszczuk <warpme@o2.pl>
> Fixes: add1e27fb703 ("arm64: dts: allwinner: h6: Enable CPU opp tables for Tanix TX6")
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  .../boot/dts/allwinner/sun50i-h6-tanix-tx6.dts | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> index be81330db14f..3e96fcb317ea 100644
> --- a/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> +++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-tanix-tx6.dts
> @@ -48,7 +48,15 @@
>  };
>  
>  &cpu0 {
> -	cpu-supply = <&reg_vdd_cpu_gpu>;
> +	/*
> +	 * Don't specify the CPU regulator, as it's a fixed
> +	 * regulator DVFS will not work as it is intructed
> +	 * to reach a voltage which can't be reached.
> +	 * Not specifying a regulator will create a dummy
> +	 * regulator allowing all OPPs.
> +	 *
> +	 * cpu-supply = <&reg_vdd_cpu_gpu>;
> +	 */

reg_vdd_cpu_gpu has 

    regulator-min-microvolt = <1135000>;
    regulator-max-microvolt = <1135000>;

top OPP is:

    opp@1800000000 {
            clock-latency-ns = <244144>; /* 8 32k periods */
            opp-hz = /bits/ 64 <1800000000>;

            opp-microvolt-speed0 = <1160000>;
            opp-microvolt-speed1 = <1100000>;
            opp-microvolt-speed2 = <1100000>;
    };

So I guess ignoring the voltage and not disabling this OPP may or may not work
based on SoC bin.

On Orange Pi One, there's a regulator that supports two voltages (that can't
support all the listed OPPs for H3), and cpufreq-dt can deal with that
automagically, if you specify OPP voltages via a tripplet of [prefered min max].
Kernell will log this in dmesg on boot:

[    0.672440] core: _opp_supported_by_regulators: OPP minuV: 1320000 maxuV: 1320000, not supported by regulator
[    0.672454] cpu cpu0: _opp_add: OPP not supported by regulators (1104000000)
[    0.672523] core: _opp_supported_by_regulators: OPP minuV: 1320000 maxuV: 1320000, not supported by regulator
[    0.672530] cpu cpu0: _opp_add: OPP not supported by regulators (1200000000)
[    0.672621] core: _opp_supported_by_regulators: OPP minuV: 1340000 maxuV: 1340000, not supported by regulator
[    0.672628] cpu cpu0: _opp_add: OPP not supported by regulators (1296000000)
[    0.672712] core: _opp_supported_by_regulators: OPP minuV: 1400000 maxuV: 1400000, not supported by regulator
[    0.672719] cpu cpu0: _opp_add: OPP not supported by regulators (1368000000)

And the list of available OPPs will be reduced at runtime to a supportable
set by the CPU regulator.

If you look at:

  https://megous.com/git/linux/commit/?h=ths-5.7&id=d231770195913cf543c0cf9539deee2ecec06680

you'll see a bunch of OPPs for H3 that are specified as a range. So
for example if you want 480MHz, and your regulator can't produce
1.04V exactly, cpufreq will set the voltage to something supportable
in the range.

I think the proper fix is to fix the OPP table for H6, so that it uses
voltage ranges for each OPP and not a single fixed voltage, to support
boards that don't have the standard PMIC that goes with H6.

regards,
	o.

>  };
>  
>  &de {
> @@ -68,7 +76,13 @@
>  };
>  
>  &gpu {
> -	mali-supply = <&reg_vdd_cpu_gpu>;
> +	/*
> +	 * Don't specify the GPU regulator, see comment
> +	 * above for the CPU supply.
> +	 *
> +	 * mali-supply = <&reg_vdd_cpu_gpu>;
> +	 */
> +
>  	status = "okay";
>  };
>  
> -- 
> 2.20.1
> 
> -- 
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe@googlegroups.com.
> To view this discussion on the web, visit https://groups.google.com/d/msgid/linux-sunxi/20200428142629.8950-1-peron.clem%40gmail.com.

_______________________________________________
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] 9+ messages in thread

* Re: [linux-sunxi] Re: [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6
  2020-04-30 13:48       ` Clément Péron
@ 2020-05-04 16:40         ` Maxime Ripard
  0 siblings, 0 replies; 9+ messages in thread
From: Maxime Ripard @ 2020-05-04 16:40 UTC (permalink / raw)
  To: Clément Péron
  Cc: devicetree, linux-sunxi, linux-kernel, Chen-Yu Tsai, Rob Herring,
	Piotr Oniszczuk, Robin Murphy, linux-arm-kernel


[-- Attachment #1.1: Type: text/plain, Size: 1782 bytes --]

Hi,

On Thu, Apr 30, 2020 at 03:48:04PM +0200, Clément Péron wrote:
> On Tue, 28 Apr 2020 at 18:45, Maxime Ripard <maxime@cerno.tech> wrote:
> >
> > On Tue, Apr 28, 2020 at 06:23:35PM +0200, Clément Péron wrote:
> > > Hi Robin,
> > >
> > > On Tue, 28 Apr 2020 at 17:21, Robin Murphy <robin.murphy@arm.com> wrote:
> > > >
> > > > On 2020-04-28 3:26 pm, Clément Péron wrote:
> > > > > Tanix TX6 has a fixed regulator. As DVFS is instructed to change
> > > > > voltage to meet OPP table, the DVFS is not working as expected.
> > > >
> > > > Hmm, isn't that really a bug in the DVFS code? I guess it's just blindly
> > > > propagating -EINVAL from the fixed regulators not implementing
> > > > set_voltage, but AFAICS it has no real excuse not to be cleverer and
> > > > still allow switching frequency as long as the voltage *is* high enough
> > > > for the given OPP. I wonder how well it works if the regulator is
> > > > programmable but shared with other consumers... that case probably can't
> > > > be hacked around in DT.
> > >
> > > Like you, I thought that the DVFS was clever enough to understand this
> > > but guess not..
> > >
> > > Maybe they are some cases where you don't want to leave the voltage high and
> > > reduce the frequency. But I don't know such case.
> >
> > I assume the intent was to prevent a regulator driver to overshoot and end up
> > over-volting the CPU which would be pretty bad.
> >
> > I guess we could check that the voltage is in the range opp < actual voltage <
> > max opp voltage ?
> 
> As this could take more time than expected,
> 
> Could you drop the commit :
> add1e27fb703f65f33191ccc70dd9d811254387c
> arm64: dts: allwinner: h6: Enable CPU opp tables for Tanix TX6

It's done, thanks!
Maxime

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
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] 9+ messages in thread

* Re: [linux-sunxi] [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6
  2020-05-04 12:27 ` [linux-sunxi] " Ondřej Jirman
@ 2020-05-04 19:34   ` Clément Péron
  0 siblings, 0 replies; 9+ messages in thread
From: Clément Péron @ 2020-05-04 19:34 UTC (permalink / raw)
  To: Ondřej Jirman, Clément Péron, Maxime Ripard,
	Chen-Yu Tsai, Rob Herring, linux-arm-kernel, devicetree,
	linux-kernel, linux-sunxi, Piotr Oniszczuk

Hi Ondrej,

On Mon, 4 May 2020 at 14:27, Ondřej Jirman <megous@megous.com> wrote:
>
> Hi Clément,
>

<snip>

>
> So I guess ignoring the voltage and not disabling this OPP may or may not work
> based on SoC bin.
>
> On Orange Pi One, there's a regulator that supports two voltages (that can't
> support all the listed OPPs for H3), and cpufreq-dt can deal with that
> automagically, if you specify OPP voltages via a tripplet of [prefered min max].
> Kernell will log this in dmesg on boot:
>
> [    0.672440] core: _opp_supported_by_regulators: OPP minuV: 1320000 maxuV: 1320000, not supported by regulator
> [    0.672454] cpu cpu0: _opp_add: OPP not supported by regulators (1104000000)
> [    0.672523] core: _opp_supported_by_regulators: OPP minuV: 1320000 maxuV: 1320000, not supported by regulator
> [    0.672530] cpu cpu0: _opp_add: OPP not supported by regulators (1200000000)
> [    0.672621] core: _opp_supported_by_regulators: OPP minuV: 1340000 maxuV: 1340000, not supported by regulator
> [    0.672628] cpu cpu0: _opp_add: OPP not supported by regulators (1296000000)
> [    0.672712] core: _opp_supported_by_regulators: OPP minuV: 1400000 maxuV: 1400000, not supported by regulator
> [    0.672719] cpu cpu0: _opp_add: OPP not supported by regulators (1368000000)
>
> And the list of available OPPs will be reduced at runtime to a supportable
> set by the CPU regulator.
>
> If you look at:
>
>   https://megous.com/git/linux/commit/?h=ths-5.7&id=d231770195913cf543c0cf9539deee2ecec06680
>
> you'll see a bunch of OPPs for H3 that are specified as a range. So
> for example if you want 480MHz, and your regulator can't produce
> 1.04V exactly, cpufreq will set the voltage to something supportable
> in the range.
>
> I think the proper fix is to fix the OPP table for H6, so that it uses
> voltage ranges for each OPP and not a single fixed voltage, to support
> boards that don't have the standard PMIC that goes with H6.

Thanks for the suggestion and I agree with you, this is a good way to
keep the same OPP table for all the H6 devices and handle both board
with PMIC and with fixed regulator.

I will propose a patch.

Thanks clement

>
> regards,
>         o.

_______________________________________________
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] 9+ messages in thread

end of thread, other threads:[~2020-05-04 19:34 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-28 14:26 [PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6 Clément Péron
2020-04-28 14:30 ` Clément Péron
2020-04-28 15:21 ` Robin Murphy
2020-04-28 16:23   ` [linux-sunxi] " Clément Péron
2020-04-28 16:45     ` Maxime Ripard
2020-04-30 13:48       ` Clément Péron
2020-05-04 16:40         ` Maxime Ripard
2020-05-04 12:27 ` [linux-sunxi] " Ondřej Jirman
2020-05-04 19:34   ` Clément Péron

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).