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/ > 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. -- Tom
signature.asc
Description: PGP signature
