On 5/6/20 16:29, Walter Lozano wrote:
Hi Tom,
On 5/6/20 13:32, Tom Rini wrote:
On Fri, Jun 05, 2020 at 01:18:00PM -0300, Walter Lozano wrote:
Hi Tom,
On 5/6/20 11:20, Tom Rini wrote:
On Thu, Jun 04, 2020 at 09:13:06PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 2 Jun 2020 at 07:53, Tom Rini <tr...@konsulko.com> wrote:
On Mon, Jun 01, 2020 at 05:55:31PM -0300, Walter Lozano wrote:
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote:
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini <tr...@konsulko.com>
wrote:
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini <tr...@konsulko.com>
wrote:
On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote:
This patch provides the documentation for a proposed
enhancement to driver
model to reduce overhead in SPL.
The actual patches are not included here because
they are based on some
pending work by Walter Lozano which is not in final form.
For now, the source tree is available at:
https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
Comments welcome!
So here's my worry. It's not clear, aside from the device
tree, how
much re-use of existing code we get with this. It feels
like it might
be fairly minimal. And at that point, are we not perhaps
making too
much work for ourselves compared with just excepting that
there will
need to be a place for non-abstracted-framework drivers?
What do we do
about TPL, when we get to the point of everything being
converted to DM
and as-needed tiny-DM but there's still TPL drivers?
The reason we have
SPL_FRAMEWORK as a symbol today is that we already had some
SoCs/architectures (primarily PowerPC) that had "SPL" but
it was very
centric to the SoCs in question.
The interface for example for mmc is:
int spl_mmc_load_image(struct spl_image_info *spl_image,
struct
spl_boot_device *bootdev) and neither part of that is
inherently DM. So
let it be MMC_TINY for the non-DM case and regular DM_MMC
for the DM
case. I wonder if we could clean that up code a little
if we let it be
separate.
The interface for example for spi is:
int spl_spi_load_image(struct spl_image_info *spl_image,
struct spl_boot_device *bootdev) and well, the same
thing. Or maybe we
can even push that up to the spi_flash_load() call.
But my worry is that a different set of abstractions here
are still
going to bring us in more overhead than writing drivers for
the
functionality we need directly, and if we define what's
allowed in this
limited case well, that might be good enough.
Some boards (e.g. x86) Need to read multiple things from the
SPI flash
(such as FSP binaries), so I still think we will want a generic
reading interface.
You could be right, but my hunch is that there is value in
having
things more generic and the cost should be minimal. The
value is that
hopefully writing a few C functions in the SPI driver will
be enough
to enable tiny SPI on an SoC, reusing much of the code in
the driver
(only the reading bits!). We won't need as much special-case
code and
an entirely different way of configuring these devices for
TPL/SPL.
It has been interesting digging into the Zephyr model. It's
drivers
are very basic and thus small. But there is still value in
using the
device tree to assemble things.
Anyway I'm not really sure at this point. It is just a
hunch. I don't
think we can know all this until we have a bit more
information.
Perhaps with a board with SPI, MMC and serial converted we
would get a
better picture?
I think it's absolutely the case that we'll have to convert
something
and see how it looks, then convert something else and see if
it still
looks good enough. At a high enough level there's not really
too much
of a difference between what it sounds like you're proposing
and what
I'm proposing. Possibly even in a progmatic way too. We
have (I think
anyhow) fairly static board configurations in this case so we
don't so
much need to "probe" for possible drivers be told what our
device
hierarchy is and to initialize what we're going to use.
Yes, we may end up with special, separate code anyway, since
if you
end up refactoring the driver so much (and putting tiny-dm
tentacles
into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with
the idea
of just direct calls into the driver (e.g. the driver implements
serial_putc() and spi_read_flash()). But then I figured it is
a very
small overhead to retain some sort of driver model, so I
thought I'd
try that.
I'll fiddle with this again in a week or so...
Thanks for this proposal.
I'm very interested in see where this implementation leads us,
as I
always felt that some work could be done in order to reduce the
overhead
of DM support in TPL/SPL. I'll review this work and hopefully
come back
to you with some comments.
In the same sense, I feel that maybe we can add some additional
intelligence to dtoc in order to produce a more customized code
for
TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi,
but this is
only a feeling.
I've been checking your work and found it very interesting. Let
me share
some comments
I really like the trade-off between size and features in this
implementation
and the way you get rid of not very useful data, such as strings
and class
overhead.
I see that you parse drivers in order to build the relationship
between
drivers and compatible strings among other things. I faced a
similar
requirement in the series I proposed, however, I proposed the
use of a
U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias.
Then main
reason behind this were to make code cleaner, avoid a lot of
parsing and
having to take into account possible valid C code which is not
cover by the
parsing.
I have no problem with your approach, on the contrary I like the
idea of
improving dtoc, but I think that if we take this path, we should
put some
constrains and to document them to avoid unexpected behavior.
Most of the
constrains maybe already used by all the drivers, like the way
we declare
compatible strings, however any limitation in the parsing should be
documented.
The same goes for parsing config files which is also a nice
improvement. I
feel that every step we give adding intelligence to dtoc is a
step towards
footprint improvement.
Would this allow us to make progress towards the idea of being
able to
discard compatibles (and referenced data) that won't be used at
run time
given the dts files for the boards we'll support at run time?
Do you mean just the compatible strings, or do you mean whole
drivers?
Compatible and data. One of the "this is ugly but works" patches to
reduce size on i.MX families was to guard compatible string + data
(see
drivers/mmc/fsl_esdhc_imx.c for example) with compile time checks. If
we could automate that somehow, that would be good.
Could you point to the "ugly but works" patch in order to have better
context?
http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-ag...@denx.de/
Thanks for pointing at it, now the context is clear.
Regarding drivers/mmc/fsl_esdhc_imx.c I have been working in adding
OF_PLATDATA support to it in order to reduce the SPL footprint on iMX6,
which I think it could be a first step for further improvements.
https://patchwork.ozlabs.org/project/uboot/list/?series=167495&state=*
Note that we're not talking about just SPL here. We have platforms that
very specifically care about full U-Boot size. In addition, we care in
general about the overall binary size. Think of it this way, especially
in the case the i.MX patch above shows. Why should a 32bit i.MX6
platform grow in binary size as part of adding support for the latest
64bit i.MX8 part, when we're talking about just adding some form or
another of a table.
I see your point and the example is very clear. Let me think about
this, as I said I think there are a lot of room for improvements if we
add a bit more of intelligence to dtoc.
Following the discussion related to [PATCH v1 00/15] add basic driver
support for broadcom NS3 soc
https://patchwork.ozlabs.org/project/uboot/list/?series=177442
As I mentioned in the other thread, there might be some possible
improvements like
1- Use OF_PLATDATA, which already works and is being improved, to
convert DTB to C structures, however this require driver support.
2- Improve dtoc to generate configuration variables based on DTB, to
allow to enable some pieces of code only if DTB contains specific
properties, this could be useful to avoid adding configuration options
than have to match with DTB properties in order to work.
3- Improve dtoc to generate C code based on DTB information, for
instance the list of compatible strings. The specific example of
compatible strings makes sense only if we don't support OF_PLATDATA.
4- Improve dtoc to filter nodes from DTB, however I still see some need
of manual work in order to specify which nodes to keep, similar to the
u-boot.dtsi approach. Maybe with some additional intelligence, only
nodes that have a driver enabled you be kept? However, this could
checked carefully as it could lead to issues at runtime difficult to debug.
Regards,
Walter
