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

[Simon Glass]

Hi,

On Thu, 4 Jun 2020 at 21:11, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Walter,
>
> On Mon, 1 Jun 2020 at 14:55, Walter Lozano <walter.lozano at collabora.com> 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 at 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 at 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
> >
>
> Thank you for your 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.
>
> Yes there is a trade-off here and I'm not sure which is best. I am
> quite suspicious of 'magic' things because they are hard for people to
> understand.
>
> >
> >
> > 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.
>
> Yes, and again it has to be worth it. So far I am just picking up the
> CONFIG_SPL_TINY_SERIAL (etc.) options to find out which subsystems are
> tiny.
>
> >
> >
> > Another thing to discuss and document are the guidelines to implement
> > the functions similar functions like probe() and tiny_probe(), in such a
> > way to try to avoid code duplication.
>
> Yes. That should be somewhat possible, by passing arguments to core
> functions, but a little painful, similar to of-platdata.
>
> >
> >
> > Lastly, I have tried to test sandbox_spl as I understand it should work
> > based on you comments, but I receive some errors when running dtoc
> >
> >    DTOC C  spl/dts/dt-platdata.c
> > Traceback (most recent call last):
> >    File "./tools/dtoc/dtoc", line 116, in <module>
> >      options.include_disabled, options.output)
> >    File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 833, in run_steps
> >      plat.generate_tables()
> >    File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 794, in generate_tables
> >      self.output_node(node)
> >    File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 722, in output_node
> >      (val))
> > ValueError: Cant' find driver for compatible '['sandbox_serial']
>
> Er yes, I moved over to a real board halfway through, and sandbox
> stopped working. Will fix it at some point.

One more thought that has been rattling around is that we could try a
more moderate idea. Tiny-DM is sort-of the nuclear approach - minimum
possible size at the cost of a somewhat parallel setup, at least in
terms of code.

Another idea would be to reduce the size of the DM data structures by
having two versions of them - a tiny one and a full one. We might be
able handle it automatically. E.g drop the platdata sizes (or compress
them), drop the pre-probe options and other things used only on i2c
and more complicated things. Use singly-linked lists. Use 16-bit
offsets for memory allocation (and perhaps even other things) instead
of pointers. It would trade off speed for space.

The benefit would be that it would look more like DM on the surface,
with less driver infrastructure to modify. Walter's series shows what
can be done by automatic processing with very little user effort.

The cost, of course, is more complexity, mostly under the hood. It
would inevitably produce more code as well, although perhaps not much.

Regards,
Simon