mbox series

[v2,0/9] Exynos Adaptive Supply Voltage support

Message ID 20190718143044.25066-1-s.nawrocki@samsung.com (mailing list archive)
Headers show
Series Exynos Adaptive Supply Voltage support | expand

Message

This is second iteration of patch series adding ASV (Adaptive Supply 
Voltage) support for Exynos SoCs. The first one can be found at:
https://lore.kernel.org/lkml/20190404171735.12815-1-s.nawrocki@samsung.com

The main changes comparing to the first (RFC) version are:
 - moving ASV data tables from DT to the driver,
 - converting the chipid and the ASV drivers to use regmap,
 - converting the ASV driver to proper platform driver.

I tried the opp-supported-hw bitmask approach as in the Qualcomm CPUFreq
DT bindings but it resulted in too many OPPs and DT nodes, around 200
per CPU cluster. So the ASV OPP tables are now in the ASV driver, as in
downstream kernels.  I might give it a try and restrucure these tables
to avoid data repetition.

This patch set includes Exynos CHIPID driver posted by Pankaj Dubey and
futher improved by Bartlomiej Zolnierkiewicz [1].

Tested on Odroid XU3, XU3 Lite, XU4.

One of the things on TODO list is support for the Adaptive Body Bias.
This will require modifications on the cpufreq driver side in order to 
support multiple voltage regulators and changes in the OPP framework 
to support adding OPPs with multiple voltages.

[1] https://lkml.org/lkml/2018/11/15/908

Pankaj Dubey (3):
  soc: samsung: Add exynos chipid driver support
  ARM: EXYNOS: enable exynos_chipid for ARCH_EXYNOS
  ARM64: EXYNOS: enable exynos_chipid for ARCH_EXYNOS

Sylwester Nawrocki (6):
  soc: samsung: Convert exynos-chipid driver to use the regmap API
  soc: samsung: Add Exynos Adaptive Supply Voltage driver
  ARM: EXYNOS: Enable exynos-asv driver for ARCH_EXYNOS
  soc: samsung: Update the CHIP ID DT binding documentation
  ARM: dts: Add "syscon" compatible string to chipid node
  ARM: dts: Add samsung,asv-bin property for odroidxu3-lite

 .../bindings/arm/samsung/exynos-chipid.txt    |  10 +-
 arch/arm/boot/dts/exynos5.dtsi                |   4 +-
 .../boot/dts/exynos5422-odroidxu3-lite.dts    |   4 +
 arch/arm/mach-exynos/Kconfig                  |   2 +
 arch/arm64/Kconfig.platforms                  |   1 +
 drivers/soc/samsung/Kconfig                   |  16 +
 drivers/soc/samsung/Makefile                  |   5 +
 drivers/soc/samsung/exynos-asv.c              | 185 +++++++
 drivers/soc/samsung/exynos-asv.h              |  82 +++
 drivers/soc/samsung/exynos-chipid.c           | 104 ++++
 drivers/soc/samsung/exynos5422-asv.c          | 499 ++++++++++++++++++
 drivers/soc/samsung/exynos5422-asv.h          |  25 +
 include/linux/soc/samsung/exynos-chipid.h     |  48 ++
 13 files changed, 981 insertions(+), 4 deletions(-)
 create mode 100644 drivers/soc/samsung/exynos-asv.c
 create mode 100644 drivers/soc/samsung/exynos-asv.h
 create mode 100644 drivers/soc/samsung/exynos-chipid.c
 create mode 100644 drivers/soc/samsung/exynos5422-asv.c
 create mode 100644 drivers/soc/samsung/exynos5422-asv.h
 create mode 100644 include/linux/soc/samsung/exynos-chipid.h

Comments

Viresh Kumar July 23, 2019, 2:04 a.m. UTC | #1
On 18-07-19, 16:30, Sylwester Nawrocki wrote:
> This is second iteration of patch series adding ASV (Adaptive Supply 
> Voltage) support for Exynos SoCs. The first one can be found at:
> https://lore.kernel.org/lkml/20190404171735.12815-1-s.nawrocki@samsung.com
> 
> The main changes comparing to the first (RFC) version are:
>  - moving ASV data tables from DT to the driver,
>  - converting the chipid and the ASV drivers to use regmap,
>  - converting the ASV driver to proper platform driver.
> 
> I tried the opp-supported-hw bitmask approach as in the Qualcomm CPUFreq
> DT bindings but it resulted in too many OPPs and DT nodes, around 200
> per CPU cluster. So the ASV OPP tables are now in the ASV driver, as in
> downstream kernels.

