| Message ID | 1438731339-58317-7-git-send-email-lina.iyer@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote: > In addition to the common power up/down actions of CPU PM domain core, > platforms may have additional configuration before the CPU domain can be > powered off or considered active. Allow platform drivers to register > handlers for CPU PM domains. > > Platform drivers may register their callbacks against a compatible > string defined by their PM domain provider device node in the DT. At > domain init, the platform driver can initialize the platform specific > genpd attributes. The init callback would need to return successfully, > for the platform power_on/off handlers to be registered with the CPU PM > domain. > > The code uses __init section to reduce memory needed for platform > handlers and therefore can be freed after the driver is initialized, a > desirable outcome for single kernel image. [...] > diff --git a/arch/arm/include/asm/arm-pd.h b/arch/arm/include/asm/arm-pd.h > new file mode 100644 > index 0000000..fc44abf > --- /dev/null > +++ b/arch/arm/include/asm/arm-pd.h > @@ -0,0 +1,30 @@ > +/* > + * linux/arch/arm/include/asm/arm-pd.h > + * > + * Copyright (C) 2015 Linaro Ltd. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > + > +#ifndef __ARM_PD_H__ > +#define __ARM_PD_H__ > + > +struct of_arm_pd_ops { > + int (*init)(struct device_node *dn, struct generic_pm_domain *d); > + int (*power_on)(struct generic_pm_domain *d); > + int (*power_off)(struct generic_pm_domain *d); > +}; > + > +struct of_arm_pd_method { > + const char *handle; > + struct of_arm_pd_ops *ops; > +}; > + > +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \ > + static const struct of_arm_pd_method __arm_pd_method_of_table_##_name \ > + __used __section(__arm_pd_method_of_table) \ > + = { .handle = _handle, .ops = _ops } AFAICT, you are not using this in this series. You should add it when you have a user. Ideally, we keep some amount of uniformity across various *_OF_DECLARE which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the sections just arrays of struct of_device_id. Not all users follow this, but most do. So instead of putting the ops in here, platforms can provide a function callback which can then set the ops. Then you also don't need the .init() ops function as the callback function can do any initialization too. Rob
On Wed, Aug 05 2015 at 08:45 -0600, Rob Herring wrote: >On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >> In addition to the common power up/down actions of CPU PM domain core, >> platforms may have additional configuration before the CPU domain can be >> powered off or considered active. Allow platform drivers to register >> handlers for CPU PM domains. >> >> Platform drivers may register their callbacks against a compatible >> string defined by their PM domain provider device node in the DT. At >> domain init, the platform driver can initialize the platform specific >> genpd attributes. The init callback would need to return successfully, >> for the platform power_on/off handlers to be registered with the CPU PM >> domain. >> >> The code uses __init section to reduce memory needed for platform >> handlers and therefore can be freed after the driver is initialized, a >> desirable outcome for single kernel image. > >[...] > >> diff --git a/arch/arm/include/asm/arm-pd.h b/arch/arm/include/asm/arm-pd.h >> new file mode 100644 >> index 0000000..fc44abf >> --- /dev/null >> +++ b/arch/arm/include/asm/arm-pd.h >> @@ -0,0 +1,30 @@ >> +/* >> + * linux/arch/arm/include/asm/arm-pd.h >> + * >> + * Copyright (C) 2015 Linaro Ltd. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> + >> +#ifndef __ARM_PD_H__ >> +#define __ARM_PD_H__ >> + >> +struct of_arm_pd_ops { >> + int (*init)(struct device_node *dn, struct generic_pm_domain *d); >> + int (*power_on)(struct generic_pm_domain *d); >> + int (*power_off)(struct generic_pm_domain *d); >> +}; >> + >> +struct of_arm_pd_method { >> + const char *handle; >> + struct of_arm_pd_ops *ops; >> +}; >> + >> +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \ >> + static const struct of_arm_pd_method __arm_pd_method_of_table_##_name \ >> + __used __section(__arm_pd_method_of_table) \ >> + = { .handle = _handle, .ops = _ops } > >AFAICT, you are not using this in this series. You should add it when >you have a user. > Sorry, I had a last minute change of heart about some commit text and could not get to send the complete series yesterday. I tried to send the rest of the patches, the one that uses this macro, as in-reply-to but that did not work for some reason. Anyways, you are Cc'd in the driver series. >Ideally, we keep some amount of uniformity across various *_OF_DECLARE >which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the >sections just arrays of struct of_device_id. Not all users follow >this, but most do. So instead of putting the ops in here, platforms >can provide a function callback which can then set the ops. Then you >also don't need the .init() ops function as the callback function can >do any initialization too. > Okay, will look into that. >Rob Thanks for your time Rob. -- Lina
On Wed, Aug 05 2015 at 08:45 -0600, Rob Herring wrote: >On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >> In addition to the common power up/down actions of CPU PM domain core, >> platforms may have additional configuration before the CPU domain can be >> powered off or considered active. Allow platform drivers to register >> handlers for CPU PM domains. >> >> Platform drivers may register their callbacks against a compatible >> string defined by their PM domain provider device node in the DT. At >> domain init, the platform driver can initialize the platform specific >> genpd attributes. The init callback would need to return successfully, >> for the platform power_on/off handlers to be registered with the CPU PM >> domain. >> >> The code uses __init section to reduce memory needed for platform >> handlers and therefore can be freed after the driver is initialized, a >> desirable outcome for single kernel image. > >[...] > >> diff --git a/arch/arm/include/asm/arm-pd.h b/arch/arm/include/asm/arm-pd.h >> new file mode 100644 >> index 0000000..fc44abf [...] >> +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \ >> + static const struct of_arm_pd_method __arm_pd_method_of_table_##_name \ >> + __used __section(__arm_pd_method_of_table) \ >> + = { .handle = _handle, .ops = _ops } > >AFAICT, you are not using this in this series. You should add it when >you have a user. > >Ideally, we keep some amount of uniformity across various *_OF_DECLARE >which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the >sections just arrays of struct of_device_id. Not all users follow >this, but most do. So instead of putting the ops in here, platforms >can provide a function callback which can then set the ops. Then you >also don't need the .init() ops function as the callback function can >do any initialization too. > I looked through these and I can switch over to _OF_DECLARE() without any issues, but using OF_DECLARE_1 or OF_DECLARE_2 is pretty limiting. If I could set up my own function type for the .data member, then its a lot more easier on the code. Like this - typedef int (*arm_pd_init)(struct device_node *dn, struct generic_pm_domain *genpd, struct of_arm_pd_ops *ops); #define ARM_PD_METHOD_OF_DECLARE(name, compat, fn) \ _OF_DECLARE(arm_pd, name, compat, fn, arm_pd_init) Its still in the of_device_id array. Is that acceptable? Thanks, Lina
On Wed, Aug 5, 2015 at 2:23 PM, Lina Iyer <lina.iyer@linaro.org> wrote: > On Wed, Aug 05 2015 at 08:45 -0600, Rob Herring wrote: >> >> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >>> >>> In addition to the common power up/down actions of CPU PM domain core, >>> platforms may have additional configuration before the CPU domain can be >>> powered off or considered active. Allow platform drivers to register >>> handlers for CPU PM domains. >>> >>> Platform drivers may register their callbacks against a compatible >>> string defined by their PM domain provider device node in the DT. At >>> domain init, the platform driver can initialize the platform specific >>> genpd attributes. The init callback would need to return successfully, >>> for the platform power_on/off handlers to be registered with the CPU PM >>> domain. >>> >>> The code uses __init section to reduce memory needed for platform >>> handlers and therefore can be freed after the driver is initialized, a >>> desirable outcome for single kernel image. >> >> >> [...] >> >>> diff --git a/arch/arm/include/asm/arm-pd.h >>> b/arch/arm/include/asm/arm-pd.h >>> new file mode 100644 >>> index 0000000..fc44abf > > > [...] > >>> +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \ >>> + static const struct of_arm_pd_method >>> __arm_pd_method_of_table_##_name \ >>> + __used __section(__arm_pd_method_of_table) \ >>> + = { .handle = _handle, .ops = _ops } >> >> >> AFAICT, you are not using this in this series. You should add it when >> you have a user. >> >> Ideally, we keep some amount of uniformity across various *_OF_DECLARE >> which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the >> sections just arrays of struct of_device_id. Not all users follow >> this, but most do. So instead of putting the ops in here, platforms >> can provide a function callback which can then set the ops. Then you >> also don't need the .init() ops function as the callback function can >> do any initialization too. >> > I looked through these and I can switch over to _OF_DECLARE() without any > issues, but using OF_DECLARE_1 or OF_DECLARE_2 is pretty limiting. Well yes, but then we only want to use it for things that will never use the driver model. > If I could set up my own function type for the .data member, then its a > lot more easier on the code. Like this - > > typedef int (*arm_pd_init)(struct device_node *dn, struct > generic_pm_domain *genpd, struct of_arm_pd_ops *ops); You alloc the genpd struct just before calling init. You can just as easily have the platform specific code call an allocation function and a function to set the ops functions. This is more inline with how a driver would work when registering with a subsystem. I also don't think "arm,pd" is a good compatible string to be matching on. I'll comment on that more in patch 5. Rob
On Wed, Aug 05 2015 at 21:01 -0600, Rob Herring wrote: >On Wed, Aug 5, 2015 at 2:23 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >> On Wed, Aug 05 2015 at 08:45 -0600, Rob Herring wrote: >>> >>> On Tue, Aug 4, 2015 at 6:35 PM, Lina Iyer <lina.iyer@linaro.org> wrote: >>>> >>>> In addition to the common power up/down actions of CPU PM domain core, >>>> platforms may have additional configuration before the CPU domain can be >>>> powered off or considered active. Allow platform drivers to register >>>> handlers for CPU PM domains. >>>> >>>> Platform drivers may register their callbacks against a compatible >>>> string defined by their PM domain provider device node in the DT. At >>>> domain init, the platform driver can initialize the platform specific >>>> genpd attributes. The init callback would need to return successfully, >>>> for the platform power_on/off handlers to be registered with the CPU PM >>>> domain. >>>> >>>> The code uses __init section to reduce memory needed for platform >>>> handlers and therefore can be freed after the driver is initialized, a >>>> desirable outcome for single kernel image. >>> >>> >>> [...] >>> >>>> diff --git a/arch/arm/include/asm/arm-pd.h >>>> b/arch/arm/include/asm/arm-pd.h >>>> new file mode 100644 >>>> index 0000000..fc44abf >> >> >> [...] >> >>>> +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \ >>>> + static const struct of_arm_pd_method >>>> __arm_pd_method_of_table_##_name \ >>>> + __used __section(__arm_pd_method_of_table) \ >>>> + = { .handle = _handle, .ops = _ops } >>> >>> >>> AFAICT, you are not using this in this series. You should add it when >>> you have a user. >>> >>> Ideally, we keep some amount of uniformity across various *_OF_DECLARE >>> which is why we have OF_DECLARE_1 and OF_DECLARE_2. This makes all the >>> sections just arrays of struct of_device_id. Not all users follow >>> this, but most do. So instead of putting the ops in here, platforms >>> can provide a function callback which can then set the ops. Then you >>> also don't need the .init() ops function as the callback function can >>> do any initialization too. >>> >> I looked through these and I can switch over to _OF_DECLARE() without any >> issues, but using OF_DECLARE_1 or OF_DECLARE_2 is pretty limiting. > >Well yes, but then we only want to use it for things that will never >use the driver model. > >> If I could set up my own function type for the .data member, then its a >> lot more easier on the code. Like this - >> >> typedef int (*arm_pd_init)(struct device_node *dn, struct >> generic_pm_domain *genpd, struct of_arm_pd_ops *ops); > >You alloc the genpd struct just before calling init. You can just as >easily have the platform specific code call an allocation function and >a function to set the ops functions. This is more inline with how a >driver would work when registering with a subsystem. > OK. That would mean I have a way of mapping the device_node to the of_arm_pd_ops, which I currently dont. Was trying to avoid a list. But, I was able to use list and use OF_DECLARE_1. The platform driver, shall call up separate API to register the ops or ge the Genpd and modify it. Will submit it in the next spin, following the discussion on arm,pd. Thanks, Lina >I also don't >think "arm,pd" is a good compatible string to be matching on. I'll >comment on that more in patch 5. > >Rob
diff --git a/Documentation/arm/cpu-domains.txt b/Documentation/arm/cpu-domains.txt index 3e535b7..a0b98f3 100644 --- a/Documentation/arm/cpu-domains.txt +++ b/Documentation/arm/cpu-domains.txt @@ -47,3 +47,29 @@ attaches the domains' CPU devices to as specified in the DT. This happens automatically at kernel init, when the domain is specified as compatible with "arm,pd". Powering on/off the common cluster hardware would also be done when the PM domain is runtime suspended or resumed. + +SoCs may have additional configuration for the CPU PM domain. The ARM code +provides a way for the platform driver to add those properties to the genpd +before the genpd object is initialized. Additionally, platform driver may also +register for CPU domain power_on/power_off callbacks. + +Platform drivers may register the callbacks using the __init section macro +ARM_PD_METHOD_OF_DECLARE. The callbacks in of_arm_pd_ops, can be specified +against a compatible flag for the domain provider. + +Callback for platform drivers - + +int (*init)(struct device_node *dn, struct generic_pm_domain *d) +The init() callback is called before the generic PM domain is registered with +the GenPD framework. The device node is provided to identify the domain that +is being initialized. The init function must return 0, in order for the +power_on and power_off callbacks to be registered with the CPU PD framework. + +int (*power_on)(struct generic_pm_domain *d); +The power_on() callback is called when the first CPU in the cluster is ready +to resume execution. The domain may be considered active at this point. + +int (*power_off)(struct generic_pm_domain *d); +The power_off() callback is called when the last CPU in the cluster enters +idle. The domain may be configured to power off and wait for a CPU's wakeup +interrupt. diff --git a/arch/arm/common/domains.c b/arch/arm/common/domains.c index 15981e9..bbffeed 100644 --- a/arch/arm/common/domains.c +++ b/arch/arm/common/domains.c @@ -18,12 +18,19 @@ #include <linux/pm_runtime.h> #include <linux/slab.h> +#include <asm/arm-pd.h> + #define NAME_MAX 36 struct arm_pm_domain { struct generic_pm_domain genpd; + struct of_arm_pd_ops platform_ops; }; +extern struct of_arm_pd_method __arm_pd_method_of_table[]; +static const struct of_arm_pd_method __arm_pd_method_of_table_sentinel + __used __section(__arm_pd_method_of_table_end); + static inline struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d) { @@ -32,20 +39,30 @@ struct arm_pm_domain *to_arm_pd(struct generic_pm_domain *d) static int arm_pd_power_down(struct generic_pm_domain *genpd) { + struct arm_pm_domain *pd = to_arm_pd(genpd); + /* * Notify CPU PM domain power down * TODO: Call the notificated directly from here. */ cpu_cluster_pm_enter(); + if (pd->platform_ops.power_off) + return pd->platform_ops.power_off(genpd); + return 0; } static int arm_pd_power_up(struct generic_pm_domain *genpd) { + struct arm_pm_domain *pd = to_arm_pd(genpd); + /* Notify CPU PM domain power up */ cpu_cluster_pm_exit(); + if (pd->platform_ops.power_on) + return pd->platform_ops.power_on(genpd); + return 0; } @@ -134,6 +151,7 @@ static int __init arm_domain_init(void) { struct device_node *np; int count = 0; + struct of_arm_pd_method *m = __arm_pd_method_of_table; for_each_compatible_node(np, NULL, "arm,pd") { struct arm_pm_domain *pd; @@ -145,6 +163,25 @@ static int __init arm_domain_init(void) if (!pd) return -ENOMEM; + /* Invoke platform initialization for the PM domain */ + for (; m->handle; m++) { + int ret; + + if (of_device_is_compatible(np, m->handle)) { + ret = m->ops->init(np, &pd->genpd); + if (!ret) { + pr_debug("CPU PD ops found for %s\n", + m->handle); + pd->platform_ops.power_on = + m->ops->power_on; + pd->platform_ops.power_off = + m->ops->power_off; + } + break; + } + } + + /* Initialize rest of CPU PM domain specifics */ pd->genpd.name = kstrndup(np->name, NAME_MAX, GFP_KERNEL); pd->genpd.power_off = arm_pd_power_down; pd->genpd.power_on = arm_pd_power_up; diff --git a/arch/arm/include/asm/arm-pd.h b/arch/arm/include/asm/arm-pd.h new file mode 100644 index 0000000..fc44abf --- /dev/null +++ b/arch/arm/include/asm/arm-pd.h @@ -0,0 +1,30 @@ +/* + * linux/arch/arm/include/asm/arm-pd.h + * + * Copyright (C) 2015 Linaro Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ + +#ifndef __ARM_PD_H__ +#define __ARM_PD_H__ + +struct of_arm_pd_ops { + int (*init)(struct device_node *dn, struct generic_pm_domain *d); + int (*power_on)(struct generic_pm_domain *d); + int (*power_off)(struct generic_pm_domain *d); +}; + +struct of_arm_pd_method { + const char *handle; + struct of_arm_pd_ops *ops; +}; + +#define ARM_PD_METHOD_OF_DECLARE(_name, _handle, _ops) \ + static const struct of_arm_pd_method __arm_pd_method_of_table_##_name \ + __used __section(__arm_pd_method_of_table) \ + = { .handle = _handle, .ops = _ops } + +#endif /* __ARM_PD_H__ */ diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h index 8bd374d..bd97a69 100644 --- a/include/asm-generic/vmlinux.lds.h +++ b/include/asm-generic/vmlinux.lds.h @@ -179,6 +179,7 @@ #define RESERVEDMEM_OF_TABLES() OF_TABLE(CONFIG_OF_RESERVED_MEM, reservedmem) #define CPU_METHOD_OF_TABLES() OF_TABLE(CONFIG_SMP, cpu_method) #define CPUIDLE_METHOD_OF_TABLES() OF_TABLE(CONFIG_CPU_IDLE, cpuidle_method) +#define ARM_PD_METHOD_OF_TABLES() OF_TABLE(CONFIG_PM_GENERIC_DOMAINS, arm_pd_method) #define EARLYCON_OF_TABLES() OF_TABLE(CONFIG_SERIAL_EARLYCON, earlycon) #define KERNEL_DTB() \ @@ -514,6 +515,7 @@ IOMMU_OF_TABLES() \ CPU_METHOD_OF_TABLES() \ CPUIDLE_METHOD_OF_TABLES() \ + ARM_PD_METHOD_OF_TABLES() \ KERNEL_DTB() \ IRQCHIP_OF_MATCH_TABLE() \ EARLYCON_TABLE() \
In addition to the common power up/down actions of CPU PM domain core, platforms may have additional configuration before the CPU domain can be powered off or considered active. Allow platform drivers to register handlers for CPU PM domains. Platform drivers may register their callbacks against a compatible string defined by their PM domain provider device node in the DT. At domain init, the platform driver can initialize the platform specific genpd attributes. The init callback would need to return successfully, for the platform power_on/off handlers to be registered with the CPU PM domain. The code uses __init section to reduce memory needed for platform handlers and therefore can be freed after the driver is initialized, a desirable outcome for single kernel image. Cc: Rob Herring <robh@kernel.org> Cc: Stephen Boyd <sboyd@codeaurora.org> Cc: Kevin Hilman <khilman@linaro.org> Cc: Ulf Hansson <ulf.hansson@linaro.org> Cc: Catalin Marinas <catalin.marinas@arm.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- Documentation/arm/cpu-domains.txt | 26 ++++++++++++++++++++++++++ arch/arm/common/domains.c | 37 +++++++++++++++++++++++++++++++++++++ arch/arm/include/asm/arm-pd.h | 30 ++++++++++++++++++++++++++++++ include/asm-generic/vmlinux.lds.h | 2 ++ 4 files changed, 95 insertions(+) create mode 100644 arch/arm/include/asm/arm-pd.h