Re: [PATCH] dmaengine: at_hdmac: new driver for the Atmel AHB DMAController

[Nicolas Ferre]

Hi Dan,

Thanks a lot for your feedback.

Dan Williams :
> On Fri, 2008-10-17 at 08:43 -0700, Nicolas Ferre wrote:
> > This AHB DMA Controller (aka HDMA or DMAC on AT91 systems) is availlable on
> > at91sam9rl chip. It will be used on other products in the future.
> >
> > This first release covers only the memory-to-memory tranfer type. This is the
> > only tranfer type supported by this chip.
> > On other products, it will be used also for peripheral DMA transfer (slave API
> > support to come).
> >
> > I used dmatest client without problem in different configurations to test
> > it.
> >
> > Full documentation for this controller can be found in the SAM9RL datasheet :
> > http://www.atmel.com/dyn/products/product_card.asp?part_id=4243
> >
> > Signed-off-by: Nicolas Ferre <nicolas.ferre@xxxxxxxxx>
> > ---
>
> Hi Nicolas,
>
> A few comments below.
>
> Also, checkpatch reported:
>
> total: 4 errors, 45 warnings, 1475 lines checked
>
> ...mostly 80 column warnings (some you may want to take a look at).

I reviewed this and manage to reduce most of 80 column warnings. An 
error remains but it is a space in "if" statement and it is for 
alignment purpose.


> Regards,
> Dan
>
> >  arch/arm/mach-at91/at91sam9rl_devices.c |   47 ++
> >  drivers/dma/Kconfig                     |    8 +
> >  drivers/dma/Makefile                    |    1 +
> >  drivers/dma/at_hdmac.c                  |  989 +++++++++++++++++++++++++++++++
> >  drivers/dma/at_hdmac_regs.h             |  377 ++++++++++++
> >  include/linux/at_hdmac.h                |   26 +
>
> ...this header should be moved somewhere under arch/arm/include.

This is where dw_dmac.h resides. Moreover, if one day this IP is 
implemented on a different architecture, it will be good not to reach it 
through arch/arm path.

[..]

> > +/**
> > + * atc_alloc_descriptor - allocate and return an initilized descriptor
> > + * @chan: the channel to allocate descriptors for
> > + * @gfp_flags: GFP allocation flags
> > + */
> > +static struct at_desc *atc_alloc_descriptor(struct dma_chan *chan,
> > +                                           gfp_t gfp_flags)
> > +{
> > +       struct at_desc  *desc = NULL;
> > +       struct at_dma   *atdma = to_at_dma(chan->device);
> > +       dma_addr_t phys;
> > +
> > +       desc = dma_pool_alloc(atdma->dma_desc_pool, gfp_flags, &phys);
> > +       if (desc) {
> > +               BUG_ON(phys & 0x3UL); /* descriptors have to be word aligned */
>
> hmm, yes this is a bug but can't we trust that dma_pool_alloc does its
> job correctly?

Indeed, it was mainly for debugging purpose, I remove it.


> > +               memset(desc, 0, sizeof(struct at_desc));
> > +               dma_async_tx_descriptor_init(&desc->txd, chan);
> > +               async_tx_ack(&desc->txd);
>
> the DMA_CTRL_ACK bit is under control of the client.  It should be
> read-only to the driver (except for extra descriptors that the driver
> creates on behalf of the client).

This is precisely where the descriptors are been created so, I thought 
it should be ok to initialize this bit. Am I right ?

[..]

> > +static irqreturn_t at_dma_interrupt(int irq, void *dev_id)
> > +{
> > +       struct at_dma           *atdma = (struct at_dma *)dev_id;
> > +       struct at_dma_chan      *atchan;
> > +       int                     i;
> > +       u32                     status, pending, imr;
> > +       int                     ret = IRQ_NONE;
> > +
> > +       do {
> > +               imr = dma_readl(atdma, EBCIMR);
> > +               status = dma_readl(atdma, EBCISR);
> > +               pending = status & imr;
> > +
> > +               if (!pending)
> > +                       break;
> > +
> > +               dev_vdbg(atdma->dma_common.dev,
> > +                       "interrupt: status = 0x%08x, 0x%08x, 0x%08x\n",
> > +                        status, imr, pending);
> > +
> > +               for (i = 0; i < atdma->dma_common.chancnt; i++) {
> > +                       atchan = &atdma->chan[i];
> > +                       if (pending & (AT_DMA_CBTC(i) | AT_DMA_ERR(i))) {
> > +                               if (pending & AT_DMA_ERR(i)) {
> > +                                       /*
> > +                                       spin_lock(atchan->lock);
> > +                                       atchan->error_status = 1;
> > +                                       spin_unlock(atchan->lock);
>
> writing to an unsigned long should already be atomic, no?

On ARM yes, on other architectures, I do not know...
Anyway, I removed those commented lines.

[..]

> > +/**
> > + * atc_alloc_chan_resources - allocate resources for DMA channel
> > + * @chan: allocate descriptor resources for this channel
> > + * @client: current client requesting the channel be ready for requests
> > + *
> > + * return - the number of allocated descriptors
> > + */
> > +static int atc_alloc_chan_resources(struct dma_chan *chan,
> > +                                   struct dma_client *client)
> > +{
> > +       struct at_dma_chan      *atchan = to_at_dma_chan(chan);
> > +       struct at_dma           *atdma = to_at_dma(chan->device);
> > +       struct at_desc          *desc;
> > +       int                     i;
> > +       LIST_HEAD(tmp_list);
> > +
> > +       dev_vdbg(&chan->dev, "alloc_chan_resources\n");
> > +

[TAG]

> > +       /* ASSERT:  channel is idle */
> > +       if (atc_chan_is_enabled(atchan)) {
> > +               dev_dbg(&chan->dev, "DMA channel not idle ?\n");
> > +               return -EIO;
> > +       }

[/TAG]

> > +
> > +       /* have we already been set up? */
> > +       if (!list_empty(&atchan->free_list))
> > +               return atchan->descs_allocated;
> > +
> > +       /* Allocate initial pool of descriptors */
> > +       for (i = 0; i < INIT_NR_DESCS_PER_CHANNEL; i++) {
> > +               desc = atc_alloc_descriptor(chan, GFP_KERNEL);
> > +               if (!desc) {
> > +                       dev_err(atdma->dma_common.dev,
> > +                               "Only %d initial descriptors\n", i);
> > +                       break;
> > +               }
> > +               list_add_tail(&desc->desc_node, &tmp_list);
> > +       }
> > +
> > +       spin_lock_bh(&atchan->lock);
> > +       atchan->descs_allocated = i;
> > +       list_splice(&tmp_list, &atchan->free_list);
> > +       atchan->completed_cookie = chan->cookie = 1;
> > +       spin_unlock_bh(&atchan->lock);
> > +
> > +       /* channel parameters */
> > +       channel_writel(atchan, CFG, ATC_DEFAULT_CFG);
> > +
> > +       tasklet_init(&atchan->tasklet, atc_tasklet, (unsigned long)atchan);
>
> This routine may be called while the channel is already active,
> potentially causing tasklet_init() to be called while a tasklet is
> pending.  Can this move to at_dma_probe()?

Oh, really ? In [TAG] above, I protect the call of this function when 
channel is enabled. Is the code at [TAG] ok ?

Ok, so I move all this.

> > +       /* clear any pending interrupt */
> > +       while (dma_readl(atdma, EBCISR))
> > +               cpu_relax();
> > +       atc_enable_irq(atchan);
>
> ditto.

Ok.


I will regenerate a new patch as soon as you acknowledge my comments.

Thanks for your help, kind regards,
--
Nicolas Ferre

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/