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 digetx@gmail.com

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.

19.08.2021 16:07, Ulf Hansson пишет:

On Wed, 18 Aug 2021 at 17:43, Dmitry Osipenko digetx@gmail.com wrote:

...

18.08.2021 13:08, Ulf Hansson пишет:

...

On Wed, 18 Aug 2021 at 11:50, Viresh Kumar viresh.kumar@linaro.org wrote:

...

On 18-08-21, 11:41, Ulf Hansson wrote:

...

On Wed, 18 Aug 2021 at 11:14, Viresh Kumar viresh.kumar@linaro.org 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;

On Thu, 19 Aug 2021 at 21:35, Dmitry Osipenko digetx@gmail.com wrote:

19.08.2021 16:07, Ulf Hansson пишет:

...

On Wed, 18 Aug 2021 at 17:43, Dmitry Osipenko digetx@gmail.com wrote:

...

18.08.2021 13:08, Ulf Hansson пишет:

...

On Wed, 18 Aug 2021 at 11:50, Viresh Kumar viresh.kumar@linaro.org wrote:

...

On 18-08-21, 11:41, Ulf Hansson wrote:

...

On Wed, 18 Aug 2021 at 11:14, Viresh Kumar viresh.kumar@linaro.org 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

[...]

...

...

...

...

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

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)

On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote:

Document tegra-clocks sub-node which describes Tegra SoC clocks that require a higher voltage of the core power domain in order to operate properly on a higher clock rates. Each node contains a phandle to OPP table and power domain.

The root PLLs and system clocks don't have any specific device dedicated to them, clock controller is in charge of managing power for them.

Signed-off-by: Dmitry Osipenko digetx@gmail.com

.../bindings/clock/nvidia,tegra20-car.yaml | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml index 459d2a525393..7f5cd27e4ce0 100644 --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml @@ -42,6 +42,48 @@ properties: "#reset-cells": const: 1

• tegra-clocks:
• description: child nodes are the output clocks from the CAR
• type: object
• patternProperties:

•  "^[a-z]+[0-9]+$":
  

•    type: object
  

•    properties:
  

•      compatible:
  

•        allOf:
  

•          - items:
  

•              - enum:
  

•                  - nvidia,tegra20-sclk
  

•                  - nvidia,tegra30-sclk
  

•                  - nvidia,tegra30-pllc
  

•                  - nvidia,tegra30-plle
  

•                  - nvidia,tegra30-pllm
  

•          - const: nvidia,tegra-clock
  

You are saying the first string must be both one of the enums and 'nvidia,tegra-clock'. You don't get an error because your pattern doesn't match 'sclk'.

•      operating-points-v2:
  

•        $ref: /schemas/types.yaml#/definitions/phandle
  

•        description:
  

•          Phandle to OPP table that contains frequencies, voltages and
  

•          opp-supported-hw property, which is a bitfield indicating
  

•          SoC process or speedo ID mask.
  

Just 'operating-points-v2: true' is enough.

•      clocks:
  

•        items:
  

•          - description: node's clock
  

•      power-domains:
  

•        maxItems: 1
  

•        description: phandle to the core SoC power domain
  

•    required:
  

•      - compatible
  

•      - operating-points-v2
  

•      - clocks
  

•      - power-domains
  

•    additionalProperties: false
  

required:

• compatible
• reg

@@ -59,6 +101,15 @@ examples: reg = <0x60006000 0x1000>; #clock-cells = <1>; #reset-cells = <1>;

