| Message ID | 89cc7192100bdc9ce546bf6000446e629457ebc1.1493138693.git.leonard.crestez@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Leonard, On Tue, Apr 25, 2017 at 1:57 PM, Leonard Crestez <leonard.crestez@nxp.com> wrote: > The board file for imx6sx-dbg overrides cpufreq operating points to use > higher voltages. This is done because the board has a shared rail for > VDD_ARM_IN and VDD_SOC_IN and when using LDO bypass the shared voltage > needs to be a value suitable for both ARM and SOC. > > This was introduced in: > > commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it default") > > This only only applies to LDO bypass mode, a feature not present in > upstream. When LDOs are enabled the effect is to use higher voltages than > necesarry for no good reason. > > Setting these higher voltages can make some boards fail to boot with ugly > semi-random crashes, reminiscent of memory corruption. These failures > happen the first time the lowest idle state is used. Remove the OPP > override in order to fix those crashes. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > > --- > It's not clear exactly why the crashes happen. Perhaps waking up from idle > draws more power than is available? Removing this override is a correct > change anyway so maybe there is no need to investigate deeper. Marek just sent a similar one a few minutes ago: http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/503230.html
On Tue, Apr 25, 2017 at 2:02 PM, Fabio Estevam <festevam@gmail.com> wrote: > Hi Leonard, > > On Tue, Apr 25, 2017 at 1:57 PM, Leonard Crestez > <leonard.crestez@nxp.com> wrote: >> The board file for imx6sx-dbg overrides cpufreq operating points to use >> higher voltages. This is done because the board has a shared rail for >> VDD_ARM_IN and VDD_SOC_IN and when using LDO bypass the shared voltage >> needs to be a value suitable for both ARM and SOC. >> >> This was introduced in: >> >> commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it default") >> >> This only only applies to LDO bypass mode, a feature not present in >> upstream. When LDOs are enabled the effect is to use higher voltages than >> necesarry for no good reason. >> >> Setting these higher voltages can make some boards fail to boot with ugly >> semi-random crashes, reminiscent of memory corruption. These failures >> happen the first time the lowest idle state is used. Remove the OPP >> override in order to fix those crashes. >> >> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >> >> --- >> It's not clear exactly why the crashes happen. Perhaps waking up from idle >> draws more power than is available? Removing this override is a correct >> change anyway so maybe there is no need to investigate deeper. > > Marek just sent a similar one a few minutes ago: > http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/503230.html Forgot to add Marek.
On Tue, 2017-04-25 at 14:02 -0300, Fabio Estevam wrote: > On Tue, Apr 25, 2017 at 2:02 PM, Fabio Estevam <festevam@gmail.com> wrote: > > > > Hi Leonard, > > > > On Tue, Apr 25, 2017 at 1:57 PM, Leonard Crestez > > <leonard.crestez@nxp.com> wrote: > > > > > > The board file for imx6sx-dbg overrides cpufreq operating points to use > > > higher voltages. This is done because the board has a shared rail for > > > VDD_ARM_IN and VDD_SOC_IN and when using LDO bypass the shared voltage > > > needs to be a value suitable for both ARM and SOC. > > > > > > This was introduced in: > > > > > > commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it default") > > > > > > This only only applies to LDO bypass mode, a feature not present in > > > upstream. When LDOs are enabled the effect is to use higher voltages than > > > necesarry for no good reason. > > > > > > Setting these higher voltages can make some boards fail to boot with ugly > > > semi-random crashes, reminiscent of memory corruption. These failures > > > happen the first time the lowest idle state is used. Remove the OPP > > > override in order to fix those crashes. > > > > > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > > > > > > --- > > > It's not clear exactly why the crashes happen. Perhaps waking up from idle > > > draws more power than is available? Removing this override is a correct > > > change anyway so maybe there is no need to investigate deeper. > > Marek just sent a similar one a few minutes ago: > > http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/503230.html > Forgot to add Marek. Wow, that was literally 15 minutes before my patch. In my defense I did search the archives before starting to format the patch but it had not arrived yet. Anyway, that version also sets the supply for reg_arm and reg_soc. It is not necessary for fixing the crash I'm seeing but is good because it will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix 1375mv. I tested Marek's patch and it works fine on my rev B board (which otherwise fails to boot upstream). -- Regards, Leonard
On Tue, Apr 25, 2017 at 2:23 PM, Leonard Crestez <leonard.crestez@nxp.com> wrote: > Wow, that was literally 15 minutes before my patch. In my defense I did > search the archives before starting to format the patch but it had not > arrived yet. > > Anyway, that version also sets the supply for reg_arm and reg_soc. It > is not necessary for fixing the crash I'm seeing but is good because it > will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix > 1375mv. I tested Marek's patch and it works fine on my rev B board > (which otherwise fails to boot upstream). Excellent! I only have revA board and do not get the crash with this revision. If you can reply to Marek's patch with your Tested-by that would be nice, thanks.
On 04/25/2017 07:23 PM, Leonard Crestez wrote: > On Tue, 2017-04-25 at 14:02 -0300, Fabio Estevam wrote: >> On Tue, Apr 25, 2017 at 2:02 PM, Fabio Estevam <festevam@gmail.com> wrote: >>> >>> Hi Leonard, >>> >>> On Tue, Apr 25, 2017 at 1:57 PM, Leonard Crestez >>> <leonard.crestez@nxp.com> wrote: >>>> >>>> The board file for imx6sx-dbg overrides cpufreq operating points to use >>>> higher voltages. This is done because the board has a shared rail for >>>> VDD_ARM_IN and VDD_SOC_IN and when using LDO bypass the shared voltage >>>> needs to be a value suitable for both ARM and SOC. >>>> >>>> This was introduced in: >>>> >>>> commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it default") >>>> >>>> This only only applies to LDO bypass mode, a feature not present in >>>> upstream. When LDOs are enabled the effect is to use higher voltages than >>>> necesarry for no good reason. >>>> >>>> Setting these higher voltages can make some boards fail to boot with ugly >>>> semi-random crashes, reminiscent of memory corruption. These failures >>>> happen the first time the lowest idle state is used. Remove the OPP >>>> override in order to fix those crashes. >>>> >>>> Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> >>>> >>>> --- >>>> It's not clear exactly why the crashes happen. Perhaps waking up from idle >>>> draws more power than is available? Removing this override is a correct >>>> change anyway so maybe there is no need to investigate deeper. > >>> Marek just sent a similar one a few minutes ago: >>> http://lists.infradead.org/pipermail/linux-arm-kernel/2017-April/503230.html > >> Forgot to add Marek. > > Wow, that was literally 15 minutes before my patch. In my defense I did > search the archives before starting to format the patch but it had not > arrived yet. Hehehe :-) > Anyway, that version also sets the supply for reg_arm and reg_soc. It > is not necessary for fixing the crash I'm seeing but is good because it > will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix > 1375mv. I tested Marek's patch and it works fine on my rev B board > (which otherwise fails to boot upstream). Oh that's nice , thanks ! I don't have SDB and I hacked it up after a brief discussion with Fabio without even compile-testing it, thus RFC. Glad to hear it works and thanks for testing it ! Can you add a formal Tested-by please ?
> >The board file for imx6sx-dbg overrides cpufreq operating points to use higher >voltages. This is done because the board has a shared rail for VDD_ARM_IN and >VDD_SOC_IN and when using LDO bypass the shared voltage needs to be a value >suitable for both ARM and SOC. > >This was introduced in: > >commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it default") > >This only only applies to LDO bypass mode, a feature not present in upstream. When >LDOs are enabled the effect is to use higher voltages than necesarry for no good >reason. > >Setting these higher voltages can make some boards fail to boot with ugly semi- >random crashes, reminiscent of memory corruption. These failures happen the first >time the lowest idle state is used. Remove the OPP override in order to fix those >crashes. > Add Anson and Robin This code has existed more than 2 years, it is strange why the bug has not reported both for internal user and external user. I run upstream kernel using imx6sx-sdb revB very often at recent years, but not meet this issue. How to trigger this unstable issue, anything needs to change at u-boot? Peter >Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> > >--- >It's not clear exactly why the crashes happen. Perhaps waking up from idle draws >more power than is available? Removing this override is a correct change anyway so >maybe there is no need to investigate deeper. > > arch/arm/boot/dts/imx6sx-sdb.dts | 17 ----------------- > 1 file changed, 17 deletions(-) > >diff --git a/arch/arm/boot/dts/imx6sx-sdb.dts b/arch/arm/boot/dts/imx6sx-sdb.dts >index 5bb8fd5..d71da30 100644 >--- a/arch/arm/boot/dts/imx6sx-sdb.dts >+++ b/arch/arm/boot/dts/imx6sx-sdb.dts >@@ -12,23 +12,6 @@ > model = "Freescale i.MX6 SoloX SDB RevB Board"; }; > >-&cpu0 { >- operating-points = < >- /* kHz uV */ >- 996000 1250000 >- 792000 1175000 >- 396000 1175000 >- 198000 1175000 >- >; >- fsl,soc-operating-points = < >- /* ARM kHz SOC uV */ >- 996000 1250000 >- 792000 1175000 >- 396000 1175000 >- 198000 1175000 >- >; >-}; >- > &i2c1 { > clock-frequency = <100000>; > pinctrl-names = "default"; >-- >2.7.4
On Tue, Apr 25, 2017 at 07:28:06PM +0200, Marek Vasut wrote: > On 04/25/2017 07:23 PM, Leonard Crestez wrote: > > Anyway, that version also sets the supply for reg_arm and reg_soc. It > > is not necessary for fixing the crash I'm seeing but is good because it > > will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix > > 1375mv. I tested Marek's patch and it works fine on my rev B board > > (which otherwise fails to boot upstream). > > Oh that's nice , thanks ! I don't have SDB and I hacked it up after a > brief discussion with Fabio without even compile-testing it, thus RFC. > Glad to hear it works and thanks for testing it ! Can you add a formal > Tested-by please ? Hi Marek, Thanks for your patch. But I prefer Leonard's version because: 1) it has a better commit log; 2) it sticks to one-patch-does-one-thing policy. But I'm going to wait for a while to get Peter's comment discussed, before I actually apply Leonard's patch. Shawn
On 05/03/2017 03:57 PM, Shawn Guo wrote: > On Tue, Apr 25, 2017 at 07:28:06PM +0200, Marek Vasut wrote: >> On 04/25/2017 07:23 PM, Leonard Crestez wrote: >>> Anyway, that version also sets the supply for reg_arm and reg_soc. It >>> is not necessary for fixing the crash I'm seeing but is good because it >>> will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix >>> 1375mv. I tested Marek's patch and it works fine on my rev B board >>> (which otherwise fails to boot upstream). >> >> Oh that's nice , thanks ! I don't have SDB and I hacked it up after a >> brief discussion with Fabio without even compile-testing it, thus RFC. >> Glad to hear it works and thanks for testing it ! Can you add a formal >> Tested-by please ? > > Hi Marek, Hi Shawn, > Thanks for your patch. But I prefer Leonard's version because: 1) it > has a better commit log; 2) it sticks to one-patch-does-one-thing > policy. Well I'd prefer this patch because 1) It has T-B 2) It actually fixes a problem with the voltage rails such that the DVFS works without leaving the system in unstable or dead state. You do need the second part of my patch if you drop the OPP hackery, without it the power framework cannot correctly configure the core voltages, so the patch from Leonard makes things worse. > But I'm going to wait for a while to get Peter's comment discussed, > before I actually apply Leonard's patch. > > Shawn >
On 05/03/2017 04:26 PM, Marek Vasut wrote: > On 05/03/2017 03:57 PM, Shawn Guo wrote: >> On Tue, Apr 25, 2017 at 07:28:06PM +0200, Marek Vasut wrote: >>> On 04/25/2017 07:23 PM, Leonard Crestez wrote: >>>> Anyway, that version also sets the supply for reg_arm and reg_soc. It >>>> is not necessary for fixing the crash I'm seeing but is good because it >>>> will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix >>>> 1375mv. I tested Marek's patch and it works fine on my rev B board >>>> (which otherwise fails to boot upstream). >>> >>> Oh that's nice , thanks ! I don't have SDB and I hacked it up after a >>> brief discussion with Fabio without even compile-testing it, thus RFC. >>> Glad to hear it works and thanks for testing it ! Can you add a formal >>> Tested-by please ? >> >> Hi Marek, > > Hi Shawn, > >> Thanks for your patch. But I prefer Leonard's version because: 1) it >> has a better commit log; 2) it sticks to one-patch-does-one-thing >> policy. > > Well I'd prefer this patch because > 1) It has T-B Correction, two TBs [1] [1] https://patchwork.kernel.org/patch/9698749/ > 2) It actually fixes a problem with the voltage rails such that the DVFS > works without leaving the system in unstable or dead state. You do > need the second part of my patch if you drop the OPP hackery, without > it the power framework cannot correctly configure the core voltages, > so the patch from Leonard makes things worse. > >> But I'm going to wait for a while to get Peter's comment discussed, >> before I actually apply Leonard's patch. >> >> Shawn >> > >
On Wed, May 03, 2017 at 04:32:06PM +0200, Marek Vasut wrote: > On 05/03/2017 04:26 PM, Marek Vasut wrote: > > On 05/03/2017 03:57 PM, Shawn Guo wrote: > >> On Tue, Apr 25, 2017 at 07:28:06PM +0200, Marek Vasut wrote: > >>> On 04/25/2017 07:23 PM, Leonard Crestez wrote: > >>>> Anyway, that version also sets the supply for reg_arm and reg_soc. It > >>>> is not necessary for fixing the crash I'm seeing but is good because it > >>>> will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix > >>>> 1375mv. I tested Marek's patch and it works fine on my rev B board > >>>> (which otherwise fails to boot upstream). > >>> > >>> Oh that's nice , thanks ! I don't have SDB and I hacked it up after a > >>> brief discussion with Fabio without even compile-testing it, thus RFC. > >>> Glad to hear it works and thanks for testing it ! Can you add a formal > >>> Tested-by please ? > >> > >> Hi Marek, > > > > Hi Shawn, > > > >> Thanks for your patch. But I prefer Leonard's version because: 1) it > >> has a better commit log; 2) it sticks to one-patch-does-one-thing > >> policy. > > > > Well I'd prefer this patch because > > 1) It has T-B > > Correction, two TBs [1] > > [1] https://patchwork.kernel.org/patch/9698749/ That doesn't mean Leonard's patch hasn't been tested by anyone. > > 2) It actually fixes a problem with the voltage rails such that the DVFS > > works without leaving the system in unstable or dead state. You do > > need the second part of my patch if you drop the OPP hackery, without > > it the power framework cannot correctly configure the core voltages, > > so the patch from Leonard makes things worse. If that's true, I will change my mind. Shawn
On 05/03/2017 04:41 PM, Shawn Guo wrote: > On Wed, May 03, 2017 at 04:32:06PM +0200, Marek Vasut wrote: >> On 05/03/2017 04:26 PM, Marek Vasut wrote: >>> On 05/03/2017 03:57 PM, Shawn Guo wrote: >>>> On Tue, Apr 25, 2017 at 07:28:06PM +0200, Marek Vasut wrote: >>>>> On 04/25/2017 07:23 PM, Leonard Crestez wrote: >>>>>> Anyway, that version also sets the supply for reg_arm and reg_soc. It >>>>>> is not necessary for fixing the crash I'm seeing but is good because it >>>>>> will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix >>>>>> 1375mv. I tested Marek's patch and it works fine on my rev B board >>>>>> (which otherwise fails to boot upstream). >>>>> >>>>> Oh that's nice , thanks ! I don't have SDB and I hacked it up after a >>>>> brief discussion with Fabio without even compile-testing it, thus RFC. >>>>> Glad to hear it works and thanks for testing it ! Can you add a formal >>>>> Tested-by please ? >>>> >>>> Hi Marek, >>> >>> Hi Shawn, >>> >>>> Thanks for your patch. But I prefer Leonard's version because: 1) it >>>> has a better commit log; 2) it sticks to one-patch-does-one-thing >>>> policy. >>> >>> Well I'd prefer this patch because >>> 1) It has T-B >> >> Correction, two TBs [1] >> >> [1] https://patchwork.kernel.org/patch/9698749/ > > That doesn't mean Leonard's patch hasn't been tested by anyone. That's what the T-B tags are for ... >>> 2) It actually fixes a problem with the voltage rails such that the DVFS >>> works without leaving the system in unstable or dead state. You do >>> need the second part of my patch if you drop the OPP hackery, without >>> it the power framework cannot correctly configure the core voltages, >>> so the patch from Leonard makes things worse. > > If that's true, I will change my mind. Well you can check the schematic ... the ARM and SOC rails share the same upstream regulator.
On Wed, 2017-05-03 at 16:26 +0200, Marek Vasut wrote: > On 05/03/2017 03:57 PM, Shawn Guo wrote: > > > > On Tue, Apr 25, 2017 at 07:28:06PM +0200, Marek Vasut wrote: > > > > > > On 04/25/2017 07:23 PM, Leonard Crestez wrote: > > > > > > > > Anyway, that version also sets the supply for reg_arm and reg_soc. It > > > > is not necessary for fixing the crash I'm seeing but is good because it > > > > will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix > > > > 1375mv. I tested Marek's patch and it works fine on my rev B board > > > > (which otherwise fails to boot upstream). > > > Oh that's nice , thanks ! I don't have SDB and I hacked it up after a > > > brief discussion with Fabio without even compile-testing it, thus RFC. > > > Glad to hear it works and thanks for testing it ! Can you add a formal > > > Tested-by please ? > > Hi Marek, > Hi Shawn, > > > Thanks for your patch. But I prefer Leonard's version because: 1) it > > has a better commit log; 2) it sticks to one-patch-does-one-thing > > policy. > 2) It actually fixes a problem with the voltage rails such that the DVFS > works without leaving the system in unstable or dead state. You do > need the second part of my patch if you drop the OPP hackery, without > it the power framework cannot correctly configure the core voltages, > so the patch from Leonard makes things worse. No, I think there is a misunderstanding here. The second part of your patch will cause cpufreq poking at LDOs to indirectly adjust the input from the PMIC to the minimum required (this is LDO target + min_dropout_uv). Without it by default VDD_ARM_SOC_IN will remain fixed as 1375mV from boot. That default value should be high enough for all cpufreq settings. Setting the LDO parent will cause this voltage to be dynamically reduced when possible (at low frequencies). This is not required for proper operation, it is just an optimization to do more of the regulation in the PMIC instead. It should work just fine without the second part. That OPP override exists for "LDO bypass" mode, a feature not present in upstream. In that mode the internal regulators are set to bypass mode and voltage is controlled directly from the PMIC. Since VDD_ARM and VDD_SOC have different minimum requirements but are joined on the board the OPP voltages are adjusted to max(VDD_ARM, VDD_SOC). If LDOs are enabled there is no good reason to do this. I don't care which patch goes in but the effect of the patch should be clarified. -- Regards, Leonard
On 05/03/2017 04:58 PM, Leonard Crestez wrote: > On Wed, 2017-05-03 at 16:26 +0200, Marek Vasut wrote: > >> On 05/03/2017 03:57 PM, Shawn Guo wrote: >>> >>> On Tue, Apr 25, 2017 at 07:28:06PM +0200, Marek Vasut wrote: >>>> >>>> On 04/25/2017 07:23 PM, Leonard Crestez wrote: >>>>> >>>>> Anyway, that version also sets the supply for reg_arm and reg_soc. It >>>>> is not necessary for fixing the crash I'm seeing but is good because it >>>>> will result in the minimum voltage on VDD_ARM_SOC_IN rather than a fix >>>>> 1375mv. I tested Marek's patch and it works fine on my rev B board >>>>> (which otherwise fails to boot upstream). >>>> Oh that's nice , thanks ! I don't have SDB and I hacked it up after a >>>> brief discussion with Fabio without even compile-testing it, thus RFC. >>>> Glad to hear it works and thanks for testing it ! Can you add a formal >>>> Tested-by please ? >>> Hi Marek, >> Hi Shawn, >> >>> Thanks for your patch. But I prefer Leonard's version because: 1) it >>> has a better commit log; 2) it sticks to one-patch-does-one-thing >>> policy. > >> 2) It actually fixes a problem with the voltage rails such that the DVFS >> works without leaving the system in unstable or dead state. You do >> need the second part of my patch if you drop the OPP hackery, without >> it the power framework cannot correctly configure the core voltages, >> so the patch from Leonard makes things worse. > > No, I think there is a misunderstanding here. The second part of your > patch will cause cpufreq poking at LDOs to indirectly adjust the input > from the PMIC to the minimum required (this is LDO target + > min_dropout_uv). Without it by default VDD_ARM_SOC_IN will remain fixed > as 1375mV from boot. Who sets / guarantees that default value for ARM and SOC rails ? With the OPP override in place, there's at least the guarantee that both rails will have the same voltage requirement. If you remove the OPP override without modeling the actual regulator wiring, the guarantee is gone. > That default value should be high enough for all cpufreq settings. > Setting the LDO parent will cause this voltage to be dynamically > reduced when possible (at low frequencies). This is not required for > proper operation, it is just an optimization to do more of the > regulation in the PMIC instead. It should work just fine without the > second part. > > That OPP override exists for "LDO bypass" mode, a feature not present > in upstream. In that mode the internal regulators are set to bypass > mode and voltage is controlled directly from the PMIC. Since VDD_ARM > and VDD_SOC have different minimum requirements but are joined on the > board the OPP voltages are adjusted to max(VDD_ARM, VDD_SOC). If LDOs > are enabled there is no good reason to do this. > > I don't care which patch goes in but the effect of the patch should be > clarified. > > -- > Regards, > Leonard >
On Wed, 2017-05-03 at 17:59 +0200, Marek Vasut wrote: > On 05/03/2017 04:58 PM, Leonard Crestez wrote: > > On Wed, 2017-05-03 at 16:26 +0200, Marek Vasut wrote: > > > 2) It actually fixes a problem with the voltage rails such that the DVFS > > > works without leaving the system in unstable or dead state. You do > > > need the second part of my patch if you drop the OPP hackery, without > > > it the power framework cannot correctly configure the core voltages, > > > so the patch from Leonard makes things worse. > > No, I think there is a misunderstanding here. The second part of your > > patch will cause cpufreq poking at LDOs to indirectly adjust the input > > from the PMIC to the minimum required (this is LDO target + > > min_dropout_uv). Without it by default VDD_ARM_SOC_IN will remain fixed > > as 1375mV from boot. > Who sets / guarantees that default value for ARM and SOC rails ? I think it's from the PMIC hardware itself (but maybe uboot plays with it). VDD_ARM_SOC_IN on this board is tied to SW1AB from MMPF0200: http://www.nxp.com/assets/documents/data/en/data-sheets/MMPF0200.pdf It seems reasonable to rely on such voltages set externally. > With the OPP override in place, there's at least the guarantee that both > rails will have the same voltage requirement. If you remove the OPP > override without modeling the actual regulator wiring, the guarantee is > gone. The imx6sx chip has internal LDO_ARM and LDO_SOC regulators which can generate separate voltages for arm/soc. The fact that these regulators share the same supply is only an issue when they are set in bypass mode. However the boot issues happen on REV C but apparently not REV B of the board. I don't have a good explanation for this so maybe I am missing something. -- Regards, Leonard
On 05/03/2017 07:58 PM, Leonard Crestez wrote: > On Wed, 2017-05-03 at 17:59 +0200, Marek Vasut wrote: >> On 05/03/2017 04:58 PM, Leonard Crestez wrote: >>> On Wed, 2017-05-03 at 16:26 +0200, Marek Vasut wrote: >>>> 2) It actually fixes a problem with the voltage rails such that the DVFS >>>> works without leaving the system in unstable or dead state. You do >>>> need the second part of my patch if you drop the OPP hackery, without >>>> it the power framework cannot correctly configure the core voltages, >>>> so the patch from Leonard makes things worse. >>> No, I think there is a misunderstanding here. The second part of your >>> patch will cause cpufreq poking at LDOs to indirectly adjust the input >>> from the PMIC to the minimum required (this is LDO target + >>> min_dropout_uv). Without it by default VDD_ARM_SOC_IN will remain fixed >>> as 1375mV from boot. > >> Who sets / guarantees that default value for ARM and SOC rails ? > > I think it's from the PMIC hardware itself (but maybe uboot plays with > it). VDD_ARM_SOC_IN on this board is tied to SW1AB from MMPF0200: > > http://www.nxp.com/assets/documents/data/en/data-sheets/MMPF0200.pdf > > It seems reasonable to rely on such voltages set externally. Isn't it an established rule that Linux should not depend on bootloader settings ? Or did that change ? >> With the OPP override in place, there's at least the guarantee that both >> rails will have the same voltage requirement. If you remove the OPP >> override without modeling the actual regulator wiring, the guarantee is >> gone. > > The imx6sx chip has internal LDO_ARM and LDO_SOC regulators which can > generate separate voltages for arm/soc. The fact that these regulators > share the same supply is only an issue when they are set in bypass > mode. > > However the boot issues happen on REV C but apparently not REV B of the > board. I don't have a good explanation for this so maybe I am missing > something. Well the regulator(s) cannot be correctly configured if the kernel doesn't have the correct power distribution described in the DT .
On Wed, 2017-05-03 at 21:33 +0200, Marek Vasut wrote: > On 05/03/2017 07:58 PM, Leonard Crestez wrote: > > On Wed, 2017-05-03 at 17:59 +0200, Marek Vasut wrote: > > > On 05/03/2017 04:58 PM, Leonard Crestez wrote: > > > > On Wed, 2017-05-03 at 16:26 +0200, Marek Vasut wrote: > > > > > 2) It actually fixes a problem with the voltage rails such that the DVFS > > > > > works without leaving the system in unstable or dead state. You do > > > > > need the second part of my patch if you drop the OPP hackery, without > > > > > it the power framework cannot correctly configure the core voltages, > > > > > so the patch from Leonard makes things worse. > > > > No, I think there is a misunderstanding here. The second part of your > > > > patch will cause cpufreq poking at LDOs to indirectly adjust the input > > > > from the PMIC to the minimum required (this is LDO target + > > > > min_dropout_uv). Without it by default VDD_ARM_SOC_IN will remain fixed > > > > as 1375mV from boot. > > > Who sets / guarantees that default value for ARM and SOC rails ? > > I think it's from the PMIC hardware itself (but maybe uboot plays with > > it). VDD_ARM_SOC_IN on this board is tied to SW1AB from MMPF0200: > > > > http://www.nxp.com/assets/documents/data/en/data-sheets/MMPF0200.pdf > > > > It seems reasonable to rely on such voltages set externally. > Isn't it an established rule that Linux should not depend on bootloader > settings ? Or did that change ? I don't actually know. Is there a hard and fast rule about this, even when it comes to voltages? In theory it is possible for a bootloader to set a low cpu frequency and low voltage and then have the chip fail when the cpufreq driver attempts to go higher. Setting vin-supply on reg_arm/reg_soc would fix that. > Well the regulator(s) cannot be correctly configured if the kernel > doesn't have the correct power distribution described in the DT . It depends on your definition of "correctness". It it certainly possible to get a functional system while only partially describing regulator relationships. I think there is a further misunderstanding here. I have a problem where imx6sx-sdb rev C boards crash on boot with upstream (but are reported to work fine with rev B). Removing the OPP overrides fixes this specific issue. I don't object to the second part of your patch, setting correct supply links is a good thing for various reasons. It is just not necessary for fixing the concrete crash mentioned above (and I tested this). It should probably go in a separate patch. It might seem a pedantic difference but it's good to accurately describe the effect of patches in commit messages. For example it might help somebody looking to backport various fixes. -- Regards, Leonard
On 05/04/2017 11:42 AM, Leonard Crestez wrote: > On Wed, 2017-05-03 at 21:33 +0200, Marek Vasut wrote: >> On 05/03/2017 07:58 PM, Leonard Crestez wrote: >>> On Wed, 2017-05-03 at 17:59 +0200, Marek Vasut wrote: >>>> On 05/03/2017 04:58 PM, Leonard Crestez wrote: >>>>> On Wed, 2017-05-03 at 16:26 +0200, Marek Vasut wrote: > >>>>>> 2) It actually fixes a problem with the voltage rails such that the DVFS >>>>>> works without leaving the system in unstable or dead state. You do >>>>>> need the second part of my patch if you drop the OPP hackery, without >>>>>> it the power framework cannot correctly configure the core voltages, >>>>>> so the patch from Leonard makes things worse. > >>>>> No, I think there is a misunderstanding here. The second part of your >>>>> patch will cause cpufreq poking at LDOs to indirectly adjust the input >>>>> from the PMIC to the minimum required (this is LDO target + >>>>> min_dropout_uv). Without it by default VDD_ARM_SOC_IN will remain fixed >>>>> as 1375mV from boot. > >>>> Who sets / guarantees that default value for ARM and SOC rails ? > >>> I think it's from the PMIC hardware itself (but maybe uboot plays with >>> it). VDD_ARM_SOC_IN on this board is tied to SW1AB from MMPF0200: >>> >>> http://www.nxp.com/assets/documents/data/en/data-sheets/MMPF0200.pdf >>> >>> It seems reasonable to rely on such voltages set externally. > >> Isn't it an established rule that Linux should not depend on bootloader >> settings ? Or did that change ? > > I don't actually know. Is there a hard and fast rule about this, even when it comes to voltages? Unless something changed recently, you are not supposed to depend on bootloader behavior. > In theory it is possible for a bootloader to set a low cpu frequency and low voltage and then have the chip fail when the cpufreq driver attempts to go higher. Setting vin-supply on reg_arm/reg_soc would fix that. > >> Well the regulator(s) cannot be correctly configured if the kernel >> doesn't have the correct power distribution described in the DT . > > It depends on your definition of "correctness". It it certainly > possible to get a functional system while only partially describing > regulator relationships. Then your description of the hardware in DT is not correct. > I think there is a further misunderstanding here. I have a problem > where imx6sx-sdb rev C boards crash on boot with upstream (but are > reported to work fine with rev B). Removing the OPP overrides fixes > this specific issue. > > I don't object to the second part of your patch, setting correct supply links is a good thing for various reasons. It is just not necessary for fixing the concrete crash mentioned above (and I tested this). It should probably go in a separate patch. Mind you, my patch is not fixing any crash, it's correcting the regulator binding and removing the OPP override which is at that point useless. > It might seem a pedantic difference but it's good to accurately describe the effect of patches in commit messages. For example it might help somebody looking to backport various fixes. Which part of the following commit message is unclear? " The imx6sx-sdb has one power supply that drives both VDDARM_IN and VDDSOC_IN, which is the sw1a regulator on the PFUZE PMIC. Connect both inputs to the sw1a regulator on the PMIC and drop the OPP hackery which is no longer needed as the power framework will take care of the regulator configuration as needed. " btw if sending obvious bugfix upstream is met with this sort of resistance and pointless discussion, it is extremely demotivating. Waiting for a maintainer reply for 2-4 weeks, only to get a kurt reply like "I don't like the commit message" doesn't help either.
On Thu, Apr 27, 2017 at 01:17:12AM +0000, Peter Chen wrote: > > > > >The board file for imx6sx-dbg overrides cpufreq operating points to use higher > >voltages. This is done because the board has a shared rail for VDD_ARM_IN and > >VDD_SOC_IN and when using LDO bypass the shared voltage needs to be a value > >suitable for both ARM and SOC. > > > >This was introduced in: > > > >commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it default") > > > >This only only applies to LDO bypass mode, a feature not present in upstream. When > >LDOs are enabled the effect is to use higher voltages than necesarry for no good > >reason. > > > >Setting these higher voltages can make some boards fail to boot with ugly semi- > >random crashes, reminiscent of memory corruption. These failures happen the first > >time the lowest idle state is used. Remove the OPP override in order to fix those > >crashes. > > > > Add Anson and Robin > > This code has existed more than 2 years, it is strange why the bug has not reported both > for internal user and external user. I run upstream kernel using imx6sx-sdb revB very often > at recent years, but not meet this issue. How to trigger this unstable issue, anything needs > to change at u-boot? Per comments from Henri and Leonard, it sounds like the issue only happens on RevC board? Shawn
Hi Shawn, On Thu, May 4, 2017 at 8:43 AM, Shawn Guo <shawnguo@kernel.org> wrote: > Per comments from Henri and Leonard, it sounds like the issue only > happens on RevC board? That's correct. I do not observe any crashes on my revA board.
On Thu, May 04, 2017 at 12:06:11PM +0200, Marek Vasut wrote: > On 05/04/2017 11:42 AM, Leonard Crestez wrote: > > I think there is a further misunderstanding here. I have a problem > > where imx6sx-sdb rev C boards crash on boot with upstream (but are > > reported to work fine with rev B). Removing the OPP overrides fixes > > this specific issue. > > > > I don't object to the second part of your patch, setting correct supply links is a good thing for various reasons. It is just not necessary for fixing the concrete crash mentioned above (and I tested this). It should probably go in a separate patch. > > Mind you, my patch is not fixing any crash, it's correcting the > regulator binding and removing the OPP override which is at that > point useless. Heh, that's the primary reason why I prefer Leonard's patch, as his patch is fixing a critical crash issue, while yours is just removing some useless stuff and correcting something that doesn't show any real problem. So Leonard's patch will need to be applied for v4.12-rc and copy stable kernel, while yours will only be applied for next merge window as a cleanup/improving thing. Patches are similar, but they can be handled very differently, because commit log tells completely different stories. Do you see how commit log matters now? > > It might seem a pedantic difference but it's good to accurately describe the effect of patches in commit messages. For example it might help somebody looking to backport various fixes. @Leonard, no, it's not pedantic at all. I really appreciate your commit message and all the comments you added in the discussion, which is extremely helpful for us to understand the changes. > Which part of the following commit message is unclear? > > " > The imx6sx-sdb has one power supply that drives both VDDARM_IN > and VDDSOC_IN, which is the sw1a regulator on the PFUZE PMIC. > Connect both inputs to the sw1a regulator on the PMIC and drop > the OPP hackery which is no longer needed as the power framework > will take care of the regulator configuration as needed. > " Something unclear in my opinion: - The OPP hackery was never needed, as it's only needed for LDO bypass mode which hasn't been supported by mainline kernel. It's not 'no longer needed as the power framework ...'. - What are the related changes in power framework? It will be more clear if we can have the particular commit mentioned here. > btw if sending obvious bugfix upstream is met with this sort of Leonard's patch is an obvious bugfix, but yours is not. > resistance and pointless discussion, it is extremely demotivating. As I said to Leonard above, the discussion is extremely helpful, IMO. > Waiting for a maintainer reply for 2-4 weeks, only to get a kurt This is not a paid job for maintainer. It's expected that he doesn't always reply in a timely manner, especially when the tree is kinda 'frozen' for merge window. > reply like "I don't like the commit message" doesn't help either. What really annoys me is your attitude to commit message. Shawn
On Tue, Apr 25, 2017 at 07:57:59PM +0300, Leonard Crestez wrote: > The board file for imx6sx-dbg overrides cpufreq operating points to use s/imx6sx-dbg/imx6sx-sdb > higher voltages. This is done because the board has a shared rail for > VDD_ARM_IN and VDD_SOC_IN and when using LDO bypass the shared voltage > needs to be a value suitable for both ARM and SOC. > > This was introduced in: > > commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it default") > > This only only applies to LDO bypass mode, a feature not present in s/only only/only > upstream. When LDOs are enabled the effect is to use higher voltages than > necesarry for no good reason. > > Setting these higher voltages can make some boards fail to boot with ugly Please make it clear it's RevC board. > semi-random crashes, reminiscent of memory corruption. These failures > happen the first time the lowest idle state is used. Remove the OPP > override in order to fix those crashes. > > Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> Please add tags for Fixes and Cc stable. Shawn
On 05/04/2017 02:44 PM, Shawn Guo wrote: > On Thu, May 04, 2017 at 12:06:11PM +0200, Marek Vasut wrote: >> On 05/04/2017 11:42 AM, Leonard Crestez wrote: >>> I think there is a further misunderstanding here. I have a problem >>> where imx6sx-sdb rev C boards crash on boot with upstream (but are >>> reported to work fine with rev B). Removing the OPP overrides fixes >>> this specific issue. >>> >>> I don't object to the second part of your patch, setting correct supply links is a good thing for various reasons. It is just not necessary for fixing the concrete crash mentioned above (and I tested this). It should probably go in a separate patch. >> >> Mind you, my patch is not fixing any crash, it's correcting the >> regulator binding and removing the OPP override which is at that >> point useless. > > Heh, that's the primary reason why I prefer Leonard's patch, as his > patch is fixing a critical crash issue, while yours is just removing > some useless stuff and correcting something that doesn't show any real > problem. Maybe you want to compare those two patches again, his patch fixing the critical crash is removing the same "some useless stuff" as mine. Plus mine is actually making sure the regulators are configured correctly, not just removing "some useless stuff" and hoping for the best that bootloader will do the right thing. > So Leonard's patch will need to be applied for v4.12-rc and copy stable > kernel, while yours will only be applied for next merge window as a > cleanup/improving thing. Well mine is bugfix, but I'm clearly unable to get that information across. > Patches are similar, but they can be handled very differently, because > commit log tells completely different stories. Do you see how commit > log matters now? I understand how commit log matters and I still believe I described the change there just fine. >>> It might seem a pedantic difference but it's good to accurately describe the effect of patches in commit messages. For example it might help somebody looking to backport various fixes. > > @Leonard, no, it's not pedantic at all. I really appreciate your commit > message and all the comments you added in the discussion, which is > extremely helpful for us to understand the changes. > >> Which part of the following commit message is unclear? >> >> " >> The imx6sx-sdb has one power supply that drives both VDDARM_IN >> and VDDSOC_IN, which is the sw1a regulator on the PFUZE PMIC. >> Connect both inputs to the sw1a regulator on the PMIC and drop >> the OPP hackery which is no longer needed as the power framework >> will take care of the regulator configuration as needed. >> " > > Something unclear in my opinion: > > - The OPP hackery was never needed, as it's only needed for LDO bypass > mode which hasn't been supported by mainline kernel. It's not 'no > longer needed as the power framework ...'. I don't know what this is about, it's not from my patch either ... > - What are the related changes in power framework? It will be more > clear if we can have the particular commit mentioned here. Uh, I don't understand your question, there are no new changes in the power framework. The DT for the SDB was always wrong, my patch fixes it. >> btw if sending obvious bugfix upstream is met with this sort of > > Leonard's patch is an obvious bugfix, but yours is not. Please consider mine one. I marked it RFC because I don't even have the board and I needed someone to test it. >> resistance and pointless discussion, it is extremely demotivating. > > As I said to Leonard above, the discussion is extremely helpful, IMO. > >> Waiting for a maintainer reply for 2-4 weeks, only to get a kurt > > This is not a paid job for maintainer. It's expected that he doesn't > always reply in a timely manner, especially when the tree is kinda > 'frozen' for merge window. > >> reply like "I don't like the commit message" doesn't help either. > > What really annoys me is your attitude to commit message. We had this discussion before ...
On Thu, May 04, 2017 at 03:08:41PM +0200, Marek Vasut wrote: > On 05/04/2017 02:44 PM, Shawn Guo wrote: > > On Thu, May 04, 2017 at 12:06:11PM +0200, Marek Vasut wrote: <snip> > >> Mind you, my patch is not fixing any crash, it's correcting the > >> regulator binding and removing the OPP override which is at that > >> point useless. > > > > Heh, that's the primary reason why I prefer Leonard's patch, as his > > patch is fixing a critical crash issue, while yours is just removing > > some useless stuff and correcting something that doesn't show any real > > problem. > > Maybe you want to compare those two patches again, his patch fixing the > critical crash is removing the same "some useless stuff" as mine. The difference is in commit message. From your commit message, people do not see what real world user noticeable issue your patch is fixing. > Plus > mine is actually making sure the regulators are configured correctly, > not just removing "some useless stuff" and hoping for the best that > bootloader will do the right thing. Without this part, we do not get anything worse. That said, it can be an improvement in another patch. > >> Which part of the following commit message is unclear? > >> > >> " > >> The imx6sx-sdb has one power supply that drives both VDDARM_IN > >> and VDDSOC_IN, which is the sw1a regulator on the PFUZE PMIC. > >> Connect both inputs to the sw1a regulator on the PMIC and drop > >> the OPP hackery which is no longer needed as the power framework > >> will take care of the regulator configuration as needed. > >> " > > > > Something unclear in my opinion: > > > > - The OPP hackery was never needed, as it's only needed for LDO bypass > > mode which hasn't been supported by mainline kernel. It's not 'no > > longer needed as the power framework ...'. > > I don't know what this is about, it's not from my patch either ... So I guess you do not understand how the OPP hackery was born and why it shouldn't be there for mainline kernel at all. > > - What are the related changes in power framework? It will be more > > clear if we can have the particular commit mentioned here. > > Uh, I don't understand your question, there are no new changes in the > power framework. The DT for the SDB was always wrong, my patch fixes it. Then 'no longer needed' in your commit message is really confusing. Shawn
On 05/04/2017 03:41 PM, Shawn Guo wrote: > On Thu, May 04, 2017 at 03:08:41PM +0200, Marek Vasut wrote: >> On 05/04/2017 02:44 PM, Shawn Guo wrote: >>> On Thu, May 04, 2017 at 12:06:11PM +0200, Marek Vasut wrote: > > <snip> > >>>> Mind you, my patch is not fixing any crash, it's correcting the >>>> regulator binding and removing the OPP override which is at that >>>> point useless. >>> >>> Heh, that's the primary reason why I prefer Leonard's patch, as his >>> patch is fixing a critical crash issue, while yours is just removing >>> some useless stuff and correcting something that doesn't show any real >>> problem. >> >> Maybe you want to compare those two patches again, his patch fixing the >> critical crash is removing the same "some useless stuff" as mine. > > The difference is in commit message. From your commit message, people > do not see what real world user noticeable issue your patch is fixing. > >> Plus >> mine is actually making sure the regulators are configured correctly, >> not just removing "some useless stuff" and hoping for the best that >> bootloader will do the right thing. > > Without this part, we do not get anything worse. That said, it can be > an improvement in another patch. > >>>> Which part of the following commit message is unclear? >>>> >>>> " >>>> The imx6sx-sdb has one power supply that drives both VDDARM_IN >>>> and VDDSOC_IN, which is the sw1a regulator on the PFUZE PMIC. >>>> Connect both inputs to the sw1a regulator on the PMIC and drop >>>> the OPP hackery which is no longer needed as the power framework >>>> will take care of the regulator configuration as needed. >>>> " >>> >>> Something unclear in my opinion: >>> >>> - The OPP hackery was never needed, as it's only needed for LDO bypass >>> mode which hasn't been supported by mainline kernel. It's not 'no >>> longer needed as the power framework ...'. >> >> I don't know what this is about, it's not from my patch either ... > > So I guess you do not understand how the OPP hackery was born and why it > shouldn't be there for mainline kernel at all. The OPP hackery is there to keep both regulators configured the same way, since they share the same input voltage rail IMO. If you model the power distribution correctly, the OPP hackery can be removed. >>> - What are the related changes in power framework? It will be more >>> clear if we can have the particular commit mentioned here. >> >> Uh, I don't understand your question, there are no new changes in the >> power framework. The DT for the SDB was always wrong, my patch fixes it. > > Then 'no longer needed' in your commit message is really confusing. > > Shawn >
On Thu, May 04, 2017 at 04:34:14PM +0200, Marek Vasut wrote: > On 05/04/2017 03:41 PM, Shawn Guo wrote: > > So I guess you do not understand how the OPP hackery was born and why it > > shouldn't be there for mainline kernel at all. > > The OPP hackery is there to keep both regulators configured the same > way, since they share the same input voltage rail IMO. Yes. But configuring both regulators the same way is only required in vendor kernel where 'LDO bypass' mode is used. With 'LDO enable' mode which is the case for upstream kernel, both regulators can be configured differently even they share the same input rail. > If you model the > power distribution correctly, the OPP hackery can be removed. The OPP hackery can be removed even without reg_arm/reg_soc modeling. That's why we can do hackery dropping and reg_arm/reg_soc modeling in separate patches. @Leonard, if someday we support 'LDO bypass' mode in upstream kernel, the OPP hackery needs to be back in some way even with reg_arm/reg_soc modeling in place, right? Or will we have a better way to ensure SW1A rail can always feed a correct voltage directly to reg_arm®_soc? Shawn
On Fri, 2017-05-05 at 09:18 +0800, Shawn Guo wrote: > On Thu, May 04, 2017 at 04:34:14PM +0200, Marek Vasut wrote: > > If you model the > > power distribution correctly, the OPP hackery can be removed. > The OPP hackery can be removed even without reg_arm/reg_soc modeling. > That's why we can do hackery dropping and reg_arm/reg_soc modeling in > separate patches. > > @Leonard, if someday we support 'LDO bypass' mode in upstream kernel, > the OPP hackery needs to be back in some way even with reg_arm/reg_soc > modeling in place, right? Or will we have a better way to ensure SW1A > rail can always feed a correct voltage directly to reg_arm®_soc? Maybe? Or maybe the cpufreq driver could detect this situation and handle it internally. In the vendor tree this the cpufreq driver has a special fsl,arm-soc-shared property for this anyway. I posted another RFC at upstreaming ldo-bypass recently but it did not handle this particular case of a shared input rail. It can be handled separately. See: https://lkml.org/lkml/2017/3/22/640 But getting that series to an acceptable state might take a long time. -- Regards, Leonard
diff --git a/arch/arm/boot/dts/imx6sx-sdb.dts b/arch/arm/boot/dts/imx6sx-sdb.dts index 5bb8fd5..d71da30 100644 --- a/arch/arm/boot/dts/imx6sx-sdb.dts +++ b/arch/arm/boot/dts/imx6sx-sdb.dts @@ -12,23 +12,6 @@ model = "Freescale i.MX6 SoloX SDB RevB Board"; }; -&cpu0 { - operating-points = < - /* kHz uV */ - 996000 1250000 - 792000 1175000 - 396000 1175000 - 198000 1175000 - >; - fsl,soc-operating-points = < - /* ARM kHz SOC uV */ - 996000 1250000 - 792000 1175000 - 396000 1175000 - 198000 1175000 - >; -}; - &i2c1 { clock-frequency = <100000>; pinctrl-names = "default";
The board file for imx6sx-dbg overrides cpufreq operating points to use higher voltages. This is done because the board has a shared rail for VDD_ARM_IN and VDD_SOC_IN and when using LDO bypass the shared voltage needs to be a value suitable for both ARM and SOC. This was introduced in: commit 54183bd7f766 ("ARM: imx6sx-sdb: add revb board and make it default") This only only applies to LDO bypass mode, a feature not present in upstream. When LDOs are enabled the effect is to use higher voltages than necesarry for no good reason. Setting these higher voltages can make some boards fail to boot with ugly semi-random crashes, reminiscent of memory corruption. These failures happen the first time the lowest idle state is used. Remove the OPP override in order to fix those crashes. Signed-off-by: Leonard Crestez <leonard.crestez@nxp.com> --- It's not clear exactly why the crashes happen. Perhaps waking up from idle draws more power than is available? Removing this override is a correct change anyway so maybe there is no need to investigate deeper. arch/arm/boot/dts/imx6sx-sdb.dts | 17 ----------------- 1 file changed, 17 deletions(-)