[02/24] drivercore: Bind/unbind power domain on probe/remove

Message ID 1402397497-26737-3-git-send-email-ulf.hansson@linaro.org
State New
Headers show

Commit Message

Ulf Hansson June 10, 2014, 10:51 a.m.
From: Tomasz Figa <t.figa@samsung.com>

On a number of platforms, devices are part of controllable power
domains, which need to be enabled before such devices can be accessed
and may be powered down when the device is idle to save some power.
This means that on systems that support power domain control using
generic power domains subsystem, it is necessary to add device to its
power domain before binding a driver to it and remove it from its power
domain after its driver is unbound to make sure that an unused device
does not affect power domain state.

Since this is not limited to particular busses and specific
archs/platforms, it is more convenient to do the above directly in
driver core, just as done with pinctrl default configuration. This patch
adds necessary code to really_probe() and __device_release_driver() to
achieve this and maintain consistent stack-like ordering of operations
happening when binding and unbinding a driver.

Signed-off-by: Tomasz Figa <t.figa@samsung.com>
Reviewed-by: Stephen Boyd <sboyd@codeaurora.org>
Reviewed-by: Philipp Zabel <philipp.zabel@gmail.com>
[on i.MX6 GK802]
Tested-by: Philipp Zabel <philipp.zabel@gmail.com>
Reviewed-by: Mark Brown <broonie@linaro.org>
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
---
 drivers/base/dd.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Comments

Rafael J. Wysocki June 10, 2014, 12:11 p.m. | #1
On Tue, Jun 10, 2014 at 12:51 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> From: Tomasz Figa <t.figa@samsung.com>
>
> On a number of platforms, devices are part of controllable power
> domains, which need to be enabled before such devices can be accessed
> and may be powered down when the device is idle to save some power.
> This means that on systems that support power domain control using
> generic power domains subsystem, it is necessary to add device to its
> power domain before binding a driver to it and remove it from its power
> domain after its driver is unbound to make sure that an unused device
> does not affect power domain state.
>
> Since this is not limited to particular busses and specific
> archs/platforms,

Actually, this isn't correrct.  It is limited to the platforms that
use Device Trees now.

Moreover, it is not consistent with the way we add devices to the ACPI PM
domain, which is the ACPI counterpart of this.

Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson June 10, 2014, 12:53 p.m. | #2
On 10 June 2014 14:11, Rafael J. Wysocki <rafael@kernel.org> wrote:
> On Tue, Jun 10, 2014 at 12:51 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>> From: Tomasz Figa <t.figa@samsung.com>
>>
>> On a number of platforms, devices are part of controllable power
>> domains, which need to be enabled before such devices can be accessed
>> and may be powered down when the device is idle to save some power.
>> This means that on systems that support power domain control using
>> generic power domains subsystem, it is necessary to add device to its
>> power domain before binding a driver to it and remove it from its power
>> domain after its driver is unbound to make sure that an unused device
>> does not affect power domain state.
>>
>> Since this is not limited to particular busses and specific
>> archs/platforms,
>
> Actually, this isn't correrct.  It is limited to the platforms that
> use Device Trees now.

Correct, we should update the commit message/docs.

>
> Moreover, it is not consistent with the way we add devices to the ACPI PM
> domain, which is the ACPI counterpart of this.

I am not sure why you think consistency for ACPI is important here.
ACPI PM will still be able to handle it's domain/device registering as
before. There are even other pm_domains that don't use genpd which
need to handle this themselves.

Or are you saying that you prefer bus notifiers in favour of making
use of the driver core for this matter? Shouldn't the driver core
handle most of the common things for a device driver? Let's compare
how the pinctrls are being managed in the driver core, for example.

Kind regards
Ulf Hansson