Hmm. Can you explain why do you have so many OPPs? How many
frequencies do you actually support per cluster and what all varies
per frequency based on hw ? How many hw version do u have ?

I am asking as the OPP core can be improved to support your case if
possible. But I need to understand the problem first.
Marek Szyprowski July 24, 2019, 1:10 p.m. UTC | #2
Hi Viresh,

On 2019-07-23 04:04, Viresh Kumar wrote:
> On 18-07-19, 16:30, Sylwester Nawrocki wrote:
>> This is second iteration of patch series adding ASV (Adaptive Supply
>> Voltage) support for Exynos SoCs. The first one can be found at:
>> https://lore.kernel.org/lkml/20190404171735.12815-1-s.nawrocki@samsung.com
>>
>> The main changes comparing to the first (RFC) version are:
>>   - moving ASV data tables from DT to the driver,
>>   - converting the chipid and the ASV drivers to use regmap,
>>   - converting the ASV driver to proper platform driver.
>>
>> I tried the opp-supported-hw bitmask approach as in the Qualcomm CPUFreq
>> DT bindings but it resulted in too many OPPs and DT nodes, around 200
>> per CPU cluster. So the ASV OPP tables are now in the ASV driver, as in
>> downstream kernels.
> Hmm. Can you explain why do you have so many OPPs? How many
> frequencies do you actually support per cluster and what all varies
> per frequency based on hw ? How many hw version do u have ?

For big cores there are 20 frequencies (2100MHz .. 200MHz). Each SoC 
might belong to one of the 3 production 'sets' and each set contains 14 
so called 'asv groups', which assign the certain voltage values for each 
of those 20 frequencies (the lower asv group means lower voltage needed 
for given frequency).

> I am asking as the OPP core can be improved to support your case if
> possible. But I need to understand the problem first.


Best regards
Viresh Kumar July 25, 2019, 2:23 a.m. UTC | #3
On 24-07-19, 15:10, Marek Szyprowski wrote:
> Hi Viresh,
> 
> On 2019-07-23 04:04, Viresh Kumar wrote:
> > On 18-07-19, 16:30, Sylwester Nawrocki wrote:
> >> This is second iteration of patch series adding ASV (Adaptive Supply
> >> Voltage) support for Exynos SoCs. The first one can be found at:
> >> https://lore.kernel.org/lkml/20190404171735.12815-1-s.nawrocki@samsung.com
> >>
> >> The main changes comparing to the first (RFC) version are:
> >>   - moving ASV data tables from DT to the driver,
> >>   - converting the chipid and the ASV drivers to use regmap,
> >>   - converting the ASV driver to proper platform driver.
> >>
> >> I tried the opp-supported-hw bitmask approach as in the Qualcomm CPUFreq
> >> DT bindings but it resulted in too many OPPs and DT nodes, around 200
> >> per CPU cluster. So the ASV OPP tables are now in the ASV driver, as in
> >> downstream kernels.
> > Hmm. Can you explain why do you have so many OPPs? How many
> > frequencies do you actually support per cluster and what all varies
> > per frequency based on hw ? How many hw version do u have ?
> 
> For big cores there are 20 frequencies (2100MHz .. 200MHz). Each SoC 
> might belong to one of the 3 production 'sets' and each set contains 14 
> so called 'asv groups', which assign the certain voltage values for each 
> of those 20 frequencies (the lower asv group means lower voltage needed 
> for given frequency).

There is another property which might be useful in this case:
"opp-microvolt-<name>" and then you can use API
dev_pm_opp_set_prop_name() to choose which voltage value to apply to
all OPPs.

opp-supported-hw property is more useful for the cases where only a
subset of frequencies will be supported for different versions of the
SoC. And what you need is a different voltage value for all
frequencies based on some h/w version.
Hi Viresh,

