[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Leonard Crestez]

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(-)

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Fabio Estevam]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Fabio Estevam]

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.

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Leonard Crestez]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Fabio Estevam]

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.

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Marek Vasut]

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 ?

-- 
Best regards,
Marek Vasut

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Shawn Guo]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Marek Vasut]

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
> 


-- 
Best regards,
Marek Vasut

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Marek Vasut]

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


-- 
Best regards,
Marek Vasut

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Shawn Guo]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Leonard Crestez]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Marek Vasut]

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
> 


-- 
Best regards,
Marek Vasut

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Leonard Crestez]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Marek Vasut]

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 .

-- 
Best regards,
Marek Vasut

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Leonard Crestez]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Shawn Guo]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Marek Vasut]

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

-- 
Best regards,
Marek Vasut

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Shawn Guo]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Marek Vasut]

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
> 


-- 
Best regards,
Marek Vasut

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Shawn Guo]

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&reg_soc?

Shawn

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Leonard Crestez]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Peter Chen]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Shawn Guo]

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

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Fabio Estevam]

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.

[PATCH] ARM: dts: imx6sx-sdb: Remove cpufreq OPP override

[Shawn Guo]

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