lkml.org 
[lkml]   [2010]   [Jul]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRE: 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2010-07-28 09:53    [W:0.051 / U:2.652 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site