On 7/25/19 04:23, Viresh Kumar wrote:
> On 24-07-19, 15:10, Marek Szyprowski wrote:
>> On 2019-07-23 04:04, Viresh Kumar wrote:
>>> On 18-07-19, 16:30, Sylwester Nawrocki wrote:
>>>> This is second iteration of patch series adding ASV (Adaptive Supply
>>>> Voltage) support for Exynos SoCs. The first one can be found at:
>>>> https://lore.kernel.org/lkml/20190404171735.12815-1-s.nawrocki@samsung.com
>>>>
>>>> The main changes comparing to the first (RFC) version are:
>>>>   - moving ASV data tables from DT to the driver,
>>>>   - converting the chipid and the ASV drivers to use regmap,
>>>>   - converting the ASV driver to proper platform driver.
>>>>
>>>> I tried the opp-supported-hw bitmask approach as in the Qualcomm CPUFreq
>>>> DT bindings but it resulted in too many OPPs and DT nodes, around 200
>>>> per CPU cluster. So the ASV OPP tables are now in the ASV driver, as in
>>>> downstream kernels.
>>> 
>>> Hmm. Can you explain why do you have so many OPPs? How many
>>> frequencies do you actually support per cluster and what all varies
>>> per frequency based on hw ? How many hw version do u have ?
>>
>> For big cores there are 20 frequencies (2100MHz .. 200MHz). Each SoC 
>> might belong to one of the 3 production 'sets' and each set contains 14 
>> so called 'asv groups', which assign the certain voltage values for each 
>> of those 20 frequencies (the lower asv group means lower voltage needed 
>> for given frequency).
>
> There is another property which might be useful in this case:
> "opp-microvolt-<name>" and then you can use API
> dev_pm_opp_set_prop_name() to choose which voltage value to apply to
> all OPPs.

Thank you for your suggestions.

For some Exynos SoC variants the algorithm of selecting CPU voltage supply
is a bit more complex than just selecting a column in the frequency/voltage 
matrix, i.e. selecting a set of voltage values for whole frequency range.

Frequency range could be divided into sub-ranges and to each such a sub-range 
part of different column could be assigned, depending on data fused in 
the CHIPID block registers.

We could create OPP node for each frequency and specify all needed voltages 
as a list of "opp-microvolt-<name>" properties but apart from the fact that 
it would have been quite many properties, e.g. 42 (3 tables * 14 columns), 
only for some SoC types the dev_pm_opp_set_prop_name() approach could be 
used. We would need to be able to set opp-microvolt-* property name 
separately for each frequency (OPP).

Probably most future proof would be a DT binding where we could still 
re-create those Exynos-specific ASV tables from DT. For example add named 
opp-microvolt-* properties or something similar to hold rows of each ASV 
table. But that conflicts with "operating-points-v2" binding, where 
multiple OPP voltage values are described by just named properties and 
multiple entries correspond to min/target/max.

opp_table0 {
	compatible = "...", "operating-points-v2";
	opp-shared;
	opp-2100000000 {
		opp-hz = /bits/ 64 <1800000000>;
		opp-microvolt = <...>;
		opp-microvolt-t1 = <1362500>, <1350000>, ....;
		opp-microvolt-t2 = <1362500>, <1360000>, ....;
		opp-microvolt-t3 = <1362500>, <1340000>, ....;
	};
	...
	opp-200000000 {
		opp-hz = /bits/ 64 <200000000>;
		opp-microvolt = <...>;
		opp-microvolt-t1 = <900000>, <900000>, ....;
		opp-microvolt-t2 = <900000>, <900000>, ....;
		opp-microvolt-t3 = <900000>, <900000>, ....;
	};
};

I might be missing some information now on how those Exynos ASV tables 
are used on other SoCs that would need to be supported.

