[PATCH v2 0/5] ARM/DT: edma: Get IP configuration from hardware

[PATCH v2 0/5] ARM/DT: edma: Get IP configuration from hardware

[Peter Ujfalusi]

Hi,

Changes sicne v1:
- added missing patch to remove the memset from edma_of_parse_dt()

We are requesting redundant information via DT for the driver since the very same
data is available in the HW: by reading and decoding the content of CCCFG
register we can get:
Number of channels: NUM_DMACH
Number of regions: NUM_REGN
Number of slots (PaRAM sets): NUM_PAENTRY
Number of TC/EQ: NUM_EVQUE

So these does not need to be provided by the DT binding.

The driver will no longer look for these properties from DT and they can be
removed from the binding documentation and from the dtsi files as well.
The change will not introduce regression when new kernel is booted using older
DTB (since we just ignore the mentioned properties).

Regards,
Peter
---
Peter Ujfalusi (5):
  ARM: edma: No need to clean the pdata in edma_of_parse_dt()
  ARM: edma: Get IP information from HW when booting with DT
  dt/bindings: ti,edma: Remove redundant properties from documentation
  ARM: dts: am33xx: Remove obsolete properties from edma node
  ARM: dts: am4372: Remove obsolete properties from edma node

 Documentation/devicetree/bindings/dma/ti-edma.txt |   6 -
 arch/arm/boot/dts/am33xx.dtsi                     |   3 -
 arch/arm/boot/dts/am4372.dtsi                     |   3 -
 arch/arm/common/edma.c                            | 130 +++++++++++++---------
 4 files changed, 79 insertions(+), 63 deletions(-)

-- 
1.9.3

[PATCH v2 1/5] ARM: edma: No need to clean the pdata in edma_of_parse_dt()

[Peter Ujfalusi]

The pdata has been just allocated with devm_kzalloc() in
edma_setup_info_from_dt() and passed to this function.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/common/edma.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index b9bd42ad2d6e..fade9ada81f8 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -1490,8 +1490,6 @@ static int edma_of_parse_dt(struct device *dev,
 	struct edma_rsv_info *rsv_info;
 	s8 (*queue_tc_map)[2], (*queue_priority_map)[2];
 
-	memset(pdata, 0, sizeof(struct edma_soc_info));
-
 	ret = of_property_read_u32(node, "dma-channels", &value);
 	if (ret < 0)
 		return ret;
-- 
1.9.3

[PATCH v2 2/5] ARM: edma: Get IP information from HW when booting with DT

[Peter Ujfalusi]

>From CCCFG register of eDMA3 we can get all the needed information for the
driver about the IP:
Number of channels: NUM_DMACH
Number of regions: NUM_REGN
Number of slots (PaRAM sets): NUM_PAENTRY
Number of TC/EQ: NUM_EVQUE

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/common/edma.c | 128 ++++++++++++++++++++++++++++++-------------------
 1 file changed, 79 insertions(+), 49 deletions(-)

diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
index fade9ada81f8..1a98f3cd4cd9 100644
--- a/arch/arm/common/edma.c
+++ b/arch/arm/common/edma.c
@@ -102,7 +102,16 @@
 #define PARM_OFFSET(param_no)	(EDMA_PARM + ((param_no) << 5))
 
 #define EDMA_DCHMAP	0x0100  /* 64 registers */
-#define CHMAP_EXIST	BIT(24)
+
+/* CCCFG register */
+#define GET_NUM_DMACH(x)	(x & 0x7) /* bits 0-2 */
+#define GET_NUM_QDMACH(x)	((x & 0x70) >> 4) /* bits 4-6 */
+#define GET_NUM_INTCH(x)	((x & 0x700) >> 8) /* bits 8-10 */
+#define GET_NUM_PAENTRY(x)	((x & 0x7000) >> 12) /* bits 12-14 */
+#define GET_NUM_EVQUE(x)	((x & 0x70000) >> 16) /* bits 16-18 */
+#define GET_NUM_REGN(x)		((x & 0x300000) >> 20) /* bits 20-21 */
+#define CHMAP_EXIST		BIT(24)
+#define MP_EXIST		BIT(25)
 
 #define EDMA_MAX_DMACH           64
 #define EDMA_MAX_PARAMENTRY     512
@@ -1415,6 +1424,68 @@ void edma_clear_event(unsigned channel)
 }
 EXPORT_SYMBOL(edma_clear_event);
 
