mbox series

[RFC,v2,0/3] RFC: tiny-dm: Proposal for using driver model in SPL

Message ID 20200702211004.1491489-1-sjg@chromium.org
Headers show
Series RFC: tiny-dm: Proposal for using driver model in SPL | expand

Message

Simon Glass July 2, 2020, 9:10 p.m. UTC
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.

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

Comments

Heinrich Schuchardt July 3, 2020, 2:19 a.m. UTC | #1
Am 2. Juli 2020 23:10:01 MESZ schrieb Simon Glass <sjg at chromium.org>:
>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.


The code size reductions sound quite impressive. 

Somehow I have missed the start of your investigation. Could you, please, summarize what the main differences between tiny DM and the current DM are and explain why tiny DM cannot be used in main U-Boot.

Best regards

Heinrich

>
>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.
>
>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
Simon Glass July 3, 2020, 3:03 a.m. UTC | #2
Hi Heinrich,

On Thu, 2 Jul 2020 at 20:24, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> Am 2. Juli 2020 23:10:01 MESZ schrieb Simon Glass <sjg at chromium.org>:
> >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.
>
>
> The code size reductions sound quite impressive.
>
> Somehow I have missed the start of your investigation. Could you, please, summarize what the main differences between tiny DM and the current DM are and explain why tiny DM cannot be used in main U-Boot.

See patch 2 (doc/driver-model/tiny-dm.rst). I probably didn't send out
v1 widely enough, but it was only a starting point then.

It is quite limited and it would be fiddly to use in U-Boot proper.
Things like automatic power domains, uclass features like setting up a
bus and full devicetree access are not available with tiny DM.

Regards,
Simon

>
> Best regards
>
> Heinrich
>
> >
> >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.
> >
> >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
>
> --
> Diese Nachricht wurde von meinem Android-Ger?t mit K-9 Mail gesendet.
Simon Glass July 10, 2020, 1:12 a.m. UTC | #3
Hi,

On Thu, 2 Jul 2020 at 21:03, Simon Glass <sjg at chromium.org> wrote:
>
> Hi Heinrich,
>
> On Thu, 2 Jul 2020 at 20:24, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> >
> >
> >
> > Am 2. Juli 2020 23:10:01 MESZ schrieb Simon Glass <sjg at chromium.org>:
> > >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!

Are there any comments on this one? I am still not sure which is the
best approach.

Regards,
Simon
Walter Lozano July 10, 2020, 4:12 a.m. UTC | #4
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.

> 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
Walter Lozano July 27, 2020, 2:44 a.m. UTC | #5
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
Simon Glass Aug. 16, 2020, 3:06 a.m. UTC | #6
Hi Walter,

On Sun, 26 Jul 2020 at 20:45, Walter Lozano <walter.lozano@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
Walter Lozano Aug. 16, 2020, 3:17 a.m. UTC | #7
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@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