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

Jagan Teki jagan at amarulasolutions.com
Thu Dec 20 12:44:22 UTC 2018


On Thu, Dec 20, 2018 at 6:12 PM Stefan Mavrodiev <stefan at 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 at 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 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;
> >> 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?


More information about the U-Boot mailing list