Le 27/02/2024 à 11:58, Uwe Kleine-König a écrit :

Hello Christophe,

On Tue, Feb 27, 2024 at 10:25:15AM +0000, Christophe Leroy wrote:

quoted

Le 27/02/2024 à 09:46, Uwe Kleine-König a écrit :

quoted

recently the spi-ppc4xx.c driver suffered from build errors and warnings
that were undetected for longer than I expected. I think it would be
very beneficial if this driver was enabled in (at least) a powerpc
allmodconfig build.

The challenge to do so is that spi-ppc4xx.c uses dcri_clrset() which is
only defined for 4xx (as these select PPC_DCR_NATIVE).

I wonder if dcri_clrset() could be defined for the PPC_DCR_MMIO case,
too. I tried and failed. The best I came up without extensive doc
reading is:

diff --git a/arch/powerpc/include/asm/dcr-native.h b/arch/powerpc/include/asm/dcr-native.h
index a92059964579..159ab7abfe46 100644
--- a/arch/powerpc/include/asm/dcr-native.h
+++ b/arch/powerpc/include/asm/dcr-native.h

@@ -115,15 +115,11 @@ static inline void __dcri_clrset(int base_addr, int base_data, int reg,
   	unsigned int val;
   
   	spin_lock_irqsave(&dcr_ind_lock, flags);
-	if (cpu_has_feature(CPU_FTR_INDEXED_DCR)) {
-		mtdcrx(base_addr, reg);
-		val = (mfdcrx(base_data) & ~clr) | set;
-		mtdcrx(base_data, val);
-	} else {
-		__mtdcr(base_addr, reg);
-		val = (__mfdcr(base_data) & ~clr) | set;
-		__mtdcr(base_data, val);
-	}
+
+	mtdcr(base_addr, reg);
+	val = (mfdcr(base_data) & ~clr) | set;
+	mtdcr(base_data, val);
+
   	spin_unlock_irqrestore(&dcr_ind_lock, flags);
   }
   

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index bc7021da2fe9..9a0a5e8c70c8 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig

@@ -810,7 +810,8 @@ config SPI_PL022
   
   config SPI_PPC4xx
   	tristate "PPC4xx SPI Controller"
-	depends on PPC32 && 4xx
+	depends on 4xx || COMPILE_TEST
+	depends on PPC32 || PPC64
   	select SPI_BITBANG
   	help
   	  This selects a driver for the PPC4xx SPI Controller.

While this is a step in the right direction (I think) it's not enough to
make the driver build (but maybe make it easier to define
dcri_clrset()?)

Could someone with more powerpc knowledge jump in and help (for the
benefit of better compile coverage of the spi driver and so less
breakage)? (If you do so based on my changes above, you don't need to
credit me for my effort, claim it as your's. I'm happy enough if the
situation improves.)

What about this ?

diff --git a/arch/powerpc/include/asm/dcr-mmio.h

b/arch/powerpc/include/asm/dcr-mmio.h
index fc6d93ef4a13..38b515afbffc 100644

--- a/arch/powerpc/include/asm/dcr-mmio.h
+++ b/arch/powerpc/include/asm/dcr-mmio.h

@@ -38,6 +38,11 @@ static inline void dcr_write_mmio(dcr_host_mmio_t host,
   	out_be32(host.token + ((host.base + dcr_n) * host.stride), value);
   }

+static inline void __dcri_clrset(int base_addr, int base_data, int reg,
+				 unsigned clr, unsigned set)
+{
+}
+

The downside of that one is that if we find a matching device where
dcr-mmio is used, the driver claims to work but silently fails. Is this
good enough?

I don't know the details of DCR, but it looks like this spi-ppc4xx 
driver is really specific to 4xx, which is PPC32.

Do you really need/want it to be built with allmodconfig ?

Maybe it would be easier to have it work with ppc32_allmodconfig ?

Or even easier with ppc44x_defconfig ?

Christophe

quoted

   #endif /* __KERNEL__ */
   #endif /* _ASM_POWERPC_DCR_MMIO_H */

diff --git a/arch/powerpc/include/asm/dcr-native.h

b/arch/powerpc/include/asm/dcr-native.h
index a92059964579..2f6221bf5406 100644

--- a/arch/powerpc/include/asm/dcr-native.h
+++ b/arch/powerpc/include/asm/dcr-native.h

@@ -135,10 +135,6 @@ static inline void __dcri_clrset(int base_addr, int

base_data, int reg,
   					 DCRN_ ## base ## _CONFIG_DATA,	\
   					 reg, data)

-#define dcri_clrset(base, reg, clr, set)	__dcri_clrset(DCRN_ ## base ##
_CONFIG_ADDR,	\
-							      DCRN_ ## base ## _CONFIG_DATA,	\
-							      reg, clr, set)
-
   #endif /* __ASSEMBLY__ */
   #endif /* __KERNEL__ */
   #endif /* _ASM_POWERPC_DCR_NATIVE_H */

diff --git a/arch/powerpc/include/asm/dcr.h b/arch/powerpc/include/asm/dcr.h
index 64030e3a1f30..15c123ae38a1 100644
--- a/arch/powerpc/include/asm/dcr.h
+++ b/arch/powerpc/include/asm/dcr.h

@@ -18,6 +18,9 @@
   #include <asm/dcr-mmio.h>
   #endif

+#define dcri_clrset(base, reg, clr, set)	__dcri_clrset(DCRN_ ## base ##

_CONFIG_ADDR,	\
+							      DCRN_ ## base ## _CONFIG_DATA,	\
+							      reg, clr, set)

   /* Indirection layer for providing both NATIVE and MMIO support. */

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index ddae0fde798e..7b003c5dd613 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig

@@ -810,7 +810,7 @@ config SPI_PL022

   config SPI_PPC4xx
   	tristate "PPC4xx SPI Controller"
-	depends on PPC32 && 4xx
+	depends on PPC && (4xx || COMPILE_TEST)

Ah, I wondered about not finding a global powerpc symbol. Just missed it
because I expected it at the top of arch/powerpc/Kconfig.

I would have split the depends line into

	depends on PPC
	depends on 4xx || COMPILE_TEST

but apart from that I like it. Maybe split the change into the powerpc
stuff and then a separate patch changing SPI_PPC4xx?

Another thing I wondered is: Should SPI_PPC4xx better depend on
PPC_DCR_NATIVE instead of 4xx? This is an orthogonal change however.

Best regards
Uwe