| Message ID | 1323704789-23923-2-git-send-email-thomas.abraham@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Mon, Dec 12, 2011 at 09:16:28PM +0530, Thomas Abraham wrote: > A device node pointer is added to generic pm domain structure to associate > the domain with a node in the device tree. The platform code parses the > device tree to find available nodes representing the generic power domain, > instantiates the available domains and initializes them by calling > pm_genpd_init(). This sounds useful but the binding needs to be documented properly.
Hi, On Monday, December 12, 2011, Thomas Abraham wrote: > A device node pointer is added to generic pm domain structure to associate > the domain with a node in the device tree. That sounds fine except for one thing: PM domains are not devices, so adding "device node" pointers to them is kind of confusing. Perhaps there should be something like struct dt_node, representing a more general device tree node? > The platform code parses the > device tree to find available nodes representing the generic power domain, > instantiates the available domains and initializes them by calling > pm_genpd_init(). > > Nodes representing the devices include a phandle of the power domain to > which it belongs. As these devices get instantiated, the driver code > checkes for availability of a power domain phandle, converts the phandle > to a device node and uses the new pm_genpd_of_add_device() api to > associate the device with a power domain. > > pm_genpd_of_add_device() runs through its list of registered power domains > and matches the OF node of the domain with the one specified as the > parameter. If a match is found, the device is associated with the matched > domain. > > Cc: Rafael J. Wysocki <rjw@sisk.pl> > Cc: Rob Herring <rob.herring@calxeda.com> > Cc: Grant Likely <grant.likely@secretlab.ca> > Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> > --- > drivers/base/power/domain.c | 14 +++++++++++++- > include/linux/pm_domain.h | 13 +++++++++++-- > 2 files changed, 24 insertions(+), 3 deletions(-) > > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c > index 92e6a90..e895dc9 100644 > --- a/drivers/base/power/domain.c > +++ b/drivers/base/power/domain.c > @@ -1118,14 +1118,26 @@ static void pm_genpd_complete(struct device *dev) > * @td: Set of PM QoS timing parameters to attach to the device. > */ > int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, > - struct gpd_timing_data *td) > + struct gpd_timing_data *td, struct device_node *node) > { > struct generic_pm_domain_data *gpd_data; > + struct generic_pm_domain *gpd = NULL; > struct pm_domain_data *pdd; > int ret = 0; > > dev_dbg(dev, "%s()\n", __func__); > > + mutex_lock(&gpd_list_lock); > + if (node) { > + list_for_each_entry(gpd, &gpd_list, gpd_list_node) { > + if (gpd->of_node == node) { > + genpd = gpd; > + break; > + } > + } > + } > + mutex_unlock(&gpd_list_lock); It looks like you might add a wrapper around the original __pm_genpd_add_device() instead of modifying __pm_genpd_add_device() itself. That would be a bit cleaner, as your new function is always called with at least one NULL arg. > + > if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)) > return -EINVAL; > > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h > index a03a0ad..2ce7cc4 100644 > --- a/include/linux/pm_domain.h > +++ b/include/linux/pm_domain.h > @@ -11,6 +11,7 @@ > > #include <linux/device.h> > #include <linux/err.h> > +#include <linux/of.h> > > enum gpd_status { > GPD_STATE_ACTIVE = 0, /* PM domain is active */ > @@ -70,6 +71,7 @@ struct generic_pm_domain { > s64 break_even_ns; /* Power break even for the entire domain. */ > s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ > ktime_t power_off_time; > + struct device_node *of_node; /* Node in device tree */ That should be initialized by the caller of pm_genpd_init(), right? > }; > > static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) > @@ -115,12 +117,19 @@ extern struct dev_power_governor simple_qos_governor; > extern struct generic_pm_domain *dev_to_genpd(struct device *dev); > extern int __pm_genpd_add_device(struct generic_pm_domain *genpd, > struct device *dev, > - struct gpd_timing_data *td); > + struct gpd_timing_data *td, > + struct device_node *gpd_node); > > static inline int pm_genpd_add_device(struct generic_pm_domain *genpd, > struct device *dev) > { > - return __pm_genpd_add_device(genpd, dev, NULL); > + return __pm_genpd_add_device(genpd, dev, NULL, NULL); > +} > + > +static inline int pm_genpd_of_add_device(struct device_node *gpd_node, > + struct device *dev) > +{ > + return __pm_genpd_add_device(NULL, dev, NULL, gpd_node); > } > > extern int pm_genpd_remove_device(struct generic_pm_domain *genpd, > Thanks, Rafael
On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote: > On Monday, December 12, 2011, Thomas Abraham wrote: > > A device node pointer is added to generic pm domain structure to associate > > the domain with a node in the device tree. > That sounds fine except for one thing: PM domains are not devices, so adding > "device node" pointers to them is kind of confusing. Perhaps there should be > something like struct dt_node, representing a more general device tree node? There's struct of_node which is exactly that, though practically speaking you need a device if you're going to bind automatically to something from the device tree in a sensible fashion and there is actual hardware under there so a device does make some sense. This is in part compatibility with the existing Exynos code which uses devices to probe the domains for non-DT systems.
On Monday, December 26, 2011, Mark Brown wrote: > On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote: > > On Monday, December 12, 2011, Thomas Abraham wrote: > > > > A device node pointer is added to generic pm domain structure to associate > > > the domain with a node in the device tree. > > > That sounds fine except for one thing: PM domains are not devices, so adding > > "device node" pointers to them is kind of confusing. Perhaps there should be > > something like struct dt_node, representing a more general device tree node? > > There's struct of_node which is exactly that, though practically > speaking you need a device if you're going to bind automatically to > something from the device tree in a sensible fashion and there is actual > hardware under there so a device does make some sense. > > This is in part compatibility with the existing Exynos code which uses > devices to probe the domains for non-DT systems. Well, that's not a general case, though. It doesn't feel approporiate to use a "device node" pointer for something that's not based on struct device, at least not a generic level, so I wonder if there's a different way. Thanks, Rafael
Hi Mark, Rafael, On 27 December 2011 02:14, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Monday, December 26, 2011, Mark Brown wrote: >> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote: >> > On Monday, December 12, 2011, Thomas Abraham wrote: >> >> > > A device node pointer is added to generic pm domain structure to associate >> > > the domain with a node in the device tree. >> >> > That sounds fine except for one thing: PM domains are not devices, so adding >> > "device node" pointers to them is kind of confusing. Perhaps there should be >> > something like struct dt_node, representing a more general device tree node? >> >> There's struct of_node which is exactly that, though practically >> speaking you need a device if you're going to bind automatically to >> something from the device tree in a sensible fashion and there is actual >> hardware under there so a device does make some sense. In patch 2/2 of this series, the platform code looks for nodes in device tree that represent a power domain. When a power domain node is found, a generic power domain is instantiated with pm_genpd_init() using the information available from the node in device tree. There is no automatic binding required in this case. The power domain node does represent a hardware that manages the power domain. >> >> This is in part compatibility with the existing Exynos code which uses >> devices to probe the domains for non-DT systems. > > Well, that's not a general case, though. > > It doesn't feel approporiate to use a "device node" pointer for something > that's not based on struct device, at least not a generic level, so I wonder > if there's a different way. A device node pointer or of_node pointer is a simple pointer to a instance of a node in device tree. All nodes in a device tree need not represent a corresponding 'struct device'. A node in device tree can described a hardware feature such as a power domain supported in the hardware. The addition of device tree support for generic power domains in this patchset is generic for all platforms. The platform code instantiates generic power domains from device tree with the of_node pointer assigned to 'struct generic_pm_domain'. Then, in __pm_genpd_add_device(), given a of_node pointer (to gen_pd), it is possible to find a matching power domain to select. Thanks, Thomas. > > Thanks, > Rafael
On Wednesday, December 28, 2011, Thomas Abraham wrote: > Hi Mark, Rafael, Hi, > On 27 December 2011 02:14, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Monday, December 26, 2011, Mark Brown wrote: > >> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote: > >> > On Monday, December 12, 2011, Thomas Abraham wrote: > >> > >> > > A device node pointer is added to generic pm domain structure to associate > >> > > the domain with a node in the device tree. > >> > >> > That sounds fine except for one thing: PM domains are not devices, so adding > >> > "device node" pointers to them is kind of confusing. Perhaps there should be > >> > something like struct dt_node, representing a more general device tree node? > >> > >> There's struct of_node which is exactly that, though practically > >> speaking you need a device if you're going to bind automatically to > >> something from the device tree in a sensible fashion and there is actual > >> hardware under there so a device does make some sense. > > In patch 2/2 of this series, the platform code looks for nodes in > device tree that represent a power domain. When a power domain node is > found, a generic power domain is instantiated with pm_genpd_init() > using the information available from the node in device tree. There is > no automatic binding required in this case. The power domain node does > represent a hardware that manages the power domain. Good. So would it be possible to use struct of_node instead of struct device_node in struct generic_pm_domain? > >> > >> This is in part compatibility with the existing Exynos code which uses > >> devices to probe the domains for non-DT systems. > > > > Well, that's not a general case, though. > > > > It doesn't feel approporiate to use a "device node" pointer for something > > that's not based on struct device, at least not a generic level, so I wonder > > if there's a different way. > > A device node pointer or of_node pointer is a simple pointer to a > instance of a node in device tree. All nodes in a device tree need not > represent a corresponding 'struct device'. A node in device tree can > described a hardware feature such as a power domain supported in the > hardware. Sure. > The addition of device tree support for generic power domains in this > patchset is generic for all platforms. The platform code instantiates > generic power domains from device tree with the of_node pointer > assigned to 'struct generic_pm_domain'. Then, in > __pm_genpd_add_device(), given a of_node pointer (to gen_pd), it is > possible to find a matching power domain to select. My point was that adding the struct device_node pointer to struct generic_pm_domain didn't look good, because that structure didn't represent a device in general. While I understand that it may be regarded as a "device object" on some platforms, there are platforms that don't regard PM domains as devices. For this reason (and only for this reason) it appears to be more appropriate to use a more generic device tree node type for struct generic_pm_domain. Thanks, Rafael
Hi Rafael, On 29 December 2011 03:47, Rafael J. Wysocki <rjw@sisk.pl> wrote: > On Wednesday, December 28, 2011, Thomas Abraham wrote: >> Hi Mark, Rafael, > > Hi, > >> On 27 December 2011 02:14, Rafael J. Wysocki <rjw@sisk.pl> wrote: >> > On Monday, December 26, 2011, Mark Brown wrote: >> >> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote: >> >> > On Monday, December 12, 2011, Thomas Abraham wrote: >> >> >> >> > > A device node pointer is added to generic pm domain structure to associate >> >> > > the domain with a node in the device tree. >> >> >> >> > That sounds fine except for one thing: PM domains are not devices, so adding >> >> > "device node" pointers to them is kind of confusing. Perhaps there should be >> >> > something like struct dt_node, representing a more general device tree node? >> >> >> >> There's struct of_node which is exactly that, though practically >> >> speaking you need a device if you're going to bind automatically to >> >> something from the device tree in a sensible fashion and there is actual >> >> hardware under there so a device does make some sense. >> >> In patch 2/2 of this series, the platform code looks for nodes in >> device tree that represent a power domain. When a power domain node is >> found, a generic power domain is instantiated with pm_genpd_init() >> using the information available from the node in device tree. There is >> no automatic binding required in this case. The power domain node does >> represent a hardware that manages the power domain. > > Good. So would it be possible to use struct of_node instead of > struct device_node in struct generic_pm_domain? Sorry, I used 'struct of_node' and 'struct device_node' interchangeably in my reply. All the nodes in a device tree are represented by 'struct device_node'. > >> >> >> >> This is in part compatibility with the existing Exynos code which uses >> >> devices to probe the domains for non-DT systems. >> > >> > Well, that's not a general case, though. >> > >> > It doesn't feel approporiate to use a "device node" pointer for something >> > that's not based on struct device, at least not a generic level, so I wonder >> > if there's a different way. >> >> A device node pointer or of_node pointer is a simple pointer to a >> instance of a node in device tree. All nodes in a device tree need not >> represent a corresponding 'struct device'. A node in device tree can >> described a hardware feature such as a power domain supported in the >> hardware. > > Sure. > >> The addition of device tree support for generic power domains in this >> patchset is generic for all platforms. The platform code instantiates >> generic power domains from device tree with the of_node pointer >> assigned to 'struct generic_pm_domain'. Then, in >> __pm_genpd_add_device(), given a of_node pointer (to gen_pd), it is >> possible to find a matching power domain to select. > > My point was that adding the struct device_node pointer to > struct generic_pm_domain didn't look good, because that structure didn't > represent a device in general. While I understand that it may be regarded > as a "device object" on some platforms, there are platforms that don't > regard PM domains as devices. For this reason (and only for this reason) > it appears to be more appropriate to use a more generic device tree node > type for struct generic_pm_domain. If a platform uses some hardware controls (register read/writes) to enable/disable/control power domain, then it can be represented in the device tree. But such a node need not represent a 'struct device'. Other nodes in the device tree can then reference the power domain node. If a platform does not have any hardware knobs for controlling power domains, then probably it will not be represented in the device tree. But I am not sure of this case. The 'struct device_node' is a representation of a instance of a device tree node. It can represent nodes that are not devices. Hence, it can be used to represent a power domain in a device tree and also included in the 'struct generic_pm_domain'. Thanks, Thomas. > > Thanks, > Rafael
On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote: > Hi, > > On Monday, December 12, 2011, Thomas Abraham wrote: > > A device node pointer is added to generic pm domain structure to associate > > the domain with a node in the device tree. > > That sounds fine except for one thing: PM domains are not devices, so adding > "device node" pointers to them is kind of confusing. Perhaps there should be > something like struct dt_node, representing a more general device tree node? struct device_node has nothing to do with struct device. device_node is a generic device tree node. Thomas' approach is fine. g.
On Monday, January 02, 2012, Grant Likely wrote: > On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote: > > Hi, > > > > On Monday, December 12, 2011, Thomas Abraham wrote: > > > A device node pointer is added to generic pm domain structure to associate > > > the domain with a node in the device tree. > > > > That sounds fine except for one thing: PM domains are not devices, so adding > > "device node" pointers to them is kind of confusing. Perhaps there should be > > something like struct dt_node, representing a more general device tree node? > > struct device_node has nothing to do with struct device. device_node is a > generic device tree node. So the name is kind of confusing. :-) > Thomas' approach is fine. Well, I guess so. Thanks, Rafael
On Monday, January 02, 2012, Thomas Abraham wrote: > Hi Rafael, > > On 29 December 2011 03:47, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > On Wednesday, December 28, 2011, Thomas Abraham wrote: > >> Hi Mark, Rafael, > > > > Hi, > > > >> On 27 December 2011 02:14, Rafael J. Wysocki <rjw@sisk.pl> wrote: > >> > On Monday, December 26, 2011, Mark Brown wrote: > >> >> On Mon, Dec 26, 2011 at 08:13:19PM +0100, Rafael J. Wysocki wrote: > >> >> > On Monday, December 12, 2011, Thomas Abraham wrote: > >> >> > >> >> > > A device node pointer is added to generic pm domain structure to associate > >> >> > > the domain with a node in the device tree. > >> >> > >> >> > That sounds fine except for one thing: PM domains are not devices, so adding > >> >> > "device node" pointers to them is kind of confusing. Perhaps there should be > >> >> > something like struct dt_node, representing a more general device tree node? > >> >> > >> >> There's struct of_node which is exactly that, though practically > >> >> speaking you need a device if you're going to bind automatically to > >> >> something from the device tree in a sensible fashion and there is actual > >> >> hardware under there so a device does make some sense. > >> > >> In patch 2/2 of this series, the platform code looks for nodes in > >> device tree that represent a power domain. When a power domain node is > >> found, a generic power domain is instantiated with pm_genpd_init() > >> using the information available from the node in device tree. There is > >> no automatic binding required in this case. The power domain node does > >> represent a hardware that manages the power domain. > > > > Good. So would it be possible to use struct of_node instead of > > struct device_node in struct generic_pm_domain? > > Sorry, I used 'struct of_node' and 'struct device_node' > interchangeably in my reply. All the nodes in a device tree are > represented by 'struct device_node'. > > > > >> >> > >> >> This is in part compatibility with the existing Exynos code which uses > >> >> devices to probe the domains for non-DT systems. > >> > > >> > Well, that's not a general case, though. > >> > > >> > It doesn't feel approporiate to use a "device node" pointer for something > >> > that's not based on struct device, at least not a generic level, so I wonder > >> > if there's a different way. > >> > >> A device node pointer or of_node pointer is a simple pointer to a > >> instance of a node in device tree. All nodes in a device tree need not > >> represent a corresponding 'struct device'. A node in device tree can > >> described a hardware feature such as a power domain supported in the > >> hardware. > > > > Sure. > > > >> The addition of device tree support for generic power domains in this > >> patchset is generic for all platforms. The platform code instantiates > >> generic power domains from device tree with the of_node pointer > >> assigned to 'struct generic_pm_domain'. Then, in > >> __pm_genpd_add_device(), given a of_node pointer (to gen_pd), it is > >> possible to find a matching power domain to select. > > > > My point was that adding the struct device_node pointer to > > struct generic_pm_domain didn't look good, because that structure didn't > > represent a device in general. While I understand that it may be regarded > > as a "device object" on some platforms, there are platforms that don't > > regard PM domains as devices. For this reason (and only for this reason) > > it appears to be more appropriate to use a more generic device tree node > > type for struct generic_pm_domain. > > If a platform uses some hardware controls (register read/writes) to > enable/disable/control power domain, then it can be represented in the > device tree. But such a node need not represent a 'struct device'. > Other nodes in the device tree can then reference the power domain > node. > > If a platform does not have any hardware knobs for controlling power > domains, then probably it will not be represented in the device tree. > But I am not sure of this case. > > The 'struct device_node' is a representation of a instance of a device > tree node. It can represent nodes that are not devices. Hence, it can > be used to represent a power domain in a device tree and also included > in the 'struct generic_pm_domain'. OK, so please address my second comment regarding the $subject patch, i.e. that you might add a wrapper around the original __pm_genpd_add_device() instead of modifying __pm_genpd_add_device() itself. Thanks, Rafael
Hi Rafael, On 4 January 2012 04:00, Rafael J. Wysocki <rjw@sisk.pl> wrote: [...] >> The 'struct device_node' is a representation of a instance of a device >> tree node. It can represent nodes that are not devices. Hence, it can >> be used to represent a power domain in a device tree and also included >> in the 'struct generic_pm_domain'. > > OK, so please address my second comment regarding the $subject patch, > i.e. that you might add a wrapper around the original > __pm_genpd_add_device() instead of modifying __pm_genpd_add_device() > itself. Thanks. I will prepare and submit the updated patch. Regards, Thomas. > > Thanks, > Rafael
diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c index 92e6a90..e895dc9 100644 --- a/drivers/base/power/domain.c +++ b/drivers/base/power/domain.c @@ -1118,14 +1118,26 @@ static void pm_genpd_complete(struct device *dev) * @td: Set of PM QoS timing parameters to attach to the device. */ int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, - struct gpd_timing_data *td) + struct gpd_timing_data *td, struct device_node *node) { struct generic_pm_domain_data *gpd_data; + struct generic_pm_domain *gpd = NULL; struct pm_domain_data *pdd; int ret = 0; dev_dbg(dev, "%s()\n", __func__); + mutex_lock(&gpd_list_lock); + if (node) { + list_for_each_entry(gpd, &gpd_list, gpd_list_node) { + if (gpd->of_node == node) { + genpd = gpd; + break; + } + } + } + mutex_unlock(&gpd_list_lock); + if (IS_ERR_OR_NULL(genpd) || IS_ERR_OR_NULL(dev)) return -EINVAL; diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h index a03a0ad..2ce7cc4 100644 --- a/include/linux/pm_domain.h +++ b/include/linux/pm_domain.h @@ -11,6 +11,7 @@ #include <linux/device.h> #include <linux/err.h> +#include <linux/of.h> enum gpd_status { GPD_STATE_ACTIVE = 0, /* PM domain is active */ @@ -70,6 +71,7 @@ struct generic_pm_domain { s64 break_even_ns; /* Power break even for the entire domain. */ s64 max_off_time_ns; /* Maximum allowed "suspended" time. */ ktime_t power_off_time; + struct device_node *of_node; /* Node in device tree */ }; static inline struct generic_pm_domain *pd_to_genpd(struct dev_pm_domain *pd) @@ -115,12 +117,19 @@ extern struct dev_power_governor simple_qos_governor; extern struct generic_pm_domain *dev_to_genpd(struct device *dev); extern int __pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev, - struct gpd_timing_data *td); + struct gpd_timing_data *td, + struct device_node *gpd_node); static inline int pm_genpd_add_device(struct generic_pm_domain *genpd, struct device *dev) { - return __pm_genpd_add_device(genpd, dev, NULL); + return __pm_genpd_add_device(genpd, dev, NULL, NULL); +} + +static inline int pm_genpd_of_add_device(struct device_node *gpd_node, + struct device *dev) +{ + return __pm_genpd_add_device(NULL, dev, NULL, gpd_node); } extern int pm_genpd_remove_device(struct generic_pm_domain *genpd,
A device node pointer is added to generic pm domain structure to associate the domain with a node in the device tree. The platform code parses the device tree to find available nodes representing the generic power domain, instantiates the available domains and initializes them by calling pm_genpd_init(). Nodes representing the devices include a phandle of the power domain to which it belongs. As these devices get instantiated, the driver code checkes for availability of a power domain phandle, converts the phandle to a device node and uses the new pm_genpd_of_add_device() api to associate the device with a power domain. pm_genpd_of_add_device() runs through its list of registered power domains and matches the OF node of the domain with the one specified as the parameter. If a match is found, the device is associated with the matched domain. Cc: Rafael J. Wysocki <rjw@sisk.pl> Cc: Rob Herring <rob.herring@calxeda.com> Cc: Grant Likely <grant.likely@secretlab.ca> Signed-off-by: Thomas Abraham <thomas.abraham@linaro.org> --- drivers/base/power/domain.c | 14 +++++++++++++- include/linux/pm_domain.h | 13 +++++++++++-- 2 files changed, 24 insertions(+), 3 deletions(-)