lkml.org 
[lkml]   [2020]   [Jun]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
From
Date


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

\
 
 \ /
  Last update: 2020-06-04 18:07    [W:2.053 / U:0.012 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site