* Increasing build coverage for drivers/spi/spi-ppc4xx.c @ 2024-02-27 8:46 Uwe Kleine-König 2024-02-27 8:54 ` Tudor Ambarus 2024-02-27 10:25 ` Christophe Leroy 0 siblings, 2 replies; 9+ messages in thread From: Uwe Kleine-König @ 2024-02-27 8:46 UTC (permalink / raw) To: Michael Ellerman Cc: linux-spi, Aneesh Kumar K.V, Mark Brown, Nicholas Piggin, Naveen N. Rao, linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 2390 bytes --] 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.) Thanks Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Increasing build coverage for drivers/spi/spi-ppc4xx.c 2024-02-27 8:46 Increasing build coverage for drivers/spi/spi-ppc4xx.c Uwe Kleine-König @ 2024-02-27 8:54 ` Tudor Ambarus 2024-02-27 9:05 ` Uwe Kleine-König 2024-02-27 10:25 ` Christophe Leroy 1 sibling, 1 reply; 9+ messages in thread From: Tudor Ambarus @ 2024-02-27 8:54 UTC (permalink / raw) To: Uwe Kleine-König, Michael Ellerman Cc: linux-spi, Aneesh Kumar K.V, Mark Brown, Nicholas Piggin, Naveen N. Rao, linuxppc-dev 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? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Increasing build coverage for drivers/spi/spi-ppc4xx.c 2024-02-27 8:54 ` Tudor Ambarus @ 2024-02-27 9:05 ` Uwe Kleine-König 0 siblings, 0 replies; 9+ messages in thread From: Uwe Kleine-König @ 2024-02-27 9:05 UTC (permalink / raw) To: Tudor Ambarus Cc: linux-spi, Aneesh Kumar K.V, Mark Brown, Nicholas Piggin, Naveen N. Rao, linuxppc-dev [-- Attachment #1: Type: text/plain, Size: 634 bytes --] 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 -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | https://www.pengutronix.de/ | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Increasing build coverage for drivers/spi/spi-ppc4xx.c 2024-02-27 8:46 Increasing build coverage for drivers/spi/spi-ppc4xx.c Uwe Kleine-König 2024-02-27 8:54 ` Tudor Ambarus @ 2024-02-27 10:25 ` Christophe Leroy 2024-02-27 10:58 ` Uwe Kleine-König 1 sibling, 1 reply; 9+ messages in thread From: Christophe Leroy @ 2024-02-27 10:25 UTC (permalink / raw) To: Uwe Kleine-König, Michael Ellerman Cc: Nicholas Piggin, linux-spi@vger.kernel.org, Aneesh Kumar K.V, Mark Brown, Naveen N. Rao, linuxppc-dev@lists.ozlabs.org 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. ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: Increasing build coverage for drivers/spi/spi-ppc4xx.c 2024-02-27 10:25 ` Christophe Leroy @ 2024-02-27 10:58 ` Uwe Kleine-König 2024-02-27 13:52 ` Christophe Leroy 0 siblings, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2024-02-27 10:58 UTC (permalink / raw) To: Christophe Leroy Cc: Nicholas Piggin, linux-spi@vger.kernel.org, Aneesh Kumar K.V, Mark Brown, Naveen N. Rao, linuxppc-dev@lists.ozlabs.org [-- Attachment #1: Type: text/plain, Size: 5623 bytes --] 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Increasing build coverage for drivers/spi/spi-ppc4xx.c 2024-02-27 10:58 ` Uwe Kleine-König @ 2024-02-27 13:52 ` Christophe Leroy 2024-02-27 14:00 ` Uwe Kleine-König 0 siblings, 1 reply; 9+ messages in thread From: Christophe Leroy @ 2024-02-27 13:52 UTC (permalink / raw) To: Uwe Kleine-König Cc: Nicholas Piggin, linux-spi@vger.kernel.org, Aneesh Kumar K.V, Mark Brown, Naveen N. Rao, linuxppc-dev@lists.ozlabs.org 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 > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Increasing build coverage for drivers/spi/spi-ppc4xx.c 2024-02-27 13:52 ` Christophe Leroy @ 2024-02-27 14:00 ` Uwe Kleine-König 2024-02-27 14:38 ` Christophe Leroy 0 siblings, 1 reply; 9+ messages in thread From: Uwe Kleine-König @ 2024-02-27 14:00 UTC (permalink / raw) To: Christophe Leroy Cc: Nicholas Piggin, linux-spi@vger.kernel.org, Aneesh Kumar K.V, Mark Brown, Naveen N. Rao, linuxppc-dev@lists.ozlabs.org [-- Attachment #1: Type: text/plain, Size: 4415 bytes --] 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 #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Increasing build coverage for drivers/spi/spi-ppc4xx.c 2024-02-27 14:00 ` Uwe Kleine-König @ 2024-02-27 14:38 ` Christophe Leroy 2024-02-28 5:37 ` Michael Ellerman 0 siblings, 1 reply; 9+ messages in thread From: Christophe Leroy @ 2024-02-27 14:38 UTC (permalink / raw) To: Uwe Kleine-König Cc: Nicholas Piggin, linux-spi@vger.kernel.org, Aneesh Kumar K.V, Mark Brown, Naveen N. Rao, linuxppc-dev@lists.ozlabs.org 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: Increasing build coverage for drivers/spi/spi-ppc4xx.c 2024-02-27 14:38 ` Christophe Leroy @ 2024-02-28 5:37 ` Michael Ellerman 0 siblings, 0 replies; 9+ messages in thread From: Michael Ellerman @ 2024-02-28 5:37 UTC (permalink / raw) To: Christophe Leroy, Uwe Kleine-König Cc: Nicholas Piggin, linux-spi@vger.kernel.org, Aneesh Kumar K.V, Mark Brown, Naveen N. Rao, linuxppc-dev@lists.ozlabs.org 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
end of thread, other threads:[~2024-02-28 5:38 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-02-27 8:46 Increasing build coverage for drivers/spi/spi-ppc4xx.c Uwe Kleine-König 2024-02-27 8:54 ` Tudor Ambarus 2024-02-27 9:05 ` Uwe Kleine-König 2024-02-27 10:25 ` Christophe Leroy 2024-02-27 10:58 ` Uwe Kleine-König 2024-02-27 13:52 ` Christophe Leroy 2024-02-27 14:00 ` Uwe Kleine-König 2024-02-27 14:38 ` Christophe Leroy 2024-02-28 5:37 ` Michael Ellerman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).