>
> Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Greg KH June 10, 2014, 9:27 p.m. | #3
On Tue, Jun 10, 2014 at 11:27:45PM +0200, Rafael J. Wysocki wrote:
> On Tuesday, June 10, 2014 02:53:26 PM Ulf Hansson wrote:
> > On 10 June 2014 14:11, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > On Tue, Jun 10, 2014 at 12:51 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> > >> From: Tomasz Figa <t.figa@samsung.com>
> > >>
> > >> On a number of platforms, devices are part of controllable power
> > >> domains, which need to be enabled before such devices can be accessed
> > >> and may be powered down when the device is idle to save some power.
> > >> This means that on systems that support power domain control using
> > >> generic power domains subsystem, it is necessary to add device to its
> > >> power domain before binding a driver to it and remove it from its power
> > >> domain after its driver is unbound to make sure that an unused device
> > >> does not affect power domain state.
> > >>
> > >> Since this is not limited to particular busses and specific
> > >> archs/platforms,
> > >
> > > Actually, this isn't correrct.  It is limited to the platforms that
> > > use Device Trees now.
> > 
> > Correct, we should update the commit message/docs.
> > 
> > >
> > > Moreover, it is not consistent with the way we add devices to the ACPI PM
> > > domain, which is the ACPI counterpart of this.
> > 
> > I am not sure why you think consistency for ACPI is important here.
> > ACPI PM will still be able to handle it's domain/device registering as
> > before. There are even other pm_domains that don't use genpd which
> > need to handle this themselves.
> 
> My point is that doing things like that in different places for different
> firmware interfaces is confusing and likely to lead to coding mistakes in
> the future.
> 
> > Or are you saying that you prefer bus notifiers in favour of making
> > use of the driver core for this matter?
> 
> Well, please grep for acpi_dev_pm_attach() and see where it is done.
> Surely not in drivers/base/dd.c.  Also I'm not sure why you're talking
> about bus notifiers in this context.
> 
> > Shouldn't the driver core handle most of the common things for a device
> > driver?
> 
> Common, yes.  Platform-specific, no.
> 
> > Let's compare how the pinctrls are being managed in the driver core, for
> > example.
> 
> pinctrl has Device Trees support only at the moment (as far as firmware
> interfaces go) and quite frankly I'm not sure if/how we'll need to change
> it to cover ACPI as well.
> 
> But for power domains, please keep that stuff away from dd.c.  That is,
> unless Greg specifically disagrees with me and decides to apply this
> patch regardless. :-)

Nope, no disagreement from me toward you at all here, keep up the good
work :)

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki June 10, 2014, 9:27 p.m. | #4
On Tuesday, June 10, 2014 02:53:26 PM Ulf Hansson wrote:
> On 10 June 2014 14:11, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > On Tue, Jun 10, 2014 at 12:51 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >> From: Tomasz Figa <t.figa@samsung.com>
> >>
> >> On a number of platforms, devices are part of controllable power
> >> domains, which need to be enabled before such devices can be accessed
> >> and may be powered down when the device is idle to save some power.
> >> This means that on systems that support power domain control using
> >> generic power domains subsystem, it is necessary to add device to its
> >> power domain before binding a driver to it and remove it from its power
> >> domain after its driver is unbound to make sure that an unused device
> >> does not affect power domain state.
> >>
> >> Since this is not limited to particular busses and specific
> >> archs/platforms,
> >
> > Actually, this isn't correrct.  It is limited to the platforms that
> > use Device Trees now.
> 
> Correct, we should update the commit message/docs.
> 
> >
> > Moreover, it is not consistent with the way we add devices to the ACPI PM
> > domain, which is the ACPI counterpart of this.
> 
> I am not sure why you think consistency for ACPI is important here.
> ACPI PM will still be able to handle it's domain/device registering as
> before. There are even other pm_domains that don't use genpd which
> need to handle this themselves.

My point is that doing things like that in different places for different
firmware interfaces is confusing and likely to lead to coding mistakes in
the future.

> Or are you saying that you prefer bus notifiers in favour of making
> use of the driver core for this matter?

Well, please grep for acpi_dev_pm_attach() and see where it is done.
Surely not in drivers/base/dd.c.  Also I'm not sure why you're talking
about bus notifiers in this context.

> Shouldn't the driver core handle most of the common things for a device
> driver?

Common, yes.  Platform-specific, no.

> Let's compare how the pinctrls are being managed in the driver core, for
> example.

pinctrl has Device Trees support only at the moment (as far as firmware
interfaces go) and quite frankly I'm not sure if/how we'll need to change
it to cover ACPI as well.

