Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
To:     Mark Brown <broonie@kernel.org>,
        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
References: <20200604034655.15930-1-f.fainelli@gmail.com>
 <20200604034655.15930-4-f.fainelli@gmail.com>
 <20200604123220.GD6644@sirena.org.uk>
From:   Florian Fainelli <f.fainelli@gmail.com>
Message-ID: <21772111-fa1f-7a50-aa92-e44b09cff4eb@gmail.com>
Date:   Thu, 4 Jun 2020 09:05:46 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101
 Firefox/68.0 Thunderbird/68.8.1
MIME-Version: 1.0
In-Reply-To: <20200604123220.GD6644@sirena.org.uk>
Content-Type: text/plain; charset=windows-1252
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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