On Mon, Dec 02, 2019 at 06:07:16PM -0700, Simon Glass wrote:
> 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.
Uh.. I'm a little lost here. Is what you're suggesting moving all of
fdt_check_header_() to the .h file, so it can be inlined (and
therefore constant folded)?
That could be a reasonable plan, though we'll need to make sure to
include an out-of-line version as well, for ABI compatibility.
> > > 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.
Sorry, I've just had real trouble making time for this amongst my day
job.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
signature.asc
Description: PGP signature
