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

[Florian Fainelli <f.fainelli@xxxxxxxxx>]

On 6/4/2020 9:05 AM, Florian Fainelli wrote:
> 
> 
> On 6/4/2020 5:32 AM, Mark Brown wrote:
>> On Wed, Jun 03, 2020 at 08:46:55PM -0700, Florian Fainelli wrote:
>>> The SPI controller found in the BCM2711 and BCM7211 SoCs is instantiated
>>> 5 times, with all instances sharing the same interrupt line. We
>>> specifically match the two compatible strings here to determine whether
>>> it is necessary to request the interrupt with the IRQF_SHARED flag and
>>> to use an appropriate interrupt handler capable of returning IRQ_NONE.
>>
>>> For the BCM2835 case which is deemed performance critical, there is no
>>> overhead since a dedicated handler that does not assume sharing is used.
>>
>> This feels hacky - it's essentially using the compatible string to set a
>> boolean flag which isn't really about the IP but rather the platform
>> integration.  It might cause problems if we do end up having to quirk
>> this version of the IP for some other reason.
> 
> I am not sure why it would be a problem, when you describe a piece of
> hardware with Device Tree, even with the IP block being strictly the
> same, its very integration into a new SoC (with details like shared
> interrupt lines) do warrant a different compatible string. Maybe this is
> more of a philosophical question.
> 
>> I'm also looking at the
>> code and wondering if the overhead of checking to see if the interrupt
>> is flagged is really that severe, it's just a check to see if a bit is
>> set in a register which we already read so should be a couple of
>> instructions (which disassembly seems to confirm).  It *is* overhead so
>> there's some value in it, I'm just surprised that it's such a hot path
>> especially with a reasonably deep FIFO like this device has.
> 
> If it was up to me, we would just add the check on BCM2835_SPI_CS_INTR
> not being set and return IRQ_NONE and be done with it. I appreciate that
> Lukas has spent some tremendous amount of time working on this
> controller driver and he has a sensitivity for performance.
> 
>>
>> I guess ideally genirq would provide a way to figure out if an interrupt
>> is actually shared in the present system, and better yet we'd have a way
>> for drivers to say they aren't using the interrupt ATM, but that might
>> be more effort than it's really worth.  If this is needed and there's no
>> better way of figuring out if the interrupt is really shared then I'd
>> suggest a boolean flag rather than a compatible string, it's still a
>> hack but it's less likely to store up trouble for the future.
> 
> Instead of counting the number of SPI devices we culd request the
> interrupt first with flags = IRQF_PROBE_SHARED, if this works, good we
> have a single SPI master enabled, if it returns -EBUSY, try again with
> flags = IRQF_SHARED and set-up the bcm2835_spi_sh_interrupt interrupt
> handler to manage the sharing.
> 
> This would not require DT changes, which is probably better anyway.
Unfortunately this does not work.. The first time we probe the driver we
need to set an interrupt handler that is capable of handling a shared
interrupt. When we probe for subsequent times, we can use the -EBUSY
return code to know that we are in a shared interrupt context, however,
it is too late to change the first controller interrupt handler.

So we do need to know for the first time we install the interrupt
handler whether we will be in a shared situation or not, I cannot think
of any solution other than counting the number of available DT nodes
with the "brcm,bcm2835-spi" compatible string.
-- 
Florian