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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
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.
--
FTTC broadband for 0.8mile line: 5.8Mbps down 500kbps up. Estimation
in database were 13.1 to 19Mbit for a good line, about 7.5+ for a bad.
Estimate before purchase was "up to 13.2Mbit".
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
On Fri, Feb 21, 2014 at 11:36 AM, Joel Fernandes <[email protected]> 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
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Thanks for your suggestions.
On Tue, Feb 25, 2014 at 10:22 AM, Rob Herring <[email protected]> wrote:
> On Fri, Feb 21, 2014 at 11:36 AM, Joel Fernandes <[email protected]> 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