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 <krzk@xxxxxxxxxx>
> > CC: Jisheng Zhang <jszhang@xxxxxxxxxxx>
> > CC: Russell King <linux@xxxxxxxxxxxxxxx>
> > CC: Dinh Nguyen <dinguyen@xxxxxxxxxxxxxxxxxxxxx>
> > CC: Patrice Chotard <patrice.chotard@xxxxxx>
> > CC: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > CC: Liviu Dudau <liviu.dudau@xxxxxxx>
> > CC: Ray Jui <rjui@xxxxxxxxxxxx>
> > CC: Stephen Warren <swarren@xxxxxxxxxxxxx>
> > CC: Heiko Stuebner <heiko@xxxxxxxxx>
> > CC: Shawn Guo <shawnguo@xxxxxxxxxx>
> > CC: Michal Simek <michal.simek@xxxxxxxxxx>
> > CC: Wei Xu <xuwei5@xxxxxxxxxxxxx>
> > CC: Andrew Lunn <andrew@xxxxxxx>
> > CC: Jun Nie <jun.nie@xxxxxxxxxx> 
> > Suggested-by: Arnd Bergmann <arnd@xxxxxxxx>
> > Signed-off-by: Pankaj Dubey <pankaj.dubey@xxxxxxxxxxx>
> > ---
> >  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 <linux/init.h>
> >  #include <linux/io.h>
> > +#include <linux/of_address.h>
> >  
> >  #include <asm/smp_plat.h>
> >  #include <asm/smp_scu.h>
> > @@ -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) {
> > +			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-kernel@xxxxxxxxxxxxxxxxxxx
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel