[RFC PATCH v2 0/3] RFC: tiny-dm: Proposal for using driver model in SPL
Walter Lozano
walter.lozano at collabora.com
Mon Jul 27 04:44:49 CEST 2020
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)
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.
>
>> Changes in v2:
>> - Various updates, and ported to chromebook_jerry (rockchip)
>>
>> Simon Glass (3):
>> dm: Driver and uclass changes for tiny-dm
>> dm: Arch-specific changes for tiny-dm
>> dm: Core changes for tiny-dm
>>
>> arch/arm/dts/rk3288-u-boot.dtsi | 17 +-
>> arch/arm/dts/rk3288-veyron.dtsi | 26 +-
>> arch/arm/dts/rk3288.dtsi | 3 +
>> arch/arm/include/asm/arch-rockchip/clock.h | 9 +
>> .../include/asm/arch-rockchip/sdram_rk3288.h | 21 ++
>> arch/arm/include/asm/arch-rockchip/spi.h | 1 +
>> arch/arm/include/asm/io.h | 18 +
>> arch/arm/mach-rockchip/rk3288/clk_rk3288.c | 46 +++
>> arch/arm/mach-rockchip/rk3288/rk3288.c | 2 +
>> arch/arm/mach-rockchip/rk3288/syscon_rk3288.c | 45 +--
>> arch/arm/mach-rockchip/sdram.c | 33 +-
>> arch/sandbox/cpu/u-boot-spl.lds | 12 +
>> arch/sandbox/dts/sandbox.dts | 3 +-
>> arch/sandbox/dts/sandbox.dtsi | 2 +-
>> arch/x86/cpu/apollolake/cpu_spl.c | 5 +-
>> arch/x86/cpu/apollolake/uart.c | 56 ++++
>> arch/x86/dts/chromebook_coral.dts | 1 +
>> arch/x86/lib/tpl.c | 4 +-
>> board/Synology/ds109/ds109.c | 3 +-
>> common/console.c | 2 +-
>> common/log.c | 36 +-
>> common/malloc_simple.c | 31 ++
>> common/spl/spl.c | 17 +-
>> common/spl/spl_spi.c | 91 +++--
>> configs/chromebook_coral_defconfig | 1 +
>> configs/chromebook_jerry_defconfig | 11 +
>> configs/rock2_defconfig | 3 +
>> doc/develop/debugging.rst | 35 ++
>> doc/driver-model/tiny-dm.rst | 315 +++++++++++++++++
>> drivers/clk/Kconfig | 54 +++
>> drivers/clk/Makefile | 4 +-
>> drivers/clk/clk-uclass.c | 53 ++-
>> drivers/clk/rockchip/clk_rk3288.c | 106 ++++--
>> drivers/core/Kconfig | 106 ++++++
>> drivers/core/Makefile | 3 +
>> drivers/core/of_extra.c | 49 ++-
>> drivers/core/regmap.c | 3 +-
>> drivers/core/syscon-uclass.c | 68 +++-
>> drivers/core/tiny.c | 249 ++++++++++++++
>> drivers/mtd/spi/Kconfig | 18 +
>> drivers/mtd/spi/sf-uclass.c | 76 +++++
>> drivers/mtd/spi/sf_probe.c | 4 +
>> drivers/mtd/spi/spi-nor-tiny.c | 166 ++++++++-
>> drivers/ram/Kconfig | 18 +
>> drivers/ram/ram-uclass.c | 12 +
>> drivers/ram/rockchip/sdram_rk3188.c | 2 +-
>> drivers/ram/rockchip/sdram_rk322x.c | 2 +-
>> drivers/ram/rockchip/sdram_rk3288.c | 231 ++++++++-----
>> drivers/ram/rockchip/sdram_rk3328.c | 2 +-
>> drivers/ram/rockchip/sdram_rk3399.c | 2 +-
>> drivers/reset/reset-rockchip.c | 4 +-
>> drivers/serial/Kconfig | 38 +++
>> drivers/serial/ns16550.c | 195 +++++++++--
>> drivers/serial/sandbox.c | 59 +++-
>> drivers/serial/serial-uclass.c | 77 +++++
>> drivers/serial/serial_omap.c | 2 +-
>> drivers/serial/serial_rockchip.c | 59 ++++
>> drivers/spi/Kconfig | 18 +
>> drivers/spi/Makefile | 2 +
>> drivers/spi/rk_spi.c | 301 ++++++++++++-----
>> drivers/spi/spi-uclass.c | 77 +++++
>> drivers/sysreset/Kconfig | 18 +
>> drivers/sysreset/sysreset-uclass.c | 124 ++++---
>> drivers/sysreset/sysreset_rockchip.c | 61 +++-
>> include/asm-generic/global_data.h | 7 +-
>> include/clk-uclass.h | 11 +
>> include/clk.h | 32 +-
>> include/dm/device.h | 121 +++++++
>> include/dm/of_extra.h | 6 +
>> include/dm/platdata.h | 20 +-
>> include/dm/tiny_struct.h | 42 +++
>> include/linker_lists.h | 6 +
>> include/linux/mtd/mtd.h | 23 +-
>> include/linux/mtd/spi-nor.h | 22 ++
>> include/log.h | 6 +
>> include/malloc.h | 3 +
>> include/ns16550.h | 7 +-
>> include/ram.h | 25 ++
>> include/regmap.h | 4 +-
>> include/serial.h | 45 ++-
>> include/spi.h | 31 ++
>> include/spi_flash.h | 7 +
>> include/spl.h | 8 +-
>> include/syscon.h | 2 +
>> include/sysreset.h | 9 +
>> scripts/Makefile.spl | 6 +-
>> tools/dtoc/dtb_platdata.py | 316 +++++++++++++++---
>> tools/dtoc/dtoc_test_simple.dts | 12 +-
>> tools/dtoc/fdt.py | 7 +-
>> tools/dtoc/main.py | 9 +-
>> tools/dtoc/test_dtoc.py | 91 ++++-
>> tools/patman/tools.py | 4 +-
>> 92 files changed, 3454 insertions(+), 540 deletions(-)
>> create mode 100644 doc/develop/debugging.rst
>> create mode 100644 doc/driver-model/tiny-dm.rst
>> create mode 100644 drivers/core/tiny.c
>> create mode 100644 include/dm/tiny_struct.h
>>
>
Regards,
Walter
More information about the U-Boot
mailing list