| Message ID | 87wq51ogjx.wl%kuninori.morimoto.gx@renesas.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Monday 05 January 2015 07:02:41 Kuninori Morimoto wrote: > From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> > > Current .dma is implemented under tmio_mmc_data. > It goes to tmio_mmc_host by this patch. > > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> The patch looks good, just like the rest of the series, but there is one aspect that I think would make sense to clean up at the same time, since you are already touching all those lines: > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > index 3d02a3c1..4c5eda8 100644 > --- a/drivers/mmc/host/tmio_mmc.h > +++ b/drivers/mmc/host/tmio_mmc.h > @@ -40,6 +40,16 @@ > > struct tmio_mmc_data; > > +struct tmio_mmc_dma { > + void *chan_priv_tx; > + void *chan_priv_rx; > + int slave_id_tx; > + int slave_id_rx; > + int alignment_shift; > + dma_addr_t dma_rx_offset; > + bool (*filter)(struct dma_chan *chan, void *arg); > +}; The slave_id/chan_priv values are now passed three times into the driver, and one should really be enough. I'd suggest removing the integer fields from both tmio_mmc_dma and tmio_mmc_data (added in patch 9), and instead pass it as a void* argument only to tmio_mmc_data. The alignment_shift and dma_rx_offset values seem to always be the same for all users (at least the remaining ones, possibly there were others originally), so you could hardcode those in tmio_mmc_dma.c and remove the tmio_mmc_dma structure entirely. The filter pointer should always be passed in the same structure as the 'void *chan_priv' argument that is used when calling it, so that would go into tmio_mmc_data as well if you add the data there. Alternatively you could keep the tmio_mmc_dma structure and fill it from the platform, but then only have the filter and void* arguments in it. For some obscure reason, the 'pdata->dma->slave_id_?x' fields currently get passed into cfg.slave_id, which is something we really want to get rid of so we can remove the slave_id field from dma_slave_config: The slave ID is generally considered specific to the channel allocation, not the configuration, and not all dmaengine drivers can express it as a single integer, so the concept is obsolete. When you remove the slave_id_?x fields here, you should also be able to just remove the cfg.slave_id assignment without any replacement, unless there is a bug in the dmaengine driver that should be fixed instead. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd Thank you for your review > > diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h > > index 3d02a3c1..4c5eda8 100644 > > --- a/drivers/mmc/host/tmio_mmc.h > > +++ b/drivers/mmc/host/tmio_mmc.h > > @@ -40,6 +40,16 @@ > > > > struct tmio_mmc_data; > > > > +struct tmio_mmc_dma { > > + void *chan_priv_tx; > > + void *chan_priv_rx; > > + int slave_id_tx; > > + int slave_id_rx; > > + int alignment_shift; > > + dma_addr_t dma_rx_offset; > > + bool (*filter)(struct dma_chan *chan, void *arg); > > +}; > > The slave_id/chan_priv values are now passed three times into the > driver, and one should really be enough. I'd suggest removing the > integer fields from both tmio_mmc_dma and tmio_mmc_data (added in > patch 9), and instead pass it as a void* argument only to tmio_mmc_data. Hmm. I guess this priv_?x and slave_id are based on filter ? sh_mobile_sdhi case, and, if it is probed via non-DT, it will use shdma_chan_filter, and then, it doesn't need both priv_?x and slave_id. And, if it is probed via DT, it will use other filter, and yes, it also doesn't need these parameter. So, from sh_mobile_sdhi point of view, yes, we can do your idea. But, if other driver want to use it with other filter, we need both ? (sh_mobile_sdhi is the only dmaengine user at this point, so, there is no problem though...) > The alignment_shift and dma_rx_offset values seem to always be > the same for all users (at least the remaining ones, possibly there > were others originally), so you could hardcode those in tmio_mmc_dma.c > and remove the tmio_mmc_dma structure entirely. Unfortunately, alignment_shift and dma_rx_offset value are based on SoC. we can't hardcode these. > For some obscure reason, the 'pdata->dma->slave_id_?x' fields currently > get passed into cfg.slave_id, which is something we really want to > get rid of so we can remove the slave_id field from dma_slave_config: > The slave ID is generally considered specific to the channel allocation, > not the configuration, and not all dmaengine drivers can express it > as a single integer, so the concept is obsolete. When you remove > the slave_id_?x fields here, you should also be able to just remove > the cfg.slave_id assignment without any replacement, unless there is > a bug in the dmaengine driver that should be fixed instead. I guess maybe there is no problem about this, but, actually I don't want do it, because of compatibility. There are many combination for DMAEngine setting. In sh-mobile-sdhi case, we are using this driver via non-DT / DT cases, and different DMAEngine (sh-dma / rcar-dma). But, I can't test all patterns today. So, I would like to keep it as-is if possible. Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sh_mobile_sdhi.c b/drivers/mmc/host/sh_mobile_sdhi.c index b2cec5b..1e13a1f 100644 --- a/drivers/mmc/host/sh_mobile_sdhi.c +++ b/drivers/mmc/host/sh_mobile_sdhi.c @@ -211,6 +211,8 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) if (!host) goto eprobe; + host->dma = dma_priv; + mmc_data->clk_enable = sh_mobile_sdhi_clk_enable; mmc_data->clk_disable = sh_mobile_sdhi_clk_disable; mmc_data->capabilities = MMC_CAP_MMC_HIGHSPEED; @@ -239,8 +241,6 @@ static int sh_mobile_sdhi_probe(struct platform_device *pdev) dma_priv->alignment_shift = 1; /* 2-byte alignment */ dma_priv->filter = shdma_chan_filter; - mmc_data->dma = dma_priv; - /* * All SDHI blocks support 2-byte and larger block sizes in 4-bit * bus width mode. diff --git a/drivers/mmc/host/tmio_mmc.h b/drivers/mmc/host/tmio_mmc.h index 3d02a3c1..4c5eda8 100644 --- a/drivers/mmc/host/tmio_mmc.h +++ b/drivers/mmc/host/tmio_mmc.h @@ -40,6 +40,16 @@ struct tmio_mmc_data; +struct tmio_mmc_dma { + void *chan_priv_tx; + void *chan_priv_rx; + int slave_id_tx; + int slave_id_rx; + int alignment_shift; + dma_addr_t dma_rx_offset; + bool (*filter)(struct dma_chan *chan, void *arg); +}; + struct tmio_mmc_host { void __iomem *ctl; struct mmc_command *cmd; @@ -59,6 +69,7 @@ struct tmio_mmc_host { struct platform_device *pdev; struct tmio_mmc_data *pdata; + struct tmio_mmc_dma *dma; /* DMA support */ bool force_pio; diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c index 7d07738..6c214d6 100644 --- a/drivers/mmc/host/tmio_mmc_dma.c +++ b/drivers/mmc/host/tmio_mmc_dma.c @@ -49,11 +49,10 @@ static void tmio_mmc_start_dma_rx(struct tmio_mmc_host *host) struct scatterlist *sg = host->sg_ptr, *sg_tmp; struct dma_async_tx_descriptor *desc = NULL; struct dma_chan *chan = host->chan_rx; - struct tmio_mmc_data *pdata = host->pdata; dma_cookie_t cookie; int ret, i; bool aligned = true, multiple = true; - unsigned int align = (1 << pdata->dma->alignment_shift) - 1; + unsigned int align = (1 << host->dma->alignment_shift) - 1; for_each_sg(sg, sg_tmp, host->sg_len, i) { if (sg_tmp->offset & align) @@ -126,11 +125,10 @@ static void tmio_mmc_start_dma_tx(struct tmio_mmc_host *host) struct scatterlist *sg = host->sg_ptr, *sg_tmp; struct dma_async_tx_descriptor *desc = NULL; struct dma_chan *chan = host->chan_tx; - struct tmio_mmc_data *pdata = host->pdata; dma_cookie_t cookie; int ret, i; bool aligned = true, multiple = true; - unsigned int align = (1 << pdata->dma->alignment_shift) - 1; + unsigned int align = (1 << host->dma->alignment_shift) - 1; for_each_sg(sg, sg_tmp, host->sg_len, i) { if (sg_tmp->offset & align) @@ -262,8 +260,8 @@ out: void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdata) { /* We can only either use DMA for both Tx and Rx or not use it at all */ - if (!pdata->dma || (!host->pdev->dev.of_node && - (!pdata->dma->chan_priv_tx || !pdata->dma->chan_priv_rx))) + if (!host->dma || (!host->pdev->dev.of_node && + (!host->dma->chan_priv_tx || !host->dma->chan_priv_rx))) return; if (!host->chan_tx && !host->chan_rx) { @@ -280,7 +278,7 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat dma_cap_set(DMA_SLAVE, mask); host->chan_tx = dma_request_slave_channel_compat(mask, - pdata->dma->filter, pdata->dma->chan_priv_tx, + host->dma->filter, host->dma->chan_priv_tx, &host->pdev->dev, "tx"); dev_dbg(&host->pdev->dev, "%s: TX: got channel %p\n", __func__, host->chan_tx); @@ -288,8 +286,8 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat if (!host->chan_tx) return; - if (pdata->dma->chan_priv_tx) - cfg.slave_id = pdata->dma->slave_id_tx; + if (host->dma->chan_priv_tx) + cfg.slave_id = host->dma->slave_id_tx; cfg.direction = DMA_MEM_TO_DEV; cfg.dst_addr = res->start + (CTL_SD_DATA_PORT << host->pdata->bus_shift); cfg.dst_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; @@ -299,7 +297,7 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat goto ecfgtx; host->chan_rx = dma_request_slave_channel_compat(mask, - pdata->dma->filter, pdata->dma->chan_priv_rx, + host->dma->filter, host->dma->chan_priv_rx, &host->pdev->dev, "rx"); dev_dbg(&host->pdev->dev, "%s: RX: got channel %p\n", __func__, host->chan_rx); @@ -307,10 +305,10 @@ void tmio_mmc_request_dma(struct tmio_mmc_host *host, struct tmio_mmc_data *pdat if (!host->chan_rx) goto ereqrx; - if (pdata->dma->chan_priv_rx) - cfg.slave_id = pdata->dma->slave_id_rx; + if (host->dma->chan_priv_rx) + cfg.slave_id = host->dma->slave_id_rx; cfg.direction = DMA_DEV_TO_MEM; - cfg.src_addr = cfg.dst_addr + pdata->dma->dma_rx_offset; + cfg.src_addr = cfg.dst_addr + host->dma->dma_rx_offset; cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; cfg.dst_addr = 0; ret = dmaengine_slave_config(host->chan_rx, &cfg); diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h index c7d9af0..8d708c7 100644 --- a/include/linux/mfd/tmio.h +++ b/include/linux/mfd/tmio.h @@ -112,16 +112,6 @@ void tmio_core_mmc_clk_div(void __iomem *cnf, int shift, int state); struct dma_chan; -struct tmio_mmc_dma { - void *chan_priv_tx; - void *chan_priv_rx; - int slave_id_tx; - int slave_id_rx; - int alignment_shift; - dma_addr_t dma_rx_offset; - bool (*filter)(struct dma_chan *chan, void *arg); -}; - struct tmio_mmc_host; /* @@ -134,7 +124,6 @@ struct tmio_mmc_data { unsigned long flags; unsigned long bus_shift; u32 ocr_mask; /* available voltages */ - struct tmio_mmc_dma *dma; unsigned int cd_gpio; void (*set_pwr)(struct platform_device *host, int state); void (*set_clk_div)(struct platform_device *host, int state);