Skip to contentSkip to conversation

Add support for Marvell's RD-DXBC2

PATCH New Chris Packham 6 participants 20 messages Dec 10, 2014 – Jan 30, 2015

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

cover Add support for Marvell's RD-DXBC2

This series allows me to boot v3.18 on Marvell's reference board and start up a basic userspace. I've sent 1/4 and 2/4 to the list individually before but hopefully they will be more meaningful when viewed in context.

Thanks,
Chris

Chris Packham (4):
ARM: mvebu: use dt_fixup to provide fallback for enable-method
clk: mvebu: armada-xp: Support for 98DX4251
ARM: mvebu: Initial support for rd-dxbc2
ARM: mvebu: Custom smp_ops for 98DX4251

 .../devicetree/bindings/clock/mvebu-core-clock.txt |   4 +
 arch/arm/boot/dts/rd-dxbc2.dts                     | 118 +++++++++++++++++++++
 arch/arm/mach-mvebu/Makefile                       |   1 +
 arch/arm/mach-mvebu/board-v7.c                     |   7 +-
 arch/arm/mach-mvebu/common.h                       |   2 +
 arch/arm/mach-mvebu/mvebu-soc-id.h                 |   3 +
 arch/arm/mach-mvebu/platsmp.c                      |  28 +++++
 arch/arm/mach-mvebu/pmsu-98dx4251.c                |  68 ++++++++++++
 drivers/clk/mvebu/armada-xp.c                      |  35 ++++++
 9 files changed, 265 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm/boot/dts/rd-dxbc2.dts
 create mode 100644 arch/arm/mach-mvebu/pmsu-98dx4251.c
Signature · 1 line
2.2.0.rc0
#1 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

We need to maintain backwards compatibility with device trees that don't define an enable method. At the same time we want the device tree to be able to specify an enable-method and have it stick.

Previously by having smp assigned in the DT_MACHINE definition this would be picked up by setup_arch() and override whatever arm_dt_init_cpu_maps() had configured. Now we move the initial assignment of default smp_ops to a dt_fixup and let arm_dt_init_cpu_maps() override that if the device tree defines an enable-method.

---

This didn't get much attention last time and is unchanged from v4. I'm pretty happy with this incarnation. It doesn't touch core code. It provides a fallback for old device trees and it achieves my original goal of allowing the device tree to configure the smp_ops via the enable-method property.

v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/300182.html
v2: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/300480.html
v3: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/302945.html
v4: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/303899.html

 arch/arm/mach-mvebu/board-v7.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
