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

[Stefan Mavrodiev <stefan@olimex.com>]

On 12/20/18 2:44 PM, Jagan Teki wrote:
> On Thu, Dec 20, 2018 at 6:12 PM Stefan Mavrodiev <stefan@olimex.com> wrote:
>>
>> On 12/20/18 2:38 PM, Jagan Teki wrote:
>>> On Thu, Dec 20, 2018 at 5:38 PM Stefan Mavrodiev <stefan@olimex.com> wrote:
>>>> On 12/20/18 1:54 PM, Jagan Teki wrote:
>>>>> On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev <stefan@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@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@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@amarulasolutions.com> wrote:
>>>>>>>>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev <stefan@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@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@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?
>> No idea. I can see it here:
>> https://patchwork.ozlabs.org/patch/1008181/
> OK, got it. I will format the commit and apply if are OK?
Sure. Thank you.