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

[Walter Lozano]

Hi Tom,

On 5/6/20 13:32, Tom Rini wrote:
> 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 <trini at 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 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
>>>>>>
>>>>>>
>>>>>> 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-agust@denx.de/
Thanks for pointing at it, now the context is clear.
>> 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.
>
I see your point and the example is very clear. Let me think about this, 
as I said I think there are a lot of room for improvements if we add a 
bit more of intelligence to dtoc.

Regards,

Walter