arch/arm/mach-mvebu/board-v7.c +6 -1
index d0d39f1..ce23c6a 100644
@@ -198,6 +198,11 @@ static void __init mvebu_dt_init(void)
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
+static void __init armada_370_xp_dt_fixup(void)
+{
+	smp_set_ops(smp_ops(armada_xp_smp_ops));
+}
+
 static const char * const armada_370_xp_dt_compat[] = {
 	"marvell,armada-370-xp",
 	NULL,
@@ -206,11 +211,11 @@ static const char * const armada_370_xp_dt_compat[] = {
 DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
-	.smp		= smp_ops(armada_xp_smp_ops),
 	.init_machine	= mvebu_dt_init,
 	.init_irq       = mvebu_init_irq,
 	.restart	= mvebu_restart,
 	.dt_compat	= armada_370_xp_dt_compat,
+	.dt_fixup	= armada_370_xp_dt_fixup,
 MACHINE_END
 
 static const char * const armada_375_dt_compat[] = {
Signature · 1 line
2.2.0.rc0
#2 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

arch/arm/mach-mvebu/board-v7.c static void __init mvebu_dt_init(void) · line 205

Quoted 7 lines · On Wednesday 10 December 2014 15:39:44 Chris Packham wrote:
arch/arm/mach-mvebu/board-v7.c +5 -0
+static void __init armada_370_xp_dt_fixup(void)
+{
+       smp_set_ops(smp_ops(armada_xp_smp_ops));
+}
+

The dt_fixup callback pointer is meant to fix up a legacy dtb file in memory. I think this would be fairly easy in this case, just add in the missing enable-method property here to make the normal boot path work for old dtbs.

Arnd

#3 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

Hi Arnd,

Quoted 14 lines · Arnd Bergmann

arch/arm/mach-mvebu/board-v7.c static void __init mvebu_dt_init(void) · line 205

Quoted 7 lines · On Wednesday 10 December 2014 15:39:44 Chris Packham wrote:
arch/arm/mach-mvebu/board-v7.c +5 -0
+static void __init armada_370_xp_dt_fixup(void)
+{
+       smp_set_ops(smp_ops(armada_xp_smp_ops));
+}
+

The dt_fixup callback pointer is meant to fix up a legacy dtb file in memory. I think this would be fairly easy in this case, just add in the missing enable-method property here to make the normal boot path work for old dtbs.

Quoted 2 lines

Arnd

I briefly explored that approach here[1]. The tricky part would be handling the fact that the enable method can be attached to either the /cpus node or and individual /cpu entry (or is that something I can ignore?).

In the end I thought that the unconditional setting of smp_ops was easier to implement and would achieve the same result.

--
[1] -
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/303465.html

#4 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

Hi Arnd,

Quoted 18 lines · Arnd Bergmann
Quoted 14 lines · On Wednesday 10 December 2014 15:39:44 Chris Packham wrote:

Review

Quoted 7 lines
(quoted diff) +5 -0
+static void __init armada_370_xp_dt_fixup(void)
+{
+       smp_set_ops(smp_ops(armada_xp_smp_ops));
+}
+

The dt_fixup callback pointer is meant to fix up a legacy dtb file in memory. I think this would be fairly easy in this case, just add in the missing enable-method property here to make the normal boot path work for old dtbs.

Quoted 2 lines

Arnd

Quoted 7 lines

I briefly explored that approach here[1]. The tricky part would be handling the fact that the enable method can be attached to either the /cpus node or and individual /cpu entry (or is that something I can ignore?).

In the end I thought that the unconditional setting of smp_ops was easier to implement and would achieve the same result.

Actually as it turns out it's not that hard to implement something that checks both /cpus and /cpus/cpu at n. I'll include this with the next round after I've waited for anymore feedback.

---8<---
Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for enable-method

When the device tree doesn't define an enable-method insert a property into the flattened device tree. arm_dt_init_cpu_maps() will then parse this an set smp_ops appropriately. Now that we have this fallback it is no longer necessary to set .smp in the DT_MACHINE definition.

---
 arch/arm/mach-mvebu/Makefile   |  2 ++
 arch/arm/mach-mvebu/board-v7.c | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)
arch/arm/mach-mvebu/Makefile +2 -0
index e24136b..68310f8 100644
@@ -14,3 +14,5 @@ endif
 obj-$(CONFIG_MACH_DOVE)		 += dove.o
 obj-$(CONFIG_MACH_KIRKWOOD)	 += kirkwood.o kirkwood-pm.o
 obj-$(CONFIG_MACH_NETXBIG)	 += netxbig.o
+
+CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
\ No newline at end of file
arch/arm/mach-mvebu/board-v7.c +36 -1
index d0d39f1..0592c76 100644
@@ -17,6 +17,8 @@
 #include <linux/clk-provider.h>
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
+#include <linux/of_fdt.h>
+#include <linux/libfdt.h>
 #include <linux/io.h>
 #include <linux/clocksource.h>
 #include <linux/dma-mapping.h>
@@ -198,6 +200,39 @@ static void __init mvebu_dt_init(void)
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 }
 
+static void __init armada_370_xp_dt_fixup(void)
+{
+	int offset, node;
+	int i, len;
+	void *prop;
+	char buffer[20];
+
+	offset = fdt_path_offset(initial_boot_params, "/cpus");
+	if (offset < 0)
+		return;
+
+	prop = fdt_getprop(initial_boot_params, offset, "enable-method", &len);
+	if (prop)
+		return;
+
+	for (i = 0; i < NR_CPUS; i++) {
+		snprintf(buffer, sizeof(buffer), "cpu@%d", i);
+		node = fdt_subnode_offset(initial_boot_params, offset, buffer);
+		if (node < 0)
+			break;
+		prop = fdt_getprop(initial_boot_params, node,
+				   "enable-method", &len);
+		if (prop)
+			return;
+	}
+
+	pr_info("No enable-method defined. "
+		"Falling back to \"marvell,armada-xp-smp\"\n");
+
+	fdt_setprop(initial_boot_params, offset, "enable-method",
+		    "marvell,armada-xp-smp", sizeof("marvell,armada-xp-smp"));
+}
+
 static const char * const armada_370_xp_dt_compat[] = {
 	"marvell,armada-370-xp",
 	NULL,
@@ -206,11 +241,11 @@ static const char * const armada_370_xp_dt_compat[] = {
 DT_MACHINE_START(ARMADA_370_XP_DT, "Marvell Armada 370/XP (Device Tree)")
 	.l2c_aux_val	= 0,
 	.l2c_aux_mask	= ~0,
-	.smp		= smp_ops(armada_xp_smp_ops),
 	.init_machine	= mvebu_dt_init,
 	.init_irq       = mvebu_init_irq,
 	.restart	= mvebu_restart,
 	.dt_compat	= armada_370_xp_dt_compat,
+	.dt_fixup	= armada_370_xp_dt_fixup,
 MACHINE_END
 
 static const char * const armada_375_dt_compat[] = {
Signature · 1 line
2.2.0.rc0
#5 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

Quoted 11 lines · On Friday 12 December 2014 17:53:55 Chris Packham wrote:
Quoted 7 lines

I briefly explored that approach here[1]. The tricky part would be handling the fact that the enable method can be attached to either the /cpus node or and individual /cpu entry (or is that something I can ignore?).

In the end I thought that the unconditional setting of smp_ops was easier to implement and would achieve the same result.

Actually as it turns out it's not that hard to implement something that checks both /cpus and /cpus/cpu at n. I'll include this with the next round after I've waited for anymore feedback.

Ah, very nice!

Quoted 19 lines

---8<---
Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for enable-method

When the device tree doesn't define an enable-method insert a property into the flattened device tree. arm_dt_init_cpu_maps() will then parse this an set smp_ops appropriately. Now that we have this fallback it is no longer necessary to set .smp in the DT_MACHINE definition.

---
 arch/arm/mach-mvebu/Makefile   |  2 ++
 arch/arm/mach-mvebu/board-v7.c | 37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 38 insertions(+), 1 deletion(-)
arch/arm/mach-mvebu/Makefile +0 -0
index e24136b..68310f8 100644

arch/arm/mach-mvebu/Makefile endif · line 18

Quoted 6 lines
arch/arm/mach-mvebu/Makefile +2 -0
@@ -14,3 +14,5 @@ endif
 obj-$(CONFIG_MACH_DOVE)		 += dove.o
 obj-$(CONFIG_MACH_KIRKWOOD)	 += kirkwood.o kirkwood-pm.o
 obj-$(CONFIG_MACH_NETXBIG)	 += netxbig.o
+
+CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
\ No newline at end of file

Why is this needed? Can't you just include <linux/libfdt.h> ?

Quoted 22 lines
arch/arm/mach-mvebu/Makefile +22 -0
+static void __init armada_370_xp_dt_fixup(void)
+{
+	int offset, node;
+	int i, len;
+	void *prop;
+	char buffer[20];
+
+	offset = fdt_path_offset(initial_boot_params, "/cpus");
+	if (offset < 0)
+		return;
+
+	prop = fdt_getprop(initial_boot_params, offset, "enable-method", &len);
+	if (prop)
+		return;
+
+	for (i = 0; i < NR_CPUS; i++) {
+		snprintf(buffer, sizeof(buffer), "cpu@%d", i);
+		node = fdt_subnode_offset(initial_boot_params, offset, buffer);
+		if (node < 0)
+			break;
+		prop = fdt_getprop(initial_boot_params, node,
+				   "enable-method", &len);

arch/arm/mach-mvebu/Makefile

Quoted 10 lines
arch/arm/mach-mvebu/Makefile +10 -0
+		if (prop)
+			return;
+	}
+
+	pr_info("No enable-method defined. "
+		"Falling back to \"marvell,armada-xp-smp\"\n");
+
+	fdt_setprop(initial_boot_params, offset, "enable-method",
+		    "marvell,armada-xp-smp", sizeof("marvell,armada-xp-smp"));
+}

I think it would be good to first check whether you are running on Armada XP or Armada 370, because the latter does not require this.

Arnd

#6 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

Hi Arnd,

Quoted 29 lines · Arnd Bergmann
Quoted 11 lines · On Friday 12 December 2014 17:53:55 Chris Packham wrote:
Quoted 7 lines

I briefly explored that approach here[1]. The tricky part would be handling the fact that the enable method can be attached to either the /cpus node or and individual /cpu entry (or is that something I can ignore?).

In the end I thought that the unconditional setting of smp_ops was easier to implement and would achieve the same result.

Actually as it turns out it's not that hard to implement something that checks both /cpus and /cpus/cpu at n. I'll include this with the next round after I've waited for anymore feedback.

Ah, very nice!

Quoted 14 lines

---8<---
Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for enable-method

When the device tree doesn't define an enable-method insert a property into the flattened device tree. arm_dt_init_cpu_maps() will then parse this an set smp_ops appropriately. Now that we have this fallback it is no longer necessary to set .smp in the DT_MACHINE definition.

---
  arch/arm/mach-mvebu/Makefile   |  2 ++
  arch/arm/mach-mvebu/board-v7.c | 37 ++++++++++++++++++++++++++++++++++++-
  2 files changed, 38 insertions(+), 1 deletion(-)
Quoted 14 lines

arch/arm/mach-mvebu/Makefile endif · line 18

Quoted 11 lines
arch/arm/mach-mvebu/Makefile +2 -0
index e24136b..68310f8 100644
@@ -14,3 +14,5 @@ endif
  obj-$(CONFIG_MACH_DOVE)		 += dove.o
  obj-$(CONFIG_MACH_KIRKWOOD)	 += kirkwood.o kirkwood-pm.o
  obj-$(CONFIG_MACH_NETXBIG)	 += netxbig.o
+
+CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
\ No newline at end of file

Why is this needed? Can't you just include <linux/libfdt.h> ?

I am already including linux/libfdt.h. Without the CFLAGS change I get the following compile error.

In file included from include/linux/libfdt.h:6:0,

                    from arch/arm/mach-mvebu/board-v7.c:21:

include/linux/../../scripts/dtc/libfdt/libfdt.h:54:24: fatal error:
libfdt_env.h: No such file or directory

There seems to be precedence in other architectures/drivers

   $ git grep -e '-I.*libfdt'
   arch/mips/cavium-octeon/Makefile:CFLAGS_octeon-platform.o = 
-I$(src)/../../../scripts/dtc/libfdt
   arch/mips/cavium-octeon/Makefile:CFLAGS_setup.o = 
-I$(src)/../../../scripts/dtc/libfdt
   arch/mips/mti-sead3/Makefile:CFLAGS_sead3-setup.o = 
-I$(src)/../../../scripts/dtc/libfdt
   arch/powerpc/kernel/Makefile:CFLAGS_prom.o              = 
-I$(src)/../../../scripts/dtc/libfdt
   drivers/firmware/efi/libstub/Makefile:CFLAGS_fdt.o 
    += -I$(srctree)/scripts/dtc/libfdt/
   drivers/of/Makefile:CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
   drivers/of/Makefile:CFLAGS_fdt_address.o = 
-I$(src)/../../scripts/dtc/libfdt
Quoted 36 lines
Quoted 22 lines
arch/arm/mach-mvebu/Makefile +22 -0
+static void __init armada_370_xp_dt_fixup(void)
+{
+	int offset, node;
+	int i, len;
+	void *prop;
+	char buffer[20];
+
+	offset = fdt_path_offset(initial_boot_params, "/cpus");
+	if (offset < 0)
+		return;
+
+	prop = fdt_getprop(initial_boot_params, offset, "enable-method", &len);
+	if (prop)
+		return;
+
+	for (i = 0; i < NR_CPUS; i++) {
+		snprintf(buffer, sizeof(buffer), "cpu@%d", i);
+		node = fdt_subnode_offset(initial_boot_params, offset, buffer);
+		if (node < 0)
+			break;
+		prop = fdt_getprop(initial_boot_params, node,
+				   "enable-method", &len);

arch/arm/mach-mvebu/Makefile

Quoted 10 lines
arch/arm/mach-mvebu/Makefile +10 -0
+		if (prop)
+			return;
+	}
+
+	pr_info("No enable-method defined. "
+		"Falling back to \"marvell,armada-xp-smp\"\n");
+
+	fdt_setprop(initial_boot_params, offset, "enable-method",
+		    "marvell,armada-xp-smp", sizeof("marvell,armada-xp-smp"));
+}

I think it would be good to first check whether you are running on Armada XP or Armada 370, because the latter does not require this.

Quoted 2 lines

Arnd

Any suggestion as to what to check for. Based on the dts files in the current source seem compatible == "marvell,armada370" seems like a good choice. But I don't know for sure that there isn't a armada-xp variant out using a .dts with this set.

Another option might be just to count the /cpu at n nodes.

#7 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

Answering my own question

Quoted 112 lines · Chris Packham

Hi Arnd,

Quoted 31 lines · Arnd Bergmann
Quoted 11 lines · On Friday 12 December 2014 17:53:55 Chris Packham wrote:
Quoted 7 lines

I briefly explored that approach here[1]. The tricky part would be handling the fact that the enable method can be attached to either the /cpus node or and individual /cpu entry (or is that something I can ignore?).

In the end I thought that the unconditional setting of smp_ops was easier to implement and would achieve the same result.

Actually as it turns out it's not that hard to implement something that checks both /cpus and /cpus/cpu at n. I'll include this with the next round after I've waited for anymore feedback.

Ah, very nice!

Quoted 16 lines

---8<---
Subject: [PATCH] ARM: mvebu: use dt_fixup to provide fallback for enable-method

When the device tree doesn't define an enable-method insert a property into the flattened device tree. arm_dt_init_cpu_maps() will then parse this an set smp_ops appropriately. Now that we have this fallback it is no longer necessary to set .smp in the DT_MACHINE definition.

---
  arch/arm/mach-mvebu/Makefile   |  2 ++
  arch/arm/mach-mvebu/board-v7.c | 37
++++++++++++++++++++++++++++++++++++-
  2 files changed, 38 insertions(+), 1 deletion(-)
Quoted 14 lines

arch/arm/mach-mvebu/Makefile endif · line 18

Quoted 11 lines
arch/arm/mach-mvebu/Makefile +2 -0
index e24136b..68310f8 100644
@@ -14,3 +14,5 @@ endif
  obj-$(CONFIG_MACH_DOVE)         += dove.o
  obj-$(CONFIG_MACH_KIRKWOOD)     += kirkwood.o kirkwood-pm.o
  obj-$(CONFIG_MACH_NETXBIG)     += netxbig.o
+
+CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
\ No newline at end of file

Why is this needed? Can't you just include <linux/libfdt.h> ?

I am already including linux/libfdt.h. Without the CFLAGS change I get the following compile error.

   In file included from include/linux/libfdt.h:6:0,
                    from arch/arm/mach-mvebu/board-v7.c:21:
   include/linux/../../scripts/dtc/libfdt/libfdt.h:54:24: fatal error:

libfdt_env.h: No such file or directory

There seems to be precedence in other architectures/drivers

   $ git grep -e '-I.*libfdt'
   arch/mips/cavium-octeon/Makefile:CFLAGS_octeon-platform.o =
(quoted diff) +0 -6
-I$(src)/../../../scripts/dtc/libfdt
   arch/mips/cavium-octeon/Makefile:CFLAGS_setup.o =
-I$(src)/../../../scripts/dtc/libfdt
   arch/mips/mti-sead3/Makefile:CFLAGS_sead3-setup.o =
-I$(src)/../../../scripts/dtc/libfdt
   arch/powerpc/kernel/Makefile:CFLAGS_prom.o              =
-I$(src)/../../../scripts/dtc/libfdt
   drivers/firmware/efi/libstub/Makefile:CFLAGS_fdt.o    +=
-I$(srctree)/scripts/dtc/libfdt/
   drivers/of/Makefile:CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
   drivers/of/Makefile:CFLAGS_fdt_address.o =
-I$(src)/../../scripts/dtc/libfdt
Quoted 39 lines
Quoted 24 lines
arch/arm/mach-mvebu/Makefile +12 -0
+static void __init armada_370_xp_dt_fixup(void)
+{
+    int offset, node;
+    int i, len;
+    void *prop;
+    char buffer[20];
+
+    offset = fdt_path_offset(initial_boot_params, "/cpus");
+    if (offset < 0)
+        return;
+
+    prop = fdt_getprop(initial_boot_params, offset, "enable-method",

&len);

arch/arm/mach-mvebu/Makefile +10 -0
+    if (prop)
+        return;
+
+    for (i = 0; i < NR_CPUS; i++) {
+        snprintf(buffer, sizeof(buffer), "cpu@%d", i);
+        node = fdt_subnode_offset(initial_boot_params, offset, buffer);
+        if (node < 0)
+            break;
+        prop = fdt_getprop(initial_boot_params, node,
+                   "enable-method", &len);

arch/arm/mach-mvebu/Makefile

Quoted 10 lines
arch/arm/mach-mvebu/Makefile +10 -0
+        if (prop)
+            return;
+    }
+
+    pr_info("No enable-method defined. "
+        "Falling back to \"marvell,armada-xp-smp\"\n");
+
+    fdt_setprop(initial_boot_params, offset, "enable-method",
+            "marvell,armada-xp-smp", sizeof("marvell,armada-xp-smp"));
+}

I think it would be good to first check whether you are running on Armada XP
or Armada 370, because the latter does not require this.

Quoted 7 lines
Quoted 2 lines

Arnd

Any suggestion as to what to check for. Based on the dts files in the current source seem compatible == "marvell,armada370" seems like a good choice. But I don't know for sure that there isn't a armada-xp variant out using a .dts with this set.

Adding the following at the top will do the trick (assuming "marvell,armada370" is the right compatible string to use).

   	unsigned long root = of_get_flat_dt_root();

if (of_flat_dt_is_compatible(root, "marvell,armada370"))
return;

Quoted 2 lines

Another option might be just to count the /cpu at n nodes.

#8 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

Quoted 18 lines · On Monday 15 December 2014 00:57:49 Chris Packham wrote:
Quoted 17 lines · Chris Packham
Quoted 6 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+
+CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
\ No newline at end of file

Why is this needed? Can't you just include <linux/libfdt.h> ?

I am already including linux/libfdt.h. Without the CFLAGS change I get the following compile error.

   In file included from include/linux/libfdt.h:6:0,
                    from arch/arm/mach-mvebu/board-v7.c:21:
   include/linux/../../scripts/dtc/libfdt/libfdt.h:54:24: fatal error:

libfdt_env.h: No such file or directory

There seems to be precedence in other architectures/drivers

Quoted 15 lines
Quoted 5 lines
   $ git grep -e '-I.*libfdt'
   arch/mips/cavium-octeon/Makefile:CFLAGS_octeon-platform.o =
(quoted diff) +0 -1
-I$(src)/../../../scripts/dtc/libfdt
   arch/mips/cavium-octeon/Makefile:CFLAGS_setup.o =

Review

Quoted 10 lines
(quoted diff) +0 -5
-I$(src)/../../../scripts/dtc/libfdt
   arch/mips/mti-sead3/Makefile:CFLAGS_sead3-setup.o =
-I$(src)/../../../scripts/dtc/libfdt
   arch/powerpc/kernel/Makefile:CFLAGS_prom.o              =
-I$(src)/../../../scripts/dtc/libfdt
   drivers/firmware/efi/libstub/Makefile:CFLAGS_fdt.o    +=
-I$(srctree)/scripts/dtc/libfdt/
   drivers/of/Makefile:CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
   drivers/of/Makefile:CFLAGS_fdt_address.o =
-I$(src)/../../scripts/dtc/libfdt

I still think we should not duplicate this but instead change the header files to make the include hack unnecessary in all those places.

Looking at the header file, it only contains

#include <linux/libfdt_env.h>
#include "../../scripts/dtc/libfdt/fdt.h"
#include "../../scripts/dtc/libfdt/libfdt.h"

so two of them get the headers from exactly that place, while the libfdt_env.h file contains the linux-specific implementation of a couple of functions instead of the baremetal implementation.

It looks like the intention was that when you already include the linux version of the header, you no longer need the other one, but that assumption is broken by current implementation.

One way to solve this would be

scripts/dtc/libfdt/libfdt.h +1 -1
index 73f49759a5e7..23c55baa8e18 100644
@@ -51,7 +51,7 @@
  *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <libfdt_env.h>
+#include "libfdt_env.h"
 #include <fdt.h>
 
 #define FDT_FIRST_SUPPORTED_VERSION	0x10

but I'm not sure if that is the best way.

Quoted 12 lines
Quoted 11 lines
Quoted 6 lines

I think it would be good to first check whether you are running on Armada XP
or Armada 370, because the latter does not require this.

Arnd

Any suggestion as to what to check for. Based on the dts files in the current source seem compatible == "marvell,armada370" seems like a good choice. But I don't know for sure that there isn't a armada-xp variant out using a .dts with this set.

Quoted 7 lines

Adding the following at the top will do the trick (assuming "marvell,armada370" is the right compatible string to use).

        unsigned long root = of_get_flat_dt_root();

        if (of_flat_dt_is_compatible(root, "marvell,armada370"))
                return;

Right. You could also add

	if (!IS_ENABLED(CONFIG_SMP) || !of_flat_dt_is_compatible(root, "marvell,armadaxp")))

to again eliminate that check for uniprocessor kernels, and to not propagate the fixup to future machines but make it clear that it is only needed for armadaxp.

Arnd

#9 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

Quoted 69 lines · Arnd Bergmann
Quoted 18 lines · On Monday 15 December 2014 00:57:49 Chris Packham wrote:
Quoted 17 lines · Chris Packham
Quoted 6 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+
+CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
\ No newline at end of file

Why is this needed? Can't you just include <linux/libfdt.h> ?

I am already including linux/libfdt.h. Without the CFLAGS change I get the following compile error.

    In file included from include/linux/libfdt.h:6:0,
                     from arch/arm/mach-mvebu/board-v7.c:21:
    include/linux/../../scripts/dtc/libfdt/libfdt.h:54:24: fatal error:

libfdt_env.h: No such file or directory

There seems to be precedence in other architectures/drivers

Quoted 15 lines
Quoted 5 lines
    $ git grep -e '-I.*libfdt'
    arch/mips/cavium-octeon/Makefile:CFLAGS_octeon-platform.o =
(quoted diff) +0 -1
-I$(src)/../../../scripts/dtc/libfdt
    arch/mips/cavium-octeon/Makefile:CFLAGS_setup.o =

Review

Quoted 10 lines
(quoted diff) +0 -5
-I$(src)/../../../scripts/dtc/libfdt
    arch/mips/mti-sead3/Makefile:CFLAGS_sead3-setup.o =
-I$(src)/../../../scripts/dtc/libfdt
    arch/powerpc/kernel/Makefile:CFLAGS_prom.o              =
-I$(src)/../../../scripts/dtc/libfdt
    drivers/firmware/efi/libstub/Makefile:CFLAGS_fdt.o    +=
-I$(srctree)/scripts/dtc/libfdt/
    drivers/of/Makefile:CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
    drivers/of/Makefile:CFLAGS_fdt_address.o =
-I$(src)/../../scripts/dtc/libfdt

I still think we should not duplicate this but instead change the header files to make the include hack unnecessary in all those places.

Looking at the header file, it only contains

#include <linux/libfdt_env.h>
#include "../../scripts/dtc/libfdt/fdt.h"
#include "../../scripts/dtc/libfdt/libfdt.h"

so two of them get the headers from exactly that place, while the libfdt_env.h file contains the linux-specific implementation of a couple of functions instead of the baremetal implementation.

It looks like the intention was that when you already include the linux version of the header, you no longer need the other one, but that assumption is broken by current implementation.

One way to solve this would be

scripts/dtc/libfdt/libfdt.h +1 -1
index 73f49759a5e7..23c55baa8e18 100644
@@ -51,7 +51,7 @@
   *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
   */
 
-#include <libfdt_env.h>
+#include "libfdt_env.h"
  #include <fdt.h>
 
  #define FDT_FIRST_SUPPORTED_VERSION	0x10
Quoted 2 lines

but I'm not sure if that is the best way.

I was hoping to limit the scope of my changes to mvebu code. But I can give it a whirl. I agree that propagating the hack isn't ideal.

Quoted 20 lines
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines

I think it would be good to first check whether you are running on Armada XP
or Armada 370, because the latter does not require this.

Arnd

Any suggestion as to what to check for. Based on the dts files in the current source seem compatible == "marvell,armada370" seems like a good choice. But I don't know for sure that there isn't a armada-xp variant out using a .dts with this set.

Quoted 7 lines

Adding the following at the top will do the trick (assuming "marvell,armada370" is the right compatible string to use).

         unsigned long root = of_get_flat_dt_root();

         if (of_flat_dt_is_compatible(root, "marvell,armada370"))
                 return;
Quoted 7 lines

Right. You could also add

	if (!IS_ENABLED(CONFIG_SMP) || !of_flat_dt_is_compatible(root, "marvell,armadaxp")))

to again eliminate that check for uniprocessor kernels, and to not propagate the fixup to future machines but make it clear that it is only needed for armadaxp.

OK makes sense.

Quoted 3 lines

Arnd

#10 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

Quoted 70 lines · Chris Packham
Quoted 67 lines · Arnd Bergmann
Quoted 18 lines · On Monday 15 December 2014 00:57:49 Chris Packham wrote:
Quoted 17 lines · Chris Packham
Quoted 6 lines

Review

Quoted 3 lines
(quoted diff) +2 -0
+
+CFLAGS_board-v7.o = -I$(src)/../../../scripts/dtc/libfdt
\ No newline at end of file

Why is this needed? Can't you just include <linux/libfdt.h> ?

I am already including linux/libfdt.h. Without the CFLAGS change I get the following compile error.

    In file included from include/linux/libfdt.h:6:0,
                     from arch/arm/mach-mvebu/board-v7.c:21:
    include/linux/../../scripts/dtc/libfdt/libfdt.h:54:24: fatal error:

libfdt_env.h: No such file or directory

There seems to be precedence in other architectures/drivers

Quoted 16 lines
Quoted 6 lines
    $ git grep -e '-I.*libfdt'
    arch/mips/cavium-octeon/Makefile:CFLAGS_octeon-platform.o =
(quoted diff) +0 -2
-I$(src)/../../../scripts/dtc/libfdt
    arch/mips/cavium-octeon/Makefile:CFLAGS_setup.o =
-I$(src)/../../../scripts/dtc/libfdt

Review

Quoted 10 lines
(quoted diff) +0 -5
    arch/mips/mti-sead3/Makefile:CFLAGS_sead3-setup.o =
-I$(src)/../../../scripts/dtc/libfdt
    arch/powerpc/kernel/Makefile:CFLAGS_prom.o              =
-I$(src)/../../../scripts/dtc/libfdt
    drivers/firmware/efi/libstub/Makefile:CFLAGS_fdt.o    +=
-I$(srctree)/scripts/dtc/libfdt/
    drivers/of/Makefile:CFLAGS_fdt.o =
-I$(src)/../../scripts/dtc/libfdt
    drivers/of/Makefile:CFLAGS_fdt_address.o =
-I$(src)/../../scripts/dtc/libfdt

I still think we should not duplicate this but instead change the header files to make the include hack unnecessary in all those places.

Looking at the header file, it only contains

#include <linux/libfdt_env.h>
#include "../../scripts/dtc/libfdt/fdt.h"
#include "../../scripts/dtc/libfdt/libfdt.h"

so two of them get the headers from exactly that place, while the libfdt_env.h file contains the linux-specific implementation of a couple of functions instead of the baremetal implementation.

It looks like the intention was that when you already include the linux version of the header, you no longer need the other one, but that assumption is broken by current implementation.

One way to solve this would be

scripts/dtc/libfdt/libfdt.h +1 -1
index 73f49759a5e7..23c55baa8e18 100644
@@ -51,7 +51,7 @@
   *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
   */
 
-#include <libfdt_env.h>
+#include "libfdt_env.h"
  #include <fdt.h>
 
Quoted 9 lines
Quoted 5 lines
scripts/dtc/libfdt/libfdt.h +0 -0
  #define FDT_FIRST_SUPPORTED_VERSION    0x10

but I'm not sure if that is the best way.

I was hoping to limit the scope of my changes to mvebu code. But I can give it a whirl. I agree that propagating the hack isn't ideal.

Well I've sent something up to the devicetree/LKML so we'll see where that goes.

Another alternative I that is not without precedence is to add a of_fdt_xxx function (like of_fdt_limit_memory) to drivers/of/fdt.c which already uses a custom CFLAGS.

Quoted 34 lines
Quoted 20 lines
Quoted 12 lines
Quoted 11 lines
Quoted 6 lines

I think it would be good to first check whether you are running on Armada XP
or Armada 370, because the latter does not require this.

Arnd

Any suggestion as to what to check for. Based on the dts files in the current source seem compatible == "marvell,armada370" seems like a good choice. But I don't know for sure that there isn't a armada-xp variant out using a .dts with this set.

Quoted 7 lines

Adding the following at the top will do the trick (assuming "marvell,armada370" is the right compatible string to use).

         unsigned long root = of_get_flat_dt_root();

         if (of_flat_dt_is_compatible(root, "marvell,armada370"))
                 return;
Quoted 8 lines

Right. You could also add

    if (!IS_ENABLED(CONFIG_SMP) || !of_flat_dt_is_compatible(root,

"marvell,armadaxp")))

to again eliminate that check for uniprocessor kernels, and to not propagate the fixup to future machines but make it clear that it is only needed for armadaxp.

OK makes sense.

Quoted 3 lines

Arnd

#11 lore raw

PATCH 1/4 ARM: mvebu: use dt_fixup to provide fallback for enable-method

Quoted 3 lines · On Tuesday 16 December 2014 02:21:16 Chris Packham wrote:

Well I've sent something up to the devicetree/LKML so we'll see where that goes.

Ok, thanks!

Quoted 3 lines

Another alternative I that is not without precedence is to add a of_fdt_xxx function (like of_fdt_limit_memory) to drivers/of/fdt.c which already uses a custom CFLAGS.

Agreed, that would work too.

Arnd

#12 lore raw

PATCH 1/1 dtc: Use quotes to include header files

In reply to Arnd Bergmann

Currently in arch and driver code that needs early access to the flattened device tree it is necessary to add specific CFLAGS so that when scripts/dtc/libfdt/libfdt.h is included the C preprocessor is able to locate the libfdt versions of libfdt_env.h and fdt.h without generating an error.

We already provide an alternative linux-specific
version of libfdt_env.h and directly include scripts/dtc/libfdt/fdt.h so the inclusion by scripts/dtc/libfdt/libfdt.h is a no-op thanks to the inclusion guards.

By using quotes in scripts/dtc/libfdt/libfdt.h it picks up fdt.h and libfdt_env.h from the source directory without needing to add CFLAGS for the sources that happen to include linux/libfdt.h.

---
Hi,

This probably should come via git://git.jdl.com/software/dtc.git however this appears to be inaccessible at the moment. Is this still the canonical source for the device tree compiler and libfdt or has it been moved? How much deviation from the canonical source are we prepared to live with in the kernel?

This came up on the arm-LKML[1]. Basically in the name of backwards compatibility I need to add a .dt_fixup to add some required nodes to the flattened device tree prior to the tree being un-flattened and processed. This is a trick powerpc makes use of fairly extensively and there are a few other instances of this.

For the files that include linux/libfdt.h we currently also have to specify additional CFLAGS to satisfy the CPP.

  $ git grep '<linux/libfdt.h>'
  arch/mips/cavium-octeon/octeon-platform.c:#include <linux/libfdt.h>
  arch/mips/cavium-octeon/setup.c:#include <linux/libfdt.h>
  arch/mips/mti-sead3/sead3-setup.c:#include <linux/libfdt.h>
  arch/powerpc/kernel/prom.c:#include <linux/libfdt.h>
  drivers/firmware/efi/libstub/fdt.c:#include <linux/libfdt.h>
  drivers/of/fdt.c:#include <linux/libfdt.h>
  drivers/of/fdt_address.c:#include <linux/libfdt.h>
  $ git grep -e '-I.*dtc/libfdt'
  arch/mips/cavium-octeon/Makefile:CFLAGS_octeon-platform.o = -I$(src)/../../../scripts/dtc/libfdt
  arch/mips/cavium-octeon/Makefile:CFLAGS_setup.o = -I$(src)/../../../scripts/dtc/libfdt
  arch/mips/mti-sead3/Makefile:CFLAGS_sead3-setup.o = -I$(src)/../../../scripts/dtc/libfdt
  arch/powerpc/kernel/Makefile:CFLAGS_prom.o              = -I$(src)/../../../scripts/dtc/libfdt
  drivers/firmware/efi/libstub/Makefile:CFLAGS_fdt.o += -I$(srctree)/scripts/dtc/libfdt/
  drivers/of/Makefile:CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
  drivers/of/Makefile:CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
  lib/Makefile:   $(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt))

