[U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check

[Jagan Teki]

On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <stefan at olimex.com> wrote:
>
>
> On 12/20/18 12:56 PM, Jagan Teki wrote:
> > On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev <stefan at olimex.com> wrote:
> >>
> >> On 12/20/18 12:45 PM, Jagan Teki wrote:
> >>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev <stefan at olimex.com> wrote:
> >>>> On 12/20/18 12:14 PM, Jagan Teki wrote:
> >>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki <jagan at amarulasolutions.com> wrote:
> >>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan at 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 <stefan at 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 <jagan at 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;