[RFC PATCH v2 0/3] RFC: tiny-dm: Proposal for using driver model in SPL

[Simon Glass <sjg@chromium.org>]

Hi Walter,

On Sun, 26 Jul 2020 at 20:45, Walter Lozano <walter.lozano@collabora.com> wrote:
>
> Hi Simon,
>
>
> On 10/7/20 01:12, Walter Lozano wrote:
> > Hi Simon,
> >
> > On 2/7/20 18:10, Simon Glass wrote:
> >> This series provides a proposed enhancement to driver model to reduce
> >> overhead in SPL.
> >>
> >> These patches should not be reviewed other than to comment on the
> >> approach. The code is all lumped together in a few patches and so cannot
> >> be applied as is.
> >>
> >> For now, the source tree is available at:
> >>
> >> https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
> >>
> >> Comments welcome!
> >>
> >> Benefits (good news)
> >> --------------------
> >>
> >> As an example of the impact of tiny-dm, chromebook_jerry is converted to
> >> use it. This shows approximately a 30% reduction in code and data
> >> size and
> >> a 85% reduction in malloc() space over of-platdata:
> >>
> >>     text       data        bss        dec        hex    filename
> >>    25248       1836         12      27096       69d8 spl/u-boot-spl
> >> (original with DT)
> >>    19727       3436         12      23175       5a87 spl/u-boot-spl
> >> (OF_PLATDATA)
> >>      78%    187%    100%     86%         as %age of original
> >>
> >>    13784       1408         12      15204       3b64 spl/u-boot-spl
> >> (SPL_TINY)
> >>      70%     41%    100%     66%         as %age of platdata
> >>      55%     77%    100%     56%         as %age of original
> >>
> >> SPL malloc() usage drops from 944 bytes (OF_PLATDATA) to 116 (SPL_TINY).
> >>
> >> Overall the 'overhead' of tiny-dm is much less than the full driver
> >> model. Code size is currently about 600 bytes for these functions on
> >> Thumb2:
> >>
> >>     00000054 T tiny_dev_probe
> >>     00000034 T tiny_dev_get_by_drvdata
> >>     00000024 T tiny_dev_find
> >>     0000001a T tiny_dev_get
> >>     0000003c T tinydev_alloc_data
> >>     0000002a t tinydev_lookup_data
> >>     00000022 T tinydev_ensure_data
> >>     00000014 T tinydev_get_data
> >>     00000004 T tinydev_get_parent
> >>
> >> Effort (bad news)
> >> -----------------
> >>
> >> Unfortunately it is quite a bit of work to convert drivers over to
> >> tiny-dm. First, the of-platdata conversion must be done. But on top of
> >> that, tiny-dm needs entirely separate code for dealing with devices.
> >> This
> >> means that instead of 'struct udevice' and 'struct uclass' there is just
> >> 'struct tinydev'. Each driver and uclass must be modified to support
> >> both, pulling common code into internal static functions.
> >>
> >> Another option
> >> --------------
> >>
> >> Note: It is assumed that any board that is space-contrained should use
> >> of-platdata in SPL (see doc/driver-model/of-plat.rst). This is shown to
> >> reduce device-tree overhead by approximately 4KB.
> >>
> >> Designing tiny-dm has suggested a number of things that could be changed
> >> in the current driver model to make it more space-efficient for TPL and
> >> SPL. The ones with least impact on driver code are (CS=reduces code
> >> size,
> >> DS=reduces data size):
> >>
> >>     CS - drop driver_bind() and create devices (struct udevice) at
> >>          build-time
> >>     CS - allocate all device- and uclass-private data at build-time
> >>     CS - remove all but the basic operations for each uclass (e.g. SPI
> >>          flash only supports reading)
> >>     DS - use 8-bit indexes instead of 32/64-bit pointers for device
> >>          pointers possible since these are created at build-time)
> >>     DS - use singly-linked lists
> >>     DS - use 16-bit offsets to private data, instead of 32/64-bit
> >> pointers
> >>          (possible since it is all in SRAM relative to malloc() base,
> >>          presumably word-aligned and < 256KB)
> >>     DS - move private pointers into a separate data structure so that
> >> NULLs
> >>          are not stored
> >>     CS / DS - Combine req_seq and seq and calculate the new value at
> >>          build-time
> >>
> >> More difficult are:
> >>
> >>     DS - drop some of the lesser-used driver and uclass methods
> >>     DS - drop all uclass methods except init()
> >>     DS - drop all driver methods except probe()
> >>     CS / DS - drop uclasses and require drivers to manually call uclass
> >>          functions
> >>
> >> Even with all of this we would not reach tiny-dm and it would muddy
> >> up the
> >> driver-model datas structures. But we might come close to tiny-dm on
> >> size
> >> and there are some advantages:
> >>
> >> - much of the benefit would apply to all boards that use of-platdata
> >> (i.e.
> >>    with very little effort on behalf of board maintainers)
> >> - the impact on the code base is much less (we keep a single, unified
> >>    driver mode in SPL and U-Boot proper)
> >>
> >> Overall I think it is worth looking at this option. While it doesn't
> >> have
> >> the 'nuclear' impact of tiny-dm, neither does it mess with the U-Boot
> >> driver code as much and it is easier to learn.
> >
> > Thanks for your hard work on this topic.
> >
> > I think that there is great value in this research and in this
> > conclusion. It is clear that there two different approaches, but I
> > feel that the improvement to  the current DM implementation would have
> > a higher impact in the community.
> >
> > Since the first version of this proposal I have been thinking in a
> > solution that takes some of the advantages of tiny-dm idea but that
> > does not require so much effort. This seems to be aligned with what
> > you have been explaining in this section.
> >
> > I found interesting your proposal about simplification some data
> > structures. In this sense one of my ideas, a bit biased by some of the
> > improvements in dtoc, is to change the the definition of struct driver
> > based on if OF_PLATDATA is enabled, and in this case remove some
> > properties.
> >
> > struct driver {
> > #if !CONFIG_IS_ENABLED(OF_PLATDATA)
> >         char *name;
> > #endif
> >         enum uclass_id id;
> > #if !CONFIG_IS_ENABLED(OF_PLATDATA)
> >         const struct udevice_id *of_match;
> > #endif
> >         int (*bind)(struct udevice *dev);
> >         int (*probe)(struct udevice *dev);
> >         int (*remove)(struct udevice *dev);
> >         int (*unbind)(struct udevice *dev);
> >         int (*ofdata_to_platdata)(struct udevice *dev);
> >         int (*child_post_bind)(struct udevice *dev);
> >         int (*child_pre_probe)(struct udevice *dev);
> >         int (*child_post_remove)(struct udevice *dev);
> >         int priv_auto_alloc_size;
> >         int platdata_auto_alloc_size;
> >         int per_child_auto_alloc_size;
> >         int per_child_platdata_auto_alloc_size;
> >         const void *ops;        /* driver-specific operations */
> >         uint32_t flags;
> > #if CONFIG_IS_ENABLED(ACPIGEN)
> >         struct acpi_ops *acpi_ops;
> > #endif
> > };
> >
> > By just removing those properties, we save some bytes as we get rid of
> > several strings. Also maybe it would be nice to add some macros to
> > make it cleaner in drivers to use or not those properties, instead of
> > adding lots of #if.
> >
> > I feel, as you clearly expressed, that some additional refactotring
> > can be made to make the logic be more similar to the tiny-dm one. I
> > also found interesting that several of your proposals will have impact
> > in U-Boot, not only in TPL/SPL.
>
>
> Just to be a bit more clear, I was thinking in something like
>
>
> diff --git a/include/dm/device.h b/include/dm/device.h
> index f5738a0cee..0ee239be8f 100644
> --- a/include/dm/device.h
> +++ b/include/dm/device.h
> @@ -203,6 +203,16 @@ struct udevice_id {
>   #define of_match_ptr(_ptr)     NULL
>   #endif /* CONFIG_IS_ENABLED(OF_CONTROL) */
>
> +#if !CONFIG_IS_ENABLED(OF_PLATDATA)
> +#undef OF_PLATDATA_TINY
> +#define STRUCT_FIELD(x) x;
> +#define STRUCT_VALUE(x) x,
> +#else
> +#define OF_PLATDATA_TINY
> +#define STRUCT_FIELD(x)
> +#define STRUCT_VALUE(x)
> +#endif
> +
>   /**
>    * struct driver - A driver for a feature or peripheral
>    *
> @@ -252,9 +262,9 @@ struct udevice_id {
>    * allowing the device to add things to the ACPI tables passed to Linux
>    */
>   struct driver {
> -       char *name;
> +       STRUCT_FIELD(char *name)
>          enum uclass_id id;
> -       const struct udevice_id *of_match;
> +       STRUCT_FIELD(const struct udevice_id *of_match)

I didn't actually reply to this, but I think this makes sense. Perhaps
have a DM_ prefix on your #defines?
(

>          int (*bind)(struct udevice *dev);
>          int (*probe)(struct udevice *dev);
>          int (*remove)(struct udevice *dev);
> diff --git a/drivers/core/simple-bus.c b/drivers/core/simple-bus.c
> index 7cc1d46009..e303d59daf 100644
> --- a/drivers/core/simple-bus.c
> +++ b/drivers/core/simple-bus.c
> @@ -57,8 +57,8 @@ static const struct udevice_id generic_simple_bus_ids[] = {
>   };
>
>   U_BOOT_DRIVER(simple_bus) = {
> -       .name   = "simple_bus",
> +       STRUCT_VALUE(.name      = "simple_bus")
>          .id     = UCLASS_SIMPLE_BUS,
> -       .of_match = generic_simple_bus_ids,
> +       STRUCT_VALUE(.of_match = generic_simple_bus_ids)
>          .flags  = DM_FLAG_PRE_RELOC,
>   };
>
>
> Please don't pay attention to how OF_PLATDATA_TINY is implemented, it is
> just part of the test. I think it should be a configuration option that
> removes some functionality from OF_PLATDATA, like having the overhead of
> the strings. On top of this you could add additional improvements, like
> the binding implementation in tiny-dm which uses DM_REF_TINY_DRIVER.
>

Yes OK. I am thinking of starting with just adding a parent. But time
is not my friend at present so it will probably be a month or so.

Regards,
SImon