diff mbox series

[3/3] i2c: i2c-qcom-geni: Add support for 'assigned-performance-states'

Message ID 20201224111210.1214-4-rojay@codeaurora.org (mailing list archive)
State New, archived
Headers show
Series Add support for assigned-performance-states for geni i2c driver | expand

Commit Message

Roja Rani Yarubandi Dec. 24, 2020, 11:12 a.m. UTC
For devices which have 'assigned-performance-states' specified in DT,
set the specified performance state during probe and drop it on remove.
Also drop/set as part of runtime suspend/resume callbacks.

Signed-off-by: Roja Rani Yarubandi <rojay@codeaurora.org>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 49 ++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

Comments

Bjorn Andersson Jan. 15, 2021, 2:43 p.m. UTC | #1
On Thu 24 Dec 05:12 CST 2020, Roja Rani Yarubandi wrote:

> @@ -629,6 +658,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>  	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>  
>  	disable_irq(gi2c->irq);
> +
> +	/* Drop the assigned performance state */
> +	if (gi2c->assigned_pstate) {
> +		ret = dev_pm_genpd_set_performance_state(dev, 0);
> +		if (ret) {
> +			dev_err(dev, "Failed to set performance state\n");
> +			return ret;
> +		}
> +	}
> +

Ulf, Viresh, I think we discussed this at the time of introducing the
performance states.

The client's state does not affect if its performance_state should
be included in the calculation of the aggregated performance_state, so
each driver that needs to keep some minimum performance state needs to
have these two snippets.

Would it not make sense to on enable/disable re-evaluate the
performance_state and potentially reconfigure the hardware
automatically?

Regards,
Bjorn

>  	ret = geni_se_resources_off(&gi2c->se);
>  	if (ret) {
>  		enable_irq(gi2c->irq);
> @@ -654,6 +693,16 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/* Set the assigned performance state */
> +	if (gi2c->assigned_pstate) {
> +		ret = dev_pm_genpd_set_performance_state(dev,
> +							 gi2c->assigned_pstate);
> +		if (ret) {
> +			dev_err(dev, "Failed to set performance state\n");
> +			return ret;
> +		}
> +	}
> +
>  	enable_irq(gi2c->irq);
>  	gi2c->suspended = 0;
>  	return 0;
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member 
> of Code Aurora Forum, hosted by The Linux Foundation
>
Rajendra Nayak Jan. 18, 2021, 5:36 a.m. UTC | #2
On 1/15/2021 8:13 PM, Bjorn Andersson wrote:
> On Thu 24 Dec 05:12 CST 2020, Roja Rani Yarubandi wrote:
> 
>> @@ -629,6 +658,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>>   	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>   
>>   	disable_irq(gi2c->irq);
>> +
>> +	/* Drop the assigned performance state */
>> +	if (gi2c->assigned_pstate) {
>> +		ret = dev_pm_genpd_set_performance_state(dev, 0);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to set performance state\n");
>> +			return ret;
>> +		}
>> +	}
>> +
> 
> Ulf, Viresh, I think we discussed this at the time of introducing the
> performance states.
> 
> The client's state does not affect if its performance_state should
> be included in the calculation of the aggregated performance_state, so
> each driver that needs to keep some minimum performance state needs to
> have these two snippets.
> 
> Would it not make sense to on enable/disable re-evaluate the
> performance_state and potentially reconfigure the hardware
> automatically?

I agree, this will be repeated across multiple drivers which would
need some minimal vote while they are active, handling this during
genpd enable/disable in genpd core makes sense.

