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