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

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

[Florian Fainelli]

Hi Mark, Lukas,

This patch series is implementing the approach that was discussed in
response to this previous submission:

https://lore.kernel.org/linux-arm-kernel/20200528185805.28991-1-nsaenzjulienne@suse.de/

It aims to have dedicated interrupt handlers for 2835 versus 2711/7211
so as to minimize the overhead for 2835.

Florian Fainelli (3):
  dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible
  ARM: dts: bcm2711: Update SPI nodes compatible strings
  spi: bcm2835: Enable shared interrupt support

 .../bindings/spi/brcm,bcm2835-spi.txt         |  3 +-
 arch/arm/boot/dts/bcm2711.dtsi                |  8 ++--
 drivers/spi/spi-bcm2835.c                     | 48 +++++++++++++++----
 3 files changed, 44 insertions(+), 15 deletions(-)

-- 
2.17.1

[PATCH 1/3] dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible

[Florian Fainelli]

The BCM2711 and BCM7211 chips use the BCM2835 SPI controller, but there
are severl instances of those in the system and they all share the same
interrupt line. Document specific compatible strings such that the
driver can take appropriate actions.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt b/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt
index f11f295c8450..3d55dd64b1be 100644
--- a/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt
+++ b/Documentation/devicetree/bindings/spi/brcm,bcm2835-spi.txt
@@ -5,7 +5,8 @@ SPI0, and the other known as the "Universal SPI Master"; part of the
 auxiliary block. This binding applies to the SPI0 controller.
 
 Required properties:
-- compatible: Should be "brcm,bcm2835-spi".
+- compatible: Should be one of "brcm,bcm2835-spi" for BCM2835/2836/2837 or
+  "brcm,bcm2711-spi" for BCM2711 or "brcm,bcm7211-spi" for BCM7211.
 - reg: Should contain register location and length.
 - interrupts: Should contain interrupt.
 - clocks: The clock feeding the SPI controller.
-- 
2.17.1

Re: [PATCH 1/3] dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible

[Lukas Wunner]

On Wed, Jun 03, 2020 at 08:46:53PM -0700, Florian Fainelli wrote:
> The BCM2711 and BCM7211 chips use the BCM2835 SPI controller, but there
> are severl instances of those in the system and they all share the same
      ^^^^^^
Nit: "several"

And apparently they do not *all* share the interrupt, but only
4 controllers (out of 5, not counting the two bcm2835aux ones)
do so.

Thanks,

Lukas

[PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings

[Florian Fainelli]

The BCM2711 SoC features 5 SPI controllers which all share the same
interrupt line, the SPI driver needs to support interrupt sharing,
therefore use the chip specific compatible string to help with that.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 arch/arm/boot/dts/bcm2711.dtsi | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
index a91cf68e3c4c..9a9ea67fbc2d 100644
--- a/arch/arm/boot/dts/bcm2711.dtsi
+++ b/arch/arm/boot/dts/bcm2711.dtsi
@@ -152,7 +152,7 @@
 		};
 
 		spi3: spi@7e204600 {
-			compatible = "brcm,bcm2835-spi";
+			compatible = "brcm,bcm2711-spi", "brcm,bcm2835-spi";
 			reg = <0x7e204600 0x0200>;
 			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&clocks BCM2835_CLOCK_VPU>;
@@ -162,7 +162,7 @@
 		};
 
 		spi4: spi@7e204800 {
-			compatible = "brcm,bcm2835-spi";
+			compatible = "brcm,bcm2711-spi", "brcm,bcm2835-spi";
 			reg = <0x7e204800 0x0200>;
 			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&clocks BCM2835_CLOCK_VPU>;
@@ -172,7 +172,7 @@
 		};
 
 		spi5: spi@7e204a00 {
-			compatible = "brcm,bcm2835-spi";
+			compatible = "brcm,bcm2711-spi", "brcm,bcm2835-spi";
 			reg = <0x7e204a00 0x0200>;
 			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&clocks BCM2835_CLOCK_VPU>;
@@ -182,7 +182,7 @@
 		};
 
 		spi6: spi@7e204c00 {
-			compatible = "brcm,bcm2835-spi";
+			compatible = "brcm,bcm2711-spi", "brcm,bcm2835-spi";
 			reg = <0x7e204c00 0x0200>;
 			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&clocks BCM2835_CLOCK_VPU>;
-- 
2.17.1

Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings

[Lukas Wunner]

On Wed, Jun 03, 2020 at 08:46:54PM -0700, Florian Fainelli wrote:
> The BCM2711 SoC features 5 SPI controllers which all share the same
> interrupt line, the SPI driver needs to support interrupt sharing,
> therefore use the chip specific compatible string to help with that.

You're saying above that the 5 controllers all share the interrupt
but below you're only changing the compatible string of 4 controllers.

So I assume spi0 still has its own interrupt and only the additional
4 controllers present on the BCM2711/BCM7211 share their interrupt?

Thanks,

Lukas

> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  arch/arm/boot/dts/bcm2711.dtsi | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/bcm2711.dtsi b/arch/arm/boot/dts/bcm2711.dtsi
> index a91cf68e3c4c..9a9ea67fbc2d 100644
> --- a/arch/arm/boot/dts/bcm2711.dtsi
> +++ b/arch/arm/boot/dts/bcm2711.dtsi
> @@ -152,7 +152,7 @@
>  		};
>  
>  		spi3: spi@7e204600 {
> -			compatible = "brcm,bcm2835-spi";
> +			compatible = "brcm,bcm2711-spi", "brcm,bcm2835-spi";
>  			reg = <0x7e204600 0x0200>;
>  			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&clocks BCM2835_CLOCK_VPU>;
> @@ -162,7 +162,7 @@
>  		};
>  
>  		spi4: spi@7e204800 {
> -			compatible = "brcm,bcm2835-spi";
> +			compatible = "brcm,bcm2711-spi", "brcm,bcm2835-spi";
>  			reg = <0x7e204800 0x0200>;
>  			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&clocks BCM2835_CLOCK_VPU>;
> @@ -172,7 +172,7 @@
>  		};
>  
>  		spi5: spi@7e204a00 {
> -			compatible = "brcm,bcm2835-spi";
> +			compatible = "brcm,bcm2711-spi", "brcm,bcm2835-spi";
>  			reg = <0x7e204a00 0x0200>;
>  			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&clocks BCM2835_CLOCK_VPU>;
> @@ -182,7 +182,7 @@
>  		};
>  
>  		spi6: spi@7e204c00 {
> -			compatible = "brcm,bcm2835-spi";
> +			compatible = "brcm,bcm2711-spi", "brcm,bcm2835-spi";
>  			reg = <0x7e204c00 0x0200>;
>  			interrupts = <GIC_SPI 118 IRQ_TYPE_LEVEL_HIGH>;
>  			clocks = <&clocks BCM2835_CLOCK_VPU>;
> -- 
> 2.17.1
> 

Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings

[Mark Brown]

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

On Thu, Jun 04, 2020 at 06:20:38AM +0200, Lukas Wunner wrote:
> On Wed, Jun 03, 2020 at 08:46:54PM -0700, Florian Fainelli wrote:
> > The BCM2711 SoC features 5 SPI controllers which all share the same
> > interrupt line, the SPI driver needs to support interrupt sharing,
> > therefore use the chip specific compatible string to help with that.

> You're saying above that the 5 controllers all share the interrupt
> but below you're only changing the compatible string of 4 controllers.

> So I assume spi0 still has its own interrupt and only the additional
> 4 controllers present on the BCM2711/BCM7211 share their interrupt?

Regardless of what's going on with the interrupts the compatible string
should reflect the IP version so unless for some reason someone taped
out two different versions of the IP it seems odd that the compatible
strings would vary within a given SoC.

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

Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings

[Lukas Wunner]

On Thu, Jun 04, 2020 at 12:13:25PM +0100, Mark Brown wrote:
> On Thu, Jun 04, 2020 at 06:20:38AM +0200, Lukas Wunner wrote:
> > On Wed, Jun 03, 2020 at 08:46:54PM -0700, Florian Fainelli wrote:
> > > The BCM2711 SoC features 5 SPI controllers which all share the same
> > > interrupt line, the SPI driver needs to support interrupt sharing,
> > > therefore use the chip specific compatible string to help with that.
> 
> > You're saying above that the 5 controllers all share the interrupt
> > but below you're only changing the compatible string of 4 controllers.
> 
> > So I assume spi0 still has its own interrupt and only the additional
> > 4 controllers present on the BCM2711/BCM7211 share their interrupt?
> 
> Regardless of what's going on with the interrupts the compatible string
> should reflect the IP version so unless for some reason someone taped
> out two different versions of the IP it seems odd that the compatible
> strings would vary within a given SoC.

Hm.  I guess it may be possible to search the DT for other devices
sharing the same interrupt line and thereby determine whether
IRQF_SHARED is necessary.  The helper to perform this search could
live in drivers/of/irq.c as I imagine it might be useful in general.

Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings

[Mark Brown]

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

On Thu, Jun 04, 2020 at 01:21:12PM +0200, Lukas Wunner wrote:
> On Thu, Jun 04, 2020 at 12:13:25PM +0100, Mark Brown wrote:

> > Regardless of what's going on with the interrupts the compatible string
> > should reflect the IP version so unless for some reason someone taped
> > out two different versions of the IP it seems odd that the compatible
> > strings would vary within a given SoC.

> Hm.  I guess it may be possible to search the DT for other devices
> sharing the same interrupt line and thereby determine whether
> IRQF_SHARED is necessary.  The helper to perform this search could
> live in drivers/of/irq.c as I imagine it might be useful in general.

That's another option, yeah - it'd be DT specific but it seems neater
than a property and much more tractable than trying to dance around
doing this in genirq (where we'd end up with callbacks when the second
device registers or something else horrible).

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

Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings

[Florian Fainelli]

On 6/3/2020 9:20 PM, Lukas Wunner wrote:
> On Wed, Jun 03, 2020 at 08:46:54PM -0700, Florian Fainelli wrote:
>> The BCM2711 SoC features 5 SPI controllers which all share the same
>> interrupt line, the SPI driver needs to support interrupt sharing,
>> therefore use the chip specific compatible string to help with that.
> 
> You're saying above that the 5 controllers all share the interrupt
> but below you're only changing the compatible string of 4 controllers.
> 
> So I assume spi0 still has its own interrupt and only the additional
> 4 controllers present on the BCM2711/BCM7211 share their interrupt?

Correct, there are 5 instances, but only the 4 that were added for 2711
actually share the interrupt line, I will correct that in the next patch
version.
-- 
Florian

Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings

[Stefan Wahren]

Am 04.06.20 um 18:40 schrieb Florian Fainelli:
>
> On 6/3/2020 9:20 PM, Lukas Wunner wrote:
>> On Wed, Jun 03, 2020 at 08:46:54PM -0700, Florian Fainelli wrote:
>>> The BCM2711 SoC features 5 SPI controllers which all share the same
>>> interrupt line, the SPI driver needs to support interrupt sharing,
>>> therefore use the chip specific compatible string to help with that.
>> You're saying above that the 5 controllers all share the interrupt
>> but below you're only changing the compatible string of 4 controllers.
>>
>> So I assume spi0 still has its own interrupt and only the additional
>> 4 controllers present on the BCM2711/BCM7211 share their interrupt?
> Correct, there are 5 instances, but only the 4 that were added for 2711
> actually share the interrupt line, I will correct that in the next patch
> version.

No, all 5 instances uses the same interrupt line. Please see my comment
before.

Regards

Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings

[Florian Fainelli]

On 6/4/2020 9:54 AM, Stefan Wahren wrote:
> Am 04.06.20 um 18:40 schrieb Florian Fainelli:
>>
>> On 6/3/2020 9:20 PM, Lukas Wunner wrote:
>>> On Wed, Jun 03, 2020 at 08:46:54PM -0700, Florian Fainelli wrote:
>>>> The BCM2711 SoC features 5 SPI controllers which all share the same
>>>> interrupt line, the SPI driver needs to support interrupt sharing,
>>>> therefore use the chip specific compatible string to help with that.
>>> You're saying above that the 5 controllers all share the interrupt
>>> but below you're only changing the compatible string of 4 controllers.
>>>
>>> So I assume spi0 still has its own interrupt and only the additional
>>> 4 controllers present on the BCM2711/BCM7211 share their interrupt?
>> Correct, there are 5 instances, but only the 4 that were added for 2711
>> actually share the interrupt line, I will correct that in the next patch
>> version.
> 
> No, all 5 instances uses the same interrupt line. Please see my comment
> before.

OK, but this is not going to be needed, I have another solution that
does not involve device tree changes.
-- 
Florian

Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings

[Stefan Wahren]

Hi Florian,

Am 04.06.20 um 05:46 schrieb Florian Fainelli:
> The BCM2711 SoC features 5 SPI controllers which all share the same
> interrupt line, the SPI driver needs to support interrupt sharing,
> therefore use the chip specific compatible string to help with that.

the commit message is correct about 5 SPI controllers, but the patch
only changes 4 ones.

Please add the new compatibles also for &spi (included from
bcm283x.dtsi) below in this file, which also share interrupt 118.

Thanks

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

[Florian Fainelli]

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.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/spi/spi-bcm2835.c | 48 +++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/spi/spi-bcm2835.c b/drivers/spi/spi-bcm2835.c
index 237bd306c268..2e73ec70ee80 100644
--- a/drivers/spi/spi-bcm2835.c
+++ b/drivers/spi/spi-bcm2835.c
@@ -361,11 +361,10 @@ static void bcm2835_spi_reset_hw(struct spi_controller *ctlr)
 	bcm2835_wr(bs, BCM2835_SPI_DLEN, 0);
 }
 
