Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 22:21, Ulf Hansson wrote:

...


OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that you
said 'although we need to extend it to cover cleanup of the earlier
registered device, via calling device_unregister().' So if we do this then
that would be fine.


Let me clarify the changelog. It's not a to-do, as it's already done
as part of $subject patch.


Yes I see it now. OK, then that's fine.

Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 22:21, Ulf Hansson wrote:

...


OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that you
said 'although we need to extend it to cover cleanup of the earlier
registered device, via calling device_unregister().' So if we do this then
that would be fine.


Let me clarify the changelog. It's not a to-do, as it's already done
as part of $subject patch.


Yes I see it now. OK, then that's fine.

Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

[...]

>>>
>>> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>>> * @dev: Device to attach.
>>> * @index: The index of the PM domain.
>>>
>>> This naming and description is a bit misleading, because really it is not
>>> attaching the device that is passed, but creating a new device to attach
>>> a
>>> PM domain to. So we should consider renaming and changing the description
>>> and indicate that users need to link the device.
>>
>>
>> I picked the name to be consistent with the existing
>> genpd_dev_pm_attach(). Do you have a better suggestion?
>
>
> Well, it appears to get more of a 'get' function and so I don't see why we
> could not have 'genpd_dev_get_by_id()' and then we could have a
> genpd_dev_put() as well (which would call genpd_dev_pm_detach).
>
>> I agree, some details is missing to the description, let me try to
>> improve it. Actually, I was trying to follow existing descriptions
>> from genpd_dev_pm_attach(), so perhaps that also needs a little
>> update.
>>
>> However, do note that, neither genpd_dev_pm_attach() or
>> genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
>> rather only by the driver core. So description may not be so
>> important.
>>
>> In regards to good descriptions, for sure the API added in patch9,
>> dev_pm_domain_attach_by_id(), needs a good one, as this is what
>> drivers should be using.
>
>
> OK. Same appears to apply here to the description as I mentioned above.
> Still seems to be more of a 'get' than an attach. So I wonder if it should
> be dev_pm_domain_get_by_id() instead?

Regarding "get" vs "attach", I suggest we continue to discuss that in
patch 9. Whatever is decided, $subject patch needs to follow.

>
>>> Finally, how is a PM domain attached via calling
>>> genpd_dev_pm_attach_by_id()
>>> detached?
>>
>>
>> Via the existing genpd_dev_pm_detach(), according to what I have
>> described in the change log. I clarify the description in regards to
>> this as well.
>
>
> OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that you
> said 'although we need to extend it to cover cleanup of the earlier
> registered device, via calling device_unregister().' So if we do this then
> that would be fine.

Let me clarify the changelog. It's not a to-do, as it's already done
as part of $subject patch.

So I guess we are in agreement that we don't need another API to deal
with detach?

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

[...]

>>>
>>> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>>> * @dev: Device to attach.
>>> * @index: The index of the PM domain.
>>>
>>> This naming and description is a bit misleading, because really it is not
>>> attaching the device that is passed, but creating a new device to attach
>>> a
>>> PM domain to. So we should consider renaming and changing the description
>>> and indicate that users need to link the device.
>>
>>
>> I picked the name to be consistent with the existing
>> genpd_dev_pm_attach(). Do you have a better suggestion?
>
>
> Well, it appears to get more of a 'get' function and so I don't see why we
> could not have 'genpd_dev_get_by_id()' and then we could have a
> genpd_dev_put() as well (which would call genpd_dev_pm_detach).
>
>> I agree, some details is missing to the description, let me try to
>> improve it. Actually, I was trying to follow existing descriptions
>> from genpd_dev_pm_attach(), so perhaps that also needs a little
>> update.
>>
>> However, do note that, neither genpd_dev_pm_attach() or
>> genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
>> rather only by the driver core. So description may not be so
>> important.
>>
>> In regards to good descriptions, for sure the API added in patch9,
>> dev_pm_domain_attach_by_id(), needs a good one, as this is what
>> drivers should be using.
>
>
> OK. Same appears to apply here to the description as I mentioned above.
> Still seems to be more of a 'get' than an attach. So I wonder if it should
> be dev_pm_domain_get_by_id() instead?

Regarding "get" vs "attach", I suggest we continue to discuss that in
patch 9. Whatever is decided, $subject patch needs to follow.

>
>>> Finally, how is a PM domain attached via calling
>>> genpd_dev_pm_attach_by_id()
>>> detached?
>>
>>
>> Via the existing genpd_dev_pm_detach(), according to what I have
>> described in the change log. I clarify the description in regards to
>> this as well.
>
>
> OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that you
> said 'although we need to extend it to cover cleanup of the earlier
> registered device, via calling device_unregister().' So if we do this then
> that would be fine.

Let me clarify the changelog. It's not a to-do, as it's already done
as part of $subject patch.

So I guess we are in agreement that we don't need another API to deal
with detach?

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 13:17, Ulf Hansson wrote:

On 24 May 2018 at 11:36, Jon Hunter  wrote:


On 24/05/18 08:04, Ulf Hansson wrote:

...


Any reason why we could not add a 'boolean' argument to the API to
indicate
whether the new device should be linked? I think that I prefer the API
handles it, but I can see there could be instances where drivers may wish
to
handle it themselves.



Coming back to this question. Both Tegra XUSB and Qcom Camera use
case, would benefit from doing the linking themselves, as it needs
different PM domains to be powered on depending on the current use
case - as to avoid wasting power.

However, I can understand that you prefer some simplicity over
optimizations, as you told us. Then, does it mean that you are
insisting on extending the APIs with a boolean for linking, or are you
fine with the driver to call device_link_add()?



I am fine with the driver calling device_link_add(), but I just wonder if we
will find a several drivers doing this and then we will end up doing this
later anyway.


Okay.



The current API is called ...

* genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
* @dev: Device to attach.
* @index: The index of the PM domain.

This naming and description is a bit misleading, because really it is not
attaching the device that is passed, but creating a new device to attach a
PM domain to. So we should consider renaming and changing the description
and indicate that users need to link the device.


I picked the name to be consistent with the existing
genpd_dev_pm_attach(). Do you have a better suggestion?


Well, it appears to get more of a 'get' function and so I don't see why 
we could not have 'genpd_dev_get_by_id()' and then we could have a 
genpd_dev_put() as well (which would call genpd_dev_pm_detach).



I agree, some details is missing to the description, let me try to
improve it. Actually, I was trying to follow existing descriptions
from genpd_dev_pm_attach(), so perhaps that also needs a little
update.

However, do note that, neither genpd_dev_pm_attach() or
genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
rather only by the driver core. So description may not be so
important.

In regards to good descriptions, for sure the API added in patch9,
dev_pm_domain_attach_by_id(), needs a good one, as this is what
drivers should be using.


OK. Same appears to apply here to the description as I mentioned above. 
Still seems to be more of a 'get' than an attach. So I wonder if it 
should be dev_pm_domain_get_by_id() instead?



Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
detached?


Via the existing genpd_dev_pm_detach(), according to what I have
described in the change log. I clarify the description in regards to
this as well.


OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that 
you said 'although we need to extend it to cover cleanup of the earlier 
registered device, via calling device_unregister().' So if we do this 
then that would be fine.


Cheers!
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 13:17, Ulf Hansson wrote:

On 24 May 2018 at 11:36, Jon Hunter  wrote:


On 24/05/18 08:04, Ulf Hansson wrote:

...


Any reason why we could not add a 'boolean' argument to the API to
indicate
whether the new device should be linked? I think that I prefer the API
handles it, but I can see there could be instances where drivers may wish
to
handle it themselves.



Coming back to this question. Both Tegra XUSB and Qcom Camera use
case, would benefit from doing the linking themselves, as it needs
different PM domains to be powered on depending on the current use
case - as to avoid wasting power.

However, I can understand that you prefer some simplicity over
optimizations, as you told us. Then, does it mean that you are
insisting on extending the APIs with a boolean for linking, or are you
fine with the driver to call device_link_add()?



I am fine with the driver calling device_link_add(), but I just wonder if we
will find a several drivers doing this and then we will end up doing this
later anyway.


Okay.



The current API is called ...

* genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
* @dev: Device to attach.
* @index: The index of the PM domain.

This naming and description is a bit misleading, because really it is not
attaching the device that is passed, but creating a new device to attach a
PM domain to. So we should consider renaming and changing the description
and indicate that users need to link the device.


I picked the name to be consistent with the existing
genpd_dev_pm_attach(). Do you have a better suggestion?


Well, it appears to get more of a 'get' function and so I don't see why 
we could not have 'genpd_dev_get_by_id()' and then we could have a 
genpd_dev_put() as well (which would call genpd_dev_pm_detach).



I agree, some details is missing to the description, let me try to
improve it. Actually, I was trying to follow existing descriptions
from genpd_dev_pm_attach(), so perhaps that also needs a little
update.

However, do note that, neither genpd_dev_pm_attach() or
genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
rather only by the driver core. So description may not be so
important.

In regards to good descriptions, for sure the API added in patch9,
dev_pm_domain_attach_by_id(), needs a good one, as this is what
drivers should be using.


OK. Same appears to apply here to the description as I mentioned above. 
Still seems to be more of a 'get' than an attach. So I wonder if it 
should be dev_pm_domain_get_by_id() instead?



Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
detached?


Via the existing genpd_dev_pm_detach(), according to what I have
described in the change log. I clarify the description in regards to
this as well.


OK, so this bit is a to-do as that is not yet exposed AFAICT. I see that 
you said 'although we need to extend it to cover cleanup of the earlier 
registered device, via calling device_unregister().' So if we do this 
then that would be fine.


Cheers!
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 24 May 2018 at 11:36, Jon Hunter  wrote:
>
> On 24/05/18 08:04, Ulf Hansson wrote:
>
> ...
>
>>> Any reason why we could not add a 'boolean' argument to the API to
>>> indicate
>>> whether the new device should be linked? I think that I prefer the API
>>> handles it, but I can see there could be instances where drivers may wish
>>> to
>>> handle it themselves.
>>
>>
>> Coming back to this question. Both Tegra XUSB and Qcom Camera use
>> case, would benefit from doing the linking themselves, as it needs
>> different PM domains to be powered on depending on the current use
>> case - as to avoid wasting power.
>>
>> However, I can understand that you prefer some simplicity over
>> optimizations, as you told us. Then, does it mean that you are
>> insisting on extending the APIs with a boolean for linking, or are you
>> fine with the driver to call device_link_add()?
>
>
> I am fine with the driver calling device_link_add(), but I just wonder if we
> will find a several drivers doing this and then we will end up doing this
> later anyway.

Okay.

>
> The current API is called ...
>
> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> * @dev: Device to attach.
> * @index: The index of the PM domain.
>
> This naming and description is a bit misleading, because really it is not
> attaching the device that is passed, but creating a new device to attach a
> PM domain to. So we should consider renaming and changing the description
> and indicate that users need to link the device.

I picked the name to be consistent with the existing
genpd_dev_pm_attach(). Do you have a better suggestion?

I agree, some details is missing to the description, let me try to
improve it. Actually, I was trying to follow existing descriptions
from genpd_dev_pm_attach(), so perhaps that also needs a little
update.

However, do note that, neither genpd_dev_pm_attach() or
genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
rather only by the driver core. So description may not be so
important.

