Re: [PATCH] Revert "ARM: dts: sunxi: Add regulators for Sinovoip BPI-M2"

[Emmanuel Vadot]

On 2018-02-05 10:05, Icenowy Zheng wrote:
> ä 2018å2æ5æ GMT+08:00 äå4:55:58, Emmanuel Vadot <manu@xxxxxxxxxxxxxxxx> 
> åå:
> > Hello,
> >
> > On Sat,  3 Feb 2018 19:23:53 +0800
> > Icenowy Zheng <icenowy@xxxxxxx> wrote:
> >
> > > This reverts commit 7daa213700758b5b08fc0daab09bb139dd334165.
> > >
> > > The original commit has several problems:
> > >
> > > - vdd-cpus and aldo3 (AVCC of the SoC) are not set to always-on,
> > which
> > > leads to system hang when disabling unused regulators.
> >
> > Indeed I should have make those always-on.
> >
> > > - GMAC (which uses dldo1 and aldo2) and Wi-Fi (which uses aldo1) are
> > not
> > > considered, and will fail to work after adding this commit.
> >
> > While I understand the problem with vdd-cpus and aldo3 I don't see why
> > when you don't declare regulator the code should do something with it.
> > DT is supposed to describe the hardware and the code should not use
> > hardware not described right ?
> > The gmac node doesn't declare any regulators and the mmc2 uses
> > reg_vcc3v0 (haven't checked on the schematics yet if it is correct).
>
> It's because the regulator support isn't present before
> this commit. However these parts really need special
> regulators. I don't have M2 schematics at hand, so you'd
> check it by yourself.

 Yes but why does the PMIC should disable regulators not defined in the 
DTS ? That the part I don't understand and want to know where it is 
described/documented.

> P.S. a proper device tree with AXP shouldn't use
> reg_vcc3v0/3v3/1v8/etc. They're dummy
> regulator nodes for
> not implemented or not controllable regulators.
>
> > > This indicates that this patch should be not tested at all.
> >
> > This have indeed not been tested with linux.
> > I think that this commit should not be reverted, I'll send a proper
> > patch tonight or tomorow night max.
>
> Please test patches sent to Linux on Linux :-)

 If my patches adhere to the bindings I don't see why.

> > P.S.: Also as I'm the original sender I think I should have been in CC
> > no ?
>
> get_maintainer.pl didn't mention you and I forgot... sorry.
>
> > Cheers,
> >
> > > Signed-off-by: Icenowy Zheng <icenowy@xxxxxxx>
> > > ---
> > >  arch/arm/boot/dts/sun6i-a31s-sinovoip-bpi-m2.dts | 57
> > ------------------------
> > >  1 file changed, 57 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/sun6i-a31s-sinovoip-bpi-m2.dts
> > b/arch/arm/boot/dts/sun6i-a31s-sinovoip-bpi-m2.dts
> > > index 51e6f1d21c32..a565316eb340 100644
> > > --- a/arch/arm/boot/dts/sun6i-a31s-sinovoip-bpi-m2.dts
> > > +++ b/arch/arm/boot/dts/sun6i-a31s-sinovoip-bpi-m2.dts
> > > @@ -86,10 +86,6 @@
> > >  	};
> > >  };
> > >
> > > -&cpu0 {
> > > -	cpu-supply = <&reg_dcdc3>;
> > > -};
> > > -
> > >  &ehci0 {
> > >  	status = "okay";
> > >  };
> > > @@ -155,17 +151,6 @@
> > >  	status = "okay";
> > >  };
> > >
> > > -&p2wi {
> > > -	status = "okay";
> > > -
> > > -	axp22x: pmic@68 {
> > > -		compatible = "x-powers,axp221";
> > > -		reg = <0x68>;
> > > -		interrupt-parent = <&nmi_intc>;
> > > -		interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> > > -	};
> > > -};
> > > -
> > >  &pio {
> > >  	gmac_phy_reset_pin_bpi_m2: gmac_phy_reset_pin@0 {
> > >  		pins = "PA21";
> > > @@ -191,48 +176,6 @@
> > >  	};
> > >  };
> > >
> > > -#include "axp22x.dtsi"
> > > -
> > > -&reg_dc5ldo {
> > > -	regulator-min-microvolt = <700000>;
> > > -	regulator-max-microvolt = <1320000>;
> > > -	regulator-name = "vdd-cpus";
> > > -};
> > > -
> > > -&reg_dcdc1 {
> > > -	regulator-always-on;
> > > -	regulator-min-microvolt = <3000000>;
> > > -	regulator-max-microvolt = <3000000>;
> > > -	regulator-name = "vdd-3v0";
> > > -};
> > > -
> > > -&reg_dcdc2 {
> > > -	regulator-min-microvolt = <700000>;
> > > -	regulator-max-microvolt = <1320000>;
> > > -	regulator-name = "vdd-gpu";
> > > -};
> > > -
> > > -&reg_dcdc3 {
> > > -	regulator-always-on;
> > > -	regulator-min-microvolt = <700000>;
> > > -	regulator-max-microvolt = <1320000>;
> > > -	regulator-name = "vdd-cpu";
> > > -};
> > > -
> > > -&reg_dcdc4 {
> > > -	regulator-always-on;
> > > -	regulator-min-microvolt = <700000>;
> > > -	regulator-max-microvolt = <1320000>;
> > > -	regulator-name = "vdd-sys-dll";
> > > -};
> > > -
> > > -&reg_dcdc5 {
> > > -	regulator-always-on;
> > > -	regulator-min-microvolt = <1500000>;
> > > -	regulator-max-microvolt = <1500000>;
> > > -	regulator-name = "vcc-dram";
> > > -};
> > > -
> > >  &uart0 {
> > >  	pinctrl-names = "default";
> > >  	pinctrl-0 = <&uart0_pins_a>;
> > > --
> > > 2.15.1
> > >
> > >
> > > _______________________________________________
> > > linux-arm-kernel mailing list
> > > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> > > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

--
Emmanuel Vadot <manu@xxxxxxxxxxxxxxxx> <manu@xxxxxxxxxxx>