linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] spi: bcm2835: Enable shared interrupt support
@ 2020-06-04  3:46 Florian Fainelli
  2020-06-04  3:46 ` [PATCH 1/3] dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible Florian Fainelli
                   ` (2 more replies)
  0 siblings, 3 replies; 20+ messages in thread
From: Florian Fainelli @ 2020-06-04  3:46 UTC (permalink / raw)
  To: linux-kernel
  Cc: Florian Fainelli, Mark Brown, Rob Herring,
	Nicolas Saenz Julienne, Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, lukas

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* [PATCH 1/3] dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible
  2020-06-04  3:46 [PATCH 0/3] spi: bcm2835: Enable shared interrupt support Florian Fainelli
@ 2020-06-04  3:46 ` Florian Fainelli
  2020-06-04  4:23   ` Lukas Wunner
  2020-06-04  3:46 ` [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings Florian Fainelli
  2020-06-04  3:46 ` [PATCH 3/3] spi: bcm2835: Enable shared interrupt support Florian Fainelli
  2 siblings, 1 reply; 20+ messages in thread
From: Florian Fainelli @ 2020-06-04  3:46 UTC (permalink / raw)
  To: linux-kernel
  Cc: Florian Fainelli, Mark Brown, Rob Herring,
	Nicolas Saenz Julienne, Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, lukas

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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
  2020-06-04  3:46 [PATCH 0/3] spi: bcm2835: Enable shared interrupt support Florian Fainelli
  2020-06-04  3:46 ` [PATCH 1/3] dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible Florian Fainelli
@ 2020-06-04  3:46 ` Florian Fainelli
  2020-06-04  4:20   ` Lukas Wunner
  2020-06-04 16:46   ` Stefan Wahren
  2020-06-04  3:46 ` [PATCH 3/3] spi: bcm2835: Enable shared interrupt support Florian Fainelli
  2 siblings, 2 replies; 20+ messages in thread
From: Florian Fainelli @ 2020-06-04  3:46 UTC (permalink / raw)
  To: linux-kernel
  Cc: Florian Fainelli, Mark Brown, Rob Herring,
	Nicolas Saenz Julienne, Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, lukas

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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
  2020-06-04  3:46 [PATCH 0/3] spi: bcm2835: Enable shared interrupt support Florian Fainelli
  2020-06-04  3:46 ` [PATCH 1/3] dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible Florian Fainelli
  2020-06-04  3:46 ` [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings Florian Fainelli
@ 2020-06-04  3:46 ` Florian Fainelli
  2020-06-04  4:17   ` Lukas Wunner
  2020-06-04 12:32   ` Mark Brown
  2 siblings, 2 replies; 20+ messages in thread
From: Florian Fainelli @ 2020-06-04  3:46 UTC (permalink / raw)
  To: linux-kernel
  Cc: Florian Fainelli, Mark Brown, Rob Herring,
	Nicolas Saenz Julienne, Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, lukas

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


^ permalink raw reply related	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
  2020-06-04  3:46 ` [PATCH 3/3] spi: bcm2835: Enable shared interrupt support Florian Fainelli
@ 2020-06-04  4:17   ` Lukas Wunner
  2020-06-15 17:25     ` Rob Herring
  2020-06-04 12:32   ` Mark Brown
  1 sibling, 1 reply; 20+ messages in thread
From: Lukas Wunner @ 2020-06-04  4:17 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-kernel, Mark Brown, Rob Herring, Nicolas Saenz Julienne,
	Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
  2020-06-04  3:46 ` [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings Florian Fainelli
@ 2020-06-04  4:20   ` Lukas Wunner
  2020-06-04 11:13     ` Mark Brown
  2020-06-04 16:40     ` Florian Fainelli
  2020-06-04 16:46   ` Stefan Wahren
  1 sibling, 2 replies; 20+ messages in thread
From: Lukas Wunner @ 2020-06-04  4:20 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-kernel, Mark Brown, Rob Herring, Nicolas Saenz Julienne,
	Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl

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
> 

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 1/3] dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible
  2020-06-04  3:46 ` [PATCH 1/3] dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible Florian Fainelli
