On Thu, Dec 20, 2018 at 5:38 PM Stefan Mavrodiev <ste...@olimex.com> wrote: > > > On 12/20/18 1:54 PM, Jagan Teki wrote: > > On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <ste...@olimex.com> wrote: > >> > >> On 12/20/18 12:56 PM, Jagan Teki wrote: > >>> On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <ste...@olimex.com> > >>> wrote: > >>>> On 12/20/18 12:45 PM, Jagan Teki wrote: > >>>>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <ste...@olimex.com> > >>>>> wrote: > >>>>>> On 12/20/18 12:14 PM, Jagan Teki wrote: > >>>>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki > >>>>>>> <ja...@amarulasolutions.com> wrote: > >>>>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <ste...@olimex.com> > >>>>>>>> wrote: > >>>>>>>>> Current driver doesn't check if the destination pointer is NULL. > >>>>>>>>> This cause the data from the FIFO to be stored inside the internal > >>>>>>>>> SDRAM ( address 0 ). > >>>>>>>>> > >>>>>>>>> The patch add simple check if the destination pointer is NULL. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Stefan Mavrodiev <ste...@olimex.com> > >>>>>>>>> --- > >>>>>>>>> drivers/spi/sun4i_spi.c | 3 ++- > >>>>>>>>> 1 file changed, 2 insertions(+), 1 deletion(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/spi/sun4i_spi.c b/drivers/spi/sun4i_spi.c > >>>>>>>>> index b86b5a00ad..38cc743c61 100644 > >>>>>>>>> --- a/drivers/spi/sun4i_spi.c > >>>>>>>>> +++ b/drivers/spi/sun4i_spi.c > >>>>>>>>> @@ -129,7 +129,8 @@ static inline void sun4i_spi_drain_fifo(struct > >>>>>>>>> sun4i_spi_priv *priv, int len) > >>>>>>>>> > >>>>>>>>> while (len--) { > >>>>>>>>> byte = readb(&priv->regs->rxdata); > >>>>>>>>> - *priv->rx_buf++ = byte; > >>>>>>>>> + if (priv->rx_buf) > >>>>>>>>> + *priv->rx_buf++ = byte; > >>>>>>>> Acked-by: Jagan Teki <ja...@openedev.com> > >>>>>>>> > >>>>>>>> But, have you tested how much data in the fifo before drained? It's > >>>>>>>> better we can get the available data before reading via fifo_sta > >>>>>> I don't understand what's the point of doing this? > >>>>> Didn't get? don't you understand what I'm saying or it not require > >>>>> from point of you? > >>>> Maybe I don't understand correctly what you're saying. > >>> You comment now and previous mail doesn't match. better be specific. > >>> > >>> For draining fifo. > >>> We can find how much data available before reading from fifo and > >>> assign to local rx. > >>> > >>> static inline void sun4i_spi_drain_fifo(struct sun4i_spi_priv *priv, int > >>> len) > >>> { > >>> u32 reg, cnt > >>> u8 byte; > >>> > >>> /* See how much data is available */ > >>> reg = readl(&priv->regs->fifo_sta); > >>> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > >>> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > >>> > >>> if (len > cnt) > >>> len = cnt; > >>> > >>> while (len--) { > >>> byte = readb(&priv->regs->rxdata); > >>> *priv->rx_buf++ = byte; > >>> } > >>> } > >>> > >>> This can be perfect drain fifo, and this is what I'm trying to test > >>> you with existing code and after your patch and verify whether all the > >>> data perfectly drain or not before and after. > >>> > >>> Hope you understand, this time. > >> > >> Hope I understood this time... > >> > >> > >> I've made some modification to sun4i_spi_xfer: > >> > >> static int sun4i_spi_xfer(struct udevice *dev, unsigned int bitlen, > >> const void *dout, void *din, unsigned long flags) > >> { > >> struct udevice *bus = dev->parent; > >> struct sun4i_spi_priv *priv = dev_get_priv(bus); > >> struct dm_spi_slave_platdata *slave_plat = > >> dev_get_parent_platdata(dev); > >> > >> u32 len = bitlen / 8; > >> u32 reg, cnt; > >> u8 nbytes; > >> int ret; > >> > >> priv->tx_buf = dout; > >> priv->rx_buf = din; > >> > >> if (bitlen % 8) { > >> debug("%s: non byte-aligned SPI transfer.\n", __func__); > >> return -ENAVAIL; > >> } > >> > >> if (flags & SPI_XFER_BEGIN) > >> sun4i_spi_set_cs(bus, slave_plat->cs, true); > >> > >> reg = readl(&priv->regs->ctl); > >> > >> /* Reset FIFOs */ > >> writel(reg | SUN4I_CTL_RF_RST | SUN4I_CTL_TF_RST, &priv->regs->ctl); > >> > >> while (len) { > >> /* Setup the transfer now... */ > >> nbytes = min(len, (u32)(SUN4I_FIFO_DEPTH - 1)); > >> > >> if (!priv->rx_buf) > >> printf("%s: Sending %d bytes, ", __func__, nbytes); > >> > >> /* Setup the counters */ > >> writel(SUN4I_BURST_CNT(nbytes), &priv->regs->bc); > >> writel(SUN4I_XMIT_CNT(nbytes), &priv->regs->tc); > >> > >> /* Fill the TX FIFO */ > >> sun4i_spi_fill_fifo(priv, nbytes); > >> > >> /* Start the transfer */ > >> reg = readl(&priv->regs->ctl); > >> writel(reg | SUN4I_CTL_XCH, &priv->regs->ctl); > >> > >> /* Wait transfer to complete */ > >> ret = wait_for_bit_le32(&priv->regs->ctl, SUN4I_CTL_XCH_MASK, > >> false, SUN4I_SPI_TIMEOUT_US, false); > >> if (ret) { > >> printf("ERROR: sun4i_spi: Timeout transferring data\n"); > >> sun4i_spi_set_cs(bus, slave_plat->cs, false); > >> return ret; > >> } > >> > >> /* Drain the RX FIFO */ > >> if (!priv->rx_buf) { > >> reg = readl(&priv->regs->fifo_sta); > >> reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > >> cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > >> printf("rx fifo: before: %d, ", cnt); > >> } > >> > >> sun4i_spi_drain_fifo(priv, nbytes); > > So it's draining fifo without need to checking the available count > > insun4i_spi_drain_fifo(), I mean the below code not require. > > > > /* See how much data is available */ > > reg = readl(&priv->regs->fifo_sta); > > reg &= SUN4I_FIFO_STA_RF_CNT_MASK; > > cnt = reg >> SUN4I_FIFO_STA_RF_CNT_BITS; > > > > if (len > cnt) > > len = cnt; > > Yes, I didn't made any modification to sun4i_spi_drain_fifo() function:
OK, thanks for the details and test. Please send the v2 with proper commit head it should be "spi: sun4i: ..." Also I didn't find this patch on patchwork, don't know why? _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot