Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Pankaj Dubey]

On 18 November 2016 at 19:02, Arnd Bergmann  wrote:
> On Friday, November 18, 2016 12:48:07 PM CET Russell King - ARM Linux wrote:
>> On Fri, Nov 18, 2016 at 01:14:35PM +0100, Arnd Bergmann wrote:
>> > @@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
>> >  {
>> >   u32 scu_ctrl;
>> >
>> > + if (scu_base)
>> > + scu_base = scu_base_addr;
>> > +
>>
>> This looks to me like nonsense.
>>
>> >  #ifdef CONFIG_ARM_ERRATA_764369
>> >   /* Cortex-A9 only */
>> >   if ((read_cpuid_id() & 0xff00) == 0x410fc090) {
>> > @@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int 
>> > mode)
>> >   unsigned int val;
>> >   int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 
>> > 0);
>> >
>> > + if (scu_base)
>> > + scu_base = scu_base_addr;
>> > +
>>
>> Ditto.
>>
>> Rather than doing this, I'd much prefer to always store the SCU base in
>> the SCU code, and remove the "void __iomem *scu_base" argment from all
>> these functions.
>
> Ok, then we just need one scu_probe_*() variant for each of the
> four methods of initializing it (iotable, of_iomap,
> ioremap(scu_a9_get_base) and hardcoded.
>
> The intention of doing the fallback for the NULL argument was
> to avoid having to add lots of new API while also allowing
> the change to be done one platform at a time.
>
> If we remove the argument from the other functions, they either
> need to get a new name, or we change them all to the new prototype
> at once. Either way works fine, do you have a preference between
> them?
>

Russell,

Any opinion on this. Are you OK, with the approach suggested by Arnd?

Thanks,
Pankaj Dubey

> Arnd
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Pankaj Dubey]

