arm: dts: lpc32xx: fixes and updates to lpc32xx.dtsi

The change corrects invalid custom translations to 1:1 translations, otherwise during initialization there are too many failed attempts to translate addresses from subnodes, which anyway result in fallback 1:1 translations, also it is found that due to this problem proper usage of ranges property in subnodes (e.g. for defining flash partitions) can not be done.

Signed-off-by Vladimir Zapolskiy <[email protected]>

Quoted 9 lines · On Tuesday 13 October 2015 02:54:02 Vladimir Zapolskiy wrote:

The change corrects invalid custom translations to 1:1 translations, otherwise during initialization there are too many failed attempts to translate addresses from subnodes, which anyway result in fallback 1:1 translations, also it is found that due to this problem proper usage of ranges property in subnodes (e.g. for defining flash partitions) can not be done.

Signed-off-by Vladimir Zapolskiy <[email protected]>

I don't get it. What kind of errors do you see? The existing version looks cleaner than the new one, as it only translates the MMIO areas that are actually used.

Quoted 4 lines · Arnd Bergmann

I don't get it. What kind of errors do you see? The existing version looks cleaner than the new one, as it only translates the MMIO areas that are actually used.

The problem is found when I add PL175 device node to lpc32xx.dtsi and expand the node in my board file. The external memory controller manages up to 4 memory devices, which by means of the controller are mapped into physical memory starting from address 0xe0000000.

Below are my declarations, similar to one found in lpc18xx.dtsi.

lpc32xx.dtsi change common for all LPC32xx boards:

emc: emc at 31080000 {
	compatible = "arm,pl175", "arm,primecell";
	reg = <0x31080000 0x1000>;
	clocks = <&scf LPC32XX_CLK_DDRAM>;
	clock-names = "mpmcclk";

	#address-cells = <1>;
	#size-cells = <1>;
	ranges = <0 0xe0000000 0x01000000>,
		 <1 0xe1000000 0x01000000>,
		 <2 0xe2000000 0x01000000>,
		 <3 0xe3000000 0x01000000>;
	status = "disabled";
};

Support of EMC device node will be added to lpc32xx.dtsi, when this kind of problem is fixed.

Quoted 15 lines · On Tuesday 13 October 2015 18:51:24 Vladimir Zapolskiy wrote:

Quoted 4 lines · Arnd Bergmann

I don't get it. What kind of errors do you see? The existing version looks cleaner than the new one, as it only translates the MMIO areas that are actually used.

The problem is found when I add PL175 device node to lpc32xx.dtsi and expand the node in my board file. The external memory controller manages up to 4 memory devices, which by means of the controller are mapped into physical memory starting from address 0xe0000000.

Below are my declarations, similar to one found in lpc18xx.dtsi.

lpc32xx.dtsi change common for all LPC32xx boards:

Quoted 15 lines

emc: emc at 31080000 {
	compatible = "arm,pl175", "arm,primecell";
	reg = <0x31080000 0x1000>;
	clocks = <&scf LPC32XX_CLK_DDRAM>;
	clock-names = "mpmcclk";

	#address-cells = <1>;
	#size-cells = <1>;
	ranges = <0 0xe0000000 0x01000000>,
		 <1 0xe1000000 0x01000000>,
		 <2 0xe2000000 0x01000000>,
		 <3 0xe3000000 0x01000000>;
	status = "disabled";
};

Ok, got it.

Quoted 2 lines

Support of EMC device node will be added to lpc32xx.dtsi, when this kind of problem is fixed.

As I see it, the problem is that you now have ranges that are not for devices inside of the device but that are external. As the memory-controller node needs both its own registers and the translation for the external ones, we unfortunately can't just put it in the root node, which would avoid the issue.

Instead, I would suggest adding a range for the 0xe0000000 area.

Ok, practically it should work for my purposes, but the change must be done along with added EMC device node description.

