[PATCH v2] arm64: dts: allwinner: h6: Use dummy regulator for Tanix TX6

[Clément Péron]

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 <war...@o2.pl>
Fixes: add1e27fb703 ("arm64: dts: allwinner: h6: Enable CPU opp tables for Tanix TX6")
Signed-off-by: Clément Péron <peron...@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

[Clément Péron]

Hi Maxime, Warpme,

On Tue, 28 Apr 2020 at 16:26, Clément Péron <peron...@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

[Robin Murphy]

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.

[Clément Péron]

Hi Robin,

On Tue, 28 Apr 2020 at 17:21, Robin Murphy <robin....@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

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

[Clément Péron]

Hi Maxime,

On Tue, 28 Apr 2020 at 18:45, Maxime Ripard <max...@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....@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

[Ondřej Jirman]

Hi Clément,

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

[Clément Péron]

Hi Ondrej,

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

<snip>

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.