•    tegra-clocks {
  

•        sclk {
  

•            compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock";
  

•            operating-points-v2 = <&opp_table>;
  

•            clocks = <&tegra_car TEGRA20_CLK_SCLK>;
  

•            power-domains = <&domain>;
  

•        };
  

•    };
  

  };

  usb-controller@c5004000 {

-- 2.32.0

On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote:

Document tegra-clocks sub-node which describes Tegra SoC clocks that require a higher voltage of the core power domain in order to operate properly on a higher clock rates. Each node contains a phandle to OPP table and power domain.

The root PLLs and system clocks don't have any specific device dedicated to them, clock controller is in charge of managing power for them.

Signed-off-by: Dmitry Osipenko digetx@gmail.com

.../bindings/clock/nvidia,tegra20-car.yaml | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml index 459d2a525393..7f5cd27e4ce0 100644 --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml @@ -42,6 +42,48 @@ properties: "#reset-cells": const: 1

• tegra-clocks:
• description: child nodes are the output clocks from the CAR
• type: object
• patternProperties:

•  "^[a-z]+[0-9]+$":
  

•    type: object
  

•    properties:
  

•      compatible:
  

•        allOf:
  

•          - items:
  

•              - enum:
  

•                  - nvidia,tegra20-sclk
  

•                  - nvidia,tegra30-sclk
  

•                  - nvidia,tegra30-pllc
  

•                  - nvidia,tegra30-plle
  

•                  - nvidia,tegra30-pllm
  

•          - const: nvidia,tegra-clock
  

•      operating-points-v2:
  

•        $ref: /schemas/types.yaml#/definitions/phandle
  

•        description:
  

•          Phandle to OPP table that contains frequencies, voltages and
  

•          opp-supported-hw property, which is a bitfield indicating
  

•          SoC process or speedo ID mask.
  

•      clocks:
  

•        items:
  

•          - description: node's clock
  

•      power-domains:
  

•        maxItems: 1
  

•        description: phandle to the core SoC power domain
  

•    required:
  

•      - compatible
  

•      - operating-points-v2
  

•      - clocks
  

•      - power-domains
  

•    additionalProperties: false
  

required:

• compatible
• reg

@@ -59,6 +101,15 @@ examples: reg = <0x60006000 0x1000>; #clock-cells = <1>; #reset-cells = <1>;

•    tegra-clocks {
  

•        sclk {
  

•            compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock";
  

•            operating-points-v2 = <&opp_table>;
  

•            clocks = <&tegra_car TEGRA20_CLK_SCLK>;
  

•            power-domains = <&domain>;
  

•        };
  

•    };
  

I wonder if it'd be better to match on the name of the node rather than add an artificial compatible string. We usually use the compatible string to match a device, but here you're really trying to add information about a resource provided by the CAR controller.

We do similar things for example in PMIC bindings where the individual regulators are represented in the device tree via nodes named after the regulator.

You could then also leave out the clocks property, which is weird as it is because it's basically a self-reference. But you don't really need the reference here in the first place because the CAR is already the parent of SCLK.

Also, I don't think the tegra- prefix is necessary here. The parent node is already identified as Tegra via the compatible string.

In the case of CAR, I'd imagine something like:

clocks { sclk { operating-points-v2 = <&opp_table>; power-domains = <&domain>; }; };

Now you've only got the bare minimum in here that you actually add. All the other data that you used to have is simply derived from the parent.

Thierry

 };

 usb-controller@c5004000 {

-- 2.32.0

On Wed, Aug 18, 2021 at 06:05:11PM +0300, Dmitry Osipenko wrote:

18.08.2021 16:59, Thierry Reding пишет:

...

On Tue, Aug 17, 2021 at 04:27:26AM +0300, Dmitry Osipenko wrote:

...

Document tegra-clocks sub-node which describes Tegra SoC clocks that require a higher voltage of the core power domain in order to operate properly on a higher clock rates. Each node contains a phandle to OPP table and power domain.

The root PLLs and system clocks don't have any specific device dedicated to them, clock controller is in charge of managing power for them.

Signed-off-by: Dmitry Osipenko digetx@gmail.com

.../bindings/clock/nvidia,tegra20-car.yaml | 51 +++++++++++++++++++ 1 file changed, 51 insertions(+)

diff --git a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml index 459d2a525393..7f5cd27e4ce0 100644 --- a/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml +++ b/Documentation/devicetree/bindings/clock/nvidia,tegra20-car.yaml @@ -42,6 +42,48 @@ properties: "#reset-cells": const: 1

• tegra-clocks:
• description: child nodes are the output clocks from the CAR
• type: object
• patternProperties:

•  "^[a-z]+[0-9]+$":
  

•    type: object
  

•    properties:
  

•      compatible:
  

•        allOf:
  

•          - items:
  

•              - enum:
  

•                  - nvidia,tegra20-sclk
  

•                  - nvidia,tegra30-sclk
  

•                  - nvidia,tegra30-pllc
  

•                  - nvidia,tegra30-plle
  

•                  - nvidia,tegra30-pllm
  

•          - const: nvidia,tegra-clock
  

•      operating-points-v2:
  

•        $ref: /schemas/types.yaml#/definitions/phandle
  

•        description:
  

•          Phandle to OPP table that contains frequencies, voltages and
  

•          opp-supported-hw property, which is a bitfield indicating
  

•          SoC process or speedo ID mask.
  

•      clocks:
  

•        items:
  

•          - description: node's clock
  

•      power-domains:
  

•        maxItems: 1
  

•        description: phandle to the core SoC power domain
  

•    required:
  

•      - compatible
  

•      - operating-points-v2
  

•      - clocks
  

•      - power-domains
  

•    additionalProperties: false
  

required:

• compatible
• reg

@@ -59,6 +101,15 @@ examples: reg = <0x60006000 0x1000>; #clock-cells = <1>; #reset-cells = <1>;

•    tegra-clocks {
  

•        sclk {
  

•            compatible = "nvidia,tegra20-sclk", "nvidia,tegra-clock";
  

•            operating-points-v2 = <&opp_table>;
  

•            clocks = <&tegra_car TEGRA20_CLK_SCLK>;
  

•            power-domains = <&domain>;
  

•        };
  

•    };
  

I wonder if it'd be better to match on the name of the node rather than add an artificial compatible string. We usually use the compatible string to match a device, but here you're really trying to add information about a resource provided by the CAR controller.

We do similar things for example in PMIC bindings where the individual regulators are represented in the device tree via nodes named after the regulator.

You could then also leave out the clocks property, which is weird as it is because it's basically a self-reference. But you don't really need the reference here in the first place because the CAR is already the parent of SCLK.

We don't have a platform device for CaR. I don't see how it's going to work. We need to create a platform device for each RPM-capable clock because that's how RPM works. The compatible string is required for instantiating OF-devices from a node, otherwise we will have to re-invent the OF core.

I think we do have a platform device for CAR. It's just not bound against by the driver because these clock drivers are "special". But from other parts of the series you're already trying to fix that, at least partially.

But it doesn't seem right to create a platform device for each RPM- capable clock. Why do they need to be devices? They aren't, so why pretend? Is it that some API that we want to use here requires the struct device?

...

Also, I don't think the tegra- prefix is necessary here. The parent node is already identified as Tegra via the compatible string.

In the case of CAR, I'd imagine something like:

clocks { sclk { operating-points-v2 = <&opp_table>; power-domains = <&domain>; }; };

Now you've only got the bare minimum in here that you actually add. All the other data that you used to have is simply derived from the parent.

'clocks' is already a generic keyword in DT. It's probably not okay to redefine it.

"clocks" is not a generic keyword. It's the name of a property and given that we're talking about the clock provider here, it doesn't need a clocks property of its own, so it should be fine to use that for the node.

Thierry

On Tue, Aug 17, 2021 at 04:27:29AM +0300, Dmitry Osipenko wrote:

Memory Client should be blocked before hardware reset is asserted in order to prevent memory corruption and hanging of memory controller.

Document Memory Client resets of Host1x, GR2D and GR3D hardware units.

Signed-off-by: Dmitry Osipenko digetx@gmail.com

.../bindings/display/tegra/nvidia,tegra20-host1x.txt | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt index 62861a8fb5c6..07a08653798b 100644 --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt @@ -18,6 +18,7 @@ Required properties:

• resets: Must contain an entry for each entry in reset-names. See ../reset/reset.txt for details.
• reset-names: Must include the following entries:

• • mc
  • host1x

New entries should be at the end. Order matters.

Optional properties: @@ -197,6 +198,7 @@ of the following host1x client modules:

• resets: Must contain an entry for each entry in reset-names. See ../reset/reset.txt for details.
• reset-names: Must include the following entries:

• • mc
  • 2d

  Optional properties:

@@ -222,6 +224,8 @@ of the following host1x client modules:

• resets: Must contain an entry for each entry in reset-names. See ../reset/reset.txt for details.
• reset-names: Must include the following entries:

• • mc
• • mc2 (Only required on SoCs with two 3D clocks)
  • 3d
  • 3d2 (Only required on SoCs with two 3D clocks)

-- 2.32.0

18.08.2021 04:37, Dmitry Osipenko пишет:

18.08.2021 04:16, Rob Herring пишет:

...

On Tue, Aug 17, 2021 at 04:27:29AM +0300, Dmitry Osipenko wrote:

...

Memory Client should be blocked before hardware reset is asserted in order to prevent memory corruption and hanging of memory controller.

Document Memory Client resets of Host1x, GR2D and GR3D hardware units.

Signed-off-by: Dmitry Osipenko digetx@gmail.com

.../bindings/display/tegra/nvidia,tegra20-host1x.txt | 4 ++++ 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt index 62861a8fb5c6..07a08653798b 100644 --- a/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt +++ b/Documentation/devicetree/bindings/display/tegra/nvidia,tegra20-host1x.txt @@ -18,6 +18,7 @@ Required properties:

• resets: Must contain an entry for each entry in reset-names. See ../reset/reset.txt for details.
• reset-names: Must include the following entries:

• • mc
  • host1x

New entries should be at the end. Order matters.

Indeed, order matters. In this case it matters by the hardware because memory reset must be asserted before the controller's reset. We rely on it in the code of the GENPD driver. Hence it's the intended order in this patch.

Although, my bad. It should be to reorder items here, it's not a GENPD binding.

On Tue, 17 Aug 2021 at 03:30, Dmitry Osipenko digetx@gmail.com wrote:

Add runtime PM and OPP support to the Host1x driver. It's required for enabling system-wide DVFS and supporting dynamic power management using a generic power domain. For the starter we will keep host1x always-on because dynamic power management require a major refactoring of the driver code since lot's of code paths will need the RPM handling and we're going to remove some of these paths in the future. Host1x doesn't consume much power so it is good enough, we at least need to resume Host1x in order to initialize the power state.

Tested-by: Peter Geis pgwipeout@gmail.com # Ouya T30 Tested-by: Paul Fertser fercerpav@gmail.com # PAZ00 T20 Tested-by: Nicolas Chauvet kwizart@gmail.com # PAZ00 T20 and TK1 T124 Tested-by: Matt Merhar mattmerhar@protonmail.com # Ouya T30 Signed-off-by: Dmitry Osipenko digetx@gmail.com

---

[...]

static int host1x_probe(struct platform_device *pdev) { struct host1x *host; @@ -394,6 +423,10 @@ static int host1x_probe(struct platform_device *pdev) /* set common host1x device data */ platform_set_drvdata(pdev, host);

•   err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
  

•   if (err)
  

•           return err;
  

•   host->regs = devm_ioremap_resource(&pdev->dev, regs);
    if (IS_ERR(host->regs))
            return PTR_ERR(host->regs);
  

@@ -423,12 +456,9 @@ static int host1x_probe(struct platform_device *pdev) return err; }

•   host->rst = devm_reset_control_get(&pdev->dev, "host1x");
  

•   if (IS_ERR(host->rst)) {
  

•           err = PTR_ERR(host->rst);
  

•           dev_err(&pdev->dev, "failed to get reset: %d\n", err);
  

•   err = host1x_get_resets(host);
  

•   if (err)
            return err;
  

•   }
  
    err = host1x_iommu_init(host);
    if (err < 0) {
  

@@ -443,22 +473,10 @@ static int host1x_probe(struct platform_device *pdev) goto iommu_exit; }

•   err = clk_prepare_enable(host->clk);
  

•   if (err < 0) {
  

•           dev_err(&pdev->dev, "failed to enable clock\n");
  

•           goto free_channels;
  

•   }
  

•   err = reset_control_deassert(host->rst);
  

•   if (err < 0) {
  

•           dev_err(&pdev->dev, "failed to deassert reset: %d\n", err);
  

•           goto unprepare_disable;
  

•   }
  

Removing the clk_prepare_enable() and reset_control_deassert() from host1x_probe(), might not be a good idea. See more about why, below.

    err = host1x_syncpt_init(host);
    if (err) {
            dev_err(&pdev->dev, "failed to initialize syncpts\n");

•           goto reset_assert;
  

•           goto free_channels;
    }
  
    err = host1x_intr_init(host, syncpt_irq);
  

@@ -467,10 +485,14 @@ static int host1x_probe(struct platform_device *pdev) goto deinit_syncpt; }

