Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
On Fri, May 22, 2020 at 08:03:25PM +0800, Feng Tang wrote: > On Fri, May 22, 2020 at 02:32:35PM +0300, Serge Semin wrote: > > On Fri, May 22, 2020 at 03:58:44PM +0800, Feng Tang wrote: > > > Hi Serge, > > > > > > On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote: > > > > > > > > + dw_spi_dma_wait_rx_done(dws); > > > > > > > > > > > > > > I can understand the problem about TX, but I don't see how RX > > > > > > > will get hurt, can you elaborate more? thanks > > > > > > > > > > > > > > - Feng > > > > > > > > > > > > Your question is correct. You are right with your hypothesis. > > > > > > Ideally upon the > > > > > > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. > > > > > > That's why the > > > > > > commit log signifies the error being mostly related with Tx FIFO. > > > > > > But > > > > > > practically there are many reasons why Rx FIFO might be left with > > > > > > data: > > > > > > DMA engine failures, incorrect DMA configuration (if DW SPI or DW > > > > > > DMA driver > > > > > > messed something up), controller hanging up, and so on. It's better > > > > > > to catch > > > > > > an error at this stage while propagating it up to the SPI device > > > > > > drivers. > > > > > > Especially seeing the wait-check implementation doesn't gives us > > > > > > much of the > > > > > > execution overhead in normal conditions. So by calling > > > > > > dw_spi_dma_wait_rx_done() > > > > > > we make sure that all the data has been fetched and we may freely > > > > > > get the > > > > > > buffers back to the client driver. > > > > > > > > > > I see your point about checking RX. But I still don't think checking > > > > > RX FIFO level is the right way to detect error. Some data left in > > > > > RX FIFO doesn't always mean a error, say for some case if there is > > > > > 20 words in RX FIFO, and the driver starts a DMA request for 16 > > > > > words, then after a sucessful DMA transaction, there are 4 words > > > > > left without any error. > > > > > > > > Neither Tx nor Rx FIFO should be left with any data after transaction is > > > > finished. If they are then something has been wrong. > > > > > > > > See, every SPI transfer starts with FIFO clearance since we > > > > disable/enable the > > > > SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and > > > > spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). > > > > Here is the > > > > SSIENR register description: "It enables and disables all SPI > > > > Controller operations. > > > > When disabled, all serial transfers are halted immediately. Transmit > > > > and receive > > > > FIFO buffers are cleared when the device is disabled. It is impossible > > > > to program > > > > some of the SPI Controller control registers when enabled" > > > > > > > > No mater whether we start DMA request or perform the normal IRQ-based > > > > PIO, we > > > > request as much data as we need and neither Tx nor Rx FIFO are supposed > > > > to > > > > be left with any data after the request is finished. If data is left, > > > > then > > > > either we didn't push all of the necessary data to the SPI bus, or we > > > > didn't > > > > pull all the data from the FIFO, and this could have happened only due > > > > to some > > > > component mulfunction (drivers, DMA engine, SPI device). In any case > > > > the SPI > > > > device driver should be notified about the problem. > > > > > > Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The > > > former in dma case means the dma hw/driver has done its job, and spi > > > hw/driver > > > hasn't done its job of pushing out the data to spi slave devices, > > > > Agreed. > > > > > while the > > > latter means the spi hw/driver has done its job, while the dma hw/driver > > > hasn't. > > > > In this particular case agreed, that the data left in the Rx FIFO means DMA > > hw/driver hasn't done its work right. Though SPI hw could be also a reason > > of > > the data left in FIFO (though this only a theoretical consideration). > > Right, that's why I was initially very curious about this RX FIFO thing, > and if possible, please give some details in commit log about the data > left in TX FIFO problem, which will help future developers when they > met simliar bugs. Ok. I'll add a more descriptive patch log. > > And I'm fine with adding the rx check, no matter the problem is in > dma side or spi side. > > > > > > > And the code is called inside the dma rx channel callback, which means the > > > dma driver is saying "hey, I've done my job", but apparently it hasn't if > > > there is data left. > > > > Right, either it hasn't, or the DMA engine claimed it has, but still is > > doing > > something (asynchronously or something, depending on the hardware > > implementation), > > or it think it has, but in fact it hasn't due to whatever problem happened > > (software/hardware/etc.). In anyway we have to at least check whether it's > >
Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
On Fri, May 22, 2020 at 02:32:35PM +0300, Serge Semin wrote: > On Fri, May 22, 2020 at 03:58:44PM +0800, Feng Tang wrote: > > Hi Serge, > > > > On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote: > > > > > > > + dw_spi_dma_wait_rx_done(dws); > > > > > > > > > > > > I can understand the problem about TX, but I don't see how RX > > > > > > will get hurt, can you elaborate more? thanks > > > > > > > > > > > > - Feng > > > > > > > > > > Your question is correct. You are right with your hypothesis. Ideally > > > > > upon the > > > > > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's > > > > > why the > > > > > commit log signifies the error being mostly related with Tx FIFO. But > > > > > practically there are many reasons why Rx FIFO might be left with > > > > > data: > > > > > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA > > > > > driver > > > > > messed something up), controller hanging up, and so on. It's better > > > > > to catch > > > > > an error at this stage while propagating it up to the SPI device > > > > > drivers. > > > > > Especially seeing the wait-check implementation doesn't gives us much > > > > > of the > > > > > execution overhead in normal conditions. So by calling > > > > > dw_spi_dma_wait_rx_done() > > > > > we make sure that all the data has been fetched and we may freely get > > > > > the > > > > > buffers back to the client driver. > > > > > > > > I see your point about checking RX. But I still don't think checking > > > > RX FIFO level is the right way to detect error. Some data left in > > > > RX FIFO doesn't always mean a error, say for some case if there is > > > > 20 words in RX FIFO, and the driver starts a DMA request for 16 > > > > words, then after a sucessful DMA transaction, there are 4 words > > > > left without any error. > > > > > > Neither Tx nor Rx FIFO should be left with any data after transaction is > > > finished. If they are then something has been wrong. > > > > > > See, every SPI transfer starts with FIFO clearance since we > > > disable/enable the > > > SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and > > > spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). > > > Here is the > > > SSIENR register description: "It enables and disables all SPI Controller > > > operations. > > > When disabled, all serial transfers are halted immediately. Transmit and > > > receive > > > FIFO buffers are cleared when the device is disabled. It is impossible to > > > program > > > some of the SPI Controller control registers when enabled" > > > > > > No mater whether we start DMA request or perform the normal IRQ-based > > > PIO, we > > > request as much data as we need and neither Tx nor Rx FIFO are supposed to > > > be left with any data after the request is finished. If data is left, then > > > either we didn't push all of the necessary data to the SPI bus, or we > > > didn't > > > pull all the data from the FIFO, and this could have happened only due to > > > some > > > component mulfunction (drivers, DMA engine, SPI device). In any case the > > > SPI > > > device driver should be notified about the problem. > > > > Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The > > former in dma case means the dma hw/driver has done its job, and spi > > hw/driver > > hasn't done its job of pushing out the data to spi slave devices, > > Agreed. > > > while the > > latter means the spi hw/driver has done its job, while the dma hw/driver > > hasn't. > > In this particular case agreed, that the data left in the Rx FIFO means DMA > hw/driver hasn't done its work right. Though SPI hw could be also a reason of > the data left in FIFO (though this only a theoretical consideration). Right, that's why I was initially very curious about this RX FIFO thing, and if possible, please give some details in commit log about the data left in TX FIFO problem, which will help future developers when they met simliar bugs. And I'm fine with adding the rx check, no matter the problem is in dma side or spi side. > > > > And the code is called inside the dma rx channel callback, which means the > > dma driver is saying "hey, I've done my job", but apparently it hasn't if > > there is data left. > > Right, either it hasn't, or the DMA engine claimed it has, but still is doing > something (asynchronously or something, depending on the hardware > implementation), > or it think it has, but in fact it hasn't due to whatever problem happened > (software/hardware/etc.). In anyway we have to at least check whether it's > really done with fetching data and to be on a safe side give it some time to > make sure that the Rx FIFO isn't going to be emptied. Whatever problem it is > having a non empty Rx FIFO at the stage of calling > spi_finalize_current_transfer() > means a certain error. > > > > > As for the wait time > > > > + nents = dw_readl(dws, DW_SPI_RXFLR); > > +
Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
On Fri, May 22, 2020 at 03:58:44PM +0800, Feng Tang wrote: > Hi Serge, > > On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote: > > > > > > + dw_spi_dma_wait_rx_done(dws); > > > > > > > > > > I can understand the problem about TX, but I don't see how RX > > > > > will get hurt, can you elaborate more? thanks > > > > > > > > > > - Feng > > > > > > > > Your question is correct. You are right with your hypothesis. Ideally > > > > upon the > > > > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's > > > > why the > > > > commit log signifies the error being mostly related with Tx FIFO. But > > > > practically there are many reasons why Rx FIFO might be left with data: > > > > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA > > > > driver > > > > messed something up), controller hanging up, and so on. It's better to > > > > catch > > > > an error at this stage while propagating it up to the SPI device > > > > drivers. > > > > Especially seeing the wait-check implementation doesn't gives us much > > > > of the > > > > execution overhead in normal conditions. So by calling > > > > dw_spi_dma_wait_rx_done() > > > > we make sure that all the data has been fetched and we may freely get > > > > the > > > > buffers back to the client driver. > > > > > > I see your point about checking RX. But I still don't think checking > > > RX FIFO level is the right way to detect error. Some data left in > > > RX FIFO doesn't always mean a error, say for some case if there is > > > 20 words in RX FIFO, and the driver starts a DMA request for 16 > > > words, then after a sucessful DMA transaction, there are 4 words > > > left without any error. > > > > Neither Tx nor Rx FIFO should be left with any data after transaction is > > finished. If they are then something has been wrong. > > > > See, every SPI transfer starts with FIFO clearance since we disable/enable > > the > > SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and > > spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here > > is the > > SSIENR register description: "It enables and disables all SPI Controller > > operations. > > When disabled, all serial transfers are halted immediately. Transmit and > > receive > > FIFO buffers are cleared when the device is disabled. It is impossible to > > program > > some of the SPI Controller control registers when enabled" > > > > No mater whether we start DMA request or perform the normal IRQ-based PIO, > > we > > request as much data as we need and neither Tx nor Rx FIFO are supposed to > > be left with any data after the request is finished. If data is left, then > > either we didn't push all of the necessary data to the SPI bus, or we didn't > > pull all the data from the FIFO, and this could have happened only due to > > some > > component mulfunction (drivers, DMA engine, SPI device). In any case the SPI > > device driver should be notified about the problem. > > Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The > former in dma case means the dma hw/driver has done its job, and spi hw/driver > hasn't done its job of pushing out the data to spi slave devices, Agreed. > while the > latter means the spi hw/driver has done its job, while the dma hw/driver > hasn't. In this particular case agreed, that the data left in the Rx FIFO means DMA hw/driver hasn't done its work right. Though SPI hw could be also a reason of the data left in FIFO (though this only a theoretical consideration). > > And the code is called inside the dma rx channel callback, which means the > dma driver is saying "hey, I've done my job", but apparently it hasn't if > there is data left. Right, either it hasn't, or the DMA engine claimed it has, but still is doing something (asynchronously or something, depending on the hardware implementation), or it think it has, but in fact it hasn't due to whatever problem happened (software/hardware/etc.). In anyway we have to at least check whether it's really done with fetching data and to be on a safe side give it some time to make sure that the Rx FIFO isn't going to be emptied. Whatever problem it is having a non empty Rx FIFO at the stage of calling spi_finalize_current_transfer() means a certain error. > > As for the wait time > > + nents = dw_readl(dws, DW_SPI_RXFLR); > + ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes * > + BITS_PER_BYTE; > > Using this formula for checking TX makes sense, but it doesn't for RX. > Because the time of pushing data in TX FIFO to spi device depends on > the clk, but the time of transferring RX FIFO to memory is up to > the DMA controller and peripheral bus. On this I agree with you. That formulae doesn't describe exactly the time left before the Rx FIFO gets empty. But at least it provides an upper limit on the time needed for the peripheral bus to fetch the data from FIFO. If for some reason the internal
Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
Hi Serge, On Thu, May 21, 2020 at 06:33:17PM +0300, Serge Semin wrote: > > > > > + dw_spi_dma_wait_rx_done(dws); > > > > > > > > I can understand the problem about TX, but I don't see how RX > > > > will get hurt, can you elaborate more? thanks > > > > > > > > - Feng > > > > > > Your question is correct. You are right with your hypothesis. Ideally > > > upon the > > > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why > > > the > > > commit log signifies the error being mostly related with Tx FIFO. But > > > practically there are many reasons why Rx FIFO might be left with data: > > > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA > > > driver > > > messed something up), controller hanging up, and so on. It's better to > > > catch > > > an error at this stage while propagating it up to the SPI device drivers. > > > Especially seeing the wait-check implementation doesn't gives us much of > > > the > > > execution overhead in normal conditions. So by calling > > > dw_spi_dma_wait_rx_done() > > > we make sure that all the data has been fetched and we may freely get the > > > buffers back to the client driver. > > > > I see your point about checking RX. But I still don't think checking > > RX FIFO level is the right way to detect error. Some data left in > > RX FIFO doesn't always mean a error, say for some case if there is > > 20 words in RX FIFO, and the driver starts a DMA request for 16 > > words, then after a sucessful DMA transaction, there are 4 words > > left without any error. > > Neither Tx nor Rx FIFO should be left with any data after transaction is > finished. If they are then something has been wrong. > > See, every SPI transfer starts with FIFO clearance since we disable/enable the > SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and > spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here > is the > SSIENR register description: "It enables and disables all SPI Controller > operations. > When disabled, all serial transfers are halted immediately. Transmit and > receive > FIFO buffers are cleared when the device is disabled. It is impossible to > program > some of the SPI Controller control registers when enabled" > > No mater whether we start DMA request or perform the normal IRQ-based PIO, we > request as much data as we need and neither Tx nor Rx FIFO are supposed to > be left with any data after the request is finished. If data is left, then > either we didn't push all of the necessary data to the SPI bus, or we didn't > pull all the data from the FIFO, and this could have happened only due to some > component mulfunction (drivers, DMA engine, SPI device). In any case the SPI > device driver should be notified about the problem. Data left in TX FIFO and Data left in RX FIFO are 2 different stories. The former in dma case means the dma hw/driver has done its job, and spi hw/driver hasn't done its job of pushing out the data to spi slave devices, while the latter means the spi hw/driver has done its job, while the dma hw/driver hasn't. And the code is called inside the dma rx channel callback, which means the dma driver is saying "hey, I've done my job", but apparently it hasn't if there is data left. As for the wait time + nents = dw_readl(dws, DW_SPI_RXFLR); + ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes * +BITS_PER_BYTE; Using this formula for checking TX makes sense, but it doesn't for RX. Because the time of pushing data in TX FIFO to spi device depends on the clk, but the time of transferring RX FIFO to memory is up to the DMA controller and peripheral bus. Also for the + while (dw_spi_dma_rx_busy(dws) && retry--) + ndelay(ns); + the rx busy bit is cleared after this rx/tx checking, and it should be always true at this point. Am I mis-reading the code? Thanks, Feng > > -Sergey >
Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
Mark, Andy,
On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
>
[nip]
> +static void dw_spi_dma_calc_delay(struct dw_spi *dws, u32 nents,
> + struct spi_delay *delay)
> +{
> + unsigned long ns, us;
> +
> + ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> + BITS_PER_BYTE;
> +
> + if (ns <= NSEC_PER_USEC) {
> + delay->unit = SPI_DELAY_UNIT_NSECS;
> + delay->value = ns;
> + } else {
> + us = DIV_ROUND_UP(ns, NSEC_PER_USEC);
> + delay->unit = SPI_DELAY_UNIT_USECS;
> + delay->value = clamp_val(us, 0, USHRT_MAX);
> + }
> +}
> +
> +static inline bool dw_spi_dma_tx_busy(struct dw_spi *dws)
> +{
> + return !(dw_readl(dws, DW_SPI_SR) & SR_TF_EMPT);
> +}
> +
> +static void dw_spi_dma_wait_tx_done(struct dw_spi *dws)
> +{
> + int retry = WAIT_RETRIES;
> + struct spi_delay delay;
> + u32 nents;
> +
> + nents = dw_readl(dws, DW_SPI_TXFLR);
> + dw_spi_dma_calc_delay(dws, nents, );
> +
> + while (dw_spi_dma_tx_busy(dws) && retry--)
> + spi_delay_exec(, NULL);
I've just discovered using spi_delay_exec() wasn't a good idea here. Look at the
call stack:
dw_dma_tasklet() -> dwc_scan_descriptors() -> dwc_descriptor_complete() ->
dw_spi_dma_tx_done() -> spi_delay_exec() -> usleep_range() -> ...
So tasklet is calling a sleeping function.((( I've absolutely forgotten to check
the context the DMA completion function is called with. We'll have to manually
select either ndelay or udelay here and nothing else. Since basically both
functions represent an atomic context and most of the platforms ndelay
fallsback to
udelay, I'll get the ndelay back to the wait functions. I'll resend a patchset
shortly.
-Sergey
Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
On Thu, May 21, 2020 at 10:55:20PM +0800, Feng Tang wrote:
> Hi Serge,
>
> On Thu, May 21, 2020 at 02:47:36PM +0300, Serge Semin wrote:
> > Hello Feng,
> >
> > On Thu, May 21, 2020 at 11:09:24AM +0800, Feng Tang wrote:
> > > Hi Serge,
> > >
> > > On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
> >
> > [nip]
> >
> > > > /*
> > > > * dws->dma_chan_busy is set before the dma transfer starts, callback
> > > > for rx
> > > > * channel will clear a corresponding bit.
> > > > @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
> > > > {
> > > > struct dw_spi *dws = arg;
> > > >
> > > > + dw_spi_dma_wait_rx_done(dws);
> > >
> > > I can understand the problem about TX, but I don't see how RX
> > > will get hurt, can you elaborate more? thanks
> > >
> > > - Feng
> >
> > Your question is correct. You are right with your hypothesis. Ideally upon
> > the
> > dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
> > commit log signifies the error being mostly related with Tx FIFO. But
> > practically there are many reasons why Rx FIFO might be left with data:
> > DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
> > messed something up), controller hanging up, and so on. It's better to catch
> > an error at this stage while propagating it up to the SPI device drivers.
> > Especially seeing the wait-check implementation doesn't gives us much of the
> > execution overhead in normal conditions. So by calling
> > dw_spi_dma_wait_rx_done()
> > we make sure that all the data has been fetched and we may freely get the
> > buffers back to the client driver.
>
> I see your point about checking RX. But I still don't think checking
> RX FIFO level is the right way to detect error. Some data left in
> RX FIFO doesn't always mean a error, say for some case if there is
> 20 words in RX FIFO, and the driver starts a DMA request for 16
> words, then after a sucessful DMA transaction, there are 4 words
> left without any error.
Neither Tx nor Rx FIFO should be left with any data after transaction is
finished. If they are then something has been wrong.
See, every SPI transfer starts with FIFO clearance since we disable/enable the
SPI controller by means of the SSIENR (spi_enable_chip(dws, 0) and
spi_enable_chip(dws, 1) called in the dw_spi_transfer_one() callback). Here is
the
SSIENR register description: "It enables and disables all SPI Controller
operations.
When disabled, all serial transfers are halted immediately. Transmit and receive
FIFO buffers are cleared when the device is disabled. It is impossible to
program
some of the SPI Controller control registers when enabled"
No mater whether we start DMA request or perform the normal IRQ-based PIO, we
request as much data as we need and neither Tx nor Rx FIFO are supposed to
be left with any data after the request is finished. If data is left, then
either we didn't push all of the necessary data to the SPI bus, or we didn't
pull all the data from the FIFO, and this could have happened only due to some
component mulfunction (drivers, DMA engine, SPI device). In any case the SPI
device driver should be notified about the problem.
-Sergey
>
> Thanks,
> Feng
>
> >
> > -Sergey
Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
Hi Serge,
On Thu, May 21, 2020 at 02:47:36PM +0300, Serge Semin wrote:
> Hello Feng,
>
> On Thu, May 21, 2020 at 11:09:24AM +0800, Feng Tang wrote:
> > Hi Serge,
> >
> > On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
>
> [nip]
>
> > > /*
> > > * dws->dma_chan_busy is set before the dma transfer starts, callback
> > > for rx
> > > * channel will clear a corresponding bit.
> > > @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
> > > {
> > > struct dw_spi *dws = arg;
> > >
> > > + dw_spi_dma_wait_rx_done(dws);
> >
> > I can understand the problem about TX, but I don't see how RX
> > will get hurt, can you elaborate more? thanks
> >
> > - Feng
>
> Your question is correct. You are right with your hypothesis. Ideally upon the
> dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
> commit log signifies the error being mostly related with Tx FIFO. But
> practically there are many reasons why Rx FIFO might be left with data:
> DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
> messed something up), controller hanging up, and so on. It's better to catch
> an error at this stage while propagating it up to the SPI device drivers.
> Especially seeing the wait-check implementation doesn't gives us much of the
> execution overhead in normal conditions. So by calling
> dw_spi_dma_wait_rx_done()
> we make sure that all the data has been fetched and we may freely get the
> buffers back to the client driver.
I see your point about checking RX. But I still don't think checking
RX FIFO level is the right way to detect error. Some data left in
RX FIFO doesn't always mean a error, say for some case if there is
20 words in RX FIFO, and the driver starts a DMA request for 16
words, then after a sucessful DMA transaction, there are 4 words
left without any error.
Thanks,
Feng
>
> -Sergey
Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
Hello Feng,
On Thu, May 21, 2020 at 11:09:24AM +0800, Feng Tang wrote:
> Hi Serge,
>
> On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
[nip]
> > /*
> > * dws->dma_chan_busy is set before the dma transfer starts, callback for
> > rx
> > * channel will clear a corresponding bit.
> > @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
> > {
> > struct dw_spi *dws = arg;
> >
> > + dw_spi_dma_wait_rx_done(dws);
>
> I can understand the problem about TX, but I don't see how RX
> will get hurt, can you elaborate more? thanks
>
> - Feng
Your question is correct. You are right with your hypothesis. Ideally upon the
dw_spi_dma_rx_done() execution Rx FIFO must be already empty. That's why the
commit log signifies the error being mostly related with Tx FIFO. But
practically there are many reasons why Rx FIFO might be left with data:
DMA engine failures, incorrect DMA configuration (if DW SPI or DW DMA driver
messed something up), controller hanging up, and so on. It's better to catch
an error at this stage while propagating it up to the SPI device drivers.
Especially seeing the wait-check implementation doesn't gives us much of the
execution overhead in normal conditions. So by calling dw_spi_dma_wait_rx_done()
we make sure that all the data has been fetched and we may freely get the
buffers back to the client driver.
-Sergey
Re: [PATCH v3 01/16] spi: dw: Add Tx/Rx finish wait methods to the MID DMA
Hi Serge,
On Thu, May 21, 2020 at 04:21:51AM +0300, Serge Semin wrote:
> Since DMA transfers are performed asynchronously with actual SPI
> transaction, then even if DMA transfers are finished it doesn't mean
> all data is actually pushed to the SPI bus. Some data might still be
> in the controller FIFO. This is specifically true for Tx-only
> transfers. In this case if the next SPI transfer is recharged while
> a tail of the previous one is still in FIFO, we'll loose that tail
> data. In order to fix this lets add the wait procedure of the Tx/Rx
> SPI transfers completion after the corresponding DMA transactions
> are finished.
>
> Co-developed-by: Georgy Vlasov
> Signed-off-by: Georgy Vlasov
> Signed-off-by: Serge Semin
> Fixes: 7063c0d942a1 ("spi/dw_spi: add DMA support")
> Cc: Ramil Zaripov
> Cc: Alexey Malahov
> Cc: Thomas Bogendoerfer
> Cc: Paul Burton
> Cc: Ralf Baechle
> Cc: Arnd Bergmann
> Cc: Andy Shevchenko
> Cc: Rob Herring
> Cc: linux-m...@vger.kernel.org
> Cc: devicet...@vger.kernel.org
>
> ---
>
> Changelog v2:
> - Use conditional statement instead of the ternary operator in the ref
> clock getter.
> - Move the patch to the head of the series so one could be picked up to
> the stable kernels as a fix.
>
> Changelog v3:
> - Use spi_delay_exec() method to wait for the current operation completion.
> ---
> drivers/spi/spi-dw-mid.c | 69
> drivers/spi/spi-dw.h | 10 ++
> 2 files changed, 79 insertions(+)
>
> diff --git a/drivers/spi/spi-dw-mid.c b/drivers/spi/spi-dw-mid.c
> index f9757a370699..3526b196a7fc 100644
> --- a/drivers/spi/spi-dw-mid.c
> +++ b/drivers/spi/spi-dw-mid.c
> @@ -17,6 +17,7 @@
> #include
> #include
>
> +#define WAIT_RETRIES 5
> #define RX_BUSY 0
> #define TX_BUSY 1
>
> @@ -143,6 +144,47 @@ static enum dma_slave_buswidth convert_dma_width(u32
> dma_width) {
> return DMA_SLAVE_BUSWIDTH_UNDEFINED;
> }
>
> +static void dw_spi_dma_calc_delay(struct dw_spi *dws, u32 nents,
> + struct spi_delay *delay)
> +{
> + unsigned long ns, us;
> +
> + ns = (NSEC_PER_SEC / spi_get_clk(dws)) * nents * dws->n_bytes *
> + BITS_PER_BYTE;
> +
> + if (ns <= NSEC_PER_USEC) {
> + delay->unit = SPI_DELAY_UNIT_NSECS;
> + delay->value = ns;
> + } else {
> + us = DIV_ROUND_UP(ns, NSEC_PER_USEC);
> + delay->unit = SPI_DELAY_UNIT_USECS;
> + delay->value = clamp_val(us, 0, USHRT_MAX);
> + }
> +}
> +
> +static inline bool dw_spi_dma_tx_busy(struct dw_spi *dws)
> +{
> + return !(dw_readl(dws, DW_SPI_SR) & SR_TF_EMPT);
> +}
> +
> +static void dw_spi_dma_wait_tx_done(struct dw_spi *dws)
> +{
> + int retry = WAIT_RETRIES;
> + struct spi_delay delay;
> + u32 nents;
> +
> + nents = dw_readl(dws, DW_SPI_TXFLR);
> + dw_spi_dma_calc_delay(dws, nents, );
> +
> + while (dw_spi_dma_tx_busy(dws) && retry--)
> + spi_delay_exec(, NULL);
> +
> + if (retry < 0) {
> + dev_err(>master->dev, "Tx hanged up\n");
> + dws->master->cur_msg->status = -EIO;
> + }
> +}
> +
> /*
> * dws->dma_chan_busy is set before the dma transfer starts, callback for tx
> * channel will clear a corresponding bit.
> @@ -151,6 +193,8 @@ static void dw_spi_dma_tx_done(void *arg)
> {
> struct dw_spi *dws = arg;
>
> + dw_spi_dma_wait_tx_done(dws);
> +
> clear_bit(TX_BUSY, >dma_chan_busy);
> if (test_bit(RX_BUSY, >dma_chan_busy))
> return;
> @@ -192,6 +236,29 @@ static struct dma_async_tx_descriptor
> *dw_spi_dma_prepare_tx(struct dw_spi *dws,
> return txdesc;
> }
>
> +static inline bool dw_spi_dma_rx_busy(struct dw_spi *dws)
> +{
> + return !!(dw_readl(dws, DW_SPI_SR) & SR_RF_NOT_EMPT);
> +}
> +
> +static void dw_spi_dma_wait_rx_done(struct dw_spi *dws)
> +{
> + int retry = WAIT_RETRIES;
> + struct spi_delay delay;
> + u32 nents;
> +
> + nents = dw_readl(dws, DW_SPI_RXFLR);
> + dw_spi_dma_calc_delay(dws, nents, );
> +
> + while (dw_spi_dma_rx_busy(dws) && retry--)
> + spi_delay_exec(, NULL);
> +
> + if (retry < 0) {
> + dev_err(>master->dev, "Rx hanged up\n");
> + dws->master->cur_msg->status = -EIO;
> + }
> +}
> +
> /*
> * dws->dma_chan_busy is set before the dma transfer starts, callback for rx
> * channel will clear a corresponding bit.
> @@ -200,6 +267,8 @@ static void dw_spi_dma_rx_done(void *arg)
> {
> struct dw_spi *dws = arg;
>
> + dw_spi_dma_wait_rx_done(dws);
I can understand the problem about TX, but I don't see how RX
will get hurt, can you elaborate more? thanks
- Feng
> +
> clear_bit(RX_BUSY, >dma_chan_busy);
> if (test_bit(TX_BUSY, >dma_chan_busy))
> return;
> diff --git a/drivers/spi/spi-dw.h b/drivers/spi/spi-dw.h
> index e92d43b9a9e6..81364f501b7e 100644
> ---
