[PATCH 2/3] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle

[Nicholas Piggin]

Introduce primitives for FDT parsing. These will be used for powerpc
cpufeatures node scanning, which has quite complex structure but should
be processed early.

Acked-by: Rob Herring 
Signed-off-by: Nicholas Piggin 
---
 drivers/of/fdt.c   | 38 ++
 include/linux/of_fdt.h |  6 ++
 2 files changed, 44 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index e5ce4b59e162..961ca97072a9 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -754,6 +754,36 @@ 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 parent,
+   int (*it)(unsigned long node,
+ const char *uname,
+ void *data),
+   void *data)
+{
+   const void *blob = initial_boot_params;
+   int node;
+
+   fdt_for_each_subnode(node, blob, parent) {
+   const char *pathp;
+   int rc;
+
+   pathp = fdt_get_name(blob, node, NULL);
+   if (*pathp == '/')
+   pathp = kbasename(pathp);
+   rc = it(node, pathp, data);
+   if (rc)
+   return rc;
+   }
+   return 0;
+}
+
+/**
  * of_get_flat_dt_subnode_by_name - get the subnode by given name
  *
  * @node: the parent node
@@ -812,6 +842,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);
-- 
2.11.0

Re: [PATCH 2/3] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle

[Rob Herring]

On Mon, Apr 10, 2017 at 12:43 AM, Nicholas Piggin  wrote:
> On Thu, 6 Apr 2017 09:09:41 -0500
> Rob Herring  wrote:
>
>> On Wed, Apr 5, 2017 at 7:38 PM, Nicholas Piggin  wrote:
>> > 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.
>
> Here is the patch with the change you suggested. Can I add your
> ack and send it via the powerpc tree with the change that uses
> these interfaces?

Acked-by: Rob Herring 

Re: [PATCH 2/3] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle

[Nicholas Piggin]

On Thu, 6 Apr 2017 09:09:41 -0500
Rob Herring  wrote:

> On Wed, Apr 5, 2017 at 7:38 PM, Nicholas Piggin  wrote:
> > 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.

Here is the patch with the change you suggested. Can I add your
ack and send it via the powerpc tree with the change that uses
these interfaces?

Thanks,
Nick

---
 drivers/of/fdt.c   | 38 ++
 include/linux/of_fdt.h |  6 ++
 2 files changed, 44 insertions(+)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index e5ce4b59e162..961ca97072a9 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -754,6 +754,36 @@ 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 parent,
+   int (*it)(unsigned long node,
+ const char *uname,
+ void *data),
+   void *data)
+{
+   const void *blob = initial_boot_params;
+   int node;
+
+   fdt_for_each_subnode(node, blob, parent) {
+   const char *pathp;
+   int rc;
+
+   pathp = fdt_get_name(blob, node, NULL);
+   if (*pathp == '/')
+   pathp = kbasename(pathp);
+   rc = it(node, pathp, data);
+   if (rc)
+   return rc;
+   }
+   return 0;
+}
+
+/**
  * of_get_flat_dt_subnode_by_name - get the subnode by given name
  *
  * @node: the parent node
@@ -812,6 +842,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);
-- 
2.11.0

Re: [PATCH 2/3] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle

[Michael Ellerman]

Rob Herring  writes:

> On Wed, Apr 5, 2017 at 9:32 AM, Nicholas Piggin  wrote:
>> On Wed, 5 Apr 2017 08:35:06 -0500
>> Rob Herring  wrote:
>>
>>> On Wed, Apr 5, 2017 at 7:37 AM, Nicholas Piggin  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

Re: [PATCH 2/3] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle

[Rob Herring]

On Wed, Apr 5, 2017 at 7:38 PM, Nicholas Piggin  wrote:
> On Thu, 06 Apr 2017 06:58:01 +1000
> Benjamin Herrenschmidt  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

Re: [PATCH 2/3] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle

[Nicholas Piggin]

On Thu, 06 Apr 2017 06:58:01 +1000
Benjamin Herrenschmidt  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

Re: [PATCH 2/3] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle

[Benjamin Herrenschmidt]

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.

Re: [PATCH 2/3] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle

[Rob Herring]

On Wed, Apr 5, 2017 at 9:32 AM, Nicholas Piggin  wrote:
> On Wed, 5 Apr 2017 08:35:06 -0500
> Rob Herring  wrote:
>
>> On Wed, Apr 5, 2017 at 7:37 AM, Nicholas Piggin  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 
>> > ---
>> >  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

[PATCH 2/3] of/fdt: introduce of_scan_flat_dt_subnodes and of_get_flat_dt_phandle

[Nicholas Piggin]

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 
---
 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)) {
+
+   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);
-- 
2.11.0