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

[Ulf Hansson]

On 11 February 2016 at 17:38, Jon Hunter <jonathanh at nvidia.com> wrote:
>
> On 11/02/16 10:28, Ulf Hansson wrote:
>> On 11 February 2016 at 11:13, Jon Hunter <jonathanh at nvidia.com> wrote:
>>>
>>> On 11/02/16 09:57, Ulf Hansson wrote:
>>>> On 11 February 2016 at 10:13, Jon Hunter <jonathanh at nvidia.com> wrote:
>>>>>
>>>>> On 10/02/16 18:25, Ulf Hansson wrote:
>>>>>
>>>>> [snip]
>>>>>
>>>>>>>> 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