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

[Uwe Kleine-König]

On Tue, Feb 27, 2024 at 01:52:07PM +0000, Christophe Leroy wrote:
> 
> 
> 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:
> >> 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?
> 
> 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 ?

The reason I'd like to see it in allmodconfig is that testing
allmodconfig on several archs is the check I do for my patch series.
Also I assume I'm not the only one relying on allmodconfig for this
purpose. So in my eyes every driver that is built there is a net win.

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/30dac5cb/attachment.sig>