difference between fdtdec and fdt_support ?

difference between fdtdec and fdt_support ?

[Marek Behún]

Hi Simon,

I am a little confused.

We have
  common/fdt_support.c
and
  lib/fdtdec.c

The second one implements for example fdtdec_get_is_enabled(), which I
would rather expect in fdt_support by name fdt_node_is_available(), or
something like that.

Also fdtdec does a strange thing with compatible strings: it declares
an enum for compatible strings and then a map which maps this enum
values to compatible strings... Why not just use the compatible strings?

What is the purpose of having two files implementing fdt stuff?

Marek

Re: difference between fdtdec and fdt_support ?

[Rasmus Villemoes]

On 06/01/2022 13.21, Marek Behún wrote:

> Also fdtdec does a strange thing with compatible strings: it declares
> an enum for compatible strings and then a map which maps this enum
> values to compatible strings... Why not just use the compatible strings?
> 
> What is the purpose of having two files implementing fdt stuff?

AFAIK, the primary purpose is to ensure powerpc binaries are bloated
with useless string literals.</sarcasm>

https://lore.kernel.org/u-boot/50288d08-3ab8-ef3c-5118-f74e30297104@prevas.dk/

Rasmus

Re: difference between fdtdec and fdt_support ?

[Simon Glass]

Hi Marek,

On Thu, 6 Jan 2022 at 05:21, Marek Behún <marek.behun@nic.cz> wrote:
>
> Hi Simon,
>
> I am a little confused.
>
> We have
>   common/fdt_support.c
> and
>   lib/fdtdec.c
>
> The second one implements for example fdtdec_get_is_enabled(), which I
> would rather expect in fdt_support by name fdt_node_is_available(), or
> something like that.

Should be moved to ofnode

>
> Also fdtdec does a strange thing with compatible strings: it declares
> an enum for compatible strings and then a map which maps this enum
> values to compatible strings... Why not just use the compatible strings?

Did you see the comment?

 * NOTE: This list is basically a TODO list for things that need to be
 * converted to driver model. So don't add new things here unless there is a
 * good reason why driver-model conversion is infeasible. Examples include
 * things which are used before driver model is available.

This is effectively a list of things that should be converted to
driver model. The list should then go away.

>
> What is the purpose of having two files implementing fdt stuff?

fdtdec - for reading from the DT. Should go away and be replaced with
the ofnode API, and fdtaddr.c
fdt_support - for updating the DT, e.g. for fixups before booting an OS

Regards,
Simon

Re: difference between fdtdec and fdt_support ?

[Marek Behún]

On Thu, 6 Jan 2022 08:48:48 -0700
Simon Glass <sjg@chromium.org> wrote:

> Hi Marek,
> 
> On Thu, 6 Jan 2022 at 05:21, Marek Behún <marek.behun@nic.cz> wrote:
> >
> > Hi Simon,
> >
> > I am a little confused.
> >
> > We have
> >   common/fdt_support.c
> > and
> >   lib/fdtdec.c
> >
> > The second one implements for example fdtdec_get_is_enabled(), which I
> > would rather expect in fdt_support by name fdt_node_is_available(), or
> > something like that.  
> 
> Should be moved to ofnode

?? But this function is needed for example when fixing device tree for
Linux.

> >
> > Also fdtdec does a strange thing with compatible strings: it declares
> > an enum for compatible strings and then a map which maps this enum
> > values to compatible strings... Why not just use the compatible strings?  
> 
> Did you see the comment?
> 
>  * NOTE: This list is basically a TODO list for things that need to be
>  * converted to driver model. So don't add new things here unless there is a
>  * good reason why driver-model conversion is infeasible. Examples include
>  * things which are used before driver model is available.
> 
> This is effectively a list of things that should be converted to
> driver model. The list should then go away.

Hmm. But can't that be simply made into a list in a comment? Because
currently this is compiled in for every board that uses any such
function from fdtdec, even if they don't use any string from those
compatible strings.

> >
> > What is the purpose of having two files implementing fdt stuff?  
> 
> fdtdec - for reading from the DT. Should go away and be replaced with
> the ofnode API, and fdtaddr.c
> fdt_support - for updating the DT, e.g. for fixups before booting an OS

Thanks! Okay that makes sense. This means that we should also have
fdt_node_is_available() in fdt_support.c which does the same thing.
(For now this can be made a static inline function that calls
 fdtdec_get_is_enabled().)

Marek

Re: difference between fdtdec and fdt_support ?

[Simon Glass]

Hi Marek,