+static int edma_setup_info_from_hw(struct device *dev,
+				   struct edma_soc_info *pdata)
+{
+	int i;
+	u32 value, cccfg, n_tc;
+	s8 (*queue_tc_map)[2], (*queue_priority_map)[2];
+
+	/* Decode the eDMA3 configuration from CCCFG register */
+	cccfg = edma_read(0, EDMA_CCCFG);
+
+	value = GET_NUM_DMACH(cccfg);
+	pdata->n_channel = BIT(value + 1);
+
+	value = GET_NUM_REGN(cccfg);
+	pdata->n_region = BIT(value);
+
+	value = GET_NUM_PAENTRY(cccfg);
+	pdata->n_slot = BIT(value + 4);
+
+	value = GET_NUM_EVQUE(cccfg);
+	n_tc = value + 1;
+
+	dev_dbg(dev, "eDMA3 HW configuration (cccfg: 0x%08x):\n", cccfg);
+	dev_dbg(dev, "n_channel: %u\n", pdata->n_channel);
+	dev_dbg(dev, "n_region: %u\n", pdata->n_region);
+	dev_dbg(dev, "n_slot: %u\n", pdata->n_slot);
+	dev_dbg(dev, "n_tc: %u\n", n_tc);
+
+	pdata->n_cc = 1;
+
+	queue_tc_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), GFP_KERNEL);
+	if (!queue_tc_map)
+		return -ENOMEM;
+
+	for (i = 0; i < n_tc; i++) {
+		queue_tc_map[i][0] = i;
+		queue_tc_map[i][1] = i;
+	}
+	queue_tc_map[i][0] = -1;
+	queue_tc_map[i][1] = -1;
+
+	pdata->queue_tc_mapping = queue_tc_map;
+
+	queue_priority_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8),
+					  GFP_KERNEL);
+	if (!queue_priority_map)
+		return -ENOMEM;
+
+	for (i = 0; i < n_tc; i++) {
+		queue_priority_map[i][0] = i;
+		queue_priority_map[i][1] = i;
+	}
+	queue_priority_map[i][0] = -1;
+	queue_priority_map[i][1] = -1;
+
+	pdata->queue_priority_mapping = queue_priority_map;
+
+	pdata->default_queue = 0;
+
+	return 0;
+}
+
 #if IS_ENABLED(CONFIG_OF) && IS_ENABLED(CONFIG_DMADEVICES)
 
 static int edma_of_read_u32_to_s16_array(const struct device_node *np,
@@ -1483,63 +1554,16 @@ static int edma_of_parse_dt(struct device *dev,
 			    struct device_node *node,
 			    struct edma_soc_info *pdata)
 {
-	int ret = 0, i;
-	u32 value;
+	int ret = 0;
 	struct property *prop;
 	size_t sz;
 	struct edma_rsv_info *rsv_info;
-	s8 (*queue_tc_map)[2], (*queue_priority_map)[2];
-
-	ret = of_property_read_u32(node, "dma-channels", &value);
-	if (ret < 0)
-		return ret;
-	pdata->n_channel = value;
-
-	ret = of_property_read_u32(node, "ti,edma-regions", &value);
-	if (ret < 0)
-		return ret;
-	pdata->n_region = value;
-
-	ret = of_property_read_u32(node, "ti,edma-slots", &value);
-	if (ret < 0)
-		return ret;
-	pdata->n_slot = value;
-
-	pdata->n_cc = 1;
 
 	rsv_info = devm_kzalloc(dev, sizeof(struct edma_rsv_info), GFP_KERNEL);
 	if (!rsv_info)
 		return -ENOMEM;
 	pdata->rsv = rsv_info;
 
-	queue_tc_map = devm_kzalloc(dev, 8*sizeof(s8), GFP_KERNEL);
-	if (!queue_tc_map)
-		return -ENOMEM;
-
-	for (i = 0; i < 3; i++) {
-		queue_tc_map[i][0] = i;
-		queue_tc_map[i][1] = i;
-	}
-	queue_tc_map[i][0] = -1;
-	queue_tc_map[i][1] = -1;
-
-	pdata->queue_tc_mapping = queue_tc_map;
-
-	queue_priority_map = devm_kzalloc(dev, 8*sizeof(s8), GFP_KERNEL);
-	if (!queue_priority_map)
-		return -ENOMEM;
-
-	for (i = 0; i < 3; i++) {
-		queue_priority_map[i][0] = i;
-		queue_priority_map[i][1] = i;
-	}
-	queue_priority_map[i][0] = -1;
-	queue_priority_map[i][1] = -1;
-
-	pdata->queue_priority_mapping = queue_priority_map;
-
-	pdata->default_queue = 0;
-
 	prop = of_find_property(node, "ti,edma-xbar-event-map", &sz);
 	if (prop)
 		ret = edma_xbar_event_map(dev, node, pdata, sz);
@@ -1655,6 +1679,12 @@ static int edma_probe(struct platform_device *pdev)
 		if (IS_ERR(edmacc_regs_base[j]))
 			return PTR_ERR(edmacc_regs_base[j]);
 
+		if (node) {
+			/* Get eDMA3 configuration from IP */
+			ret = edma_setup_info_from_hw(dev, info[j]);
+			if (ret)
+				return ret;
+		}
 		edma_cc[j] = devm_kzalloc(&pdev->dev, sizeof(struct edma),
 					  GFP_KERNEL);
 		if (!edma_cc[j])
-- 
1.9.3

[PATCH v2 3/5] dt/bindings: ti,edma: Remove redundant properties from documentation

[Peter Ujfalusi]

>From CCCFG register of eDMA3 we can get all the needed information for the
driver about the IP:
Number of channels: NUM_DMACH
Number of regions: NUM_REGN
Number of slots (PaRAM sets): NUM_PAENTRY
Number of TC/EQ: NUM_EVQUE

The ti,edma-regions; ti,edma-slots and dma-channels in DT are
redundant since the very same information can be obtained from the HW.
The mentioned properties can be removed from the binding document.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 Documentation/devicetree/bindings/dma/ti-edma.txt | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/dma/ti-edma.txt b/Documentation/devicetree/bindings/dma/ti-edma.txt
index 9fbbdb783a72..cf8d0a2d5b33 100644
--- a/Documentation/devicetree/bindings/dma/ti-edma.txt
+++ b/Documentation/devicetree/bindings/dma/ti-edma.txt
@@ -2,11 +2,8 @@ TI EDMA
 
 Required properties:
 - compatible : "ti,edma3"
-- ti,edma-regions: Number of regions
-- ti,edma-slots: Number of slots
 - #dma-cells: Should be set to <1>
               Clients should use a single channel number per DMA request.
-- dma-channels: Specify total DMA channels per CC
 - reg: Memory map for accessing module
 - interrupt-parent: Interrupt controller the interrupt is routed through
 - interrupts: Exactly 3 interrupts need to be specified in the order:
@@ -26,9 +23,6 @@ edma: edma@49000000 {
 	compatible = "ti,edma3";
 	ti,hwmods = "tpcc", "tptc0", "tptc1", "tptc2";
 	#dma-cells = <1>;
-	dma-channels = <64>;
-	ti,edma-regions = <4>;
-	ti,edma-slots = <256>;
 	ti,edma-xbar-event-map = <1 12
 				  2 13>;
 };
-- 
1.9.3

[PATCH v2 4/5] ARM: dts: am33xx: Remove obsolete properties from edma node

[Peter Ujfalusi]

dma-channels, ti,edma-regions and ti,edma-slots no longer needed in DT since
the the same information is available in the IP's CCCFG register.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/boot/dts/am33xx.dtsi | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index baf56cc92040..5e8f647ee4ec 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -147,9 +147,6 @@
 				<0x44e10f90 0x10>;
 			interrupts = <12 13 14>;
 			#dma-cells = <1>;
-			dma-channels = <64>;
-			ti,edma-regions = <4>;
-			ti,edma-slots = <256>;
 		};
 
 		gpio0: gpio@44e07000 {
-- 
1.9.3

[PATCH v2 5/5] ARM: dts: am4372: Remove obsolete properties from edma node

[Peter Ujfalusi]

dma-channels, ti,edma-regions and ti,edma-slots no longer needed in DT since
the the same information is available in the IP's CCCFG register.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 arch/arm/boot/dts/am4372.dtsi | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/arch/arm/boot/dts/am4372.dtsi b/arch/arm/boot/dts/am4372.dtsi
index 03a225505126..b9f83b705f4b 100644
--- a/arch/arm/boot/dts/am4372.dtsi
+++ b/arch/arm/boot/dts/am4372.dtsi
@@ -108,9 +108,6 @@
 					<GIC_SPI 13 IRQ_TYPE_LEVEL_HIGH>,
 					<GIC_SPI 14 IRQ_TYPE_LEVEL_HIGH>;
 			#dma-cells = <1>;
-			dma-channels = <64>;
-			ti,edma-regions = <4>;
-			ti,edma-slots = <256>;
 		};
 
 		uart0: serial@44e09000 {
-- 
1.9.3

Re: [PATCH v2 4/5] ARM: dts: am33xx: Remove obsolete properties from edma node

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [140513 03:31]:
> dma-channels, ti,edma-regions and ti,edma-slots no longer needed in DT since
> the the same information is available in the IP's CCCFG register.

Sounds like this must go with the edma patches or else
applied later on. Probably best to merge it with edma:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH v2 5/5] ARM: dts: am4372: Remove obsolete properties from edma node

[Tony Lindgren]

* Peter Ujfalusi <peter.ujfalusi@ti.com> [140513 03:31]:
> dma-channels, ti,edma-regions and ti,edma-slots no longer needed in DT since
> the the same information is available in the IP's CCCFG register.

This too:

Acked-by: Tony Lindgren <tony@atomide.com>

Re: [PATCH v2 3/5] dt/bindings: ti,edma: Remove redundant properties from documentation

[Arnd Bergmann]

On Tuesday 13 May 2014 13:30:30 Peter Ujfalusi wrote:
> From CCCFG register of eDMA3 we can get all the needed information for the
> driver about the IP:
> Number of channels: NUM_DMACH
> Number of regions: NUM_REGN
> Number of slots (PaRAM sets): NUM_PAENTRY
> Number of TC/EQ: NUM_EVQUE
> 
> The ti,edma-regions; ti,edma-slots and dma-channels in DT are
> redundant since the very same information can be obtained from the HW.
> The mentioned properties can be removed from the binding document.
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>

I wonder if we should keep them listed as "optional" properties so you
can have a dtb file that still works with older kernels which need them.

What you do is an incompatible change to the binding, which we shouldn't
do lightly. Any new dts files don't need this information of course, but
as a general rule, I'd rather keep things like this around unless we
already have to enforce an ABI break that is well documented.

	Arnd

Re: [PATCH v2 3/5] dt/bindings: ti,edma: Remove redundant properties from documentation

[Peter Ujfalusi]

On 05/15/2014 11:01 AM, Arnd Bergmann wrote:
> On Tuesday 13 May 2014 13:30:30 Peter Ujfalusi wrote:
>> From CCCFG register of eDMA3 we can get all the needed information for the
>> driver about the IP:
>> Number of channels: NUM_DMACH
>> Number of regions: NUM_REGN
>> Number of slots (PaRAM sets): NUM_PAENTRY
>> Number of TC/EQ: NUM_EVQUE
>>
>> The ti,edma-regions; ti,edma-slots and dma-channels in DT are
>> redundant since the very same information can be obtained from the HW.
>> The mentioned properties can be removed from the binding document.
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> 
> I wonder if we should keep them listed as "optional" properties so you
> can have a dtb file that still works with older kernels which need them.
> 
> What you do is an incompatible change to the binding, which we shouldn't
> do lightly. Any new dts files don't need this information of course, but
> as a general rule, I'd rather keep things like this around unless we
> already have to enforce an ABI break that is well documented.

We could keep them as optional, or to be precise: ignored properties since we
are not going to even look at them in the future.
But I do not really see the reason to do so. With this change new kernels will
boot with old DTB - if it can not be changed which I have not seen yet. Sure
old kernel would not boot with this change, but why would you downgrade the
kernel and update the DTB on a board?
Bisecting is not a good reason since the bug you might be hunting for could be
in the DTS or in the kernel code so you need to update both of them to be sure
you reach the commit you are looking for.

-- 
Péter

Re: [PATCH v2 2/5] ARM: edma: Get IP information from HW when booting with DT

[Sekhar Nori]

Hi Peter,

On Tuesday 13 May 2014 04:00 PM, Peter Ujfalusi wrote:
> From CCCFG register of eDMA3 we can get all the needed information for the
> driver about the IP:
> Number of channels: NUM_DMACH
> Number of regions: NUM_REGN
> Number of slots (PaRAM sets): NUM_PAENTRY
> Number of TC/EQ: NUM_EVQUE
> 
> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
> ---
>  arch/arm/common/edma.c | 128 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 79 insertions(+), 49 deletions(-)
> 
> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
> index fade9ada81f8..1a98f3cd4cd9 100644
> --- a/arch/arm/common/edma.c
> +++ b/arch/arm/common/edma.c
> @@ -102,7 +102,16 @@
>  #define PARM_OFFSET(param_no)	(EDMA_PARM + ((param_no) << 5))
>  
>  #define EDMA_DCHMAP	0x0100  /* 64 registers */
> -#define CHMAP_EXIST	BIT(24)
> +
> +/* CCCFG register */
> +#define GET_NUM_DMACH(x)	(x & 0x7) /* bits 0-2 */
> +#define GET_NUM_QDMACH(x)	((x & 0x70) >> 4) /* bits 4-6 */
> +#define GET_NUM_INTCH(x)	((x & 0x700) >> 8) /* bits 8-10 */
> +#define GET_NUM_PAENTRY(x)	((x & 0x7000) >> 12) /* bits 12-14 */
> +#define GET_NUM_EVQUE(x)	((x & 0x70000) >> 16) /* bits 16-18 */
> +#define GET_NUM_REGN(x)		((x & 0x300000) >> 20) /* bits 20-21 */
> +#define CHMAP_EXIST		BIT(24)
> +#define MP_EXIST		BIT(25)

Of these new defines, you do not use GET_NUM_QDMACH(), GET_NUM_INTCH()
and MP_EXIST (at least in this patch). Can you please get rid of them if
not needed? May be its just me but its pretty annoying to search for a
define only to find its never used :)

