[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