[U-Boot] [PATCH v5 1/5] dma: lpc32xx: add DMA driver

[Vladimir Zapolskiy]

Hi Sylvain,

On 06.08.2015 16:50, LEMIEUX, SYLVAIN wrote:
> Hi Vladimir,
> 
> Thanks for the feedback;
> 
> Marek, Vladimir,
> 
> there is a question below; I will wait for feedback before sending an updated version of the patch.
> 
> 
> Sylvain
> 
>> -----Original Message-----
>> From: Vladimir Zapolskiy [mailto:vz at mleia.com]
>>
>> Hi Sylvain,
>>
>> On 05.08.2015 21:31, slemieux.tyco at gmail.com wrote:
>>> From: Sylvain Lemieux <slemieux at tycoint.com>
>>>
>>> Incorporate DMA driver from legacy LPCLinux NXP BSP.
>>> The files taken from the legacy patch are:
>>> - lpc32xx DMA driver
>>> - lpc3250 header file DMA registers definition.
>>>
>>> The legacy driver was updated and clean-up as part of the integration with the latest u-boot.
>>>
>>> Signed-off-by: Sylvain Lemieux <slemieux at tycoint.com>
>>> ---
> 
> [...]
> 
>>>
>>> diff --git a/arch/arm/include/asm/arch-lpc32xx/dma.h b/arch/arm/include/asm/arch-lpc32xx/dma.h
>>> new file mode 100644
>>> index 0000000..15d829c
>>> --- /dev/null
>>> +++ b/arch/arm/include/asm/arch-lpc32xx/dma.h
>>> @@ -0,0 +1,37 @@
>>> +/*
>>> + * LPC32xx DMA Controller Interface
>>> + *
> 
> [...]
> 
>>> +
>>> +int lpc32xx_dma_get_channel(void);
>>> +int lpc32xx_dma_start_xfer(int channel, const struct lpc32xx_dmac_ll *desc,
>>> +                      u32 config);
>>> +int lpc32xx_dma_wait_status(int channel);
>>> +void lpc32xx_dma_put_channel(int channel);
>>
>> There is no users of lpc32xx_dma_put_channel() interface, do you have
>> them in mind?
>>
> The legacy NXP BSP driver was providing the support the get and release a channel;
> I kept it there, knowing is currently not used.
> 
> Do we want to keep this feature or remove it and only allowed channel allocation only?
> 
> [...]

I have no definite answer, in my opinion dead code should be removed, if
its use is not planned. I would recommend to get the answer from a
maintainer, who accepts this code (Tom Rini ?).

>>> +++ b/drivers/dma/lpc32xx_dma.c
>>> @@ -0,0 +1,163 @@
>>> +/*
> 
> 
> [...]
> 
>>> +
>>> +int lpc32xx_dma_start_xfer(int channel, const struct lpc32xx_dmac_ll *desc,
>>> +                      u32 config)
>>> +{
>>> +   if (unlikely((BIT_MASK(channel) & alloc_ch) == 0)) {
>>
>> Would it be possible to change "int channel" to some unsigned type?
>>
>> If the intention is to reserve (channel < 0) for potentially
>> invalid/unallocated channel, as it is done in NAND SLC, please add a
>> check for this case here.
> 
> This was the legacy NXP BSP original behavior.
> 
> I will change the channel to "unsigned int" for the transfer API;
> the reserve API will continue to be return the channel as "int", to allowed validation;

Generally there is no need to preserve any correspondence between
mainlined code and code found in legacy BSP.

> the NAND SLC will keep the channel number as an "unsigned int".

Okay, in this case the correctness of the passed argument is offloaded
to a client. Fortunately the sole client NAND SLC does not pass -1 here
(the statement is based on code review).

>>
>>> +           printf("ERR: Request for xfer on unallocated channel %d\r\n",
>>
> [...]
>>
>>> +                  channel);
>>> +           BUG();
>>
>> The function returns int type, but in fact it returns only 0, either
>> this function return type can be changed to void, or IMO better to
>> return error here instead of fatal BUG() and add a check on client side.
> 
> I will modify it to return -1 on error and check on client side;
> The client side will generate the fatal BUG().
> 

Sounds good, I can imagine a potential client, which may gracefully
handle this error, but NAND drivers are not in the list. If problem is
encountered here, it means that the problem is on the client side and it
is fair enough to place BUG() right on that client side.

--
With best wishes,
Vladimir