Simply by switching to using quotes we can avoid having this extra step.

Assuming that this patch is acceptable a follow on clean up would be to remove the instances of CFLAGS_... listed above.

Regards,
Chris
--
[1] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310840.html

 scripts/dtc/libfdt/libfdt.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
scripts/dtc/libfdt/libfdt.h +2 -2
index 73f4975..ea1ddcd 100644
@@ -51,8 +51,8 @@
  *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <libfdt_env.h>
-#include <fdt.h>
+#include "libfdt_env.h"
+#include "fdt.h"
 
 #define FDT_FIRST_SUPPORTED_VERSION	0x10
 #define FDT_LAST_SUPPORTED_VERSION	0x11
Signature · 1 line
2.2.0.rc0
#13 lore raw

Re: [PATCH] dtc: Use quotes to include header files

Quoted 7 lines · Chris Packham

Hi,

This probably should come via git://git.jdl.com/software/dtc.git however this appears to be inaccessible at the moment. Is this still the canonical source for the device tree compiler and libfdt or has it been moved? How much deviation from the canonical source are we prepared to live with in the kernel?

That is because the official DTC repository has moved to git.kernel.org!

Quoted 11 lines

Currently in arch and driver code that needs early access to the flattened device tree it is necessary to add specific CFLAGS so that when scripts/dtc/libfdt/libfdt.h is included the C preprocessor is able to locate the libfdt versions of libfdt_env.h and fdt.h without generating an error.

