[U-Boot] [PATCH 1/1] arm: sunxi: Add NULL pointer check
Stefan Mavrodiev
stefan at olimex.com
Thu Dec 20 12:41:57 UTC 2018
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/
More information about the U-Boot
mailing list