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@xxxxxxxxxxx> >> --- >> (...) > 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