There will be even more data to include when adding support for the Body
Bias voltage, for each CPU supply voltage we could possibly have 
corresponding Body Bias voltage.
Viresh Kumar Aug. 19, 2019, 9:09 a.m. UTC | #5
On 09-08-19, 17:58, Sylwester Nawrocki wrote:
> Thank you for your suggestions.
> 
> For some Exynos SoC variants the algorithm of selecting CPU voltage supply
> is a bit more complex than just selecting a column in the frequency/voltage 
> matrix, i.e. selecting a set of voltage values for whole frequency range.
> 
> Frequency range could be divided into sub-ranges and to each such a sub-range 
> part of different column could be assigned, depending on data fused in 
> the CHIPID block registers.
> 
> We could create OPP node for each frequency and specify all needed voltages 
> as a list of "opp-microvolt-<name>" properties but apart from the fact that 
> it would have been quite many properties, e.g. 42 (3 tables * 14 columns), 
> only for some SoC types the dev_pm_opp_set_prop_name() approach could be 
> used. We would need to be able to set opp-microvolt-* property name 
> separately for each frequency (OPP).
> 
> Probably most future proof would be a DT binding where we could still 
> re-create those Exynos-specific ASV tables from DT. For example add named 
> opp-microvolt-* properties or something similar to hold rows of each ASV 
> table. But that conflicts with "operating-points-v2" binding, where 
> multiple OPP voltage values are described by just named properties and 
> multiple entries correspond to min/target/max.
> 
> opp_table0 {
> 	compatible = "...", "operating-points-v2";
> 	opp-shared;
> 	opp-2100000000 {
> 		opp-hz = /bits/ 64 <1800000000>;
> 		opp-microvolt = <...>;
> 		opp-microvolt-t1 = <1362500>, <1350000>, ....;
> 		opp-microvolt-t2 = <1362500>, <1360000>, ....;
> 		opp-microvolt-t3 = <1362500>, <1340000>, ....;
> 	};
> 	...
> 	opp-200000000 {
> 		opp-hz = /bits/ 64 <200000000>;
> 		opp-microvolt = <...>;
> 		opp-microvolt-t1 = <900000>, <900000>, ....;
> 		opp-microvolt-t2 = <900000>, <900000>, ....;
> 		opp-microvolt-t3 = <900000>, <900000>, ....;
> 	};
> };
> 
> I might be missing some information now on how those Exynos ASV tables 
> are used on other SoCs that would need to be supported.
> 
> There will be even more data to include when adding support for the Body
> Bias voltage, for each CPU supply voltage we could possibly have 
> corresponding Body Bias voltage.

Will something like this help ?

https://lore.kernel.org/lkml/1442623929-4507-3-git-send-email-sboyd@codeaurora.org/

This never got merged but the idea was AVS only.
Viresh Kumar Aug. 19, 2019, 10:06 a.m. UTC | #6
On 19-08-19, 14:39, Viresh Kumar wrote:
> On 09-08-19, 17:58, Sylwester Nawrocki wrote:
> > Thank you for your suggestions.
> > 
> > For some Exynos SoC variants the algorithm of selecting CPU voltage supply
> > is a bit more complex than just selecting a column in the frequency/voltage 
> > matrix, i.e. selecting a set of voltage values for whole frequency range.
> > 
> > Frequency range could be divided into sub-ranges and to each such a sub-range 
> > part of different column could be assigned, depending on data fused in 
> > the CHIPID block registers.
> > 
> > We could create OPP node for each frequency and specify all needed voltages 
> > as a list of "opp-microvolt-<name>" properties but apart from the fact that 
> > it would have been quite many properties, e.g. 42 (3 tables * 14 columns), 
> > only for some SoC types the dev_pm_opp_set_prop_name() approach could be 
> > used. We would need to be able to set opp-microvolt-* property name 
> > separately for each frequency (OPP).
> > 
> > Probably most future proof would be a DT binding where we could still 
> > re-create those Exynos-specific ASV tables from DT. For example add named 
> > opp-microvolt-* properties or something similar to hold rows of each ASV 
> > table. But that conflicts with "operating-points-v2" binding, where 
> > multiple OPP voltage values are described by just named properties and 
> > multiple entries correspond to min/target/max.
> > 
> > opp_table0 {
> > 	compatible = "...", "operating-points-v2";
> > 	opp-shared;
> > 	opp-2100000000 {
> > 		opp-hz = /bits/ 64 <1800000000>;
> > 		opp-microvolt = <...>;
> > 		opp-microvolt-t1 = <1362500>, <1350000>, ....;
> > 		opp-microvolt-t2 = <1362500>, <1360000>, ....;
> > 		opp-microvolt-t3 = <1362500>, <1340000>, ....;
> > 	};
> > 	...
> > 	opp-200000000 {
> > 		opp-hz = /bits/ 64 <200000000>;
> > 		opp-microvolt = <...>;
> > 		opp-microvolt-t1 = <900000>, <900000>, ....;
> > 		opp-microvolt-t2 = <900000>, <900000>, ....;
> > 		opp-microvolt-t3 = <900000>, <900000>, ....;
> > 	};
> > };
> > 
> > I might be missing some information now on how those Exynos ASV tables 
> > are used on other SoCs that would need to be supported.
> > 
> > There will be even more data to include when adding support for the Body
> > Bias voltage, for each CPU supply voltage we could possibly have 
> > corresponding Body Bias voltage.
> 
> Will something like this help ?
> 
> https://lore.kernel.org/lkml/1442623929-4507-3-git-send-email-sboyd@codeaurora.org/
> 
> This never got merged but the idea was AVS only.

