| Message ID | 20240813081324.3205944-1-nick.hu@sifive.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | cpuidle: riscv-sbi: Allow cpuidle pd used by other devices | expand |
On Tue, Aug 13, 2024 at 1:43 PM Nick Hu <nick.hu@sifive.com> wrote: > > To prevent the probe of consumer devices being deferred, create the > platform devices for each pd node under '/cpus/power-domains' and move the > driver initailization to the arch_initcall. To prevent the probe deferral of consumer devices, you can simply use fwnode_dev_initialized() instead of creating a dummy platform device. > The consumer devices that inside the cpu/cluster power domain may register > the genpd notifier where their power domains are point to the pd nodes > under '/cpus/power-domains'. If the cpuidle.off==1, the genpd notifier > will fail due to sbi_cpuidle_pd_allow_domain_state is not set. We also > need the sbi_cpuidle_cpuhp_up/down to invoke the callbacks. Therefore, > add a cpuidle_disabled() check before registering the cpuidle driver to > address the issue. I think dealing with cpuidle.off==1 case should be a separate patch. > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > Link: https://lore.kernel.org/lkml/20240226065113.1690534-1-nick.hu@sifive.com/ > Suggested-by: Anup Patel <apatel@ventanamicro.com> > --- > drivers/cpuidle/cpuidle-riscv-sbi.c | 24 +++++++++++++++++++++--- > 1 file changed, 21 insertions(+), 3 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c > index a6e123dfe394..d6b01fc64f94 100644 > --- a/drivers/cpuidle/cpuidle-riscv-sbi.c > +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c > @@ -16,6 +16,7 @@ > #include <linux/kernel.h> > #include <linux/module.h> > #include <linux/of.h> > +#include <linux/of_platform.h> > #include <linux/slab.h> > #include <linux/platform_device.h> > #include <linux/pm_domain.h> > @@ -25,6 +26,7 @@ > #include <asm/smp.h> > #include <asm/suspend.h> > > +#include "cpuidle.h" > #include "dt_idle_states.h" > #include "dt_idle_genpd.h" > > @@ -336,6 +338,9 @@ static int sbi_cpuidle_init_cpu(struct device *dev, int cpu) > return ret; > } > > + if (cpuidle_disabled()) > + return 0; > + > ret = cpuidle_register(drv, NULL); > if (ret) > goto deinit; > @@ -380,20 +385,26 @@ static int sbi_cpuidle_pd_power_off(struct generic_pm_domain *pd) > struct sbi_pd_provider { > struct list_head link; > struct device_node *node; > + struct platform_device *pdev; > }; > > static LIST_HEAD(sbi_pd_providers); > > static int sbi_pd_init(struct device_node *np) > { > + struct platform_device *pdev; > struct generic_pm_domain *pd; > struct sbi_pd_provider *pd_provider; > struct dev_power_governor *pd_gov; > int ret = -ENOMEM; > > + pdev = of_platform_device_create(np, np->name, NULL); > + if (!pdev) > + goto out; > + > pd = dt_idle_pd_alloc(np, sbi_dt_parse_state_node); > if (!pd) > - goto out; > + goto free_pdev; > > pd_provider = kzalloc(sizeof(*pd_provider), GFP_KERNEL); > if (!pd_provider) > @@ -419,6 +430,7 @@ static int sbi_pd_init(struct device_node *np) > goto remove_pd; > > pd_provider->node = of_node_get(np); > + pd_provider->pdev = pdev; > list_add(&pd_provider->link, &sbi_pd_providers); > > pr_debug("init PM domain %s\n", pd->name); > @@ -430,6 +442,8 @@ static int sbi_pd_init(struct device_node *np) > kfree(pd_provider); > free_pd: > dt_idle_pd_free(pd); > +free_pdev: > + of_platform_device_destroy(&pdev->dev, NULL); > out: > pr_err("failed to init PM domain ret=%d %pOF\n", ret, np); > return ret; > @@ -447,6 +461,7 @@ static void sbi_pd_remove(void) > if (!IS_ERR(genpd)) > kfree(genpd); > > + of_platform_device_destroy(&pd_provider->pdev->dev, NULL); > of_node_put(pd_provider->node); > list_del(&pd_provider->link); > kfree(pd_provider); > @@ -548,7 +563,10 @@ static int sbi_cpuidle_probe(struct platform_device *pdev) > /* Setup CPU hotplut notifiers */ > sbi_idle_init_cpuhp(); > > - pr_info("idle driver registered for all CPUs\n"); > + if (cpuidle_disabled()) > + pr_info("cpuidle is disabled\n"); > + else > + pr_info("idle driver registered for all CPUs\n"); > > return 0; > > @@ -592,4 +610,4 @@ static int __init sbi_cpuidle_init(void) > > return 0; > } > -device_initcall(sbi_cpuidle_init); > +arch_initcall(sbi_cpuidle_init); > -- > 2.34.1 > Regards, Anup
Hi Anup Thanks for your feedback! On Tue, Aug 13, 2024 at 8:25 PM Anup Patel <anup@brainfault.org> wrote: > > On Tue, Aug 13, 2024 at 1:43 PM Nick Hu <nick.hu@sifive.com> wrote: > > > > To prevent the probe of consumer devices being deferred, create the > > platform devices for each pd node under '/cpus/power-domains' and move the > > driver initailization to the arch_initcall. > > To prevent the probe deferral of consumer devices, you can simply use > fwnode_dev_initialized() instead of creating a dummy platform device. > You are right. I'll remove the dummy platform device. If the fwnode_dev_initialized() was done before fw_devlink_create_devlink(), the link will be dropped. The fwnode_dev_initialized() is already included in the of_genpd_add_provider_simple() and If the fwnode_dev_initialized() was done before fw_devlink_create_devlink(), the link will be dropped. so I think we only need to move the driver initialization to the arch_initcall. Thanks! > > The consumer devices that inside the cpu/cluster power domain may register > > the genpd notifier where their power domains are point to the pd nodes > > under '/cpus/power-domains'. If the cpuidle.off==1, the genpd notifier > > will fail due to sbi_cpuidle_pd_allow_domain_state is not set. We also > > need the sbi_cpuidle_cpuhp_up/down to invoke the callbacks. Therefore, > > add a cpuidle_disabled() check before registering the cpuidle driver to > > address the issue. > > I think dealing with cpuidle.off==1 case should be a separate patch. > Sure, I'll update it in the next patch > > > > Signed-off-by: Nick Hu <nick.hu@sifive.com> > > Link: https://lore.kernel.org/lkml/20240226065113.1690534-1-nick.hu@sifive.com/ > > Suggested-by: Anup Patel <apatel@ventanamicro.com> > > --- > > drivers/cpuidle/cpuidle-riscv-sbi.c | 24 +++++++++++++++++++++--- > > 1 file changed, 21 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c > > index a6e123dfe394..d6b01fc64f94 100644 > > --- a/drivers/cpuidle/cpuidle-riscv-sbi.c > > +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c > > @@ -16,6 +16,7 @@ > > #include <linux/kernel.h> > > #include <linux/module.h> > > #include <linux/of.h> > > +#include <linux/of_platform.h> > > #include <linux/slab.h> > > #include <linux/platform_device.h> > > #include <linux/pm_domain.h> > > @@ -25,6 +26,7 @@ > > #include <asm/smp.h> > > #include <asm/suspend.h> > > > > +#include "cpuidle.h" > > #include "dt_idle_states.h" > > #include "dt_idle_genpd.h" > > > > @@ -336,6 +338,9 @@ static int sbi_cpuidle_init_cpu(struct device *dev, int cpu) > > return ret; > > } > > > > + if (cpuidle_disabled()) > > + return 0; > > + > > ret = cpuidle_register(drv, NULL); > > if (ret) > > goto deinit; > > @@ -380,20 +385,26 @@ static int sbi_cpuidle_pd_power_off(struct generic_pm_domain *pd) > > struct sbi_pd_provider { > > struct list_head link; > > struct device_node *node; > > + struct platform_device *pdev; > > }; > > > > static LIST_HEAD(sbi_pd_providers); > > > > static int sbi_pd_init(struct device_node *np) > > { > > + struct platform_device *pdev; > > struct generic_pm_domain *pd; > > struct sbi_pd_provider *pd_provider; > > struct dev_power_governor *pd_gov; > > int ret = -ENOMEM; > > > > + pdev = of_platform_device_create(np, np->name, NULL); > > + if (!pdev) > > + goto out; > > + > > pd = dt_idle_pd_alloc(np, sbi_dt_parse_state_node); > > if (!pd) > > - goto out; > > + goto free_pdev; > > > > pd_provider = kzalloc(sizeof(*pd_provider), GFP_KERNEL); > > if (!pd_provider) > > @@ -419,6 +430,7 @@ static int sbi_pd_init(struct device_node *np) > > goto remove_pd; > > > > pd_provider->node = of_node_get(np); > > + pd_provider->pdev = pdev; > > list_add(&pd_provider->link, &sbi_pd_providers); > > > > pr_debug("init PM domain %s\n", pd->name); > > @@ -430,6 +442,8 @@ static int sbi_pd_init(struct device_node *np) > > kfree(pd_provider); > > free_pd: > > dt_idle_pd_free(pd); > > +free_pdev: > > + of_platform_device_destroy(&pdev->dev, NULL); > > out: > > pr_err("failed to init PM domain ret=%d %pOF\n", ret, np); > > return ret; > > @@ -447,6 +461,7 @@ static void sbi_pd_remove(void) > > if (!IS_ERR(genpd)) > > kfree(genpd); > > > > + of_platform_device_destroy(&pd_provider->pdev->dev, NULL); > > of_node_put(pd_provider->node); > > list_del(&pd_provider->link); > > kfree(pd_provider); > > @@ -548,7 +563,10 @@ static int sbi_cpuidle_probe(struct platform_device *pdev) > > /* Setup CPU hotplut notifiers */ > > sbi_idle_init_cpuhp(); > > > > - pr_info("idle driver registered for all CPUs\n"); > > + if (cpuidle_disabled()) > > + pr_info("cpuidle is disabled\n"); > > + else > > + pr_info("idle driver registered for all CPUs\n"); > > > > return 0; > > > > @@ -592,4 +610,4 @@ static int __init sbi_cpuidle_init(void) > > > > return 0; > > } > > -device_initcall(sbi_cpuidle_init); > > +arch_initcall(sbi_cpuidle_init); > > -- > > 2.34.1 > > > > Regards, > Anup Regards, Nick
diff --git a/drivers/cpuidle/cpuidle-riscv-sbi.c b/drivers/cpuidle/cpuidle-riscv-sbi.c index a6e123dfe394..d6b01fc64f94 100644 --- a/drivers/cpuidle/cpuidle-riscv-sbi.c +++ b/drivers/cpuidle/cpuidle-riscv-sbi.c @@ -16,6 +16,7 @@ #include <linux/kernel.h> #include <linux/module.h> #include <linux/of.h> +#include <linux/of_platform.h> #include <linux/slab.h> #include <linux/platform_device.h> #include <linux/pm_domain.h> @@ -25,6 +26,7 @@ #include <asm/smp.h> #include <asm/suspend.h> +#include "cpuidle.h" #include "dt_idle_states.h" #include "dt_idle_genpd.h" @@ -336,6 +338,9 @@ static int sbi_cpuidle_init_cpu(struct device *dev, int cpu) return ret; } + if (cpuidle_disabled()) + return 0; + ret = cpuidle_register(drv, NULL); if (ret) goto deinit; @@ -380,20 +385,26 @@ static int sbi_cpuidle_pd_power_off(struct generic_pm_domain *pd) struct sbi_pd_provider { struct list_head link; struct device_node *node; + struct platform_device *pdev; }; static LIST_HEAD(sbi_pd_providers); static int sbi_pd_init(struct device_node *np) { + struct platform_device *pdev; struct generic_pm_domain *pd; struct sbi_pd_provider *pd_provider; struct dev_power_governor *pd_gov; int ret = -ENOMEM; + pdev = of_platform_device_create(np, np->name, NULL); + if (!pdev) + goto out; + pd = dt_idle_pd_alloc(np, sbi_dt_parse_state_node); if (!pd) - goto out; + goto free_pdev; pd_provider = kzalloc(sizeof(*pd_provider), GFP_KERNEL); if (!pd_provider) @@ -419,6 +430,7 @@ static int sbi_pd_init(struct device_node *np) goto remove_pd; pd_provider->node = of_node_get(np); + pd_provider->pdev = pdev; list_add(&pd_provider->link, &sbi_pd_providers); pr_debug("init PM domain %s\n", pd->name); @@ -430,6 +442,8 @@ static int sbi_pd_init(struct device_node *np) kfree(pd_provider); free_pd: dt_idle_pd_free(pd); +free_pdev: + of_platform_device_destroy(&pdev->dev, NULL); out: pr_err("failed to init PM domain ret=%d %pOF\n", ret, np); return ret; @@ -447,6 +461,7 @@ static void sbi_pd_remove(void) if (!IS_ERR(genpd)) kfree(genpd); + of_platform_device_destroy(&pd_provider->pdev->dev, NULL); of_node_put(pd_provider->node); list_del(&pd_provider->link); kfree(pd_provider); @@ -548,7 +563,10 @@ static int sbi_cpuidle_probe(struct platform_device *pdev) /* Setup CPU hotplut notifiers */ sbi_idle_init_cpuhp(); - pr_info("idle driver registered for all CPUs\n"); + if (cpuidle_disabled()) + pr_info("cpuidle is disabled\n"); + else + pr_info("idle driver registered for all CPUs\n"); return 0; @@ -592,4 +610,4 @@ static int __init sbi_cpuidle_init(void) return 0; } -device_initcall(sbi_cpuidle_init); +arch_initcall(sbi_cpuidle_init);
To prevent the probe of consumer devices being deferred, create the platform devices for each pd node under '/cpus/power-domains' and move the driver initailization to the arch_initcall. The consumer devices that inside the cpu/cluster power domain may register the genpd notifier where their power domains are point to the pd nodes under '/cpus/power-domains'. If the cpuidle.off==1, the genpd notifier will fail due to sbi_cpuidle_pd_allow_domain_state is not set. We also need the sbi_cpuidle_cpuhp_up/down to invoke the callbacks. Therefore, add a cpuidle_disabled() check before registering the cpuidle driver to address the issue. Signed-off-by: Nick Hu <nick.hu@sifive.com> Link: https://lore.kernel.org/lkml/20240226065113.1690534-1-nick.hu@sifive.com/ Suggested-by: Anup Patel <apatel@ventanamicro.com> --- drivers/cpuidle/cpuidle-riscv-sbi.c | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-)