| Message ID | 1414194024-55547-2-git-send-email-lina.iyer@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 10/25/2014 01:40 AM, Lina Iyer wrote: > Set the warmboot address using an SCM call, only if the new address is > different than the old one. Please could you elaborate why a new address can be changed ? > Signed-off-by: Lina Iyer <lina.iyer@linaro.org> > --- > drivers/soc/qcom/scm-boot.c | 22 ++++++++++++++++++++++ > include/soc/qcom/scm-boot.h | 1 + > 2 files changed, 23 insertions(+) > > diff --git a/drivers/soc/qcom/scm-boot.c b/drivers/soc/qcom/scm-boot.c > index 60ff7b4..5710967 100644 > --- a/drivers/soc/qcom/scm-boot.c > +++ b/drivers/soc/qcom/scm-boot.c > @@ -37,3 +37,25 @@ int scm_set_boot_addr(phys_addr_t addr, int flags) > &cmd, sizeof(cmd), NULL, 0); > } > EXPORT_SYMBOL(scm_set_boot_addr); > + > + extra line. > +int scm_set_warm_boot_addr(void *entry, int cpu) > +{ > + static int flags[NR_CPUS] = { > + SCM_FLAG_WARMBOOT_CPU0, > + SCM_FLAG_WARMBOOT_CPU1, > + SCM_FLAG_WARMBOOT_CPU2, > + SCM_FLAG_WARMBOOT_CPU3, > + }; Please do not do that, you don't know what NR_CPUS value could be in the future with the single kernel image and that could lead to a bug very hard to find. The kernel stack is 4096. Move this out of the function: static int scm_flags[] = { SCM_FLAG_WARMBOOT_CPU0, SCM_FLAG_WARMBOOT_CPU1, SCM_FLAG_WARMBOOT_CPU2, SCM_FLAG_WARMBOOT_CPU3, }; > + static DEFINE_PER_CPU(void *, last_known_entry); It sounds odd to add those static declaration here even if I understand that is to encapsulate them. > + int ret; > + > + if (entry == per_cpu(last_known_entry, cpu)) > + return 0; My question is: why scm_set_warm_boot_addr could be called with different addresses ? If this is really needed, please replace the per_cpu variable by: struct scm_boot_addr { int flag; phys_addr_t entry; }; static struct scm_boot_addr scm_flags[] = { { SCM_FLAG_WARMBOOT_CPU0, }, { SCM_FLAG_WARMBOOT_CPU1, }, { SCM_FLAG_WARMBOOT_CPU2, }, { SCM_FLAG_WARMBOOT_CPU3, }, }; > + ret = scm_set_boot_addr(virt_to_phys(entry), flags[cpu]); > + if (!ret) > + per_cpu(last_known_entry, cpu) = entry; > + > + return ret; > +} > diff --git a/include/soc/qcom/scm-boot.h b/include/soc/qcom/scm-boot.h > index 02b445c..100938b 100644 > --- a/include/soc/qcom/scm-boot.h > +++ b/include/soc/qcom/scm-boot.h > @@ -22,5 +22,6 @@ > #define SCM_FLAG_WARMBOOT_CPU3 0x40 By the way, if you look for encapsulation, perhaps these macros could be moved into scm-boot.c, no ? > int scm_set_boot_addr(phys_addr_t addr, int flags); > +int scm_set_warm_boot_addr(void *entry, int cpu); > > #endif >
On Fri, Nov 14 2014 at 01:30 -0700, Daniel Lezcano wrote: >On 10/25/2014 01:40 AM, Lina Iyer wrote: >>Set the warmboot address using an SCM call, only if the new address is >>different than the old one. > >Please could you elaborate why a new address can be changed ? > Hotplug could have a different warmboot address. >>Signed-off-by: Lina Iyer <lina.iyer@linaro.org> >>--- >> drivers/soc/qcom/scm-boot.c | 22 ++++++++++++++++++++++ >> include/soc/qcom/scm-boot.h | 1 + >> 2 files changed, 23 insertions(+) >> >>diff --git a/drivers/soc/qcom/scm-boot.c b/drivers/soc/qcom/scm-boot.c >>index 60ff7b4..5710967 100644 >>--- a/drivers/soc/qcom/scm-boot.c >>+++ b/drivers/soc/qcom/scm-boot.c >>@@ -37,3 +37,25 @@ int scm_set_boot_addr(phys_addr_t addr, int flags) >> &cmd, sizeof(cmd), NULL, 0); >> } >> EXPORT_SYMBOL(scm_set_boot_addr); >>+ >>+ > >extra line. > >>+int scm_set_warm_boot_addr(void *entry, int cpu) >>+{ >>+ static int flags[NR_CPUS] = { >>+ SCM_FLAG_WARMBOOT_CPU0, >>+ SCM_FLAG_WARMBOOT_CPU1, >>+ SCM_FLAG_WARMBOOT_CPU2, >>+ SCM_FLAG_WARMBOOT_CPU3, >>+ }; > >Please do not do that, you don't know what NR_CPUS value could be in >the future with the single kernel image and that could lead to a bug >very hard to find. The kernel stack is 4096. > >Move this out of the function: > >static int scm_flags[] = { > SCM_FLAG_WARMBOOT_CPU0, > SCM_FLAG_WARMBOOT_CPU1, > SCM_FLAG_WARMBOOT_CPU2, > SCM_FLAG_WARMBOOT_CPU3, >}; Sure. > >>+ static DEFINE_PER_CPU(void *, last_known_entry); > >It sounds odd to add those static declaration here even if I >understand that is to encapsulate them. > Sure. >>+ int ret; >>+ >>+ if (entry == per_cpu(last_known_entry, cpu)) >>+ return 0; > >My question is: why scm_set_warm_boot_addr could be called with >different addresses ? > >If this is really needed, please replace the per_cpu variable by: > >struct scm_boot_addr { > int flag; > phys_addr_t entry; >}; > >static struct scm_boot_addr scm_flags[] = { > { SCM_FLAG_WARMBOOT_CPU0, }, > { SCM_FLAG_WARMBOOT_CPU1, }, > { SCM_FLAG_WARMBOOT_CPU2, }, > { SCM_FLAG_WARMBOOT_CPU3, }, >}; > Hmm.. OK. >>+ ret = scm_set_boot_addr(virt_to_phys(entry), flags[cpu]); >>+ if (!ret) >>+ per_cpu(last_known_entry, cpu) = entry; >>+ >>+ return ret; >>+} >>diff --git a/include/soc/qcom/scm-boot.h b/include/soc/qcom/scm-boot.h >>index 02b445c..100938b 100644 >>--- a/include/soc/qcom/scm-boot.h >>+++ b/include/soc/qcom/scm-boot.h >>@@ -22,5 +22,6 @@ >> #define SCM_FLAG_WARMBOOT_CPU3 0x40 > >By the way, if you look for encapsulation, perhaps these macros could >be moved into scm-boot.c, no ? > Dont think anybody outside the file needs it. >> int scm_set_boot_addr(phys_addr_t addr, int flags); >>+int scm_set_warm_boot_addr(void *entry, int cpu); >> >> #endif >> > > >-- > <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs > >Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | ><http://twitter.com/#!/linaroorg> Twitter | ><http://www.linaro.org/linaro-blog/> Blog > -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/soc/qcom/scm-boot.c b/drivers/soc/qcom/scm-boot.c index 60ff7b4..5710967 100644 --- a/drivers/soc/qcom/scm-boot.c +++ b/drivers/soc/qcom/scm-boot.c @@ -37,3 +37,25 @@ int scm_set_boot_addr(phys_addr_t addr, int flags) &cmd, sizeof(cmd), NULL, 0); } EXPORT_SYMBOL(scm_set_boot_addr); + + +int scm_set_warm_boot_addr(void *entry, int cpu) +{ + static int flags[NR_CPUS] = { + SCM_FLAG_WARMBOOT_CPU0, + SCM_FLAG_WARMBOOT_CPU1, + SCM_FLAG_WARMBOOT_CPU2, + SCM_FLAG_WARMBOOT_CPU3, + }; + static DEFINE_PER_CPU(void *, last_known_entry); + int ret; + + if (entry == per_cpu(last_known_entry, cpu)) + return 0; + + ret = scm_set_boot_addr(virt_to_phys(entry), flags[cpu]); + if (!ret) + per_cpu(last_known_entry, cpu) = entry; + + return ret; +} diff --git a/include/soc/qcom/scm-boot.h b/include/soc/qcom/scm-boot.h index 02b445c..100938b 100644 --- a/include/soc/qcom/scm-boot.h +++ b/include/soc/qcom/scm-boot.h @@ -22,5 +22,6 @@ #define SCM_FLAG_WARMBOOT_CPU3 0x40 int scm_set_boot_addr(phys_addr_t addr, int flags); +int scm_set_warm_boot_addr(void *entry, int cpu); #endif
Set the warmboot address using an SCM call, only if the new address is different than the old one. Signed-off-by: Lina Iyer <lina.iyer@linaro.org> --- drivers/soc/qcom/scm-boot.c | 22 ++++++++++++++++++++++ include/soc/qcom/scm-boot.h | 1 + 2 files changed, 23 insertions(+)