[PATCH] of/lib: Export fdt routines to modules

[PATCH] of/lib: Export fdt routines to modules

[Michael Bohan]

Ever since the following commit, libfdt has been available for
usage in the kernel:

     commit ab25383983fb8d7786696f5371e75e79c3e9a405
     Author: David Daney <david.daney@cavium.com>
     Date:   Thu Jul 5 18:12:38 2012 +0200

     of/lib: Allow scripts/dtc/libfdt to be used from kernel code

Export these functions to modules so that they may be used
from device drivers.

Change-Id: I7f540b7cf860c4be414e32ce183be5268b2ae6af
Signed-off-by: Michael Bohan <mbohan@codeaurora.org>
---
 lib/fdt.c     |    6 ++++++
 lib/fdt_ro.c  |   28 ++++++++++++++++++++++++++++
 lib/fdt_rw.c  |   13 +++++++++++++
 lib/fdt_sw.c  |    9 +++++++++
 lib/fdt_wip.c |    6 ++++++
 5 files changed, 62 insertions(+), 0 deletions(-)

diff --git a/lib/fdt.c b/lib/fdt.c
index 97f2006..a3fe87b 100644
--- a/lib/fdt.c
+++ b/lib/fdt.c
@@ -1,2 +1,8 @@
 #include <linux/libfdt_env.h>
+#include <linux/module.h>
 #include "../scripts/dtc/libfdt/fdt.c"
+
+EXPORT_SYMBOL_GPL(fdt_next_tag);
+EXPORT_SYMBOL_GPL(fdt_next_node);
+EXPORT_SYMBOL_GPL(fdt_check_header);
+EXPORT_SYMBOL_GPL(fdt_move);
diff --git a/lib/fdt_ro.c b/lib/fdt_ro.c
index f73c04e..97269bc 100644
--- a/lib/fdt_ro.c
+++ b/lib/fdt_ro.c
@@ -1,2 +1,30 @@
 #include <linux/libfdt_env.h>
+#include <linux/module.h>
 #include "../scripts/dtc/libfdt/fdt_ro.c"
+
+EXPORT_SYMBOL_GPL(fdt_string);
+EXPORT_SYMBOL_GPL(fdt_num_mem_rsv);
+EXPORT_SYMBOL_GPL(fdt_get_mem_rsv);
+EXPORT_SYMBOL_GPL(fdt_subnode_offset_namelen);
+EXPORT_SYMBOL_GPL(fdt_subnode_offset);
+EXPORT_SYMBOL_GPL(fdt_path_offset);
+EXPORT_SYMBOL_GPL(fdt_get_name);
+EXPORT_SYMBOL_GPL(fdt_first_property_offset);
+EXPORT_SYMBOL_GPL(fdt_next_property_offset);
+EXPORT_SYMBOL_GPL(fdt_get_property_by_offset);
+EXPORT_SYMBOL_GPL(fdt_get_property_namelen);
+EXPORT_SYMBOL_GPL(fdt_get_property);
+EXPORT_SYMBOL_GPL(fdt_getprop_by_offset);
+EXPORT_SYMBOL_GPL(fdt_getprop_namelen);
+EXPORT_SYMBOL_GPL(fdt_getprop);
+EXPORT_SYMBOL_GPL(fdt_get_phandle);
+EXPORT_SYMBOL_GPL(fdt_get_alias_namelen);
+EXPORT_SYMBOL_GPL(fdt_get_alias);
+EXPORT_SYMBOL_GPL(fdt_get_path);
+EXPORT_SYMBOL_GPL(fdt_supernode_atdepth_offset);
+EXPORT_SYMBOL_GPL(fdt_node_depth);
+EXPORT_SYMBOL_GPL(fdt_parent_offset);
+EXPORT_SYMBOL_GPL(fdt_node_offset_by_prop_value);
+EXPORT_SYMBOL_GPL(fdt_node_offset_by_phandle);
+EXPORT_SYMBOL_GPL(fdt_node_check_compatible);
+EXPORT_SYMBOL_GPL(fdt_node_offset_by_compatible);
diff --git a/lib/fdt_rw.c b/lib/fdt_rw.c
index 0c1f0f4..30de9ec 100644
--- a/lib/fdt_rw.c
+++ b/lib/fdt_rw.c
@@ -1,2 +1,15 @@
 #include <linux/libfdt_env.h>
+#include <linux/module.h>
 #include "../scripts/dtc/libfdt/fdt_rw.c"
