| Message ID | 20190718143044.25066-1-s.nawrocki@samsung.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Exynos Adaptive Supply Voltage support | expand |
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.
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
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.
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.
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.
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
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().
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 ?
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.