Increasing build coverage for drivers/spi/spi-ppc4xx.c
Christophe Leroy
christophe.leroy at csgroup.eu
Wed Feb 28 01:38:58 AEDT 2024
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
More information about the Linuxppc-dev
mailing list