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(-)
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);
--
2.11.0
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 <[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
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 <[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.
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 <[email protected]>
> ---
> 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);
>
>
--
- Daniel
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>> ---
>> 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