We already provide an alternative linux-specific
version of libfdt_env.h and directly include scripts/dtc/libfdt/fdt.h so the inclusion by scripts/dtc/libfdt/libfdt.h is a no-op thanks to the inclusion guards.

Quoted 6 lines

By using quotes in scripts/dtc/libfdt/libfdt.h it picks up fdt.h and libfdt_env.h from the source directory without needing to add CFLAGS for the sources that happen to include linux/libfdt.h.

---

and:

Quoted 5 lines
scripts/dtc/libfdt/libfdt.h +0 -0
index 73f4975..ea1ddcd 100644

scripts/dtc/libfdt/libfdt.h line 58

Quoted 10 lines
scripts/dtc/libfdt/libfdt.h +2 -2
@@ -51,8 +51,8 @@
  *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <libfdt_env.h>
-#include <fdt.h>
+#include "libfdt_env.h"
+#include "fdt.h"
 
 #define FDT_FIRST_SUPPORTED_VERSION	0x10
 #define FDT_LAST_SUPPORTED_VERSION	0x11

Hmm. I seem to recall that the use of angle brackets was intentional. I believe that was predicated on installing the FDT library on some target machine and needing it to be able to find the other header files in an installation, not source, directory.

Minor repository spelunking suggests that the angle brackets have been around since Day Zero:

    commit 3da0f9a10dfa9b615d06c350c7b9fe29f360a6e0