>  
>  #define EDMA_MAX_DMACH           64
>  #define EDMA_MAX_PARAMENTRY     512
> @@ -1415,6 +1424,68 @@ void edma_clear_event(unsigned channel)
>  }
>  EXPORT_SYMBOL(edma_clear_event);
>  
> +static int edma_setup_info_from_hw(struct device *dev,
> +				   struct edma_soc_info *pdata)
> +{
> +	int i;
> +	u32 value, cccfg, n_tc;
> +	s8 (*queue_tc_map)[2], (*queue_priority_map)[2];
> +
> +	/* Decode the eDMA3 configuration from CCCFG register */
> +	cccfg = edma_read(0, EDMA_CCCFG);

You do not handle the second controller here, but its pretty straight
forward to add that by passing the controller number to this function.

I was wondering why we never read the hardware for this information
before, and strangely enough cannot find any SoC where the CCCFG
register does not publish this information correctly. I have tested on
DA850, DA830, DM365, DM355 and DM6467.

Instead of keeping this specific to OF case, the code can be simplified
much better if we read from hardware all the time. We can directly
populate the equivalent variables in the internal structure 'struct
edma' instead of populating them in 'struct edma_soc_info' and then
copying then over.

> +
> +	value = GET_NUM_DMACH(cccfg);
> +	pdata->n_channel = BIT(value + 1);
> +
> +	value = GET_NUM_REGN(cccfg);
> +	pdata->n_region = BIT(value);
> +
> +	value = GET_NUM_PAENTRY(cccfg);
> +	pdata->n_slot = BIT(value + 4);
> +
> +	value = GET_NUM_EVQUE(cccfg);
> +	n_tc = value + 1;
> +
> +	dev_dbg(dev, "eDMA3 HW configuration (cccfg: 0x%08x):\n", cccfg);
> +	dev_dbg(dev, "n_channel: %u\n", pdata->n_channel);
> +	dev_dbg(dev, "n_region: %u\n", pdata->n_region);
> +	dev_dbg(dev, "n_slot: %u\n", pdata->n_slot);
> +	dev_dbg(dev, "n_tc: %u\n", n_tc);
> +

[snip]

> +	pdata->n_cc = 1;
> +
> +	queue_tc_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), GFP_KERNEL);
> +	if (!queue_tc_map)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n_tc; i++) {
> +		queue_tc_map[i][0] = i;
> +		queue_tc_map[i][1] = i;
> +	}
> +	queue_tc_map[i][0] = -1;
> +	queue_tc_map[i][1] = -1;
> +
> +	pdata->queue_tc_mapping = queue_tc_map;
> +
> +	queue_priority_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8),
> +					  GFP_KERNEL);
> +	if (!queue_priority_map)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < n_tc; i++) {
> +		queue_priority_map[i][0] = i;
> +		queue_priority_map[i][1] = i;
> +	}
> +	queue_priority_map[i][0] = -1;
> +	queue_priority_map[i][1] = -1;
> +
> +	pdata->queue_priority_mapping = queue_priority_map;
> +
> +	pdata->default_queue = 0;