Here is a recent version under review.

https://lore.kernel.org/lkml/1565703113-31479-1-git-send-email-andrew-sh.cheng@mediatek.com
On 8/19/19 11:09, Viresh Kumar wrote:
> Will something like this help ?
> 
> https://lore.kernel.org/lkml/1442623929-4507-3-git-send-email-sboyd@codeaurora.org/
> 
> This never got merged but the idea was AVS only.

It's quite interesting work, it seems to be for a more advanced use case 
where OPP voltage is being adjusted at runtime.

We could use it instead of removing an OPP and then adding with updated 
voltage. On Exynos there is there is just a need to update OPPs once at boot 
time, so it is more "static". However the requirements could presumably 
change in future.

If that's your preference I could switch to that notifier approach.
AFAICS the API would still need to be extended to support multiple voltages,
when in future we add support for the Body Bias regulator.
Viresh Kumar Aug. 19, 2019, 11:25 a.m. UTC | #8
On 19-08-19, 13:16, Sylwester Nawrocki wrote:
> On 8/19/19 11:09, Viresh Kumar wrote:
> > Will something like this help ?
> > 
> > https://lore.kernel.org/lkml/1442623929-4507-3-git-send-email-sboyd@codeaurora.org/
> > 
> > This never got merged but the idea was AVS only.
> 
> It's quite interesting work, it seems to be for a more advanced use case 
> where OPP voltage is being adjusted at runtime.
> 
> We could use it instead of removing an OPP and then adding with updated 
> voltage. On Exynos there is there is just a need to update OPPs once at boot 
> time, so it is more "static". However the requirements could presumably 
> change in future.

The API is about changing the values after they are parsed once from DT. You can
change it once or multiple times depending on the use case.

> If that's your preference I could switch to that notifier approach.

You shouldn't be required to use the notifier. Just add the OPP table and update
the values right after that. So no one would be using the values at that time.

> AFAICS the API would still need to be extended to support multiple voltages,
> when in future we add support for the Body Bias regulator. 

Right.

Will this patchset solve the problems for you and make your DT light weight ?
On 8/19/19 13:25, Viresh Kumar wrote:
> On 19-08-19, 13:16, Sylwester Nawrocki wrote:
>> On 8/19/19 11:09, Viresh Kumar wrote:
>>> Will something like this help ?
>>>
>>> https://lore.kernel.org/lkml/1442623929-4507-3-git-send-email-sboyd@codeaurora.org/
>>>
>>> This never got merged but the idea was AVS only.
>>
>> It's quite interesting work, it seems to be for a more advanced use case 
>> where OPP voltage is being adjusted at runtime.
>>
>> We could use it instead of removing an OPP and then adding with updated 
>> voltage. On Exynos there is there is just a need to update OPPs once at boot 
>> time, so it is more "static". However the requirements could presumably 
>> change in future.
> 
> The API is about changing the values after they are parsed once from DT. You can
> change it once or multiple times depending on the use case.
> 
>> If that's your preference I could switch to that notifier approach.
> 
> You shouldn't be required to use the notifier. Just add the OPP table and update
> the values right after that. So no one would be using the values at that time.

OK, now I see dev_pm_opp_adjust_voltage() actually changes OPP's voltage 
and the notifier is optional.

> Will this patchset solve the problems for you and make your DT light weight ?

