Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Ulf Hansson]

On 9 April 2018 at 09:53, Viresh Kumar  wrote:
> On 22-03-18, 11:18, Ulf Hansson wrote:
>> On 22 March 2018 at 10:59, Viresh Kumar  wrote:
>> > On 22-03-18, 10:30, Ulf Hansson wrote:
>> >> On 22 December 2017 at 08:26, Viresh Kumar  
>> >> wrote:
>
>> >> > +   ret = device_add(>dev);
>> >>
>> >> What's the point of adding the device? Can we skip this step, as I
>> >> guess the opp core is only using the device as cookie rather actual
>> >> using it? No?
>> >
>> > We also use it for the OPP debugfs stuff, so that would be required I 
>> > believe.
>>
>> Right, however, isn't that only using the dev_name(dev), which you
>> don't need to add the device to make use of.
>>
>> Or maybe I missing something around this...
>
> So I tested this bit. The code works fine even if the device isn't added
> (registered), but this looks a bit sloppy to attempt.
>
> Please let me know what would you prefer in this case, add the device or not.

Well, I don't know if there is policy of how to do this in general. Or
perhaps this can be considered as a special case as the device is only
going to be used as cookie (at least so far).

I suggest we keep it as simple as possible and *not* add (register) the device.

Kind regards
Uffe

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Ulf Hansson]

On 9 April 2018 at 09:53, Viresh Kumar  wrote:
> On 22-03-18, 11:18, Ulf Hansson wrote:
>> On 22 March 2018 at 10:59, Viresh Kumar  wrote:
>> > On 22-03-18, 10:30, Ulf Hansson wrote:
>> >> On 22 December 2017 at 08:26, Viresh Kumar  
>> >> wrote:
>
>> >> > +   ret = device_add(>dev);
>> >>
>> >> What's the point of adding the device? Can we skip this step, as I
>> >> guess the opp core is only using the device as cookie rather actual
>> >> using it? No?
>> >
>> > We also use it for the OPP debugfs stuff, so that would be required I 
>> > believe.
>>
>> Right, however, isn't that only using the dev_name(dev), which you
>> don't need to add the device to make use of.
>>
>> Or maybe I missing something around this...
>
> So I tested this bit. The code works fine even if the device isn't added
> (registered), but this looks a bit sloppy to attempt.
>
> Please let me know what would you prefer in this case, add the device or not.

Well, I don't know if there is policy of how to do this in general. Or
perhaps this can be considered as a special case as the device is only
going to be used as cookie (at least so far).

I suggest we keep it as simple as possible and *not* add (register) the device.

Kind regards
Uffe

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Viresh Kumar]

On 22-03-18, 11:18, Ulf Hansson wrote:
> On 22 March 2018 at 10:59, Viresh Kumar  wrote:
> > On 22-03-18, 10:30, Ulf Hansson wrote:
> >> On 22 December 2017 at 08:26, Viresh Kumar  wrote:

> >> > +   ret = device_add(>dev);
> >>
> >> What's the point of adding the device? Can we skip this step, as I
> >> guess the opp core is only using the device as cookie rather actual
> >> using it? No?
> >
> > We also use it for the OPP debugfs stuff, so that would be required I 
> > believe.
> 
> Right, however, isn't that only using the dev_name(dev), which you
> don't need to add the device to make use of.
> 
> Or maybe I missing something around this...

So I tested this bit. The code works fine even if the device isn't added
(registered), but this looks a bit sloppy to attempt.

Please let me know what would you prefer in this case, add the device or not.

-- 
viresh

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Viresh Kumar]

On 22-03-18, 11:18, Ulf Hansson wrote:
> On 22 March 2018 at 10:59, Viresh Kumar  wrote:
> > On 22-03-18, 10:30, Ulf Hansson wrote:
> >> On 22 December 2017 at 08:26, Viresh Kumar  wrote:

