Hi Ezequiel,
On 5/11/19 20:30, Ezequiel Garcia wrote:
On Tue, 5 Nov 2019 at 15:12, Walter Lozano <walter.loz...@collabora.com> wrote:
Hi Ezequiel,
On 5/11/19 13:56, Ezequiel Garcia wrote:
Hello Walter,
Thanks for the patch.
On Tue, 5 Nov 2019 at 12:27, Walter Lozano <walter.loz...@collabora.com> wrote:
The support of libfdt should only be needed when OF_CONTROL
is enabled and OF_PLATDATA is not, as in other cases there is no
DT file to query.
This patch fixes this dependency allowing to save some space.
Can you add some more information about the space saving?
Sure, I will add some additional information about the static footprint.
However according to my understanding the impact depends on how well
different drivers supports features like OF_PLATDATA. For instance, in
my current configuration it saves 2 KB.
Not sure I follow you. This patch adds a condition which adds/removes code
based on some conditions. So, it should depend on the arch, but otherwise
the reduction is an invariant as it just depend on the size of the
code that is being
added/removed. Or am I missing something?
The idea behind this patch is to break the dependency of libfdt when it
is not needed. A specific example of this is found when building SPL
using OF_PLATDATA, which basically removes DT so there is no sense of
having libfdt present in SPL. Unfortunately as OF_PLATDATA has little
support, drivers tend to assume that there is a DT and try to query
different properties, which of course return no data. In this context,
the idea is to keep the same behavior but reducing the SPL size, while
trying to improve drivers.
The ./scripts/bloat-o-meter will help you get some info
on static footprint.
Thanks for your suggestion. I think you are pointing to a script found
in the Linux kernel, right?
Ah, yes. I have this script imported on my U-Boot repo, so I assumed
it was here as well.
Thanks for confirm this.
I also think it could be useful to have a
deep understanding on how the static footprint of some .o files changes,
but it won't give us much information about the end result of the binary
image, which is affected by the dependencies and compiler/linker
optimizations. Is this correct?
Normally, bloat-o-meter gives you a rough idea and allows you to
weigh code complexity vs. size reduction.
I see, however, in this context the reduction is due to the remove of
dependencies.
Signed-off-by: Walter Lozano <walter.loz...@collabora.com>
---
drivers/core/ofnode.c | 132 +++++++++++++++++++++++++++++++++++++--
include/dm/read.h | 141 ++++++++++++++++++++++++++++++++++++++++++
2 files changed, 267 insertions(+), 6 deletions(-)
You should try to avoid adding #ifdefery to the code. Normally,
the way to ifdef code is by having this in a header:
#ifdef CONFIG_FOO
int foo_feature_optional(struct foo *priv);
#else
static inline int foo_feature_optional(struct foo *priv)
{
return 0;
}
#endif
The user of foo_feature_optional shouldn't be bothered with
FOO being enabled or not. Pushing ifdefs away from .c to .h
is a common pattern, well described in a classic old article:
http://www.literateprogramming.com/ifdefs.pdf
Do you think you can try to rework the patch to reduce its impact
as much as possible?
Thanks for your review and your suggestions, I will be happy to rework
this to improve it.
The intention of this RFC is to get some feedback about if this is
something worth to be added and if this is the right approach. The use
of OF_PLATDATA is quite limited as it needs drivers to support it, which
will probably be a long process. Enabling it but having some drivers to
query DT properties has no sense and requires additional dependencies,
like libfdt.
Also I think it should be possible to remove some extra components but
it will require extra work to avoid break some configurations.
It's always good to shave off bytes, specially in SPL -- not so much
in proper U-Boot, right?
Yes, this is specially interesting for SPL where the size constrain is
more problematic.
BTW, what is the motivation of this patch: which platform are you
working on, and
how is it size constrained?
I'm currently working on iMX6 where typically SPL size should be less
than 68 KB. That I think this approach (or other in the same direction)
could be useful in many other platforms with more severe restrictions.
Thanks,
Ezequiel
Thanks again Ezequiel for you review. I wil work in a better approach.
Regards,
Walter
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