This is part is not really setting up from hardware (rather falling back
to some sane defaults for the DT case). Could you leave them in
edma_of_parse_dt()?

> @@ -1655,6 +1679,12 @@ static int edma_probe(struct platform_device *pdev)
>  		if (IS_ERR(edmacc_regs_base[j]))
>  			return PTR_ERR(edmacc_regs_base[j]);
>  
> +		if (node) {
> +			/* Get eDMA3 configuration from IP */
> +			ret = edma_setup_info_from_hw(dev, info[j]);
> +			if (ret)
> +				return ret;

No need to do this only for the DT case, I think. Also, once we get rid
of the edma_soc_info dependency, just pass edma_cc[] directly

		edma_setup_info_from_hw(dev, j, edma_cc[j]);

Thanks,
Sekhar

Re: [PATCH v2 3/5] dt/bindings: ti,edma: Remove redundant properties from documentation

[Sekhar Nori]

+ DT bindings maintainers

On Thursday 15 May 2014 01:48 PM, Peter Ujfalusi wrote:
> On 05/15/2014 11:01 AM, Arnd Bergmann wrote:
>> On Tuesday 13 May 2014 13:30:30 Peter Ujfalusi wrote:
>>> From CCCFG register of eDMA3 we can get all the needed information for the
>>> driver about the IP:
>>> Number of channels: NUM_DMACH
>>> Number of regions: NUM_REGN
>>> Number of slots (PaRAM sets): NUM_PAENTRY
>>> Number of TC/EQ: NUM_EVQUE
>>>
>>> The ti,edma-regions; ti,edma-slots and dma-channels in DT are
>>> redundant since the very same information can be obtained from the HW.
>>> The mentioned properties can be removed from the binding document.
>>>
>>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>>
>> I wonder if we should keep them listed as "optional" properties so you
>> can have a dtb file that still works with older kernels which need them.
>>
>> What you do is an incompatible change to the binding, which we shouldn't
>> do lightly. Any new dts files don't need this information of course, but
>> as a general rule, I'd rather keep things like this around unless we
>> already have to enforce an ABI break that is well documented.
> 
> We could keep them as optional, or to be precise: ignored properties since we
> are not going to even look at them in the future.
> But I do not really see the reason to do so. With this change new kernels will
> boot with old DTB - if it can not be changed which I have not seen yet. Sure
> old kernel would not boot with this change, but why would you downgrade the
> kernel and update the DTB on a board?
> Bisecting is not a good reason since the bug you might be hunting for could be
> in the DTS or in the kernel code so you need to update both of them to be sure
> you reach the commit you are looking for.

Agree with Peter here. The bisecting corner case can hopefully be
handled by appended DTB even if the original DTB was ROMed.

Thanks,
Sekhar

Re: [PATCH v2 2/5] ARM: edma: Get IP information from HW when booting with DT

[Peter Ujfalusi]

Hi Sekhar,

On 05/15/2014 11:53 AM, Sekhar Nori wrote:
> Hi Peter,
> 
> On Tuesday 13 May 2014 04:00 PM, Peter Ujfalusi wrote:
>> From CCCFG register of eDMA3 we can get all the needed information for the
>> driver about the IP:
>> Number of channels: NUM_DMACH
>> Number of regions: NUM_REGN
>> Number of slots (PaRAM sets): NUM_PAENTRY
>> Number of TC/EQ: NUM_EVQUE
>>
>> Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
>> ---
>>  arch/arm/common/edma.c | 128 ++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 79 insertions(+), 49 deletions(-)
>>
>> diff --git a/arch/arm/common/edma.c b/arch/arm/common/edma.c
>> index fade9ada81f8..1a98f3cd4cd9 100644
>> --- a/arch/arm/common/edma.c
>> +++ b/arch/arm/common/edma.c
>> @@ -102,7 +102,16 @@
>>  #define PARM_OFFSET(param_no)	(EDMA_PARM + ((param_no) << 5))
>>  
>>  #define EDMA_DCHMAP	0x0100  /* 64 registers */
>> -#define CHMAP_EXIST	BIT(24)
>> +
>> +/* CCCFG register */
>> +#define GET_NUM_DMACH(x)	(x & 0x7) /* bits 0-2 */
>> +#define GET_NUM_QDMACH(x)	((x & 0x70) >> 4) /* bits 4-6 */
>> +#define GET_NUM_INTCH(x)	((x & 0x700) >> 8) /* bits 8-10 */
>> +#define GET_NUM_PAENTRY(x)	((x & 0x7000) >> 12) /* bits 12-14 */
>> +#define GET_NUM_EVQUE(x)	((x & 0x70000) >> 16) /* bits 16-18 */
>> +#define GET_NUM_REGN(x)		((x & 0x300000) >> 20) /* bits 20-21 */
>> +#define CHMAP_EXIST		BIT(24)
>> +#define MP_EXIST		BIT(25)
> 
> Of these new defines, you do not use GET_NUM_QDMACH(), GET_NUM_INTCH()
> and MP_EXIST (at least in this patch). Can you please get rid of them if
> not needed? May be its just me but its pretty annoying to search for a
> define only to find its never used :)