•   host1x_debug_init(host);
  

•   pm_runtime_enable(&pdev->dev);
  
  

•   if (host->info->has_hypervisor)
  

•           host1x_setup_sid_table(host);
  

•   /* the driver's code isn't ready yet for the dynamic RPM */
  

•   err = pm_runtime_resume_and_get(&pdev->dev);
  

If the driver is being built with the CONFIG_PM Kconfig option being unset, pm_runtime_resume_and_get() will return 0 to indicate success - and without calling the ->runtime_resume() callback. In other words, the clock will remain gated and the reset will not be deasserted, likely causing the driver to be malfunctioning.

If the driver isn't ever being built with CONFIG_PM unset, feel free to ignore my above comments.

Otherwise, if it needs to work both with and without CONFIG_PM being set, you may use the following pattern in host1x_probe() to deploy runtime PM support:

"Enable the needed resources to probe the device" pm_runtime_get_noresume() pm_runtime_set_active() pm_runtime_enable()

"Before successfully completing probe" pm_runtime_put()

•   if (err)
  

•           goto deinit_intr;
  

•   host1x_debug_init(host);
  
    err = host1x_register(host);
    if (err < 0)
  

@@ -486,13 +508,13 @@ static int host1x_probe(struct platform_device *pdev) host1x_unregister(host); deinit_debugfs: host1x_debug_deinit(host);