On Thu, 6 Jan 2022 at 09:10, Marek Behún <marek.behun@nic.cz> wrote:
>
> On Thu, 6 Jan 2022 08:48:48 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Marek,
> >
> > On Thu, 6 Jan 2022 at 05:21, Marek Behún <marek.behun@nic.cz> wrote:
> > >
> > > Hi Simon,
> > >
> > > I am a little confused.
> > >
> > > We have
> > >   common/fdt_support.c
> > > and
> > >   lib/fdtdec.c
> > >
> > > The second one implements for example fdtdec_get_is_enabled(), which I
> > > would rather expect in fdt_support by name fdt_node_is_available(), or
> > > something like that.
> >
> > Should be moved to ofnode
>
> ?? But this function is needed for example when fixing device tree for
> Linux.
>
> > >
> > > Also fdtdec does a strange thing with compatible strings: it declares
> > > an enum for compatible strings and then a map which maps this enum
> > > values to compatible strings... Why not just use the compatible strings?
> >
> > Did you see the comment?
> >
> >  * NOTE: This list is basically a TODO list for things that need to be
> >  * converted to driver model. So don't add new things here unless there is a
> >  * good reason why driver-model conversion is infeasible. Examples include
> >  * things which are used before driver model is available.
> >
> > This is effectively a list of things that should be converted to
> > driver model. The list should then go away.
>
> Hmm. But can't that be simply made into a list in a comment? Because
> currently this is compiled in for every board that uses any such
> function from fdtdec, even if they don't use any string from those
> compatible strings.

Well another option would be to delete the boards that need it, since
no one has seen fit to resolve this all these years. Or create some
drivers for them.

>
> > >
> > > What is the purpose of having two files implementing fdt stuff?
> >
> > fdtdec - for reading from the DT. Should go away and be replaced with
> > the ofnode API, and fdtaddr.c
> > fdt_support - for updating the DT, e.g. for fixups before booting an OS
>
> Thanks! Okay that makes sense. This means that we should also have
> fdt_node_is_available() in fdt_support.c which does the same thing.
> (For now this can be made a static inline function that calls
>  fdtdec_get_is_enabled().)

OK.

BTW fdt_support.c should really move to use ofnode. There are few
ofnode 'write' functions at present, but we should fill those out so
we don't need to use flattree for anything, with OF_LIVE is enabled.

Regards,
Simon

Re: difference between fdtdec and fdt_support ?

[Marek Behún]

On Thu, 6 Jan 2022 09:15:17 -0700
Simon Glass <sjg@chromium.org> wrote:

> Hi Marek,
> 
> On Thu, 6 Jan 2022 at 09:10, Marek Behún <marek.behun@nic.cz> wrote:
> >
> > On Thu, 6 Jan 2022 08:48:48 -0700
> > Simon Glass <sjg@chromium.org> wrote:
> >  
> > > Hi Marek,
> > >
> > > On Thu, 6 Jan 2022 at 05:21, Marek Behún <marek.behun@nic.cz> wrote:  
> > > >
> > > > Hi Simon,
> > > >
> > > > I am a little confused.
> > > >
> > > > We have
> > > >   common/fdt_support.c
> > > > and
> > > >   lib/fdtdec.c
> > > >
> > > > The second one implements for example fdtdec_get_is_enabled(), which I
> > > > would rather expect in fdt_support by name fdt_node_is_available(), or
> > > > something like that.  
> > >
> > > Should be moved to ofnode  
> >
> > ?? But this function is needed for example when fixing device tree for
> > Linux.
> >  
> > > >
> > > > Also fdtdec does a strange thing with compatible strings: it declares
> > > > an enum for compatible strings and then a map which maps this enum
> > > > values to compatible strings... Why not just use the compatible strings?  
> > >
> > > Did you see the comment?
> > >
> > >  * NOTE: This list is basically a TODO list for things that need to be
> > >  * converted to driver model. So don't add new things here unless there is a
> > >  * good reason why driver-model conversion is infeasible. Examples include
> > >  * things which are used before driver model is available.
> > >
> > > This is effectively a list of things that should be converted to
> > > driver model. The list should then go away.  
> >
> > Hmm. But can't that be simply made into a list in a comment? Because
> > currently this is compiled in for every board that uses any such
> > function from fdtdec, even if they don't use any string from those
> > compatible strings.  
> 
> Well another option would be to delete the boards that need it, since
> no one has seen fit to resolve this all these years. Or create some
> drivers for them.

I still don't quite understand why the compatible strings have to be in
this fdtdec.c file.

For example in
  arch/arm/mach-socfpga/clock_manager_arria10.c
we call
  node = fdtdec_next_compatible(blob, 0,
                       COMPAT_ALTERA_SOCFPGA_CLK_INIT);
This constant, COMPAT_ALTERA_SOCFPGA_CLK_INIT, is an enum constant,
which is then in fdtdec.c translated to string compatible with
  compat_names[id]
for which fdt_node_offset_by_compatible() is used.

So why not simply put this string constant into
  arch/arm/mach-socfpga/clock_manager_arria10.c
by calling
  node = fdt_node_offset_by_compatible(blob, node,
                        "altr,socfpga-a10-clk-init");
??

That way at least the string literals won't be compiled in into every
u-boot binary, even those that don't need those literals at all.

