Increasing build coverage for drivers/spi/spi-ppc4xx.c

[Uwe Kleine-König]

Hello Christophe,

On Tue, Feb 27, 2024 at 10:25:15AM +0000, Christophe Leroy wrote:
> Le 27/02/2024 à 09:46, Uwe Kleine-König a écrit :
> > 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?

>   #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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20240227/875ab857/attachment-0001.sig>