David?

HTH?,
jdl

#14 lore raw

Re: [PATCH] dtc: Use quotes to include header files

In reply to Chris Packham
Quoted 71 lines · Chris Packham

Currently in arch and driver code that needs early access to the flattened device tree it is necessary to add specific CFLAGS so that when scripts/dtc/libfdt/libfdt.h is included the C preprocessor is able to locate the libfdt versions of libfdt_env.h and fdt.h without generating an error.

We already provide an alternative linux-specific
version of libfdt_env.h and directly include scripts/dtc/libfdt/fdt.h so the inclusion by scripts/dtc/libfdt/libfdt.h is a no-op thanks to the inclusion guards.

By using quotes in scripts/dtc/libfdt/libfdt.h it picks up fdt.h and libfdt_env.h from the source directory without needing to add CFLAGS for the sources that happen to include linux/libfdt.h.

---
Hi,

This probably should come via git://git.jdl.com/software/dtc.git however this appears to be inaccessible at the moment. Is this still the canonical source for the device tree compiler and libfdt or has it been moved? How much deviation from the canonical source are we prepared to live with in the kernel?

This came up on the arm-LKML[1]. Basically in the name of backwards compatibility I need to add a .dt_fixup to add some required nodes to the flattened device tree prior to the tree being un-flattened and processed. This is a trick powerpc makes use of fairly extensively and there are a few other instances of this.