But for power domains, please keep that stuff away from dd.c.  That is,
unless Greg specifically disagrees with me and decides to apply this
patch regardless. :-)

Rafael

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa June 10, 2014, 9:42 p.m. | #5
On 10.06.2014 23:27, Greg Kroah-Hartman wrote:
> On Tue, Jun 10, 2014 at 11:27:45PM +0200, Rafael J. Wysocki wrote:
>> On Tuesday, June 10, 2014 02:53:26 PM Ulf Hansson wrote:
>>> On 10 June 2014 14:11, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>> On Tue, Jun 10, 2014 at 12:51 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>> From: Tomasz Figa <t.figa@samsung.com>
>>>>>
>>>>> On a number of platforms, devices are part of controllable power
>>>>> domains, which need to be enabled before such devices can be accessed
>>>>> and may be powered down when the device is idle to save some power.
>>>>> This means that on systems that support power domain control using
>>>>> generic power domains subsystem, it is necessary to add device to its
>>>>> power domain before binding a driver to it and remove it from its power
>>>>> domain after its driver is unbound to make sure that an unused device
>>>>> does not affect power domain state.
>>>>>
>>>>> Since this is not limited to particular busses and specific
>>>>> archs/platforms,
>>>>
>>>> Actually, this isn't correrct.  It is limited to the platforms that
>>>> use Device Trees now.
>>>
>>> Correct, we should update the commit message/docs.
>>>
>>>>
>>>> Moreover, it is not consistent with the way we add devices to the ACPI PM
>>>> domain, which is the ACPI counterpart of this.
>>>
>>> I am not sure why you think consistency for ACPI is important here.
>>> ACPI PM will still be able to handle it's domain/device registering as
>>> before. There are even other pm_domains that don't use genpd which
>>> need to handle this themselves.
>>
>> My point is that doing things like that in different places for different
>> firmware interfaces is confusing and likely to lead to coding mistakes in
>> the future.
>>
>>> Or are you saying that you prefer bus notifiers in favour of making
>>> use of the driver core for this matter?
>>
>> Well, please grep for acpi_dev_pm_attach() and see where it is done.
>> Surely not in drivers/base/dd.c.  Also I'm not sure why you're talking
>> about bus notifiers in this context.
>>
>>> Shouldn't the driver core handle most of the common things for a device
>>> driver?
>>
>> Common, yes.  Platform-specific, no.
>>
>>> Let's compare how the pinctrls are being managed in the driver core, for
>>> example.
>>
>> pinctrl has Device Trees support only at the moment (as far as firmware
>> interfaces go) and quite frankly I'm not sure if/how we'll need to change
>> it to cover ACPI as well.
>>
>> But for power domains, please keep that stuff away from dd.c.  That is,
>> unless Greg specifically disagrees with me and decides to apply this
>> patch regardless. :-)
> 
> Nope, no disagreement from me toward you at all here, keep up the good
> work :)

OK, so proposed solution is to put this in:

- platform_drv_probe(),
- spi_drv_probe(),
- i2c_device_probe(),
- amba_probe(),

...

- and any other bus type, which can have devices instantiated from DT.