@ 2020-06-04  4:23   ` Lukas Wunner
  0 siblings, 0 replies; 20+ messages in thread
From: Lukas Wunner @ 2020-06-04  4:23 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-kernel, Mark Brown, Rob Herring, Nicolas Saenz Julienne,
	Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
  2020-06-04  4:20   ` Lukas Wunner
@ 2020-06-04 11:13     ` Mark Brown
  2020-06-04 11:21       ` Lukas Wunner
  2020-06-04 16:40     ` Florian Fainelli
  1 sibling, 1 reply; 20+ messages in thread
From: Mark Brown @ 2020-06-04 11:13 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Florian Fainelli, linux-kernel, Rob Herring,
	Nicolas Saenz Julienne, Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
  2020-06-04 11:13     ` Mark Brown
@ 2020-06-04 11:21       ` Lukas Wunner
  2020-06-04 14:05         ` Mark Brown
  0 siblings, 1 reply; 20+ messages in thread
From: Lukas Wunner @ 2020-06-04 11:21 UTC (permalink / raw)
  To: Mark Brown
  Cc: Florian Fainelli, linux-kernel, Rob Herring,
	Nicolas Saenz Julienne, Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl

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.

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
  2020-06-04  3:46 ` [PATCH 3/3] spi: bcm2835: Enable shared interrupt support Florian Fainelli
  2020-06-04  4:17   ` Lukas Wunner
@ 2020-06-04 12:32   ` Mark Brown
  2020-06-04 16:05     ` Florian Fainelli
  1 sibling, 1 reply; 20+ messages in thread
From: Mark Brown @ 2020-06-04 12:32 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-kernel, Rob Herring, Nicolas Saenz Julienne, Ray Jui,
	Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, lukas

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
  2020-06-04 11:21       ` Lukas Wunner
@ 2020-06-04 14:05         ` Mark Brown
  0 siblings, 0 replies; 20+ messages in thread
From: Mark Brown @ 2020-06-04 14:05 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Florian Fainelli, linux-kernel, Rob Herring,
	Nicolas Saenz Julienne, Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
  2020-06-04 12:32   ` Mark Brown
@ 2020-06-04 16:05     ` Florian Fainelli
  2020-06-04 20:24       ` Florian Fainelli
  2020-06-05 10:28       ` Mark Brown
  0 siblings, 2 replies; 20+ messages in thread
From: Florian Fainelli @ 2020-06-04 16:05 UTC (permalink / raw)
  To: Mark Brown, Florian Fainelli
  Cc: linux-kernel, Rob Herring, Nicolas Saenz Julienne, Ray Jui,
	Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, lukas



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
  2020-06-04  4:20   ` Lukas Wunner
  2020-06-04 11:13     ` Mark Brown
@ 2020-06-04 16:40     ` Florian Fainelli
  2020-06-04 16:54       ` Stefan Wahren
  1 sibling, 1 reply; 20+ messages in thread
From: Florian Fainelli @ 2020-06-04 16:40 UTC (permalink / raw)
  To: Lukas Wunner, Florian Fainelli
  Cc: linux-kernel, Mark Brown, Rob Herring, Nicolas Saenz Julienne,
	Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
  2020-06-04  3:46 ` [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings Florian Fainelli
  2020-06-04  4:20   ` Lukas Wunner
@ 2020-06-04 16:46   ` Stefan Wahren
  1 sibling, 0 replies; 20+ messages in thread
From: Stefan Wahren @ 2020-06-04 16:46 UTC (permalink / raw)
  To: Florian Fainelli, linux-kernel
  Cc: moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Scott Branden, lukas, Ray Jui, Rob Herring,
	open list:SPI SUBSYSTEM, Mark Brown,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, Nicolas Saenz Julienne

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



^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
  2020-06-04 16:40     ` Florian Fainelli
@ 2020-06-04 16:54       ` Stefan Wahren
  2020-06-04 16:56         ` Florian Fainelli
  0 siblings, 1 reply; 20+ messages in thread
From: Stefan Wahren @ 2020-06-04 16:54 UTC (permalink / raw)
  To: Florian Fainelli, Lukas Wunner
  Cc: moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Scott Branden, Ray Jui, linux-kernel, Rob Herring,
	open list:SPI SUBSYSTEM, Mark Brown,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, Nicolas Saenz Julienne

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


