| Message ID | 1403875377-940-8-git-send-email-gregory.clement@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 27/06/2014 15:22, Gregory CLEMENT wrote: > In order to support more mvebu SoCs, this patch use an initialization > specific function associated to each SoCs which support CPU Idle. > > Then each SoC will have his own set of check and of data > configuration. > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > --- > arch/arm/mach-mvebu/pmsu.c | 39 +++++++++++++++++++++++++++++++-------- > 1 file changed, 31 insertions(+), 8 deletions(-) > > diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c > index 087157c20b8a..454f0f9ede6b 100644 > --- a/arch/arm/mach-mvebu/pmsu.c > +++ b/arch/arm/mach-mvebu/pmsu.c > @@ -293,23 +293,47 @@ static struct notifier_block mvebu_v7_cpu_pm_notifier = { > .notifier_call = mvebu_v7_cpu_pm_notify, > }; > > +static bool (*mvebu_v7_cpu_idle_init)(void); > + > +static __init bool armada_xp_cpuidle_init(void) > +{ > + struct device_node *np; > + > + np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"); > + if (!np) > + return false; > + of_node_put(np); > + > + mvebu_v7_cpuidle_device.dev.platform_data = armada_xp_370_cpu_suspend; > + return true; > +} > + > +static struct of_device_id of_cpuidle_table[] __initdata = { > + { .compatible = "marvell,armadaxp", > + .data = (void *)armada_xp_cpuidle_init, > + }, > + { /* end of list */ }, > +}; > + > static int __init mvebu_v7_cpu_pm_init(void) > { > struct device_node *np; > + const struct of_device_id *match; > + > + np = of_find_matching_node_and_match(NULL, of_cpuidle_table, > + &match); > + The following part was missing (without this the kernel hang on Armada 375) + if (!np) + return 0; > > /* > * Check that all the requirements are available to enable > - * cpuidle. So far, it is only supported on Armada XP, cpuidle > - * needs the coherency fabric and the PMSU enabled > + * cpuidle. Each SoCs comes with its own requirements and > + * configuration > */ > > - if (!of_machine_is_compatible("marvell,armadaxp")) > - return 0; > + mvebu_v7_cpu_idle_init = (bool (*)(void))match->data; > > - np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"); > - if (!np) > + if (!mvebu_v7_cpu_idle_init()) > return 0; > - of_node_put(np); > > np = of_find_matching_node(NULL, of_pmsu_table); > if (!np) > @@ -329,7 +353,6 @@ static int __init mvebu_v7_cpu_pm_init(void) > PMSU_BOOT_ADDR_REDIRECT_OFFSET(0)); > > mvebu_v7_pmsu_enable_l2_powerdown_onidle(); > - mvebu_v7_cpuidle_device.dev.platform_data = armada_xp_370_cpu_suspend; > platform_device_register(&mvebu_v7_cpuidle_device); > cpu_pm_register_notifier(&mvebu_v7_cpu_pm_notifier); > >
Gregory, On Fri, Jun 27, 2014 at 04:15:44PM +0200, Gregory CLEMENT wrote: > On 27/06/2014 15:22, Gregory CLEMENT wrote: > > In order to support more mvebu SoCs, this patch use an initialization > > specific function associated to each SoCs which support CPU Idle. > > > > Then each SoC will have his own set of check and of data > > configuration. > > > > Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> > > --- > > arch/arm/mach-mvebu/pmsu.c | 39 +++++++++++++++++++++++++++++++-------- > > 1 file changed, 31 insertions(+), 8 deletions(-) > > > > diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c > > index 087157c20b8a..454f0f9ede6b 100644 > > --- a/arch/arm/mach-mvebu/pmsu.c > > +++ b/arch/arm/mach-mvebu/pmsu.c > > @@ -293,23 +293,47 @@ static struct notifier_block mvebu_v7_cpu_pm_notifier = { > > .notifier_call = mvebu_v7_cpu_pm_notify, > > }; > > > > +static bool (*mvebu_v7_cpu_idle_init)(void); > > + > > +static __init bool armada_xp_cpuidle_init(void) > > +{ > > + struct device_node *np; > > + > > + np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"); > > + if (!np) > > + return false; > > + of_node_put(np); > > + > > + mvebu_v7_cpuidle_device.dev.platform_data = armada_xp_370_cpu_suspend; > > + return true; > > +} > > + > > +static struct of_device_id of_cpuidle_table[] __initdata = { > > + { .compatible = "marvell,armadaxp", > > + .data = (void *)armada_xp_cpuidle_init, > > + }, > > + { /* end of list */ }, > > +}; > > + > > static int __init mvebu_v7_cpu_pm_init(void) > > { > > struct device_node *np; > > + const struct of_device_id *match; > > + > > + np = of_find_matching_node_and_match(NULL, of_cpuidle_table, > > + &match); > > + > > The following part was missing (without this the kernel hang on Armada 375) > > + if (!np) > + return 0; If there's no other changes, I can fix this up when I apply it. As well as the S-o-b. thx, Jason.
Hi Jason, >>> + np = of_find_matching_node_and_match(NULL, of_cpuidle_table, >>> + &match); >>> + >> >> The following part was missing (without this the kernel hang on Armada 375) >> >> + if (!np) >> + return 0; > > If there's no other changes, I can fix this up when I apply it. As well > as the S-o-b. Thanks. Let's hope it will be the case, and there will no need to a new version of this series. :) Gregory
Gregory, On Fri, 27 Jun 2014 15:22:48 +0200, Gregory CLEMENT wrote: > +static bool (*mvebu_v7_cpu_idle_init)(void); > + > +static __init bool armada_xp_cpuidle_init(void) > +{ > + struct device_node *np; > + > + np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"); > + if (!np) > + return false; > + of_node_put(np); > + > + mvebu_v7_cpuidle_device.dev.platform_data = armada_xp_370_cpu_suspend; > + return true; > +} > + > +static struct of_device_id of_cpuidle_table[] __initdata = { > + { .compatible = "marvell,armadaxp", > + .data = (void *)armada_xp_cpuidle_init, > + }, > + { /* end of list */ }, > +}; > + > static int __init mvebu_v7_cpu_pm_init(void) > { > struct device_node *np; > + const struct of_device_id *match; > + > + np = of_find_matching_node_and_match(NULL, of_cpuidle_table, > + &match); I'm not sure I like using of_find_matching_node_and_match() on the entire tree to find the root node compatible string. Wouldn't it be simpler and shorter to just do: if (of_machine_is_compatible("marvell,armadaxp")) ret = armadaxp_cpuidle_init(); else if (of_machine_is_compatible("marvell,armada370")) ret = armada370_cpuidle_init(); else if (of_machine_is_compaitble("marvell,armada380")) ret = armada38x_cpuidle_init(); if (ret) return ret; Also, using a boolean to return a success/error status is not the kernel way of doing things, you should use an int instead and use proper error codes or return 0 on success. Thanks, Thomas
Hi Thomas, struct device_node *np; >> + const struct of_device_id *match; >> + >> + np = of_find_matching_node_and_match(NULL, of_cpuidle_table, >> + &match); > > I'm not sure I like using of_find_matching_node_and_match() on the > entire tree to find the root node compatible string. Wouldn't it be > simpler and shorter to just do: > > if (of_machine_is_compatible("marvell,armadaxp")) > ret = armadaxp_cpuidle_init(); > else if (of_machine_is_compatible("marvell,armada370")) > ret = armada370_cpuidle_init(); > else if (of_machine_is_compaitble("marvell,armada380")) > ret = armada38x_cpuidle_init(); > > if (ret) > return ret; Indeed, as I don't use any resource from the device tree here, using of_find_matching_node_and_match is overkill. And your alternative will be enough. > > Also, using a boolean to return a success/error status is not the > kernel way of doing things, you should use an int instead and use > proper error codes or return 0 on success. OK Thanks, Gregory
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c index 087157c20b8a..454f0f9ede6b 100644 --- a/arch/arm/mach-mvebu/pmsu.c +++ b/arch/arm/mach-mvebu/pmsu.c @@ -293,23 +293,47 @@ static struct notifier_block mvebu_v7_cpu_pm_notifier = { .notifier_call = mvebu_v7_cpu_pm_notify, }; +static bool (*mvebu_v7_cpu_idle_init)(void); + +static __init bool armada_xp_cpuidle_init(void) +{ + struct device_node *np; + + np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"); + if (!np) + return false; + of_node_put(np); + + mvebu_v7_cpuidle_device.dev.platform_data = armada_xp_370_cpu_suspend; + return true; +} + +static struct of_device_id of_cpuidle_table[] __initdata = { + { .compatible = "marvell,armadaxp", + .data = (void *)armada_xp_cpuidle_init, + }, + { /* end of list */ }, +}; + static int __init mvebu_v7_cpu_pm_init(void) { struct device_node *np; + const struct of_device_id *match; + + np = of_find_matching_node_and_match(NULL, of_cpuidle_table, + &match); + /* * Check that all the requirements are available to enable - * cpuidle. So far, it is only supported on Armada XP, cpuidle - * needs the coherency fabric and the PMSU enabled + * cpuidle. Each SoCs comes with its own requirements and + * configuration */ - if (!of_machine_is_compatible("marvell,armadaxp")) - return 0; + mvebu_v7_cpu_idle_init = (bool (*)(void))match->data; - np = of_find_compatible_node(NULL, NULL, "marvell,coherency-fabric"); - if (!np) + if (!mvebu_v7_cpu_idle_init()) return 0; - of_node_put(np); np = of_find_matching_node(NULL, of_pmsu_table); if (!np) @@ -329,7 +353,6 @@ static int __init mvebu_v7_cpu_pm_init(void) PMSU_BOOT_ADDR_REDIRECT_OFFSET(0)); mvebu_v7_pmsu_enable_l2_powerdown_onidle(); - mvebu_v7_cpuidle_device.dev.platform_data = armada_xp_370_cpu_suspend; platform_device_register(&mvebu_v7_cpuidle_device); cpu_pm_register_notifier(&mvebu_v7_cpu_pm_notifier);
In order to support more mvebu SoCs, this patch use an initialization specific function associated to each SoCs which support CPU Idle. Then each SoC will have his own set of check and of data configuration. Signed-off-by: Gregory CLEMENT <gregory.clement@free-electrons.com> --- arch/arm/mach-mvebu/pmsu.c | 39 +++++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-)