spi: lantiq: Synchronize interrupts, transfers and add new features

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.

drivers/spi/spi-lantiq-ssc.c +2 -3

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

Quoted 7 lines · 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.
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.

drivers/spi/spi-lantiq-ssc.c static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) · line 653

Quoted 10 lines

drivers/spi/spi-lantiq-ssc.c +1 -5

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

Quoted 15 lines

drivers/spi/spi-lantiq-ssc.c +4 -4

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

drivers/spi/spi-lantiq-ssc.c static int lantiq_ssc_probe(struct platform_device *pdev) · line 863

Quoted 4 lines

drivers/spi/spi-lantiq-ssc.c +2 -2

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

Quoted 10 lines · Mark Brown

Quoted 7 lines · 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.
Fixes the wrongly populated interrupt register offsets too.

Quoted 4 lines

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.

Quoted 10 lines

drivers/spi/spi-lantiq-ssc.c static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) · line 653

Quoted 8 lines

drivers/spi/spi-lantiq-ssc.c +1 -5

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

Quoted 4 lines

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.

Quoted 13 lines · Dilip Kota

Quoted 8 lines · Mark Brown

Quoted 7 lines · 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.
Fixes the wrongly populated interrupt register offsets too.

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 10 lines

Quoted 10 lines

drivers/spi/spi-lantiq-ssc.c static irqreturn_t lantiq_ssc_xmit_interrupt(int irq, void *data) · line 653

Quoted 9 lines

drivers/spi/spi-lantiq-ssc.c +1 -5

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

Quoted 5 lines

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.

Quoted 11 lines

Quoted 4 lines

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?

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.

Quoted 3 lines · Dilip Kota

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?

Quoted 2 lines

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.

Quoted 4 lines · Mark Brown

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?

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

---
 drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 44 deletions(-)

Quoted 8 lines · 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.
Fixes the wrongly populated interrupt register offsets too.

Quoted 7 lines

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

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

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

---
 drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 44 deletions(-)

drivers/spi/spi-lantiq-ssc.c +1 -1

index 1fd7ee53d451..b67f5925bcb0 100644
@@ -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>

drivers/spi/spi-lantiq-ssc.c +42 -41

 #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);
 

Am 24.04.20 um 12:42 schrieb Dilip Kota:

Quoted 6 lines

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.

Quoted 35 lines

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

---
 drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++----------------------
 1 file changed, 45 insertions(+), 44 deletions(-)

drivers/spi/spi-lantiq-ssc.c +1 -3

index 1fd7ee53d451..b67f5925bcb0 100644
@@ -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

drivers/spi/spi-lantiq-ssc.c line 54

Quoted 2 lines

drivers/spi/spi-lantiq-ssc.c +2 -0

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

Quoted 3 lines · Daniel Schwierzeck

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?

Am 24.04.20 um 12:42 schrieb Dilip Kota:

Quoted 6 lines

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.

Quoted 35 lines

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

---
  drivers/spi/spi-lantiq-ssc.c | 89 ++++++++++++++++++++++----------------------
  1 file changed, 45 insertions(+), 44 deletions(-)

drivers/spi/spi-lantiq-ssc.c +1 -2

index 1fd7ee53d451..b67f5925bcb0 100644
@@ -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

drivers/spi/spi-lantiq-ssc.c line 54

Quoted 3 lines

drivers/spi/spi-lantiq-ssc.c +2 -1

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

Quoted 2 lines

Quoted 2 lines · Daniel Schwierzeck

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.

Quoted 4 lines

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.

Quoted 5 lines · Dilip Kota

Quoted 2 lines · Daniel Schwierzeck

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.

Quoted 3 lines

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.

Quoted 5 lines

Quoted 4 lines

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

Quoted 2 lines · Mark Brown

Quoted 1 line

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.

Quoted 6 lines · Dilip Kota

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.

Quoted 8 lines · Mark Brown

Quoted 6 lines · Dilip Kota

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

Add support to SPI controller on Intel Atom based Lightning Mountain SoC which reuses the Lantiq SPI controller IP.

Signed-off-by Dilip Kota <[email protected]>

---
 .../devicetree/bindings/spi/spi-lantiq-ssc.txt      | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)