^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings
  2020-06-04 16:54       ` Stefan Wahren
@ 2020-06-04 16:56         ` Florian Fainelli
  0 siblings, 0 replies; 20+ messages in thread
From: Florian Fainelli @ 2020-06-04 16:56 UTC (permalink / raw)
  To: Stefan Wahren, Lukas Wunner
  Cc: moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Scott Branden, Ray Jui, linux-kernel, Rob Herring,
	open list:SPI SUBSYSTEM, Mark Brown,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, Nicolas Saenz Julienne



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
  2020-06-04 16:05     ` Florian Fainelli
@ 2020-06-04 20:24       ` Florian Fainelli
  2020-06-05 11:35         ` Lukas Wunner
  2020-06-05 10:28       ` Mark Brown
  1 sibling, 1 reply; 20+ messages in thread
From: Florian Fainelli @ 2020-06-04 20:24 UTC (permalink / raw)
  To: Mark Brown
  Cc: linux-kernel, Rob Herring, Nicolas Saenz Julienne, Ray Jui,
	Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, lukas



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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
  2020-06-04 16:05     ` Florian Fainelli
  2020-06-04 20:24       ` Florian Fainelli
@ 2020-06-05 10:28       ` Mark Brown
  1 sibling, 0 replies; 20+ messages in thread
From: Mark Brown @ 2020-06-05 10:28 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: linux-kernel, Rob Herring, Nicolas Saenz Julienne, Ray Jui,
	Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl, lukas

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
  2020-06-04 20:24       ` Florian Fainelli
@ 2020-06-05 11:35         ` Lukas Wunner
  0 siblings, 0 replies; 20+ messages in thread
From: Lukas Wunner @ 2020-06-05 11:35 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Mark Brown, linux-kernel, Rob Herring, Nicolas Saenz Julienne,
	Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

* Re: [PATCH 3/3] spi: bcm2835: Enable shared interrupt support
  2020-06-04  4:17   ` Lukas Wunner
@ 2020-06-15 17:25     ` Rob Herring
  0 siblings, 0 replies; 20+ messages in thread
From: Rob Herring @ 2020-06-15 17:25 UTC (permalink / raw)
  To: Lukas Wunner
  Cc: Florian Fainelli, linux-kernel, Mark Brown,
	Nicolas Saenz Julienne, Ray Jui, Scott Branden,
	maintainer:BROADCOM BCM281XX/BCM11XXX/BCM216XX ARM ARCHITE...,
	open list:SPI SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	moderated list:BROADCOM BCM2711/BCM2835 ARM ARCHITECTURE,
	Martin Sperl

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

^ permalink raw reply	[flat|nested] 20+ messages in thread

end of thread, other threads:[~2020-06-15 17:26 UTC | newest]

Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-06-04  3:46 [PATCH 0/3] spi: bcm2835: Enable shared interrupt support Florian Fainelli
2020-06-04  3:46 ` [PATCH 1/3] dt-bindings: spi: Document bcm2711 and bcm7211 SPI compatible Florian Fainelli
2020-06-04  4:23   ` Lukas Wunner
2020-06-04  3:46 ` [PATCH 2/3] ARM: dts: bcm2711: Update SPI nodes compatible strings Florian Fainelli
2020-06-04  4:20   ` Lukas Wunner
2020-06-04 11:13     ` Mark Brown
2020-06-04 11:21       ` Lukas Wunner
2020-06-04 14:05         ` Mark Brown
2020-06-04 16:40     ` Florian Fainelli
2020-06-04 16:54       ` Stefan Wahren
2020-06-04 16:56         ` Florian Fainelli
2020-06-04 16:46   ` Stefan Wahren
2020-06-04  3:46 ` [PATCH 3/3] spi: bcm2835: Enable shared interrupt support Florian Fainelli
2020-06-04  4:17   ` Lukas Wunner
2020-06-15 17:25     ` Rob Herring
2020-06-04 12:32   ` Mark Brown
2020-06-04 16:05     ` Florian Fainelli
2020-06-04 20:24       ` Florian Fainelli
2020-06-05 11:35         ` Lukas Wunner
2020-06-05 10:28       ` Mark Brown

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).