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

Walter Lozano walter.lozano at collabora.com
Fri Jun 5 22:20:45 CEST 2020 

On 5/6/20 16:29, Walter Lozano wrote:
> 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.
>

Following the discussion related to [PATCH v1 00/15] add basic driver 
support for broadcom NS3 soc

https://patchwork.ozlabs.org/project/uboot/list/?series=177442

As I mentioned in the other thread, there might be some possible 
improvements like

1- Use OF_PLATDATA, which already works and is being improved, to 
convert DTB to C structures, however this require driver support.

2- Improve dtoc to generate configuration variables based on DTB, to 
allow to enable some pieces of code only if DTB contains specific 
properties, this could be useful to avoid adding configuration options 
than have to match with DTB properties in order to work.

3- Improve dtoc to generate C code based on DTB information, for 
instance the list of compatible strings. The specific example of 
compatible strings makes sense only if we don't support OF_PLATDATA.

4- Improve dtoc to filter nodes from DTB, however I still see some need 
of manual work in order to specify which nodes to keep, similar to the 
u-boot.dtsi approach. Maybe with some additional intelligence, only 
nodes that have a driver enabled you be kept? However, this could 
checked carefully as it could lead to issues at runtime difficult to debug.

Regards,

Walter

More information about the U-Boot mailing list