If this is what you mean, I still think putting this in dd is cleaner
and more scalable, but I'm not going to insist, as I believe you have
good reasons to prefer this approach over current one.

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ulf Hansson June 10, 2014, 10:15 p.m. | #6
On 10 June 2014 23:42, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On 10.06.2014 23:27, Greg Kroah-Hartman wrote:
>> On Tue, Jun 10, 2014 at 11:27:45PM +0200, Rafael J. Wysocki wrote:
>>> On Tuesday, June 10, 2014 02:53:26 PM Ulf Hansson wrote:
>>>> On 10 June 2014 14:11, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>>> On Tue, Jun 10, 2014 at 12:51 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>> From: Tomasz Figa <t.figa@samsung.com>
>>>>>>
>>>>>> On a number of platforms, devices are part of controllable power
>>>>>> domains, which need to be enabled before such devices can be accessed
>>>>>> and may be powered down when the device is idle to save some power.
>>>>>> This means that on systems that support power domain control using
>>>>>> generic power domains subsystem, it is necessary to add device to its
>>>>>> power domain before binding a driver to it and remove it from its power
>>>>>> domain after its driver is unbound to make sure that an unused device
>>>>>> does not affect power domain state.
>>>>>>
>>>>>> Since this is not limited to particular busses and specific
>>>>>> archs/platforms,
>>>>>
>>>>> Actually, this isn't correrct.  It is limited to the platforms that
>>>>> use Device Trees now.
>>>>
>>>> Correct, we should update the commit message/docs.
>>>>
>>>>>
>>>>> Moreover, it is not consistent with the way we add devices to the ACPI PM
>>>>> domain, which is the ACPI counterpart of this.
>>>>
>>>> I am not sure why you think consistency for ACPI is important here.
>>>> ACPI PM will still be able to handle it's domain/device registering as
>>>> before. There are even other pm_domains that don't use genpd which
>>>> need to handle this themselves.
>>>
>>> My point is that doing things like that in different places for different
>>> firmware interfaces is confusing and likely to lead to coding mistakes in
>>> the future.
>>>
>>>> Or are you saying that you prefer bus notifiers in favour of making
>>>> use of the driver core for this matter?
>>>
>>> Well, please grep for acpi_dev_pm_attach() and see where it is done.
>>> Surely not in drivers/base/dd.c.  Also I'm not sure why you're talking
>>> about bus notifiers in this context.
>>>
>>>> Shouldn't the driver core handle most of the common things for a device
>>>> driver?
>>>
>>> Common, yes.  Platform-specific, no.
>>>
>>>> Let's compare how the pinctrls are being managed in the driver core, for
>>>> example.
>>>
>>> pinctrl has Device Trees support only at the moment (as far as firmware
>>> interfaces go) and quite frankly I'm not sure if/how we'll need to change
>>> it to cover ACPI as well.
>>>
>>> But for power domains, please keep that stuff away from dd.c.  That is,
>>> unless Greg specifically disagrees with me and decides to apply this
>>> patch regardless. :-)
>>
>> Nope, no disagreement from me toward you at all here, keep up the good
>> work :)
>
> OK, so proposed solution is to put this in:
>
> - platform_drv_probe(),
> - spi_drv_probe(),
> - i2c_device_probe(),
> - amba_probe(),
>
> ...
>
> - and any other bus type, which can have devices instantiated from DT.
>
> If this is what you mean, I still think putting this in dd is cleaner
> and more scalable, but I'm not going to insist, as I believe you have
> good reasons to prefer this approach over current one.

I am not so sure we should ignore scalability.

Currently there are SoCs those that uses bus notifiers to handle the
device registration for it's pm_domain. When each of them converts to
use bus' ->probe() function instead, they will have their own version
of acpi_dev_pm_attach(), and the scalability becomes an issue.

An option could be to invent a common "pm_domain_attach_dev() API",
which will check what kind of power domain the device may reside in.
In principle invoke acpi_dev_pm_attach() and if no attachment happens,
try genpd_bind_domain().