In regards to good descriptions, for sure the API added in patch9,
dev_pm_domain_attach_by_id(), needs a good one, as this is what
drivers should be using.

>
> Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
> detached?

Via the existing genpd_dev_pm_detach(), according to what I have
described in the change log. I clarify the description in regards to
this as well.

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 24 May 2018 at 11:36, Jon Hunter  wrote:
>
> On 24/05/18 08:04, Ulf Hansson wrote:
>
> ...
>
>>> Any reason why we could not add a 'boolean' argument to the API to
>>> indicate
>>> whether the new device should be linked? I think that I prefer the API
>>> handles it, but I can see there could be instances where drivers may wish
>>> to
>>> handle it themselves.
>>
>>
>> Coming back to this question. Both Tegra XUSB and Qcom Camera use
>> case, would benefit from doing the linking themselves, as it needs
>> different PM domains to be powered on depending on the current use
>> case - as to avoid wasting power.
>>
>> However, I can understand that you prefer some simplicity over
>> optimizations, as you told us. Then, does it mean that you are
>> insisting on extending the APIs with a boolean for linking, or are you
>> fine with the driver to call device_link_add()?
>
>
> I am fine with the driver calling device_link_add(), but I just wonder if we
> will find a several drivers doing this and then we will end up doing this
> later anyway.

Okay.

>
> The current API is called ...
>
> * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> * @dev: Device to attach.
> * @index: The index of the PM domain.
>
> This naming and description is a bit misleading, because really it is not
> attaching the device that is passed, but creating a new device to attach a
> PM domain to. So we should consider renaming and changing the description
> and indicate that users need to link the device.

I picked the name to be consistent with the existing
genpd_dev_pm_attach(). Do you have a better suggestion?

I agree, some details is missing to the description, let me try to
improve it. Actually, I was trying to follow existing descriptions
from genpd_dev_pm_attach(), so perhaps that also needs a little
update.

However, do note that, neither genpd_dev_pm_attach() or
genpd_dev_pm_attach_by_id() is supposed to be called by drivers, but
rather only by the driver core. So description may not be so
important.

In regards to good descriptions, for sure the API added in patch9,
dev_pm_domain_attach_by_id(), needs a good one, as this is what
drivers should be using.

>
> Finally, how is a PM domain attached via calling genpd_dev_pm_attach_by_id()
> detached?

Via the existing genpd_dev_pm_detach(), according to what I have
described in the change log. I clarify the description in regards to
this as well.

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 08:04, Ulf Hansson wrote:

...


Any reason why we could not add a 'boolean' argument to the API to indicate
whether the new device should be linked? I think that I prefer the API
handles it, but I can see there could be instances where drivers may wish to
handle it themselves.


Coming back to this question. Both Tegra XUSB and Qcom Camera use
case, would benefit from doing the linking themselves, as it needs
different PM domains to be powered on depending on the current use
case - as to avoid wasting power.

However, I can understand that you prefer some simplicity over
optimizations, as you told us. Then, does it mean that you are
insisting on extending the APIs with a boolean for linking, or are you
fine with the driver to call device_link_add()?


I am fine with the driver calling device_link_add(), but I just wonder 
if we will find a several drivers doing this and then we will end up 
doing this later anyway.


The current API is called ...

* genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
* @dev: Device to attach.
* @index: The index of the PM domain.

This naming and description is a bit misleading, because really it is 
not attaching the device that is passed, but creating a new device to 
attach a PM domain to. So we should consider renaming and changing the 
description and indicate that users need to link the device.


Finally, how is a PM domain attached via calling 
genpd_dev_pm_attach_by_id() detached?


Cheers
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 24/05/18 08:04, Ulf Hansson wrote:

...


Any reason why we could not add a 'boolean' argument to the API to indicate
whether the new device should be linked? I think that I prefer the API
handles it, but I can see there could be instances where drivers may wish to
handle it themselves.


Coming back to this question. Both Tegra XUSB and Qcom Camera use
case, would benefit from doing the linking themselves, as it needs
different PM domains to be powered on depending on the current use
case - as to avoid wasting power.

However, I can understand that you prefer some simplicity over
optimizations, as you told us. Then, does it mean that you are
insisting on extending the APIs with a boolean for linking, or are you
fine with the driver to call device_link_add()?


I am fine with the driver calling device_link_add(), but I just wonder 
if we will find a several drivers doing this and then we will end up 
doing this later anyway.


The current API is called ...

* genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
* @dev: Device to attach.
* @index: The index of the PM domain.

This naming and description is a bit misleading, because really it is 
not attaching the device that is passed, but creating a new device to 
attach a PM domain to. So we should consider renaming and changing the 
description and indicate that users need to link the device.


Finally, how is a PM domain attached via calling 
genpd_dev_pm_attach_by_id() detached?


Cheers
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 23 May 2018 at 11:07, Jon Hunter  wrote:
>
> On 23/05/18 07:12, Ulf Hansson wrote:
>
> ...
>
>
>> Thanks for sending this. Believe it or not this has still been on my
>> to-do list
>> and so we definitely need a solution for Tegra.
>>
>> Looking at the above it appears that additional power-domains exposed
>> as devices
>> to the client device. So I assume that this means that the drivers for
>> devices
>> with multiple power-domains will need to call RPM APIs for each of
>> these
>> additional power-domains. Is that correct?
>
>
> They can, but should not!
>
> Instead, the driver shall use device_link_add() and device_link_del(),
> dynamically, depending on what PM domain that their original device
> needs for the current running use case.
>
> In that way, they keep existing runtime PM deployment, operating on
> its original device.


 OK, sounds good. Any reason why the linking cannot be handled by the
 above API? Is there a use-case where you would not want it linked?
>>>
>>>
>>> I am guessing the linking is what would give the driver the ability to
>>> decide which subset of powerdomains it actually wants to control
>>> at any point using runtime PM. If we have cases wherein the driver would
>>> want to turn on/off _all_ its associated powerdomains _always_
>>> then a default linking of all would help.
>>
>>
>> First, I think we need to decide on *where* the linking should be
>> done, not at both places, as that would just mess up synchronization
>> of who is responsible for calling the device_link_del() at detach.
>>
>> Second, It would in principle be fine to call device_link_add() and
>> device_link_del() as a part of the attach/detach APIs. However, there
>> is a downside to such solution, which would be that the driver then
>> needs call the detach API, just to do device_link_del(). Of course
>> then it would also needs to call the attach API later if/when needed.
>> Doing this adds unnecessary overhead - comparing to just let the
>> driver call device_link_add|del() when needed. On the upside, yes, it
>> would put less burden on the drivers as it then only needs to care
>> about using one set of functions.
>>
>> Which solution do you prefer?
>
>
> Any reason why we could not add a 'boolean' argument to the API to indicate
> whether the new device should be linked? I think that I prefer the API
> handles it, but I can see there could be instances where drivers may wish to
> handle it themselves.

Coming back to this question. Both Tegra XUSB and Qcom Camera use
case, would benefit from doing the linking themselves, as it needs
different PM domains to be powered on depending on the current use
case - as to avoid wasting power.

However, I can understand that you prefer some simplicity over
optimizations, as you told us. Then, does it mean that you are
insisting on extending the APIs with a boolean for linking, or are you
fine with the driver to call device_link_add()?

[...]

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 23 May 2018 at 11:07, Jon Hunter  wrote:
>
> On 23/05/18 07:12, Ulf Hansson wrote:
>
> ...
>
>
>> Thanks for sending this. Believe it or not this has still been on my
>> to-do list
>> and so we definitely need a solution for Tegra.
>>
>> Looking at the above it appears that additional power-domains exposed
>> as devices
>> to the client device. So I assume that this means that the drivers for
>> devices
>> with multiple power-domains will need to call RPM APIs for each of
>> these
>> additional power-domains. Is that correct?
>
>
> They can, but should not!
>
> Instead, the driver shall use device_link_add() and device_link_del(),
> dynamically, depending on what PM domain that their original device
> needs for the current running use case.
>
> In that way, they keep existing runtime PM deployment, operating on
> its original device.


 OK, sounds good. Any reason why the linking cannot be handled by the
 above API? Is there a use-case where you would not want it linked?
>>>
>>>
>>> I am guessing the linking is what would give the driver the ability to
>>> decide which subset of powerdomains it actually wants to control
>>> at any point using runtime PM. If we have cases wherein the driver would
>>> want to turn on/off _all_ its associated powerdomains _always_
>>> then a default linking of all would help.
>>
>>
>> First, I think we need to decide on *where* the linking should be
>> done, not at both places, as that would just mess up synchronization
>> of who is responsible for calling the device_link_del() at detach.
>>
>> Second, It would in principle be fine to call device_link_add() and
>> device_link_del() as a part of the attach/detach APIs. However, there
>> is a downside to such solution, which would be that the driver then
>> needs call the detach API, just to do device_link_del(). Of course
>> then it would also needs to call the attach API later if/when needed.
>> Doing this adds unnecessary overhead - comparing to just let the
>> driver call device_link_add|del() when needed. On the upside, yes, it
>> would put less burden on the drivers as it then only needs to care
>> about using one set of functions.
>>
>> Which solution do you prefer?
>
>
> Any reason why we could not add a 'boolean' argument to the API to indicate
> whether the new device should be linked? I think that I prefer the API
> handles it, but I can see there could be instances where drivers may wish to
> handle it themselves.

Coming back to this question. Both Tegra XUSB and Qcom Camera use
case, would benefit from doing the linking themselves, as it needs
different PM domains to be powered on depending on the current use
case - as to avoid wasting power.

However, I can understand that you prefer some simplicity over
optimizations, as you told us. Then, does it mean that you are
insisting on extending the APIs with a boolean for linking, or are you
fine with the driver to call device_link_add()?

[...]

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 23/05/18 10:47, Ulf Hansson wrote:

On 23 May 2018 at 11:45, Jon Hunter  wrote:


On 23/05/18 10:33, Ulf Hansson wrote:


On 23 May 2018 at 11:27, Rajendra Nayak  wrote:




On 05/23/2018 02:37 PM, Jon Hunter wrote:



On 23/05/18 07:12, Ulf Hansson wrote:

...


Thanks for sending this. Believe it or not this has still been on
my to-do list
and so we definitely need a solution for Tegra.

Looking at the above it appears that additional power-domains
exposed as devices
to the client device. So I assume that this means that the drivers
for devices
with multiple power-domains will need to call RPM APIs for each of
these
additional power-domains. Is that correct?



They can, but should not!

Instead, the driver shall use device_link_add() and
device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.



OK, sounds good. Any reason why the linking cannot be handled by the
above API? Is there a use-case where you would not want it linked?



I am guessing the linking is what would give the driver the ability to
decide which subset of powerdomains it actually wants to control
at any point using runtime PM. If we have cases wherein the driver
would want to turn on/off _all_ its associated powerdomains _always_
then a default linking of all would help.



First, I think we need to decide on *where* the linking should be
done, not at both places, as that would just mess up synchronization
of who is responsible for calling the device_link_del() at detach.

Second, It would in principle be fine to call device_link_add() and
device_link_del() as a part of the attach/detach APIs. However, there
is a downside to such solution, which would be that the driver then
needs call the detach API, just to do device_link_del(). Of course
then it would also needs to call the attach API later if/when needed.
Doing this adds unnecessary overhead - comparing to just let the
driver call device_link_add|del() when needed. On the upside, yes, it
would put less burden on the drivers as it then only needs to care
about using one set of functions.

