| Message ID | qvuhez7vrcoui7i6s4yohd4ednneuoejcp6tw6iwzeefgpyvd6@fkwwtwozhakf (mailing list archive) |
|---|---|
| State | New, archived |
| 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?
Hello, On Tue, Feb 27, 2024 at 08:54:03AM +0000, Tudor Ambarus wrote: > 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? I know at least one user who noticed the driver being broken because he needs it and not because a build bot stumbled. Best regards Uwe
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.
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
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 >
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
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 : >>> 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. I think for powerpc you should really check ppc32_allmodconfig in addition to allmodconfig > 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. When I look into drivers/net/ethernet/ibm/emac/core.c it is not much different, they #ifdef out the call to dcri_clrset() when CONFIG_PPC_DCR_NATIVE is not defined. A possible way is to do: +static inline void __dcri_clrset(int base_addr, int base_data, int reg, + unsigned clr, unsigned set) +{ + BUILD_BUG_ON(!IS_ENABLED(CONFIG_COMPILE_TEST); +} Christophe
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.