Kind regards
Ulf Hansson
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Rafael J. Wysocki June 11, 2014, 12:18 a.m. | #7
On Tuesday, June 10, 2014 11:42:25 PM Tomasz Figa wrote:
> On 10.06.2014 23:27, Greg Kroah-Hartman wrote:
> > On Tue, Jun 10, 2014 at 11:27:45PM +0200, Rafael J. Wysocki wrote:
> >> On Tuesday, June 10, 2014 02:53:26 PM Ulf Hansson wrote:
> >>> On 10 June 2014 14:11, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >>>> On Tue, Jun 10, 2014 at 12:51 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
> >>>>> From: Tomasz Figa <t.figa@samsung.com>
> >>>>>
> >>>>> On a number of platforms, devices are part of controllable power
> >>>>> domains, which need to be enabled before such devices can be accessed
> >>>>> and may be powered down when the device is idle to save some power.
> >>>>> This means that on systems that support power domain control using
> >>>>> generic power domains subsystem, it is necessary to add device to its
> >>>>> power domain before binding a driver to it and remove it from its power
> >>>>> domain after its driver is unbound to make sure that an unused device
> >>>>> does not affect power domain state.
> >>>>>
> >>>>> Since this is not limited to particular busses and specific
> >>>>> archs/platforms,
> >>>>
> >>>> Actually, this isn't correrct.  It is limited to the platforms that
> >>>> use Device Trees now.
> >>>
> >>> Correct, we should update the commit message/docs.
> >>>
> >>>>
> >>>> Moreover, it is not consistent with the way we add devices to the ACPI PM
> >>>> domain, which is the ACPI counterpart of this.
> >>>
> >>> I am not sure why you think consistency for ACPI is important here.
> >>> ACPI PM will still be able to handle it's domain/device registering as
> >>> before. There are even other pm_domains that don't use genpd which
> >>> need to handle this themselves.
> >>
> >> My point is that doing things like that in different places for different
> >> firmware interfaces is confusing and likely to lead to coding mistakes in
> >> the future.
> >>
> >>> Or are you saying that you prefer bus notifiers in favour of making
> >>> use of the driver core for this matter?
> >>
> >> Well, please grep for acpi_dev_pm_attach() and see where it is done.
> >> Surely not in drivers/base/dd.c.  Also I'm not sure why you're talking
> >> about bus notifiers in this context.
> >>
> >>> Shouldn't the driver core handle most of the common things for a device
> >>> driver?
> >>
> >> Common, yes.  Platform-specific, no.
> >>
> >>> Let's compare how the pinctrls are being managed in the driver core, for
> >>> example.
> >>
> >> pinctrl has Device Trees support only at the moment (as far as firmware
> >> interfaces go) and quite frankly I'm not sure if/how we'll need to change
> >> it to cover ACPI as well.
> >>
> >> But for power domains, please keep that stuff away from dd.c.  That is,
> >> unless Greg specifically disagrees with me and decides to apply this
> >> patch regardless. :-)
> > 
> > Nope, no disagreement from me toward you at all here, keep up the good
> > work :)
> 
> OK, so proposed solution is to put this in:
> 
> - platform_drv_probe(),
> - spi_drv_probe(),
> - i2c_device_probe(),
> - amba_probe(),

Yes.  Specifically, those are bus types that don't do their own power
management.

> ...
> 
> - and any other bus type, which can have devices instantiated from DT.

And doesn't do its own power management, in which case it will need to
incorporate PM domains handling into its code in a more sophisticated way,
most likely.

> If this is what you mean, I still think putting this in dd is cleaner
> and more scalable, but I'm not going to insist, as I believe you have
> good reasons to prefer this approach over current one.

It may look cleaner, but need not be correct.

Rafael

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Kevin Hilman June 11, 2014, 6:16 p.m. | #8
Tomasz Figa <tomasz.figa@gmail.com> writes:

> On 10.06.2014 23:27, Greg Kroah-Hartman wrote:
>> On Tue, Jun 10, 2014 at 11:27:45PM +0200, Rafael J. Wysocki wrote:
>>> On Tuesday, June 10, 2014 02:53:26 PM Ulf Hansson wrote:
>>>> On 10 June 2014 14:11, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>>> On Tue, Jun 10, 2014 at 12:51 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>> From: Tomasz Figa <t.figa@samsung.com>
>>>>>>
>>>>>> On a number of platforms, devices are part of controllable power
>>>>>> domains, which need to be enabled before such devices can be accessed
>>>>>> and may be powered down when the device is idle to save some power.
>>>>>> This means that on systems that support power domain control using
>>>>>> generic power domains subsystem, it is necessary to add device to its
>>>>>> power domain before binding a driver to it and remove it from its power
>>>>>> domain after its driver is unbound to make sure that an unused device
>>>>>> does not affect power domain state.
>>>>>>
>>>>>> Since this is not limited to particular busses and specific
>>>>>> archs/platforms,
>>>>>
>>>>> Actually, this isn't correrct.  It is limited to the platforms that
>>>>> use Device Trees now.
>>>>
>>>> Correct, we should update the commit message/docs.
>>>>
>>>>>
>>>>> Moreover, it is not consistent with the way we add devices to the ACPI PM
>>>>> domain, which is the ACPI counterpart of this.
>>>>
>>>> I am not sure why you think consistency for ACPI is important here.
>>>> ACPI PM will still be able to handle it's domain/device registering as
>>>> before. There are even other pm_domains that don't use genpd which
>>>> need to handle this themselves.
>>>
>>> My point is that doing things like that in different places for different
>>> firmware interfaces is confusing and likely to lead to coding mistakes in
>>> the future.
>>>
>>>> Or are you saying that you prefer bus notifiers in favour of making
>>>> use of the driver core for this matter?
>>>
>>> Well, please grep for acpi_dev_pm_attach() and see where it is done.
>>> Surely not in drivers/base/dd.c.  Also I'm not sure why you're talking
>>> about bus notifiers in this context.
>>>
>>>> Shouldn't the driver core handle most of the common things for a device
>>>> driver?
>>>
>>> Common, yes.  Platform-specific, no.
>>>
>>>> Let's compare how the pinctrls are being managed in the driver core, for
>>>> example.
>>>
>>> pinctrl has Device Trees support only at the moment (as far as firmware
>>> interfaces go) and quite frankly I'm not sure if/how we'll need to change
>>> it to cover ACPI as well.
>>>
>>> But for power domains, please keep that stuff away from dd.c.  That is,
>>> unless Greg specifically disagrees with me and decides to apply this
>>> patch regardless. :-)
>> 
>> Nope, no disagreement from me toward you at all here, keep up the good
>> work :)
>
> OK, so proposed solution is to put this in:
>
> - platform_drv_probe(),
> - spi_drv_probe(),
> - i2c_device_probe(),
> - amba_probe(),
>
> ...
>
> - and any other bus type, which can have devices instantiated from DT.