Which solution do you prefer?



Any reason why we could not add a 'boolean' argument to the API to
indicate whether the new device should be linked? I think that I prefer the
API handles it, but I can see there could be instances where drivers may
wish to handle it themselves.

Rajendra, do you have a use-case right now where the driver would want
to handle the linking?



So if I understand this right, any driver which does want to control
individual powerdomain state would
need to do the linking itself right?

What I am saying is, if I have device A, with powerdomains X and Y, and
if I want to turn on only X,
then I would want only X to be linked with A, and at a later point if I
want both X and Y to be turned on,
I would then go ahead and link both X and Y to A? Is that correct or did
I get it all wrong?



Correct!



I know atleast Camera on msm8996 would need to do this since it has 2 vfe
powerdoamins, which can be
turned on one at a time (depending on what resolution needs to be
supported) or both together if we
really need very high resolution using both vfe modules.



I think this is also the case for the Tegra XUSB subsystem.

The usb device is always attached to one PM domain, but depending on
if super-speed mode is used, another PM domain for that logic needs to
be powered on as well.

Jon, please correct me if I am wrong!



Yes this is technically correct, however, in reality I think we are always
going to enable the superspeed domain if either the host or device domain is
enabled. So we would probably always link the superspeed with the host and
device devices.


Why? Wouldn't that waste power if the superspeed mode isn't used?


Simply to reduce complexity.

Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 23/05/18 10:47, Ulf Hansson wrote:

On 23 May 2018 at 11:45, Jon Hunter  wrote:


On 23/05/18 10:33, Ulf Hansson wrote:


On 23 May 2018 at 11:27, Rajendra Nayak  wrote:




On 05/23/2018 02:37 PM, Jon Hunter wrote:



On 23/05/18 07:12, Ulf Hansson wrote:

...


Thanks for sending this. Believe it or not this has still been on
my to-do list
and so we definitely need a solution for Tegra.

Looking at the above it appears that additional power-domains
exposed as devices
to the client device. So I assume that this means that the drivers
for devices
with multiple power-domains will need to call RPM APIs for each of
these
additional power-domains. Is that correct?



They can, but should not!

Instead, the driver shall use device_link_add() and
device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.



OK, sounds good. Any reason why the linking cannot be handled by the
above API? Is there a use-case where you would not want it linked?



I am guessing the linking is what would give the driver the ability to
decide which subset of powerdomains it actually wants to control
at any point using runtime PM. If we have cases wherein the driver
would want to turn on/off _all_ its associated powerdomains _always_
then a default linking of all would help.



First, I think we need to decide on *where* the linking should be
done, not at both places, as that would just mess up synchronization
of who is responsible for calling the device_link_del() at detach.

Second, It would in principle be fine to call device_link_add() and
device_link_del() as a part of the attach/detach APIs. However, there
is a downside to such solution, which would be that the driver then
needs call the detach API, just to do device_link_del(). Of course
then it would also needs to call the attach API later if/when needed.
Doing this adds unnecessary overhead - comparing to just let the
driver call device_link_add|del() when needed. On the upside, yes, it
would put less burden on the drivers as it then only needs to care
about using one set of functions.

Which solution do you prefer?



Any reason why we could not add a 'boolean' argument to the API to
indicate whether the new device should be linked? I think that I prefer the
API handles it, but I can see there could be instances where drivers may
wish to handle it themselves.

Rajendra, do you have a use-case right now where the driver would want
to handle the linking?



So if I understand this right, any driver which does want to control
individual powerdomain state would
need to do the linking itself right?

What I am saying is, if I have device A, with powerdomains X and Y, and
if I want to turn on only X,
then I would want only X to be linked with A, and at a later point if I
want both X and Y to be turned on,
I would then go ahead and link both X and Y to A? Is that correct or did
I get it all wrong?



Correct!



I know atleast Camera on msm8996 would need to do this since it has 2 vfe
powerdoamins, which can be
turned on one at a time (depending on what resolution needs to be
supported) or both together if we
really need very high resolution using both vfe modules.



I think this is also the case for the Tegra XUSB subsystem.

The usb device is always attached to one PM domain, but depending on
if super-speed mode is used, another PM domain for that logic needs to
be powered on as well.

Jon, please correct me if I am wrong!



Yes this is technically correct, however, in reality I think we are always
going to enable the superspeed domain if either the host or device domain is
enabled. So we would probably always link the superspeed with the host and
device devices.


Why? Wouldn't that waste power if the superspeed mode isn't used?


Simply to reduce complexity.

Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 23 May 2018 at 11:45, Jon Hunter  wrote:
>
> On 23/05/18 10:33, Ulf Hansson wrote:
>>
>> On 23 May 2018 at 11:27, Rajendra Nayak  wrote:
>>>
>>>
>>>
>>> On 05/23/2018 02:37 PM, Jon Hunter wrote:


 On 23/05/18 07:12, Ulf Hansson wrote:

 ...

> Thanks for sending this. Believe it or not this has still been on
> my to-do list
> and so we definitely need a solution for Tegra.
>
> Looking at the above it appears that additional power-domains
> exposed as devices
> to the client device. So I assume that this means that the drivers
> for devices
> with multiple power-domains will need to call RPM APIs for each of
> these
> additional power-domains. Is that correct?


 They can, but should not!

 Instead, the driver shall use device_link_add() and
 device_link_del(),
 dynamically, depending on what PM domain that their original device
 needs for the current running use case.

 In that way, they keep existing runtime PM deployment, operating on
 its original device.
>>>
>>>
>>> OK, sounds good. Any reason why the linking cannot be handled by the
>>> above API? Is there a use-case where you would not want it linked?
>>
>>
>> I am guessing the linking is what would give the driver the ability to
>> decide which subset of powerdomains it actually wants to control
>> at any point using runtime PM. If we have cases wherein the driver
>> would want to turn on/off _all_ its associated powerdomains _always_
>> then a default linking of all would help.
>
>
> First, I think we need to decide on *where* the linking should be
> done, not at both places, as that would just mess up synchronization
> of who is responsible for calling the device_link_del() at detach.
>
> Second, It would in principle be fine to call device_link_add() and
> device_link_del() as a part of the attach/detach APIs. However, there
> is a downside to such solution, which would be that the driver then
> needs call the detach API, just to do device_link_del(). Of course
> then it would also needs to call the attach API later if/when needed.
> Doing this adds unnecessary overhead - comparing to just let the
> driver call device_link_add|del() when needed. On the upside, yes, it
> would put less burden on the drivers as it then only needs to care
> about using one set of functions.
>
> Which solution do you prefer?


 Any reason why we could not add a 'boolean' argument to the API to
 indicate whether the new device should be linked? I think that I prefer the
 API handles it, but I can see there could be instances where drivers may
 wish to handle it themselves.

 Rajendra, do you have a use-case right now where the driver would want
 to handle the linking?
>>>
>>>
>>> So if I understand this right, any driver which does want to control
>>> individual powerdomain state would
>>> need to do the linking itself right?
>>>
>>> What I am saying is, if I have device A, with powerdomains X and Y, and
>>> if I want to turn on only X,
>>> then I would want only X to be linked with A, and at a later point if I
>>> want both X and Y to be turned on,
>>> I would then go ahead and link both X and Y to A? Is that correct or did
>>> I get it all wrong?
>>
>>
>> Correct!
>>
>>>
>>> I know atleast Camera on msm8996 would need to do this since it has 2 vfe
>>> powerdoamins, which can be
>>> turned on one at a time (depending on what resolution needs to be
>>> supported) or both together if we
>>> really need very high resolution using both vfe modules.
>>
>>
>> I think this is also the case for the Tegra XUSB subsystem.
>>
>> The usb device is always attached to one PM domain, but depending on
>> if super-speed mode is used, another PM domain for that logic needs to
>> be powered on as well.
>>
>> Jon, please correct me if I am wrong!
>
>
> Yes this is technically correct, however, in reality I think we are always
> going to enable the superspeed domain if either the host or device domain is
> enabled. So we would probably always link the superspeed with the host and
> device devices.

Why? Wouldn't that waste power if the superspeed mode isn't used?

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 23 May 2018 at 11:45, Jon Hunter  wrote:
>
> On 23/05/18 10:33, Ulf Hansson wrote:
>>
>> On 23 May 2018 at 11:27, Rajendra Nayak  wrote:
>>>
>>>
>>>
>>> On 05/23/2018 02:37 PM, Jon Hunter wrote:


 On 23/05/18 07:12, Ulf Hansson wrote:

 ...

> Thanks for sending this. Believe it or not this has still been on
> my to-do list
> and so we definitely need a solution for Tegra.
>
> Looking at the above it appears that additional power-domains
> exposed as devices
> to the client device. So I assume that this means that the drivers
> for devices
> with multiple power-domains will need to call RPM APIs for each of
> these
> additional power-domains. Is that correct?


 They can, but should not!

 Instead, the driver shall use device_link_add() and
 device_link_del(),
 dynamically, depending on what PM domain that their original device
 needs for the current running use case.

 In that way, they keep existing runtime PM deployment, operating on
 its original device.
>>>
>>>
>>> OK, sounds good. Any reason why the linking cannot be handled by the
>>> above API? Is there a use-case where you would not want it linked?
>>
>>
>> I am guessing the linking is what would give the driver the ability to
>> decide which subset of powerdomains it actually wants to control
>> at any point using runtime PM. If we have cases wherein the driver
>> would want to turn on/off _all_ its associated powerdomains _always_
>> then a default linking of all would help.
>
>
> First, I think we need to decide on *where* the linking should be
> done, not at both places, as that would just mess up synchronization
> of who is responsible for calling the device_link_del() at detach.
>
> Second, It would in principle be fine to call device_link_add() and
> device_link_del() as a part of the attach/detach APIs. However, there
> is a downside to such solution, which would be that the driver then
> needs call the detach API, just to do device_link_del(). Of course
> then it would also needs to call the attach API later if/when needed.
> Doing this adds unnecessary overhead - comparing to just let the
> driver call device_link_add|del() when needed. On the upside, yes, it
> would put less burden on the drivers as it then only needs to care
> about using one set of functions.
>
> Which solution do you prefer?


 Any reason why we could not add a 'boolean' argument to the API to
 indicate whether the new device should be linked? I think that I prefer the
 API handles it, but I can see there could be instances where drivers may
 wish to handle it themselves.

 Rajendra, do you have a use-case right now where the driver would want
 to handle the linking?
>>>
>>>
>>> So if I understand this right, any driver which does want to control
>>> individual powerdomain state would
>>> need to do the linking itself right?
>>>
>>> What I am saying is, if I have device A, with powerdomains X and Y, and
>>> if I want to turn on only X,
>>> then I would want only X to be linked with A, and at a later point if I
>>> want both X and Y to be turned on,
>>> I would then go ahead and link both X and Y to A? Is that correct or did
>>> I get it all wrong?
>>
>>
>> Correct!
>>
>>>
>>> I know atleast Camera on msm8996 would need to do this since it has 2 vfe
>>> powerdoamins, which can be
>>> turned on one at a time (depending on what resolution needs to be
>>> supported) or both together if we
>>> really need very high resolution using both vfe modules.
>>
>>
>> I think this is also the case for the Tegra XUSB subsystem.
>>
>> The usb device is always attached to one PM domain, but depending on
>> if super-speed mode is used, another PM domain for that logic needs to
>> be powered on as well.
>>
>> Jon, please correct me if I am wrong!
>
>
> Yes this is technically correct, however, in reality I think we are always
> going to enable the superspeed domain if either the host or device domain is
> enabled. So we would probably always link the superspeed with the host and
> device devices.