+
+EXPORT_SYMBOL_GPL(fdt_open_into);
+EXPORT_SYMBOL_GPL(fdt_pack);
+EXPORT_SYMBOL_GPL(fdt_add_mem_rsv);
+EXPORT_SYMBOL_GPL(fdt_del_mem_rsv);
+EXPORT_SYMBOL_GPL(fdt_set_name);
+EXPORT_SYMBOL_GPL(fdt_setprop);
+EXPORT_SYMBOL_GPL(fdt_appendprop);
+EXPORT_SYMBOL_GPL(fdt_delprop);
+EXPORT_SYMBOL_GPL(fdt_add_subnode_namelen);
+EXPORT_SYMBOL_GPL(fdt_add_subnode);
+EXPORT_SYMBOL_GPL(fdt_del_node);
diff --git a/lib/fdt_sw.c b/lib/fdt_sw.c
index 9ac7e50..17d6ba1 100644
--- a/lib/fdt_sw.c
+++ b/lib/fdt_sw.c
@@ -1,2 +1,11 @@
 #include <linux/libfdt_env.h>
+#include <linux/module.h>
 #include "../scripts/dtc/libfdt/fdt_sw.c"
+
+EXPORT_SYMBOL_GPL(fdt_create);
+EXPORT_SYMBOL_GPL(fdt_add_reservemap_entry);
+EXPORT_SYMBOL_GPL(fdt_finish_reservemap);
+EXPORT_SYMBOL_GPL(fdt_begin_node);
+EXPORT_SYMBOL_GPL(fdt_property);
+EXPORT_SYMBOL_GPL(fdt_end_node);
+EXPORT_SYMBOL_GPL(fdt_finish);
diff --git a/lib/fdt_wip.c b/lib/fdt_wip.c
index 45b3fc3..22b606d 100644
--- a/lib/fdt_wip.c
+++ b/lib/fdt_wip.c
@@ -1,2 +1,8 @@
 #include <linux/libfdt_env.h>
+#include <linux/module.h>
 #include "../scripts/dtc/libfdt/fdt_wip.c"
+
+EXPORT_SYMBOL_GPL(fdt_setprop_inplace);
+EXPORT_SYMBOL_GPL(fdt_setprop_inplace_u32);
+EXPORT_SYMBOL_GPL(fdt_nop_property);
+EXPORT_SYMBOL_GPL(fdt_nop_node);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of/lib: Export fdt routines to modules

[David Daney]

On 10/16/2013 04:27 PM, Michael Bohan wrote:
> Ever since the following commit, libfdt has been available for
> usage in the kernel:
>
>       commit ab25383983fb8d7786696f5371e75e79c3e9a405
>       Author: David Daney <david.daney@cavium.com>
>       Date:   Thu Jul 5 18:12:38 2012 +0200
>
>       of/lib: Allow scripts/dtc/libfdt to be used from kernel code
>
> Export these functions to modules so that they may be used
> from device drivers.
>
> Change-Id: I7f540b7cf860c4be414e32ce183be5268b2ae6af
> Signed-off-by: Michael Bohan <mbohan@codeaurora.org>
> ---
>   lib/fdt.c     |    6 ++++++
>   lib/fdt_ro.c  |   28 ++++++++++++++++++++++++++++
>   lib/fdt_rw.c  |   13 +++++++++++++
>   lib/fdt_sw.c  |    9 +++++++++
>   lib/fdt_wip.c |    6 ++++++
>   5 files changed, 62 insertions(+), 0 deletions(-)
>
> diff --git a/lib/fdt.c b/lib/fdt.c
> index 97f2006..a3fe87b 100644
> --- a/lib/fdt.c
> +++ b/lib/fdt.c
> @@ -1,2 +1,8 @@
>   #include <linux/libfdt_env.h>
> +#include <linux/module.h>
>   #include "../scripts/dtc/libfdt/fdt.c"
> +
> +EXPORT_SYMBOL_GPL(fdt_next_tag);

The code was all written by David Gibson, and is dual GPL/BSD licensed. 
  So I am not sure you should be using the GPL flavor of the export 
directive.


But more than this.  I don't understand why a driver would be parsing 
the FDT in the first place.  If there is a device tree, why hasn't it 
been unflattened, and thus used via the normal device tree functions.

David Daney

Re: [PATCH] of/lib: Export fdt routines to modules

[Michael Bohan]

On Wed, Oct 16, 2013 at 04:39:03PM -0700, David Daney wrote:
> On 10/16/2013 04:27 PM, Michael Bohan wrote:
> >Ever since the following commit, libfdt has been available for
> >usage in the kernel:
> >
> >      commit ab25383983fb8d7786696f5371e75e79c3e9a405
> >      Author: David Daney <david.daney@cavium.com>
> >      Date:   Thu Jul 5 18:12:38 2012 +0200
> >
> >      of/lib: Allow scripts/dtc/libfdt to be used from kernel code
> >
> >Export these functions to modules so that they may be used
> >from device drivers.
> >---
> >+EXPORT_SYMBOL_GPL(fdt_next_tag);
> 
> The code was all written by David Gibson, and is dual GPL/BSD
> licensed.  So I am not sure you should be using the GPL flavor of
> the export directive.