> >> > +   ret = device_add(>dev);
> >>
> >> What's the point of adding the device? Can we skip this step, as I
> >> guess the opp core is only using the device as cookie rather actual
> >> using it? No?
> >
> > We also use it for the OPP debugfs stuff, so that would be required I 
> > believe.
> 
> Right, however, isn't that only using the dev_name(dev), which you
> don't need to add the device to make use of.
> 
> Or maybe I missing something around this...

So I tested this bit. The code works fine even if the device isn't added
(registered), but this looks a bit sloppy to attempt.

Please let me know what would you prefer in this case, add the device or not.

-- 
viresh

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Ulf Hansson]

On 22 March 2018 at 10:59, Viresh Kumar  wrote:
> On 22-03-18, 10:30, Ulf Hansson wrote:
>> On 22 December 2017 at 08:26, Viresh Kumar  wrote:
>> > The power-domain core would be using the OPP core going forward and the
>> > OPP core has the basic requirement of a device structure for its working.
>>
>> According to the OPP core also seems to require the ->dev.of_node to
>> be set. Actually, it seems like that part belongs in patch4 instead,
>> while starting to make use of the opp OF APIs. Could you please move
>> it?
>
> You meaning setting of the of_node to the next patch? I can do that if that;s
> what you want.

Yes, please.

>
>> > +static struct bus_type genpd_bus_type = {
>> > +   .name = "genpd",
>> > +};
>>
>> This seems silly. Can't we just avoid having a bus type altogether,
>> thus no need to call bus_register() as well?
>
> I thought we need a bus where we want to add the device, haven't tried with 
> that
> pointer being empty. Will try that and let you know.

Great!