I'm not so confident that it is correct to add description of static memory banks to ahb node though, please give me a short confirmation,

then I'll send a change for inclusion of EMC description -- the one above excluding clocks and clock-names properties, work on CCF is in progress.

Quoted 5 lines · On Wednesday 14 October 2015 02:13:49 Vladimir Zapolskiy wrote:

Ok, practically it should work for my purposes, but the change must be done along with added EMC device node description.

I'm not so confident that it is correct to add description of static memory banks to ahb node though, please give me a short confirmation,

The DT should reflect whichever memory ranges are accessible on the bus. I've looked up the memory map in the data sheet, and I think a reasonable representation would be

/ahb {
	ranges = <0x20000000 0x20000000 0x10000000>, /* AHB port 5 */
		 <0x30000000 0x30000000 0x10000000>, /* AHB port 6 */
		 <0x40000000 0x40000000 0x10000000>, /* AHB port 7 */
		 <0x80000000 0x80000000 0x40000000>, /* EMC DYCS0/1 */
		 <0xE0000000 0xE0000000 0x04000000>; /* EMC CS0-3 */

	apb {
		ranges = <0x20080000 0x20080000 0x00020000>;
	};

};

alternatively, each AHB port could be a separate node, with a more direct translation like

/ahb5 {
	ranges = <0 0x20000000 0x10000000>;

	apb {
		ranges = <0 0x80000 0x20000>;
	};
};
/ahb6 {
	ranges = <0 0x30000000 0x10000000>,	      /* AHB registers */
		 <0x80000000 0x80000000 0x40000000>, /* EMC DYCS0/1 */
		 <0xE0000000 0xE0000000 0x04000000>; /* EMC CS0-3 */

	memory-controller at 1080000 {
		reg = <0x1080000 0x10000>;
	};
};

...

Does this make sense?

Quoted 3 lines

then I'll send a change for inclusion of EMC description -- the one above excluding clocks and clock-names properties, work on CCF is in progress.

Ah, very nice! That should get us very close to multiplatform support! I've just tried building lpc32xx without the headers to check for other issues, and ended up with the patch below to get it building. It's clearly wrong, but it highlights a number of issues.

Quoted 22 lines · Arnd Bergmann

Quoted 5 lines · On Wednesday 14 October 2015 02:13:49 Vladimir Zapolskiy wrote:

Ok, practically it should work for my purposes, but the change must be done along with added EMC device node description.

I'm not so confident that it is correct to add description of static memory banks to ahb node though, please give me a short confirmation,

The DT should reflect whichever memory ranges are accessible on the bus. I've looked up the memory map in the data sheet, and I think a reasonable representation would be

/ahb {
      ranges = <0x20000000 0x20000000 0x10000000>, /* AHB port 5 */
               <0x30000000 0x30000000 0x10000000>, /* AHB port 6 */
               <0x40000000 0x40000000 0x10000000>, /* AHB port 7 */
               <0x80000000 0x80000000 0x40000000>, /* EMC DYCS0/1 */
               <0xE0000000 0xE0000000 0x04000000>; /* EMC CS0-3 */

      apb {
              ranges = <0x20080000 0x20080000 0x00020000>;
      };

Quoted 1 line

};

A simpler version of this change in /ahb I successfully tested with EMC yesterday:

Quoted 8 lines

Quoted 3 lines

then I'll send a change for inclusion of EMC description -- the one above excluding clocks and clock-names properties, work on CCF is in progress.

Ah, very nice! That should get us very close to multiplatform support! I've just tried building lpc32xx without the headers to check for other issues, and ended up with the patch below to get it building. It's clearly wrong, but it highlights a number of issues.

Sure, I'll review the highlighted problems, thanks.

Quoted 5 lines · On Wednesday 14 October 2015 02:13:49 Vladimir Zapolskiy wrote:

Ok, practically it should work for my purposes, but the change must be done along with added EMC device node description.