Yeah, I wasn't quite sure about this myself. I can remove the GPL
if it's incorrect.

> But more than this.  I don't understand why a driver would be
> parsing the FDT in the first place.  If there is a device tree, why
> hasn't it been unflattened, and thus used via the normal device tree
> functions.

My motivation is actually to use the fdt format as a firmware.
I have a requirement to express driver metadata that's loadable
from the filesystem. This data is not reasonable to place in the
system Device Tree, since it's application specific and does not
actually describe hardware. The fact that the format chosen is
'flattened device tree' is merely just a coincidence. 

When considering formats, dts / fdt is convenient since:

-The dts syntax meets requirements by being human-readable.
-There exists a device-tree compiler already
-Linux knows how to deal with fdt, so the driver implementation
 becomes more simple.

Thanks,
Mike

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of/lib: Export fdt routines to modules

[Guenter Roeck]

On 10/16/2013 05:27 PM, Michael Bohan wrote:
> On Wed, Oct 16, 2013 at 04:39:03PM -0700, David Daney wrote:
>> On 10/16/2013 04:27 PM, Michael Bohan wrote:
>>> Ever since the following commit, libfdt has been available for
>>> usage in the kernel:
>>>
>>>       commit ab25383983fb8d7786696f5371e75e79c3e9a405
>>>       Author: David Daney <david.daney@cavium.com>
>>>       Date:   Thu Jul 5 18:12:38 2012 +0200
>>>
>>>       of/lib: Allow scripts/dtc/libfdt to be used from kernel code
>>>
>>> Export these functions to modules so that they may be used
>> >from device drivers.
>>> ---
>>> +EXPORT_SYMBOL_GPL(fdt_next_tag);
>>
>> The code was all written by David Gibson, and is dual GPL/BSD
>> licensed.  So I am not sure you should be using the GPL flavor of
>> the export directive.
>
> Yeah, I wasn't quite sure about this myself. I can remove the GPL
> if it's incorrect.
>
>> But more than this.  I don't understand why a driver would be
>> parsing the FDT in the first place.  If there is a device tree, why
>> hasn't it been unflattened, and thus used via the normal device tree
>> functions.
>
> My motivation is actually to use the fdt format as a firmware.
> I have a requirement to express driver metadata that's loadable
> from the filesystem. This data is not reasonable to place in the
> system Device Tree, since it's application specific and does not
> actually describe hardware. The fact that the format chosen is
> 'flattened device tree' is merely just a coincidence.
>
Still, what prevents you from unflattening it and just using the
normal device tree functions as David suggested ?

Guenter

> When considering formats, dts / fdt is convenient since:
>
> -The dts syntax meets requirements by being human-readable.
> -There exists a device-tree compiler already
> -Linux knows how to deal with fdt, so the driver implementation
>   becomes more simple.
>
> Thanks,
> Mike
>

Re: [PATCH] of/lib: Export fdt routines to modules

[Michael Bohan]

On Wed, Oct 16, 2013 at 09:54:27PM -0700, Guenter Roeck wrote:
> On 10/16/2013 05:27 PM, Michael Bohan wrote:
> >My motivation is actually to use the fdt format as a firmware.
> >I have a requirement to express driver metadata that's loadable
> >from the filesystem. This data is not reasonable to place in the
> >system Device Tree, since it's application specific and does not
> >actually describe hardware. The fact that the format chosen is
> >'flattened device tree' is merely just a coincidence.
> >
> Still, what prevents you from unflattening it and just using the
> normal device tree functions as David suggested ?

I'm assuming you're suggesting to use of_fdt_unflatten_tree()?
That's an interesting thought. I was planning to scan the fdt
only once and populate my own structures, but I suppose I could
use the of_* APIs equivalently.

It seems there are some problems though.  of_fdt_unflatten_tree()
does not return errors, and so for the purposes of my driver it
would not be sufficient to detect an invalid firmware image. 

Would people entertain changing this API
(and implicitly __unflatten_device_tree) to return errors? I'm
guessing the reason it's coded that way is because the normal
usecase is 'system boot', at which time errors aren't that
meaningful.

Also, there's no way to free the memory that was allocated from
the unflatten process. May I add one?

Thanks,
Mike

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of/lib: Export fdt routines to modules

[Guenter Roeck]

On 10/17/2013 04:51 PM, Michael Bohan wrote:
> On Wed, Oct 16, 2013 at 09:54:27PM -0700, Guenter Roeck wrote:
>> On 10/16/2013 05:27 PM, Michael Bohan wrote:
>>> My motivation is actually to use the fdt format as a firmware.
>>> I have a requirement to express driver metadata that's loadable
>> >from the filesystem. This data is not reasonable to place in the
>>> system Device Tree, since it's application specific and does not
>>> actually describe hardware. The fact that the format chosen is
>>> 'flattened device tree' is merely just a coincidence.
>>>
>> Still, what prevents you from unflattening it and just using the
>> normal device tree functions as David suggested ?
>
> I'm assuming you're suggesting to use of_fdt_unflatten_tree()?

