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

[LEMIEUX, SYLVAIN]

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?

[...]

> > +++ 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;
the NAND SLC will keep the channel number as an "unsigned int".

>
> > +           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().

>

[...]

>
> --
> With best wishes,
> Vladimir

________________________________

This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.