| Message ID | 1358506755-13705-1-git-send-email-hdoyu@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Friday 18 January 2013 04:29 PM, Hiroshi Doyu wrote: > Add API to detect SCU base address from CP15. > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > --- > NOTE: > This wasn't delivered to linux-arm-kernel@lists.infradead.org, resending.... > > For usage: http://patchwork.ozlabs.org/patch/212013/ > --- > arch/arm/include/asm/smp_scu.h | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h > index 4eb6d00..f619eef 100644 > --- a/arch/arm/include/asm/smp_scu.h > +++ b/arch/arm/include/asm/smp_scu.h > @@ -6,6 +6,23 @@ > #define SCU_PM_POWEROFF 3 > > #ifndef __ASSEMBLER__ > + > +#include <asm/cputype.h> > + > +static inline phys_addr_t scu_get_base(void) > +{ > + phys_addr_t pa; > + unsigned long part_number = read_cpuid_part_number(); > + > + switch (part_number) { > + case ARM_CPU_PART_CORTEX_A9: > + /* Get SCU physical base */ > + asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa)); > + return pa; > + default: > + return 0; > + } > +} You may not need the switch case considering peripheral SCU is specific to A9 SOCs. Would just if like below is better ? phys_addr_t pa = 0; if (ARM_CPU_PART_CORTEX_A9 == read_cpuid_part_number()) asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa)); return pa; Regards, Santosh
On Fri, 18 Jan 2013 13:54:34 +0100 Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > On Friday 18 January 2013 04:29 PM, Hiroshi Doyu wrote: > > Add API to detect SCU base address from CP15. > > > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > > --- > > NOTE: > > This wasn't delivered to linux-arm-kernel@lists.infradead.org, resending.... > > > > For usage: http://patchwork.ozlabs.org/patch/212013/ > > --- > > arch/arm/include/asm/smp_scu.h | 17 +++++++++++++++++ > > 1 file changed, 17 insertions(+) > > > > diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h > > index 4eb6d00..f619eef 100644 > > --- a/arch/arm/include/asm/smp_scu.h > > +++ b/arch/arm/include/asm/smp_scu.h > > @@ -6,6 +6,23 @@ > > #define SCU_PM_POWEROFF 3 > > > > #ifndef __ASSEMBLER__ > > + > > +#include <asm/cputype.h> > > + > > +static inline phys_addr_t scu_get_base(void) > > +{ > > + phys_addr_t pa; > > + unsigned long part_number = read_cpuid_part_number(); > > + > > + switch (part_number) { > > + case ARM_CPU_PART_CORTEX_A9: > > + /* Get SCU physical base */ > > + asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa)); > > + return pa; > > + default: > > + return 0; > > + } > > +} > You may not need the switch case considering peripheral SCU is > specific to A9 SOCs. Would just if like below is better ? > > phys_addr_t pa = 0; > > if (ARM_CPU_PART_CORTEX_A9 == read_cpuid_part_number()) > asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa)); > return pa; I just considered the case if there will be another A?, which is SCU detectable, added later. If no possibility, yours would be enough.
On Friday 18 January 2013 07:59 PM, Hiroshi Doyu wrote: > On Fri, 18 Jan 2013 13:54:34 +0100 > Santosh Shilimkar <santosh.shilimkar@ti.com> wrote: > >> On Friday 18 January 2013 04:29 PM, Hiroshi Doyu wrote: >>> Add API to detect SCU base address from CP15. >>> >>> Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> >>> --- >>> NOTE: >>> This wasn't delivered to linux-arm-kernel@lists.infradead.org, resending.... >>> >>> For usage: http://patchwork.ozlabs.org/patch/212013/ >>> --- >>> arch/arm/include/asm/smp_scu.h | 17 +++++++++++++++++ >>> 1 file changed, 17 insertions(+) >>> >>> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h >>> index 4eb6d00..f619eef 100644 >>> --- a/arch/arm/include/asm/smp_scu.h >>> +++ b/arch/arm/include/asm/smp_scu.h >>> @@ -6,6 +6,23 @@ >>> #define SCU_PM_POWEROFF 3 >>> >>> #ifndef __ASSEMBLER__ >>> + >>> +#include <asm/cputype.h> >>> + >>> +static inline phys_addr_t scu_get_base(void) >>> +{ >>> + phys_addr_t pa; >>> + unsigned long part_number = read_cpuid_part_number(); >>> + >>> + switch (part_number) { >>> + case ARM_CPU_PART_CORTEX_A9: >>> + /* Get SCU physical base */ >>> + asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa)); >>> + return pa; >>> + default: >>> + return 0; >>> + } >>> +} >> You may not need the switch case considering peripheral SCU is >> specific to A9 SOCs. Would just if like below is better ? >> >> phys_addr_t pa = 0; >> >> if (ARM_CPU_PART_CORTEX_A9 == read_cpuid_part_number()) >> asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa)); >> return pa; > > I just considered the case if there will be another A?, which is SCU > detectable, added later. If no possibility, yours would be enough. > We can convert if into switch case if we need that in future :-) For now if() should be just fine. Feel free add my ack on updated patch. Regards, Santosh
On 01/18/2013 03:59 AM, Hiroshi Doyu wrote:
> Add API to detect SCU base address from CP15.
So this patch appears to be a dependency for the Tegra114 series from
Hiroshi. As such, I need to get it into the Tegra tree somehow.
What I'd like to propose is that I put this one patch in a topic branch
based on v3.8-rcN, send a pull request to arm-soc containing that
branch, and then merge the branch into the Tegra tree. This will allow
anyone else to use the patch in code destined for 3.9.
Or, if nobody else wants to use this function, I could just apply it
directly to the Tegra tree.
But either way, I'd need an ack from a core ARM maintainer in order to
apply this (well, v2 once it's posted).
On Mon, Jan 21, 2013 at 09:42:55AM +0200, Hiroshi Doyu wrote: > Add API to detect SCU base address from CP15. > > Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> > Acked-by: Santosh Shilimkar <santosh.shilimkar@ti.com> > --- > For usage: http://patchwork.ozlabs.org/patch/212013/ > --- > arch/arm/include/asm/smp_scu.h | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h > index 4eb6d00..1733ec7 100644 > --- a/arch/arm/include/asm/smp_scu.h > +++ b/arch/arm/include/asm/smp_scu.h > @@ -6,6 +6,18 @@ > #define SCU_PM_POWEROFF 3 > > #ifndef __ASSEMBLER__ > + > +#include <asm/cputype.h> > + > +static inline phys_addr_t scu_get_base(void) > +{ > + if (read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9) { > + phys_addr_t pa; > + asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa)); > + return pa; > + } > + return 0; > +} > unsigned int scu_get_core_count(void __iomem *); > void scu_enable(void __iomem *); > int scu_power_mode(void __iomem *, unsigned int); Not sure what iteration this patch is at but... it's easy to avoid more iterations when you review the patch yourself before sending. Reasonable coding style suggests there should be a blank line after the new } and before the prototypes. However, as I _am_ commenting on this patch because of the above, I'll also suggest that we don't do it like this. And actually, the above code is buggy. If phys_addr_t is 64-bit, the upper half of 'pa' won't be set. I'd suggest this instead: static inline bool scu_a9_has_base(void) { return read_cpuid_part_number() == ARM_CPU_PART_CORTEX_A9; } static inline unsigned long scu_a9_get_base(void) { unsigned long pa; asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa)); return pa; } and let the user of these functions decide whether to read it using scu_a9_has_base(). And why 'unsigned long' ? Well, it could also be u32, but on 32-bit ARM, it's been more conventional to use unsigned long for phys addresses which can't be larger than 32-bit - which this one can't because the mrc instruction is limited to writing one 32-bit register.
diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h index 4eb6d00..f619eef 100644 --- a/arch/arm/include/asm/smp_scu.h +++ b/arch/arm/include/asm/smp_scu.h @@ -6,6 +6,23 @@ #define SCU_PM_POWEROFF 3 #ifndef __ASSEMBLER__ + +#include <asm/cputype.h> + +static inline phys_addr_t scu_get_base(void) +{ + phys_addr_t pa; + unsigned long part_number = read_cpuid_part_number(); + + switch (part_number) { + case ARM_CPU_PART_CORTEX_A9: + /* Get SCU physical base */ + asm("mrc p15, 4, %0, c15, c0, 0" : "=r" (pa)); + return pa; + default: + return 0; + } +} unsigned int scu_get_core_count(void __iomem *); void scu_enable(void __iomem *); int scu_power_mode(void __iomem *, unsigned int);
Add API to detect SCU base address from CP15. Signed-off-by: Hiroshi Doyu <hdoyu@nvidia.com> --- NOTE: This wasn't delivered to linux-arm-kernel@lists.infradead.org, resending.... For usage: http://patchwork.ozlabs.org/patch/212013/ --- arch/arm/include/asm/smp_scu.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)