On 11 February 2016 at 17:38, Jon Hunter [off-list ref] wrote:

On 11/02/16 10:28, Ulf Hansson wrote:

quoted

On 11 February 2016 at 11:13, Jon Hunter [off-list ref] wrote:

quoted

On 11/02/16 09:57, Ulf Hansson wrote:

quoted

On 11 February 2016 at 10:13, Jon Hunter [off-list ref] wrote:

quoted

On 10/02/16 18:25, Ulf Hansson wrote:

[snip]

quoted

quoted

quoted

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?

Yes, sounds good. May be it is simpler just to add a new member and let
the platform genpd driver handle it.

I am wondering if in addition to pm_genpd_remove(), we then just have a
function called pm_genpd_remove_tail(), which allows you to pass the
struct device pointer and will remove the last pm-domain from the
gpd_list and return the genpd pointer if successful. Internally, it will
call pm_genpd_remove(). It seems to me that if there are nested
pm-domains, then we probably want to remove them starting from the tail
as opposed to the head.

How does that sound?

Why not make pm_genpd_remove() to behave as you describe for
pm_genpd_remove_tail()?
That's probably the only sane way to remove genpds anyhow!?

Simply to offer flexibility. I could see that for some devices that have
no dependencies between pm-domains and have a static list of pm-domains,
they can simply call pm_genpd_remove() for a given pm-domain. However,
that said, I can envision a case where a single pm-domain would be
removed by itself and so may be there is no benefit?

If I understand correctly, you agree to try with the most simple
approach first and thus without providing too much flexibility.

Anyway, I am looking forward to review your next version of the patchset! :-)

So one snag I hit with this, is that in order to remove a pm-domain, I
first need to check if it is a subdomain of any other domains and if so
remove it as a subdomain first. Before, this was simple because I called
pm_genpd_remove_subdomain if the domain had a parent, and then called
pm_genpd_remove().

You certainly have a point here.

One more thing, we not only have to check whether the domain is a
subdomain. we also need to check if the domain is a parent (master)
for other subdomains. It being a parent, prevents us from removing it
until all its subdomains are removed.

With the proposal we have discussed, I no longer have any knowledge of
whether the pm-domain is a subdomain or not because pm_genpd_remove()
would remove the tail and then return it. So this means that I now need
to handle the removal of the subdomain within pm_genpd_remove(), which
is ok, but creates more changes as I need to parse the slave_links list,
etc.

I am wondering if it would be better to add a simple function called
pm_genpd_list_get_tail(struct device *dev) that would return the last
pm-domain register for a given device and then simply call
pm_genpd_remove_subdomain() (if needed) and pm_genpd_remove()?

That should work. Please go ahead and have try!

Kind regards
Uffe