of: Create asm-generic/of.h and provide default of_node_to_nid()

PATCH In review Grant Likely 4 participants 10 messages Jul 26, 2010 – Jul 27, 2010

Subsystems (4)

PowerPC SPARC + UltraSPARC (sparc/sparc64) Device tree Microblaze architecture

lore

Share & tools

lore

Grant Likely

author

2010-07-26 22:04 UTC #0 lore raw

PATCH v3 1/1 of: Create asm-generic/of.h and provide default of_node_to_nid()

of_node_to_nid() is only relevant in a few architectures. Don't force everyone to implement it anyway. This patch also adds asm-generic/of.h which will be used to contain other overrideable symbols.

Signed-off-by Grant Likely <[email protected]>

---

Changes in v3: don't use asm-generic, just keep macros in of.h Changes in v2: address comments from sfr, add asm-generic/of.h

 arch/microblaze/include/asm/topology.h |   10 ----------
 arch/powerpc/include/asm/prom.h        |    5 +++++
 arch/powerpc/include/asm/topology.h    |    7 -------
 arch/sparc/include/asm/prom.h          |    1 +
 include/linux/of.h                     |    5 +++++
 5 files changed, 11 insertions(+), 17 deletions(-)

arch/microblaze/include/asm/topology.h +0 ~~-10~~

index 96bcea5..5428f33 100644
@@ -1,11 +1 @@
 #include <asm-generic/topology.h>
-
-#ifndef _ASM_MICROBLAZE_TOPOLOGY_H
-#define _ASM_MICROBLAZE_TOPOLOGY_H
-
-struct device_node;
-static inline int of_node_to_nid(struct device_node *device)
-{
-	return 0;
-}
-#endif /* _ASM_MICROBLAZE_TOPOLOGY_H */

arch/powerpc/include/asm/prom.h +5 -0

index da7dd63..dca25a5 100644
@@ -103,6 +103,11 @@ struct device_node *of_find_next_cache_node(struct device_node *np);
 /* Get the MAC address */
 extern const void *of_get_mac_address(struct device_node *np);
 
+#ifdef CONFIG_NUMA
+extern int of_node_to_nid(struct device_node *device);
+#define of_node_to_nid of_node_to_nid
+#endif
+
 /**
  * of_irq_map_pci - Resolve the interrupt for a PCI device
  * @pdev:	the device whose interrupt is to be resolved

arch/powerpc/include/asm/topology.h +0 -7

index 32adf72..09dd38c 100644
@@ -41,8 +41,6 @@ static inline int cpu_to_node(int cpu)
 			       cpu_all_mask :				\
 			       node_to_cpumask_map[node])
 
-int of_node_to_nid(struct device_node *device);
-
 struct pci_bus;
 #ifdef CONFIG_PCI
 extern int pcibus_to_node(struct pci_bus *bus);

@@ -94,11 +92,6 @@ extern void sysfs_remove_device_from_node(struct sys_device *dev, int nid);
 
 #else
 
-static inline int of_node_to_nid(struct device_node *device)
-{
-	return 0;
-}
-
 static inline void dump_numa_cpu_topology(void) {}
 
 static inline int sysfs_add_device_to_node(struct sys_device *dev, int nid)

arch/sparc/include/asm/prom.h +1 -0

index c82a7da..73befa5 100644
@@ -43,6 +43,7 @@ extern int of_getintprop_default(struct device_node *np,
 extern int of_find_in_proplist(const char *list, const char *match, int len);
 #ifdef CONFIG_NUMA
 extern int of_node_to_nid(struct device_node *dp);
+#define of_node_to_nid of_node_to_nid
 #else
 #define of_node_to_nid(dp)	(-1)
 #endif

include/linux/of.h +5 -0

index b0756f3..cc936ca 100644
@@ -146,6 +146,11 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
 
 #define OF_BAD_ADDR	((u64)-1)
 
+#ifndef of_node_to_nid
+static inline int of_node_to_nid(struct device_node *np) { return 0; }
+#define of_node_to_nid of_node_to_nid
+#endif
+
 extern struct device_node *of_find_node_by_name(struct device_node *from,
 	const char *name);
 #define for_each_node_by_name(dn, name) \

Grant Likely

author

2010-07-26 22:05 UTC #1 lore raw

Quoted 5 lines · Grant Likely

of_node_to_nid() is only relevant in a few architectures. Don't force everyone to implement it anyway. This patch also adds asm-generic/of.h which will be used to contain other overrideable symbols.
Signed-off-by Grant Likely <[email protected]>

Oops. The patch subject I will change before mergine.

Quoted 99 lines

---

Changes in v3: don't use asm-generic, just keep macros in of.h Changes in v2: address comments from sfr, add asm-generic/of.h

 5 files changed, 11 insertions(+), 17 deletions(-)

arch/microblaze/include/asm/topology.h +0 ~~-10~~

index 96bcea5..5428f33 100644
@@ -1,11 +1 @@
  #include <asm-generic/topology.h>
-
-#ifndef _ASM_MICROBLAZE_TOPOLOGY_H
-#define _ASM_MICROBLAZE_TOPOLOGY_H
-
-struct device_node;
-static inline int of_node_to_nid(struct device_node *device)
-{
-       return 0;
-}
-#endif /* _ASM_MICROBLAZE_TOPOLOGY_H */