Sure, I can remove the ones we are not using in the code. I usually prefer to
have full description if the register even if we only use one bit from the
register.

> 
>>  
>>  #define EDMA_MAX_DMACH           64
>>  #define EDMA_MAX_PARAMENTRY     512
>> @@ -1415,6 +1424,68 @@ void edma_clear_event(unsigned channel)
>>  }
>>  EXPORT_SYMBOL(edma_clear_event);
>>  
>> +static int edma_setup_info_from_hw(struct device *dev,
>> +				   struct edma_soc_info *pdata)
>> +{
>> +	int i;
>> +	u32 value, cccfg, n_tc;
>> +	s8 (*queue_tc_map)[2], (*queue_priority_map)[2];
>> +
>> +	/* Decode the eDMA3 configuration from CCCFG register */
>> +	cccfg = edma_read(0, EDMA_CCCFG);
> 
> You do not handle the second controller here, but its pretty straight
> forward to add that by passing the controller number to this function.

The second controller is not handled because in DT boot we only handle 1 cc as
far as I know. I don't know why, but this is how the DT support has been
written and used.

> I was wondering why we never read the hardware for this information
> before, and strangely enough cannot find any SoC where the CCCFG
> register does not publish this information correctly. I have tested on
> DA850, DA830, DM365, DM355 and DM6467.

Good question. I was also puzzled about this.

> Instead of keeping this specific to OF case, the code can be simplified
> much better if we read from hardware all the time. We can directly
> populate the equivalent variables in the internal structure 'struct
> edma' instead of populating them in 'struct edma_soc_info' and then
> copying then over.

Yes, we can switch the non DT boot to this mode as well, true. At first I
wanted to change code which I can test easily. For non DT boot I would need to
set up my old daVinci board ;)