Why? Wouldn't that waste power if the superspeed mode isn't used?

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 23/05/18 10:33, Ulf Hansson wrote:

On 23 May 2018 at 11:27, Rajendra Nayak  wrote:



On 05/23/2018 02:37 PM, Jon Hunter wrote:


On 23/05/18 07:12, Ulf Hansson wrote:

...


Thanks for sending this. Believe it or not this has still been on my to-do list
and so we definitely need a solution for Tegra.

Looking at the above it appears that additional power-domains exposed as devices
to the client device. So I assume that this means that the drivers for devices
with multiple power-domains will need to call RPM APIs for each of these
additional power-domains. Is that correct?


They can, but should not!

Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.


OK, sounds good. Any reason why the linking cannot be handled by the above API? 
Is there a use-case where you would not want it linked?


I am guessing the linking is what would give the driver the ability to decide 
which subset of powerdomains it actually wants to control
at any point using runtime PM. If we have cases wherein the driver would want 
to turn on/off _all_ its associated powerdomains _always_
then a default linking of all would help.


First, I think we need to decide on *where* the linking should be
done, not at both places, as that would just mess up synchronization
of who is responsible for calling the device_link_del() at detach.

Second, It would in principle be fine to call device_link_add() and
device_link_del() as a part of the attach/detach APIs. However, there
is a downside to such solution, which would be that the driver then
needs call the detach API, just to do device_link_del(). Of course
then it would also needs to call the attach API later if/when needed.
Doing this adds unnecessary overhead - comparing to just let the
driver call device_link_add|del() when needed. On the upside, yes, it
would put less burden on the drivers as it then only needs to care
about using one set of functions.

Which solution do you prefer?


Any reason why we could not add a 'boolean' argument to the API to indicate 
whether the new device should be linked? I think that I prefer the API handles 
it, but I can see there could be instances where drivers may wish to handle it 
themselves.

Rajendra, do you have a use-case right now where the driver would want to 
handle the linking?


So if I understand this right, any driver which does want to control individual 
powerdomain state would
need to do the linking itself right?

What I am saying is, if I have device A, with powerdomains X and Y, and if I 
want to turn on only X,
then I would want only X to be linked with A, and at a later point if I want 
both X and Y to be turned on,
I would then go ahead and link both X and Y to A? Is that correct or did I get 
it all wrong?


Correct!



I know atleast Camera on msm8996 would need to do this since it has 2 vfe 
powerdoamins, which can be
turned on one at a time (depending on what resolution needs to be supported) or 
both together if we
really need very high resolution using both vfe modules.


I think this is also the case for the Tegra XUSB subsystem.

The usb device is always attached to one PM domain, but depending on
if super-speed mode is used, another PM domain for that logic needs to
be powered on as well.

Jon, please correct me if I am wrong!


Yes this is technically correct, however, in reality I think we are 
always going to enable the superspeed domain if either the host or 
device domain is enabled. So we would probably always link the 
superspeed with the host and device devices.


Cheers
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 23/05/18 10:33, Ulf Hansson wrote:

On 23 May 2018 at 11:27, Rajendra Nayak  wrote:



On 05/23/2018 02:37 PM, Jon Hunter wrote:


On 23/05/18 07:12, Ulf Hansson wrote:

...


Thanks for sending this. Believe it or not this has still been on my to-do list
and so we definitely need a solution for Tegra.

Looking at the above it appears that additional power-domains exposed as devices
to the client device. So I assume that this means that the drivers for devices
with multiple power-domains will need to call RPM APIs for each of these
additional power-domains. Is that correct?


They can, but should not!

Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.


OK, sounds good. Any reason why the linking cannot be handled by the above API? 
Is there a use-case where you would not want it linked?


I am guessing the linking is what would give the driver the ability to decide 
which subset of powerdomains it actually wants to control
at any point using runtime PM. If we have cases wherein the driver would want 
to turn on/off _all_ its associated powerdomains _always_
then a default linking of all would help.


First, I think we need to decide on *where* the linking should be
done, not at both places, as that would just mess up synchronization
of who is responsible for calling the device_link_del() at detach.

Second, It would in principle be fine to call device_link_add() and
device_link_del() as a part of the attach/detach APIs. However, there
is a downside to such solution, which would be that the driver then
needs call the detach API, just to do device_link_del(). Of course
then it would also needs to call the attach API later if/when needed.
Doing this adds unnecessary overhead - comparing to just let the
driver call device_link_add|del() when needed. On the upside, yes, it
would put less burden on the drivers as it then only needs to care
about using one set of functions.

Which solution do you prefer?


Any reason why we could not add a 'boolean' argument to the API to indicate 
whether the new device should be linked? I think that I prefer the API handles 
it, but I can see there could be instances where drivers may wish to handle it 
themselves.

Rajendra, do you have a use-case right now where the driver would want to 
handle the linking?


So if I understand this right, any driver which does want to control individual 
powerdomain state would
need to do the linking itself right?

What I am saying is, if I have device A, with powerdomains X and Y, and if I 
want to turn on only X,
then I would want only X to be linked with A, and at a later point if I want 
both X and Y to be turned on,
I would then go ahead and link both X and Y to A? Is that correct or did I get 
it all wrong?


Correct!



I know atleast Camera on msm8996 would need to do this since it has 2 vfe 
powerdoamins, which can be
turned on one at a time (depending on what resolution needs to be supported) or 
both together if we
really need very high resolution using both vfe modules.


I think this is also the case for the Tegra XUSB subsystem.

The usb device is always attached to one PM domain, but depending on
if super-speed mode is used, another PM domain for that logic needs to
be powered on as well.

Jon, please correct me if I am wrong!


Yes this is technically correct, however, in reality I think we are 
always going to enable the superspeed domain if either the host or 
device domain is enabled. So we would probably always link the 
superspeed with the host and device devices.


Cheers
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 23 May 2018 at 11:27, Rajendra Nayak  wrote:
>
>
> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>
>> On 23/05/18 07:12, Ulf Hansson wrote:
>>
>> ...
>>
>>> Thanks for sending this. Believe it or not this has still been on my 
>>> to-do list
>>> and so we definitely need a solution for Tegra.
>>>
>>> Looking at the above it appears that additional power-domains exposed 
>>> as devices
>>> to the client device. So I assume that this means that the drivers for 
>>> devices
>>> with multiple power-domains will need to call RPM APIs for each of these
>>> additional power-domains. Is that correct?
>>
>> They can, but should not!
>>
>> Instead, the driver shall use device_link_add() and device_link_del(),
>> dynamically, depending on what PM domain that their original device
>> needs for the current running use case.
>>
>> In that way, they keep existing runtime PM deployment, operating on
>> its original device.
>
> OK, sounds good. Any reason why the linking cannot be handled by the 
> above API? Is there a use-case where you would not want it linked?

 I am guessing the linking is what would give the driver the ability to 
 decide which subset of powerdomains it actually wants to control
 at any point using runtime PM. If we have cases wherein the driver would 
 want to turn on/off _all_ its associated powerdomains _always_
 then a default linking of all would help.
>>>
>>> First, I think we need to decide on *where* the linking should be
>>> done, not at both places, as that would just mess up synchronization
>>> of who is responsible for calling the device_link_del() at detach.
>>>
>>> Second, It would in principle be fine to call device_link_add() and
>>> device_link_del() as a part of the attach/detach APIs. However, there
>>> is a downside to such solution, which would be that the driver then
>>> needs call the detach API, just to do device_link_del(). Of course
>>> then it would also needs to call the attach API later if/when needed.
>>> Doing this adds unnecessary overhead - comparing to just let the
>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>> would put less burden on the drivers as it then only needs to care
>>> about using one set of functions.
>>>
>>> Which solution do you prefer?
>>
>> Any reason why we could not add a 'boolean' argument to the API to indicate 
>> whether the new device should be linked? I think that I prefer the API 
>> handles it, but I can see there could be instances where drivers may wish to 
>> handle it themselves.
>>
>> Rajendra, do you have a use-case right now where the driver would want to 
>> handle the linking?
>
> So if I understand this right, any driver which does want to control 
> individual powerdomain state would
> need to do the linking itself right?
>
> What I am saying is, if I have device A, with powerdomains X and Y, and if I 
> want to turn on only X,
> then I would want only X to be linked with A, and at a later point if I want 
> both X and Y to be turned on,
> I would then go ahead and link both X and Y to A? Is that correct or did I 
> get it all wrong?

Correct!

>
> I know atleast Camera on msm8996 would need to do this since it has 2 vfe 
> powerdoamins, which can be
> turned on one at a time (depending on what resolution needs to be supported) 
> or both together if we
> really need very high resolution using both vfe modules.

I think this is also the case for the Tegra XUSB subsystem.

The usb device is always attached to one PM domain, but depending on
if super-speed mode is used, another PM domain for that logic needs to
be powered on as well.

Jon, please correct me if I am wrong!

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

On 23 May 2018 at 11:27, Rajendra Nayak  wrote:
>
>
> On 05/23/2018 02:37 PM, Jon Hunter wrote:
>>
>> On 23/05/18 07:12, Ulf Hansson wrote:
>>
>> ...
>>
>>> Thanks for sending this. Believe it or not this has still been on my 
>>> to-do list
>>> and so we definitely need a solution for Tegra.
>>>
>>> Looking at the above it appears that additional power-domains exposed 
>>> as devices
>>> to the client device. So I assume that this means that the drivers for 
>>> devices
>>> with multiple power-domains will need to call RPM APIs for each of these
>>> additional power-domains. Is that correct?
>>
>> They can, but should not!
>>
>> Instead, the driver shall use device_link_add() and device_link_del(),
>> dynamically, depending on what PM domain that their original device
>> needs for the current running use case.
>>
>> In that way, they keep existing runtime PM deployment, operating on
>> its original device.
>
> OK, sounds good. Any reason why the linking cannot be handled by the 
> above API? Is there a use-case where you would not want it linked?

 I am guessing the linking is what would give the driver the ability to 
 decide which subset of powerdomains it actually wants to control
 at any point using runtime PM. If we have cases wherein the driver would 
 want to turn on/off _all_ its associated powerdomains _always_
 then a default linking of all would help.