>
>> > +
>> >  /**
>> >   * pm_genpd_init - Initialize a generic I/O PM domain object.
>> >   * @genpd: PM domain object to initialize.
>> > @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>> > return ret;
>> > }
>> >
>> > +   genpd->dev.bus = _bus_type;
>> > +   device_initialize(>dev);
>> > +   dev_set_name(>dev, "%s", genpd->name);
>> > +
>> > +   ret = device_add(>dev);
>>
>> What's the point of adding the device? Can we skip this step, as I
>> guess the opp core is only using the device as cookie rather actual
>> using it? No?
>
> We also use it for the OPP debugfs stuff, so that would be required I believe.

Right, however, isn't that only using the dev_name(dev), which you
don't need to add the device to make use of.

Or maybe I missing something around this...

> We also use it for playing with clk/regulator stuff as well, though we may not
> use it in genpd case for now.
>
> --
> viresh

Kind regards
Uffe

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Ulf Hansson]

On 22 March 2018 at 10:59, Viresh Kumar  wrote:
> On 22-03-18, 10:30, Ulf Hansson wrote:
>> On 22 December 2017 at 08:26, Viresh Kumar  wrote:
>> > The power-domain core would be using the OPP core going forward and the
>> > OPP core has the basic requirement of a device structure for its working.
>>
>> According to the OPP core also seems to require the ->dev.of_node to
>> be set. Actually, it seems like that part belongs in patch4 instead,
>> while starting to make use of the opp OF APIs. Could you please move
>> it?
>
> You meaning setting of the of_node to the next patch? I can do that if that;s
> what you want.

Yes, please.

>
>> > +static struct bus_type genpd_bus_type = {
>> > +   .name = "genpd",
>> > +};
>>
>> This seems silly. Can't we just avoid having a bus type altogether,
>> thus no need to call bus_register() as well?
>
> I thought we need a bus where we want to add the device, haven't tried with 
> that
> pointer being empty. Will try that and let you know.

Great!

>
>> > +
>> >  /**
>> >   * pm_genpd_init - Initialize a generic I/O PM domain object.
>> >   * @genpd: PM domain object to initialize.
>> > @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
>> > return ret;
>> > }
>> >
>> > +   genpd->dev.bus = _bus_type;
>> > +   device_initialize(>dev);
>> > +   dev_set_name(>dev, "%s", genpd->name);
>> > +
>> > +   ret = device_add(>dev);
>>
>> What's the point of adding the device? Can we skip this step, as I
>> guess the opp core is only using the device as cookie rather actual
>> using it? No?
>
> We also use it for the OPP debugfs stuff, so that would be required I believe.

Right, however, isn't that only using the dev_name(dev), which you
don't need to add the device to make use of.

Or maybe I missing something around this...

> We also use it for playing with clk/regulator stuff as well, though we may not
> use it in genpd case for now.
>
> --
> viresh

Kind regards
Uffe

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Viresh Kumar]

On 22-03-18, 10:30, Ulf Hansson wrote:
> On 22 December 2017 at 08:26, Viresh Kumar  wrote:
> > The power-domain core would be using the OPP core going forward and the
> > OPP core has the basic requirement of a device structure for its working.
> 
> According to the OPP core also seems to require the ->dev.of_node to
> be set. Actually, it seems like that part belongs in patch4 instead,
> while starting to make use of the opp OF APIs. Could you please move
> it?

You meaning setting of the of_node to the next patch? I can do that if that;s
what you want.

> > +static struct bus_type genpd_bus_type = {
> > +   .name = "genpd",
> > +};
> 
> This seems silly. Can't we just avoid having a bus type altogether,
> thus no need to call bus_register() as well?

I thought we need a bus where we want to add the device, haven't tried with that
pointer being empty. Will try that and let you know.

> > +
> >  /**
> >   * pm_genpd_init - Initialize a generic I/O PM domain object.
> >   * @genpd: PM domain object to initialize.
> > @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> > return ret;
> > }
> >
> > +   genpd->dev.bus = _bus_type;
> > +   device_initialize(>dev);
> > +   dev_set_name(>dev, "%s", genpd->name);
> > +
> > +   ret = device_add(>dev);
> 
> What's the point of adding the device? Can we skip this step, as I
> guess the opp core is only using the device as cookie rather actual
> using it? No?

We also use it for the OPP debugfs stuff, so that would be required I believe.
We also use it for playing with clk/regulator stuff as well, though we may not
use it in genpd case for now.

-- 
viresh

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Viresh Kumar]

On 22-03-18, 10:30, Ulf Hansson wrote:
> On 22 December 2017 at 08:26, Viresh Kumar  wrote:
> > The power-domain core would be using the OPP core going forward and the
> > OPP core has the basic requirement of a device structure for its working.
> 
> According to the OPP core also seems to require the ->dev.of_node to
> be set. Actually, it seems like that part belongs in patch4 instead,
> while starting to make use of the opp OF APIs. Could you please move
> it?

You meaning setting of the of_node to the next patch? I can do that if that;s
what you want.

> > +static struct bus_type genpd_bus_type = {
> > +   .name = "genpd",
> > +};
> 
> This seems silly. Can't we just avoid having a bus type altogether,
> thus no need to call bus_register() as well?

I thought we need a bus where we want to add the device, haven't tried with that
pointer being empty. Will try that and let you know.

> > +
> >  /**
> >   * pm_genpd_init - Initialize a generic I/O PM domain object.
> >   * @genpd: PM domain object to initialize.
> > @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> > return ret;
> > }
> >
> > +   genpd->dev.bus = _bus_type;
> > +   device_initialize(>dev);
> > +   dev_set_name(>dev, "%s", genpd->name);
> > +
> > +   ret = device_add(>dev);
> 
> What's the point of adding the device? Can we skip this step, as I
> guess the opp core is only using the device as cookie rather actual
> using it? No?

We also use it for the OPP debugfs stuff, so that would be required I believe.
We also use it for playing with clk/regulator stuff as well, though we may not
use it in genpd case for now.

-- 
viresh

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Ulf Hansson]

On 22 December 2017 at 08:26, Viresh Kumar  wrote:
> The power-domain core would be using the OPP core going forward and the
> OPP core has the basic requirement of a device structure for its working.

According to the OPP core also seems to require the ->dev.of_node to
be set. Actually, it seems like that part belongs in patch4 instead,
while starting to make use of the opp OF APIs. Could you please move
it?
>
> Add a struct device to the genpd structure and also add a genpd bus type
> for the devices.

This seems reasonable, however I am wondering if we could simplify
this as bit. See more comments below.

>
> Signed-off-by: Viresh Kumar 
> ---
>  drivers/base/power/domain.c | 37 +
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 38 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0c80bea05bcb..013c1e206167 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1630,6 +1630,10 @@ static void genpd_lock_init(struct generic_pm_domain 
> *genpd)
> }
>  }
>
> +static struct bus_type genpd_bus_type = {
> +   .name = "genpd",
> +};

This seems silly. Can't we just avoid having a bus type altogether,
thus no need to call bus_register() as well?

> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> return ret;
> }
>
> +   genpd->dev.bus = _bus_type;
> +   device_initialize(>dev);
> +   dev_set_name(>dev, "%s", genpd->name);
> +
> +   ret = device_add(>dev);

What's the point of adding the device? Can we skip this step, as I
guess the opp core is only using the device as cookie rather actual
using it? No?

> +   if (ret) {
> +   dev_err(>dev, "failed to add device: %d\n", ret);
> +   put_device(>dev);
> +   kfree(genpd->free);
> +   return ret;
> +   }
> +
> mutex_lock(_list_lock);
> list_add(>gpd_list_node, _list);
> mutex_unlock(_list_lock);
> @@ -1724,6 +1740,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
>
> list_del(>gpd_list_node);
> genpd_unlock(genpd);
> +   device_del(>dev);
> cancel_work_sync(>power_off_work);
> kfree(genpd->free);
> pr_debug("%s: removed %s\n", __func__, genpd->name);
> @@ -1888,6 +1905,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
> if (!ret) {
> genpd->provider = >fwnode;
> genpd->has_provider = true;
> +   genpd->dev.of_node = np;
> }
> }
>
> @@ -1924,6 +1942,7 @@ int of_genpd_add_provider_onecell(struct device_node 
> *np,
>
> data->domains[i]->provider = >fwnode;
> data->domains[i]->has_provider = true;
> +   data->domains[i]->dev.of_node = np;
> }
>
> ret = genpd_add_provider(np, data->xlate, data);
> @@ -2691,3 +2710,21 @@ static void __exit genpd_debug_exit(void)
>  }
>  __exitcall(genpd_debug_exit);
>  #endif /* CONFIG_DEBUG_FS */
> +
> +static int __init pm_genpd_core_init(void)
> +{
> +   int ret;
> +
> +   ret = bus_register(_bus_type);
> +   if (ret)
> +   pr_err("bus_register failed (%d)\n", ret);
> +
> +   return ret;
> +}
> +pure_initcall(pm_genpd_core_init);
> +
> +static void __exit pm_genpd_core_exit(void)
> +{
> +   bus_unregister(_bus_type);
> +}
> +__exitcall(pm_genpd_core_exit);
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 04dbef9847d3..aaacaa35005d 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -49,6 +49,7 @@ struct genpd_power_state {
>  struct genpd_lock_ops;
>
>  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 */
> --
> 2.15.0.194.g9af6a3dea062
>

Kind regards
Uffe

Re: [PATCH 3/7] PM / Domain: Add struct device to genpd

[Ulf Hansson]

On 22 December 2017 at 08:26, Viresh Kumar  wrote:
> The power-domain core would be using the OPP core going forward and the
> OPP core has the basic requirement of a device structure for its working.

According to the OPP core also seems to require the ->dev.of_node to
be set. Actually, it seems like that part belongs in patch4 instead,
while starting to make use of the opp OF APIs. Could you please move
it?
>
> Add a struct device to the genpd structure and also add a genpd bus type
> for the devices.

This seems reasonable, however I am wondering if we could simplify
this as bit. See more comments below.

>
> Signed-off-by: Viresh Kumar 
> ---
>  drivers/base/power/domain.c | 37 +
>  include/linux/pm_domain.h   |  1 +
>  2 files changed, 38 insertions(+)
>
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 0c80bea05bcb..013c1e206167 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -1630,6 +1630,10 @@ static void genpd_lock_init(struct generic_pm_domain 
> *genpd)
> }
>  }
>
> +static struct bus_type genpd_bus_type = {
> +   .name = "genpd",
> +};

