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

On 11/02/16 10:13, Jon Hunter wrote:

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?

By the way, do you think that instead of passing the struct device * to
pm_genpd_remove(), we should just have a void *dev_id in the same way
the request_irq()/free_irq() work? In other words, it would allow people
to use the struct device or struct device_node, etc?

Cheers
Jon