Hi Franklin, > So the only time your missing the last byte is when a timeout occurs? Yes, the bit OMAP2_MCSPI_CHSTAT_RXS was not always set. So the received telegram was not completed --> without last byte. This was the case with very low baudrate (1465 bit/s). I hadn't any problem with other SPI device ( with 1,7 Mbit/s). Regards, Walter -----Original Message----- From: Franklin S Cooper Jr [mailto:fcooper@xxxxxx] Sent: Freitag, 10. Februar 2017 21:39 To: Tony Lindgren <tony@xxxxxxxxxxx>; Walter Wagner <walter.wagner@xxxxxxxxxx> Cc: linux-omap@xxxxxxxxxxxxxxx; Peter Ujfalusi <peter.ujfalusi@xxxxxx>; Michael Welling <mwelling@xxxxxxxx> Subject: Re: TI AM33x/AM43x SPI DRIVER Hi Walter On 02/09/2017 04:59 PM, Tony Lindgren wrote: > Hi, > > * Walter Wagner <walter.wagner@xxxxxxxxxx> [170209 00:40]: >> Dear Tony Lindgren, >> >> I hope I'm right here and You are responsible for drivers/spi/spi-omap2-mcspi.c too. > > Well I don't know much about the spi-omap2-mcspi.c driver.. But as > Franklin, Peter and Micheal have been patching it, they might have a > better idea what's going on. I've added them to Cc. > >> To the history: >> >> We use TI AM33xx/AM43xx SOCs, especially SPI interface under kernel 4.1.20. >> Testing a device with very low baudrate (1,5 KHz) I made following experience: >> >> 1. The last byte was not always sure received --> sometimes I got timeout So the only time your missing the last byte is when a timeout occurs? >> >> 2. The telegrams we have contain < 160 bytes, but we need DMA mode and not PIO (because CPU load) >> >> Therefore I've changed the driver. >> I hope these changes help other developer. >> >> I've attached the patch and the files itself for simple comparing. > > Please check Documentation/SubmittingPatches and CodingStyle etc, also > note that with git you can easily generate patches :) > > I've attached your diff below for easier reading. > > Regards, > > Tony > > 8< -------------------- > --- a/drivers/spi/spi-omap2-mcspi.c 2016-03-17 19:11:03.000000000 +0100 > +++ b/drivers/spi/spi-omap2-mcspi.c 2017-02-06 10:35:02.518061500 +0100 > @@ -116,6 +116,11 @@ > */ > #define DMA_MIN_BYTES 160 > > +/* Default value of delay at waiting for a bit in a register. > + * Needed to avoid latency at waiting. > + */ > +#define BUSY_WAIT_DELAY 1 /* in ms */ > + > > /* > * Used for context save and restore, structure members to be updated > whenever @@ -138,6 +143,8 @@ > struct omap2_mcspi_regs ctx; > int fifo_depth; > unsigned int pin_dir:1; > + unsigned long dma_min_bytes; > + unsigned long busy_wait_delay; Can't you calculate a value for this based on the SPI transaction speed instead of making this user select able? > }; > > struct omap2_mcspi_cs { > @@ -363,6 +370,23 @@ > return 0; > } > > +static int mcspi_wait_for_reg_bit_interruptible(void __iomem *reg, > +unsigned long bit, unsigned long delay) { > + unsigned long timeout; > + > + timeout = jiffies + msecs_to_jiffies(1000); > + while (!(readl_relaxed(reg) & bit)) { > + if (time_after(jiffies, timeout)) { > + if (!(readl_relaxed(reg) & bit)) > + return -ETIMEDOUT; > + else > + return 0; > + } > + msleep_interruptible(delay); > + } > + return 0; > +} > + > static void omap2_mcspi_rx_callback(void *data) { > struct spi_device *spi = data; > @@ -496,8 +520,9 @@ > if (l & OMAP2_MCSPI_CHCONF_TURBO) { > elements--; > > - if (likely(mcspi_read_cs_reg(spi, OMAP2_MCSPI_CHSTAT0) > - & OMAP2_MCSPI_CHSTAT_RXS)) { > + if (mcspi_wait_for_reg_bit_interruptible(cs->base + OMAP2_MCSPI_CHSTAT0, > + OMAP2_MCSPI_CHSTAT_RXS, mcspi->busy_wait_delay) == 0) { > + /* reading completed */ > u32 w; > > w = mcspi_read_cs_reg(spi, OMAP2_MCSPI_RX0); @@ -508,6 +533,7 @@ > else /* word_len <= 32 */ > ((u32 *)xfer->rx_buf)[elements++] = w; > } else { > + /* timeout */ > int bytes_per_word = mcspi_bytes_per_word(word_len); > dev_err(&spi->dev, "DMA RX penultimate word empty\n"); > count -= (bytes_per_word << 1); > @@ -515,8 +541,9 @@ > return count; > } > } > - if (likely(q(spi, OMAP2_MCSPI_CHSTAT0) > - & OMAP2_MCSPI_CHSTAT_RXS)) { > + if (mcspi_wait_for_reg_bit_interruptible(cs->base + OMAP2_MCSPI_CHSTAT0, > + OMAP2_MCSPI_CHSTAT_RXS, mcspi->busy_wait_delay) == 0) { > + /* reading completed */ > u32 w; > > w = mcspi_read_cs_reg(spi, OMAP2_MCSPI_RX0); @@ -527,6 +554,7 @@ > else /* word_len <= 32 */ > ((u32 *)xfer->rx_buf)[elements] = w; > } else { > + /* timeout */ > dev_err(&spi->dev, "DMA RX last word empty\n"); > count -= mcspi_bytes_per_word(word_len); > } > @@ -1141,7 +1169,7 @@ > unsigned count; > > if ((mcspi_dma->dma_rx && mcspi_dma->dma_tx) && > - (m->is_dma_mapped || t->len >= DMA_MIN_BYTES)) > + (m->is_dma_mapped || t->len >= mcspi->dma_min_bytes)) > omap2_mcspi_set_fifo(spi, t, 1); > > omap2_mcspi_set_enable(spi, 1); > @@ -1152,7 +1180,7 @@ > + OMAP2_MCSPI_TX0); > > if ((mcspi_dma->dma_rx && mcspi_dma->dma_tx) && > - (m->is_dma_mapped || t->len >= DMA_MIN_BYTES)) > + (m->is_dma_mapped || t->len >= mcspi->dma_min_bytes)) > count = omap2_mcspi_txrx_dma(spi, t); > else > count = omap2_mcspi_txrx_pio(spi, t); @@ -1234,7 +1262,7 @@ > goto out; > } > > - if (m->is_dma_mapped || len < DMA_MIN_BYTES) > + if (m->is_dma_mapped || len < mcspi->dma_min_bytes) > continue; > > if (mcspi_dma->dma_tx && tx_buf != NULL) { @@ -1375,6 +1403,11 @@ > master->bus_num = pdev->id; > mcspi->pin_dir = pdata->pin_dir; > } > + mcspi->dma_min_bytes = DMA_MIN_BYTES; /* default value */ > + of_property_read_u32(node, "ti,dma-min-bytes", &mcspi->dma_min_bytes); > + mcspi->busy_wait_delay = BUSY_WAIT_DELAY; /* default value */ > + of_property_read_u32(node, "ti,busy-wait-delay", > +&mcspi->busy_wait_delay); > + > regs_offset = pdata->regs_offset; > > r = platform_get_resource(pdev, IORESOURCE_MEM, 0); > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
