| Message ID | 3bf88d24b9cad9f3df1da8ed65bf55c05693b0f2.1587702428.git.eswara.kota@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | spi: lantiq: Synchronize interrupts, transfers and add new features | expand |
On Fri, Apr 24, 2020 at 06:42:30PM +0800, Dilip Kota wrote: > Synchronize tx, rx and error interrupts by registering to the > same interrupt handler. Interrupt handler will recognize and process > the appropriate interrupt on the basis of interrupt status register. > Also, establish synchronization between the interrupt handler and > transfer operation by taking the locks and registering the interrupt > handler as thread IRQ which avoids the bottom half. > Fixes the wrongly populated interrupt register offsets too. This sounds like at least three different changes mixed together in one commit, it makes it quite hard to tell what's going on. If nothing else the conversion from a workqueue to threaded interrupts should probably be split out from merging the interrupts. > -static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) > +static irqreturn_t lantiq_ssc_err_interrupt(struct lantiq_ssc_spi *spi) > { > - struct lantiq_ssc_spi *spi = data; > u32 stat = lantiq_ssc_readl(spi, LTQ_SPI_STAT); > > - if (!(stat & LTQ_SPI_STAT_ERRORS)) > - return IRQ_NONE; > - Why drop this? > - err = devm_request_irq(dev, rx_irq, lantiq_ssc_xmit_interrupt, > - 0, LTQ_SPI_RX_IRQ_NAME, spi); > + err = devm_request_threaded_irq(dev, rx_irq, NULL, lantiq_ssc_isr, > + IRQF_ONESHOT, LTQ_SPI_RX_IRQ_NAME, spi); > if (err) > goto err_master_put; > > - err = devm_request_irq(dev, tx_irq, lantiq_ssc_xmit_interrupt, > - 0, LTQ_SPI_TX_IRQ_NAME, spi); > + err = devm_request_threaded_irq(dev, tx_irq, NULL, lantiq_ssc_isr, > + IRQF_ONESHOT, LTQ_SPI_TX_IRQ_NAME, spi); > if (err) > goto err_master_put; > > - err = devm_request_irq(dev, err_irq, lantiq_ssc_err_interrupt, > - 0, LTQ_SPI_ERR_IRQ_NAME, spi); > + err = devm_request_threaded_irq(dev, err_irq, NULL, lantiq_ssc_isr, > + IRQF_ONESHOT, LTQ_SPI_ERR_IRQ_NAME, spi); It's not clear to me that it's a benefit to combine all the interrupts unconditionally - obviously where they're shared we need to but could that be accomplished with IRQF_SHARED and even if it can't it seems like something conditional would be better.
On 4/24/2020 7:25 PM, Mark Brown wrote: > On Fri, Apr 24, 2020 at 06:42:30PM +0800, Dilip Kota wrote: > >> Synchronize tx, rx and error interrupts by registering to the >> same interrupt handler. Interrupt handler will recognize and process >> the appropriate interrupt on the basis of interrupt status register. >> Also, establish synchronization between the interrupt handler and >> transfer operation by taking the locks and registering the interrupt >> handler as thread IRQ which avoids the bottom half. >> Fixes the wrongly populated interrupt register offsets too. > This sounds like at least three different changes mixed together in one > commit, it makes it quite hard to tell what's going on. If nothing else > the conversion from a workqueue to threaded interrupts should probably > be split out from merging the interrupts. While preparing the patches, i got puzzled to go with separate patches (for threaded interrupts, unified interrupt handler and fixing the register offset) or as a single patch!!. Finally i choose to go with single patch, because establishing synchronization is the major reason for this change, for that reason threaded interrupts and unified interrupts changes are done. And the fixing offset is a single line change, so included in this patch itself. And, on a lighter note, the whole patch is coming under 45 lines of code changes. Please let me know your view. > >> -static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) >> +static irqreturn_t lantiq_ssc_err_interrupt(struct lantiq_ssc_spi *spi) >> { >> - struct lantiq_ssc_spi *spi = data; >> u32 stat = lantiq_ssc_readl(spi, LTQ_SPI_STAT); >> >> - if (!(stat & LTQ_SPI_STAT_ERRORS)) >> - return IRQ_NONE; >> - > Why drop this? lantiq_ssc_err_interrupt() getting called, only if LTQ_SPI_IRNEN_E is set in the interrupt status register. Once the 'LTQ_SPI_IRNEN_E' bit is set, there is no chance of all error bits being unset in the SPI_STAT register, so the 'if condition' will never be successful. Hence dropped it. > >> - err = devm_request_irq(dev, rx_irq, lantiq_ssc_xmit_interrupt, >> - 0, LTQ_SPI_RX_IRQ_NAME, spi); >> + err = devm_request_threaded_irq(dev, rx_irq, NULL, lantiq_ssc_isr, >> + IRQF_ONESHOT, LTQ_SPI_RX_IRQ_NAME, spi); >> if (err) >> goto err_master_put; >> >> - err = devm_request_irq(dev, tx_irq, lantiq_ssc_xmit_interrupt, >> - 0, LTQ_SPI_TX_IRQ_NAME, spi); >> + err = devm_request_threaded_irq(dev, tx_irq, NULL, lantiq_ssc_isr, >> + IRQF_ONESHOT, LTQ_SPI_TX_IRQ_NAME, spi); >> if (err) >> goto err_master_put; >> >> - err = devm_request_irq(dev, err_irq, lantiq_ssc_err_interrupt, >> - 0, LTQ_SPI_ERR_IRQ_NAME, spi); >> + err = devm_request_threaded_irq(dev, err_irq, NULL, lantiq_ssc_isr, >> + IRQF_ONESHOT, LTQ_SPI_ERR_IRQ_NAME, spi); > It's not clear to me that it's a benefit to combine all the interrupts > unconditionally - obviously where they're shared we need to but could > that be accomplished with IRQF_SHARED and even if it can't it seems like > something conditional would be better. Lets take a case where Tx/Rx transfer interrupt got triggered and followed by error interrupt(before finishing the tx/rx interrupt execution) which is very less likely to occur, unified interrupt handler establishes synchronization. Comparatively, unified interrupt handler is better for adding support to the latest SoCs on which SPI have single interrupt line for tx,rx and errors. On basis of these two points i felt to go with unified interrupt handler. Regards, Dilip
On Mon, Apr 27, 2020 at 02:01:29PM +0800, Dilip Kota wrote: > On 4/24/2020 7:25 PM, Mark Brown wrote: > > On Fri, Apr 24, 2020 at 06:42:30PM +0800, Dilip Kota wrote: > > > Synchronize tx, rx and error interrupts by registering to the > > > same interrupt handler. Interrupt handler will recognize and process > > > the appropriate interrupt on the basis of interrupt status register. > > > Also, establish synchronization between the interrupt handler and > > > transfer operation by taking the locks and registering the interrupt > > > handler as thread IRQ which avoids the bottom half. > > > Fixes the wrongly populated interrupt register offsets too. > > This sounds like at least three different changes mixed together in one > > commit, it makes it quite hard to tell what's going on. If nothing else > > the conversion from a workqueue to threaded interrupts should probably > > be split out from merging the interrupts. > While preparing the patches, i got puzzled to go with separate patches (for > threaded interrupts, unified interrupt handler and fixing the register > offset) or as a single patch!!. > Finally i choose to go with single patch, because establishing > synchronization is the major reason for this change, for that reason > threaded interrupts and unified interrupts changes are done. And the fixing > offset is a single line change, so included in this patch itself. And, on a > lighter note, the whole patch is coming under 45 lines of code changes. > Please let me know your view. The single line change to fix the offset sounds like an especially good candidate for splitting out as a separate patch. It's not really about the number of lines but rather complexity. > > > -static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) > > > +static irqreturn_t lantiq_ssc_err_interrupt(struct lantiq_ssc_spi *spi) > > > { > > > - struct lantiq_ssc_spi *spi = data; > > > u32 stat = lantiq_ssc_readl(spi, LTQ_SPI_STAT); > > > - if (!(stat & LTQ_SPI_STAT_ERRORS)) > > > - return IRQ_NONE; > > > - > > Why drop this? > lantiq_ssc_err_interrupt() getting called, only if LTQ_SPI_IRNEN_E is set in > the interrupt status register. > Once the 'LTQ_SPI_IRNEN_E' bit is set, there is no chance of all error bits > being unset in the SPI_STAT register, so the 'if condition' will never be > successful. Hence dropped it. So this is another separate change and TBH it doesn't seem like a huge win in that it's still potentially adding a bit of robustness. > > It's not clear to me that it's a benefit to combine all the interrupts > > unconditionally - obviously where they're shared we need to but could > > that be accomplished with IRQF_SHARED and even if it can't it seems like > > something conditional would be better. > Lets take a case where Tx/Rx transfer interrupt got triggered and followed > by error interrupt(before finishing the tx/rx interrupt execution) which is > very less likely to occur, unified interrupt handler establishes > synchronization. > Comparatively, unified interrupt handler is better for adding support to the > latest SoCs on which SPI have single interrupt line for tx,rx and errors. > On basis of these two points i felt to go with unified interrupt handler. Does the mutex not do this regardless of how the interrupt handlers are wired up?
On 4/24/20 12:42 PM, Dilip Kota wrote: > Synchronize tx, rx and error interrupts by registering to the > same interrupt handler. Interrupt handler will recognize and process > the appropriate interrupt on the basis of interrupt status register. > Also, establish synchronization between the interrupt handler and > transfer operation by taking the locks and registering the interrupt > handler as thread IRQ which avoids the bottom half. > Fixes the wrongly populated interrupt register offsets too. > > Fixes: 17f84b793c01 ("spi: lantiq-ssc: add support for Lantiq SSC SPI controller") > Fixes: ad2fca0721d1 ("spi: lantiq-ssc: add LTQ_ prefix to defines") > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++---------------------- > 1 file changed, 45 insertions(+), 44 deletions(-) > Hi, I tried this patch series on a TP-LINK TD-W8970 (VRX200 with SPI flash) and the SPI controller is failing like this: ----- [ 6.947194] printk: bootconsole [early0] disabled [ 6.964210] spi-lantiq-ssc 1e100800.spi: Lantiq SSC SPI controller (Rev 8, TXFS 8, RXFS 8, DMA 1) [ 7.175188] spi-nor spi0.4: SPI transfer timed out [ 7.178558] spi_master spi0: failed to transfer one message from queue [ 7.185120] spi-nor spi0.4: error -145 reading JEDEC ID [ 7.190378] spi-nor: probe of spi0.4 failed with error -2 [ 7.199729] libphy: Fixed MDIO Bus: probed ------ It already fails when applying this first patch only. Without this patch is works like this: ----- [ 6.939498] printk: bootconsole [early0] disabled [ 6.954016] spi-lantiq-ssc 1e100800.spi: Lantiq SSC SPI controller (Rev 8, TXFS 8, RXFS 8, DMA 1) [ 6.975465] spi-nor spi0.4: s25fl064k (8192 Kbytes) [ 6.979066] 4 fixed-partitions partitions found on MTD device spi0.4 [ 6.985338] Creating 4 MTD partitions on "spi0.4": [ 6.990127] 0x000000000000-0x000000020000 : "u-boot" [ 6.997422] 0x000000020000-0x0000007c0000 : "firmware" [ 7.212304] random: crng init done [ 8.796128] 2 tplink-fw partitions found on MTD device firmware [ 8.800674] 0x000000020000-0x00000027878f : "kernel" [ 8.807776] 0x000000278790-0x0000007c0000 : "rootfs" [ 8.813611] mtd: device 3 (rootfs) set to be root filesystem [ 8.818268] 1 squashfs-split partitions found on MTD device rootfs [ 8.824123] 0x000000590000-0x0000007c0000 : "rootfs_data" [ 8.831772] 0x0000007c0000-0x0000007d0000 : "config" [ 8.837785] 0x0000007d0000-0x000000800000 : "boardconfig" [ 8.848193] libphy: Fixed MDIO Bus: probed ------ This was done by applying your patches on top of kernel 5.4.35 and adding this: "spi: lantiq-ssc: Use devm_platform_ioremap_resource() in lantiq_ssc_probe()" in OpenWrt master. Hauke
On 4/27/2020 9:45 PM, Mark Brown wrote: > On Mon, Apr 27, 2020 at 02:01:29PM +0800, Dilip Kota wrote: >> On 4/24/2020 7:25 PM, Mark Brown wrote: >>> On Fri, Apr 24, 2020 at 06:42:30PM +0800, Dilip Kota wrote: >>>> Synchronize tx, rx and error interrupts by registering to the >>>> same interrupt handler. Interrupt handler will recognize and process >>>> the appropriate interrupt on the basis of interrupt status register. >>>> Also, establish synchronization between the interrupt handler and >>>> transfer operation by taking the locks and registering the interrupt >>>> handler as thread IRQ which avoids the bottom half. >>>> Fixes the wrongly populated interrupt register offsets too. >>> This sounds like at least three different changes mixed together in one >>> commit, it makes it quite hard to tell what's going on. If nothing else >>> the conversion from a workqueue to threaded interrupts should probably >>> be split out from merging the interrupts. >> While preparing the patches, i got puzzled to go with separate patches (for >> threaded interrupts, unified interrupt handler and fixing the register >> offset) or as a single patch!!. >> Finally i choose to go with single patch, because establishing >> synchronization is the major reason for this change, for that reason >> threaded interrupts and unified interrupts changes are done. And the fixing >> offset is a single line change, so included in this patch itself. And, on a >> lighter note, the whole patch is coming under 45 lines of code changes. >> Please let me know your view. > The single line change to fix the offset sounds like an especially good > candidate for splitting out as a separate patch. It's not really about > the number of lines but rather complexity. Sure, i will do as separate patch. > >>>> -static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) >>>> +static irqreturn_t lantiq_ssc_err_interrupt(struct lantiq_ssc_spi *spi) >>>> { >>>> - struct lantiq_ssc_spi *spi = data; >>>> u32 stat = lantiq_ssc_readl(spi, LTQ_SPI_STAT); >>>> - if (!(stat & LTQ_SPI_STAT_ERRORS)) >>>> - return IRQ_NONE; >>>> - >>> Why drop this? >> lantiq_ssc_err_interrupt() getting called, only if LTQ_SPI_IRNEN_E is set in >> the interrupt status register. >> Once the 'LTQ_SPI_IRNEN_E' bit is set, there is no chance of all error bits >> being unset in the SPI_STAT register, so the 'if condition' will never be >> successful. Hence dropped it. > So this is another separate change and TBH it doesn't seem like a huge > win in that it's still potentially adding a bit of robustness. > >>> It's not clear to me that it's a benefit to combine all the interrupts >>> unconditionally - obviously where they're shared we need to but could >>> that be accomplished with IRQF_SHARED and even if it can't it seems like >>> something conditional would be better. >> Lets take a case where Tx/Rx transfer interrupt got triggered and followed >> by error interrupt(before finishing the tx/rx interrupt execution) which is >> very less likely to occur, unified interrupt handler establishes >> synchronization. >> Comparatively, unified interrupt handler is better for adding support to the >> latest SoCs on which SPI have single interrupt line for tx,rx and errors. >> On basis of these two points i felt to go with unified interrupt handler. > Does the mutex not do this regardless of how the interrupt handlers are > wired up? Yes, taking mutex and defining in the single ISR will be better i feel while adding support for multiple SoCs with different number of interrupt lines. Do you suggest to use different ISRs for multiple interrupt lines and single ISR for single interrupt line? I see, this results in writing repetitive code lines. Does single ISR looks erroneous! Please let me know. Regards, Dilip
On 4/28/2020 5:52 AM, Hauke Mehrtens wrote: > On 4/24/20 12:42 PM, Dilip Kota wrote: >> Synchronize tx, rx and error interrupts by registering to the >> same interrupt handler. Interrupt handler will recognize and process >> the appropriate interrupt on the basis of interrupt status register. >> Also, establish synchronization between the interrupt handler and >> transfer operation by taking the locks and registering the interrupt >> handler as thread IRQ which avoids the bottom half. >> Fixes the wrongly populated interrupt register offsets too. >> >> Fixes: 17f84b793c01 ("spi: lantiq-ssc: add support for Lantiq SSC SPI controller") >> Fixes: ad2fca0721d1 ("spi: lantiq-ssc: add LTQ_ prefix to defines") >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++---------------------- >> 1 file changed, 45 insertions(+), 44 deletions(-) >> > Hi, > > I tried this patch series on a TP-LINK TD-W8970 (VRX200 with SPI flash) > and the SPI controller is failing like this: > ----- > [ 6.947194] printk: bootconsole [early0] disabled > [ 6.964210] spi-lantiq-ssc 1e100800.spi: Lantiq SSC SPI controller > (Rev 8, TXFS 8, RXFS 8, DMA 1) > [ 7.175188] spi-nor spi0.4: SPI transfer timed out > [ 7.178558] spi_master spi0: failed to transfer one message from queue > [ 7.185120] spi-nor spi0.4: error -145 reading JEDEC ID > [ 7.190378] spi-nor: probe of spi0.4 failed with error -2 > [ 7.199729] libphy: Fixed MDIO Bus: probed > ------ > It already fails when applying this first patch only. This change is working successfully on Lightning Mountain SoC.(along with other changes in the patch series, as this alone doesnt configure SPI controller on LGM). The major changes this patch does is: Finding out the appropriate interrupt by reading the LTQ_SPI_IRNCR register. So i think, this failure could be at interrupt handling. And offsets of LTQ_SPI_IRNICR and LTQ_SPI_IRNCR registers also corrected. When i added you in the internal review of this patch, i remember you are saying Interrupt controller on VR9/xrx200 is different and acknowledges the interrupts inside the IP automatically. Does this leads to clearing the interrupt registers LTQ_SPI_IRNCR and LTQ_SPI_IRNICR? If it is the case, the SPI driver cannot figure out the cause of the interrupt and result in timeout. Could you please print the LTQ_SPI_IRNCR and LTQ_SPI_IRNICR register values in the ISR and share the logs. Regards, Dilip > > > Without this patch is works like this: > ----- > [ 6.939498] printk: bootconsole [early0] disabled > [ 6.954016] spi-lantiq-ssc 1e100800.spi: Lantiq SSC SPI controller > (Rev 8, TXFS 8, RXFS 8, DMA 1) > [ 6.975465] spi-nor spi0.4: s25fl064k (8192 Kbytes) > [ 6.979066] 4 fixed-partitions partitions found on MTD device spi0.4 > [ 6.985338] Creating 4 MTD partitions on "spi0.4": > [ 6.990127] 0x000000000000-0x000000020000 : "u-boot" > [ 6.997422] 0x000000020000-0x0000007c0000 : "firmware" > [ 7.212304] random: crng init done > [ 8.796128] 2 tplink-fw partitions found on MTD device firmware > [ 8.800674] 0x000000020000-0x00000027878f : "kernel" > [ 8.807776] 0x000000278790-0x0000007c0000 : "rootfs" > [ 8.813611] mtd: device 3 (rootfs) set to be root filesystem > [ 8.818268] 1 squashfs-split partitions found on MTD device rootfs > [ 8.824123] 0x000000590000-0x0000007c0000 : "rootfs_data" > [ 8.831772] 0x0000007c0000-0x0000007d0000 : "config" > [ 8.837785] 0x0000007d0000-0x000000800000 : "boardconfig" > [ 8.848193] libphy: Fixed MDIO Bus: probed > ------ > > This was done by applying your patches on top of kernel 5.4.35 and > adding this: "spi: lantiq-ssc: Use devm_platform_ioremap_resource() in > lantiq_ssc_probe()" in OpenWrt master. > > Hauke
On Tue, Apr 28, 2020 at 01:39:06PM +0800, Dilip Kota wrote: > Do you suggest to use different ISRs for multiple interrupt lines and single > ISR for single interrupt line? I see, this results in writing repetitive > code lines. It looks like the shared case is mainly a handler that calls the two other handlers? > Does single ISR looks erroneous! Please let me know. The change was not entirely clear, I was having trouble convincing myself that all the transformations were OK partly because I kept on finding little extra changes in there and partly because there were several things going on. In theory it could work.
Am 24.04.20 um 12:42 schrieb Dilip Kota: > Synchronize tx, rx and error interrupts by registering to the > same interrupt handler. Interrupt handler will recognize and process > the appropriate interrupt on the basis of interrupt status register. > Also, establish synchronization between the interrupt handler and > transfer operation by taking the locks and registering the interrupt > handler as thread IRQ which avoids the bottom half. actually there is no real bottom half. Reading or writing the FIFOs is fast and is therefore be done in hard IRQ context. But as the comment for lantiq_ssc_bussy_work() state, the driver needs some busy-waiting after the last interrupt. I don't think it's worth to replace this with threaded interrupts which add more runtime overhead and likely decrease the maximum transfer speed. > Fixes the wrongly populated interrupt register offsets too. > > Fixes: 17f84b793c01 ("spi: lantiq-ssc: add support for Lantiq SSC SPI controller") > Fixes: ad2fca0721d1 ("spi: lantiq-ssc: add LTQ_ prefix to defines") > Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> > --- > drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++---------------------- > 1 file changed, 45 insertions(+), 44 deletions(-) > > diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c > index 1fd7ee53d451..b67f5925bcb0 100644 > --- a/drivers/spi/spi-lantiq-ssc.c > +++ b/drivers/spi/spi-lantiq-ssc.c > @@ -6,6 +6,7 @@ > > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/mutex.h> > #include <linux/of_device.h> > #include <linux/clk.h> > #include <linux/io.h> > @@ -13,7 +14,6 @@ > #include <linux/interrupt.h> > #include <linux/sched.h> > #include <linux/completion.h> > -#include <linux/spinlock.h> > #include <linux/err.h> > #include <linux/gpio.h> > #include <linux/pm_runtime.h> > @@ -50,8 +50,8 @@ > #define LTQ_SPI_RXCNT 0x84 > #define LTQ_SPI_DMACON 0xec > #define LTQ_SPI_IRNEN 0xf4 > -#define LTQ_SPI_IRNICR 0xf8 > -#define LTQ_SPI_IRNCR 0xfc > +#define LTQ_SPI_IRNCR 0xf8 > +#define LTQ_SPI_IRNICR 0xfc the values are matching the datasheets for Danube and VRX200 family. AFAICS the registers have been swapped for some newer SoCs like GRX330 or GRX550. It didn't matter until now because those registers were unused by the driver. So if you want to use those registers, you have to deal somehow with the register offset swap in struct lantiq_ssc_hwcfg. > > #define LTQ_SPI_CLC_SMC_S 16 /* Clock divider for sleep mode */ > #define LTQ_SPI_CLC_SMC_M (0xFF << LTQ_SPI_CLC_SMC_S) > @@ -171,9 +171,7 @@ struct lantiq_ssc_spi { > struct clk *fpi_clk; > const struct lantiq_ssc_hwcfg *hwcfg; > > - spinlock_t lock; > - struct workqueue_struct *wq; > - struct work_struct work; > + struct mutex lock; > > const u8 *tx; > u8 *rx; > @@ -186,6 +184,8 @@ struct lantiq_ssc_spi { > unsigned int base_cs; > }; > > +static void lantiq_ssc_busy_wait(struct lantiq_ssc_spi *spi); > + > static u32 lantiq_ssc_readl(const struct lantiq_ssc_spi *spi, u32 reg) > { > return __raw_readl(spi->regbase + reg); > @@ -464,8 +464,6 @@ static int lantiq_ssc_unprepare_message(struct spi_master *master, > { > struct lantiq_ssc_spi *spi = spi_master_get_devdata(master); > > - flush_workqueue(spi->wq); > - > /* Disable transmitter and receiver while idle */ > lantiq_ssc_maskl(spi, 0, LTQ_SPI_CON_TXOFF | LTQ_SPI_CON_RXOFF, > LTQ_SPI_CON); > @@ -610,10 +608,8 @@ static void rx_request(struct lantiq_ssc_spi *spi) > lantiq_ssc_writel(spi, rxreq, LTQ_SPI_RXREQ); > } > > -static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) > +static irqreturn_t lantiq_ssc_xmit_interrupt(struct lantiq_ssc_spi *spi) > { > - struct lantiq_ssc_spi *spi = data; > - > if (spi->tx) { > if (spi->rx && spi->rx_todo) > rx_fifo_read_full_duplex(spi); > @@ -638,19 +634,15 @@ static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) > return IRQ_HANDLED; > > completed: > - queue_work(spi->wq, &spi->work); > + lantiq_ssc_busy_wait(spi); > > return IRQ_HANDLED; > } > > -static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) > +static irqreturn_t lantiq_ssc_err_interrupt(struct lantiq_ssc_spi *spi) > { > - struct lantiq_ssc_spi *spi = data; > u32 stat = lantiq_ssc_readl(spi, LTQ_SPI_STAT); > > - if (!(stat & LTQ_SPI_STAT_ERRORS)) > - return IRQ_NONE; > - > if (stat & LTQ_SPI_STAT_RUE) > dev_err(spi->dev, "receive underflow error\n"); > if (stat & LTQ_SPI_STAT_TUE) > @@ -670,17 +662,39 @@ static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) > /* set bad status so it can be retried */ > if (spi->master->cur_msg) > spi->master->cur_msg->status = -EIO; > - queue_work(spi->wq, &spi->work); > + > + spi_finalize_current_transfer(spi->master); > > return IRQ_HANDLED; > } > > +static irqreturn_t lantiq_ssc_isr(int irq, void *data) > +{ > + struct lantiq_ssc_spi *spi = data; > + const struct lantiq_ssc_hwcfg *hwcfg = spi->hwcfg; > + irqreturn_t ret = IRQ_NONE; > + u32 val; > + > + mutex_lock(&spi->lock); > + val = lantiq_ssc_readl(spi, LTQ_SPI_IRNCR); > + lantiq_ssc_maskl(spi, val, 0, LTQ_SPI_IRNEN); > + > + if (val & LTQ_SPI_IRNEN_E) > + ret = lantiq_ssc_err_interrupt(spi); > + > + if ((val & hwcfg->irnen_t) || (val & hwcfg->irnen_r)) > + ret = lantiq_ssc_xmit_interrupt(spi); > + > + lantiq_ssc_maskl(spi, 0, val, LTQ_SPI_IRNEN); > + mutex_unlock(&spi->lock); > + > + return ret; > +} > + > static int transfer_start(struct lantiq_ssc_spi *spi, struct spi_device *spidev, > struct spi_transfer *t) > { > - unsigned long flags; > - > - spin_lock_irqsave(&spi->lock, flags); > + mutex_lock(&spi->lock); > > spi->tx = t->tx_buf; > spi->rx = t->rx_buf; > @@ -700,7 +714,7 @@ static int transfer_start(struct lantiq_ssc_spi *spi, struct spi_device *spidev, > rx_request(spi); > } > > - spin_unlock_irqrestore(&spi->lock, flags); > + mutex_unlock(&spi->lock); > > return t->len; > } > @@ -712,14 +726,11 @@ static int transfer_start(struct lantiq_ssc_spi *spi, struct spi_device *spidev, > * write the last word to the wire, not when it is finished. Do busy > * waiting till it finishes. > */ > -static void lantiq_ssc_bussy_work(struct work_struct *work) > +static void lantiq_ssc_busy_wait(struct lantiq_ssc_spi *spi) > { > - struct lantiq_ssc_spi *spi; > unsigned long long timeout = 8LL * 1000LL; > unsigned long end; > > - spi = container_of(work, typeof(*spi), work); > - > do_div(timeout, spi->speed_hz); > timeout += timeout + 100; /* some tolerance */ > > @@ -838,18 +849,18 @@ static int lantiq_ssc_probe(struct platform_device *pdev) > goto err_master_put; > } > > - err = devm_request_irq(dev, rx_irq, lantiq_ssc_xmit_interrupt, > - 0, LTQ_SPI_RX_IRQ_NAME, spi); > + err = devm_request_threaded_irq(dev, rx_irq, NULL, lantiq_ssc_isr, > + IRQF_ONESHOT, LTQ_SPI_RX_IRQ_NAME, spi); > if (err) > goto err_master_put; > > - err = devm_request_irq(dev, tx_irq, lantiq_ssc_xmit_interrupt, > - 0, LTQ_SPI_TX_IRQ_NAME, spi); > + err = devm_request_threaded_irq(dev, tx_irq, NULL, lantiq_ssc_isr, > + IRQF_ONESHOT, LTQ_SPI_TX_IRQ_NAME, spi); > if (err) > goto err_master_put; > > - err = devm_request_irq(dev, err_irq, lantiq_ssc_err_interrupt, > - 0, LTQ_SPI_ERR_IRQ_NAME, spi); > + err = devm_request_threaded_irq(dev, err_irq, NULL, lantiq_ssc_isr, > + IRQF_ONESHOT, LTQ_SPI_ERR_IRQ_NAME, spi); > if (err) > goto err_master_put; > > @@ -882,7 +893,7 @@ static int lantiq_ssc_probe(struct platform_device *pdev) > spi->base_cs = 1; > of_property_read_u32(pdev->dev.of_node, "base-cs", &spi->base_cs); > > - spin_lock_init(&spi->lock); > + mutex_init(&spi->lock); > spi->bits_per_word = 8; > spi->speed_hz = 0; > > @@ -899,13 +910,6 @@ static int lantiq_ssc_probe(struct platform_device *pdev) > master->bits_per_word_mask = SPI_BPW_RANGE_MASK(2, 8) | > SPI_BPW_MASK(16) | SPI_BPW_MASK(32); > > - spi->wq = alloc_ordered_workqueue(dev_name(dev), 0); > - if (!spi->wq) { > - err = -ENOMEM; > - goto err_clk_put; > - } > - INIT_WORK(&spi->work, lantiq_ssc_bussy_work); > - > id = lantiq_ssc_readl(spi, LTQ_SPI_ID); > spi->tx_fifo_size = (id & LTQ_SPI_ID_TXFS_M) >> LTQ_SPI_ID_TXFS_S; > spi->rx_fifo_size = (id & LTQ_SPI_ID_RXFS_M) >> LTQ_SPI_ID_RXFS_S; > @@ -921,13 +925,11 @@ static int lantiq_ssc_probe(struct platform_device *pdev) > err = devm_spi_register_master(dev, master); > if (err) { > dev_err(dev, "failed to register spi_master\n"); > - goto err_wq_destroy; > + goto err_clk_put; > } > > return 0; > > -err_wq_destroy: > - destroy_workqueue(spi->wq); > err_clk_put: > clk_put(spi->fpi_clk); > err_clk_disable: > @@ -948,7 +950,6 @@ static int lantiq_ssc_remove(struct platform_device *pdev) > tx_fifo_flush(spi); > hw_enter_config_mode(spi); > > - destroy_workqueue(spi->wq); > clk_disable_unprepare(spi->spi_clk); > clk_put(spi->fpi_clk); > >
On 4/28/20 1:10 PM, Daniel Schwierzeck wrote: > > > Am 24.04.20 um 12:42 schrieb Dilip Kota: >> Synchronize tx, rx and error interrupts by registering to the >> same interrupt handler. Interrupt handler will recognize and process >> the appropriate interrupt on the basis of interrupt status register. >> Also, establish synchronization between the interrupt handler and >> transfer operation by taking the locks and registering the interrupt >> handler as thread IRQ which avoids the bottom half. > > actually there is no real bottom half. Reading or writing the FIFOs is > fast and is therefore be done in hard IRQ context. But as the comment > for lantiq_ssc_bussy_work() state, the driver needs some busy-waiting > after the last interrupt. I don't think it's worth to replace this with > threaded interrupts which add more runtime overhead and likely decrease > the maximum transfer speed. > >> Fixes the wrongly populated interrupt register offsets too. >> >> Fixes: 17f84b793c01 ("spi: lantiq-ssc: add support for Lantiq SSC SPI controller") >> Fixes: ad2fca0721d1 ("spi: lantiq-ssc: add LTQ_ prefix to defines") >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++---------------------- >> 1 file changed, 45 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c >> index 1fd7ee53d451..b67f5925bcb0 100644 >> --- a/drivers/spi/spi-lantiq-ssc.c >> +++ b/drivers/spi/spi-lantiq-ssc.c >> @@ -6,6 +6,7 @@ >> >> #include <linux/kernel.h> >> #include <linux/module.h> >> +#include <linux/mutex.h> >> #include <linux/of_device.h> >> #include <linux/clk.h> >> #include <linux/io.h> >> @@ -13,7 +14,6 @@ >> #include <linux/interrupt.h> >> #include <linux/sched.h> >> #include <linux/completion.h> >> -#include <linux/spinlock.h> >> #include <linux/err.h> >> #include <linux/gpio.h> >> #include <linux/pm_runtime.h> >> @@ -50,8 +50,8 @@ >> #define LTQ_SPI_RXCNT 0x84 >> #define LTQ_SPI_DMACON 0xec >> #define LTQ_SPI_IRNEN 0xf4 >> -#define LTQ_SPI_IRNICR 0xf8 >> -#define LTQ_SPI_IRNCR 0xfc >> +#define LTQ_SPI_IRNCR 0xf8 >> +#define LTQ_SPI_IRNICR 0xfc > > the values are matching the datasheets for Danube and VRX200 family. > AFAICS the registers have been swapped for some newer SoCs like GRX330 > or GRX550. It didn't matter until now because those registers were > unused by the driver. So if you want to use those registers, you have to > deal somehow with the register offset swap in struct lantiq_ssc_hwcfg. Hi, The Interrupt controller found on Danube till xrx300 which is probably from Infineon like this SPI controller IP acknowledges the interrupts also inside this SPI controller IP automatically, this has to be done manually on the xrx500 and probably also LGM as they use a different interrupt controller. I prepared patches for this internally 2.5 years ago but did not send them upstream because of internal processes. I would suggest to only do this ack on the newer platforms starting with the xrx500 and not on the older. On SMP systems a lock is needed in lantiq_ssc_xmit_interrupt() to protect against an other thread reading from the RX buffer or writing to the TX buffer in parallel. @Dilip. Did you try the patches I send you one months ago on the LGM? I would be helpful to split this patch into multiple like already suggest to make it easier to find the bugs. Hauke
On 4/28/2020 6:00 PM, Mark Brown wrote: > On Tue, Apr 28, 2020 at 01:39:06PM +0800, Dilip Kota wrote: > >> Do you suggest to use different ISRs for multiple interrupt lines and single >> ISR for single interrupt line? I see, this results in writing repetitive >> code lines. > It looks like the shared case is mainly a handler that calls the two > other handlers? Yes. > >> Does single ISR looks erroneous! Please let me know. > The change was not entirely clear, I was having trouble convincing > myself that all the transformations were OK partly because I kept on > finding little extra changes in there and partly because there were > several things going on. In theory it could work. You want me to split this in to multiple patches? Regards, Dilip
On 4/28/2020 7:10 PM, Daniel Schwierzeck wrote: > > Am 24.04.20 um 12:42 schrieb Dilip Kota: >> Synchronize tx, rx and error interrupts by registering to the >> same interrupt handler. Interrupt handler will recognize and process >> the appropriate interrupt on the basis of interrupt status register. >> Also, establish synchronization between the interrupt handler and >> transfer operation by taking the locks and registering the interrupt >> handler as thread IRQ which avoids the bottom half. > actually there is no real bottom half. Reading or writing the FIFOs is > fast and is therefore be done in hard IRQ context. But as the comment Doing FIFO r/w in threaded irqs shouldn't cause any impact on maximum transfer rate i think. Also the ISR should be quick enough, doing FIFO r/w in ISR adds up more latency to ISR. Handling the FIFOs r/w in threaded irq will be a better way. > for lantiq_ssc_bussy_work() state, the driver needs some busy-waiting > after the last interrupt. I don't think it's worth to replace this with > threaded interrupts which add more runtime overhead and likely decrease > the maximum transfer speed. Workqueue has a higher chances of causing SPI transfers timedout. > >> Fixes the wrongly populated interrupt register offsets too. >> >> Fixes: 17f84b793c01 ("spi: lantiq-ssc: add support for Lantiq SSC SPI controller") >> Fixes: ad2fca0721d1 ("spi: lantiq-ssc: add LTQ_ prefix to defines") >> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> >> --- >> drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++---------------------- >> 1 file changed, 45 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c >> index 1fd7ee53d451..b67f5925bcb0 100644 >> --- a/drivers/spi/spi-lantiq-ssc.c >> +++ b/drivers/spi/spi-lantiq-ssc.c >> @@ -6,6 +6,7 @@ >> >> #include <linux/kernel.h> >> #include <linux/module.h> >> +#include <linux/mutex.h> >> #include <linux/of_device.h> >> #include <linux/clk.h> >> #include <linux/io.h> >> @@ -13,7 +14,6 @@ >> #include <linux/interrupt.h> >> #include <linux/sched.h> >> #include <linux/completion.h> >> -#include <linux/spinlock.h> >> #include <linux/err.h> >> #include <linux/gpio.h> >> #include <linux/pm_runtime.h> >> @@ -50,8 +50,8 @@ >> #define LTQ_SPI_RXCNT 0x84 >> #define LTQ_SPI_DMACON 0xec >> #define LTQ_SPI_IRNEN 0xf4 >> -#define LTQ_SPI_IRNICR 0xf8 >> -#define LTQ_SPI_IRNCR 0xfc >> +#define LTQ_SPI_IRNCR 0xf8 >> +#define LTQ_SPI_IRNICR 0xfc > the values are matching the datasheets for Danube and VRX200 family. > AFAICS the registers have been swapped for some newer SoCs like GRX330 > or GRX550. It didn't matter until now because those registers were > unused by the driver. So if you want to use those registers, you have to > deal somehow with the register offset swap in struct lantiq_ssc_hwcfg. In the initial phase of the patch, i have written the code considering the interrupt offsets in latest chipsets are different from the older chipsets. But, when i refered the Hauke's changes to add support for xrx500(which he done internally), offsets are corrected . So i did the same. I will define these offsets in the SoC data structure which i have done initially. Regards, Dilip >> >>
On 4/28/2020 7:30 PM, Hauke Mehrtens wrote: > On 4/28/20 1:10 PM, Daniel Schwierzeck wrote: >> >> Am 24.04.20 um 12:42 schrieb Dilip Kota: >> ... > Hi, > > The Interrupt controller found on Danube till xrx300 which is probably > from Infineon like this SPI controller IP acknowledges the interrupts > also inside this SPI controller IP automatically, this has to be done > manually on the xrx500 and probably also LGM as they use a different > interrupt controller. I prepared patches for this internally 2.5 years > ago but did not send them upstream because of internal processes. > > I would suggest to only do this ack on the newer platforms starting with > the xrx500 and not on the older. > > On SMP systems a lock is needed in lantiq_ssc_xmit_interrupt() to > protect against an other thread reading from the RX buffer or writing to > the TX buffer in parallel. > > @Dilip. Did you try the patches I send you one months ago on the LGM? All the cases you mentioned are taken care in the patch, could you please have a look once. And the patch you shared internally, has done below change. By referring it i have updated the offsets, mentioning offsets are wrong. But actual case is vrx200 are having different offsets and xrx500, latest chipsets are having different offsets. I think this could be the reason for SPI transfer timeouts when you run test on vrx200 with my patches. -#define LTQ_SPI_IRNICR 0xf8 -#define LTQ_SPI_IRNCR 0xfc +#define LTQ_SPI_IRNCR 0xf8 +#define LTQ_SPI_IRNICR 0xfc These offsets need to be defined in SoC data structure as they are different across the chipsets(which i have done in initial phase of the patch which i submitted for internal review. I hope you had got a chance to review it). Regards, Dilip
On Wed, Apr 29, 2020 at 04:20:53PM +0800, Dilip Kota wrote: > On 4/28/2020 7:10 PM, Daniel Schwierzeck wrote: > > actually there is no real bottom half. Reading or writing the FIFOs is > > fast and is therefore be done in hard IRQ context. But as the comment > Doing FIFO r/w in threaded irqs shouldn't cause any impact on maximum > transfer rate i think. Have you actually tested this? Generally adding extra latency is going to lead to some opportunity for the hardware to idle and the longer the hardware is idle the lower the throughput. > Also the ISR should be quick enough, doing FIFO r/w in ISR adds up more > latency to ISR. > Handling the FIFOs r/w in threaded irq will be a better way. Consider what happens on a heavily loaded system - the threaded interrupt will have to be scheduled along with other tasks. > > for lantiq_ssc_bussy_work() state, the driver needs some busy-waiting > > after the last interrupt. I don't think it's worth to replace this with > > threaded interrupts which add more runtime overhead and likely decrease > > the maximum transfer speed. > Workqueue has a higher chances of causing SPI transfers timedout. because...?
On Wed, Apr 29, 2020 at 03:20:21PM +0800, Dilip Kota wrote: > On 4/28/2020 6:00 PM, Mark Brown wrote: > > The change was not entirely clear, I was having trouble convincing > > myself that all the transformations were OK partly because I kept on > > finding little extra changes in there and partly because there were > > several things going on. In theory it could work. > You want me to split this in to multiple patches? It needs to be clearer I think, splitting would probably help.
On 4/29/2020 8:13 PM, Mark Brown wrote: > On Wed, Apr 29, 2020 at 04:20:53PM +0800, Dilip Kota wrote: >> On 4/28/2020 7:10 PM, Daniel Schwierzeck wrote: >>> actually there is no real bottom half. Reading or writing the FIFOs is >>> fast and is therefore be done in hard IRQ context. But as the comment >> Doing FIFO r/w in threaded irqs shouldn't cause any impact on maximum >> transfer rate i think. > Have you actually tested this? Generally adding extra latency is going > to lead to some opportunity for the hardware to idle and the longer the > hardware is idle the lower the throughput. > >> Also the ISR should be quick enough, doing FIFO r/w in ISR adds up more >> latency to ISR. >> Handling the FIFOs r/w in threaded irq will be a better way. > Consider what happens on a heavily loaded system - the threaded > interrupt will have to be scheduled along with other tasks. > >>> for lantiq_ssc_bussy_work() state, the driver needs some busy-waiting >>> after the last interrupt. I don't think it's worth to replace this with >>> threaded interrupts which add more runtime overhead and likely decrease >>> the maximum transfer speed. >> Workqueue has a higher chances of causing SPI transfers timedout. > because...? I just tried to get the history of removing workqueue in SPI driver, on GRX500 (earlier chipset of LGM) the SPI transfers got timedout with workqueues during regression testing. Once changed to threaded IRQs transfers are working successfully. Regards, Dilip
On Mon, May 04, 2020 at 06:15:47PM +0800, Dilip Kota wrote: > On 4/29/2020 8:13 PM, Mark Brown wrote: > > > Workqueue has a higher chances of causing SPI transfers timedout. > > because...? > I just tried to get the history of removing workqueue in SPI driver, on > GRX500 (earlier chipset of LGM) the SPI transfers got timedout with > workqueues during regression testing. Once changed to threaded IRQs > transfers are working successfully. That doesn't really explain why though, it just explains what.
On 5/5/2020 7:23 PM, Mark Brown wrote: > On Mon, May 04, 2020 at 06:15:47PM +0800, Dilip Kota wrote: >> On 4/29/2020 8:13 PM, Mark Brown wrote: > >> I just tried to get the history of removing workqueue in SPI driver, on >> GRX500 (earlier chipset of LGM) the SPI transfers got timedout with >> workqueues during regression testing. Once changed to threaded IRQs >> transfers are working successfully. > That doesn't really explain why though, it just explains what. I didnt find more information about it. I will work to reproduce the issue and share the detailed information sooner i get the accessibility of the SoC (because of covid19 doing wfh) Regards, Dilip
On 5/6/2020 3:40 PM, Dilip Kota wrote: > > On 5/5/2020 7:23 PM, Mark Brown wrote: >> On Mon, May 04, 2020 at 06:15:47PM +0800, Dilip Kota wrote: >>> On 4/29/2020 8:13 PM, Mark Brown wrote: >> >>> I just tried to get the history of removing workqueue in SPI driver, on >>> GRX500 (earlier chipset of LGM) the SPI transfers got timedout with >>> workqueues during regression testing. Once changed to threaded IRQs >>> transfers are working successfully. >> That doesn't really explain why though, it just explains what. > I didnt find more information about it. I will work to reproduce the > issue and share the detailed information sooner i get the > accessibility of the SoC (because of covid19 doing wfh) I got the GRX500 setup and reproduced the timeout issue. [ 88.721883] spi-loopback-test spi1.2: SPI transfer timed out [ 88.726488] spi-loopback-test spi1.2: spi-message timed out - reruning... [ 88.961786] spi-loopback-test spi1.2: SPI transfer timed out [ 88.966027] spi-loopback-test spi1.2: Failed to execute spi_message: -145 Timeout is happening because of not acknowledging or not clearing the interrupt status registers. Workqueue is not causing any issue, I am working on the changes, will submit the patches for review. Regards, Dilip > > Regards, > Dilip
diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c index 1fd7ee53d451..b67f5925bcb0 100644 --- a/drivers/spi/spi-lantiq-ssc.c +++ b/drivers/spi/spi-lantiq-ssc.c @@ -6,6 +6,7 @@ #include <linux/kernel.h> #include <linux/module.h> +#include <linux/mutex.h> #include <linux/of_device.h> #include <linux/clk.h> #include <linux/io.h> @@ -13,7 +14,6 @@ #include <linux/interrupt.h> #include <linux/sched.h> #include <linux/completion.h> -#include <linux/spinlock.h> #include <linux/err.h> #include <linux/gpio.h> #include <linux/pm_runtime.h> @@ -50,8 +50,8 @@ #define LTQ_SPI_RXCNT 0x84 #define LTQ_SPI_DMACON 0xec #define LTQ_SPI_IRNEN 0xf4 -#define LTQ_SPI_IRNICR 0xf8 -#define LTQ_SPI_IRNCR 0xfc +#define LTQ_SPI_IRNCR 0xf8 +#define LTQ_SPI_IRNICR 0xfc #define LTQ_SPI_CLC_SMC_S 16 /* Clock divider for sleep mode */ #define LTQ_SPI_CLC_SMC_M (0xFF << LTQ_SPI_CLC_SMC_S) @@ -171,9 +171,7 @@ struct lantiq_ssc_spi { struct clk *fpi_clk; const struct lantiq_ssc_hwcfg *hwcfg; - spinlock_t lock; - struct workqueue_struct *wq; - struct work_struct work; + struct mutex lock; const u8 *tx; u8 *rx; @@ -186,6 +184,8 @@ struct lantiq_ssc_spi { unsigned int base_cs; }; +static void lantiq_ssc_busy_wait(struct lantiq_ssc_spi *spi); + static u32 lantiq_ssc_readl(const struct lantiq_ssc_spi *spi, u32 reg) { return __raw_readl(spi->regbase + reg); @@ -464,8 +464,6 @@ static int lantiq_ssc_unprepare_message(struct spi_master *master, { struct lantiq_ssc_spi *spi = spi_master_get_devdata(master); - flush_workqueue(spi->wq); - /* Disable transmitter and receiver while idle */ lantiq_ssc_maskl(spi, 0, LTQ_SPI_CON_TXOFF | LTQ_SPI_CON_RXOFF, LTQ_SPI_CON); @@ -610,10 +608,8 @@ static void rx_request(struct lantiq_ssc_spi *spi) lantiq_ssc_writel(spi, rxreq, LTQ_SPI_RXREQ); } -static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) +static irqreturn_t lantiq_ssc_xmit_interrupt(struct lantiq_ssc_spi *spi) { - struct lantiq_ssc_spi *spi = data; - if (spi->tx) { if (spi->rx && spi->rx_todo) rx_fifo_read_full_duplex(spi); @@ -638,19 +634,15 @@ static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) return IRQ_HANDLED; completed: - queue_work(spi->wq, &spi->work); + lantiq_ssc_busy_wait(spi); return IRQ_HANDLED; } -static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) +static irqreturn_t lantiq_ssc_err_interrupt(struct lantiq_ssc_spi *spi) { - struct lantiq_ssc_spi *spi = data; u32 stat = lantiq_ssc_readl(spi, LTQ_SPI_STAT); - if (!(stat & LTQ_SPI_STAT_ERRORS)) - return IRQ_NONE; - if (stat & LTQ_SPI_STAT_RUE) dev_err(spi->dev, "receive underflow error\n"); if (stat & LTQ_SPI_STAT_TUE) @@ -670,17 +662,39 @@ static irqreturn_t lantiq_ssc_err_interrupt(int irq, void *data) /* set bad status so it can be retried */ if (spi->master->cur_msg) spi->master->cur_msg->status = -EIO; - queue_work(spi->wq, &spi->work); + + spi_finalize_current_transfer(spi->master); return IRQ_HANDLED; } +static irqreturn_t lantiq_ssc_isr(int irq, void *data) +{ + struct lantiq_ssc_spi *spi = data; + const struct lantiq_ssc_hwcfg *hwcfg = spi->hwcfg; + irqreturn_t ret = IRQ_NONE; + u32 val; + + mutex_lock(&spi->lock); + val = lantiq_ssc_readl(spi, LTQ_SPI_IRNCR); + lantiq_ssc_maskl(spi, val, 0, LTQ_SPI_IRNEN); + + if (val & LTQ_SPI_IRNEN_E) + ret = lantiq_ssc_err_interrupt(spi); + + if ((val & hwcfg->irnen_t) || (val & hwcfg->irnen_r)) + ret = lantiq_ssc_xmit_interrupt(spi); + + lantiq_ssc_maskl(spi, 0, val, LTQ_SPI_IRNEN); + mutex_unlock(&spi->lock); + + return ret; +} + static int transfer_start(struct lantiq_ssc_spi *spi, struct spi_device *spidev, struct spi_transfer *t) { - unsigned long flags; - - spin_lock_irqsave(&spi->lock, flags); + mutex_lock(&spi->lock); spi->tx = t->tx_buf; spi->rx = t->rx_buf; @@ -700,7 +714,7 @@ static int transfer_start(struct lantiq_ssc_spi *spi, struct spi_device *spidev, rx_request(spi); } - spin_unlock_irqrestore(&spi->lock, flags); + mutex_unlock(&spi->lock); return t->len; } @@ -712,14 +726,11 @@ static int transfer_start(struct lantiq_ssc_spi *spi, struct spi_device *spidev, * write the last word to the wire, not when it is finished. Do busy * waiting till it finishes. */ -static void lantiq_ssc_bussy_work(struct work_struct *work) +static void lantiq_ssc_busy_wait(struct lantiq_ssc_spi *spi) { - struct lantiq_ssc_spi *spi; unsigned long long timeout = 8LL * 1000LL; unsigned long end; - spi = container_of(work, typeof(*spi), work); - do_div(timeout, spi->speed_hz); timeout += timeout + 100; /* some tolerance */ @@ -838,18 +849,18 @@ static int lantiq_ssc_probe(struct platform_device *pdev) goto err_master_put; } - err = devm_request_irq(dev, rx_irq, lantiq_ssc_xmit_interrupt, - 0, LTQ_SPI_RX_IRQ_NAME, spi); + err = devm_request_threaded_irq(dev, rx_irq, NULL, lantiq_ssc_isr, + IRQF_ONESHOT, LTQ_SPI_RX_IRQ_NAME, spi); if (err) goto err_master_put; - err = devm_request_irq(dev, tx_irq, lantiq_ssc_xmit_interrupt, - 0, LTQ_SPI_TX_IRQ_NAME, spi); + err = devm_request_threaded_irq(dev, tx_irq, NULL, lantiq_ssc_isr, + IRQF_ONESHOT, LTQ_SPI_TX_IRQ_NAME, spi); if (err) goto err_master_put; - err = devm_request_irq(dev, err_irq, lantiq_ssc_err_interrupt, - 0, LTQ_SPI_ERR_IRQ_NAME, spi); + err = devm_request_threaded_irq(dev, err_irq, NULL, lantiq_ssc_isr, + IRQF_ONESHOT, LTQ_SPI_ERR_IRQ_NAME, spi); if (err) goto err_master_put; @@ -882,7 +893,7 @@ static int lantiq_ssc_probe(struct platform_device *pdev) spi->base_cs = 1; of_property_read_u32(pdev->dev.of_node, "base-cs", &spi->base_cs); - spin_lock_init(&spi->lock); + mutex_init(&spi->lock); spi->bits_per_word = 8; spi->speed_hz = 0; @@ -899,13 +910,6 @@ static int lantiq_ssc_probe(struct platform_device *pdev) master->bits_per_word_mask = SPI_BPW_RANGE_MASK(2, 8) | SPI_BPW_MASK(16) | SPI_BPW_MASK(32); - spi->wq = alloc_ordered_workqueue(dev_name(dev), 0); - if (!spi->wq) { - err = -ENOMEM; - goto err_clk_put; - } - INIT_WORK(&spi->work, lantiq_ssc_bussy_work); - id = lantiq_ssc_readl(spi, LTQ_SPI_ID); spi->tx_fifo_size = (id & LTQ_SPI_ID_TXFS_M) >> LTQ_SPI_ID_TXFS_S; spi->rx_fifo_size = (id & LTQ_SPI_ID_RXFS_M) >> LTQ_SPI_ID_RXFS_S; @@ -921,13 +925,11 @@ static int lantiq_ssc_probe(struct platform_device *pdev) err = devm_spi_register_master(dev, master); if (err) { dev_err(dev, "failed to register spi_master\n"); - goto err_wq_destroy; + goto err_clk_put; } return 0; -err_wq_destroy: - destroy_workqueue(spi->wq); err_clk_put: clk_put(spi->fpi_clk); err_clk_disable: @@ -948,7 +950,6 @@ static int lantiq_ssc_remove(struct platform_device *pdev) tx_fifo_flush(spi); hw_enter_config_mode(spi); - destroy_workqueue(spi->wq); clk_disable_unprepare(spi->spi_clk); clk_put(spi->fpi_clk);
Synchronize tx, rx and error interrupts by registering to the same interrupt handler. Interrupt handler will recognize and process the appropriate interrupt on the basis of interrupt status register. Also, establish synchronization between the interrupt handler and transfer operation by taking the locks and registering the interrupt handler as thread IRQ which avoids the bottom half. Fixes the wrongly populated interrupt register offsets too. Fixes: 17f84b793c01 ("spi: lantiq-ssc: add support for Lantiq SSC SPI controller") Fixes: ad2fca0721d1 ("spi: lantiq-ssc: add LTQ_ prefix to defines") Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com> --- drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++---------------------- 1 file changed, 45 insertions(+), 44 deletions(-)