[RFC PATCH v2 0/3] RFC: tiny-dm: Proposal for using driver model in SPL
Simon Glass
sjg at chromium.org
Sun Aug 16 05:06:42 CEST 2020
Hi Walter,
On Sun, 26 Jul 2020 at 20:45, Walter Lozano <walter.lozano at 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
More information about the U-Boot
mailing list