arch/powerpc/include/asm/prom.h +5 -0

index da7dd63..dca25a5 100644
@@ -103,6 +103,11 @@ struct device_node *of_find_next_cache_node(struct device_node *np);
  /* Get the MAC address */
  extern const void *of_get_mac_address(struct device_node *np);
 
+#ifdef CONFIG_NUMA
+extern int of_node_to_nid(struct device_node *device);
+#define of_node_to_nid of_node_to_nid
+#endif
+
  /**
  * of_irq_map_pci - Resolve the interrupt for a PCI device
  * @pdev:      the device whose interrupt is to be resolved

arch/powerpc/include/asm/topology.h +0 -7

index 32adf72..09dd38c 100644
@@ -41,8 +41,6 @@ static inline int cpu_to_node(int cpu)
                               cpu_all_mask :                           \
                               node_to_cpumask_map[node])
 
-int of_node_to_nid(struct device_node *device);
-
  struct pci_bus;
  #ifdef CONFIG_PCI
  extern int pcibus_to_node(struct pci_bus *bus);

@@ -94,11 +92,6 @@ extern void sysfs_remove_device_from_node(struct sys_device *dev, int nid);
 
  #else
 
-static inline int of_node_to_nid(struct device_node *device)
-{
-       return 0;
-}
-
  static inline void dump_numa_cpu_topology(void) {}
 
  static inline int sysfs_add_device_to_node(struct sys_device *dev, int nid)

arch/sparc/include/asm/prom.h +1 -0

index c82a7da..73befa5 100644
@@ -43,6 +43,7 @@ extern int of_getintprop_default(struct device_node *np,
  extern int of_find_in_proplist(const char *list, const char *match, int len);
  #ifdef CONFIG_NUMA
  extern int of_node_to_nid(struct device_node *dp);
+#define of_node_to_nid of_node_to_nid
  #else
  #define of_node_to_nid(dp)     (-1)
  #endif

include/linux/of.h +5 -0

index b0756f3..cc936ca 100644
@@ -146,6 +146,11 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
 
  #define OF_BAD_ADDR    ((u64)-1)
 
+#ifndef of_node_to_nid
+static inline int of_node_to_nid(struct device_node *np) { return 0; }
+#define of_node_to_nid of_node_to_nid
+#endif
+
  extern struct device_node *of_find_node_by_name(struct device_node *from,
        const char *name);
  #define for_each_node_by_name(dn, name) \

Signature · 6 lines

Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sam Ravnborg

2010-07-26 22:26 UTC #2 lore raw

In reply to Grant Likely

Quoted 9 lines · Grant Likely

of_node_to_nid() is only relevant in a few architectures. Don't force everyone to implement it anyway. This patch also adds asm-generic/of.h which will be used to contain other overrideable symbols.
Signed-off-by Grant Likely <[email protected]>
---
Changes in v3: don't use asm-generic, just keep macros in of.h Changes in v2: address comments from sfr, add asm-generic/of.h

The use of asm-generic makes perfect sense. This is how we usually deal with arch specific stuff.

With v3 of your patch we have a different result depending on if we do:
#include <linux/of.h>

or we do:
#include <asm/prom.h>

This is undesireable.

I suggest to go back to v2 of your patch where you use asm-generic/of.h.

linux/of.h shall include asm/of.h
Then all archs shall have a of.h that may
include the asm-generic variant.

One patch to introduce of.h all over.
And a second patch to do the of_node_to_nid stuff would be appropriate.

arch/powerpc/include/asm/prom.h struct device_node *of_find_next_cache_node(struct device_node *np); · line 105

Quoted 8 lines

arch/powerpc/include/asm/prom.h +0 -0

index da7dd63..dca25a5 100644
@@ -103,6 +103,11 @@ struct device_node *of_find_next_cache_node(struct device_node *np);
 /* Get the MAC address */
 extern const void *of_get_mac_address(struct device_node *np);

This shall go in asm/of.h

arch/powerpc/include/asm/prom.h struct device_node *of_find_next_cache_node(struct device_node *np); · line 108

Quoted 3 lines

arch/powerpc/include/asm/prom.h +3 -0

+#ifdef CONFIG_NUMA
+extern int of_node_to_nid(struct device_node *device);
+#define of_node_to_nid of_node_to_nid

This define is used to tell asm-generic/of.h that the arch has a local definition - OK.

Quoted 11 lines

arch/powerpc/include/asm/prom.h +2 -0

+#endif
+
 /**
  * of_irq_map_pci - Resolve the interrupt for a PCI device
  * @pdev:	the device whose interrupt is to be resolved

include/linux/of.h +0 -0

index b0756f3..cc936ca 100644

include/linux/of.h static inline unsigned long of_read_ulong(const __be32 *cell, int size) · line 151

Quoted 6 lines

include/linux/of.h +3 -0

@@ -146,6 +146,11 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
 
 #define OF_BAD_ADDR	((u64)-1)
 
+#ifndef of_node_to_nid
+static inline int of_node_to_nid(struct device_node *np) { return 0; }
+#define of_node_to_nid of_node_to_nid

But I fail to see the purpose of this define.

Sam

Grant Likely

author

2010-07-26 22:42 UTC #3 lore raw

Quoted 14 lines · Sam Ravnborg

Quoted 9 lines · Grant Likely
of_node_to_nid() is only relevant in a few architectures. Don't force everyone to implement it anyway. This patch also adds asm-generic/of.h which will be used to contain other overrideable symbols.
Signed-off-by Grant Likely <[email protected]>
---
Changes in v3: don't use asm-generic, just keep macros in of.h Changes in v2: address comments from sfr, add asm-generic/of.h
The use of asm-generic makes perfect sense. This is how we usually deal with arch specific stuff.

Quoted 8 lines

With v3 of your patch we have a different result depending on if we do:
#include <linux/of.h>
or we do:
#include <asm/prom.h>
This is undesireable.

The patch does maintain consistency. Including only asm/prom.h may mean that of_node_to_nid is undefined, but it will never result in a different definition. linux/of.h includes asm/prom.h before doing the #ifdef test.

Quoted 1 line

I suggest to go back to v2 of your patch where you use asm-generic/of.h.

Stephen suggested dropping asm-generic/of.h. I'm happy to do it either way.

Quoted 38 lines

linux/of.h shall include asm/of.h
Then all archs shall have a of.h that may
include the asm-generic variant.
One patch to introduce of.h all over.
And a second patch to do the of_node_to_nid stuff would be appropriate.
arch/powerpc/include/asm/prom.h struct device_node *of_find_next_cache_node(struct device_node *np); · line 105
Quoted 8 lines
arch/powerpc/include/asm/prom.h +0 -0
index da7dd63..dca25a5 100644
@@ -103,6 +103,11 @@ struct device_node *of_find_next_cache_node(struct device_node *np);
  /* Get the MAC address */
  extern const void *of_get_mac_address(struct device_node *np);
 
