Re: [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