> >  
> > > >
> > > > What is the purpose of having two files implementing fdt stuff?  
> > >
> > > fdtdec - for reading from the DT. Should go away and be replaced with
> > > the ofnode API, and fdtaddr.c
> > > fdt_support - for updating the DT, e.g. for fixups before booting an OS  
> >
> > Thanks! Okay that makes sense. This means that we should also have
> > fdt_node_is_available() in fdt_support.c which does the same thing.
> > (For now this can be made a static inline function that calls
> >  fdtdec_get_is_enabled().)  
> 
> OK.
> 
> BTW fdt_support.c should really move to use ofnode. There are few
> ofnode 'write' functions at present, but we should fill those out so
> we don't need to use flattree for anything, with OF_LIVE is enabled.

Can OF_LIVE then generate dtb for kernel?

Marek

Re: difference between fdtdec and fdt_support ?

[Simon Glass]

Hi Marek,

On Thu, 6 Jan 2022 at 09:55, Marek Behún <marek.behun@nic.cz> wrote:
>
> On Thu, 6 Jan 2022 09:15:17 -0700
> Simon Glass <sjg@chromium.org> wrote:
>
> > Hi Marek,
> >
> > On Thu, 6 Jan 2022 at 09:10, Marek Behún <marek.behun@nic.cz> wrote:
> > >
> > > On Thu, 6 Jan 2022 08:48:48 -0700
> > > Simon Glass <sjg@chromium.org> wrote:
> > >
> > > > Hi Marek,
> > > >
> > > > On Thu, 6 Jan 2022 at 05:21, Marek Behún <marek.behun@nic.cz> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > I am a little confused.
> > > > >
> > > > > We have
> > > > >   common/fdt_support.c
> > > > > and
> > > > >   lib/fdtdec.c
> > > > >
> > > > > The second one implements for example fdtdec_get_is_enabled(), which I
> > > > > would rather expect in fdt_support by name fdt_node_is_available(), or
> > > > > something like that.
> > > >
> > > > Should be moved to ofnode
> > >
> > > ?? But this function is needed for example when fixing device tree for
> > > Linux.
> > >
> > > > >
> > > > > Also fdtdec does a strange thing with compatible strings: it declares
> > > > > an enum for compatible strings and then a map which maps this enum
> > > > > values to compatible strings... Why not just use the compatible strings?
> > > >
> > > > Did you see the comment?
> > > >
> > > >  * NOTE: This list is basically a TODO list for things that need to be
> > > >  * converted to driver model. So don't add new things here unless there is a
> > > >  * good reason why driver-model conversion is infeasible. Examples include
> > > >  * things which are used before driver model is available.
> > > >
> > > > This is effectively a list of things that should be converted to
> > > > driver model. The list should then go away.
> > >
> > > Hmm. But can't that be simply made into a list in a comment? Because
> > > currently this is compiled in for every board that uses any such
> > > function from fdtdec, even if they don't use any string from those
> > > compatible strings.
> >
> > Well another option would be to delete the boards that need it, since
> > no one has seen fit to resolve this all these years. Or create some
> > drivers for them.
>
> I still don't quite understand why the compatible strings have to be in
> this fdtdec.c file.
>
> For example in
>   arch/arm/mach-socfpga/clock_manager_arria10.c
> we call
>   node = fdtdec_next_compatible(blob, 0,
>                        COMPAT_ALTERA_SOCFPGA_CLK_INIT);
> This constant, COMPAT_ALTERA_SOCFPGA_CLK_INIT, is an enum constant,
> which is then in fdtdec.c translated to string compatible with
>   compat_names[id]
> for which fdt_node_offset_by_compatible() is used.
>
> So why not simply put this string constant into
>   arch/arm/mach-socfpga/clock_manager_arria10.c
> by calling
>   node = fdt_node_offset_by_compatible(blob, node,
>                         "altr,socfpga-a10-clk-init");
> ??
>
> That way at least the string literals won't be compiled in into every
> u-boot binary, even those that don't need those literals at all.

Yes, indeed, but really these uses should be dropped and converted to
driver model.

>
> > >
> > > > >
> > > > > What is the purpose of having two files implementing fdt stuff?
> > > >
> > > > fdtdec - for reading from the DT. Should go away and be replaced with
> > > > the ofnode API, and fdtaddr.c
> > > > fdt_support - for updating the DT, e.g. for fixups before booting an OS
> > >
> > > Thanks! Okay that makes sense. This means that we should also have
> > > fdt_node_is_available() in fdt_support.c which does the same thing.
> > > (For now this can be made a static inline function that calls
> > >  fdtdec_get_is_enabled().)
> >
> > OK.
> >
> > BTW fdt_support.c should really move to use ofnode. There are few
> > ofnode 'write' functions at present, but we should fill those out so
> > we don't need to use flattree for anything, with OF_LIVE is enabled.
>
> Can OF_LIVE then generate dtb for kernel?

It isn't implemented, but we should add it. It is a simple piece of
code to flatten the tree.

Regards,
Simon