On 18 November 2016 at 19:02, Arnd Bergmann  wrote:
> On Friday, November 18, 2016 12:48:07 PM CET Russell King - ARM Linux wrote:
>> On Fri, Nov 18, 2016 at 01:14:35PM +0100, Arnd Bergmann wrote:
>> > @@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
>> >  {
>> >   u32 scu_ctrl;
>> >
>> > + if (scu_base)
>> > + scu_base = scu_base_addr;
>> > +
>>
>> This looks to me like nonsense.
>>
>> >  #ifdef CONFIG_ARM_ERRATA_764369
>> >   /* Cortex-A9 only */
>> >   if ((read_cpuid_id() & 0xff00) == 0x410fc090) {
>> > @@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int 
>> > mode)
>> >   unsigned int val;
>> >   int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 
>> > 0);
>> >
>> > + if (scu_base)
>> > + scu_base = scu_base_addr;
>> > +
>>
>> Ditto.
>>
>> Rather than doing this, I'd much prefer to always store the SCU base in
>> the SCU code, and remove the "void __iomem *scu_base" argment from all
>> these functions.
>
> Ok, then we just need one scu_probe_*() variant for each of the
> four methods of initializing it (iotable, of_iomap,
> ioremap(scu_a9_get_base) and hardcoded.
>
> The intention of doing the fallback for the NULL argument was
> to avoid having to add lots of new API while also allowing
> the change to be done one platform at a time.
>
> If we remove the argument from the other functions, they either
> need to get a new name, or we change them all to the new prototype
> at once. Either way works fine, do you have a preference between
> them?
>

Russell,

Any opinion on this. Are you OK, with the approach suggested by Arnd?

Thanks,
Pankaj Dubey

> Arnd
>
> ___
> linux-arm-kernel mailing list
> linux-arm-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Friday, November 18, 2016 12:48:07 PM CET Russell King - ARM Linux wrote:
> On Fri, Nov 18, 2016 at 01:14:35PM +0100, Arnd Bergmann wrote:
> > @@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
> >  {
> >   u32 scu_ctrl;
> >  
> > + if (scu_base)
> > + scu_base = scu_base_addr;
> > +
> 
> This looks to me like nonsense.
> 
> >  #ifdef CONFIG_ARM_ERRATA_764369
> >   /* Cortex-A9 only */
> >   if ((read_cpuid_id() & 0xff00) == 0x410fc090) {
> > @@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int 
> > mode)
> >   unsigned int val;
> >   int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 
> > 0);
> >  
> > + if (scu_base)
> > + scu_base = scu_base_addr;
> > +
> 
> Ditto.
> 
> Rather than doing this, I'd much prefer to always store the SCU base in
> the SCU code, and remove the "void __iomem *scu_base" argment from all
> these functions.

Ok, then we just need one scu_probe_*() variant for each of the
four methods of initializing it (iotable, of_iomap,
ioremap(scu_a9_get_base) and hardcoded.

The intention of doing the fallback for the NULL argument was
to avoid having to add lots of new API while also allowing
the change to be done one platform at a time.

If we remove the argument from the other functions, they either
need to get a new name, or we change them all to the new prototype
at once. Either way works fine, do you have a preference between
them?

Arnd

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Friday, November 18, 2016 12:48:07 PM CET Russell King - ARM Linux wrote:
> On Fri, Nov 18, 2016 at 01:14:35PM +0100, Arnd Bergmann wrote:
> > @@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
> >  {
> >   u32 scu_ctrl;
> >  
> > + if (scu_base)
> > + scu_base = scu_base_addr;
> > +
> 
> This looks to me like nonsense.
> 
> >  #ifdef CONFIG_ARM_ERRATA_764369
> >   /* Cortex-A9 only */
> >   if ((read_cpuid_id() & 0xff00) == 0x410fc090) {
> > @@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int 
> > mode)
> >   unsigned int val;
> >   int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 
> > 0);
> >  
> > + if (scu_base)
> > + scu_base = scu_base_addr;
> > +
> 
> Ditto.
> 
> Rather than doing this, I'd much prefer to always store the SCU base in
> the SCU code, and remove the "void __iomem *scu_base" argment from all
> these functions.

Ok, then we just need one scu_probe_*() variant for each of the
four methods of initializing it (iotable, of_iomap,
ioremap(scu_a9_get_base) and hardcoded.

The intention of doing the fallback for the NULL argument was
to avoid having to add lots of new API while also allowing
the change to be done one platform at a time.

If we remove the argument from the other functions, they either
need to get a new name, or we change them all to the new prototype
at once. Either way works fine, do you have a preference between
them?

Arnd

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Russell King - ARM Linux]

On Fri, Nov 18, 2016 at 01:14:35PM +0100, Arnd Bergmann wrote:
> @@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
>  {
>   u32 scu_ctrl;
>  
> + if (scu_base)
> + scu_base = scu_base_addr;
> +

This looks to me like nonsense.

>  #ifdef CONFIG_ARM_ERRATA_764369
>   /* Cortex-A9 only */
>   if ((read_cpuid_id() & 0xff00) == 0x410fc090) {
> @@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int 
> mode)
>   unsigned int val;
>   int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
>  
> + if (scu_base)
> + scu_base = scu_base_addr;
> +

Ditto.

Rather than doing this, I'd much prefer to always store the SCU base in
the SCU code, and remove the "void __iomem *scu_base" argment from all
these functions.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Russell King - ARM Linux]

On Fri, Nov 18, 2016 at 01:14:35PM +0100, Arnd Bergmann wrote:
> @@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
>  {
>   u32 scu_ctrl;
>  
> + if (scu_base)
> + scu_base = scu_base_addr;
> +

This looks to me like nonsense.

>  #ifdef CONFIG_ARM_ERRATA_764369
>   /* Cortex-A9 only */
>   if ((read_cpuid_id() & 0xff00) == 0x410fc090) {
> @@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int 
> mode)
>   unsigned int val;
>   int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
>  
> + if (scu_base)
> + scu_base = scu_base_addr;
> +

Ditto.

Rather than doing this, I'd much prefer to always store the SCU base in
the SCU code, and remove the "void __iomem *scu_base" argment from all
these functions.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Friday, November 18, 2016 8:54:30 AM CET pankaj.dubey wrote:
> >> Please let me know if any concern in this approach.
> > 
> > I think ideally we wouldn't even need to know the virtual address
> > outside of smp_scu.c. If we can move all users of the address
> > into that file directly, it could become a local variable and
> > we change scu_power_mode() and scu_get_core_count() instead to
> > not require the address argument.
> > 
> 
> If we change scu_get_core_count() without address argument, what about
> those platforms which are calling this API very early boot (mostly in
> smp_init_cpus), during this stage we can't use iomap. These platforms
> are doing static mapping of SCU base, and passing virtual address.
> 
> Currently following are platforms which needs SCU virtual address at
> very early stage mostly for calling scu_get_core_count(scu_address):
> IMX, ZYNQ, SPEAr, and OMAP2.

Ah, you are right, I missed how this is done earlier than the
scu_enable() call.

> I can think of handling of these platform's early need of SCU in the
> following way:
> 
> Probably we need something like early_a9_scu_get_core_count() which can
> be handled in this way in smp_scu.c file itself:
> 
> +static struct map_desc scu_map __initdata = {
> +   .length = SZ_256,
> +   .type   = MT_DEVICE,
> +};
> +
> +static void __iomem *early_scu_map_io(void)
> +{
> +   unsigned long base;
> +   void __iomem *scu_base;
> +
> +   base = scu_a9_get_base();
> +   scu_map.pfn = __phys_to_pfn(base);
> +   scu_map.virtual = base;
> +   iotable_init(_map, 1);
> +   scu_base = (void __iomem *)base;
> +   return scu_base;
> +}

Unfortunately, this doesn't work because scu_map.virtual must be
picked by the platform code in a way that doesn't conflict
with anything else.  Setting up the iotable is probably not
something we should move into the smp_scu.c file but leave where
it currently is. You copied the trick from zynq that happens
to work there but probably not on the other three.

At some point we decided that at least new platforms should
always get the core count from DT rather than from the SCU,
but I don't know if we have to keep supporting old DTB files
without a full description of the CPUs on any of the
four platforms.

I see that there are four other users of scu_get_core_count()
that all call it from ->prepare_cpus, and we can probably
just use num_possible_cpus() at that point as the count
must have been initialized from DT already. 

> +/*
> + * early_a9_scu_get_core_count - Get number of CPU cores at very early boot
> + * Only platforms which needs number_of_cores during early boot should
> call this
> + */
> +unsigned int __init early_a9_scu_get_core_count(void)
> +{
> +   void __iomem *scu_base;
> +
> +   scu_base = early_scu_map_io();
> +   return scu_get_core_count(scu_base);
> +}
> +
> 
> Please let me know how about using above approach to simplify platform
> specific code of early users of SCU address.
> 
> Also for rest platforms, which are not using scu base at very early
> stage, but are using virtual address which is mapped either from SCU
> device node or using s9_scu_get_base() to map to SCU virtual address. To
> separate these two we discussed to separate scu_enable in two helper
> APIs as of_scu_enable and s9_scu_enable. So if we change
> scu_get_core_count which do not requires address argument, this also
> needs to be separated in two as of_scu_get_core_count and
> s9_scu_get_core_count.

We can probably leave get_core_count code alone for now, or
we change it so that we pass a virtual address into it
from the platform and have the SCU mapped there. One nice
property of the early mapping is that the later ioremap()
will just return the same virtual address, so we don't get
a double mapping when calling the scu_enable variant later.

Adding two functions of each doesn't sound too great though,
it would make the API more complicated when the intention was
to make it simpler by leaving out the address argument.

Maybe something like this?

diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 72f9241ad5db..c248a16e980f 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -24,6 +24,8 @@
 #define SCU_INVALIDATE 0x0c
 #define SCU_FPGA_REVISION  0x10
 
+static void __iomem *scu_base_addr;
+
 #ifdef CONFIG_SMP
 /*
  * Get the number of CPU cores from the SCU configuration
@@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
 {
u32 scu_ctrl;
 
+   if (scu_base)
+   scu_base = scu_base_addr;
+
 #ifdef CONFIG_ARM_ERRATA_764369
/* Cortex-A9 only */
if ((read_cpuid_id() & 0xff00) == 0x410fc090) {
@@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
unsigned int val;
int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
 
+   if (scu_base)
+   scu_base = scu_base_addr;
+
if (mode > 3 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Friday, November 18, 2016 8:54:30 AM CET pankaj.dubey wrote:
> >> Please let me know if any concern in this approach.
> > 
> > I think ideally we wouldn't even need to know the virtual address
> > outside of smp_scu.c. If we can move all users of the address
> > into that file directly, it could become a local variable and
> > we change scu_power_mode() and scu_get_core_count() instead to
> > not require the address argument.
> > 
> 
> If we change scu_get_core_count() without address argument, what about
> those platforms which are calling this API very early boot (mostly in
> smp_init_cpus), during this stage we can't use iomap. These platforms
> are doing static mapping of SCU base, and passing virtual address.
> 
> Currently following are platforms which needs SCU virtual address at
> very early stage mostly for calling scu_get_core_count(scu_address):
> IMX, ZYNQ, SPEAr, and OMAP2.

Ah, you are right, I missed how this is done earlier than the
scu_enable() call.

> I can think of handling of these platform's early need of SCU in the
> following way:
> 
> Probably we need something like early_a9_scu_get_core_count() which can
> be handled in this way in smp_scu.c file itself:
> 
> +static struct map_desc scu_map __initdata = {
> +   .length = SZ_256,
> +   .type   = MT_DEVICE,
> +};
> +
> +static void __iomem *early_scu_map_io(void)
> +{
> +   unsigned long base;
> +   void __iomem *scu_base;
> +
> +   base = scu_a9_get_base();
> +   scu_map.pfn = __phys_to_pfn(base);
> +   scu_map.virtual = base;
> +   iotable_init(_map, 1);
> +   scu_base = (void __iomem *)base;
> +   return scu_base;
> +}

Unfortunately, this doesn't work because scu_map.virtual must be
picked by the platform code in a way that doesn't conflict
with anything else.  Setting up the iotable is probably not
something we should move into the smp_scu.c file but leave where
it currently is. You copied the trick from zynq that happens
to work there but probably not on the other three.

At some point we decided that at least new platforms should
always get the core count from DT rather than from the SCU,
but I don't know if we have to keep supporting old DTB files
without a full description of the CPUs on any of the
four platforms.

I see that there are four other users of scu_get_core_count()
that all call it from ->prepare_cpus, and we can probably
just use num_possible_cpus() at that point as the count
must have been initialized from DT already. 

> +/*
> + * early_a9_scu_get_core_count - Get number of CPU cores at very early boot
> + * Only platforms which needs number_of_cores during early boot should
> call this
> + */
> +unsigned int __init early_a9_scu_get_core_count(void)
> +{
> +   void __iomem *scu_base;
> +
> +   scu_base = early_scu_map_io();
> +   return scu_get_core_count(scu_base);
> +}
> +
> 
> Please let me know how about using above approach to simplify platform
> specific code of early users of SCU address.
> 
> Also for rest platforms, which are not using scu base at very early
> stage, but are using virtual address which is mapped either from SCU
> device node or using s9_scu_get_base() to map to SCU virtual address. To
> separate these two we discussed to separate scu_enable in two helper
> APIs as of_scu_enable and s9_scu_enable. So if we change
> scu_get_core_count which do not requires address argument, this also
> needs to be separated in two as of_scu_get_core_count and
> s9_scu_get_core_count.

We can probably leave get_core_count code alone for now, or
we change it so that we pass a virtual address into it
from the platform and have the SCU mapped there. One nice
property of the early mapping is that the later ioremap()
will just return the same virtual address, so we don't get
a double mapping when calling the scu_enable variant later.

Adding two functions of each doesn't sound too great though,
it would make the API more complicated when the intention was
to make it simpler by leaving out the address argument.

Maybe something like this?

diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
index 72f9241ad5db..c248a16e980f 100644
--- a/arch/arm/kernel/smp_scu.c
+++ b/arch/arm/kernel/smp_scu.c
@@ -24,6 +24,8 @@
 #define SCU_INVALIDATE 0x0c
 #define SCU_FPGA_REVISION  0x10
 
+static void __iomem *scu_base_addr;
+
 #ifdef CONFIG_SMP
 /*
  * Get the number of CPU cores from the SCU configuration
@@ -41,6 +43,9 @@ void scu_enable(void __iomem *scu_base)
 {
u32 scu_ctrl;
 
+   if (scu_base)
+   scu_base = scu_base_addr;
+
 #ifdef CONFIG_ARM_ERRATA_764369
/* Cortex-A9 only */
if ((read_cpuid_id() & 0xff00) == 0x410fc090) {
@@ -85,6 +90,9 @@ int scu_power_mode(void __iomem *scu_base, unsigned int mode)
unsigned int val;
int cpu = MPIDR_AFFINITY_LEVEL(cpu_logical_map(smp_processor_id()), 0);
 
+   if (scu_base)
+   scu_base = scu_base_addr;
+
if (mode > 3 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

On Thursday 17 November 2016 10:33 PM, Arnd Bergmann wrote:
> On Thursday, November 17, 2016 9:50:27 AM CET pankaj.dubey wrote:
>>
> of_scu_enable() which _only_ looks up the SCU address in DT and enables
> it if it finds it, otherwise returning failure.
>
> a9_scu_enable() which tries to use the A9 provided SCU address and
> enables it if it finds it, otherwise returning failure.
>
>>
>> OK, In that case I can see need for following four helpers as:
>>
>> 1: of_scu_enable() which will __only__ lookup the SCU address in DT and
>> enables it if it finds, otherwise return -ENOMEM failure.
>> This helper APIs is required and sufficient for most of platforms such
>> as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
>> mvebu
>>
>> 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
>> enables it, if address mapped successfully, otherwise returning failure.
>> This helper APIs is required and sufficient for two ARM platforms as of
>> now tegra and hisi.
>>
>> 3: of_scu_get_base() which will lookup the SCU address in DT and if node
>> found maps address and returns ioremapped address to caller.
>> This helper APIs is required for three ARM plaforms rockchip, mvebu and
>> ux500, along with scu_enable() API to enable and find number_of_cores.
>>
>> 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
>> do ioremap of scu address and returns ioremapped address to the caller
>> along with ownership (caller has responsibility to unmap it).
>> This helper APIs is required to simplify SCU enable and related code in
>> two ARM plaforms BCM ans ZX.
>>
>> For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
>> are useful for the time-being, as they need SCU mapping very early of
>> boot, where we can't use iomap APIs. So I will drop patches related to
>> these platforms in v2 version.
>>
>> Please let me know if any concern in this approach.
> 
> I think ideally we wouldn't even need to know the virtual address
> outside of smp_scu.c. If we can move all users of the address
> into that file directly, it could become a local variable and
> we change scu_power_mode() and scu_get_core_count() instead to
> not require the address argument.
> 

If we change scu_get_core_count() without address argument, what about
those platforms which are calling this API very early boot (mostly in
smp_init_cpus), during this stage we can't use iomap. These platforms
are doing static mapping of SCU base, and passing virtual address.

Currently following are platforms which needs SCU virtual address at
very early stage mostly for calling scu_get_core_count(scu_address):
IMX, ZYNQ, SPEAr, and OMAP2.

I can think of handling of these platform's early need of SCU in the
following way:

Probably we need something like early_a9_scu_get_core_count() which can
be handled in this way in smp_scu.c file itself:

+static struct map_desc scu_map __initdata = {
+   .length = SZ_256,
+   .type   = MT_DEVICE,
+};
+
+static void __iomem *early_scu_map_io(void)
+{
+   unsigned long base;
+   void __iomem *scu_base;
+
+   base = scu_a9_get_base();
+   scu_map.pfn = __phys_to_pfn(base);
+   scu_map.virtual = base;
+   iotable_init(_map, 1);
+   scu_base = (void __iomem *)base;
+   return scu_base;
+}
+
+/*
+ * early_a9_scu_get_core_count - Get number of CPU cores at very early boot
+ * Only platforms which needs number_of_cores during early boot should
call this
+ */
+unsigned int __init early_a9_scu_get_core_count(void)
+{
+   void __iomem *scu_base;
+
+   scu_base = early_scu_map_io();
+   return scu_get_core_count(scu_base);
+}
+

Please let me know how about using above approach to simplify platform
specific code of early users of SCU address.

Also for rest platforms, which are not using scu base at very early
stage, but are using virtual address which is mapped either from SCU
device node or using s9_scu_get_base() to map to SCU virtual address. To
separate these two we discussed to separate scu_enable in two helper
APIs as of_scu_enable and s9_scu_enable. So if we change
scu_get_core_count which do not requires address argument, this also
needs to be separated in two as of_scu_get_core_count and
s9_scu_get_core_count.


Thanks,
Pankaj Dubey

> The only user I could find outside of that file is
> 
> static int shmobile_smp_scu_psr_core_disabled(int cpu)
> {
> unsigned long mask = SCU_PM_POWEROFF << (cpu * 8);
> 
> if ((__raw_readl(shmobile_scu_base + 8) & mask) == mask)
> return 1;
> 
> return 0;
> }
> 
> which can be done in the same file as well.
> 
> Then callers can decide which of these to call, and what error messages
> to print on their failures.

 Splitting the function in two is probably simpler overall, but
 we may still have to look at all the callers: Any platform that
 currently tries to map it on any CPU and doesn't warn about 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

On Thursday 17 November 2016 10:33 PM, Arnd Bergmann wrote:
> On Thursday, November 17, 2016 9:50:27 AM CET pankaj.dubey wrote:
>>
> of_scu_enable() which _only_ looks up the SCU address in DT and enables
> it if it finds it, otherwise returning failure.
>
> a9_scu_enable() which tries to use the A9 provided SCU address and
> enables it if it finds it, otherwise returning failure.
>
>>
>> OK, In that case I can see need for following four helpers as:
>>
>> 1: of_scu_enable() which will __only__ lookup the SCU address in DT and
>> enables it if it finds, otherwise return -ENOMEM failure.
>> This helper APIs is required and sufficient for most of platforms such
>> as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
>> mvebu
>>
>> 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
>> enables it, if address mapped successfully, otherwise returning failure.
>> This helper APIs is required and sufficient for two ARM platforms as of
>> now tegra and hisi.
>>
>> 3: of_scu_get_base() which will lookup the SCU address in DT and if node
>> found maps address and returns ioremapped address to caller.
>> This helper APIs is required for three ARM plaforms rockchip, mvebu and
>> ux500, along with scu_enable() API to enable and find number_of_cores.
>>
>> 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
>> do ioremap of scu address and returns ioremapped address to the caller
>> along with ownership (caller has responsibility to unmap it).
>> This helper APIs is required to simplify SCU enable and related code in
>> two ARM plaforms BCM ans ZX.
>>
>> For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
>> are useful for the time-being, as they need SCU mapping very early of
>> boot, where we can't use iomap APIs. So I will drop patches related to
>> these platforms in v2 version.
>>
>> Please let me know if any concern in this approach.
> 
> I think ideally we wouldn't even need to know the virtual address
> outside of smp_scu.c. If we can move all users of the address
> into that file directly, it could become a local variable and
> we change scu_power_mode() and scu_get_core_count() instead to
> not require the address argument.
> 

If we change scu_get_core_count() without address argument, what about
those platforms which are calling this API very early boot (mostly in
smp_init_cpus), during this stage we can't use iomap. These platforms
are doing static mapping of SCU base, and passing virtual address.

Currently following are platforms which needs SCU virtual address at
very early stage mostly for calling scu_get_core_count(scu_address):
IMX, ZYNQ, SPEAr, and OMAP2.

I can think of handling of these platform's early need of SCU in the
following way:

Probably we need something like early_a9_scu_get_core_count() which can
be handled in this way in smp_scu.c file itself:

+static struct map_desc scu_map __initdata = {
+   .length = SZ_256,
+   .type   = MT_DEVICE,
+};
+
+static void __iomem *early_scu_map_io(void)
+{
+   unsigned long base;
+   void __iomem *scu_base;
+
+   base = scu_a9_get_base();
+   scu_map.pfn = __phys_to_pfn(base);
+   scu_map.virtual = base;
+   iotable_init(_map, 1);
+   scu_base = (void __iomem *)base;
+   return scu_base;
+}
+
+/*
+ * early_a9_scu_get_core_count - Get number of CPU cores at very early boot
+ * Only platforms which needs number_of_cores during early boot should
call this
+ */
+unsigned int __init early_a9_scu_get_core_count(void)
+{
+   void __iomem *scu_base;
+
+   scu_base = early_scu_map_io();
+   return scu_get_core_count(scu_base);
+}
+

Please let me know how about using above approach to simplify platform
specific code of early users of SCU address.

Also for rest platforms, which are not using scu base at very early
stage, but are using virtual address which is mapped either from SCU
device node or using s9_scu_get_base() to map to SCU virtual address. To
separate these two we discussed to separate scu_enable in two helper
APIs as of_scu_enable and s9_scu_enable. So if we change
scu_get_core_count which do not requires address argument, this also
needs to be separated in two as of_scu_get_core_count and
s9_scu_get_core_count.


Thanks,
Pankaj Dubey

> The only user I could find outside of that file is
> 
> static int shmobile_smp_scu_psr_core_disabled(int cpu)
> {
> unsigned long mask = SCU_PM_POWEROFF << (cpu * 8);
> 
> if ((__raw_readl(shmobile_scu_base + 8) & mask) == mask)
> return 1;
> 
> return 0;
> }
> 
> which can be done in the same file as well.
> 
> Then callers can decide which of these to call, and what error messages
> to print on their failures.

 Splitting the function in two is probably simpler overall, but
 we may still have to look at all the callers: Any platform that
 currently tries to map it on any CPU and doesn't warn about 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Thursday, November 17, 2016 9:50:27 AM CET pankaj.dubey wrote:
> 
> >>> of_scu_enable() which _only_ looks up the SCU address in DT and enables
> >>> it if it finds it, otherwise returning failure.
> >>>
> >>> a9_scu_enable() which tries to use the A9 provided SCU address and
> >>> enables it if it finds it, otherwise returning failure.
> >>>
> 
> OK, In that case I can see need for following four helpers as:
> 
> 1: of_scu_enable() which will __only__ lookup the SCU address in DT and
> enables it if it finds, otherwise return -ENOMEM failure.
> This helper APIs is required and sufficient for most of platforms such
> as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
> mvebu
> 
> 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
> enables it, if address mapped successfully, otherwise returning failure.
> This helper APIs is required and sufficient for two ARM platforms as of
> now tegra and hisi.
> 
> 3: of_scu_get_base() which will lookup the SCU address in DT and if node
> found maps address and returns ioremapped address to caller.
> This helper APIs is required for three ARM plaforms rockchip, mvebu and
> ux500, along with scu_enable() API to enable and find number_of_cores.
> 
> 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
> do ioremap of scu address and returns ioremapped address to the caller
> along with ownership (caller has responsibility to unmap it).
> This helper APIs is required to simplify SCU enable and related code in
> two ARM plaforms BCM ans ZX.
>
> For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
> are useful for the time-being, as they need SCU mapping very early of
> boot, where we can't use iomap APIs. So I will drop patches related to
> these platforms in v2 version.
> 
> Please let me know if any concern in this approach.

I think ideally we wouldn't even need to know the virtual address
outside of smp_scu.c. If we can move all users of the address
into that file directly, it could become a local variable and
we change scu_power_mode() and scu_get_core_count() instead to
not require the address argument.

The only user I could find outside of that file is

static int shmobile_smp_scu_psr_core_disabled(int cpu)
{
unsigned long mask = SCU_PM_POWEROFF << (cpu * 8);

if ((__raw_readl(shmobile_scu_base + 8) & mask) == mask)
return 1;

return 0;
}

which can be done in the same file as well.

> >>> Then callers can decide which of these to call, and what error messages
> >>> to print on their failures.
> >>
> >> Splitting the function in two is probably simpler overall, but
> >> we may still have to look at all the callers: Any platform that
> >> currently tries to map it on any CPU and doesn't warn about the
> >> absence of the device node (or about scu_a9_has_base() == false)
> >> should really continue not to warn about that.
> > 
> > Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
> > should produce any warnings or errors to be printed.  It's up to the
> > caller to report the failure, otherwise doing this doesn't make sense:
> > 
> >   if (of_scu_enable() < 0 && a9_scu_enable() < 0)
> >   pr_err("Failed to map and enable the SCU\n");
> > 
> > because if of_scu_enable() prints a warning/error, then it's patently
> > misleading.
> > 

That's why I said "otherwise we can leave the warning in the caller
after checking the return code of the new APIs." for the case where
we actually need it.

> I will move out error message out of these helpers and let caller
> (platform specific code) handle and print error if required.

Ok.

Arnd

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Thursday, November 17, 2016 9:50:27 AM CET pankaj.dubey wrote:
> 
> >>> of_scu_enable() which _only_ looks up the SCU address in DT and enables
> >>> it if it finds it, otherwise returning failure.
> >>>
> >>> a9_scu_enable() which tries to use the A9 provided SCU address and
> >>> enables it if it finds it, otherwise returning failure.
> >>>
> 
> OK, In that case I can see need for following four helpers as:
> 
> 1: of_scu_enable() which will __only__ lookup the SCU address in DT and
> enables it if it finds, otherwise return -ENOMEM failure.
> This helper APIs is required and sufficient for most of platforms such
> as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
> mvebu
> 
> 2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
> enables it, if address mapped successfully, otherwise returning failure.
> This helper APIs is required and sufficient for two ARM platforms as of
> now tegra and hisi.
> 
> 3: of_scu_get_base() which will lookup the SCU address in DT and if node
> found maps address and returns ioremapped address to caller.
> This helper APIs is required for three ARM plaforms rockchip, mvebu and
> ux500, along with scu_enable() API to enable and find number_of_cores.
> 
> 4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
> do ioremap of scu address and returns ioremapped address to the caller
> along with ownership (caller has responsibility to unmap it).
> This helper APIs is required to simplify SCU enable and related code in
> two ARM plaforms BCM ans ZX.
>
> For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
> are useful for the time-being, as they need SCU mapping very early of
> boot, where we can't use iomap APIs. So I will drop patches related to
> these platforms in v2 version.
> 
> Please let me know if any concern in this approach.

I think ideally we wouldn't even need to know the virtual address
outside of smp_scu.c. If we can move all users of the address
into that file directly, it could become a local variable and
we change scu_power_mode() and scu_get_core_count() instead to
not require the address argument.

The only user I could find outside of that file is

static int shmobile_smp_scu_psr_core_disabled(int cpu)
{
unsigned long mask = SCU_PM_POWEROFF << (cpu * 8);

if ((__raw_readl(shmobile_scu_base + 8) & mask) == mask)
return 1;

return 0;
}

which can be done in the same file as well.

> >>> Then callers can decide which of these to call, and what error messages
> >>> to print on their failures.
> >>
> >> Splitting the function in two is probably simpler overall, but
> >> we may still have to look at all the callers: Any platform that
> >> currently tries to map it on any CPU and doesn't warn about the
> >> absence of the device node (or about scu_a9_has_base() == false)
> >> should really continue not to warn about that.
> > 
> > Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
> > should produce any warnings or errors to be printed.  It's up to the
> > caller to report the failure, otherwise doing this doesn't make sense:
> > 
> >   if (of_scu_enable() < 0 && a9_scu_enable() < 0)
> >   pr_err("Failed to map and enable the SCU\n");
> > 
> > because if of_scu_enable() prints a warning/error, then it's patently
> > misleading.
> > 

That's why I said "otherwise we can leave the warning in the caller
after checking the return code of the new APIs." for the case where
we actually need it.

> I will move out error message out of these helpers and let caller
> (platform specific code) handle and print error if required.

Ok.

Arnd

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

Hi Russell,

On Monday 14 November 2016 08:21 PM, Russell King - ARM Linux wrote:
> On Mon, Nov 14, 2016 at 03:37:44PM +0100, Arnd Bergmann wrote:
>> On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
>>> On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
 On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
>>> +scu_base = of_iomap(np, 0);
>>> +of_node_put(np);
>>> +if (!scu_base) {
>>> +pr_err("%s failed to map scu_base via DT\n", __func__);
>>
>> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
>> what does it mean, but it may confuse normal users. In current version,
>> berlin doesn't complain like this for non-ca9 SoCs
>>
>
> OK, let me see other reviewer's comment on this. Then we will decide if
> this error message is required or can be omitted.

 We need to look at all callers here, to see if the function ever gets
 called for a CPU that doesn't have an SCU. I'd say we should warn if
 we know there is an SCU but we cannot map it, but never warn on
 any of the CPU cores that don't support an SCU.
>>>
>>> Maybe there should be two helpers:
>>>
>>> of_scu_enable() which _only_ looks up the SCU address in DT and enables
>>> it if it finds it, otherwise returning failure.
>>>
>>> a9_scu_enable() which tries to use the A9 provided SCU address and
>>> enables it if it finds it, otherwise returning failure.
>>>

OK, In that case I can see need for following four helpers as:

1: of_scu_enable() which will __only__ lookup the SCU address in DT and
enables it if it finds, otherwise return -ENOMEM failure.
This helper APIs is required and sufficient for most of platforms such
as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
mvebu

2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
enables it, if address mapped successfully, otherwise returning failure.
This helper APIs is required and sufficient for two ARM platforms as of
now tegra and hisi.

3: of_scu_get_base() which will lookup the SCU address in DT and if node
found maps address and returns ioremapped address to caller.
This helper APIs is required for three ARM plaforms rockchip, mvebu and
ux500, along with scu_enable() API to enable and find number_of_cores.

4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
do ioremap of scu address and returns ioremapped address to the caller
along with ownership (caller has responsibility to unmap it).
This helper APIs is required to simplify SCU enable and related code in
two ARM plaforms BCM ans ZX.

For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
are useful for the time-being, as they need SCU mapping very early of
boot, where we can't use iomap APIs. So I will drop patches related to
these platforms in v2 version.

Please let me know if any concern in this approach.


>>> Then callers can decide which of these to call, and what error messages
>>> to print on their failures.
>>
>> Splitting the function in two is probably simpler overall, but
>> we may still have to look at all the callers: Any platform that
>> currently tries to map it on any CPU and doesn't warn about the
>> absence of the device node (or about scu_a9_has_base() == false)
>> should really continue not to warn about that.
> 
> Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
> should produce any warnings or errors to be printed.  It's up to the
> caller to report the failure, otherwise doing this doesn't make sense:
> 
>   if (of_scu_enable() < 0 && a9_scu_enable() < 0)
>   pr_err("Failed to map and enable the SCU\n");
> 
> because if of_scu_enable() prints a warning/error, then it's patently
> misleading.
> 

I will move out error message out of these helpers and let caller
(platform specific code) handle and print error if required.


Thanks,
Pankaj Dubey

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

Hi Russell,

On Monday 14 November 2016 08:21 PM, Russell King - ARM Linux wrote:
> On Mon, Nov 14, 2016 at 03:37:44PM +0100, Arnd Bergmann wrote:
>> On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
>>> On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
 On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
>>> +scu_base = of_iomap(np, 0);
>>> +of_node_put(np);
>>> +if (!scu_base) {
>>> +pr_err("%s failed to map scu_base via DT\n", __func__);
>>
>> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
>> what does it mean, but it may confuse normal users. In current version,
>> berlin doesn't complain like this for non-ca9 SoCs
>>
>
> OK, let me see other reviewer's comment on this. Then we will decide if
> this error message is required or can be omitted.

 We need to look at all callers here, to see if the function ever gets
 called for a CPU that doesn't have an SCU. I'd say we should warn if
 we know there is an SCU but we cannot map it, but never warn on
 any of the CPU cores that don't support an SCU.
>>>
>>> Maybe there should be two helpers:
>>>
>>> of_scu_enable() which _only_ looks up the SCU address in DT and enables
>>> it if it finds it, otherwise returning failure.
>>>
>>> a9_scu_enable() which tries to use the A9 provided SCU address and
>>> enables it if it finds it, otherwise returning failure.
>>>

OK, In that case I can see need for following four helpers as:

1: of_scu_enable() which will __only__ lookup the SCU address in DT and
enables it if it finds, otherwise return -ENOMEM failure.
This helper APIs is required and sufficient for most of platforms such
as exynos, berlin, realview, socfpga, STi, ux500, vexpress, rockchip and
mvebu

2: a9_scu_enable(), which will __only__ use A9 provided SCU address and
enables it, if address mapped successfully, otherwise returning failure.
This helper APIs is required and sufficient for two ARM platforms as of
now tegra and hisi.

3: of_scu_get_base() which will lookup the SCU address in DT and if node
found maps address and returns ioremapped address to caller.
This helper APIs is required for three ARM plaforms rockchip, mvebu and
ux500, along with scu_enable() API to enable and find number_of_cores.

4: s9_scu_iomap_base() which will internally use s9_scu_get_base() and
do ioremap of scu address and returns ioremapped address to the caller
along with ownership (caller has responsibility to unmap it).
This helper APIs is required to simplify SCU enable and related code in
two ARM plaforms BCM ans ZX.

For remaining two ARM platforms (IMX and ZYNQ), none of these helpers
are useful for the time-being, as they need SCU mapping very early of
boot, where we can't use iomap APIs. So I will drop patches related to
these platforms in v2 version.

Please let me know if any concern in this approach.


>>> Then callers can decide which of these to call, and what error messages
>>> to print on their failures.
>>
>> Splitting the function in two is probably simpler overall, but
>> we may still have to look at all the callers: Any platform that
>> currently tries to map it on any CPU and doesn't warn about the
>> absence of the device node (or about scu_a9_has_base() == false)
>> should really continue not to warn about that.
> 
> Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
> should produce any warnings or errors to be printed.  It's up to the
> caller to report the failure, otherwise doing this doesn't make sense:
> 
>   if (of_scu_enable() < 0 && a9_scu_enable() < 0)
>   pr_err("Failed to map and enable the SCU\n");
> 
> because if of_scu_enable() prints a warning/error, then it's patently
> misleading.
> 

I will move out error message out of these helpers and let caller
(platform specific code) handle and print error if required.


Thanks,
Pankaj Dubey

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

Hi Russell,

On Monday 14 November 2016 07:18 PM, Russell King - ARM Linux wrote:
> This should be sent _to_ me because it's touching generic ARM code.
> Thanks.
> 

Sorry for this.

I had included your email in CC for this patch, but looks like my email
client had some issue and this patch could not reach to your mailbox. I
will take care in future.

> On Mon, Nov 14, 2016 at 10:31:56AM +0530, Pankaj Dubey wrote:
>> Many platforms are duplicating code for enabling SCU, lets add
>> common code to enable SCU by parsing SCU device node so the duplication
>> in each platform can be avoided.
>>
>> CC: Krzysztof Kozlowski 
>> CC: Jisheng Zhang 
>> CC: Russell King 
>> CC: Dinh Nguyen 
>> CC: Patrice Chotard 
>> CC: Linus Walleij 
>> CC: Liviu Dudau 
>> CC: Ray Jui 
>> CC: Stephen Warren 
>> CC: Heiko Stuebner 
>> CC: Shawn Guo 
>> CC: Michal Simek 
>> CC: Wei Xu 
>> CC: Andrew Lunn 
>> CC: Jun Nie  
>> Suggested-by: Arnd Bergmann 
>> Signed-off-by: Pankaj Dubey 
>> ---
>>  arch/arm/include/asm/smp_scu.h |  4 +++
>>  arch/arm/kernel/smp_scu.c  | 56 
>> ++
>>  2 files changed, 60 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
>> index bfe163c..fdeec07 100644
>> --- a/arch/arm/include/asm/smp_scu.h
>> +++ b/arch/arm/include/asm/smp_scu.h
>> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, 
>> unsigned int mode)
>>  
>>  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
>>  void scu_enable(void __iomem *scu_base);
>> +void __iomem *of_scu_get_base(void);
>> +int of_scu_enable(void);
>>  #else
>>  static inline void scu_enable(void __iomem *scu_base) {}
>> +static inline void __iomem *of_scu_get_base(void) {return NULL; }
>> +static inline int of_scu_enable(void) {return 0; }
>>  #endif
>>  
>>  #endif
>> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
>> index 72f9241..d0ac3ed 100644
>> --- a/arch/arm/kernel/smp_scu.c
>> +++ b/arch/arm/kernel/smp_scu.c
>> @@ -10,6 +10,7 @@
>>   */
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  #include 
>> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
>>   */
>>  flush_cache_all();
>>  }
>> +
>> +static const struct of_device_id scu_match[] = {
>> +{ .compatible = "arm,cortex-a9-scu", },
>> +{ .compatible = "arm,cortex-a5-scu", },
>> +{ }
>> +};
>> +
>> +/*
>> + * Helper API to get SCU base address
>> + * In case platform DT do not have SCU node, or iomap fails
>> + * this call will fallback and will try to map via call to
>> + * scu_a9_get_base.
>> + * This will return ownership of scu_base to the caller
>> + */
>> +void __iomem *of_scu_get_base(void)
>> +{
>> +unsigned long base = 0;
>> +struct device_node *np;
>> +void __iomem *scu_base;
>> +
>> +np = of_find_matching_node(NULL, scu_match);
>> +scu_base = of_iomap(np, 0);
>> +of_node_put(np);
>> +if (!scu_base) {
>> +pr_err("%s failed to map scu_base via DT\n", __func__);
>> +if (scu_a9_has_base()) {
>> +base = scu_a9_get_base();
>> +scu_base = ioremap(base, SZ_4K);
>> +}
>> +if (!scu_base) {
>> +pr_err("%s failed to map scu_base\n", __func__);
>> +return IOMEM_ERR_PTR(-ENOMEM);
> 
> I can't see the point of using error-pointers here - it's not like we
> really know why we're failing, so just return NULL.
> 
>> +}
>> +}
>> +return scu_base;
>> +}
>> +
>> +/*
>> + * Enable SCU via mapping scu_base DT
>> + * If scu_base mapped successfully scu will be enabled and in case of
>> + * failure if will return non-zero error code
>> + */
>> +int of_scu_enable(void)
>> +{
>> +void __iomem *scu_base;
>> +
>> +scu_base = of_scu_get_base();
>> +if (!IS_ERR(scu_base)) {
>> +scu_enable(scu_base);
>> +iounmap(scu_base);
>> +return 0;
>> +}
>> +return PTR_ERR(scu_base);
> 
> and just return your -ENOMEM here.
> 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

Hi Russell,

On Monday 14 November 2016 07:18 PM, Russell King - ARM Linux wrote:
> This should be sent _to_ me because it's touching generic ARM code.
> Thanks.
> 

Sorry for this.

I had included your email in CC for this patch, but looks like my email
client had some issue and this patch could not reach to your mailbox. I
will take care in future.

> On Mon, Nov 14, 2016 at 10:31:56AM +0530, Pankaj Dubey wrote:
>> Many platforms are duplicating code for enabling SCU, lets add
>> common code to enable SCU by parsing SCU device node so the duplication
>> in each platform can be avoided.
>>
>> CC: Krzysztof Kozlowski 
>> CC: Jisheng Zhang 
>> CC: Russell King 
>> CC: Dinh Nguyen 
>> CC: Patrice Chotard 
>> CC: Linus Walleij 
>> CC: Liviu Dudau 
>> CC: Ray Jui 
>> CC: Stephen Warren 
>> CC: Heiko Stuebner 
>> CC: Shawn Guo 
>> CC: Michal Simek 
>> CC: Wei Xu 
>> CC: Andrew Lunn 
>> CC: Jun Nie  
>> Suggested-by: Arnd Bergmann 
>> Signed-off-by: Pankaj Dubey 
>> ---
>>  arch/arm/include/asm/smp_scu.h |  4 +++
>>  arch/arm/kernel/smp_scu.c  | 56 
>> ++
>>  2 files changed, 60 insertions(+)
>>
>> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
>> index bfe163c..fdeec07 100644
>> --- a/arch/arm/include/asm/smp_scu.h
>> +++ b/arch/arm/include/asm/smp_scu.h
>> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, 
>> unsigned int mode)
>>  
>>  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
>>  void scu_enable(void __iomem *scu_base);
>> +void __iomem *of_scu_get_base(void);
>> +int of_scu_enable(void);
>>  #else
>>  static inline void scu_enable(void __iomem *scu_base) {}
>> +static inline void __iomem *of_scu_get_base(void) {return NULL; }
>> +static inline int of_scu_enable(void) {return 0; }
>>  #endif
>>  
>>  #endif
>> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
>> index 72f9241..d0ac3ed 100644
>> --- a/arch/arm/kernel/smp_scu.c
>> +++ b/arch/arm/kernel/smp_scu.c
>> @@ -10,6 +10,7 @@
>>   */
>>  #include 
>>  #include 
>> +#include 
>>  
>>  #include 
>>  #include 
>> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
>>   */
>>  flush_cache_all();
>>  }
>> +
>> +static const struct of_device_id scu_match[] = {
>> +{ .compatible = "arm,cortex-a9-scu", },
>> +{ .compatible = "arm,cortex-a5-scu", },
>> +{ }
>> +};
>> +
>> +/*
>> + * Helper API to get SCU base address
>> + * In case platform DT do not have SCU node, or iomap fails
>> + * this call will fallback and will try to map via call to
>> + * scu_a9_get_base.
>> + * This will return ownership of scu_base to the caller
>> + */
>> +void __iomem *of_scu_get_base(void)
>> +{
>> +unsigned long base = 0;
>> +struct device_node *np;
>> +void __iomem *scu_base;
>> +
>> +np = of_find_matching_node(NULL, scu_match);
>> +scu_base = of_iomap(np, 0);
>> +of_node_put(np);
>> +if (!scu_base) {
>> +pr_err("%s failed to map scu_base via DT\n", __func__);
>> +if (scu_a9_has_base()) {
>> +base = scu_a9_get_base();
>> +scu_base = ioremap(base, SZ_4K);
>> +}
>> +if (!scu_base) {
>> +pr_err("%s failed to map scu_base\n", __func__);
>> +return IOMEM_ERR_PTR(-ENOMEM);
> 
> I can't see the point of using error-pointers here - it's not like we
> really know why we're failing, so just return NULL.
> 
>> +}
>> +}
>> +return scu_base;
>> +}
>> +
>> +/*
>> + * Enable SCU via mapping scu_base DT
>> + * If scu_base mapped successfully scu will be enabled and in case of
>> + * failure if will return non-zero error code
>> + */
>> +int of_scu_enable(void)
>> +{
>> +void __iomem *scu_base;
>> +
>> +scu_base = of_scu_get_base();
>> +if (!IS_ERR(scu_base)) {
>> +scu_enable(scu_base);
>> +iounmap(scu_base);
>> +return 0;
>> +}
>> +return PTR_ERR(scu_base);
> 
> and just return your -ENOMEM here.
> 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Russell King - ARM Linux]

On Mon, Nov 14, 2016 at 03:37:44PM +0100, Arnd Bergmann wrote:
> On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
> > On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
> > > On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> > > > >> +scu_base = of_iomap(np, 0);
> > > > >> +of_node_put(np);
> > > > >> +if (!scu_base) {
> > > > >> +pr_err("%s failed to map scu_base via DT\n", __func__);
> > > > > 
> > > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We 
> > > > > understand
> > > > > what does it mean, but it may confuse normal users. In current 
> > > > > version,
> > > > > berlin doesn't complain like this for non-ca9 SoCs
> > > > > 
> > > > 
> > > > OK, let me see other reviewer's comment on this. Then we will decide if
> > > > this error message is required or can be omitted.
> > > 
> > > We need to look at all callers here, to see if the function ever gets
> > > called for a CPU that doesn't have an SCU. I'd say we should warn if
> > > we know there is an SCU but we cannot map it, but never warn on
> > > any of the CPU cores that don't support an SCU.
> > 
> > Maybe there should be two helpers:
> > 
> > of_scu_enable() which _only_ looks up the SCU address in DT and enables
> > it if it finds it, otherwise returning failure.
> > 
> > a9_scu_enable() which tries to use the A9 provided SCU address and
> > enables it if it finds it, otherwise returning failure.
> > 
> > Then callers can decide which of these to call, and what error messages
> > to print on their failures.
> 
> Splitting the function in two is probably simpler overall, but
> we may still have to look at all the callers: Any platform that
> currently tries to map it on any CPU and doesn't warn about the
> absence of the device node (or about scu_a9_has_base() == false)
> should really continue not to warn about that.

Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
should produce any warnings or errors to be printed.  It's up to the
caller to report the failure, otherwise doing this doesn't make sense:

if (of_scu_enable() < 0 && a9_scu_enable() < 0)
pr_err("Failed to map and enable the SCU\n");

because if of_scu_enable() prints a warning/error, then it's patently
misleading.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Russell King - ARM Linux]

On Mon, Nov 14, 2016 at 03:37:44PM +0100, Arnd Bergmann wrote:
> On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
> > On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
> > > On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> > > > >> +scu_base = of_iomap(np, 0);
> > > > >> +of_node_put(np);
> > > > >> +if (!scu_base) {
> > > > >> +pr_err("%s failed to map scu_base via DT\n", __func__);
> > > > > 
> > > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We 
> > > > > understand
> > > > > what does it mean, but it may confuse normal users. In current 
> > > > > version,
> > > > > berlin doesn't complain like this for non-ca9 SoCs
> > > > > 
> > > > 
> > > > OK, let me see other reviewer's comment on this. Then we will decide if
> > > > this error message is required or can be omitted.
> > > 
> > > We need to look at all callers here, to see if the function ever gets
> > > called for a CPU that doesn't have an SCU. I'd say we should warn if
> > > we know there is an SCU but we cannot map it, but never warn on
> > > any of the CPU cores that don't support an SCU.
> > 
> > Maybe there should be two helpers:
> > 
> > of_scu_enable() which _only_ looks up the SCU address in DT and enables
> > it if it finds it, otherwise returning failure.
> > 
> > a9_scu_enable() which tries to use the A9 provided SCU address and
> > enables it if it finds it, otherwise returning failure.
> > 
> > Then callers can decide which of these to call, and what error messages
> > to print on their failures.
> 
> Splitting the function in two is probably simpler overall, but
> we may still have to look at all the callers: Any platform that
> currently tries to map it on any CPU and doesn't warn about the
> absence of the device node (or about scu_a9_has_base() == false)
> should really continue not to warn about that.

Did you miss the bit where none of of_scu_enable() or a9_scu_enable()
should produce any warnings or errors to be printed.  It's up to the
caller to report the failure, otherwise doing this doesn't make sense:

if (of_scu_enable() < 0 && a9_scu_enable() < 0)
pr_err("Failed to map and enable the SCU\n");

because if of_scu_enable() prints a warning/error, then it's patently
misleading.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
> On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
> > On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> > > >> +scu_base = of_iomap(np, 0);
> > > >> +of_node_put(np);
> > > >> +if (!scu_base) {
> > > >> +pr_err("%s failed to map scu_base via DT\n", __func__);
> > > > 
> > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > > > what does it mean, but it may confuse normal users. In current version,
> > > > berlin doesn't complain like this for non-ca9 SoCs
> > > > 
> > > 
> > > OK, let me see other reviewer's comment on this. Then we will decide if
> > > this error message is required or can be omitted.
> > 
> > We need to look at all callers here, to see if the function ever gets
> > called for a CPU that doesn't have an SCU. I'd say we should warn if
> > we know there is an SCU but we cannot map it, but never warn on
> > any of the CPU cores that don't support an SCU.
> 
> Maybe there should be two helpers:
> 
> of_scu_enable() which _only_ looks up the SCU address in DT and enables
> it if it finds it, otherwise returning failure.
> 
> a9_scu_enable() which tries to use the A9 provided SCU address and
> enables it if it finds it, otherwise returning failure.
> 
> Then callers can decide which of these to call, and what error messages
> to print on their failures.

Splitting the function in two is probably simpler overall, but
we may still have to look at all the callers: Any platform that
currently tries to map it on any CPU and doesn't warn about the
absence of the device node (or about scu_a9_has_base() == false)
should really continue not to warn about that.

If all platforms only call these on SMP machines with an
ARM11MPcore, Cortex-A5 or Cortex-A9, everything should be
fine here, otherwise we can leave the warning in the caller
after checking the return code of the new APIs.

Arnd

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Monday, November 14, 2016 1:50:18 PM CET Russell King - ARM Linux wrote:
> On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
> > On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> > > >> +scu_base = of_iomap(np, 0);
> > > >> +of_node_put(np);
> > > >> +if (!scu_base) {
> > > >> +pr_err("%s failed to map scu_base via DT\n", __func__);
> > > > 
> > > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > > > what does it mean, but it may confuse normal users. In current version,
> > > > berlin doesn't complain like this for non-ca9 SoCs
> > > > 
> > > 
> > > OK, let me see other reviewer's comment on this. Then we will decide if
> > > this error message is required or can be omitted.
> > 
> > We need to look at all callers here, to see if the function ever gets
> > called for a CPU that doesn't have an SCU. I'd say we should warn if
> > we know there is an SCU but we cannot map it, but never warn on
> > any of the CPU cores that don't support an SCU.
> 
> Maybe there should be two helpers:
> 
> of_scu_enable() which _only_ looks up the SCU address in DT and enables
> it if it finds it, otherwise returning failure.
> 
> a9_scu_enable() which tries to use the A9 provided SCU address and
> enables it if it finds it, otherwise returning failure.
> 
> Then callers can decide which of these to call, and what error messages
> to print on their failures.

Splitting the function in two is probably simpler overall, but
we may still have to look at all the callers: Any platform that
currently tries to map it on any CPU and doesn't warn about the
absence of the device node (or about scu_a9_has_base() == false)
should really continue not to warn about that.

If all platforms only call these on SMP machines with an
ARM11MPcore, Cortex-A5 or Cortex-A9, everything should be
fine here, otherwise we can leave the warning in the caller
after checking the return code of the new APIs.

Arnd

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Russell King - ARM Linux]

On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
> On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> > >> +scu_base = of_iomap(np, 0);
> > >> +of_node_put(np);
> > >> +if (!scu_base) {
> > >> +pr_err("%s failed to map scu_base via DT\n", __func__);
> > > 
> > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > > what does it mean, but it may confuse normal users. In current version,
> > > berlin doesn't complain like this for non-ca9 SoCs
> > > 
> > 
> > OK, let me see other reviewer's comment on this. Then we will decide if
> > this error message is required or can be omitted.
> 
> We need to look at all callers here, to see if the function ever gets
> called for a CPU that doesn't have an SCU. I'd say we should warn if
> we know there is an SCU but we cannot map it, but never warn on
> any of the CPU cores that don't support an SCU.

Maybe there should be two helpers:

of_scu_enable() which _only_ looks up the SCU address in DT and enables
it if it finds it, otherwise returning failure.

a9_scu_enable() which tries to use the A9 provided SCU address and
enables it if it finds it, otherwise returning failure.

Then callers can decide which of these to call, and what error messages
to print on their failures.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Russell King - ARM Linux]

On Mon, Nov 14, 2016 at 01:03:09PM +0100, Arnd Bergmann wrote:
> On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> > >> +scu_base = of_iomap(np, 0);
> > >> +of_node_put(np);
> > >> +if (!scu_base) {
> > >> +pr_err("%s failed to map scu_base via DT\n", __func__);
> > > 
> > > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > > what does it mean, but it may confuse normal users. In current version,
> > > berlin doesn't complain like this for non-ca9 SoCs
> > > 
> > 
> > OK, let me see other reviewer's comment on this. Then we will decide if
> > this error message is required or can be omitted.
> 
> We need to look at all callers here, to see if the function ever gets
> called for a CPU that doesn't have an SCU. I'd say we should warn if
> we know there is an SCU but we cannot map it, but never warn on
> any of the CPU cores that don't support an SCU.

Maybe there should be two helpers:

of_scu_enable() which _only_ looks up the SCU address in DT and enables
it if it finds it, otherwise returning failure.

a9_scu_enable() which tries to use the A9 provided SCU address and
enables it if it finds it, otherwise returning failure.

Then callers can decide which of these to call, and what error messages
to print on their failures.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Russell King - ARM Linux]

This should be sent _to_ me because it's touching generic ARM code.
Thanks.

On Mon, Nov 14, 2016 at 10:31:56AM +0530, Pankaj Dubey wrote:
> Many platforms are duplicating code for enabling SCU, lets add
> common code to enable SCU by parsing SCU device node so the duplication
> in each platform can be avoided.
> 
> CC: Krzysztof Kozlowski 
> CC: Jisheng Zhang 
> CC: Russell King 
> CC: Dinh Nguyen 
> CC: Patrice Chotard 
> CC: Linus Walleij 
> CC: Liviu Dudau 
> CC: Ray Jui 
> CC: Stephen Warren 
> CC: Heiko Stuebner 
> CC: Shawn Guo 
> CC: Michal Simek 
> CC: Wei Xu 
> CC: Andrew Lunn 
> CC: Jun Nie  
> Suggested-by: Arnd Bergmann 
> Signed-off-by: Pankaj Dubey 
> ---
>  arch/arm/include/asm/smp_scu.h |  4 +++
>  arch/arm/kernel/smp_scu.c  | 56 
> ++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> index bfe163c..fdeec07 100644
> --- a/arch/arm/include/asm/smp_scu.h
> +++ b/arch/arm/include/asm/smp_scu.h
> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, 
> unsigned int mode)
>  
>  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
>  void scu_enable(void __iomem *scu_base);
> +void __iomem *of_scu_get_base(void);
> +int of_scu_enable(void);
>  #else
>  static inline void scu_enable(void __iomem *scu_base) {}
> +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> +static inline int of_scu_enable(void) {return 0; }
>  #endif
>  
>  #endif
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 72f9241..d0ac3ed 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -10,6 +10,7 @@
>   */
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
>*/
>   flush_cache_all();
>  }
> +
> +static const struct of_device_id scu_match[] = {
> + { .compatible = "arm,cortex-a9-scu", },
> + { .compatible = "arm,cortex-a5-scu", },
> + { }
> +};
> +
> +/*
> + * Helper API to get SCU base address
> + * In case platform DT do not have SCU node, or iomap fails
> + * this call will fallback and will try to map via call to
> + * scu_a9_get_base.
> + * This will return ownership of scu_base to the caller
> + */
> +void __iomem *of_scu_get_base(void)
> +{
> + unsigned long base = 0;
> + struct device_node *np;
> + void __iomem *scu_base;
> +
> + np = of_find_matching_node(NULL, scu_match);
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base via DT\n", __func__);
> + if (scu_a9_has_base()) {
> + base = scu_a9_get_base();
> + scu_base = ioremap(base, SZ_4K);
> + }
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base\n", __func__);
> + return IOMEM_ERR_PTR(-ENOMEM);

I can't see the point of using error-pointers here - it's not like we
really know why we're failing, so just return NULL.

> + }
> + }
> + return scu_base;
> +}
> +
> +/*
> + * Enable SCU via mapping scu_base DT
> + * If scu_base mapped successfully scu will be enabled and in case of
> + * failure if will return non-zero error code
> + */
> +int of_scu_enable(void)
> +{
> + void __iomem *scu_base;
> +
> + scu_base = of_scu_get_base();
> + if (!IS_ERR(scu_base)) {
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + return 0;
> + }
> + return PTR_ERR(scu_base);

and just return your -ENOMEM here.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Russell King - ARM Linux]

This should be sent _to_ me because it's touching generic ARM code.
Thanks.

On Mon, Nov 14, 2016 at 10:31:56AM +0530, Pankaj Dubey wrote:
> Many platforms are duplicating code for enabling SCU, lets add
> common code to enable SCU by parsing SCU device node so the duplication
> in each platform can be avoided.
> 
> CC: Krzysztof Kozlowski 
> CC: Jisheng Zhang 
> CC: Russell King 
> CC: Dinh Nguyen 
> CC: Patrice Chotard 
> CC: Linus Walleij 
> CC: Liviu Dudau 
> CC: Ray Jui 
> CC: Stephen Warren 
> CC: Heiko Stuebner 
> CC: Shawn Guo 
> CC: Michal Simek 
> CC: Wei Xu 
> CC: Andrew Lunn 
> CC: Jun Nie  
> Suggested-by: Arnd Bergmann 
> Signed-off-by: Pankaj Dubey 
> ---
>  arch/arm/include/asm/smp_scu.h |  4 +++
>  arch/arm/kernel/smp_scu.c  | 56 
> ++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> index bfe163c..fdeec07 100644
> --- a/arch/arm/include/asm/smp_scu.h
> +++ b/arch/arm/include/asm/smp_scu.h
> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, 
> unsigned int mode)
>  
>  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
>  void scu_enable(void __iomem *scu_base);
> +void __iomem *of_scu_get_base(void);
> +int of_scu_enable(void);
>  #else
>  static inline void scu_enable(void __iomem *scu_base) {}
> +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> +static inline int of_scu_enable(void) {return 0; }
>  #endif
>  
>  #endif
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 72f9241..d0ac3ed 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -10,6 +10,7 @@
>   */
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
>*/
>   flush_cache_all();
>  }
> +
> +static const struct of_device_id scu_match[] = {
> + { .compatible = "arm,cortex-a9-scu", },
> + { .compatible = "arm,cortex-a5-scu", },
> + { }
> +};
> +
> +/*
> + * Helper API to get SCU base address
> + * In case platform DT do not have SCU node, or iomap fails
> + * this call will fallback and will try to map via call to
> + * scu_a9_get_base.
> + * This will return ownership of scu_base to the caller
> + */
> +void __iomem *of_scu_get_base(void)
> +{
> + unsigned long base = 0;
> + struct device_node *np;
> + void __iomem *scu_base;
> +
> + np = of_find_matching_node(NULL, scu_match);
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base via DT\n", __func__);
> + if (scu_a9_has_base()) {
> + base = scu_a9_get_base();
> + scu_base = ioremap(base, SZ_4K);
> + }
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base\n", __func__);
> + return IOMEM_ERR_PTR(-ENOMEM);

I can't see the point of using error-pointers here - it's not like we
really know why we're failing, so just return NULL.

> + }
> + }
> + return scu_base;
> +}
> +
> +/*
> + * Enable SCU via mapping scu_base DT
> + * If scu_base mapped successfully scu will be enabled and in case of
> + * failure if will return non-zero error code
> + */
> +int of_scu_enable(void)
> +{
> + void __iomem *scu_base;
> +
> + scu_base = of_scu_get_base();
> + if (!IS_ERR(scu_base)) {
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + return 0;
> + }
> + return PTR_ERR(scu_base);

and just return your -ENOMEM here.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> >> +scu_base = of_iomap(np, 0);
> >> +of_node_put(np);
> >> +if (!scu_base) {
> >> +pr_err("%s failed to map scu_base via DT\n", __func__);
> > 
> > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > what does it mean, but it may confuse normal users. In current version,
> > berlin doesn't complain like this for non-ca9 SoCs
> > 
> 
> OK, let me see other reviewer's comment on this. Then we will decide if
> this error message is required or can be omitted.

We need to look at all callers here, to see if the function ever gets
called for a CPU that doesn't have an SCU. I'd say we should warn if
we know there is an SCU but we cannot map it, but never warn on
any of the CPU cores that don't support an SCU.

Arnd

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Arnd Bergmann]

On Monday, November 14, 2016 2:10:16 PM CET pankaj.dubey wrote:
> >> +scu_base = of_iomap(np, 0);
> >> +of_node_put(np);
> >> +if (!scu_base) {
> >> +pr_err("%s failed to map scu_base via DT\n", __func__);
> > 
> > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > what does it mean, but it may confuse normal users. In current version,
> > berlin doesn't complain like this for non-ca9 SoCs
> > 
> 
> OK, let me see other reviewer's comment on this. Then we will decide if
> this error message is required or can be omitted.

We need to look at all callers here, to see if the function ever gets
called for a CPU that doesn't have an SCU. I'd say we should warn if
we know there is an SCU but we cannot map it, but never warn on
any of the CPU cores that don't support an SCU.

Arnd

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Jisheng Zhang]

Hi Pankaj,

On Mon, 14 Nov 2016 14:54:59 +0800 Jisheng Zhang wrote:

> On Mon, 14 Nov 2016 14:12:51 +0800 Jisheng Zhang wrote:
> 
> > Hi Pankaj,
> > 
> > On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:
> >   
> > > Many platforms are duplicating code for enabling SCU, lets add
> > > common code to enable SCU by parsing SCU device node so the duplication
> > > in each platform can be avoided.
> > > 
> > > CC: Krzysztof Kozlowski 
> > > CC: Jisheng Zhang 
> > > CC: Russell King 
> > > CC: Dinh Nguyen 
> > > CC: Patrice Chotard 
> > > CC: Linus Walleij 
> > > CC: Liviu Dudau 
> > > CC: Ray Jui 
> > > CC: Stephen Warren 
> > > CC: Heiko Stuebner 
> > > CC: Shawn Guo 
> > > CC: Michal Simek 
> > > CC: Wei Xu 
> > > CC: Andrew Lunn 
> > > CC: Jun Nie  
> > > Suggested-by: Arnd Bergmann 
> > > Signed-off-by: Pankaj Dubey 
> > > ---
> > >  arch/arm/include/asm/smp_scu.h |  4 +++
> > >  arch/arm/kernel/smp_scu.c  | 56 
> > > ++
> > >  2 files changed, 60 insertions(+)
> > > 
> > > diff --git a/arch/arm/include/asm/smp_scu.h 
> > > b/arch/arm/include/asm/smp_scu.h
> > > index bfe163c..fdeec07 100644
> > > --- a/arch/arm/include/asm/smp_scu.h
> > > +++ b/arch/arm/include/asm/smp_scu.h
> > > @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem 
> > > *scu_base, unsigned int mode)
> > >  
> > >  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> > >  void scu_enable(void __iomem *scu_base);
> > > +void __iomem *of_scu_get_base(void);
> > > +int of_scu_enable(void);
> > >  #else
> > >  static inline void scu_enable(void __iomem *scu_base) {}
> > > +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> > > +static inline int of_scu_enable(void) {return 0; }
> > >  #endif
> > >  
> > >  #endif
> > > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > > index 72f9241..d0ac3ed 100644
> > > --- a/arch/arm/kernel/smp_scu.c
> > > +++ b/arch/arm/kernel/smp_scu.c
> > > @@ -10,6 +10,7 @@
> > >   */
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #include 
> > >  #include 
> > > @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
> > >*/
> > >   flush_cache_all();
> > >  }
> > > +
> > > +static const struct of_device_id scu_match[] = {
> > > + { .compatible = "arm,cortex-a9-scu", },
> > > + { .compatible = "arm,cortex-a5-scu", },
> > > + { }
> > > +};
> > > +
> > > +/*
> > > + * Helper API to get SCU base address
> > > + * In case platform DT do not have SCU node, or iomap fails
> > > + * this call will fallback and will try to map via call to
> > > + * scu_a9_get_base.
> > > + * This will return ownership of scu_base to the caller
> > > + */
> > > +void __iomem *of_scu_get_base(void)
> > > +{
> > > + unsigned long base = 0;
> > > + struct device_node *np;
> > > + void __iomem *scu_base;
> > > +
> > > + np = of_find_matching_node(NULL, scu_match);
> > 
> > could we check np before calling of_iomap()?
> >   
> > > + scu_base = of_iomap(np, 0);
> > > + of_node_put(np);
> > > + if (!scu_base) {
> > > + pr_err("%s failed to map scu_base via DT\n", __func__);
> > 
> > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > what does it mean, but it may confuse normal users. In current version,
> > berlin doesn't complain like this for non-ca9 SoCs  
> 
> oops, I just realized that the non-ca9 berlin arm SoC version isn't 
> upstreamed.
> Below is the draft version I planed. Basically speaking, the code tries to
> find "arm,cortex-a9-scu" node from DT, if can't, we think we don't need to
> worry about SCU. Is there any elegant solution for my situation?

I just realized that (another realized :D) we uses PSCI enable method for
non-ca9 base SoCs, so "marvell,berlin-smp" enable method is only for the SoCs
which have CA9 compatible SCU. Sorry for the noise, your patch looks good to me.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Jisheng Zhang]

Hi Pankaj,

On Mon, 14 Nov 2016 14:54:59 +0800 Jisheng Zhang wrote:

> On Mon, 14 Nov 2016 14:12:51 +0800 Jisheng Zhang wrote:
> 
> > Hi Pankaj,
> > 
> > On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:
> >   
> > > Many platforms are duplicating code for enabling SCU, lets add
> > > common code to enable SCU by parsing SCU device node so the duplication
> > > in each platform can be avoided.
> > > 
> > > CC: Krzysztof Kozlowski 
> > > CC: Jisheng Zhang 
> > > CC: Russell King 
> > > CC: Dinh Nguyen 
> > > CC: Patrice Chotard 
> > > CC: Linus Walleij 
> > > CC: Liviu Dudau 
> > > CC: Ray Jui 
> > > CC: Stephen Warren 
> > > CC: Heiko Stuebner 
> > > CC: Shawn Guo 
> > > CC: Michal Simek 
> > > CC: Wei Xu 
> > > CC: Andrew Lunn 
> > > CC: Jun Nie  
> > > Suggested-by: Arnd Bergmann 
> > > Signed-off-by: Pankaj Dubey 
> > > ---
> > >  arch/arm/include/asm/smp_scu.h |  4 +++
> > >  arch/arm/kernel/smp_scu.c  | 56 
> > > ++
> > >  2 files changed, 60 insertions(+)
> > > 
> > > diff --git a/arch/arm/include/asm/smp_scu.h 
> > > b/arch/arm/include/asm/smp_scu.h
> > > index bfe163c..fdeec07 100644
> > > --- a/arch/arm/include/asm/smp_scu.h
> > > +++ b/arch/arm/include/asm/smp_scu.h
> > > @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem 
> > > *scu_base, unsigned int mode)
> > >  
> > >  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> > >  void scu_enable(void __iomem *scu_base);
> > > +void __iomem *of_scu_get_base(void);
> > > +int of_scu_enable(void);
> > >  #else
> > >  static inline void scu_enable(void __iomem *scu_base) {}
> > > +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> > > +static inline int of_scu_enable(void) {return 0; }
> > >  #endif
> > >  
> > >  #endif
> > > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > > index 72f9241..d0ac3ed 100644
> > > --- a/arch/arm/kernel/smp_scu.c
> > > +++ b/arch/arm/kernel/smp_scu.c
> > > @@ -10,6 +10,7 @@
> > >   */
> > >  #include 
> > >  #include 
> > > +#include 
> > >  
> > >  #include 
> > >  #include 
> > > @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
> > >*/
> > >   flush_cache_all();
> > >  }
> > > +
> > > +static const struct of_device_id scu_match[] = {
> > > + { .compatible = "arm,cortex-a9-scu", },
> > > + { .compatible = "arm,cortex-a5-scu", },
> > > + { }
> > > +};
> > > +
> > > +/*
> > > + * Helper API to get SCU base address
> > > + * In case platform DT do not have SCU node, or iomap fails
> > > + * this call will fallback and will try to map via call to
> > > + * scu_a9_get_base.
> > > + * This will return ownership of scu_base to the caller
> > > + */
> > > +void __iomem *of_scu_get_base(void)
> > > +{
> > > + unsigned long base = 0;
> > > + struct device_node *np;
> > > + void __iomem *scu_base;
> > > +
> > > + np = of_find_matching_node(NULL, scu_match);
> > 
> > could we check np before calling of_iomap()?
> >   
> > > + scu_base = of_iomap(np, 0);
> > > + of_node_put(np);
> > > + if (!scu_base) {
> > > + pr_err("%s failed to map scu_base via DT\n", __func__);
> > 
> > For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> > what does it mean, but it may confuse normal users. In current version,
> > berlin doesn't complain like this for non-ca9 SoCs  
> 
> oops, I just realized that the non-ca9 berlin arm SoC version isn't 
> upstreamed.
> Below is the draft version I planed. Basically speaking, the code tries to
> find "arm,cortex-a9-scu" node from DT, if can't, we think we don't need to
> worry about SCU. Is there any elegant solution for my situation?

I just realized that (another realized :D) we uses PSCI enable method for
non-ca9 base SoCs, so "marvell,berlin-smp" enable method is only for the SoCs
which have CA9 compatible SCU. Sorry for the noise, your patch looks good to me.

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

Hi Jisheng,

On Monday 14 November 2016 12:24 PM, Jisheng Zhang wrote:
> 
> On Mon, 14 Nov 2016 14:12:51 +0800 Jisheng Zhang wrote:
> 
>> Hi Pankaj,
>>



>>> + * Helper API to get SCU base address
>>> + * In case platform DT do not have SCU node, or iomap fails
>>> + * this call will fallback and will try to map via call to
>>> + * scu_a9_get_base.
>>> + * This will return ownership of scu_base to the caller
>>> + */
>>> +void __iomem *of_scu_get_base(void)
>>> +{
>>> +   unsigned long base = 0;
>>> +   struct device_node *np;
>>> +   void __iomem *scu_base;
>>> +
>>> +   np = of_find_matching_node(NULL, scu_match);  
>>
>> could we check np before calling of_iomap()?
>>
>>> +   scu_base = of_iomap(np, 0);
>>> +   of_node_put(np);
>>> +   if (!scu_base) {
>>> +   pr_err("%s failed to map scu_base via DT\n", __func__);  
>>
>> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
>> what does it mean, but it may confuse normal users. In current version,
>> berlin doesn't complain like this for non-ca9 SoCs
> 
> oops, I just realized that the non-ca9 berlin arm SoC version isn't 
> upstreamed.
> Below is the draft version I planed. Basically speaking, the code tries to
> find "arm,cortex-a9-scu" node from DT, if can't, we think we don't need to
> worry about SCU. Is there any elegant solution for my situation?
> 

To adopt new generic API I have submitted a patch for Berlin (along with
other various platforms) here:

https://patchwork.kernel.org/patch/9426457/

Please review and if possible test and let me know feedback.

Thanks,
Pankaj Dubey

> Thanks,
> Jisheng
> 
> 
> 8<---
> --- a/arch/arm/mach-berlin/platsmp.c
> +++ b/arch/arm/mach-berlin/platsmp.c
> @@ -56,22 +56,25 @@ static void __init berlin_smp_prepare_cpus(unsigned int 
> max_cpus)
>   void __iomem *vectors_base;
>  
>   np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
> - scu_base = of_iomap(np, 0);
> - of_node_put(np);
> - if (!scu_base)
> - return;
> + if (np) {
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base)
> + return;
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + }
>  
>   np = of_find_compatible_node(NULL, NULL, "marvell,berlin-cpu-ctrl");
>   cpu_ctrl = of_iomap(np, 0);
>   of_node_put(np);
>   if (!cpu_ctrl)
> - goto unmap_scu;
> + return;
>  
>   vectors_base = ioremap(CONFIG_VECTORS_BASE, SZ_32K);
>   if (!vectors_base)
> - goto unmap_scu;
> + return;
>  
> - scu_enable(scu_base);
>   flush_cache_all();
>  
>   /*
> @@ -87,8 +90,6 @@ static void __init berlin_smp_prepare_cpus(unsigned int 
> max_cpus)
>   writel(virt_to_phys(secondary_startup), vectors_base + SW_RESET_ADDR);
>  
>   iounmap(vectors_base);
> -unmap_scu:
> - iounmap(scu_base);
>  }
>  
>  static struct smp_operations berlin_smp_ops __initdata = {
> 
> 
>>
>>> +   if (scu_a9_has_base()) {
>>> +   base = scu_a9_get_base();
>>> +   scu_base = ioremap(base, SZ_4K);
>>> +   }
>>> +   if (!scu_base) {
>>> +   pr_err("%s failed to map scu_base\n", __func__);  
>>
>> ditto
>>
>>> +   return IOMEM_ERR_PTR(-ENOMEM);
>>> +   }
>>> +   }
>>> +   return scu_base;
>>> +}
>>> +
>>> +/*
>>> + * Enable SCU via mapping scu_base DT
>>> + * If scu_base mapped successfully scu will be enabled and in case of
>>> + * failure if will return non-zero error code
>>> + */
>>> +int of_scu_enable(void)
>>> +{
>>> +   void __iomem *scu_base;
>>> +
>>> +   scu_base = of_scu_get_base();
>>> +   if (!IS_ERR(scu_base)) {
>>> +   scu_enable(scu_base);
>>> +   iounmap(scu_base);
>>> +   return 0;
>>> +   }
>>> +   return PTR_ERR(scu_base);
>>> +}
>>> +
>>>  #endif
>>>  
>>>  /*  
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

Hi Jisheng,

On Monday 14 November 2016 12:24 PM, Jisheng Zhang wrote:
> 
> On Mon, 14 Nov 2016 14:12:51 +0800 Jisheng Zhang wrote:
> 
>> Hi Pankaj,
>>



>>> + * Helper API to get SCU base address
>>> + * In case platform DT do not have SCU node, or iomap fails
>>> + * this call will fallback and will try to map via call to
>>> + * scu_a9_get_base.
>>> + * This will return ownership of scu_base to the caller
>>> + */
>>> +void __iomem *of_scu_get_base(void)
>>> +{
>>> +   unsigned long base = 0;
>>> +   struct device_node *np;
>>> +   void __iomem *scu_base;
>>> +
>>> +   np = of_find_matching_node(NULL, scu_match);  
>>
>> could we check np before calling of_iomap()?
>>
>>> +   scu_base = of_iomap(np, 0);
>>> +   of_node_put(np);
>>> +   if (!scu_base) {
>>> +   pr_err("%s failed to map scu_base via DT\n", __func__);  
>>
>> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
>> what does it mean, but it may confuse normal users. In current version,
>> berlin doesn't complain like this for non-ca9 SoCs
> 
> oops, I just realized that the non-ca9 berlin arm SoC version isn't 
> upstreamed.
> Below is the draft version I planed. Basically speaking, the code tries to
> find "arm,cortex-a9-scu" node from DT, if can't, we think we don't need to
> worry about SCU. Is there any elegant solution for my situation?
> 

To adopt new generic API I have submitted a patch for Berlin (along with
other various platforms) here:

https://patchwork.kernel.org/patch/9426457/

Please review and if possible test and let me know feedback.

Thanks,
Pankaj Dubey

> Thanks,
> Jisheng
> 
> 
> 8<---
> --- a/arch/arm/mach-berlin/platsmp.c
> +++ b/arch/arm/mach-berlin/platsmp.c
> @@ -56,22 +56,25 @@ static void __init berlin_smp_prepare_cpus(unsigned int 
> max_cpus)
>   void __iomem *vectors_base;
>  
>   np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
> - scu_base = of_iomap(np, 0);
> - of_node_put(np);
> - if (!scu_base)
> - return;
> + if (np) {
> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base)
> + return;
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + }
>  
>   np = of_find_compatible_node(NULL, NULL, "marvell,berlin-cpu-ctrl");
>   cpu_ctrl = of_iomap(np, 0);
>   of_node_put(np);
>   if (!cpu_ctrl)
> - goto unmap_scu;
> + return;
>  
>   vectors_base = ioremap(CONFIG_VECTORS_BASE, SZ_32K);
>   if (!vectors_base)
> - goto unmap_scu;
> + return;
>  
> - scu_enable(scu_base);
>   flush_cache_all();
>  
>   /*
> @@ -87,8 +90,6 @@ static void __init berlin_smp_prepare_cpus(unsigned int 
> max_cpus)
>   writel(virt_to_phys(secondary_startup), vectors_base + SW_RESET_ADDR);
>  
>   iounmap(vectors_base);
> -unmap_scu:
> - iounmap(scu_base);
>  }
>  
>  static struct smp_operations berlin_smp_ops __initdata = {
> 
> 
>>
>>> +   if (scu_a9_has_base()) {
>>> +   base = scu_a9_get_base();
>>> +   scu_base = ioremap(base, SZ_4K);
>>> +   }
>>> +   if (!scu_base) {
>>> +   pr_err("%s failed to map scu_base\n", __func__);  
>>
>> ditto
>>
>>> +   return IOMEM_ERR_PTR(-ENOMEM);
>>> +   }
>>> +   }
>>> +   return scu_base;
>>> +}
>>> +
>>> +/*
>>> + * Enable SCU via mapping scu_base DT
>>> + * If scu_base mapped successfully scu will be enabled and in case of
>>> + * failure if will return non-zero error code
>>> + */
>>> +int of_scu_enable(void)
>>> +{
>>> +   void __iomem *scu_base;
>>> +
>>> +   scu_base = of_scu_get_base();
>>> +   if (!IS_ERR(scu_base)) {
>>> +   scu_enable(scu_base);
>>> +   iounmap(scu_base);
>>> +   return 0;
>>> +   }
>>> +   return PTR_ERR(scu_base);
>>> +}
>>> +
>>>  #endif
>>>  
>>>  /*  
>>
>>
>> ___
>> linux-arm-kernel mailing list
>> linux-arm-ker...@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> 
> 
> 
> 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

Hi Jisheng,

On Monday 14 November 2016 11:42 AM, Jisheng Zhang wrote:
> Hi Pankaj,
> 
> On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:



>> +
>> +np = of_find_matching_node(NULL, scu_match);
> 
> could we check np before calling of_iomap()?
> 

of_iomap takes care of that, and will return NULL if np is NULL.

So additional check of np is not required here.

>> +scu_base = of_iomap(np, 0);
>> +of_node_put(np);
>> +if (!scu_base) {
>> +pr_err("%s failed to map scu_base via DT\n", __func__);
> 
> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> what does it mean, but it may confuse normal users. In current version,
> berlin doesn't complain like this for non-ca9 SoCs
> 

OK, let me see other reviewer's comment on this. Then we will decide if
this error message is required or can be omitted.


Thanks,
Pankaj Dubey

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[pankaj.dubey]

Hi Jisheng,

On Monday 14 November 2016 11:42 AM, Jisheng Zhang wrote:
> Hi Pankaj,
> 
> On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:



>> +
>> +np = of_find_matching_node(NULL, scu_match);
> 
> could we check np before calling of_iomap()?
> 

of_iomap takes care of that, and will return NULL if np is NULL.

So additional check of np is not required here.

>> +scu_base = of_iomap(np, 0);
>> +of_node_put(np);
>> +if (!scu_base) {
>> +pr_err("%s failed to map scu_base via DT\n", __func__);
> 
> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> what does it mean, but it may confuse normal users. In current version,
> berlin doesn't complain like this for non-ca9 SoCs
> 

OK, let me see other reviewer's comment on this. Then we will decide if
this error message is required or can be omitted.


Thanks,
Pankaj Dubey

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Jisheng Zhang]

On Mon, 14 Nov 2016 14:12:51 +0800 Jisheng Zhang wrote:

> Hi Pankaj,
> 
> On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:
> 
> > Many platforms are duplicating code for enabling SCU, lets add
> > common code to enable SCU by parsing SCU device node so the duplication
> > in each platform can be avoided.
> > 
> > CC: Krzysztof Kozlowski 
> > CC: Jisheng Zhang 
> > CC: Russell King 
> > CC: Dinh Nguyen 
> > CC: Patrice Chotard 
> > CC: Linus Walleij 
> > CC: Liviu Dudau 
> > CC: Ray Jui 
> > CC: Stephen Warren 
> > CC: Heiko Stuebner 
> > CC: Shawn Guo 
> > CC: Michal Simek 
> > CC: Wei Xu 
> > CC: Andrew Lunn 
> > CC: Jun Nie  
> > Suggested-by: Arnd Bergmann 
> > Signed-off-by: Pankaj Dubey 
> > ---
> >  arch/arm/include/asm/smp_scu.h |  4 +++
> >  arch/arm/kernel/smp_scu.c  | 56 
> > ++
> >  2 files changed, 60 insertions(+)
> > 
> > diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> > index bfe163c..fdeec07 100644
> > --- a/arch/arm/include/asm/smp_scu.h
> > +++ b/arch/arm/include/asm/smp_scu.h
> > @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, 
> > unsigned int mode)
> >  
> >  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> >  void scu_enable(void __iomem *scu_base);
> > +void __iomem *of_scu_get_base(void);
> > +int of_scu_enable(void);
> >  #else
> >  static inline void scu_enable(void __iomem *scu_base) {}
> > +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> > +static inline int of_scu_enable(void) {return 0; }
> >  #endif
> >  
> >  #endif
> > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > index 72f9241..d0ac3ed 100644
> > --- a/arch/arm/kernel/smp_scu.c
> > +++ b/arch/arm/kernel/smp_scu.c
> > @@ -10,6 +10,7 @@
> >   */
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  #include 
> > @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
> >  */
> > flush_cache_all();
> >  }
> > +
> > +static const struct of_device_id scu_match[] = {
> > +   { .compatible = "arm,cortex-a9-scu", },
> > +   { .compatible = "arm,cortex-a5-scu", },
> > +   { }
> > +};
> > +
> > +/*
> > + * Helper API to get SCU base address
> > + * In case platform DT do not have SCU node, or iomap fails
> > + * this call will fallback and will try to map via call to
> > + * scu_a9_get_base.
> > + * This will return ownership of scu_base to the caller
> > + */
> > +void __iomem *of_scu_get_base(void)
> > +{
> > +   unsigned long base = 0;
> > +   struct device_node *np;
> > +   void __iomem *scu_base;
> > +
> > +   np = of_find_matching_node(NULL, scu_match);  
> 
> could we check np before calling of_iomap()?
> 
> > +   scu_base = of_iomap(np, 0);
> > +   of_node_put(np);
> > +   if (!scu_base) {
> > +   pr_err("%s failed to map scu_base via DT\n", __func__);  
> 
> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> what does it mean, but it may confuse normal users. In current version,
> berlin doesn't complain like this for non-ca9 SoCs

oops, I just realized that the non-ca9 berlin arm SoC version isn't upstreamed.
Below is the draft version I planed. Basically speaking, the code tries to
find "arm,cortex-a9-scu" node from DT, if can't, we think we don't need to
worry about SCU. Is there any elegant solution for my situation?

Thanks,
Jisheng


8<---
--- a/arch/arm/mach-berlin/platsmp.c
+++ b/arch/arm/mach-berlin/platsmp.c
@@ -56,22 +56,25 @@ static void __init berlin_smp_prepare_cpus(unsigned int 
max_cpus)
void __iomem *vectors_base;
 
np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
-   scu_base = of_iomap(np, 0);
-   of_node_put(np);
-   if (!scu_base)
-   return;
+   if (np) {
+   scu_base = of_iomap(np, 0);
+   of_node_put(np);
+   if (!scu_base)
+   return;
+   scu_enable(scu_base);
+   iounmap(scu_base);
+   }
 
np = of_find_compatible_node(NULL, NULL, "marvell,berlin-cpu-ctrl");
cpu_ctrl = of_iomap(np, 0);
of_node_put(np);
if (!cpu_ctrl)
-   goto unmap_scu;
+   return;
 
vectors_base = ioremap(CONFIG_VECTORS_BASE, SZ_32K);
if (!vectors_base)
-   goto unmap_scu;
+   return;
 
-   scu_enable(scu_base);
flush_cache_all();
 
/*
@@ -87,8 +90,6 @@ static void __init berlin_smp_prepare_cpus(unsigned int 
max_cpus)
writel(virt_to_phys(secondary_startup), 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Jisheng Zhang]

On Mon, 14 Nov 2016 14:12:51 +0800 Jisheng Zhang wrote:

> Hi Pankaj,
> 
> On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:
> 
> > Many platforms are duplicating code for enabling SCU, lets add
> > common code to enable SCU by parsing SCU device node so the duplication
> > in each platform can be avoided.
> > 
> > CC: Krzysztof Kozlowski 
> > CC: Jisheng Zhang 
> > CC: Russell King 
> > CC: Dinh Nguyen 
> > CC: Patrice Chotard 
> > CC: Linus Walleij 
> > CC: Liviu Dudau 
> > CC: Ray Jui 
> > CC: Stephen Warren 
> > CC: Heiko Stuebner 
> > CC: Shawn Guo 
> > CC: Michal Simek 
> > CC: Wei Xu 
> > CC: Andrew Lunn 
> > CC: Jun Nie  
> > Suggested-by: Arnd Bergmann 
> > Signed-off-by: Pankaj Dubey 
> > ---
> >  arch/arm/include/asm/smp_scu.h |  4 +++
> >  arch/arm/kernel/smp_scu.c  | 56 
> > ++
> >  2 files changed, 60 insertions(+)
> > 
> > diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> > index bfe163c..fdeec07 100644
> > --- a/arch/arm/include/asm/smp_scu.h
> > +++ b/arch/arm/include/asm/smp_scu.h
> > @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, 
> > unsigned int mode)
> >  
> >  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
> >  void scu_enable(void __iomem *scu_base);
> > +void __iomem *of_scu_get_base(void);
> > +int of_scu_enable(void);
> >  #else
> >  static inline void scu_enable(void __iomem *scu_base) {}
> > +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> > +static inline int of_scu_enable(void) {return 0; }
> >  #endif
> >  
> >  #endif
> > diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> > index 72f9241..d0ac3ed 100644
> > --- a/arch/arm/kernel/smp_scu.c
> > +++ b/arch/arm/kernel/smp_scu.c
> > @@ -10,6 +10,7 @@
> >   */
> >  #include 
> >  #include 
> > +#include 
> >  
> >  #include 
> >  #include 
> > @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
> >  */
> > flush_cache_all();
> >  }
> > +
> > +static const struct of_device_id scu_match[] = {
> > +   { .compatible = "arm,cortex-a9-scu", },
> > +   { .compatible = "arm,cortex-a5-scu", },
> > +   { }
> > +};
> > +
> > +/*
> > + * Helper API to get SCU base address
> > + * In case platform DT do not have SCU node, or iomap fails
> > + * this call will fallback and will try to map via call to
> > + * scu_a9_get_base.
> > + * This will return ownership of scu_base to the caller
> > + */
> > +void __iomem *of_scu_get_base(void)
> > +{
> > +   unsigned long base = 0;
> > +   struct device_node *np;
> > +   void __iomem *scu_base;
> > +
> > +   np = of_find_matching_node(NULL, scu_match);  
> 
> could we check np before calling of_iomap()?
> 
> > +   scu_base = of_iomap(np, 0);
> > +   of_node_put(np);
> > +   if (!scu_base) {
> > +   pr_err("%s failed to map scu_base via DT\n", __func__);  
> 
> For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
> what does it mean, but it may confuse normal users. In current version,
> berlin doesn't complain like this for non-ca9 SoCs

oops, I just realized that the non-ca9 berlin arm SoC version isn't upstreamed.
Below is the draft version I planed. Basically speaking, the code tries to
find "arm,cortex-a9-scu" node from DT, if can't, we think we don't need to
worry about SCU. Is there any elegant solution for my situation?

Thanks,
Jisheng


8<---
--- a/arch/arm/mach-berlin/platsmp.c
+++ b/arch/arm/mach-berlin/platsmp.c
@@ -56,22 +56,25 @@ static void __init berlin_smp_prepare_cpus(unsigned int 
max_cpus)
void __iomem *vectors_base;
 
np = of_find_compatible_node(NULL, NULL, "arm,cortex-a9-scu");
-   scu_base = of_iomap(np, 0);
-   of_node_put(np);
-   if (!scu_base)
-   return;
+   if (np) {
+   scu_base = of_iomap(np, 0);
+   of_node_put(np);
+   if (!scu_base)
+   return;
+   scu_enable(scu_base);
+   iounmap(scu_base);
+   }
 
np = of_find_compatible_node(NULL, NULL, "marvell,berlin-cpu-ctrl");
cpu_ctrl = of_iomap(np, 0);
of_node_put(np);
if (!cpu_ctrl)
-   goto unmap_scu;
+   return;
 
vectors_base = ioremap(CONFIG_VECTORS_BASE, SZ_32K);
if (!vectors_base)
-   goto unmap_scu;
+   return;
 
-   scu_enable(scu_base);
flush_cache_all();
 
/*
@@ -87,8 +90,6 @@ static void __init berlin_smp_prepare_cpus(unsigned int 
max_cpus)
writel(virt_to_phys(secondary_startup), vectors_base + SW_RESET_ADDR);
 
iounmap(vectors_base);
-unmap_scu:
-   iounmap(scu_base);
 }
 
 static struct smp_operations berlin_smp_ops __initdata = {


> 
> > +   if (scu_a9_has_base()) {
> > +   base = scu_a9_get_base();
> > +   scu_base = ioremap(base, SZ_4K);
> > +   }
> > +   if (!scu_base) 

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Jisheng Zhang]

Hi Pankaj,

On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:

> Many platforms are duplicating code for enabling SCU, lets add
> common code to enable SCU by parsing SCU device node so the duplication
> in each platform can be avoided.
> 
> CC: Krzysztof Kozlowski 
> CC: Jisheng Zhang 
> CC: Russell King 
> CC: Dinh Nguyen 
> CC: Patrice Chotard 
> CC: Linus Walleij 
> CC: Liviu Dudau 
> CC: Ray Jui 
> CC: Stephen Warren 
> CC: Heiko Stuebner 
> CC: Shawn Guo 
> CC: Michal Simek 
> CC: Wei Xu 
> CC: Andrew Lunn 
> CC: Jun Nie  
> Suggested-by: Arnd Bergmann 
> Signed-off-by: Pankaj Dubey 
> ---
>  arch/arm/include/asm/smp_scu.h |  4 +++
>  arch/arm/kernel/smp_scu.c  | 56 
> ++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> index bfe163c..fdeec07 100644
> --- a/arch/arm/include/asm/smp_scu.h
> +++ b/arch/arm/include/asm/smp_scu.h
> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, 
> unsigned int mode)
>  
>  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
>  void scu_enable(void __iomem *scu_base);
> +void __iomem *of_scu_get_base(void);
> +int of_scu_enable(void);
>  #else
>  static inline void scu_enable(void __iomem *scu_base) {}
> +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> +static inline int of_scu_enable(void) {return 0; }
>  #endif
>  
>  #endif
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 72f9241..d0ac3ed 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -10,6 +10,7 @@
>   */
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
>*/
>   flush_cache_all();
>  }
> +
> +static const struct of_device_id scu_match[] = {
> + { .compatible = "arm,cortex-a9-scu", },
> + { .compatible = "arm,cortex-a5-scu", },
> + { }
> +};
> +
> +/*
> + * Helper API to get SCU base address
> + * In case platform DT do not have SCU node, or iomap fails
> + * this call will fallback and will try to map via call to
> + * scu_a9_get_base.
> + * This will return ownership of scu_base to the caller
> + */
> +void __iomem *of_scu_get_base(void)
> +{
> + unsigned long base = 0;
> + struct device_node *np;
> + void __iomem *scu_base;
> +
> + np = of_find_matching_node(NULL, scu_match);

could we check np before calling of_iomap()?

> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base via DT\n", __func__);

For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
what does it mean, but it may confuse normal users. In current version,
berlin doesn't complain like this for non-ca9 SoCs

> + if (scu_a9_has_base()) {
> + base = scu_a9_get_base();
> + scu_base = ioremap(base, SZ_4K);
> + }
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base\n", __func__);

ditto

> + return IOMEM_ERR_PTR(-ENOMEM);
> + }
> + }
> + return scu_base;
> +}
> +
> +/*
> + * Enable SCU via mapping scu_base DT
> + * If scu_base mapped successfully scu will be enabled and in case of
> + * failure if will return non-zero error code
> + */
> +int of_scu_enable(void)
> +{
> + void __iomem *scu_base;
> +
> + scu_base = of_scu_get_base();
> + if (!IS_ERR(scu_base)) {
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + return 0;
> + }
> + return PTR_ERR(scu_base);
> +}
> +
>  #endif
>  
>  /*

Re: [PATCH 01/16] ARM: scu: Provide support for parsing SCU device node to enable SCU

[Jisheng Zhang]

Hi Pankaj,

On Mon, 14 Nov 2016 10:31:56 +0530 Pankaj Dubey wrote:

> Many platforms are duplicating code for enabling SCU, lets add
> common code to enable SCU by parsing SCU device node so the duplication
> in each platform can be avoided.
> 
> CC: Krzysztof Kozlowski 
> CC: Jisheng Zhang 
> CC: Russell King 
> CC: Dinh Nguyen 
> CC: Patrice Chotard 
> CC: Linus Walleij 
> CC: Liviu Dudau 
> CC: Ray Jui 
> CC: Stephen Warren 
> CC: Heiko Stuebner 
> CC: Shawn Guo 
> CC: Michal Simek 
> CC: Wei Xu 
> CC: Andrew Lunn 
> CC: Jun Nie  
> Suggested-by: Arnd Bergmann 
> Signed-off-by: Pankaj Dubey 
> ---
>  arch/arm/include/asm/smp_scu.h |  4 +++
>  arch/arm/kernel/smp_scu.c  | 56 
> ++
>  2 files changed, 60 insertions(+)
> 
> diff --git a/arch/arm/include/asm/smp_scu.h b/arch/arm/include/asm/smp_scu.h
> index bfe163c..fdeec07 100644
> --- a/arch/arm/include/asm/smp_scu.h
> +++ b/arch/arm/include/asm/smp_scu.h
> @@ -39,8 +39,12 @@ static inline int scu_power_mode(void __iomem *scu_base, 
> unsigned int mode)
>  
>  #if defined(CONFIG_SMP) && defined(CONFIG_HAVE_ARM_SCU)
>  void scu_enable(void __iomem *scu_base);
> +void __iomem *of_scu_get_base(void);
> +int of_scu_enable(void);
>  #else
>  static inline void scu_enable(void __iomem *scu_base) {}
> +static inline void __iomem *of_scu_get_base(void) {return NULL; }
> +static inline int of_scu_enable(void) {return 0; }
>  #endif
>  
>  #endif
> diff --git a/arch/arm/kernel/smp_scu.c b/arch/arm/kernel/smp_scu.c
> index 72f9241..d0ac3ed 100644
> --- a/arch/arm/kernel/smp_scu.c
> +++ b/arch/arm/kernel/smp_scu.c
> @@ -10,6 +10,7 @@
>   */
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -70,6 +71,61 @@ void scu_enable(void __iomem *scu_base)
>*/
>   flush_cache_all();
>  }
> +
> +static const struct of_device_id scu_match[] = {
> + { .compatible = "arm,cortex-a9-scu", },
> + { .compatible = "arm,cortex-a5-scu", },
> + { }
> +};
> +
> +/*
> + * Helper API to get SCU base address
> + * In case platform DT do not have SCU node, or iomap fails
> + * this call will fallback and will try to map via call to
> + * scu_a9_get_base.
> + * This will return ownership of scu_base to the caller
> + */
> +void __iomem *of_scu_get_base(void)
> +{
> + unsigned long base = 0;
> + struct device_node *np;
> + void __iomem *scu_base;
> +
> + np = of_find_matching_node(NULL, scu_match);

could we check np before calling of_iomap()?

> + scu_base = of_iomap(np, 0);
> + of_node_put(np);
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base via DT\n", __func__);

For non-ca5, non-ca9 based SoCs, we'll see this error msg. We understand
what does it mean, but it may confuse normal users. In current version,
berlin doesn't complain like this for non-ca9 SoCs

> + if (scu_a9_has_base()) {
> + base = scu_a9_get_base();
> + scu_base = ioremap(base, SZ_4K);
> + }
> + if (!scu_base) {
> + pr_err("%s failed to map scu_base\n", __func__);

ditto

> + return IOMEM_ERR_PTR(-ENOMEM);
> + }
> + }
> + return scu_base;
> +}
> +
> +/*
> + * Enable SCU via mapping scu_base DT
> + * If scu_base mapped successfully scu will be enabled and in case of
> + * failure if will return non-zero error code
> + */
> +int of_scu_enable(void)
> +{
> + void __iomem *scu_base;
> +
> + scu_base = of_scu_get_base();
> + if (!IS_ERR(scu_base)) {
> + scu_enable(scu_base);
> + iounmap(scu_base);
> + return 0;
> + }
> + return PTR_ERR(scu_base);
> +}
> +
>  #endif
>  
>  /*