From mboxrd@z Thu Jan 1 00:00:00 1970 From: Walter Lozano Date: Sun, 26 Jul 2020 23:44:49 -0300 Subject: [RFC PATCH v2 0/3] RFC: tiny-dm: Proposal for using driver model in SPL In-Reply-To: <9f1c745f-d124-bcfb-da6e-15b01b20a3d9@collabora.com> References: <20200702211004.1491489-1-sjg@chromium.org> <9f1c745f-d124-bcfb-da6e-15b01b20a3d9@collabora.com> Message-ID: List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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