| Message ID | 20230803104438.24720-3-dpsmith@apertussolutions.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Rebranding dom0less to hyperlaunch part 1 | expand |
Hi Daniel, On 03/08/2023 12:44, Daniel P. Smith wrote: > > > This refactors reusable code from Arm's bootfdt.c and device-tree.h that is > general fdt handling code. The Kconfig parameter CORE_DEVICE_TREE is IIUC, you just try to untangle the code for fdt from dt (unflattened). CORE_DEVICE_TREE is a bit ambiguous in my opinion, so maybe just CONFIG_FDT, especially since you use it to guard libfdt/? > introduced for when the ability of parsing DTB files is needed by a capability > such as hyperlaunch. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > --- > MAINTAINERS | 8 +- > xen/arch/arm/bootfdt.c | 141 +--------------------------- > xen/arch/arm/domain_build.c | 1 + > xen/arch/arm/include/asm/setup.h | 6 -- > xen/common/Kconfig | 4 + > xen/common/Makefile | 3 +- > xen/common/fdt.c | 153 +++++++++++++++++++++++++++++++ > xen/include/xen/device_tree.h | 50 +--------- > xen/include/xen/fdt.h | 79 ++++++++++++++++ > 9 files changed, 246 insertions(+), 199 deletions(-) > create mode 100644 xen/common/fdt.c > create mode 100644 xen/include/xen/fdt.h > [...] > diff --git a/xen/common/fdt.c b/xen/common/fdt.c > new file mode 100644 > index 0000000000..8d7acaaa43 > --- /dev/null > +++ b/xen/common/fdt.c > @@ -0,0 +1,153 @@ > +/* SPDX missing for a new file > + * Flattened Device Tree > + * > + * Copyright (C) 2012-2014 Citrix Systems, Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ > +#include <xen/fdt.h> > +#include <xen/init.h> > +#include <xen/lib.h> > +#include <xen/libfdt/libfdt.h> > +#include <xen/types.h> > + > +bool __init device_tree_node_matches( > + const void *fdt, int node, const char *match) FWICS, this code style (that you use for every function in this patch) is rather uncommon in Xen so maybe better to follow the generic style. Also, this would avoid changing the style of functions/prototypes you move. [...] > diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h > index 1d79e23b28..82db38b140 100644 > --- a/xen/include/xen/device_tree.h > +++ b/xen/include/xen/device_tree.h > @@ -14,13 +14,12 @@ > #include <asm/device.h> > #include <public/xen.h> > #include <public/device_tree_defs.h> > +#include <xen/fdt.h> > #include <xen/kernel.h> > #include <xen/string.h> > #include <xen/types.h> > #include <xen/list.h> > > -#define DEVICE_TREE_MAX_DEPTH 16 > - > /* > * Struct used for matching a device > */ > @@ -159,17 +158,8 @@ struct dt_raw_irq { > u32 specifier[DT_MAX_IRQ_SPEC]; > }; > > -typedef int (*device_tree_node_func)(const void *fdt, > - int node, const char *name, int depth, > - u32 address_cells, u32 size_cells, > - void *data); > - > extern const void *device_tree_flattened; > > -int device_tree_for_each_node(const void *fdt, int node, > - device_tree_node_func func, > - void *data); > - > /** > * dt_unflatten_host_device_tree - Unflatten the host device tree > * > @@ -214,14 +204,6 @@ extern const struct dt_device_node *dt_interrupt_controller; > struct dt_device_node * > dt_find_interrupt_controller(const struct dt_device_match *matches); > > -#define dt_prop_cmp(s1, s2) strcmp((s1), (s2)) > -#define dt_node_cmp(s1, s2) strcasecmp((s1), (s2)) > -#define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2)) > - > -/* Default #address and #size cells */ > -#define DT_ROOT_NODE_ADDR_CELLS_DEFAULT 2 > -#define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1 > - > #define dt_for_each_property_node(dn, pp) \ > for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next ) > > @@ -231,16 +213,6 @@ dt_find_interrupt_controller(const struct dt_device_match *matches); > #define dt_for_each_child_node(dt, dn) \ > for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling ) > > -/* Helper to read a big number; size is in cells (not bytes) */ > -static inline u64 dt_read_number(const __be32 *cell, int size) > -{ > - u64 r = 0; > - > - while ( size-- ) > - r = (r << 32) | be32_to_cpu(*(cell++)); > - return r; > -} > - > /* Wrapper for dt_read_number() to return paddr_t (instead of uint64_t) */ > static inline paddr_t dt_read_paddr(const __be32 *cell, int size) Shouldn't this also go to fdt.h as it is just a wrapper for dt_read_number() you moved? ~Michal
On 8/3/23 08:48, Michal Orzel wrote: > Hi Daniel, > > On 03/08/2023 12:44, Daniel P. Smith wrote: >> >> >> This refactors reusable code from Arm's bootfdt.c and device-tree.h that is >> general fdt handling code. The Kconfig parameter CORE_DEVICE_TREE is > IIUC, you just try to untangle the code for fdt from dt (unflattened). CORE_DEVICE_TREE is > a bit ambiguous in my opinion, so maybe just CONFIG_FDT, especially since you use it to guard libfdt/? Untangle is a very good way to phrase it. ( ^_^) Yes, I don't see any reason why CONFIG_FDT could be used instead. >> introduced for when the ability of parsing DTB files is needed by a capability >> such as hyperlaunch. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> --- >> MAINTAINERS | 8 +- >> xen/arch/arm/bootfdt.c | 141 +--------------------------- >> xen/arch/arm/domain_build.c | 1 + >> xen/arch/arm/include/asm/setup.h | 6 -- >> xen/common/Kconfig | 4 + >> xen/common/Makefile | 3 +- >> xen/common/fdt.c | 153 +++++++++++++++++++++++++++++++ >> xen/include/xen/device_tree.h | 50 +--------- >> xen/include/xen/fdt.h | 79 ++++++++++++++++ >> 9 files changed, 246 insertions(+), 199 deletions(-) >> create mode 100644 xen/common/fdt.c >> create mode 100644 xen/include/xen/fdt.h >> > [...] > >> diff --git a/xen/common/fdt.c b/xen/common/fdt.c >> new file mode 100644 >> index 0000000000..8d7acaaa43 >> --- /dev/null >> +++ b/xen/common/fdt.c >> @@ -0,0 +1,153 @@ >> +/* > SPDX missing for a new file Ack. >> + * Flattened Device Tree >> + * >> + * Copyright (C) 2012-2014 Citrix Systems, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ >> +#include <xen/fdt.h> >> +#include <xen/init.h> >> +#include <xen/lib.h> >> +#include <xen/libfdt/libfdt.h> >> +#include <xen/types.h> >> + >> +bool __init device_tree_node_matches( >> + const void *fdt, int node, const char *match) > FWICS, this code style (that you use for every function in this patch) is rather uncommon in Xen so maybe better to follow the generic style. > Also, this would avoid changing the style of functions/prototypes you move. Um, my understanding is that this is the official style for Xen function definitions, unless it has changed due to MISRA, and that anytime a function definition is changed that the style should be corrected to this form. Another coding style change I have been pinged on in the past that I surprised I have not been pinged on for this series is allowing the use of the short hand notation for ints, e.g. u8, u16, and u32. > [...] > >> diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h >> index 1d79e23b28..82db38b140 100644 >> --- a/xen/include/xen/device_tree.h >> +++ b/xen/include/xen/device_tree.h >> @@ -14,13 +14,12 @@ >> #include <asm/device.h> >> #include <public/xen.h> >> #include <public/device_tree_defs.h> >> +#include <xen/fdt.h> >> #include <xen/kernel.h> >> #include <xen/string.h> >> #include <xen/types.h> >> #include <xen/list.h> >> >> -#define DEVICE_TREE_MAX_DEPTH 16 >> - >> /* >> * Struct used for matching a device >> */ >> @@ -159,17 +158,8 @@ struct dt_raw_irq { >> u32 specifier[DT_MAX_IRQ_SPEC]; >> }; >> >> -typedef int (*device_tree_node_func)(const void *fdt, >> - int node, const char *name, int depth, >> - u32 address_cells, u32 size_cells, >> - void *data); >> - >> extern const void *device_tree_flattened; >> >> -int device_tree_for_each_node(const void *fdt, int node, >> - device_tree_node_func func, >> - void *data); >> - >> /** >> * dt_unflatten_host_device_tree - Unflatten the host device tree >> * >> @@ -214,14 +204,6 @@ extern const struct dt_device_node *dt_interrupt_controller; >> struct dt_device_node * >> dt_find_interrupt_controller(const struct dt_device_match *matches); >> >> -#define dt_prop_cmp(s1, s2) strcmp((s1), (s2)) >> -#define dt_node_cmp(s1, s2) strcasecmp((s1), (s2)) >> -#define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2)) >> - >> -/* Default #address and #size cells */ >> -#define DT_ROOT_NODE_ADDR_CELLS_DEFAULT 2 >> -#define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1 >> - >> #define dt_for_each_property_node(dn, pp) \ >> for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next ) >> >> @@ -231,16 +213,6 @@ dt_find_interrupt_controller(const struct dt_device_match *matches); >> #define dt_for_each_child_node(dt, dn) \ >> for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling ) >> >> -/* Helper to read a big number; size is in cells (not bytes) */ >> -static inline u64 dt_read_number(const __be32 *cell, int size) >> -{ >> - u64 r = 0; >> - >> - while ( size-- ) >> - r = (r << 32) | be32_to_cpu(*(cell++)); >> - return r; >> -} >> - >> /* Wrapper for dt_read_number() to return paddr_t (instead of uint64_t) */ >> static inline paddr_t dt_read_paddr(const __be32 *cell, int size) > Shouldn't this also go to fdt.h as it is just a wrapper for dt_read_number() you moved? This patch came from hyperlaunch v1 series were I only moved what I needed, not necessarily everything that could reasonably be moved. I will gladly incorporate any additional macros, inlines, and functions that is felt is general enough to be included. v/r, dps
Hi Daniel, [...] > diff --git a/xen/common/fdt.c b/xen/common/fdt.c > new file mode 100644 > index 0000000000..8d7acaaa43 > --- /dev/null > +++ b/xen/common/fdt.c > @@ -0,0 +1,153 @@ > +/* > + * Flattened Device Tree > + * > + * Copyright (C) 2012-2014 Citrix Systems, Inc. > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + */ Can you add an empty line here, I think it improves readability, I know that some other headers don’t add it unfortunately > +#include <xen/fdt.h> > +#include <xen/init.h> > +#include <xen/lib.h> > +#include <xen/libfdt/libfdt.h> > +#include <xen/types.h> > + > +bool __init device_tree_node_matches( > + const void *fdt, int node, const char *match) > +{ [...] Empty line > +#ifndef __XEN_FDT_H__ > +#define __XEN_FDT_H__ > + > +#include <xen/init.h> > +#include <xen/libfdt/libfdt.h> > +#include <xen/types.h> For the new files, apart from Michal’s comments, if I remember correctly in the past I was asked to add these lines to the end of the file: /* * Local variables: * mode: C * c-file-style: "BSD" * c-basic-offset: 4 * indent-tabs-mode: nil * End: */ Regarding the coding style, I think it’s better to keep the style you’ve found in the original file, and change only some bits when the code is not following it. I know there is nothing enforcing parameters on the same line of the function definition at the moment, but it is how it’s done from the original file so I would stick with it. Regarding the u32/u64 types, maybe since you are moving the code it can be the occasion to convert them, but check with the maintainer before. I’ve also tested this serie and it works fine! I’m not leaving any tag because this patch is going to change anyway. Cheers, Luca
On 8/3/23 14:05, Luca Fancellu wrote: > Hi Daniel, > > [...] >> diff --git a/xen/common/fdt.c b/xen/common/fdt.c >> new file mode 100644 >> index 0000000000..8d7acaaa43 >> --- /dev/null >> +++ b/xen/common/fdt.c >> @@ -0,0 +1,153 @@ >> +/* >> + * Flattened Device Tree >> + * >> + * Copyright (C) 2012-2014 Citrix Systems, Inc. >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + */ > > Can you add an empty line here, I think it improves readability, I know that some other > headers don’t add it unfortunately Yah, that is no problem. >> +#include <xen/fdt.h> >> +#include <xen/init.h> >> +#include <xen/lib.h> >> +#include <xen/libfdt/libfdt.h> >> +#include <xen/types.h> >> + >> +bool __init device_tree_node_matches( >> + const void *fdt, int node, const char *match) >> +{ > [...] > > Empty line Ack. >> +#ifndef __XEN_FDT_H__ >> +#define __XEN_FDT_H__ >> + >> +#include <xen/init.h> >> +#include <xen/libfdt/libfdt.h> >> +#include <xen/types.h> > > For the new files, apart from Michal’s comments, if I remember correctly in the past I was asked > to add these lines to the end of the file: > > /* > * Local variables: > * mode: C > * c-file-style: "BSD" > * c-basic-offset: 4 > * indent-tabs-mode: nil > * End: > */ You are correct, I will get them added. > Regarding the coding style, I think it’s better to keep the style you’ve found in the original file, > and change only some bits when the code is not following it. > > I know there is nothing enforcing parameters on the same line of the function definition at the > moment, but it is how it’s done from the original file so I would stick with it. > > Regarding the u32/u64 types, maybe since you are moving the code it can be the occasion to > convert them, but check with the maintainer before. I can leave the main code as is, but I do think header decl's should be styled correctly as there is no need to have them churn in the future over purely style changes. > I’ve also tested this serie and it works fine! I’m not leaving any tag because this patch is going to > change anyway. No worries, thank you for taking the time to review the series. v/r, dps
> >> Regarding the coding style, I think it’s better to keep the style you’ve found in the original file, >> and change only some bits when the code is not following it. >> I know there is nothing enforcing parameters on the same line of the function definition at the >> moment, but it is how it’s done from the original file so I would stick with it. >> Regarding the u32/u64 types, maybe since you are moving the code it can be the occasion to >> convert them, but check with the maintainer before. > > I can leave the main code as is, but I do think header decl's should be styled correctly as there is no need to have them churn in the future over purely style changes. Uhm, when you say “styled correctly” do you mean as below? >>> +bool __init device_tree_node_matches( >>> + const void *fdt, int node, const char *match) >>> +{ If that’s the case, it seems to me that there is nothing like that in the codebase, in my work with clang-format I’ve configured it to match as much as I can the Xen style and this function would be formatted as the old style that it had. Can I ask you where did you find instruction to style in that way?
Hi Daniel, > -----Original Message----- > Subject: [PATCH v2 2/2] fdt: make fdt handling reusable across arch > > This refactors reusable code from Arm's bootfdt.c and device-tree.h that is > general fdt handling code. The Kconfig parameter CORE_DEVICE_TREE is > introduced for when the ability of parsing DTB files is needed by a capability > such as hyperlaunch. > > Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> As said yesterday, I tested this patch and can confirm this patch will not break any of the boards we used for our testing. So Tested-by: Henry Wang <Henry.Wang@arm.com> (But I saw there are some comments from Michal and Luca about this patch so I think these comments need to be addressed) Kind regards, Henry
On 8/3/23 16:37, Luca Fancellu wrote: > >> >>> Regarding the coding style, I think it’s better to keep the style you’ve found in the original file, >>> and change only some bits when the code is not following it. >>> I know there is nothing enforcing parameters on the same line of the function definition at the >>> moment, but it is how it’s done from the original file so I would stick with it. >>> Regarding the u32/u64 types, maybe since you are moving the code it can be the occasion to >>> convert them, but check with the maintainer before. >> >> I can leave the main code as is, but I do think header decl's should be styled correctly as there is no need to have them churn in the future over purely style changes. > > Uhm, when you say “styled correctly” do you mean as below? I am retracting that after going back to review my notes, see below. >>>> +bool __init device_tree_node_matches( >>>> + const void *fdt, int node, const char *match) >>>> +{ > > > If that’s the case, it seems to me that there is nothing like that in the codebase, > in my work with clang-format I’ve configured it to match as much as I can the > Xen style and this function would be formatted as the old style that it had. > > Can I ask you where did you find instruction to style in that way? > I went back to my old notes on styling to make sure I was correct. Turns out I was incorrect and do apologies. There are two accepted styles for function declarations[1] and the one I used in this patch was the one that I got a recommendation to use. I have gotten so use to that style, that I must have lost track the other was valid. As was pointed out elsewhere, I should use the form that the maintainer desires. As XSM maintainer, I use the one I submitted here and would ask for a style correct if someone submitted a patch using the form that is desired here. I will roll back the declaration styling for now and per the prior guidance[1], I will flip the linux-compat int over C spec fixed width int notation, unless there is objection to it happening during the move. [1] https://lists.xenproject.org/archives/html/xen-devel/2021-07/msg01133.html v/r dps
On 8/4/23 00:10, Henry Wang wrote: > Hi Daniel, Hey Henry! >> -----Original Message----- >> Subject: [PATCH v2 2/2] fdt: make fdt handling reusable across arch >> >> This refactors reusable code from Arm's bootfdt.c and device-tree.h that is >> general fdt handling code. The Kconfig parameter CORE_DEVICE_TREE is >> introduced for when the ability of parsing DTB files is needed by a capability >> such as hyperlaunch. >> >> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> > > As said yesterday, I tested this patch and can confirm this patch will not > break any of the boards we used for our testing. So > > Tested-by: Henry Wang <Henry.Wang@arm.com> Thank your for running it through your tests. > (But I saw there are some comments from Michal and Luca about this > patch so I think these comments need to be addressed) So far the changes are style and a few mechanical. While unlikely to cause a functional change that could break, probably best to hold off adding your Tb for now. v/r dps
Hi Daniel, > On Aug 9, 2023, at 05:05, Daniel P. Smith <dpsmith@apertussolutions.com> wrote: > > On 8/4/23 00:10, Henry Wang wrote: >> Hi Daniel, > > Hey Henry! > >>> -----Original Message----- >>> Subject: [PATCH v2 2/2] fdt: make fdt handling reusable across arch >>> >>> This refactors reusable code from Arm's bootfdt.c and device-tree.h that is >>> general fdt handling code. The Kconfig parameter CORE_DEVICE_TREE is >>> introduced for when the ability of parsing DTB files is needed by a capability >>> such as hyperlaunch. >>> >>> Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> >> As said yesterday, I tested this patch and can confirm this patch will not >> break any of the boards we used for our testing. So >> Tested-by: Henry Wang <Henry.Wang@arm.com> > > Thank your for running it through your tests. My pleasure. > >> (But I saw there are some comments from Michal and Luca about this >> patch so I think these comments need to be addressed) > > So far the changes are style and a few mechanical. While unlikely to cause a functional change that could break, probably best to hold off adding your Tb for now. Sorry for the late response, this email somehow fall through the cracks… I am ok to retain my T-by tag as the comments are only about styles and mechanical changes. Kind regards, Henry > > v/r > dps >
diff --git a/MAINTAINERS b/MAINTAINERS index 694412a961..ad684a2148 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -299,11 +299,13 @@ DEVICE TREE M: Stefano Stabellini <sstabellini@kernel.org> M: Julien Grall <julien@xen.org> S: Supported -F: xen/common/libfdt/ F: xen/common/device_tree.c -F: xen/include/xen/libfdt/ -F: xen/include/xen/device_tree.h +F: xen/common/fdt.c +F: xen/common/libfdt/ F: xen/drivers/passthrough/device_tree.c +F: xen/include/xen/device_tree.h +F: xen/include/xen/fdt.h +F: xen/include/xen/libfdt/ ECLAIR R: Simone Ballarin <simone.ballarin@bugseng.com> diff --git a/xen/arch/arm/bootfdt.c b/xen/arch/arm/bootfdt.c index 2673ad17a1..cdf4f17789 100644 --- a/xen/arch/arm/bootfdt.c +++ b/xen/arch/arm/bootfdt.c @@ -12,79 +12,11 @@ #include <xen/device_tree.h> #include <xen/lib.h> #include <xen/libfdt/libfdt-xen.h> +#include <xen/fdt.h> #include <xen/sort.h> #include <xsm/xsm.h> #include <asm/setup.h> -static bool __init device_tree_node_matches(const void *fdt, int node, - const char *match) -{ - const char *name; - size_t match_len; - - name = fdt_get_name(fdt, node, NULL); - match_len = strlen(match); - - /* Match both "match" and "match@..." patterns but not - "match-foo". */ - return strncmp(name, match, match_len) == 0 - && (name[match_len] == '@' || name[match_len] == '\0'); -} - -static bool __init device_tree_node_compatible(const void *fdt, int node, - const char *match) -{ - int len, l; - const void *prop; - - prop = fdt_getprop(fdt, node, "compatible", &len); - if ( prop == NULL ) - return false; - - while ( len > 0 ) { - if ( !dt_compat_cmp(prop, match) ) - return true; - l = strlen(prop) + 1; - prop += l; - len -= l; - } - - return false; -} - -void __init device_tree_get_reg(const __be32 **cell, uint32_t address_cells, - uint32_t size_cells, paddr_t *start, - paddr_t *size) -{ - uint64_t dt_start, dt_size; - - /* - * dt_next_cell will return uint64_t whereas paddr_t may not be 64-bit. - * Thus, there is an implicit cast from uint64_t to paddr_t. - */ - dt_start = dt_next_cell(address_cells, cell); - dt_size = dt_next_cell(size_cells, cell); - - if ( dt_start != (paddr_t)dt_start ) - { - printk("Physical address greater than max width supported\n"); - WARN(); - } - - if ( dt_size != (paddr_t)dt_size ) - { - printk("Physical size greater than max width supported\n"); - WARN(); - } - - /* - * Xen will truncate the address/size if it is greater than the maximum - * supported width and it will give an appropriate warning. - */ - *start = dt_start; - *size = dt_size; -} - static int __init device_tree_get_meminfo(const void *fdt, int node, const char *prop_name, u32 address_cells, u32 size_cells, @@ -135,77 +67,6 @@ static int __init device_tree_get_meminfo(const void *fdt, int node, return 0; } -u32 __init device_tree_get_u32(const void *fdt, int node, - const char *prop_name, u32 dflt) -{ - const struct fdt_property *prop; - - prop = fdt_get_property(fdt, node, prop_name, NULL); - if ( !prop || prop->len < sizeof(u32) ) - return dflt; - - return fdt32_to_cpu(*(uint32_t*)prop->data); -} - -/** - * device_tree_for_each_node - iterate over all device tree sub-nodes - * @fdt: flat device tree. - * @node: parent node to start the search from - * @func: function to call for each sub-node. - * @data: data to pass to @func. - * - * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored. - * - * Returns 0 if all nodes were iterated over successfully. If @func - * returns a value different from 0, that value is returned immediately. - */ -int __init device_tree_for_each_node(const void *fdt, int node, - device_tree_node_func func, - void *data) -{ - /* - * We only care about relative depth increments, assume depth of - * node is 0 for simplicity. - */ - int depth = 0; - const int first_node = node; - u32 address_cells[DEVICE_TREE_MAX_DEPTH]; - u32 size_cells[DEVICE_TREE_MAX_DEPTH]; - int ret; - - do { - const char *name = fdt_get_name(fdt, node, NULL); - u32 as, ss; - - if ( depth >= DEVICE_TREE_MAX_DEPTH ) - { - printk("Warning: device tree node `%s' is nested too deep\n", - name); - continue; - } - - as = depth > 0 ? address_cells[depth-1] : DT_ROOT_NODE_ADDR_CELLS_DEFAULT; - ss = depth > 0 ? size_cells[depth-1] : DT_ROOT_NODE_SIZE_CELLS_DEFAULT; - - address_cells[depth] = device_tree_get_u32(fdt, node, - "#address-cells", as); - size_cells[depth] = device_tree_get_u32(fdt, node, - "#size-cells", ss); - - /* skip the first node */ - if ( node != first_node ) - { - ret = func(fdt, node, name, depth, as, ss, data); - if ( ret != 0 ) - return ret; - } - - node = fdt_next_node(fdt, node, &depth); - } while ( node >= 0 && depth > 0 ); - - return 0; -} - static int __init process_memory_node(const void *fdt, int node, const char *name, int depth, u32 address_cells, u32 size_cells, diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c index 39b4ee03a5..7bd6c009d1 100644 --- a/xen/arch/arm/domain_build.c +++ b/xen/arch/arm/domain_build.c @@ -30,6 +30,7 @@ #include <asm/cpufeature.h> #include <asm/domain_build.h> #include <xen/event.h> +#include <xen/fdt.h> #include <xen/irq.h> #include <xen/grant_table.h> diff --git a/xen/arch/arm/include/asm/setup.h b/xen/arch/arm/include/asm/setup.h index 19dc637d55..ae2bfcc7e6 100644 --- a/xen/arch/arm/include/asm/setup.h +++ b/xen/arch/arm/include/asm/setup.h @@ -159,12 +159,6 @@ const char *boot_module_kind_as_string(bootmodule_kind kind); extern uint32_t hyp_traps_vector[]; void init_traps(void); -void device_tree_get_reg(const __be32 **cell, uint32_t address_cells, - uint32_t size_cells, paddr_t *start, paddr_t *size); - -u32 device_tree_get_u32(const void *fdt, int node, - const char *prop_name, u32 dflt); - int map_range_to_domain(const struct dt_device_node *dev, uint64_t addr, uint64_t len, void *data); diff --git a/xen/common/Kconfig b/xen/common/Kconfig index 0d248ab941..e2fdb3cbc3 100644 --- a/xen/common/Kconfig +++ b/xen/common/Kconfig @@ -38,8 +38,12 @@ config HAS_ALTERNATIVE config HAS_COMPAT bool +config CORE_DEVICE_TREE + bool + config HAS_DEVICE_TREE bool + select CORE_DEVICE_TREE config HAS_EX_TABLE bool diff --git a/xen/common/Makefile b/xen/common/Makefile index 46049eac35..fd3769e1c6 100644 --- a/xen/common/Makefile +++ b/xen/common/Makefile @@ -11,6 +11,7 @@ obj-y += domain.o obj-y += event_2l.o obj-y += event_channel.o obj-y += event_fifo.o +obj-$(CONFIG_CORE_DEVICE_TREE) += fdt.o obj-$(CONFIG_CRASH_DEBUG) += gdbstub.o obj-$(CONFIG_GRANT_TABLE) += grant_table.o obj-y += guestcopy.o @@ -75,7 +76,7 @@ obj-y += sched/ obj-$(CONFIG_UBSAN) += ubsan/ obj-$(CONFIG_NEEDS_LIBELF) += libelf/ -obj-$(CONFIG_HAS_DEVICE_TREE) += libfdt/ +obj-$(CONFIG_CORE_DEVICE_TREE) += libfdt/ CONF_FILE := $(if $(patsubst /%,,$(KCONFIG_CONFIG)),$(objtree)/)$(KCONFIG_CONFIG) $(obj)/config.gz: $(CONF_FILE) diff --git a/xen/common/fdt.c b/xen/common/fdt.c new file mode 100644 index 0000000000..8d7acaaa43 --- /dev/null +++ b/xen/common/fdt.c @@ -0,0 +1,153 @@ +/* + * Flattened Device Tree + * + * Copyright (C) 2012-2014 Citrix Systems, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#include <xen/fdt.h> +#include <xen/init.h> +#include <xen/lib.h> +#include <xen/libfdt/libfdt.h> +#include <xen/types.h> + +bool __init device_tree_node_matches( + const void *fdt, int node, const char *match) +{ + const char *name; + size_t match_len; + + name = fdt_get_name(fdt, node, NULL); + match_len = strlen(match); + + /* Match both "match" and "match@..." patterns but not + "match-foo". */ + return strncmp(name, match, match_len) == 0 + && (name[match_len] == '@' || name[match_len] == '\0'); +} + +bool __init device_tree_node_compatible( + const void *fdt, int node, const char *match) +{ + int len, l; + const void *prop; + + prop = fdt_getprop(fdt, node, "compatible", &len); + if ( prop == NULL ) + return false; + + while ( len > 0 ) { + if ( !dt_compat_cmp(prop, match) ) + return true; + l = strlen(prop) + 1; + prop += l; + len -= l; + } + + return false; +} + +void __init device_tree_get_reg( + const __be32 **cell, uint32_t address_cells, uint32_t size_cells, + paddr_t *start, paddr_t *size) +{ + uint64_t dt_start, dt_size; + + /* + * dt_next_cell will return uint64_t whereas paddr_t may not be 64-bit. + * Thus, there is an implicit cast from uint64_t to paddr_t. + */ + dt_start = dt_next_cell(address_cells, cell); + dt_size = dt_next_cell(size_cells, cell); + + if ( dt_start != (paddr_t)dt_start ) + { + printk("Physical address greater than max width supported\n"); + WARN(); + } + + if ( dt_size != (paddr_t)dt_size ) + { + printk("Physical size greater than max width supported\n"); + WARN(); + } + + /* + * Xen will truncate the address/size if it is greater than the maximum + * supported width and it will give an appropriate warning. + */ + *start = dt_start; + *size = dt_size; +} + +u32 __init device_tree_get_u32( + const void *fdt, int node, const char *prop_name, u32 dflt) +{ + const struct fdt_property *prop; + + prop = fdt_get_property(fdt, node, prop_name, NULL); + if ( !prop || prop->len < sizeof(u32) ) + return dflt; + + return fdt32_to_cpu(*(uint32_t*)prop->data); +} + +/** + * device_tree_for_each_node - iterate over all device tree sub-nodes + * @fdt: flat device tree. + * @node: parent node to start the search from + * @func: function to call for each sub-node. + * @data: data to pass to @func. + * + * Any nodes nested at DEVICE_TREE_MAX_DEPTH or deeper are ignored. + * + * Returns 0 if all nodes were iterated over successfully. If @func + * returns a value different from 0, that value is returned immediately. + */ +int __init device_tree_for_each_node( + const void *fdt, int node, device_tree_node_func func, void *data) +{ + /* + * We only care about relative depth increments, assume depth of + * node is 0 for simplicity. + */ + int depth = 0; + const int first_node = node; + u32 address_cells[DEVICE_TREE_MAX_DEPTH]; + u32 size_cells[DEVICE_TREE_MAX_DEPTH]; + int ret; + + do { + const char *name = fdt_get_name(fdt, node, NULL); + u32 as, ss; + + if ( depth >= DEVICE_TREE_MAX_DEPTH ) + { + printk("Warning: device tree node `%s' is nested too deep\n", + name); + continue; + } + + as = depth > 0 ? address_cells[depth-1] : DT_ROOT_NODE_ADDR_CELLS_DEFAULT; + ss = depth > 0 ? size_cells[depth-1] : DT_ROOT_NODE_SIZE_CELLS_DEFAULT; + + address_cells[depth] = device_tree_get_u32(fdt, node, + "#address-cells", as); + size_cells[depth] = device_tree_get_u32(fdt, node, + "#size-cells", ss); + + /* skip the first node */ + if ( node != first_node ) + { + ret = func(fdt, node, name, depth, as, ss, data); + if ( ret != 0 ) + return ret; + } + + node = fdt_next_node(fdt, node, &depth); + } while ( node >= 0 && depth > 0 ); + + return 0; +} diff --git a/xen/include/xen/device_tree.h b/xen/include/xen/device_tree.h index 1d79e23b28..82db38b140 100644 --- a/xen/include/xen/device_tree.h +++ b/xen/include/xen/device_tree.h @@ -14,13 +14,12 @@ #include <asm/device.h> #include <public/xen.h> #include <public/device_tree_defs.h> +#include <xen/fdt.h> #include <xen/kernel.h> #include <xen/string.h> #include <xen/types.h> #include <xen/list.h> -#define DEVICE_TREE_MAX_DEPTH 16 - /* * Struct used for matching a device */ @@ -159,17 +158,8 @@ struct dt_raw_irq { u32 specifier[DT_MAX_IRQ_SPEC]; }; -typedef int (*device_tree_node_func)(const void *fdt, - int node, const char *name, int depth, - u32 address_cells, u32 size_cells, - void *data); - extern const void *device_tree_flattened; -int device_tree_for_each_node(const void *fdt, int node, - device_tree_node_func func, - void *data); - /** * dt_unflatten_host_device_tree - Unflatten the host device tree * @@ -214,14 +204,6 @@ extern const struct dt_device_node *dt_interrupt_controller; struct dt_device_node * dt_find_interrupt_controller(const struct dt_device_match *matches); -#define dt_prop_cmp(s1, s2) strcmp((s1), (s2)) -#define dt_node_cmp(s1, s2) strcasecmp((s1), (s2)) -#define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2)) - -/* Default #address and #size cells */ -#define DT_ROOT_NODE_ADDR_CELLS_DEFAULT 2 -#define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1 - #define dt_for_each_property_node(dn, pp) \ for ( pp = (dn)->properties; (pp) != NULL; pp = (pp)->next ) @@ -231,16 +213,6 @@ dt_find_interrupt_controller(const struct dt_device_match *matches); #define dt_for_each_child_node(dt, dn) \ for ( dn = (dt)->child; (dn) != NULL; dn = (dn)->sibling ) -/* Helper to read a big number; size is in cells (not bytes) */ -static inline u64 dt_read_number(const __be32 *cell, int size) -{ - u64 r = 0; - - while ( size-- ) - r = (r << 32) | be32_to_cpu(*(cell++)); - return r; -} - /* Wrapper for dt_read_number() to return paddr_t (instead of uint64_t) */ static inline paddr_t dt_read_paddr(const __be32 *cell, int size) { @@ -268,26 +240,6 @@ static inline paddr_t dt_read_paddr(const __be32 *cell, int size) return r; } -/* Helper to convert a number of cells to bytes */ -static inline int dt_cells_to_size(int size) -{ - return (size * sizeof (u32)); -} - -/* Helper to convert a number of bytes to cells, rounds down */ -static inline int dt_size_to_cells(int bytes) -{ - return (bytes / sizeof(u32)); -} - -static inline u64 dt_next_cell(int s, const __be32 **cellp) -{ - const __be32 *p = *cellp; - - *cellp = p + s; - return dt_read_number(p, s); -} - static inline const char *dt_node_full_name(const struct dt_device_node *np) { return (np && np->full_name) ? np->full_name : "<no-node>"; diff --git a/xen/include/xen/fdt.h b/xen/include/xen/fdt.h new file mode 100644 index 0000000000..00ae6a5c8b --- /dev/null +++ b/xen/include/xen/fdt.h @@ -0,0 +1,79 @@ +/* + * Flattened Device Tree + * + * Copyright (C) 2012 Citrix Systems, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + */ +#ifndef __XEN_FDT_H__ +#define __XEN_FDT_H__ + +#include <xen/init.h> +#include <xen/libfdt/libfdt.h> +#include <xen/types.h> + +#define DEVICE_TREE_MAX_DEPTH 16 + +/* Default #address and #size cells */ +#define DT_ROOT_NODE_ADDR_CELLS_DEFAULT 2 +#define DT_ROOT_NODE_SIZE_CELLS_DEFAULT 1 + +#define dt_prop_cmp(s1, s2) strcmp((s1), (s2)) +#define dt_node_cmp(s1, s2) strcasecmp((s1), (s2)) +#define dt_compat_cmp(s1, s2) strcasecmp((s1), (s2)) + +/* Helper to read a big number; size is in cells (not bytes) */ +static inline u64 dt_read_number(const __be32 *cell, int size) +{ + u64 r = 0; + + while ( size-- ) + r = (r << 32) | be32_to_cpu(*(cell++)); + return r; +} + +/* Helper to convert a number of cells to bytes */ +static inline int dt_cells_to_size(int size) +{ + return (size * sizeof (u32)); +} + +/* Helper to convert a number of bytes to cells, rounds down */ +static inline int dt_size_to_cells(int bytes) +{ + return (bytes / sizeof(u32)); +} + +static inline u64 dt_next_cell(int s, const __be32 **cellp) +{ + const __be32 *p = *cellp; + + *cellp = p + s; + return dt_read_number(p, s); +} + + +bool device_tree_node_matches( + const void *fdt, int node, const char *match); + +bool device_tree_node_compatible( + const void *fdt, int node, const char *match); + +void device_tree_get_reg( + const __be32 **cell, u32 address_cells, u32 size_cells, u64 *start, + u64 *size); + +u32 device_tree_get_u32( + const void *fdt, int node, const char *prop_name, u32 dflt); + +typedef int (*device_tree_node_func)( + const void *fdt, int node, const char *name, int depth, u32 address_cells, + u32 size_cells, void *data); + +int device_tree_for_each_node( + const void *fdt, int node, device_tree_node_func func, void *data); + + +#endif /* __XEN_FDT_H__ */
This refactors reusable code from Arm's bootfdt.c and device-tree.h that is general fdt handling code. The Kconfig parameter CORE_DEVICE_TREE is introduced for when the ability of parsing DTB files is needed by a capability such as hyperlaunch. Signed-off-by: Daniel P. Smith <dpsmith@apertussolutions.com> --- MAINTAINERS | 8 +- xen/arch/arm/bootfdt.c | 141 +--------------------------- xen/arch/arm/domain_build.c | 1 + xen/arch/arm/include/asm/setup.h | 6 -- xen/common/Kconfig | 4 + xen/common/Makefile | 3 +- xen/common/fdt.c | 153 +++++++++++++++++++++++++++++++ xen/include/xen/device_tree.h | 50 +--------- xen/include/xen/fdt.h | 79 ++++++++++++++++ 9 files changed, 246 insertions(+), 199 deletions(-) create mode 100644 xen/common/fdt.c create mode 100644 xen/include/xen/fdt.h