For the files that include linux/libfdt.h we currently also have to specify additional CFLAGS to satisfy the CPP.

  $ git grep '<linux/libfdt.h>'
  arch/mips/cavium-octeon/octeon-platform.c:#include <linux/libfdt.h>
  arch/mips/cavium-octeon/setup.c:#include <linux/libfdt.h>
  arch/mips/mti-sead3/sead3-setup.c:#include <linux/libfdt.h>
  arch/powerpc/kernel/prom.c:#include <linux/libfdt.h>
  drivers/firmware/efi/libstub/fdt.c:#include <linux/libfdt.h>
  drivers/of/fdt.c:#include <linux/libfdt.h>
  drivers/of/fdt_address.c:#include <linux/libfdt.h>
  $ git grep -e '-I.*dtc/libfdt'
  arch/mips/cavium-octeon/Makefile:CFLAGS_octeon-platform.o = -I$(src)/../../../scripts/dtc/libfdt
  arch/mips/cavium-octeon/Makefile:CFLAGS_setup.o = -I$(src)/../../../scripts/dtc/libfdt
  arch/mips/mti-sead3/Makefile:CFLAGS_sead3-setup.o = -I$(src)/../../../scripts/dtc/libfdt
  arch/powerpc/kernel/Makefile:CFLAGS_prom.o              = -I$(src)/../../../scripts/dtc/libfdt
  drivers/firmware/efi/libstub/Makefile:CFLAGS_fdt.o += -I$(srctree)/scripts/dtc/libfdt/
  drivers/of/Makefile:CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
  drivers/of/Makefile:CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
  lib/Makefile:   $(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt))

Simply by switching to using quotes we can avoid having this extra step.

Assuming that this patch is acceptable a follow on clean up would be to remove the instances of CFLAGS_... listed above.

Regards,
Chris
--
[1] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310840.html

 scripts/dtc/libfdt/libfdt.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
scripts/dtc/libfdt/libfdt.h +0 -0
index 73f4975..ea1ddcd 100644

scripts/dtc/libfdt/libfdt.h line 55

Quoted 7 lines
scripts/dtc/libfdt/libfdt.h +2 -2
@@ -51,8 +51,8 @@
  *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <libfdt_env.h>
-#include <fdt.h>
+#include "libfdt_env.h"
+#include "fdt.h"

Until this is resolved with upstream DTC, what about adding dummy libfdt.h, libfdt_env.h and fdt.h to include/linux? That would get this out of the blocker path for merging this code.

g.

#15 lore raw

Re: [PATCH] dtc: Use quotes to include header files

Quoted 8 lines · Grant Likely
Quoted 5 lines · Chris Packham

Currently in arch and driver code that needs early access to the flattened device tree it is necessary to add specific CFLAGS so that when scripts/dtc/libfdt/libfdt.h is included the C preprocessor is able to locate the libfdt versions of libfdt_env.h and fdt.h without generating an error.

[...]

Quoted 8 lines
Quoted 8 lines
scripts/dtc/libfdt/libfdt.h +0 -0
index 73f4975..ea1ddcd 100644
@@ -51,8 +51,8 @@
  *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
Quoted 8 lines

scripts/dtc/libfdt/libfdt.h line 55

Quoted 4 lines
scripts/dtc/libfdt/libfdt.h +2 -2
-#include <libfdt_env.h>
-#include <fdt.h>
+#include "libfdt_env.h"
+#include "fdt.h"

Until this is resolved with upstream DTC, what about adding dummy libfdt.h, libfdt_env.h and fdt.h to include/linux? That would get this out of the blocker path for merging this code.

2 of those already exist. Is it only a matter of adding fdt.h or is there more to it?

This could also be carried as part of the import script to fix up.

Rob

#16 lore raw

Re: [PATCH] dtc: Use quotes to include header files

In reply to Grant Likely
Quoted 86 lines · Grant Likely
Quoted 71 lines · Chris Packham

Currently in arch and driver code that needs early access to the flattened device tree it is necessary to add specific CFLAGS so that when scripts/dtc/libfdt/libfdt.h is included the C preprocessor is able to locate the libfdt versions of libfdt_env.h and fdt.h without generating an error.

We already provide an alternative linux-specific
version of libfdt_env.h and directly include scripts/dtc/libfdt/fdt.h so the inclusion by scripts/dtc/libfdt/libfdt.h is a no-op thanks to the inclusion guards.

By using quotes in scripts/dtc/libfdt/libfdt.h it picks up fdt.h and libfdt_env.h from the source directory without needing to add CFLAGS for the sources that happen to include linux/libfdt.h.

---
Hi,

This probably should come via git://git.jdl.com/software/dtc.git however this appears to be inaccessible at the moment. Is this still the canonical source for the device tree compiler and libfdt or has it been moved? How much deviation from the canonical source are we prepared to live with in the kernel?

This came up on the arm-LKML[1]. Basically in the name of backwards compatibility I need to add a .dt_fixup to add some required nodes to the flattened device tree prior to the tree being un-flattened and processed. This is a trick powerpc makes use of fairly extensively and there are a few other instances of this.

For the files that include linux/libfdt.h we currently also have to specify additional CFLAGS to satisfy the CPP.

  $ git grep '<linux/libfdt.h>'
  arch/mips/cavium-octeon/octeon-platform.c:#include <linux/libfdt.h>
  arch/mips/cavium-octeon/setup.c:#include <linux/libfdt.h>
  arch/mips/mti-sead3/sead3-setup.c:#include <linux/libfdt.h>
  arch/powerpc/kernel/prom.c:#include <linux/libfdt.h>
  drivers/firmware/efi/libstub/fdt.c:#include <linux/libfdt.h>
  drivers/of/fdt.c:#include <linux/libfdt.h>
  drivers/of/fdt_address.c:#include <linux/libfdt.h>
  $ git grep -e '-I.*dtc/libfdt'
  arch/mips/cavium-octeon/Makefile:CFLAGS_octeon-platform.o = -I$(src)/../../../scripts/dtc/libfdt
  arch/mips/cavium-octeon/Makefile:CFLAGS_setup.o = -I$(src)/../../../scripts/dtc/libfdt
  arch/mips/mti-sead3/Makefile:CFLAGS_sead3-setup.o = -I$(src)/../../../scripts/dtc/libfdt
  arch/powerpc/kernel/Makefile:CFLAGS_prom.o              = -I$(src)/../../../scripts/dtc/libfdt
  drivers/firmware/efi/libstub/Makefile:CFLAGS_fdt.o += -I$(srctree)/scripts/dtc/libfdt/
  drivers/of/Makefile:CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
  drivers/of/Makefile:CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
  lib/Makefile:   $(eval CFLAGS_$(file) = -I$(src)/../scripts/dtc/libfdt))

Simply by switching to using quotes we can avoid having this extra step.

