| Message ID | 20220222103437.194779-5-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | RZN1 DMA support | expand |
Hi Miquel,
I love your patch! Yet something to improve:
[auto build test ERROR on geert-renesas-devel/next]
[also build test ERROR on geert-renesas-drivers/renesas-clk robh/for-next linus/master v5.17-rc5 next-20220217]
[cannot apply to vkoul-dmaengine/next]
[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/Miquel-Raynal/RZN1-DMA-support/20220222-183549
base: https://git.kernel.org/pub/scm/linux/kernel/git/geert/renesas-devel.git next
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20220223/202202230444.xjzzeOlo-lkp@intel.com/config)
compiler: m68k-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/b62059e893c7e3779b96e1c69ae6818260d352de
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Miquel-Raynal/RZN1-DMA-support/20220222-183549
git checkout b62059e893c7e3779b96e1c69ae6818260d352de
# save the config file to linux build tree
mkdir build_dir
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.2.0 make.cross O=build_dir ARCH=m68k SHELL=/bin/bash
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All errors (new ones prefixed by >>, old ones prefixed by <<):
ERROR: modpost: missing MODULE_LICENSE() in drivers/dma/dw/dmamux.o
>> ERROR: modpost: "r9a06g032_sysctrl_set_dmamux" [drivers/dma/dw/dmamux.ko] undefined!
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hello, > ERROR: modpost: missing MODULE_LICENSE() in drivers/dma/dw/dmamux.o > ERROR: modpost: "r9a06g032_sysctrl_set_dmamux" [drivers/dma/dw/dmamux.ko] undefined! I will wait for more feedback against this version and then send a v3 with the missing macros. Thanks, Miquèl
[...] > +static int rzn1_dmamux_init(void) > +{ > + return platform_driver_register(&rzn1_dmamux_driver); > +} > +arch_initcall(rzn1_dmamux_init); I don't think this driver actually needs the arch_initcall() level, I'll propose a v3 using the regular platform init level. And this driver is now missing MODULE_* macros. Thanks, Miquèl
Hi Miquel, On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > The Renesas RZN1 DMA IP is a 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> Thanks for your patch! > --- /dev/null > +++ b/drivers/dma/dw/dmamux.c rzn1-dmamux.c? > @@ -0,0 +1,167 @@ > +// 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/slab.h> > +#include <linux/err.h> > +#include <linux/init.h> > +#include <linux/list.h> > +#include <linux/io.h> > +#include <linux/of_address.h> > +#include <linux/of_device.h> > +#include <linux/of_dma.h> > +#include <linux/soc/renesas/r9a06g032-sysctrl.h> > + > +#define RZN1_DMAMUX_LINES 64 Unused. But using it wouldn't hurt, I guess? > +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 master, chan, val; > + u32 mask; > + int ret; > + > + map = kzalloc(sizeof(*map), GFP_KERNEL); > + if (!map) > + return ERR_PTR(-ENOMEM); > + > + if (dma_spec->args_count != 6) > + return ERR_PTR(-EINVAL); > + > + chan = dma_spec->args[0]; > + map->req_idx = dma_spec->args[4]; > + val = dma_spec->args[5]; > + dma_spec->args_count -= 2; > + > + if (chan >= dmamux->dmac_requests) { > + dev_err(&pdev->dev, "Invalid DMA request line: %d\n", chan); %u > + return ERR_PTR(-EINVAL); > + } > + > + if (map->req_idx >= dmamux->dmamux_requests || > + map->req_idx % dmamux->dmac_requests != chan) { The reliance on .dmac_requests (i.e. "dma-requests" in the parent DMA controller DT node) looks fragile to me. Currently there are two masters, each providing 16 channels, hence using all 2 x 16 = 32 bits in the DMAMUX register. What if a variant used the same mux, and the same 16/16 split, but the parent DMACs don't have all channels available? I think it would be safer to hardcode this as 16 (using a #define, ofc). > + dev_err(&pdev->dev, "Invalid MUX request line: %d\n", map->req_idx); %u > + return ERR_PTR(-EINVAL); > + } > + > + /* The of_node_put() will be done in the core for the node */ > + master = map->req_idx >= dmamux->dmac_requests ? 1 : 0; The name "master" confused me: initially I thought it was used as a boolean flag, but it really is the index of the parent DMAC. > + dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", master); > + if (!dma_spec->np) { > + dev_err(&pdev->dev, "Can't get DMA master\n"); > + return ERR_PTR(-EINVAL); > + } > + > + dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n", > + map->req_idx, master, chan); > + > + mask = BIT(map->req_idx); > + mutex_lock(&dmamux->lock); > + dmamux->used_chans |= mask; > + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); > + mutex_unlock(&dmamux->lock); > + if (ret) { > + rzn1_dmamux_free(&pdev->dev, map); > + return ERR_PTR(ret); > + } > + > + return map; > +} > + > +static const struct of_device_id rzn1_dmac_match[] __maybe_unused = { > + { .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); > + if (!match) { > + of_node_put(dmac_node); > + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); > + } > + > + if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) { > + of_node_put(dmac_node); > + return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n"); > + } > + > + of_node_put(dmac_node); When hardcoding dmac_requests to 16, I guess the whole dmac_node handling can be removed? > + > + if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) { Don't obtain from DT, but fix to 32? > + return dev_err_probe(&pdev->dev, -EINVAL, "Missing mux requests information\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); > +} Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100: > Hi Miquel, > > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > The Renesas RZN1 DMA IP is a 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> > > Thanks for your patch! > > > --- /dev/null > > +++ b/drivers/dma/dw/dmamux.c > > rzn1-dmamux.c? Ok. > > > @@ -0,0 +1,167 @@ > > +// 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/slab.h> > > +#include <linux/err.h> > > +#include <linux/init.h> > > +#include <linux/list.h> > > +#include <linux/io.h> > > +#include <linux/of_address.h> > > +#include <linux/of_device.h> > > +#include <linux/of_dma.h> > > +#include <linux/soc/renesas/r9a06g032-sysctrl.h> > > + > > +#define RZN1_DMAMUX_LINES 64 > > Unused. But using it wouldn't hurt, I guess? > > > +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 master, chan, val; > > + u32 mask; > > + int ret; > > + > > + map = kzalloc(sizeof(*map), GFP_KERNEL); > > + if (!map) > > + return ERR_PTR(-ENOMEM); > > + > > + if (dma_spec->args_count != 6) > > + return ERR_PTR(-EINVAL); > > + > > + chan = dma_spec->args[0]; > > + map->req_idx = dma_spec->args[4]; > > + val = dma_spec->args[5]; > > + dma_spec->args_count -= 2; > > + > > + if (chan >= dmamux->dmac_requests) { > > + dev_err(&pdev->dev, "Invalid DMA request line: %d\n", chan); > > %u > > > + return ERR_PTR(-EINVAL); > > + } > > + > > + if (map->req_idx >= dmamux->dmamux_requests || > > + map->req_idx % dmamux->dmac_requests != chan) { > > The reliance on .dmac_requests (i.e. "dma-requests" in the parent > DMA controller DT node) looks fragile to me. Currently there are two > masters, each providing 16 channels, hence using all 2 x 16 = > 32 bits in the DMAMUX register. > What if a variant used the same mux, and the same 16/16 split, but > the parent DMACs don't have all channels available? > I think it would be safer to hardcode this as 16 (using a #define, ofc). That's right, I assumed this was safe but indeed it does not work in all cases. I will change the second condition to: map->req_idx % <16> != chan > > > + dev_err(&pdev->dev, "Invalid MUX request line: %d\n", map->req_idx); > > %u > > > + return ERR_PTR(-EINVAL); > > + } > > + > > + /* The of_node_put() will be done in the core for the node */ > > + master = map->req_idx >= dmamux->dmac_requests ? 1 : 0; > > The name "master" confused me: initially I thought it was used as a > boolean flag, but it really is the index of the parent DMAC. I personally prefer using true/false for booleans ;) Whatever, the name is badly chosen I agree, I'll switch to "dmac_idx" which seems more accurate. > > > + dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", master); > > + if (!dma_spec->np) { > > + dev_err(&pdev->dev, "Can't get DMA master\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n", > > + map->req_idx, master, chan); > > + > > + mask = BIT(map->req_idx); > > + mutex_lock(&dmamux->lock); > > + dmamux->used_chans |= mask; > > + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); > > + mutex_unlock(&dmamux->lock); > > + if (ret) { > > + rzn1_dmamux_free(&pdev->dev, map); > > + return ERR_PTR(ret); > > + } > > + > > + return map; > > +} > > + > > +static const struct of_device_id rzn1_dmac_match[] __maybe_unused = { > > + { .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); > > + if (!match) { > > + of_node_put(dmac_node); > > + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); > > + } > > + > > + if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) { > > + of_node_put(dmac_node); > > + return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n"); > > + } > > + > > + of_node_put(dmac_node); > > When hardcoding dmac_requests to 16, I guess the whole dmac_node > handling can be removed? Not really, I think the following checks are still valid and fortunate, and they need some of_ handling to work properly: - verify that the chan requested is within the range of dmac_requests in the _route_allocate() callback - ensure the dmamux is wired to a supported DMAC in the DT (this condition might be loosen in the future if needed or dropped entirely if considered useless) - I would like to add a check against the number of requests supported by the dmamux and the dmac (not done yet). For the record, I've taken inspiration to write these lines on the other dma router driver from TI. Unless, and I know some people think like that, we do not try to validate the DT and if the DT is wrong that is none of our business. > > > + > > + if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) { > > Don't obtain from DT, but fix to 32? I believe the answer to the previous question should give me a clue about why you would prefer hardcoding than reading from the DT such an information. Perhaps I should mention that all these properties are already part of the bindings, and are not specific to the driver, the information will be in the DT anyway. Thanks, Miquèl
Hi Miquel, On Wed, Feb 23, 2022 at 5:49 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100: > > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal > > <miquel.raynal@bootlin.com> wrote: > > > The Renesas RZN1 DMA IP is a 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> > > > > Thanks for your patch! > > > > > --- /dev/null > > > +++ b/drivers/dma/dw/dmamux.c > > > +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); > > > + if (!match) { > > > + of_node_put(dmac_node); > > > + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); > > > + } > > > + > > > + if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) { > > > + of_node_put(dmac_node); > > > + return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n"); > > > + } > > > + > > > + of_node_put(dmac_node); > > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node > > handling can be removed? > > Not really, I think the following checks are still valid and fortunate, > and they need some of_ handling to work properly: > - verify that the chan requested is within the range of dmac_requests > in the _route_allocate() callback > - ensure the dmamux is wired to a supported DMAC in the DT (this > condition might be loosen in the future if needed or dropped entirely > if considered useless) > - I would like to add a check against the number of requests supported > by the dmamux and the dmac (not done yet). > For the record, I've taken inspiration to write these lines on the other > dma router driver from TI. > > Unless, and I know some people think like that, we do not try to > validate the DT and if the DT is wrong that is none of our business. > > > > > > + > > > + if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) { > > > > Don't obtain from DT, but fix to 32? > > I believe the answer to the previous question should give me a clue > about why you would prefer hardcoding than reading from the DT such > an information. Perhaps I should mention that all these properties are > already part of the bindings, and are not specific to the driver, the > information will be in the DT anyway. The 32 is a property of the hardware (32 bits in DMAMUX register). So IMHO it falls under the "differentiate by compatible value, not by property" rule. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, geert@linux-m68k.org wrote on Thu, 24 Feb 2022 10:14:48 +0100: > Hi Miquel, > > On Wed, Feb 23, 2022 at 5:49 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100: > > > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal > > > <miquel.raynal@bootlin.com> wrote: > > > > The Renesas RZN1 DMA IP is a 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> > > > > > > Thanks for your patch! > > > > > > > --- /dev/null > > > > +++ b/drivers/dma/dw/dmamux.c > > > > > +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); > > > > + if (!match) { > > > > + of_node_put(dmac_node); > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); > > > > + } > > > > + > > > > + if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) { > > > > + of_node_put(dmac_node); > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n"); > > > > + } > > > > + > > > > + of_node_put(dmac_node); > > > > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node > > > handling can be removed? > > > > Not really, I think the following checks are still valid and fortunate, > > and they need some of_ handling to work properly: > > - verify that the chan requested is within the range of dmac_requests > > in the _route_allocate() callback > > - ensure the dmamux is wired to a supported DMAC in the DT (this > > condition might be loosen in the future if needed or dropped entirely > > if considered useless) > > - I would like to add a check against the number of requests supported > > by the dmamux and the dmac (not done yet). > > For the record, I've taken inspiration to write these lines on the other > > dma router driver from TI. > > > > Unless, and I know some people think like that, we do not try to > > validate the DT and if the DT is wrong that is none of our business. > > > > > > > > > + > > > > + if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) { > > > > > > Don't obtain from DT, but fix to 32? > > > > I believe the answer to the previous question should give me a clue > > about why you would prefer hardcoding than reading from the DT such > > an information. Perhaps I should mention that all these properties are > > already part of the bindings, and are not specific to the driver, the > > information will be in the DT anyway. > > The 32 is a property of the hardware (32 bits in DMAMUX register). > So IMHO it falls under the "differentiate by compatible value, > not by property" rule. I agree this is a property of the hardware and feels redundant here. What about the checks below, do you agree with the fact that I should keep them or do you prefer dropping them (all? partially?)? Thanks, Miquèl
Hi Miquel, On Thu, Feb 24, 2022 at 10:27 AM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > geert@linux-m68k.org wrote on Thu, 24 Feb 2022 10:14:48 +0100: > > On Wed, Feb 23, 2022 at 5:49 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > geert@linux-m68k.org wrote on Wed, 23 Feb 2022 13:46:11 +0100: > > > > On Tue, Feb 22, 2022 at 11:35 AM Miquel Raynal > > > > <miquel.raynal@bootlin.com> wrote: > > > > > The Renesas RZN1 DMA IP is a 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> > > > > > > > > Thanks for your patch! > > > > > > > > > --- /dev/null > > > > > +++ b/drivers/dma/dw/dmamux.c > > > > > > > +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); > > > > > + if (!match) { > > > > > + of_node_put(dmac_node); > > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); > > > > > + } > > > > > + > > > > > + if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) { > > > > > + of_node_put(dmac_node); > > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n"); > > > > > + } > > > > > + > > > > > + of_node_put(dmac_node); > > > > > > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node > > > > handling can be removed? > > > > > > Not really, I think the following checks are still valid and fortunate, > > > and they need some of_ handling to work properly: > > > - verify that the chan requested is within the range of dmac_requests > > > in the _route_allocate() callback > > > - ensure the dmamux is wired to a supported DMAC in the DT (this > > > condition might be loosen in the future if needed or dropped entirely > > > if considered useless) > > > - I would like to add a check against the number of requests supported > > > by the dmamux and the dmac (not done yet). > > > For the record, I've taken inspiration to write these lines on the other > > > dma router driver from TI. > > > > > > Unless, and I know some people think like that, we do not try to > > > validate the DT and if the DT is wrong that is none of our business. > > > > > > > > > > > > + > > > > > + if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) { > > > > > > > > Don't obtain from DT, but fix to 32? > > > > > > I believe the answer to the previous question should give me a clue > > > about why you would prefer hardcoding than reading from the DT such > > > an information. Perhaps I should mention that all these properties are > > > already part of the bindings, and are not specific to the driver, the > > > information will be in the DT anyway. > > > > The 32 is a property of the hardware (32 bits in DMAMUX register). > > So IMHO it falls under the "differentiate by compatible value, > > not by property" rule. > > I agree this is a property of the hardware and feels redundant here. > > What about the checks below, do you agree with the fact that I should > keep them or do you prefer dropping them (all? partially?)? There are no checks below? /me confused. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, > > > > > > +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); > > > > > > + if (!match) { > > > > > > + of_node_put(dmac_node); > > > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); > > > > > > + } > > > > > > + > > > > > > + if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) { > > > > > > + of_node_put(dmac_node); > > > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n"); > > > > > > + } > > > > > > + > > > > > > + of_node_put(dmac_node); > > > > > > > > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node > > > > > handling can be removed? > > > > > > > > Not really, I think the following checks are still valid and fortunate, > > > > and they need some of_ handling to work properly: > > > > - verify that the chan requested is within the range of dmac_requests > > > > in the _route_allocate() callback > > > > - ensure the dmamux is wired to a supported DMAC in the DT (this > > > > condition might be loosen in the future if needed or dropped entirely > > > > if considered useless) > > > > - I would like to add a check against the number of requests supported > > > > by the dmamux and the dmac (not done yet). > > > > For the record, I've taken inspiration to write these lines on the other > > > > dma router driver from TI. ^^^^^^^^^^^ ... these checks > > > > > > > > Unless, and I know some people think like that, we do not try to > > > > validate the DT and if the DT is wrong that is none of our business. > > > > > > > > > > > > > > > + > > > > > > + if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) { > > > > > > > > > > Don't obtain from DT, but fix to 32? > > > > > > > > I believe the answer to the previous question should give me a clue > > > > about why you would prefer hardcoding than reading from the DT such > > > > an information. Perhaps I should mention that all these properties are > > > > already part of the bindings, and are not specific to the driver, the > > > > information will be in the DT anyway. > > > > > > The 32 is a property of the hardware (32 bits in DMAMUX register). > > > So IMHO it falls under the "differentiate by compatible value, > > > not by property" rule. > > > > I agree this is a property of the hardware and feels redundant here. > > > > What about the checks below, do you agree with the fact that I should > > keep them or do you prefer dropping them (all? partially?)? > > There are no checks below? I meant above /o\ ... > /me confused. > > Gr{oetje,eeting}s, > > Geert >
Hi Miquel, On Thu, Feb 24, 2022 at 12:36 PM Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > +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); > > > > > > > + if (!match) { > > > > > > > + of_node_put(dmac_node); > > > > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); > > > > > > > + } > > > > > > > + > > > > > > > + if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) { > > > > > > > + of_node_put(dmac_node); > > > > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n"); > > > > > > > + } > > > > > > > + > > > > > > > + of_node_put(dmac_node); > > > > > > > > > > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node > > > > > > handling can be removed? > > > > > > > > > > Not really, I think the following checks are still valid and fortunate, > > > > > and they need some of_ handling to work properly: > > > > > - verify that the chan requested is within the range of dmac_requests > > > > > in the _route_allocate() callback > > > > > - ensure the dmamux is wired to a supported DMAC in the DT (this > > > > > condition might be loosen in the future if needed or dropped entirely > > > > > if considered useless) > > > > > - I would like to add a check against the number of requests supported > > > > > by the dmamux and the dmac (not done yet). > > > > > For the record, I've taken inspiration to write these lines on the other > > > > > dma router driver from TI. > > ^^^^^^^^^^^ > ... these checks I don't know. Some of them will be checked when calling into the parent DMAC, right? > > > > > > > > > > > Unless, and I know some people think like that, we do not try to > > > > > validate the DT and if the DT is wrong that is none of our business. > > > > > > > > > > > > > > > > > > + > > > > > > > + if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) { > > > > > > > > > > > > Don't obtain from DT, but fix to 32? > > > > > > > > > > I believe the answer to the previous question should give me a clue > > > > > about why you would prefer hardcoding than reading from the DT such > > > > > an information. Perhaps I should mention that all these properties are > > > > > already part of the bindings, and are not specific to the driver, the > > > > > information will be in the DT anyway. > > > > > > > > The 32 is a property of the hardware (32 bits in DMAMUX register). > > > > So IMHO it falls under the "differentiate by compatible value, > > > > not by property" rule. > > > > > > I agree this is a property of the hardware and feels redundant here. > > > > > > What about the checks below, do you agree with the fact that I should > > > keep them or do you prefer dropping them (all? partially?)? > > > > There are no checks below? > > I meant above /o\ ... Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Geert, geert@linux-m68k.org wrote on Thu, 24 Feb 2022 13:16:09 +0100: > Hi Miquel, > > On Thu, Feb 24, 2022 at 12:36 PM Miquel Raynal > <miquel.raynal@bootlin.com> wrote: > > > > > > > > +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); > > > > > > > > + if (!match) { > > > > > > > > + of_node_put(dmac_node); > > > > > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); > > > > > > > > + } > > > > > > > > + > > > > > > > > + if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) { > > > > > > > > + of_node_put(dmac_node); > > > > > > > > + return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n"); > > > > > > > > + } > > > > > > > > + > > > > > > > > + of_node_put(dmac_node); > > > > > > > > > > > > > > When hardcoding dmac_requests to 16, I guess the whole dmac_node > > > > > > > handling can be removed? > > > > > > > > > > > > Not really, I think the following checks are still valid and fortunate, > > > > > > and they need some of_ handling to work properly: > > > > > > - verify that the chan requested is within the range of dmac_requests > > > > > > in the _route_allocate() callback > > > > > > - ensure the dmamux is wired to a supported DMAC in the DT (this > > > > > > condition might be loosen in the future if needed or dropped entirely > > > > > > if considered useless) > > > > > > - I would like to add a check against the number of requests supported > > > > > > by the dmamux and the dmac (not done yet). > > > > > > For the record, I've taken inspiration to write these lines on the other > > > > > > dma router driver from TI. > > > > ^^^^^^^^^^^ > > ... these checks > > I don't know. Some of them will be checked when calling into the > parent DMAC, right? Only the first item above will be validated by the DMAC driver. But I prefer to error out sooner than later, because getting the mux in place while knowing that the request is invalid sounds silly. Thanks, Miquèl
diff --git a/drivers/dma/dw/Kconfig b/drivers/dma/dw/Kconfig index db25f9b7778c..dd53d4a9fa92 100644 --- a/drivers/dma/dw/Kconfig +++ b/drivers/dma/dw/Kconfig @@ -16,6 +16,14 @@ 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 + 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..7c6e0ab6fcd8 100644 --- a/drivers/dma/dw/Makefile +++ b/drivers/dma/dw/Makefile @@ -7,5 +7,7 @@ obj-$(CONFIG_DW_DMAC) += dw_dmac.o dw_dmac-y := platform.o dw_dmac-$(CONFIG_OF) += of.o +obj-$(CONFIG_RZN1_DMAMUX) += dmamux.o + obj-$(CONFIG_DW_DMAC_PCI) += dw_dmac_pci.o dw_dmac_pci-y := pci.o diff --git a/drivers/dma/dw/dmamux.c b/drivers/dma/dw/dmamux.c new file mode 100644 index 000000000000..5fb3ffb82e88 --- /dev/null +++ b/drivers/dma/dw/dmamux.c @@ -0,0 +1,167 @@ +// 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/slab.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/list.h> +#include <linux/io.h> +#include <linux/of_address.h> +#include <linux/of_device.h> +#include <linux/of_dma.h> +#include <linux/soc/renesas/r9a06g032-sysctrl.h> + +#define RZN1_DMAMUX_LINES 64 + +struct rzn1_dmamux_data { + struct dma_router dmarouter; + unsigned int dmac_requests; + unsigned int dmamux_requests; + 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 master, chan, val; + u32 mask; + int ret; + + map = kzalloc(sizeof(*map), GFP_KERNEL); + if (!map) + return ERR_PTR(-ENOMEM); + + if (dma_spec->args_count != 6) + return ERR_PTR(-EINVAL); + + chan = dma_spec->args[0]; + map->req_idx = dma_spec->args[4]; + val = dma_spec->args[5]; + dma_spec->args_count -= 2; + + if (chan >= dmamux->dmac_requests) { + dev_err(&pdev->dev, "Invalid DMA request line: %d\n", chan); + return ERR_PTR(-EINVAL); + } + + if (map->req_idx >= dmamux->dmamux_requests || + map->req_idx % dmamux->dmac_requests != chan) { + dev_err(&pdev->dev, "Invalid MUX request line: %d\n", map->req_idx); + return ERR_PTR(-EINVAL); + } + + /* The of_node_put() will be done in the core for the node */ + master = map->req_idx >= dmamux->dmac_requests ? 1 : 0; + dma_spec->np = of_parse_phandle(ofdma->of_node, "dma-masters", master); + if (!dma_spec->np) { + dev_err(&pdev->dev, "Can't get DMA master\n"); + return ERR_PTR(-EINVAL); + } + + dev_dbg(&pdev->dev, "Mapping DMAMUX request %u to DMAC%u request %u\n", + map->req_idx, master, chan); + + mask = BIT(map->req_idx); + mutex_lock(&dmamux->lock); + dmamux->used_chans |= mask; + ret = r9a06g032_sysctrl_set_dmamux(mask, val ? mask : 0); + mutex_unlock(&dmamux->lock); + if (ret) { + rzn1_dmamux_free(&pdev->dev, map); + return ERR_PTR(ret); + } + + return map; +} + +static const struct of_device_id rzn1_dmac_match[] __maybe_unused = { + { .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); + if (!match) { + of_node_put(dmac_node); + return dev_err_probe(&pdev->dev, -EINVAL, "DMA master is not supported\n"); + } + + if (of_property_read_u32(dmac_node, "dma-requests", &dmamux->dmac_requests)) { + of_node_put(dmac_node); + return dev_err_probe(&pdev->dev, -EINVAL, "Missing DMAC requests information\n"); + } + + of_node_put(dmac_node); + + if (of_property_read_u32(mux_node, "dma-requests", &dmamux->dmamux_requests)) { + return dev_err_probe(&pdev->dev, -EINVAL, "Missing mux requests information\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, +}; + +static int rzn1_dmamux_init(void) +{ + return platform_driver_register(&rzn1_dmamux_driver); +} +arch_initcall(rzn1_dmamux_init);
The Renesas RZN1 DMA IP is a 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> --- drivers/dma/dw/Kconfig | 8 ++ drivers/dma/dw/Makefile | 2 + drivers/dma/dw/dmamux.c | 167 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 177 insertions(+) create mode 100644 drivers/dma/dw/dmamux.c