RE: [PATCH v2] mmc: add a driver for the Renesas usdhi6rol0 SD/SDIO host controller

[Phil Edworthy <phil.edworthy@renesas.com>]

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