Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
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
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
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
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. signature.asc Description: PGP signature
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
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
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. signature.asc Description: PGP signature
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
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...? signature.asc Description: PGP signature
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
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
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
---
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
#include
+#include
#include
#include
#include
@@ -13,7 +14,6 @@
#include
#include
#include
-#include
#include
#include
#include
@@ -50,8 +50,8 @@
#define LTQ_SPI_RXCNT 0x84
#define LTQ_SPI_DMACON0xec
#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
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
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
>> ---
>> 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
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -13,7 +14,6 @@
>> #include
>> #include
>> #include
>> -#include
>> #include
>> #include
>> #include
>> @@ -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
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
> ---
> 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
> #include
> +#include
> #include
> #include
> #include
> @@ -13,7 +14,6 @@
> #include
> #include
> #include
> -#include
> #include
> #include
> #include
> @@ -50,8 +50,8 @@
> #define LTQ_SPI_RXCNT0x84
> #define LTQ_SPI_DMACON 0xec
> #define LTQ_SPI_IRNEN0xf4
> -#define LTQ_SPI_IRNICR 0xf8
> -#define LTQ_SPI_IRNCR0xfc
> +#define LTQ_SPI_IRNCR0xf8
> +#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_S16 /* 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 mutexlock;
>
> const u8*tx;
> u8 *rx;
> @@ -186,6 +184,8 @@ struct lantiq_ssc_spi {
> unsigned intbase_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, >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,
Re: [PATCH 1/4] spi: lantiq: Synchronize interrupt handlers and transfers
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. signature.asc Description: PGP signature
