Re: Increasing build coverage for drivers/spi/spi-ppc4xx.c

[Christophe Leroy <christophe.leroy@xxxxxxxxxx>]

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