Re: [PATCH V5 11/14] soc: tegra: pmc: Add generic PM domain support

[...]

quoted

quoted

 /**
  * tegra_powergate_power_on() - power on partition
  * @id: partition ID

@@ -319,35 +512,20 @@ EXPORT_SYMBOL(tegra_powergate_remove_clamping);
 int tegra_powergate_sequence_power_up(unsigned int id, struct clk *clk,
                                      struct reset_control *rst)

There seems to be two viable ways for a driver to control tegra powergates.

1)
$Subject patch enables the use of runtime PM.

2)
The current tegra_powergate_sequence_power_up() and
tegra_powergate_power_off() API.

It seems fragile to allow both options, but perhaps your are
protecting this with some lock to prevent concurrent accesses?

There is a lock protecting accesses to the PMC registers which
ultimately control the power domain. However, may be it would be better
to ensure that any power-domain registered with genpd cannot be
controlled by the legacy APIs. I have added a bitmap to mark valid
power-domains to ensure that only valid power domains can be controlled
by these legacy APIs. I could mark the power-domain invalid after
registering with genpd to ensure that it cannot be accessed by the
legacy APIs.

That seems like a good way of making it more robust!

quoted

Also, I assume you need the two options in a transition phase, before
you have deployed runtime PM for these drivers?

Right and some of the legacy APIs are entrenched in some drivers. So to
keep the patch set manageable it seems best to get some support in place
then start migrating the drivers.

Thanks for elaborating on this! I get and like the idea of moving forward!

[...]

quoted

quoted

+
+static void tegra_powergate_remove(struct tegra_pmc *pmc)
+{
+       struct tegra_powergate *pg, *n;
+
+       list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) {

The tegra powergate driver will hold a list of nvidia powergates
domains, and the generic PM domain will hold a list of all generic PM
domains.

Perhaps there's a way to allow the generic PM domain to control this
by itself. If we for example used the struct device corresponding to
the powergate driver, genpd could use it to distinguish between
various instances of genpd structs..!? Maybe it would simplify the way
to deal with removing domains?

Yes, that would be ideal. However, would have require changing
genpd_init()? I am not sure how genpd would be able to access the device
struct for the powergate driver because we don't provide this via any
API I am aware of? And I am guessing that you don't wish to expose the
gpd_list to the world either.

If there is an easy way, I am open to it, but looking at it today, I am
not sure I see a simple way in which we could add a new API to do this.
However, may be I am missing something!

If we add a new __pm_genpd_init() API, that could require a struct
device to be provided. That API will thus invoke the existing
pm_genpd_init() but also deal with the extra things needed here.

I would also allow such an API to return an error code.

Correspondingly, pm_genpd_remove() could be required to be provided
with a struct device.

Existing users of pm_genpd_init() can then convert to
__pm_genpd_init() whenever suitable.

Of course, another option is just to add new member in the genpd
struct for the struct *device. The caller of pm_genpd_init() could
check it, but allow it to be NULL. Although, the pm_genpd_remove() API
would require that pointer to the struct device to be set...

What do you think?

Kind regards
Uffe