| Message ID | qvuhez7vrcoui7i6s4yohd4ednneuoejcp6tw6iwzeefgpyvd6@fkwwtwozhakf |
|---|---|
| State | New |
| Headers | show |
| Series | Increasing build coverage for drivers/spi/spi-ppc4xx.c | expand |
On 2/27/24 08:46, Uwe Kleine-König wrote: > 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 long enough so that we remove the driver altogether?
Le 27/02/2024 à 09:46, Uwe Kleine-König a écrit : > Hello, > > 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) +{ +} + #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) select SPI_BITBANG help This selects a driver for the PPC4xx SPI Controller.
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 ? Christophe > >> #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 >
Christophe Leroy <christophe.leroy@csgroup.eu> writes: > Le 27/02/2024 à 15:00, Uwe Kleine-König a écrit : >> 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 : >>>> 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). ... >> >> 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. > > I think for powerpc you should really check ppc32_allmodconfig in > addition to allmodconfig Yeah. arch/powerpc is really ~7 separate sub architectures. Building allmodconfig and ppc32_allmodconfig at least covers the bulk of the code. But it doesn't include 4xx, or several other platforms. I think the best/easiest option is just to enable this driver in the 44x defconfig. At least that way the auto builders should catch any problems. eg. diff --git a/arch/powerpc/configs/ppc44x_defconfig b/arch/powerpc/configs/ppc44x_defconfig index 8b595f67068c..95a1e7efb79f 100644 --- a/arch/powerpc/configs/ppc44x_defconfig +++ b/arch/powerpc/configs/ppc44x_defconfig @@ -67,6 +67,8 @@ CONFIG_I2C=m CONFIG_I2C_CHARDEV=m CONFIG_I2C_GPIO=m CONFIG_I2C_IBM_IIC=m +CONFIG_SPI=y +CONFIG_SPI_PPC4xx=y # CONFIG_HWMON is not set CONFIG_FB=m CONFIG_USB=m cheers
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.