This shall go in asm/of.h
arch/powerpc/include/asm/prom.h struct device_node *of_find_next_cache_node(struct device_node *np); · line 108
Quoted 3 lines
arch/powerpc/include/asm/prom.h +3 -0
+#ifdef CONFIG_NUMA
+extern int of_node_to_nid(struct device_node *device);
+#define of_node_to_nid of_node_to_nid
This define is used to tell asm-generic/of.h that the arch has a local definition - OK.
Quoted 12 lines
arch/powerpc/include/asm/prom.h +2 -0
+#endif
+
  /**
   * of_irq_map_pci - Resolve the interrupt for a PCI device
   * @pdev:    the device whose interrupt is to be resolved
include/linux/of.h +0 -0
index b0756f3..cc936ca 100644
@@ -146,6 +146,11 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size)
 

Quoted 7 lines

include/linux/of.h static inline unsigned long of_read_ulong(const __be32 *cell, int size) · line 151
Quoted 5 lines
include/linux/of.h +3 -0
  #define OF_BAD_ADDR  ((u64)-1)
 
+#ifndef of_node_to_nid
+static inline int of_node_to_nid(struct device_node *np) { return 0; }
+#define of_node_to_nid of_node_to_nid
But I fail to see the purpose of this define.

It protects against some later include file doing a #define of_node_to_nid and thus resulting in an inconsistent definition. If some code tries to do this then the preprocessor will complain. This is the pattern that Stephen suggested.

g.
--
To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

David Miller

maintainer

2010-07-26 22:48 UTC #4 lore raw

From: Grant Likely <[email protected]>
Date: Mon, 26 Jul 2010 16:42:20 -0600

Quoted 4 lines

It protects against some later include file doing a #define of_node_to_nid and thus resulting in an inconsistent definition. If some code tries to do this then the preprocessor will complain. This is the pattern that Stephen suggested.

Also, it would be nice to unravel the "0" vs. "-1" default inconsistency.

Grant Likely

author

2010-07-26 22:51 UTC #5 lore raw

Quoted 10 lines · David Miller

From: Grant Likely <[email protected]>
Date: Mon, 26 Jul 2010 16:42:20 -0600
Quoted 4 lines
It protects against some later include file doing a #define of_node_to_nid and thus resulting in an inconsistent definition. If some code tries to do this then the preprocessor will complain. This is the pattern that Stephen suggested.
Also, it would be nice to unravel the "0" vs. "-1" default inconsistency.

Indeed. I looked at it briefly, but it wasn't immediately clear what the impact would be to switch powerpc over to -1, and it looked to me like sparc depends on -1 to signify no node association.

David Miller

maintainer

2010-07-27 01:37 UTC #6 lore raw

From: Grant Likely <[email protected]>
Date: Mon, 26 Jul 2010 16:51:01 -0600

Quoted 3 lines

Indeed. I looked at it briefly, but it wasn't immediately clear what the impact would be to switch powerpc over to -1, and it looked to me like sparc depends on -1 to signify no node association.

Kernel wide, NUMA interfaces tend to take "-1" to mean "any node."

I had looked over the powerpc cases, and besides 1 or 2 strange locations the powerpc call sites were ready to handle -1.

Grant Likely

author

2010-07-27 01:57 UTC #7 lore raw

Quoted 11 lines · David Miller

From: Grant Likely <grant.likely-s3s/[email protected]>
Date: Mon, 26 Jul 2010 16:51:01 -0600
Quoted 3 lines
Indeed. I looked at it briefly, but it wasn't immediately clear what the impact would be to switch powerpc over to -1, and it looked to me like sparc depends on -1 to signify no node association.
Kernel wide, NUMA interfaces tend to take "-1" to mean "any node."
I had looked over the powerpc cases, and besides 1 or 2 strange locations the powerpc call sites were ready to handle -1.

Okay, well let me rework the patch to make -1 the default and let powerpc override it. A follow-on patch can fix up the powerpc usage of 0.

Arnd Bergmann

2010-07-27 13:34 UTC #8 lore raw

In reply to Grant Likely

Quoted 3 lines · Grant Likely

Quoted 1 line
I suggest to go back to v2 of your patch where you use asm-generic/of.h.
Stephen suggested dropping asm-generic/of.h. I'm happy to do it either way.

I don't mind adding stuff to asm-generic, but I think in this case it would be easier to keep this in linux/of.h because there is nothing wrong with all architectures including it.

Most files in asm-generic are there only for historical reasons, where some architectures use them but others don't. IMHO we should use the include/linux headers preferred for new stuff though.

Arnd

Sam Ravnborg

2010-07-27 16:52 UTC #9 lore raw

Quoted 5 lines · Arnd Bergmann

Quoted 3 lines · Grant Likely
Quoted 1 line
I suggest to go back to v2 of your patch where you use asm-generic/of.h.
Stephen suggested dropping asm-generic/of.h. I'm happy to do it either way.

Quoted 7 lines

I don't mind adding stuff to asm-generic, but I think in this case it would be easier to keep this in linux/of.h because there is nothing wrong with all architectures including it.
Most files in asm-generic are there only for historical reasons, where some architectures use them but others don't. IMHO we should use the include/linux headers preferred for new stuff though.

Hi Arnd.

Thanks for this explanation. Obviously my previous post about asm-generic was wrong (as Grant already indicated).

Sam