Assuming that this patch is acceptable a follow on clean up would be to remove the instances of CFLAGS_... listed above.

Regards,
Chris
--
[1] - http://lists.infradead.org/pipermail/linux-arm-kernel/2014-December/310840.html

 scripts/dtc/libfdt/libfdt.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
scripts/dtc/libfdt/libfdt.h +0 -0
index 73f4975..ea1ddcd 100644

scripts/dtc/libfdt/libfdt.h line 55

Quoted 7 lines
scripts/dtc/libfdt/libfdt.h +2 -2
@@ -51,8 +51,8 @@
  *     EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
  */
 
-#include <libfdt_env.h>
-#include <fdt.h>
+#include "libfdt_env.h"
+#include "fdt.h"

Until this is resolved with upstream DTC, what about adding dummy libfdt.h, libfdt_env.h and fdt.h to include/linux? That would get this out of the blocker path for merging this code.

Quoted 2 lines

g.

I'm traveling right now but I could work up an example patch next week.

Quoted 1 line

From the other discussions I think there are arguments in favor of keeping

the angle brackets for the userspace libfdt usage. Any change in the kernel either adding dummy files in the system include path or performing a subsititution in an import script would probably be permanent.

#17 lore raw

PATCH 2/4 clk: mvebu: armada-xp: Support for 98DX4251

In reply to Chris Packham

The 98DX4251 is one of a range of packet processors with integrated CPUs based on armada-xp. One difference is that the TCLK frequency is fixed at 200MHz as opposed to the fixed 250MHz used on armada-xp. Additionally the clock-gating options are a subset of what's available on the armada-xp.

---

As per Thomas' and Andrews comments I've made this specific to the 98DX4251 and separated out the gating definitions. It should be good for other variants of what Marvell call "MSYS" but I don't have access to any to test with.

v1: http://lists.infradead.org/pipermail/linux-arm-kernel/2014-November/304387.html

 .../devicetree/bindings/clock/mvebu-core-clock.txt |  4 +++
 drivers/clk/mvebu/armada-xp.c                      | 35 ++++++++++++++++++++++
 2 files changed, 39 insertions(+)
Documentation/devicetree/bindings/clock/mvebu-core-clock.txt +4 -0
index dc5ea5b..503849e 100644
@@ -34,6 +34,9 @@ The following is a list of provided IDs and clock names on Orion5x:
  1 = cpuclk (CPU0 clock)
  2 = ddrclk (DDR controller clock derived from CPU0 clock)
 
+The following is a list of provided IDs and clock names on MV98DX4251:
+ 0 = tclk   (Internal Bus clock)
+
 Required properties:
 - compatible : shall be one of the following:
 	"marvell,armada-370-core-clock" - For Armada 370 SoC core clocks
@@ -46,6 +49,7 @@ Required properties:
 	"marvell,mv88f5182-core-clock" - for Orion MV88F5182 SoC
 	"marvell,mv88f5281-core-clock" - for Orion MV88F5281 SoC
 	"marvell,mv88f6183-core-clock" - for Orion MV88F6183 SoC
+	"marvell,mv98dx4251-core-clock" - for MV98DX4251 SoC
 - reg : shall be the register address of the Sample-At-Reset (SAR) register
 - #clock-cells : from common clock binding; shall be set to 1
 
drivers/clk/mvebu/armada-xp.c +35 -0
index b309431..eb17817 100644
@@ -52,6 +52,12 @@ static u32 __init axp_get_tclk_freq(void __iomem *sar)
 	return 250000000;
 }
 
