Message-Id: <20180214201654.cb8cb64bf50b958a53ebebbd@bidouilliste.com>
Date:   Wed, 14 Feb 2018 20:16:54 +0100
From:   Emmanuel Vadot <manu@...ouilliste.com>
To:     Chen-Yu Tsai <wens@...e.org>
Cc:     devicetree <devicetree@...r.kernel.org>,
        Maxime Ripard <maxime.ripard@...tlin.com>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linux-sunxi <linux-sunxi@...glegroups.com>,
        linux-arm-kernel <linux-arm-kernel@...ts.infradead.org>,
        Icenowy Zheng <icenowy@...c.io>
Subject: Re: [PATCH] Revert
 "ARM: dts: sunxi: Add regulators for Sinovoip BPI-M2"

On Tue, 13 Feb 2018 18:36:24 +0800
Chen-Yu Tsai <wens@...e.org> wrote:

> On Sat, Feb 10, 2018 at 5:20 AM, Emmanuel Vadot <manu@...ouilliste.com> wrote:
> > On 2018-02-05 10:05, Icenowy Zheng wrote:
> >>
> >> ? 2018?2?5? GMT+08:00 ??4:55:58, Emmanuel Vadot <manu@...ouilliste.com>
> >> ??:
> >>>
> >>>
> >>> Hello,
> >>>
> >>> On Sat,  3 Feb 2018 19:23:53 +0800
> >>> Icenowy Zheng <icenowy@...c.io> 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.
> 
> They are defined. See axp22x.dtsi, which you included in your patch.
> 
> Now the system is free to do whatever it wants under the constraints
> of the device tree. Since you do not reference the regulator, the
> kernel is free to turn it off to save power.

 Yeah I realized that now and feel stupid ...
 Thanks for the clarification.

> >
> >> 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.
> 
> It adheres to the bindings, but does not accurately describe the
> hardware constraints.

 Is there a place where there is build of the linux kernel for
sunxi/multi7 and initrd available for download so I can add that to my
test bench ?

 Cheers,

> ChenYu
> 
> >
> >
> >>>
> >>> 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@...c.io>
> >>>> ---
> >>>>  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@...ts.infradead.org
> >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >>
> >>
> >> _______________________________________________
> >> linux-arm-kernel mailing list
> >> linux-arm-kernel@...ts.infradead.org
> >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
> >
> > --
> > Emmanuel Vadot <manu@...ouilliste.com> <manu@...ebsd.org>
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@...ts.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel


-- 
Emmanuel Vadot <manu@...ouilliste.com> <manu@...ebsd.org>

Powered by blists - more mailing lists