| Message ID | 1529930051-14122-1-git-send-email-yibin.gong@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Acked-by: Anson Huang <anson.huang@nxp.com> I also saw such issue on i.MX6SLL evk board, and also sent out patch for i.MX6SLL. Anson Huang Best Regards! > -----Original Message----- > From: Robin Gong > Sent: Monday, June 25, 2018 8:34 PM > To: shawnguo@kernel.org; kernel@pengutronix.de; Fabio Estevam > <fabio.estevam@nxp.com>; robh+dt@kernel.org; mark.rutland@arm.com; > Anson Huang <anson.huang@nxp.com> > Cc: linux-arm-kernel@lists.infradead.org; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: [PATCH v1] ARM: dts: imx6sl-evk: keep sw4 always on > > SW4 is one power rail for LPDDR2 on i.mx6sl-evk, so it should be kept always on. > But it's disabled after switch disabled interface implemented in pfuze driver > 'commit 5fe156f1cab4 > ("regulator: pfuze100: add enable/disable for switch")'.Thus, it breaks kernel > bootup. Add 'regulator-always-on' for SW4. > > Signed-off-by: Robin Gong <yibin.gong@nxp.com> > --- > arch/arm/boot/dts/imx6sl-evk.dts | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/boot/dts/imx6sl-evk.dts > b/arch/arm/boot/dts/imx6sl-evk.dts > index 37e792f..798df66 100644 > --- a/arch/arm/boot/dts/imx6sl-evk.dts > +++ b/arch/arm/boot/dts/imx6sl-evk.dts > @@ -199,6 +199,7 @@ > sw4_reg: sw4 { > regulator-min-microvolt = <800000>; > regulator-max-microvolt = <3300000>; > + regulator-always-on; > }; > > swbst_reg: swbst { > -- > 2.7.4
On Mon, Jun 25, 2018 at 08:34:11PM +0800, Robin Gong wrote: > SW4 is one power rail for LPDDR2 on i.mx6sl-evk, so it should > be kept always on. But it's disabled after switch disabled > interface implemented in pfuze driver > 'commit 5fe156f1cab4 > ("regulator: pfuze100: add enable/disable for switch")'.Thus, > it breaks kernel bootup. Add 'regulator-always-on' for SW4. > > Signed-off-by: Robin Gong <yibin.gong@nxp.com> Does that mean boards with existing DTB installed will stop working with new kernel? That's bad, and the kernel commit should probably be reverted. Shawn > --- > arch/arm/boot/dts/imx6sl-evk.dts | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/arm/boot/dts/imx6sl-evk.dts b/arch/arm/boot/dts/imx6sl-evk.dts > index 37e792f..798df66 100644 > --- a/arch/arm/boot/dts/imx6sl-evk.dts > +++ b/arch/arm/boot/dts/imx6sl-evk.dts > @@ -199,6 +199,7 @@ > sw4_reg: sw4 { > regulator-min-microvolt = <800000>; > regulator-max-microvolt = <3300000>; > + regulator-always-on; > }; > > swbst_reg: swbst { > -- > 2.7.4 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Sun, Jul 1, 2018 at 6:34 AM, Shawn Guo <shawnguo@kernel.org> wrote: > On Mon, Jun 25, 2018 at 08:34:11PM +0800, Robin Gong wrote: >> SW4 is one power rail for LPDDR2 on i.mx6sl-evk, so it should >> be kept always on. But it's disabled after switch disabled >> interface implemented in pfuze driver >> 'commit 5fe156f1cab4 >> ("regulator: pfuze100: add enable/disable for switch")'.Thus, >> it breaks kernel bootup. Add 'regulator-always-on' for SW4. >> >> Signed-off-by: Robin Gong <yibin.gong@nxp.com> > > Does that mean boards with existing DTB installed will stop working with > new kernel? That's bad, and the kernel commit should probably be > reverted. Yes, this is a good point. Anson, Should 5fe156f1cab4 ("regulator: pfuze100: add enable/disable for switch") be reverted to avoid such breakage?
Anson Huang Best Regards! > -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Monday, July 2, 2018 7:33 AM > To: Shawn Guo <shawnguo@kernel.org>; Anson Huang > <anson.huang@nxp.com> > Cc: Robin Gong <yibin.gong@nxp.com>; Mark Rutland > <mark.rutland@arm.com>; open list:OPEN FIRMWARE AND FLATTENED > DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; linux-kernel > <linux-kernel@vger.kernel.org>; Rob Herring <robh+dt@kernel.org>; > dl-linux-imx <linux-imx@nxp.com>; Sascha Hauer <kernel@pengutronix.de>; > Fabio Estevam <fabio.estevam@nxp.com>; moderated list:ARM/FREESCALE > IMX / MXC ARM ARCHITECTURE <linux-arm-kernel@lists.infradead.org> > Subject: Re: [PATCH v1] ARM: dts: imx6sl-evk: keep sw4 always on > > On Sun, Jul 1, 2018 at 6:34 AM, Shawn Guo <shawnguo@kernel.org> wrote: > > On Mon, Jun 25, 2018 at 08:34:11PM +0800, Robin Gong wrote: > >> SW4 is one power rail for LPDDR2 on i.mx6sl-evk, so it should be kept > >> always on. But it's disabled after switch disabled interface > >> implemented in pfuze driver 'commit 5fe156f1cab4 > >> ("regulator: pfuze100: add enable/disable for switch")'.Thus, it > >> breaks kernel bootup. Add 'regulator-always-on' for SW4. > >> > >> Signed-off-by: Robin Gong <yibin.gong@nxp.com> > > > > Does that mean boards with existing DTB installed will stop working > > with new kernel? That's bad, and the kernel commit should probably be > > reverted. > > Yes, this is a good point. > > Anson, > > Should 5fe156f1cab4 ("regulator: pfuze100: add enable/disable for > switch") be reverted to avoid such breakage? Yes, I think we can revert it to avoid breakage. Didn't notice that some i.MX platform do NOT have those critical switches always-on. Anson.
Hi Anson, On Sun, Jul 1, 2018 at 9:53 PM, Anson Huang <anson.huang@nxp.com> wrote: > Yes, I think we can revert it to avoid breakage. Didn't notice that some > i.MX platform do NOT have those critical switches always-on. Ok, thanks for confirming. I will send a revert patch then. Thanks
Hi, Fabio Anson Huang Best Regards! > -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Monday, July 2, 2018 8:55 AM > To: Anson Huang <anson.huang@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org>; Robin Gong <yibin.gong@nxp.com>; > Mark Rutland <mark.rutland@arm.com>; open list:OPEN FIRMWARE AND > FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > linux-kernel <linux-kernel@vger.kernel.org>; Rob Herring > <robh+dt@kernel.org>; dl-linux-imx <linux-imx@nxp.com>; Sascha Hauer > <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>; > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE > <linux-arm-kernel@lists.infradead.org> > Subject: Re: [PATCH v1] ARM: dts: imx6sl-evk: keep sw4 always on > > Hi Anson, > > On Sun, Jul 1, 2018 at 9:53 PM, Anson Huang <anson.huang@nxp.com> wrote: > > > Yes, I think we can revert it to avoid breakage. Didn't notice that > > some i.MX platform do NOT have those critical switches always-on. > > Ok, thanks for confirming. > > I will send a revert patch then. > > Thanks Just want to know how to handle such case? The kernel patch will never be applied or is there any way to make kernel patch and dtb patch applied together to avoid any breakage? Anson.
Hi Anson, On Sun, Jul 1, 2018 at 9:57 PM, Anson Huang <anson.huang@nxp.com> wrote: > Just want to know how to handle such case? The kernel patch will never > be applied or is there any way to make kernel patch and dtb patch applied > together to avoid any breakage? We always want to avoid breaking a working dtb when it is used with a newer kernel. In this case we need to revert the kernel patch as it causes regression with old dtbs.
Anson Huang Best Regards! > -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Monday, July 2, 2018 9:00 AM > To: Anson Huang <anson.huang@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org>; Robin Gong <yibin.gong@nxp.com>; > Mark Rutland <mark.rutland@arm.com>; open list:OPEN FIRMWARE AND > FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > linux-kernel <linux-kernel@vger.kernel.org>; Rob Herring > <robh+dt@kernel.org>; dl-linux-imx <linux-imx@nxp.com>; Sascha Hauer > <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>; > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE > <linux-arm-kernel@lists.infradead.org> > Subject: Re: [PATCH v1] ARM: dts: imx6sl-evk: keep sw4 always on > > Hi Anson, > > On Sun, Jul 1, 2018 at 9:57 PM, Anson Huang <anson.huang@nxp.com> wrote: > > > Just want to know how to handle such case? The kernel patch will never > > be applied or is there any way to make kernel patch and dtb patch > > applied together to avoid any breakage? > > We always want to avoid breaking a working dtb when it is used with a newer > kernel. > > In this case we need to revert the kernel patch as it causes regression with old > dtbs. So that mean such kind of kernel patch will never be into kernel? Even if it is a necessary patch for fixing some other issues? I just wonder how this case being handled. Anson.
On Sun, Jul 1, 2018 at 10:03 PM, Anson Huang <anson.huang@nxp.com> wrote: > So that mean such kind of kernel patch will never be into kernel? Even if it is a > necessary patch for fixing some other issues? I just wonder how this case being > handled. What is the issue that 5fe156f1cab4f fixes? It is not clear by looking at the commit log.
Anson Huang Best Regards! > -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Monday, July 2, 2018 9:05 AM > To: Anson Huang <anson.huang@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org>; Robin Gong <yibin.gong@nxp.com>; > Mark Rutland <mark.rutland@arm.com>; open list:OPEN FIRMWARE AND > FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > linux-kernel <linux-kernel@vger.kernel.org>; Rob Herring > <robh+dt@kernel.org>; dl-linux-imx <linux-imx@nxp.com>; Sascha Hauer > <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>; > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE > <linux-arm-kernel@lists.infradead.org> > Subject: Re: [PATCH v1] ARM: dts: imx6sl-evk: keep sw4 always on > > On Sun, Jul 1, 2018 at 10:03 PM, Anson Huang <anson.huang@nxp.com> > wrote: > > > So that mean such kind of kernel patch will never be into kernel? Even > > if it is a necessary patch for fixing some other issues? I just wonder > > how this case being handled. > > What is the issue that 5fe156f1cab4f fixes? It is not clear by looking at the > commit log. On some new i.MX platforms, PFuze switches are used for supplying GPU/VPU or other non-critical modules only, these switches need to be turned off by runtime PM to avoid very high power leakage, like on mScale850D. Anson
On Sun, Jul 1, 2018 at 10:09 PM, Anson Huang <anson.huang@nxp.com> wrote: > On some new i.MX platforms, PFuze switches are used for supplying GPU/VPU > or other non-critical modules only, these switches need to be turned off by > runtime PM to avoid very high power leakage, like on mScale850D. Ok, in this case I suggest adding a new property so that the switches can be turned off only when the new property is present. When this new property is absent, then we keep the current behavior and avoid dtb breakage. Since MX8M support is not in place yet, this is not urgent, so I will send a revert and then you can re-work the patch so that it does not affect the old dtbs. Do you agree with such approach?
Anson Huang Best Regards! > -----Original Message----- > From: Fabio Estevam [mailto:festevam@gmail.com] > Sent: Monday, July 2, 2018 9:17 AM > To: Anson Huang <anson.huang@nxp.com> > Cc: Shawn Guo <shawnguo@kernel.org>; Robin Gong <yibin.gong@nxp.com>; > Mark Rutland <mark.rutland@arm.com>; open list:OPEN FIRMWARE AND > FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > linux-kernel <linux-kernel@vger.kernel.org>; Rob Herring > <robh+dt@kernel.org>; dl-linux-imx <linux-imx@nxp.com>; Sascha Hauer > <kernel@pengutronix.de>; Fabio Estevam <fabio.estevam@nxp.com>; > moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE > <linux-arm-kernel@lists.infradead.org> > Subject: Re: [PATCH v1] ARM: dts: imx6sl-evk: keep sw4 always on > > On Sun, Jul 1, 2018 at 10:09 PM, Anson Huang <anson.huang@nxp.com> > wrote: > > > On some new i.MX platforms, PFuze switches are used for supplying > > GPU/VPU or other non-critical modules only, these switches need to be > > turned off by runtime PM to avoid very high power leakage, like on > mScale850D. > > Ok, in this case I suggest adding a new property so that the switches can be > turned off only when the new property is present. > > When this new property is absent, then we keep the current behavior and avoid > dtb breakage. > > Since MX8M support is not in place yet, this is not urgent, so I will send a revert > and then you can re-work the patch so that it does not affect the old dtbs. > > Do you agree with such approach? Sure, I agree for now. As I did NOT want to have any breakage. Thanks. Anson.
On 日, 2018-07-01 at 22:17 -0300, Fabio Estevam wrote: > On Sun, Jul 1, 2018 at 10:09 PM, Anson Huang <anson.huang@nxp.com> > wrote: > > > > > On some new i.MX platforms, PFuze switches are used for supplying > > GPU/VPU > > or other non-critical modules only, these switches need to be > > turned off by > > runtime PM to avoid very high power leakage, like on mScale850D. > Ok, in this case I suggest adding a new property so that the switches > can be turned off only when the new property is present. > > When this new property is absent, then we keep the current behavior > and avoid dtb breakage. > > Since MX8M support is not in place yet, this is not urgent, so I will > send a revert and then you can re-work the patch so that it does not > affect the old dtbs. > > Do you agree with such approach? But in fact, the original dts is not correct without 'regulator-always- on'since SW4 is the critical DDR power rail, although, it's kept on in the previous kernel by no switches enable/disable interfaces provided in pfuze driver. Adding new property which can be done totally by the common 'regulator-always-on' is not a good choice. Keep the dts patch adding 'regulator-always-on' ahead of pfuze driver pach adding enable/disable interface is enough for such case I think.
Hi Robin, On Sun, Jul 1, 2018 at 11:12 PM, Robin Gong <yibin.gong@nxp.com> wrote: > But in fact, the original dts is not correct without 'regulator-always- > on'since SW4 is the critical DDR power rail, although, it's kept on in > the previous kernel by no switches enable/disable interfaces provided > in pfuze driver. Adding new property which can be done totally by the > common 'regulator-always-on' is not a good choice. Keep the dts patch > adding 'regulator-always-on' ahead of pfuze driver pach adding > enable/disable interface is enough for such case I think. We really want to avoid breaking old dtb's running a new kernel.
On Mon, Jul 02, 2018 at 02:12:52AM +0000, Robin Gong wrote: > But in fact, the original dts is not correct without 'regulator-always- > on'since SW4 is the critical DDR power rail, although, it's kept on in > the previous kernel by no switches enable/disable interfaces provided > in pfuze driver. Adding new property which can be done totally by the > common 'regulator-always-on' is not a good choice. Keep the dts patch > adding 'regulator-always-on' ahead of pfuze driver pach adding > enable/disable interface is enough for such case I think. We can not just break the installed DTBs like this. If patching regulator driver with a new property is really difficult, we could migrate the existing users in a 'soft' way: - Add required regulator-always-on for regulator nodes in DTS. - Patch i.MX platform code to check the presence of regulator-always-on property for critical regulators, and give a big warning if it's missing. - Wait for a couple of release cycles for users to migrate. - Add regulator driver patch back and break users who keep ignoring the warning. Shawn
Hi, Shawn Anson Huang Best Regards! > -----Original Message----- > From: Shawn Guo [mailto:shawnguo@kernel.org] > Sent: Tuesday, July 3, 2018 1:39 PM > To: Robin Gong <yibin.gong@nxp.com> > Cc: festevam@gmail.com; Anson Huang <anson.huang@nxp.com>; > mark.rutland@arm.com; devicetree@vger.kernel.org; > linux-kernel@vger.kernel.org; robh+dt@kernel.org; dl-linux-imx > <linux-imx@nxp.com>; kernel@pengutronix.de; Fabio Estevam > <fabio.estevam@nxp.com>; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH v1] ARM: dts: imx6sl-evk: keep sw4 always on > > On Mon, Jul 02, 2018 at 02:12:52AM +0000, Robin Gong wrote: > > But in fact, the original dts is not correct without > > 'regulator-always- on'since SW4 is the critical DDR power rail, > > although, it's kept on in the previous kernel by no switches > > enable/disable interfaces provided in pfuze driver. Adding new > > property which can be done totally by the common 'regulator-always-on' > > is not a good choice. Keep the dts patch adding 'regulator-always-on' > > ahead of pfuze driver pach adding enable/disable interface is enough for such > case I think. > > We can not just break the installed DTBs like this. If patching regulator driver > with a new property is really difficult, we could migrate the existing users in a > 'soft' way: Patching regulator driver needs to add property for those regulators can be OFF, it will make users confuse with original regulator framework knowledge, NOT a good idea. > > - Add required regulator-always-on for regulator nodes in DTS. I & Yibin already sent out patch to add " regulator-always-on " for regulator nodes in DTS, so they can be applied first? > - Patch i.MX platform code to check the presence of regulator-always-on > property for critical regulators, and give a big warning if it's > missing. It is NOT easy to identify which switch is critical or NOT, and different platforms have different board design, it will introduce many platform specified code, so I think just revert the pfuze100 switch enable/disable patch should be OK for now. > - Wait for a couple of release cycles for users to migrate. > - Add regulator driver patch back and break users who keep ignoring > the warning. After a couple of release cycles, add the pfuze100 switch enable/disable patch back to support this feature, I believe users should switch to new dtb with "regulator-always-on" existing already. Anson. > > Shawn
Hi Anson, On Tue, Jul 3, 2018 at 4:44 AM, Anson Huang <anson.huang@nxp.com> wrote: > It is NOT easy to identify which switch is critical or NOT, and different platforms > have different board design, it will introduce many platform specified code, so I think > just revert the pfuze100 switch enable/disable patch should be OK for now. I have sent the pfuze100 regulator patch revert and it is linux-next now. Should probably reach 4.18-rc4. > After a couple of release cycles, add the pfuze100 switch enable/disable patch > back to support this feature, I believe users should switch to new dtb with "regulator-always-on" > existing already. That will still break old dtb compatibility. You cannot force users to use "regulator-always-on" and the old dtbs need to always work. So whatever new feature you need to introduce it needs to be done in such a way that the existing dtb's will continue working.
On 二, 2018-07-03 at 08:10 -0300, Fabio Estevam wrote: > Hi Anson, > > On Tue, Jul 3, 2018 at 4:44 AM, Anson Huang <anson.huang@nxp.com> > wrote: > > > > > It is NOT easy to identify which switch is critical or NOT, and > > different platforms > > have different board design, it will introduce many platform > > specified code, so I think > > just revert the pfuze100 switch enable/disable patch should be OK > > for now. > I have sent the pfuze100 regulator patch revert and it is linux-next > now. Should probably reach 4.18-rc4. > > > > > After a couple of release cycles, add the pfuze100 switch > > enable/disable patch > > back to support this feature, I believe users should switch to new > > dtb with "regulator-always-on" > > existing already. > That will still break old dtb compatibility. > > You cannot force users to use "regulator-always-on" and the old dtbs > need to always work. > > So whatever new feature you need to introduce it needs to be done in > such a way that the existing dtb's will continue working. But actually existing dtb is not right since the critical power rail missing 'regulator-always-on'. It's a fix patch for dts, not related with following dtb/kernel break rules, just a simple dts patch. Why should we make promise for the wrong dtbs?
Hi, On Wed, 4 Jul 2018 01:42:54 +0000 Robin Gong wrote: > On 二, 2018-07-03 at 08:10 -0300, Fabio Estevam wrote: > > Hi Anson, > > > > On Tue, Jul 3, 2018 at 4:44 AM, Anson Huang <anson.huang@nxp.com> > > wrote: > > > > > > > > It is NOT easy to identify which switch is critical or NOT, and > > > different platforms > > > have different board design, it will introduce many platform > > > specified code, so I think > > > just revert the pfuze100 switch enable/disable patch should be OK > > > for now. > > I have sent the pfuze100 regulator patch revert and it is linux-next > > now. Should probably reach 4.18-rc4. > > > > > > > > After a couple of release cycles, add the pfuze100 switch > > > enable/disable patch > > > back to support this feature, I believe users should switch to new > > > dtb with "regulator-always-on" > > > existing already. > > That will still break old dtb compatibility. > > > > You cannot force users to use "regulator-always-on" and the old dtbs > > need to always work. > > > > So whatever new feature you need to introduce it needs to be done in > > such a way that the existing dtb's will continue working. > But actually existing dtb is not right since the critical power rail > missing 'regulator-always-on'. It's a fix patch for dts, not related > with following dtb/kernel break rules, just a simple dts patch. Why > should we make promise for the wrong dtbs? > Because they are living in the outside world on real devices. Lothar Waßmann
diff --git a/arch/arm/boot/dts/imx6sl-evk.dts b/arch/arm/boot/dts/imx6sl-evk.dts index 37e792f..798df66 100644 --- a/arch/arm/boot/dts/imx6sl-evk.dts +++ b/arch/arm/boot/dts/imx6sl-evk.dts @@ -199,6 +199,7 @@ sw4_reg: sw4 { regulator-min-microvolt = <800000>; regulator-max-microvolt = <3300000>; + regulator-always-on; }; swbst_reg: swbst {
SW4 is one power rail for LPDDR2 on i.mx6sl-evk, so it should be kept always on. But it's disabled after switch disabled interface implemented in pfuze driver 'commit 5fe156f1cab4 ("regulator: pfuze100: add enable/disable for switch")'.Thus, it breaks kernel bootup. Add 'regulator-always-on' for SW4. Signed-off-by: Robin Gong <yibin.gong@nxp.com> --- arch/arm/boot/dts/imx6sl-evk.dts | 1 + 1 file changed, 1 insertion(+)