Yes, that was the idea.

> That's an interesting thought. I was planning to scan the fdt
> only once and populate my own structures, but I suppose I could
> use the of_* APIs equivalently.
>
> It seems there are some problems though.  of_fdt_unflatten_tree()
> does not return errors, and so for the purposes of my driver it
> would not be sufficient to detect an invalid firmware image.
>
It does so, at least partially. If there is an error, it won't set
the nodes pointer. Granted, that is not perfect, but it is at least
a start. Ultimately, I considered it 'good enough' for my purpose
(for devicetree overlays - see [1] below), as any missing mandatory
properties or nodes are detected later when trying to actually read
the properties. In my case, I also have a couple of validation
properties to ensure that the overlay is acceptable (specifically
I use 'compatible' and 'assembly-ids', but that is really a detail).

> Would people entertain changing this API
> (and implicitly __unflatten_device_tree) to return errors? I'm
> guessing the reason it's coded that way is because the normal
> usecase is 'system boot', at which time errors aren't that
> meaningful.
>
> Also, there's no way to free the memory that was allocated from
> the unflatten process. May I add one?
>

The patchset submitted by Pantelis Antoniou to add support for
devicetree overlays adds this and other related functionality.
See [1], specifically the patch titled "OF: Introduce utility
helper functions". Not sure where that is going, though.
It may need some cleanup to be accepted upstream.
Copying Pantelis for comments.

I also updated the devicetree discussion list address to get comments
from the experts.

Thanks,
Guenter

[1] https://lkml.org/lkml/2013/1/4/276

Re: [PATCH] of/lib: Export fdt routines to modules

[Michael Bohan]

On Thu, Oct 17, 2013 at 05:44:07PM -0700, Guenter Roeck wrote:
> On 10/17/2013 04:51 PM, Michael Bohan wrote:
> >On Wed, Oct 16, 2013 at 09:54:27PM -0700, Guenter Roeck wrote:
> >>Still, what prevents you from unflattening it and just using the
> >>normal device tree functions as David suggested ?
> >
> >I'm assuming you're suggesting to use of_fdt_unflatten_tree()?
> 
> Yes, that was the idea.
> 
> >That's an interesting thought. I was planning to scan the fdt
> >only once and populate my own structures, but I suppose I could
> >use the of_* APIs equivalently.
> >
> >It seems there are some problems though.  of_fdt_unflatten_tree()
> >does not return errors, and so for the purposes of my driver it
> >would not be sufficient to detect an invalid firmware image.
> >
> It does so, at least partially. If there is an error, it won't set
> the nodes pointer. Granted, that is not perfect, but it is at least
> a start. Ultimately, I considered it 'good enough' for my purpose
> (for devicetree overlays - see [1] below), as any missing mandatory
> properties or nodes are detected later when trying to actually read
> the properties. In my case, I also have a couple of validation
> properties to ensure that the overlay is acceptable (specifically
> I use 'compatible' and 'assembly-ids', but that is really a detail).

That's certainly better than nothing, but I think it would be
useful to make a distinction between a malformed fdt and a fdt
that's simply missing the right information. Without error
codes, I think we lose this aspect.

> >Would people entertain changing this API
> >(and implicitly __unflatten_device_tree) to return errors? I'm
> >guessing the reason it's coded that way is because the normal
> >usecase is 'system boot', at which time errors aren't that
> >meaningful.
> >
> >Also, there's no way to free the memory that was allocated from
> >the unflatten process. May I add one?
> >
> 
> The patchset submitted by Pantelis Antoniou to add support for
> devicetree overlays adds this and other related functionality.
> See [1], specifically the patch titled "OF: Introduce utility
> helper functions". Not sure where that is going, though.
> It may need some cleanup to be accepted upstream.
> Copying Pantelis for comments.
> Guenter
> 
> [1] https://lkml.org/lkml/2013/1/4/276

Thanks. So it seems that Pantelis's __of_free_tree() is what I'm
looking for.

Mike

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of/lib: Export fdt routines to modules

[Pantelis Antoniou]

Hi Michael,

On Oct 18, 2013, at 5:54 AM, Michael Bohan wrote:

