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

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

On 11/02/16 10:28, Ulf Hansson wrote:
> On 11 February 2016 at 11:13, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> On 11/02/16 09:57, Ulf Hansson wrote:
>>> On 11 February 2016 at 10:13, Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org> 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().

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()?

Let me know what you think.

Cheers
Jon