| Message ID | 1392956661-28787-1-git-send-email-joelf@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Feb 21, 2014 at 04:24:21AM +0000, Joel Fernandes wrote: > Currently, EDMA driver uses of_address_to_resource for getting Channel > controller and x-bar register resources. Use platform_get_resource_by_name > instead regardless of whether its DT-boot or not, document the new reg-names > properties. Doesn't this break existing DTBs? > > Also, while at it get rid of the assumption in the code that "CC" is at reg > index 0 in the DT and xbar is at offset 1. Instead use reg-names to get the > memory resource in concern keeping things much cleaner and simpler. This also > makes it possible to have multiple channel controllers. While this is nice I think we have to have a fallback to the existing behaviour if there's no reg-names property present, unless we know for certain no-one is possibly using an existing DTB. Cheers, Mark. > > Signed-off-by: Joel Fernandes <joelf@ti.com> > --- > Documentation/devicetree/bindings/dma/ti-edma.txt | 7 ++++++ > arch/arm/boot/dts/am33xx.dtsi | 1 + > arch/arm/boot/dts/am4372.dtsi | 1 + > arch/arm/common/edma.c | 28 ++++++++++----------- > 4 files changed, 22 insertions(+), 15 deletions(-) > > diff --git a/Documentation/devicetree/bindings/dma/ti-edma.txt b/Documentation/devicetree/bindings/dma/ti-edma.txt > index 9fbbdb7..176e42b 100644 > --- a/Documentation/devicetree/bindings/dma/ti-edma.txt > +++ b/Documentation/devicetree/bindings/dma/ti-edma.txt > @@ -8,6 +8,12 @@ Required properties: > Clients should use a single channel number per DMA request. > - dma-channels: Specify total DMA channels per CC > - reg: Memory map for accessing module > +- reg-names: Since there can be different memory regions, each reg > + entry should correspond to one of the following reg-names (X being 0 to N): > + edma_ccX: memory map for Xth Channel Controller > + edma_tcX: memory map for Xth Transfer Controller > + Additionally there can be a memory map for xbar (in control module): > + edma_xbar: memory map for xbar access. > - interrupt-parent: Interrupt controller the interrupt is routed through > - interrupts: Exactly 3 interrupts need to be specified in the order: > 1. Transfer completion interrupt. > @@ -21,6 +27,7 @@ Example: > > edma: edma@49000000 { > reg = <0x49000000 0x10000>; > + reg-names = "edma_cc0"; > interrupt-parent = <&intc>; > interrupts = <12 13 14>; > compatible = "ti,edma3"; > diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi > index 2b66e67..55f5723 100644 > --- a/arch/arm/boot/dts/am33xx.dtsi > +++ b/arch/arm/boot/dts/am33xx.dtsi > @@ -123,6 +123,7 @@ > ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2"; > reg = <0x49000000 0x10000>, > <0x44e10f90 0x10>; > + reg-names = "edma_cc0", "edma_xbar"; > interrupts = <12 13 14>; > #dma-cells = <1>; > dma-channels = <64>; > diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi > index babdc84..1323a97 100644 > --- a/arch/arm/boot/dts/am4372.dtsi > +++ b/arch/arm/boot/dts/am4372.dtsi > @@ -100,6 +100,7 @@ > ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2"; > reg = <0x49000000 0x10000>, > <0x44e10f90 0x10>; > + reg-names = "edma_cc0", "edma_xbar"; > interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>, > <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; > diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c > index dc95efc..ae0ccae 100644 > --- a/arch/arm/common/edma.c > +++ b/arch/arm/common/edma.c > @@ -1458,10 +1458,11 @@ static int edma_xbar_event_map(struct device *dev, > struct edma_soc_info *pdata, int len) > { > int ret, i; > - struct resource res; > + struct resource *res; > void __iomem *xbar; > const s16 (*xbar_chans)[2]; > u32 shift, offset, mux; > + struct platform_device *pdev; > > xbar_chans = devm_kzalloc(dev, > len/sizeof(s16) + 2*sizeof(s16), > @@ -1469,11 +1470,14 @@ static int edma_xbar_event_map(struct device *dev, > if (!xbar_chans) > return -ENOMEM; > > - ret = of_address_to_resource(node, 1, &res); > - if (ret) > + pdev = to_platform_device(dev); > + res = platform_get_resource_byname(pdev, > + IORESOURCE_MEM, > + "edma_xbar"); > + if (!res) > return -EIO; > > - xbar = devm_ioremap(dev, res.start, resource_size(&res)); > + xbar = devm_ioremap(dev, res->start, resource_size(res)); > if (!xbar) > return -ENOMEM; > > @@ -1614,7 +1618,6 @@ static int edma_probe(struct platform_device *pdev) > int irq[EDMA_MAX_CC] = {0, 0}; > int err_irq[EDMA_MAX_CC] = {0, 0}; > struct resource *r[EDMA_MAX_CC] = {NULL}; > - struct resource res[EDMA_MAX_CC]; > char res_name[10]; > char irq_name[10]; > struct device_node *node = pdev->dev.of_node; > @@ -1653,16 +1656,11 @@ static int edma_probe(struct platform_device *pdev) > return -ENODEV; > break; > } > - if (node) { > - ret = of_address_to_resource(node, j, &res[j]); > - if (!ret) > - r[j] = &res[j]; > - } else { > - sprintf(res_name, "edma_cc%d", j); > - r[j] = platform_get_resource_byname(pdev, > - IORESOURCE_MEM, > - res_name); > - } > + > + sprintf(res_name, "edma_cc%d", j); > + r[j] = platform_get_resource_byname(pdev, > + IORESOURCE_MEM, > + res_name); > if (!r[j]) { > if (found) > break; > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Feb 20, 2014 at 10:24:21PM -0600, Joel Fernandes wrote: > diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c > index dc95efc..ae0ccae 100644 > --- a/arch/arm/common/edma.c > +++ b/arch/arm/common/edma.c > @@ -1469,11 +1470,14 @@ static int edma_xbar_event_map(struct device *dev, > if (!xbar_chans) > return -ENOMEM; > > - ret = of_address_to_resource(node, 1, &res); > - if (ret) > + pdev = to_platform_device(dev); > + res = platform_get_resource_byname(pdev, > + IORESOURCE_MEM, > + "edma_xbar"); > + if (!res) > return -EIO; > > - xbar = devm_ioremap(dev, res.start, resource_size(&res)); > + xbar = devm_ioremap(dev, res->start, resource_size(res)); > if (!xbar) > return -ENOMEM; Irrespective of what Mark said, this should be upgraded to use devm_ioremap_resource() which does all the checking necessary (and claims the resource.) That can be done irrespective of this change.
On 02/21/2014 06:15 AM, Mark Rutland wrote: >> Also, while at it get rid of the assumption in the code that "CC" is at reg >> index 0 in the DT and xbar is at offset 1. Instead use reg-names to get the >> memory resource in concern keeping things much cleaner and simpler. This also >> makes it possible to have multiple channel controllers. > > While this is nice I think we have to have a fallback to the existing > behaviour if there's no reg-names property present, unless we know for > certain no-one is possibly using an existing DTB. Yes, its true it break existing DTBs but currently only 2 TI SoCs use EDMA this way, the vast majority of EDMA users are yet to follow where we can do it right. Further, the old bindings are really limiting specially the 2 CC case and if additionally memory maps are used in the future. So keeping the old binding is limiting in this regard. Here is what the platform_data used to look like when used by mach-davinci: static struct resource da850_edma_resources[] = { { .name = "edma_cc0", .start = DA8XX_TPCC_BASE, .end = DA8XX_TPCC_BASE + SZ_32K - 1, .flags = IORESOURCE_MEM, }, { .name = "edma_tc0", .start = DA8XX_TPTC0_BASE, .end = DA8XX_TPTC0_BASE + SZ_1K - 1, .flags = IORESOURCE_MEM, }, { .name = "edma_tc1", .start = DA8XX_TPTC1_BASE, .end = DA8XX_TPTC1_BASE + SZ_1K - 1, .flags = IORESOURCE_MEM, }, { .name = "edma_cc1", .start = DA850_TPCC1_BASE, .end = DA850_TPCC1_BASE + SZ_32K - 1, .flags = IORESOURCE_MEM, }, As you can see, there are several memory maps and different interpretations. Considering this, IMO- it makes sense to pay a small price to keep the semantics sane. On the other hand, the other 2 options are: 1. We add a fallback if reg-names look up fails. 2. We inject reg-names property into edma DT nodes that don't have them, and make sure all future dtsi with edma nodes mention the reg-names property. These 2 are a bit error prone though, for example if someone deliberately forgets to mention reg-names, and the code still works, but misbehaves in some way. Regards, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Feb 21, 2014 at 11:36 AM, Joel Fernandes <joelf@ti.com> wrote: > On 02/21/2014 06:15 AM, Mark Rutland wrote: >>> Also, while at it get rid of the assumption in the code that "CC" is at reg >>> index 0 in the DT and xbar is at offset 1. Instead use reg-names to get the >>> memory resource in concern keeping things much cleaner and simpler. This also >>> makes it possible to have multiple channel controllers. >> >> While this is nice I think we have to have a fallback to the existing >> behaviour if there's no reg-names property present, unless we know for >> certain no-one is possibly using an existing DTB. > > Yes, its true it break existing DTBs but currently only 2 TI SoCs use EDMA this > way, the vast majority of EDMA users are yet to follow where we can do it right. > Further, the old bindings are really limiting specially the 2 CC case and if > additionally memory maps are used in the future. So keeping the old binding is > limiting in this regard. > > Here is what the platform_data used to look like when used by mach-davinci: > > static struct resource da850_edma_resources[] = { > { > .name = "edma_cc0", > .start = DA8XX_TPCC_BASE, > .end = DA8XX_TPCC_BASE + SZ_32K - 1, > .flags = IORESOURCE_MEM, > }, > { > .name = "edma_tc0", > .start = DA8XX_TPTC0_BASE, > .end = DA8XX_TPTC0_BASE + SZ_1K - 1, > .flags = IORESOURCE_MEM, > }, > { > .name = "edma_tc1", > .start = DA8XX_TPTC1_BASE, > .end = DA8XX_TPTC1_BASE + SZ_1K - 1, > .flags = IORESOURCE_MEM, > }, > { > .name = "edma_cc1", > .start = DA850_TPCC1_BASE, > .end = DA850_TPCC1_BASE + SZ_32K - 1, > .flags = IORESOURCE_MEM, > }, > > As you can see, there are several memory maps and different interpretations. > Considering this, IMO- it makes sense to pay a small price to keep the semantics > sane. Then do 1 compatible string per interpretation. That can then define the number of CCs and TCs. > > On the other hand, the other 2 options are: > 1. We add a fallback if reg-names look up fails. reg-names is supposed to be optional, so you should already have that fallback. Rob > 2. We inject reg-names property into edma DT nodes that don't have them, and > make sure all future dtsi with edma nodes mention the reg-names property. > > These 2 are a bit error prone though, for example if someone deliberately > forgets to mention reg-names, and the code still works, but misbehaves in some way. > > Regards, > -Joel > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for your suggestions. On Tue, Feb 25, 2014 at 10:22 AM, Rob Herring <robherring2@gmail.com> wrote: > On Fri, Feb 21, 2014 at 11:36 AM, Joel Fernandes <joelf@ti.com> wrote: >> On 02/21/2014 06:15 AM, Mark Rutland wrote: >>>> Also, while at it get rid of the assumption in the code that "CC" is at reg >>>> index 0 in the DT and xbar is at offset 1. Instead use reg-names to get the >>>> memory resource in concern keeping things much cleaner and simpler. This also >>>> makes it possible to have multiple channel controllers. >>> >>> While this is nice I think we have to have a fallback to the existing >>> behaviour if there's no reg-names property present, unless we know for >>> certain no-one is possibly using an existing DTB. >> >> Yes, its true it break existing DTBs but currently only 2 TI SoCs use EDMA this >> way, the vast majority of EDMA users are yet to follow where we can do it right. >> Further, the old bindings are really limiting specially the 2 CC case and if >> additionally memory maps are used in the future. So keeping the old binding is >> limiting in this regard. >> >> Here is what the platform_data used to look like when used by mach-davinci: >> >> static struct resource da850_edma_resources[] = { >> { >> .name = "edma_cc0", >> .start = DA8XX_TPCC_BASE, >> .end = DA8XX_TPCC_BASE + SZ_32K - 1, >> .flags = IORESOURCE_MEM, >> }, >> { >> .name = "edma_tc0", >> .start = DA8XX_TPTC0_BASE, >> .end = DA8XX_TPTC0_BASE + SZ_1K - 1, >> .flags = IORESOURCE_MEM, >> }, >> { >> .name = "edma_tc1", >> .start = DA8XX_TPTC1_BASE, >> .end = DA8XX_TPTC1_BASE + SZ_1K - 1, >> .flags = IORESOURCE_MEM, >> }, >> { >> .name = "edma_cc1", >> .start = DA850_TPCC1_BASE, >> .end = DA850_TPCC1_BASE + SZ_32K - 1, >> .flags = IORESOURCE_MEM, >> }, >> >> As you can see, there are several memory maps and different interpretations. >> Considering this, IMO- it makes sense to pay a small price to keep the semantics >> sane. > > Then do 1 compatible string per interpretation. That can then define > the number of CCs and TCs. Ok. >> >> On the other hand, the other 2 options are: >> 1. We add a fallback if reg-names look up fails. > > reg-names is supposed to be optional, so you should already have that fallback. With the new binding, without reg-names not sure how to fall back there can variable number of different reg entries of different types. There's no way to know when one ends and the other begins. The fall back I mentioned can only work for old dtbs that only specify 1 CC. Thanks, -Joel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/dma/ti-edma.txt b/Documentation/devicetree/bindings/dma/ti-edma.txt index 9fbbdb7..176e42b 100644 --- a/Documentation/devicetree/bindings/dma/ti-edma.txt +++ b/Documentation/devicetree/bindings/dma/ti-edma.txt @@ -8,6 +8,12 @@ Required properties: Clients should use a single channel number per DMA request. - dma-channels: Specify total DMA channels per CC - reg: Memory map for accessing module +- reg-names: Since there can be different memory regions, each reg + entry should correspond to one of the following reg-names (X being 0 to N): + edma_ccX: memory map for Xth Channel Controller + edma_tcX: memory map for Xth Transfer Controller + Additionally there can be a memory map for xbar (in control module): + edma_xbar: memory map for xbar access. - interrupt-parent: Interrupt controller the interrupt is routed through - interrupts: Exactly 3 interrupts need to be specified in the order: 1. Transfer completion interrupt. @@ -21,6 +27,7 @@ Example: edma: edma@49000000 { reg = <0x49000000 0x10000>; + reg-names = "edma_cc0"; interrupt-parent = <&intc>; interrupts = <12 13 14>; compatible = "ti,edma3"; diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi index 2b66e67..55f5723 100644 --- a/arch/arm/boot/dts/am33xx.dtsi +++ b/arch/arm/boot/dts/am33xx.dtsi @@ -123,6 +123,7 @@ ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2"; reg = <0x49000000 0x10000>, <0x44e10f90 0x10>; + reg-names = "edma_cc0", "edma_xbar"; interrupts = <12 13 14>; #dma-cells = <1>; dma-channels = <64>; diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi index babdc84..1323a97 100644 --- a/arch/arm/boot/dts/am4372.dtsi +++ b/arch/arm/boot/dts/am4372.dtsi @@ -100,6 +100,7 @@ ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2"; reg = <0x49000000 0x10000>, <0x44e10f90 0x10>; + reg-names = "edma_cc0", "edma_xbar"; interrupts = <GIC_SPI 12 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>; diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c index dc95efc..ae0ccae 100644 --- a/arch/arm/common/edma.c +++ b/arch/arm/common/edma.c @@ -1458,10 +1458,11 @@ static int edma_xbar_event_map(struct device *dev, struct edma_soc_info *pdata, int len) { int ret, i; - struct resource res; + struct resource *res; void __iomem *xbar; const s16 (*xbar_chans)[2]; u32 shift, offset, mux; + struct platform_device *pdev; xbar_chans = devm_kzalloc(dev, len/sizeof(s16) + 2*sizeof(s16), @@ -1469,11 +1470,14 @@ static int edma_xbar_event_map(struct device *dev, if (!xbar_chans) return -ENOMEM; - ret = of_address_to_resource(node, 1, &res); - if (ret) + pdev = to_platform_device(dev); + res = platform_get_resource_byname(pdev, + IORESOURCE_MEM, + "edma_xbar"); + if (!res) return -EIO; - xbar = devm_ioremap(dev, res.start, resource_size(&res)); + xbar = devm_ioremap(dev, res->start, resource_size(res)); if (!xbar) return -ENOMEM; @@ -1614,7 +1618,6 @@ static int edma_probe(struct platform_device *pdev) int irq[EDMA_MAX_CC] = {0, 0}; int err_irq[EDMA_MAX_CC] = {0, 0}; struct resource *r[EDMA_MAX_CC] = {NULL}; - struct resource res[EDMA_MAX_CC]; char res_name[10]; char irq_name[10]; struct device_node *node = pdev->dev.of_node; @@ -1653,16 +1656,11 @@ static int edma_probe(struct platform_device *pdev) return -ENODEV; break; } - if (node) { - ret = of_address_to_resource(node, j, &res[j]); - if (!ret) - r[j] = &res[j]; - } else { - sprintf(res_name, "edma_cc%d", j); - r[j] = platform_get_resource_byname(pdev, - IORESOURCE_MEM, - res_name); - } + + sprintf(res_name, "edma_cc%d", j); + r[j] = platform_get_resource_byname(pdev, + IORESOURCE_MEM, + res_name); if (!r[j]) { if (found) break;
Currently, EDMA driver uses of_address_to_resource for getting Channel controller and x-bar register resources. Use platform_get_resource_by_name instead regardless of whether its DT-boot or not, document the new reg-names properties. Also, while at it get rid of the assumption in the code that "CC" is at reg index 0 in the DT and xbar is at offset 1. Instead use reg-names to get the memory resource in concern keeping things much cleaner and simpler. This also makes it possible to have multiple channel controllers. Signed-off-by: Joel Fernandes <joelf@ti.com> --- Documentation/devicetree/bindings/dma/ti-edma.txt | 7 ++++++ arch/arm/boot/dts/am33xx.dtsi | 1 + arch/arm/boot/dts/am4372.dtsi | 1 + arch/arm/common/edma.c | 28 ++++++++++----------- 4 files changed, 22 insertions(+), 15 deletions(-)