> 
> Regards,
> Bjorn
> 
>>   	ret = geni_se_resources_off(&gi2c->se);
>>   	if (ret) {
>>   		enable_irq(gi2c->irq);
>> @@ -654,6 +693,16 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>>   	if (ret)
>>   		return ret;
>>   
>> +	/* Set the assigned performance state */
>> +	if (gi2c->assigned_pstate) {
>> +		ret = dev_pm_genpd_set_performance_state(dev,
>> +							 gi2c->assigned_pstate);
>> +		if (ret) {
>> +			dev_err(dev, "Failed to set performance state\n");
>> +			return ret;
>> +		}
>> +	}
>> +
>>   	enable_irq(gi2c->irq);
>>   	gi2c->suspended = 0;
>>   	return 0;
>> -- 
>> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
>> of Code Aurora Forum, hosted by The Linux Foundation
>>
Ulf Hansson Jan. 19, 2021, 11:02 a.m. UTC | #3
On Mon, 18 Jan 2021 at 06:36, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
>
> On 1/15/2021 8:13 PM, Bjorn Andersson wrote:
> > On Thu 24 Dec 05:12 CST 2020, Roja Rani Yarubandi wrote:
> >
> >> @@ -629,6 +658,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
> >>      struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
> >>
> >>      disable_irq(gi2c->irq);
> >> +
> >> +    /* Drop the assigned performance state */
> >> +    if (gi2c->assigned_pstate) {
> >> +            ret = dev_pm_genpd_set_performance_state(dev, 0);
> >> +            if (ret) {
> >> +                    dev_err(dev, "Failed to set performance state\n");
> >> +                    return ret;
> >> +            }
> >> +    }
> >> +
> >
> > Ulf, Viresh, I think we discussed this at the time of introducing the
> > performance states.
> >
> > The client's state does not affect if its performance_state should
> > be included in the calculation of the aggregated performance_state, so
> > each driver that needs to keep some minimum performance state needs to
> > have these two snippets.
> >
> > Would it not make sense to on enable/disable re-evaluate the
> > performance_state and potentially reconfigure the hardware
> > automatically?
>
> I agree, this will be repeated across multiple drivers which would
> need some minimal vote while they are active, handling this during
> genpd enable/disable in genpd core makes sense.

Initially that's what we tried out, but we realized that it was
difficult to deal with this internally in genpd, but more importantly
it also removed some flexibility from consumers and providers. See
commit 68de2fe57a8f ("PM / Domains: Make genpd performance states
orthogonal to the idlestates").

As a matter of fact this was quite recently discussed [1], which also
pointed out some issues when using the "required-opps" in combination,
but perhaps that got resolved? Viresh?

My concern is, if we would make this kind of change to the internals
of genpd, it would lead to the following limitation: A consumer driver
can no longer make its vote for its device to stick around, when the
device becomes runtime suspended - and how do we know that we never
need to support such a case?

>
> >
> > Regards,
> > Bjorn
> >
> >>      ret = geni_se_resources_off(&gi2c->se);
> >>      if (ret) {
> >>              enable_irq(gi2c->irq);
> >> @@ -654,6 +693,16 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
> >>      if (ret)
> >>              return ret;
> >>
> >> +    /* Set the assigned performance state */
> >> +    if (gi2c->assigned_pstate) {
> >> +            ret = dev_pm_genpd_set_performance_state(dev,
> >> +                                                     gi2c->assigned_pstate);
> >> +            if (ret) {
> >> +                    dev_err(dev, "Failed to set performance state\n");
> >> +                    return ret;
> >> +            }
> >> +    }
> >> +
> >>      enable_irq(gi2c->irq);
> >>      gi2c->suspended = 0;
> >>      return 0;

Kind regards
Uffe

[1]
https://lkml.org/lkml/2020/9/11/230
Viresh Kumar Jan. 19, 2021, 11:05 a.m. UTC | #4
On 19-01-21, 12:02, Ulf Hansson wrote:
> As a matter of fact this was quite recently discussed [1], which also
> pointed out some issues when using the "required-opps" in combination,
> but perhaps that got resolved? Viresh?

Perhaps we never did anything there ..
Ulf Hansson Jan. 20, 2021, 1:31 p.m. UTC | #5
On Tue, 19 Jan 2021 at 12:05, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 19-01-21, 12:02, Ulf Hansson wrote:
> > As a matter of fact this was quite recently discussed [1], which also
> > pointed out some issues when using the "required-opps" in combination,
> > but perhaps that got resolved? Viresh?
>
> Perhaps we never did anything there ..

Okay. Looks like we should pick up that discussion again, to conclude
on how to move forward.

>
> --
> viresh

Kind regards
Uffe
Roja Rani Yarubandi Feb. 12, 2021, 9:21 a.m. UTC | #6
On 2021-01-20 19:01, Ulf Hansson wrote:
> On Tue, 19 Jan 2021 at 12:05, Viresh Kumar <viresh.kumar@linaro.org> 
> wrote:
>> 
>> On 19-01-21, 12:02, Ulf Hansson wrote:
>> > As a matter of fact this was quite recently discussed [1], which also
>> > pointed out some issues when using the "required-opps" in combination,
>> > but perhaps that got resolved? Viresh?
>> 
>> Perhaps we never did anything there ..
> 
> Okay. Looks like we should pick up that discussion again, to conclude
> on how to move forward.
> 

Soft Reminder!

>> 
>> --
>> viresh
> 
> Kind regards
> Uffe

- Roja
Roja Rani Yarubandi April 1, 2021, 6:39 a.m. UTC | #7
On 2021-02-12 14:51, rojay@codeaurora.org wrote:
> On 2021-01-20 19:01, Ulf Hansson wrote:
>> On Tue, 19 Jan 2021 at 12:05, Viresh Kumar <viresh.kumar@linaro.org> 
>> wrote:
>>> 
>>> On 19-01-21, 12:02, Ulf Hansson wrote:
>>> > As a matter of fact this was quite recently discussed [1], which also
>>> > pointed out some issues when using the "required-opps" in combination,
>>> > but perhaps that got resolved? Viresh?
>>> 
>>> Perhaps we never did anything there ..
>> 
>> Okay. Looks like we should pick up that discussion again, to conclude
>> on how to move forward.
>> 
> 
> Soft Reminder!
> 

Request Viresh, Uffe to discuss on the way forward.

>>> 
>>> --
>>> viresh
>> 
>> Kind regards
>> Uffe
> 
> - Roja
Roja Rani Yarubandi April 29, 2021, 7:02 a.m. UTC | #8
On 2021-04-01 12:09, rojay@codeaurora.org wrote:
> On 2021-02-12 14:51, rojay@codeaurora.org wrote:
>> On 2021-01-20 19:01, Ulf Hansson wrote:
>>> On Tue, 19 Jan 2021 at 12:05, Viresh Kumar <viresh.kumar@linaro.org> 
>>> wrote:
>>>> 
>>>> On 19-01-21, 12:02, Ulf Hansson wrote:
>>>> > As a matter of fact this was quite recently discussed [1], which also
>>>> > pointed out some issues when using the "required-opps" in combination,
>>>> > but perhaps that got resolved? Viresh?
>>>> 
>>>> Perhaps we never did anything there ..
>>> 
>>> Okay. Looks like we should pick up that discussion again, to conclude
>>> on how to move forward.
>>> 
>> 
>> Soft Reminder!
>> 
> 
> Request Viresh, Uffe to discuss on the way forward.
> 

Hi Viresh, Uffe, looking forward for your discussion/updates.

Thanks,
Roja

>>>> 
>>>> --
>>>> viresh
>>> 
>>> Kind regards
>>> Uffe
>> 
>> - Roja
Viresh Kumar April 29, 2021, 7:50 a.m. UTC | #9
Sorry Roja for dragging this too long, unfortunately I didn't have a
lot to add on. Lemme try start this thread again.

On 19-01-21, 12:02, Ulf Hansson wrote:
> On Mon, 18 Jan 2021 at 06:36, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> >
> >
> > On 1/15/2021 8:13 PM, Bjorn Andersson wrote:
> > > On Thu 24 Dec 05:12 CST 2020, Roja Rani Yarubandi wrote:
> > >
> > >> @@ -629,6 +658,16 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
> > >>      struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
> > >>
> > >>      disable_irq(gi2c->irq);
> > >> +
> > >> +    /* Drop the assigned performance state */
> > >> +    if (gi2c->assigned_pstate) {
> > >> +            ret = dev_pm_genpd_set_performance_state(dev, 0);
> > >> +            if (ret) {
> > >> +                    dev_err(dev, "Failed to set performance state\n");
> > >> +                    return ret;
> > >> +            }
> > >> +    }
> > >> +
> > >
> > > Ulf, Viresh, I think we discussed this at the time of introducing the
> > > performance states.
> > >
> > > The client's state does not affect if its performance_state should
> > > be included in the calculation of the aggregated performance_state, so
> > > each driver that needs to keep some minimum performance state needs to
> > > have these two snippets.
> > >
> > > Would it not make sense to on enable/disable re-evaluate the
> > > performance_state and potentially reconfigure the hardware
> > > automatically?
> >
> > I agree, this will be repeated across multiple drivers which would
> > need some minimal vote while they are active, handling this during
> > genpd enable/disable in genpd core makes sense.
> 
> Initially that's what we tried out, but we realized that it was
> difficult to deal with this internally in genpd, but more importantly
> it also removed some flexibility from consumers and providers. See
> commit 68de2fe57a8f ("PM / Domains: Make genpd performance states
> orthogonal to the idlestates").
> 
> As a matter of fact this was quite recently discussed [1], which also
> pointed out some issues when using the "required-opps" in combination,
> but perhaps that got resolved? Viresh?

So I looked again at that thread in detail today. The basic idea was
to enable/disable the genpd from within the OPP core and there were
doubts on how to do that efficiently as there are cases where domains
may be enabled for an OPP, but not for others.. etc. etc.

I am not sure if I consider that thread as part of the discussion we
are having here, they may be related, but that thread doesn't block
anything to be done in the genpd core.

> My concern is, if we would make this kind of change to the internals
> of genpd, it would lead to the following limitation: A consumer driver
> can no longer make its vote for its device to stick around, when the
> device becomes runtime suspended - and how do we know that we never
> need to support such a case?

What about doing this just for the assigned-performance-state case as
the clients don't want to play with it at all.
Rajendra Nayak May 4, 2021, 7:17 a.m. UTC | #10
[]...
>>>>
>>>> Ulf, Viresh, I think we discussed this at the time of introducing the
>>>> performance states.
>>>>
>>>> The client's state does not affect if its performance_state should
>>>> be included in the calculation of the aggregated performance_state, so
>>>> each driver that needs to keep some minimum performance state needs to
>>>> have these two snippets.
>>>>
>>>> Would it not make sense to on enable/disable re-evaluate the
>>>> performance_state and potentially reconfigure the hardware
>>>> automatically?
>>>
>>> I agree, this will be repeated across multiple drivers which would
>>> need some minimal vote while they are active, handling this during
>>> genpd enable/disable in genpd core makes sense.
>>
>> Initially that's what we tried out, but we realized that it was
>> difficult to deal with this internally in genpd, but more importantly
>> it also removed some flexibility from consumers and providers. See
>> commit 68de2fe57a8f ("PM / Domains: Make genpd performance states
>> orthogonal to the idlestates").
>>
>> As a matter of fact this was quite recently discussed [1], which also
>> pointed out some issues when using the "required-opps" in combination,
>> but perhaps that got resolved? Viresh?
> 
> So I looked again at that thread in detail today. The basic idea was
> to enable/disable the genpd from within the OPP core and there were
> doubts on how to do that efficiently as there are cases where domains
> may be enabled for an OPP, but not for others.. etc. etc.
> 
> I am not sure if I consider that thread as part of the discussion we
> are having here, they may be related, but that thread doesn't block
> anything to be done in the genpd core.

That's true, the 2 threads are different in the sense that one talks
about having OPP core managing power on/off along with setting perf state,
while the other talks about genpd core managing a default perf state
along with power on/off, but they are similar in the sense that both
are related to the discussion whether we should treat powering on and off
a domain related to setting its performance state or if it should be
considered completely orthogonal.

I think the clock framework treats setting clock rates and turning
on/off a clock orthogonal because there is an inherent assumption that
once the clock is turned off, what rate it was set to should not matter,
and it can be running at the same rate when we turn the clock back on.

I guess we can have the same assumption here that a perf state of a
power domain should not matter if the power domain is turned off
and hence the perf state need not be dropped explicitly during power off,
atleast that should be true for the qcom power domains supporting perf
state upstream.

Should that be the approach taken here? I guess that would mean the patch
I had proposed earlier [1] to manage this in the genpd core would have to set the default
perf state at attach and remove it only during a detach of the device to
the pm_domain, and not manage it during the runtime_suspend/resume of the device.

>> A consumer driver
>> can no longer make its vote for its device to stick around, when the
>> device becomes runtime suspended - and how do we know that we never
>> need to support such a case?

The above approach should take care of this but the down side of it would be,
unlike in the case of clocks where the devices assigning a default clock rate
might be doing so on a device specific clock (rarely shared with other devices)
in case of power domain, and especially in the qcom implementation of these
power domains which support perf state, these can be large domains with lots of devices,
and any device being active (not necessarily wanting any default perf state) will keep
the domain at the default perf state, requested by a device which isn't really active.

> What about doing this just for the assigned-performance-state case as
> the clients don't want to play with it at all.

well, thats possible too, but you obviously can't reuse the same bindings
in such cases

[1] https://lore.kernel.org/patchwork/patch/1284042/
Ulf Hansson May 7, 2021, 9:06 a.m. UTC | #11
On Tue, 4 May 2021 at 09:18, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
>
> []...
> >>>>
> >>>> Ulf, Viresh, I think we discussed this at the time of introducing the
> >>>> performance states.
> >>>>
> >>>> The client's state does not affect if its performance_state should
> >>>> be included in the calculation of the aggregated performance_state, so
> >>>> each driver that needs to keep some minimum performance state needs to
> >>>> have these two snippets.
> >>>>
> >>>> Would it not make sense to on enable/disable re-evaluate the
> >>>> performance_state and potentially reconfigure the hardware
> >>>> automatically?
> >>>
> >>> I agree, this will be repeated across multiple drivers which would
> >>> need some minimal vote while they are active, handling this during
> >>> genpd enable/disable in genpd core makes sense.
> >>
> >> Initially that's what we tried out, but we realized that it was
> >> difficult to deal with this internally in genpd, but more importantly
> >> it also removed some flexibility from consumers and providers. See
> >> commit 68de2fe57a8f ("PM / Domains: Make genpd performance states
> >> orthogonal to the idlestates").
> >>
> >> As a matter of fact this was quite recently discussed [1], which also
> >> pointed out some issues when using the "required-opps" in combination,
> >> but perhaps that got resolved? Viresh?
> >
> > So I looked again at that thread in detail today. The basic idea was
> > to enable/disable the genpd from within the OPP core and there were
> > doubts on how to do that efficiently as there are cases where domains
> > may be enabled for an OPP, but not for others.. etc. etc.
> >
> > I am not sure if I consider that thread as part of the discussion we
> > are having here, they may be related, but that thread doesn't block
> > anything to be done in the genpd core.
>
> That's true, the 2 threads are different in the sense that one talks
> about having OPP core managing power on/off along with setting perf state,
> while the other talks about genpd core managing a default perf state
> along with power on/off, but they are similar in the sense that both
> are related to the discussion whether we should treat powering on and off
> a domain related to setting its performance state or if it should be
> considered completely orthogonal.
>
> I think the clock framework treats setting clock rates and turning
> on/off a clock orthogonal because there is an inherent assumption that
> once the clock is turned off, what rate it was set to should not matter,
> and it can be running at the same rate when we turn the clock back on.
>
> I guess we can have the same assumption here that a perf state of a
> power domain should not matter if the power domain is turned off
> and hence the perf state need not be dropped explicitly during power off,
> atleast that should be true for the qcom power domains supporting perf
> state upstream.
>
> Should that be the approach taken here? I guess that would mean the patch
> I had proposed earlier [1] to manage this in the genpd core would have to set the default
> perf state at attach and remove it only during a detach of the device to
> the pm_domain, and not manage it during the runtime_suspend/resume of the device.

Right, I think this would be a step in the right direction, but it's
not sufficient to solve the complete problem. As you also point out
below.

>
> >> A consumer driver
> >> can no longer make its vote for its device to stick around, when the
> >> device becomes runtime suspended - and how do we know that we never
> >> need to support such a case?
>
> The above approach should take care of this but the down side of it would be,
> unlike in the case of clocks where the devices assigning a default clock rate
> might be doing so on a device specific clock (rarely shared with other devices)
> in case of power domain, and especially in the qcom implementation of these
> power domains which support perf state, these can be large domains with lots of devices,
> and any device being active (not necessarily wanting any default perf state) will keep
> the domain at the default perf state, requested by a device which isn't really active.

Yep, this certainly sounds suboptimal. To me, this isn't good enough.

>
> > What about doing this just for the assigned-performance-state case as
> > the clients don't want to play with it at all.
>
> well, thats possible too, but you obviously can't reuse the same bindings
> in such cases

Not sure I understand the issue with the DT binding? Let me elaborate
on how I think we could move forward.

It looks like we have two problems to solve:

*) We need a new DT binding.
If that becomes a generic property along the lines of the
"assigned-performance-state" as suggested - or if we decide to add a
SoC specific binding via using an additional cell in "power-domains"
(suggested by Rob), doesn't really matter much to me. The arguments
for the new DT property are very much similar to why we added
"assigned-clock-rates" for clocks.

**) We want to avoid boiler-plate code in drivers to manage
"assigned-performance-state" for their devices.
No matter what DT property we decide on (generic or SoC specific), we
should be able to manage this from the PM domain (genpd) layer. No
changes in the drivers should be needed.
If a generic binding is used, we could consider to let genpd
internally manage the whole thing (DT parsing and updating performance
state votes for assigned-performance-state only).
If we go for an SoC specific binding, the genpd provider needs to be
updated. It can manage DT parsing from the ->attach|detach_dev()
callbacks and update performance votes from the ->start|stop()
callbacks.
We could also consider a hybrid of these two solutions.

>
> [1] https://lore.kernel.org/patchwork/patch/1284042/

Kind regards
Uffe
Rajendra Nayak May 10, 2021, 6:37 a.m. UTC | #12
On 5/7/2021 2:36 PM, Ulf Hansson wrote:
> On Tue, 4 May 2021 at 09:18, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>>
>>
>> []...
>>>>>>
>>>>>> Ulf, Viresh, I think we discussed this at the time of introducing the
>>>>>> performance states.
>>>>>>
>>>>>> The client's state does not affect if its performance_state should
>>>>>> be included in the calculation of the aggregated performance_state, so
>>>>>> each driver that needs to keep some minimum performance state needs to
>>>>>> have these two snippets.
>>>>>>
>>>>>> Would it not make sense to on enable/disable re-evaluate the
>>>>>> performance_state and potentially reconfigure the hardware
>>>>>> automatically?
>>>>>
>>>>> I agree, this will be repeated across multiple drivers which would
>>>>> need some minimal vote while they are active, handling this during
>>>>> genpd enable/disable in genpd core makes sense.
>>>>
>>>> Initially that's what we tried out, but we realized that it was
>>>> difficult to deal with this internally in genpd, but more importantly
>>>> it also removed some flexibility from consumers and providers. See
>>>> commit 68de2fe57a8f ("PM / Domains: Make genpd performance states
>>>> orthogonal to the idlestates").
>>>>
>>>> As a matter of fact this was quite recently discussed [1], which also
>>>> pointed out some issues when using the "required-opps" in combination,
>>>> but perhaps that got resolved? Viresh?
>>>
>>> So I looked again at that thread in detail today. The basic idea was
>>> to enable/disable the genpd from within the OPP core and there were
>>> doubts on how to do that efficiently as there are cases where domains
>>> may be enabled for an OPP, but not for others.. etc. etc.
>>>
>>> I am not sure if I consider that thread as part of the discussion we
>>> are having here, they may be related, but that thread doesn't block
>>> anything to be done in the genpd core.
>>
>> That's true, the 2 threads are different in the sense that one talks
>> about having OPP core managing power on/off along with setting perf state,
>> while the other talks about genpd core managing a default perf state
>> along with power on/off, but they are similar in the sense that both
>> are related to the discussion whether we should treat powering on and off
>> a domain related to setting its performance state or if it should be
>> considered completely orthogonal.
>>
>> I think the clock framework treats setting clock rates and turning
>> on/off a clock orthogonal because there is an inherent assumption that
>> once the clock is turned off, what rate it was set to should not matter,
>> and it can be running at the same rate when we turn the clock back on.
>>
>> I guess we can have the same assumption here that a perf state of a
>> power domain should not matter if the power domain is turned off
>> and hence the perf state need not be dropped explicitly during power off,
>> atleast that should be true for the qcom power domains supporting perf
>> state upstream.
>>
>> Should that be the approach taken here? I guess that would mean the patch
>> I had proposed earlier [1] to manage this in the genpd core would have to set the default
>> perf state at attach and remove it only during a detach of the device to
>> the pm_domain, and not manage it during the runtime_suspend/resume of the device.
> 
> Right, I think this would be a step in the right direction, but it's
> not sufficient to solve the complete problem. As you also point out
> below.
> 
>>
>>>> A consumer driver
>>>> can no longer make its vote for its device to stick around, when the
>>>> device becomes runtime suspended - and how do we know that we never
>>>> need to support such a case?
>>
>> The above approach should take care of this but the down side of it would be,
>> unlike in the case of clocks where the devices assigning a default clock rate
>> might be doing so on a device specific clock (rarely shared with other devices)
>> in case of power domain, and especially in the qcom implementation of these
>> power domains which support perf state, these can be large domains with lots of devices,
>> and any device being active (not necessarily wanting any default perf state) will keep
>> the domain at the default perf state, requested by a device which isn't really active.
> 
> Yep, this certainly sounds suboptimal. To me, this isn't good enough.
> 
>>
>>> What about doing this just for the assigned-performance-state case as
>>> the clients don't want to play with it at all.
>>
>> well, thats possible too, but you obviously can't reuse the same bindings
>> in such cases
> 
> Not sure I understand the issue with the DT binding? Let me elaborate
> on how I think we could move forward.
> 
> It looks like we have two problems to solve:
> 
> *) We need a new DT binding.
> If that becomes a generic property along the lines of the
> "assigned-performance-state" as suggested - or if we decide to add a
> SoC specific binding via using an additional cell in "power-domains"
> (suggested by Rob), doesn't really matter much to me. The arguments
> for the new DT property are very much similar to why we added
> "assigned-clock-rates" for clocks.
> 
> **) We want to avoid boiler-plate code in drivers to manage
> "assigned-performance-state" for their devices.
> No matter what DT property we decide on (generic or SoC specific), we
> should be able to manage this from the PM domain (genpd) layer. No
> changes in the drivers should be needed.
> If a generic binding is used, we could consider to let genpd
> internally manage the whole thing (DT parsing and updating performance
> state votes for assigned-performance-state only).

Sure, so for starters does that mean I should re-spin my series which
adds the generic 'assigned-performance-states' bindings and see if Rob
is OK with that? I am guessing you are OK with the way that binding gets
used within genpd core in that series, or would you want it to be handled
differently?

> If we go for an SoC specific binding, the genpd provider needs to be
> updated. It can manage DT parsing from the ->attach|detach_dev()
> callbacks and update performance votes from the ->start|stop()
> callbacks.
> We could also consider a hybrid of these two solutions.
>>
>> [1] https://lore.kernel.org/patchwork/patch/1284042/
> 
> Kind regards
> Uffe
>
Ulf Hansson May 12, 2021, 1:50 p.m. UTC | #13
On Mon, 10 May 2021 at 08:37, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
>
>
> On 5/7/2021 2:36 PM, Ulf Hansson wrote:
> > On Tue, 4 May 2021 at 09:18, Rajendra Nayak <rnayak@codeaurora.org> wrote:
> >>
> >>
> >> []...
> >>>>>>
> >>>>>> Ulf, Viresh, I think we discussed this at the time of introducing the
> >>>>>> performance states.
> >>>>>>
> >>>>>> The client's state does not affect if its performance_state should
> >>>>>> be included in the calculation of the aggregated performance_state, so
> >>>>>> each driver that needs to keep some minimum performance state needs to
> >>>>>> have these two snippets.
> >>>>>>
> >>>>>> Would it not make sense to on enable/disable re-evaluate the
> >>>>>> performance_state and potentially reconfigure the hardware
> >>>>>> automatically?
> >>>>>
> >>>>> I agree, this will be repeated across multiple drivers which would
> >>>>> need some minimal vote while they are active, handling this during
> >>>>> genpd enable/disable in genpd core makes sense.
> >>>>
> >>>> Initially that's what we tried out, but we realized that it was
> >>>> difficult to deal with this internally in genpd, but more importantly
> >>>> it also removed some flexibility from consumers and providers. See
> >>>> commit 68de2fe57a8f ("PM / Domains: Make genpd performance states
> >>>> orthogonal to the idlestates").
> >>>>
> >>>> As a matter of fact this was quite recently discussed [1], which also
> >>>> pointed out some issues when using the "required-opps" in combination,
> >>>> but perhaps that got resolved? Viresh?
> >>>
> >>> So I looked again at that thread in detail today. The basic idea was
> >>> to enable/disable the genpd from within the OPP core and there were
> >>> doubts on how to do that efficiently as there are cases where domains
> >>> may be enabled for an OPP, but not for others.. etc. etc.
> >>>
> >>> I am not sure if I consider that thread as part of the discussion we
> >>> are having here, they may be related, but that thread doesn't block
> >>> anything to be done in the genpd core.
> >>
> >> That's true, the 2 threads are different in the sense that one talks
> >> about having OPP core managing power on/off along with setting perf state,
> >> while the other talks about genpd core managing a default perf state
> >> along with power on/off, but they are similar in the sense that both
> >> are related to the discussion whether we should treat powering on and off
> >> a domain related to setting its performance state or if it should be
> >> considered completely orthogonal.
> >>
> >> I think the clock framework treats setting clock rates and turning
> >> on/off a clock orthogonal because there is an inherent assumption that
> >> once the clock is turned off, what rate it was set to should not matter,
> >> and it can be running at the same rate when we turn the clock back on.
> >>
> >> I guess we can have the same assumption here that a perf state of a
> >> power domain should not matter if the power domain is turned off
> >> and hence the perf state need not be dropped explicitly during power off,
> >> atleast that should be true for the qcom power domains supporting perf
> >> state upstream.
> >>
> >> Should that be the approach taken here? I guess that would mean the patch
> >> I had proposed earlier [1] to manage this in the genpd core would have to set the default
> >> perf state at attach and remove it only during a detach of the device to
> >> the pm_domain, and not manage it during the runtime_suspend/resume of the device.
> >
> > Right, I think this would be a step in the right direction, but it's
> > not sufficient to solve the complete problem. As you also point out
> > below.
> >
> >>
> >>>> A consumer driver
> >>>> can no longer make its vote for its device to stick around, when the
> >>>> device becomes runtime suspended - and how do we know that we never
> >>>> need to support such a case?
> >>
> >> The above approach should take care of this but the down side of it would be,
> >> unlike in the case of clocks where the devices assigning a default clock rate
> >> might be doing so on a device specific clock (rarely shared with other devices)
> >> in case of power domain, and especially in the qcom implementation of these
> >> power domains which support perf state, these can be large domains with lots of devices,
> >> and any device being active (not necessarily wanting any default perf state) will keep
> >> the domain at the default perf state, requested by a device which isn't really active.
> >
> > Yep, this certainly sounds suboptimal. To me, this isn't good enough.
> >
> >>
> >>> What about doing this just for the assigned-performance-state case as
> >>> the clients don't want to play with it at all.
> >>
> >> well, thats possible too, but you obviously can't reuse the same bindings
> >> in such cases
> >
> > Not sure I understand the issue with the DT binding? Let me elaborate
> > on how I think we could move forward.
> >
> > It looks like we have two problems to solve:
> >
> > *) We need a new DT binding.
> > If that becomes a generic property along the lines of the
> > "assigned-performance-state" as suggested - or if we decide to add a
> > SoC specific binding via using an additional cell in "power-domains"
> > (suggested by Rob), doesn't really matter much to me. The arguments
> > for the new DT property are very much similar to why we added
> > "assigned-clock-rates" for clocks.
> >
> > **) We want to avoid boiler-plate code in drivers to manage
> > "assigned-performance-state" for their devices.
> > No matter what DT property we decide on (generic or SoC specific), we
> > should be able to manage this from the PM domain (genpd) layer. No
> > changes in the drivers should be needed.
> > If a generic binding is used, we could consider to let genpd
> > internally manage the whole thing (DT parsing and updating performance
> > state votes for assigned-performance-state only).
>
> Sure, so for starters does that mean I should re-spin my series which
> adds the generic 'assigned-performance-states' bindings and see if Rob
> is OK with that? I am guessing you are OK with the way that binding gets
> used within genpd core in that series, or would you want it to be handled
> differently?

A re-spin would definitely move this forward. If we can convince Rob
about the generic DT binding, I will certainly agree to change genpd
to help/manage the DT parsing.

Although, I am not sure yet what is the best. Let genpd to do the
parsing internally in genpd_add_device() or just provide a helper
function that can be called from an ->attach|detach_dev() callback.

In the end it boils done to decide whether we should use the
->start|stop() callbacks or let genpd internally manage the "assigned
performance state". Honestly, I would prefer to look at how the code
would need to  be changed, before answering this.

>
> > If we go for an SoC specific binding, the genpd provider needs to be
> > updated. It can manage DT parsing from the ->attach|detach_dev()
> > callbacks and update performance votes from the ->start|stop()
> > callbacks.
> > We could also consider a hybrid of these two solutions.
> >>
> >> [1] https://lore.kernel.org/patchwork/patch/1284042/

Kind regards
Uffe
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 046d241183c5..250773784631 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -11,6 +11,7 @@ 
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/qcom-geni-se.h>
 #include <linux/spinlock.h>
@@ -86,6 +87,7 @@  struct geni_i2c_dev {
 	u32 clk_freq_out;
 	const struct geni_i2c_clk_fld *clk_fld;
 	int suspended;
+	unsigned int assigned_pstate;
 };
 
 struct geni_i2c_err_log {
@@ -497,6 +499,7 @@  static int geni_i2c_probe(struct platform_device *pdev)
 	u32 proto, tx_depth;
 	int ret;
 	struct device *dev = &pdev->dev;
+	unsigned int assigned_pstate;
 
 	gi2c = devm_kzalloc(dev, sizeof(*gi2c), GFP_KERNEL);
 	if (!gi2c)
@@ -520,6 +523,20 @@  static int geni_i2c_probe(struct platform_device *pdev)
 		gi2c->clk_freq_out = KHZ(100);
 	}
 
+	/* Set the assigned performance state */
+	if (!of_property_read_u32(pdev->dev.of_node, "assigned-performance-states",
+					&assigned_pstate)) {
+		if (assigned_pstate) {
+			ret = dev_pm_genpd_set_performance_state(dev,
+								 assigned_pstate);
+			if (ret) {
+				dev_err(dev, "Failed to set performance state\n");
+				return ret;
+			}
+			gi2c->assigned_pstate = assigned_pstate;
+		}
+	}
+
 	if (has_acpi_companion(dev))
 		ACPI_COMPANION_SET(&gi2c->adap.dev, ACPI_COMPANION(dev));
 
@@ -616,10 +633,22 @@  static int geni_i2c_probe(struct platform_device *pdev)
 
 static int geni_i2c_remove(struct platform_device *pdev)
 {
+	int ret;
+	struct device *dev = &pdev->dev;
 	struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
 
 	i2c_del_adapter(&gi2c->adap);
 	pm_runtime_disable(gi2c->se.dev);
+
+	/* Drop the assigned performance state */
+	if (gi2c->assigned_pstate) {
+		ret = dev_pm_genpd_set_performance_state(dev, 0);
+		if (ret) {
+			dev_err(dev, "Failed to set performance state\n");
+			return ret;
+		}
+	}
+
 	return 0;
 }
 
@@ -629,6 +658,16 @@  static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
 
 	disable_irq(gi2c->irq);
+
+	/* Drop the assigned performance state */
+	if (gi2c->assigned_pstate) {
+		ret = dev_pm_genpd_set_performance_state(dev, 0);
+		if (ret) {
+			dev_err(dev, "Failed to set performance state\n");
+			return ret;
+		}
+	}
+
 	ret = geni_se_resources_off(&gi2c->se);
 	if (ret) {
 		enable_irq(gi2c->irq);
@@ -654,6 +693,16 @@  static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
 	if (ret)
 		return ret;
 
+	/* Set the assigned performance state */
+	if (gi2c->assigned_pstate) {
+		ret = dev_pm_genpd_set_performance_state(dev,
+							 gi2c->assigned_pstate);
+		if (ret) {
+			dev_err(dev, "Failed to set performance state\n");
+			return ret;
+		}
+	}
+
 	enable_irq(gi2c->irq);
 	gi2c->suspended = 0;
 	return 0;