•   pm_runtime_put(&pdev->dev);
  

•   pm_runtime_disable(&pdev->dev);
  

+deinit_intr: host1x_intr_deinit(host); deinit_syncpt: host1x_syncpt_deinit(host); -reset_assert:

•   reset_control_assert(host->rst);
  

-unprepare_disable:

•   clk_disable_unprepare(host->clk);
  

free_channels: host1x_channel_list_free(&host->channel_list); iommu_exit:

[...]

Kind regards Uffe

On Tue, 17 Aug 2021 at 16:03, Thierry Reding thierry.reding@gmail.com wrote:

On Tue, Aug 17, 2021 at 02:04:38PM +0200, Ulf Hansson wrote:

...

On Tue, 17 Aug 2021 at 03:30, Dmitry Osipenko digetx@gmail.com wrote:

...

Add runtime PM and OPP support to the Host1x driver. It's required for enabling system-wide DVFS and supporting dynamic power management using a generic power domain. For the starter we will keep host1x always-on because dynamic power management require a major refactoring of the driver code since lot's of code paths will need the RPM handling and we're going to remove some of these paths in the future. Host1x doesn't consume much power so it is good enough, we at least need to resume Host1x in order to initialize the power state.

Tested-by: Peter Geis pgwipeout@gmail.com # Ouya T30 Tested-by: Paul Fertser fercerpav@gmail.com # PAZ00 T20 Tested-by: Nicolas Chauvet kwizart@gmail.com # PAZ00 T20 and TK1 T124 Tested-by: Matt Merhar mattmerhar@protonmail.com # Ouya T30 Signed-off-by: Dmitry Osipenko digetx@gmail.com

