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

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

Hi Phil,

Thanks for the comments.

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.

> ...
> > +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
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/