| Message ID | 7306460.62PqiTEhxi@wuerfel (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
Hi Arnd, Ulf Thank you for your feedback > > > 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. (snip) > 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. I don't want to use #ifdef in driver :P OK, I understand your opinion. I can try fix this DMAEngine issue (without #ifdef :) But, I want "step by step" for this cleanup. So, can you please accept about current "header cleanup" patch-set as 1st step ? Then, I want to try "dmaengine cleanup" patch-set as 2nd step if possible. I guess [8/9] and [9/9] are not good for "header cleanup" (?) I don't know. Arnd, Ulf, what is your opinion ? Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd again > > > 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. About .alignment_shift, it is not implemented today, but our new SoC want to use different value (= .alignment_shift = 5). About .dma_rx_offset, please check this ${LINUX}/drivers/mmc/host/sh_mobile_sdhi.c :: of_rcar_gen1_compatible ${LINUX}/drivers/mmc/host/sh_mobile_sdhi.c :: of_rcar_gen2_compatible or 384b2cbd56a02efb16358ed7c0c039e4afca5ed0 (mmc: tmio: care about DMA tx/rx addr offset) Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 06 January 2015 02:38:53 Kuninori Morimoto wrote: > Hi Arnd again > > > > > 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. > > About .alignment_shift, it is not implemented today, but our new SoC > want to use different value (= .alignment_shift = 5). Ok, I see. > About .dma_rx_offset, please check this > ${LINUX}/drivers/mmc/host/sh_mobile_sdhi.c :: of_rcar_gen1_compatible > ${LINUX}/drivers/mmc/host/sh_mobile_sdhi.c :: of_rcar_gen2_compatible > or > 384b2cbd56a02efb16358ed7c0c039e4afca5ed0 > (mmc: tmio: care about DMA tx/rx addr offset) Right. How about moving these two into tmio_mmc_data then along with the other members of tmio_mmc_dma? Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday 06 January 2015 00:20:59 Kuninori Morimoto wrote: > > Hi Arnd, Ulf > > Thank you for your feedback > > > > > 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. > (snip) > > 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. > > I don't want to use #ifdef in driver :P > OK, I understand your opinion. I can try fix this DMAEngine issue > (without #ifdef :) Ok, thanks. I have also posted a patch now for the jz4740 platform that is the other user of slave_id. > But, I want "step by step" for this cleanup. > So, can you please accept about current "header cleanup" patch-set as 1st step ? Fair enough. > Then, I want to try "dmaengine cleanup" patch-set as 2nd step if possible. > I guess [8/9] and [9/9] are not good for "header cleanup" (?) I don't know. > Arnd, Ulf, what is your opinion ? Patch 8 looks great to me. Patch 9 as I mentioned is a bit strange because it duplicates the some of the data between tmio_mmc_data and tmio_mmc_dma, which are both visible to the tmio_mmc_dma.c file. Simply folding tmio_mmc_dma into tmio_mmc_data seems like the simplest solution to that. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, Ulf Thank you for your feedback > > Then, I want to try "dmaengine cleanup" patch-set as 2nd step if possible. > > I guess [8/9] and [9/9] are not good for "header cleanup" (?) I don't know. > > Arnd, Ulf, what is your opinion ? > > Patch 8 looks great to me. > > Patch 9 as I mentioned is a bit strange because it duplicates the some > of the data between tmio_mmc_data and tmio_mmc_dma, which are both > visible to the tmio_mmc_dma.c file. Simply folding tmio_mmc_dma into > tmio_mmc_data seems like the simplest solution to that. OK, I see. I will fixup 9/9. Ulf Can you please care about 1/9 - 8/9 as 1st step ? (I got review from Geert about 8/9, so, 1/9 - 7/9 ?) I will re-try about 9/9 (or 8/9, 9/9) part. And, then, I will try dmaengine cleanup. Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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 DMAEngine fixup patch. I tried this patch, and have comment. > 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(-) (snip) > @@ -284,16 +286,35 @@ bool shdma_chan_filter(struct dma_chan *chan, void *arg) > shdma_alloc_chan_resources) > return false; > > - if (match < 0) > + schan = to_shdma_chan(chan); > + sdev = to_shdma_dev(chan->device); > + > + /* > + * For DT, the schan->slave_id field is generated by the > + * set_slave function from the slave ID that is passed in > + * from xlate. For the non-DT case, the slave ID is > + * directly passed into the filter function by the driver > + */ > + if (schan->dev->of_node) { > + ret = sdev->ops->set_slave(schan, slave_id, 0, true); > + if (ret < 0) > + return false; > + > + schan->real_slave_id = schan->slave_id; > + return true; > + } > + > + if (slave_id < 0) { > /* No slave requested - arbitrary channel */ > + dev_warn(sdev->dma_dev.dev, "invalid slave ID passed to dma_request_slave\n"); > return true; > + } > > - schan = to_shdma_chan(chan); > - if (!schan->dev->of_node && match >= slave_num) > + if (slave_id >= slave_num) > return false; > > - sdev = to_shdma_dev(schan->dma_chan.device); > - ret = sdev->ops->set_slave(schan, match, 0, true); > + ret = sdev->ops->set_slave(schan, schan->real_slave_id, 0, true); > + schan->real_slave_id = slave_id; > if (ret < 0) > return false; Here, your patch is ret = sdev->ops->set_slave(schan, schan->real_slave_id, 0, true); schan->real_slave_id = slave_id; if (ret < 0) But, it doesn't work. Maybe, you want this schan->real_slave_id = slave_id; ret = sdev->ops->set_slave(schan, schan->real_slave_id, 0, true); if (ret < 0) > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c > index 7d9d6a321521..df3a537f5a83 100644 > --- a/drivers/mmc/host/sh_mmcif.c > +++ b/drivers/mmc/host/sh_mmcif.c > @@ -388,7 +388,7 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, > { > struct dma_slave_config cfg = { 0, }; > struct dma_chan *chan; > - unsigned int slave_id; > + void *slave_data; > struct resource *res; > dma_cap_mask_t mask; > int ret; > @@ -414,8 +414,6 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, > > res = platform_get_resource(host->pd, IORESOURCE_MEM, 0); > > - /* In the OF case the driver will get the slave ID from the DT */ > - cfg.slave_id = slave_id; > cfg.direction = direction; > > if (direction == DMA_DEV_TO_MEM) { I got error here /opt/home/morimoto/linux/drivers/mmc/host/sh_mmcif.c: In function ‘sh_mmcif_request_dma_one’: /opt/home/morimoto/linux/drivers/mmc/host/sh_mmcif.c:400:3: error: ‘slave_id’ undeclared (first use in this function) slave_id = direction == DMA_MEM_TO_DEV ^ /opt/home/morimoto/linux/drivers/mmc/host/sh_mmcif.c:400:3: note: each undeclared identifier is reported only once for each function it appears in /opt/home/morimoto/linux/drivers/mmc/host/sh_mmcif.c:391:8: warning: unused variable ‘slave_data’ [-Wunused-variable] void *slave_data; ^ Maybe you are missing this if (pdata) - slave_id = direction == DMA_MEM_TO_DEV - ? pdata->slave_id_tx : pdata->slave_id_rx; + slave_data = direction == DMA_MEM_TO_DEV + ? (void *)pdata->slave_id_tx : (void *)pdata->slave_id_rx; else - slave_id = 0; + slave_data = 0; chan = dma_request_slave_channel_compat(mask, shdma_chan_filter, - (void *)(unsigned long)slave_id, &host->pd->dev, + slave_data, &host->pd->dev, direction == DMA_MEM_TO_DEV ? "tx" : "rx"); dev_dbg(&host->pd->dev, "%s: %s: got channel %p\n", __func__, sh_mobile_sdhi DMA works if I fixuped above Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, Ulf > > > > > 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. > > > > About .alignment_shift, it is not implemented today, but our new SoC > > want to use different value (= .alignment_shift = 5). > > Ok, I see. > > > About .dma_rx_offset, please check this > > ${LINUX}/drivers/mmc/host/sh_mobile_sdhi.c :: of_rcar_gen1_compatible > > ${LINUX}/drivers/mmc/host/sh_mobile_sdhi.c :: of_rcar_gen2_compatible > > or > > 384b2cbd56a02efb16358ed7c0c039e4afca5ed0 > > (mmc: tmio: care about DMA tx/rx addr offset) > > Right. How about moving these two into tmio_mmc_data then along with > the other members of tmio_mmc_dma? OK, I can do it. Ulf, can I send above as additional patch ? Or do you want v2 patch-set ? Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi 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(-) (snip) > - /* In the OF case the driver will get the slave ID from the DT */ > - cfg.slave_id = slave_id; > cfg.direction = direction; My other drivers are using slave_id linux/sound/soc/sh/fsi.c linux/sound/soc/sh/rcar/core.c I guess it should remove cfg.slave_id = xxx ? Can you include these in this patch ? or I can do it as other patch Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 07 January 2015 03:01:22 Kuninori Morimoto wrote: > Hi 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(-) > (snip) > > - /* In the OF case the driver will get the slave ID from the DT */ > > - cfg.slave_id = slave_id; > > cfg.direction = direction; > > My other drivers are using slave_id > linux/sound/soc/sh/fsi.c > linux/sound/soc/sh/rcar/core.c My mistake, I did a 'git grep' the linux/drivers but missed linux/sound/soc. > I guess it should remove cfg.slave_id = xxx ? Correct. I checked both drivers, and they behave just like the ones I included in my patch. > Can you include these in this patch ? > or I can do it as other patch I was actually hoping that you could pick up my patch and send it as a follow-up to your series once you have a working version. As I mentioned, the driver changes to remove the slave_id assignment can be done either together with the base patch or as a follow-up. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 07 January 2015 02:28:43 Kuninori Morimoto wrote: > > Hi Arnd > > Thank you for your DMAEngine fixup patch. > I tried this patch, and have comment. Thanks for looking at the changes > > > > - sdev = to_shdma_dev(schan->dma_chan.device); > > - ret = sdev->ops->set_slave(schan, match, 0, true); > > + ret = sdev->ops->set_slave(schan, schan->real_slave_id, 0, true); > > + schan->real_slave_id = slave_id; > > if (ret < 0) > > return false; > > Here, your patch is > > ret = sdev->ops->set_slave(schan, schan->real_slave_id, 0, true); > schan->real_slave_id = slave_id; > if (ret < 0) > > But, it doesn't work. Maybe, you want this > > schan->real_slave_id = slave_id; > ret = sdev->ops->set_slave(schan, schan->real_slave_id, 0, true); > if (ret < 0) Right, I broke it in a last-minute change. Originally I had schan->real_slave_id = slave_id; ret = sdev->ops->set_slave(schan, schan->real_slave_id, 0, true); if (ret < 0) { schan->real_slave_id = 0; return ret; } and then meant to shorten it to ret = sdev->ops->set_slave(schan, slave_id, 0, true); if (ret < 0) return ret; schan->real_slave_id = slave_id; Either of those should be fine, but what I wrote was instead was completely wrong. > > diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c > > index 7d9d6a321521..df3a537f5a83 100644 > > --- a/drivers/mmc/host/sh_mmcif.c > > +++ b/drivers/mmc/host/sh_mmcif.c > > @@ -388,7 +388,7 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, > > { > > struct dma_slave_config cfg = { 0, }; > > struct dma_chan *chan; > > - unsigned int slave_id; > > + void *slave_data; > > struct resource *res; > > dma_cap_mask_t mask; > > int ret; > > @@ -414,8 +414,6 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, > > > > res = platform_get_resource(host->pd, IORESOURCE_MEM, 0); > > > > - /* In the OF case the driver will get the slave ID from the DT */ > > - cfg.slave_id = slave_id; > > cfg.direction = direction; > > > > if (direction == DMA_DEV_TO_MEM) { > > I got error here > > > /opt/home/morimoto/linux/drivers/mmc/host/sh_mmcif.c: In function ‘sh_mmcif_request_dma_one’: > /opt/home/morimoto/linux/drivers/mmc/host/sh_mmcif.c:400:3: error: ‘slave_id’ undeclared (first use in this function) > slave_id = direction == DMA_MEM_TO_DEV > ^ > /opt/home/morimoto/linux/drivers/mmc/host/sh_mmcif.c:400:3: note: each undeclared identifier is reported only once for each function it appears in > /opt/home/morimoto/linux/drivers/mmc/host/sh_mmcif.c:391:8: warning: unused variable ‘slave_data’ [-Wunused-variable] > void *slave_data; > ^ > > Maybe you are missing this > > if (pdata) > - slave_id = direction == DMA_MEM_TO_DEV > - ? pdata->slave_id_tx : pdata->slave_id_rx; > + slave_data = direction == DMA_MEM_TO_DEV > + ? (void *)pdata->slave_id_tx : (void *)pdata->slave_id_rx; > else > - slave_id = 0; > + slave_data = 0; > > chan = dma_request_slave_channel_compat(mask, shdma_chan_filter, > - (void *)(unsigned long)slave_id, &host->pd->dev, > + slave_data, &host->pd->dev, > direction == DMA_MEM_TO_DEV ? "tx" : "rx"); > > dev_dbg(&host->pd->dev, "%s: %s: got channel %p\n", __func__, > > > sh_mobile_sdhi DMA works if I fixuped above Either that or undo the change to the type. I originally planned to change the sh_mmcif_plat_data to use a void* type already, but then didn't do that because it conflicts with your other patch, and I failed to revert my earlier change correctly. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi 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> > > > ---- (snip) > I was actually hoping that you could pick up my patch and send it as a > follow-up to your series once you have a working version. As I mentioned, > the driver changes to remove the slave_id assignment can be done either > together with the base patch or as a follow-up. OK, I can do it if you want :) Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, Ulf > > > > 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> > > > > ---- > (snip) > Either that or undo the change to the type. I originally planned to change the > sh_mmcif_plat_data to use a void* type already, but then didn't do that because > it conflicts with your other patch, and I failed to revert my earlier change > correctly. Hmm... indeed Arnd's patch and my patch-set conflicts. I have these patch / patch-set 1) header cleanup for tmio 2) slave_id cleanup for shdma 3) add DMA feature for sh_mobile_sdhi 1 ) and 2) conflicts here. one idea is like this 1) header cleanup for tmio 2) add DMA feature for sh_mobile_sdhi 3) slave_id cleanup for shdma 1) and 2) can be controled by Ulf with no-conflict. if these are merged correctly, I can send 3) to DMAEngine ML. Then, I can point the Ulf's branch as base branch. Arnd, Ulf what do you think ? Best regards --- Kuninori Morimoto -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday 08 January 2015 07:30:36 Kuninori Morimoto wrote: > > Hmm... indeed Arnd's patch and my patch-set conflicts. > I have these patch / patch-set > 1) header cleanup for tmio > 2) slave_id cleanup for shdma > 3) add DMA feature for sh_mobile_sdhi > > 1 ) and 2) conflicts here. one idea is like this > 1) header cleanup for tmio > 2) add DMA feature for sh_mobile_sdhi > 3) slave_id cleanup for shdma > > 1) and 2) can be controled by Ulf with no-conflict. > if these are merged correctly, I can send 3) to DMAEngine ML. > Then, I can point the Ulf's branch as base branch. > > Arnd, Ulf what do you think ? > Sounds good. You could also leave out the sh_mobile_sdhi part from 3) patch to avoid the conflict, and add a comment in that place as part of 2), to say that the slave_id assignment can be removed once the other parts are done. That way, we know where we're at if we want to remove slave_id from dma_slave_config and it's still part of the sdhi driver. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Arnd, Ulf > > Hmm... indeed Arnd's patch and my patch-set conflicts. > > I have these patch / patch-set > > 1) header cleanup for tmio > > 2) slave_id cleanup for shdma > > 3) add DMA feature for sh_mobile_sdhi > > > > 1 ) and 2) conflicts here. one idea is like this > > 1) header cleanup for tmio > > 2) add DMA feature for sh_mobile_sdhi > > 3) slave_id cleanup for shdma > > > > 1) and 2) can be controled by Ulf with no-conflict. > > if these are merged correctly, I can send 3) to DMAEngine ML. > > Then, I can point the Ulf's branch as base branch. > > > > Arnd, Ulf what do you think ? > > > > Sounds good. You could also leave out the sh_mobile_sdhi part from > 3) patch to avoid the conflict, and add a comment in that place > as part of 2), to say that the slave_id assignment can be removed > once the other parts are done. That way, we know where we're at > if we want to remove slave_id from dma_slave_config and it's still > part of the sdhi driver. Thank you. I wait Ulf's opinion -- To unsubscribe from this list: send the line "unsubscribe linux-sh" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 9 January 2015 at 10:44, Kuninori Morimoto <kuninori.morimoto.gx@renesas.com> wrote: > > Hi Arnd, Ulf > >> > Hmm... indeed Arnd's patch and my patch-set conflicts. >> > I have these patch / patch-set >> > 1) header cleanup for tmio >> > 2) slave_id cleanup for shdma >> > 3) add DMA feature for sh_mobile_sdhi >> > >> > 1 ) and 2) conflicts here. one idea is like this >> > 1) header cleanup for tmio >> > 2) add DMA feature for sh_mobile_sdhi >> > 3) slave_id cleanup for shdma >> > >> > 1) and 2) can be controled by Ulf with no-conflict. >> > if these are merged correctly, I can send 3) to DMAEngine ML. >> > Then, I can point the Ulf's branch as base branch. >> > >> > Arnd, Ulf what do you think ? >> > >> >> Sounds good. You could also leave out the sh_mobile_sdhi part from >> 3) patch to avoid the conflict, and add a comment in that place >> as part of 2), to say that the slave_id assignment can be removed >> once the other parts are done. That way, we know where we're at >> if we want to remove slave_id from dma_slave_config and it's still >> part of the sdhi driver. > > Thank you. > I wait Ulf's opinion > I am happy to share an immutable branch of needed. Please post a v2 with patches for me to review/pick up for mmc. Kind regards Uffe -- To unsubscribe from this list: send the line "unsubscribe linux-sh" 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/dma/sh/shdma-base.c b/drivers/dma/sh/shdma-base.c index 3a2adb131d46..cb3e7e3b5027 100644 --- a/drivers/dma/sh/shdma-base.c +++ b/drivers/dma/sh/shdma-base.c @@ -171,8 +171,7 @@ static struct shdma_desc *shdma_get_desc(struct shdma_chan *schan) return NULL; } -static int shdma_setup_slave(struct shdma_chan *schan, int slave_id, - dma_addr_t slave_addr) +static int shdma_setup_slave(struct shdma_chan *schan, dma_addr_t slave_addr) { struct shdma_dev *sdev = to_shdma_dev(schan->dma_chan.device); const struct shdma_ops *ops = sdev->ops; @@ -183,25 +182,23 @@ static int shdma_setup_slave(struct shdma_chan *schan, int slave_id, ret = ops->set_slave(schan, match, slave_addr, true); if (ret < 0) return ret; - - slave_id = schan->slave_id; } else { - match = slave_id; + match = schan->real_slave_id; } - if (slave_id < 0 || slave_id >= slave_num) + if (schan->real_slave_id < 0 || schan->real_slave_id >= slave_num) return -EINVAL; - if (test_and_set_bit(slave_id, shdma_slave_used)) + if (test_and_set_bit(schan->real_slave_id, shdma_slave_used)) return -EBUSY; ret = ops->set_slave(schan, match, slave_addr, false); if (ret < 0) { - clear_bit(slave_id, shdma_slave_used); + clear_bit(schan->real_slave_id, shdma_slave_used); return ret; } - schan->slave_id = slave_id; + schan->slave_id = schan->real_slave_id; return 0; } @@ -221,10 +218,12 @@ static int shdma_alloc_chan_resources(struct dma_chan *chan) */ if (slave) { /* Legacy mode: .private is set in filter */ - ret = shdma_setup_slave(schan, slave->slave_id, 0); + schan->real_slave_id = slave->slave_id; + ret = shdma_setup_slave(schan, 0); if (ret < 0) goto esetslave; } else { + /* Normal mode: real_slave_id was set by filter */ schan->slave_id = -EINVAL; } @@ -258,11 +257,14 @@ esetslave: /* * This is the standard shdma filter function to be used as a replacement to the - * "old" method, using the .private pointer. If for some reason you allocate a - * channel without slave data, use something like ERR_PTR(-EINVAL) as a filter + * "old" method, using the .private pointer. + * You always have to pass a valid slave id as the argument, old drivers that + * pass ERR_PTR(-EINVAL) as a filter parameter and set it up in dma_slave_config + * need to be updated so we can remove the slave_id field from dma_slave_config. * parameter. If this filter is used, the slave driver, after calling * dma_request_channel(), will also have to call dmaengine_slave_config() with - * .slave_id, .direction, and either .src_addr or .dst_addr set. + * .direction, and either .src_addr or .dst_addr set. + * * NOTE: this filter doesn't support multiple DMAC drivers with the DMA_SLAVE * capability! If this becomes a requirement, hardware glue drivers, using this * services would have to provide their own filters, which first would check @@ -276,7 +278,7 @@ bool shdma_chan_filter(struct dma_chan *chan, void *arg) { struct shdma_chan *schan; struct shdma_dev *sdev; - int match = (long)arg; + int slave_id = (long)arg; int ret; /* Only support channels handled by this driver. */ @@ -284,16 +286,35 @@ bool shdma_chan_filter(struct dma_chan *chan, void *arg) shdma_alloc_chan_resources) return false; - if (match < 0) + schan = to_shdma_chan(chan); + sdev = to_shdma_dev(chan->device); + + /* + * For DT, the schan->slave_id field is generated by the + * set_slave function from the slave ID that is passed in + * from xlate. For the non-DT case, the slave ID is + * directly passed into the filter function by the driver + */ + if (schan->dev->of_node) { + ret = sdev->ops->set_slave(schan, slave_id, 0, true); + if (ret < 0) + return false; + + schan->real_slave_id = schan->slave_id; + return true; + } + + if (slave_id < 0) { /* No slave requested - arbitrary channel */ + dev_warn(sdev->dma_dev.dev, "invalid slave ID passed to dma_request_slave\n"); return true; + } - schan = to_shdma_chan(chan); - if (!schan->dev->of_node && match >= slave_num) + if (slave_id >= slave_num) return false; - sdev = to_shdma_dev(schan->dma_chan.device); - ret = sdev->ops->set_slave(schan, match, 0, true); + ret = sdev->ops->set_slave(schan, schan->real_slave_id, 0, true); + schan->real_slave_id = slave_id; if (ret < 0) return false; @@ -452,6 +473,8 @@ static void shdma_free_chan_resources(struct dma_chan *chan) chan->private = NULL; } + schan->real_slave_id = 0; + spin_lock_irq(&schan->chan_lock); list_splice_init(&schan->ld_free, &list); @@ -767,7 +790,14 @@ static int shdma_control(struct dma_chan *chan, enum dma_ctrl_cmd cmd, * channel, while using it... */ config = (struct dma_slave_config *)arg; - ret = shdma_setup_slave(schan, config->slave_id, + + /* overriding the slave_id through dma_slave_config is deprecated, + * but possibly some out-of-tree drivers still do it. */ + if (WARN_ON_ONCE(config->slave_id && + config->slave_id != schan->real_slave_id)) + schan->real_slave_id = config->slave_id; + + ret = shdma_setup_slave(schan, config->direction == DMA_DEV_TO_MEM ? config->src_addr : config->dst_addr); if (ret < 0) diff --git a/drivers/mmc/host/sh_mmcif.c b/drivers/mmc/host/sh_mmcif.c index 7d9d6a321521..df3a537f5a83 100644 --- a/drivers/mmc/host/sh_mmcif.c +++ b/drivers/mmc/host/sh_mmcif.c @@ -388,7 +388,7 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, { struct dma_slave_config cfg = { 0, }; struct dma_chan *chan; - unsigned int slave_id; + void *slave_data; struct resource *res; dma_cap_mask_t mask; int ret; @@ -414,8 +414,6 @@ sh_mmcif_request_dma_one(struct sh_mmcif_host *host, res = platform_get_resource(host->pd, IORESOURCE_MEM, 0); - /* In the OF case the driver will get the slave ID from the DT */ - cfg.slave_id = slave_id; cfg.direction = direction; if (direction == DMA_DEV_TO_MEM) { diff --git a/drivers/mmc/host/tmio_mmc_dma.c b/drivers/mmc/host/tmio_mmc_dma.c index 7d077388b9eb..2ba47ab689ff 100644 --- a/drivers/mmc/host/tmio_mmc_dma.c +++ b/drivers/mmc/host/tmio_mmc_dma.c @@ -288,8 +288,6 @@ 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; 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; @@ -307,8 +305,6 @@ 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; cfg.direction = DMA_DEV_TO_MEM; cfg.src_addr = cfg.dst_addr + pdata->dma->dma_rx_offset; cfg.src_addr_width = DMA_SLAVE_BUSWIDTH_2_BYTES; diff --git a/drivers/mtd/nand/sh_flctl.c b/drivers/mtd/nand/sh_flctl.c index a21c378f096a..c3ce81c1a716 100644 --- a/drivers/mtd/nand/sh_flctl.c +++ b/drivers/mtd/nand/sh_flctl.c @@ -159,7 +159,6 @@ static void flctl_setup_dma(struct sh_flctl *flctl) return; memset(&cfg, 0, sizeof(cfg)); - cfg.slave_id = pdata->slave_id_fifo0_tx; cfg.direction = DMA_MEM_TO_DEV; cfg.dst_addr = (dma_addr_t)FLDTFIFO(flctl); cfg.src_addr = 0; @@ -175,7 +174,6 @@ static void flctl_setup_dma(struct sh_flctl *flctl) if (!flctl->chan_fifo0_rx) goto err; - cfg.slave_id = pdata->slave_id_fifo0_rx; cfg.direction = DMA_DEV_TO_MEM; cfg.dst_addr = 0; cfg.src_addr = (dma_addr_t)FLDTFIFO(flctl); diff --git a/drivers/spi/spi-rspi.c b/drivers/spi/spi-rspi.c index 2071f788c6fb..6cbcdc04c947 100644 --- a/drivers/spi/spi-rspi.c +++ b/drivers/spi/spi-rspi.c @@ -918,7 +918,6 @@ static struct dma_chan *rspi_request_dma_chan(struct device *dev, } memset(&cfg, 0, sizeof(cfg)); - cfg.slave_id = id; cfg.direction = dir; if (dir == DMA_MEM_TO_DEV) { cfg.dst_addr = port_addr; diff --git a/drivers/spi/spi-sh-msiof.c b/drivers/spi/spi-sh-msiof.c index 239be7cbe5a8..786ac9ec11fc 100644 --- a/drivers/spi/spi-sh-msiof.c +++ b/drivers/spi/spi-sh-msiof.c @@ -984,7 +984,6 @@ static struct dma_chan *sh_msiof_request_dma_chan(struct device *dev, } memset(&cfg, 0, sizeof(cfg)); - cfg.slave_id = id; cfg.direction = dir; if (dir == DMA_MEM_TO_DEV) { cfg.dst_addr = port_addr; diff --git a/include/linux/shdma-base.h b/include/linux/shdma-base.h index abdf1f229dc3..dd0ba502ccb3 100644 --- a/include/linux/shdma-base.h +++ b/include/linux/shdma-base.h @@ -69,6 +69,7 @@ struct shdma_chan { int id; /* Raw id of this channel */ int irq; /* Channel IRQ */ int slave_id; /* Client ID for slave DMA */ + int real_slave_id; /* argument passed to filter function */ int hw_req; /* DMA request line for slave DMA - same * as MID/RID, used with DT */ enum shdma_pm_state pm_state;