Hi David,
On Tue, 19 Nov 2019 at 21:01, David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Nov 12, 2019 at 06:54:17PM -0700, Simon Glass wrote:
> > There does not seem to be a strong reason to inline this function.
>
> Well.. I think my rationale was just that this function should be a
> single load, followed by the call to fdt_header_size_(). I suspect
> it's not called enough for removing that to balance out the code
> overhead of an extra function. Or have you made measurements which
> show I'm wrong about that?
Well it isn't called in U-Boot at all, other than via
fdt_check_header(). The inlined version saves 16 bytes of code space
on Thumb2.
But even better would be to drop fdt_check_header_(). something like:
size_t fdt_header_size(uint32_t version)
{
if (fdt_chk_version()) {
if (version <= 1)
return FDT_V1_SIZE;
else if (version <= 2)
return FDT_V2_SIZE;
else if (version <= 3)
return FDT_V3_SIZE;
else if (version <= 16)
return FDT_V16_SIZE;
}
return FDT_V17_SIZE;
}
That saves an additional 8 bytes.
>
> > Also
> > we are about to add some extra code to it which will increase its size.
>
> Will they, though? As far as I can tell, the only changes you make to
> this function in this series should be resolved at compile time. So
> that it should either be identical object code to what we have now, or
> even simpler (in the assume case it will be returning a fixed value,
> so in fact you'd probably get even better code size by allowing that
> to be inlined).
Hmm yes that's true about it resolving at compile time.
But inlining a compile-time check presumably is no better than having
it in a normal non-inlined function, since it is a compile-time check.
For your second part, see the function I wrote above.
>
> > Move it into fdt.c and use a simple declaration in libfdt.h
> >
> > Signed-off-by: Simon Glass <sjg@xxxxxxxxxxxx>
> > ---
> >
> > Changes in v4:
> > - Add fdt_header_size to version.lds
> >
> > Changes in v3:
> > - Add a new patch to de-inline fdt_header_size()
> >
> > Changes in v2: None
> >
> > libfdt/fdt.c | 5 +++++
> > libfdt/libfdt.h | 9 +++++----
> > libfdt/version.lds | 1 +
> > tests/testutils.c | 1 -
> > 4 files changed, 11 insertions(+), 5 deletions(-)
> >
> > diff --git a/libfdt/fdt.c b/libfdt/fdt.c
> > index d6ce7c0..3e37a4b 100644
> > --- a/libfdt/fdt.c
> > +++ b/libfdt/fdt.c
> > @@ -70,6 +70,11 @@ size_t fdt_header_size_(uint32_t version)
> > return FDT_V17_SIZE;
> > }
> >
> > +size_t fdt_header_size(const void *fdt)
> > +{
> > + return fdt_header_size_(fdt_version(fdt));
> > +}
> > +
> > int fdt_check_header(const void *fdt)
> > {
> > size_t hdrsize;
> > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > index fc4c496..48f375c 100644
> > --- a/libfdt/libfdt.h
> > +++ b/libfdt/libfdt.h
> > @@ -266,11 +266,12 @@ fdt_set_hdr_(size_dt_struct);
> > * fdt_header_size - return the size of the tree's header
> > * @fdt: pointer to a flattened device tree
> > */
> > +size_t fdt_header_size(const void *fdt);
> > +
> > +/**
> > + * fdt_header_size_ - internal function which takes a version number
> > + */
> > size_t fdt_header_size_(uint32_t version);
> > -static inline size_t fdt_header_size(const void *fdt)
> > -{
> > - return fdt_header_size_(fdt_version(fdt));
> > -}
> >
> > /**
> > * fdt_check_header - sanity check a device tree header
> > diff --git a/libfdt/version.lds b/libfdt/version.lds
> > index ae32924..7ab85f1 100644
> > --- a/libfdt/version.lds
> > +++ b/libfdt/version.lds
> > @@ -20,6 +20,7 @@ LIBFDT_1.2 {
> > fdt_get_alias_namelen;
> > fdt_get_alias;
> > fdt_get_path;
> > + fdt_header_size;
> > fdt_supernode_atdepth_offset;
> > fdt_node_depth;
> > fdt_parent_offset;
> > diff --git a/tests/testutils.c b/tests/testutils.c
> > index 5e494c5..53cb35f 100644
> > --- a/tests/testutils.c
> > +++ b/tests/testutils.c
> > @@ -261,7 +261,6 @@ void vg_prepare_blob(void *fdt, size_t bufsize)
> >
> > VALGRIND_MAKE_MEM_UNDEFINED(blob, bufsize);
> > VALGRIND_MAKE_MEM_DEFINED(blob, FDT_V1_SIZE);
> > - VALGRIND_MAKE_MEM_DEFINED(blob, fdt_header_size(fdt));
> >
> > if (fdt_magic(fdt) == FDT_MAGIC) {
> > off_strings = fdt_off_dt_strings(fdt);
>
BTW I didn't see any other comments on this series so though I might
as well reply on just this one patch.
Regards,
Simon
