Re: [PATCH 2/2] powerpc: mpc8xxx_spi: Catch bad chip variants earlier

["J. Neuschäfer" <j.ne@posteo.net>]

On Mon, Mar 03, 2025 at 12:08:38PM +0800, Peng Fan wrote:
> Hi,
> 
> On Mon, Feb 17, 2025 at 04:48:48PM +0100, J. Neuschäfer via B4 Relay wrote:
> >From: "J. Neuschäfer" <j.ne@posteo.net>
> >
> >Currently, enabling the MPC8xxx SPI driver on an unexpected SoC results
> >in a wall of errors because spi8xxx_t isn't defined. This is quite a bad
> >experience, so let's catch this kind of issue in mpc8xxx_spi.h.
> >
> >Signed-off-by: J. Neuschäfer <j.ne@posteo.net>
> >---
> > arch/powerpc/include/asm/mpc8xxx_spi.h | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> >diff --git a/arch/powerpc/include/asm/mpc8xxx_spi.h b/arch/powerpc/include/asm/mpc8xxx_spi.h
> >index 2b2095ad481d19a48e1f8d521626a0356773c0b5..3efd4bdcac2c3725a8065b3bb7faa11c78da0c78 100644
> >--- a/arch/powerpc/include/asm/mpc8xxx_spi.h
> >+++ b/arch/powerpc/include/asm/mpc8xxx_spi.h
> >@@ -28,6 +28,8 @@ typedef struct spi8xxx {
> > } spi8xxx_t;
> > static_assert(sizeof(spi8xxx_t) == 0x1000);
> > 
> >+#else
> >+#error "SPI register layout not defined: Unknown chip variant"
> > #endif
> 
> This cause build error, would you please give a look?:
>    powerpc:  +   kmcoge5ne

A quick analysis of this:

 - CONFIG_TARGET_KMCOGE5NE (indirectly) selects CONFIG_ARCH_MPC8360,
   because the KM boards are based on the MPC8360
 - In immap_83xx.h, the MMIO space definition for MPC8360 does not
   use spi8xxx_t, which is consistent with the MPC8360 datasheet,
   because the MPC8360 uses the QUICC Engine block for SPI functionality

I didn't expect this, and my compile-testing didn't catch it, but in
conclusion, I think my patch is wrong, and I will not include it in
version 2 of this series.

> +In file included from arch/powerpc/include/asm/immap_83xx.h:19,
> +                 from arch/powerpc/include/asm/ppc.h:24,
> +                 from arch/powerpc/include/asm/u-boot.h:23,
> +                 from arch/powerpc/include/asm/global_data.h:98,
> +                 from lib/asm-offsets.c:15:
> +arch/powerpc/include/asm/mpc8xxx_spi.h:32:2: error: #error "SPI register layout not defined: Unknown chip variant"
> +   32 | #error "SPI register layout not defined: Unknown chip variant"
> +      |  ^~~~~
> +make[2]: *** [scripts/Makefile.build:146: lib/asm-offsets.s] Error 1
> +make[1]: *** [Makefile:1980: prepare0] Error 2
> +make: *** [Makefile:177: sub-make] Error 2
>    powerpc:  +   kmeter1

KMETER1 is probably very similar.

> +In file included from arch/powerpc/include/asm/immap_83xx.h:19,
> +                 from arch/powerpc/include/asm/ppc.h:24,
> +                 from arch/powerpc/include/asm/u-boot.h:23,
> +                 from arch/powerpc/include/asm/global_data.h:98,
> +                 from lib/asm-offsets.c:15:
> +arch/powerpc/include/asm/mpc8xxx_spi.h:32:2: error: #error "SPI register layout not defined: Unknown chip variant"
> +   32 | #error "SPI register layout not defined: Unknown chip variant"
> +      |  ^~~~~
> +make[2]: *** [scripts/Makefile.build:146: lib/asm-offsets.s] Error 1
> +make[1]: *** [Makefile:1980: prepare0] Error 2
> +make: *** [Makefile:177: sub-make] Error 2
> microblaze:  w+   microblaze-generic

MicroBlaze is strange, it shouldn't be affected by this


Thank you for your reply!

J. Neuschäfer