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

On 6/4/2020 9:05 AM, Florian Fainelli wrote:

On 6/4/2020 5:32 AM, Mark Brown wrote:

quoted

On Wed, Jun 03, 2020 at 08:46:55PM -0700, Florian Fainelli wrote:

quoted

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.

quoted

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.

quoted

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.

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel