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

[Christophe Leroy <christophe.leroy@xxxxxxxxxx>]

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
>