[PATCH 00/11] dmaengine: kill off dma_slave_config->slave_id
From: Arnd Bergmann arnd@arndb.de
I recently came across some new uses of the 'slave_id' field that I had (almost) removed a few years ago. There are no legitimate uses of this field in the kernel, only a few stale references and two drivers that abuse the field as a side-channel between the dmaengine driver and its client.
Let's change the xilinx and qualcomm drivers to use the documented side-channel (peripheral_data) instead, and remove the remnants of it to prevent new users from coming in.
As the last patch in the series depends on all the others, it would be nice have everything merged into the dmaengine tree for v5.17.
Arnd
Arnd Bergmann (11): ASoC: dai_dma: remove slave_id field spi: pic32: stop setting dma_config->slave_id mmc: bcm2835: stop setting chan_config->slave_id dmaengine: shdma: remove legacy slave_id parsing dmaengine: pxa/mmp: stop referencing config->slave_id dmaengine: sprd: stop referencing config->slave_id dmaengine: qcom-adm: stop abusing slave_id config dmaengine: xilinx_dpdma: stop using slave_id field dmaengine: tegra20-apb: stop checking config->slave_id staging: ralink-gdma: stop using slave_id config dmaengine: remove slave_id config field
drivers/dma/mmp_pdma.c | 6 --- drivers/dma/pxa_dma.c | 7 --- drivers/dma/qcom/qcom_adm.c | 56 ++++++++++++++++++++--- drivers/dma/sh/shdma-base.c | 8 ---- drivers/dma/sprd-dma.c | 3 -- drivers/dma/tegra20-apb-dma.c | 6 --- drivers/dma/xilinx/xilinx_dpdma.c | 12 +++-- drivers/gpu/drm/xlnx/zynqmp_disp.c | 9 +++- drivers/mmc/host/bcm2835.c | 2 - drivers/mtd/nand/raw/qcom_nandc.c | 14 +++++- drivers/spi/spi-pic32.c | 2 - drivers/staging/ralink-gdma/ralink-gdma.c | 12 ++--- drivers/tty/serial/msm_serial.c | 15 +++++- include/linux/dma/qcom_adm.h | 12 +++++ include/linux/dma/xilinx_dpdma.h | 11 +++++ include/linux/dmaengine.h | 4 -- include/sound/dmaengine_pcm.h | 2 - sound/core/pcm_dmaengine.c | 5 +- sound/soc/tegra/tegra20_spdif.c | 1 - 19 files changed, 119 insertions(+), 68 deletions(-) create mode 100644 include/linux/dma/qcom_adm.h create mode 100644 include/linux/dma/xilinx_dpdma.h
From: Arnd Bergmann arnd@arndb.de
This field is never set, and serves no purpose, so remove it.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- include/sound/dmaengine_pcm.h | 2 -- sound/core/pcm_dmaengine.c | 5 ++--- sound/soc/tegra/tegra20_spdif.c | 1 - 3 files changed, 2 insertions(+), 6 deletions(-)
diff --git a/include/sound/dmaengine_pcm.h b/include/sound/dmaengine_pcm.h index 9144bd547851..7403870c28bd 100644 --- a/include/sound/dmaengine_pcm.h +++ b/include/sound/dmaengine_pcm.h @@ -58,7 +58,6 @@ struct dma_chan *snd_dmaengine_pcm_get_chan(struct snd_pcm_substream *substream) * @maxburst: Maximum number of words(note: words, as in units of the * src_addr_width member, not bytes) that can be send to or received from the * DAI in one burst. - * @slave_id: Slave requester id for the DMA channel. * @filter_data: Custom DMA channel filter data, this will usually be used when * requesting the DMA channel. * @chan_name: Custom channel name to use when requesting DMA channel. @@ -72,7 +71,6 @@ struct snd_dmaengine_dai_dma_data { dma_addr_t addr; enum dma_slave_buswidth addr_width; u32 maxburst; - unsigned int slave_id; void *filter_data; const char *chan_name; unsigned int fifo_size; diff --git a/sound/core/pcm_dmaengine.c b/sound/core/pcm_dmaengine.c index af08bb4bf578..6762fb2083e1 100644 --- a/sound/core/pcm_dmaengine.c +++ b/sound/core/pcm_dmaengine.c @@ -91,8 +91,8 @@ EXPORT_SYMBOL_GPL(snd_hwparams_to_dma_slave_config); * @dma_data: DAI DMA data * @slave_config: DMA slave configuration * - * Initializes the {dst,src}_addr, {dst,src}_maxburst, {dst,src}_addr_width and - * slave_id fields of the DMA slave config from the same fields of the DAI DMA + * Initializes the {dst,src}_addr, {dst,src}_maxburst, {dst,src}_addr_width + * fields of the DMA slave config from the same fields of the DAI DMA * data struct. The src and dst fields will be initialized depending on the * direction of the substream. If the substream is a playback stream the dst * fields will be initialized, if it is a capture stream the src fields will be @@ -124,7 +124,6 @@ void snd_dmaengine_pcm_set_config_from_dai_data( slave_config->src_addr_width = dma_data->addr_width; }
- slave_config->slave_id = dma_data->slave_id; slave_config->peripheral_config = dma_data->peripheral_config; slave_config->peripheral_size = dma_data->peripheral_size; } diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c index 9fdc82d58db3..1c3385da6f82 100644 --- a/sound/soc/tegra/tegra20_spdif.c +++ b/sound/soc/tegra/tegra20_spdif.c @@ -284,7 +284,6 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev) spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT; spdif->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; spdif->playback_dma_data.maxburst = 4; - spdif->playback_dma_data.slave_id = dmareq->start;
pm_runtime_enable(&pdev->dev);
On 11/15/21 9:53 AM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
This field is never set, and serves no purpose, so remove it.
I agree that we should remove it. Its been legacy support code for a while, but the description that there is no user is not right.
The tegra20_spdif driver obviously uses it and that user is removed in this patch. I think it makes sense to split that out into a separate patch with a description why the driver will still work even with slave_id removed. Maybe the best is to remove the whole tegra20_spdif driver.
diff --git a/sound/soc/tegra/tegra20_spdif.c b/sound/soc/tegra/tegra20_spdif.c index 9fdc82d58db3..1c3385da6f82 100644 --- a/sound/soc/tegra/tegra20_spdif.c +++ b/sound/soc/tegra/tegra20_spdif.c @@ -284,7 +284,6 @@ static int tegra20_spdif_platform_probe(struct platform_device *pdev) spdif->playback_dma_data.addr = mem->start + TEGRA20_SPDIF_DATA_OUT; spdif->playback_dma_data.addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; spdif->playback_dma_data.maxburst = 4;
- spdif->playback_dma_data.slave_id = dmareq->start;
dmareq is now unused and should be removed as well.
pm_runtime_enable(&pdev->dev);
On Mon, Nov 15, 2021 at 11:14 AM Lars-Peter Clausen lars@metafoo.de wrote:
On 11/15/21 9:53 AM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
This field is never set, and serves no purpose, so remove it.
I agree that we should remove it. Its been legacy support code for a while, but the description that there is no user is not right.
The tegra20_spdif driver obviously uses it and that user is removed in this patch. I think it makes sense to split that out into a separate patch with a description why the driver will still work even with slave_id removed. Maybe the best is to remove the whole tegra20_spdif driver.
Ok, I'll split out the tegra patch and try to come up with a better description for it. What I saw in that driver is it just passes down the slave_id number from a 'struct resource', but there is nothing in the kernel that sets up this resource.
Do you or someone else have more information on the state of this driver? I can see that it does not contain any of_device_id based probing, so it seems that this is either dead code, the platform_device gets created by some other code that is no longer compatible with this driver.
Arnd
On 11/15/21 11:42 AM, Arnd Bergmann wrote:
On Mon, Nov 15, 2021 at 11:14 AM Lars-Peter Clausen lars@metafoo.de wrote:
On 11/15/21 9:53 AM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
This field is never set, and serves no purpose, so remove it.
I agree that we should remove it. Its been legacy support code for a while, but the description that there is no user is not right.
The tegra20_spdif driver obviously uses it and that user is removed in this patch. I think it makes sense to split that out into a separate patch with a description why the driver will still work even with slave_id removed. Maybe the best is to remove the whole tegra20_spdif driver.
Ok, I'll split out the tegra patch and try to come up with a better description for it. What I saw in that driver is it just passes down the slave_id number from a 'struct resource', but there is nothing in the kernel that sets up this resource.
Do you or someone else have more information on the state of this driver? I can see that it does not contain any of_device_id based probing, so it seems that this is either dead code, the platform_device gets created by some other code that is no longer compatible with this driver.
I've looked into this a while back, when I tried to remove slave_id. And as far as I can tell there were never any in-tree users of this driver, even back when we used platform board files. Maybe somebody from Nvidia knows if there are out-of-tree users.
- Lars
15.11.2021 14:53, Lars-Peter Clausen пишет:
On 11/15/21 11:42 AM, Arnd Bergmann wrote:
On Mon, Nov 15, 2021 at 11:14 AM Lars-Peter Clausen lars@metafoo.de wrote:
On 11/15/21 9:53 AM, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
This field is never set, and serves no purpose, so remove it.
I agree that we should remove it. Its been legacy support code for a while, but the description that there is no user is not right.
The tegra20_spdif driver obviously uses it and that user is removed in this patch. I think it makes sense to split that out into a separate patch with a description why the driver will still work even with slave_id removed. Maybe the best is to remove the whole tegra20_spdif driver.
Ok, I'll split out the tegra patch and try to come up with a better description for it. What I saw in that driver is it just passes down the slave_id number from a 'struct resource', but there is nothing in the kernel that sets up this resource.
Do you or someone else have more information on the state of this driver? I can see that it does not contain any of_device_id based probing, so it seems that this is either dead code, the platform_device gets created by some other code that is no longer compatible with this driver.
I've looked into this a while back, when I tried to remove slave_id. And as far as I can tell there were never any in-tree users of this driver, even back when we used platform board files. Maybe somebody from Nvidia knows if there are out-of-tree users.
That Tegra SPDIF driver was never used. Still there is a growing interest nowadays in making it alive by implementing HDMI audio support for Tegra20 SoC. It was on my todo list for a long time, I'll try to prioritize that work 5.17, it shouldn't take much effort.
The slave_id should be removed anyways, it won't be needed.
On Mon, Nov 15, 2021 at 3:46 PM Dmitry Osipenko digetx@gmail.com wrote:
15.11.2021 14:53, Lars-Peter Clausen пишет: That Tegra SPDIF driver was never used. Still there is a growing interest nowadays in making it alive by implementing HDMI audio support for Tegra20 SoC. It was on my todo list for a long time, I'll try to prioritize that work 5.17, it shouldn't take much effort.
The slave_id should be removed anyways, it won't be needed.
Ok, thanks for the background, I'll mention that in the changelog text then.
Arnd
From: Arnd Bergmann arnd@arndb.de
Setting slave_id makes no sense with DT based probing, and should eventually get removed entirely. Address this driver by no longer setting the field here.
I could not find which DMA driver is used on PIC32, if it's in the tree at all, but none of the obvious ones even care about slave_id any more.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/spi/spi-pic32.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/spi/spi-pic32.c b/drivers/spi/spi-pic32.c index 5eb7b61bbb4d..f86433b29260 100644 --- a/drivers/spi/spi-pic32.c +++ b/drivers/spi/spi-pic32.c @@ -370,7 +370,6 @@ static int pic32_spi_dma_config(struct pic32_spi *pic32s, u32 dma_width) cfg.src_addr_width = dma_width; cfg.dst_addr_width = dma_width; /* tx channel */ - cfg.slave_id = pic32s->tx_irq; cfg.direction = DMA_MEM_TO_DEV; ret = dmaengine_slave_config(master->dma_tx, &cfg); if (ret) { @@ -378,7 +377,6 @@ static int pic32_spi_dma_config(struct pic32_spi *pic32s, u32 dma_width) return ret; } /* rx channel */ - cfg.slave_id = pic32s->rx_irq; cfg.direction = DMA_DEV_TO_MEM; ret = dmaengine_slave_config(master->dma_rx, &cfg); if (ret)
On Mon, Nov 15, 2021 at 09:53:54AM +0100, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
Setting slave_id makes no sense with DT based probing, and should eventually get removed entirely. Address this driver by no longer setting the field here.
Acked-by: Mark Brown broonie@kernel.org
From: Arnd Bergmann arnd@arndb.de
The field is not interpreted by the DMA engine driver, as all the data is passed from devicetree instead. Remove the assignment so the field can eventually be deleted.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/mmc/host/bcm2835.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/mmc/host/bcm2835.c b/drivers/mmc/host/bcm2835.c index 8c2361e66277..463b707d9e99 100644 --- a/drivers/mmc/host/bcm2835.c +++ b/drivers/mmc/host/bcm2835.c @@ -1293,14 +1293,12 @@ static int bcm2835_add_host(struct bcm2835_host *host)
host->dma_cfg_tx.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; host->dma_cfg_tx.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; - host->dma_cfg_tx.slave_id = 13; /* DREQ channel */ host->dma_cfg_tx.direction = DMA_MEM_TO_DEV; host->dma_cfg_tx.src_addr = 0; host->dma_cfg_tx.dst_addr = host->phys_addr + SDDATA;
host->dma_cfg_rx.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; host->dma_cfg_rx.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; - host->dma_cfg_rx.slave_id = 13; /* DREQ channel */ host->dma_cfg_rx.direction = DMA_DEV_TO_MEM; host->dma_cfg_rx.src_addr = host->phys_addr + SDDATA; host->dma_cfg_rx.dst_addr = 0;
On Mon, 2021-11-15 at 09:53 +0100, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
The field is not interpreted by the DMA engine driver, as all the data is passed from devicetree instead. Remove the assignment so the field can eventually be deleted.
Signed-off-by: Arnd Bergmann arnd@arndb.de
Reviewed-by: Nicolas Saenz Julienne nsaenz@kernel.org
Regards, Nicolas
On Mon, 15 Nov 2021 at 09:55, Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
The field is not interpreted by the DMA engine driver, as all the data is passed from devicetree instead. Remove the assignment so the field can eventually be deleted.
Signed-off-by: Arnd Bergmann arnd@arndb.de
Acked-by: Ulf Hansson ulf.hansson@linaro.org
Kind regards Uffe
drivers/mmc/host/bcm2835.c | 2 -- 1 file changed, 2 deletions(-)
diff --git a/drivers/mmc/host/bcm2835.c b/drivers/mmc/host/bcm2835.c index 8c2361e66277..463b707d9e99 100644 --- a/drivers/mmc/host/bcm2835.c +++ b/drivers/mmc/host/bcm2835.c @@ -1293,14 +1293,12 @@ static int bcm2835_add_host(struct bcm2835_host *host)
host->dma_cfg_tx.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; host->dma_cfg_tx.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
host->dma_cfg_tx.slave_id = 13; /* DREQ channel */ host->dma_cfg_tx.direction = DMA_MEM_TO_DEV; host->dma_cfg_tx.src_addr = 0; host->dma_cfg_tx.dst_addr = host->phys_addr + SDDATA; host->dma_cfg_rx.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES; host->dma_cfg_rx.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
host->dma_cfg_rx.slave_id = 13; /* DREQ channel */ host->dma_cfg_rx.direction = DMA_DEV_TO_MEM; host->dma_cfg_rx.src_addr = host->phys_addr + SDDATA; host->dma_cfg_rx.dst_addr = 0;
-- 2.30.2
From: Arnd Bergmann arnd@arndb.de
The slave device is picked through either devicetree or a filter function, and any remaining out-of-tree drivers would have warned about this usage since 2015.
Stop interpreting the field finally so it can be removed from the interface.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/dma/sh/shdma-base.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/drivers/dma/sh/shdma-base.c b/drivers/dma/sh/shdma-base.c index 7f72b3f4cd1a..41c6bc650fa3 100644 --- a/drivers/dma/sh/shdma-base.c +++ b/drivers/dma/sh/shdma-base.c @@ -786,14 +786,6 @@ static int shdma_config(struct dma_chan *chan, if (!config) return -EINVAL;
- /* - * 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; - /* * We could lock this, but you shouldn't be configuring the * channel, while using it...
Hi Arnd,
Thank you for the patch.
On Mon, Nov 15, 2021 at 09:53:56AM +0100, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
The slave device is picked through either devicetree or a filter function, and any remaining out-of-tree drivers would have warned about this usage since 2015.
Stop interpreting the field finally so it can be removed from the interface.
Signed-off-by: Arnd Bergmann arnd@arndb.de
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
drivers/dma/sh/shdma-base.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/drivers/dma/sh/shdma-base.c b/drivers/dma/sh/shdma-base.c index 7f72b3f4cd1a..41c6bc650fa3 100644 --- a/drivers/dma/sh/shdma-base.c +++ b/drivers/dma/sh/shdma-base.c @@ -786,14 +786,6 @@ static int shdma_config(struct dma_chan *chan, if (!config) return -EINVAL;
- /*
* 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;
- /*
- We could lock this, but you shouldn't be configuring the
- channel, while using it...
From: Arnd Bergmann arnd@arndb.de
The last driver referencing the slave_id on Marvell PXA and MMP platforms was the SPI driver, but this stopped doing so a long time ago, so the TODO from the earlier patch can no be removed.
Fixes: b729bf34535e ("spi/pxa2xx: Don't use slave_id of dma_slave_config") Fixes: 13b3006b8ebd ("dma: mmp_pdma: add filter function") Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/dma/mmp_pdma.c | 6 ------ drivers/dma/pxa_dma.c | 7 ------- 2 files changed, 13 deletions(-)
diff --git a/drivers/dma/mmp_pdma.c b/drivers/dma/mmp_pdma.c index a23563cd118b..5a53d7fcef01 100644 --- a/drivers/dma/mmp_pdma.c +++ b/drivers/dma/mmp_pdma.c @@ -727,12 +727,6 @@ static int mmp_pdma_config_write(struct dma_chan *dchan,
chan->dir = direction; chan->dev_addr = addr; - /* FIXME: drivers should be ported over to use the filter - * function. Once that's done, the following two lines can - * be removed. - */ - if (cfg->slave_id) - chan->drcmr = cfg->slave_id;
return 0; } diff --git a/drivers/dma/pxa_dma.c b/drivers/dma/pxa_dma.c index 52d04641e361..6078cc81892e 100644 --- a/drivers/dma/pxa_dma.c +++ b/drivers/dma/pxa_dma.c @@ -909,13 +909,6 @@ static void pxad_get_config(struct pxad_chan *chan, *dcmd |= PXA_DCMD_BURST16; else if (maxburst == 32) *dcmd |= PXA_DCMD_BURST32; - - /* FIXME: drivers should be ported over to use the filter - * function. Once that's done, the following two lines can - * be removed. - */ - if (chan->cfg.slave_id) - chan->drcmr = chan->cfg.slave_id; }
static struct dma_async_tx_descriptor *
From: Arnd Bergmann arnd@arndb.de
It appears that the code that reads the slave_id from the channel config was copied incorrectly from other drivers. Nothing ever sets this field on platforms that use this driver, so remove the reference.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/dma/sprd-dma.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c index 4357d2395e6b..7f158ef5672d 100644 --- a/drivers/dma/sprd-dma.c +++ b/drivers/dma/sprd-dma.c @@ -795,9 +795,6 @@ static int sprd_dma_fill_desc(struct dma_chan *chan, return dst_datawidth; }
- if (slave_cfg->slave_id) - schan->dev_id = slave_cfg->slave_id; - hw->cfg = SPRD_DMA_DONOT_WAIT_BDONE << SPRD_DMA_WAIT_BDONE_OFFSET;
/*
Hi Arnd,
On Mon, Nov 15, 2021 at 4:55 PM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
It appears that the code that reads the slave_id from the channel config was copied incorrectly from other drivers. Nothing ever sets this field on platforms that use this driver, so remove the reference.
Signed-off-by: Arnd Bergmann arnd@arndb.de
Thanks. LGTM. Reviewed-by: Baolin Wang baolin.wang7@gmail.com
-- Baolin Wang
From: Arnd Bergmann arnd@arndb.de
The slave_id was previously used to pick one DMA slave instead of another, but this is now done through the DMA descriptors in device tree.
For the qcom_adm driver, the configuration is documented in the DT binding to contain a tuple of device identifier and a "crci" field, but the implementation ends up using only a single cell for identifying the slave, with the crci getting passed in nonstandard properties of the device, and passed through the dma driver using the old slave_id field. Part of the problem apparently is that the nand driver ends up using only a single DMA request ID, but requires distinct values for "crci" depending on the type of transfer.
Change both the dmaengine driver and the two slave drivers to allow the documented binding to work in addition to the ad-hoc passing of crci values. In order to no longer abuse the slave_id field, pass the data using the "peripheral_config" mechanism instead.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/dma/qcom/qcom_adm.c | 56 +++++++++++++++++++++++++++---- drivers/mtd/nand/raw/qcom_nandc.c | 14 ++++++-- drivers/tty/serial/msm_serial.c | 15 +++++++-- include/linux/dma/qcom_adm.h | 12 +++++++ 4 files changed, 86 insertions(+), 11 deletions(-) create mode 100644 include/linux/dma/qcom_adm.h
diff --git a/drivers/dma/qcom/qcom_adm.c b/drivers/dma/qcom/qcom_adm.c index ee78bed8d60d..bb338b303af6 100644 --- a/drivers/dma/qcom/qcom_adm.c +++ b/drivers/dma/qcom/qcom_adm.c @@ -8,6 +8,7 @@ #include <linux/device.h> #include <linux/dmaengine.h> #include <linux/dma-mapping.h> +#include <linux/dma/qcom_adm.h> #include <linux/init.h> #include <linux/interrupt.h> #include <linux/io.h> @@ -140,6 +141,8 @@ struct adm_chan {
struct adm_async_desc *curr_txd; struct dma_slave_config slave; + u32 crci; + u32 mux; struct list_head node;
int error; @@ -379,8 +382,8 @@ static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan, return ERR_PTR(-EINVAL); }
- crci = achan->slave.slave_id & 0xf; - if (!crci || achan->slave.slave_id > 0x1f) { + crci = achan->crci & 0xf; + if (!crci || achan->crci > 0x1f) { dev_err(adev->dev, "invalid crci value\n"); return ERR_PTR(-EINVAL); } @@ -403,9 +406,7 @@ static struct dma_async_tx_descriptor *adm_prep_slave_sg(struct dma_chan *chan, if (!async_desc) return ERR_PTR(-ENOMEM);
- if (crci) - async_desc->mux = achan->slave.slave_id & ADM_CRCI_MUX_SEL ? - ADM_CRCI_CTL_MUX_SEL : 0; + async_desc->mux = achan->mux ? ADM_CRCI_CTL_MUX_SEL : 0; async_desc->crci = crci; async_desc->blk_size = blk_size; async_desc->dma_len = single_count * sizeof(struct adm_desc_hw_single) + @@ -488,10 +489,13 @@ static int adm_terminate_all(struct dma_chan *chan) static int adm_slave_config(struct dma_chan *chan, struct dma_slave_config *cfg) { struct adm_chan *achan = to_adm_chan(chan); + struct qcom_adm_peripheral_config *config = cfg->peripheral_config; unsigned long flag;
spin_lock_irqsave(&achan->vc.lock, flag); memcpy(&achan->slave, cfg, sizeof(struct dma_slave_config)); + if (cfg->peripheral_size == sizeof(config)) + achan->crci = config->crci; spin_unlock_irqrestore(&achan->vc.lock, flag);
return 0; @@ -694,6 +698,45 @@ static void adm_channel_init(struct adm_device *adev, struct adm_chan *achan, achan->vc.desc_free = adm_dma_free_desc; }
+/** + * adm_dma_xlate + * @dma_spec: pointer to DMA specifier as found in the device tree + * @ofdma: pointer to DMA controller data + * + * This can use either 1-cell or 2-cell formats, the first cell + * identifies the slave device, while the optional second cell + * contains the crci value. + * + * Returns pointer to appropriate dma channel on success or NULL on error. + */ +struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, + struct of_dma *ofdma) +{ + struct dma_device *dev = ofdma->of_dma_data; + struct dma_chan *chan, *candidate = NULL; + struct adm_chan *achan; + + if (!dev || dma_spec->args_count > 2) + return NULL; + + list_for_each_entry(chan, &dev->channels, device_node) + if (chan->chan_id == dma_spec->args[0]) { + candidate = chan; + break; + } + + if (!candidate) + return NULL; + + achan = to_adm_chan(candidate); + if (dma_spec->args_count == 2) + achan->crci = dma_spec->args[1]; + else + achan->crci = 0; + + return dma_get_slave_channel(candidate); +} + static int adm_dma_probe(struct platform_device *pdev) { struct adm_device *adev; @@ -838,8 +881,7 @@ static int adm_dma_probe(struct platform_device *pdev) goto err_disable_clks; }
- ret = of_dma_controller_register(pdev->dev.of_node, - of_dma_xlate_by_chan_id, + ret = of_dma_controller_register(pdev->dev.of_node, adm_dma_xlate, &adev->common); if (ret) goto err_unregister_dma; diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c index 04e6f7b26706..7c6efa3b6255 100644 --- a/drivers/mtd/nand/raw/qcom_nandc.c +++ b/drivers/mtd/nand/raw/qcom_nandc.c @@ -6,6 +6,7 @@ #include <linux/clk.h> #include <linux/slab.h> #include <linux/bitops.h> +#include <linux/dma/qcom_adm.h> #include <linux/dma-mapping.h> #include <linux/dmaengine.h> #include <linux/module.h> @@ -952,6 +953,7 @@ static int prep_adm_dma_desc(struct qcom_nand_controller *nandc, bool read, struct dma_async_tx_descriptor *dma_desc; struct scatterlist *sgl; struct dma_slave_config slave_conf; + struct qcom_adm_peripheral_config periph_conf = {}; enum dma_transfer_direction dir_eng; int ret;
@@ -983,11 +985,19 @@ static int prep_adm_dma_desc(struct qcom_nand_controller *nandc, bool read, if (read) { slave_conf.src_maxburst = 16; slave_conf.src_addr = nandc->base_dma + reg_off; - slave_conf.slave_id = nandc->data_crci; + if (nandc->data_crci) { + periph_conf.crci = nandc->data_crci; + slave_conf.peripheral_config = &periph_conf; + slave_conf.peripheral_size = sizeof(periph_conf); + } } else { slave_conf.dst_maxburst = 16; slave_conf.dst_addr = nandc->base_dma + reg_off; - slave_conf.slave_id = nandc->cmd_crci; + if (nandc->cmd_crci) { + periph_conf.crci = nandc->cmd_crci; + slave_conf.peripheral_config = &periph_conf; + slave_conf.peripheral_size = sizeof(periph_conf); + } }
ret = dmaengine_slave_config(nandc->chan, &slave_conf); diff --git a/drivers/tty/serial/msm_serial.c b/drivers/tty/serial/msm_serial.c index fcef7a961430..c6be09f44dc1 100644 --- a/drivers/tty/serial/msm_serial.c +++ b/drivers/tty/serial/msm_serial.c @@ -9,6 +9,7 @@
#include <linux/kernel.h> #include <linux/atomic.h> +#include <linux/dma/qcom_adm.h> #include <linux/dma-mapping.h> #include <linux/dmaengine.h> #include <linux/module.h> @@ -290,6 +291,7 @@ static void msm_request_tx_dma(struct msm_port *msm_port, resource_size_t base) { struct device *dev = msm_port->uart.dev; struct dma_slave_config conf; + struct qcom_adm_peripheral_config periph_conf = {}; struct msm_dma *dma; u32 crci = 0; int ret; @@ -308,7 +310,11 @@ static void msm_request_tx_dma(struct msm_port *msm_port, resource_size_t base) conf.device_fc = true; conf.dst_addr = base + UARTDM_TF; conf.dst_maxburst = UARTDM_BURST_SIZE; - conf.slave_id = crci; + if (crci) { + conf.peripheral_config = &periph_conf; + conf.peripheral_size = sizeof(periph_conf); + periph_conf.crci = crci; + }
ret = dmaengine_slave_config(dma->chan, &conf); if (ret) @@ -333,6 +339,7 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base) { struct device *dev = msm_port->uart.dev; struct dma_slave_config conf; + struct qcom_adm_peripheral_config periph_conf = {}; struct msm_dma *dma; u32 crci = 0; int ret; @@ -355,7 +362,11 @@ static void msm_request_rx_dma(struct msm_port *msm_port, resource_size_t base) conf.device_fc = true; conf.src_addr = base + UARTDM_RF; conf.src_maxburst = UARTDM_BURST_SIZE; - conf.slave_id = crci; + if (crci) { + conf.peripheral_config = &periph_conf; + conf.peripheral_size = sizeof(periph_conf); + periph_conf.crci = crci; + }
ret = dmaengine_slave_config(dma->chan, &conf); if (ret) diff --git a/include/linux/dma/qcom_adm.h b/include/linux/dma/qcom_adm.h new file mode 100644 index 000000000000..af20df674f0c --- /dev/null +++ b/include/linux/dma/qcom_adm.h @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: GPL-2.0-only +#ifndef __LINUX_DMA_QCOM_ADM_H +#define __LINUX_DMA_QCOM_ADM_H + +#include <linux/types.h> + +struct qcom_adm_peripheral_config { + u32 crci; + u32 mux; +}; + +#endif /* __LINUX_DMA_QCOM_ADM_H */
Hi Arnd,
I love your patch! Perhaps something to improve:
[auto build test WARNING on vkoul-dmaengine/next] [also build test WARNING on tiwai-sound/for-next staging/staging-testing linus/master v5.16-rc1 next-20211118] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/dmaengine-kill-off-dm... base: https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git next config: xtensa-buildonly-randconfig-r005-20211115 (attached as .config) compiler: xtensa-linux-gcc (GCC) 11.2.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/0day-ci/linux/commit/f2e7f9ee67ce784864f75db39f20d1060c93... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Arnd-Bergmann/dmaengine-kill-off-dma_slave_config-slave_id/20211115-165731 git checkout f2e7f9ee67ce784864f75db39f20d1060c932279 # save the attached .config to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross ARCH=xtensa
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/dma/qcom/qcom_adm.c:712:18: warning: no previous prototype for 'adm_dma_xlate' [-Wmissing-prototypes]
712 | struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, | ^~~~~~~~~~~~~
vim +/adm_dma_xlate +712 drivers/dma/qcom/qcom_adm.c
700 701 /** 702 * adm_dma_xlate 703 * @dma_spec: pointer to DMA specifier as found in the device tree 704 * @ofdma: pointer to DMA controller data 705 * 706 * This can use either 1-cell or 2-cell formats, the first cell 707 * identifies the slave device, while the optional second cell 708 * contains the crci value. 709 * 710 * Returns pointer to appropriate dma channel on success or NULL on error. 711 */
712 struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec,
713 struct of_dma *ofdma) 714 { 715 struct dma_device *dev = ofdma->of_dma_data; 716 struct dma_chan *chan, *candidate = NULL; 717 struct adm_chan *achan; 718 719 if (!dev || dma_spec->args_count > 2) 720 return NULL; 721 722 list_for_each_entry(chan, &dev->channels, device_node) 723 if (chan->chan_id == dma_spec->args[0]) { 724 candidate = chan; 725 break; 726 } 727 728 if (!candidate) 729 return NULL; 730 731 achan = to_adm_chan(candidate); 732 if (dma_spec->args_count == 2) 733 achan->crci = dma_spec->args[1]; 734 else 735 achan->crci = 0; 736 737 return dma_get_slave_channel(candidate); 738 } 739
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Arnd,
I love your patch! Perhaps something to improve:
[auto build test WARNING on vkoul-dmaengine/next] [also build test WARNING on tiwai-sound/for-next staging/staging-testing linus/master v5.16-rc2 next-20211125] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Arnd-Bergmann/dmaengine-kill-off-dm... base: https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git next config: riscv-randconfig-r016-20211115 (https://download.01.org/0day-ci/archive/20211125/202111251538.x6sJNCka-lkp@i...) compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project fbe72e41b99dc7994daac300d208a955be3e4a0a) reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # install riscv cross compiling tool for clang build # apt-get install binutils-riscv64-linux-gnu # https://github.com/0day-ci/linux/commit/f2e7f9ee67ce784864f75db39f20d1060c93... git remote add linux-review https://github.com/0day-ci/linux git fetch --no-tags linux-review Arnd-Bergmann/dmaengine-kill-off-dma_slave_config-slave_id/20211115-165731 git checkout f2e7f9ee67ce784864f75db39f20d1060c932279 # save the config file to linux build tree COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=riscv
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/dma/qcom/qcom_adm.c:712:18: warning: no previous prototype for function 'adm_dma_xlate' [-Wmissing-prototypes]
struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, ^ drivers/dma/qcom/qcom_adm.c:712:1: note: declare 'static' if the function is not intended to be used outside of this translation unit struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, ^ static 1 warning generated.
vim +/adm_dma_xlate +712 drivers/dma/qcom/qcom_adm.c
700 701 /** 702 * adm_dma_xlate 703 * @dma_spec: pointer to DMA specifier as found in the device tree 704 * @ofdma: pointer to DMA controller data 705 * 706 * This can use either 1-cell or 2-cell formats, the first cell 707 * identifies the slave device, while the optional second cell 708 * contains the crci value. 709 * 710 * Returns pointer to appropriate dma channel on success or NULL on error. 711 */
712 struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec,
713 struct of_dma *ofdma) 714 { 715 struct dma_device *dev = ofdma->of_dma_data; 716 struct dma_chan *chan, *candidate = NULL; 717 struct adm_chan *achan; 718 719 if (!dev || dma_spec->args_count > 2) 720 return NULL; 721 722 list_for_each_entry(chan, &dev->channels, device_node) 723 if (chan->chan_id == dma_spec->args[0]) { 724 candidate = chan; 725 break; 726 } 727 728 if (!candidate) 729 return NULL; 730 731 achan = to_adm_chan(candidate); 732 if (dma_spec->args_count == 2) 733 achan->crci = dma_spec->args[1]; 734 else 735 achan->crci = 0; 736 737 return dma_get_slave_channel(candidate); 738 } 739
--- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On Thu, Nov 25, 2021 at 8:57 AM kernel test robot lkp@intel.com wrote:
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/dma/qcom/qcom_adm.c:712:18: warning: no previous prototype for function 'adm_dma_xlate' [-Wmissing-prototypes]
struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, ^ drivers/dma/qcom/qcom_adm.c:712:1: note: declare 'static' if the function is not intended to be used outside of this translation unit struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, ^ static 1 warning generated.
I noticed this mistake slipped into v2 as well, the function needs to be marked 'static'.
Vinod, let me know how you want me to address this. Should I just fold the fix (see below) and the final Acks into the patch and send an updated pull request, or do a complete v3 submission?
Arnd
8<--- diff --git a/drivers/dma/qcom/qcom_adm.c b/drivers/dma/qcom/qcom_adm.c index bb338b303af6..65697bee4db0 100644 --- a/drivers/dma/qcom/qcom_adm.c +++ b/drivers/dma/qcom/qcom_adm.c @@ -709,8 +709,8 @@ static void adm_channel_init(struct adm_device *adev, struct adm_chan *achan, * * Returns pointer to appropriate dma channel on success or NULL on error. */ -struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, - struct of_dma *ofdma) +static struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, + struct of_dma *ofdma) { struct dma_device *dev = ofdma->of_dma_data; struct dma_chan *chan, *candidate = NULL;
On 25-11-21, 09:25, Arnd Bergmann wrote:
On Thu, Nov 25, 2021 at 8:57 AM kernel test robot lkp@intel.com wrote:
If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot lkp@intel.com
All warnings (new ones prefixed by >>):
drivers/dma/qcom/qcom_adm.c:712:18: warning: no previous prototype for function 'adm_dma_xlate' [-Wmissing-prototypes]
struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, ^ drivers/dma/qcom/qcom_adm.c:712:1: note: declare 'static' if the function is not intended to be used outside of this translation unit struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec, ^ static 1 warning generated.
I noticed this mistake slipped into v2 as well, the function needs to be marked 'static'.
Vinod, let me know how you want me to address this. Should I just fold the fix (see below) and the final Acks into the patch and send an updated pull request, or do a complete v3 submission?
I can fold this while applying, the series lgtm, I will wait a day before applying...
Thanks
Arnd
8<--- diff --git a/drivers/dma/qcom/qcom_adm.c b/drivers/dma/qcom/qcom_adm.c index bb338b303af6..65697bee4db0 100644 --- a/drivers/dma/qcom/qcom_adm.c +++ b/drivers/dma/qcom/qcom_adm.c @@ -709,8 +709,8 @@ static void adm_channel_init(struct adm_device *adev, struct adm_chan *achan,
- Returns pointer to appropriate dma channel on success or NULL on error.
*/ -struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec,
struct of_dma *ofdma)
+static struct dma_chan *adm_dma_xlate(struct of_phandle_args *dma_spec,
struct of_dma *ofdma)
{ struct dma_device *dev = ofdma->of_dma_data; struct dma_chan *chan, *candidate = NULL;
From: Arnd Bergmann arnd@arndb.de
The display driver wants to pass a custom flag to the DMA engine driver, which it started doing by using the slave_id field that was traditionally used for a different purpose.
As there is no longer a correct use for the slave_id field, it should really be removed, and the remaining users changed over to something different.
The new mechanism for passing nonstandard settings is using the .peripheral_config field, so use that to pass a newly defined structure here, making it clear that this will not work in portable drivers.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/dma/xilinx/xilinx_dpdma.c | 12 ++++++++---- drivers/gpu/drm/xlnx/zynqmp_disp.c | 9 +++++++-- include/linux/dma/xilinx_dpdma.h | 11 +++++++++++ 3 files changed, 26 insertions(+), 6 deletions(-) create mode 100644 include/linux/dma/xilinx_dpdma.h
diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c index ce5c66e6897d..e2c1ef7a659c 100644 --- a/drivers/dma/xilinx/xilinx_dpdma.c +++ b/drivers/dma/xilinx/xilinx_dpdma.c @@ -12,6 +12,7 @@ #include <linux/clk.h> #include <linux/debugfs.h> #include <linux/delay.h> +#include <linux/dma/xilinx_dpdma.h> #include <linux/dmaengine.h> #include <linux/dmapool.h> #include <linux/interrupt.h> @@ -1273,6 +1274,7 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, struct dma_slave_config *config) { struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan); + struct xilinx_dpdma_peripheral_config *pconfig; unsigned long flags;
/* @@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, spin_lock_irqsave(&chan->lock, flags);
/* - * Abuse the slave_id to indicate that the channel is part of a video - * group. + * Abuse the peripheral_config to indicate that the channel is part + * of a video group. */ - if (chan->id <= ZYNQMP_DPDMA_VIDEO2) - chan->video_group = config->slave_id != 0; + pconfig = config->peripheral_config; + if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && + config->peripheral_size == sizeof(*pconfig)) + chan->video_group = pconfig->video_group;
spin_unlock_irqrestore(&chan->lock, flags);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index ff2b308d8651..11c409cbc88e 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -24,6 +24,7 @@
#include <linux/clk.h> #include <linux/delay.h> +#include <linux/dma/xilinx_dpdma.h> #include <linux/dma-mapping.h> #include <linux/dmaengine.h> #include <linux/module.h> @@ -1058,14 +1059,18 @@ static void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
/* - * Set slave_id for each DMA channel to indicate they're part of a + * Set pconfig for each DMA channel to indicate they're part of a * video group. */ for (i = 0; i < info->num_planes; i++) { struct zynqmp_disp_layer_dma *dma = &layer->dmas[i]; + struct xilinx_dpdma_peripheral_config pconfig = { + .video_group = true, + }; struct dma_slave_config config = { .direction = DMA_MEM_TO_DEV, - .slave_id = 1, + .peripheral_config = &pconfig, + .peripheral_size = sizeof(pconfig), };
dmaengine_slave_config(dma->chan, &config); diff --git a/include/linux/dma/xilinx_dpdma.h b/include/linux/dma/xilinx_dpdma.h new file mode 100644 index 000000000000..83a1377f03f8 --- /dev/null +++ b/include/linux/dma/xilinx_dpdma.h @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: GPL-2.0 +#ifndef __LINUX_DMA_XILINX_DPDMA_H +#define __LINUX_DMA_XILINX_DPDMA_H + +#include <linux/types.h> + +struct xilinx_dpdma_peripheral_config { + bool video_group; +}; + +#endif /* __LINUX_DMA_XILINX_DPDMA_H */
Hi Arnd,
Thank you for the patch.
On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
The display driver wants to pass a custom flag to the DMA engine driver, which it started doing by using the slave_id field that was traditionally used for a different purpose.
As there is no longer a correct use for the slave_id field, it should really be removed, and the remaining users changed over to something different.
The new mechanism for passing nonstandard settings is using the .peripheral_config field, so use that to pass a newly defined structure here, making it clear that this will not work in portable drivers.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/dma/xilinx/xilinx_dpdma.c | 12 ++++++++---- drivers/gpu/drm/xlnx/zynqmp_disp.c | 9 +++++++-- include/linux/dma/xilinx_dpdma.h | 11 +++++++++++ 3 files changed, 26 insertions(+), 6 deletions(-) create mode 100644 include/linux/dma/xilinx_dpdma.h
diff --git a/drivers/dma/xilinx/xilinx_dpdma.c b/drivers/dma/xilinx/xilinx_dpdma.c index ce5c66e6897d..e2c1ef7a659c 100644 --- a/drivers/dma/xilinx/xilinx_dpdma.c +++ b/drivers/dma/xilinx/xilinx_dpdma.c @@ -12,6 +12,7 @@ #include <linux/clk.h> #include <linux/debugfs.h> #include <linux/delay.h> +#include <linux/dma/xilinx_dpdma.h> #include <linux/dmaengine.h> #include <linux/dmapool.h> #include <linux/interrupt.h> @@ -1273,6 +1274,7 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, struct dma_slave_config *config) { struct xilinx_dpdma_chan *chan = to_xilinx_chan(dchan);
struct xilinx_dpdma_peripheral_config *pconfig; unsigned long flags;
/*
@@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, spin_lock_irqsave(&chan->lock, flags);
/*
* Abuse the slave_id to indicate that the channel is part of a video
* group.
* Abuse the peripheral_config to indicate that the channel is part
Is it still an abuse, or is this now the right way to pass custom data to the DMA engine driver ?
*/* of a video group.
- if (chan->id <= ZYNQMP_DPDMA_VIDEO2)
chan->video_group = config->slave_id != 0;
- pconfig = config->peripheral_config;
This could be moved to the variable declaration above, up to you.
- if (chan->id <= ZYNQMP_DPDMA_VIDEO2 &&
config->peripheral_size == sizeof(*pconfig))
Silently ignoring a size mismatch isn't nice. Could we validate the size at the beginning of the function and return an error ?
With these issues addressed,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
chan->video_group = pconfig->video_group;
spin_unlock_irqrestore(&chan->lock, flags);
diff --git a/drivers/gpu/drm/xlnx/zynqmp_disp.c b/drivers/gpu/drm/xlnx/zynqmp_disp.c index ff2b308d8651..11c409cbc88e 100644 --- a/drivers/gpu/drm/xlnx/zynqmp_disp.c +++ b/drivers/gpu/drm/xlnx/zynqmp_disp.c @@ -24,6 +24,7 @@
#include <linux/clk.h> #include <linux/delay.h> +#include <linux/dma/xilinx_dpdma.h> #include <linux/dma-mapping.h> #include <linux/dmaengine.h> #include <linux/module.h> @@ -1058,14 +1059,18 @@ static void zynqmp_disp_layer_set_format(struct zynqmp_disp_layer *layer, zynqmp_disp_avbuf_set_format(layer->disp, layer, layer->disp_fmt);
/*
* Set slave_id for each DMA channel to indicate they're part of a
* Set pconfig for each DMA channel to indicate they're part of a
*/ for (i = 0; i < info->num_planes; i++) { struct zynqmp_disp_layer_dma *dma = &layer->dmas[i];
- video group.
struct xilinx_dpdma_peripheral_config pconfig = {
.video_group = true,
struct dma_slave_config config = { .direction = DMA_MEM_TO_DEV,};
.slave_id = 1,
.peripheral_config = &pconfig,
.peripheral_size = sizeof(pconfig),
};
dmaengine_slave_config(dma->chan, &config);
diff --git a/include/linux/dma/xilinx_dpdma.h b/include/linux/dma/xilinx_dpdma.h new file mode 100644 index 000000000000..83a1377f03f8 --- /dev/null +++ b/include/linux/dma/xilinx_dpdma.h @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: GPL-2.0 +#ifndef __LINUX_DMA_XILINX_DPDMA_H +#define __LINUX_DMA_XILINX_DPDMA_H
+#include <linux/types.h>
+struct xilinx_dpdma_peripheral_config {
- bool video_group;
+};
+#endif /* __LINUX_DMA_XILINX_DPDMA_H */
On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote:
@@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, spin_lock_irqsave(&chan->lock, flags);
/*
* Abuse the slave_id to indicate that the channel is part of a video
* group.
* Abuse the peripheral_config to indicate that the channel is part
Is it still an abuse, or is this now the right way to pass custom data to the DMA engine driver ?
It doesn't make the driver any more portable, but it's now being more explicit about it. As far as I can tell, this is the best way to pass data that cannot be expressed through the regular interfaces in DT and the dmaengine API.
Ideally there would be a generic way to pass this flag, but I couldn't figure out what this is actually doing, or whether there is a better way. Maybe Vinod has an idea.
I'll change s/Abuse/Use/ for the moment until I get a definite answer.
* of a video group. */
if (chan->id <= ZYNQMP_DPDMA_VIDEO2)
chan->video_group = config->slave_id != 0;
pconfig = config->peripheral_config;
This could be moved to the variable declaration above, up to you.
I considered that but since it doesn't fit in a normal 80-column line, it seemed best to do it here.
if (chan->id <= ZYNQMP_DPDMA_VIDEO2 &&
config->peripheral_size == sizeof(*pconfig))
Silently ignoring a size mismatch isn't nice. Could we validate the size at the beginning of the function and return an error ?
Yes, good idea. Since this would mean a bug in another driver, I'll add a WARN_ON() as well to make it clear which driver caused it. This is what I have now, let me know if you have any further suggestions:
/* * Use the peripheral_config to indicate that the channel is part * of a video group. This requires matching use of the custom * structure in each driver. */ pconfig = config->peripheral_config; if (WARN_ON(config->peripheral_size != 0 && config->peripheral_size != sizeof(*pconfig))) return -EINVAL;
spin_lock_irqsave(&chan->lock, flags); if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && config->peripheral_size == sizeof(*pconfig)) chan->video_group = pconfig->video_group; spin_unlock_irqrestore(&chan->lock, flags);
return 0;
With these issues addressed,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Thanks,
Arnd
Hi Arnd,
On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote:
On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote:
On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote:
@@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, spin_lock_irqsave(&chan->lock, flags);
/*
* Abuse the slave_id to indicate that the channel is part of a video
* group.
* Abuse the peripheral_config to indicate that the channel is part
Is it still an abuse, or is this now the right way to pass custom data to the DMA engine driver ?
It doesn't make the driver any more portable, but it's now being more explicit about it. As far as I can tell, this is the best way to pass data that cannot be expressed through the regular interfaces in DT and the dmaengine API.
Ideally there would be a generic way to pass this flag, but I couldn't figure out what this is actually doing, or whether there is a better way. Maybe Vinod has an idea.
I don't think we need a generic API in this case. The DMA engine is specific to the display device, I don't foresee a need to mix-n-match.
I'll change s/Abuse/Use/ for the moment until I get a definite answer.
* of a video group. */
if (chan->id <= ZYNQMP_DPDMA_VIDEO2)
chan->video_group = config->slave_id != 0;
pconfig = config->peripheral_config;
This could be moved to the variable declaration above, up to you.
I considered that but since it doesn't fit in a normal 80-column line, it seemed best to do it here.
if (chan->id <= ZYNQMP_DPDMA_VIDEO2 &&
config->peripheral_size == sizeof(*pconfig))
Silently ignoring a size mismatch isn't nice. Could we validate the size at the beginning of the function and return an error ?
Yes, good idea. Since this would mean a bug in another driver, I'll add a WARN_ON() as well to make it clear which driver caused it. This is what I have now, let me know if you have any further suggestions:
/* * Use the peripheral_config to indicate that the channel is part * of a video group. This requires matching use of the custom * structure in each driver. */ pconfig = config->peripheral_config; if (WARN_ON(config->peripheral_size != 0 && config->peripheral_size != sizeof(*pconfig))) return -EINVAL;
How about
if (WARN_ON(config->peripheral_config && config->peripheral_size != sizeof(*pconfig)))
spin_lock_irqsave(&chan->lock, flags); if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && config->peripheral_size == sizeof(*pconfig))
And here you can test pconfig != NULL.
chan->video_group = pconfig->video_group; spin_unlock_irqrestore(&chan->lock, flags); return 0;
With these issues addressed,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
On Mon, Nov 15, 2021 at 12:49 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote:
On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote:
On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote:
@@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, spin_lock_irqsave(&chan->lock, flags);
/*
* Abuse the slave_id to indicate that the channel is part of a video
* group.
* Abuse the peripheral_config to indicate that the channel is part
Is it still an abuse, or is this now the right way to pass custom data to the DMA engine driver ?
It doesn't make the driver any more portable, but it's now being more explicit about it. As far as I can tell, this is the best way to pass data that cannot be expressed through the regular interfaces in DT and the dmaengine API.
Ideally there would be a generic way to pass this flag, but I couldn't figure out what this is actually doing, or whether there is a better way. Maybe Vinod has an idea.
I don't think we need a generic API in this case. The DMA engine is specific to the display device, I don't foresee a need to mix-n-match.
Right. I wonder if there is even a point in using the dmaengine API in that case, I think for other single-purpose drivers we tend to just integrate the functionality in the client driver. No point changing this now of course, but it does feel odd.
From my earlier reading of the driver, my impression was that this
is just a memory-to-memory device, so it could be used that way as well, but does need a flag when working on the video memory. I couldn't quite make sense of that though.
/* * Use the peripheral_config to indicate that the channel is part * of a video group. This requires matching use of the custom * structure in each driver. */ pconfig = config->peripheral_config; if (WARN_ON(config->peripheral_size != 0 && config->peripheral_size != sizeof(*pconfig))) return -EINVAL;
How about
if (WARN_ON(config->peripheral_config && config->peripheral_size != sizeof(*pconfig)))
spin_lock_irqsave(&chan->lock, flags); if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && config->peripheral_size == sizeof(*pconfig))
And here you can test pconfig != NULL.
Good idea. Changed now, using 'if (pconfig)' without the '!= NULL' in both expressions.
Arnd
Hi Arnd,
On Mon, Nov 15, 2021 at 01:38:07PM +0100, Arnd Bergmann wrote:
On Mon, Nov 15, 2021 at 12:49 PM Laurent Pinchart wrote:
On Mon, Nov 15, 2021 at 11:21:30AM +0100, Arnd Bergmann wrote:
On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart wrote:
On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote:
@@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, spin_lock_irqsave(&chan->lock, flags);
/*
* Abuse the slave_id to indicate that the channel is part of a video
* group.
* Abuse the peripheral_config to indicate that the channel is part
Is it still an abuse, or is this now the right way to pass custom data to the DMA engine driver ?
It doesn't make the driver any more portable, but it's now being more explicit about it. As far as I can tell, this is the best way to pass data that cannot be expressed through the regular interfaces in DT and the dmaengine API.
Ideally there would be a generic way to pass this flag, but I couldn't figure out what this is actually doing, or whether there is a better way. Maybe Vinod has an idea.
I don't think we need a generic API in this case. The DMA engine is specific to the display device, I don't foresee a need to mix-n-match.
Right. I wonder if there is even a point in using the dmaengine API in that case, I think for other single-purpose drivers we tend to just integrate the functionality in the client driver. No point changing this now of course, but it does feel odd.
I agree, and that's what I would have done as well, if it wasn't for the fact that the DMA engine also supports a second client for audio. This isn't supported in upstream yet. We could still have created an ad-hoc solution, possibly based on the components framework, but the DMA engine subsystem wasn't a bad fit.
From my earlier reading of the driver, my impression was that this is just a memory-to-memory device, so it could be used that way as well, but does need a flag when working on the video memory. I couldn't quite make sense of that though.
It's only memory-to-device (video and audio). See figures 33-1 and 33-16 in https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrasc...
/* * Use the peripheral_config to indicate that the channel is part * of a video group. This requires matching use of the custom * structure in each driver. */ pconfig = config->peripheral_config; if (WARN_ON(config->peripheral_size != 0 && config->peripheral_size != sizeof(*pconfig))) return -EINVAL;
How about
if (WARN_ON(config->peripheral_config && config->peripheral_size != sizeof(*pconfig)))
spin_lock_irqsave(&chan->lock, flags); if (chan->id <= ZYNQMP_DPDMA_VIDEO2 && config->peripheral_size == sizeof(*pconfig))
And here you can test pconfig != NULL.
Good idea. Changed now, using 'if (pconfig)' without the '!= NULL' in both expressions.
Sounds good to me.
On Mon, Nov 15, 2021 at 2:05 PM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Mon, Nov 15, 2021 at 01:38:07PM +0100, Arnd Bergmann wrote:
On Mon, Nov 15, 2021 at 12:49 PM Laurent Pinchart wrote:
Right. I wonder if there is even a point in using the dmaengine API in that case, I think for other single-purpose drivers we tend to just integrate the functionality in the client driver. No point changing this now of course, but it does feel odd.
I agree, and that's what I would have done as well, if it wasn't for the fact that the DMA engine also supports a second client for audio. This isn't supported in upstream yet. We could still have created an ad-hoc solution, possibly based on the components framework, but the DMA engine subsystem wasn't a bad fit.
Ah, makes sense. In this case, I guess the data could have been part of the DMA specifier after all, in a second cell after the channel number.
Arnd
On 15-11-21, 11:21, Arnd Bergmann wrote:
On Mon, Nov 15, 2021 at 10:14 AM Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
On Mon, Nov 15, 2021 at 09:54:00AM +0100, Arnd Bergmann wrote:
@@ -1285,11 +1287,13 @@ static int xilinx_dpdma_config(struct dma_chan *dchan, spin_lock_irqsave(&chan->lock, flags);
/*
* Abuse the slave_id to indicate that the channel is part of a video
* group.
* Abuse the peripheral_config to indicate that the channel is part
Is it still an abuse, or is this now the right way to pass custom data to the DMA engine driver ?
It doesn't make the driver any more portable, but it's now being more explicit about it. As far as I can tell, this is the best way to pass data that cannot be expressed through the regular interfaces in DT and the dmaengine API.
Ideally there would be a generic way to pass this flag, but I couldn't figure out what this is actually doing, or whether there is a better way. Maybe Vinod has an idea.
I'll change s/Abuse/Use/ for the moment until I get a definite answer.
I would feel this is still not use for the peripheral_config, but lets keep it to get rid of slave_id.
Also, I would be better if this was moved to DT as the next cell, don't recall why that was not done/feasible.
From: Arnd Bergmann arnd@arndb.de
Nothing sets the slave_id field any more, so stop accessing it to allow the removal of this field.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/dma/tegra20-apb-dma.c | 6 ------ 1 file changed, 6 deletions(-)
diff --git a/drivers/dma/tegra20-apb-dma.c b/drivers/dma/tegra20-apb-dma.c index b7260749e8ee..eaafcbe4ca94 100644 --- a/drivers/dma/tegra20-apb-dma.c +++ b/drivers/dma/tegra20-apb-dma.c @@ -343,12 +343,6 @@ static int tegra_dma_slave_config(struct dma_chan *dc, }
memcpy(&tdc->dma_sconfig, sconfig, sizeof(*sconfig)); - if (tdc->slave_id == TEGRA_APBDMA_SLAVE_ID_INVALID && - sconfig->device_fc) { - if (sconfig->slave_id > TEGRA_APBDMA_CSR_REQ_SEL_MASK) - return -EINVAL; - tdc->slave_id = sconfig->slave_id; - } tdc->config_init = true;
return 0;
From: Arnd Bergmann arnd@arndb.de
Picking the connection between a DMA controller and its attached device is done through devicetree using the "dmas" property, which is implemented by the gdma driver, but it also allows overriding the "req" configuration with the slave_id field, as it was done in some linux-2.6 era drivers.
There is no driver in the tree that sets these values, so stop interpreting them before anything accidentally starts relying on it. Rename the field in the channel from "slave_id" to "req" to better match the purpose and the naming in the hardware.
If any driver actually starts using this DMA engine, it may be necessary to implement a .xlate callback that sets this field at probe time.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- drivers/staging/ralink-gdma/ralink-gdma.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
diff --git a/drivers/staging/ralink-gdma/ralink-gdma.c b/drivers/staging/ralink-gdma/ralink-gdma.c index b5229bc6eae5..f00240e62e1b 100644 --- a/drivers/staging/ralink-gdma/ralink-gdma.c +++ b/drivers/staging/ralink-gdma/ralink-gdma.c @@ -106,7 +106,7 @@ struct gdma_dma_desc { struct gdma_dmaengine_chan { struct virt_dma_chan vchan; unsigned int id; - unsigned int slave_id; + unsigned int req;
dma_addr_t fifo_addr; enum gdma_dma_transfer_size burst_size; @@ -194,7 +194,6 @@ static int gdma_dma_config(struct dma_chan *c, dev_err(dma_dev->ddev.dev, "only support 4 byte buswidth\n"); return -EINVAL; } - chan->slave_id = config->slave_id; chan->fifo_addr = config->dst_addr; chan->burst_size = gdma_dma_maxburst(config->dst_maxburst); break; @@ -203,7 +202,6 @@ static int gdma_dma_config(struct dma_chan *c, dev_err(dma_dev->ddev.dev, "only support 4 byte buswidth\n"); return -EINVAL; } - chan->slave_id = config->slave_id; chan->fifo_addr = config->src_addr; chan->burst_size = gdma_dma_maxburst(config->src_maxburst); break; @@ -288,12 +286,12 @@ static int rt305x_gdma_start_transfer(struct gdma_dmaengine_chan *chan) dst_addr = chan->fifo_addr; ctrl0 = GDMA_REG_CTRL0_DST_ADDR_FIXED | (8 << GDMA_RT305X_CTRL0_SRC_REQ_SHIFT) | - (chan->slave_id << GDMA_RT305X_CTRL0_DST_REQ_SHIFT); + (chan->req << GDMA_RT305X_CTRL0_DST_REQ_SHIFT); } else if (chan->desc->direction == DMA_DEV_TO_MEM) { src_addr = chan->fifo_addr; dst_addr = sg->dst_addr; ctrl0 = GDMA_REG_CTRL0_SRC_ADDR_FIXED | - (chan->slave_id << GDMA_RT305X_CTRL0_SRC_REQ_SHIFT) | + (chan->req << GDMA_RT305X_CTRL0_SRC_REQ_SHIFT) | (8 << GDMA_RT305X_CTRL0_DST_REQ_SHIFT); } else if (chan->desc->direction == DMA_MEM_TO_MEM) { /* @@ -365,12 +363,12 @@ static int rt3883_gdma_start_transfer(struct gdma_dmaengine_chan *chan) dst_addr = chan->fifo_addr; ctrl0 = GDMA_REG_CTRL0_DST_ADDR_FIXED; ctrl1 = (32 << GDMA_REG_CTRL1_SRC_REQ_SHIFT) | - (chan->slave_id << GDMA_REG_CTRL1_DST_REQ_SHIFT); + (chan->req << GDMA_REG_CTRL1_DST_REQ_SHIFT); } else if (chan->desc->direction == DMA_DEV_TO_MEM) { src_addr = chan->fifo_addr; dst_addr = sg->dst_addr; ctrl0 = GDMA_REG_CTRL0_SRC_ADDR_FIXED; - ctrl1 = (chan->slave_id << GDMA_REG_CTRL1_SRC_REQ_SHIFT) | + ctrl1 = (chan->req << GDMA_REG_CTRL1_SRC_REQ_SHIFT) | (32 << GDMA_REG_CTRL1_DST_REQ_SHIFT) | GDMA_REG_CTRL1_COHERENT; } else if (chan->desc->direction == DMA_MEM_TO_MEM) {
Hi Arnd,
On Mon, Nov 15, 2021 at 9:55 AM Arnd Bergmann arnd@kernel.org wrote:
From: Arnd Bergmann arnd@arndb.de
Picking the connection between a DMA controller and its attached device is done through devicetree using the "dmas" property, which is implemented by the gdma driver, but it also allows overriding the "req" configuration with the slave_id field, as it was done in some linux-2.6 era drivers.
There is no driver in the tree that sets these values, so stop interpreting them before anything accidentally starts relying on it. Rename the field in the channel from "slave_id" to "req" to better match the purpose and the naming in the hardware.
If any driver actually starts using this DMA engine, it may be necessary to implement a .xlate callback that sets this field at probe time.
Signed-off-by: Arnd Bergmann arnd@arndb.de
drivers/staging/ralink-gdma/ralink-gdma.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
This driver has been already deleted from the staging tree. See [0].
Best regards, Sergio Paracuellos
[0]: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h...
diff --git a/drivers/staging/ralink-gdma/ralink-gdma.c b/drivers/staging/ralink-gdma/ralink-gdma.c index b5229bc6eae5..f00240e62e1b 100644 --- a/drivers/staging/ralink-gdma/ralink-gdma.c +++ b/drivers/staging/ralink-gdma/ralink-gdma.c @@ -106,7 +106,7 @@ struct gdma_dma_desc { struct gdma_dmaengine_chan { struct virt_dma_chan vchan; unsigned int id;
unsigned int slave_id;
unsigned int req; dma_addr_t fifo_addr; enum gdma_dma_transfer_size burst_size;
@@ -194,7 +194,6 @@ static int gdma_dma_config(struct dma_chan *c, dev_err(dma_dev->ddev.dev, "only support 4 byte buswidth\n"); return -EINVAL; }
chan->slave_id = config->slave_id; chan->fifo_addr = config->dst_addr; chan->burst_size = gdma_dma_maxburst(config->dst_maxburst); break;
@@ -203,7 +202,6 @@ static int gdma_dma_config(struct dma_chan *c, dev_err(dma_dev->ddev.dev, "only support 4 byte buswidth\n"); return -EINVAL; }
chan->slave_id = config->slave_id; chan->fifo_addr = config->src_addr; chan->burst_size = gdma_dma_maxburst(config->src_maxburst); break;
@@ -288,12 +286,12 @@ static int rt305x_gdma_start_transfer(struct gdma_dmaengine_chan *chan) dst_addr = chan->fifo_addr; ctrl0 = GDMA_REG_CTRL0_DST_ADDR_FIXED | (8 << GDMA_RT305X_CTRL0_SRC_REQ_SHIFT) |
(chan->slave_id << GDMA_RT305X_CTRL0_DST_REQ_SHIFT);
(chan->req << GDMA_RT305X_CTRL0_DST_REQ_SHIFT); } else if (chan->desc->direction == DMA_DEV_TO_MEM) { src_addr = chan->fifo_addr; dst_addr = sg->dst_addr; ctrl0 = GDMA_REG_CTRL0_SRC_ADDR_FIXED |
(chan->slave_id << GDMA_RT305X_CTRL0_SRC_REQ_SHIFT) |
(chan->req << GDMA_RT305X_CTRL0_SRC_REQ_SHIFT) | (8 << GDMA_RT305X_CTRL0_DST_REQ_SHIFT); } else if (chan->desc->direction == DMA_MEM_TO_MEM) { /*
@@ -365,12 +363,12 @@ static int rt3883_gdma_start_transfer(struct gdma_dmaengine_chan *chan) dst_addr = chan->fifo_addr; ctrl0 = GDMA_REG_CTRL0_DST_ADDR_FIXED; ctrl1 = (32 << GDMA_REG_CTRL1_SRC_REQ_SHIFT) |
(chan->slave_id << GDMA_REG_CTRL1_DST_REQ_SHIFT);
(chan->req << GDMA_REG_CTRL1_DST_REQ_SHIFT); } else if (chan->desc->direction == DMA_DEV_TO_MEM) { src_addr = chan->fifo_addr; dst_addr = sg->dst_addr; ctrl0 = GDMA_REG_CTRL0_SRC_ADDR_FIXED;
ctrl1 = (chan->slave_id << GDMA_REG_CTRL1_SRC_REQ_SHIFT) |
ctrl1 = (chan->req << GDMA_REG_CTRL1_SRC_REQ_SHIFT) | (32 << GDMA_REG_CTRL1_DST_REQ_SHIFT) | GDMA_REG_CTRL1_COHERENT; } else if (chan->desc->direction == DMA_MEM_TO_MEM) {
-- 2.30.2
On Mon, Nov 15, 2021 at 10:55 AM Sergio Paracuellos sergio.paracuellos@gmail.com wrote:
On Mon, Nov 15, 2021 at 9:55 AM Arnd Bergmann arnd@kernel.org wrote:
drivers/staging/ralink-gdma/ralink-gdma.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-)
This driver has been already deleted from the staging tree. See [0].
Ok, thanks! I'll just leave out the patch from future submissions, and remove it completely once your commit hits mainline.
Arnd
From: Arnd Bergmann arnd@arndb.de
All references to the slave_id field have been removed, so remove the field as well to prevent new references from creeping in again.
Signed-off-by: Arnd Bergmann arnd@arndb.de --- include/linux/dmaengine.h | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 9000f3ffce8b..0349b35235e6 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -418,9 +418,6 @@ enum dma_slave_buswidth { * @device_fc: Flow Controller Settings. Only valid for slave channels. Fill * with 'true' if peripheral should be flow controller. Direction will be * selected at Runtime. - * @slave_id: Slave requester id. Only valid for slave channels. The dma - * slave peripheral will have unique id as dma requester which need to be - * pass as slave config. * @peripheral_config: peripheral configuration for programming peripheral * for dmaengine transfer * @peripheral_size: peripheral configuration buffer size @@ -448,7 +445,6 @@ struct dma_slave_config { u32 src_port_window_size; u32 dst_port_window_size; bool device_fc; - unsigned int slave_id; void *peripheral_config; size_t peripheral_size; };
Hi Arnd,
Thank you for the patch.
On Mon, Nov 15, 2021 at 09:54:03AM +0100, Arnd Bergmann wrote:
From: Arnd Bergmann arnd@arndb.de
All references to the slave_id field have been removed, so remove the field as well to prevent new references from creeping in again.
A rationale to explain why the slave_id field shouldn't be used would be nice.
Signed-off-by: Arnd Bergmann arnd@arndb.de
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
include/linux/dmaengine.h | 4 ---- 1 file changed, 4 deletions(-)
diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 9000f3ffce8b..0349b35235e6 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -418,9 +418,6 @@ enum dma_slave_buswidth {
- @device_fc: Flow Controller Settings. Only valid for slave channels. Fill
- with 'true' if peripheral should be flow controller. Direction will be
- selected at Runtime.
- @slave_id: Slave requester id. Only valid for slave channels. The dma
- slave peripheral will have unique id as dma requester which need to be
- pass as slave config.
- @peripheral_config: peripheral configuration for programming peripheral
- for dmaengine transfer
- @peripheral_size: peripheral configuration buffer size
@@ -448,7 +445,6 @@ struct dma_slave_config { u32 src_port_window_size; u32 dst_port_window_size; bool device_fc;
- unsigned int slave_id; void *peripheral_config; size_t peripheral_size;
};
participants (11)
-
Arnd Bergmann
-
Baolin Wang
-
Dmitry Osipenko
-
kernel test robot
-
Lars-Peter Clausen
-
Laurent Pinchart
-
Mark Brown
-
nicolas saenz julienne
-
Sergio Paracuellos
-
Ulf Hansson
-
Vinod Koul