This seems silly. Can't we just avoid having a bus type altogether,
thus no need to call bus_register() as well?

> +
>  /**
>   * pm_genpd_init - Initialize a generic I/O PM domain object.
>   * @genpd: PM domain object to initialize.
> @@ -1687,6 +1691,18 @@ int pm_genpd_init(struct generic_pm_domain *genpd,
> return ret;
> }
>
> +   genpd->dev.bus = _bus_type;
> +   device_initialize(>dev);
> +   dev_set_name(>dev, "%s", genpd->name);
> +
> +   ret = device_add(>dev);

What's the point of adding the device? Can we skip this step, as I
guess the opp core is only using the device as cookie rather actual
using it? No?

> +   if (ret) {
> +   dev_err(>dev, "failed to add device: %d\n", ret);
> +   put_device(>dev);
> +   kfree(genpd->free);
> +   return ret;
> +   }
> +
> mutex_lock(_list_lock);
> list_add(>gpd_list_node, _list);
> mutex_unlock(_list_lock);
> @@ -1724,6 +1740,7 @@ static int genpd_remove(struct generic_pm_domain *genpd)
>
> list_del(>gpd_list_node);
> genpd_unlock(genpd);
> +   device_del(>dev);
> cancel_work_sync(>power_off_work);
> kfree(genpd->free);
> pr_debug("%s: removed %s\n", __func__, genpd->name);
> @@ -1888,6 +1905,7 @@ int of_genpd_add_provider_simple(struct device_node *np,
> if (!ret) {
> genpd->provider = >fwnode;
> genpd->has_provider = true;
> +   genpd->dev.of_node = np;
> }
> }
>
> @@ -1924,6 +1942,7 @@ int of_genpd_add_provider_onecell(struct device_node 
> *np,
>
> data->domains[i]->provider = >fwnode;
> data->domains[i]->has_provider = true;
> +   data->domains[i]->dev.of_node = np;
> }
>
> ret = genpd_add_provider(np, data->xlate, data);
> @@ -2691,3 +2710,21 @@ static void __exit genpd_debug_exit(void)
>  }
>  __exitcall(genpd_debug_exit);
>  #endif /* CONFIG_DEBUG_FS */
> +
> +static int __init pm_genpd_core_init(void)
> +{
> +   int ret;
> +
> +   ret = bus_register(_bus_type);
> +   if (ret)
> +   pr_err("bus_register failed (%d)\n", ret);
> +
> +   return ret;
> +}
> +pure_initcall(pm_genpd_core_init);
> +
> +static void __exit pm_genpd_core_exit(void)
> +{
> +   bus_unregister(_bus_type);
> +}
> +__exitcall(pm_genpd_core_exit);
> diff --git a/include/linux/pm_domain.h b/include/linux/pm_domain.h
> index 04dbef9847d3..aaacaa35005d 100644
> --- a/include/linux/pm_domain.h
> +++ b/include/linux/pm_domain.h
> @@ -49,6 +49,7 @@ struct genpd_power_state {
>  struct genpd_lock_ops;
>
>  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 */
> --
> 2.15.0.194.g9af6a3dea062
>

Kind regards
Uffe