[PATCH 0/4] spi: lantiq: Synchronize interrupts, transfers and add new features

[PATCH 0/4] spi: lantiq: Synchronize interrupts, transfers and add new features

[Dilip Kota]

This patch series add changes to synchronize the SPI interrupt handlers
and transfer operations. Intorduces the configurations to support
SPI controller on multiple SoCs.
Adds support to Lightning Mountain SoC.

Dilip Kota (4):
  spi: lantiq: Synchronize interrupt handlers and transfers
  spi: lantiq: Dynamic configuration of interrupts and FIFO size
  dt-bindings: spi: Add support to Lightning Mountain SoC
  spi: lantiq: Add support to Lightning Mountain SoC

 .../devicetree/bindings/spi/spi-lantiq-ssc.txt     |  21 ++-
 drivers/spi/Kconfig                                |   4 +-
 drivers/spi/spi-lantiq-ssc.c                       | 194 ++++++++++++---------
 3 files changed, 137 insertions(+), 82 deletions(-)

-- 
2.11.0

[PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

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

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Dilip Kota]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

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?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Dilip Kota]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 627 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Dilip Kota]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Hauke Mehrtens]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Dilip Kota]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Daniel Schwierzeck]

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

-- 
- Daniel

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Hauke Mehrtens]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Dilip Kota]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Dilip Kota]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1168 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Dilip Kota]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 508 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Dilip Kota]

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

Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers

[Dilip Kota]

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

[PATCH 2/4] spi: lantiq: Dynamic configuration of interrupts and FIFO size

[Dilip Kota]

Newer chipsets are having larger transfer FIFOs and lesser interrupt lines.
So, add the interrupt configuration operation and FIFO size mask to the SoC
specific data structure.

On the existing chipsets, interrupt controller acknowledges the triggered
interrupts automatically. Whereas in the newer chipsests like Lightning
Mountain interrupt status register should be cleared for the acknowledgment
of triggered interrupts. So, add a flag in the SoC specific data structure
to specify driver to clear the interrupt status register.

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
 drivers/spi/spi-lantiq-ssc.c | 97 ++++++++++++++++++++++++++------------------
 1 file changed, 57 insertions(+), 40 deletions(-)

diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c
index b67f5925bcb0..74a5d36c5048 100644
--- a/drivers/spi/spi-lantiq-ssc.c
+++ b/drivers/spi/spi-lantiq-ssc.c
@@ -61,9 +61,7 @@
 #define LTQ_SPI_CLC_DISR	BIT(0)	/* Disable request bit */
 
 #define LTQ_SPI_ID_TXFS_S	24	/* Implemented TX FIFO size */
-#define LTQ_SPI_ID_TXFS_M	(0x3F << LTQ_SPI_ID_TXFS_S)
 #define LTQ_SPI_ID_RXFS_S	16	/* Implemented RX FIFO size */
-#define LTQ_SPI_ID_RXFS_M	(0x3F << LTQ_SPI_ID_RXFS_S)
 #define LTQ_SPI_ID_MOD_S	8	/* Module ID */
 #define LTQ_SPI_ID_MOD_M	(0xff << LTQ_SPI_ID_MOD_S)
 #define LTQ_SPI_ID_CFG_S	5	/* DMA interface support */
@@ -126,19 +124,15 @@
 					 LTQ_SPI_WHBSTATE_CLRTUE)
 
 #define LTQ_SPI_RXFCON_RXFITL_S	8	/* FIFO interrupt trigger level */
-#define LTQ_SPI_RXFCON_RXFITL_M	(0x3F << LTQ_SPI_RXFCON_RXFITL_S)
 #define LTQ_SPI_RXFCON_RXFLU	BIT(1)	/* FIFO flush */
 #define LTQ_SPI_RXFCON_RXFEN	BIT(0)	/* FIFO enable */
 
 #define LTQ_SPI_TXFCON_TXFITL_S	8	/* FIFO interrupt trigger level */
-#define LTQ_SPI_TXFCON_TXFITL_M	(0x3F << LTQ_SPI_TXFCON_TXFITL_S)
 #define LTQ_SPI_TXFCON_TXFLU	BIT(1)	/* FIFO flush */
 #define LTQ_SPI_TXFCON_TXFEN	BIT(0)	/* FIFO enable */
 
 #define LTQ_SPI_FSTAT_RXFFL_S	0
-#define LTQ_SPI_FSTAT_RXFFL_M	(0x3f << LTQ_SPI_FSTAT_RXFFL_S)
 #define LTQ_SPI_FSTAT_TXFFL_S	8