I'm not so confident that it is correct to add description of static memory banks to ahb node though, please give me a short confirmation,

The DT should reflect whichever memory ranges are accessible on the bus. I've looked up the memory map in the data sheet, and I think a reasonable representation would be

/ahb {
        ranges = <0x20000000 0x20000000 0x10000000>, /* AHB port 5 */
                 <0x30000000 0x30000000 0x10000000>, /* AHB port 6 */
                 <0x40000000 0x40000000 0x10000000>, /* AHB port 7 */
                 <0x80000000 0x80000000 0x40000000>, /* EMC DYCS0/1 */
                 <0xE0000000 0xE0000000 0x04000000>; /* EMC CS0-3 */

        apb {
                ranges = <0x20080000 0x20080000 0x00020000>;
        };

};

alternatively, each AHB port could be a separate node, with a more direct translation like

/ahb5 {
        ranges = <0 0x20000000 0x10000000>;

        apb {
                ranges = <0 0x80000 0x20000>;
        };
};
/ahb6 {
        ranges = <0 0x30000000 0x10000000>,           /* AHB registers */
                 <0x80000000 0x80000000 0x40000000>, /* EMC DYCS0/1 */
                 <0xE0000000 0xE0000000 0x04000000>; /* EMC CS0-3 */

        memory-controller at 1080000 {
                reg = <0x1080000 0x10000>;
        };
};

Quoted 17 lines

/ahb5 {
        ranges = <0 0x20000000 0x10000000>;

        apb {
                ranges = <0 0x80000 0x20000>;
        };
};
/ahb6 {
        ranges = <0 0x30000000 0x10000000>,           /* AHB registers */
                 <0x80000000 0x80000000 0x40000000>, /* EMC DYCS0/1 */
                 <0xE0000000 0xE0000000 0x04000000>; /* EMC CS0-3 */

        memory-controller at 1080000 {
                reg = <0x1080000 0x10000>;
        };
};

Sorry for hijacking the thread, but I have related question to this.

What is the advantage of using a hierarchical bus structure in dt? I thought the recommendation, at least for new device trees, was to keep it flat under a "soc"-node.

If doesn't offer any advantages why not remove instead of adding the ranges property which seems to grow a bit complex now?
Of course removing it would create a lot of churn because of the re-indentation but at least the end result would be simpler.

Quoted 20 lines · On Wednesday 14 October 2015 19:23:09 Joachim Eastwood wrote:

Quoted 17 lines

/ahb5 {
        ranges = <0 0x20000000 0x10000000>;

        apb {
                ranges = <0 0x80000 0x20000>;
        };
};
/ahb6 {
        ranges = <0 0x30000000 0x10000000>,           /* AHB registers */
                 <0x80000000 0x80000000 0x40000000>, /* EMC DYCS0/1 */
                 <0xE0000000 0xE0000000 0x04000000>; /* EMC CS0-3 */

        memory-controller at 1080000 {
                reg = <0x1080000 0x10000>;
        };
};

Sorry for hijacking the thread, but I have related question to this.

Quoted 8 lines

What is the advantage of using a hierarchical bus structure in dt? I thought the recommendation, at least for new device trees, was to keep it flat under a "soc"-node.

If doesn't offer any advantages why not remove instead of adding the ranges property which seems to grow a bit complex now?
Of course removing it would create a lot of churn because of the re-indentation but at least the end result would be simpler.

The general recommendation is to have the DT structure resemble the hardware as closely as possible. If the chip has a clear hierarchy and you know it, then it's best to describe it that way.

The reason for having a single 'soc' node in a lot of the modern dtsi files is that either it's not documented, or they use AXI 'buses' that are not really hierarchical but are point-to-point connections.

There is no addressable devices under cpus device node, remove noisy address and size cells properties.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
 arch/arm/boot/dts/lpc32xx.dtsi | 3 ---
 1 file changed, 3 deletions(-)

