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