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/ |
Attachment:
signature.asc
Description: PGP signature
