| Message ID | 1429076351-4429-1-git-send-email-nicoleotsuka@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Tue, Apr 14, 2015 at 10:39:11PM -0700, Nicolin Chen wrote: > The SDMA on imx6sx has a few DMA event remapping configurations > inside the GPR (General Purpose Register) of that SoC. When users > want to use a non-default DMA event, they need to configure the > GPR register. So this patch gives an interface of the GPR and > implements it in the SDMA driver so as to finish the DMA event > remapping. > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > --- > .../devicetree/bindings/dma/fsl-imx-sdma.txt | 9 ++++ > drivers/dma/imx-sdma.c | 58 ++++++++++++++++++++++ > 2 files changed, 67 insertions(+) > > diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > index dc8d3aa..03315a6 100644 > --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > @@ -8,6 +8,7 @@ Required properties: > "fsl,imx51-sdma" > "fsl,imx53-sdma" > "fsl,imx6q-sdma" > + "fsl,imx6sx-sdma" > The -to variants should be preferred since they allow to determine the > correct ROM script addresses needed for the driver to work without additional > firmware. > @@ -19,6 +20,14 @@ Required properties: > - fsl,sdma-ram-script-name : Should contain the full path of SDMA RAM > scripts firmware > > +Optional properties: > +- fsl, sdma-event-remap : List of one or more DMA event remapping > + configurations. Its format: <&gpr addr shift val> > + gpr : the gpr phandle > + addr : the register address of the GPR > + shift : the bit shift of the GPR register > + val : the value of that bit This binding allows arbitrary register writes to the GPR register space. I don't think we want to allow that. A binding for this if necessary at all must be higher level. > +static int sdma_event_remap(struct sdma_engine *sdma) > +{ > + struct device_node *np = sdma->dev->of_node; > + char propname[] = "fsl,sdma-event-remap"; > + struct of_phandle_args gpr_spec; > + struct regmap *gpr; > + int i = 0, ret = 0; > + > + /* Only support DT */ > + if (IS_ERR(np)) > + return ret; > + > + /* Only apply to imx6sx platform */ > + if (!of_device_is_compatible(np, "fsl,imx6sx-sdma")) > + return ret; We already have struct sdma_driver_data. Please add a have_remap field or similar there. > + > + /* Bypass if no need to remap */ > + if (!of_find_property(np, propname, NULL)) > + return ret; > + > + /* Fetch the remap configurations of GPR */ > + ret = of_parse_phandle_with_args(np, propname, "#gpr-cells", i++, > + &gpr_spec); > + if (ret) { > + dev_err(sdma->dev, "failed to get a correct %s property: %d\n", > + propname, ret); > + return ret; > + } > + > +next: > + gpr = syscon_node_to_regmap(gpr_spec.np); > + if (IS_ERR(gpr)) { > + ret = PTR_ERR(gpr); > + dev_err(sdma->dev, "failed to get gpr regmap: %d\n", reg); > + return ret; > + } > + > + /* 0 : Register address, 1 : Bit shift, 2 : Bit value */ > + regmap_update_bits(gpr, gpr_spec.args[0], BIT(gpr_spec.args[1]), > + gpr_spec.args[2] << gpr_spec.args[1]); > + > + if (!of_parse_phandle_with_args(np, propname, "#gpr-cells", i++, > + &gpr_spec)) > + goto next; C has native support for loops. Please use it. Sascha
Hi Sascha, Thank you for the comments. On Mon, Apr 20, 2015 at 11:45:41AM +0200, Sascha Hauer wrote: > On Tue, Apr 14, 2015 at 10:39:11PM -0700, Nicolin Chen wrote: > > The SDMA on imx6sx has a few DMA event remapping configurations > > inside the GPR (General Purpose Register) of that SoC. When users > > want to use a non-default DMA event, they need to configure the > > GPR register. So this patch gives an interface of the GPR and > > implements it in the SDMA driver so as to finish the DMA event > > remapping. > > > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > > diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > > index dc8d3aa..03315a6 100644 > > --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > > +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > > @@ -8,6 +8,7 @@ Required properties: > > "fsl,imx51-sdma" > > "fsl,imx53-sdma" > > "fsl,imx6q-sdma" > > + "fsl,imx6sx-sdma" > > The -to variants should be preferred since they allow to determine the > > correct ROM script addresses needed for the driver to work without additional > > firmware. > > @@ -19,6 +20,14 @@ Required properties: > > - fsl,sdma-ram-script-name : Should contain the full path of SDMA RAM > > scripts firmware > > > > +Optional properties: > > +- fsl, sdma-event-remap : List of one or more DMA event remapping > > + configurations. Its format: <&gpr addr shift val> > > + gpr : the gpr phandle > > + addr : the register address of the GPR > > + shift : the bit shift of the GPR register > > + val : the value of that bit > > This binding allows arbitrary register writes to the GPR register space. > I don't think we want to allow that. A binding for this if necessary at > all must be higher level. I was actually wondering where could be the best place to put this in. But I couldn't figure out a better place than this driver while being worried about the violation as you mentioned. But what could be that higher level? Thank you Nicolin -- To unsubscribe from this list: send the line "unsubscribe dmaengine" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Apr 20, 2015 at 10:34:33AM -0700, Nicolin Chen wrote: > Hi Sascha, > > Thank you for the comments. > > On Mon, Apr 20, 2015 at 11:45:41AM +0200, Sascha Hauer wrote: > > On Tue, Apr 14, 2015 at 10:39:11PM -0700, Nicolin Chen wrote: > > > The SDMA on imx6sx has a few DMA event remapping configurations > > > inside the GPR (General Purpose Register) of that SoC. When users > > > want to use a non-default DMA event, they need to configure the > > > GPR register. So this patch gives an interface of the GPR and > > > implements it in the SDMA driver so as to finish the DMA event > > > remapping. > > > > > > Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> > > > > diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > > > index dc8d3aa..03315a6 100644 > > > --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > > > +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt > > > @@ -8,6 +8,7 @@ Required properties: > > > "fsl,imx51-sdma" > > > "fsl,imx53-sdma" > > > "fsl,imx6q-sdma" > > > + "fsl,imx6sx-sdma" > > > The -to variants should be preferred since they allow to determine the > > > correct ROM script addresses needed for the driver to work without additional > > > firmware. > > > @@ -19,6 +20,14 @@ Required properties: > > > - fsl,sdma-ram-script-name : Should contain the full path of SDMA RAM > > > scripts firmware > > > > > > +Optional properties: > > > +- fsl, sdma-event-remap : List of one or more DMA event remapping > > > + configurations. Its format: <&gpr addr shift val> > > > + gpr : the gpr phandle > > > + addr : the register address of the GPR > > > + shift : the bit shift of the GPR register > > > + val : the value of that bit > > > > This binding allows arbitrary register writes to the GPR register space. > > I don't think we want to allow that. A binding for this if necessary at > > all must be higher level. > > I was actually wondering where could be the best place to put this in. > But I couldn't figure out a better place than this driver while being > worried about the violation as you mentioned. > > But what could be that higher level? This gpr register effectivly increases the number of event IDs available by muxing them with existing event IDs. Looking at the SDMA event mapping table we have: 0 UART6 UART6 Rx FIFO; controlled by IOMUXC register GPR0[0] 1 ADC1 / I2C4 ADC1 DMA request ;Muxed I2C4 DMA event by IOMUXC register GPR0[1] ADC1 DMA request 2 IOMUX / CSI2 Muxed with external DMA pad #1, controlled by IOMUXC register GPR0[20]. ... 47 UART6 UART6 Tx FIFO; controlled by IOMUXC register GPR0[19] So I would extend that list by additional entries for the muxed dma events. event 48 then means event 0 alternative, with GPR0[0] set to 1. BTW. Using defines for the SDMA events like done nowadays for clocks and other stuff would improve readability, especially when the list of dma events gets extended by non obvious numbers which do not directly map to hardware numbers. Sascha
diff --git a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt index dc8d3aa..03315a6 100644 --- a/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt +++ b/Documentation/devicetree/bindings/dma/fsl-imx-sdma.txt @@ -8,6 +8,7 @@ Required properties: "fsl,imx51-sdma" "fsl,imx53-sdma" "fsl,imx6q-sdma" + "fsl,imx6sx-sdma" The -to variants should be preferred since they allow to determine the correct ROM script addresses needed for the driver to work without additional firmware. @@ -19,6 +20,14 @@ Required properties: - fsl,sdma-ram-script-name : Should contain the full path of SDMA RAM scripts firmware +Optional properties: +- fsl, sdma-event-remap : List of one or more DMA event remapping + configurations. Its format: <&gpr addr shift val> + gpr : the gpr phandle + addr : the register address of the GPR + shift : the bit shift of the GPR register + val : the value of that bit + The second cell of dma phandle specifies the peripheral type of DMA transfer. The full ID of peripheral types can be found below. diff --git a/drivers/dma/imx-sdma.c b/drivers/dma/imx-sdma.c index 62bbd79..f0339ab 100644 --- a/drivers/dma/imx-sdma.c +++ b/drivers/dma/imx-sdma.c @@ -25,6 +25,7 @@ #include <linux/interrupt.h> #include <linux/clk.h> #include <linux/delay.h> +#include <linux/regmap.h> #include <linux/sched.h> #include <linux/semaphore.h> #include <linux/spinlock.h> @@ -39,6 +40,7 @@ #include <linux/of_dma.h> #include <asm/irq.h> +#include <linux/mfd/syscon.h> #include <linux/platform_data/dma-imx-sdma.h> #include <linux/platform_data/dma-imx.h> @@ -440,12 +442,16 @@ static struct platform_device_id sdma_devtypes[] = { .name = "imx6q-sdma", .driver_data = (unsigned long)&sdma_imx6q, }, { + .name = "imx6sx-sdma", + .driver_data = (unsigned long)&sdma_imx6q, + }, { /* sentinel */ } }; MODULE_DEVICE_TABLE(platform, sdma_devtypes); static const struct of_device_id sdma_dt_ids[] = { + { .compatible = "fsl,imx6sx-sdma", .data = &sdma_imx6q, }, { .compatible = "fsl,imx6q-sdma", .data = &sdma_imx6q, }, { .compatible = "fsl,imx53-sdma", .data = &sdma_imx53, }, { .compatible = "fsl,imx51-sdma", .data = &sdma_imx51, }, @@ -1337,6 +1343,54 @@ err_firmware: release_firmware(fw); } +static int sdma_event_remap(struct sdma_engine *sdma) +{ + struct device_node *np = sdma->dev->of_node; + char propname[] = "fsl,sdma-event-remap"; + struct of_phandle_args gpr_spec; + struct regmap *gpr; + int i = 0, ret = 0; + + /* Only support DT */ + if (IS_ERR(np)) + return ret; + + /* Only apply to imx6sx platform */ + if (!of_device_is_compatible(np, "fsl,imx6sx-sdma")) + return ret; + + /* Bypass if no need to remap */ + if (!of_find_property(np, propname, NULL)) + return ret; + + /* Fetch the remap configurations of GPR */ + ret = of_parse_phandle_with_args(np, propname, "#gpr-cells", i++, + &gpr_spec); + if (ret) { + dev_err(sdma->dev, "failed to get a correct %s property: %d\n", + propname, ret); + return ret; + } + +next: + gpr = syscon_node_to_regmap(gpr_spec.np); + if (IS_ERR(gpr)) { + ret = PTR_ERR(gpr); + dev_err(sdma->dev, "failed to get gpr regmap: %d\n", reg); + return ret; + } + + /* 0 : Register address, 1 : Bit shift, 2 : Bit value */ + regmap_update_bits(gpr, gpr_spec.args[0], BIT(gpr_spec.args[1]), + gpr_spec.args[2] << gpr_spec.args[1]); + + if (!of_parse_phandle_with_args(np, propname, "#gpr-cells", i++, + &gpr_spec)) + goto next; + + return ret; +} + static int sdma_get_firmware(struct sdma_engine *sdma, const char *fw_name) { @@ -1551,6 +1605,10 @@ static int sdma_probe(struct platform_device *pdev) if (ret) goto err_init; + ret = sdma_event_remap(sdma); + if (ret) + goto err_init; + if (sdma->drvdata->script_addrs) sdma_add_scripts(sdma, sdma->drvdata->script_addrs); if (pdata && pdata->script_addrs)
The SDMA on imx6sx has a few DMA event remapping configurations inside the GPR (General Purpose Register) of that SoC. When users want to use a non-default DMA event, they need to configure the GPR register. So this patch gives an interface of the GPR and implements it in the SDMA driver so as to finish the DMA event remapping. Signed-off-by: Nicolin Chen <nicoleotsuka@gmail.com> --- .../devicetree/bindings/dma/fsl-imx-sdma.txt | 9 ++++ drivers/dma/imx-sdma.c | 58 ++++++++++++++++++++++ 2 files changed, 67 insertions(+)