| Message ID | 20170405123706.6081-3-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Apr 5, 2017 at 9:32 AM, Nicholas Piggin <npiggin@gmail.com> wrote: > On Wed, 5 Apr 2017 08:35:06 -0500 > Rob Herring <robh+dt@kernel.org> wrote: > >> On Wed, Apr 5, 2017 at 7:37 AM, Nicholas Piggin <npiggin@gmail.com> wrote: >> > Introduce primitives for FDT parsing. These will be used for powerpc >> > cpufeatures node scanning, which has quite complex structure but should >> > be processed early. >> >> Have you looked at unflattening the FDT earlier? > > Hi, thanks for taking a look. Did you mean to trim the cc list? Ugg, no. I've added everyone back. > It may be possible but I'd like to avoid it if we can. There might > turn out to be some errata or feature that requires early setup. And > the current cpu feature parsing code does it with flat dt. Well, I'd like to avoid expanding usage of flat DT parsing in the kernel. But you could just put this function into arch/powerpc and I'd never see it, but I like that even less. Mainly, I just wanted to raise the point. Your argument works until you need that setup in assembly code, then you are in the situation that you need to either handle the setup in bootloader/firmware or have an simple way to determine that condition. Rob > >> > Signed-off-by: Nicholas Piggin <npiggin@gmail.com> >> > --- >> > drivers/of/fdt.c | 39 +++++++++++++++++++++++++++++++++++++++ >> > include/linux/of_fdt.h | 6 ++++++ >> > 2 files changed, 45 insertions(+) >> > >> > diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c >> > index e5ce4b59e162..a45854fe5156 100644 >> > --- a/drivers/of/fdt.c >> > +++ b/drivers/of/fdt.c >> > @@ -754,6 +754,37 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node, >> > } >> > >> > /** >> > + * of_scan_flat_dt_subnodes - scan sub-nodes of a node call callback on each. >> > + * @it: callback function >> > + * @data: context data pointer >> > + * >> > + * This function is used to scan sub-nodes of a node. >> > + */ >> > +int __init of_scan_flat_dt_subnodes(unsigned long node, >> > + int (*it)(unsigned long node, >> > + const char *uname, >> > + void *data), >> > + void *data) >> > +{ >> > + const void *blob = initial_boot_params; >> > + const char *pathp; >> > + int offset, rc = 0; >> > + >> > + offset = node; >> > + for (offset = fdt_first_subnode(blob, offset); >> > + offset >= 0 && !rc; >> > + offset = fdt_next_subnode(blob, offset)) { >> >> fdt_for_each_subnode() > > Got it. > >> >> > + >> > + pathp = fdt_get_name(blob, offset, NULL); >> > + if (*pathp == '/') >> > + pathp = kbasename(pathp); >> >> Seems a bit odd that you parse the name in this function. Perhaps the >> caller should do that, or if you want subnodes matching a certain >> name, then do the matching here. But you didn't copy me on the rest of >> the series, so I don't know how you are using this. > > Hmm, it was a while since writing that part. I guess I just copied > of_scan_flat_dt interface. > > Caller is in this patch: > > https://patchwork.ozlabs.org/patch/747262/ > > I'll include you in subsequent post if you prefer. > > Thanks, > Nick
On Wed, 2017-04-05 at 10:58 -0500, Rob Herring wrote: > Well, I'd like to avoid expanding usage of flat DT parsing in the > kernel. But you could just put this function into arch/powerpc and I'd > never see it, but I like that even less. Mainly, I just wanted to > raise the point. > > Your argument works until you need that setup in assembly code, then > you are in the situation that you need to either handle the setup in > bootloader/firmware or have an simple way to determine that condition. The main issue is that changing that is a very very invasive change in an extremely fragile and rather nasty area of code shared by 32 and 64- bit for which we don't even have easy access to all the machines to test with anymore :) It's probably not impossible, but it would delay the new cpu feature stuff that Nick is making by a lot, probably monthes, making it nearly impossible to get back into distros etc... So while it might be something to consider, I would definitely keep that as a separate unit of work to do later. Cheers, Ben.
On Thu, 06 Apr 2017 06:58:01 +1000 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > On Wed, 2017-04-05 at 10:58 -0500, Rob Herring wrote: > > Well, I'd like to avoid expanding usage of flat DT parsing in the > > kernel. But you could just put this function into arch/powerpc and I'd > > never see it, but I like that even less. Mainly, I just wanted to > > raise the point. > > > > Your argument works until you need that setup in assembly code, then > > you are in the situation that you need to either handle the setup in > > bootloader/firmware or have an simple way to determine that condition. > > The main issue is that changing that is a very very invasive change in > an extremely fragile and rather nasty area of code shared by 32 and 64- > bit for which we don't even have easy access to all the machines to > test with anymore :) > > It's probably not impossible, but it would delay the new cpu feature > stuff that Nick is making by a lot, probably monthes, making it nearly > impossible to get back into distros etc... > > So while it might be something to consider, I would definitely keep > that as a separate unit of work to do later. Yeah, it's no longer a "drop in" replacement for existing features testing if we do this, which makes it hard to backport too (we will need this for compatibility with future firmware, so it will have to go into distro kernels.) Given that it's quite a small addition to of/fdt code, hopefully that gives you a reasonable justification to accept it. If you prefer not to, that's okay, but I think we would have to carry it in arch/powerpc at least for a time, because of the schedule we're working to for POWER9 enablement. As a longer term item I agree with you and Ben, it would be worth considering unflattening earlier. Thanks, Nick
On Wed, Apr 5, 2017 at 7:38 PM, Nicholas Piggin <npiggin@gmail.com> wrote: > On Thu, 06 Apr 2017 06:58:01 +1000 > Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote: > >> On Wed, 2017-04-05 at 10:58 -0500, Rob Herring wrote: >> > Well, I'd like to avoid expanding usage of flat DT parsing in the >> > kernel. But you could just put this function into arch/powerpc and I'd >> > never see it, but I like that even less. Mainly, I just wanted to >> > raise the point. >> > >> > Your argument works until you need that setup in assembly code, then >> > you are in the situation that you need to either handle the setup in >> > bootloader/firmware or have an simple way to determine that condition. >> >> The main issue is that changing that is a very very invasive change in >> an extremely fragile and rather nasty area of code shared by 32 and 64- >> bit for which we don't even have easy access to all the machines to >> test with anymore :) >> >> It's probably not impossible, but it would delay the new cpu feature >> stuff that Nick is making by a lot, probably monthes, making it nearly >> impossible to get back into distros etc... >> >> So while it might be something to consider, I would definitely keep >> that as a separate unit of work to do later. > > Yeah, it's no longer a "drop in" replacement for existing features > testing if we do this, which makes it hard to backport too (we will > need this for compatibility with future firmware, so it will have to > go into distro kernels.) > > Given that it's quite a small addition to of/fdt code, hopefully > that gives you a reasonable justification to accept it. > > If you prefer not to, that's okay, but I think we would have to carry > it in arch/powerpc at least for a time, because of the schedule we're > working to for POWER9 enablement. As a longer term item I agree with you > and Ben, it would be worth considering unflattening earlier. As I mentioned, keeping it in arch/powerpc I like even less. So this is fine. Rob
Rob Herring <robh+dt@kernel.org> writes: > On Wed, Apr 5, 2017 at 9:32 AM, Nicholas Piggin <npiggin@gmail.com> wrote: >> On Wed, 5 Apr 2017 08:35:06 -0500 >> Rob Herring <robh+dt@kernel.org> wrote: >> >>> On Wed, Apr 5, 2017 at 7:37 AM, Nicholas Piggin <npiggin@gmail.com> wrote: >>> > Introduce primitives for FDT parsing. These will be used for powerpc >>> > cpufeatures node scanning, which has quite complex structure but should >>> > be processed early. >>> >>> Have you looked at unflattening the FDT earlier? >> >> Hi, thanks for taking a look. Did you mean to trim the cc list? > > Ugg, no. I've added everyone back. > >> It may be possible but I'd like to avoid it if we can. There might >> turn out to be some errata or feature that requires early setup. And >> the current cpu feature parsing code does it with flat dt. > > Well, I'd like to avoid expanding usage of flat DT parsing in the > kernel. But you could just put this function into arch/powerpc and I'd > never see it, but I like that even less. Mainly, I just wanted to > raise the point. > > Your argument works until you need that setup in assembly code, then > you are in the situation that you need to either handle the setup in > bootloader/firmware or have an simple way to determine that condition. Where Nick does this FDT parsing is literally the first C code that runs in the kernel. The MMU is off, we don't even know how much memory we have, or where it is. So it's basically impossible to do the unflattening earlier. What we could do is move the CPU feature setup *later*. As Ben & Nick said that would be a pretty intrusive change, and definitely not something we want to do now. What we could probably do is split some of the flat tree parsing, so that we discover memory, even just some of it, then unflatten, and then do more setup using the unflattened tree. But that will require a lot of delicate work. So we'd definitely like to do that separately from this series. cheers
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c index e5ce4b59e162..a45854fe5156 100644 --- a/drivers/of/fdt.c +++ b/drivers/of/fdt.c @@ -754,6 +754,37 @@ int __init of_scan_flat_dt(int (*it)(unsigned long node, } /** + * of_scan_flat_dt_subnodes - scan sub-nodes of a node call callback on each. + * @it: callback function + * @data: context data pointer + * + * This function is used to scan sub-nodes of a node. + */ +int __init of_scan_flat_dt_subnodes(unsigned long node, + int (*it)(unsigned long node, + const char *uname, + void *data), + void *data) +{ + const void *blob = initial_boot_params; + const char *pathp; + int offset, rc = 0; + + offset = node; + for (offset = fdt_first_subnode(blob, offset); + offset >= 0 && !rc; + offset = fdt_next_subnode(blob, offset)) { + + pathp = fdt_get_name(blob, offset, NULL); + if (*pathp == '/') + pathp = kbasename(pathp); + rc = it(offset, pathp, data); + } + return rc; +} + + +/** * of_get_flat_dt_subnode_by_name - get the subnode by given name * * @node: the parent node @@ -812,6 +843,14 @@ int __init of_flat_dt_match(unsigned long node, const char *const *compat) return of_fdt_match(initial_boot_params, node, compat); } +/** + * of_get_flat_dt_prop - Given a node in the flat blob, return the phandle + */ +uint32_t __init of_get_flat_dt_phandle(unsigned long node) +{ + return fdt_get_phandle(initial_boot_params, node); +} + struct fdt_scan_status { const char *name; int namelen; diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h index 271b3fdf0070..1dfbfd0d8040 100644 --- a/include/linux/of_fdt.h +++ b/include/linux/of_fdt.h @@ -54,6 +54,11 @@ extern char __dtb_end[]; extern int of_scan_flat_dt(int (*it)(unsigned long node, const char *uname, int depth, void *data), void *data); +extern int of_scan_flat_dt_subnodes(unsigned long node, + int (*it)(unsigned long node, + const char *uname, + void *data), + void *data); extern int of_get_flat_dt_subnode_by_name(unsigned long node, const char *uname); extern const void *of_get_flat_dt_prop(unsigned long node, const char *name, @@ -62,6 +67,7 @@ extern int of_flat_dt_is_compatible(unsigned long node, const char *name); extern int of_flat_dt_match(unsigned long node, const char *const *matches); extern unsigned long of_get_flat_dt_root(void); extern int of_get_flat_dt_size(void); +extern uint32_t of_get_flat_dt_phandle(unsigned long node); extern int early_init_dt_scan_chosen(unsigned long node, const char *uname, int depth, void *data);
Introduce primitives for FDT parsing. These will be used for powerpc cpufeatures node scanning, which has quite complex structure but should be processed early. Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- drivers/of/fdt.c | 39 +++++++++++++++++++++++++++++++++++++++ include/linux/of_fdt.h | 6 ++++++ 2 files changed, 45 insertions(+)