RE: [RFC PATCH 1/1] PM / Domains: Add multi PM domains support for attach_dev

[Aisheng Dong]

> From: Ulf Hansson [mailto:ulf.hansson@xxxxxxxxxx]
> Sent: Friday, December 28, 2018 11:37 PM
> 
> On Thu, 27 Dec 2018 at 18:14, Aisheng Dong <aisheng.dong@xxxxxxx>
> wrote:
> >
> > Currently attach_dev() in power domain infrastructure still does not
> > support multi domains case as the struct device *dev passed down from
> > genpd_dev_pm_attach_by_id() is a virtual PD device, it does not help
> > for parsing the real device information from device tree, e.g.
> > Device/Power IDs, Clocks and it's unware of which real power domain
> > the device should attach.
> 
> Thanks for working on this!
> 
> I would appreciate if the changelog could clarify the problem a bit.
> Perhaps something along the lines of the below.
> 

Sounds good to me.
I will add them in commit message.
Thanks for the suggestion.

> "A genpd provider's ->attach_dev() callback may be invoked with a so called
> virtual device, which is created by genpd, at the point when a device is being
> attached to one of its corresponding multiple PM domains.
> 
> In these cases, the genpd provider fails to look up any resource, by a
> clk_get() for example, for the virtual device in question. This is because, the
> virtual device that was created by genpd, does not have the virt_dev->of_node
> assigned."
> 
> >
> > Extend the framework a bit to store the multi PM domains information
> > in per-device struct generic_pm_domain_data, then power domain driver
> > could retrieve it for necessary operations during attach_dev().
> >
> > Two new APIs genpd_is_mpd_device() and dev_gpd_mpd_data() are also
> > introduced to ease the driver operation.
> >
> > Cc: "Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>
> > Cc: Kevin Hilman <khilman@xxxxxxxxxx>
> > Cc: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Dong Aisheng <aisheng.dong@xxxxxxx>
> > ---
> > This patch is a follow-up work of the earlier discussion with Ulf
> > Hansson about the multi PM domains support for the attach_dev() function
> [1].
> > After a bit more thinking, this is a less intrusive implementation
> > with the mininum impact on the exist function definitions and calling
> follows.
> > One known little drawback is that we have to use the device driver
> > private data (device.drvdata) to pass down the multi domains
> > information in a earlier time. However, as multi PD devices are
> > created by domain framework, this seems to be safe to use it in domain
> > core code as device driver is not likely going to use it.
> > Anyway, if any better ideas, please let me know.
> >
> > With the two new APIs, the using can be simply as:
> > static int xxx_attach_dev(struct generic_pm_domain *domain,
> >                           struct device *dev) {
> >         ...
> >         if (genpd_is_mpd_device(dev)) {
> >                 mpd_data = dev_gpd_mpd_data(dev);
> >                 np = mpd_data->parent->of_node;
> >                 idx = mpd_data->index;
> >                 //dts parsing
> >                 ...
> >         }
> >         ...
> 
> I think we can make this a lot less complicated. Just assign virt_dev->of_node =
> of_node_get(dev->of_node), somewhere in
> genpd_dev_pm_attach_by_id() and before calling __genpd_dev_pm_attach().
> 
> Doing that, would mean the genpd provider's ->attach_dev() callback, don't
> have to distinguish between virtual and non-virtual devices.
> Instead they should be able to look up resources in the same way as they did
> before.
> 

Yes, that seems like a smart way.
But there's still a remain problem that how about the domain index information
needed for attach_dev()?

Regards
Dong Aisheng

> Kind regards
> Uffe
> 
> > }
> >
> > ---
> >  drivers/base/power/domain.c | 31 +++++++++++++++++++++++++++++++
> >  include/linux/pm_domain.h   | 23 +++++++++++++++++++++++
> >  2 files changed, 54 insertions(+)
> >
> > diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> > index 7f38a92..1aa0918 100644
> > --- a/drivers/base/power/domain.c
> > +++ b/drivers/base/power/domain.c
> > @@ -1343,6 +1343,9 @@ static struct generic_pm_domain_data
> *genpd_alloc_dev_data(struct device *dev,
> >         gpd_data->td.effective_constraint_ns =
> PM_QOS_RESUME_LATENCY_NO_CONSTRAINT_NS;
> >         gpd_data->nb.notifier_call = genpd_dev_pm_qos_notifier;
> >
> > +       if (genpd_is_mpd_device(dev))
> > +               gpd_data->mpd_data = dev_get_drvdata(dev);
> > +
> >         spin_lock_irq(&dev->power.lock);
> >
> >         if (dev->power.subsys_data->domain_data) { @@ -2179,6 +2182,7
> > @@ EXPORT_SYMBOL_GPL(of_genpd_remove_last);
> >
> >  static void genpd_release_dev(struct device *dev)  {
> > +       kfree(dev->driver_data);
> >         kfree(dev);
> >  }
> >
> > @@ -2320,6 +2324,20 @@ int genpd_dev_pm_attach(struct device *dev)
> > EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
> >
> >  /**
> > + * genpd_is_mpd_device - Check if a device is associated with multi
> > + PM domains
> > + * @dev: Device to check.
> > + */
> > +
> > +bool genpd_is_mpd_device(struct device *dev) {
> > +       if (!dev || (dev && !dev->bus))
> > +               return false;
> > +
> > +       return dev->bus == &genpd_bus_type; };
> > +EXPORT_SYMBOL_GPL(genpd_is_mpd_device);
> > +
> > +/**
> >   * genpd_dev_pm_attach_by_id - Associate a device with one of its PM
> domains.
> >   * @dev: The device used to lookup the PM domain.
> >   * @index: The index of the PM domain.
> > @@ -2338,6 +2356,7 @@ EXPORT_SYMBOL_GPL(genpd_dev_pm_attach);
> >  struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> >                                          unsigned int index)  {
> > +       struct pm_domain_mpd_data *mpd_data;
> >         struct device *genpd_dev;
> >         int num_domains;
> >         int ret;
> > @@ -2366,6 +2385,18 @@ struct device
> *genpd_dev_pm_attach_by_id(struct device *dev,
> >                 return ERR_PTR(ret);
> >         }
> >
> > +       /* Allocate multi power domains data */
> > +       mpd_data = kzalloc(sizeof(*mpd_data), GFP_KERNEL);
> > +       if (!mpd_data) {
> > +               device_unregister(genpd_dev);
> > +               return ERR_PTR(-ENOMEM);
> > +       }
> > +
> > +       mpd_data->parent = dev;
> > +       mpd_data->index = index;
> > +
> > +       dev_set_drvdata(genpd_dev, mpd_data);
> > +
> >         /* Try to attach the device to the PM domain at the specified index.
> */
> >         ret = __genpd_dev_pm_attach(genpd_dev, dev->of_node, index,
> false);
> >         if (ret < 1) {
> > diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> > index 3b5d728..106d4e7 100644
> > --- a/include/linux/pm_domain.h
> > +++ b/include/linux/pm_domain.h
> > @@ -144,6 +144,11 @@ struct gpd_timing_data {
> >         bool cached_suspend_ok;
> >  };
> >
> > +struct pm_domain_mpd_data {
> > +       struct device *parent;
> > +       unsigned int index;
> > +};
> > +
> >  struct pm_domain_data {
> >         struct list_head list_node;
> >         struct device *dev;
> > @@ -151,6 +156,7 @@ struct pm_domain_data {
> >
> >  struct generic_pm_domain_data {
> >         struct pm_domain_data base;
> > +       struct pm_domain_mpd_data *mpd_data;
> >         struct gpd_timing_data td;
> >         struct notifier_block nb;
> >         unsigned int performance_state; @@ -262,10 +268,17 @@
> unsigned
> > int of_genpd_opp_to_performance_state(struct device *dev,
> >                                 struct device_node *np);
> >
> >  int genpd_dev_pm_attach(struct device *dev);
> > +bool genpd_is_mpd_device(struct device *dev);
> >  struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> >                                          unsigned int index);  struct
> > device *genpd_dev_pm_attach_by_name(struct device *dev,
> >                                            char *name);
> > +
> > +static inline struct pm_domain_mpd_data *dev_gpd_mpd_data(struct
> > +device *dev) {
> > +       return dev_gpd_data(dev)->mpd_data; }
> > +
> >  #else /* !CONFIG_PM_GENERIC_DOMAINS_OF */  static inline int
> > of_genpd_add_provider_simple(struct device_node *np,
> >                                         struct generic_pm_domain
> > *genpd) @@ -311,6 +324,11 @@ static inline int
> genpd_dev_pm_attach(struct device *dev)
> >         return 0;
> >  }
> >
> > +static bool genpd_is_mpd_device(struct device *dev) {
> > +       return false;
> > +}
> > +
> >  static inline struct device *genpd_dev_pm_attach_by_id(struct device *dev,
> >                                                        unsigned
> int
> > index)  { @@ -323,6 +341,11 @@ static inline struct device
> > *genpd_dev_pm_attach_by_name(struct device *dev,
> >         return NULL;
> >  }
> >
> > +static inline struct pm_domain_mpd_data *dev_gpd_mpd_data(struct
> > +device *dev) {
> > +       return ERR_PTR(-ENOSYS);
> > +}
> > +
> >  static inline
> >  struct generic_pm_domain *of_genpd_remove_last(struct device_node
> > *np)  {
> > --
> > 2.7.4
> >