---

[...]

...

static int host1x_probe(struct platform_device *pdev) { struct host1x *host; @@ -394,6 +423,10 @@ static int host1x_probe(struct platform_device *pdev) /* set common host1x device data */ platform_set_drvdata(pdev, host);

•   err = devm_tegra_core_dev_init_opp_table_simple(&pdev->dev);
  

•   if (err)
  

•           return err;
  

•   host->regs = devm_ioremap_resource(&pdev->dev, regs);
    if (IS_ERR(host->regs))
            return PTR_ERR(host->regs);
  

@@ -423,12 +456,9 @@ static int host1x_probe(struct platform_device *pdev) return err; }

•   host->rst = devm_reset_control_get(&pdev->dev, "host1x");
  

•   if (IS_ERR(host->rst)) {
  

•           err = PTR_ERR(host->rst);
  

•           dev_err(&pdev->dev, "failed to get reset: %d\n", err);
  

•   err = host1x_get_resets(host);
  

•   if (err)
            return err;
  

•   }
  
    err = host1x_iommu_init(host);
    if (err < 0) {
  

@@ -443,22 +473,10 @@ static int host1x_probe(struct platform_device *pdev) goto iommu_exit; }

•   err = clk_prepare_enable(host->clk);
  

•   if (err < 0) {
  

•           dev_err(&pdev->dev, "failed to enable clock\n");
  

•           goto free_channels;
  

•   }
  

