[RFC PATCH v2 0/3] RFC: tiny-dm: Proposal for using driver model in SPL
Walter Lozano
walter.lozano at collabora.com
Sun Aug 16 05:17:41 CEST 2020
Hi Simon,
On 16/8/20 00:06, Simon Glass wrote:
> 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?
> (
Thanks for your reply and paying attention to my comments. I totally
agree that if you implement this idea you should rename #defines to make
them more readable.
>> 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.
Thanks for sharing your plans. Time is always an issue, but I think we
are in the right track, and that all the research you have done with
tiny-dm will give great benefits.
Regards,
Walter
More information about the U-Boot
mailing list