>>>
>>> First, I think we need to decide on *where* the linking should be
>>> done, not at both places, as that would just mess up synchronization
>>> of who is responsible for calling the device_link_del() at detach.
>>>
>>> Second, It would in principle be fine to call device_link_add() and
>>> device_link_del() as a part of the attach/detach APIs. However, there
>>> is a downside to such solution, which would be that the driver then
>>> needs call the detach API, just to do device_link_del(). Of course
>>> then it would also needs to call the attach API later if/when needed.
>>> Doing this adds unnecessary overhead - comparing to just let the
>>> driver call device_link_add|del() when needed. On the upside, yes, it
>>> would put less burden on the drivers as it then only needs to care
>>> about using one set of functions.
>>>
>>> Which solution do you prefer?
>>
>> Any reason why we could not add a 'boolean' argument to the API to indicate 
>> whether the new device should be linked? I think that I prefer the API 
>> handles it, but I can see there could be instances where drivers may wish to 
>> handle it themselves.
>>
>> Rajendra, do you have a use-case right now where the driver would want to 
>> handle the linking?
>
> So if I understand this right, any driver which does want to control 
> individual powerdomain state would
> need to do the linking itself right?
>
> What I am saying is, if I have device A, with powerdomains X and Y, and if I 
> want to turn on only X,
> then I would want only X to be linked with A, and at a later point if I want 
> both X and Y to be turned on,
> I would then go ahead and link both X and Y to A? Is that correct or did I 
> get it all wrong?

Correct!

>
> I know atleast Camera on msm8996 would need to do this since it has 2 vfe 
> powerdoamins, which can be
> turned on one at a time (depending on what resolution needs to be supported) 
> or both together if we
> really need very high resolution using both vfe modules.

I think this is also the case for the Tegra XUSB subsystem.

The usb device is always attached to one PM domain, but depending on
if super-speed mode is used, another PM domain for that logic needs to
be powered on as well.

Jon, please correct me if I am wrong!

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Rajendra Nayak]

On 05/23/2018 02:37 PM, Jon Hunter wrote:
> 
> On 23/05/18 07:12, Ulf Hansson wrote:
> 
> ...
> 
>> Thanks for sending this. Believe it or not this has still been on my 
>> to-do list
>> and so we definitely need a solution for Tegra.
>>
>> Looking at the above it appears that additional power-domains exposed as 
>> devices
>> to the client device. So I assume that this means that the drivers for 
>> devices
>> with multiple power-domains will need to call RPM APIs for each of these
>> additional power-domains. Is that correct?
>
> They can, but should not!
>
> Instead, the driver shall use device_link_add() and device_link_del(),
> dynamically, depending on what PM domain that their original device
> needs for the current running use case.
>
> In that way, they keep existing runtime PM deployment, operating on
> its original device.

 OK, sounds good. Any reason why the linking cannot be handled by the above 
 API? Is there a use-case where you would not want it linked?
>>>
>>> I am guessing the linking is what would give the driver the ability to 
>>> decide which subset of powerdomains it actually wants to control
>>> at any point using runtime PM. If we have cases wherein the driver would 
>>> want to turn on/off _all_ its associated powerdomains _always_
>>> then a default linking of all would help.
>>
>> First, I think we need to decide on *where* the linking should be
>> done, not at both places, as that would just mess up synchronization
>> of who is responsible for calling the device_link_del() at detach.
>>
>> Second, It would in principle be fine to call device_link_add() and
>> device_link_del() as a part of the attach/detach APIs. However, there
>> is a downside to such solution, which would be that the driver then
>> needs call the detach API, just to do device_link_del(). Of course
>> then it would also needs to call the attach API later if/when needed.
>> Doing this adds unnecessary overhead - comparing to just let the
>> driver call device_link_add|del() when needed. On the upside, yes, it
>> would put less burden on the drivers as it then only needs to care
>> about using one set of functions.
>>
>> Which solution do you prefer?
> 
> Any reason why we could not add a 'boolean' argument to the API to indicate 
> whether the new device should be linked? I think that I prefer the API 
> handles it, but I can see there could be instances where drivers may wish to 
> handle it themselves.
> 
> Rajendra, do you have a use-case right now where the driver would want to 
> handle the linking?

So if I understand this right, any driver which does want to control individual 
powerdomain state would
need to do the linking itself right?

What I am saying is, if I have device A, with powerdomains X and Y, and if I 
want to turn on only X,
then I would want only X to be linked with A, and at a later point if I want 
both X and Y to be turned on,
I would then go ahead and link both X and Y to A? Is that correct or did I get 
it all wrong?

I know atleast Camera on msm8996 would need to do this since it has 2 vfe 
powerdoamins, which can be
turned on one at a time (depending on what resolution needs to be supported) or 
both together if we
really need very high resolution using both vfe modules. 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Rajendra Nayak]

On 05/23/2018 02:37 PM, Jon Hunter wrote:
> 
> On 23/05/18 07:12, Ulf Hansson wrote:
> 
> ...
> 
>> Thanks for sending this. Believe it or not this has still been on my 
>> to-do list
>> and so we definitely need a solution for Tegra.
>>
>> Looking at the above it appears that additional power-domains exposed as 
>> devices
>> to the client device. So I assume that this means that the drivers for 
>> devices
>> with multiple power-domains will need to call RPM APIs for each of these
>> additional power-domains. Is that correct?
>
> They can, but should not!
>
> Instead, the driver shall use device_link_add() and device_link_del(),
> dynamically, depending on what PM domain that their original device
> needs for the current running use case.
>
> In that way, they keep existing runtime PM deployment, operating on
> its original device.

 OK, sounds good. Any reason why the linking cannot be handled by the above 
 API? Is there a use-case where you would not want it linked?
>>>
>>> I am guessing the linking is what would give the driver the ability to 
>>> decide which subset of powerdomains it actually wants to control
>>> at any point using runtime PM. If we have cases wherein the driver would 
>>> want to turn on/off _all_ its associated powerdomains _always_
>>> then a default linking of all would help.
>>
>> First, I think we need to decide on *where* the linking should be
>> done, not at both places, as that would just mess up synchronization
>> of who is responsible for calling the device_link_del() at detach.
>>
>> Second, It would in principle be fine to call device_link_add() and
>> device_link_del() as a part of the attach/detach APIs. However, there
>> is a downside to such solution, which would be that the driver then
>> needs call the detach API, just to do device_link_del(). Of course
>> then it would also needs to call the attach API later if/when needed.
>> Doing this adds unnecessary overhead - comparing to just let the
>> driver call device_link_add|del() when needed. On the upside, yes, it
>> would put less burden on the drivers as it then only needs to care
>> about using one set of functions.
>>
>> Which solution do you prefer?
> 
> Any reason why we could not add a 'boolean' argument to the API to indicate 
> whether the new device should be linked? I think that I prefer the API 
> handles it, but I can see there could be instances where drivers may wish to 
> handle it themselves.
> 
> Rajendra, do you have a use-case right now where the driver would want to 
> handle the linking?

So if I understand this right, any driver which does want to control individual 
powerdomain state would
need to do the linking itself right?

What I am saying is, if I have device A, with powerdomains X and Y, and if I 
want to turn on only X,
then I would want only X to be linked with A, and at a later point if I want 
both X and Y to be turned on,
I would then go ahead and link both X and Y to A? Is that correct or did I get 
it all wrong?

I know atleast Camera on msm8996 would need to do this since it has 2 vfe 
powerdoamins, which can be
turned on one at a time (depending on what resolution needs to be supported) or 
both together if we
really need very high resolution using both vfe modules. 

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 23/05/18 07:12, Ulf Hansson wrote:

...


Thanks for sending this. Believe it or not this has still been on my to-do list
and so we definitely need a solution for Tegra.

Looking at the above it appears that additional power-domains exposed as devices
to the client device. So I assume that this means that the drivers for devices
with multiple power-domains will need to call RPM APIs for each of these
additional power-domains. Is that correct?


They can, but should not!

Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.


OK, sounds good. Any reason why the linking cannot be handled by the above API? 
Is there a use-case where you would not want it linked?


I am guessing the linking is what would give the driver the ability to decide 
which subset of powerdomains it actually wants to control
at any point using runtime PM. If we have cases wherein the driver would want 
to turn on/off _all_ its associated powerdomains _always_
then a default linking of all would help.


First, I think we need to decide on *where* the linking should be
done, not at both places, as that would just mess up synchronization
of who is responsible for calling the device_link_del() at detach.

Second, It would in principle be fine to call device_link_add() and
device_link_del() as a part of the attach/detach APIs. However, there
is a downside to such solution, which would be that the driver then
needs call the detach API, just to do device_link_del(). Of course
then it would also needs to call the attach API later if/when needed.
Doing this adds unnecessary overhead - comparing to just let the
driver call device_link_add|del() when needed. On the upside, yes, it
would put less burden on the drivers as it then only needs to care
about using one set of functions.

Which solution do you prefer?


Any reason why we could not add a 'boolean' argument to the API to 
indicate whether the new device should be linked? I think that I prefer 
the API handles it, but I can see there could be instances where drivers 
may wish to handle it themselves.


Rajendra, do you have a use-case right now where the driver would want 
to handle the linking?


Cheers
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 23/05/18 07:12, Ulf Hansson wrote:

...


Thanks for sending this. Believe it or not this has still been on my to-do list
and so we definitely need a solution for Tegra.

Looking at the above it appears that additional power-domains exposed as devices
to the client device. So I assume that this means that the drivers for devices
with multiple power-domains will need to call RPM APIs for each of these
additional power-domains. Is that correct?


They can, but should not!

Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.


OK, sounds good. Any reason why the linking cannot be handled by the above API? 
Is there a use-case where you would not want it linked?


I am guessing the linking is what would give the driver the ability to decide 
which subset of powerdomains it actually wants to control
at any point using runtime PM. If we have cases wherein the driver would want 
to turn on/off _all_ its associated powerdomains _always_
then a default linking of all would help.


First, I think we need to decide on *where* the linking should be
done, not at both places, as that would just mess up synchronization
of who is responsible for calling the device_link_del() at detach.

Second, It would in principle be fine to call device_link_add() and
device_link_del() as a part of the attach/detach APIs. However, there
is a downside to such solution, which would be that the driver then
needs call the detach API, just to do device_link_del(). Of course
then it would also needs to call the attach API later if/when needed.
Doing this adds unnecessary overhead - comparing to just let the
driver call device_link_add|del() when needed. On the upside, yes, it
would put less burden on the drivers as it then only needs to care
about using one set of functions.

Which solution do you prefer?


Any reason why we could not add a 'boolean' argument to the API to 
indicate whether the new device should be linked? I think that I prefer 
the API handles it, but I can see there could be instances where drivers 
may wish to handle it themselves.


Rajendra, do you have a use-case right now where the driver would want 
to handle the linking?


Cheers
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

Rajendra, Jon,