•   err = reset_control_deassert(host->rst);
  

•   if (err < 0) {
  

•           dev_err(&pdev->dev, "failed to deassert reset: %d\n", err);
  

•           goto unprepare_disable;
  

•   }
  

Removing the clk_prepare_enable() and reset_control_deassert() from host1x_probe(), might not be a good idea. See more about why, below.

...

    err = host1x_syncpt_init(host);
    if (err) {
            dev_err(&pdev->dev, "failed to initialize syncpts\n");

•           goto reset_assert;
  

•           goto free_channels;
    }
  
    err = host1x_intr_init(host, syncpt_irq);
  

@@ -467,10 +485,14 @@ static int host1x_probe(struct platform_device *pdev) goto deinit_syncpt; }

•   host1x_debug_init(host);
  

•   pm_runtime_enable(&pdev->dev);
  
  

•   if (host->info->has_hypervisor)
  

•           host1x_setup_sid_table(host);
  

•   /* the driver's code isn't ready yet for the dynamic RPM */
  

•   err = pm_runtime_resume_and_get(&pdev->dev);
  

If the driver is being built with the CONFIG_PM Kconfig option being unset, pm_runtime_resume_and_get() will return 0 to indicate success - and without calling the ->runtime_resume() callback. In other words, the clock will remain gated and the reset will not be deasserted, likely causing the driver to be malfunctioning.

If the driver isn't ever being built with CONFIG_PM unset, feel free to ignore my above comments.

Otherwise, if it needs to work both with and without CONFIG_PM being set, you may use the following pattern in host1x_probe() to deploy runtime PM support:

"Enable the needed resources to probe the device" pm_runtime_get_noresume() pm_runtime_set_active() pm_runtime_enable()

"Before successfully completing probe" pm_runtime_put()

We made a conscious decision a few years ago to have ARCH_TEGRA select PM on both 32-bit and 64-bit ARM, specifically to avoid the need to do this dance (though there are still a few drivers left that do this, I think).

So I think this should be unnecessary. Unless perhaps if the sysfs PM controls have any influence on this. As far as I know, as long as the PM kconfig option is enabled, the sysfs control only influence the runtime behaviour (i.e. setting the sysfs PM control to "on" is going to force runtime PM to be resumed) but there's no way to disable runtime PM altogether via sysfs that would make the above necessary.

Thanks for clarifying! As I said, feel free to ignore my comments then.

For this and the other patches in the series, I assume you only need to care about whether the driver is a cross SoC driver and used on other platforms than Tegra then.

Thierry

Kind regards Uffe

On Tue, Aug 17, 2021 at 04:27:40AM +0300, Dmitry Osipenko wrote:

The SDHCI on Tegra belongs to the core power domain and we're going to enable GENPD support for the core domain. Now SDHCI must be resumed using runtime PM API in order to initialize the SDHCI power state. The SDHCI clock rate must be changed using OPP API that will reconfigure the power domain performance state in accordance to the rate. Add runtime PM and OPP support to the SDHCI driver.

Signed-off-by: Dmitry Osipenko digetx@gmail.com