-#define LTQ_SPI_FSTAT_TXFFL_M	(0x3f << LTQ_SPI_FSTAT_TXFFL_S)
 
 #define LTQ_SPI_GPOCON_ISCSBN_S	8
 #define LTQ_SPI_GPOCON_INVOUTN_S	0
@@ -158,9 +152,14 @@
 #define LTQ_SPI_IRNEN_T_XRX	BIT(0)	/* Receive end interrupt request */
 #define LTQ_SPI_IRNEN_ALL	0x1F
 
+struct lantiq_ssc_spi;
+
 struct lantiq_ssc_hwcfg {
+	int (*cfg_irq)(struct platform_device *pdev, struct lantiq_ssc_spi *spi);
+	u32 fifo_size_mask;
 	unsigned int irnen_r;
 	unsigned int irnen_t;
+	bool irq_ack;
 };
 
 struct lantiq_ssc_spi {
@@ -209,16 +208,18 @@ static void lantiq_ssc_maskl(const struct lantiq_ssc_spi *spi, u32 clr,
 
 static unsigned int tx_fifo_level(const struct lantiq_ssc_spi *spi)
 {
+	const struct lantiq_ssc_hwcfg *hwcfg = spi->hwcfg;
 	u32 fstat = lantiq_ssc_readl(spi, LTQ_SPI_FSTAT);
 
-	return (fstat & LTQ_SPI_FSTAT_TXFFL_M) >> LTQ_SPI_FSTAT_TXFFL_S;
+	return (fstat >> LTQ_SPI_FSTAT_TXFFL_S) & hwcfg->fifo_size_mask;
 }
 
 static unsigned int rx_fifo_level(const struct lantiq_ssc_spi *spi)
 {
+	const struct lantiq_ssc_hwcfg *hwcfg = spi->hwcfg;
 	u32 fstat = lantiq_ssc_readl(spi, LTQ_SPI_FSTAT);
 
-	return fstat & LTQ_SPI_FSTAT_RXFFL_M;
+	return (fstat >> LTQ_SPI_FSTAT_RXFFL_S) & hwcfg->fifo_size_mask;
 }
 
 static unsigned int tx_fifo_free(const struct lantiq_ssc_spi *spi)
@@ -678,6 +679,8 @@ static irqreturn_t lantiq_ssc_isr(int irq, void *data)
 	mutex_lock(&spi->lock);
 	val = lantiq_ssc_readl(spi, LTQ_SPI_IRNCR);
 	lantiq_ssc_maskl(spi, val, 0, LTQ_SPI_IRNEN);
+	if (hwcfg->irq_ack)
+		lantiq_ssc_writel(spi, val, LTQ_SPI_IRNCR);
 
 	if (val & LTQ_SPI_IRNEN_E)
 		ret = lantiq_ssc_err_interrupt(spi);
@@ -786,14 +789,52 @@ static int lantiq_ssc_transfer_one(struct spi_master *master,
 	return transfer_start(spi, spidev, t);
 }
 
+static int lantiq_cfg_irq(struct platform_device *pdev, struct lantiq_ssc_spi *spi)
+{
+	int irq, err;
+
+	irq = platform_get_irq_byname(pdev, LTQ_SPI_RX_IRQ_NAME);
+	if (irq < 0)
+		return irq;
+
+	err = devm_request_threaded_irq(&pdev->dev, irq, NULL, lantiq_ssc_isr,
+					IRQF_ONESHOT, LTQ_SPI_RX_IRQ_NAME, spi);
+	if (err)
+		return err;
+
+	irq = platform_get_irq_byname(pdev, LTQ_SPI_TX_IRQ_NAME);
+	if (irq < 0)
+		return irq;
+
+	err = devm_request_threaded_irq(&pdev->dev, irq, NULL, lantiq_ssc_isr,
+					IRQF_ONESHOT, LTQ_SPI_TX_IRQ_NAME, spi);
+	if (err)
+		return err;
+
+	irq = platform_get_irq_byname(pdev, LTQ_SPI_ERR_IRQ_NAME);
+	if (irq < 0)
+		return irq;
+
+	err = devm_request_threaded_irq(&pdev->dev, irq, NULL, lantiq_ssc_isr,
+					IRQF_ONESHOT, LTQ_SPI_ERR_IRQ_NAME, spi);
+
+	return err;
+}
+
 static const struct lantiq_ssc_hwcfg lantiq_ssc_xway = {
-	.irnen_r = LTQ_SPI_IRNEN_R_XWAY,
-	.irnen_t = LTQ_SPI_IRNEN_T_XWAY,
+	.irnen_r	= LTQ_SPI_IRNEN_R_XWAY,
+	.irnen_t	= LTQ_SPI_IRNEN_T_XWAY,
+	.fifo_size_mask	= GENMASK(5, 0),
+	.cfg_irq	= lantiq_cfg_irq,
+	.irq_ack	= false,
 };
 
 static const struct lantiq_ssc_hwcfg lantiq_ssc_xrx = {
-	.irnen_r = LTQ_SPI_IRNEN_R_XRX,
-	.irnen_t = LTQ_SPI_IRNEN_T_XRX,
+	.irnen_r	= LTQ_SPI_IRNEN_R_XRX,
+	.irnen_t	= LTQ_SPI_IRNEN_T_XRX,
+	.fifo_size_mask	= GENMASK(5, 0),
+	.cfg_irq	= lantiq_cfg_irq,
+	.irq_ack	= false,
 };
 
 static const struct of_device_id lantiq_ssc_match[] = {
@@ -811,9 +852,9 @@ static int lantiq_ssc_probe(struct platform_device *pdev)
 	struct lantiq_ssc_spi *spi;
 	const struct lantiq_ssc_hwcfg *hwcfg;
 	const struct of_device_id *match;
-	int err, rx_irq, tx_irq, err_irq;
 	u32 id, supports_dma, revision;
 	unsigned int num_cs;
+	int err;
 
 	match = of_match_device(lantiq_ssc_match, dev);
 	if (!match) {
@@ -821,19 +862,6 @@ static int lantiq_ssc_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 	hwcfg = match->data;
-
-	rx_irq = platform_get_irq_byname(pdev, LTQ_SPI_RX_IRQ_NAME);
-	if (rx_irq < 0)
-		return -ENXIO;
-
-	tx_irq = platform_get_irq_byname(pdev, LTQ_SPI_TX_IRQ_NAME);
-	if (tx_irq < 0)
-		return -ENXIO;
-
-	err_irq = platform_get_irq_byname(pdev, LTQ_SPI_ERR_IRQ_NAME);
-	if (err_irq < 0)
-		return -ENXIO;
-
 	master = spi_alloc_master(dev, sizeof(struct lantiq_ssc_spi));
 	if (!master)
 		return -ENOMEM;
@@ -849,18 +877,7 @@ static int lantiq_ssc_probe(struct platform_device *pdev)
 		goto err_master_put;
 	}
 
-	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_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_threaded_irq(dev, err_irq, NULL, lantiq_ssc_isr,
-					IRQF_ONESHOT, LTQ_SPI_ERR_IRQ_NAME, spi);
+	err = hwcfg->cfg_irq(pdev, spi);
 	if (err)
 		goto err_master_put;
 
@@ -911,8 +928,8 @@ static int lantiq_ssc_probe(struct platform_device *pdev)
 				     SPI_BPW_MASK(16) | SPI_BPW_MASK(32);
 
 	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;
+	spi->tx_fifo_size = (id >> LTQ_SPI_ID_TXFS_S) & hwcfg->fifo_size_mask;
+	spi->rx_fifo_size = (id >> LTQ_SPI_ID_RXFS_S) & hwcfg->fifo_size_mask;
 	supports_dma = (id & LTQ_SPI_ID_CFG_M) >> LTQ_SPI_ID_CFG_S;
 	revision = id & LTQ_SPI_ID_REV_M;
 
-- 
2.11.0

[PATCH 3/4] dt-bindings: spi: Add support to Lightning Mountain SoC

[Dilip Kota]

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

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
 .../devicetree/bindings/spi/spi-lantiq-ssc.txt      | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/spi/spi-lantiq-ssc.txt b/Documentation/devicetree/bindings/spi/spi-lantiq-ssc.txt
index ce3230c8e28d..76a3dd35f796 100644
--- a/Documentation/devicetree/bindings/spi/spi-lantiq-ssc.txt
+++ b/Documentation/devicetree/bindings/spi/spi-lantiq-ssc.txt
@@ -1,11 +1,17 @@
 Lantiq Synchronous Serial Controller (SSC) SPI master driver
 
 Required properties:
-- compatible: "lantiq,ase-spi", "lantiq,falcon-spi", "lantiq,xrx100-spi"
+- compatible: "lantiq,ase-spi", "lantiq,falcon-spi", "lantiq,xrx100-spi",
+  "intel,lgm-spi"
 - #address-cells: see spi-bus.txt
 - #size-cells: see spi-bus.txt
 - reg: address and length of the spi master registers
-- interrupts: should contain the "spi_rx", "spi_tx" and "spi_err" interrupt.
+- interrupts:
+  For compatible "intel,lgm-ssc" - the common interrupt number for
+  all of tx rx & err interrupts.
+       or
+  For rest of the compatibles, should contain the "spi_rx", "spi_tx" and
+  "spi_err" interrupt.
 
 
 Optional properties:
@@ -27,3 +33,14 @@ spi: spi@e100800 {
 	num-cs = <6>;
 	base-cs = <1>;
 };
+
+ssc0: spi@e0800000 {
+	compatible = "intel,lgm-spi";
+	reg = <0xe0800000 0x400>;
+	interrupt-parent = <&ioapic1>;
+	interrupts = <35 1>;
+	#address-cells = <1>;
+	#size-cells = <0>;
+	clocks = <&cgu0 LGM_CLK_NGI>, <&cgu0 LGM_GCLK_SSC0>;
+	clock-names = "freq", "gate";
+};
-- 
2.11.0

Re: [PATCH 3/4] dt-bindings: spi: Add support to Lightning Mountain SoC

[Rob Herring]

On Fri, 24 Apr 2020 18:42:32 +0800, Dilip Kota wrote:
> Add support to SPI controller on Intel Atom based Lightning Mountain
> SoC which reuses the Lantiq SPI controller IP.
> 
> Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
> ---
>  .../devicetree/bindings/spi/spi-lantiq-ssc.txt      | 21 +++++++++++++++++++--
>  1 file changed, 19 insertions(+), 2 deletions(-)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 4/4] spi: lantiq: Add support to Lightning Mountain SoC

[Dilip Kota]

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

Signed-off-by: Dilip Kota <eswara.kota@linux.intel.com>
---
 drivers/spi/Kconfig          |  4 ++--
 drivers/spi/spi-lantiq-ssc.c | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 4887be7f16de..089b0ba4afe3 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -495,11 +495,11 @@ config SPI_NPCM_PSPI
 
 config SPI_LANTIQ_SSC
 	tristate "Lantiq SSC SPI controller"
-	depends on LANTIQ || COMPILE_TEST
+	depends on (LANTIQ || X86) || COMPILE_TEST
 	help
 	  This driver supports the Lantiq SSC SPI controller in master
 	  mode. This controller is found on Intel (former Lantiq) SoCs like
-	  the Danube, Falcon, xRX200, xRX300.
+	  the Danube, Falcon, xRX200, xRX300, Lightning Mountain.
 
 config SPI_OC_TINY
 	tristate "OpenCores tiny SPI"
diff --git a/drivers/spi/spi-lantiq-ssc.c b/drivers/spi/spi-lantiq-ssc.c
index 74a5d36c5048..f07fa6fbadbe 100644
--- a/drivers/spi/spi-lantiq-ssc.c
+++ b/drivers/spi/spi-lantiq-ssc.c
@@ -789,6 +789,18 @@ static int lantiq_ssc_transfer_one(struct spi_master *master,
 	return transfer_start(spi, spidev, t);
 }
 
+static int intel_cfg_irq(struct platform_device *pdev, struct lantiq_ssc_spi *spi)
+{
+	int irq;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	return devm_request_threaded_irq(&pdev->dev, irq, NULL, lantiq_ssc_isr,
+					IRQF_ONESHOT, "spi", spi);
+}
+
 static int lantiq_cfg_irq(struct platform_device *pdev, struct lantiq_ssc_spi *spi)
 {
 	int irq, err;
@@ -837,10 +849,18 @@ static const struct lantiq_ssc_hwcfg lantiq_ssc_xrx = {
 	.irq_ack	= false,
 };
 
+static const struct lantiq_ssc_hwcfg intel_ssc_lgm = {
+	.irnen_r	= LTQ_SPI_IRNEN_R_XRX,
+	.irnen_t	= LTQ_SPI_IRNEN_T_XRX,
+	.fifo_size_mask	= GENMASK(7, 0),
+	.cfg_irq	= intel_cfg_irq,
+};
+
 static const struct of_device_id lantiq_ssc_match[] = {
 	{ .compatible = "lantiq,ase-spi", .data = &lantiq_ssc_xway, },
 	{ .compatible = "lantiq,falcon-spi", .data = &lantiq_ssc_xrx, },
 	{ .compatible = "lantiq,xrx100-spi", .data = &lantiq_ssc_xrx, },
+	{ .compatible = "intel,lgm-spi", .data = &intel_ssc_lgm, },
 	{},
 };
 MODULE_DEVICE_TABLE(of, lantiq_ssc_match);
-- 
2.11.0