spi: atmel: fix corruption caused by too early transfer completion
Subsystems (3)
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
The PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is raised when the current and the next transfer has finished (RXBUFF). The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running. This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)
drivers/spi/spi-atmel.c +8 -9
@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }
@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );
@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending);
Signature · 1 line
1.9.3
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
Quoted 11 lines · Ronald Wahl
The PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is raised when the current and the next transfer has finished (RXBUFF). The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running. This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
Forgot to Cc: Nicolas Ferre ...
- ron
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
In reply to Ronald WahlHi,
Quoted 8 lines
-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Ronald Wahl
Sent: Wednesday, August 06, 2014 9:01 PM
To: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi: atmel: fix corruption caused by too early transfer completion
Quoted 10 lines
The PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is raised when the current and the next transfer has finished (RXBUFF). The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running.
This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
The patch can work, but it maybe decrease the performance.
Could you share the scenario caused the overruns and/or corruption, or log message?
Quoted 57 lines
Signed-off-by Ronald Wahl <[email protected]>--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)drivers/spi/spi-atmel.c index 113c83f..3f7d138 renamed from drivers/spi/spi-atmel.c +0
-0100644
drivers/spi/spi-atmel.c +8
-10@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending); --1.9.3
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Best Regards,
Wenyou Yang
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
Hi,
Quoted 20 lines · Wenyou
Hi,
Quoted 8 lines
-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Ronald Wahl
Sent: Wednesday, August 06, 2014 9:01 PM
To: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi: atmel: fix corruption caused by too early transfer completionQuoted 10 lines
The PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is raised when the current and the next transfer has finished (RXBUFF). The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running.
This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
Quoted 1 line
The patch can work, but it maybe decrease the performance.
Can you give more details? I had also the same feeling at the beginning but due to the way pdc is implemented in the spi driver (doesn't really take advantage of the double buffer), I think it should not change performances.
My only concern is why ENDRX was chosen instead of RXBUFF? Simple bug or to manage a specific case we are thinking about.
Quoted 72 lines
Could you share the scenario caused the overruns and/or corruption, or log message?
Quoted 57 lines
Signed-off-by Ronald Wahl <[email protected]>--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)drivers/spi/spi-atmel.c index 113c83f..3f7d138 renamed from drivers/spi/spi-atmel.c +0
-0100644
drivers/spi/spi-atmel.c +8
-10@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending); --1.9.3
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelBest Regards,
Wenyou Yang_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
Quoted 35 lines
-----Original Message-----
From: Ludovic Desroches [mailto:ludovic.desroches at atmel.com]
Sent: Wednesday, August 13, 2014 2:17 PM
To: Yang, Wenyou
Cc: Ronald Wahl; linux-arm-kernel at lists.infradead.org; Ferre, Nicolas
Subject: Re: [PATCH] spi: atmel: fix corruption caused by too early transfer completionHi,
Quoted 15 lines · Wenyou
Hi,
Quoted 14 lines
-----Original Message-----
From: linux-arm-kernel
[mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Ronald Wahl
Sent: Wednesday, August 06, 2014 9:01 PM
To: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi: atmel: fix corruption caused by too early transfer completionThe PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is
raised when the current and the next transfer has finished (RXBUFF).
Quoted 6 lines
Quoted 5 lines
The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running. This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
Quoted 5 lines
Quoted 1 line
The patch can work, but it maybe decrease the performance.
Can you give more details? I had also the same feeling at the beginning but due to the way pdc is implemented in the spi driver (doesn't really take advantage of the double buffer), I think it should not change performances.
I am not sure, but the next transfer registers is used in the driver code
Quoted 3 lines
My only concern is why ENDRX was chosen instead of RXBUFF? Simple bug or to manage a specific case we are thinking about.
Both the current transfer and the next transfer is done, RXBUFF bit is set. Either the current transfer or the next transfer is done, ENDRX bit set. I think it is OK.
Quoted 75 lines
Quoted 3 lines
Could you share the scenario caused the overruns and/or corruption, or log
message?
Quoted 71 lines
Quoted 59 lines
Signed-off-by Ronald Wahl <[email protected]>--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)drivers/spi/spi-atmel.c index renamed from drivers/spi/spi-atmel.c +0
-0113c83f..3f7d138
100644drivers/spi/spi-atmel.c +8
-10@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending); --1.9.3
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelBest Regards,
Wenyou Yang_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
Quoted 35 lines · Wenyou
Quoted 35 lines
-----Original Message-----
From: Ludovic Desroches [mailto:ludovic.desroches at atmel.com]
Sent: Wednesday, August 13, 2014 2:17 PM
To: Yang, Wenyou
Cc: Ronald Wahl; linux-arm-kernel at lists.infradead.org; Ferre, Nicolas
Subject: Re: [PATCH] spi: atmel: fix corruption caused by too early transfer completionHi,
Quoted 15 lines · Wenyou
Hi,
Quoted 14 lines
-----Original Message-----
From: linux-arm-kernel
[mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Ronald Wahl
Sent: Wednesday, August 06, 2014 9:01 PM
To: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi: atmel: fix corruption caused by too early transfer completionThe PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is
raised when the current and the next transfer has finished (RXBUFF).
Quoted 6 lines
Quoted 5 lines
The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running. This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
The patch can work, but it maybe decrease the performance.
Can you give more details? I had also the same feeling at the beginning but due to the way pdc is implemented in the spi driver (doesn't really take advantage of the double buffer), I think it should not change performances.
I am not sure, but the next transfer registers is used in the driver code
Yes it is used but not in the optimal way. Here, we configure the transfer to send two buffers. Once done, we will do it again if needed.
Taking advantage of the double buffer means when we receive ENDRX we immediately prepare the next transfer. Then we won't have dead time. Since it's not done in this way in the driver I don't think it will decrease performances.
Quoted 83 lines
Quoted 4 lines
My only concern is why ENDRX was chosen instead of RXBUFF? Simple bug or to manage a specific case we are thinking about.
Both the current transfer and the next transfer is done, RXBUFF bit is set. Either the current transfer or the next transfer is done, ENDRX bit set. I think it is OK.
Quoted 75 lines
Quoted 3 lines
Could you share the scenario caused the overruns and/or corruption, or log
message?
Quoted 71 lines
Quoted 59 lines
Signed-off-by Ronald Wahl <[email protected]>--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)drivers/spi/spi-atmel.c index renamed from drivers/spi/spi-atmel.c +0
-0113c83f..3f7d138
100644drivers/spi/spi-atmel.c +8
-10@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending); --1.9.3
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelBest Regards,
Wenyou Yang_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
Quoted 43 lines · Ludovic
Quoted 35 lines · Wenyou
Quoted 35 lines
-----Original Message-----
From: Ludovic Desroches [mailto:ludovic.desroches at atmel.com]
Sent: Wednesday, August 13, 2014 2:17 PM
To: Yang, Wenyou
Cc: Ronald Wahl; linux-arm-kernel at lists.infradead.org; Ferre, Nicolas
Subject: Re: [PATCH] spi: atmel: fix corruption caused by too early transfer completionHi,
Quoted 15 lines · Wenyou
Hi,
Quoted 14 lines
-----Original Message-----
From: linux-arm-kernel
[mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Ronald Wahl
Sent: Wednesday, August 06, 2014 9:01 PM
To: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi: atmel: fix corruption caused by too early transfer completionThe PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is
raised when the current and the next transfer has finished (RXBUFF).
Quoted 6 lines
Quoted 5 lines
The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running. This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
The patch can work, but it maybe decrease the performance.
Can you give more details? I had also the same feeling at the beginning but due to the way pdc is implemented in the spi driver (doesn't really take advantage of the double buffer), I think it should not change performances.
I am not sure, but the next transfer registers is used in the driver code
Quoted 7 lines
Yes it is used but not in the optimal way. Here, we configure the transfer to send two buffers. Once done, we will do it again if needed.
Taking advantage of the double buffer means when we receive ENDRX we immediately prepare the next transfer. Then we won't have dead time. Since it's not done in this way in the driver I don't think it will decrease performances.
In kernel 3.10 the code was designed this way (programming next transfer registers while a transfer was still running). Scheduling the next transfer was happening from ENDRX interrupt context. The only thing I'm not sure about is if it behaves correctly when the primary transfer finishes but the driver thinks it is still running and so only programs the next transfer. Will this transfer start?
Quoted 82 lines
Quoted 81 lines
Quoted 2 lines
My only concern is why ENDRX was chosen instead of RXBUFF? Simple bug or to manage a specific case we are thinking about.
Both the current transfer and the next transfer is done, RXBUFF bit is set. Either the current transfer or the next transfer is done, ENDRX bit set. I think it is OK.
Quoted 75 lines
Quoted 3 lines
Could you share the scenario caused the overruns and/or corruption, or log
message?
Quoted 71 lines
Quoted 59 lines
Signed-off-by Ronald Wahl <[email protected]>--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)drivers/spi/spi-atmel.c index renamed from drivers/spi/spi-atmel.c +0
-0113c83f..3f7d138
100644drivers/spi/spi-atmel.c +8
-10@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending); --1.9.3
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelBest Regards,
Wenyou Yang_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 5 lines
Ronald Wahl - ronald.wahl at raritan.com - Phone +49 375271349-0 Fax -99 Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749 Amtsgericht Chemnitz HRB 23605 Gesch?ftsf?hrung: Stuart Hopper, Ralf Ploenes
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
Quoted 54 lines · Ronald Wahl
Quoted 43 lines · Ludovic
Quoted 35 lines · Wenyou
Quoted 35 lines
-----Original Message-----
From: Ludovic Desroches [mailto:ludovic.desroches at atmel.com]
Sent: Wednesday, August 13, 2014 2:17 PM
To: Yang, Wenyou
Cc: Ronald Wahl; linux-arm-kernel at lists.infradead.org; Ferre, Nicolas
Subject: Re: [PATCH] spi: atmel: fix corruption caused by too early transfer completionHi,
Quoted 15 lines · Wenyou
Hi,
Quoted 14 lines
-----Original Message-----
From: linux-arm-kernel
[mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Ronald Wahl
Sent: Wednesday, August 06, 2014 9:01 PM
To: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi: atmel: fix corruption caused by too early transfer completionThe PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is
raised when the current and the next transfer has finished (RXBUFF).
Quoted 6 lines
Quoted 5 lines
The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running. This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
The patch can work, but it maybe decrease the performance.
Can you give more details? I had also the same feeling at the beginning but due to the way pdc is implemented in the spi driver (doesn't really take advantage of the double buffer), I think it should not change performances.
I am not sure, but the next transfer registers is used in the driver code
Quoted 7 lines
Yes it is used but not in the optimal way. Here, we configure the transfer to send two buffers. Once done, we will do it again if needed.
Taking advantage of the double buffer means when we receive ENDRX we immediately prepare the next transfer. Then we won't have dead time. Since it's not done in this way in the driver I don't think it will decrease performances.
Quoted 7 lines
In kernel 3.10 the code was designed this way (programming next transfer registers while a transfer was still running). Scheduling the next transfer was happening from ENDRX interrupt context. The only thing I'm not sure about is if it behaves correctly when the primary transfer finishes but the driver thinks it is still running and so only programs the next transfer. Will this transfer start?
I don't see in which case it can progam only the next transfer (on 3.16). It configures the primary transfer and the next transfer if there are remaining bytes but maybe I miss something.
In a general way, when using PDC, at the end of the primary transfer, if RNCR is not zero then RNPR and RNCR are copied to RPR and RCR. So yes, PDC will start the transfer.
Nevertheless, it will be safer to use RXBUFF before configuring PDC again because it can cause some corruption as you observed.
Quoted 94 lines
Quoted 82 lines
Quoted 81 lines
Quoted 2 lines
My only concern is why ENDRX was chosen instead of RXBUFF? Simple bug or to manage a specific case we are thinking about.
Both the current transfer and the next transfer is done, RXBUFF bit is set. Either the current transfer or the next transfer is done, ENDRX bit set. I think it is OK.
Quoted 75 lines
Quoted 3 lines
Could you share the scenario caused the overruns and/or corruption, or log
message?
Quoted 71 lines
Quoted 59 lines
Signed-off-by Ronald Wahl <[email protected]>--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)drivers/spi/spi-atmel.c index renamed from drivers/spi/spi-atmel.c +0
-0113c83f..3f7d138
100644drivers/spi/spi-atmel.c +8
-10@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending); --1.9.3
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelBest Regards,
Wenyou Yang_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelSignature · 10 lines
Ronald Wahl - ronald.wahl at raritan.com - Phone +49 375271349-0 Fax -99 Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749 Amtsgericht Chemnitz HRB 23605 Gesch?ftsf?hrung: Stuart Hopper, Ralf Ploenes _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
Quoted 63 lines · Ludovic Desroches
Quoted 54 lines · Ronald Wahl
Quoted 43 lines · Ludovic
Quoted 35 lines · Wenyou
Quoted 35 lines
-----Original Message-----
From: Ludovic Desroches [mailto:ludovic.desroches at atmel.com]
Sent: Wednesday, August 13, 2014 2:17 PM
To: Yang, Wenyou
Cc: Ronald Wahl; linux-arm-kernel at lists.infradead.org; Ferre, Nicolas
Subject: Re: [PATCH] spi: atmel: fix corruption caused by too early transfer completionHi,
Quoted 15 lines · Wenyou
Hi,
Quoted 14 lines
-----Original Message-----
From: linux-arm-kernel
[mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Ronald Wahl
Sent: Wednesday, August 06, 2014 9:01 PM
To: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi: atmel: fix corruption caused by too early transfer completionThe PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is
raised when the current and the next transfer has finished (RXBUFF).
Quoted 6 lines
Quoted 5 lines
The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running. This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
The patch can work, but it maybe decrease the performance.
Can you give more details? I had also the same feeling at the beginning but due to the way pdc is implemented in the spi driver (doesn't really take advantage of the double buffer), I think it should not change performances.
I am not sure, but the next transfer registers is used in the driver code
Quoted 7 lines
Yes it is used but not in the optimal way. Here, we configure the transfer to send two buffers. Once done, we will do it again if needed.
Taking advantage of the double buffer means when we receive ENDRX we immediately prepare the next transfer. Then we won't have dead time. Since it's not done in this way in the driver I don't think it will decrease performances.
Quoted 7 lines
In kernel 3.10 the code was designed this way (programming next transfer registers while a transfer was still running). Scheduling the next transfer was happening from ENDRX interrupt context. The only thing I'm not sure about is if it behaves correctly when the primary transfer finishes but the driver thinks it is still running and so only programs the next transfer. Will this transfer start?
Quoted 7 lines
I don't see in which case it can progam only the next transfer (on 3.16). It configures the primary transfer and the next transfer if there are remaining bytes but maybe I miss something.
In a general way, when using PDC, at the end of the primary transfer, if RNCR is not zero then RNPR and RNCR are copied to RPR and RCR. So yes, PDC will start the transfer.
Current driver design does not have this issue but also does not make efficient use of the double buffering. My question above was related to the driver design in 3.10 where the double buffering was used more effectively.
So what happens if primary and next transfer is RCR and RNCR are both already zero (hence no transfer running anymore) and only RNCR is programmed? I think this can happen in the old driver design in 3.10. I ask just because someone might want to rearrage the driver so that it uses the next transfer more effectively as it was in 3.10.
Quoted 98 lines
Nevertheless, it will be safer to use RXBUFF before configuring PDC again because it can cause some corruption as you observed.
Quoted 94 lines
Quoted 77 lines
Quoted 77 lines
Quoted 2 lines
My only concern is why ENDRX was chosen instead of RXBUFF? Simple bug or to manage a specific case we are thinking about.
Both the current transfer and the next transfer is done, RXBUFF bit is set. Either the current transfer or the next transfer is done, ENDRX bit set. I think it is OK.
Quoted 71 lines
Quoted 3 lines
Could you share the scenario caused the overruns and/or corruption, or log
message?
Quoted 67 lines
Quoted 59 lines
Signed-off-by Ronald Wahl <[email protected]>--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)drivers/spi/spi-atmel.c index renamed from drivers/spi/spi-atmel.c +0
-0113c83f..3f7d138
100644drivers/spi/spi-atmel.c +8
-10@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending); --1.9.3
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelBest Regards,
Wenyou YangQuoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel--
Ronald Wahl - ronald.wahl at raritan.com - Phone +49 375271349-0 Fax -99 Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Gesch?ftsf?hrung: Stuart Hopper, Ralf Ploenes_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Signature · 5 lines
Ronald Wahl - ronald.wahl at raritan.com - Phone +49 375271349-0 Fax -99 Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749 Amtsgericht Chemnitz HRB 23605 Gesch?ftsf?hrung: Stuart Hopper, Ralf Ploenes
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
Quoted 79 lines · Ronald Wahl
Quoted 63 lines · Ludovic Desroches
Quoted 54 lines · Ronald Wahl
Quoted 43 lines · Ludovic
Quoted 35 lines · Wenyou
Quoted 35 lines
-----Original Message-----
From: Ludovic Desroches [mailto:ludovic.desroches at atmel.com]
Sent: Wednesday, August 13, 2014 2:17 PM
To: Yang, Wenyou
Cc: Ronald Wahl; linux-arm-kernel at lists.infradead.org; Ferre, Nicolas
Subject: Re: [PATCH] spi: atmel: fix corruption caused by too early transfer completionHi,
Quoted 15 lines · Wenyou
Hi,
Quoted 14 lines
-----Original Message-----
From: linux-arm-kernel
[mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Ronald Wahl
Sent: Wednesday, August 06, 2014 9:01 PM
To: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi: atmel: fix corruption caused by too early transfer completionThe PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is
raised when the current and the next transfer has finished (RXBUFF).
Quoted 6 lines
Quoted 5 lines
The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running. This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
Quoted 6 lines
Quoted 5 lines
Quoted 1 line
The patch can work, but it maybe decrease the performance.
Can you give more details? I had also the same feeling at the beginning but due to the way pdc is implemented in the spi driver (doesn't really take advantage of the double buffer), I think it should not change performances.
I am not sure, but the next transfer registers is used in the driver code
Quoted 7 lines
Yes it is used but not in the optimal way. Here, we configure the transfer to send two buffers. Once done, we will do it again if needed.
Taking advantage of the double buffer means when we receive ENDRX we immediately prepare the next transfer. Then we won't have dead time. Since it's not done in this way in the driver I don't think it will decrease performances.
Quoted 7 lines
In kernel 3.10 the code was designed this way (programming next transfer registers while a transfer was still running). Scheduling the next transfer was happening from ENDRX interrupt context. The only thing I'm not sure about is if it behaves correctly when the primary transfer finishes but the driver thinks it is still running and so only programs the next transfer. Will this transfer start?
Quoted 7 lines
I don't see in which case it can progam only the next transfer (on 3.16). It configures the primary transfer and the next transfer if there are remaining bytes but maybe I miss something.
In a general way, when using PDC, at the end of the primary transfer, if RNCR is not zero then RNPR and RNCR are copied to RPR and RCR. So yes, PDC will start the transfer.
Current driver design does not have this issue but also does not make efficient use of the double buffering. My question above was related to the driver design in 3.10 where the double buffering was used more effectively.
Quoted 6 lines
So what happens if primary and next transfer is RCR and RNCR are both already zero (hence no transfer running anymore) and only RNCR is programmed? I think this can happen in the old driver design in 3.10. I ask just because someone might want to rearrage the driver so that it uses the next transfer more effectively as it was in 3.10.
The transfer should start, writting into RNCR should trigger a copy of RNCR to RCR. You could always write into RNCR and never into RCR.
Quoted 111 lines
Quoted 99 lines
Nevertheless, it will be safer to use RXBUFF before configuring PDC again because it can cause some corruption as you observed.
Quoted 94 lines
Quoted 77 lines
Quoted 77 lines
Quoted 2 lines
My only concern is why ENDRX was chosen instead of RXBUFF? Simple bug or to manage a specific case we are thinking about.
Both the current transfer and the next transfer is done, RXBUFF bit is set. Either the current transfer or the next transfer is done, ENDRX bit set. I think it is OK.
Quoted 71 lines
Quoted 3 lines
Could you share the scenario caused the overruns and/or corruption, or log
message?
Quoted 67 lines
Quoted 59 lines
Signed-off-by Ronald Wahl <[email protected]>--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)drivers/spi/spi-atmel.c index renamed from drivers/spi/spi-atmel.c +0
-0113c83f..3f7d138
100644drivers/spi/spi-atmel.c +8
-10@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending); --1.9.3
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelBest Regards,
Wenyou YangQuoted 5 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel--
Ronald Wahl - ronald.wahl at raritan.com - Phone +49 375271349-0 Fax -99 Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749
Amtsgericht Chemnitz HRB 23605
Gesch?ftsf?hrung: Stuart Hopper, Ralf Ploenes_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelSignature · 10 lines
Ronald Wahl - ronald.wahl at raritan.com - Phone +49 375271349-0 Fax -99 Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749 Amtsgericht Chemnitz HRB 23605 Gesch?ftsf?hrung: Stuart Hopper, Ralf Ploenes _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel at lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH 1/1 spi: atmel: fix corruption caused by too early transfer completion
In reply to Yang, WenyouQuoted 20 lines · Wenyou
Hi,
Quoted 8 lines
-----Original Message-----
From: linux-arm-kernel [mailto:linux-arm-kernel-bounces at lists.infradead.org] On Behalf Of Ronald Wahl
Sent: Wednesday, August 06, 2014 9:01 PM
To: linux-arm-kernel at lists.infradead.org
Subject: [PATCH] spi: atmel: fix corruption caused by too early transfer completionQuoted 10 lines
The PDC (peripheral DMA controller) on AT91 supports two transfer counters and associated registers - one for current and one for the next transfer. If the current transfer is done the next transfer is moved into the current transfer. Now there are two interrupts: one is raised whenever a single transfer is done (ENDRX) and the other one is raised when the current and the next transfer has finished (RXBUFF). The issue is that the driver only enables the ENDRX interrupt which may lead to queuing a new request while there is still a transfer running.
This can lead to overruns and/or corruption. By using the RXBUFF interrupt only we queue new requests only when the hardware queue is empty avoiding this problem.
Quoted 3 lines
The patch can work, but it maybe decrease the performance.
Could you share the scenario caused the overruns and/or corruption, or log message?
The problem is that without the patch the interrupt handler signals completion of the transfer while the next transfer registers are moved into the primary transfer registers hence PDC is still active. This happens only when the amount of data was large enough so that the next transfer register is used. Since we signaled completion the driver can schedule the next transfer which then would overwrite (corrupt) the possibly still running transfer.
In my specific case a JFFS2 filesystem has been corrupted while accessing a cramfs on the same flash chip. The former uses small requests that can run as a single pdc spi transfer but the latter used 16kB transfer requests that are split into 4 pdc spi transfers of 4k each.
Quoted 64 lines
Quoted 56 lines
Signed-off-by Ronald Wahl <[email protected]>--- drivers/spi/spi-atmel.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)drivers/spi/spi-atmel.c index 113c83f..3f7d138 renamed from drivers/spi/spi-atmel.c +0
-0100644
drivers/spi/spi-atmel.c +8
-10@@ -775,17 +775,17 @@ static void atmel_spi_pdc_next_xfer(struct spi_master *master, (unsigned long long)xfer->rx_dma); } -/* REVISIT: We're waiting for ENDRX before we start the next +/* REVISIT: We're waiting for RXBUFF before we start the next * transfer because we need to handle some difficult timing -* issues otherwise. If we wait for ENDTX in one transfer and -* then starts waiting for ENDRX in the next, it's difficult -* to tell the difference between the ENDRX interrupt we're -* actually waiting for and the ENDRX interrupt of the +* issues otherwise. If we wait for TXBUFE in one transfer and +* then starts waiting for RXBUFF in the next, it's difficult +* to tell the difference between the RXBUFF interrupt we're +* actually waiting for and the RXBUFF interrupt of the * previous transfer. * * It should be doable, though. Just not now... */ -spi_writel (as,IER ,SPI_BIT (ENDRX ) |SPI_BIT (OVRES )); +spi_writel (as,IER ,SPI_BIT (RXBUFF ) |SPI_BIT (OVRES ));spi_writel (as,PTCR ,SPI_BIT (TXTEN ) |SPI_BIT (RXTEN )); }@@ -956,8 +956,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id) ret =IRQ_HANDLED ; -spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ) - |SPI_BIT (OVRES ))); +spi_writel (as,IDR , (SPI_BIT (RXBUFF ) |SPI_BIT (OVRES )));/* Clear any overrun happening while cleaning up */ spi_readl (as,SR );@@ -966,7 +965,7 @@ atmel_spi_pdc_interrupt(int irq, void *dev_id)complete (&as->xfer_completion); - }else if (pending & (SPI_BIT (RXBUFF ) |SPI_BIT (ENDRX ))) { + }else if (pending & (SPI_BIT (RXBUFF ))) { ret =IRQ_HANDLED ;spi_writel (as,IDR , pending); --1.9.3
Quoted 4 lines
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernelBest Regards,
Wenyou Yang
Signature · 5 lines
Ronald Wahl - ronald.wahl at raritan.com - Phone +49 375271349-0 Fax -99 Raritan Deutschland GmbH, Kornmarkt 7, 08056 Zwickau, Germany USt-IdNr. DE813094160, Steuer-Nr. 227/117/01749 Amtsgericht Chemnitz HRB 23605 Gesch?ftsf?hrung: Stuart Hopper, Ralf Ploenes