| Message ID | 20200604034655.15930-4-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: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
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.
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.
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.
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.
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
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,
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(-)