From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kuninori Morimoto Date: Mon, 05 Jan 2015 09:35:16 +0000 Subject: Re: [PATCH 2/9] mmc: tmio: tmio_mmc_host has .dma Message-Id: <87bnmdo9hk.wl%kuninori.morimoto.gx@renesas.com> List-Id: References: <87zj9xogln.wl%kuninori.morimoto.gx@renesas.com> <87wq51ogjx.wl%kuninori.morimoto.gx@renesas.com> <4405974.QQJ6fqAfNe@wuerfel> In-Reply-To: <4405974.QQJ6fqAfNe@wuerfel> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Arnd Bergmann Cc: Ulf Hansson , Chris Ball , Simon , Linux-SH , linux-mmc 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kuninori Morimoto Subject: Re: [PATCH 2/9] mmc: tmio: tmio_mmc_host has .dma Date: Mon, 5 Jan 2015 09:35:16 +0000 Message-ID: <87bnmdo9hk.wl%kuninori.morimoto.gx@renesas.com> References: <87zj9xogln.wl%kuninori.morimoto.gx@renesas.com> <87wq51ogjx.wl%kuninori.morimoto.gx@renesas.com> <4405974.QQJ6fqAfNe@wuerfel> Mime-Version: 1.0 (generated by SEMI 1.14.6 - "Maruoka") Content-Type: text/plain; charset="US-ASCII" Return-path: In-Reply-To: <4405974.QQJ6fqAfNe@wuerfel> Sender: linux-sh-owner@vger.kernel.org To: Arnd Bergmann Cc: Ulf Hansson , Chris Ball , Simon , Linux-SH , linux-mmc List-Id: linux-mmc@vger.kernel.org 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