Message-ID: <438BB0150E931F4B9CE701519A44630104A547AD6D@bgsmsx502.gar.corp.intel.com>
Date:	Wed, 28 Jul 2010 13:20:37 +0530
From:	"Koul, Vinod" <vinod.koul@...el.com>
To:	"Williams, Dan J" <dan.j.williams@...el.com>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linus.ml.walleij@...il.com" <linus.ml.walleij@...il.com>,
	Alan Cox <alan@...ux.intel.com>
Subject: RE: Resend [PATCH] intel_mid: Add Mrst & Mfld DMA Drivers

> The use of the prep_dma_memcpy routine instead of prep_slave_sg is a
> bit odd for device-to-memory dma because the dma_addr_t type only
> describes the bus address view of host memory from the device
> perspective.  Other slave dma devices are using a backdoor method
> (since a good abstraction does not exist) to associate a known channel
> to a known device and then specifying the host address and a direction
> via prep_slave_sg.  I see that this usage of dma_addr_t was inherited
> from the dw_spi implementation, so not much we can do about it now,
> but I don't think its a model we want to perpetuate.
For the four controllers this drivers supports, two don't have sg support in 
hardware. For the other two I am going to add the sg support next, expect the 
patch for that in few weeks or so.


> I do see that the dw_spi_mid driver is only calling
> dma_request_channel(mask, NULL, NULL).  Are we guaranteed that
> dw_spi_mid silicon collateral will only ever exist on an intel_mid_dma
> platform?  Otherwise can the dw_mid_spi driver survive talking to
> another random DMA_SLAVE|DMA_MEMCPY channel?  The purpose of the
> filter_fn parameter to dma_request_channel() is to guarantee (by
> arch-specific hackery) that you are talking to a known compatible
> channel for your device.
Yes, the filter function in these drivers is supposed to look for this device 
and accept dma channel for specific controller only.
I looked at dw_spi_mid driver and yes it is indeed doing the wrong thing, I will 
send the patch to its maintainer to fix this.
Other driver which use this and are not upstream seem to be using the filter 
function properly

> > +{
> > +       struct intel_mid_dma_chan *midc;
> > +       struct intel_mid_dma_desc *desc = NULL;
> > +       struct intel_mid_dma_slave *mids;
> > +       union intel_mid_dma_ctl_lo ctl_lo;
> > +       union intel_mid_dma_ctl_hi ctl_hi;
> > +       union intel_mid_dma_cfg_lo cfg_lo;
> > +       union intel_mid_dma_cfg_hi cfg_hi;
> > +       enum intel_mid_dma_width width = 0;
> > +
> > +       pr_debug("MDMA: Prep for memcpy\n");
> > +       WARN_ON(!chan);
> > +       if (!len)
> > +               return NULL;
> > +
> > +       mids = chan->private;
> > +       WARN_ON(!mids);
> > +
> > +       midc = to_intel_mid_dma_chan(chan);
> > +       WARN_ON(!midc);
> 
> These WARN_ONs are a bit odd because if they trigger we'll hit null
> pointer dereferences soon afterwards.  Is that what you wanted?
Agreed, I will send another patch to fix this

> > +struct intel_mid_dma_slave {
[snip]
> 
> For 2.6.37 I'd like to see a conversion of this to the result of
> Linus' dma_slave_config work, but this isn't a blocker for 2.6.36.
Yes, I will update this to use dma_slave_config when I add the DMA sg support

> 
> *applied with a fix for:
> drivers/dma/intel_mid_dma.c:511: warning: format %d expects type int,
> but argument 4 has type size_t
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists