ARM: integrator: fix OF-related regression

Commit 07e461cd7e73a84f0e3757932b93cc80976fd749 "of: Ensure unique names without sacrificing determinism" caused a boot failure regression on the Integrator machines.

The problem is probably caused by fiddling too much with the device tree population in the OF init function, such as passing the SoC bus device as parent when populating the device tree.

This patch fixes the problem by:

- Avoiding to explicitly look up the tree root
- Look up devices needed before device population from the match only, passing NULL as root
- Passing NULL as root and parent when calling of_platform_populate()

After this the Integrators boot again. Tested on Integrator/AP and Integrator/CP.

Cc Grant Likely <[email protected]>

Cc Arnd Bergmann <[email protected]>

Signed-off-by Linus Walleij <[email protected]>

---
ARM SoC maintainers: if you're happy with this fix, please apply it directly for fixes in the ARM SoC tree, thanks.

---
 arch/arm/mach-integrator/integrator_ap.c | 26 +++++++-------------------
 arch/arm/mach-integrator/integrator_cp.c | 23 ++++++-----------------
 2 files changed, 13 insertions(+), 36 deletions(-)

arch/arm/mach-integrator/integrator_ap.c +7 -19

index dd0cc677d596..660ca6feff40 100644
@@ -480,25 +480,18 @@ static const struct of_device_id ebi_match[] = {
 static void __init ap_init_of(void)
 {
        unsigned long sc_dec;
-       struct device_node *root;
        struct device_node *syscon;
        struct device_node *ebi;
        struct device *parent;
        struct soc_device *soc_dev;
        struct soc_device_attribute *soc_dev_attr;
        u32 ap_sc_id;
-       int err;
        int i;
 
-       /* Here we create an SoC device for the root node */
-       root = of_find_node_by_path("/");
-       if (!root)
-               return;
-
-       syscon = of_find_matching_node(root, ap_syscon_match);
+       syscon = of_find_matching_node(NULL, ap_syscon_match);
        if (!syscon)
                return;
-       ebi = of_find_matching_node(root, ebi_match);
+       ebi = of_find_matching_node(NULL, ebi_match);
        if (!ebi)
                return;
 

@@ -509,19 +502,17 @@ static void __init ap_init_of(void)
        if (!ebi_base)
                return;
 
+       of_platform_populate(NULL, of_default_bus_match_table,
+                       ap_auxdata_lookup, NULL);
+
        ap_sc_id = readl(ap_syscon_base);
 
        soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
        if (!soc_dev_attr)
                return;
 
-       err = of_property_read_string(root, "compatible",
-                                     &soc_dev_attr->soc_id);
-       if (err)
-               return;
-       err = of_property_read_string(root, "model", &soc_dev_attr->machine);
-       if (err)
-               return;
+       soc_dev_attr->soc_id = "XVC";
+       soc_dev_attr->machine = "Integrator/AP";
        soc_dev_attr->family = "Integrator";
        soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%c",
                                           'A' + (ap_sc_id & 0x0f));

@@ -536,9 +527,6 @@ static void __init ap_init_of(void)
        parent = soc_device_to_device(soc_dev);
        integrator_init_sysfs(parent, ap_sc_id);
 
-       of_platform_populate(root, of_default_bus_match_table,
-                       ap_auxdata_lookup, parent);
-
        sc_dec = readl(ap_syscon_base + INTEGRATOR_SC_DEC_OFFSET);
        for (i = 0; i < 4; i++) {
                struct lm_device *lmdev;

arch/arm/mach-integrator/integrator_cp.c +6 -18

index a938242b0c95..0e57f8f820a5 100644
@@ -279,20 +279,13 @@ static const struct of_device_id intcp_syscon_match[] = {
 
 static void __init intcp_init_of(void)
 {
-       struct device_node *root;
        struct device_node *cpcon;
        struct device *parent;
        struct soc_device *soc_dev;
        struct soc_device_attribute *soc_dev_attr;
        u32 intcp_sc_id;
-       int err;
 
-       /* Here we create an SoC device for the root node */
-       root = of_find_node_by_path("/");
-       if (!root)
-               return;
-
-       cpcon = of_find_matching_node(root, intcp_syscon_match);
+       cpcon = of_find_matching_node(NULL, intcp_syscon_match);
        if (!cpcon)
                return;
 

@@ -300,19 +293,17 @@ static void __init intcp_init_of(void)
        if (!intcp_con_base)
                return;
 
+       of_platform_populate(NULL, of_default_bus_match_table,
+                            intcp_auxdata_lookup, NULL);
+
        intcp_sc_id = readl(intcp_con_base);
 
        soc_dev_attr = kzalloc(sizeof(*soc_dev_attr), GFP_KERNEL);
        if (!soc_dev_attr)
                return;
 
-       err = of_property_read_string(root, "compatible",
-                                     &soc_dev_attr->soc_id);
-       if (err)
-               return;
-       err = of_property_read_string(root, "model", &soc_dev_attr->machine);
-       if (err)
-               return;
+       soc_dev_attr->soc_id = "XCV";
+       soc_dev_attr->machine = "Integrator/CP";
        soc_dev_attr->family = "Integrator";
        soc_dev_attr->revision = kasprintf(GFP_KERNEL, "%c",
                                           'A' + (intcp_sc_id & 0x0f));

@@ -326,8 +317,6 @@ static void __init intcp_init_of(void)
 
        parent = soc_device_to_device(soc_dev);
        integrator_init_sysfs(parent, intcp_sc_id);
-       of_platform_populate(root, of_default_bus_match_table,
-                       intcp_auxdata_lookup, parent);
 }
 
 static const char * intcp_dt_board_compat[] = {
--

1.9.3

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Quoted 11 lines · Linus Walleij

Commit 07e461cd7e73a84f0e3757932b93cc80976fd749 "of: Ensure unique names without sacrificing determinism" caused a boot failure regression on the Integrator machines.

The problem is probably caused by fiddling too much with the device tree population in the OF init function, such as passing the SoC bus device as parent when populating the device tree.

This patch fixes the problem by:

Quoted 5 lines

- Avoiding to explicitly look up the tree root
- Look up devices needed before device population from the match only, passing NULL as root
- Passing NULL as root and parent when calling of_platform_populate()

Just curious, I don't see how this fixes booting for Integrator. Where exactly does boot fail?

Quoted 18 lines · Rob Herring

Quoted 16 lines · Linus Walleij

Commit 07e461cd7e73a84f0e3757932b93cc80976fd749 "of: Ensure unique names without sacrificing determinism" caused a boot failure regression on the Integrator machines.

The problem is probably caused by fiddling too much with the device tree population in the OF init function, such as passing the SoC bus device as parent when populating the device tree.

This patch fixes the problem by:

- Avoiding to explicitly look up the tree root
- Look up devices needed before device population from the match only, passing NULL as root
- Passing NULL as root and parent when calling of_platform_populate()

Quoted 2 lines

Just curious, I don't see how this fixes booting for Integrator. Where exactly does boot fail?

It fails by failing to populate the devicetree somehow. I don't have the proper debugging needs, I just trial-and-horror-fixed it by making the board init similar to other platforms :-/

Quoted 22 lines · Linus Walleij

Quoted 20 lines · Rob Herring

Quoted 9 lines · Linus Walleij

Commit 07e461cd7e73a84f0e3757932b93cc80976fd749 "of: Ensure unique names without sacrificing determinism" caused a boot failure regression on the Integrator machines.

The problem is probably caused by fiddling too much with the device tree population in the OF init function, such as passing the SoC bus device as parent when populating the device tree.

Quoted 7 lines

This patch fixes the problem by:

- Avoiding to explicitly look up the tree root
- Look up devices needed before device population from the match only, passing NULL as root
- Passing NULL as root and parent when calling of_platform_populate()

Just curious, I don't see how this fixes booting for Integrator. Where exactly does boot fail?

Quoted 3 lines

It fails by failing to populate the devicetree somehow. I don't have the proper debugging needs, I just trial-and-horror-fixed it by making the board init similar to other platforms :-/

earlyprintk doesn't work on Integrator hardware? It does on QEMU's integratorcp so it's trivial to debug there.

The problem is that integrator_init_sysfs() adds a device attribute called "fpga" to the soc0 bus, and the soc0 bus device is also used as the root of the device tree, and the device tree has a node called "fpga".

Before 07e461cd7e73a84f0e3757932b93cc80976fd749 this node directory and attribute file had unique paths:

sysfs: creating file '/devices/soc0/fpga'
sysfs: creating dir '/devices/soc0/fpga.0'

07e461cd7e73a84f0e3757932b93cc80976fd749 removed the .0 from the fpga node's directory, leading to that node (and its subnodes) failing to register due to a filename conflict with the fpga attribute's file:

Quoted 23 lines · Rabin Vincent

Quoted 22 lines · Linus Walleij

Quoted 20 lines · Rob Herring

Quoted 9 lines · Linus Walleij

Commit 07e461cd7e73a84f0e3757932b93cc80976fd749 "of: Ensure unique names without sacrificing determinism" caused a boot failure regression on the Integrator machines.

The problem is probably caused by fiddling too much with the device tree population in the OF init function, such as passing the SoC bus device as parent when populating the device tree.

Quoted 7 lines

This patch fixes the problem by:

- Avoiding to explicitly look up the tree root
- Look up devices needed before device population from the match only, passing NULL as root
- Passing NULL as root and parent when calling of_platform_populate()

Just curious, I don't see how this fixes booting for Integrator. Where exactly does boot fail?

Quoted 6 lines

Quoted 3 lines

It fails by failing to populate the devicetree somehow. I don't have the proper debugging needs, I just trial-and-horror-fixed it by making the board init similar to other platforms :-/

earlyprintk doesn't work on Integrator hardware? It does on QEMU's integratorcp so it's trivial to debug there.

I got as far as building a kernel to do that before getting distracted...

Quoted 7 lines

The problem is that integrator_init_sysfs() adds a device attribute called "fpga" to the soc0 bus, and the soc0 bus device is also used as the root of the device tree, and the device tree has a node called "fpga".

Before 07e461cd7e73a84f0e3757932b93cc80976fd749 this node directory and attribute file had unique paths:

Quoted 6 lines

sysfs: creating file '/devices/soc0/fpga'
sysfs: creating dir '/devices/soc0/fpga.0'

07e461cd7e73a84f0e3757932b93cc80976fd749 removed the .0 from the fpga node's directory, leading to that node (and its subnodes) failing to register due to a filename conflict with the fpga attribute's file:

Thanks for debugging. Now this makes sense as to why this is specific to Integrator. I think this is the right direction and we should not put devices under soc0/. That way the structure is consistent across SOCs and regardless of whether you decide to create an soc_device or not.

earlyprintk doesn't work on Integrator hardware?

It does on QEMU's
integratorcp so it's trivial to debug there.

The problem is that integrator_init_sysfs() adds a device attribute called "fpga" to the soc0 bus, and the soc0 bus device is also used as the root of the device tree, and the device tree has a node called "fpga".

07e461cd7e73a84f0e3757932b93cc80976fd749 removed the .0 from the fpga node's directory, leading to that node (and its subnodes) failing to register due to a filename conflict with the fpga attribute's file:

After Linus' patch, the root is no longer soc0 so the fpga node's directory becomes /devices/fpga while the attribute file remains at /devices/soc0/fpga.

Quoted 1 line · Rabin Vincent

earlyprintk doesn't work on Integrator hardware?

It does, but loading the kernel is done over serial line 115200 bps so iterative tests take "some time" :-)

Quoted 2 lines

It does on QEMU's
integratorcp so it's trivial to debug there.

Yeah I should be using QEMU for things like this ... poke self.

Quoted 3 lines

The problem is that integrator_init_sysfs() adds a device attribute called "fpga" to the soc0 bus, and the soc0 bus device is also used as the root of the device tree, and the device tree has a node called "fpga".

(...)

Quoted 3 lines

07e461cd7e73a84f0e3757932b93cc80976fd749 removed the .0 from the fpga node's directory, leading to that node (and its subnodes) failing to register due to a filename conflict with the fpga attribute's file:

Aha! Thanks for root-causing this Rabin. Awesome.

Quoted 2 lines

After Linus' patch, the root is no longer soc0 so the fpga node's directory becomes /devices/fpga while the attribute file remains at /devices/soc0/fpga.

It appears the patch is doing the right thing, though I had very vague ideas as to why.

Quoted 25 lines · Linus Walleij

Quoted 1 line · Rabin Vincent

earlyprintk doesn't work on Integrator hardware?

It does, but loading the kernel is done over serial line 115200 bps so iterative tests take "some time" :-)

Quoted 2 lines

It does on QEMU's
integratorcp so it's trivial to debug there.

Yeah I should be using QEMU for things like this ... poke self.

Quoted 3 lines

The problem is that integrator_init_sysfs() adds a device attribute called "fpga" to the soc0 bus, and the soc0 bus device is also used as the root of the device tree, and the device tree has a node called "fpga".

(...)

Quoted 3 lines

07e461cd7e73a84f0e3757932b93cc80976fd749 removed the .0 from the fpga node's directory, leading to that node (and its subnodes) failing to register due to a filename conflict with the fpga attribute's file:

Aha! Thanks for root-causing this Rabin. Awesome.

Quoted 2 lines

After Linus' patch, the root is no longer soc0 so the fpga node's directory becomes /devices/fpga while the attribute file remains at /devices/soc0/fpga.

Quoted 2 lines

It appears the patch is doing the right thing, though I had very vague ideas as to why.

Ok, sounds like this is good to go. I'll apply it to the fixes branch, we can fairly easily take it out right now if it needs to bake more so please speak up quickly if that is the case.