[v8,01/34] opp: Add dev_pm_opp_sync() helper

Commit Message

Dmitry Osipenko Aug. 17, 2021, 1:27 a.m. UTC

Add dev_pm_opp_sync() helper which syncs OPP table with hardware state
and vice versa.

Signed-off-by: Dmitry Osipenko <[e-mail]>
---
 drivers/opp/core.c     | 42 +++++++++++++++++++++++++++++++++++++++---
 include/linux/pm_opp.h |  6 ++++++
 2 files changed, 45 insertions(+), 3 deletions(-)

Comments

Viresh Kumar Aug. 17, 2021, 7:55 a.m. UTC | #1

On 17-08-21, 04:27, Dmitry Osipenko wrote:
> Add dev_pm_opp_sync() helper which syncs OPP table with hardware state
> and vice versa.
> 
> Signed-off-by: Dmitry Osipenko <[e-mail]>
> ---
>  drivers/opp/core.c     | 42 +++++++++++++++++++++++++++++++++++++++---
>  include/linux/pm_opp.h |  6 ++++++
>  2 files changed, 45 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 5543c54dacc5..18016e49605f 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -939,7 +939,8 @@ static int _set_required_opps(struct device *dev,
>  	return ret;
>  }
>  
> -static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
> +static struct dev_pm_opp *
> +_find_current_opp(struct device *dev, struct opp_table *opp_table)
>  {
>  	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
>  	unsigned long freq;
> @@ -961,7 +962,7 @@ static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
>  		mutex_unlock(&opp_table->lock);
>  	}
>  
> -	opp_table->current_opp = opp;
> +	return opp;
>  }
>  
>  static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
> @@ -1003,7 +1004,7 @@ static int _set_opp(struct device *dev, struct opp_table *opp_table,
>  
>  	/* Find the currently set OPP if we don't know already */
>  	if (unlikely(!opp_table->current_opp))
> -		_find_current_opp(dev, opp_table);
> +		opp_table->current_opp = _find_current_opp(dev, opp_table);
>  
>  	old_opp = opp_table->current_opp;
>  
> @@ -2931,3 +2932,38 @@ int dev_pm_opp_sync_regulators(struct device *dev)
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
> +
> +/**
> + * dev_pm_opp_sync() - Sync OPP state
> + * @dev:	device for which we do this operation
> + *
> + * Initialize OPP table accordingly to current clock rate or
> + * first available OPP if clock not available for this device.
> + *
> + * Return: 0 on success or a negative error value.
> + */
> +int dev_pm_opp_sync(struct device *dev)
> +{
> +	struct opp_table *opp_table;
> +	struct dev_pm_opp *opp;
> +	int ret = 0;
> +
> +	/* Device may not have OPP table */
> +	opp_table = _find_opp_table(dev);
> +	if (IS_ERR(opp_table))
> +		return 0;
> +
> +	if (!_get_opp_count(opp_table))
> +		goto put_table;
> +
> +	opp = _find_current_opp(dev, opp_table);
> +	ret = _set_opp(dev, opp_table, opp, opp->rate);

And I am not sure how this will end up working, since new OPP will be
equal to old one. Since I see you call this from resume() at many
places.

what exactly are you trying to do here ? Those details would be good
to have in commit log as well, I haven't really followed V7 of your
series.

Dmitry Osipenko Aug. 17, 2021, 3:49 p.m. UTC | #2

17.08.2021 10:55, Viresh Kumar пишет:
...
>> +int dev_pm_opp_sync(struct device *dev)
>> +{
>> +	struct opp_table *opp_table;
>> +	struct dev_pm_opp *opp;
>> +	int ret = 0;
>> +
>> +	/* Device may not have OPP table */
>> +	opp_table = _find_opp_table(dev);
>> +	if (IS_ERR(opp_table))
>> +		return 0;
>> +
>> +	if (!_get_opp_count(opp_table))
>> +		goto put_table;
>> +
>> +	opp = _find_current_opp(dev, opp_table);
>> +	ret = _set_opp(dev, opp_table, opp, opp->rate);
> 
> And I am not sure how this will end up working, since new OPP will be
> equal to old one. Since I see you call this from resume() at many
> places.

Initially OPP table is "uninitialized" and opp_table->enabled=false,
hence the first sync always works even if OPP is equal to old one. Once
OPP has been synced, all further syncs are NO-OPs, hence it doesn't
matter how many times syncing is called.

https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012

> what exactly are you trying to do here ? Those details would be good
> to have in commit log as well, I haven't really followed V7 of your
> series.

I'm initializing voltage/power state of OPP table in accordance to the
clock rate, bumping voltage before clock is enabled by device driver.
I'll improve the commit message.

An alternative to the explicit syncing could be something like a new
dev_pm_opp_resume/suspend helpers that will take care of
enabling/disabling the OPP table clock/etc and syncing the OPP state
with h/w.

Viresh Kumar Aug. 18, 2021, 3:55 a.m. UTC | #3

On 17-08-21, 18:49, Dmitry Osipenko wrote:
> 17.08.2021 10:55, Viresh Kumar пишет:
> ...
> >> +int dev_pm_opp_sync(struct device *dev)
> >> +{
> >> +	struct opp_table *opp_table;
> >> +	struct dev_pm_opp *opp;
> >> +	int ret = 0;
> >> +
> >> +	/* Device may not have OPP table */
> >> +	opp_table = _find_opp_table(dev);
> >> +	if (IS_ERR(opp_table))
> >> +		return 0;
> >> +
> >> +	if (!_get_opp_count(opp_table))
> >> +		goto put_table;
> >> +
> >> +	opp = _find_current_opp(dev, opp_table);
> >> +	ret = _set_opp(dev, opp_table, opp, opp->rate);
> > 
> > And I am not sure how this will end up working, since new OPP will be
> > equal to old one. Since I see you call this from resume() at many
> > places.
> 
> Initially OPP table is "uninitialized" and opp_table->enabled=false,
> hence the first sync always works even if OPP is equal to old one. Once
> OPP has been synced, all further syncs are NO-OPs, hence it doesn't
> matter how many times syncing is called.
> 
> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012

Right, but how will this work from Resume ? Won't that be a no-op ?

Dmitry Osipenko Aug. 18, 2021, 4:12 a.m. UTC | #4

18.08.2021 06:55, Viresh Kumar пишет:
> On 17-08-21, 18:49, Dmitry Osipenko wrote:
>> 17.08.2021 10:55, Viresh Kumar пишет:
>> ...
>>>> +int dev_pm_opp_sync(struct device *dev)
>>>> +{
>>>> +	struct opp_table *opp_table;
>>>> +	struct dev_pm_opp *opp;
>>>> +	int ret = 0;
>>>> +
>>>> +	/* Device may not have OPP table */
>>>> +	opp_table = _find_opp_table(dev);
>>>> +	if (IS_ERR(opp_table))
>>>> +		return 0;
>>>> +
>>>> +	if (!_get_opp_count(opp_table))
>>>> +		goto put_table;
>>>> +
>>>> +	opp = _find_current_opp(dev, opp_table);
>>>> +	ret = _set_opp(dev, opp_table, opp, opp->rate);
>>>
>>> And I am not sure how this will end up working, since new OPP will be
>>> equal to old one. Since I see you call this from resume() at many
>>> places.
>>
>> Initially OPP table is "uninitialized" and opp_table->enabled=false,
>> hence the first sync always works even if OPP is equal to old one. Once
>> OPP has been synced, all further syncs are NO-OPs, hence it doesn't
>> matter how many times syncing is called.
>>
>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012
> 
> Right, but how will this work from Resume ? Won't that be a no-op ?

The first resume initializes the OPP state on sync, all further syncs on
resume are no-ops.

Dmitry Osipenko Aug. 18, 2021, 4:29 a.m. UTC | #5

18.08.2021 07:12, Dmitry Osipenko пишет:
> 18.08.2021 06:55, Viresh Kumar пишет:
>> On 17-08-21, 18:49, Dmitry Osipenko wrote:
>>> 17.08.2021 10:55, Viresh Kumar пишет:
>>> ...
>>>>> +int dev_pm_opp_sync(struct device *dev)
>>>>> +{
>>>>> +	struct opp_table *opp_table;
>>>>> +	struct dev_pm_opp *opp;
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	/* Device may not have OPP table */
>>>>> +	opp_table = _find_opp_table(dev);
>>>>> +	if (IS_ERR(opp_table))
>>>>> +		return 0;
>>>>> +
>>>>> +	if (!_get_opp_count(opp_table))
>>>>> +		goto put_table;
>>>>> +
>>>>> +	opp = _find_current_opp(dev, opp_table);
>>>>> +	ret = _set_opp(dev, opp_table, opp, opp->rate);
>>>>
>>>> And I am not sure how this will end up working, since new OPP will be
>>>> equal to old one. Since I see you call this from resume() at many
>>>> places.
>>>
>>> Initially OPP table is "uninitialized" and opp_table->enabled=false,
>>> hence the first sync always works even if OPP is equal to old one. Once
>>> OPP has been synced, all further syncs are NO-OPs, hence it doesn't
>>> matter how many times syncing is called.
>>>
>>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012
>>
>> Right, but how will this work from Resume ? Won't that be a no-op ?
> 
> The first resume initializes the OPP state on sync, all further syncs on
> resume are no-ops.
> 

Notice that we use GENPD here. GENPD core takes care of storing PD's
performance state (voltage in case of Tegra) and dropping it to 0 after
rpm-suspend, GENPD core also restores the state before rpm-resume.

Dmitry Osipenko Aug. 18, 2021, 4:30 a.m. UTC | #6

18.08.2021 07:29, Dmitry Osipenko пишет:
> 18.08.2021 07:12, Dmitry Osipenko пишет:
>> 18.08.2021 06:55, Viresh Kumar пишет:
>>> On 17-08-21, 18:49, Dmitry Osipenko wrote:
>>>> 17.08.2021 10:55, Viresh Kumar пишет:
>>>> ...
>>>>>> +int dev_pm_opp_sync(struct device *dev)
>>>>>> +{
>>>>>> +	struct opp_table *opp_table;
>>>>>> +	struct dev_pm_opp *opp;
>>>>>> +	int ret = 0;
>>>>>> +
>>>>>> +	/* Device may not have OPP table */
>>>>>> +	opp_table = _find_opp_table(dev);
>>>>>> +	if (IS_ERR(opp_table))
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	if (!_get_opp_count(opp_table))
>>>>>> +		goto put_table;
>>>>>> +
>>>>>> +	opp = _find_current_opp(dev, opp_table);
>>>>>> +	ret = _set_opp(dev, opp_table, opp, opp->rate);
>>>>>
>>>>> And I am not sure how this will end up working, since new OPP will be
>>>>> equal to old one. Since I see you call this from resume() at many
>>>>> places.
>>>>
>>>> Initially OPP table is "uninitialized" and opp_table->enabled=false,
>>>> hence the first sync always works even if OPP is equal to old one. Once
>>>> OPP has been synced, all further syncs are NO-OPs, hence it doesn't
>>>> matter how many times syncing is called.
>>>>
>>>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012
>>>
>>> Right, but how will this work from Resume ? Won't that be a no-op ?
>>
>> The first resume initializes the OPP state on sync, all further syncs on
>> resume are no-ops.
>>
> 
> Notice that we use GENPD here. GENPD core takes care of storing PD's
> performance state (voltage in case of Tegra) and dropping it to 0 after
> rpm-suspend, GENPD core also restores the state before rpm-resume.

By 'here' I mean in this series.

Viresh Kumar Aug. 18, 2021, 4:31 a.m. UTC | #7

On 18-08-21, 07:12, Dmitry Osipenko wrote:
> 18.08.2021 06:55, Viresh Kumar пишет:
> > On 17-08-21, 18:49, Dmitry Osipenko wrote:
> >> 17.08.2021 10:55, Viresh Kumar пишет:
> >> ...
> >>>> +int dev_pm_opp_sync(struct device *dev)
> >>>> +{
> >>>> +	struct opp_table *opp_table;
> >>>> +	struct dev_pm_opp *opp;
> >>>> +	int ret = 0;
> >>>> +
> >>>> +	/* Device may not have OPP table */
> >>>> +	opp_table = _find_opp_table(dev);
> >>>> +	if (IS_ERR(opp_table))
> >>>> +		return 0;
> >>>> +
> >>>> +	if (!_get_opp_count(opp_table))
> >>>> +		goto put_table;
> >>>> +
> >>>> +	opp = _find_current_opp(dev, opp_table);
> >>>> +	ret = _set_opp(dev, opp_table, opp, opp->rate);
> >>>
> >>> And I am not sure how this will end up working, since new OPP will be
> >>> equal to old one. Since I see you call this from resume() at many
> >>> places.
> >>
> >> Initially OPP table is "uninitialized" and opp_table->enabled=false,
> >> hence the first sync always works even if OPP is equal to old one. Once
> >> OPP has been synced, all further syncs are NO-OPs, hence it doesn't
> >> matter how many times syncing is called.
> >>
> >> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012
> > 
> > Right, but how will this work from Resume ? Won't that be a no-op ?
> 
> The first resume initializes the OPP state on sync, all further syncs on
> resume are no-ops.

But the OPPs should already be initialized as someone must have called
opp-set-rate earlier ? Why do this from resume and not probe ?

Viresh Kumar Aug. 18, 2021, 4:34 a.m. UTC | #8

On 18-08-21, 07:30, Dmitry Osipenko wrote:
> 18.08.2021 07:29, Dmitry Osipenko пишет:
> >> The first resume initializes the OPP state on sync, all further syncs on
> >> resume are no-ops.
> >>
> > 
> > Notice that we use GENPD here. GENPD core takes care of storing PD's
> > performance state (voltage in case of Tegra) and dropping it to 0 after
> > rpm-suspend, GENPD core also restores the state before rpm-resume.
> 
> By 'here' I mean in this series.

It is still not clear to me why you need to this on resume, and not
probe.

Dmitry Osipenko Aug. 18, 2021, 4:37 a.m. UTC | #9

18.08.2021 07:31, Viresh Kumar пишет:
> On 18-08-21, 07:12, Dmitry Osipenko wrote:
>> 18.08.2021 06:55, Viresh Kumar пишет:
>>> On 17-08-21, 18:49, Dmitry Osipenko wrote:
>>>> 17.08.2021 10:55, Viresh Kumar пишет:
>>>> ...
>>>>>> +int dev_pm_opp_sync(struct device *dev)
>>>>>> +{
>>>>>> +	struct opp_table *opp_table;
>>>>>> +	struct dev_pm_opp *opp;
>>>>>> +	int ret = 0;
>>>>>> +
>>>>>> +	/* Device may not have OPP table */
>>>>>> +	opp_table = _find_opp_table(dev);
>>>>>> +	if (IS_ERR(opp_table))
>>>>>> +		return 0;
>>>>>> +
>>>>>> +	if (!_get_opp_count(opp_table))
>>>>>> +		goto put_table;
>>>>>> +
>>>>>> +	opp = _find_current_opp(dev, opp_table);
>>>>>> +	ret = _set_opp(dev, opp_table, opp, opp->rate);
>>>>>
>>>>> And I am not sure how this will end up working, since new OPP will be
>>>>> equal to old one. Since I see you call this from resume() at many
>>>>> places.
>>>>
>>>> Initially OPP table is "uninitialized" and opp_table->enabled=false,
>>>> hence the first sync always works even if OPP is equal to old one. Once
>>>> OPP has been synced, all further syncs are NO-OPs, hence it doesn't
>>>> matter how many times syncing is called.
>>>>
>>>> https://elixir.bootlin.com/linux/v5.14-rc6/source/drivers/opp/core.c#L1012
>>>
>>> Right, but how will this work from Resume ? Won't that be a no-op ?
>>
>> The first resume initializes the OPP state on sync, all further syncs on
>> resume are no-ops.
> 
> But the OPPs should already be initialized as someone must have called
> opp-set-rate earlier ? Why do this from resume and not probe ?

This will set voltage level without having an actively used hardware.
Take a 3d driver for example, if you set the rate on probe and
rpm-resume will never be called, then the voltage will be set high,
while hardware is kept suspended if userspace will never wake it up by
executing a 3d job.

Viresh Kumar Aug. 18, 2021, 4:53 a.m. UTC | #10

On 18-08-21, 07:37, Dmitry Osipenko wrote:
> This will set voltage level without having an actively used hardware.
> Take a 3d driver for example, if you set the rate on probe and
> rpm-resume will never be called, then the voltage will be set high,
> while hardware is kept suspended if userspace will never wake it up by
> executing a 3d job.

What exactly are we looking to achieve with this stuff ? Cache the
current performance state with genpd (based on the state bootloader's
has set) ?

Or anything else as well ?

Dmitry Osipenko Aug. 18, 2021, 5:21 a.m. UTC | #11

18.08.2021 07:53, Viresh Kumar пишет:
> On 18-08-21, 07:37, Dmitry Osipenko wrote:
>> This will set voltage level without having an actively used hardware.
>> Take a 3d driver for example, if you set the rate on probe and
>> rpm-resume will never be called, then the voltage will be set high,
>> while hardware is kept suspended if userspace will never wake it up by
>> executing a 3d job.
> 
> What exactly are we looking to achieve with this stuff ? Cache the
> current performance state with genpd (based on the state bootloader's
> has set) ?

Yes, GENPD will cache the perf state across suspend/resume and initially
cached value is out of sync with h/w.

> Or anything else as well ?

Nothing else. But let me clarify it all again.

Initially the performance state of all GENPDs is 0 for all devices.

The clock rate is preinitialized for all devices to a some default rate
by clk driver, or by bootloader or by assigned-clocks in DT.

When device is rpm-resumed, the resume callback of a device driver
enables the clock.

Before clock is enabled, the voltage needs to be configured in
accordance to the clk rate.

So now we have a GENPD with pstate=0 on a first rpm-resume, which
doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the
pstate in accordance to the h/w config.

In a previous v7 I proposed to preset the rpm_pstate of GENPD (perf
level that is restored before device is rpm-resumed) from PD's
attach_dev callback, but Ulf didn't like that because it requires to use
and modify GENPD 'private' variables from a PD driver. We decided that
will be better to make device drivers to explicitly sync the perf state,
which I implemented in this v8.

Viresh Kumar Aug. 18, 2021, 5:58 a.m. UTC | #12

On 18-08-21, 08:21, Dmitry Osipenko wrote:
> Yes, GENPD will cache the perf state across suspend/resume and initially
> cached value is out of sync with h/w.
> 
> Nothing else. But let me clarify it all again.

Thanks for your explanation.

> Initially the performance state of all GENPDs is 0 for all devices.
> 
> The clock rate is preinitialized for all devices to a some default rate
> by clk driver, or by bootloader or by assigned-clocks in DT.
> 
> When device is rpm-resumed, the resume callback of a device driver
> enables the clock.
> 
> Before clock is enabled, the voltage needs to be configured in
> accordance to the clk rate.
> 
> So now we have a GENPD with pstate=0 on a first rpm-resume, which
> doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the
> pstate in accordance to the h/w config.

What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here
instead ? That will work, right ? The advantage is it works without
any special routine to do so.

I also wonder looking at your gr3d.c changes, you set a set-opp
helper, but the driver doesn't call set_opp_rate at all. Who calls it
?

And if it is all about just syncing the genpd core, then can the genpd
core do something like what clk framework does? i.e. allow a new
optional genpd callback, get_performance_state() (just like
set_performance_state()), which can be called initially by the core to
get the performance to something other than zero. opp-set-rate is
there to set the performance state and enable the stuff as well.
That's why it looks incorrect in your case, where the function was
only required to be called once, and you are ending up calling it on
each resume. Limiting that with another local variable is bad as well.

> In a previous v7 I proposed to preset the rpm_pstate of GENPD (perf
> level that is restored before device is rpm-resumed) from PD's
> attach_dev callback, but Ulf didn't like that because it requires to use
> and modify GENPD 'private' variables from a PD driver. We decided that
> will be better to make device drivers to explicitly sync the perf state,
> which I implemented in this v8.

Viresh Kumar Aug. 18, 2021, 6 a.m. UTC | #13

On 18-08-21, 11:28, Viresh Kumar wrote:
> On 18-08-21, 08:21, Dmitry Osipenko wrote:
> > Yes, GENPD will cache the perf state across suspend/resume and initially
> > cached value is out of sync with h/w.
> > 
> > Nothing else. But let me clarify it all again.
> 
> Thanks for your explanation.
> 
> > Initially the performance state of all GENPDs is 0 for all devices.
> > 
> > The clock rate is preinitialized for all devices to a some default rate
> > by clk driver, or by bootloader or by assigned-clocks in DT.
> > 
> > When device is rpm-resumed, the resume callback of a device driver
> > enables the clock.
> > 
> > Before clock is enabled, the voltage needs to be configured in
> > accordance to the clk rate.
> > 
> > So now we have a GENPD with pstate=0 on a first rpm-resume, which
> > doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the
> > pstate in accordance to the h/w config.
> 
> What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here
> instead ? That will work, right ? The advantage is it works without
> any special routine to do so.
> 
> I also wonder looking at your gr3d.c changes, you set a set-opp
> helper, but the driver doesn't call set_opp_rate at all. Who calls it
> ?
> 
> And if it is all about just syncing the genpd core, then can the genpd
> core do something like what clk framework does? i.e. allow a new
> optional genpd callback, get_performance_state() (just like
> set_performance_state()), which can be called initially by the core to
> get the performance to something other than zero. opp-set-rate is
> there to set the performance state and enable the stuff as well.
> That's why it looks incorrect in your case, where the function was
> only required to be called once, and you are ending up calling it on
> each resume. Limiting that with another local variable is bad as well.

Ulf, this last part is for you :)

Dmitry Osipenko Aug. 18, 2021, 6:22 a.m. UTC | #14

18.08.2021 08:58, Viresh Kumar пишет:
> On 18-08-21, 08:21, Dmitry Osipenko wrote:
>> Yes, GENPD will cache the perf state across suspend/resume and initially
>> cached value is out of sync with h/w.
>>
>> Nothing else. But let me clarify it all again.
> 
> Thanks for your explanation.
> 
>> Initially the performance state of all GENPDs is 0 for all devices.
>>
>> The clock rate is preinitialized for all devices to a some default rate
>> by clk driver, or by bootloader or by assigned-clocks in DT.
>>
>> When device is rpm-resumed, the resume callback of a device driver
>> enables the clock.
>>
>> Before clock is enabled, the voltage needs to be configured in
>> accordance to the clk rate.
>>
>> So now we have a GENPD with pstate=0 on a first rpm-resume, which
>> doesn't match the h/w configuration. Calling dev_pm_opp_sync() sets the
>> pstate in accordance to the h/w config.
> 
> What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here
> instead ? That will work, right ? The advantage is it works without
> any special routine to do so.

It will work, but a dedicated helper is nicer.

> I also wonder looking at your gr3d.c changes, you set a set-opp
> helper, but the driver doesn't call set_opp_rate at all. Who calls it
> ?

dev_pm_opp_sync() calls it from _set_opp().

> And if it is all about just syncing the genpd core, then can the genpd
> core do something like what clk framework does? i.e. allow a new
> optional genpd callback, get_performance_state() (just like
> set_performance_state()), which can be called initially by the core to
> get the performance to something other than zero. opp-set-rate is
> there to set the performance state and enable the stuff as well.
> That's why it looks incorrect in your case, where the function was
> only required to be called once, and you are ending up calling it on
> each resume. Limiting that with another local variable is bad as well.

We discussed variant with get_performance_state() previously and Ulf
didn't like it either since it still requires to touch 'internals' of GENPD.

Viresh Kumar Aug. 18, 2021, 6:27 a.m. UTC | #15

On 18-08-21, 09:22, Dmitry Osipenko wrote:
> 18.08.2021 08:58, Viresh Kumar пишет:
> > What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here
> > instead ? That will work, right ? The advantage is it works without
> > any special routine to do so.
> 
> It will work, but a dedicated helper is nicer.
> 
> > I also wonder looking at your gr3d.c changes, you set a set-opp
> > helper, but the driver doesn't call set_opp_rate at all. Who calls it
> > ?
> 
> dev_pm_opp_sync() calls it from _set_opp().

Okay, please use dev_pm_opp_set_rate() instead then. New helper just
adds to the confusion and isn't doing anything special apart from
doing clk_get_rate() for you.

> > And if it is all about just syncing the genpd core, then can the genpd
> > core do something like what clk framework does? i.e. allow a new
> > optional genpd callback, get_performance_state() (just like
> > set_performance_state()), which can be called initially by the core to
> > get the performance to something other than zero. opp-set-rate is
> > there to set the performance state and enable the stuff as well.
> > That's why it looks incorrect in your case, where the function was
> > only required to be called once, and you are ending up calling it on
> > each resume. Limiting that with another local variable is bad as well.
> 
> We discussed variant with get_performance_state() previously and Ulf
> didn't like it either since it still requires to touch 'internals' of GENPD.

Hmm, I wonder if that would be a problem since only genpd core is
going to call that routine anyway.

Ulf Hansson Aug. 18, 2021, 8:29 a.m. UTC | #16

On Wed, 18 Aug 2021 at 08:27, Viresh Kumar <[e-mail]> wrote:
>
> On 18-08-21, 09:22, Dmitry Osipenko wrote:
> > 18.08.2021 08:58, Viresh Kumar пишет:
> > > What about calling dev_pm_opp_set_rate(dev, clk_get_rate(dev)) here
> > > instead ? That will work, right ? The advantage is it works without
> > > any special routine to do so.
> >
> > It will work, but a dedicated helper is nicer.
> >
> > > I also wonder looking at your gr3d.c changes, you set a set-opp
> > > helper, but the driver doesn't call set_opp_rate at all. Who calls it
> > > ?
> >
> > dev_pm_opp_sync() calls it from _set_opp().
>
> Okay, please use dev_pm_opp_set_rate() instead then. New helper just
> adds to the confusion and isn't doing anything special apart from
> doing clk_get_rate() for you.
>
> > > And if it is all about just syncing the genpd core, then can the genpd
> > > core do something like what clk framework does? i.e. allow a new
> > > optional genpd callback, get_performance_state() (just like
> > > set_performance_state()), which can be called initially by the core to
> > > get the performance to something other than zero. opp-set-rate is
> > > there to set the performance state and enable the stuff as well.
> > > That's why it looks incorrect in your case, where the function was
> > > only required to be called once, and you are ending up calling it on
> > > each resume. Limiting that with another local variable is bad as well.
> >
> > We discussed variant with get_performance_state() previously and Ulf
> > didn't like it either since it still requires to touch 'internals' of GENPD.
>
> Hmm, I wonder if that would be a problem since only genpd core is
> going to call that routine anyway.

Me and Dmitry discussed adding a new genpd callback for this. I agreed
that it seems like a reasonable thing to add, if he insists.

The intent was to invoke the new callback from __genpd_dev_pm_attach()
when the device has been attached to its genpd. This allows the
callback, to invoke clk_get_rate() and then dev_pm_opp_set_rate(), to
update the vote according to the current state of the HW.

I am not sure if/why that approach seemed insufficient?

Another option to solve the problem, I think, is simply to patch
drivers to let them call dev_pm_opp_set_rate() during ->probe(), this
should synchronize the HW state too.

Dmitry, can you please elaborate on this?

Kind regards
Uffe

Viresh Kumar Aug. 18, 2021, 9:14 a.m. UTC | #17

On 18-08-21, 10:29, Ulf Hansson wrote:
> Me and Dmitry discussed adding a new genpd callback for this. I agreed
> that it seems like a reasonable thing to add, if he insists.
> 
> The intent was to invoke the new callback from __genpd_dev_pm_attach()
> when the device has been attached to its genpd. This allows the
> callback, to invoke clk_get_rate() and then dev_pm_opp_set_rate(), to
> update the vote according to the current state of the HW.

I wouldn't call dev_pm_opp_set_rate() from there, since it means
configure and enable (both) for different resources, clk, regulator,
genpd, etc..

What we need here is just configure. So something like this then:

- genpd->get_performance_state()
  -> dev_pm_opp_get_current_opp() //New API
  -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);

This can be done just once from probe() then.

> I am not sure if/why that approach seemed insufficient?
> 
> Another option to solve the problem, I think, is simply to patch
> drivers to let them call dev_pm_opp_set_rate() during ->probe(), this
> should synchronize the HW state too.

Dmitry already mentioned that this will make the device start
consuming power, and he doesn't want that, else we need an explicit
disble call as well.

Ulf Hansson Aug. 18, 2021, 9:41 a.m. UTC | #18

On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
>
> On 18-08-21, 10:29, Ulf Hansson wrote:
> > Me and Dmitry discussed adding a new genpd callback for this. I agreed
> > that it seems like a reasonable thing to add, if he insists.
> >
> > The intent was to invoke the new callback from __genpd_dev_pm_attach()
> > when the device has been attached to its genpd. This allows the
> > callback, to invoke clk_get_rate() and then dev_pm_opp_set_rate(), to
> > update the vote according to the current state of the HW.
>
> I wouldn't call dev_pm_opp_set_rate() from there, since it means
> configure and enable (both) for different resources, clk, regulator,
> genpd, etc..

Right, good point!

dev_pm_opp_set_rate() is best called from consumer drivers, as they
need to be in control.

>
> What we need here is just configure. So something like this then:
>
> - genpd->get_performance_state()
>   -> dev_pm_opp_get_current_opp() //New API
>   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
>
> This can be done just once from probe() then.

How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?

>
> > I am not sure if/why that approach seemed insufficient?
> >
> > Another option to solve the problem, I think, is simply to patch
> > drivers to let them call dev_pm_opp_set_rate() during ->probe(), this
> > should synchronize the HW state too.
>
> Dmitry already mentioned that this will make the device start
> consuming power, and he doesn't want that, else we need an explicit
> disble call as well.

I am sure I understand the problem. When a device is getting probed,
it needs to consume power, how else can the corresponding driver
successfully probe it?

>
> --
> viresh

Kind regards
Uffe

Ulf Hansson Aug. 18, 2021, 9:42 a.m. UTC | #19

On Wed, 18 Aug 2021 at 11:41, Ulf Hansson <[e-mail]> wrote:
>
> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
> >
> > On 18-08-21, 10:29, Ulf Hansson wrote:
> > > Me and Dmitry discussed adding a new genpd callback for this. I agreed
> > > that it seems like a reasonable thing to add, if he insists.
> > >
> > > The intent was to invoke the new callback from __genpd_dev_pm_attach()
> > > when the device has been attached to its genpd. This allows the
> > > callback, to invoke clk_get_rate() and then dev_pm_opp_set_rate(), to
> > > update the vote according to the current state of the HW.
> >
> > I wouldn't call dev_pm_opp_set_rate() from there, since it means
> > configure and enable (both) for different resources, clk, regulator,
> > genpd, etc..
>
> Right, good point!
>
> dev_pm_opp_set_rate() is best called from consumer drivers, as they
> need to be in control.
>
> >
> > What we need here is just configure. So something like this then:
> >
> > - genpd->get_performance_state()
> >   -> dev_pm_opp_get_current_opp() //New API
> >   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
> >
> > This can be done just once from probe() then.
>
> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?
>
> >
> > > I am not sure if/why that approach seemed insufficient?
> > >
> > > Another option to solve the problem, I think, is simply to patch
> > > drivers to let them call dev_pm_opp_set_rate() during ->probe(), this
> > > should synchronize the HW state too.
> >
> > Dmitry already mentioned that this will make the device start
> > consuming power, and he doesn't want that, else we need an explicit
> > disble call as well.
>
> I am sure I understand the problem. When a device is getting probed,

/s/I am sure/I am not sure

> it needs to consume power, how else can the corresponding driver
> successfully probe it?
>
> >
> > --
> > viresh
>
> Kind regards
> Uffe

Viresh Kumar Aug. 18, 2021, 9:50 a.m. UTC | #20

On 18-08-21, 11:41, Ulf Hansson wrote:
> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
> > What we need here is just configure. So something like this then:
> >
> > - genpd->get_performance_state()
> >   -> dev_pm_opp_get_current_opp() //New API
> >   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
> >
> > This can be done just once from probe() then.
> 
> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?

The opp core already has a way of finding current OPP, that's what
Dmitry is trying to use here. It finds it using clk_get_rate(), if
that is zero, it picks the lowest freq possible.

> I am sure I understand the problem. When a device is getting probed,
> it needs to consume power, how else can the corresponding driver
> successfully probe it?

Dmitry can answer that better, but a device doesn't necessarily need
to consume energy in probe. It can consume bus clock, like APB we
have, but the more energy consuming stuff can be left disabled until
the time a user comes up. Probe will just end up registering the
driver and initializing it.

Ulf Hansson Aug. 18, 2021, 10:08 a.m. UTC | #21

On Wed, 18 Aug 2021 at 11:50, Viresh Kumar <[e-mail]> wrote:
>
> On 18-08-21, 11:41, Ulf Hansson wrote:
> > On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
> > > What we need here is just configure. So something like this then:
> > >
> > > - genpd->get_performance_state()
> > >   -> dev_pm_opp_get_current_opp() //New API
> > >   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
> > >
> > > This can be done just once from probe() then.
> >
> > How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?
>
> The opp core already has a way of finding current OPP, that's what
> Dmitry is trying to use here. It finds it using clk_get_rate(), if
> that is zero, it picks the lowest freq possible.
>
> > I am sure I understand the problem. When a device is getting probed,
> > it needs to consume power, how else can the corresponding driver
> > successfully probe it?
>
> Dmitry can answer that better, but a device doesn't necessarily need
> to consume energy in probe. It can consume bus clock, like APB we
> have, but the more energy consuming stuff can be left disabled until
> the time a user comes up. Probe will just end up registering the
> driver and initializing it.

That's perfectly fine, as then it's likely that it won't vote for an
OPP, but can postpone that as well.

Perhaps the problem is rather that the HW may already carry a non-zero
vote made from a bootloader. If the consumer driver tries to clear
that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would
still not lead to any updates of the performance state in genpd,
because genpd internally has initialized the performance-state to
zero.

Dmitry?

>
> --
> viresh

Kind regards
Uffe

Dmitry Osipenko Aug. 18, 2021, 3:43 p.m. UTC | #22

18.08.2021 13:08, Ulf Hansson пишет:
> On Wed, 18 Aug 2021 at 11:50, Viresh Kumar <[e-mail]> wrote:
>>
>> On 18-08-21, 11:41, Ulf Hansson wrote:
>>> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
>>>> What we need here is just configure. So something like this then:
>>>>
>>>> - genpd->get_performance_state()
>>>>   -> dev_pm_opp_get_current_opp() //New API
>>>>   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
>>>>
>>>> This can be done just once from probe() then.
>>>
>>> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?
>>
>> The opp core already has a way of finding current OPP, that's what
>> Dmitry is trying to use here. It finds it using clk_get_rate(), if
>> that is zero, it picks the lowest freq possible.
>>
>>> I am sure I understand the problem. When a device is getting probed,
>>> it needs to consume power, how else can the corresponding driver
>>> successfully probe it?
>>
>> Dmitry can answer that better, but a device doesn't necessarily need
>> to consume energy in probe. It can consume bus clock, like APB we
>> have, but the more energy consuming stuff can be left disabled until
>> the time a user comes up. Probe will just end up registering the
>> driver and initializing it.
> 
> That's perfectly fine, as then it's likely that it won't vote for an
> OPP, but can postpone that as well.
> 
> Perhaps the problem is rather that the HW may already carry a non-zero
> vote made from a bootloader. If the consumer driver tries to clear
> that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would
> still not lead to any updates of the performance state in genpd,
> because genpd internally has initialized the performance-state to
> zero.

We don't need to discover internal SoC devices because we use
device-tree on ARM. For most devices power isn't required at a probe
time because probe function doesn't touch h/w at all, thus devices are
left in suspended state after probe.

We have three components comprising PM on Tegra:

1. Power gate
2. Clock state
3. Voltage state

GENPD on/off represents the 'power gate'.

Clock and reset are controlled by device drivers using clk and rst APIs.

Voltage state is represented by GENPD's performance level.

GENPD core assumes that at a first rpm-resume of a consumer device, its
genpd_performance=0. Not true for Tegra because h/w of the device is
preconfigured to a non-zero perf level initially, h/w may not support
zero level at all.

GENPD core assumes that consumer devices can work at any performance
level. Not true for Tegra because voltage needs to be set in accordance
to the clock rate before clock is enabled, otherwise h/w won't work
properly, perhaps clock may be unstable or h/w won't be latching.

Performance level should be set to 0 while device is suspended.
Performance level needs to be bumped on rpm-resume of a device in
accordance to h/w state before hardware is enabled.

Dmitry Osipenko Aug. 18, 2021, 3:46 p.m. UTC | #23

18.08.2021 18:43, Dmitry Osipenko пишет:
> 18.08.2021 13:08, Ulf Hansson пишет:
>> On Wed, 18 Aug 2021 at 11:50, Viresh Kumar <[e-mail]> wrote:
>>>
>>> On 18-08-21, 11:41, Ulf Hansson wrote:
>>>> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
>>>>> What we need here is just configure. So something like this then:
>>>>>
>>>>> - genpd->get_performance_state()
>>>>>   -> dev_pm_opp_get_current_opp() //New API
>>>>>   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
>>>>>
>>>>> This can be done just once from probe() then.
>>>>
>>>> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?
>>>
>>> The opp core already has a way of finding current OPP, that's what
>>> Dmitry is trying to use here. It finds it using clk_get_rate(), if
>>> that is zero, it picks the lowest freq possible.
>>>
>>>> I am sure I understand the problem. When a device is getting probed,
>>>> it needs to consume power, how else can the corresponding driver
>>>> successfully probe it?
>>>
>>> Dmitry can answer that better, but a device doesn't necessarily need
>>> to consume energy in probe. It can consume bus clock, like APB we
>>> have, but the more energy consuming stuff can be left disabled until
>>> the time a user comes up. Probe will just end up registering the
>>> driver and initializing it.
>>
>> That's perfectly fine, as then it's likely that it won't vote for an
>> OPP, but can postpone that as well.
>>
>> Perhaps the problem is rather that the HW may already carry a non-zero
>> vote made from a bootloader. If the consumer driver tries to clear
>> that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would
>> still not lead to any updates of the performance state in genpd,
>> because genpd internally has initialized the performance-state to
>> zero.
> 
> We don't need to discover internal SoC devices because we use
> device-tree on ARM. For most devices power isn't required at a probe
> time because probe function doesn't touch h/w at all, thus devices are
> left in suspended state after probe.
> 
> We have three components comprising PM on Tegra:
> 
> 1. Power gate
> 2. Clock state
> 3. Voltage state
> 
> GENPD on/off represents the 'power gate'.
> 
> Clock and reset are controlled by device drivers using clk and rst APIs.
> 
> Voltage state is represented by GENPD's performance level.

OPP framework couples the performance level with the clock rate.

> GENPD core assumes that at a first rpm-resume of a consumer device, its
> genpd_performance=0. Not true for Tegra because h/w of the device is
> preconfigured to a non-zero perf level initially, h/w may not support
> zero level at all.
> 
> GENPD core assumes that consumer devices can work at any performance
> level. Not true for Tegra because voltage needs to be set in accordance
> to the clock rate before clock is enabled, otherwise h/w won't work
> properly, perhaps clock may be unstable or h/w won't be latching.
> 
> Performance level should be set to 0 while device is suspended.
> Performance level needs to be bumped on rpm-resume of a device in
> accordance to h/w state before hardware is enabled.
>

Dmitry Osipenko Aug. 18, 2021, 3:55 p.m. UTC | #24

18.08.2021 12:41, Ulf Hansson пишет:
> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
>>
>> On 18-08-21, 10:29, Ulf Hansson wrote:
>>> Me and Dmitry discussed adding a new genpd callback for this. I agreed
>>> that it seems like a reasonable thing to add, if he insists.

Either way gives the equal result. The new callback allows to remove the
boilerplate dev_pm_opp_set_rate(clk_get_rate() code from the rpm-resume
of consumer devices, that's it.

>>> The intent was to invoke the new callback from __genpd_dev_pm_attach()
>>> when the device has been attached to its genpd. This allows the
>>> callback, to invoke clk_get_rate() and then dev_pm_opp_set_rate(), to
>>> update the vote according to the current state of the HW.
>>
>> I wouldn't call dev_pm_opp_set_rate() from there, since it means
>> configure and enable (both) for different resources, clk, regulator,
>> genpd, etc..
> 
> Right, good point!
> 
> dev_pm_opp_set_rate() is best called from consumer drivers, as they
> need to be in control.
>> What we need here is just configure. So something like this then:
The intent wasn't to use dev_pm_opp_set_rate() from
__genpd_dev_pm_attach(), but to set genpd->rpm_pstate in accordance to
the h/w configuration.

On Tegra we have a chain of PDs and it's not trivial to convert the
device's OPP into pstate because only the parent domain can translate
the required OPP.

Viresh, please take a look at what I did in [1]. Maybe it could be done
in another way.

[1]
https://patchwork.ozlabs.org/project/linux-tegra/patch/[e-mail]/

Viresh Kumar Aug. 19, 2021, 6:16 a.m. UTC | #25

On 18-08-21, 18:55, Dmitry Osipenko wrote:
> 18.08.2021 12:41, Ulf Hansson пишет:
> 
> Either way gives the equal result. The new callback allows to remove the
> boilerplate dev_pm_opp_set_rate(clk_get_rate() code from the rpm-resume
> of consumer devices, that's it.

It may not be equal, as dev_pm_opp_set_rate() may do additional stuff,
now or in a later implementation. Currently it only does
regulator_enable() as a special case, but it can be clk_enable() as
well. Also, this tries to solve the problem in a tricky/hacky way,
while all you wanted was to make the genpd aware of what the
performance state should be.

Your driver can break tomorrow if we started to do more stuff from
this API at another time.

> > dev_pm_opp_set_rate() is best called from consumer drivers, as they
> > need to be in control.
> >> What we need here is just configure. So something like this then:
> The intent wasn't to use dev_pm_opp_set_rate() from
> __genpd_dev_pm_attach(), but to set genpd->rpm_pstate in accordance to
> the h/w configuration.

Right.

> On Tegra we have a chain of PDs and it's not trivial to convert the
> device's OPP into pstate because only the parent domain can translate
> the required OPP.

The driver should just be required to make a call, and OPP/genpd core
should return it a value. This is already done today while setting the
pstate for a device. The same frameworks must be able to supply a
value to be used for the device.

> Viresh, please take a look at what I did in [1]. Maybe it could be done
> in another way.

I looked into this and looked like too much trouble. The
implementation needs to be simple. I am not sure I understand all the
problems you faced while doing that, would be better to start with a
simpler implementation of get_performance_state() kind of API for
genpd, after the domain is attached and its OPP table is initialized.

Note, that the OPP table isn't required to be fully initialized for
the device at this point, we can parse the DT as well if needed be.

Ulf Hansson Aug. 19, 2021, 1:07 p.m. UTC | #26

On Wed, 18 Aug 2021 at 17:43, Dmitry Osipenko <[e-mail]> wrote:
>
> 18.08.2021 13:08, Ulf Hansson пишет:
> > On Wed, 18 Aug 2021 at 11:50, Viresh Kumar <[e-mail]> wrote:
> >>
> >> On 18-08-21, 11:41, Ulf Hansson wrote:
> >>> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
> >>>> What we need here is just configure. So something like this then:
> >>>>
> >>>> - genpd->get_performance_state()
> >>>>   -> dev_pm_opp_get_current_opp() //New API
> >>>>   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
> >>>>
> >>>> This can be done just once from probe() then.
> >>>
> >>> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?
> >>
> >> The opp core already has a way of finding current OPP, that's what
> >> Dmitry is trying to use here. It finds it using clk_get_rate(), if
> >> that is zero, it picks the lowest freq possible.
> >>
> >>> I am sure I understand the problem. When a device is getting probed,
> >>> it needs to consume power, how else can the corresponding driver
> >>> successfully probe it?
> >>
> >> Dmitry can answer that better, but a device doesn't necessarily need
> >> to consume energy in probe. It can consume bus clock, like APB we
> >> have, but the more energy consuming stuff can be left disabled until
> >> the time a user comes up. Probe will just end up registering the
> >> driver and initializing it.
> >
> > That's perfectly fine, as then it's likely that it won't vote for an
> > OPP, but can postpone that as well.
> >
> > Perhaps the problem is rather that the HW may already carry a non-zero
> > vote made from a bootloader. If the consumer driver tries to clear
> > that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would
> > still not lead to any updates of the performance state in genpd,
> > because genpd internally has initialized the performance-state to
> > zero.
>
> We don't need to discover internal SoC devices because we use
> device-tree on ARM. For most devices power isn't required at a probe
> time because probe function doesn't touch h/w at all, thus devices are
> left in suspended state after probe.
>
> We have three components comprising PM on Tegra:
>
> 1. Power gate
> 2. Clock state
> 3. Voltage state
>
> GENPD on/off represents the 'power gate'.
>
> Clock and reset are controlled by device drivers using clk and rst APIs.
>
> Voltage state is represented by GENPD's performance level.
>
> GENPD core assumes that at a first rpm-resume of a consumer device, its
> genpd_performance=0. Not true for Tegra because h/w of the device is
> preconfigured to a non-zero perf level initially, h/w may not support
> zero level at all.

I think you may be misunderstanding genpd's behaviour around this, but
let me elaborate.

In genpd_runtime_resume(), we try to restore the performance state for
the device that genpd_runtime_suspend() *may* have dropped earlier.
That means, if genpd_runtime_resume() is called prior
genpd_runtime_suspend() for the first time, it means that
genpd_runtime_resume() will *not* restore a performance state, but
instead just leave the performance state as is for the device (see
genpd_restore_performance_state()).

In other words, a consumer driver may use the following sequence to
set an initial performance state for the device during ->probe():

...
rate = clk_get_rate()
dev_pm_opp_set_rate(rate)

pm_runtime_enable()
pm_runtime_resume_and_get()
...

Note that, it's the consumer driver's responsibility to manage device
specific resources, in its ->runtime_suspend|resume() callbacks.
Typically that means dealing with clock gating/ungating, for example.

In the other scenario where a consumer driver prefers to *not* call
pm_runtime_resume_and_get() in its ->probe(), because it doesn't need
to power on the device to complete probing, then we don't want to vote
for an OPP at all - and we also want the performance state for the
device in genpd to be set to zero. Correct?

Is this the main problem you are trying to solve, because I think this
doesn't work out of the box as of today?

There is another concern though, but perhaps it's not a problem after
all. Viresh told us that dev_pm_opp_set_rate() may turn on resources
like clock/regulators. That could certainly be problematic, in
particular if the device and its genpd have OPP tables associated with
it and the consumer driver wants to follow the above sequence in
probe.

Viresh, can you please chime in here and elaborate on some of the
magic happening behind dev_pm_opp_set_rate() API - is there a problem
here or not?

>
> GENPD core assumes that consumer devices can work at any performance
> level. Not true for Tegra because voltage needs to be set in accordance
> to the clock rate before clock is enabled, otherwise h/w won't work
> properly, perhaps clock may be unstable or h/w won't be latching.

Correct. Genpd relies on the callers to use the OPP framework if there
are constraints like you describe above.

That said, it's not forbidden for a consumer driver to call
dev_pm_genpd_set_performance_state() directly, but then it better
knows exactly what it's doing.

>
> Performance level should be set to 0 while device is suspended.

Do you mean system suspend or runtime suspend? Or both?

> Performance level needs to be bumped on rpm-resume of a device in
> accordance to h/w state before hardware is enabled.

Assuming there was a performance state set for the device when
genpd_runtime_suspend() was called, genpd_runtime_resume() will
restore that state according to the sequence you described.

Kind regards
Uffe

Ulf Hansson Aug. 19, 2021, 2:55 p.m. UTC | #27

On Thu, 19 Aug 2021 at 08:17, Viresh Kumar <[e-mail]> wrote:
>
> On 18-08-21, 18:55, Dmitry Osipenko wrote:
> > 18.08.2021 12:41, Ulf Hansson пишет:
> >
> > Either way gives the equal result. The new callback allows to remove the
> > boilerplate dev_pm_opp_set_rate(clk_get_rate() code from the rpm-resume
> > of consumer devices, that's it.
>
> It may not be equal, as dev_pm_opp_set_rate() may do additional stuff,
> now or in a later implementation. Currently it only does
> regulator_enable() as a special case, but it can be clk_enable() as
> well. Also, this tries to solve the problem in a tricky/hacky way,
> while all you wanted was to make the genpd aware of what the
> performance state should be.
>
> Your driver can break tomorrow if we started to do more stuff from
> this API at another time.
>
> > > dev_pm_opp_set_rate() is best called from consumer drivers, as they
> > > need to be in control.
> > >> What we need here is just configure. So something like this then:
> > The intent wasn't to use dev_pm_opp_set_rate() from
> > __genpd_dev_pm_attach(), but to set genpd->rpm_pstate in accordance to
> > the h/w configuration.
>
> Right.
>
> > On Tegra we have a chain of PDs and it's not trivial to convert the
> > device's OPP into pstate because only the parent domain can translate
> > the required OPP.
>
> The driver should just be required to make a call, and OPP/genpd core
> should return it a value. This is already done today while setting the
> pstate for a device. The same frameworks must be able to supply a
> value to be used for the device.

Right, that sounds reasonable.

We already have pm_genpd_opp_to_performance_state() which translates
an OPP to a performance state. This function invokes the
->opp_to_performance_state() for a genpd. Maybe we need to allow a
genpd to not have ->opp_to_performance_state() callback assigned
though, but continue up in the hierarchy to see if the parent has the
callback assigned, to make this work for Tegra?

Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(),
allowing us to pass the device instead of the genpd. But that's a
minor thing.

Finally, the precondition to use the above, is to first get a handle
to an OPP table. This is where I am struggling to find a generic
solution, because I guess that would be platform or even consumer
driver specific for how to do this. And at what point should we do
this?

>
> > Viresh, please take a look at what I did in [1]. Maybe it could be done
> > in another way.
>
> I looked into this and looked like too much trouble. The
> implementation needs to be simple. I am not sure I understand all the
> problems you faced while doing that, would be better to start with a
> simpler implementation of get_performance_state() kind of API for
> genpd, after the domain is attached and its OPP table is initialized.
>
> Note, that the OPP table isn't required to be fully initialized for
> the device at this point, we can parse the DT as well if needed be.

Sure, but as I indicated above, you need some kind of input data to
figure out what OPP table to pick, before you can translate that into
a performance state. Is that always the clock rate, for example?

Perhaps, we should start with adding a dev_pm_opp_get_from_rate() or
what do you think? Do you have other suggestions?

>
> --
> viresh

Kind regards
Uffe

Dmitry Osipenko Aug. 19, 2021, 7:35 p.m. UTC | #28

19.08.2021 16:07, Ulf Hansson пишет:
> On Wed, 18 Aug 2021 at 17:43, Dmitry Osipenko <[e-mail]> wrote:
>>
>> 18.08.2021 13:08, Ulf Hansson пишет:
>>> On Wed, 18 Aug 2021 at 11:50, Viresh Kumar <[e-mail]> wrote:
>>>>
>>>> On 18-08-21, 11:41, Ulf Hansson wrote:
>>>>> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
>>>>>> What we need here is just configure. So something like this then:
>>>>>>
>>>>>> - genpd->get_performance_state()
>>>>>>   -> dev_pm_opp_get_current_opp() //New API
>>>>>>   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
>>>>>>
>>>>>> This can be done just once from probe() then.
>>>>>
>>>>> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?
>>>>
>>>> The opp core already has a way of finding current OPP, that's what
>>>> Dmitry is trying to use here. It finds it using clk_get_rate(), if
>>>> that is zero, it picks the lowest freq possible.
>>>>
>>>>> I am sure I understand the problem. When a device is getting probed,
>>>>> it needs to consume power, how else can the corresponding driver
>>>>> successfully probe it?
>>>>
>>>> Dmitry can answer that better, but a device doesn't necessarily need
>>>> to consume energy in probe. It can consume bus clock, like APB we
>>>> have, but the more energy consuming stuff can be left disabled until
>>>> the time a user comes up. Probe will just end up registering the
>>>> driver and initializing it.
>>>
>>> That's perfectly fine, as then it's likely that it won't vote for an
>>> OPP, but can postpone that as well.
>>>
>>> Perhaps the problem is rather that the HW may already carry a non-zero
>>> vote made from a bootloader. If the consumer driver tries to clear
>>> that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would
>>> still not lead to any updates of the performance state in genpd,
>>> because genpd internally has initialized the performance-state to
>>> zero.
>>
>> We don't need to discover internal SoC devices because we use
>> device-tree on ARM. For most devices power isn't required at a probe
>> time because probe function doesn't touch h/w at all, thus devices are
>> left in suspended state after probe.
>>
>> We have three components comprising PM on Tegra:
>>
>> 1. Power gate
>> 2. Clock state
>> 3. Voltage state
>>
>> GENPD on/off represents the 'power gate'.
>>
>> Clock and reset are controlled by device drivers using clk and rst APIs.
>>
>> Voltage state is represented by GENPD's performance level.
>>
>> GENPD core assumes that at a first rpm-resume of a consumer device, its
>> genpd_performance=0. Not true for Tegra because h/w of the device is
>> preconfigured to a non-zero perf level initially, h/w may not support
>> zero level at all.
> 
> I think you may be misunderstanding genpd's behaviour around this, but
> let me elaborate.
> 
> In genpd_runtime_resume(), we try to restore the performance state for
> the device that genpd_runtime_suspend() *may* have dropped earlier.
> That means, if genpd_runtime_resume() is called prior
> genpd_runtime_suspend() for the first time, it means that
> genpd_runtime_resume() will *not* restore a performance state, but
> instead just leave the performance state as is for the device (see
> genpd_restore_performance_state()).
> 
> In other words, a consumer driver may use the following sequence to
> set an initial performance state for the device during ->probe():
> 
> ...
> rate = clk_get_rate()
> dev_pm_opp_set_rate(rate)
> 
> pm_runtime_enable()
> pm_runtime_resume_and_get()
> ...
> 
> Note that, it's the consumer driver's responsibility to manage device
> specific resources, in its ->runtime_suspend|resume() callbacks.
> Typically that means dealing with clock gating/ungating, for example.
> 
> In the other scenario where a consumer driver prefers to *not* call
> pm_runtime_resume_and_get() in its ->probe(), because it doesn't need
> to power on the device to complete probing, then we don't want to vote
> for an OPP at all - and we also want the performance state for the
> device in genpd to be set to zero. Correct?

Yes

> Is this the main problem you are trying to solve, because I think this
> doesn't work out of the box as of today?

The main problem is that the restored performance state is zero for the
first genpd_runtime_resume(), while it's not zero from the h/w perspective.

> There is another concern though, but perhaps it's not a problem after
> all. Viresh told us that dev_pm_opp_set_rate() may turn on resources
> like clock/regulators. That could certainly be problematic, in
> particular if the device and its genpd have OPP tables associated with
> it and the consumer driver wants to follow the above sequence in
> probe.

dev_pm_opp_set_rate() won't enable clocks and regulators, but it may
change the clock rate and voltage. This is also platform/driver specific
because it's up to OPP user how to configure OPP table. On Tegra we only
assign clock to OPP table, regulators are unused.

> Viresh, can you please chime in here and elaborate on some of the
> magic happening behind dev_pm_opp_set_rate() API - is there a problem
> here or not?
> 
>>
>> GENPD core assumes that consumer devices can work at any performance
>> level. Not true for Tegra because voltage needs to be set in accordance
>> to the clock rate before clock is enabled, otherwise h/w won't work
>> properly, perhaps clock may be unstable or h/w won't be latching.
> 
> Correct. Genpd relies on the callers to use the OPP framework if there
> are constraints like you describe above.
> 
> That said, it's not forbidden for a consumer driver to call
> dev_pm_genpd_set_performance_state() directly, but then it better
> knows exactly what it's doing.
> 
>>
>> Performance level should be set to 0 while device is suspended.
> 
> Do you mean system suspend or runtime suspend? Or both?

Runtime suspend.

>> Performance level needs to be bumped on rpm-resume of a device in
>> accordance to h/w state before hardware is enabled.
> 
> Assuming there was a performance state set for the device when
> genpd_runtime_suspend() was called, genpd_runtime_resume() will
> restore that state according to the sequence you described.

What do you think about adding API that will allow drivers to explicitly
set the restored performance state of a power domain?

Another option could be to change the GENPD core, making it to set the
rpm_pstate when dev_pm_genpd_set_performance_state(dev) is invoked and
device is rpm-suspended, instead of calling the
genpd->set_performance_state callback.

Then drivers will be able to sync the perf state at a probe time.

What do you think?

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index a934c679e6ce..cc15ab9eacc9 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -435,7 +435,7 @@ static void genpd_restore_performance_state(struct
device *dev,
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int
state)
 {
 	struct generic_pm_domain *genpd;
-	int ret;
+	int ret = 0;

 	genpd = dev_to_genpd_safe(dev);
 	if (!genpd)
@@ -446,7 +446,10 @@ int dev_pm_genpd_set_performance_state(struct
device *dev, unsigned int state)
 		return -EINVAL;

 	genpd_lock(genpd);
-	ret = genpd_set_performance_state(dev, state);
+	if (pm_runtime_suspended(dev))
+		dev_gpd_data(dev)->rpm_pstate = state;
+	else
+		ret = genpd_set_performance_state(dev, state);
 	genpd_unlock(genpd);

 	return ret;

Viresh Kumar Aug. 20, 2021, 5:07 a.m. UTC | #29

On 19-08-21, 22:35, Dmitry Osipenko wrote:
> 19.08.2021 16:07, Ulf Hansson пишет:
> > In the other scenario where a consumer driver prefers to *not* call
> > pm_runtime_resume_and_get() in its ->probe(), because it doesn't need
> > to power on the device to complete probing, then we don't want to vote
> > for an OPP at all - and we also want the performance state for the
> > device in genpd to be set to zero. Correct?
> 
> Yes
> 
> > Is this the main problem you are trying to solve, because I think this
> > doesn't work out of the box as of today?
> 
> The main problem is that the restored performance state is zero for the
> first genpd_runtime_resume(), while it's not zero from the h/w perspective.

This is exactly why I have been advocating that the genpd needs to
sync up with the hardware before any calls are made to it from the
consumer driver. Just what clock framework does to get the clock rate.

> > There is another concern though, but perhaps it's not a problem after
> > all. Viresh told us that dev_pm_opp_set_rate() may turn on resources
> > like clock/regulators. That could certainly be problematic, in
> > particular if the device and its genpd have OPP tables associated with
> > it and the consumer driver wants to follow the above sequence in
> > probe.
> 
> dev_pm_opp_set_rate() won't enable clocks and regulators, but it may

It does enable regulators right now, it may choose to enable clocks
later on, no guarantees.

> change the clock rate and voltage. This is also platform/driver specific
> because it's up to OPP user how to configure OPP table. On Tegra we only
> assign clock to OPP table, regulators are unused.

Right, over that platforms can set their own version of set-opp
callback, where all this is done from a platform specific callback.

> > Viresh, can you please chime in here and elaborate on some of the
> > magic happening behind dev_pm_opp_set_rate() API - is there a problem
> > here or not?

It configures clock, regulators, genpds, any required OPPs, + it
enables regulators right now.

Viresh Kumar Aug. 20, 2021, 5:18 a.m. UTC | #30

On 19-08-21, 16:55, Ulf Hansson wrote:
> Right, that sounds reasonable.
> 
> We already have pm_genpd_opp_to_performance_state() which translates
> an OPP to a performance state. This function invokes the
> ->opp_to_performance_state() for a genpd. Maybe we need to allow a
> genpd to not have ->opp_to_performance_state() callback assigned
> though, but continue up in the hierarchy to see if the parent has the
> callback assigned, to make this work for Tegra?
> 
> Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(),
> allowing us to pass the device instead of the genpd. But that's a
> minor thing.

I am not concerned a lot about how it gets implemented, and am not
sure as well, as I haven't looked into these details since sometime.
Any reasonable thing will be accepted, as simple as that.

> Finally, the precondition to use the above, is to first get a handle
> to an OPP table. This is where I am struggling to find a generic
> solution, because I guess that would be platform or even consumer
> driver specific for how to do this. And at what point should we do
> this?

Hmm, I am not very clear with the whole picture at this point of time.

Dmitry, can you try to frame a sequence of events/calls/etc that will
define what kind of devices we are looking at here, and how this can
be made to work ?

> > > Viresh, please take a look at what I did in [1]. Maybe it could be done
> > > in another way.
> >
> > I looked into this and looked like too much trouble. The
> > implementation needs to be simple. I am not sure I understand all the
> > problems you faced while doing that, would be better to start with a
> > simpler implementation of get_performance_state() kind of API for
> > genpd, after the domain is attached and its OPP table is initialized.
> >
> > Note, that the OPP table isn't required to be fully initialized for
> > the device at this point, we can parse the DT as well if needed be.
> 
> Sure, but as I indicated above, you need some kind of input data to
> figure out what OPP table to pick, before you can translate that into
> a performance state. Is that always the clock rate, for example?

Eventually it can be clock, bandwidth, or pstate of anther genpd, not
sure what all we are looking for now. It should be just clock right
now as far as I can imagine :)

> Perhaps, we should start with adding a dev_pm_opp_get_from_rate() or
> what do you think? Do you have other suggestions?

We already have similar APIs, so that won't be a problem. We also have
a mechanism inside the OPP core, frequency based, which is used to
guess the current OPP. Maybe we can enhance and use that directly
here.

Ulf Hansson Aug. 20, 2021, 12:42 p.m. UTC | #31

On Thu, 19 Aug 2021 at 21:35, Dmitry Osipenko <[e-mail]> wrote:
>
> 19.08.2021 16:07, Ulf Hansson пишет:
> > On Wed, 18 Aug 2021 at 17:43, Dmitry Osipenko <[e-mail]> wrote:
> >>
> >> 18.08.2021 13:08, Ulf Hansson пишет:
> >>> On Wed, 18 Aug 2021 at 11:50, Viresh Kumar <[e-mail]> wrote:
> >>>>
> >>>> On 18-08-21, 11:41, Ulf Hansson wrote:
> >>>>> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
> >>>>>> What we need here is just configure. So something like this then:
> >>>>>>
> >>>>>> - genpd->get_performance_state()
> >>>>>>   -> dev_pm_opp_get_current_opp() //New API
> >>>>>>   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
> >>>>>>
> >>>>>> This can be done just once from probe() then.
> >>>>>
> >>>>> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?
> >>>>
> >>>> The opp core already has a way of finding current OPP, that's what
> >>>> Dmitry is trying to use here. It finds it using clk_get_rate(), if
> >>>> that is zero, it picks the lowest freq possible.
> >>>>
> >>>>> I am sure I understand the problem. When a device is getting probed,
> >>>>> it needs to consume power, how else can the corresponding driver
> >>>>> successfully probe it?
> >>>>
> >>>> Dmitry can answer that better, but a device doesn't necessarily need
> >>>> to consume energy in probe. It can consume bus clock, like APB we
> >>>> have, but the more energy consuming stuff can be left disabled until
> >>>> the time a user comes up. Probe will just end up registering the
> >>>> driver and initializing it.
> >>>
> >>> That's perfectly fine, as then it's likely that it won't vote for an
> >>> OPP, but can postpone that as well.
> >>>
> >>> Perhaps the problem is rather that the HW may already carry a non-zero
> >>> vote made from a bootloader. If the consumer driver tries to clear
> >>> that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would
> >>> still not lead to any updates of the performance state in genpd,
> >>> because genpd internally has initialized the performance-state to
> >>> zero.
> >>
> >> We don't need to discover internal SoC devices because we use
> >> device-tree on ARM. For most devices power isn't required at a probe
> >> time because probe function doesn't touch h/w at all, thus devices are
> >> left in suspended state after probe.
> >>
> >> We have three components comprising PM on Tegra:
> >>
> >> 1. Power gate
> >> 2. Clock state
> >> 3. Voltage state
> >>
> >> GENPD on/off represents the 'power gate'.
> >>
> >> Clock and reset are controlled by device drivers using clk and rst APIs.
> >>
> >> Voltage state is represented by GENPD's performance level.
> >>
> >> GENPD core assumes that at a first rpm-resume of a consumer device, its
> >> genpd_performance=0. Not true for Tegra because h/w of the device is
> >> preconfigured to a non-zero perf level initially, h/w may not support
> >> zero level at all.
> >
> > I think you may be misunderstanding genpd's behaviour around this, but
> > let me elaborate.
> >
> > In genpd_runtime_resume(), we try to restore the performance state for
> > the device that genpd_runtime_suspend() *may* have dropped earlier.
> > That means, if genpd_runtime_resume() is called prior
> > genpd_runtime_suspend() for the first time, it means that
> > genpd_runtime_resume() will *not* restore a performance state, but
> > instead just leave the performance state as is for the device (see
> > genpd_restore_performance_state()).
> >
> > In other words, a consumer driver may use the following sequence to
> > set an initial performance state for the device during ->probe():
> >
> > ...
> > rate = clk_get_rate()
> > dev_pm_opp_set_rate(rate)
> >
> > pm_runtime_enable()
> > pm_runtime_resume_and_get()
> > ...
> >
> > Note that, it's the consumer driver's responsibility to manage device
> > specific resources, in its ->runtime_suspend|resume() callbacks.
> > Typically that means dealing with clock gating/ungating, for example.
> >
> > In the other scenario where a consumer driver prefers to *not* call
> > pm_runtime_resume_and_get() in its ->probe(), because it doesn't need
> > to power on the device to complete probing, then we don't want to vote
> > for an OPP at all - and we also want the performance state for the
> > device in genpd to be set to zero. Correct?
>
> Yes
>
> > Is this the main problem you are trying to solve, because I think this
> > doesn't work out of the box as of today?
>
> The main problem is that the restored performance state is zero for the
> first genpd_runtime_resume(), while it's not zero from the h/w perspective.

This should not be a problem, but can be handled by the consumer driver.

genpd_runtime_resume() calls genpd_restore_performance_state() to
restore a performance state for the device. However, in the scenario
you describe, "gpd_data->rpm_pstate" is zero, which makes
genpd_restore_performance_state() to just leave the device's
performance state as is - it will *not* restore the performance state
to zero.

To make the consumer driver deal with this, it would need to call
dev_pm_opp_set_rate() from within its ->runtime_resume() callback.

>
> > There is another concern though, but perhaps it's not a problem after
> > all. Viresh told us that dev_pm_opp_set_rate() may turn on resources
> > like clock/regulators. That could certainly be problematic, in
> > particular if the device and its genpd have OPP tables associated with
> > it and the consumer driver wants to follow the above sequence in
> > probe.
>
> dev_pm_opp_set_rate() won't enable clocks and regulators, but it may
> change the clock rate and voltage. This is also platform/driver specific
> because it's up to OPP user how to configure OPP table. On Tegra we only
> assign clock to OPP table, regulators are unused.
>
> > Viresh, can you please chime in here and elaborate on some of the
> > magic happening behind dev_pm_opp_set_rate() API - is there a problem
> > here or not?
> >
> >>
> >> GENPD core assumes that consumer devices can work at any performance
> >> level. Not true for Tegra because voltage needs to be set in accordance
> >> to the clock rate before clock is enabled, otherwise h/w won't work
> >> properly, perhaps clock may be unstable or h/w won't be latching.
> >
> > Correct. Genpd relies on the callers to use the OPP framework if there
> > are constraints like you describe above.
> >
> > That said, it's not forbidden for a consumer driver to call
> > dev_pm_genpd_set_performance_state() directly, but then it better
> > knows exactly what it's doing.
> >
> >>
> >> Performance level should be set to 0 while device is suspended.
> >
> > Do you mean system suspend or runtime suspend? Or both?
>
> Runtime suspend.

Alright. So that's already taken care of for us in genpd_runtime_suspend().

Or perhaps you have discovered some problem with this?

>
> >> Performance level needs to be bumped on rpm-resume of a device in
> >> accordance to h/w state before hardware is enabled.
> >
> > Assuming there was a performance state set for the device when
> > genpd_runtime_suspend() was called, genpd_runtime_resume() will
> > restore that state according to the sequence you described.
>
> What do you think about adding API that will allow drivers to explicitly
> set the restored performance state of a power domain?
>
> Another option could be to change the GENPD core, making it to set the
> rpm_pstate when dev_pm_genpd_set_performance_state(dev) is invoked and
> device is rpm-suspended, instead of calling the
> genpd->set_performance_state callback.
>
> Then drivers will be able to sync the perf state at a probe time.
>
> What do you think?

I don't think it's needed, see my reply earlier above. However your
change touches another problem though, see below.

>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index a934c679e6ce..cc15ab9eacc9 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -435,7 +435,7 @@ static void genpd_restore_performance_state(struct
> device *dev,
>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int
> state)
>  {
>         struct generic_pm_domain *genpd;
> -       int ret;
> +       int ret = 0;
>
>         genpd = dev_to_genpd_safe(dev);
>         if (!genpd)
> @@ -446,7 +446,10 @@ int dev_pm_genpd_set_performance_state(struct
> device *dev, unsigned int state)
>                 return -EINVAL;
>
>         genpd_lock(genpd);
> -       ret = genpd_set_performance_state(dev, state);
> +       if (pm_runtime_suspended(dev))
> +               dev_gpd_data(dev)->rpm_pstate = state;
> +       else
> +               ret = genpd_set_performance_state(dev, state);
>         genpd_unlock(genpd);

This doesn't work for all cases. For example, when a consumer driver
deploys runtime PM support in its ->probe() according to the below
sequence:

...
dev_pm_opp_set_rate(rate)
pm_runtime_get_noresume()
pm_runtime_set_active()
pm_runtime_enable()
...
pm_runtime_put()
...

We need to call genpd_set_performance_state() independently of whether
the device is runtime suspended or not.

Although, it actually seems like good idea to update
dev_gpd_data(dev)->rpm_pstate = state here, as to make sure
genpd_runtime_resume() doesn't restore an old/invalid value that was
saved while dropping the performance state vote for the device in
genpd_runtime_suspend() earlier.

Let me send a patch for this shortly, to close this window of a possible error.

>
>         return ret;
>
>

Kind regards
Uffe

Ulf Hansson Aug. 20, 2021, 12:57 p.m. UTC | #32

On Fri, 20 Aug 2021 at 07:18, Viresh Kumar <[e-mail]> wrote:
>
> On 19-08-21, 16:55, Ulf Hansson wrote:
> > Right, that sounds reasonable.
> >
> > We already have pm_genpd_opp_to_performance_state() which translates
> > an OPP to a performance state. This function invokes the
> > ->opp_to_performance_state() for a genpd. Maybe we need to allow a
> > genpd to not have ->opp_to_performance_state() callback assigned
> > though, but continue up in the hierarchy to see if the parent has the
> > callback assigned, to make this work for Tegra?
> >
> > Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(),
> > allowing us to pass the device instead of the genpd. But that's a
> > minor thing.
>
> I am not concerned a lot about how it gets implemented, and am not
> sure as well, as I haven't looked into these details since sometime.
> Any reasonable thing will be accepted, as simple as that.
>
> > Finally, the precondition to use the above, is to first get a handle
> > to an OPP table. This is where I am struggling to find a generic
> > solution, because I guess that would be platform or even consumer
> > driver specific for how to do this. And at what point should we do
> > this?
>
> Hmm, I am not very clear with the whole picture at this point of time.
>
> Dmitry, can you try to frame a sequence of events/calls/etc that will
> define what kind of devices we are looking at here, and how this can
> be made to work ?
>
> > > > Viresh, please take a look at what I did in [1]. Maybe it could be done
> > > > in another way.
> > >
> > > I looked into this and looked like too much trouble. The
> > > implementation needs to be simple. I am not sure I understand all the
> > > problems you faced while doing that, would be better to start with a
> > > simpler implementation of get_performance_state() kind of API for
> > > genpd, after the domain is attached and its OPP table is initialized.
> > >
> > > Note, that the OPP table isn't required to be fully initialized for
> > > the device at this point, we can parse the DT as well if needed be.
> >
> > Sure, but as I indicated above, you need some kind of input data to
> > figure out what OPP table to pick, before you can translate that into
> > a performance state. Is that always the clock rate, for example?
>
> Eventually it can be clock, bandwidth, or pstate of anther genpd, not
> sure what all we are looking for now. It should be just clock right
> now as far as I can imagine :)
>
> > Perhaps, we should start with adding a dev_pm_opp_get_from_rate() or
> > what do you think? Do you have other suggestions?
>
> We already have similar APIs, so that won't be a problem. We also have
> a mechanism inside the OPP core, frequency based, which is used to
> guess the current OPP. Maybe we can enhance and use that directly
> here.

After reading the last reply from Dmitry, I am starting to think that
the problem he is facing can be described and solved in a much easier
way.

If I am correct, it looks like we don't need to add APIs to get OPPs
for a clock rate or set initial performance state values according to
the HW in genpd.

See my other response to Dmitry, let's see where that leads us.

Kind regards
Uffe

Dmitry Osipenko Aug. 21, 2021, 5:34 p.m. UTC | #33

20.08.2021 15:42, Ulf Hansson пишет:
> On Thu, 19 Aug 2021 at 21:35, Dmitry Osipenko <[e-mail]> wrote:
>>
>> 19.08.2021 16:07, Ulf Hansson пишет:
>>> On Wed, 18 Aug 2021 at 17:43, Dmitry Osipenko <[e-mail]> wrote:
>>>>
>>>> 18.08.2021 13:08, Ulf Hansson пишет:
>>>>> On Wed, 18 Aug 2021 at 11:50, Viresh Kumar <[e-mail]> wrote:
>>>>>>
>>>>>> On 18-08-21, 11:41, Ulf Hansson wrote:
>>>>>>> On Wed, 18 Aug 2021 at 11:14, Viresh Kumar <[e-mail]> wrote:
>>>>>>>> What we need here is just configure. So something like this then:
>>>>>>>>
>>>>>>>> - genpd->get_performance_state()
>>>>>>>>   -> dev_pm_opp_get_current_opp() //New API
>>>>>>>>   -> dev_pm_genpd_set_performance_state(dev, current_opp->pstate);
>>>>>>>>
>>>>>>>> This can be done just once from probe() then.
>>>>>>>
>>>>>>> How would dev_pm_opp_get_current_opp() work? Do you have a suggestion?
>>>>>>
>>>>>> The opp core already has a way of finding current OPP, that's what
>>>>>> Dmitry is trying to use here. It finds it using clk_get_rate(), if
>>>>>> that is zero, it picks the lowest freq possible.
>>>>>>
>>>>>>> I am sure I understand the problem. When a device is getting probed,
>>>>>>> it needs to consume power, how else can the corresponding driver
>>>>>>> successfully probe it?
>>>>>>
>>>>>> Dmitry can answer that better, but a device doesn't necessarily need
>>>>>> to consume energy in probe. It can consume bus clock, like APB we
>>>>>> have, but the more energy consuming stuff can be left disabled until
>>>>>> the time a user comes up. Probe will just end up registering the
>>>>>> driver and initializing it.
>>>>>
>>>>> That's perfectly fine, as then it's likely that it won't vote for an
>>>>> OPP, but can postpone that as well.
>>>>>
>>>>> Perhaps the problem is rather that the HW may already carry a non-zero
>>>>> vote made from a bootloader. If the consumer driver tries to clear
>>>>> that vote (calling dev_pm_opp_set_rate(dev, 0), for example), it would
>>>>> still not lead to any updates of the performance state in genpd,
>>>>> because genpd internally has initialized the performance-state to
>>>>> zero.
>>>>
>>>> We don't need to discover internal SoC devices because we use
>>>> device-tree on ARM. For most devices power isn't required at a probe
>>>> time because probe function doesn't touch h/w at all, thus devices are
>>>> left in suspended state after probe.
>>>>
>>>> We have three components comprising PM on Tegra:
>>>>
>>>> 1. Power gate
>>>> 2. Clock state
>>>> 3. Voltage state
>>>>
>>>> GENPD on/off represents the 'power gate'.
>>>>
>>>> Clock and reset are controlled by device drivers using clk and rst APIs.
>>>>
>>>> Voltage state is represented by GENPD's performance level.
>>>>
>>>> GENPD core assumes that at a first rpm-resume of a consumer device, its
>>>> genpd_performance=0. Not true for Tegra because h/w of the device is
>>>> preconfigured to a non-zero perf level initially, h/w may not support
>>>> zero level at all.
>>>
>>> I think you may be misunderstanding genpd's behaviour around this, but
>>> let me elaborate.
>>>
>>> In genpd_runtime_resume(), we try to restore the performance state for
>>> the device that genpd_runtime_suspend() *may* have dropped earlier.
>>> That means, if genpd_runtime_resume() is called prior
>>> genpd_runtime_suspend() for the first time, it means that
>>> genpd_runtime_resume() will *not* restore a performance state, but
>>> instead just leave the performance state as is for the device (see
>>> genpd_restore_performance_state()).
>>>
>>> In other words, a consumer driver may use the following sequence to
>>> set an initial performance state for the device during ->probe():
>>>
>>> ...
>>> rate = clk_get_rate()
>>> dev_pm_opp_set_rate(rate)
>>>
>>> pm_runtime_enable()
>>> pm_runtime_resume_and_get()
>>> ...
>>>
>>> Note that, it's the consumer driver's responsibility to manage device
>>> specific resources, in its ->runtime_suspend|resume() callbacks.
>>> Typically that means dealing with clock gating/ungating, for example.
>>>
>>> In the other scenario where a consumer driver prefers to *not* call
>>> pm_runtime_resume_and_get() in its ->probe(), because it doesn't need
>>> to power on the device to complete probing, then we don't want to vote
>>> for an OPP at all - and we also want the performance state for the
>>> device in genpd to be set to zero. Correct?
>>
>> Yes
>>
>>> Is this the main problem you are trying to solve, because I think this
>>> doesn't work out of the box as of today?
>>
>> The main problem is that the restored performance state is zero for the
>> first genpd_runtime_resume(), while it's not zero from the h/w perspective.
> 
> This should not be a problem, but can be handled by the consumer driver.
> 
> genpd_runtime_resume() calls genpd_restore_performance_state() to
> restore a performance state for the device. However, in the scenario
> you describe, "gpd_data->rpm_pstate" is zero, which makes
> genpd_restore_performance_state() to just leave the device's
> performance state as is - it will *not* restore the performance state
> to zero.
> 
> To make the consumer driver deal with this, it would need to call
> dev_pm_opp_set_rate() from within its ->runtime_resume() callback.
> 
>>
>>> There is another concern though, but perhaps it's not a problem after
>>> all. Viresh told us that dev_pm_opp_set_rate() may turn on resources
>>> like clock/regulators. That could certainly be problematic, in
>>> particular if the device and its genpd have OPP tables associated with
>>> it and the consumer driver wants to follow the above sequence in
>>> probe.
>>
>> dev_pm_opp_set_rate() won't enable clocks and regulators, but it may
>> change the clock rate and voltage. This is also platform/driver specific
>> because it's up to OPP user how to configure OPP table. On Tegra we only
>> assign clock to OPP table, regulators are unused.
>>
>>> Viresh, can you please chime in here and elaborate on some of the
>>> magic happening behind dev_pm_opp_set_rate() API - is there a problem
>>> here or not?
>>>
>>>>
>>>> GENPD core assumes that consumer devices can work at any performance
>>>> level. Not true for Tegra because voltage needs to be set in accordance
>>>> to the clock rate before clock is enabled, otherwise h/w won't work
>>>> properly, perhaps clock may be unstable or h/w won't be latching.
>>>
>>> Correct. Genpd relies on the callers to use the OPP framework if there
>>> are constraints like you describe above.
>>>
>>> That said, it's not forbidden for a consumer driver to call
>>> dev_pm_genpd_set_performance_state() directly, but then it better
>>> knows exactly what it's doing.
>>>
>>>>
>>>> Performance level should be set to 0 while device is suspended.
>>>
>>> Do you mean system suspend or runtime suspend? Or both?
>>
>> Runtime suspend.
> 
> Alright. So that's already taken care of for us in genpd_runtime_suspend().
> 
> Or perhaps you have discovered some problem with this?
> 
>>
>>>> Performance level needs to be bumped on rpm-resume of a device in
>>>> accordance to h/w state before hardware is enabled.
>>>
>>> Assuming there was a performance state set for the device when
>>> genpd_runtime_suspend() was called, genpd_runtime_resume() will
>>> restore that state according to the sequence you described.
>>
>> What do you think about adding API that will allow drivers to explicitly
>> set the restored performance state of a power domain?
>>
>> Another option could be to change the GENPD core, making it to set the
>> rpm_pstate when dev_pm_genpd_set_performance_state(dev) is invoked and
>> device is rpm-suspended, instead of calling the
>> genpd->set_performance_state callback.
>>
>> Then drivers will be able to sync the perf state at a probe time.
>>
>> What do you think?
> 
> I don't think it's needed, see my reply earlier above. However your
> change touches another problem though, see below.
> 
>>
>> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
>> index a934c679e6ce..cc15ab9eacc9 100644
>> --- a/drivers/base/power/domain.c
>> +++ b/drivers/base/power/domain.c
>> @@ -435,7 +435,7 @@ static void genpd_restore_performance_state(struct
>> device *dev,
>>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int
>> state)
>>  {
>>         struct generic_pm_domain *genpd;
>> -       int ret;
>> +       int ret = 0;
>>
>>         genpd = dev_to_genpd_safe(dev);
>>         if (!genpd)
>> @@ -446,7 +446,10 @@ int dev_pm_genpd_set_performance_state(struct
>> device *dev, unsigned int state)
>>                 return -EINVAL;
>>
>>         genpd_lock(genpd);
>> -       ret = genpd_set_performance_state(dev, state);
>> +       if (pm_runtime_suspended(dev))
>> +               dev_gpd_data(dev)->rpm_pstate = state;
>> +       else
>> +               ret = genpd_set_performance_state(dev, state);
>>         genpd_unlock(genpd);
> 
> This doesn't work for all cases. For example, when a consumer driver
> deploys runtime PM support in its ->probe() according to the below
> sequence:
> 
> ...
> dev_pm_opp_set_rate(rate)
> pm_runtime_get_noresume()
> pm_runtime_set_active()
> pm_runtime_enable()
> ...
> pm_runtime_put()
> ...
> 
> We need to call genpd_set_performance_state() independently of whether
> the device is runtime suspended or not.

I don't see where is the problem in yours example.

pm_runtime_suspended() = false while RPM is disabled. When device is
resumed, the rpm_pstate=0, so it won't change the pstate on resume.

> Although, it actually seems like good idea to update
> dev_gpd_data(dev)->rpm_pstate = state here, as to make sure
> genpd_runtime_resume() doesn't restore an old/invalid value that was
> saved while dropping the performance state vote for the device in
> genpd_runtime_suspend() earlier.
> 
> Let me send a patch for this shortly, to close this window of a possible error.

It will also remove the need to resume device just to change the clock
rate, like I needed to do it in the PWM patch of this series.

Dmitry Osipenko Aug. 22, 2021, 6:35 p.m. UTC | #34

20.08.2021 08:18, Viresh Kumar пишет:
> On 19-08-21, 16:55, Ulf Hansson wrote:
>> Right, that sounds reasonable.
>>
>> We already have pm_genpd_opp_to_performance_state() which translates
>> an OPP to a performance state. This function invokes the
>> ->opp_to_performance_state() for a genpd. Maybe we need to allow a
>> genpd to not have ->opp_to_performance_state() callback assigned
>> though, but continue up in the hierarchy to see if the parent has the
>> callback assigned, to make this work for Tegra?
>>
>> Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(),
>> allowing us to pass the device instead of the genpd. But that's a
>> minor thing.
> 
> I am not concerned a lot about how it gets implemented, and am not
> sure as well, as I haven't looked into these details since sometime.
> Any reasonable thing will be accepted, as simple as that.
> 
>> Finally, the precondition to use the above, is to first get a handle
>> to an OPP table. This is where I am struggling to find a generic
>> solution, because I guess that would be platform or even consumer
>> driver specific for how to do this. And at what point should we do
>> this?

GENPD core can't get OPP table handle, setting up OPP table is a platform/driver specific operation.

> Hmm, I am not very clear with the whole picture at this point of time.
> 
> Dmitry, can you try to frame a sequence of events/calls/etc that will
> define what kind of devices we are looking at here, and how this can
> be made to work ?

Could you please clarify what do you mean by a "kind of devices"?

I made hack based on the recent discussions and it partially works. Getting clock rate involves resuming device which backs the clock and it also may use GENPD, so lockings are becoming complicated. It doesn't work at all if device uses multiple domains because virtual domain device doesn't have OPP table.

Setting up the performance state from a consumer driver is a cleaner variant so far. 

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index e1c8994ae225..faa0bbe99c98 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -410,11 +410,16 @@ static int genpd_drop_performance_state(struct device *dev)
 	return 0;
 }
 
-static void genpd_restore_performance_state(struct device *dev,
-					    unsigned int state)
+static int genpd_restore_performance_state(struct generic_pm_domain *genpd,
+					   struct device *dev,
+					   unsigned int state)
 {
+	int ret = 0;
+
 	if (state)
-		genpd_set_performance_state(dev, state);
+		ret = genpd_set_performance_state(dev, state);
+
+	return ret;
 }
 
 /**
@@ -435,7 +440,7 @@ static void genpd_restore_performance_state(struct device *dev,
 int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 {
 	struct generic_pm_domain *genpd;
-	int ret;
+	int ret = 0;
 
 	genpd = dev_to_genpd_safe(dev);
 	if (!genpd)
@@ -446,7 +451,10 @@ int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int state)
 		return -EINVAL;
 
 	genpd_lock(genpd);
-	ret = genpd_set_performance_state(dev, state);
+	if (pm_runtime_suspended(dev))
+		dev_gpd_data(dev)->rpm_pstate = state;
+	else
+		ret = genpd_set_performance_state(dev, state);
 	genpd_unlock(genpd);
 
 	return ret;
@@ -959,10 +967,25 @@ static int genpd_runtime_resume(struct device *dev)
 		goto out;
 	}
 
+	if (genpd->get_performance_state) {
+		ret = genpd->get_performance_state(genpd, dev);
+		if (ret < 0)
+			return ret;
+
+		if (ret > 0)
+			gpd_data->rpm_pstate = ret;
+	}
+
 	genpd_lock(genpd);
 	ret = genpd_power_on(genpd, 0);
-	if (!ret)
-		genpd_restore_performance_state(dev, gpd_data->rpm_pstate);
+	if (!ret) {
+		ret = genpd_restore_performance_state(genpd, dev,
+						      gpd_data->rpm_pstate);
+		if (ret)
+			genpd_power_off(genpd, true, 0);
+		else
+			gpd_data->rpm_pstate = 0;
+	}
 	genpd_unlock(genpd);
 
 	if (ret)
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 18016e49605f..982be2dba21e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2967,3 +2967,33 @@ int dev_pm_opp_sync(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync);
+
+/**
+ * dev_pm_opp_from_clk_rate() - Get OPP from current clock rate
+ * @dev:	device for which we do this operation
+ *
+ * Get OPP which corresponds to the current clock rate of a device.
+ *
+ * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
+ */
+struct dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
+{
+	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
+	struct opp_table *opp_table;
+	unsigned long freq;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return ERR_CAST(opp_table);
+
+	if (!IS_ERR(opp_table->clk)) {
+		freq = clk_get_rate(opp_table->clk);
+		opp = _find_freq_ceil(opp_table, &freq);
+	}
+
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_from_clk_rate);
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 7c9bc93147f1..03bad16e5318 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -506,6 +506,63 @@ static void tegra_pmc_scratch_writel(struct tegra_pmc *pmc, u32 value,
 		writel(value, pmc->scratch + offset);
 }
 
+static const char * const tegra_skip_compats[] = {
+	"nvidia,tegra20-sclk",
+	"nvidia,tegra30-sclk",
+	"nvidia,tegra30-pllc",
+	"nvidia,tegra30-plle",
+	"nvidia,tegra30-pllm",
+	"nvidia,tegra20-dc",
+	"nvidia,tegra30-dc",
+	"nvidia,tegra20-emc",
+	"nvidia,tegra30-emc",
+	NULL,
+};
+
+static int tegra_pmc_pd_get_performance_state(struct generic_pm_domain *genpd,
+					      struct device *dev)
+{
+	struct dev_pm_opp *opp;
+	int ret;
+
+	/*
+	 * Tegra114+ SocS don't support OPP yet.  But if they will get OPP
+	 * support, then we want to skip OPP for older kernels to preserve
+	 * compatibility of newer DTBs with older kernels.
+	 */
+	if (!pmc->soc->supports_core_domain)
+		return 0;
+
+	/*
+	 * The EMC devices are a special case because we have a protection
+	 * from non-EMC drivers getting clock handle before EMC driver is
+	 * fully initialized.  The goal of the protection is to prevent
+	 * devfreq driver from getting failures if it will try to change
+	 * EMC clock rate until clock is fully initialized.  The EMC drivers
+	 * will initialize the performance state by themselves.
+	 *
+	 * Display controller also is a special case because only controller
+	 * driver could get the clock rate based on configuration of internal
+	 * divider.
+	 *
+	 * Clock driver uses its own state syncing.
+	 */
+	if (of_device_compatible_match(dev->of_node, tegra_skip_compats))
+		return 0;
+
+	opp = dev_pm_opp_from_clk_rate(dev);
+	if (IS_ERR(opp)) {
+		dev_err(&genpd->dev, "failed to get current OPP for %s: %pe\n",
+			dev_name(dev), opp);
+		ret = PTR_ERR(opp);
+	} else {
+		ret = dev_pm_opp_get_required_pstate(opp, 0);
+		dev_pm_opp_put(opp);
+	}
+
+	return ret;
+}
+
 /*
  * TODO Figure out a way to call this with the struct tegra_pmc * passed in.
  * This currently doesn't work because readx_poll_timeout() can only operate
@@ -1238,6 +1295,7 @@ static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 
 	pg->id = id;
 	pg->genpd.name = np->name;
+	pg->genpd.get_performance_state = tegra_pmc_pd_get_performance_state;
 	pg->genpd.power_off = tegra_genpd_power_off;
 	pg->genpd.power_on = tegra_genpd_power_on;
 	pg->pmc = pmc;
@@ -1354,6 +1412,7 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
 		return -ENOMEM;
 
 	genpd->name = "core";
+	genpd->get_performance_state = tegra_pmc_pd_get_performance_state;
 	genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
 	genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 67017c9390c8..abe33be9828f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -133,6 +133,8 @@ struct generic_pm_domain {
 						 struct dev_pm_opp *opp);
 	int (*set_performance_state)(struct generic_pm_domain *genpd,
 				     unsigned int state);
+	int (*get_performance_state)(struct generic_pm_domain *genpd,
+				     struct device *dev);
 	struct gpd_dev_ops dev_ops;
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	ktime_t next_wakeup;	/* Maintained by the domain governor */
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 686122b59935..e7fd0dd493ca 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -169,6 +169,7 @@ void dev_pm_opp_remove_table(struct device *dev);
 void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
 int dev_pm_opp_sync_regulators(struct device *dev);
 int dev_pm_opp_sync(struct device *dev);
+struct dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev);
 #else
 static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
 {
@@ -440,6 +441,11 @@ static inline int dev_pm_opp_sync(struct device *dev)
 	return -EOPNOTSUPP;
 }
 
+static struct inline dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)
--

Ulf Hansson Aug. 23, 2021, 10:46 a.m. UTC | #35

[...]

> >>>> We have three components comprising PM on Tegra:
> >>>>
> >>>> 1. Power gate
> >>>> 2. Clock state
> >>>> 3. Voltage state
> >>>>
> >>>> GENPD on/off represents the 'power gate'.
> >>>>
> >>>> Clock and reset are controlled by device drivers using clk and rst APIs.
> >>>>
> >>>> Voltage state is represented by GENPD's performance level.
> >>>>
> >>>> GENPD core assumes that at a first rpm-resume of a consumer device, its
> >>>> genpd_performance=0. Not true for Tegra because h/w of the device is
> >>>> preconfigured to a non-zero perf level initially, h/w may not support
> >>>> zero level at all.
> >>>
> >>> I think you may be misunderstanding genpd's behaviour around this, but
> >>> let me elaborate.
> >>>
> >>> In genpd_runtime_resume(), we try to restore the performance state for
> >>> the device that genpd_runtime_suspend() *may* have dropped earlier.
> >>> That means, if genpd_runtime_resume() is called prior
> >>> genpd_runtime_suspend() for the first time, it means that
> >>> genpd_runtime_resume() will *not* restore a performance state, but
> >>> instead just leave the performance state as is for the device (see
> >>> genpd_restore_performance_state()).
> >>>
> >>> In other words, a consumer driver may use the following sequence to
> >>> set an initial performance state for the device during ->probe():
> >>>
> >>> ...
> >>> rate = clk_get_rate()
> >>> dev_pm_opp_set_rate(rate)
> >>>
> >>> pm_runtime_enable()
> >>> pm_runtime_resume_and_get()
> >>> ...
> >>>
> >>> Note that, it's the consumer driver's responsibility to manage device
> >>> specific resources, in its ->runtime_suspend|resume() callbacks.
> >>> Typically that means dealing with clock gating/ungating, for example.
> >>>
> >>> In the other scenario where a consumer driver prefers to *not* call
> >>> pm_runtime_resume_and_get() in its ->probe(), because it doesn't need
> >>> to power on the device to complete probing, then we don't want to vote
> >>> for an OPP at all - and we also want the performance state for the
> >>> device in genpd to be set to zero. Correct?
> >>
> >> Yes
> >>
> >>> Is this the main problem you are trying to solve, because I think this
> >>> doesn't work out of the box as of today?
> >>
> >> The main problem is that the restored performance state is zero for the
> >> first genpd_runtime_resume(), while it's not zero from the h/w perspective.
> >
> > This should not be a problem, but can be handled by the consumer driver.
> >
> > genpd_runtime_resume() calls genpd_restore_performance_state() to
> > restore a performance state for the device. However, in the scenario
> > you describe, "gpd_data->rpm_pstate" is zero, which makes
> > genpd_restore_performance_state() to just leave the device's
> > performance state as is - it will *not* restore the performance state
> > to zero.
> >
> > To make the consumer driver deal with this, it would need to call
> > dev_pm_opp_set_rate() from within its ->runtime_resume() callback.
> >
> >>
> >>> There is another concern though, but perhaps it's not a problem after
> >>> all. Viresh told us that dev_pm_opp_set_rate() may turn on resources
> >>> like clock/regulators. That could certainly be problematic, in
> >>> particular if the device and its genpd have OPP tables associated with
> >>> it and the consumer driver wants to follow the above sequence in
> >>> probe.
> >>
> >> dev_pm_opp_set_rate() won't enable clocks and regulators, but it may
> >> change the clock rate and voltage. This is also platform/driver specific
> >> because it's up to OPP user how to configure OPP table. On Tegra we only
> >> assign clock to OPP table, regulators are unused.
> >>
> >>> Viresh, can you please chime in here and elaborate on some of the
> >>> magic happening behind dev_pm_opp_set_rate() API - is there a problem
> >>> here or not?
> >>>
> >>>>
> >>>> GENPD core assumes that consumer devices can work at any performance
> >>>> level. Not true for Tegra because voltage needs to be set in accordance
> >>>> to the clock rate before clock is enabled, otherwise h/w won't work
> >>>> properly, perhaps clock may be unstable or h/w won't be latching.
> >>>
> >>> Correct. Genpd relies on the callers to use the OPP framework if there
> >>> are constraints like you describe above.
> >>>
> >>> That said, it's not forbidden for a consumer driver to call
> >>> dev_pm_genpd_set_performance_state() directly, but then it better
> >>> knows exactly what it's doing.
> >>>
> >>>>
> >>>> Performance level should be set to 0 while device is suspended.
> >>>
> >>> Do you mean system suspend or runtime suspend? Or both?
> >>
> >> Runtime suspend.
> >
> > Alright. So that's already taken care of for us in genpd_runtime_suspend().
> >
> > Or perhaps you have discovered some problem with this?
> >
> >>
> >>>> Performance level needs to be bumped on rpm-resume of a device in
> >>>> accordance to h/w state before hardware is enabled.
> >>>
> >>> Assuming there was a performance state set for the device when
> >>> genpd_runtime_suspend() was called, genpd_runtime_resume() will
> >>> restore that state according to the sequence you described.
> >>
> >> What do you think about adding API that will allow drivers to explicitly
> >> set the restored performance state of a power domain?
> >>
> >> Another option could be to change the GENPD core, making it to set the
> >> rpm_pstate when dev_pm_genpd_set_performance_state(dev) is invoked and
> >> device is rpm-suspended, instead of calling the
> >> genpd->set_performance_state callback.
> >>
> >> Then drivers will be able to sync the perf state at a probe time.
> >>
> >> What do you think?
> >
> > I don't think it's needed, see my reply earlier above. However your
> > change touches another problem though, see below.
> >
> >>
> >> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> >> index a934c679e6ce..cc15ab9eacc9 100644
> >> --- a/drivers/base/power/domain.c
> >> +++ b/drivers/base/power/domain.c
> >> @@ -435,7 +435,7 @@ static void genpd_restore_performance_state(struct
> >> device *dev,
> >>  int dev_pm_genpd_set_performance_state(struct device *dev, unsigned int
> >> state)
> >>  {
> >>         struct generic_pm_domain *genpd;
> >> -       int ret;
> >> +       int ret = 0;
> >>
> >>         genpd = dev_to_genpd_safe(dev);
> >>         if (!genpd)
> >> @@ -446,7 +446,10 @@ int dev_pm_genpd_set_performance_state(struct
> >> device *dev, unsigned int state)
> >>                 return -EINVAL;
> >>
> >>         genpd_lock(genpd);
> >> -       ret = genpd_set_performance_state(dev, state);
> >> +       if (pm_runtime_suspended(dev))
> >> +               dev_gpd_data(dev)->rpm_pstate = state;
> >> +       else
> >> +               ret = genpd_set_performance_state(dev, state);
> >>         genpd_unlock(genpd);
> >
> > This doesn't work for all cases. For example, when a consumer driver
> > deploys runtime PM support in its ->probe() according to the below
> > sequence:
> >
> > ...
> > dev_pm_opp_set_rate(rate)
> > pm_runtime_get_noresume()
> > pm_runtime_set_active()
> > pm_runtime_enable()
> > ...
> > pm_runtime_put()
> > ...
> >
> > We need to call genpd_set_performance_state() independently of whether
> > the device is runtime suspended or not.
>
> I don't see where is the problem in yours example.
>
> pm_runtime_suspended() = false while RPM is disabled. When device is
> resumed, the rpm_pstate=0, so it won't change the pstate on resume.

Yes, you are certainly correct, my bad! I mixed it up with
pm_runtime_status_suspended(), which only cares about the status.

So, after a second thought, your suggestion sounds very much
reasonable to me! I have also tried to consider all different
scenarios, including the system suspend/resume path, but I think it
should be fine.

I also think that a patch like the above should be considered as a
fix, because it actually fixes a problem, according to what I said in
my earlier reply, below.

Fixes : 5937c3ce2122 ("PM: domains: Drop/restore performance state
votes for devices at runtime PM").

>
> > Although, it actually seems like good idea to update
> > dev_gpd_data(dev)->rpm_pstate = state here, as to make sure
> > genpd_runtime_resume() doesn't restore an old/invalid value that was
> > saved while dropping the performance state vote for the device in
> > genpd_runtime_suspend() earlier.
> >
> > Let me send a patch for this shortly, to close this window of a possible error.
>
> It will also remove the need to resume device just to change the clock
> rate, like I needed to do it in the PWM patch of this series.

Do you want to send the patch formally? Or do you prefer it if I do it?

Kind regards
Uffe

Dmitry Osipenko Aug. 23, 2021, 3:54 p.m. UTC | #36

23.08.2021 13:46, Ulf Hansson пишет:
>>> ...
>>> dev_pm_opp_set_rate(rate)
>>> pm_runtime_get_noresume()
>>> pm_runtime_set_active()
>>> pm_runtime_enable()
>>> ...
>>> pm_runtime_put()
>>> ...
>>>
>>> We need to call genpd_set_performance_state() independently of whether
>>> the device is runtime suspended or not.
>>
>> I don't see where is the problem in yours example.
>>
>> pm_runtime_suspended() = false while RPM is disabled. When device is
>> resumed, the rpm_pstate=0, so it won't change the pstate on resume.
> 
> Yes, you are certainly correct, my bad! I mixed it up with
> pm_runtime_status_suspended(), which only cares about the status.
> 
> So, after a second thought, your suggestion sounds very much
> reasonable to me! I have also tried to consider all different
> scenarios, including the system suspend/resume path, but I think it
> should be fine.

It could be improved slightly to cover more cases.

> I also think that a patch like the above should be considered as a
> fix, because it actually fixes a problem, according to what I said in
> my earlier reply, below.
> 
> Fixes : 5937c3ce2122 ("PM: domains: Drop/restore performance state
> votes for devices at runtime PM").
> 
>>
>>> Although, it actually seems like good idea to update
>>> dev_gpd_data(dev)->rpm_pstate = state here, as to make sure
>>> genpd_runtime_resume() doesn't restore an old/invalid value that was
>>> saved while dropping the performance state vote for the device in
>>> genpd_runtime_suspend() earlier.
>>>
>>> Let me send a patch for this shortly, to close this window of a possible error.
>>
>> It will also remove the need to resume device just to change the clock
>> rate, like I needed to do it in the PWM patch of this series.
> 
> Do you want to send the patch formally? Or do you prefer it if I do it?

I'll send the patch.

Dmitry Osipenko Aug. 23, 2021, 8:24 p.m. UTC | #37

20.08.2021 15:57, Ulf Hansson пишет:
...
>> We already have similar APIs, so that won't be a problem. We also have
>> a mechanism inside the OPP core, frequency based, which is used to
>> guess the current OPP. Maybe we can enhance and use that directly
>> here.
> 
> After reading the last reply from Dmitry, I am starting to think that
> the problem he is facing can be described and solved in a much easier
> way.
> 
> If I am correct, it looks like we don't need to add APIs to get OPPs
> for a clock rate or set initial performance state values according to
> the HW in genpd.
> 
> See my other response to Dmitry, let's see where that leads us.

I'm going to start preparing v9 with GENPD performance state syncing moved into driver's probe where appropriate.

It's not clear to me whether it will be okay to add a generic OPP syncing by clock rate or should it be a Tegra-specific helper. Viresh, what do you think about this generic OPP helper:

/**
 * dev_pm_opp_sync_with_clk_rate() - Sync OPP state with clock rate
 * @dev:	device for which we do this operation
 *
 * Sync OPP table state with the current clock rate of device.
 *
 * Return: 0 on success or a negative error value.
 */
int dev_pm_opp_sync_with_clk_rate(struct device *dev)
{
	struct opp_table *opp_table;
	int ret = 0;

	/* Device may not have OPP table */
	opp_table = _find_opp_table(dev);
	if (IS_ERR(opp_table))
		return 0;

	/* Device may not use clock */
	if (IS_ERR(opp_table->clk))
		goto put_table;

	/* Device may have empty OPP table */
	if (!_get_opp_count(opp_table))
		goto put_table;

	ret = dev_pm_opp_set_rate(dev, clk_get_rate(opp_table->clk));
put_table:
	/* Drop reference taken by _find_opp_table() */
	dev_pm_opp_put_opp_table(opp_table);

	return ret;
}
EXPORT_SYMBOL_GPL(dev_pm_opp_sync_with_clk_rate);

Viresh Kumar Aug. 24, 2021, 3:04 a.m. UTC | #38

On 23-08-21, 23:24, Dmitry Osipenko wrote:
> It's not clear to me whether it will be okay to add a generic OPP syncing by clock rate or should it be a Tegra-specific helper. Viresh, what do you think about this generic OPP helper:
> 
> /**
>  * dev_pm_opp_sync_with_clk_rate() - Sync OPP state with clock rate
>  * @dev:	device for which we do this operation
>  *
>  * Sync OPP table state with the current clock rate of device.
>  *
>  * Return: 0 on success or a negative error value.
>  */
> int dev_pm_opp_sync_with_clk_rate(struct device *dev)
> {
> 	struct opp_table *opp_table;
> 	int ret = 0;
> 
> 	/* Device may not have OPP table */
> 	opp_table = _find_opp_table(dev);
> 	if (IS_ERR(opp_table))
> 		return 0;
> 
> 	/* Device may not use clock */
> 	if (IS_ERR(opp_table->clk))
> 		goto put_table;
> 
> 	/* Device may have empty OPP table */
> 	if (!_get_opp_count(opp_table))
> 		goto put_table;
> 
> 	ret = dev_pm_opp_set_rate(dev, clk_get_rate(opp_table->clk));
> put_table:
> 	/* Drop reference taken by _find_opp_table() */
> 	dev_pm_opp_put_opp_table(opp_table);
> 
> 	return ret;
> }
> EXPORT_SYMBOL_GPL(dev_pm_opp_sync_with_clk_rate);

I am not sure why you still need this, hope we were going another way
? Anyway I will have a look at what you have posted now.

Dmitry Osipenko Aug. 25, 2021, 3:41 p.m. UTC | #39

22.08.2021 21:35, Dmitry Osipenko пишет:
> 20.08.2021 08:18, Viresh Kumar пишет:
>> On 19-08-21, 16:55, Ulf Hansson wrote:
>>> Right, that sounds reasonable.
>>>
>>> We already have pm_genpd_opp_to_performance_state() which translates
>>> an OPP to a performance state. This function invokes the
>>> ->opp_to_performance_state() for a genpd. Maybe we need to allow a
>>> genpd to not have ->opp_to_performance_state() callback assigned
>>> though, but continue up in the hierarchy to see if the parent has the
>>> callback assigned, to make this work for Tegra?
>>>
>>> Perhaps we should add an API dev_pm_genpd_opp_to_performance_state(),
>>> allowing us to pass the device instead of the genpd. But that's a
>>> minor thing.
>>
>> I am not concerned a lot about how it gets implemented, and am not
>> sure as well, as I haven't looked into these details since sometime.
>> Any reasonable thing will be accepted, as simple as that.
>>
>>> Finally, the precondition to use the above, is to first get a handle
>>> to an OPP table. This is where I am struggling to find a generic
>>> solution, because I guess that would be platform or even consumer
>>> driver specific for how to do this. And at what point should we do
>>> this?
> 
> GENPD core can't get OPP table handle, setting up OPP table is a platform/driver specific operation.
> 
>> Hmm, I am not very clear with the whole picture at this point of time.
>>
>> Dmitry, can you try to frame a sequence of events/calls/etc that will
>> define what kind of devices we are looking at here, and how this can
>> be made to work ?
> 
> Could you please clarify what do you mean by a "kind of devices"?
> 
> I made hack based on the recent discussions and it partially works. Getting clock rate involves resuming device which backs the clock and it also may use GENPD, so lockings are becoming complicated. It doesn't work at all if device uses multiple domains because virtual domain device doesn't have OPP table.
> 
> Setting up the performance state from a consumer driver is a cleaner variant so far. 

Thinking a bit more about this, I got a nicer variant which actually works in all cases for Tegra.

Viresh / Ulf, what do you think about this:

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index 3a13a942d012..814b0f7a1909 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -2700,15 +2700,28 @@ static int __genpd_dev_pm_attach(struct device *dev, struct device *base_dev,
 		goto err;
 	} else if (pstate > 0) {
 		ret = dev_pm_genpd_set_performance_state(dev, pstate);
-		if (ret)
+		if (ret) {
+			dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
+				pd->name, ret);
 			goto err;
+		}
 		dev_gpd_data(dev)->default_pstate = pstate;
 	}
+
+	if (pd->get_performance_state) {
+		ret = pd->get_performance_state(pd, base_dev);
+		if (ret < 0) {
+			dev_err(dev, "failed to get performance state for power-domain %s: %d\n",
+				pd->name, ret);
+			goto err;
+		}
+
+		dev_gpd_data(dev)->rpm_pstate = ret;
+	}
+
 	return 1;
 
 err:
-	dev_err(dev, "failed to set required performance state for power-domain %s: %d\n",
-		pd->name, ret);
 	genpd_remove_device(pd, dev);
 	return ret;
 }
diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 2f1da33c2cd5..5f045030879b 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -2136,7 +2136,7 @@ struct opp_table *dev_pm_opp_set_clkname(struct device *dev, const char *name)
 	}
 
 	/* clk shouldn't be initialized at this point */
-	if (WARN_ON(opp_table->clk)) {
+	if (WARN_ON(!IS_ERR_OR_NULL(opp_table->clk))) {
 		ret = -EBUSY;
 		goto err;
 	}
@@ -2967,3 +2967,33 @@ int dev_pm_opp_sync(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync);
+
+/**
+ * dev_pm_opp_from_clk_rate() - Get OPP from current clock rate
+ * @dev:	device for which we do this operation
+ *
+ * Get OPP which corresponds to the current clock rate of a device.
+ *
+ * Return: pointer to 'struct dev_pm_opp' on success and errorno otherwise.
+ */
+struct dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
+{
+	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
+	struct opp_table *opp_table;
+	unsigned long freq;
+
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return ERR_CAST(opp_table);
+
+	if (!IS_ERR(opp_table->clk)) {
+		freq = clk_get_rate(opp_table->clk);
+		opp = _find_freq_ceil(opp_table, &freq);
+	}
+
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return opp;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_from_clk_rate);
diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
index 7c9bc93147f1..fc863d84f8d5 100644
--- a/drivers/soc/tegra/pmc.c
+++ b/drivers/soc/tegra/pmc.c
@@ -506,6 +506,96 @@ static void tegra_pmc_scratch_writel(struct tegra_pmc *pmc, u32 value,
 		writel(value, pmc->scratch + offset);
 }
 
+static const char * const tegra_pd_no_perf_compats[] = {
+	"nvidia,tegra20-sclk",
+	"nvidia,tegra30-sclk",
+	"nvidia,tegra30-pllc",
+	"nvidia,tegra30-plle",
+	"nvidia,tegra30-pllm",
+	"nvidia,tegra20-dc",
+	"nvidia,tegra30-dc",
+	"nvidia,tegra20-emc",
+	"nvidia,tegra30-emc",
+	NULL,
+};
+
+static int tegra_pmc_pd_get_performance_state(struct generic_pm_domain *genpd,
+					      struct device *dev)
+{
+	struct opp_table *hw_opp_table, *clk_opp_table;
+	struct dev_pm_opp *opp;
+	u32 hw_version;
+	int ret;
+
+	/*
+	 * Tegra114+ SocS don't support OPP yet.  But if they will get OPP
+	 * support, then we want to skip OPP for older kernels to preserve
+	 * compatibility of newer DTBs with older kernels.
+	 */
+	if (!pmc->soc->supports_core_domain)
+		return 0;
+
+	/*
+	 * The EMC devices are a special case because we have a protection
+	 * from non-EMC drivers getting clock handle before EMC driver is
+	 * fully initialized.  The goal of the protection is to prevent
+	 * devfreq driver from getting failures if it will try to change
+	 * EMC clock rate until clock is fully initialized.  The EMC drivers
+	 * will initialize the performance state by themselves.
+	 *
+	 * Display controller also is a special case because only controller
+	 * driver could get the clock rate based on configuration of internal
+	 * divider.
+	 *
+	 * Clock driver uses its own state syncing.
+	 */
+	if (of_device_compatible_match(dev->of_node, tegra_pd_no_perf_compats))
+		return 0;
+
+	if (of_machine_is_compatible("nvidia,tegra20"))
+		hw_version = BIT(tegra_sku_info.soc_process_id);
+	else
+		hw_version = BIT(tegra_sku_info.soc_speedo_id);
+
+	hw_opp_table = dev_pm_opp_set_supported_hw(dev, &hw_version, 1);
+	if (IS_ERR(hw_opp_table)){
+		dev_err(dev, "failed to set OPP supported HW: %pe\n",
+			hw_opp_table);
+		return PTR_ERR(hw_opp_table);
+	}
+
+	clk_opp_table = dev_pm_opp_set_clkname(dev, NULL);
+	if (IS_ERR(clk_opp_table)){
+		dev_err(dev, "failed to set OPP clk: %pe\n", clk_opp_table);
+		ret = PTR_ERR(clk_opp_table);
+		goto put_hw;
+	}
+
+	ret = devm_pm_opp_of_add_table(dev);
+	if (ret) {
+		dev_err(dev, "failed to add OPP table: %d\n", ret);
+		goto put_clk;
+	}
+
+	opp = dev_pm_opp_from_clk_rate(dev);
+	if (IS_ERR(opp)) {
+		dev_err(&genpd->dev, "failed to get current OPP for %s: %pe\n",
+			dev_name(dev), opp);
+		ret = PTR_ERR(opp);
+	} else {
+		ret = dev_pm_opp_get_required_pstate(opp, 0);
+		dev_pm_opp_put(opp);
+	}
+
+	dev_pm_opp_of_remove_table(dev);
+put_clk:
+	dev_pm_opp_put_clkname(clk_opp_table);
+put_hw:
+	dev_pm_opp_put_supported_hw(hw_opp_table);
+
+	return ret;
+}
+
 /*
  * TODO Figure out a way to call this with the struct tegra_pmc * passed in.
  * This currently doesn't work because readx_poll_timeout() can only operate
@@ -1238,6 +1328,7 @@ static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np)
 
 	pg->id = id;
 	pg->genpd.name = np->name;
+	pg->genpd.get_performance_state = tegra_pmc_pd_get_performance_state;
 	pg->genpd.power_off = tegra_genpd_power_off;
 	pg->genpd.power_on = tegra_genpd_power_on;
 	pg->pmc = pmc;
@@ -1354,6 +1445,7 @@ static int tegra_pmc_core_pd_add(struct tegra_pmc *pmc, struct device_node *np)
 		return -ENOMEM;
 
 	genpd->name = "core";
+	genpd->get_performance_state = tegra_pmc_pd_get_performance_state;
 	genpd->set_performance_state = tegra_pmc_core_pd_set_performance_state;
 	genpd->opp_to_performance_state = tegra_pmc_core_pd_opp_to_performance_state;
 
diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
index 67017c9390c8..abe33be9828f 100644
--- a/include/linux/pm_domain.h
+++ b/include/linux/pm_domain.h
@@ -133,6 +133,8 @@ struct generic_pm_domain {
 						 struct dev_pm_opp *opp);
 	int (*set_performance_state)(struct generic_pm_domain *genpd,
 				     unsigned int state);
+	int (*get_performance_state)(struct generic_pm_domain *genpd,
+				     struct device *dev);
 	struct gpd_dev_ops dev_ops;
 	s64 max_off_time_ns;	/* Maximum allowed "suspended" time. */
 	ktime_t next_wakeup;	/* Maintained by the domain governor */
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 686122b59935..e7fd0dd493ca 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -169,6 +169,7 @@ void dev_pm_opp_remove_table(struct device *dev);
 void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
 int dev_pm_opp_sync_regulators(struct device *dev);
 int dev_pm_opp_sync(struct device *dev);
+struct dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev);
 #else
 static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
 {
@@ -440,6 +441,11 @@ static inline int dev_pm_opp_sync(struct device *dev)
 	return -EOPNOTSUPP;
 }
 
+static struct inline dev_pm_opp *dev_pm_opp_from_clk_rate(struct device *dev)
+{
+	return ERR_PTR(-EOPNOTSUPP);
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)

Viresh Kumar Aug. 26, 2021, 2:54 a.m. UTC | #40

On 25-08-21, 18:41, Dmitry Osipenko wrote:
> Thinking a bit more about this, I got a nicer variant which actually works in all cases for Tegra.
> 
> Viresh / Ulf, what do you think about this:

This is what I have been suggesting from day 1 :)

https://lore.kernel.org/linux-staging/20210818055849.ybfajzu75ecpdrbn@vireshk-i7/

 "
  And if it is all about just syncing the genpd core, then can the
  genpd core do something like what clk framework does? i.e. allow a
  new optional genpd callback, get_performance_state() (just like
  set_performance_state()), which can be called initially by the core
  to get the performance to something other than zero.
 "

Looks good to me :)

Viresh Kumar Aug. 26, 2021, 2:55 a.m. UTC | #41

On 26-08-21, 08:24, Viresh Kumar wrote:
> On 25-08-21, 18:41, Dmitry Osipenko wrote:
> > Thinking a bit more about this, I got a nicer variant which actually works in all cases for Tegra.
> > 
> > Viresh / Ulf, what do you think about this:
> 
> This is what I have been suggesting from day 1 :)
> 
> https://lore.kernel.org/linux-staging/20210818055849.ybfajzu75ecpdrbn@vireshk-i7/
> 
>  "
>   And if it is all about just syncing the genpd core, then can the
>   genpd core do something like what clk framework does? i.e. allow a
>   new optional genpd callback, get_performance_state() (just like
>   set_performance_state()), which can be called initially by the core
>   to get the performance to something other than zero.
>  "
> 
> Looks good to me :)

When you refresh this stuff, please send only 3-4 patches to update
the core stuff and show an example. Once we finalize with the
interface, you can update all the users. Else this is just noise for
everyone else.

Patch

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 5543c54dacc5..18016e49605f 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -939,7 +939,8 @@  static int _set_required_opps(struct device *dev,
 	return ret;
 }
 
-static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
+static struct dev_pm_opp *
+_find_current_opp(struct device *dev, struct opp_table *opp_table)
 {
 	struct dev_pm_opp *opp = ERR_PTR(-ENODEV);
 	unsigned long freq;
@@ -961,7 +962,7 @@  static void _find_current_opp(struct device *dev, struct opp_table *opp_table)
 		mutex_unlock(&opp_table->lock);
 	}
 
-	opp_table->current_opp = opp;
+	return opp;
 }
 
 static int _disable_opp_table(struct device *dev, struct opp_table *opp_table)
@@ -1003,7 +1004,7 @@  static int _set_opp(struct device *dev, struct opp_table *opp_table,
 
 	/* Find the currently set OPP if we don't know already */
 	if (unlikely(!opp_table->current_opp))
-		_find_current_opp(dev, opp_table);
+		opp_table->current_opp = _find_current_opp(dev, opp_table);
 
 	old_opp = opp_table->current_opp;
 
@@ -2931,3 +2932,38 @@  int dev_pm_opp_sync_regulators(struct device *dev)
 	return ret;
 }
 EXPORT_SYMBOL_GPL(dev_pm_opp_sync_regulators);
+
+/**
+ * dev_pm_opp_sync() - Sync OPP state
+ * @dev:	device for which we do this operation
+ *
+ * Initialize OPP table accordingly to current clock rate or
+ * first available OPP if clock not available for this device.
+ *
+ * Return: 0 on success or a negative error value.
+ */
+int dev_pm_opp_sync(struct device *dev)
+{
+	struct opp_table *opp_table;
+	struct dev_pm_opp *opp;
+	int ret = 0;
+
+	/* Device may not have OPP table */
+	opp_table = _find_opp_table(dev);
+	if (IS_ERR(opp_table))
+		return 0;
+
+	if (!_get_opp_count(opp_table))
+		goto put_table;
+
+	opp = _find_current_opp(dev, opp_table);
+	ret = _set_opp(dev, opp_table, opp, opp->rate);
+	dev_pm_opp_put(opp);
+
+put_table:
+	/* Drop reference taken by _find_opp_table() */
+	dev_pm_opp_put_opp_table(opp_table);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(dev_pm_opp_sync);
diff --git a/include/linux/pm_opp.h b/include/linux/pm_opp.h
index 84150a22fd7c..686122b59935 100644
--- a/include/linux/pm_opp.h
+++ b/include/linux/pm_opp.h
@@ -168,6 +168,7 @@  int dev_pm_opp_get_sharing_cpus(struct device *cpu_dev, struct cpumask *cpumask)
 void dev_pm_opp_remove_table(struct device *dev);
 void dev_pm_opp_cpumask_remove_table(const struct cpumask *cpumask);
 int dev_pm_opp_sync_regulators(struct device *dev);
+int dev_pm_opp_sync(struct device *dev);
 #else
 static inline struct opp_table *dev_pm_opp_get_opp_table(struct device *dev)
 {
@@ -434,6 +435,11 @@  static inline int dev_pm_opp_sync_regulators(struct device *dev)
 	return -EOPNOTSUPP;
 }
 
+static inline int dev_pm_opp_sync(struct device *dev)
+{
+	return -EOPNOTSUPP;
+}
+
 #endif		/* CONFIG_PM_OPP */
 
 #if defined(CONFIG_PM_OPP) && defined(CONFIG_OF)