[PATCH] RFC: tiny-dm: Proposal for using driver model in SPL

Simon Glass
2020-06-05T03:13:06Z

Hi Tom,

On Tue, 2 Jun 2020 at 07:53, Tom Rini <trini@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 <trini@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 <trini@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?

Regards,
Simon