Re: [PATCH v2 0/9] Exynos Adaptive Supply Voltage support

From: Sylwester Nawrocki
Date: Wed Sep 04 2019 - 08:37:47 EST


Hi,

On 8/20/19 11:21, Viresh Kumar wrote:
> On 20-08-19, 11:03, Sylwester Nawrocki wrote:
>> On 8/20/19 05:01, Viresh Kumar wrote:
>>> Sorry but I am unable to understand the difficulty you are facing now. So what I
>>> suggest is something like this.
>>
>> The difficulty was about representing data from tables asv_{arm,kfc}_table[][]
>> added in patch 3/9 of the series in devicetree. If you have no objections
>> about keeping those tables in the driver then I can't see any difficulties.
>
> The problem with keeping such tables in kernel is that they contain too much
> magic values which very few people understand. And after some amount of time,
> even they don't remember any of it.
>
> What I was expecting was to remove as much of these tables as possible and do
> the calculations to get them at runtime with some logical code which people can
> understand later on.

It might be indeed far from a good design but this is all what we get from the
SoC vendor. AFAIU those tables are generated based on data from the production
process, likely from some measurements.

I am afraid I will not get more information for that fairly old SoCs in order to
replace those tables with some sensible code generating the data programmatically,
I'm not sure it would be at all possible to do.

I could add some more comments, similar to description as in my previous RFC
DT binding (https://patchwork.kernel.org/patch/10886013).

The tables are rather simple, it's mostly all voltage values, only selecting
values per each frequency and chip revision might get a bit complex.
I'm not sure we could change that now though.
>> Then IIUC what I would need to change is to modify exynos_asv_update_cpu_opps()
>> function in patch 3/9 to use dev_pm_opp_adjust_voltage() rather than
>> dev_pm_opp_remove(), dev_pm_opp_add().
>
> That and somehow add code to get those tables if possible.

I have changed the code to use dev_pm_opp_adjust_voltage(). I was wondering
though, what did you mean by "triplet" when commenting on this patch
https://patchwork.kernel.org/patch/11092245 ?

--
Regards,
Sylwester