| Message ID | 20210811102423.28908-20-wei.chen@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add device tree based NUMA support to Arm64 | expand |
Hi Wei, On 11/08/2021 11:24, Wei Chen wrote: > In later patches, we will parse CPU and memory NUMA information > from device tree. FDT is using device type property to indicate > CPU nodes and memory nodes. So we introduce fdt_node_check_type > in this patch to avoid redundant code in subsequent patches. > > Signed-off-by: Wei Chen <wei.chen@arm.com> > --- > xen/common/libfdt/fdt_ro.c | 15 +++++++++++++++ > xen/include/xen/libfdt/libfdt.h | 25 +++++++++++++++++++++++++ This is meant to be a verbatim copy of libfdt. So I am not entirely in favor of adding a new function therefore without been upstreamed to libfdt first. > 2 files changed, 40 insertions(+) > > diff --git a/xen/common/libfdt/fdt_ro.c b/xen/common/libfdt/fdt_ro.c > index 36f9b480d1..ae7794d870 100644 > --- a/xen/common/libfdt/fdt_ro.c > +++ b/xen/common/libfdt/fdt_ro.c > @@ -545,6 +545,21 @@ int fdt_node_check_compatible(const void *fdt, int nodeoffset, > return 1; > } > > +int fdt_node_check_type(const void *fdt, int nodeoffset, > + const char *type) > +{ > + const void *prop; > + int len; > + > + prop = fdt_getprop(fdt, nodeoffset, "device_type", &len); > + if (!prop) > + return len; > + if (fdt_stringlist_contains(prop, len, type)) The "device_type" is not a list of string. So I am a bit confused why you are using this helper. Shouldn't we simply check that the property value and type matches? > + return 0; > + else > + return 1; > +} > + > int fdt_node_offset_by_compatible(const void *fdt, int startoffset, > const char *compatible) > { > diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h > index 7c75688a39..7e4930dbcd 100644 > --- a/xen/include/xen/libfdt/libfdt.h > +++ b/xen/include/xen/libfdt/libfdt.h > @@ -799,6 +799,31 @@ int fdt_node_offset_by_phandle(const void *fdt, uint32_t phandle); > int fdt_node_check_compatible(const void *fdt, int nodeoffset, > const char *compatible); > > +/** > + * fdt_node_check_type: check a node's device_type property > + * @fdt: pointer to the device tree blob > + * @nodeoffset: offset of a tree node > + * @type: string to match against > + * > + * > + * fdt_node_check_type() returns 0 if the given node contains a 'device_type' > + * property with the given string as one of its elements, it returns non-zero > + * otherwise, or on error. > + * > + * returns: > + * 0, if the node has a 'device_type' property listing the given string > + * 1, if the node has a 'device_type' property, but it does not list > + * the given string > + * -FDT_ERR_NOTFOUND, if the given node has no 'device_type' property > + * -FDT_ERR_BADOFFSET, if nodeoffset does not refer to a BEGIN_NODE tag > + * -FDT_ERR_BADMAGIC, > + * -FDT_ERR_BADVERSION, > + * -FDT_ERR_BADSTATE, > + * -FDT_ERR_BADSTRUCTURE, standard meanings > + */ > +int fdt_node_check_type(const void *fdt, int nodeoffset, > + const char *type); > + > /** > * fdt_node_offset_by_compatible - find nodes with a given 'compatible' value > * @fdt: pointer to the device tree blob > Cheers,
Hi Julien, > -----Original Message----- > From: Julien Grall <julien@xen.org> > Sent: 2021年8月25日 21:39 > To: Wei Chen <Wei.Chen@arm.com>; xen-devel@lists.xenproject.org; > sstabellini@kernel.org > Cc: Bertrand Marquis <Bertrand.Marquis@arm.com>; Jan Beulich > <jbeulich@suse.com> > Subject: Re: [XEN RFC PATCH 19/40] xen: fdt: Introduce a helper to check > fdt node type > > Hi Wei, > > On 11/08/2021 11:24, Wei Chen wrote: > > In later patches, we will parse CPU and memory NUMA information > > from device tree. FDT is using device type property to indicate > > CPU nodes and memory nodes. So we introduce fdt_node_check_type > > in this patch to avoid redundant code in subsequent patches. > > > > Signed-off-by: Wei Chen <wei.chen@arm.com> > > --- > > xen/common/libfdt/fdt_ro.c | 15 +++++++++++++++ > > xen/include/xen/libfdt/libfdt.h | 25 +++++++++++++++++++++++++ > > This is meant to be a verbatim copy of libfdt. So I am not entirely in > favor of adding a new function therefore without been upstreamed to > libfdt first. > Oh, if we need to upstream this change in libfdt. I think I'd better to remove this change in libfdt. Because we can implement type checking in other place, and I don't want to introduce a dependency on external repo upstream in this series. > > 2 files changed, 40 insertions(+) > > > > diff --git a/xen/common/libfdt/fdt_ro.c b/xen/common/libfdt/fdt_ro.c > > index 36f9b480d1..ae7794d870 100644 > > --- a/xen/common/libfdt/fdt_ro.c > > +++ b/xen/common/libfdt/fdt_ro.c > > @@ -545,6 +545,21 @@ int fdt_node_check_compatible(const void *fdt, int > nodeoffset, > > return 1; > > } > > > > +int fdt_node_check_type(const void *fdt, int nodeoffset, > > + const char *type) > > +{ > > + const void *prop; > > + int len; > > + > > + prop = fdt_getprop(fdt, nodeoffset, "device_type", &len); > > + if (!prop) > > + return len; > > + if (fdt_stringlist_contains(prop, len, type)) > > The "device_type" is not a list of string. So I am a bit confused why > you are using this helper. Shouldn't we simply check that the property > value and type matches? > Yes, I think you're right. This function was based on the modification of fdt_node_check_compatible, and I forgot to replace fdt_stringlist_contains. And, as I reply above, we can simply the check. And I will implement it out of libfdt. > > + return 0; > > + else > > + return 1; > > +} > > + > > int fdt_node_offset_by_compatible(const void *fdt, int startoffset, > > const char *compatible) > > { > > diff --git a/xen/include/xen/libfdt/libfdt.h > b/xen/include/xen/libfdt/libfdt.h > > index 7c75688a39..7e4930dbcd 100644 > > --- a/xen/include/xen/libfdt/libfdt.h > > +++ b/xen/include/xen/libfdt/libfdt.h > > @@ -799,6 +799,31 @@ int fdt_node_offset_by_phandle(const void *fdt, > uint32_t phandle); > > int fdt_node_check_compatible(const void *fdt, int nodeoffset, > > const char *compatible); > > > > +/** > > + * fdt_node_check_type: check a node's device_type property > > + * @fdt: pointer to the device tree blob > > + * @nodeoffset: offset of a tree node > > + * @type: string to match against > > + * > > + * > > + * fdt_node_check_type() returns 0 if the given node contains a > 'device_type' > > + * property with the given string as one of its elements, it returns > non-zero > > + * otherwise, or on error. > > + * > > + * returns: > > + * 0, if the node has a 'device_type' property listing the given string > > + * 1, if the node has a 'device_type' property, but it does not list > > + * the given string > > + * -FDT_ERR_NOTFOUND, if the given node has no 'device_type' property > > + * -FDT_ERR_BADOFFSET, if nodeoffset does not refer to a > BEGIN_NODE tag > > + * -FDT_ERR_BADMAGIC, > > + * -FDT_ERR_BADVERSION, > > + * -FDT_ERR_BADSTATE, > > + * -FDT_ERR_BADSTRUCTURE, standard meanings > > + */ > > +int fdt_node_check_type(const void *fdt, int nodeoffset, > > + const char *type); > > + > > /** > > * fdt_node_offset_by_compatible - find nodes with a given > 'compatible' value > > * @fdt: pointer to the device tree blob > > > > Cheers, > > -- > Julien Grall
diff --git a/xen/common/libfdt/fdt_ro.c b/xen/common/libfdt/fdt_ro.c index 36f9b480d1..ae7794d870 100644 --- a/xen/common/libfdt/fdt_ro.c +++ b/xen/common/libfdt/fdt_ro.c @@ -545,6 +545,21 @@ int fdt_node_check_compatible(const void *fdt, int nodeoffset, return 1; } +int fdt_node_check_type(const void *fdt, int nodeoffset, + const char *type) +{ + const void *prop; + int len; + + prop = fdt_getprop(fdt, nodeoffset, "device_type", &len); + if (!prop) + return len; + if (fdt_stringlist_contains(prop, len, type)) + return 0; + else + return 1; +} + int fdt_node_offset_by_compatible(const void *fdt, int startoffset, const char *compatible) { diff --git a/xen/include/xen/libfdt/libfdt.h b/xen/include/xen/libfdt/libfdt.h index 7c75688a39..7e4930dbcd 100644 --- a/xen/include/xen/libfdt/libfdt.h +++ b/xen/include/xen/libfdt/libfdt.h @@ -799,6 +799,31 @@ int fdt_node_offset_by_phandle(const void *fdt, uint32_t phandle); int fdt_node_check_compatible(const void *fdt, int nodeoffset, const char *compatible); +/** + * fdt_node_check_type: check a node's device_type property + * @fdt: pointer to the device tree blob + * @nodeoffset: offset of a tree node + * @type: string to match against + * + * + * fdt_node_check_type() returns 0 if the given node contains a 'device_type' + * property with the given string as one of its elements, it returns non-zero + * otherwise, or on error. + * + * returns: + * 0, if the node has a 'device_type' property listing the given string + * 1, if the node has a 'device_type' property, but it does not list + * the given string + * -FDT_ERR_NOTFOUND, if the given node has no 'device_type' property + * -FDT_ERR_BADOFFSET, if nodeoffset does not refer to a BEGIN_NODE tag + * -FDT_ERR_BADMAGIC, + * -FDT_ERR_BADVERSION, + * -FDT_ERR_BADSTATE, + * -FDT_ERR_BADSTRUCTURE, standard meanings + */ +int fdt_node_check_type(const void *fdt, int nodeoffset, + const char *type); + /** * fdt_node_offset_by_compatible - find nodes with a given 'compatible' value * @fdt: pointer to the device tree blob
In later patches, we will parse CPU and memory NUMA information from device tree. FDT is using device type property to indicate CPU nodes and memory nodes. So we introduce fdt_node_check_type in this patch to avoid redundant code in subsequent patches. Signed-off-by: Wei Chen <wei.chen@arm.com> --- xen/common/libfdt/fdt_ro.c | 15 +++++++++++++++ xen/include/xen/libfdt/libfdt.h | 25 +++++++++++++++++++++++++ 2 files changed, 40 insertions(+)