[v4,0/5] Add required-opps support to devfreq passive gov
mbox series

Message ID 20190724014222.110767-1-saravanak@google.com
Headers show
Series
  • Add required-opps support to devfreq passive gov
Related show

Message

Saravana Kannan July 24, 2019, 1:42 a.m. UTC
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(-)

Comments

Viresh Kumar July 25, 2019, 2:30 a.m. UTC | #1
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.
Saravana Kannan July 25, 2019, 3:40 a.m. UTC | #2
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
Viresh Kumar July 25, 2019, 5:22 a.m. UTC | #3
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.
Saravana Kannan July 26, 2019, 1:56 a.m. UTC | #4
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
Chanwoo Choi Nov. 14, 2019, 7:54 a.m. UTC | #5
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(-)
>
Saravana Kannan Nov. 14, 2019, 8:23 a.m. UTC | #6
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
Viresh Kumar Nov. 14, 2019, 8:35 a.m. UTC | #7
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 :)
Chanwoo Choi Nov. 14, 2019, 8:39 a.m. UTC | #8
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
Saravana Kannan Nov. 14, 2019, 7:39 p.m. UTC | #9
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
Chanwoo Choi Dec. 19, 2019, 4:17 p.m. UTC | #10
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