[2/9] mmc: tmio: tmio_mmc_host has .dma

On Monday 05 January 2015 09:35:16 Kuninori Morimoto wrote:
> 
> 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 ?

priv_?x needs to be in a format that matches the filter function,
passing slave_id is basically always wrong, but dmaengine drivers
generally just ignore it.

> 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.

Ok.

> 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...)

No other driver besides shmobile should use the slave_id, and a lot
of them cannot even use it because it is not in a format that makes
sense to a driver. Passing just chan_priv is the only way to do
a platform-independent driver with our API, so if we ever wanted to
use DMA on another SoC with this driver, we have to make that change
anyway.

> > 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.

Which SoCs use a different value here? Both of these look like
implementation details of the tmio_mmc, not of the integration
into the SoC, so they could just be keyed off the device identification.

> > 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.

Maybe we can hide the slave_id field in dma_slave_config within
'#if defined(CONFIG_ARCH_SH_MOBILE) && defined(CONFIG_ATAGS)' then?

I would really like to prevent other people from copying the mistake.
Apparently it has already happened on MIPS jz4740, but that one seems
easy enough to fix. Tegra used to use slave_id as well, but it's been
converted to DT-only a while ago and that is just dead code for them.

I've had a closer look at the existing code now and came up with a
patch that should work for all out-of-tree drivers you may be worried
about.

	Arnd
---
dmaengine: shdma: use normal interface for passing slave id

The shmobile platform is one of only two users of the slave_id field
in dma_slave_config, which is incompatible with the way that the
dmaengine API normally works.

I've had a closer look at the existing code now and found that all
slave drivers that pass a slave_id in dma_slave_config for SH do that
right after passing the same ID into shdma_chan_filter, so we can just
rely on that. However, the various shdma drivers currently do not
remember the slave ID that was passed into the filter function when
used in non-DT mode and only check the value to find a matching channel,
unlike all other drivers.

There might still be drivers that are not part of the kernel that rely
on setting the slave_id to some other value, so to be on the safe side,
this adds another 'real_slave_id' field to shdma_chan that remembers
the ID and uses it when a driver passes a zero slave_id in dma_slave_config,
like most drivers do.

Eventually, the real_slave_id and slave_id fields should just get merged
into one field, but that requires other changes.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
----
 drivers/dma/sh/shdma-base.c     | 70 ++++++++++++++++++++++++++++++++++++++++++++++++-------------------
 drivers/mmc/host/sh_mmcif.c     |  4 +---
 drivers/mmc/host/tmio_mmc_dma.c |  4 ----
 drivers/mtd/nand/sh_flctl.c     |  2 --
 drivers/spi/spi-rspi.c          |  1 -
 drivers/spi/spi-sh-msiof.c      |  1 -
 include/linux/shdma-base.h      |  1 +
 7 files changed, 52 insertions(+), 31 deletions(-)

Note: this also changes all affected drivers to no longer pass the slave_id
field in dma_slave_config, but those changes can also be done later, as
long as the main patch gets merged first.

--
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

[2/9] mmc: tmio: tmio_mmc_host has .dma

Commit Message

Comments

Patch