>> +
>> +	value = GET_NUM_DMACH(cccfg);
>> +	pdata->n_channel = BIT(value + 1);
>> +
>> +	value = GET_NUM_REGN(cccfg);
>> +	pdata->n_region = BIT(value);
>> +
>> +	value = GET_NUM_PAENTRY(cccfg);
>> +	pdata->n_slot = BIT(value + 4);
>> +
>> +	value = GET_NUM_EVQUE(cccfg);
>> +	n_tc = value + 1;
>> +
>> +	dev_dbg(dev, "eDMA3 HW configuration (cccfg: 0x%08x):\n", cccfg);
>> +	dev_dbg(dev, "n_channel: %u\n", pdata->n_channel);
>> +	dev_dbg(dev, "n_region: %u\n", pdata->n_region);
>> +	dev_dbg(dev, "n_slot: %u\n", pdata->n_slot);
>> +	dev_dbg(dev, "n_tc: %u\n", n_tc);
>> +
> 
> [snip]
> 
>> +	pdata->n_cc = 1;
>> +
>> +	queue_tc_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), GFP_KERNEL);
>> +	if (!queue_tc_map)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < n_tc; i++) {
>> +		queue_tc_map[i][0] = i;
>> +		queue_tc_map[i][1] = i;
>> +	}
>> +	queue_tc_map[i][0] = -1;
>> +	queue_tc_map[i][1] = -1;
>> +
>> +	pdata->queue_tc_mapping = queue_tc_map;
>> +
>> +	queue_priority_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8),
>> +					  GFP_KERNEL);
>> +	if (!queue_priority_map)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < n_tc; i++) {
>> +		queue_priority_map[i][0] = i;
>> +		queue_priority_map[i][1] = i;
>> +	}
>> +	queue_priority_map[i][0] = -1;
>> +	queue_priority_map[i][1] = -1;
>> +
>> +	pdata->queue_priority_mapping = queue_priority_map;
>> +
>> +	pdata->default_queue = 0;
> 
> This is part is not really setting up from hardware (rather falling back
> to some sane defaults for the DT case). Could you leave them in
> edma_of_parse_dt()?

