| Message ID | 20200604034655.15930-3-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | spi: bcm2835: Enable shared interrupt support | expand |
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 >
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.
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.
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).
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.
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
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
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.
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>;
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(-)