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

[Stefan Mavrodiev]

On 12/14/18 5:55 PM, Maxime Ripard wrote:
> On Fri, Dec 14, 2018 at 04:14:31PM +0200, Stefan Mavrodiev wrote:
>> On 12/14/18 11:25 AM, Maxime Ripard wrote:
>>> On Thu, Dec 13, 2018 at 09:12:57AM +0200, Stefan Mavrodiev wrote:
>>>> On 12/6/18 8:41 AM, Stefan Mavrodiev wrote:
>>>>> On 12/5/18 5:46 PM, Maxime Ripard wrote:
>>>>>> On Wed, Dec 05, 2018 at 02:27:57PM +0200, 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 <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;
>>>>>> It seems pretty inefficient to test the pointer at each access, it
>>>>>> would be better to check it once before starting the transfer.
>>>>>>
>>>>>> I'm not sure if that can even happen?
>>>>> I've tried to check that before draining the receive fifo, but
>>>>> then the controller doesn't work. I'm thinking that the fifo must
>>>>> be drained in any case.
>>>> Any further comments?
>>> I was expecting you to comment on whether the FIFO needed to be
>>> drained or not :)
>> Sorry. I didn't understand that.
>>
>> Anyway. After some code checking, I found that the FIFO needs to be drained
>> because TP_EN (Transmit Pause Enable) bit is set during bus claim.
>>
>> "....
>>
>> In master mode, it is used to control transmit state machine to
>> stop smart burst sending when RX FIFO is full.
>>
>> ..."
>>
>> Perhaps this bit should be enabled only when we want to read back data?
> It's been a while since I last looked at the spi driver. What is linux
> doing?

In the kernel version there is the same check, like the one in the patch:

drivers/spi/spi-sun4i.c:
static inline void sun4i_spi_drain_fifo(struct sun4i_spi *sspi, int len)
......

while (len--) {
     byte = readb(sspi->base_addr + SUN4I_RXDATA_REG);
     if (sspi->rx_buf)
         *sspi->rx_buf++ = byte;
}
.....

Guess I've missed this check when I was adopting the driver for u-boot.


>
> Maxime
>