+/* MV98DX4251 TCLK frequency is fixed to 200MHz */
+static u32 __init mv98dx4251_get_tclk_freq(void __iomem *sar)
+{
+	return 200000000;
+}
+
 static const u32 axp_cpu_freqs[] __initconst = {
 	1000000000,
 	1066000000,
@@ -158,6 +164,14 @@ static const struct coreclk_soc_desc axp_coreclks = {
 	.num_ratios = ARRAY_SIZE(axp_coreclk_ratios),
 };
 
+static const struct coreclk_soc_desc mv98dx4251_coreclks = {
+	.get_tclk_freq = mv98dx4251_get_tclk_freq,
+	.get_cpu_freq = axp_get_cpu_freq,
+	.get_clk_ratio = axp_get_clk_ratio,
+	.ratios = axp_coreclk_ratios,
+	.num_ratios = ARRAY_SIZE(axp_coreclk_ratios),
+};
+
 /*
  * Clock Gating Control
  */
@@ -195,6 +209,15 @@ static const struct clk_gating_soc_desc axp_gating_desc[] __initconst = {
 	{ }
 };
 
+static const struct clk_gating_soc_desc mv98dx4251_gating_desc[] __initconst = {
+	{ "ge1", NULL, 3, 0 },
+	{ "ge0", NULL, 4, 0 },
+	{ "pex00", NULL, 5, 0 },
+	{ "sdio", NULL, 17, 0 },
+	{ "xor0", NULL, 22, 0 },
+	{ }
+};
+
 static void __init axp_clk_init(struct device_node *np)
 {
 	struct device_node *cgnp =
@@ -206,3 +229,15 @@ static void __init axp_clk_init(struct device_node *np)
 		mvebu_clk_gating_setup(cgnp, axp_gating_desc);
 }
 CLK_OF_DECLARE(axp_clk, "marvell,armada-xp-core-clock", axp_clk_init);
+
+static void __init mv98dx4251_clk_init(struct device_node *np)
+{
+	struct device_node *cgnp =
+		of_find_compatible_node(NULL, NULL, "marvell,armada-xp-gating-clock");
+
+	mvebu_coreclk_setup(np, &mv98dx4251_coreclks);
+
+	if (cgnp)
+		mvebu_clk_gating_setup(cgnp, mv98dx4251_gating_desc);
+}
+CLK_OF_DECLARE(mv98dx4251_clk, "marvell,mv98dx4251-core-clock", mv98dx4251_clk_init);
Signature · 1 line
2.2.0.rc0
#18 lore raw

PATCH 3/4 ARM: mvebu: Initial support for rd-dxbc2

In reply to Chris Packham

The 98DX4251 Control and Management subsystem is a feature reduction derivative of the ARMADA XP with some functional changes.

The following table highlights differences between the ARMADA XP MV78230 and the Control and Management subsystem. There is a complete table in the "Control and Management Subsystem Functional Specification" .

Feature                     Bobcat2             MV78230
-------                     -------             -------
CPU Core (ARMv7 compliant   Dual CPU @ up       Dual CPU @ up
with FPU)                   to 800 MHz          to 1.6 GHz

L2 Cache 2MB 1MB

PCIe 1 x1 1 x4 or 4 x1

                                                1 x1

XOR DMA 2 Channels 4 Channels

SPI interface 1 Port 2 Ports

---

I'm not sure how much of a blurb people want about the new platform. I'd just be repeating what's in Marvell's datasheets anyway.

I deliberately left the copyright notice in rd-dxbc2.dts since it was created by copying armada-xp-db.dts. And I don't think anything I did is worth asserting a different copyright. At the moment the mvebu-soc-id.h definition is not strictly needed for anything but hopefully Marvell will come to the party and add the other SoCs.

 arch/arm/boot/dts/rd-dxbc2.dts     | 109 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-mvebu/mvebu-soc-id.h |   3 +
 2 files changed, 112 insertions(+)
 create mode 100644 arch/arm/boot/dts/rd-dxbc2.dts
arch/arm/boot/dts/rd-dxbc2.dts new file +109 -0
new file mode 100644
index 0000000..97a72d4
@@ -0,0 +1,109 @@
+/*
+ * Device Tree file for RD-DXBC2 board
+ *
+ * Copyright (C) 2012-2014 Marvell
+ *
+ * Lior Amsalem <[email protected]>
+ * Gregory CLEMENT <[email protected]>
+ * Thomas Petazzoni <[email protected]>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+  *
+ * Note: this Device Tree assumes that the bootloader has remapped the
+ * internal registers to 0xf1000000 (instead of the default
+ * 0xd0000000). The 0xf1000000 is the default used by the recent,
+ * DT-capable, U-Boot bootloaders provided by Marvell. Some earlier
+ * boards were delivered with an older version of the bootloader that
+ * left internal registers mapped at 0xd0000000. If you are in this
+ * situation, you should either update your bootloader (preferred
+ * solution) or the below Device Tree should be adjusted.
+ */
+
+/dts-v1/;
+#include "armada-xp-mv78260.dtsi"
+
+/ {
+	model = "Marvell Bobcat2 Evaluation Board";
+	compatible = "marvell,axp-db", "marvell,armadaxp-mv78260", "marvell,armadaxp", "marvell,armada-370-xp";
+
+	chosen {
+		bootargs = "console=ttyS0,115200 earlyprintk";
+	};
+
+	memory {
+		device_type = "memory";
+		reg = <0 0x00000000 0 0x20000000>; /* 512 MB */
+	};
+
+	soc {
+		ranges = <MBUS_ID(0xf0, 0x01) 0 0 0xf1000000 0x100000
+			  MBUS_ID(0x01, 0x1d) 0 0 0xfff00000 0x100000
+			  MBUS_ID(0x01, 0x2f) 0 0 0xf0000000 0x1000000>;
+
+		devbus-bootcs {
+			status = "okay";
+
+			/* Device Bus parameters are required */
+
+			/* Read parameters */
+			devbus,bus-width    = <16>;
+			devbus,turn-off-ps  = <60000>;
+			devbus,badr-skew-ps = <0>;
+			devbus,acc-first-ps = <124000>;
+			devbus,acc-next-ps  = <248000>;
+			devbus,rd-setup-ps  = <0>;
+			devbus,rd-hold-ps   = <0>;
+
+			/* Write parameters */
+			devbus,sync-enable = <0>;
+			devbus,wr-high-ps  = <60000>;
+			devbus,wr-low-ps   = <60000>;
+			devbus,ale-wr-ps   = <60000>;
+		};
+
+		internal-regs {
+			serial at 12000 {
+				status = "okay";
+			};
+			serial at 12100 {
+				status = "okay";
+			};
+
+			coreclk: mvebu-sar at 18230 {
+				compatible = "marvell,mv98dx4251-core-clock";
+			};
+
+			i2c at 11000 {
+				compatible = "marvell,mv64xxx-i2c";
+				clock-frequency = <400000>;
+				status = "okay";
+			};
+
+			mvsdio at d4000 {
+				pinctrl-0 = <&sdio_pins>;
+				pinctrl-names = "default";
+				status = "okay";
+				/* No CD or WP GPIOs */
+				broken-cd;
+			};
+
+			spi0: spi at 10600 {
+				status = "okay";
+
+				spi-flash at 0 {
+					#address-cells = <1>;
+					#size-cells = <1>;
+					compatible = "m25p64";
+					reg = <0>; /* Chip select 0 */
+					spi-max-frequency = <20000000>;
+				};
+			};
+
+			xor at f0900 {
+				status = "disabled";
+			};
+		};
+	};
+};
arch/arm/mach-mvebu/mvebu-soc-id.h +3 -0
index c16bb68..98a9dca 100644
@@ -24,6 +24,9 @@
 #define ARMADA_375_Z1_REV   0x0
 #define ARMADA_375_A0_REV   0x3
 
+/* Packet Processors with integrated CPU */
+#define MV98DX4251	    0xFC00
+
 #ifdef CONFIG_ARCH_MVEBU
 int mvebu_get_soc_id(u32 *dev, u32 *rev);
 #else
Signature · 1 line
2.2.0.rc0
#19 lore raw

PATCH 4/4 ARM: mvebu: Custom smp_ops for 98DX4251

In reply to Chris Packham

Compared to the armada-xp the 98DX4251 uses different registers to set the boot address for the secondary CPU. This seems to contradict the datasheet which lists the same PMSU registers. This could be an error in the datasheet since it is likely to reproduce much of the armada-xp information. Or it could be an artifact of the early silicon revision being used.

---

I wasn't sure about the name "pmsu-98dx4251" originally I called this "msys" but following previous conversations on the subject I've opted to go with something specific to the chip I'm dealing with. I could fold this into pmsu.c so it was alongside mvebu_pmsu_set_cpu_boot_addr if people think that would be better.

 arch/arm/boot/dts/rd-dxbc2.dts      |  9 +++++
 arch/arm/mach-mvebu/Makefile        |  1 +
 arch/arm/mach-mvebu/common.h        |  2 ++
 arch/arm/mach-mvebu/platsmp.c       | 28 +++++++++++++++
 arch/arm/mach-mvebu/pmsu-98dx4251.c | 68 +++++++++++++++++++++++++++++++++++++
 5 files changed, 108 insertions(+)
 create mode 100644 arch/arm/mach-mvebu/pmsu-98dx4251.c
arch/arm/boot/dts/rd-dxbc2.dts +9 -0
index 97a72d4..5763c4f 100644
@@ -28,6 +28,10 @@
 	model = "Marvell Bobcat2 Evaluation Board";
 	compatible = "marvell,axp-db", "marvell,armadaxp-mv78260", "marvell,armadaxp", "marvell,armada-370-xp";
 
+	cpus {
+		enable-method = "marvell,98dx4251-smp";
+	};
+
 	chosen {
 		bootargs = "console=ttyS0,115200 earlyprintk";
 	};
@@ -104,6 +108,11 @@
 			xor at f0900 {
 				status = "disabled";
 			};
+
+			resume at 20980 {
+				compatible = "marvell,98dx4251-resume-ctrl";
+				reg = <0x20980 0x10>;
+			};
 		};
 	};
 };
arch/arm/mach-mvebu/Makefile +1 -0
index e24136b..866822e 100644
@@ -8,6 +8,7 @@ obj-$(CONFIG_MACH_MVEBU_ANY)	 += system-controller.o mvebu-soc-id.o
 
 ifeq ($(CONFIG_MACH_MVEBU_V7),y)
 obj-y				 += cpu-reset.o board-v7.o coherency.o coherency_ll.o pmsu.o pmsu_ll.o
+obj-y				 += pmsu-98dx4251.o
 obj-$(CONFIG_SMP)		 += platsmp.o headsmp.o platsmp-a9.o headsmp-a9.o
 endif
 
arch/arm/mach-mvebu/common.h +2 -0
index 3ccb40c..5064ac5 100644
@@ -25,4 +25,6 @@ int mvebu_system_controller_get_soc_id(u32 *dev, u32 *rev);
 
 void __iomem *mvebu_get_scu_base(void);
 
+void mv98dx4251_resume_set_cpu_boot_addr(int hw_cpu, void *boot_addr);
+
 #endif
arch/arm/mach-mvebu/platsmp.c +28 -0
index 895dc37..2a2ed65d 100644
@@ -180,5 +180,33 @@ struct smp_operations armada_xp_smp_ops __initdata = {
 #endif
 };
 
+static int mv98dx4251_boot_secondary(unsigned int cpu, struct task_struct *idle)
+{
+	int ret, hw_cpu;
+
+	pr_info("Booting CPU %d\n", cpu);
+
+	hw_cpu = cpu_logical_map(cpu);
+	mv98dx4251_resume_set_cpu_boot_addr(hw_cpu, armada_xp_secondary_startup);
+	ret = mvebu_cpu_reset_deassert(hw_cpu);
+	if (ret) {
+		pr_warn("unable to boot CPU: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+struct smp_operations mv98dx4251_smp_ops __initdata = {
+	.smp_init_cpus		= armada_xp_smp_init_cpus,
+	.smp_prepare_cpus	= armada_xp_smp_prepare_cpus,
+	.smp_boot_secondary	= mv98dx4251_boot_secondary,
+#ifdef CONFIG_HOTPLUG_CPU
+	.cpu_die		= armada_xp_cpu_die,
+#endif
+};
+
 CPU_METHOD_OF_DECLARE(armada_xp_smp, "marvell,armada-xp-smp",
 		      &armada_xp_smp_ops);
+CPU_METHOD_OF_DECLARE(mv98dx4251_smp, "marvell,98dx4251-smp",
+		      &mv98dx4251_smp_ops);
arch/arm/mach-mvebu/pmsu-98dx4251.c new file +68 -0

68 lines not on this page · raw message

Signature · 1 line
2.2.0.rc0