> On Thu, Oct 17, 2013 at 05:44:07PM -0700, Guenter Roeck wrote:
>> On 10/17/2013 04:51 PM, Michael Bohan wrote:
>>> On Wed, Oct 16, 2013 at 09:54:27PM -0700, Guenter Roeck wrote:
>>>> Still, what prevents you from unflattening it and just using the
>>>> normal device tree functions as David suggested ?
>>> 
>>> I'm assuming you're suggesting to use of_fdt_unflatten_tree()?
>> 
>> Yes, that was the idea.
>> 
>>> That's an interesting thought. I was planning to scan the fdt
>>> only once and populate my own structures, but I suppose I could
>>> use the of_* APIs equivalently.
>>> 
>>> It seems there are some problems though.  of_fdt_unflatten_tree()
>>> does not return errors, and so for the purposes of my driver it
>>> would not be sufficient to detect an invalid firmware image.
>>> 
>> It does so, at least partially. If there is an error, it won't set
>> the nodes pointer. Granted, that is not perfect, but it is at least
>> a start. Ultimately, I considered it 'good enough' for my purpose
>> (for devicetree overlays - see [1] below), as any missing mandatory
>> properties or nodes are detected later when trying to actually read
>> the properties. In my case, I also have a couple of validation
>> properties to ensure that the overlay is acceptable (specifically
>> I use 'compatible' and 'assembly-ids', but that is really a detail).
> 
> That's certainly better than nothing, but I think it would be
> useful to make a distinction between a malformed fdt and a fdt
> that's simply missing the right information. Without error
> codes, I think we lose this aspect.
> 
>>> Would people entertain changing this API
>>> (and implicitly __unflatten_device_tree) to return errors? I'm
>>> guessing the reason it's coded that way is because the normal
>>> usecase is 'system boot', at which time errors aren't that
>>> meaningful.
>>> 
>>> Also, there's no way to free the memory that was allocated from
>>> the unflatten process. May I add one?
>>> 
>> 
>> The patchset submitted by Pantelis Antoniou to add support for
>> devicetree overlays adds this and other related functionality.
>> See [1], specifically the patch titled "OF: Introduce utility
>> helper functions". Not sure where that is going, though.
>> It may need some cleanup to be accepted upstream.
>> Copying Pantelis for comments.
>> Guenter
>> 
>> [1] https://lkml.org/lkml/2013/1/4/276
> 
> Thanks. So it seems that Pantelis's __of_free_tree() is what I'm
> looking for.
> 

I guess it's time for another try to getting it in?

DT maintainers, which one of you will want to review?

> Mike
> 
> -- 
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> hosted by The Linux Foundation

Regards

-- Pantelis

Re: [PATCH] of/lib: Export fdt routines to modules

[Rob Herring]

On 10/18/2013 08:28 AM, Pantelis Antoniou wrote:
> Hi Michael,
> 
> On Oct 18, 2013, at 5:54 AM, Michael Bohan wrote:
> 
>> On Thu, Oct 17, 2013 at 05:44:07PM -0700, Guenter Roeck wrote:
>>> On 10/17/2013 04:51 PM, Michael Bohan wrote:
>>>> On Wed, Oct 16, 2013 at 09:54:27PM -0700, Guenter Roeck wrote:
>>>>> Still, what prevents you from unflattening it and just using the
>>>>> normal device tree functions as David suggested ?
>>>>
>>>> I'm assuming you're suggesting to use of_fdt_unflatten_tree()?
>>>
>>> Yes, that was the idea.
>>>
>>>> That's an interesting thought. I was planning to scan the fdt
>>>> only once and populate my own structures, but I suppose I could
>>>> use the of_* APIs equivalently.
>>>>
>>>> It seems there are some problems though.  of_fdt_unflatten_tree()
>>>> does not return errors, and so for the purposes of my driver it
>>>> would not be sufficient to detect an invalid firmware image.
>>>>
>>> It does so, at least partially. If there is an error, it won't set
>>> the nodes pointer. Granted, that is not perfect, but it is at least
>>> a start. Ultimately, I considered it 'good enough' for my purpose
>>> (for devicetree overlays - see [1] below), as any missing mandatory
>>> properties or nodes are detected later when trying to actually read
>>> the properties. In my case, I also have a couple of validation
>>> properties to ensure that the overlay is acceptable (specifically
>>> I use 'compatible' and 'assembly-ids', but that is really a detail).
>>
>> That's certainly better than nothing, but I think it would be
>> useful to make a distinction between a malformed fdt and a fdt
>> that's simply missing the right information. Without error
>> codes, I think we lose this aspect.
>>
>>>> Would people entertain changing this API
>>>> (and implicitly __unflatten_device_tree) to return errors? I'm
>>>> guessing the reason it's coded that way is because the normal
>>>> usecase is 'system boot', at which time errors aren't that
>>>> meaningful.
>>>>
>>>> Also, there's no way to free the memory that was allocated from
>>>> the unflatten process. May I add one?
>>>>
>>>
>>> The patchset submitted by Pantelis Antoniou to add support for
>>> devicetree overlays adds this and other related functionality.
>>> See [1], specifically the patch titled "OF: Introduce utility
>>> helper functions". Not sure where that is going, though.
>>> It may need some cleanup to be accepted upstream.
>>> Copying Pantelis for comments.
>>> Guenter
>>>
>>> [1] https://lkml.org/lkml/2013/1/4/276
>>
>> Thanks. So it seems that Pantelis's __of_free_tree() is what I'm
>> looking for.
>>
> 
> I guess it's time for another try to getting it in?
> 
> DT maintainers, which one of you will want to review?