On 23 May 2018 at 06:51, Rajendra Nayak  wrote:
>
>
> On 05/23/2018 02:25 AM, Jon Hunter wrote:
>>
>> On 22/05/18 15:47, Ulf Hansson wrote:
>>> [...]
>>>
>
> +/**
> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> + * @dev: Device to attach.
> + * @index: The index of the PM domain.
> + *
> + * Parse device's OF node to find a PM domain specifier at the provided 
> @index.
> + * If such is found, allocates a new device and attaches it to retrieved
> + * pm_domain ops.
> + *
> + * Returns the allocated device if successfully attached PM domain, NULL 
> when
> + * the device don't need a PM domain or have a single PM domain, else 
> PTR_ERR()
> + * in case of failures. Note that if a power-domain exists for the 
> device, but
> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to 
> ensure
> + * that the device is not probed and to re-try again later.
> + */
> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> +  unsigned int index)
> +{
> + struct device *genpd_dev;
> + int num_domains;
> + int ret;
> +
> + if (!dev->of_node)
> + return NULL;
> +
> + /* Deal only with devices using multiple PM domains. */
> + num_domains = of_count_phandle_with_args(dev->of_node, 
> "power-domains",
> +  "#power-domain-cells");
> + if (num_domains < 2 || index >= num_domains)
> + return NULL;
> +
> + /* Allocate and register device on the genpd bus. */
> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
> + if (!genpd_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
> + genpd_dev->bus = _bus_type;
> + genpd_dev->release = genpd_release_dev;
> +
> + ret = device_register(genpd_dev);
> + if (ret) {
> + kfree(genpd_dev);
> + return ERR_PTR(ret);
> + }
> +
> + /* Try to attach the device to the PM domain at the specified 
> index. */
> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
> + if (ret < 1) {
> + device_unregister(genpd_dev);
> + return ret ? ERR_PTR(ret) : NULL;
> + }
> +
> + pm_runtime_set_active(genpd_dev);
> + pm_runtime_enable(genpd_dev);
> +
> + return genpd_dev;
> +}
> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);

 Thanks for sending this. Believe it or not this has still been on my to-do 
 list
 and so we definitely need a solution for Tegra.

 Looking at the above it appears that additional power-domains exposed as 
 devices
 to the client device. So I assume that this means that the drivers for 
 devices
 with multiple power-domains will need to call RPM APIs for each of these
 additional power-domains. Is that correct?
>>>
>>> They can, but should not!
>>>
>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>> dynamically, depending on what PM domain that their original device
>>> needs for the current running use case.
>>>
>>> In that way, they keep existing runtime PM deployment, operating on
>>> its original device.
>>
>> OK, sounds good. Any reason why the linking cannot be handled by the above 
>> API? Is there a use-case where you would not want it linked?
>
> I am guessing the linking is what would give the driver the ability to decide 
> which subset of powerdomains it actually wants to control
> at any point using runtime PM. If we have cases wherein the driver would want 
> to turn on/off _all_ its associated powerdomains _always_
> then a default linking of all would help.

First, I think we need to decide on *where* the linking should be
done, not at both places, as that would just mess up synchronization
of who is responsible for calling the device_link_del() at detach.

Second, It would in principle be fine to call device_link_add() and
device_link_del() as a part of the attach/detach APIs. However, there
is a downside to such solution, which would be that the driver then
needs call the detach API, just to do device_link_del(). Of course
then it would also needs to call the attach API later if/when needed.
Doing this adds unnecessary overhead - comparing to just let the
driver call device_link_add|del() when needed. On the upside, yes, it
would put less burden on the drivers as it then only needs to care
about using one set of functions.

Which solution do you prefer?

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

Rajendra, Jon,

On 23 May 2018 at 06:51, Rajendra Nayak  wrote:
>
>
> On 05/23/2018 02:25 AM, Jon Hunter wrote:
>>
>> On 22/05/18 15:47, Ulf Hansson wrote:
>>> [...]
>>>
>
> +/**
> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> + * @dev: Device to attach.
> + * @index: The index of the PM domain.
> + *
> + * Parse device's OF node to find a PM domain specifier at the provided 
> @index.
> + * If such is found, allocates a new device and attaches it to retrieved
> + * pm_domain ops.
> + *
> + * Returns the allocated device if successfully attached PM domain, NULL 
> when
> + * the device don't need a PM domain or have a single PM domain, else 
> PTR_ERR()
> + * in case of failures. Note that if a power-domain exists for the 
> device, but
> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to 
> ensure
> + * that the device is not probed and to re-try again later.
> + */
> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> +  unsigned int index)
> +{
> + struct device *genpd_dev;
> + int num_domains;
> + int ret;
> +
> + if (!dev->of_node)
> + return NULL;
> +
> + /* Deal only with devices using multiple PM domains. */
> + num_domains = of_count_phandle_with_args(dev->of_node, 
> "power-domains",
> +  "#power-domain-cells");
> + if (num_domains < 2 || index >= num_domains)
> + return NULL;
> +
> + /* Allocate and register device on the genpd bus. */
> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
> + if (!genpd_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
> + genpd_dev->bus = _bus_type;
> + genpd_dev->release = genpd_release_dev;
> +
> + ret = device_register(genpd_dev);
> + if (ret) {
> + kfree(genpd_dev);
> + return ERR_PTR(ret);
> + }
> +
> + /* Try to attach the device to the PM domain at the specified 
> index. */
> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
> + if (ret < 1) {
> + device_unregister(genpd_dev);
> + return ret ? ERR_PTR(ret) : NULL;
> + }
> +
> + pm_runtime_set_active(genpd_dev);
> + pm_runtime_enable(genpd_dev);
> +
> + return genpd_dev;
> +}
> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);

 Thanks for sending this. Believe it or not this has still been on my to-do 
 list
 and so we definitely need a solution for Tegra.

 Looking at the above it appears that additional power-domains exposed as 
 devices
 to the client device. So I assume that this means that the drivers for 
 devices
 with multiple power-domains will need to call RPM APIs for each of these
 additional power-domains. Is that correct?
>>>
>>> They can, but should not!
>>>
>>> Instead, the driver shall use device_link_add() and device_link_del(),
>>> dynamically, depending on what PM domain that their original device
>>> needs for the current running use case.
>>>
>>> In that way, they keep existing runtime PM deployment, operating on
>>> its original device.
>>
>> OK, sounds good. Any reason why the linking cannot be handled by the above 
>> API? Is there a use-case where you would not want it linked?
>
> I am guessing the linking is what would give the driver the ability to decide 
> which subset of powerdomains it actually wants to control
> at any point using runtime PM. If we have cases wherein the driver would want 
> to turn on/off _all_ its associated powerdomains _always_
> then a default linking of all would help.

First, I think we need to decide on *where* the linking should be
done, not at both places, as that would just mess up synchronization
of who is responsible for calling the device_link_del() at detach.

Second, It would in principle be fine to call device_link_add() and
device_link_del() as a part of the attach/detach APIs. However, there
is a downside to such solution, which would be that the driver then
needs call the detach API, just to do device_link_del(). Of course
then it would also needs to call the attach API later if/when needed.
Doing this adds unnecessary overhead - comparing to just let the
driver call device_link_add|del() when needed. On the upside, yes, it
would put less burden on the drivers as it then only needs to care
about using one set of functions.

Which solution do you prefer?

Kind regards
Uffe

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Rajendra Nayak]

On 05/23/2018 02:25 AM, Jon Hunter wrote:
> 
> On 22/05/18 15:47, Ulf Hansson wrote:
>> [...]
>>

 +/**
 + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
 + * @dev: Device to attach.
 + * @index: The index of the PM domain.
 + *
 + * Parse device's OF node to find a PM domain specifier at the provided 
 @index.
 + * If such is found, allocates a new device and attaches it to retrieved
 + * pm_domain ops.
 + *
 + * Returns the allocated device if successfully attached PM domain, NULL 
 when
 + * the device don't need a PM domain or have a single PM domain, else 
 PTR_ERR()
 + * in case of failures. Note that if a power-domain exists for the 
 device, but
 + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to 
 ensure
 + * that the device is not probed and to re-try again later.
 + */
 +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
 +  unsigned int index)
 +{
 + struct device *genpd_dev;
 + int num_domains;
 + int ret;
 +
 + if (!dev->of_node)
 + return NULL;
 +
 + /* Deal only with devices using multiple PM domains. */
 + num_domains = of_count_phandle_with_args(dev->of_node, 
 "power-domains",
 +  "#power-domain-cells");
 + if (num_domains < 2 || index >= num_domains)
 + return NULL;
 +
 + /* Allocate and register device on the genpd bus. */
 + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
 + if (!genpd_dev)
 + return ERR_PTR(-ENOMEM);
 +
 + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
 + genpd_dev->bus = _bus_type;
 + genpd_dev->release = genpd_release_dev;
 +
 + ret = device_register(genpd_dev);
 + if (ret) {
 + kfree(genpd_dev);
 + return ERR_PTR(ret);
 + }
 +
 + /* Try to attach the device to the PM domain at the specified index. 
 */
 + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
 + if (ret < 1) {
 + device_unregister(genpd_dev);
 + return ret ? ERR_PTR(ret) : NULL;
 + }
 +
 + pm_runtime_set_active(genpd_dev);
 + pm_runtime_enable(genpd_dev);
 +
 + return genpd_dev;
 +}
 +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>>>
>>> Thanks for sending this. Believe it or not this has still been on my to-do 
>>> list
>>> and so we definitely need a solution for Tegra.
>>>
>>> Looking at the above it appears that additional power-domains exposed as 
>>> devices
>>> to the client device. So I assume that this means that the drivers for 
>>> devices
>>> with multiple power-domains will need to call RPM APIs for each of these
>>> additional power-domains. Is that correct?
>>
>> They can, but should not!
>>
>> Instead, the driver shall use device_link_add() and device_link_del(),
>> dynamically, depending on what PM domain that their original device
>> needs for the current running use case.
>>
>> In that way, they keep existing runtime PM deployment, operating on
>> its original device.
> 
> OK, sounds good. Any reason why the linking cannot be handled by the above 
> API? Is there a use-case where you would not want it linked?

I am guessing the linking is what would give the driver the ability to decide 
which subset of powerdomains it actually wants to control
at any point using runtime PM. If we have cases wherein the driver would want 
to turn on/off _all_ its associated powerdomains _always_
then a default linking of all would help.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Rajendra Nayak]

On 05/23/2018 02:25 AM, Jon Hunter wrote:
> 
> On 22/05/18 15:47, Ulf Hansson wrote:
>> [...]
>>

 +/**
 + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
 + * @dev: Device to attach.
 + * @index: The index of the PM domain.
 + *
 + * Parse device's OF node to find a PM domain specifier at the provided 
 @index.
 + * If such is found, allocates a new device and attaches it to retrieved
 + * pm_domain ops.
 + *
 + * Returns the allocated device if successfully attached PM domain, NULL 
 when
 + * the device don't need a PM domain or have a single PM domain, else 
 PTR_ERR()
 + * in case of failures. Note that if a power-domain exists for the 
 device, but
 + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to 
 ensure
 + * that the device is not probed and to re-try again later.
 + */
 +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
 +  unsigned int index)
 +{
 + struct device *genpd_dev;
 + int num_domains;
 + int ret;
 +
 + if (!dev->of_node)
 + return NULL;
 +
 + /* Deal only with devices using multiple PM domains. */
 + num_domains = of_count_phandle_with_args(dev->of_node, 
 "power-domains",
 +  "#power-domain-cells");
 + if (num_domains < 2 || index >= num_domains)
 + return NULL;
 +
 + /* Allocate and register device on the genpd bus. */
 + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
 + if (!genpd_dev)
 + return ERR_PTR(-ENOMEM);
 +
 + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
 + genpd_dev->bus = _bus_type;
 + genpd_dev->release = genpd_release_dev;
 +
 + ret = device_register(genpd_dev);
 + if (ret) {
 + kfree(genpd_dev);
 + return ERR_PTR(ret);
 + }
 +
 + /* Try to attach the device to the PM domain at the specified index. 
 */
 + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
 + if (ret < 1) {
 + device_unregister(genpd_dev);
 + return ret ? ERR_PTR(ret) : NULL;
 + }
 +
 + pm_runtime_set_active(genpd_dev);
 + pm_runtime_enable(genpd_dev);
 +
 + return genpd_dev;
 +}
 +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>>>