Unfortunately not, the patch set as I see it is another way of updating 
an OPP after it was parsed from DT.  OPP remove/add could work equally 
well in our use case.
 
The problem is that we have the information on how to translate the 
common OPP voltage to a voltage specific to given silicon encoded jointly 
in the ASV tables and the CHIPID registers (efuse/OTP memory). 
Additionally, algorithm of selecting ASV data (OPP voltage) based on 
the "key" data from registers is not generic, it is usually different 
per each SoC type.

I tried to identify some patterns in those tables in order to simplify 
possible DT binding, but that was not really successful. I ended up just 
keeping whole tables.

--
Regards, 
Sylwester
Viresh Kumar Aug. 20, 2019, 3:01 a.m. UTC | #10
On 19-08-19, 15:39, Sylwester Nawrocki wrote:
> Unfortunately not, the patch set as I see it is another way of updating 
> an OPP after it was parsed from DT.  OPP remove/add could work equally 
> well in our use case.

Adding OPPs dynamically has limitations, you can't set many values which are
otherwise possible with DT. And removing/adding is not the right thing to do
technically.

> The problem is that we have the information on how to translate the 
> common OPP voltage to a voltage specific to given silicon encoded jointly 
> in the ASV tables and the CHIPID registers (efuse/OTP memory). 
> Additionally, algorithm of selecting ASV data (OPP voltage) based on 
> the "key" data from registers is not generic, it is usually different 
> per each SoC type.
> 
> I tried to identify some patterns in those tables in order to simplify 
> possible DT binding, but that was not really successful. I ended up just 
> keeping whole tables.

Sorry but I am unable to understand the difficulty you are facing now. So what I
suggest is something like this.

- Use DT to get a frequency and voltage for each frequency.
- At runtime, based on SoC, registers, efuses, etc, update the voltage of the
  OPPs.
- This algo can be different for each SoC, no one is stopping you from doing
  that.

Am I missing something ?
On 8/20/19 05:01, Viresh Kumar wrote:
> On 19-08-19, 15:39, Sylwester Nawrocki wrote:
>> Unfortunately not, the patch set as I see it is another way of updating 
>> an OPP after it was parsed from DT.  OPP remove/add could work equally 
>> well in our use case.
> 
> Adding OPPs dynamically has limitations, you can't set many values which are
> otherwise possible with DT. And removing/adding is not the right thing to do
> technically.

Thanks for explanation, I was not aware of that.

>> The problem is that we have the information on how to translate the 
>> common OPP voltage to a voltage specific to given silicon encoded jointly 
>> in the ASV tables and the CHIPID registers (efuse/OTP memory). 
>> Additionally, algorithm of selecting ASV data (OPP voltage) based on 
>> the "key" data from registers is not generic, it is usually different 
>> per each SoC type.
>>
>> I tried to identify some patterns in those tables in order to simplify 
>> possible DT binding, but that was not really successful. I ended up just 
>> keeping whole tables.
> 
> 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. 
 
> - Use DT to get a frequency and voltage for each frequency.

Yes, this is what happens now, we have common OPPs in DT that work for each SoC
revision. 

> - At runtime, based on SoC, registers, efuses, etc, update the voltage of the
>   OPPs.
> - This algo can be different for each SoC, no one is stopping you from doing
>   that.
> 
> Am I missing something ?

Not really, this is basically what happens in the $subject patch series. 

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().
Viresh Kumar Aug. 20, 2019, 9:21 a.m. UTC | #12
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.

> > - Use DT to get a frequency and voltage for each frequency.
> 
> Yes, this is what happens now, we have common OPPs in DT that work for each SoC
> revision. 
> 
> > - At runtime, based on SoC, registers, efuses, etc, update the voltage of the
> >   OPPs.
> > - This algo can be different for each SoC, no one is stopping you from doing
> >   that.
> > 
> > Am I missing something ?
> 
> Not really, this is basically what happens in the $subject patch series. 
> 
> 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.
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 ?
Viresh Kumar Sept. 5, 2019, 5:01 a.m. UTC | #14
On 04-09-19, 14:37, Sylwester Nawrocki wrote:
> 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 ?

The voltage value in the OPP core is stored as a triplet,
min/max/target kind of stuff. You can check DT binding for OPPs and
see the details. That's called as triplet.