Since this is a DT feature, what about calling
__pm_genpd_of_add_device()  when the power-domain nodes are discovered?

Kevin
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tomasz Figa June 11, 2014, 6:19 p.m. | #9
On 11.06.2014 20:16, Kevin Hilman wrote:
> Tomasz Figa <tomasz.figa@gmail.com> writes:
> 
>> On 10.06.2014 23:27, Greg Kroah-Hartman wrote:
>>> On Tue, Jun 10, 2014 at 11:27:45PM +0200, Rafael J. Wysocki wrote:
>>>> On Tuesday, June 10, 2014 02:53:26 PM Ulf Hansson wrote:
>>>>> On 10 June 2014 14:11, Rafael J. Wysocki <rafael@kernel.org> wrote:
>>>>>> On Tue, Jun 10, 2014 at 12:51 PM, Ulf Hansson <ulf.hansson@linaro.org> wrote:
>>>>>>> From: Tomasz Figa <t.figa@samsung.com>
>>>>>>>
>>>>>>> On a number of platforms, devices are part of controllable power
>>>>>>> domains, which need to be enabled before such devices can be accessed
>>>>>>> and may be powered down when the device is idle to save some power.
>>>>>>> This means that on systems that support power domain control using
>>>>>>> generic power domains subsystem, it is necessary to add device to its
>>>>>>> power domain before binding a driver to it and remove it from its power
>>>>>>> domain after its driver is unbound to make sure that an unused device
>>>>>>> does not affect power domain state.
>>>>>>>
>>>>>>> Since this is not limited to particular busses and specific
>>>>>>> archs/platforms,
>>>>>>
>>>>>> Actually, this isn't correrct.  It is limited to the platforms that
>>>>>> use Device Trees now.
>>>>>
>>>>> Correct, we should update the commit message/docs.
>>>>>
>>>>>>
>>>>>> Moreover, it is not consistent with the way we add devices to the ACPI PM
>>>>>> domain, which is the ACPI counterpart of this.
>>>>>
>>>>> I am not sure why you think consistency for ACPI is important here.
>>>>> ACPI PM will still be able to handle it's domain/device registering as
>>>>> before. There are even other pm_domains that don't use genpd which
>>>>> need to handle this themselves.
>>>>
>>>> My point is that doing things like that in different places for different
>>>> firmware interfaces is confusing and likely to lead to coding mistakes in
>>>> the future.
>>>>
>>>>> Or are you saying that you prefer bus notifiers in favour of making
>>>>> use of the driver core for this matter?
>>>>
>>>> Well, please grep for acpi_dev_pm_attach() and see where it is done.
>>>> Surely not in drivers/base/dd.c.  Also I'm not sure why you're talking
>>>> about bus notifiers in this context.
>>>>
>>>>> Shouldn't the driver core handle most of the common things for a device
>>>>> driver?
>>>>
>>>> Common, yes.  Platform-specific, no.
>>>>
>>>>> Let's compare how the pinctrls are being managed in the driver core, for
>>>>> example.
>>>>
>>>> pinctrl has Device Trees support only at the moment (as far as firmware
>>>> interfaces go) and quite frankly I'm not sure if/how we'll need to change
>>>> it to cover ACPI as well.
>>>>
>>>> But for power domains, please keep that stuff away from dd.c.  That is,
>>>> unless Greg specifically disagrees with me and decides to apply this
>>>> patch regardless. :-)
>>>
>>> Nope, no disagreement from me toward you at all here, keep up the good
>>> work :)
>>
>> OK, so proposed solution is to put this in:
>>
>> - platform_drv_probe(),
>> - spi_drv_probe(),
>> - i2c_device_probe(),
>> - amba_probe(),
>>
>> ...
>>
>> - and any other bus type, which can have devices instantiated from DT.
> 
> Since this is a DT feature, what about calling
> __pm_genpd_of_add_device()  when the power-domain nodes are discovered?