This falls in Grant's and my plate since we are talking kernel DT
support code rather than bindings. Unflattening is definitely the right
direction to go here.

Rob

Re: [PATCH] of/lib: Export fdt routines to modules

[Guenter Roeck]

On Fri, Oct 18, 2013 at 10:57:24AM -0500, Rob Herring wrote:
> On 10/18/2013 08:28 AM, Pantelis Antoniou wrote:
> > Hi Michael,
> > 
> > On Oct 18, 2013, at 5:54 AM, Michael Bohan wrote:
> > 
> >> On Thu, Oct 17, 2013 at 05:44:07PM -0700, Guenter Roeck wrote:
> >>> On 10/17/2013 04:51 PM, Michael Bohan wrote:
> >>>> On Wed, Oct 16, 2013 at 09:54:27PM -0700, Guenter Roeck wrote:
> >>>>> Still, what prevents you from unflattening it and just using the
> >>>>> normal device tree functions as David suggested ?
> >>>>
> >>>> I'm assuming you're suggesting to use of_fdt_unflatten_tree()?
> >>>
> >>> Yes, that was the idea.
> >>>
> >>>> That's an interesting thought. I was planning to scan the fdt
> >>>> only once and populate my own structures, but I suppose I could
> >>>> use the of_* APIs equivalently.
> >>>>
> >>>> It seems there are some problems though.  of_fdt_unflatten_tree()
> >>>> does not return errors, and so for the purposes of my driver it
> >>>> would not be sufficient to detect an invalid firmware image.
> >>>>
> >>> It does so, at least partially. If there is an error, it won't set
> >>> the nodes pointer. Granted, that is not perfect, but it is at least
> >>> a start. Ultimately, I considered it 'good enough' for my purpose
> >>> (for devicetree overlays - see [1] below), as any missing mandatory
> >>> properties or nodes are detected later when trying to actually read
> >>> the properties. In my case, I also have a couple of validation
> >>> properties to ensure that the overlay is acceptable (specifically
> >>> I use 'compatible' and 'assembly-ids', but that is really a detail).
> >>
> >> That's certainly better than nothing, but I think it would be
> >> useful to make a distinction between a malformed fdt and a fdt
> >> that's simply missing the right information. Without error
> >> codes, I think we lose this aspect.
> >>
> >>>> Would people entertain changing this API
> >>>> (and implicitly __unflatten_device_tree) to return errors? I'm
> >>>> guessing the reason it's coded that way is because the normal
> >>>> usecase is 'system boot', at which time errors aren't that
> >>>> meaningful.
> >>>>
> >>>> Also, there's no way to free the memory that was allocated from
> >>>> the unflatten process. May I add one?
> >>>>
> >>>
> >>> The patchset submitted by Pantelis Antoniou to add support for
> >>> devicetree overlays adds this and other related functionality.
> >>> See [1], specifically the patch titled "OF: Introduce utility
> >>> helper functions". Not sure where that is going, though.
> >>> It may need some cleanup to be accepted upstream.
> >>> Copying Pantelis for comments.
> >>> Guenter
> >>>
> >>> [1] https://lkml.org/lkml/2013/1/4/276
> >>
> >> Thanks. So it seems that Pantelis's __of_free_tree() is what I'm
> >> looking for.
> >>
> > 
> > I guess it's time for another try to getting it in?
> > 
> > DT maintainers, which one of you will want to review?
> 
> This falls in Grant's and my plate since we are talking kernel DT
> support code rather than bindings. Unflattening is definitely the right
> direction to go here.
> 
I think Pantelis may have been asking about his devicetree overlay
support patch series.

Pantelis, did you have time to address the review comments you got earlier ?

Thanks,
Guenter

Re: [PATCH] of/lib: Export fdt routines to modules

[David Daney]

On 10/18/2013 08:57 AM, Rob Herring wrote:
[...]
>
> Unflattening is definitely the right
> direction to go here.
>

I wonder if that is really true.

The device tree in question is very short lived, and used to control the 
configuration of some hardware device when loading the driver.

The use of it is completely contained within a single driver (at least 
that is my understanding), it is not information that needs to be shared 
system wide.

