| Message ID | 20210923132046.1860549-3-claudiu.beznea@microchip.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | clk: at91: updates for power management and dvfs | expand |
Quoting Claudiu Beznea (2021-09-23 06:20:31) > Before going to backup mode architecture specific PM code sets the first > word in securam (file arch/arm/mach-at91/pm.c, function at91_pm_begin()). > Thus take this into account when suspending/resuming clocks. This will > avoid executing unnecessary instructions when suspending to non backup > modes. Also this commit changed the postcore_initcall() with > subsys_initcall() to be able to execute of_find_compatible_node() since > this was not available at the moment of postcore_initcall(). This should > not alter the tcb_clksrc since the changes are related to clocks > suspend/resume procedure that will be executed at the user space request, > thus long ago after subsys_initcall(). Is the comment still relevant though? > > Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > --- > diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c > index b2806946a77a..58e9c088cb22 100644 > --- a/drivers/clk/at91/pmc.c > +++ b/drivers/clk/at91/pmc.c > @@ -110,13 +112,35 @@ struct pmc_data *pmc_data_allocate(unsigned int ncore, unsigned int nsystem, > } > > #ifdef CONFIG_PM > + > +/* Address in SECURAM that say if we suspend to backup mode. */ > +static void __iomem *at91_pmc_backup_suspend; > + > static int at91_pmc_suspend(void) > { > + unsigned int backup; > + > + if (!at91_pmc_backup_suspend) > + return 0; > + > + backup = *(unsigned int *)at91_pmc_backup_suspend; This will fail sparse. Why are we reading iomem without using iomem reading wrapper? > + if (!backup)
On 08.10.2021 06:51, Stephen Boyd wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Quoting Claudiu Beznea (2021-09-23 06:20:31) >> Before going to backup mode architecture specific PM code sets the first >> word in securam (file arch/arm/mach-at91/pm.c, function at91_pm_begin()). >> Thus take this into account when suspending/resuming clocks. This will >> avoid executing unnecessary instructions when suspending to non backup >> modes. Also this commit changed the postcore_initcall() with >> subsys_initcall() to be able to execute of_find_compatible_node() since >> this was not available at the moment of postcore_initcall(). This should >> not alter the tcb_clksrc since the changes are related to clocks >> suspend/resume procedure that will be executed at the user space request, >> thus long ago after subsys_initcall(). > > Is the comment still relevant though? For architecture PM code yes, the securam is set in [1]. Related to replacing postcore_init() with subsys_initcall() to be able to have the proper result of of_find_compatible_node() I have to re-check (don't know if something has been changed in this area since January). If you know something please let me know. [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-at91/pm.c#n290 > >> >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> >> --- >> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c >> index b2806946a77a..58e9c088cb22 100644 >> --- a/drivers/clk/at91/pmc.c >> +++ b/drivers/clk/at91/pmc.c >> @@ -110,13 +112,35 @@ struct pmc_data *pmc_data_allocate(unsigned int ncore, unsigned int nsystem, >> } >> >> #ifdef CONFIG_PM >> + >> +/* Address in SECURAM that say if we suspend to backup mode. */ >> +static void __iomem *at91_pmc_backup_suspend; >> + >> static int at91_pmc_suspend(void) >> { >> + unsigned int backup; >> + >> + if (!at91_pmc_backup_suspend) >> + return 0; >> + >> + backup = *(unsigned int *)at91_pmc_backup_suspend; > > This will fail sparse. Why are we reading iomem without using iomem > reading wrapper? By mistake. I'll switch to iomem reading wrapper. Is it OK to send soon a new version with these adjustments or do you have other patches in this series to review? Thank you, Claudiu Beznea > >> + if (!backup)
Quoting Claudiu.Beznea@microchip.com (2021-10-07 23:47:14) > On 08.10.2021 06:51, Stephen Boyd wrote: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Quoting Claudiu Beznea (2021-09-23 06:20:31) > >> Before going to backup mode architecture specific PM code sets the first > >> word in securam (file arch/arm/mach-at91/pm.c, function at91_pm_begin()). > >> Thus take this into account when suspending/resuming clocks. This will > >> avoid executing unnecessary instructions when suspending to non backup > >> modes. Also this commit changed the postcore_initcall() with > >> subsys_initcall() to be able to execute of_find_compatible_node() since > >> this was not available at the moment of postcore_initcall(). This should > >> not alter the tcb_clksrc since the changes are related to clocks > >> suspend/resume procedure that will be executed at the user space request, > >> thus long ago after subsys_initcall(). > > > > Is the comment still relevant though? > > For architecture PM code yes, the securam is set in [1]. > > Related to replacing postcore_init() with subsys_initcall() to be able to > have the proper result of of_find_compatible_node() I have to re-check > (don't know if something has been changed in this area since January). If > you know something please let me know. I mostly don't want to lose the comment if it is still useful. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm/mach-at91/pm.c#n290 > > > > >> > >> Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> > >> --- > >> diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c > >> index b2806946a77a..58e9c088cb22 100644 > >> --- a/drivers/clk/at91/pmc.c > >> +++ b/drivers/clk/at91/pmc.c > >> @@ -110,13 +112,35 @@ struct pmc_data *pmc_data_allocate(unsigned int ncore, unsigned int nsystem, > >> } > >> > >> #ifdef CONFIG_PM > >> + > >> +/* Address in SECURAM that say if we suspend to backup mode. */ > >> +static void __iomem *at91_pmc_backup_suspend; > >> + > >> static int at91_pmc_suspend(void) > >> { > >> + unsigned int backup; > >> + > >> + if (!at91_pmc_backup_suspend) > >> + return 0; > >> + > >> + backup = *(unsigned int *)at91_pmc_backup_suspend; > > > > This will fail sparse. Why are we reading iomem without using iomem > > reading wrapper? > > By mistake. I'll switch to iomem reading wrapper. > > Is it OK to send soon a new version with these adjustments or do you have > other patches in this series to review? > Feel free to resend.
diff --git a/drivers/clk/at91/pmc.c b/drivers/clk/at91/pmc.c index b2806946a77a..58e9c088cb22 100644 --- a/drivers/clk/at91/pmc.c +++ b/drivers/clk/at91/pmc.c @@ -8,6 +8,8 @@ #include <linux/clkdev.h> #include <linux/clk/at91_pmc.h> #include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_platform.h> #include <linux/mfd/syscon.h> #include <linux/platform_device.h> #include <linux/regmap.h> @@ -110,13 +112,35 @@ struct pmc_data *pmc_data_allocate(unsigned int ncore, unsigned int nsystem, } #ifdef CONFIG_PM + +/* Address in SECURAM that say if we suspend to backup mode. */ +static void __iomem *at91_pmc_backup_suspend; + static int at91_pmc_suspend(void) { + unsigned int backup; + + if (!at91_pmc_backup_suspend) + return 0; + + backup = *(unsigned int *)at91_pmc_backup_suspend; + if (!backup) + return 0; + return clk_save_context(); } static void at91_pmc_resume(void) { + unsigned int backup; + + if (!at91_pmc_backup_suspend) + return; + + backup = *(unsigned int *)at91_pmc_backup_suspend; + if (!backup) + return; + clk_restore_context(); } @@ -132,6 +156,7 @@ static const struct of_device_id sama5d2_pmc_dt_ids[] = { static int __init pmc_register_ops(void) { + struct platform_device *pdev; struct device_node *np; np = of_find_matching_node(NULL, sama5d2_pmc_dt_ids); @@ -144,10 +169,30 @@ static int __init pmc_register_ops(void) } of_node_put(np); + np = of_find_compatible_node(NULL, NULL, "atmel,sama5d2-securam"); + if (!np) + return -ENODEV; + + if (!of_device_is_available(np)) { + of_node_put(np); + return -ENODEV; + } + + pdev = of_find_device_by_node(np); + of_node_put(np); + if (!pdev) + return -ENODEV; + + at91_pmc_backup_suspend = of_iomap(np, 0); + if (!at91_pmc_backup_suspend) { + pr_warn("%s(): unable to map securam\n", __func__); + return -ENOMEM; + } + register_syscore_ops(&pmc_syscore_ops); return 0; } -/* This has to happen before arch_initcall because of the tcb_clksrc driver */ -postcore_initcall(pmc_register_ops); + +subsys_initcall(pmc_register_ops); #endif
Before going to backup mode architecture specific PM code sets the first word in securam (file arch/arm/mach-at91/pm.c, function at91_pm_begin()). Thus take this into account when suspending/resuming clocks. This will avoid executing unnecessary instructions when suspending to non backup modes. Also this commit changed the postcore_initcall() with subsys_initcall() to be able to execute of_find_compatible_node() since this was not available at the moment of postcore_initcall(). This should not alter the tcb_clksrc since the changes are related to clocks suspend/resume procedure that will be executed at the user space request, thus long ago after subsys_initcall(). Signed-off-by: Claudiu Beznea <claudiu.beznea@microchip.com> --- drivers/clk/at91/pmc.c | 49 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 47 insertions(+), 2 deletions(-)