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

[Simon Glass]

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.

Regards,
Simon