>>> Thanks for sending this. Believe it or not this has still been on my to-do 
>>> list
>>> and so we definitely need a solution for Tegra.
>>>
>>> Looking at the above it appears that additional power-domains exposed as 
>>> devices
>>> to the client device. So I assume that this means that the drivers for 
>>> devices
>>> with multiple power-domains will need to call RPM APIs for each of these
>>> additional power-domains. Is that correct?
>>
>> They can, but should not!
>>
>> Instead, the driver shall use device_link_add() and device_link_del(),
>> dynamically, depending on what PM domain that their original device
>> needs for the current running use case.
>>
>> In that way, they keep existing runtime PM deployment, operating on
>> its original device.
> 
> OK, sounds good. Any reason why the linking cannot be handled by the above 
> API? Is there a use-case where you would not want it linked?

I am guessing the linking is what would give the driver the ability to decide 
which subset of powerdomains it actually wants to control
at any point using runtime PM. If we have cases wherein the driver would want 
to turn on/off _all_ its associated powerdomains _always_
then a default linking of all would help.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 22/05/18 15:47, Ulf Hansson wrote:

[...]



+/**
+ * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
+ * @dev: Device to attach.
+ * @index: The index of the PM domain.
+ *
+ * Parse device's OF node to find a PM domain specifier at the provided @index.
+ * If such is found, allocates a new device and attaches it to retrieved
+ * pm_domain ops.
+ *
+ * Returns the allocated device if successfully attached PM domain, NULL when
+ * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
+ * in case of failures. Note that if a power-domain exists for the device, but
+ * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
+ * that the device is not probed and to re-try again later.
+ */
+struct device *genpd_dev_pm_attach_by_id(struct device *dev,
+  unsigned int index)
+{
+ struct device *genpd_dev;
+ int num_domains;
+ int ret;
+
+ if (!dev->of_node)
+ return NULL;
+
+ /* Deal only with devices using multiple PM domains. */
+ num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
+  "#power-domain-cells");
+ if (num_domains < 2 || index >= num_domains)
+ return NULL;
+
+ /* Allocate and register device on the genpd bus. */
+ genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
+ if (!genpd_dev)
+ return ERR_PTR(-ENOMEM);
+
+ dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
+ genpd_dev->bus = _bus_type;
+ genpd_dev->release = genpd_release_dev;
+
+ ret = device_register(genpd_dev);
+ if (ret) {
+ kfree(genpd_dev);
+ return ERR_PTR(ret);
+ }
+
+ /* Try to attach the device to the PM domain at the specified index. */
+ ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
+ if (ret < 1) {
+ device_unregister(genpd_dev);
+ return ret ? ERR_PTR(ret) : NULL;
+ }
+
+ pm_runtime_set_active(genpd_dev);
+ pm_runtime_enable(genpd_dev);
+
+ return genpd_dev;
+}
+EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);


Thanks for sending this. Believe it or not this has still been on my to-do list
and so we definitely need a solution for Tegra.

Looking at the above it appears that additional power-domains exposed as devices
to the client device. So I assume that this means that the drivers for devices
with multiple power-domains will need to call RPM APIs for each of these
additional power-domains. Is that correct?


They can, but should not!

Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.


OK, sounds good. Any reason why the linking cannot be handled by the 
above API? Is there a use-case where you would not want it linked?


Thanks
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

On 22/05/18 15:47, Ulf Hansson wrote:

[...]



+/**
+ * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
+ * @dev: Device to attach.
+ * @index: The index of the PM domain.
+ *
+ * Parse device's OF node to find a PM domain specifier at the provided @index.
+ * If such is found, allocates a new device and attaches it to retrieved
+ * pm_domain ops.
+ *
+ * Returns the allocated device if successfully attached PM domain, NULL when
+ * the device don't need a PM domain or have a single PM domain, else PTR_ERR()
+ * in case of failures. Note that if a power-domain exists for the device, but
+ * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
+ * that the device is not probed and to re-try again later.
+ */
+struct device *genpd_dev_pm_attach_by_id(struct device *dev,
+  unsigned int index)
+{
+ struct device *genpd_dev;
+ int num_domains;
+ int ret;
+
+ if (!dev->of_node)
+ return NULL;
+
+ /* Deal only with devices using multiple PM domains. */
+ num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
+  "#power-domain-cells");
+ if (num_domains < 2 || index >= num_domains)
+ return NULL;
+
+ /* Allocate and register device on the genpd bus. */
+ genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
+ if (!genpd_dev)
+ return ERR_PTR(-ENOMEM);
+
+ dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
+ genpd_dev->bus = _bus_type;
+ genpd_dev->release = genpd_release_dev;
+
+ ret = device_register(genpd_dev);
+ if (ret) {
+ kfree(genpd_dev);
+ return ERR_PTR(ret);
+ }
+
+ /* Try to attach the device to the PM domain at the specified index. */
+ ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
+ if (ret < 1) {
+ device_unregister(genpd_dev);
+ return ret ? ERR_PTR(ret) : NULL;
+ }
+
+ pm_runtime_set_active(genpd_dev);
+ pm_runtime_enable(genpd_dev);
+
+ return genpd_dev;
+}
+EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);


Thanks for sending this. Believe it or not this has still been on my to-do list
and so we definitely need a solution for Tegra.

Looking at the above it appears that additional power-domains exposed as devices
to the client device. So I assume that this means that the drivers for devices
with multiple power-domains will need to call RPM APIs for each of these
additional power-domains. Is that correct?


They can, but should not!

Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.


OK, sounds good. Any reason why the linking cannot be handled by the 
above API? Is there a use-case where you would not want it linked?


Thanks
Jon

--
nvpublic

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

[...]

>>
>> +/**
>> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>> + * @dev: Device to attach.
>> + * @index: The index of the PM domain.
>> + *
>> + * Parse device's OF node to find a PM domain specifier at the provided 
>> @index.
>> + * If such is found, allocates a new device and attaches it to retrieved
>> + * pm_domain ops.
>> + *
>> + * Returns the allocated device if successfully attached PM domain, NULL 
>> when
>> + * the device don't need a PM domain or have a single PM domain, else 
>> PTR_ERR()
>> + * in case of failures. Note that if a power-domain exists for the device, 
>> but
>> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to 
>> ensure
>> + * that the device is not probed and to re-try again later.
>> + */
>> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>> +  unsigned int index)
>> +{
>> + struct device *genpd_dev;
>> + int num_domains;
>> + int ret;
>> +
>> + if (!dev->of_node)
>> + return NULL;
>> +
>> + /* Deal only with devices using multiple PM domains. */
>> + num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
>> +  "#power-domain-cells");
>> + if (num_domains < 2 || index >= num_domains)
>> + return NULL;
>> +
>> + /* Allocate and register device on the genpd bus. */
>> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
>> + if (!genpd_dev)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
>> + genpd_dev->bus = _bus_type;
>> + genpd_dev->release = genpd_release_dev;
>> +
>> + ret = device_register(genpd_dev);
>> + if (ret) {
>> + kfree(genpd_dev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + /* Try to attach the device to the PM domain at the specified index. */
>> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
>> + if (ret < 1) {
>> + device_unregister(genpd_dev);
>> + return ret ? ERR_PTR(ret) : NULL;
>> + }
>> +
>> + pm_runtime_set_active(genpd_dev);
>> + pm_runtime_enable(genpd_dev);
>> +
>> + return genpd_dev;
>> +}
>> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>
> Thanks for sending this. Believe it or not this has still been on my to-do 
> list
> and so we definitely need a solution for Tegra.
>
> Looking at the above it appears that additional power-domains exposed as 
> devices
> to the client device. So I assume that this means that the drivers for devices
> with multiple power-domains will need to call RPM APIs for each of these
> additional power-domains. Is that correct?

They can, but should not!

Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.

>
> If so, I can see that that would work, but it does not seem to fit the RPM 
> model
> where currently for something like device clocks, the RPM callbacks can handle
> all clocks at once.
>
> I was wondering why we could not add a list of genpd domains to the
> dev_pm_domain struct for the device? For example ...

See above answer, hopefully that explains it.

FYI, that's why I also discovered the bug around using device links
with runtime PM during probe.
https://patchwork.kernel.org/patch/10408825/

>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e723b78d8357..a11d6db3c077 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -659,6 +659,7 @@ extern void dev_pm_put_subsys_data(struct device *dev);
>   * subsystem-level and driver-level callbacks.
>   */
>  struct dev_pm_domain {
> +   struct list_headgenpd_list;
> struct dev_pm_ops   ops;
> void (*detach)(struct device *dev, bool power_off);
> int (*activate)(struct device *dev);
> @@ -666,6 +667,11 @@ struct dev_pm_domain {
> void (*dismiss)(struct device *dev);
>  };
>
> +struct dev_pm_domain_link {
> +   struct generic_pm_domain *genpd;
> +   struct list_head node;
> +};
> +
>  /*
>   * The PM_EVENT_ messages are also used by drivers implementing the legacy
>   * suspend framework, based on the ->suspend() and ->resume() callbacks 
> common
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index e61b5cd79fe2..019593804670 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -51,7 +51,6 @@ struct dev_pm_opp;
>
>  struct generic_pm_domain {
> struct device dev;
> -   struct dev_pm_domain domain;/* PM domain operations */
> struct list_head gpd_list_node; /* Node in the global PM domains list 
> */
> struct list_head master_links;  /* Links with PM domain as a master */
>  

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Ulf Hansson]

[...]

>>
>> +/**
>> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
>> + * @dev: Device to attach.
>> + * @index: The index of the PM domain.
>> + *
>> + * Parse device's OF node to find a PM domain specifier at the provided 
>> @index.
>> + * If such is found, allocates a new device and attaches it to retrieved
>> + * pm_domain ops.
>> + *
>> + * Returns the allocated device if successfully attached PM domain, NULL 
>> when
>> + * the device don't need a PM domain or have a single PM domain, else 
>> PTR_ERR()
>> + * in case of failures. Note that if a power-domain exists for the device, 
>> but
>> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to 
>> ensure
>> + * that the device is not probed and to re-try again later.
>> + */
>> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
>> +  unsigned int index)
>> +{
>> + struct device *genpd_dev;
>> + int num_domains;
>> + int ret;
>> +
>> + if (!dev->of_node)
>> + return NULL;
>> +
>> + /* Deal only with devices using multiple PM domains. */
>> + num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
>> +  "#power-domain-cells");
>> + if (num_domains < 2 || index >= num_domains)
>> + return NULL;
>> +
>> + /* Allocate and register device on the genpd bus. */
>> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
>> + if (!genpd_dev)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
>> + genpd_dev->bus = _bus_type;
>> + genpd_dev->release = genpd_release_dev;
>> +
>> + ret = device_register(genpd_dev);
>> + if (ret) {
>> + kfree(genpd_dev);
>> + return ERR_PTR(ret);
>> + }
>> +
>> + /* Try to attach the device to the PM domain at the specified index. */
>> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
>> + if (ret < 1) {
>> + device_unregister(genpd_dev);
>> + return ret ? ERR_PTR(ret) : NULL;
>> + }
>> +
>> + pm_runtime_set_active(genpd_dev);
>> + pm_runtime_enable(genpd_dev);
>> +
>> + return genpd_dev;
>> +}
>> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);
>
> Thanks for sending this. Believe it or not this has still been on my to-do 
> list
> and so we definitely need a solution for Tegra.
>
> Looking at the above it appears that additional power-domains exposed as 
> devices
> to the client device. So I assume that this means that the drivers for devices
> with multiple power-domains will need to call RPM APIs for each of these
> additional power-domains. Is that correct?