Given that it is a driver implementation issue, rather than making 
things work nicely system wide, I don't think it really matters what is 
done.

It may be that the overhead of unflattening the tree and then freeing 
it, is much greater than just extracting a few things from the FDT.

That said, I don't really have a preference for what is done.  My 
original questions were targeted at understanding this particular use case.

David Daney

Re: [PATCH] of/lib: Export fdt routines to modules

[Michael Bohan]

On Fri, Oct 18, 2013 at 09:30:32AM -0700, David Daney wrote:
> On 10/18/2013 08:57 AM, Rob Herring wrote:
> [...]
> >
> >Unflattening is definitely the right
> >direction to go here.
> >
> 
> I wonder if that is really true.
> 
> The device tree in question is very short lived, and used to control
> the configuration of some hardware device when loading the driver.
> 
> The use of it is completely contained within a single driver (at
> least that is my understanding), it is not information that needs to
> be shared system wide.

That's correct.

> Given that it is a driver implementation issue, rather than making
> things work nicely system wide, I don't think it really matters what
> is done.
> 
> It may be that the overhead of unflattening the tree and then
> freeing it, is much greater than just extracting a few things from
> the FDT.

Yes, this was my original thought as well. On the other hand,
having libfdt in the kernel does add a little extra bloat, and so
it seems a tradeoff from one-time runtime overhead to footprint.

> That said, I don't really have a preference for what is done.  My
> original questions were targeted at understanding this particular
> use case.

My preference is probably straight libfdt calls, but if others
think that unpacking is a better solution, I'm able to go that
route as well. My only concern there is that we provide a means
to detect invalid dtb image (ex. handle error codes) and also
free the device_node allocations once the device is released.

Thanks,
Mike

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/lib: Export fdt routines to modules

[Rob Herring]

On 10/18/2013 02:32 PM, Michael Bohan wrote:
> On Fri, Oct 18, 2013 at 09:30:32AM -0700, David Daney wrote:
>> On 10/18/2013 08:57 AM, Rob Herring wrote:
>> [...]
>>>
>>> Unflattening is definitely the right
>>> direction to go here.
>>>
>>
>> I wonder if that is really true.
>>
>> The device tree in question is very short lived, and used to control
>> the configuration of some hardware device when loading the driver.
>>
>> The use of it is completely contained within a single driver (at
>> least that is my understanding), it is not information that needs to
>> be shared system wide.
> 
> That's correct.
> 
>> Given that it is a driver implementation issue, rather than making
>> things work nicely system wide, I don't think it really matters what
>> is done.
>>
>> It may be that the overhead of unflattening the tree and then
>> freeing it, is much greater than just extracting a few things from
>> the FDT.
> 
> Yes, this was my original thought as well. On the other hand,
> having libfdt in the kernel does add a little extra bloat, and so
> it seems a tradeoff from one-time runtime overhead to footprint.
> 
>> That said, I don't really have a preference for what is done.  My
>> original questions were targeted at understanding this particular
>> use case.
> 
> My preference is probably straight libfdt calls, but if others
> think that unpacking is a better solution, I'm able to go that
> route as well. My only concern there is that we provide a means
> to detect invalid dtb image (ex. handle error codes) and also
> free the device_node allocations once the device is released.

I think we need to understand what you are putting in the DT first.

Given there are other desired uses like overlays which need to add the
necessary loading and unflattening support, a common solution is likely
more desirable.

Rob

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] of/lib: Export fdt routines to modules

[Michael Bohan]

On Fri, Oct 18, 2013 at 04:20:09PM -0500, Rob Herring wrote:
> On 10/18/2013 02:32 PM, Michael Bohan wrote:
> > My preference is probably straight libfdt calls, but if others
> > think that unpacking is a better solution, I'm able to go that
> > route as well. My only concern there is that we provide a means
> > to detect invalid dtb image (ex. handle error codes) and also
> > free the device_node allocations once the device is released.
> 
> I think we need to understand what you are putting in the DT first.

That's understandable. Please see my response to Mark.

> Given there are other desired uses like overlays which need to add the
> necessary loading and unflattening support, a common solution is likely
> more desirable.

But by convention, would overlays allow for 'application
specific' data, or are they expected to meet the more rigid
requirements of a real Device Tree?

Thanks,
Mike

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH] of/lib: Export fdt routines to modules

[Guenter Roeck]

On 10/18/2013 06:49 PM, Michael Bohan wrote:
> On Fri, Oct 18, 2013 at 04:20:09PM -0500, Rob Herring wrote:
>> On 10/18/2013 02:32 PM, Michael Bohan wrote:
>>> My preference is probably straight libfdt calls, but if others
>>> think that unpacking is a better solution, I'm able to go that
>>> route as well. My only concern there is that we provide a means
>>> to detect invalid dtb image (ex. handle error codes) and also
>>> free the device_node allocations once the device is released.
>>
>> I think we need to understand what you are putting in the DT first.
>
> That's understandable. Please see my response to Mark.
>
>> Given there are other desired uses like overlays which need to add the
>> necessary loading and unflattening support, a common solution is likely
>> more desirable.
>
> But by convention, would overlays allow for 'application
> specific' data, or are they expected to meet the more rigid
> requirements of a real Device Tree?
>

