Re: [PATCH] spi: bcm2835: Enable shared interrupt support

[Florian Fainelli]

On 5/29/20 10:53 AM, Lukas Wunner wrote:
> On Fri, May 29, 2020 at 10:46:01AM -0700, Florian Fainelli wrote:
>> On 5/29/20 10:43 AM, Lukas Wunner wrote:
>>> On Thu, May 28, 2020 at 08:58:04PM +0200, Nicolas Saenz Julienne wrote:
>>>> --- a/drivers/spi/spi-bcm2835.c
>>>> +++ b/drivers/spi/spi-bcm2835.c
>>>> @@ -379,6 +379,10 @@ static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
>>>>  	if (bs->tx_len && cs & BCM2835_SPI_CS_DONE)
>>>>  		bcm2835_wr_fifo_blind(bs, BCM2835_SPI_FIFO_SIZE);
>>>>  
>>>> +	/* check if we got interrupt enabled */
>>>> +	if (!(bcm2835_rd(bs, BCM2835_SPI_CS) & BCM2835_SPI_CS_INTR))
>>>> +		return IRQ_NONE;
>>>> +
>>>>  	/* Read as many bytes as possible from FIFO */
>>>>  	bcm2835_rd_fifo(bs);
>>>>  	/* Write as many bytes as possible to FIFO */
> [...]
>>> Finally, it would be nice if the check would be optimized away when
>>> compiling for pre-RasPi4 products, maybe something like:
>>>
>>> +	if (IS_ENABLED(CONFIG_ARM_LPAE) && !(cs & BCM2835_SPI_CS_INTR))
>>> +		return IRQ_NONE;
>>
>> Rather than keying this off ARM_LPAE or any other option, this should be
>> keyed off a compatible string, that way we can even conditionally pass
>> IRQF_SHARED to the interrupt handler if we care so much about performance.
> 
> But a compatible string can't be checked at compile time, can it?

No, but you can have a different interrupt handler that it set at
runtime if you want to completely eliminate this comparison.

My point is that CONFIG_ARM_LPAE is just too brittle, there is nothing
that prevents you from using a non-LPAE kernel on the Pi 4, even PCIe
could be made to work if using super section mappings to map the PCIe
outbound space. Even on models with over 4GB of DRAM, if you are willing
to lose some of it, it can work.

> 
> The point is that at the least the Foundation compiles and ships a separate
> kernel for each of the three platforms BCM2835, BCM2837, BCM2711.  It's
> unnecessary to check whether an interrupt was actually raised if we *know*
> in advance that it's not shared (as is the case with kernels compiled for
> BCM2835 and BCM2837).

I am fine with any solution that does not involve keying off
CONFIG_ARM_LPAE to discriminate 2711 from any other chip.
-- 
Florian