Not really since the number of tc is not know as early as edma_of_parse_dt(),
we used to a magic number of 3 in place for n_tc previously.
We are doing similar things as previously done in edma_of_parse_dt() but with
'correct' number of tc.

>> @@ -1655,6 +1679,12 @@ static int edma_probe(struct platform_device *pdev)
>>  		if (IS_ERR(edmacc_regs_base[j]))
>>  			return PTR_ERR(edmacc_regs_base[j]);
>>  
>> +		if (node) {
>> +			/* Get eDMA3 configuration from IP */
>> +			ret = edma_setup_info_from_hw(dev, info[j]);
>> +			if (ret)
>> +				return ret;
> 
> No need to do this only for the DT case, I think. Also, once we get rid
> of the edma_soc_info dependency, just pass edma_cc[] directly
> 
> 		edma_setup_info_from_hw(dev, j, edma_cc[j]);

Yes, let's do this for DT and not DT boot as well.
I will keep the queue_tc_map and queue_priority_map setup in there I think to
be done in case of DT boot.

I'll try to craft v3 as soon as I can.

Thanks,
Péter

Re: [PATCH v2 2/5] ARM: edma: Get IP information from HW when booting with DT

[Sekhar Nori]

On Thursday 15 May 2014 06:00 PM, Peter Ujfalusi wrote:

> The second controller is not handled because in DT boot we only handle 1 cc as
> far as I know. I don't know why, but this is how the DT support has been
> written and used.

Its just because none of the platforms under heavy development use two
controllers. Joel promised to work on it at some point ;)