Not that I am the authority on it, but the idea is that devicetree overlays
will be merged with the existing devicetree, which implies that the same
requirements would apply.

However, you would not really use overlays. You would use the devicetree
infrastructure to create a logically separate instance of a devicetree
to dynamically configure your driver (which I think is an ingenious idea).
I don't think the same rules would or should apply there.

Guenter

Re: [PATCH] of/lib: Export fdt routines to modules

[Mark Rutland]

Hi all,

Guenter, thanks for adding devicetree to Cc.

On Fri, Oct 18, 2013 at 01:44:07AM +0100, Guenter Roeck wrote:
> On 10/17/2013 04:51 PM, Michael Bohan wrote:
> > On Wed, Oct 16, 2013 at 09:54:27PM -0700, Guenter Roeck wrote:
> >> On 10/16/2013 05:27 PM, Michael Bohan wrote:
> >>> My motivation is actually to use the fdt format as a firmware.
> >>> I have a requirement to express driver metadata that's loadable
> >> >from the filesystem. This data is not reasonable to place in the
> >>> system Device Tree, since it's application specific and does not
> >>> actually describe hardware. The fact that the format chosen is
> >>> 'flattened device tree' is merely just a coincidence.

Michael, could you elaborate on using the fdt format "as a firmware" and
what driver metadata you need to be loadable from the filesystem?

How are you intending to pass the DT to the kernel from userspace?

Given that you mention this is application-specific configuration, why
is using sysfs attributes or some other sort of filesystem interaction
not appropriate?

Thanks,
Mark.

Re: [PATCH] of/lib: Export fdt routines to modules

[Michael Bohan]

On Fri, Oct 18, 2013 at 07:38:04PM +0100, Mark Rutland wrote:

Hi Mark,

> On Fri, Oct 18, 2013 at 01:44:07AM +0100, Guenter Roeck wrote:
> > On 10/17/2013 04:51 PM, Michael Bohan wrote:
> > > On Wed, Oct 16, 2013 at 09:54:27PM -0700, Guenter Roeck wrote:
> > >> On 10/16/2013 05:27 PM, Michael Bohan wrote:
> > >>> My motivation is actually to use the fdt format as a firmware.
> > >>> I have a requirement to express driver metadata that's loadable
> > >> >from the filesystem. This data is not reasonable to place in the
> > >>> system Device Tree, since it's application specific and does not
> > >>> actually describe hardware. The fact that the format chosen is
> > >>> 'flattened device tree' is merely just a coincidence.
> 
> Michael, could you elaborate on using the fdt format "as a firmware" and
> what driver metadata you need to be loadable from the filesystem?

We have a hardware device in which of its internal register state
is reconfigured for each application that uses it. And we may
need to quickly swap between applications many times within the
life of the system, so all of the data needs to be preloaded
ahead of the time. There's a lot of data that needs to be updated
in one-shot, and unfortunately there's really no great way to
abstract much of this information.

This metadata file is generated by a tool and lives on the
filesystem, and exists mainly so that each higher level
application driver doesn't need to change its code when such
parameters change. We also want it to be human-readable so that
people can view it, and possibly even tweak it by hand. As
mentioned, most of the data represent actual hardware
configurations, but some others describe per application software
implementation details.

> How are you intending to pass the DT to the kernel from userspace?

request_firmware()

> Given that you mention this is application-specific configuration, why
> is using sysfs attributes or some other sort of filesystem interaction
> not appropriate?

Sysfs is another option, but the major problem is that the driver
doesn't know the full list of applications until it reads the
firmware file. So if we went the sysfs route, it seems we'd need
to form some sort of 'virtual device' hierarchy so that the sysfs
files map 1:1 with each application. That is, read the metadata
file, and for each 'application node', instruct the driver to
create a virtual device for that application node, exposing sysfs
files that can hold its data. To me this seems a bit excessive
and complicated.

Another option would to have a flat sysfs representation and
have the write of one special file create an internal application
database entry for that configuration.

The point is, we can't proceed with normal operation until we
have the total list of applications and their respective
properties registered in memory.

But writing all the sysfs files could be expensive. There would
be complicated tables of different formats that would be
consuming to run through all the different possible entries.

To be honest, I mostly just thought the fdt would be a cool
implementation. It handles all of our requirements, while reusing
infrastructure within and external to the kernel.

Thanks,
Mike

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html