Re: [PATCH v2] soc: samsung: Add simple voltage coupler for Exynos5800

[Marek Szyprowski <m.szyprowski@samsung.com>]

Hi Dmitry,

On 02.06.2020 17:15, Dmitry Osipenko wrote:
> 02.06.2020 16:02, Marek Szyprowski пишет:
>> Add a simple custom voltage regulator coupler for Exynos5800 SoCs, which
>> require coupling between "vdd_arm" and "vdd_int" regulators. This coupler
>> ensures that the voltage balancing for the coupled regulators is done
>> only when clients for the each regulator apply their constraints, so the
>> voltage values don't go beyond the bootloader-selected operation point
>> during the boot process. This also ensures proper voltage balancing if
>> any of the client driver is missing.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   (...)
> Hello Marek,
>
> Does this mean that you're going to allow to violate the coupling
> constraints while coupled regulator has no consumers?
>
> I don't think that you may want to skip the coupled balancing ever.
> Instead you may want to assume that the min-voltage constraint equals to
> the current regulator's voltage while the coupled regulator has no
> consumers.
>
> Yours variant of the balancer doesn't prevent the voltage dropping on
> regulator's enabling while coupled regulator doesn't have active
> consumers. This is the problem which we previously had once OPP code was
> changed to enable regulator.
>
> Secondly, yours variant of the balancer also doesn't handle the case
> where set_voltage() is invoked while one of the couples doesn't have
> active consumers because voltage of this couple may drop more than
> allowed on the voltage re-balancing.
Indeed. I've focused on disabling balancing when there are no consumers 
and I didn't notice that the max_spread might be violated in such case.
> I'd suggest to simply change the regulator_get_optimal_voltage() to
> limit the desired_min_uV to the current voltage if coupled regulator has
> no consumers.

Right, this sounds like a best solution. I have an idea to try to add it 
again to the core as a simple check: if regultor is boot_on, use current 
voltage as min_uV until a consumer is registered. I've checked and this 
approach fixes the issue. I will submit a patch in a few minutes.

> I don't think that any of the today's upstream kernel coupled-regulator
> users really need to support the case where a regulator couple is
> allowed *not* to have active consumers, so for now it should be fine to
> change the core code to accommodate the needs of the Exynos regulators
> (IMO). We may get back to this later on once there will be a real need
> support that case.
>
> Please also note that I'm assuming that each of the coupled regulators
> doesn't have more than one consumer at a time in yours case (correct?),
> because yours solution won't work well in a case of multiple consumers.
> There is no universal solution for this bootstrapping problem yet.

There are only a single consumers for each coupled regulator (cpufreq 
for vdd_arm and devfreq for vdd_int).

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland