On Fri, 4 Jul 2008 16:33:53 +0100
"Sosnowski, Maciej" <maciej.sosnowski@intel.com> wrote:
quoted text
> Coulpe of questions and comments from my side below.
> Apart from that the code looks fine to me.
> 
> Acked-by: Maciej Sosnowski <maciej.sosnowski@intel.com>

Thanks a lot for reviewing!

quoted text
> > +/* Called with dwc->lock held and bh disabled */
> > +static void dwc_dostart(struct dw_dma_chan *dwc, struct dw_desc
> *first)
> > +{
> > +       struct dw_dma   *dw = to_dw_dma(dwc->chan.device);
> > +
> > +       /* ASSERT:  channel is idle */
> > +       if (dma_readl(dw, CH_EN) & dwc->mask) {
> > +               dev_err(&dwc->chan.dev,
> > +                       "BUG: Attempted to start non-idle channel\n");
> > +               dev_err(&dwc->chan.dev,
> > +                       "  SAR: 0x%x DAR: 0x%x LLP: 0x%x CTL:
> 0x%x:%08x\n",
> > +                       channel_readl(dwc, SAR),
> > +                       channel_readl(dwc, DAR),
> > +                       channel_readl(dwc, LLP),
> > +                       channel_readl(dwc, CTL_HI),
> > +                       channel_readl(dwc, CTL_LO));
> > +
> > +               /* The tasklet will hopefully advance the queue... */
> > +               return;
> 
> Should not at this point an error status be returned 
> so that it can be handled accordingly by dwc_dostart() caller?

There's not a whole lot of meaningful things to do for the caller. It
should never happen in the first place, but if the channel _is_ active
at this point, we will eventually get an xfer complete interrupt when
the currently pending transfers are done. The descriptors have already
been added to the list, so the driver should recover from this kind of
bug automatically.

I've never actually triggered this code, so I can't really say for
certain that it works, but at least in theory it makes much more sense
to fix things up when the channel eventually becomes idle.

quoted text
> > +       ctllo = DWC_DEFAULT_CTLLO
> > +                       | DWC_CTLL_DST_WIDTH(dst_width)
> > +                       | DWC_CTLL_SRC_WIDTH(src_width)
> > +                       | DWC_CTLL_DST_INC
> > +                       | DWC_CTLL_SRC_INC
> > +                       | DWC_CTLL_FC_M2M;
> > +       prev = first = NULL;
> > +
> > +       for (offset = 0; offset < len; offset += xfer_count <<
> src_width) {
> > +               xfer_count = min_t(size_t, (len - offset) >>
> src_width,
> > +                               DWC_MAX_COUNT);
> 
> Here it looks like the maximum xfer_count value can change - it depends
> on src_width, 
> so it may be different for different transactions.
> Is that ok?

Yes, the maximum tranfer count is defined as the maximum number of
source transactions on the bus. So if the controller is set up to do 32
bits at a time on the source side, the maximum transfer _length_ is
four times the maximum transfer _count_.

The value written to the descriptor is also a transaction count, not a
byte count.

quoted text
> This driver does not perform any self-test during initialization.
> What about adding some initial HW checking?

I'm not sure if it makes a lot of sense -- this device is typically
integrated on the same silicon as the CPU, so if there are any issues
with the DMA controller, they should be caught during production
testing.

I'm using the dmatest module for validating the driver, so I feel the
self-test stuff becomes somewhat redundant.

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