| Message ID | 20170501151852.26670-9-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
On Mon, May 01, 2017 at 09:18:51AM -0600, Simon Glass wrote: > With live tree we need a struct device_node * to reference a node. With > the existing flat tree, we need an int offset. We need to unify these into > a single value which can represent both. > > Add an ofnode union for this and adjust existing code to move to this. > > Signed-off-by: Simon Glass <sjg@chromium.org> Reviewed-by: Tom Rini <trini@konsulko.com>
2017-05-02 0:18 GMT+09:00 Simon Glass <sjg@chromium.org>: > diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h > new file mode 100644 > index 0000000000..f952c989d2 > --- /dev/null > +++ b/include/dm/ofnode.h > @@ -0,0 +1,100 @@ > +/* > + * Copyright (c) 2017 Google, Inc > + * Written by Simon Glass <sjg@chromium.org> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#ifndef _DM_OFNODE_H > +#define _DM_OFNODE_H > + > +DECLARE_GLOBAL_DATA_PTR; Which line in this header references gd? > + > +struct fdtdec_phandle_args; > + > +/** > + * ofnode - reference to a device tree node > + * > + * This union can hold either a straightforward pointer to a struct device_node > + * in the live device tree, or an offset within the flat device tree. In the > + * latter case, the pointer value is just the integer offset within the flat DT. > + * > + * Thus we can reference nodes in both the live tree (once available) and the > + * flat tree (until then). Functions are available to translate between an > + * ofnode and either an offset or a struct device_node *. > + * > + * The reference can also hold a null offset, in which case the pointer value > + * here is (void *)-1. This corresponds to a struct device_node * value of > + * NULL, or an offset of -1. > + * > + * There is no ambiguity as to whether ofnode holds an offset or a node > + * pointer: when the live tree is active it holds a node pointer, otherwise it > + * holds an offset. The value itself does not need to be unique and in theory > + * the same value could point to a valid device node or a valid offset. We > + * could arrange for a unique value to be used (e.g. by making the pointer > + * point to an offset within the flat device tree in the case of an offset) but > + * this increases code size slightly due to the subtraction. Since it offers no > + * real benefit, the approach described here seems best. > + * > + * For now these points use constant types, since we don't allow writing > + * the DT. > + * > + * @np: Pointer to device node, used for live tree > + * @flat_ptr: Pointer into flat device tree, used for flat tree. Note that this > + * is not a really a pointer to a node: it is an offset value. See above. > + */ > +typedef union ofnode_union { > + const struct device_node *np; /* will be used for future live tree */ > + long of_offset; > +} ofnode; > + > +/** > + * ofnode_to_offset() - convert an ofnode to a flat DT offset > + * > + * This cannot be called if the reference contains a node pointer. > + * > + * @node: Reference containing offset (possibly invalid) > + * @return DT offset (can be -1) > + */ > +static inline int ofnode_to_offset(ofnode node) > +{ > + return node.of_offset; > +} > + > +/** > + * ofnode_valid() - check if an ofnode is valid > + * > + * @return true if the reference contains a valid ofnode, false if it is NULL > + */ > +static inline bool ofnode_valid(ofnode node) > +{ > + return node.of_offset != -1; > +} > + > +/** > + * offset_to_ofnode() - convert a DT offset to an ofnode > + * > + * @of_offset: DT offset (either valid, or -1) > + * @return reference to the associated DT offset > + */ > +static inline ofnode offset_to_ofnode(int of_offset) > +{ > + ofnode node; > + > + node.of_offset = of_offset; > + > + return node; > +} > + > +/** > + * ofnode_equal() - check if two references are equal > + * > + * @return true if equal, else false > + */ > +static inline bool ofnode_equal(ofnode ref1, ofnode ref2) > +{ > + /* We only need to compare the contents */ > + return ref1.of_offset == ref2.of_offset; > +} > + > +#endif
Hi Masahiro, On 10 May 2017 at 20:33, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > 2017-05-02 0:18 GMT+09:00 Simon Glass <sjg@chromium.org>: > >> diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h >> new file mode 100644 >> index 0000000000..f952c989d2 >> --- /dev/null >> +++ b/include/dm/ofnode.h >> @@ -0,0 +1,100 @@ >> +/* >> + * Copyright (c) 2017 Google, Inc >> + * Written by Simon Glass <sjg@chromium.org> >> + * >> + * SPDX-License-Identifier: GPL-2.0+ >> + */ >> + >> +#ifndef _DM_OFNODE_H >> +#define _DM_OFNODE_H >> + >> +DECLARE_GLOBAL_DATA_PTR; > > Which line in this header references gd? It is actually needed in dev.h when that is inlined. I originally had to access it here but can move it. Regards, Simon
Hi Simon, 2017-05-02 0:18 GMT+09:00 Simon Glass <sjg@chromium.org>: > With live tree we need a struct device_node * to reference a node. With > the existing flat tree, we need an int offset. We need to unify these into > a single value which can represent both. > > Add an ofnode union for this and adjust existing code to move to this. > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > Changes in v2: None > > drivers/core/device.c | 2 +- > drivers/core/root.c | 2 +- > include/dm.h | 1 + > include/dm/device.h | 14 +++++-- > include/dm/ofnode.h | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 113 insertions(+), 6 deletions(-) > create mode 100644 include/dm/ofnode.h > > diff --git a/drivers/core/device.c b/drivers/core/device.c > index 483f8368f7..2738685092 100644 > --- a/drivers/core/device.c > +++ b/drivers/core/device.c > @@ -60,7 +60,7 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv, > dev->platdata = platdata; > dev->driver_data = driver_data; > dev->name = name; > - dev->of_offset = of_offset; > + dev->node = offset_to_ofnode(of_offset); > dev->parent = parent; > dev->driver = drv; > dev->uclass = uc; > diff --git a/drivers/core/root.c b/drivers/core/root.c > index 4bb71f3cac..570b4d855f 100644 > --- a/drivers/core/root.c > +++ b/drivers/core/root.c > @@ -167,7 +167,7 @@ int dm_init(void) > if (ret) > return ret; > #if CONFIG_IS_ENABLED(OF_CONTROL) > - DM_ROOT_NON_CONST->of_offset = 0; > + DM_ROOT_NON_CONST->node = offset_to_ofnode(0); > #endif > ret = device_probe(DM_ROOT_NON_CONST); > if (ret) > diff --git a/include/dm.h b/include/dm.h > index 84f789d807..e634814d74 100644 > --- a/include/dm.h > +++ b/include/dm.h > @@ -7,6 +7,7 @@ > #ifndef _DM_H_ > #define _DM_H_ > > +#include <dm/ofnode.h> > #include <dm/device.h> > #include <dm/fdtaddr.h> > #include <dm/platdata.h> > diff --git a/include/dm/device.h b/include/dm/device.h > index 6c4aab6c96..89f9f53c43 100644 > --- a/include/dm/device.h > +++ b/include/dm/device.h > @@ -11,6 +11,7 @@ > #ifndef _DM_DEVICE_H > #define _DM_DEVICE_H > > +#include <dm/ofnode.h> > #include <dm/uclass-id.h> > #include <fdtdec.h> > #include <linker_lists.h> > @@ -94,7 +95,7 @@ enum { > * @platdata: Configuration data for this device > * @parent_platdata: The parent bus's configuration data for this device > * @uclass_platdata: The uclass's configuration data for this device > - * @of_offset: Device tree node offset for this device (- for none) > + * @node: Reference to device tree node for this device > * @driver_data: Driver data word for the entry that matched this device with > * its driver > * @parent: Parent of this device, or NULL for the top level device > @@ -120,7 +121,7 @@ struct udevice { > void *platdata; > void *parent_platdata; > void *uclass_platdata; > - int of_offset; > + ofnode node; > ulong driver_data; > struct udevice *parent; > void *priv; > @@ -149,12 +150,17 @@ struct udevice { > > static inline int dev_of_offset(const struct udevice *dev) > { > - return dev->of_offset; > + return ofnode_to_offset(dev->node); > } > > static inline void dev_set_of_offset(struct udevice *dev, int of_offset) > { > - dev->of_offset = of_offset; > + dev->node = offset_to_ofnode(of_offset); > +} > + > +static inline bool dev_has_of_node(struct udevice *dev) > +{ > + return ofnode_valid(dev->node); > } > > /** > diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h > new file mode 100644 > index 0000000000..f952c989d2 > --- /dev/null > +++ b/include/dm/ofnode.h > @@ -0,0 +1,100 @@ > +/* > + * Copyright (c) 2017 Google, Inc > + * Written by Simon Glass <sjg@chromium.org> > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#ifndef _DM_OFNODE_H > +#define _DM_OFNODE_H > + > +DECLARE_GLOBAL_DATA_PTR; > + > +struct fdtdec_phandle_args; > + > +/** > + * ofnode - reference to a device tree node > + * > + * This union can hold either a straightforward pointer to a struct device_node > + * in the live device tree, or an offset within the flat device tree. In the > + * latter case, the pointer value is just the integer offset within the flat DT. > + * > + * Thus we can reference nodes in both the live tree (once available) and the > + * flat tree (until then). Functions are available to translate between an > + * ofnode and either an offset or a struct device_node *. > + * > + * The reference can also hold a null offset, in which case the pointer value > + * here is (void *)-1. This corresponds to a struct device_node * value of > + * NULL, or an offset of -1. > + * > + * There is no ambiguity as to whether ofnode holds an offset or a node > + * pointer: when the live tree is active it holds a node pointer, otherwise it > + * holds an offset. The value itself does not need to be unique and in theory > + * the same value could point to a valid device node or a valid offset. We > + * could arrange for a unique value to be used (e.g. by making the pointer > + * point to an offset within the flat device tree in the case of an offset) but > + * this increases code size slightly due to the subtraction. Since it offers no > + * real benefit, the approach described here seems best. > + * > + * For now these points use constant types, since we don't allow writing > + * the DT. > + * > + * @np: Pointer to device node, used for live tree > + * @flat_ptr: Pointer into flat device tree, used for flat tree. Note that this > + * is not a really a pointer to a node: it is an offset value. See above. > + */ > +typedef union ofnode_union { > + const struct device_node *np; /* will be used for future live tree */ > + long of_offset; > +} ofnode; > + > +/** > + * ofnode_to_offset() - convert an ofnode to a flat DT offset > + * > + * This cannot be called if the reference contains a node pointer. > + * > + * @node: Reference containing offset (possibly invalid) > + * @return DT offset (can be -1) > + */ > +static inline int ofnode_to_offset(ofnode node) > +{ > + return node.of_offset; > +} > + > +/** > + * ofnode_valid() - check if an ofnode is valid > + * > + * @return true if the reference contains a valid ofnode, false if it is NULL > + */ > +static inline bool ofnode_valid(ofnode node) > +{ > + return node.of_offset != -1; > +} > + > +/** > + * offset_to_ofnode() - convert a DT offset to an ofnode > + * > + * @of_offset: DT offset (either valid, or -1) > + * @return reference to the associated DT offset > + */ > +static inline ofnode offset_to_ofnode(int of_offset) > +{ > + ofnode node; > + > + node.of_offset = of_offset; > + > + return node; > +} > + > +/** > + * ofnode_equal() - check if two references are equal > + * > + * @return true if equal, else false > + */ > +static inline bool ofnode_equal(ofnode ref1, ofnode ref2) > +{ > + /* We only need to compare the contents */ > + return ref1.of_offset == ref2.of_offset; > +} > + > +#endif When you add a new header, please make sure it is self-contained. You use bool for offset_toofnode() and ofnode_equal(). So you need to include <stdbool.h> from this header. (or, use "int" for the return type.)
Hi Masahiro, On 16 May 2017 at 04:35, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > Hi Simon, > > > 2017-05-02 0:18 GMT+09:00 Simon Glass <sjg@chromium.org>: > > With live tree we need a struct device_node * to reference a node. With > > the existing flat tree, we need an int offset. We need to unify these into > > a single value which can represent both. > > > > Add an ofnode union for this and adjust existing code to move to this. > > > > Signed-off-by: Simon Glass <sjg@chromium.org> > > --- > > > > Changes in v2: None > > > > drivers/core/device.c | 2 +- > > drivers/core/root.c | 2 +- > > include/dm.h | 1 + > > include/dm/device.h | 14 +++++-- > > include/dm/ofnode.h | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 5 files changed, 113 insertions(+), 6 deletions(-) > > create mode 100644 include/dm/ofnode.h > When you add a new header, please make sure it is self-contained. > > You use bool for offset_toofnode() and ofnode_equal(). > So you need to include <stdbool.h> from this header. > > (or, use "int" for the return type.) I'm wondering about this problem: In general we want to minimise the size of headers included by U-Boot. For something like ofnode which is used by several header files we end up including it many times, once per header. With dm.h we avoid this since the header is only included ones (and in the correct order). What is the best solution to this? Regards, Simon
Hi Simon, 2017-05-20 11:29 GMT+09:00 Simon Glass <sjg@chromium.org>: > Hi Masahiro, > > On 16 May 2017 at 04:35, Masahiro Yamada <yamada.masahiro@socionext.com> wrote: >> >> Hi Simon, >> >> >> 2017-05-02 0:18 GMT+09:00 Simon Glass <sjg@chromium.org>: >> > With live tree we need a struct device_node * to reference a node. With >> > the existing flat tree, we need an int offset. We need to unify these into >> > a single value which can represent both. >> > >> > Add an ofnode union for this and adjust existing code to move to this. >> > >> > Signed-off-by: Simon Glass <sjg@chromium.org> >> > --- >> > >> > Changes in v2: None >> > >> > drivers/core/device.c | 2 +- >> > drivers/core/root.c | 2 +- >> > include/dm.h | 1 + >> > include/dm/device.h | 14 +++++-- >> > include/dm/ofnode.h | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> > 5 files changed, 113 insertions(+), 6 deletions(-) >> > create mode 100644 include/dm/ofnode.h > >> When you add a new header, please make sure it is self-contained. >> >> You use bool for offset_toofnode() and ofnode_equal(). >> So you need to include <stdbool.h> from this header. >> >> (or, use "int" for the return type.) > > I'm wondering about this problem: > > In general we want to minimise the size of headers included by U-Boot. > For something like ofnode which is used by several header files we end > up including it many times, once per header. It is no problem if the header body is surrounded by an include guard. With include guards, each header is _parsed_ only once even if _included_ multiple times. > With dm.h we avoid this since the header is only included ones (and in > the correct order). Generally, we should not rely on a specific include order. Even if we compel drivers to include dm.h, each of dm/{device.h, platdata.h, uclass.h} should be self-contained. We usually sort includes alphabetically, but technically it should work in any order. It would be possible if each header is self-contained. > What is the best solution to this?
diff --git a/drivers/core/device.c b/drivers/core/device.c index 483f8368f7..2738685092 100644 --- a/drivers/core/device.c +++ b/drivers/core/device.c @@ -60,7 +60,7 @@ static int device_bind_common(struct udevice *parent, const struct driver *drv, dev->platdata = platdata; dev->driver_data = driver_data; dev->name = name; - dev->of_offset = of_offset; + dev->node = offset_to_ofnode(of_offset); dev->parent = parent; dev->driver = drv; dev->uclass = uc; diff --git a/drivers/core/root.c b/drivers/core/root.c index 4bb71f3cac..570b4d855f 100644 --- a/drivers/core/root.c +++ b/drivers/core/root.c @@ -167,7 +167,7 @@ int dm_init(void) if (ret) return ret; #if CONFIG_IS_ENABLED(OF_CONTROL) - DM_ROOT_NON_CONST->of_offset = 0; + DM_ROOT_NON_CONST->node = offset_to_ofnode(0); #endif ret = device_probe(DM_ROOT_NON_CONST); if (ret) diff --git a/include/dm.h b/include/dm.h index 84f789d807..e634814d74 100644 --- a/include/dm.h +++ b/include/dm.h @@ -7,6 +7,7 @@ #ifndef _DM_H_ #define _DM_H_ +#include <dm/ofnode.h> #include <dm/device.h> #include <dm/fdtaddr.h> #include <dm/platdata.h> diff --git a/include/dm/device.h b/include/dm/device.h index 6c4aab6c96..89f9f53c43 100644 --- a/include/dm/device.h +++ b/include/dm/device.h @@ -11,6 +11,7 @@ #ifndef _DM_DEVICE_H #define _DM_DEVICE_H +#include <dm/ofnode.h> #include <dm/uclass-id.h> #include <fdtdec.h> #include <linker_lists.h> @@ -94,7 +95,7 @@ enum { * @platdata: Configuration data for this device * @parent_platdata: The parent bus's configuration data for this device * @uclass_platdata: The uclass's configuration data for this device - * @of_offset: Device tree node offset for this device (- for none) + * @node: Reference to device tree node for this device * @driver_data: Driver data word for the entry that matched this device with * its driver * @parent: Parent of this device, or NULL for the top level device @@ -120,7 +121,7 @@ struct udevice { void *platdata; void *parent_platdata; void *uclass_platdata; - int of_offset; + ofnode node; ulong driver_data; struct udevice *parent; void *priv; @@ -149,12 +150,17 @@ struct udevice { static inline int dev_of_offset(const struct udevice *dev) { - return dev->of_offset; + return ofnode_to_offset(dev->node); } static inline void dev_set_of_offset(struct udevice *dev, int of_offset) { - dev->of_offset = of_offset; + dev->node = offset_to_ofnode(of_offset); +} + +static inline bool dev_has_of_node(struct udevice *dev) +{ + return ofnode_valid(dev->node); } /** diff --git a/include/dm/ofnode.h b/include/dm/ofnode.h new file mode 100644 index 0000000000..f952c989d2 --- /dev/null +++ b/include/dm/ofnode.h @@ -0,0 +1,100 @@ +/* + * Copyright (c) 2017 Google, Inc + * Written by Simon Glass <sjg@chromium.org> + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#ifndef _DM_OFNODE_H +#define _DM_OFNODE_H + +DECLARE_GLOBAL_DATA_PTR; + +struct fdtdec_phandle_args; + +/** + * ofnode - reference to a device tree node + * + * This union can hold either a straightforward pointer to a struct device_node + * in the live device tree, or an offset within the flat device tree. In the + * latter case, the pointer value is just the integer offset within the flat DT. + * + * Thus we can reference nodes in both the live tree (once available) and the + * flat tree (until then). Functions are available to translate between an + * ofnode and either an offset or a struct device_node *. + * + * The reference can also hold a null offset, in which case the pointer value + * here is (void *)-1. This corresponds to a struct device_node * value of + * NULL, or an offset of -1. + * + * There is no ambiguity as to whether ofnode holds an offset or a node + * pointer: when the live tree is active it holds a node pointer, otherwise it + * holds an offset. The value itself does not need to be unique and in theory + * the same value could point to a valid device node or a valid offset. We + * could arrange for a unique value to be used (e.g. by making the pointer + * point to an offset within the flat device tree in the case of an offset) but + * this increases code size slightly due to the subtraction. Since it offers no + * real benefit, the approach described here seems best. + * + * For now these points use constant types, since we don't allow writing + * the DT. + * + * @np: Pointer to device node, used for live tree + * @flat_ptr: Pointer into flat device tree, used for flat tree. Note that this + * is not a really a pointer to a node: it is an offset value. See above. + */ +typedef union ofnode_union { + const struct device_node *np; /* will be used for future live tree */ + long of_offset; +} ofnode; + +/** + * ofnode_to_offset() - convert an ofnode to a flat DT offset + * + * This cannot be called if the reference contains a node pointer. + * + * @node: Reference containing offset (possibly invalid) + * @return DT offset (can be -1) + */ +static inline int ofnode_to_offset(ofnode node) +{ + return node.of_offset; +} + +/** + * ofnode_valid() - check if an ofnode is valid + * + * @return true if the reference contains a valid ofnode, false if it is NULL + */ +static inline bool ofnode_valid(ofnode node) +{ + return node.of_offset != -1; +} + +/** + * offset_to_ofnode() - convert a DT offset to an ofnode + * + * @of_offset: DT offset (either valid, or -1) + * @return reference to the associated DT offset + */ +static inline ofnode offset_to_ofnode(int of_offset) +{ + ofnode node; + + node.of_offset = of_offset; + + return node; +} + +/** + * ofnode_equal() - check if two references are equal + * + * @return true if equal, else false + */ +static inline bool ofnode_equal(ofnode ref1, ofnode ref2) +{ + /* We only need to compare the contents */ + return ref1.of_offset == ref2.of_offset; +} + +#endif
With live tree we need a struct device_node * to reference a node. With the existing flat tree, we need an int offset. We need to unify these into a single value which can represent both. Add an ofnode union for this and adjust existing code to move to this. Signed-off-by: Simon Glass <sjg@chromium.org> --- Changes in v2: None drivers/core/device.c | 2 +- drivers/core/root.c | 2 +- include/dm.h | 1 + include/dm/device.h | 14 +++++-- include/dm/ofnode.h | 100 ++++++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 113 insertions(+), 6 deletions(-) create mode 100644 include/dm/ofnode.h