| Message ID | 20200619141657.498868116@linuxfoundation.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | None | expand |
On Fri 2020-06-19 16:32:47, Greg Kroah-Hartman wrote: > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > [ Upstream commit f0410bbf7d0fb80149e3b17d11d31f5b5197873e ] > > DW APB SSI DMA-part of the driver may need to perform the requested > SPI-transfer synchronously. In that case the dma_transfer() callback > will return 0 as a marker of the SPI transfer being finished so the > SPI core doesn't need to wait and may proceed with the SPI message > trasnfers pumping procedure. This will be needed to fix the problem > when DMA transactions are finished, but there is still data left in > the SPI Tx/Rx FIFOs being sent/received. But for now make dma_transfer > to return 1 as the normal dw_spi_transfer_one() method. As far as I understand, this is support for new SoC, not a fix? > +++ b/drivers/spi/spi-dw.c > @@ -383,11 +383,8 @@ static int dw_spi_transfer_one(struct spi_controller *master, > > spi_enable_chip(dws, 1); > > - if (dws->dma_mapped) { > - ret = dws->dma_ops->dma_transfer(dws, transfer); > - if (ret < 0) > - return ret; > - } > + if (dws->dma_mapped) > + return dws->dma_ops->dma_transfer(dws, transfer); > > if (chip->poll_mode) > return poll_transfer(dws); Mainline patch simply changes return value, but code is different in v4.19, and poll_transfer will now be avoided when dws->dma_mapped. Is that a problem? Best regards, Pavel
Hello Pavel On Fri, Jun 19, 2020 at 11:07:19PM +0200, Pavel Machek wrote: > On Fri 2020-06-19 16:32:47, Greg Kroah-Hartman wrote: > > From: Serge Semin <Sergey.Semin@baikalelectronics.ru> > > > > [ Upstream commit f0410bbf7d0fb80149e3b17d11d31f5b5197873e ] > > > > DW APB SSI DMA-part of the driver may need to perform the requested > > SPI-transfer synchronously. In that case the dma_transfer() callback > > will return 0 as a marker of the SPI transfer being finished so the > > SPI core doesn't need to wait and may proceed with the SPI message > > trasnfers pumping procedure. This will be needed to fix the problem > > when DMA transactions are finished, but there is still data left in > > the SPI Tx/Rx FIFOs being sent/received. But for now make dma_transfer > > to return 1 as the normal dw_spi_transfer_one() method. > > As far as I understand, this is support for new SoC, not a fix? Not really. That patch is a first one of a series fixing a problem with SPI transfer completion: 33726eff3d98 spi: dw: Add SPI Rx-done wait method to DMA-based transfer 1ade2d8a72f9 spi: dw: Add SPI Tx-done wait method to DMA-based transfer bdbdf0f06337 spi: dw: Locally wait for the DMA transfers completion f0410bbf7d0f spi: dw: Return any value retrieved from the dma_transfer callback In anyway having just first commit applied is harmless, though pretty much pointless in fixing the problem it had been originally introduced for. But it can be useful for something else. See my comment below. > > > +++ b/drivers/spi/spi-dw.c > > @@ -383,11 +383,8 @@ static int dw_spi_transfer_one(struct spi_controller *master, > > > > spi_enable_chip(dws, 1); > > > > - if (dws->dma_mapped) { > > - ret = dws->dma_ops->dma_transfer(dws, transfer); > > - if (ret < 0) > > - return ret; > > - } > > + if (dws->dma_mapped) > > + return dws->dma_ops->dma_transfer(dws, transfer); > > > > if (chip->poll_mode) > > return poll_transfer(dws); > > Mainline patch simply changes return value, but code is different in > v4.19, and poll_transfer will now be avoided when dws->dma_mapped. Is > that a problem? Actually no.) In that old 4.19 context it's even better to return straight away no matter what value is returned by the dma_transfer() callback. In the code without this patch applied, the transfer_one() method will check the poll_mode flag state even if the dma_transfer() returns a positive value. The positive value (1) means that the DMA transfer has been executed and the SPI core must wait for its completion. Needless to say, that if the poll_mode flag state gets to be true, then a poll-transfer will be executed alongside with the DMA transfer. Which as you understand will be very wrong. So by having this patch applied we implicitly fix that problem. Although a probability of the problematic situation is very low, since the DW APB SSI driver poll-mode hasn't been utilized by any SPI client driver since long time ago... -Sergey > > Best regards, > Pavel > -- > (english) http://www.livejournal.com/~pavelmachek > (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Hi Serge, Pavel, Greg, On Mon, Jun 22, 2020 at 11:51:21PM +0300, Serge Semin wrote: >Hello Pavel > >On Fri, Jun 19, 2020 at 11:07:19PM +0200, Pavel Machek wrote: > >> Mainline patch simply changes return value, but code is different in >> v4.19, and poll_transfer will now be avoided when dws->dma_mapped. Is >> that a problem? > >Actually no.) In that old 4.19 context it's even better to return straight away >no matter what value is returned by the dma_transfer() callback. This patch changes the return dma_transfer return value from 0 to 1, however it was only done in spi-dw-mid.c func mid_spi_dma_transfer(). There is an identical function in spi-dw-mmio.c that needs the same treatment, otherwise access to the SPI device becomes erratic and even causes kernel to hang. Guess how I found this ;-) So the following patch is needed as well, at least in 4.9 and 4.19, I did not check/test other versions. Mainline does not need this, since the code seems to have been refactored to avoid the duplication. Regards, -Ralph diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c index c563c2815093..99641c485288 100644 --- a/drivers/spi/spi-dw-mmio.c +++ b/drivers/spi/spi-dw-mmio.c @@ -358,7 +358,7 @@ static int mmio_spi_dma_transfer(struct dw_spi *dws, struct spi_transfer *xfer) dma_async_issue_pending(dws->txchan); } - return 0; + return 1; } static void mmio_spi_dma_stop(struct dw_spi *dws)
On Fri, Jun 26, 2020 at 11:18:00AM -0400, Ralph Siemsen wrote: >Hi Serge, Pavel, Greg, > >On Mon, Jun 22, 2020 at 11:51:21PM +0300, Serge Semin wrote: >>Hello Pavel >> >>On Fri, Jun 19, 2020 at 11:07:19PM +0200, Pavel Machek wrote: >> >>>Mainline patch simply changes return value, but code is different in >>>v4.19, and poll_transfer will now be avoided when dws->dma_mapped. Is >>>that a problem? >> >>Actually no.) In that old 4.19 context it's even better to return straight away >>no matter what value is returned by the dma_transfer() callback. > >This patch changes the return dma_transfer return value from 0 to 1, >however it was only done in spi-dw-mid.c func mid_spi_dma_transfer(). > >There is an identical function in spi-dw-mmio.c that needs the same >treatment, otherwise access to the SPI device becomes erratic and even >causes kernel to hang. Guess how I found this ;-) > >So the following patch is needed as well, at least in 4.9 and 4.19, I >did not check/test other versions. Mainline does not need this, since >the code seems to have been refactored to avoid the duplication. Could you add your signed-off-by tag please? :)
Hi Sasha, On Fri, Jun 26, 2020 at 04:07:10PM -0400, Sasha Levin wrote: >On Fri, Jun 26, 2020 at 11:18:00AM -0400, Ralph Siemsen wrote: >> >>So the following patch is needed as well, at least in 4.9 and 4.19, >>I did not check/test other versions. Mainline does not need this, >>since the code seems to have been refactored to avoid the >>duplication. > >Could you add your signed-off-by tag please? :) Whoops, for some reason I snipped it out... sorry about that! Here it is again, with the commit message tweaked for clarity. Subject: [PATCH] spi: dw: Fix return value of dma_transfer callback Earlier commit "spi: dw: Return any value retrieved from the dma_transfer callback" changed the return code of mid_spi_dma_transfer() from 0 to 1 in drivers/spi/spi-dw-mid.c. A similar change is needed spi-dw-mmio.c for mmio_spi_dma_transfer() function. Note this only applies to older branches, as mainline has refactored the code to avoid duplication. Signed-off-by: Ralph Siemsen <ralph.siemsen@linaro.org> --- drivers/spi/spi-dw-mmio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c index c563c2815093..99641c485288 100644 --- a/drivers/spi/spi-dw-mmio.c +++ b/drivers/spi/spi-dw-mmio.c @@ -358,7 +358,7 @@ static int mmio_spi_dma_transfer(struct dw_spi *dws, struct spi_transfer *xfer) dma_async_issue_pending(dws->txchan); } - return 0; + return 1; } static void mmio_spi_dma_stop(struct dw_spi *dws)
On Fri, Jun 26, 2020 at 11:18:00AM -0400, Ralph Siemsen wrote: >Hi Serge, Pavel, Greg, > >On Mon, Jun 22, 2020 at 11:51:21PM +0300, Serge Semin wrote: >>Hello Pavel >> >>On Fri, Jun 19, 2020 at 11:07:19PM +0200, Pavel Machek wrote: >> >>>Mainline patch simply changes return value, but code is different in >>>v4.19, and poll_transfer will now be avoided when dws->dma_mapped. Is >>>that a problem? >> >>Actually no.) In that old 4.19 context it's even better to return straight away >>no matter what value is returned by the dma_transfer() callback. > >This patch changes the return dma_transfer return value from 0 to 1, >however it was only done in spi-dw-mid.c func mid_spi_dma_transfer(). > >There is an identical function in spi-dw-mmio.c that needs the same >treatment, otherwise access to the SPI device becomes erratic and even >causes kernel to hang. Guess how I found this ;-) > >So the following patch is needed as well, at least in 4.9 and 4.19, I >did not check/test other versions. Mainline does not need this, since >the code seems to have been refactored to avoid the duplication. > >Regards, >-Ralph > >diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c >index c563c2815093..99641c485288 100644 >--- a/drivers/spi/spi-dw-mmio.c >+++ b/drivers/spi/spi-dw-mmio.c >@@ -358,7 +358,7 @@ static int mmio_spi_dma_transfer(struct dw_spi *dws, struct spi_transfer *xfer) Um, I can't find this function anywhere... what am I missing?
On Mon, Jun 29, 2020 at 10:26:06AM -0400, Sasha Levin wrote: >>diff --git a/drivers/spi/spi-dw-mmio.c b/drivers/spi/spi-dw-mmio.c >>index c563c2815093..99641c485288 100644 >>--- a/drivers/spi/spi-dw-mmio.c >>+++ b/drivers/spi/spi-dw-mmio.c >>@@ -358,7 +358,7 @@ static int mmio_spi_dma_transfer(struct dw_spi *dws, struct spi_transfer *xfer) > >Um, I can't find this function anywhere... what am I missing? Nothing... my bad. The code in question was added on a vendor branch (https://github.com/renesas-rz/rzn1_linux/blob/rzn1-stable-v4.19/drivers/spi/spi-dw-mmio.c#L338 if you are curious). I'm very sorry for wasting your time... please disregard the patch! -Ralph
diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c index e1b34ef9a31c..10f328558d55 100644 --- a/drivers/spi/spi-dw-mid.c +++ b/drivers/spi/spi-dw-mid.c @@ -274,7 +274,7 @@ static int mid_spi_dma_transfer(struct dw_spi *dws, struct spi_transfer *xfer) dma_async_issue_pending(dws->txchan); } - return 0; + return 1; } static void mid_spi_dma_stop(struct dw_spi *dws) diff --git a/drivers/spi/spi-dw.c b/drivers/spi/spi-dw.c index 3fbd6f01fb10..b1c137261d0f 100644 --- a/drivers/spi/spi-dw.c +++ b/drivers/spi/spi-dw.c @@ -383,11 +383,8 @@ static int dw_spi_transfer_one(struct spi_controller *master, spi_enable_chip(dws, 1); - if (dws->dma_mapped) { - ret = dws->dma_ops->dma_transfer(dws, transfer); - if (ret < 0) - return ret; - } + if (dws->dma_mapped) + return dws->dma_ops->dma_transfer(dws, transfer); if (chip->poll_mode) return poll_transfer(dws);