> 
>> I was wondering why we never read the hardware for this information
>> before, and strangely enough cannot find any SoC where the CCCFG
>> register does not publish this information correctly. I have tested on
>> DA850, DA830, DM365, DM355 and DM6467.
> 
> Good question. I was also puzzled about this.
> 
>> Instead of keeping this specific to OF case, the code can be simplified
>> much better if we read from hardware all the time. We can directly
>> populate the equivalent variables in the internal structure 'struct
>> edma' instead of populating them in 'struct edma_soc_info' and then
>> copying then over.
> 
> Yes, we can switch the non DT boot to this mode as well, true. At first I
> wanted to change code which I can test easily. For non DT boot I would need to
> set up my old daVinci board ;)

I can help testing (and even with writing the DaVinci platform specific
patches).

>>> +	pdata->n_cc = 1;
>>> +
>>> +	queue_tc_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8), GFP_KERNEL);
>>> +	if (!queue_tc_map)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < n_tc; i++) {
>>> +		queue_tc_map[i][0] = i;
>>> +		queue_tc_map[i][1] = i;
>>> +	}
>>> +	queue_tc_map[i][0] = -1;
>>> +	queue_tc_map[i][1] = -1;
>>> +
>>> +	pdata->queue_tc_mapping = queue_tc_map;
>>> +
>>> +	queue_priority_map = devm_kzalloc(dev, (n_tc + 1) * sizeof(s8),
>>> +					  GFP_KERNEL);
>>> +	if (!queue_priority_map)
>>> +		return -ENOMEM;
>>> +
>>> +	for (i = 0; i < n_tc; i++) {
>>> +		queue_priority_map[i][0] = i;
>>> +		queue_priority_map[i][1] = i;
>>> +	}
>>> +	queue_priority_map[i][0] = -1;
>>> +	queue_priority_map[i][1] = -1;
>>> +
>>> +	pdata->queue_priority_mapping = queue_priority_map;
>>> +
>>> +	pdata->default_queue = 0;
>>
>> This is part is not really setting up from hardware (rather falling back
>> to some sane defaults for the DT case). Could you leave them in
>> edma_of_parse_dt()?
> 
> Not really since the number of tc is not know as early as edma_of_parse_dt(),
> we used to a magic number of 3 in place for n_tc previously.
> We are doing similar things as previously done in edma_of_parse_dt() but with
> 'correct' number of tc.

Okay. I did not notice the n_tc hardcoding. In that case to make this
function usable on non-DT case we will have to do something like:

	/* Default to 1 if nothing passed */
	if (!pdata->n_cc)
		pdata->n_cc = 1;

	if (!pdata->queue_priority_mapping) {
		queue_priority_map = devm_kzalloc(...)
	}

I was hoping we could avoid that.

>>> @@ -1655,6 +1679,12 @@ static int edma_probe(struct platform_device *pdev)
>>>  		if (IS_ERR(edmacc_regs_base[j]))
>>>  			return PTR_ERR(edmacc_regs_base[j]);
>>>  
>>> +		if (node) {
>>> +			/* Get eDMA3 configuration from IP */
>>> +			ret = edma_setup_info_from_hw(dev, info[j]);
>>> +			if (ret)
>>> +				return ret;
>>
>> No need to do this only for the DT case, I think. Also, once we get rid
>> of the edma_soc_info dependency, just pass edma_cc[] directly
>>
>> 		edma_setup_info_from_hw(dev, j, edma_cc[j]);
> 
> Yes, let's do this for DT and not DT boot as well.
> I will keep the queue_tc_map and queue_priority_map setup in there I think to
> be done in case of DT boot.

Right.

> 
> I'll try to craft v3 as soon as I can.

Thanks.

Regards,
Sekhar

Re: [PATCH v2 2/5] ARM: edma: Get IP information from HW when booting with DT

[Joel Fernandes]

On 05/15/2014 07:48 AM, Sekhar Nori wrote:
> On Thursday 15 May 2014 06:00 PM, Peter Ujfalusi wrote:
> 
>> The second controller is not handled because in DT boot we only handle 1 cc as
>> far as I know. I don't know why, but this is how the DT support has been
>> written and used.
> 
> Its just because none of the platforms under heavy development use two
> controllers. Joel promised to work on it at some point ;)

Yeah, often things like that go down in priority when there aren't
users, and when other things on fire need to be tended to :-D

I think the next on-fire thing as far as EDMA goes would be to move out
into drivers/edma. I like Peter's series in that it probably is a step
forward there.

thanks,

-Joel