| Message ID | 20180830190523.31474-4-robh@kernel.org |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | of: root #{size,address}-cells clean-ups | expand |
On 08/30/18 12:05, Rob Herring wrote: > Only some old OpenFirmware implementations rely on default sizes. Any > FDT and modern implementation should have explicit properties. Make the > OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside > users. > > This also gets us one step closer to removing the asm/prom.h dependency on > Sparc. > > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Frank Rowand <frowand.list@gmail.com> > Cc: sparclinux@vger.kernel.org > Signed-off-by: Rob Herring <robh@kernel.org> > --- > arch/sparc/include/asm/prom.h | 3 --- > drivers/of/of_private.h | 8 ++++++++ > include/linux/of.h | 6 ------ > 3 files changed, 8 insertions(+), 9 deletions(-) Reviewed-by: Frank Rowand <frank.rowand@sony.com> > diff --git a/arch/sparc/include/asm/prom.h b/arch/sparc/include/asm/prom.h > index d955c8df62d6..1902db27ff4b 100644 > --- a/arch/sparc/include/asm/prom.h > +++ b/arch/sparc/include/asm/prom.h > @@ -24,9 +24,6 @@ > #include <linux/atomic.h> > #include <linux/irqdomain.h> > > -#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 2 > -#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1 > - > #define of_compat_cmp(s1, s2, l) strncmp((s1), (s2), (l)) > #define of_prop_cmp(s1, s2) strcasecmp((s1), (s2)) > #define of_node_cmp(s1, s2) strcmp((s1), (s2)) > diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h > index 216175d11d3d..5d1567025358 100644 > --- a/drivers/of/of_private.h > +++ b/drivers/of/of_private.h > @@ -27,6 +27,14 @@ struct alias_prop { > char stem[0]; > }; > > +#if defined(CONFIG_SPARC) > +#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 2 > +#else > +#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 1 > +#endif > + > +#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1 > + > extern struct mutex of_mutex; > extern struct list_head aliases_lookup; > extern struct kset *of_kset; > diff --git a/include/linux/of.h b/include/linux/of.h > index 506beca9588d..49d85f670c75 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -247,12 +247,6 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size) > #include <asm/prom.h> > #endif > > -/* Default #address and #size cells. Allow arch asm/prom.h to override */ > -#if !defined(OF_ROOT_NODE_ADDR_CELLS_DEFAULT) > -#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 1 > -#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1 > -#endif > - > #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags) > #define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags) > >
On 05/09/18 02:55, Frank Rowand wrote: > On 08/30/18 12:05, Rob Herring wrote: >> Only some old OpenFirmware implementations rely on default sizes. Any >> FDT and modern implementation should have explicit properties. Make the >> OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside >> users. >> >> This also gets us one step closer to removing the asm/prom.h dependency on >> Sparc. Just for the record: you say "any FDT and modern implementation should have explicit properties", however the default values of these properties when missing are clearly defined in the IEEE-1275 specification (Annex A): "#address-cells" Standard property name to define the package’s address format. ... In a package with a "decode-unit" method, a missing "#address-cells" property signifies that the number of address cells is two. "#size-cells" Standard property name to define the package’s address size format. ... A missing "#size-cells" property signifies the default value of one. I can't speak for FDT but it isn't completely unreasonable for a guest parsing a DT without these properties to assume these default values. ATB, Mark.
On Tue, Sep 4, 2018 at 11:59 PM Mark Cave-Ayland <mark.cave-ayland@ilande.co.uk> wrote: > > On 05/09/18 02:55, Frank Rowand wrote: > > > On 08/30/18 12:05, Rob Herring wrote: > >> Only some old OpenFirmware implementations rely on default sizes. Any > >> FDT and modern implementation should have explicit properties. Make the > >> OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside > >> users. > >> > >> This also gets us one step closer to removing the asm/prom.h dependency on > >> Sparc. > > Just for the record: you say "any FDT and modern implementation should > have explicit properties", however the default values of these > properties when missing are clearly defined in the IEEE-1275 > specification (Annex A): The spec may define defaults, but best practice is to be explicit. For FDT, dtc doesn't allow defaults (and never has). No arch added in the last 10 years has relied on the defaults. > "#address-cells" > Standard property name to define the package’s address format. > ... > In a package with a "decode-unit" method, a missing "#address-cells" > property signifies that the number of > address cells is two. So only Sparc is compliant as the default for everyone else is 1. > "#size-cells" > Standard property name to define the package’s address size format. > ... > A missing "#size-cells" property signifies the default value of one. > > I can't speak for FDT but it isn't completely unreasonable for a guest > parsing a DT without these properties to assume these default values. I'm not removing the defaults. Just not allowing for code outside of drivers/of/ to use the defaults. Rob
On 05/09/18 13:12, Rob Herring wrote: > On Tue, Sep 4, 2018 at 11:59 PM Mark Cave-Ayland > <mark.cave-ayland@ilande.co.uk> wrote: >> >> On 05/09/18 02:55, Frank Rowand wrote: >> >>> On 08/30/18 12:05, Rob Herring wrote: >>>> Only some old OpenFirmware implementations rely on default sizes. Any >>>> FDT and modern implementation should have explicit properties. Make the >>>> OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside >>>> users. >>>> >>>> This also gets us one step closer to removing the asm/prom.h dependency on >>>> Sparc. >> >> Just for the record: you say "any FDT and modern implementation should >> have explicit properties", however the default values of these >> properties when missing are clearly defined in the IEEE-1275 >> specification (Annex A): > > The spec may define defaults, but best practice is to be explicit. For > FDT, dtc doesn't allow defaults (and never has). No arch added in the > last 10 years has relied on the defaults. > >> "#address-cells" >> Standard property name to define the package’s address format. >> ... >> In a package with a "decode-unit" method, a missing "#address-cells" >> property signifies that the number of >> address cells is two. > > So only Sparc is compliant as the default for everyone else is 1. > >> "#size-cells" >> Standard property name to define the package’s address size format. >> ... >> A missing "#size-cells" property signifies the default value of one. >> >> I can't speak for FDT but it isn't completely unreasonable for a guest >> parsing a DT without these properties to assume these default values. > > I'm not removing the defaults. Just not allowing for code outside of > drivers/of/ to use the defaults. Got it - if dtc requires explicit properties, then I agree there should be no issues with this patch. Sorry for the noise. ATB, Mark.
diff --git a/arch/sparc/include/asm/prom.h b/arch/sparc/include/asm/prom.h index d955c8df62d6..1902db27ff4b 100644 --- a/arch/sparc/include/asm/prom.h +++ b/arch/sparc/include/asm/prom.h @@ -24,9 +24,6 @@ #include <linux/atomic.h> #include <linux/irqdomain.h> -#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 2 -#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1 - #define of_compat_cmp(s1, s2, l) strncmp((s1), (s2), (l)) #define of_prop_cmp(s1, s2) strcasecmp((s1), (s2)) #define of_node_cmp(s1, s2) strcmp((s1), (s2)) diff --git a/drivers/of/of_private.h b/drivers/of/of_private.h index 216175d11d3d..5d1567025358 100644 --- a/drivers/of/of_private.h +++ b/drivers/of/of_private.h @@ -27,6 +27,14 @@ struct alias_prop { char stem[0]; }; +#if defined(CONFIG_SPARC) +#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 2 +#else +#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 1 +#endif + +#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1 + extern struct mutex of_mutex; extern struct list_head aliases_lookup; extern struct kset *of_kset; diff --git a/include/linux/of.h b/include/linux/of.h index 506beca9588d..49d85f670c75 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -247,12 +247,6 @@ static inline unsigned long of_read_ulong(const __be32 *cell, int size) #include <asm/prom.h> #endif -/* Default #address and #size cells. Allow arch asm/prom.h to override */ -#if !defined(OF_ROOT_NODE_ADDR_CELLS_DEFAULT) -#define OF_ROOT_NODE_ADDR_CELLS_DEFAULT 1 -#define OF_ROOT_NODE_SIZE_CELLS_DEFAULT 1 -#endif - #define OF_IS_DYNAMIC(x) test_bit(OF_DYNAMIC, &x->_flags) #define OF_MARK_DYNAMIC(x) set_bit(OF_DYNAMIC, &x->_flags)
Only some old OpenFirmware implementations rely on default sizes. Any FDT and modern implementation should have explicit properties. Make the OF_ROOT_NODE_*_CELLS_DEFAULT defines private so we don't get any outside users. This also gets us one step closer to removing the asm/prom.h dependency on Sparc. Cc: "David S. Miller" <davem@davemloft.net> Cc: Frank Rowand <frowand.list@gmail.com> Cc: sparclinux@vger.kernel.org Signed-off-by: Rob Herring <robh@kernel.org> --- arch/sparc/include/asm/prom.h | 3 --- drivers/of/of_private.h | 8 ++++++++ include/linux/of.h | 6 ------ 3 files changed, 8 insertions(+), 9 deletions(-)