arch/arm/boot/dts/lpc32xx.dtsi +0 -0

index dcb52cb..929458d 100644

Hi Vladimir,

Quoted 13 lines · Vladimir Zapolskiy

There is no addressable devices under cpus device node, remove noisy address and size cells properties.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
 arch/arm/boot/dts/lpc32xx.dtsi | 3 ---
 1 file changed, 3 deletions(-)

arch/arm/boot/dts/lpc32xx.dtsi +0 -0

index dcb52cb..929458d 100644

arch/arm/boot/dts/lpc32xx.dtsi line 23

Quoted 6 lines

arch/arm/boot/dts/lpc32xx.dtsi +0 -3

@@ -18,9 +18,6 @@
        interrupt-parent = <&mic>;
 
        cpus {
-               #address-cells = <0>;
-               #size-cells = <0>;
-

According to Documentation/devicetree/bindings/arm/cpus.txt these properties are required.

Take a look at Example 3 in the doc for it should look like on a ARM 926EJ-S uniprocessor 32-bit system.

LPC32xx SoCs have two independent PWM controllers, they have different clock parents, clock gates and even slightly different controls, each of these two PWM controllers has one output channel. Due to almost similar controls arranged in a row it is incorrectly assumed that there is one PWM controller with two channels, fix this problem in lpc32xx.dtsi, which at the moment prevents separate configuration of different clock parents and gates for both PWM controllers.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
 arch/arm/boot/dts/lpc32xx.dtsi | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

arch/arm/boot/dts/lpc32xx.dtsi +1 -2

index 929458d..f4d2a0e 100644
@@ -286,9 +286,15 @@
                                status = "disabled";
                        };
 
-                       pwm: pwm at 4005C000 {
+                       pwm1: pwm at 4005C000 {
                                compatible = "nxp,lpc3220-pwm";
-                               reg = <0x4005C000 0x8>;

Hi Vladimir,

Quoted 25 lines · Vladimir Zapolskiy

LPC32xx SoCs have two independent PWM controllers, they have different clock parents, clock gates and even slightly different controls, each of these two PWM controllers has one output channel. Due to almost similar controls arranged in a row it is incorrectly assumed that there is one PWM controller with two channels, fix this problem in lpc32xx.dtsi, which at the moment prevents separate configuration of different clock parents and gates for both PWM controllers.

Signed-off-by Vladimir Zapolskiy <[email protected]>

---
 arch/arm/boot/dts/lpc32xx.dtsi | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

arch/arm/boot/dts/lpc32xx.dtsi +1 -2

index 929458d..f4d2a0e 100644
@@ -286,9 +286,15 @@
                                status = "disabled";
                        };
 
-                       pwm: pwm at 4005C000 {
+                       pwm1: pwm at 4005C000 {
                                compatible = "nxp,lpc3220-pwm";
-                               reg = <0x4005C000 0x8>;

arch/arm/boot/dts/lpc32xx.dtsi line 300

Quoted 10 lines

arch/arm/boot/dts/lpc32xx.dtsi +7 -0

+                               reg = <0x4005C000 0x4>;
+                               status = "disabled";
+                       };
+
+                       pwm2: pwm at 4005C004 {
+                               compatible = "nxp,lpc3220-pwm";
+                               reg = <0x4005C004 0x4>;
                                status = "disabled";
                        };
                };

I am not really against your change, but...

What's wrong with a binding like the one below?

pwm: pwm at 0x4005c000 {
    compatible = "nxp,lpc3220-pwm";
    reg = <0x4005C000 0x8>;
    clocks =<&clk CLK_PWM1, &clk CLK_PWM2>;
    clock-names = "pwm1", "pwm2";
    #pwm-cells = <3>;
};

With two clocks and where the first pwm-cell would select either PWM1 or PWM2.

Seems like the driver only handle one clock, but should be fairly easy to fix.

Note: with your DT change you would also need to change the driver since it currently sets npwm to 2.