From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jagan Teki Date: Thu, 20 Dec 2018 17:24:32 +0530 Subject: [U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check In-Reply-To: <14480b84-da66-885b-38a0-6f414719ee01@olimex.com> References: <20181205122757.14523-1-stefan@olimex.com> <18e25666-911a-8dc6-f8f5-223f1761eafa@olimex.com> <14480b84-da66-885b-38a0-6f414719ee01@olimex.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de On Thu, Dec 20, 2018 at 4:49 PM Stefan Mavrodiev wrote: > > > On 12/20/18 12:56 PM, Jagan Teki wrote: > > On Thu, Dec 20, 2018 at 4:18 PM Stefan Mavrodiev wrote: > >> > >> On 12/20/18 12:45 PM, Jagan Teki wrote: > >>> On Thu, Dec 20, 2018 at 3:59 PM Stefan Mavrodiev wrote: > >>>> On 12/20/18 12:14 PM, Jagan Teki wrote: > >>>>> On Fri, Dec 14, 2018 at 3:48 PM Jagan Teki wrote: > >>>>>> On Wed, Dec 5, 2018 at 5:58 PM Stefan Mavrodiev 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 > >>>>>>> --- > >>>>>>> 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 > >>>>>> > >>>>>> 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;