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

From: Mark Brown <broonie@kernel.org>
To: Florian Fainelli <f.fainelli@gmail.com>
Cc: linux-kernel@vger.kernel.org, Rob Herring <robh+dt@kernel.org>,
	Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
	Ray Jui <rjui@broadcom.com>,
	Scott Branden <sbranden@broadcom.com>,
	"maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE..." 
	<bcm-kernel-feedback-list@broadcom.com>,
	"open list:SPI SUBSYSTEM" <linux-spi@vger.kernel.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
	<linux-rpi-kernel@lists.infradead.org>,
	"moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>,
	Martin Sperl <kernel@martin.sperl.org>,
	lukas@wunner.de
Subject: Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
Date: Thu, 4 Jun 2020 13:32:20 +0100	[thread overview]
Message-ID: <20200604123220.GD6644@sirena.org.uk> (raw)
In-Reply-To: <20200604034655.15930-4-f.fainelli@gmail.com>

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

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'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.

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]