I'm not sure how this would work. Could you elaborate on this?

Best regards,
Tomasz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown June 12, 2014, 4:39 p.m. | #10
On Wed, Jun 11, 2014 at 02:18:44AM +0200, Rafael J. Wysocki wrote:
> On Tuesday, June 10, 2014 11:42:25 PM Tomasz Figa wrote:

> > OK, so proposed solution is to put this in:

> > - platform_drv_probe(),
> > - spi_drv_probe(),
> > - i2c_device_probe(),
> > - amba_probe(),

> Yes.  Specifically, those are bus types that don't do their own power
> management.

This sounds tedious.  Can we not instead introduce some generic helpers
which all such buses call/use and then implement the hooks in there
(along with anything else that they should all do)?  There must be other
things that all these buses are having to cut'n'paste.
Rafael Wysocki June 12, 2014, 7:33 p.m. | #11
On Thu, Jun 12, 2014 at 6:39 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Jun 11, 2014 at 02:18:44AM +0200, Rafael J. Wysocki wrote:
>> On Tuesday, June 10, 2014 11:42:25 PM Tomasz Figa wrote:
>
>> > OK, so proposed solution is to put this in:
>
>> > - platform_drv_probe(),
>> > - spi_drv_probe(),
>> > - i2c_device_probe(),
>> > - amba_probe(),
>
>> Yes.  Specifically, those are bus types that don't do their own power
>> management.
>
> This sounds tedious.  Can we not instead introduce some generic helpers
> which all such buses call/use and then implement the hooks in there
> (along with anything else that they should all do)?

Well, good idea.

> There must be other things that all these buses are having to cut'n'paste.

Quite likely.

Rafael
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..80ad789 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -23,6 +23,7 @@ 
 #include <linux/kthread.h>
 #include <linux/wait.h>
 #include <linux/async.h>
+#include <linux/pm_domain.h>
 #include <linux/pm_runtime.h>
 #include <linux/pinctrl/devinfo.h>
 
@@ -287,6 +288,11 @@  static int really_probe(struct device *dev, struct device_driver *drv)
 
 	dev->driver = drv;
 
+	/* If using genpd, bind power domain now before probing */
+	ret = genpd_bind_domain(dev);
+	if (ret)
+		goto probe_failed;
+
 	/* If using pinctrl, bind pins now before probing */
 	ret = pinctrl_bind_pins(dev);
 	if (ret)
@@ -317,6 +323,7 @@  static int really_probe(struct device *dev, struct device_driver *drv)
 probe_failed:
 	devres_release_all(dev);
 	driver_sysfs_remove(dev);
+	genpd_unbind_domain(dev);
 	dev->driver = NULL;
 	dev_set_drvdata(dev, NULL);
 
@@ -530,7 +537,7 @@  static void __device_release_driver(struct device *dev)
 			blocking_notifier_call_chain(&dev->bus->p->bus_notifier,
 						     BUS_NOTIFY_UNBOUND_DRIVER,
 						     dev);
-
+		genpd_unbind_domain(dev);
 	}
 }