Hi Walter, On Mon, 1 Jun 2020 at 14:55, Walter Lozano <walter.loz...@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 <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 >
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