They can, but should not!

Instead, the driver shall use device_link_add() and device_link_del(),
dynamically, depending on what PM domain that their original device
needs for the current running use case.

In that way, they keep existing runtime PM deployment, operating on
its original device.

>
> If so, I can see that that would work, but it does not seem to fit the RPM 
> model
> where currently for something like device clocks, the RPM callbacks can handle
> all clocks at once.
>
> I was wondering why we could not add a list of genpd domains to the
> dev_pm_domain struct for the device? For example ...

See above answer, hopefully that explains it.

FYI, that's why I also discovered the bug around using device links
with runtime PM during probe.
https://patchwork.kernel.org/patch/10408825/

>
> diff --git a/include/linux/pm.h b/include/linux/pm.h
> index e723b78d8357..a11d6db3c077 100644
> --- a/include/linux/pm.h
> +++ b/include/linux/pm.h
> @@ -659,6 +659,7 @@ extern void dev_pm_put_subsys_data(struct device *dev);
>   * subsystem-level and driver-level callbacks.
>   */
>  struct dev_pm_domain {
> +   struct list_headgenpd_list;
> struct dev_pm_ops   ops;
> void (*detach)(struct device *dev, bool power_off);
> int (*activate)(struct device *dev);
> @@ -666,6 +667,11 @@ struct dev_pm_domain {
> void (*dismiss)(struct device *dev);
>  };
>
> +struct dev_pm_domain_link {
> +   struct generic_pm_domain *genpd;
> +   struct list_head node;
> +};
> +
>  /*
>   * The PM_EVENT_ messages are also used by drivers implementing the legacy
>   * suspend framework, based on the ->suspend() and ->resume() callbacks 
> common
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index e61b5cd79fe2..019593804670 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -51,7 +51,6 @@ struct dev_pm_opp;
>
>  struct generic_pm_domain {
> struct device dev;
> -   struct dev_pm_domain domain;/* PM domain operations */
> struct list_head gpd_list_node; /* Node in the global PM domains list 
> */
> struct list_head master_links;  /* Links with PM domain as a master */
>  

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

Hi Ulf,

On 18/05/18 11:31, Ulf Hansson wrote:
> To support devices being partitioned across multiple PM domains, let's
> start by extending genpd to cope with these configurations.
> 
> More precisely, add a new exported function, genpd_dev_pm_attach_by_id(),
> similar to genpd_dev_pm_attach(), but the new function also allows the
> caller to provide an index to what PM domain it wants to attach.
> 
> Furthermore, let genpd register a new virtual struct device via calling
> device_register() and attach it to the corresponding PM domain, which is
> looked up via calling the existing genpd OF functions. Note that the new
> device is needed, because only one PM domain can be attached per device. At
> successful attachment, genpd_dev_pm_attach_by_id() returns the new device,
> allowing the caller to operate on it to deal with power management.
> 
> To deal with detaching of a PM domain for multiple PM domain case, we can
> still re-use the existing genpd_dev_pm_detach() function, although we need
> to extend it to cover cleanup of the earlier registered device, via calling
> device_unregister().
> 
> An important note, genpd_dev_pm_attach_by_id() shall only be called by the
> driver core / PM core, similar to how genpd_dev_pm_attach() is used.
> Following changes deploys this.
> 
> Signed-off-by: Ulf Hansson 
> ---
>   drivers/base/power/domain.c | 79 
> +
>   include/linux/pm_domain.h   |  8 +
>   2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index d538640..ffeb6ea 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2171,6 +2171,15 @@ struct generic_pm_domain *of_genpd_remove_last(struct 
> device_node *np)
>   }
>   EXPORT_SYMBOL_GPL(of_genpd_remove_last);
>   
> +static void genpd_release_dev(struct device *dev)
> +{
> + kfree(dev);
> +}
> +
> +static struct bus_type genpd_bus_type = {
> + .name   = "genpd",
> +};
> +
>   /**
>* genpd_dev_pm_detach - Detach a device from its PM domain.
>* @dev: Device to detach.
> @@ -2208,6 +2217,10 @@ static void genpd_dev_pm_detach(struct device *dev, 
> bool power_off)
>   
>   /* Check if PM domain can be powered off after removing this device. */
>   genpd_queue_power_off_work(pd);
> +
> + /* Unregister the device if it was created by genpd. */
> + if (dev->bus == _bus_type)
> + device_unregister(dev);
>   }
>   
>   static void genpd_dev_pm_sync(struct device *dev)
> @@ -2298,6 +2311,66 @@ int genpd_dev_pm_attach(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>   
> +/**
> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> + * @dev: Device to attach.
> + * @index: The index of the PM domain.
> + *
> + * Parse device's OF node to find a PM domain specifier at the provided 
> @index.
> + * If such is found, allocates a new device and attaches it to retrieved
> + * pm_domain ops.
> + *
> + * Returns the allocated device if successfully attached PM domain, NULL when
> + * the device don't need a PM domain or have a single PM domain, else 
> PTR_ERR()
> + * in case of failures. Note that if a power-domain exists for the device, 
> but
> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
> + * that the device is not probed and to re-try again later.
> + */
> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> +  unsigned int index)
> +{
> + struct device *genpd_dev;
> + int num_domains;
> + int ret;
> +
> + if (!dev->of_node)
> + return NULL;
> +
> + /* Deal only with devices using multiple PM domains. */
> + num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
> +  "#power-domain-cells");
> + if (num_domains < 2 || index >= num_domains)
> + return NULL;
> +
> + /* Allocate and register device on the genpd bus. */
> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
> + if (!genpd_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
> + genpd_dev->bus = _bus_type;
> + genpd_dev->release = genpd_release_dev;
> +
> + ret = device_register(genpd_dev);
> + if (ret) {
> + kfree(genpd_dev);
> + return ERR_PTR(ret);
> + }
> +
> + /* Try to attach the device to the PM domain at the specified index. */
> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
> + if (ret < 1) {
> + device_unregister(genpd_dev);
> + return ret ? ERR_PTR(ret) : NULL;
> + }
> +
> + pm_runtime_set_active(genpd_dev);
> + pm_runtime_enable(genpd_dev);
> +
> + return genpd_dev;
> +}
> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);

Thanks for sending this. Believe it 

Re: [PATCH 8/9] PM / Domains: Add support for multi PM domains per device to genpd

[Jon Hunter]

Hi Ulf,

On 18/05/18 11:31, Ulf Hansson wrote:
> To support devices being partitioned across multiple PM domains, let's
> start by extending genpd to cope with these configurations.
> 
> More precisely, add a new exported function, genpd_dev_pm_attach_by_id(),
> similar to genpd_dev_pm_attach(), but the new function also allows the
> caller to provide an index to what PM domain it wants to attach.
> 
> Furthermore, let genpd register a new virtual struct device via calling
> device_register() and attach it to the corresponding PM domain, which is
> looked up via calling the existing genpd OF functions. Note that the new
> device is needed, because only one PM domain can be attached per device. At
> successful attachment, genpd_dev_pm_attach_by_id() returns the new device,
> allowing the caller to operate on it to deal with power management.
> 
> To deal with detaching of a PM domain for multiple PM domain case, we can
> still re-use the existing genpd_dev_pm_detach() function, although we need
> to extend it to cover cleanup of the earlier registered device, via calling
> device_unregister().
> 
> An important note, genpd_dev_pm_attach_by_id() shall only be called by the
> driver core / PM core, similar to how genpd_dev_pm_attach() is used.
> Following changes deploys this.
> 
> Signed-off-by: Ulf Hansson 
> ---
>   drivers/base/power/domain.c | 79 
> +
>   include/linux/pm_domain.h   |  8 +
>   2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index d538640..ffeb6ea 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -2171,6 +2171,15 @@ struct generic_pm_domain *of_genpd_remove_last(struct 
> device_node *np)
>   }
>   EXPORT_SYMBOL_GPL(of_genpd_remove_last);
>   
> +static void genpd_release_dev(struct device *dev)
> +{
> + kfree(dev);
> +}
> +
> +static struct bus_type genpd_bus_type = {
> + .name   = "genpd",
> +};
> +
>   /**
>* genpd_dev_pm_detach - Detach a device from its PM domain.
>* @dev: Device to detach.
> @@ -2208,6 +2217,10 @@ static void genpd_dev_pm_detach(struct device *dev, 
> bool power_off)
>   
>   /* Check if PM domain can be powered off after removing this device. */
>   genpd_queue_power_off_work(pd);
> +
> + /* Unregister the device if it was created by genpd. */
> + if (dev->bus == _bus_type)
> + device_unregister(dev);
>   }
>   
>   static void genpd_dev_pm_sync(struct device *dev)
> @@ -2298,6 +2311,66 @@ int genpd_dev_pm_attach(struct device *dev)
>   }
>   EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
>   
> +/**
> + * genpd_dev_pm_attach_by_id() - Attach a device to one of its PM domain.
> + * @dev: Device to attach.
> + * @index: The index of the PM domain.
> + *
> + * Parse device's OF node to find a PM domain specifier at the provided 
> @index.
> + * If such is found, allocates a new device and attaches it to retrieved
> + * pm_domain ops.
> + *
> + * Returns the allocated device if successfully attached PM domain, NULL when
> + * the device don't need a PM domain or have a single PM domain, else 
> PTR_ERR()
> + * in case of failures. Note that if a power-domain exists for the device, 
> but
> + * cannot be found or turned on, then return PTR_ERR(-EPROBE_DEFER) to ensure
> + * that the device is not probed and to re-try again later.
> + */
> +struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> +  unsigned int index)
> +{
> + struct device *genpd_dev;
> + int num_domains;
> + int ret;
> +
> + if (!dev->of_node)
> + return NULL;
> +
> + /* Deal only with devices using multiple PM domains. */
> + num_domains = of_count_phandle_with_args(dev->of_node, "power-domains",
> +  "#power-domain-cells");
> + if (num_domains < 2 || index >= num_domains)
> + return NULL;
> +
> + /* Allocate and register device on the genpd bus. */
> + genpd_dev = kzalloc(sizeof(*genpd_dev), GFP_KERNEL);
> + if (!genpd_dev)
> + return ERR_PTR(-ENOMEM);
> +
> + dev_set_name(genpd_dev, "genpd:%u:%s", index, dev_name(dev));
> + genpd_dev->bus = _bus_type;
> + genpd_dev->release = genpd_release_dev;
> +
> + ret = device_register(genpd_dev);
> + if (ret) {
> + kfree(genpd_dev);
> + return ERR_PTR(ret);
> + }
> +
> + /* Try to attach the device to the PM domain at the specified index. */
> + ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index);
> + if (ret < 1) {
> + device_unregister(genpd_dev);
> + return ret ? ERR_PTR(ret) : NULL;
> + }
> +
> + pm_runtime_set_active(genpd_dev);
> + pm_runtime_enable(genpd_dev);
> +
> + return genpd_dev;
> +}
> +EXPORT_SYMBOL_GPL(genpd_dev_pm_attach_by_id);

Thanks for sending this. Believe it or not this has still