drivers/mmc/host/sdhci-tegra.c | 146 ++++++++++++++++++++++++--------- 1 file changed, 105 insertions(+), 41 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 387ce9cdbd7c..a3583359c972 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -15,6 +15,8 @@ #include <linux/of.h> #include <linux/of_device.h> #include <linux/pinctrl/consumer.h> +#include <linux/pm_opp.h> +#include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> #include <linux/reset.h> #include <linux/mmc/card.h> @@ -24,6 +26,8 @@ #include <linux/gpio/consumer.h> #include <linux/ktime.h>

+#include <soc/tegra/common.h>

#include "sdhci-pltfm.h" #include "cqhci.h"

@@ -123,6 +127,12 @@ SDHCI_TRNS_BLK_CNT_EN | \ SDHCI_TRNS_DMA)

+enum {

• TEGRA_CLK_BULK_SDHCI,
• TEGRA_CLK_BULK_TMCLK,
• TEGRA_CLK_BULK_NUM,

+};

struct sdhci_tegra_soc_data { const struct sdhci_pltfm_data *pdata; u64 dma_mask; @@ -171,6 +181,8 @@ struct sdhci_tegra { bool enable_hwcq; unsigned long curr_clk_rate; u8 tuned_tap_delay;

• struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM];

This doesn't seem worth it to me. There's a lot of churn in this driver that's only needed to convert this to the clk_bulk API and it makes the code a lot more difficult to read, in my opinion.

It looks like the only benefit that this gives us is that runtime suspend and resume become a few lines shorter.

Thierry

On Fri, Aug 20, 2021 at 01:37:13AM +0300, Dmitry Osipenko wrote:

19.08.2021 20:03, Thierry Reding пишет:

...

On Tue, Aug 17, 2021 at 04:27:40AM +0300, Dmitry Osipenko wrote:

...

The SDHCI on Tegra belongs to the core power domain and we're going to enable GENPD support for the core domain. Now SDHCI must be resumed using runtime PM API in order to initialize the SDHCI power state. The SDHCI clock rate must be changed using OPP API that will reconfigure the power domain performance state in accordance to the rate. Add runtime PM and OPP support to the SDHCI driver.

Signed-off-by: Dmitry Osipenko digetx@gmail.com

drivers/mmc/host/sdhci-tegra.c | 146 ++++++++++++++++++++++++--------- 1 file changed, 105 insertions(+), 41 deletions(-)

diff --git a/drivers/mmc/host/sdhci-tegra.c b/drivers/mmc/host/sdhci-tegra.c index 387ce9cdbd7c..a3583359c972 100644 --- a/drivers/mmc/host/sdhci-tegra.c +++ b/drivers/mmc/host/sdhci-tegra.c @@ -15,6 +15,8 @@ #include <linux/of.h> #include <linux/of_device.h> #include <linux/pinctrl/consumer.h> +#include <linux/pm_opp.h> +#include <linux/pm_runtime.h> #include <linux/regulator/consumer.h> #include <linux/reset.h> #include <linux/mmc/card.h> @@ -24,6 +26,8 @@ #include <linux/gpio/consumer.h> #include <linux/ktime.h>

+#include <soc/tegra/common.h>

#include "sdhci-pltfm.h" #include "cqhci.h"

@@ -123,6 +127,12 @@ SDHCI_TRNS_BLK_CNT_EN | \ SDHCI_TRNS_DMA)

+enum {

• TEGRA_CLK_BULK_SDHCI,
• TEGRA_CLK_BULK_TMCLK,
• TEGRA_CLK_BULK_NUM,

+};

struct sdhci_tegra_soc_data { const struct sdhci_pltfm_data *pdata; u64 dma_mask; @@ -171,6 +181,8 @@ struct sdhci_tegra { bool enable_hwcq; unsigned long curr_clk_rate; u8 tuned_tap_delay;

• struct clk_bulk_data clocks[TEGRA_CLK_BULK_NUM];

This doesn't seem worth it to me. There's a lot of churn in this driver that's only needed to convert this to the clk_bulk API and it makes the code a lot more difficult to read, in my opinion.

It looks like the only benefit that this gives us is that runtime suspend and resume become a few lines shorter.

The driver probe code looks cleaner with that. You should be looking at the final result and not at the patch to see it.

I did look at the final result and didn't find it cleaner at all. =)

Thierry