-static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
+static inline irqreturn_t bcm2835_spi_interrupt_common(struct spi_controller *ctlr,
+						       u32 cs)
 {
-	struct spi_controller *ctlr = dev_id;
 	struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
-	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
 
 	/*
 	 * An interrupt is signaled either if DONE is set (TX FIFO empty)
@@ -394,6 +393,27 @@ static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static irqreturn_t bcm2835_spi_interrupt(int irq, void *dev_id)
+{
+	struct spi_controller *ctlr = dev_id;
+	struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
+	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
+
+	return bcm2835_spi_interrupt_common(ctlr, cs);
+}
+
+static irqreturn_t bcm2835_spi_sh_interrupt(int irq, void *dev_id)
+{
+	struct spi_controller *ctlr = dev_id;
+	struct bcm2835_spi *bs = spi_controller_get_devdata(ctlr);
+	u32 cs = bcm2835_rd(bs, BCM2835_SPI_CS);
+
+	if (!(cs & BCM2835_SPI_CS_INTR))
+		return IRQ_NONE;
+
+	return bcm2835_spi_interrupt_common(ctlr, cs);
+}
+
 static int bcm2835_spi_transfer_one_irq(struct spi_controller *ctlr,
 					struct spi_device *spi,
 					struct spi_transfer *tfr,
@@ -1287,12 +1307,26 @@ static int bcm2835_spi_setup(struct spi_device *spi)
 	return 0;
 }
 
+static const struct of_device_id bcm2835_spi_match[] = {
+	{ .compatible = "brcm,bcm2835-spi", .data = &bcm2835_spi_interrupt },
+	{ .compatible = "brcm,bcm2711-spi", .data = &bcm2835_spi_sh_interrupt },
+	{ .compatible = "brcm,bcm7211-spi", .data = &bcm2835_spi_sh_interrupt },
+	{}
+};
+MODULE_DEVICE_TABLE(of, bcm2835_spi_match);
+
 static int bcm2835_spi_probe(struct platform_device *pdev)
 {
+	irqreturn_t (*bcm2835_spi_isr_func)(int, void *);
 	struct spi_controller *ctlr;
+	unsigned long flags = 0;
 	struct bcm2835_spi *bs;
 	int err;
 
+	bcm2835_spi_isr_func = of_device_get_match_data(&pdev->dev);
+	if (bcm2835_spi_isr_func == &bcm2835_spi_sh_interrupt)
+		flags = IRQF_SHARED;
+
 	ctlr = spi_alloc_master(&pdev->dev, ALIGN(sizeof(*bs),
 						  dma_get_cache_alignment()));
 	if (!ctlr)
@@ -1344,7 +1378,7 @@ static int bcm2835_spi_probe(struct platform_device *pdev)
 	bcm2835_wr(bs, BCM2835_SPI_CS,
 		   BCM2835_SPI_CS_CLEAR_RX | BCM2835_SPI_CS_CLEAR_TX);
 
-	err = devm_request_irq(&pdev->dev, bs->irq, bcm2835_spi_interrupt, 0,
+	err = devm_request_irq(&pdev->dev, bs->irq, bcm2835_spi_isr_func, flags,
 			       dev_name(&pdev->dev), ctlr);
 	if (err) {
 		dev_err(&pdev->dev, "could not request IRQ: %d\n", err);
@@ -1400,12 +1434,6 @@ static void bcm2835_spi_shutdown(struct platform_device *pdev)
 		dev_err(&pdev->dev, "failed to shutdown\n");
 }
 
-static const struct of_device_id bcm2835_spi_match[] = {
-	{ .compatible = "brcm,bcm2835-spi", },
-	{}
-};
-MODULE_DEVICE_TABLE(of, bcm2835_spi_match);
-
 static struct platform_driver bcm2835_spi_driver = {
 	.driver		= {
 		.name		= DRV_NAME,
-- 
2.17.1

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

[Lukas Wunner]

On Wed, Jun 03, 2020 at 08:46:55PM -0700, Florian Fainelli wrote:
> +static const struct of_device_id bcm2835_spi_match[] = {
> +	{ .compatible = "brcm,bcm2835-spi", .data = &bcm2835_spi_interrupt },
> +	{ .compatible = "brcm,bcm2711-spi", .data = &bcm2835_spi_sh_interrupt },
> +	{ .compatible = "brcm,bcm7211-spi", .data = &bcm2835_spi_sh_interrupt },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, bcm2835_spi_match);

Maybe I'm missing something but I think you either have to reverse the
order of the entries in this array or change patch [2/3] to drop
"brcm,bcm2835-spi" from the compatible string:

__of_match_node() iterates over the entries in the array above and
calls __of_device_is_compatible() for each of them, which returns
success if the entry matches any of the device's compatible string.

Because "brcm,bcm2835-spi" is checked first and that string is
present on the controllers with shared interrupt, they're all
deemed not to use shared interrupts.

If you opt so fix this by dropping "brcm,bcm2835-spi" from the
device's compatible strings, then you have to move patch [2/3]
behind patch [3/3].


>  static int bcm2835_spi_probe(struct platform_device *pdev)
>  {
> +	irqreturn_t (*bcm2835_spi_isr_func)(int, void *);

A more succinct alternative is:

	irq_handler_t bcm2835_spi_isr_func;

Otherwise this patch LGTM.

Thanks,

Lukas

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

[Rob Herring]

On Thu, Jun 04, 2020 at 06:17:32AM +0200, Lukas Wunner wrote:
> On Wed, Jun 03, 2020 at 08:46:55PM -0700, Florian Fainelli wrote:
> > +static const struct of_device_id bcm2835_spi_match[] = {
> > +	{ .compatible = "brcm,bcm2835-spi", .data = &bcm2835_spi_interrupt },
> > +	{ .compatible = "brcm,bcm2711-spi", .data = &bcm2835_spi_sh_interrupt },
> > +	{ .compatible = "brcm,bcm7211-spi", .data = &bcm2835_spi_sh_interrupt },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, bcm2835_spi_match);
> 
> Maybe I'm missing something but I think you either have to reverse the
> order of the entries in this array or change patch [2/3] to drop
> "brcm,bcm2835-spi" from the compatible string:
> 
> __of_match_node() iterates over the entries in the array above and
> calls __of_device_is_compatible() for each of them, which returns
> success if the entry matches any of the device's compatible string.

The order here doesn't matter. I'm pretty sure we fixed this years ago 
to always match to the most specific compatible.

Rob

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

[Mark Brown]

[-- 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 --]

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

[Florian Fainelli]

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

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

[Florian Fainelli]

On 6/4/2020 9:05 AM, Florian Fainelli wrote:
> 
> 
> 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.
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

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

[Lukas Wunner]

On Thu, Jun 04, 2020 at 01:24:54PM -0700, Florian Fainelli wrote:
> 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.

In principle it would be possible to iterate over the entire DT using
for_each_of_allnodes() and call of_irq_parse_one() on each device_node
if it's enabled and not the one we're probing.  Then check if any of that
device_node's IRQs is identical to that of the device_node we're probing.
That would give you a generic method to test for sharedness of an
interrupt.

However the solution you've found is simpler and cheaper than such a
brute-force search, hence seems perfectly valid to me.


> I appreciate that
> Lukas has spent some tremendous amount of time working on this
> controller driver and he has a sensitivity for performance.

Thanks!  Indeed I think spi-bcm2835.c is by now among the best performing
and most featureful SPI drivers in the kernel.  I've recently had a
discussion on netdev with someone testing an SPI-attached Ethernet
chip on iMX6Q and on STM32MP1 and couldn't get it to work.
With a BCM2837 no problem at all:

https://lore.kernel.org/netdev/ac0f7227-a4ae-b6cd-36ec-3bcb02b1adbe@denx.de/

Lukas

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

[Mark Brown]

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

On Thu, Jun 04, 2020 at 09:05:46AM -0700, Florian Fainelli wrote:
> On 6/4/2020 5:32 AM, Mark Brown wrote:

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

The big concern here is trying to support things going forwards - if it
turns out that any quirks are required by this version of the IP then it
gets very confusing and hard to keep DTs stable if you've already
quirked something that clearly isn't the IP version with the compatible
string.  Conversely if we start putting flags into the binding for every
feature that might be changed in a given IP that gets complex as we
can't ever learn new things about an existing IP version without
updating all the DTs which is also bad.

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

Like you said in a followup patch that doesn't work as the first device
to probe will think the interrupt isn't shared.  You'd need a callback
to change to shared mode from genirq which feels...  inelegant.

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