| Message ID | 20190724014222.110767-1-saravanak@google.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add required-opps support to devfreq passive gov | expand |
On 23-07-19, 18:42, Saravana Kannan wrote: > The devfreq passive governor scales the frequency of a "child" device based > on the current frequency of a "parent" device (not parent/child in the > sense of device hierarchy). As of today, the passive governor requires one > of the following to work correctly: > 1. The parent and child device have the same number of frequencies > 2. The child device driver passes a mapping function to translate from > parent frequency to child frequency. > v3 -> v4: > - Fixed documentation comments > - Fixed order of functions in .h file > - Renamed the new xlate API > - Caused _set_required_opps() to fail if all required opps tables aren't > linked. We are already in the middle of a discussion for your previous version and I haven't said yet that I am happy with what you suggested just 2 days back. Why send another version so soon ? The next merge window is also very far in time from now. Please wait for a few days before sending newer versions, I will continue discussion on the previous version only for now.
On Wed, Jul 24, 2019 at 7:30 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 23-07-19, 18:42, Saravana Kannan wrote: > > The devfreq passive governor scales the frequency of a "child" device based > > on the current frequency of a "parent" device (not parent/child in the > > sense of device hierarchy). As of today, the passive governor requires one > > of the following to work correctly: > > 1. The parent and child device have the same number of frequencies > > 2. The child device driver passes a mapping function to translate from > > parent frequency to child frequency. > > > v3 -> v4: > > - Fixed documentation comments > > - Fixed order of functions in .h file > > - Renamed the new xlate API > > - Caused _set_required_opps() to fail if all required opps tables aren't > > linked. > > We are already in the middle of a discussion for your previous version > and I haven't said yet that I am happy with what you suggested just 2 > days back. Why send another version so soon ? I wanted you to see how I addressed your comments. It didn't look like you were going to make more comments on the code. > The next merge window is > also very far in time from now. > > Please wait for a few days before sending newer versions, I will > continue discussion on the previous version only for now. Ok -Saravana
On 24-07-19, 20:40, Saravana Kannan wrote: > On Wed, Jul 24, 2019 at 7:30 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > On 23-07-19, 18:42, Saravana Kannan wrote: > > > The devfreq passive governor scales the frequency of a "child" device based > > > on the current frequency of a "parent" device (not parent/child in the > > > sense of device hierarchy). As of today, the passive governor requires one > > > of the following to work correctly: > > > 1. The parent and child device have the same number of frequencies > > > 2. The child device driver passes a mapping function to translate from > > > parent frequency to child frequency. > > > > > v3 -> v4: > > > - Fixed documentation comments > > > - Fixed order of functions in .h file > > > - Renamed the new xlate API > > > - Caused _set_required_opps() to fail if all required opps tables aren't > > > linked. > > > > We are already in the middle of a discussion for your previous version > > and I haven't said yet that I am happy with what you suggested just 2 > > days back. Why send another version so soon ? > > I wanted you to see how I addressed your comments. Sure, but that is just half the comments. > It didn't look like > you were going to make more comments on the code. I posted some queries and you posted your opinions on them. Now shouldn't I get a chance to reply again to see if I agree with your replies or if we can settle to something else ? I only got one day in between where I was busy with other stuff and so couldn't come back to it. Please wait a little longer specially when the comments aren't minor in nature. Anyway, lets get over it now. Lets continue our discussion on V3 and then we can have a V5 :) Have a good day Saravana.
On Wed, Jul 24, 2019 at 10:22 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 24-07-19, 20:40, Saravana Kannan wrote: > > On Wed, Jul 24, 2019 at 7:30 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > > > > > On 23-07-19, 18:42, Saravana Kannan wrote: > > > > The devfreq passive governor scales the frequency of a "child" device based > > > > on the current frequency of a "parent" device (not parent/child in the > > > > sense of device hierarchy). As of today, the passive governor requires one > > > > of the following to work correctly: > > > > 1. The parent and child device have the same number of frequencies > > > > 2. The child device driver passes a mapping function to translate from > > > > parent frequency to child frequency. > > > > > > > v3 -> v4: > > > > - Fixed documentation comments > > > > - Fixed order of functions in .h file > > > > - Renamed the new xlate API > > > > - Caused _set_required_opps() to fail if all required opps tables aren't > > > > linked. > > > > > > We are already in the middle of a discussion for your previous version > > > and I haven't said yet that I am happy with what you suggested just 2 > > > days back. Why send another version so soon ? > > > > I wanted you to see how I addressed your comments. > > Sure, but that is just half the comments. > > > It didn't look like > > you were going to make more comments on the code. > > I posted some queries and you posted your opinions on them. Now > shouldn't I get a chance to reply again to see if I agree with your > replies or if we can settle to something else ? I only got one day in > between where I was busy with other stuff and so couldn't come back to > it. Please wait a little longer specially when the comments aren't > minor in nature. Sorry if it came off as trying to rush you. That wasn't the intention. Just some misunderstanding on my part. > Anyway, lets get over it now. Lets continue our discussion on V3 and > then we can have a V5 :) > > Have a good day Saravana. Sounds good. You too Viresh! :) -Saravana
Dear Saravana, Any other progress of this series? Regards, Chanwoo Choi On 7/24/19 10:42 AM, Saravana Kannan wrote: > The devfreq passive governor scales the frequency of a "child" device based > on the current frequency of a "parent" device (not parent/child in the > sense of device hierarchy). As of today, the passive governor requires one > of the following to work correctly: > 1. The parent and child device have the same number of frequencies > 2. The child device driver passes a mapping function to translate from > parent frequency to child frequency. > > When (1) is not true, (2) is the only option right now. But often times, > all that is required is a simple mapping from parent's frequency to child's > frequency. > > Since OPPs already support pointing to other "required-opps", add support > for using that to map from parent device frequency to child device > frequency. That way, every child device driver doesn't have to implement a > separate mapping function anytime (1) isn't true. > > Some common (but not comprehensive) reason for needing a devfreq passive > governor to adjust the frequency of one device based on another are: > > 1. These were the combination of frequencies that were validated/screened > during the manufacturing process. > 2. These are the sensible performance combinations between two devices > interacting with each other. So that when one runs fast the other > doesn't become the bottleneck. > 3. Hardware bugs requiring some kind of frequency ratio between devices. > > For example, the following mapping can't be captured in DT as it stands > today because the parent and child device have different number of OPPs. > But with this patch series, this mapping can be captured cleanly. > > In arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi you have something > like this with the following changes: > > bus_g2d_400: bus0 { > compatible = "samsung,exynos-bus"; > clocks = <&cmu_top CLK_ACLK_G2D_400>; > clock-names = "bus"; > operating-points-v2 = <&bus_g2d_400_opp_table>; > status = "disabled"; > }; > > bus_noc2: bus9 { > compatible = "samsung,exynos-bus"; > clocks = <&cmu_mif CLK_ACLK_BUS2_400>; > clock-names = "bus"; > operating-points-v2 = <&bus_noc2_opp_table>; > status = "disabled"; > }; > > bus_g2d_400_opp_table: opp_table2 { > compatible = "operating-points-v2"; > opp-shared; > > opp-400000000 { > opp-hz = /bits/ 64 <400000000>; > opp-microvolt = <1075000>; > required-opps = <&noc2_400>; > }; > opp-267000000 { > opp-hz = /bits/ 64 <267000000>; > opp-microvolt = <1000000>; > required-opps = <&noc2_200>; > }; > opp-200000000 { > opp-hz = /bits/ 64 <200000000>; > opp-microvolt = <975000>; > required-opps = <&noc2_200>; > }; > opp-160000000 { > opp-hz = /bits/ 64 <160000000>; > opp-microvolt = <962500>; > required-opps = <&noc2_134>; > }; > opp-134000000 { > opp-hz = /bits/ 64 <134000000>; > opp-microvolt = <950000>; > required-opps = <&noc2_134>; > }; > opp-100000000 { > opp-hz = /bits/ 64 <100000000>; > opp-microvolt = <937500>; > required-opps = <&noc2_100>; > }; > }; > > bus_noc2_opp_table: opp_table6 { > compatible = "operating-points-v2"; > > noc2_400: opp-400000000 { > opp-hz = /bits/ 64 <400000000>; > }; > noc2_200: opp-200000000 { > opp-hz = /bits/ 64 <200000000>; > }; > noc2_134: opp-134000000 { > opp-hz = /bits/ 64 <134000000>; > }; > noc2_100: opp-100000000 { > opp-hz = /bits/ 64 <100000000>; > }; > }; > > -Saravana > > v3 -> v4: > - Fixed documentation comments > - Fixed order of functions in .h file > - Renamed the new xlate API > - Caused _set_required_opps() to fail if all required opps tables aren't > linked. > v2 -> v3: > - Rebased onto linux-next. > - Added documentation comment for new fields. > - Added support for lazy required-opps linking. > - Updated Ack/Reviewed-bys. > v1 -> v2: > - Cached OPP table reference in devfreq to avoid looking up every time. > - Renamed variable in passive governor to be more intuitive. > - Updated cover letter with examples. > > > Saravana Kannan (5): > OPP: Allow required-opps even if the device doesn't have power-domains > OPP: Add function to look up required OPP's for a given OPP > OPP: Improve required-opps linking > PM / devfreq: Cache OPP table reference in devfreq > PM / devfreq: Add required OPPs support to passive governor > > drivers/devfreq/devfreq.c | 6 ++ > drivers/devfreq/governor_passive.c | 20 ++++-- > drivers/opp/core.c | 83 +++++++++++++++++++--- > drivers/opp/of.c | 108 ++++++++++++++--------------- > drivers/opp/opp.h | 5 ++ > include/linux/devfreq.h | 2 + > include/linux/pm_opp.h | 11 +++ > 7 files changed, 165 insertions(+), 70 deletions(-) >
On Wed, Nov 13, 2019 at 11:48 PM Chanwoo Choi <cw00.choi@samsung.com> wrote: > > Dear Saravana, > > Any other progress of this series? Hi Chanwoo, Thanks for checking. I haven't abandoned this patch series. This patch series depends on "lazy linking" of required-opps to avoid a cyclic dependency between devfreq and OPP table population. But Viresh wasn't happy with my implementation of the lazy liking for reasonable reasons. I had a chat with Viresh during one of the several conferences that I met him at. To fix the lazy linking in the way he wanted it meant we had to fix other issues in the OPP framework that arise when OPP tables are shared in DT but not in memory. So he was kind enough to sign up to add lazy linking support to OPPs so that I won't have to do it. So, I'm waiting on that. So once that's added, I should be able to drop a few patches in this series, do some minor updates and then this will be good to go. Thanks, Saravana
On 14-11-19, 00:23, Saravana Kannan wrote: > Thanks for checking. I haven't abandoned this patch series. This patch > series depends on "lazy linking" of required-opps to avoid a cyclic > dependency between devfreq and OPP table population. But Viresh wasn't > happy with my implementation of the lazy liking for reasonable > reasons. > > I had a chat with Viresh during one of the several conferences that I > met him at. To fix the lazy linking in the way he wanted it meant we > had to fix other issues in the OPP framework that arise when OPP > tables are shared in DT but not in memory. So he was kind enough to > sign up to add lazy linking support to OPPs so that I won't have to do > it. So, I'm waiting on that. So once that's added, I should be able to > drop a few patches in this series, do some minor updates and then this > will be good to go. I am fixing few other issues in OPP core right now and lazy linking is next in the list :)
Hi Saravana, On 11/14/19 5:23 PM, Saravana Kannan wrote: > On Wed, Nov 13, 2019 at 11:48 PM Chanwoo Choi <cw00.choi@samsung.com> wrote: >> >> Dear Saravana, >> >> Any other progress of this series? > > Hi Chanwoo, > > Thanks for checking. I haven't abandoned this patch series. This patch > series depends on "lazy linking" of required-opps to avoid a cyclic > dependency between devfreq and OPP table population. But Viresh wasn't > happy with my implementation of the lazy liking for reasonable > reasons. > > I had a chat with Viresh during one of the several conferences that I > met him at. To fix the lazy linking in the way he wanted it meant we > had to fix other issues in the OPP framework that arise when OPP > tables are shared in DT but not in memory. So he was kind enough to > sign up to add lazy linking support to OPPs so that I won't have to do > it. So, I'm waiting on that. So once that's added, I should be able to > drop a few patches in this series, do some minor updates and then this > will be good to go. Thanks for the detailed explanation. I'll expect the your next version. Thanks, Chanwoo Choi
On Thu, Nov 14, 2019 at 12:35 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 14-11-19, 00:23, Saravana Kannan wrote: > > Thanks for checking. I haven't abandoned this patch series. This patch > > series depends on "lazy linking" of required-opps to avoid a cyclic > > dependency between devfreq and OPP table population. But Viresh wasn't > > happy with my implementation of the lazy liking for reasonable > > reasons. > > > > I had a chat with Viresh during one of the several conferences that I > > met him at. To fix the lazy linking in the way he wanted it meant we > > had to fix other issues in the OPP framework that arise when OPP > > tables are shared in DT but not in memory. So he was kind enough to > > sign up to add lazy linking support to OPPs so that I won't have to do > > it. So, I'm waiting on that. So once that's added, I should be able to > > drop a few patches in this series, do some minor updates and then this > > will be good to go. > > I am fixing few other issues in OPP core right now and lazy linking > is next in the list :) Thanks Viresh! Glad the offer still stands :) -Saravana
Hi Saravana, 2019년 11월 14일 (목) 오후 5:36, Chanwoo Choi <cw00.choi@samsung.com>님이 작성: > > Hi Saravana, > > On 11/14/19 5:23 PM, Saravana Kannan wrote: > > On Wed, Nov 13, 2019 at 11:48 PM Chanwoo Choi <cw00.choi@samsung.com> wrote: > >> > >> Dear Saravana, > >> > >> Any other progress of this series? > > > > Hi Chanwoo, > > > > Thanks for checking. I haven't abandoned this patch series. This patch > > series depends on "lazy linking" of required-opps to avoid a cyclic > > dependency between devfreq and OPP table population. But Viresh wasn't > > happy with my implementation of the lazy liking for reasonable > > reasons. > > > > I had a chat with Viresh during one of the several conferences that I > > met him at. To fix the lazy linking in the way he wanted it meant we > > had to fix other issues in the OPP framework that arise when OPP > > tables are shared in DT but not in memory. So he was kind enough to > > sign up to add lazy linking support to OPPs so that I won't have to do > > it. So, I'm waiting on that. So once that's added, I should be able to > > drop a few patches in this series, do some minor updates and then this > > will be good to go. > > Thanks for the detailed explanation. I'll expect the your next version. As I know, the lazy linking issue was fixed by Viresh. If possible, I want to know your plan about 'required-opp' with passive governor. Because I think that the patch[1] is good for devfreq device. As you mentioned, you have the other idea to implement the 'cpu based scaling support' to passive governor without cpu notifier. Actually, if there are no any other best solution, I prefer to use cpu notifier for 'cpu based scaling support to passive_governor'. [1] https://patchwork.kernel.org/patch/11046147/ - [RFC,v2] PM / devfreq: Add cpu based scaling support to passive_governor
The devfreq passive governor scales the frequency of a "child" device based on the current frequency of a "parent" device (not parent/child in the sense of device hierarchy). As of today, the passive governor requires one of the following to work correctly: 1. The parent and child device have the same number of frequencies 2. The child device driver passes a mapping function to translate from parent frequency to child frequency. When (1) is not true, (2) is the only option right now. But often times, all that is required is a simple mapping from parent's frequency to child's frequency. Since OPPs already support pointing to other "required-opps", add support for using that to map from parent device frequency to child device frequency. That way, every child device driver doesn't have to implement a separate mapping function anytime (1) isn't true. Some common (but not comprehensive) reason for needing a devfreq passive governor to adjust the frequency of one device based on another are: 1. These were the combination of frequencies that were validated/screened during the manufacturing process. 2. These are the sensible performance combinations between two devices interacting with each other. So that when one runs fast the other doesn't become the bottleneck. 3. Hardware bugs requiring some kind of frequency ratio between devices. For example, the following mapping can't be captured in DT as it stands today because the parent and child device have different number of OPPs. But with this patch series, this mapping can be captured cleanly. In arch/arm64/boot/dts/exynos/exynos5433-bus.dtsi you have something like this with the following changes: bus_g2d_400: bus0 { compatible = "samsung,exynos-bus"; clocks = <&cmu_top CLK_ACLK_G2D_400>; clock-names = "bus"; operating-points-v2 = <&bus_g2d_400_opp_table>; status = "disabled"; }; bus_noc2: bus9 { compatible = "samsung,exynos-bus"; clocks = <&cmu_mif CLK_ACLK_BUS2_400>; clock-names = "bus"; operating-points-v2 = <&bus_noc2_opp_table>; status = "disabled"; }; bus_g2d_400_opp_table: opp_table2 { compatible = "operating-points-v2"; opp-shared; opp-400000000 { opp-hz = /bits/ 64 <400000000>; opp-microvolt = <1075000>; required-opps = <&noc2_400>; }; opp-267000000 { opp-hz = /bits/ 64 <267000000>; opp-microvolt = <1000000>; required-opps = <&noc2_200>; }; opp-200000000 { opp-hz = /bits/ 64 <200000000>; opp-microvolt = <975000>; required-opps = <&noc2_200>; }; opp-160000000 { opp-hz = /bits/ 64 <160000000>; opp-microvolt = <962500>; required-opps = <&noc2_134>; }; opp-134000000 { opp-hz = /bits/ 64 <134000000>; opp-microvolt = <950000>; required-opps = <&noc2_134>; }; opp-100000000 { opp-hz = /bits/ 64 <100000000>; opp-microvolt = <937500>; required-opps = <&noc2_100>; }; }; bus_noc2_opp_table: opp_table6 { compatible = "operating-points-v2"; noc2_400: opp-400000000 { opp-hz = /bits/ 64 <400000000>; }; noc2_200: opp-200000000 { opp-hz = /bits/ 64 <200000000>; }; noc2_134: opp-134000000 { opp-hz = /bits/ 64 <134000000>; }; noc2_100: opp-100000000 { opp-hz = /bits/ 64 <100000000>; }; }; -Saravana v3 -> v4: - Fixed documentation comments - Fixed order of functions in .h file - Renamed the new xlate API - Caused _set_required_opps() to fail if all required opps tables aren't linked. v2 -> v3: - Rebased onto linux-next. - Added documentation comment for new fields. - Added support for lazy required-opps linking. - Updated Ack/Reviewed-bys. v1 -> v2: - Cached OPP table reference in devfreq to avoid looking up every time. - Renamed variable in passive governor to be more intuitive. - Updated cover letter with examples. Saravana Kannan (5): OPP: Allow required-opps even if the device doesn't have power-domains OPP: Add function to look up required OPP's for a given OPP OPP: Improve required-opps linking PM / devfreq: Cache OPP table reference in devfreq PM / devfreq: Add required OPPs support to passive governor drivers/devfreq/devfreq.c | 6 ++ drivers/devfreq/governor_passive.c | 20 ++++-- drivers/opp/core.c | 83 +++++++++++++++++++--- drivers/opp/of.c | 108 ++++++++++++++--------------- drivers/opp/opp.h | 5 ++ include/linux/devfreq.h | 2 + include/linux/pm_opp.h | 11 +++ 7 files changed, 165 insertions(+), 70 deletions(-)