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

[Stefan Mavrodiev]

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);

         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("after: %d\n", cnt);
         }

         len -= nbytes;
     }

     if (flags & SPI_XFER_END)
         sun4i_spi_set_cs(bus, slave_plat->cs, false);

     return 0;
}

This will print only when destination buffer is NULL. This gave me the 
following output:

sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0
sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0
sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0
sun4i_spi_xfer: Sending 4 bytes, rx fifo: before: 4, after: 0
sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0
sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0
sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0
sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0
sun4i_spi_xfer: Sending 4 bytes, rx fifo: before: 4, after: 0
sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0
sun4i_spi_xfer: Sending 1 bytes, rx fifo: before: 1, after: 0
sun4i_spi_xfer: Sending 4 bytes, rx fifo: before: 4, after: 0
sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0
sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0
sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0
sun4i_spi_xfer: Sending 63 bytes, rx fifo: before: 63, after: 0
.... etc

As you can see the fifo is drained.