Re: [PATCH 1/6] ARM: sunxi: Add Machine support for A33

[Maxime Ripard]

Hi,

On Sun, May 10, 2015 at 12:16:18PM +0530, Vishnu Patekar wrote:
> Allwinnner A33 quad core cortex-a7 based SOC.

There's one n to many in Allwinner, and having a verb in that sentence
would help

> It is similar to A23.
> 
> Renamed cpu method to "allwinner,sun8i" for common sun8i smp.
> smp code is generic for A23, A33 and hopefully H3.

Please do only one thing in a patch.

> Signed-off-by: Vishnu Patekar <vishnupatekar0510@xxxxxxxxx>
> ---
>  Documentation/devicetree/bindings/arm/sunxi.txt | 1 +
>  arch/arm/mach-sunxi/Kconfig                     | 2 +-
>  arch/arm/mach-sunxi/platsmp.c                   | 2 +-
>  arch/arm/mach-sunxi/sunxi.c                     | 4 ++--
>  4 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/arm/sunxi.txt b/Documentation/devicetree/bindings/arm/sunxi.txt
> index 42941fd..e32f082 100644
> --- a/Documentation/devicetree/bindings/arm/sunxi.txt
> +++ b/Documentation/devicetree/bindings/arm/sunxi.txt
> @@ -9,4 +9,5 @@ using one of the following compatible strings:
>    allwinner,sun6i-a31
>    allwinner,sun7i-a20
>    allwinner,sun8i-a23
> +  allwinner,sun8i-a33

Here you're introducing a new compatible for a machine that is
sun8i-a33.... [1]

>    allwinner,sun9i-a80
> diff --git a/arch/arm/mach-sunxi/Kconfig b/arch/arm/mach-sunxi/Kconfig
> index 81502b9..38bedd8 100644
> --- a/arch/arm/mach-sunxi/Kconfig
> +++ b/arch/arm/mach-sunxi/Kconfig
> @@ -35,7 +35,7 @@ config MACH_SUN7I
>  	select SUN5I_HSTIMER
>  
>  config MACH_SUN8I
> -	bool "Allwinner A23 (sun8i) SoCs support"
> +	bool "Allwinner (sun8i) SoCs support"
>  	default ARCH_SUNXI
>  	select ARM_GIC
>  	select MFD_SUN6I_PRCM
> diff --git a/arch/arm/mach-sunxi/platsmp.c b/arch/arm/mach-sunxi/platsmp.c
> index e8483ec..c56b501 100644
> --- a/arch/arm/mach-sunxi/platsmp.c
> +++ b/arch/arm/mach-sunxi/platsmp.c
> @@ -189,4 +189,4 @@ struct smp_operations sun8i_smp_ops __initdata = {
>  	.smp_prepare_cpus	= sun8i_smp_prepare_cpus,
>  	.smp_boot_secondary	= sun8i_smp_boot_secondary,
>  };
> -CPU_METHOD_OF_DECLARE(sun8i_a23_smp, "allwinner,sun8i-a23", &sun8i_smp_ops);
> +CPU_METHOD_OF_DECLARE(sun8i_smp, "allwinner,sun8i", &sun8i_smp_ops);

Like I was saying, this is an unrelated thing, it should be in a
separate patch.

And this is wrong.

A compatible should be made for the first IP that uses it. The first
user of that particular method has been the A23, it should be what's
in the compatible.

If the A33 is by chance using the exact same code, then we have two
choices, either reuse that compatible, or introduce a new one if it
slightly differs. And since the A33 has more cores than the A23, it
does differ.

So please add a new compatible.

That also breaks the SMP code in the A23 which is a no-go, since the
compatible would have changed but not the DT.

> diff --git a/arch/arm/mach-sunxi/sunxi.c b/arch/arm/mach-sunxi/sunxi.c
> index 1bc811a..8937d0d 100644
> --- a/arch/arm/mach-sunxi/sunxi.c
> +++ b/arch/arm/mach-sunxi/sunxi.c
> @@ -66,11 +66,11 @@ DT_MACHINE_START(SUN7I_DT, "Allwinner sun7i (A20) Family")
>  MACHINE_END
>  
>  static const char * const sun8i_board_dt_compat[] = {
> -	"allwinner,sun8i-a23",
> +	"allwinner,sun8i",

[1] ... And here, you don't introduce that new machine compatible, but
remove one a use another one instead....

Apart from the documentation mismatch, you really shouldn't do that.

The machine compatible should be a identifier for the board and the
SoC, so that we can identify both easily, and possibly enable
quirks. The only question you should ask yourself whenever you add a
new compatible is "is this exactly the same IP" ?

In such a case, is the A23 *exactly* the same as the H3 and the A33?

The answer is obviously no, otherwise we would not have this patchset
in the first place.

So you just need to introduce a new compatible for the A33, just like
you did in the Documentation, and add that new compatible in the machine.

>  	NULL,
>  };
>  
> -DT_MACHINE_START(SUN8I_DT, "Allwinner sun8i (A23) Family")
> +DT_MACHINE_START(SUN8I_DT, "Allwinner sun8i Family")
>  	.dt_compat	= sun8i_board_dt_compat,
>  	.init_late	= sunxi_dt_cpufreq_init,
>  MACHINE_END
> -- 
> 1.9.1
> 

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Attachment: signature.asc
Description: Digital signature