| Message ID | 20220406161856.1669069-6-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | RZN1 DMA support | expand |
On Wed, 6 Apr 2022, Miquel Raynal wrote: > The Renesas RZN1 DMA IP is based on a DW core, with eg. an additional > dmamux register located in the system control area which can take up to > 32 requests (16 per DMA controller). Each DMA channel can be wired to > two different peripherals. > > We need two additional information from the 'dmas' property: the channel > (bit in the dmamux register) that must be accessed and the value of the > mux for this channel. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > +static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec, > + struct of_dma *ofdma) > +{ > + struct platform_device *pdev = of_find_device_by_node(ofdma->of_node); > + struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev); > + struct rzn1_dmamux_map *map; > + unsigned int dmac_idx, chan, val; > + u32 mask; > + int ret; > + > + if (dma_spec->args_count != 6) > + return ERR_PTR(-EINVAL); > + > + map = kzalloc(sizeof(*map), GFP_KERNEL); > + if (!map) > + return ERR_PTR(-ENOMEM); > + > + chan = dma_spec->args[0]; > + map->req_idx = dma_spec->args[4]; > + val = dma_spec->args[5]; > + dma_spec->args_count -= 2; > + > + if (chan >= RZN1_DMAMUX_MAX_LINES) { > + dev_err(&pdev->dev, "Invalid DMA request line: %u\n", chan); > + ret = -EINVAL; > + goto free_map; > + } > + > + if (map->req_idx >= RZN1_DMAMUX_LINES || > + (map->req_idx % RZN1_DMAMUX_MAX_LINES) != chan) { > + dev_err(&pdev->dev, "Invalid MUX request line: %u\n", map->req_idx); > + ret = -EINVAL; > + goto free_map; > + } > + > + dmac_idx = map->req_idx >= RZN1_DMAMUX_MAX_LINES ? 1 : 0; > + dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", dmac_idx); > + if (!dma_spec->np) { > + dev_err(&pdev->dev, "Can't get DMA master\n"); > + ret = -EINVAL; > + goto free_map; > + } > + > + dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n", > + map->req_idx, dmac_idx, chan); > + > + mask = BIT(map->req_idx); > + mutex_lock(&dmamux->lock); > + dmamux->used_chans |= mask; > + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); > + if (ret) > + goto release_chan_and_unlock; > + > + mutex_unlock(&dmamux->lock); > + > + return map; > + > +release_chan_and_unlock: > + dmamux->used_chans &= ~mask; Now that I check this again, I'm not sure why dmamux->used_chans |= mask; couldn't be done after r9a06g032_sysctrl_set_dmamux() call so this rollback of it wouldn't be necessary. Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
On Fri, Apr 08, 2022 at 12:55:47PM +0300, Ilpo Järvinen wrote: > On Wed, 6 Apr 2022, Miquel Raynal wrote: > > > The Renesas RZN1 DMA IP is based on a DW core, with eg. an additional > > dmamux register located in the system control area which can take up to > > 32 requests (16 per DMA controller). Each DMA channel can be wired to > > two different peripherals. > > > > We need two additional information from the 'dmas' property: the channel > > (bit in the dmamux register) that must be accessed and the value of the > > mux for this channel. > > + mask = BIT(map->req_idx); > > + mutex_lock(&dmamux->lock); > > + dmamux->used_chans |= mask; > > + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); > > + if (ret) > > + goto release_chan_and_unlock; > > + > > + mutex_unlock(&dmamux->lock); > > + > > + return map; > > + > > +release_chan_and_unlock: > > + dmamux->used_chans &= ~mask; > > Now that I check this again, I'm not sure why dmamux->used_chans |= mask; > couldn't be done after r9a06g032_sysctrl_set_dmamux() call so this > rollback of it wouldn't be necessary. I would still need the mutex unlock which I believe is down path there under some other label. Hence you are proposing something like mask = BIT(map->req_idx); mutex_lock(&dmamux->lock); ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); if (ret) goto err_unlock; // or whatever label is dmamux->used_chans |= mask; mutex_unlock(&dmamux->lock); return map; Is that correct? If so, I don't see impediments either. > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
On Fri, 8 Apr 2022, Andy Shevchenko wrote: > On Fri, Apr 08, 2022 at 12:55:47PM +0300, Ilpo Järvinen wrote: > > On Wed, 6 Apr 2022, Miquel Raynal wrote: > > > > > The Renesas RZN1 DMA IP is based on a DW core, with eg. an additional > > > dmamux register located in the system control area which can take up to > > > 32 requests (16 per DMA controller). Each DMA channel can be wired to > > > two different peripherals. > > > > > > We need two additional information from the 'dmas' property: the channel > > > (bit in the dmamux register) that must be accessed and the value of the > > > mux for this channel. > > > > + mask = BIT(map->req_idx); > > > + mutex_lock(&dmamux->lock); > > > + dmamux->used_chans |= mask; > > > + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); > > > + if (ret) > > > + goto release_chan_and_unlock; > > > + > > > + mutex_unlock(&dmamux->lock); > > > + > > > + return map; > > > + > > > +release_chan_and_unlock: > > > + dmamux->used_chans &= ~mask; > > > > Now that I check this again, I'm not sure why dmamux->used_chans |= mask; > > couldn't be done after r9a06g032_sysctrl_set_dmamux() call so this > > rollback of it wouldn't be necessary. > > I would still need the mutex unlock which I believe is down path there under > some other label. Hence you are proposing something like > > mask = BIT(map->req_idx); > > mutex_lock(&dmamux->lock); > ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); > if (ret) > goto err_unlock; // or whatever label is > > dmamux->used_chans |= mask; > mutex_unlock(&dmamux->lock); > > return map; > > Is that correct? If so, I don't see impediments either. Yes, and yes, the mutex still has to be unlocked on that error path. > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
On 06-04-22, 18:18, Miquel Raynal wrote: > The Renesas RZN1 DMA IP is based on a DW core, with eg. an additional > dmamux register located in the system control area which can take up to > 32 requests (16 per DMA controller). Each DMA channel can be wired to > two different peripherals. > > We need two additional information from the 'dmas' property: the channel > (bit in the dmamux register) that must be accessed and the value of the > mux for this channel. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > --- > drivers/dma/dw/Kconfig | 9 ++ > drivers/dma/dw/Makefile | 2 + > drivers/dma/dw/rzn1-dmamux.c | 157 +++++++++++++++++++++++++++++++++++ > 3 files changed, 168 insertions(+) > create mode 100644 drivers/dma/dw/rzn1-dmamux.c > > diff --git a/drivers/dma/dw/Kconfig b/drivers/dma/dw/Kconfig > index db25f9b7778c..a9828ddd6d06 100644 > --- a/drivers/dma/dw/Kconfig > +++ b/drivers/dma/dw/Kconfig > @@ -16,6 +16,15 @@ config DW_DMAC > Support the Synopsys DesignWare AHB DMA controller. This > can be integrated in chips such as the Intel Cherrytrail. > > +config RZN1_DMAMUX > + tristate "Renesas RZ/N1 DMAMUX driver" > + depends on DW_DMAC > + depends on ARCH_RZN1 || COMPILE_TEST > + help > + Support the Renesas RZ/N1 DMAMUX which is located in front of > + the Synopsys DesignWare AHB DMA controller located on Renesas > + SoCs. > + > config DW_DMAC_PCI > tristate "Synopsys DesignWare AHB DMA PCI driver" > depends on PCI > diff --git a/drivers/dma/dw/Makefile b/drivers/dma/dw/Makefile > index a6f358ad8591..e1796015f213 100644 > --- a/drivers/dma/dw/Makefile > +++ b/drivers/dma/dw/Makefile > @@ -9,3 +9,5 @@ dw_dmac-$(CONFIG_OF) += of.o > > obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o > dw_dmac_pci-y := pci.o > + > +obj-$(CONFIG_RZN1_DMAMUX) += rzn1-dmamux.o > diff --git a/drivers/dma/dw/rzn1-dmamux.c b/drivers/dma/dw/rzn1-dmamux.c > new file mode 100644 > index 000000000000..5f878a55158f > --- /dev/null > +++ b/drivers/dma/dw/rzn1-dmamux.c > @@ -0,0 +1,157 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2022 Schneider-Electric > + * Author: Miquel Raynal <miquel.raynal@bootlin.com > + * Based on TI crossbar driver written by Peter Ujfalusi <peter.ujfalusi@ti.com> > + */ > +#include <linux/of_device.h> > +#include <linux/of_dma.h> > +#include <linux/slab.h> > +#include <linux/soc/renesas/r9a06g032-sysctrl.h> > + > +#define RZN1_DMAMUX_LINES 64 > +#define RZN1_DMAMUX_MAX_LINES 16 > + > +struct rzn1_dmamux_data { > + struct dma_router dmarouter; > + u32 used_chans; > + struct mutex lock; > +}; > + > +struct rzn1_dmamux_map { > + unsigned int req_idx; > +}; > + > +static void rzn1_dmamux_free(struct device *dev, void *route_data) > +{ > + struct rzn1_dmamux_data *dmamux = dev_get_drvdata(dev); > + struct rzn1_dmamux_map *map = route_data; > + > + dev_dbg(dev, "Unmapping DMAMUX request %u\n", map->req_idx); > + > + mutex_lock(&dmamux->lock); > + dmamux->used_chans &= ~BIT(map->req_idx); > + mutex_unlock(&dmamux->lock); Why not use idr or bitmap for this. Hint: former does locking as well > + > + kfree(map); > +} > + > +static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec, > + struct of_dma *ofdma) > +{ > + struct platform_device *pdev = of_find_device_by_node(ofdma->of_node); > + struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev); > + struct rzn1_dmamux_map *map; > + unsigned int dmac_idx, chan, val; > + u32 mask; > + int ret; > + > + if (dma_spec->args_count != 6) magic > + return ERR_PTR(-EINVAL); > + > + map = kzalloc(sizeof(*map), GFP_KERNEL); > + if (!map) > + return ERR_PTR(-ENOMEM); > + > + chan = dma_spec->args[0]; > + map->req_idx = dma_spec->args[4]; > + val = dma_spec->args[5]; > + dma_spec->args_count -= 2; > + > + if (chan >= RZN1_DMAMUX_MAX_LINES) { > + dev_err(&pdev->dev, "Invalid DMA request line: %u\n", chan); > + ret = -EINVAL; > + goto free_map; > + } > + > + if (map->req_idx >= RZN1_DMAMUX_LINES || > + (map->req_idx % RZN1_DMAMUX_MAX_LINES) != chan) { > + dev_err(&pdev->dev, "Invalid MUX request line: %u\n", map->req_idx); > + ret = -EINVAL; > + goto free_map; > + } > + > + dmac_idx = map->req_idx >= RZN1_DMAMUX_MAX_LINES ? 1 : 0; > + dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", dmac_idx); > + if (!dma_spec->np) { > + dev_err(&pdev->dev, "Can't get DMA master\n"); > + ret = -EINVAL; > + goto free_map; > + } > + > + dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n", > + map->req_idx, dmac_idx, chan); > + > + mask = BIT(map->req_idx); > + mutex_lock(&dmamux->lock); > + dmamux->used_chans |= mask; > + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); I guess due to this it would be merged by whosoever merges this api. Please mention this in cover letter and how you propose this should be merged
Hi Vinod, > > +static void rzn1_dmamux_free(struct device *dev, void *route_data) > > +{ > > + struct rzn1_dmamux_data *dmamux = dev_get_drvdata(dev); > > + struct rzn1_dmamux_map *map = route_data; > > + > > + dev_dbg(dev, "Unmapping DMAMUX request %u\n", map->req_idx); > > + > > + mutex_lock(&dmamux->lock); > > + dmamux->used_chans &= ~BIT(map->req_idx); > > + mutex_unlock(&dmamux->lock); > > Why not use idr or bitmap for this. Hint: former does locking as well I've changed the code to use a proper bitmap. > > > + > > + kfree(map); > > +} > > + > > +static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec, > > + struct of_dma *ofdma) > > +{ > > + struct platform_device *pdev = of_find_device_by_node(ofdma->of_node); > > + struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev); > > + struct rzn1_dmamux_map *map; > > + unsigned int dmac_idx, chan, val; > > + u32 mask; > > + int ret; > > + > > + if (dma_spec->args_count != 6) > > magic Defined. > > > + return ERR_PTR(-EINVAL); > > + > > + map = kzalloc(sizeof(*map), GFP_KERNEL); > > + if (!map) > > + return ERR_PTR(-ENOMEM); > > + > > + chan = dma_spec->args[0]; > > + map->req_idx = dma_spec->args[4]; > > + val = dma_spec->args[5]; > > + dma_spec->args_count -= 2; > > + [...] > > + dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n", > > + map->req_idx, dmac_idx, chan); > > + > > + mask = BIT(map->req_idx); > > + mutex_lock(&dmamux->lock); > > + dmamux->used_chans |= mask; > > + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); > > I guess due to this it would be merged by whosoever merges this api. > Please mention this in cover letter and how you propose this should be > merged Yes, the cover letter mentions this issue, but since then Geert proposed to take everything through the renesas trees, which I agree with. I will send a v9 and if you agree with it please provide your Ack so that Geert can take it. Thanks, Miquèl
Hi Ilpo, Andy, ilpo.jarvinen@linux.intel.com wrote on Fri, 8 Apr 2022 15:38:48 +0300 (EEST): > On Fri, 8 Apr 2022, Andy Shevchenko wrote: > > > On Fri, Apr 08, 2022 at 12:55:47PM +0300, Ilpo Järvinen wrote: > > > On Wed, 6 Apr 2022, Miquel Raynal wrote: > > > > > > > The Renesas RZN1 DMA IP is based on a DW core, with eg. an additional > > > > dmamux register located in the system control area which can take up to > > > > 32 requests (16 per DMA controller). Each DMA channel can be wired to > > > > two different peripherals. > > > > > > > > We need two additional information from the 'dmas' property: the channel > > > > (bit in the dmamux register) that must be accessed and the value of the > > > > mux for this channel. > > > > > > + mask = BIT(map->req_idx); > > > > + mutex_lock(&dmamux->lock); > > > > + dmamux->used_chans |= mask; > > > > + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); > > > > + if (ret) > > > > + goto release_chan_and_unlock; > > > > + > > > > + mutex_unlock(&dmamux->lock); > > > > + > > > > + return map; > > > > + > > > > +release_chan_and_unlock: > > > > + dmamux->used_chans &= ~mask; > > > > > > Now that I check this again, I'm not sure why dmamux->used_chans |= mask; > > > couldn't be done after r9a06g032_sysctrl_set_dmamux() call so this > > > rollback of it wouldn't be necessary. > > > > I would still need the mutex unlock which I believe is down path there under > > some other label. Hence you are proposing something like > > > > mask = BIT(map->req_idx); > > > > mutex_lock(&dmamux->lock); > > ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); > > if (ret) > > goto err_unlock; // or whatever label is > > > > dmamux->used_chans |= mask; > > mutex_unlock(&dmamux->lock); > > > > return map; > > > > Is that correct? If so, I don't see impediments either. > > Yes, and yes, the mutex still has to be unlocked on that error path. > > > > Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> I've done the modification, thanks for your feedback. Thanks, Miquèl
diff --git a/drivers/dma/dw/Kconfig b/drivers/dma/dw/Kconfig index db25f9b7778c..a9828ddd6d06 100644 --- a/drivers/dma/dw/Kconfig +++ b/drivers/dma/dw/Kconfig @@ -16,6 +16,15 @@ config DW_DMAC Support the Synopsys DesignWare AHB DMA controller. This can be integrated in chips such as the Intel Cherrytrail. +config RZN1_DMAMUX + tristate "Renesas RZ/N1 DMAMUX driver" + depends on DW_DMAC + depends on ARCH_RZN1 || COMPILE_TEST + help + Support the Renesas RZ/N1 DMAMUX which is located in front of + the Synopsys DesignWare AHB DMA controller located on Renesas + SoCs. + config DW_DMAC_PCI tristate "Synopsys DesignWare AHB DMA PCI driver" depends on PCI diff --git a/drivers/dma/dw/Makefile b/drivers/dma/dw/Makefile index a6f358ad8591..e1796015f213 100644 --- a/drivers/dma/dw/Makefile +++ b/drivers/dma/dw/Makefile @@ -9,3 +9,5 @@ dw_dmac-$(CONFIG_OF) += of.o obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o dw_dmac_pci-y := pci.o + +obj-$(CONFIG_RZN1_DMAMUX) += rzn1-dmamux.o diff --git a/drivers/dma/dw/rzn1-dmamux.c b/drivers/dma/dw/rzn1-dmamux.c new file mode 100644 index 000000000000..5f878a55158f --- /dev/null +++ b/drivers/dma/dw/rzn1-dmamux.c @@ -0,0 +1,157 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2022 Schneider-Electric + * Author: Miquel Raynal <miquel.raynal@bootlin.com + * Based on TI crossbar driver written by Peter Ujfalusi <peter.ujfalusi@ti.com> + */ +#include <linux/of_device.h> +#include <linux/of_dma.h> +#include <linux/slab.h> +#include <linux/soc/renesas/r9a06g032-sysctrl.h> + +#define RZN1_DMAMUX_LINES 64 +#define RZN1_DMAMUX_MAX_LINES 16 + +struct rzn1_dmamux_data { + struct dma_router dmarouter; + u32 used_chans; + struct mutex lock; +}; + +struct rzn1_dmamux_map { + unsigned int req_idx; +}; + +static void rzn1_dmamux_free(struct device *dev, void *route_data) +{ + struct rzn1_dmamux_data *dmamux = dev_get_drvdata(dev); + struct rzn1_dmamux_map *map = route_data; + + dev_dbg(dev, "Unmapping DMAMUX request %u\n", map->req_idx); + + mutex_lock(&dmamux->lock); + dmamux->used_chans &= ~BIT(map->req_idx); + mutex_unlock(&dmamux->lock); + + kfree(map); +} + +static void *rzn1_dmamux_route_allocate(struct of_phandle_args *dma_spec, + struct of_dma *ofdma) +{ + struct platform_device *pdev = of_find_device_by_node(ofdma->of_node); + struct rzn1_dmamux_data *dmamux = platform_get_drvdata(pdev); + struct rzn1_dmamux_map *map; + unsigned int dmac_idx, chan, val; + u32 mask; + int ret; + + if (dma_spec->args_count != 6) + return ERR_PTR(-EINVAL); + + map = kzalloc(sizeof(*map), GFP_KERNEL); + if (!map) + return ERR_PTR(-ENOMEM); + + chan = dma_spec->args[0]; + map->req_idx = dma_spec->args[4]; + val = dma_spec->args[5]; + dma_spec->args_count -= 2; + + if (chan >= RZN1_DMAMUX_MAX_LINES) { + dev_err(&pdev->dev, "Invalid DMA request line: %u\n", chan); + ret = -EINVAL; + goto free_map; + } + + if (map->req_idx >= RZN1_DMAMUX_LINES || + (map->req_idx % RZN1_DMAMUX_MAX_LINES) != chan) { + dev_err(&pdev->dev, "Invalid MUX request line: %u\n", map->req_idx); + ret = -EINVAL; + goto free_map; + } + + dmac_idx = map->req_idx >= RZN1_DMAMUX_MAX_LINES ? 1 : 0; + dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", dmac_idx); + if (!dma_spec->np) { + dev_err(&pdev->dev, "Can't get DMA master\n"); + ret = -EINVAL; + goto free_map; + } + + dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n", + map->req_idx, dmac_idx, chan); + + mask = BIT(map->req_idx); + mutex_lock(&dmamux->lock); + dmamux->used_chans |= mask; + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); + if (ret) + goto release_chan_and_unlock; + + mutex_unlock(&dmamux->lock); + + return map; + +release_chan_and_unlock: + dmamux->used_chans &= ~mask; + mutex_unlock(&dmamux->lock); +free_map: + kfree(map); + + return ERR_PTR(ret); +} + +static const struct of_device_id rzn1_dmac_match[] = { + { .compatible = "renesas,rzn1-dma" }, + {} +}; + +static int rzn1_dmamux_probe(struct platform_device *pdev) +{ + struct device_node *mux_node = pdev->dev.of_node; + const struct of_device_id *match; + struct device_node *dmac_node; + struct rzn1_dmamux_data *dmamux; + + dmamux = devm_kzalloc(&pdev->dev, sizeof(*dmamux), GFP_KERNEL); + if (!dmamux) + return -ENOMEM; + + mutex_init(&dmamux->lock); + + dmac_node = of_parse_phandle(mux_node, "dma-masters", 0); + if (!dmac_node) + return dev_err_probe(&pdev->dev, -ENODEV, "Can't get DMA master node\n"); + + match = of_match_node(rzn1_dmac_match, dmac_node); + of_node_put(dmac_node); + if (!match) + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); + + dmamux->dmarouter.dev = &pdev->dev; + dmamux->dmarouter.route_free = rzn1_dmamux_free; + + platform_set_drvdata(pdev, dmamux); + + return of_dma_router_register(mux_node, rzn1_dmamux_route_allocate, + &dmamux->dmarouter); +} + +static const struct of_device_id rzn1_dmamux_match[] = { + { .compatible = "renesas,rzn1-dmamux" }, + {} +}; + +static struct platform_driver rzn1_dmamux_driver = { + .driver = { + .name = "renesas,rzn1-dmamux", + .of_match_table = rzn1_dmamux_match, + }, + .probe = rzn1_dmamux_probe, +}; +module_platform_driver(rzn1_dmamux_driver); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Miquel Raynal <miquel.raynal@bootlin.com"); +MODULE_DESCRIPTION("Renesas RZ/N1 DMAMUX driver");