Hi Guennadi,
On 03 May 2014 10:09, Guennadi wrote:
> On Mon, 28 Apr 2014, Phil Edworthy wrote:
>
> > Hi Guennadi,
> >
> > On 26 April 2014 13:06, Guennadi wrote:
> > > Subject: [PATCH v2] mmc: add a driver for the Renesas usdhi6rol0
> SD/SDIO
> > > host controller
> > >
> > > This patch adds a driver for the Renesas usdhi6rol0 SD/SDIO host
> controller
> > > in both PIO and DMA modes.
> >
> > ...
> > > +static void usdhi6_dma_stop_unmap(struct usdhi6_host *host)
> > > +{
> > > + struct mmc_data *data = host->mrq->data;
> > > +
> > > + if (!host->dma_active)
> > > + return;
> > > +
> > > + usdhi6_write(host, USDHI6_CC_EXT_MODE, 0);
> > > + host->dma_active = false;
> > > +
> > > + if (data->flags & MMC_DATA_READ)
> > > + /* TODO: do we have to synchronise? */
> > > + dma_unmap_sg(host->chan_rx->device->dev, data->sg,
> > > + data->sg_len, DMA_FROM_DEVICE);
> > Yes, you have to sync, so you can remove this TODO comment.
>
> Why do you think so? If we really do, we'd have to add it. And I did think
> synchronisation was needed, that's why I posted this patch series:
>
> http://thread.gmane.org/gmane.linux.kernel.mmc/24969
>
> but it never got applied, even though it got 2 acks. And actually now I
> think that synchronisation isn't needed. I think dma_map_sg() /
> dma_unmap_sg() do that for us already. E.g.
>
> dma_map_sg_attrs()
> arm_dma_map_page()
> __dma_page_cpu_to_dev()
> dmac_map_area()
> cpu_cache.dma_map_area()
> v7_dma_inv_range()
> v7_dma_clean_range()
>
> Similar for unmapping. So, I think it would be best to remove the comment
> without adding synchronisation.
>
> Let me do this: I'll prepare a separate patch for adding dma syncs, but I
> won't submit it for now, or I can just provide it for reference.
Ok, my comment was based on the fact that your above patches got acks... Based on your comments above, I think it's best just to remove the TODO comment.
> > ...
> > > +static int usdhi6_probe(struct platform_device *pdev)
> > > +{
> > ...
> > > + host = mmc_priv(mmc);
> > > + host->mmc = mmc;
> > > + host->wait = USDHI6_WAIT_FOR_REQUEST;
> > > + host->timeout = msecs_to_jiffies(1000);
>
> > In all places you use host->timeout, the code uses host->timeout * 4.
> > Wouldn't it better to just set it here to 4 seconds?
>
> ok, I'll do that for v3.
>
> Thanks
> Guennadi
Thanks
Phil
--
To unsubscribe from this list: send the line "unsubscribe linux-mmc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
