[PATCH] RFC: tiny-dm: Proposal for using driver model in SPL
This patch provides the documentation for a proposed enhancement to driver model to reduce overhead in SPL.
The actual patches are not included here because they are based on some pending work by Walter Lozano which is not in final form.
For now, the source tree is available at:
https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
Comments welcome!
Signed-off-by: Simon Glass sjg@chromium.org ---
doc/driver-model/dm-tiny.rst | 315 +++++++++++++++++++++++++++++++++++ 1 file changed, 315 insertions(+) create mode 100644 doc/driver-model/dm-tiny.rst
diff --git a/doc/driver-model/dm-tiny.rst b/doc/driver-model/dm-tiny.rst new file mode 100644 index 0000000000..cc5c3e00b1 --- /dev/null +++ b/doc/driver-model/dm-tiny.rst @@ -0,0 +1,315 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +Tiny driver model (tiny-dm) +=========================== + +Purpose +------- + +Reduce the overhead of using driver model in SPL and TPL. + + +Introduction +------------ + +On some platforms that use SPL [1]_, SRAM is extremely limited. There is a +need to use as little space as possible for U-Boot SPL. + +With the migration to driver model and devicetree, the extra software complexity +has created more pressure on U-Boot's code and data size. + +A few features have been introduced to help with this problem: + + - fdtgrep, introduced in 2015, automatically removes unnecessary parts of the + device tree, e.g. those used by drivers not present in SPL. At the time, + this typically reduced SPL size from about 40KB to perhaps 3KB and made + it feasible to look at using driver model with SPL. The minimum overhead + was reduced to approximately 7KB on Thumb systems, for example [2]_ + - of-platdata, introduced in 2016 [3]_, converts the device tree into C data + structures which are placed in the SPL image. This saves approximately + 3KB of code and replaces the devicetree with something typically 30% + smaller. + +However the problem still exists. Even with of-platdata, the driver-model +overhead is typically 3KB at the minimum. This excludes the size of allocated +data structures, which is 84 bytes per device and 76 bytes per uclass on +32-bit machines. On 64-bit machines these sizes approximately double. + +With the driver-model migration deadlines passing, a solution is needed to +allow boards to complete migration to driver model in SPL, without taking on +the full ~5KB overhead that this entails. + + +Concept +------- + +The idea of tiny devices ('tiny-dm') builds on of-platdata, but additionally +removes most of the rich feature-set of driver model. + +In particular tiny-dm takes away the concept of a uclass (except that it stil +uses uclass IDs), drastically reduces the size of a device (to 16 bytes on +32-bit) and removes the need for a driver_info structure. + +With tiny-dm, dtoc outputs U_BOOT_TINY_DEVICE() instead of U_BOOT_DEVICE(). +A new 'struct tiny_dev' is used instead of 'struct udevice'. Devices can be +located based on uclass ID and sequence number with tiny_dev_find(). Devices can +be probed with tiny_dev_probe(). + +In fact, tiny-dm is effectively a bypass for most of driver model. It retains +some capability with in (chiefly by using the same device tree), but new code +is added to implement simple features in a simple way. + +Tiny-dm is not suitable for complex device and interactions, but it can +support a serial port (output only), I2C buses and other features needed to +set up the machine just enough to load U-Boot proper. + +It is possible to enable Tiny-dm on a subsystem-by-subsystem basis. For example, +enabling CONFIG_TPL_TINY_SERIAL on chromebook_coral saves about 900 bytes of +code and data, with no perceptable difference in operation. + + +Tiny devices +------------ + +Below is an example of a tiny device, a UART that uses NS16550. It works by +setting up a platform structure to pass to the ns16550 driver, perhaps the +worst driver in U-Boot. + +.. code-block:: c + + static int apl_ns16550_tiny_probe(struct tiny_dev *tdev) + { + struct dtd_intel_apl_ns16550 *dtplat = tdev->dtplat; + struct ns16550_platdata *plat = tdev->priv; + ulong base; + pci_dev_t bdf; + + base = dtplat->early_regs[0]; + bdf = pci_ofplat_get_devfn(dtplat->reg[0]); + + if (!CONFIG_IS_ENABLED(PCI)) + apl_uart_init(bdf, base); + + plat->base = base; + plat->reg_shift = dtplat->reg_shift; + plat->reg_width = 1; + plat->clock = dtplat->clock_frequency; + plat->fcr = UART_FCR_DEFVAL; + + return ns16550_tiny_probe_plat(plat); + } + + static int apl_ns16550_tiny_setbrg(struct tiny_dev *tdev, int baudrate) + { + struct ns16550_platdata *plat = tdev->priv; + + return ns16550_tiny_setbrg(plat, baudrate); + } + + static int apl_ns16550_tiny_putc(struct tiny_dev *tdev, const char ch) + { + struct ns16550_platdata *plat = tdev->priv; + + return ns16550_tiny_putc(plat, ch); + } + + struct tiny_serial_ops apl_ns16550_tiny_ops = { + .probe = apl_ns16550_tiny_probe, + .setbrg = apl_ns16550_tiny_setbrg, + .putc = apl_ns16550_tiny_putc, + }; + + U_BOOT_TINY_DRIVER(apl_ns16550) = { + .uclass_id = UCLASS_SERIAL, + .probe = apl_ns16550_tiny_probe, + .ops = &apl_ns16550_tiny_ops, + DM_TINY_PRIV(<ns16550.h>, sizeof(struct ns16550_platdata)) + }; + +The probe function is responsible for setting up the hardware so that the UART +can output characters. This driver enables the device on PCI and assigns an +address to its BAR (Base-Address Register). That code is in apl_uart_init() and +is not show here. Then it sets up a platdata data structure for use by the +ns16550 driver and calls its probe function. + +The 'tdev' device is declared like this in the device tree: + +.. code-block:: c + + serial: serial@18,2 { + reg = <0x0200c210 0 0 0 0>; + u-boot,dm-pre-reloc; + compatible = "intel,apl-ns16550"; + early-regs = <0xde000000 0x20>; + reg-shift = <2>; + clock-frequency = <1843200>; + current-speed = <115200>; + }; + +When dtoc runs it outputs the following code for this, into dt-platdata.c: + +.. code-block:: c + + static struct dtd_intel_apl_ns16550 dtv_serial_at_18_2 = { + .clock_frequency = 0x1c2000, + .current_speed = 0x1c200, + .early_regs = {0xde000000, 0x20}, + .reg = {0x200c210, 0x0}, + .reg_shift = 0x2, + }; + + DM_DECL_TINY_DRIVER(apl_ns16550); + #include <ns16550.h> + u8 _serial_at_18_2_priv[sizeof(struct ns16550_platdata)] __attribute__ ((section (".data"))); + U_BOOT_TINY_DEVICE(serial_at_18_2) = { + .dtplat = &dtv_serial_at_18_2, + .drv = DM_REF_TINY_DRIVER(apl_ns16550), + .priv = _serial_at_18_2_priv, + }; + +This basically creates a device, with a pointer to the dtplat data (a C +structure similar to the devicetree node) and a pointer to the driver, the +U_BOOT_TINY_DRIVER() thing shown above. + +So far, tiny-dm might look pretty similar to the full driver model, but there +are quite a few differences that may not be immediately apparent: + + - Whereas U_BOOT_DEVICE() emits a driver_info structure and then allocates + the udevice structure at runtime, U_BOOT_TINY_DEVICE() emits an actual + tiny_dev device structure into the image. On platforms where SPL runs in + read-only memory, U-Boot automatically copies this into RAM as needed. + - The DM_TINY_PRIV() macro tells U-Boot about the private data needed by + the device. But this is not allocated at runtime. Instead it is declared + in the C structure above. However on platforms where SPL runs in read-only + memory, allocation is left until runtime. + - There is a corresponding 'full' driver in the same file with the same name. + Like of-platdata, it is not possible to use tiny-dm without 'full' support + added as well. This makes sense because the device needs to be supported + in U-Boot proper as well. + - While this driver is in the UCLASS_SERIAL uclass, there is in fact no + uclass available. The serial-uclass.c implementation has an entirely + separate (small) piece of code to support tiny-dm: + +.. code-block:: c + + int serial_init(void) + { + struct tiny_dev *tdev; + int ret; + + tdev = tiny_dev_find(UCLASS_SERIAL, 0); + if (!tdev) { + if (IS_ENABLED(CONFIG_REQUIRE_SERIAL_CONSOLE)) + panic_str("No serial"); + return -ENODEV; + } + ret = tiny_dev_probe(tdev); + if (ret) + return log_msg_ret("probe", ret); + gd->tiny_serial = tdev; + gd->flags |= GD_FLG_SERIAL_READY; + serial_setbrg(); + + return 0; + } + + void serial_putc(const char ch) + { + struct tiny_dev *tdev = gd->tiny_serial; + struct tiny_serial_ops *ops; + + if (!tdev) + goto err; + + ops = tdev->drv->ops; + if (!ops->putc) + goto err; + if (ch == '\n') + ops->putc(tdev, '\r'); + ops->putc(tdev, ch); + + return; + err: + if (IS_ENABLED(DEBUG_UART)) + printch(ch); + } + + void serial_puts(const char *str) + { + for (const char *s = str; *s; s++) + serial_putc(*s); + } + + +When serial_putc() is called from within U-Boot, this code looks up the tiny-dm +device and sends it the character. + + +Potential costs and benefits +---------------------------- + +It is hard to estimate the savings to be had by switching a subsystem over to +tiny-dm. Further work will illuminate this. In the example above (on x86), +about 1KB bytes is saved (code and data), but this may or may not be +representative of other subsystems. + +If all devices in an image use tiny-dm then it is possible to remove all the +core driver-model support. This is the 3KB mentioned earlier. Of course, tiny-dm +has its own overhead, although it is substantialy less than the full driver +model. + +These benefits come with some drawbacks: + + - Drivers that want to use it must implement tiny-dm in addition to their + normal support. + - of-platdata must be used. This cannot be made to work with device tree. + - Tiny-dm drivers have none of the rich support provided by driver model. + There is no pre-probe support, no concept of buses holding information + about child devices, no automatic pin control or power control when a + device is probed. Tiny-dm is designed to save memory, not to make it easy + to write complex device drivers. + - Subsystems must be fully migrated to driver model with the old code + removed. This is partly a technical limitation (see ns16550.c for how ugly + it is to support both, let alone three) and partly a quid-pro-quo for + this feature, since it should remove existing concerns about migrating to + driver model. + + +Next steps +---------- + +This is currently an RFC so the final result may change somewhat from what is +presented here. Some features are missing, in particular the concept of sequence +numbers is designed but not implemented. The code is extremely rough. + +To judge the impact of tiny-dm a suitable board needs to be fully converted to +it. At present I am leaning towards rock2, since it already supports +of-platdata. + +The goal is to sent initial patches in June 2020 with the first version in +mainline in July 2020 ready for the October release. Refinements based on +feedback and patches received can come after that. It isn't clear yet when this +could become a 'stable' feature, but likely after a release or two, perhaps with +5-10 boards converted. + + +Trying it out +------------- + +The source tree is available at https://github.com/sjg20/u-boot/tree/dtoc-working + +Only two boards are supported at present: + + - sandbox_spl - run spl/u-boot-spl to try the SPL with tiny-dm + - chromebook_coral - TPL uses tiny-dm + + +.. [1] This discussion refers to SPL but for devices that use TPL, the same + features are available there. +.. [2] https://www.elinux.org/images/c/c4/%5COrder_at_last_-_U-Boot_driver_model_sl... +.. [3] https://elinux.org/images/8/82/What%27s_New_with_U-Boot_%281%29.pdf + + +.. Simon Glass sjg@chromium.org +.. Google LLC +.. Memorial Day 2020
On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote:
This patch provides the documentation for a proposed enhancement to driver model to reduce overhead in SPL.
The actual patches are not included here because they are based on some pending work by Walter Lozano which is not in final form.
For now, the source tree is available at:
https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
Comments welcome!
So here's my worry. It's not clear, aside from the device tree, how much re-use of existing code we get with this. It feels like it might be fairly minimal. And at that point, are we not perhaps making too much work for ourselves compared with just excepting that there will need to be a place for non-abstracted-framework drivers? What do we do about TPL, when we get to the point of everything being converted to DM and as-needed tiny-DM but there's still TPL drivers? The reason we have SPL_FRAMEWORK as a symbol today is that we already had some SoCs/architectures (primarily PowerPC) that had "SPL" but it was very centric to the SoCs in question.
The interface for example for mmc is: int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and neither part of that is inherently DM. So let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM case. I wonder if we could clean that up code a little if we let it be separate.
The interface for example for spi is: int spl_spi_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and well, the same thing. Or maybe we can even push that up to the spi_flash_load() call.
But my worry is that a different set of abstractions here are still going to bring us in more overhead than writing drivers for the functionality we need directly, and if we define what's allowed in this limited case well, that might be good enough.
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote:
This patch provides the documentation for a proposed enhancement to driver model to reduce overhead in SPL.
The actual patches are not included here because they are based on some pending work by Walter Lozano which is not in final form.
For now, the source tree is available at:
https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
Comments welcome!
So here's my worry. It's not clear, aside from the device tree, how much re-use of existing code we get with this. It feels like it might be fairly minimal. And at that point, are we not perhaps making too much work for ourselves compared with just excepting that there will need to be a place for non-abstracted-framework drivers? What do we do about TPL, when we get to the point of everything being converted to DM and as-needed tiny-DM but there's still TPL drivers? The reason we have SPL_FRAMEWORK as a symbol today is that we already had some SoCs/architectures (primarily PowerPC) that had "SPL" but it was very centric to the SoCs in question.
The interface for example for mmc is: int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and neither part of that is inherently DM. So let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM case. I wonder if we could clean that up code a little if we let it be separate.
The interface for example for spi is: int spl_spi_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and well, the same thing. Or maybe we can even push that up to the spi_flash_load() call.
But my worry is that a different set of abstractions here are still going to bring us in more overhead than writing drivers for the functionality we need directly, and if we define what's allowed in this limited case well, that might be good enough.
Some boards (e.g. x86) Need to read multiple things from the SPI flash (such as FSP binaries), so I still think we will want a generic reading interface.
You could be right, but my hunch is that there is value in having things more generic and the cost should be minimal. The value is that hopefully writing a few C functions in the SPI driver will be enough to enable tiny SPI on an SoC, reusing much of the code in the driver (only the reading bits!). We won't need as much special-case code and an entirely different way of configuring these devices for TPL/SPL.
It has been interesting digging into the Zephyr model. It's drivers are very basic and thus small. But there is still value in using the device tree to assemble things.
Anyway I'm not really sure at this point. It is just a hunch. I don't think we can know all this until we have a bit more information. Perhaps with a board with SPI, MMC and serial converted we would get a better picture?
Regards, Simon
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote:
This patch provides the documentation for a proposed enhancement to driver model to reduce overhead in SPL.
The actual patches are not included here because they are based on some pending work by Walter Lozano which is not in final form.
For now, the source tree is available at:
https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
Comments welcome!
So here's my worry. It's not clear, aside from the device tree, how much re-use of existing code we get with this. It feels like it might be fairly minimal. And at that point, are we not perhaps making too much work for ourselves compared with just excepting that there will need to be a place for non-abstracted-framework drivers? What do we do about TPL, when we get to the point of everything being converted to DM and as-needed tiny-DM but there's still TPL drivers? The reason we have SPL_FRAMEWORK as a symbol today is that we already had some SoCs/architectures (primarily PowerPC) that had "SPL" but it was very centric to the SoCs in question.
The interface for example for mmc is: int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and neither part of that is inherently DM. So let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM case. I wonder if we could clean that up code a little if we let it be separate.
The interface for example for spi is: int spl_spi_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and well, the same thing. Or maybe we can even push that up to the spi_flash_load() call.
But my worry is that a different set of abstractions here are still going to bring us in more overhead than writing drivers for the functionality we need directly, and if we define what's allowed in this limited case well, that might be good enough.
Some boards (e.g. x86) Need to read multiple things from the SPI flash (such as FSP binaries), so I still think we will want a generic reading interface.
You could be right, but my hunch is that there is value in having things more generic and the cost should be minimal. The value is that hopefully writing a few C functions in the SPI driver will be enough to enable tiny SPI on an SoC, reusing much of the code in the driver (only the reading bits!). We won't need as much special-case code and an entirely different way of configuring these devices for TPL/SPL.
It has been interesting digging into the Zephyr model. It's drivers are very basic and thus small. But there is still value in using the device tree to assemble things.
Anyway I'm not really sure at this point. It is just a hunch. I don't think we can know all this until we have a bit more information. Perhaps with a board with SPI, MMC and serial converted we would get a better picture?
I think it's absolutely the case that we'll have to convert something and see how it looks, then convert something else and see if it still looks good enough. At a high enough level there's not really too much of a difference between what it sounds like you're proposing and what I'm proposing. Possibly even in a progmatic way too. We have (I think anyhow) fairly static board configurations in this case so we don't so much need to "probe" for possible drivers be told what our device hierarchy is and to initialize what we're going to use.
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote:
This patch provides the documentation for a proposed enhancement to driver model to reduce overhead in SPL.
The actual patches are not included here because they are based on some pending work by Walter Lozano which is not in final form.
For now, the source tree is available at:
https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
Comments welcome!
So here's my worry. It's not clear, aside from the device tree, how much re-use of existing code we get with this. It feels like it might be fairly minimal. And at that point, are we not perhaps making too much work for ourselves compared with just excepting that there will need to be a place for non-abstracted-framework drivers? What do we do about TPL, when we get to the point of everything being converted to DM and as-needed tiny-DM but there's still TPL drivers? The reason we have SPL_FRAMEWORK as a symbol today is that we already had some SoCs/architectures (primarily PowerPC) that had "SPL" but it was very centric to the SoCs in question.
The interface for example for mmc is: int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and neither part of that is inherently DM. So let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM case. I wonder if we could clean that up code a little if we let it be separate.
The interface for example for spi is: int spl_spi_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and well, the same thing. Or maybe we can even push that up to the spi_flash_load() call.
But my worry is that a different set of abstractions here are still going to bring us in more overhead than writing drivers for the functionality we need directly, and if we define what's allowed in this limited case well, that might be good enough.
Some boards (e.g. x86) Need to read multiple things from the SPI flash (such as FSP binaries), so I still think we will want a generic reading interface.
You could be right, but my hunch is that there is value in having things more generic and the cost should be minimal. The value is that hopefully writing a few C functions in the SPI driver will be enough to enable tiny SPI on an SoC, reusing much of the code in the driver (only the reading bits!). We won't need as much special-case code and an entirely different way of configuring these devices for TPL/SPL.
It has been interesting digging into the Zephyr model. It's drivers are very basic and thus small. But there is still value in using the device tree to assemble things.
Anyway I'm not really sure at this point. It is just a hunch. I don't think we can know all this until we have a bit more information. Perhaps with a board with SPI, MMC and serial converted we would get a better picture?
I think it's absolutely the case that we'll have to convert something and see how it looks, then convert something else and see if it still looks good enough. At a high enough level there's not really too much of a difference between what it sounds like you're proposing and what I'm proposing. Possibly even in a progmatic way too. We have (I think anyhow) fairly static board configurations in this case so we don't so much need to "probe" for possible drivers be told what our device hierarchy is and to initialize what we're going to use.
Yes, we may end up with special, separate code anyway, since if you end up refactoring the driver so much (and putting tiny-dm tentacles into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with the idea of just direct calls into the driver (e.g. the driver implements serial_putc() and spi_read_flash()). But then I figured it is a very small overhead to retain some sort of driver model, so I thought I'd try that.
I'll fiddle with this again in a week or so...
Regards, Simon
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote:
This patch provides the documentation for a proposed enhancement to driver model to reduce overhead in SPL.
The actual patches are not included here because they are based on some pending work by Walter Lozano which is not in final form.
For now, the source tree is available at:
https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-workingComments welcome!
So here's my worry. It's not clear, aside from the device tree, how much re-use of existing code we get with this. It feels like it might be fairly minimal. And at that point, are we not perhaps making too much work for ourselves compared with just excepting that there will need to be a place for non-abstracted-framework drivers? What do we do about TPL, when we get to the point of everything being converted to DM and as-needed tiny-DM but there's still TPL drivers? The reason we have SPL_FRAMEWORK as a symbol today is that we already had some SoCs/architectures (primarily PowerPC) that had "SPL" but it was very centric to the SoCs in question.
The interface for example for mmc is: int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and neither part of that is inherently DM. So let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM case. I wonder if we could clean that up code a little if we let it be separate.
The interface for example for spi is: int spl_spi_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and well, the same thing. Or maybe we can even push that up to the spi_flash_load() call.
But my worry is that a different set of abstractions here are still going to bring us in more overhead than writing drivers for the functionality we need directly, and if we define what's allowed in this limited case well, that might be good enough.
Some boards (e.g. x86) Need to read multiple things from the SPI flash (such as FSP binaries), so I still think we will want a generic reading interface.
You could be right, but my hunch is that there is value in having things more generic and the cost should be minimal. The value is that hopefully writing a few C functions in the SPI driver will be enough to enable tiny SPI on an SoC, reusing much of the code in the driver (only the reading bits!). We won't need as much special-case code and an entirely different way of configuring these devices for TPL/SPL.
It has been interesting digging into the Zephyr model. It's drivers are very basic and thus small. But there is still value in using the device tree to assemble things.
Anyway I'm not really sure at this point. It is just a hunch. I don't think we can know all this until we have a bit more information. Perhaps with a board with SPI, MMC and serial converted we would get a better picture?
I think it's absolutely the case that we'll have to convert something and see how it looks, then convert something else and see if it still looks good enough. At a high enough level there's not really too much of a difference between what it sounds like you're proposing and what I'm proposing. Possibly even in a progmatic way too. We have (I think anyhow) fairly static board configurations in this case so we don't so much need to "probe" for possible drivers be told what our device hierarchy is and to initialize what we're going to use.
Yes, we may end up with special, separate code anyway, since if you end up refactoring the driver so much (and putting tiny-dm tentacles into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with the idea of just direct calls into the driver (e.g. the driver implements serial_putc() and spi_read_flash()). But then I figured it is a very small overhead to retain some sort of driver model, so I thought I'd try that.
I'll fiddle with this again in a week or so...
Thanks for this proposal.
I'm very interested in see where this implementation leads us, as I always felt that some work could be done in order to reduce the overhead of DM support in TPL/SPL. I'll review this work and hopefully come back to you with some comments.
In the same sense, I feel that maybe we can add some additional intelligence to dtoc in order to produce a more customized code for TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is only a feeling.
Regards.
Water
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote:
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote:
This patch provides the documentation for a proposed enhancement to driver model to reduce overhead in SPL.
The actual patches are not included here because they are based on some pending work by Walter Lozano which is not in final form.
For now, the source tree is available at:
https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working
Comments welcome!
So here's my worry. It's not clear, aside from the device tree, how much re-use of existing code we get with this. It feels like it might be fairly minimal. And at that point, are we not perhaps making too much work for ourselves compared with just excepting that there will need to be a place for non-abstracted-framework drivers? What do we do about TPL, when we get to the point of everything being converted to DM and as-needed tiny-DM but there's still TPL drivers? The reason we have SPL_FRAMEWORK as a symbol today is that we already had some SoCs/architectures (primarily PowerPC) that had "SPL" but it was very centric to the SoCs in question.
The interface for example for mmc is: int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and neither part of that is inherently DM. So let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM case. I wonder if we could clean that up code a little if we let it be separate.
The interface for example for spi is: int spl_spi_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and well, the same thing. Or maybe we can even push that up to the spi_flash_load() call.
But my worry is that a different set of abstractions here are still going to bring us in more overhead than writing drivers for the functionality we need directly, and if we define what's allowed in this limited case well, that might be good enough.
Some boards (e.g. x86) Need to read multiple things from the SPI flash (such as FSP binaries), so I still think we will want a generic reading interface.
You could be right, but my hunch is that there is value in having things more generic and the cost should be minimal. The value is that hopefully writing a few C functions in the SPI driver will be enough to enable tiny SPI on an SoC, reusing much of the code in the driver (only the reading bits!). We won't need as much special-case code and an entirely different way of configuring these devices for TPL/SPL.
It has been interesting digging into the Zephyr model. It's drivers are very basic and thus small. But there is still value in using the device tree to assemble things.
Anyway I'm not really sure at this point. It is just a hunch. I don't think we can know all this until we have a bit more information. Perhaps with a board with SPI, MMC and serial converted we would get a better picture?
I think it's absolutely the case that we'll have to convert something and see how it looks, then convert something else and see if it still looks good enough. At a high enough level there's not really too much of a difference between what it sounds like you're proposing and what I'm proposing. Possibly even in a progmatic way too. We have (I think anyhow) fairly static board configurations in this case so we don't so much need to "probe" for possible drivers be told what our device hierarchy is and to initialize what we're going to use.
Yes, we may end up with special, separate code anyway, since if you end up refactoring the driver so much (and putting tiny-dm tentacles into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with the idea of just direct calls into the driver (e.g. the driver implements serial_putc() and spi_read_flash()). But then I figured it is a very small overhead to retain some sort of driver model, so I thought I'd try that.
I'll fiddle with this again in a week or so...
Thanks for this proposal.
I'm very interested in see where this implementation leads us, as I always felt that some work could be done in order to reduce the overhead of DM support in TPL/SPL. I'll review this work and hopefully come back to you with some comments.
In the same sense, I feel that maybe we can add some additional intelligence to dtoc in order to produce a more customized code for TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is only a feeling.
I've been checking your work and found it very interesting. Let me share some comments
I really like the trade-off between size and features in this implementation and the way you get rid of not very useful data, such as strings and class overhead.
I see that you parse drivers in order to build the relationship between drivers and compatible strings among other things. I faced a similar requirement in the series I proposed, however, I proposed the use of a U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. Then main reason behind this were to make code cleaner, avoid a lot of parsing and having to take into account possible valid C code which is not cover by the parsing.
I have no problem with your approach, on the contrary I like the idea of improving dtoc, but I think that if we take this path, we should put some constrains and to document them to avoid unexpected behavior. Most of the constrains maybe already used by all the drivers, like the way we declare compatible strings, however any limitation in the parsing should be documented.
The same goes for parsing config files which is also a nice improvement. I feel that every step we give adding intelligence to dtoc is a step towards footprint improvement.
Another thing to discuss and document are the guidelines to implement the functions similar functions like probe() and tiny_probe(), in such a way to try to avoid code duplication.
Lastly, I have tried to test sandbox_spl as I understand it should work based on you comments, but I receive some errors when running dtoc
DTOC C spl/dts/dt-platdata.c Traceback (most recent call last): File "./tools/dtoc/dtoc", line 116, in <module> options.include_disabled, options.output) File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 833, in run_steps plat.generate_tables() File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 794, in generate_tables self.output_node(node) File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 722, in output_node (val)) ValueError: Cant' find driver for compatible '['sandbox_serial']
Regards,
Walter
On Mon, Jun 01, 2020 at 05:55:31PM -0300, Walter Lozano wrote:
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote:
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote:
> This patch provides the documentation for a proposed > enhancement to driver > model to reduce overhead in SPL. > > The actual patches are not included here because > they are based on some > pending work by Walter Lozano which is not in final form. > > For now, the source tree is available at: > > https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working > > > Comments welcome! So here's my worry. It's not clear, aside from the device tree, how much re-use of existing code we get with this. It feels like it might be fairly minimal. And at that point, are we not perhaps making too much work for ourselves compared with just excepting that there will need to be a place for non-abstracted-framework drivers? What do we do about TPL, when we get to the point of everything being converted to DM and as-needed tiny-DM but there's still TPL drivers? The reason we have SPL_FRAMEWORK as a symbol today is that we already had some SoCs/architectures (primarily PowerPC) that had "SPL" but it was very centric to the SoCs in question.
The interface for example for mmc is: int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and neither part of that is inherently DM. So let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM case. I wonder if we could clean that up code a little if we let it be separate.
The interface for example for spi is: int spl_spi_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and well, the same thing. Or maybe we can even push that up to the spi_flash_load() call.
But my worry is that a different set of abstractions here are still going to bring us in more overhead than writing drivers for the functionality we need directly, and if we define what's allowed in this limited case well, that might be good enough.
Some boards (e.g. x86) Need to read multiple things from the SPI flash (such as FSP binaries), so I still think we will want a generic reading interface.
You could be right, but my hunch is that there is value in having things more generic and the cost should be minimal. The value is that hopefully writing a few C functions in the SPI driver will be enough to enable tiny SPI on an SoC, reusing much of the code in the driver (only the reading bits!). We won't need as much special-case code and an entirely different way of configuring these devices for TPL/SPL.
It has been interesting digging into the Zephyr model. It's drivers are very basic and thus small. But there is still value in using the device tree to assemble things.
Anyway I'm not really sure at this point. It is just a hunch. I don't think we can know all this until we have a bit more information. Perhaps with a board with SPI, MMC and serial converted we would get a better picture?
I think it's absolutely the case that we'll have to convert something and see how it looks, then convert something else and see if it still looks good enough. At a high enough level there's not really too much of a difference between what it sounds like you're proposing and what I'm proposing. Possibly even in a progmatic way too. We have (I think anyhow) fairly static board configurations in this case so we don't so much need to "probe" for possible drivers be told what our device hierarchy is and to initialize what we're going to use.
Yes, we may end up with special, separate code anyway, since if you end up refactoring the driver so much (and putting tiny-dm tentacles into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with the idea of just direct calls into the driver (e.g. the driver implements serial_putc() and spi_read_flash()). But then I figured it is a very small overhead to retain some sort of driver model, so I thought I'd try that.
I'll fiddle with this again in a week or so...
Thanks for this proposal.
I'm very interested in see where this implementation leads us, as I always felt that some work could be done in order to reduce the overhead of DM support in TPL/SPL. I'll review this work and hopefully come back to you with some comments.
In the same sense, I feel that maybe we can add some additional intelligence to dtoc in order to produce a more customized code for TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is only a feeling.
I've been checking your work and found it very interesting. Let me share some comments
I really like the trade-off between size and features in this implementation and the way you get rid of not very useful data, such as strings and class overhead.
I see that you parse drivers in order to build the relationship between drivers and compatible strings among other things. I faced a similar requirement in the series I proposed, however, I proposed the use of a U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. Then main reason behind this were to make code cleaner, avoid a lot of parsing and having to take into account possible valid C code which is not cover by the parsing.
I have no problem with your approach, on the contrary I like the idea of improving dtoc, but I think that if we take this path, we should put some constrains and to document them to avoid unexpected behavior. Most of the constrains maybe already used by all the drivers, like the way we declare compatible strings, however any limitation in the parsing should be documented.
The same goes for parsing config files which is also a nice improvement. I feel that every step we give adding intelligence to dtoc is a step towards footprint improvement.
Would this allow us to make progress towards the idea of being able to discard compatibles (and referenced data) that won't be used at run time given the dts files for the boards we'll support at run time?
Hi Tom,
On Tue, 2 Jun 2020 at 07:53, Tom Rini trini@konsulko.com wrote:
On Mon, Jun 01, 2020 at 05:55:31PM -0300, Walter Lozano wrote:
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote:
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote: > On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote: > > > This patch provides the documentation for a proposed > > enhancement to driver > > model to reduce overhead in SPL. > > > > The actual patches are not included here because > > they are based on some > > pending work by Walter Lozano which is not in final form. > > > > For now, the source tree is available at: > > > > https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working > > > > > > Comments welcome! > So here's my worry. It's not clear, aside from the device tree, how > much re-use of existing code we get with this. It feels > like it might > be fairly minimal. And at that point, are we not perhaps making too > much work for ourselves compared with just excepting that there will > need to be a place for non-abstracted-framework drivers? > What do we do > about TPL, when we get to the point of everything being > converted to DM > and as-needed tiny-DM but there's still TPL drivers? > The reason we have > SPL_FRAMEWORK as a symbol today is that we already had some > SoCs/architectures (primarily PowerPC) that had "SPL" but it was very > centric to the SoCs in question. > > The interface for example for mmc is: > int spl_mmc_load_image(struct spl_image_info *spl_image, struct > spl_boot_device *bootdev) and neither part of that is > inherently DM. So > let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM > case. I wonder if we could clean that up code a little > if we let it be > separate. > > The interface for example for spi is: > int spl_spi_load_image(struct spl_image_info *spl_image, > struct spl_boot_device *bootdev) and well, the same > thing. Or maybe we > can even push that up to the spi_flash_load() call. > > But my worry is that a different set of abstractions here are still > going to bring us in more overhead than writing drivers for the > functionality we need directly, and if we define what's > allowed in this > limited case well, that might be good enough. Some boards (e.g. x86) Need to read multiple things from the SPI flash (such as FSP binaries), so I still think we will want a generic reading interface.
You could be right, but my hunch is that there is value in having things more generic and the cost should be minimal. The value is that hopefully writing a few C functions in the SPI driver will be enough to enable tiny SPI on an SoC, reusing much of the code in the driver (only the reading bits!). We won't need as much special-case code and an entirely different way of configuring these devices for TPL/SPL.
It has been interesting digging into the Zephyr model. It's drivers are very basic and thus small. But there is still value in using the device tree to assemble things.
Anyway I'm not really sure at this point. It is just a hunch. I don't think we can know all this until we have a bit more information. Perhaps with a board with SPI, MMC and serial converted we would get a better picture?
I think it's absolutely the case that we'll have to convert something and see how it looks, then convert something else and see if it still looks good enough. At a high enough level there's not really too much of a difference between what it sounds like you're proposing and what I'm proposing. Possibly even in a progmatic way too. We have (I think anyhow) fairly static board configurations in this case so we don't so much need to "probe" for possible drivers be told what our device hierarchy is and to initialize what we're going to use.
Yes, we may end up with special, separate code anyway, since if you end up refactoring the driver so much (and putting tiny-dm tentacles into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with the idea of just direct calls into the driver (e.g. the driver implements serial_putc() and spi_read_flash()). But then I figured it is a very small overhead to retain some sort of driver model, so I thought I'd try that.
I'll fiddle with this again in a week or so...
Thanks for this proposal.
I'm very interested in see where this implementation leads us, as I always felt that some work could be done in order to reduce the overhead of DM support in TPL/SPL. I'll review this work and hopefully come back to you with some comments.
In the same sense, I feel that maybe we can add some additional intelligence to dtoc in order to produce a more customized code for TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is only a feeling.
I've been checking your work and found it very interesting. Let me share some comments
I really like the trade-off between size and features in this implementation and the way you get rid of not very useful data, such as strings and class overhead.
I see that you parse drivers in order to build the relationship between drivers and compatible strings among other things. I faced a similar requirement in the series I proposed, however, I proposed the use of a U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. Then main reason behind this were to make code cleaner, avoid a lot of parsing and having to take into account possible valid C code which is not cover by the parsing.
I have no problem with your approach, on the contrary I like the idea of improving dtoc, but I think that if we take this path, we should put some constrains and to document them to avoid unexpected behavior. Most of the constrains maybe already used by all the drivers, like the way we declare compatible strings, however any limitation in the parsing should be documented.
The same goes for parsing config files which is also a nice improvement. I feel that every step we give adding intelligence to dtoc is a step towards footprint improvement.
Would this allow us to make progress towards the idea of being able to discard compatibles (and referenced data) that won't be used at run time given the dts files for the boards we'll support at run time?
Do you mean just the compatible strings, or do you mean whole drivers?
Regards, Simon
On Thu, Jun 04, 2020 at 09:13:06PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 2 Jun 2020 at 07:53, Tom Rini trini@konsulko.com wrote:
On Mon, Jun 01, 2020 at 05:55:31PM -0300, Walter Lozano wrote:
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote:
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote: > Hi Tom, > > On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote: > > On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote: > > > > > This patch provides the documentation for a proposed > > > enhancement to driver > > > model to reduce overhead in SPL. > > > > > > The actual patches are not included here because > > > they are based on some > > > pending work by Walter Lozano which is not in final form. > > > > > > For now, the source tree is available at: > > > > > > https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working > > > > > > > > > Comments welcome! > > So here's my worry. It's not clear, aside from the device tree, how > > much re-use of existing code we get with this. It feels > > like it might > > be fairly minimal. And at that point, are we not perhaps making too > > much work for ourselves compared with just excepting that there will > > need to be a place for non-abstracted-framework drivers? > > What do we do > > about TPL, when we get to the point of everything being > > converted to DM > > and as-needed tiny-DM but there's still TPL drivers? > > The reason we have > > SPL_FRAMEWORK as a symbol today is that we already had some > > SoCs/architectures (primarily PowerPC) that had "SPL" but it was very > > centric to the SoCs in question. > > > > The interface for example for mmc is: > > int spl_mmc_load_image(struct spl_image_info *spl_image, struct > > spl_boot_device *bootdev) and neither part of that is > > inherently DM. So > > let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM > > case. I wonder if we could clean that up code a little > > if we let it be > > separate. > > > > The interface for example for spi is: > > int spl_spi_load_image(struct spl_image_info *spl_image, > > struct spl_boot_device *bootdev) and well, the same > > thing. Or maybe we > > can even push that up to the spi_flash_load() call. > > > > But my worry is that a different set of abstractions here are still > > going to bring us in more overhead than writing drivers for the > > functionality we need directly, and if we define what's > > allowed in this > > limited case well, that might be good enough. > Some boards (e.g. x86) Need to read multiple things from the SPI flash > (such as FSP binaries), so I still think we will want a generic > reading interface. > > You could be right, but my hunch is that there is value in having > things more generic and the cost should be minimal. The value is that > hopefully writing a few C functions in the SPI driver will be enough > to enable tiny SPI on an SoC, reusing much of the code in the driver > (only the reading bits!). We won't need as much special-case code and > an entirely different way of configuring these devices for TPL/SPL. > > It has been interesting digging into the Zephyr model. It's drivers > are very basic and thus small. But there is still value in using the > device tree to assemble things. > > Anyway I'm not really sure at this point. It is just a hunch. I don't > think we can know all this until we have a bit more information. > Perhaps with a board with SPI, MMC and serial converted we would get a > better picture? I think it's absolutely the case that we'll have to convert something and see how it looks, then convert something else and see if it still looks good enough. At a high enough level there's not really too much of a difference between what it sounds like you're proposing and what I'm proposing. Possibly even in a progmatic way too. We have (I think anyhow) fairly static board configurations in this case so we don't so much need to "probe" for possible drivers be told what our device hierarchy is and to initialize what we're going to use.
Yes, we may end up with special, separate code anyway, since if you end up refactoring the driver so much (and putting tiny-dm tentacles into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with the idea of just direct calls into the driver (e.g. the driver implements serial_putc() and spi_read_flash()). But then I figured it is a very small overhead to retain some sort of driver model, so I thought I'd try that.
I'll fiddle with this again in a week or so...
Thanks for this proposal.
I'm very interested in see where this implementation leads us, as I always felt that some work could be done in order to reduce the overhead of DM support in TPL/SPL. I'll review this work and hopefully come back to you with some comments.
In the same sense, I feel that maybe we can add some additional intelligence to dtoc in order to produce a more customized code for TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is only a feeling.
I've been checking your work and found it very interesting. Let me share some comments
I really like the trade-off between size and features in this implementation and the way you get rid of not very useful data, such as strings and class overhead.
I see that you parse drivers in order to build the relationship between drivers and compatible strings among other things. I faced a similar requirement in the series I proposed, however, I proposed the use of a U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. Then main reason behind this were to make code cleaner, avoid a lot of parsing and having to take into account possible valid C code which is not cover by the parsing.
I have no problem with your approach, on the contrary I like the idea of improving dtoc, but I think that if we take this path, we should put some constrains and to document them to avoid unexpected behavior. Most of the constrains maybe already used by all the drivers, like the way we declare compatible strings, however any limitation in the parsing should be documented.
The same goes for parsing config files which is also a nice improvement. I feel that every step we give adding intelligence to dtoc is a step towards footprint improvement.
Would this allow us to make progress towards the idea of being able to discard compatibles (and referenced data) that won't be used at run time given the dts files for the boards we'll support at run time?
Do you mean just the compatible strings, or do you mean whole drivers?
Compatible and data. One of the "this is ugly but works" patches to reduce size on i.MX families was to guard compatible string + data (see drivers/mmc/fsl_esdhc_imx.c for example) with compile time checks. If we could automate that somehow, that would be good.
Hi Tom,
On 5/6/20 11:20, Tom Rini wrote:
On Thu, Jun 04, 2020 at 09:13:06PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 2 Jun 2020 at 07:53, Tom Rini trini@konsulko.com wrote:
On Mon, Jun 01, 2020 at 05:55:31PM -0300, Walter Lozano wrote:
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote:
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote: > On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote: >> Hi Tom, >> >> On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote: >>> On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote: >>> >>>> This patch provides the documentation for a proposed >>>> enhancement to driver >>>> model to reduce overhead in SPL. >>>> >>>> The actual patches are not included here because >>>> they are based on some >>>> pending work by Walter Lozano which is not in final form. >>>> >>>> For now, the source tree is available at: >>>> >>>> https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working >>>> >>>> >>>> Comments welcome! >>> So here's my worry. It's not clear, aside from the device tree, how >>> much re-use of existing code we get with this. It feels >>> like it might >>> be fairly minimal. And at that point, are we not perhaps making too >>> much work for ourselves compared with just excepting that there will >>> need to be a place for non-abstracted-framework drivers? >>> What do we do >>> about TPL, when we get to the point of everything being >>> converted to DM >>> and as-needed tiny-DM but there's still TPL drivers? >>> The reason we have >>> SPL_FRAMEWORK as a symbol today is that we already had some >>> SoCs/architectures (primarily PowerPC) that had "SPL" but it was very >>> centric to the SoCs in question. >>> >>> The interface for example for mmc is: >>> int spl_mmc_load_image(struct spl_image_info *spl_image, struct >>> spl_boot_device *bootdev) and neither part of that is >>> inherently DM. So >>> let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM >>> case. I wonder if we could clean that up code a little >>> if we let it be >>> separate. >>> >>> The interface for example for spi is: >>> int spl_spi_load_image(struct spl_image_info *spl_image, >>> struct spl_boot_device *bootdev) and well, the same >>> thing. Or maybe we >>> can even push that up to the spi_flash_load() call. >>> >>> But my worry is that a different set of abstractions here are still >>> going to bring us in more overhead than writing drivers for the >>> functionality we need directly, and if we define what's >>> allowed in this >>> limited case well, that might be good enough. >> Some boards (e.g. x86) Need to read multiple things from the SPI flash >> (such as FSP binaries), so I still think we will want a generic >> reading interface. >> >> You could be right, but my hunch is that there is value in having >> things more generic and the cost should be minimal. The value is that >> hopefully writing a few C functions in the SPI driver will be enough >> to enable tiny SPI on an SoC, reusing much of the code in the driver >> (only the reading bits!). We won't need as much special-case code and >> an entirely different way of configuring these devices for TPL/SPL. >> >> It has been interesting digging into the Zephyr model. It's drivers >> are very basic and thus small. But there is still value in using the >> device tree to assemble things. >> >> Anyway I'm not really sure at this point. It is just a hunch. I don't >> think we can know all this until we have a bit more information. >> Perhaps with a board with SPI, MMC and serial converted we would get a >> better picture? > I think it's absolutely the case that we'll have to convert something > and see how it looks, then convert something else and see if it still > looks good enough. At a high enough level there's not really too much > of a difference between what it sounds like you're proposing and what > I'm proposing. Possibly even in a progmatic way too. We have (I think > anyhow) fairly static board configurations in this case so we don't so > much need to "probe" for possible drivers be told what our device > hierarchy is and to initialize what we're going to use. Yes, we may end up with special, separate code anyway, since if you end up refactoring the driver so much (and putting tiny-dm tentacles into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with the idea of just direct calls into the driver (e.g. the driver implements serial_putc() and spi_read_flash()). But then I figured it is a very small overhead to retain some sort of driver model, so I thought I'd try that.
I'll fiddle with this again in a week or so...
Thanks for this proposal.
I'm very interested in see where this implementation leads us, as I always felt that some work could be done in order to reduce the overhead of DM support in TPL/SPL. I'll review this work and hopefully come back to you with some comments.
In the same sense, I feel that maybe we can add some additional intelligence to dtoc in order to produce a more customized code for TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is only a feeling.
I've been checking your work and found it very interesting. Let me share some comments
I really like the trade-off between size and features in this implementation and the way you get rid of not very useful data, such as strings and class overhead.
I see that you parse drivers in order to build the relationship between drivers and compatible strings among other things. I faced a similar requirement in the series I proposed, however, I proposed the use of a U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. Then main reason behind this were to make code cleaner, avoid a lot of parsing and having to take into account possible valid C code which is not cover by the parsing.
I have no problem with your approach, on the contrary I like the idea of improving dtoc, but I think that if we take this path, we should put some constrains and to document them to avoid unexpected behavior. Most of the constrains maybe already used by all the drivers, like the way we declare compatible strings, however any limitation in the parsing should be documented.
The same goes for parsing config files which is also a nice improvement. I feel that every step we give adding intelligence to dtoc is a step towards footprint improvement.
Would this allow us to make progress towards the idea of being able to discard compatibles (and referenced data) that won't be used at run time given the dts files for the boards we'll support at run time?
Do you mean just the compatible strings, or do you mean whole drivers?
Compatible and data. One of the "this is ugly but works" patches to reduce size on i.MX families was to guard compatible string + data (see drivers/mmc/fsl_esdhc_imx.c for example) with compile time checks. If we could automate that somehow, that would be good.
Could you point to the "ugly but works" patch in order to have better context?
Regarding drivers/mmc/fsl_esdhc_imx.c I have been working in adding OF_PLATDATA support to it in order to reduce the SPL footprint on iMX6, which I think it could be a first step for further improvements.
https://patchwork.ozlabs.org/project/uboot/list/?series=167495&state=*
Regards,
Walter
On Fri, Jun 05, 2020 at 01:18:00PM -0300, Walter Lozano wrote:
Hi Tom,
On 5/6/20 11:20, Tom Rini wrote:
On Thu, Jun 04, 2020 at 09:13:06PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 2 Jun 2020 at 07:53, Tom Rini trini@konsulko.com wrote:
On Mon, Jun 01, 2020 at 05:55:31PM -0300, Walter Lozano wrote:
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote:
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote: > Hi Tom, > > On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote: > > On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote: > > > Hi Tom, > > > > > > On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote: > > > > On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote: > > > > > > > > > This patch provides the documentation for a proposed > > > > > enhancement to driver > > > > > model to reduce overhead in SPL. > > > > > > > > > > The actual patches are not included here because > > > > > they are based on some > > > > > pending work by Walter Lozano which is not in final form. > > > > > > > > > > For now, the source tree is available at: > > > > > > > > > > https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working > > > > > > > > > > > > > > > Comments welcome! > > > > So here's my worry. It's not clear, aside from the device tree, how > > > > much re-use of existing code we get with this. It feels > > > > like it might > > > > be fairly minimal. And at that point, are we not perhaps making too > > > > much work for ourselves compared with just excepting that there will > > > > need to be a place for non-abstracted-framework drivers? > > > > What do we do > > > > about TPL, when we get to the point of everything being > > > > converted to DM > > > > and as-needed tiny-DM but there's still TPL drivers? > > > > The reason we have > > > > SPL_FRAMEWORK as a symbol today is that we already had some > > > > SoCs/architectures (primarily PowerPC) that had "SPL" but it was very > > > > centric to the SoCs in question. > > > > > > > > The interface for example for mmc is: > > > > int spl_mmc_load_image(struct spl_image_info *spl_image, struct > > > > spl_boot_device *bootdev) and neither part of that is > > > > inherently DM. So > > > > let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM > > > > case. I wonder if we could clean that up code a little > > > > if we let it be > > > > separate. > > > > > > > > The interface for example for spi is: > > > > int spl_spi_load_image(struct spl_image_info *spl_image, > > > > struct spl_boot_device *bootdev) and well, the same > > > > thing. Or maybe we > > > > can even push that up to the spi_flash_load() call. > > > > > > > > But my worry is that a different set of abstractions here are still > > > > going to bring us in more overhead than writing drivers for the > > > > functionality we need directly, and if we define what's > > > > allowed in this > > > > limited case well, that might be good enough. > > > Some boards (e.g. x86) Need to read multiple things from the SPI flash > > > (such as FSP binaries), so I still think we will want a generic > > > reading interface. > > > > > > You could be right, but my hunch is that there is value in having > > > things more generic and the cost should be minimal. The value is that > > > hopefully writing a few C functions in the SPI driver will be enough > > > to enable tiny SPI on an SoC, reusing much of the code in the driver > > > (only the reading bits!). We won't need as much special-case code and > > > an entirely different way of configuring these devices for TPL/SPL. > > > > > > It has been interesting digging into the Zephyr model. It's drivers > > > are very basic and thus small. But there is still value in using the > > > device tree to assemble things. > > > > > > Anyway I'm not really sure at this point. It is just a hunch. I don't > > > think we can know all this until we have a bit more information. > > > Perhaps with a board with SPI, MMC and serial converted we would get a > > > better picture? > > I think it's absolutely the case that we'll have to convert something > > and see how it looks, then convert something else and see if it still > > looks good enough. At a high enough level there's not really too much > > of a difference between what it sounds like you're proposing and what > > I'm proposing. Possibly even in a progmatic way too. We have (I think > > anyhow) fairly static board configurations in this case so we don't so > > much need to "probe" for possible drivers be told what our device > > hierarchy is and to initialize what we're going to use. > Yes, we may end up with special, separate code anyway, since if you > end up refactoring the driver so much (and putting tiny-dm tentacles > into it) that it becomes harder to maintain, it isn't a win. > > Basically I started out similar to what you are saying, with the idea > of just direct calls into the driver (e.g. the driver implements > serial_putc() and spi_read_flash()). But then I figured it is a very > small overhead to retain some sort of driver model, so I thought I'd > try that. > > I'll fiddle with this again in a week or so... Thanks for this proposal.
I'm very interested in see where this implementation leads us, as I always felt that some work could be done in order to reduce the overhead of DM support in TPL/SPL. I'll review this work and hopefully come back to you with some comments.
In the same sense, I feel that maybe we can add some additional intelligence to dtoc in order to produce a more customized code for TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is only a feeling.
I've been checking your work and found it very interesting. Let me share some comments
I really like the trade-off between size and features in this implementation and the way you get rid of not very useful data, such as strings and class overhead.
I see that you parse drivers in order to build the relationship between drivers and compatible strings among other things. I faced a similar requirement in the series I proposed, however, I proposed the use of a U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. Then main reason behind this were to make code cleaner, avoid a lot of parsing and having to take into account possible valid C code which is not cover by the parsing.
I have no problem with your approach, on the contrary I like the idea of improving dtoc, but I think that if we take this path, we should put some constrains and to document them to avoid unexpected behavior. Most of the constrains maybe already used by all the drivers, like the way we declare compatible strings, however any limitation in the parsing should be documented.
The same goes for parsing config files which is also a nice improvement. I feel that every step we give adding intelligence to dtoc is a step towards footprint improvement.
Would this allow us to make progress towards the idea of being able to discard compatibles (and referenced data) that won't be used at run time given the dts files for the boards we'll support at run time?
Do you mean just the compatible strings, or do you mean whole drivers?
Compatible and data. One of the "this is ugly but works" patches to reduce size on i.MX families was to guard compatible string + data (see drivers/mmc/fsl_esdhc_imx.c for example) with compile time checks. If we could automate that somehow, that would be good.
Could you point to the "ugly but works" patch in order to have better context?
http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@...
Regarding drivers/mmc/fsl_esdhc_imx.c I have been working in adding OF_PLATDATA support to it in order to reduce the SPL footprint on iMX6, which I think it could be a first step for further improvements.
https://patchwork.ozlabs.org/project/uboot/list/?series=167495&state=*
Note that we're not talking about just SPL here. We have platforms that very specifically care about full U-Boot size. In addition, we care in general about the overall binary size. Think of it this way, especially in the case the i.MX patch above shows. Why should a 32bit i.MX6 platform grow in binary size as part of adding support for the latest 64bit i.MX8 part, when we're talking about just adding some form or another of a table.
Hi Tom,
On 5/6/20 13:32, Tom Rini wrote:
On Fri, Jun 05, 2020 at 01:18:00PM -0300, Walter Lozano wrote:
Hi Tom,
On 5/6/20 11:20, Tom Rini wrote:
On Thu, Jun 04, 2020 at 09:13:06PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 2 Jun 2020 at 07:53, Tom Rini trini@konsulko.com wrote:
On Mon, Jun 01, 2020 at 05:55:31PM -0300, Walter Lozano wrote:
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote: > Hi Simon, > > On 25/5/20 18:40, Simon Glass wrote: >> Hi Tom, >> >> On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote: >>> On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote: >>>> Hi Tom, >>>> >>>> On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote: >>>>> On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote: >>>>> >>>>>> This patch provides the documentation for a proposed >>>>>> enhancement to driver >>>>>> model to reduce overhead in SPL. >>>>>> >>>>>> The actual patches are not included here because >>>>>> they are based on some >>>>>> pending work by Walter Lozano which is not in final form. >>>>>> >>>>>> For now, the source tree is available at: >>>>>> >>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working >>>>>> >>>>>> >>>>>> Comments welcome! >>>>> So here's my worry. It's not clear, aside from the device tree, how >>>>> much re-use of existing code we get with this. It feels >>>>> like it might >>>>> be fairly minimal. And at that point, are we not perhaps making too >>>>> much work for ourselves compared with just excepting that there will >>>>> need to be a place for non-abstracted-framework drivers? >>>>> What do we do >>>>> about TPL, when we get to the point of everything being >>>>> converted to DM >>>>> and as-needed tiny-DM but there's still TPL drivers? >>>>> The reason we have >>>>> SPL_FRAMEWORK as a symbol today is that we already had some >>>>> SoCs/architectures (primarily PowerPC) that had "SPL" but it was very >>>>> centric to the SoCs in question. >>>>> >>>>> The interface for example for mmc is: >>>>> int spl_mmc_load_image(struct spl_image_info *spl_image, struct >>>>> spl_boot_device *bootdev) and neither part of that is >>>>> inherently DM. So >>>>> let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM >>>>> case. I wonder if we could clean that up code a little >>>>> if we let it be >>>>> separate. >>>>> >>>>> The interface for example for spi is: >>>>> int spl_spi_load_image(struct spl_image_info *spl_image, >>>>> struct spl_boot_device *bootdev) and well, the same >>>>> thing. Or maybe we >>>>> can even push that up to the spi_flash_load() call. >>>>> >>>>> But my worry is that a different set of abstractions here are still >>>>> going to bring us in more overhead than writing drivers for the >>>>> functionality we need directly, and if we define what's >>>>> allowed in this >>>>> limited case well, that might be good enough. >>>> Some boards (e.g. x86) Need to read multiple things from the SPI flash >>>> (such as FSP binaries), so I still think we will want a generic >>>> reading interface. >>>> >>>> You could be right, but my hunch is that there is value in having >>>> things more generic and the cost should be minimal. The value is that >>>> hopefully writing a few C functions in the SPI driver will be enough >>>> to enable tiny SPI on an SoC, reusing much of the code in the driver >>>> (only the reading bits!). We won't need as much special-case code and >>>> an entirely different way of configuring these devices for TPL/SPL. >>>> >>>> It has been interesting digging into the Zephyr model. It's drivers >>>> are very basic and thus small. But there is still value in using the >>>> device tree to assemble things. >>>> >>>> Anyway I'm not really sure at this point. It is just a hunch. I don't >>>> think we can know all this until we have a bit more information. >>>> Perhaps with a board with SPI, MMC and serial converted we would get a >>>> better picture? >>> I think it's absolutely the case that we'll have to convert something >>> and see how it looks, then convert something else and see if it still >>> looks good enough. At a high enough level there's not really too much >>> of a difference between what it sounds like you're proposing and what >>> I'm proposing. Possibly even in a progmatic way too. We have (I think >>> anyhow) fairly static board configurations in this case so we don't so >>> much need to "probe" for possible drivers be told what our device >>> hierarchy is and to initialize what we're going to use. >> Yes, we may end up with special, separate code anyway, since if you >> end up refactoring the driver so much (and putting tiny-dm tentacles >> into it) that it becomes harder to maintain, it isn't a win. >> >> Basically I started out similar to what you are saying, with the idea >> of just direct calls into the driver (e.g. the driver implements >> serial_putc() and spi_read_flash()). But then I figured it is a very >> small overhead to retain some sort of driver model, so I thought I'd >> try that. >> >> I'll fiddle with this again in a week or so... > Thanks for this proposal. > > I'm very interested in see where this implementation leads us, as I > always felt that some work could be done in order to reduce the overhead > of DM support in TPL/SPL. I'll review this work and hopefully come back > to you with some comments. > > In the same sense, I feel that maybe we can add some additional > intelligence to dtoc in order to produce a more customized code for > TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is > only a feeling. > > I've been checking your work and found it very interesting. Let me share some comments
I really like the trade-off between size and features in this implementation and the way you get rid of not very useful data, such as strings and class overhead.
I see that you parse drivers in order to build the relationship between drivers and compatible strings among other things. I faced a similar requirement in the series I proposed, however, I proposed the use of a U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. Then main reason behind this were to make code cleaner, avoid a lot of parsing and having to take into account possible valid C code which is not cover by the parsing.
I have no problem with your approach, on the contrary I like the idea of improving dtoc, but I think that if we take this path, we should put some constrains and to document them to avoid unexpected behavior. Most of the constrains maybe already used by all the drivers, like the way we declare compatible strings, however any limitation in the parsing should be documented.
The same goes for parsing config files which is also a nice improvement. I feel that every step we give adding intelligence to dtoc is a step towards footprint improvement.
Would this allow us to make progress towards the idea of being able to discard compatibles (and referenced data) that won't be used at run time given the dts files for the boards we'll support at run time?
Do you mean just the compatible strings, or do you mean whole drivers?
Compatible and data. One of the "this is ugly but works" patches to reduce size on i.MX families was to guard compatible string + data (see drivers/mmc/fsl_esdhc_imx.c for example) with compile time checks. If we could automate that somehow, that would be good.
Could you point to the "ugly but works" patch in order to have better context?
http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@...
Thanks for pointing at it, now the context is clear.
Regarding drivers/mmc/fsl_esdhc_imx.c I have been working in adding OF_PLATDATA support to it in order to reduce the SPL footprint on iMX6, which I think it could be a first step for further improvements.
https://patchwork.ozlabs.org/project/uboot/list/?series=167495&state=*
Note that we're not talking about just SPL here. We have platforms that very specifically care about full U-Boot size. In addition, we care in general about the overall binary size. Think of it this way, especially in the case the i.MX patch above shows. Why should a 32bit i.MX6 platform grow in binary size as part of adding support for the latest 64bit i.MX8 part, when we're talking about just adding some form or another of a table.
I see your point and the example is very clear. Let me think about this, as I said I think there are a lot of room for improvements if we add a bit more of intelligence to dtoc.
Regards,
Walter
On 5/6/20 16:29, Walter Lozano wrote:
Hi Tom,
On 5/6/20 13:32, Tom Rini wrote:
On Fri, Jun 05, 2020 at 01:18:00PM -0300, Walter Lozano wrote:
Hi Tom,
On 5/6/20 11:20, Tom Rini wrote:
On Thu, Jun 04, 2020 at 09:13:06PM -0600, Simon Glass wrote:
Hi Tom,
On Tue, 2 Jun 2020 at 07:53, Tom Rini trini@konsulko.com wrote:
On Mon, Jun 01, 2020 at 05:55:31PM -0300, Walter Lozano wrote: > Hi Simon, > > On 26/5/20 15:39, Walter Lozano wrote: >> Hi Simon, >> >> On 25/5/20 18:40, Simon Glass wrote: >>> Hi Tom, >>> >>> On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com >>> wrote: >>>> On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote: >>>>> Hi Tom, >>>>> >>>>> On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com >>>>> wrote: >>>>>> On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote: >>>>>> >>>>>>> This patch provides the documentation for a proposed >>>>>>> enhancement to driver >>>>>>> model to reduce overhead in SPL. >>>>>>> >>>>>>> The actual patches are not included here because >>>>>>> they are based on some >>>>>>> pending work by Walter Lozano which is not in final form. >>>>>>> >>>>>>> For now, the source tree is available at: >>>>>>> >>>>>>> https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working >>>>>>> >>>>>>> >>>>>>> >>>>>>> Comments welcome! >>>>>> So here's my worry. It's not clear, aside from the device >>>>>> tree, how >>>>>> much re-use of existing code we get with this. It feels >>>>>> like it might >>>>>> be fairly minimal. And at that point, are we not perhaps >>>>>> making too >>>>>> much work for ourselves compared with just excepting that >>>>>> there will >>>>>> need to be a place for non-abstracted-framework drivers? >>>>>> What do we do >>>>>> about TPL, when we get to the point of everything being >>>>>> converted to DM >>>>>> and as-needed tiny-DM but there's still TPL drivers? >>>>>> The reason we have >>>>>> SPL_FRAMEWORK as a symbol today is that we already had some >>>>>> SoCs/architectures (primarily PowerPC) that had "SPL" but >>>>>> it was very >>>>>> centric to the SoCs in question. >>>>>> >>>>>> The interface for example for mmc is: >>>>>> int spl_mmc_load_image(struct spl_image_info *spl_image, >>>>>> struct >>>>>> spl_boot_device *bootdev) and neither part of that is >>>>>> inherently DM. So >>>>>> let it be MMC_TINY for the non-DM case and regular DM_MMC >>>>>> for the DM >>>>>> case. I wonder if we could clean that up code a little >>>>>> if we let it be >>>>>> separate. >>>>>> >>>>>> The interface for example for spi is: >>>>>> int spl_spi_load_image(struct spl_image_info *spl_image, >>>>>> struct spl_boot_device *bootdev) and well, the same >>>>>> thing. Or maybe we >>>>>> can even push that up to the spi_flash_load() call. >>>>>> >>>>>> But my worry is that a different set of abstractions here >>>>>> are still >>>>>> going to bring us in more overhead than writing drivers for >>>>>> the >>>>>> functionality we need directly, and if we define what's >>>>>> allowed in this >>>>>> limited case well, that might be good enough. >>>>> Some boards (e.g. x86) Need to read multiple things from the >>>>> SPI flash >>>>> (such as FSP binaries), so I still think we will want a generic >>>>> reading interface. >>>>> >>>>> You could be right, but my hunch is that there is value in >>>>> having >>>>> things more generic and the cost should be minimal. The >>>>> value is that >>>>> hopefully writing a few C functions in the SPI driver will >>>>> be enough >>>>> to enable tiny SPI on an SoC, reusing much of the code in >>>>> the driver >>>>> (only the reading bits!). We won't need as much special-case >>>>> code and >>>>> an entirely different way of configuring these devices for >>>>> TPL/SPL. >>>>> >>>>> It has been interesting digging into the Zephyr model. It's >>>>> drivers >>>>> are very basic and thus small. But there is still value in >>>>> using the >>>>> device tree to assemble things. >>>>> >>>>> Anyway I'm not really sure at this point. It is just a >>>>> hunch. I don't >>>>> think we can know all this until we have a bit more >>>>> information. >>>>> Perhaps with a board with SPI, MMC and serial converted we >>>>> would get a >>>>> better picture? >>>> I think it's absolutely the case that we'll have to convert >>>> something >>>> and see how it looks, then convert something else and see if >>>> it still >>>> looks good enough. At a high enough level there's not really >>>> too much >>>> of a difference between what it sounds like you're proposing >>>> and what >>>> I'm proposing. Possibly even in a progmatic way too. We >>>> have (I think >>>> anyhow) fairly static board configurations in this case so we >>>> don't so >>>> much need to "probe" for possible drivers be told what our >>>> device >>>> hierarchy is and to initialize what we're going to use. >>> Yes, we may end up with special, separate code anyway, since >>> if you >>> end up refactoring the driver so much (and putting tiny-dm >>> tentacles >>> into it) that it becomes harder to maintain, it isn't a win. >>> >>> Basically I started out similar to what you are saying, with >>> the idea >>> of just direct calls into the driver (e.g. the driver implements >>> serial_putc() and spi_read_flash()). But then I figured it is >>> a very >>> small overhead to retain some sort of driver model, so I >>> thought I'd >>> try that. >>> >>> I'll fiddle with this again in a week or so... >> Thanks for this proposal. >> >> I'm very interested in see where this implementation leads us, >> as I >> always felt that some work could be done in order to reduce the >> overhead >> of DM support in TPL/SPL. I'll review this work and hopefully >> come back >> to you with some comments. >> >> In the same sense, I feel that maybe we can add some additional >> intelligence to dtoc in order to produce a more customized code >> for >> TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, >> but this is >> only a feeling. >> >> > I've been checking your work and found it very interesting. Let > me share > some comments > > > I really like the trade-off between size and features in this > implementation > and the way you get rid of not very useful data, such as strings > and class > overhead. > > > I see that you parse drivers in order to build the relationship > between > drivers and compatible strings among other things. I faced a > similar > requirement in the series I proposed, however, I proposed the > use of a > U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. > Then main > reason behind this were to make code cleaner, avoid a lot of > parsing and > having to take into account possible valid C code which is not > cover by the > parsing. > > I have no problem with your approach, on the contrary I like the > idea of > improving dtoc, but I think that if we take this path, we should > put some > constrains and to document them to avoid unexpected behavior. > Most of the > constrains maybe already used by all the drivers, like the way > we declare > compatible strings, however any limitation in the parsing should be > documented. > > > The same goes for parsing config files which is also a nice > improvement. I > feel that every step we give adding intelligence to dtoc is a > step towards > footprint improvement. Would this allow us to make progress towards the idea of being able to discard compatibles (and referenced data) that won't be used at run time given the dts files for the boards we'll support at run time?
Do you mean just the compatible strings, or do you mean whole drivers?
Compatible and data. One of the "this is ugly but works" patches to reduce size on i.MX families was to guard compatible string + data (see drivers/mmc/fsl_esdhc_imx.c for example) with compile time checks. If we could automate that somehow, that would be good.
Could you point to the "ugly but works" patch in order to have better context?
http://patchwork.ozlabs.org/project/uboot/patch/20200525202429.2146-1-agust@...
Thanks for pointing at it, now the context is clear.
Regarding drivers/mmc/fsl_esdhc_imx.c I have been working in adding OF_PLATDATA support to it in order to reduce the SPL footprint on iMX6, which I think it could be a first step for further improvements.
https://patchwork.ozlabs.org/project/uboot/list/?series=167495&state=*
Note that we're not talking about just SPL here. We have platforms that very specifically care about full U-Boot size. In addition, we care in general about the overall binary size. Think of it this way, especially in the case the i.MX patch above shows. Why should a 32bit i.MX6 platform grow in binary size as part of adding support for the latest 64bit i.MX8 part, when we're talking about just adding some form or another of a table.
I see your point and the example is very clear. Let me think about this, as I said I think there are a lot of room for improvements if we add a bit more of intelligence to dtoc.
Following the discussion related to [PATCH v1 00/15] add basic driver support for broadcom NS3 soc
https://patchwork.ozlabs.org/project/uboot/list/?series=177442
As I mentioned in the other thread, there might be some possible improvements like
1- Use OF_PLATDATA, which already works and is being improved, to convert DTB to C structures, however this require driver support.
2- Improve dtoc to generate configuration variables based on DTB, to allow to enable some pieces of code only if DTB contains specific properties, this could be useful to avoid adding configuration options than have to match with DTB properties in order to work.
3- Improve dtoc to generate C code based on DTB information, for instance the list of compatible strings. The specific example of compatible strings makes sense only if we don't support OF_PLATDATA.
4- Improve dtoc to filter nodes from DTB, however I still see some need of manual work in order to specify which nodes to keep, similar to the u-boot.dtsi approach. Maybe with some additional intelligence, only nodes that have a driver enabled you be kept? However, this could checked carefully as it could lead to issues at runtime difficult to debug.
Regards,
Walter
Hi Walter,
On Mon, 1 Jun 2020 at 14:55, Walter Lozano walter.lozano@collabora.com wrote:
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote:
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote:
> This patch provides the documentation for a proposed enhancement > to driver > model to reduce overhead in SPL. > > The actual patches are not included here because they are based > on some > pending work by Walter Lozano which is not in final form. > > For now, the source tree is available at: > > https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working > > > Comments welcome! So here's my worry. It's not clear, aside from the device tree, how much re-use of existing code we get with this. It feels like it might be fairly minimal. And at that point, are we not perhaps making too much work for ourselves compared with just excepting that there will need to be a place for non-abstracted-framework drivers? What do we do about TPL, when we get to the point of everything being converted to DM and as-needed tiny-DM but there's still TPL drivers? The reason we have SPL_FRAMEWORK as a symbol today is that we already had some SoCs/architectures (primarily PowerPC) that had "SPL" but it was very centric to the SoCs in question.
The interface for example for mmc is: int spl_mmc_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and neither part of that is inherently DM. So let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM case. I wonder if we could clean that up code a little if we let it be separate.
The interface for example for spi is: int spl_spi_load_image(struct spl_image_info *spl_image, struct spl_boot_device *bootdev) and well, the same thing. Or maybe we can even push that up to the spi_flash_load() call.
But my worry is that a different set of abstractions here are still going to bring us in more overhead than writing drivers for the functionality we need directly, and if we define what's allowed in this limited case well, that might be good enough.
Some boards (e.g. x86) Need to read multiple things from the SPI flash (such as FSP binaries), so I still think we will want a generic reading interface.
You could be right, but my hunch is that there is value in having things more generic and the cost should be minimal. The value is that hopefully writing a few C functions in the SPI driver will be enough to enable tiny SPI on an SoC, reusing much of the code in the driver (only the reading bits!). We won't need as much special-case code and an entirely different way of configuring these devices for TPL/SPL.
It has been interesting digging into the Zephyr model. It's drivers are very basic and thus small. But there is still value in using the device tree to assemble things.
Anyway I'm not really sure at this point. It is just a hunch. I don't think we can know all this until we have a bit more information. Perhaps with a board with SPI, MMC and serial converted we would get a better picture?
I think it's absolutely the case that we'll have to convert something and see how it looks, then convert something else and see if it still looks good enough. At a high enough level there's not really too much of a difference between what it sounds like you're proposing and what I'm proposing. Possibly even in a progmatic way too. We have (I think anyhow) fairly static board configurations in this case so we don't so much need to "probe" for possible drivers be told what our device hierarchy is and to initialize what we're going to use.
Yes, we may end up with special, separate code anyway, since if you end up refactoring the driver so much (and putting tiny-dm tentacles into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with the idea of just direct calls into the driver (e.g. the driver implements serial_putc() and spi_read_flash()). But then I figured it is a very small overhead to retain some sort of driver model, so I thought I'd try that.
I'll fiddle with this again in a week or so...
Thanks for this proposal.
I'm very interested in see where this implementation leads us, as I always felt that some work could be done in order to reduce the overhead of DM support in TPL/SPL. I'll review this work and hopefully come back to you with some comments.
In the same sense, I feel that maybe we can add some additional intelligence to dtoc in order to produce a more customized code for TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is only a feeling.
I've been checking your work and found it very interesting. Let me share some comments
Thank you for your comments.
I really like the trade-off between size and features in this implementation and the way you get rid of not very useful data, such as strings and class overhead.
I see that you parse drivers in order to build the relationship between drivers and compatible strings among other things. I faced a similar requirement in the series I proposed, however, I proposed the use of a U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. Then main reason behind this were to make code cleaner, avoid a lot of parsing and having to take into account possible valid C code which is not cover by the parsing.
I have no problem with your approach, on the contrary I like the idea of improving dtoc, but I think that if we take this path, we should put some constrains and to document them to avoid unexpected behavior. Most of the constrains maybe already used by all the drivers, like the way we declare compatible strings, however any limitation in the parsing should be documented.
Yes there is a trade-off here and I'm not sure which is best. I am quite suspicious of 'magic' things because they are hard for people to understand.
The same goes for parsing config files which is also a nice improvement. I feel that every step we give adding intelligence to dtoc is a step towards footprint improvement.
Yes, and again it has to be worth it. So far I am just picking up the CONFIG_SPL_TINY_SERIAL (etc.) options to find out which subsystems are tiny.
Another thing to discuss and document are the guidelines to implement the functions similar functions like probe() and tiny_probe(), in such a way to try to avoid code duplication.
Yes. That should be somewhat possible, by passing arguments to core functions, but a little painful, similar to of-platdata.
Lastly, I have tried to test sandbox_spl as I understand it should work based on you comments, but I receive some errors when running dtoc
DTOC C spl/dts/dt-platdata.c Traceback (most recent call last): File "./tools/dtoc/dtoc", line 116, in <module> options.include_disabled, options.output) File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 833, in run_steps plat.generate_tables() File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 794, in generate_tables self.output_node(node) File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 722, in output_node (val)) ValueError: Cant' find driver for compatible '['sandbox_serial']
Er yes, I moved over to a real board halfway through, and sandbox stopped working. Will fix it at some point.
Regards, Simon
Hi,
On Thu, 4 Jun 2020 at 21:11, Simon Glass sjg@chromium.org wrote:
Hi Walter,
On Mon, 1 Jun 2020 at 14:55, Walter Lozano walter.lozano@collabora.com wrote:
Hi Simon,
On 26/5/20 15:39, Walter Lozano wrote:
Hi Simon,
On 25/5/20 18:40, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 14:57, Tom Rini trini@konsulko.com wrote:
On Mon, May 25, 2020 at 02:34:20PM -0600, Simon Glass wrote:
Hi Tom,
On Mon, 25 May 2020 at 13:47, Tom Rini trini@konsulko.com wrote: > On Mon, May 25, 2020 at 09:35:44AM -0600, Simon Glass wrote: > >> This patch provides the documentation for a proposed enhancement >> to driver >> model to reduce overhead in SPL. >> >> The actual patches are not included here because they are based >> on some >> pending work by Walter Lozano which is not in final form. >> >> For now, the source tree is available at: >> >> https://gitlab.denx.de/u-boot/custodians/u-boot-dm/-/tree/dtoc-working >> >> >> Comments welcome! > So here's my worry. It's not clear, aside from the device tree, how > much re-use of existing code we get with this. It feels like it > might > be fairly minimal. And at that point, are we not perhaps making too > much work for ourselves compared with just excepting that there will > need to be a place for non-abstracted-framework drivers? What do > we do > about TPL, when we get to the point of everything being converted > to DM > and as-needed tiny-DM but there's still TPL drivers? The reason > we have > SPL_FRAMEWORK as a symbol today is that we already had some > SoCs/architectures (primarily PowerPC) that had "SPL" but it was very > centric to the SoCs in question. > > The interface for example for mmc is: > int spl_mmc_load_image(struct spl_image_info *spl_image, struct > spl_boot_device *bootdev) and neither part of that is inherently > DM. So > let it be MMC_TINY for the non-DM case and regular DM_MMC for the DM > case. I wonder if we could clean that up code a little if we let > it be > separate. > > The interface for example for spi is: > int spl_spi_load_image(struct spl_image_info *spl_image, > struct spl_boot_device *bootdev) and well, the same thing. Or > maybe we > can even push that up to the spi_flash_load() call. > > But my worry is that a different set of abstractions here are still > going to bring us in more overhead than writing drivers for the > functionality we need directly, and if we define what's allowed in > this > limited case well, that might be good enough. Some boards (e.g. x86) Need to read multiple things from the SPI flash (such as FSP binaries), so I still think we will want a generic reading interface.
You could be right, but my hunch is that there is value in having things more generic and the cost should be minimal. The value is that hopefully writing a few C functions in the SPI driver will be enough to enable tiny SPI on an SoC, reusing much of the code in the driver (only the reading bits!). We won't need as much special-case code and an entirely different way of configuring these devices for TPL/SPL.
It has been interesting digging into the Zephyr model. It's drivers are very basic and thus small. But there is still value in using the device tree to assemble things.
Anyway I'm not really sure at this point. It is just a hunch. I don't think we can know all this until we have a bit more information. Perhaps with a board with SPI, MMC and serial converted we would get a better picture?
I think it's absolutely the case that we'll have to convert something and see how it looks, then convert something else and see if it still looks good enough. At a high enough level there's not really too much of a difference between what it sounds like you're proposing and what I'm proposing. Possibly even in a progmatic way too. We have (I think anyhow) fairly static board configurations in this case so we don't so much need to "probe" for possible drivers be told what our device hierarchy is and to initialize what we're going to use.
Yes, we may end up with special, separate code anyway, since if you end up refactoring the driver so much (and putting tiny-dm tentacles into it) that it becomes harder to maintain, it isn't a win.
Basically I started out similar to what you are saying, with the idea of just direct calls into the driver (e.g. the driver implements serial_putc() and spi_read_flash()). But then I figured it is a very small overhead to retain some sort of driver model, so I thought I'd try that.
I'll fiddle with this again in a week or so...
Thanks for this proposal.
I'm very interested in see where this implementation leads us, as I always felt that some work could be done in order to reduce the overhead of DM support in TPL/SPL. I'll review this work and hopefully come back to you with some comments.
In the same sense, I feel that maybe we can add some additional intelligence to dtoc in order to produce a more customized code for TPL/SPL, maybe relaying in some custom stuff in u-boot.dtsi, but this is only a feeling.
I've been checking your work and found it very interesting. Let me share some comments
Thank you for your comments.
I really like the trade-off between size and features in this implementation and the way you get rid of not very useful data, such as strings and class overhead.
I see that you parse drivers in order to build the relationship between drivers and compatible strings among other things. I faced a similar requirement in the series I proposed, however, I proposed the use of a U_BOOT_DRIVER_ALIAS in order to explicitly declare the alias. Then main reason behind this were to make code cleaner, avoid a lot of parsing and having to take into account possible valid C code which is not cover by the parsing.
I have no problem with your approach, on the contrary I like the idea of improving dtoc, but I think that if we take this path, we should put some constrains and to document them to avoid unexpected behavior. Most of the constrains maybe already used by all the drivers, like the way we declare compatible strings, however any limitation in the parsing should be documented.
Yes there is a trade-off here and I'm not sure which is best. I am quite suspicious of 'magic' things because they are hard for people to understand.
The same goes for parsing config files which is also a nice improvement. I feel that every step we give adding intelligence to dtoc is a step towards footprint improvement.
Yes, and again it has to be worth it. So far I am just picking up the CONFIG_SPL_TINY_SERIAL (etc.) options to find out which subsystems are tiny.
Another thing to discuss and document are the guidelines to implement the functions similar functions like probe() and tiny_probe(), in such a way to try to avoid code duplication.
Yes. That should be somewhat possible, by passing arguments to core functions, but a little painful, similar to of-platdata.
Lastly, I have tried to test sandbox_spl as I understand it should work based on you comments, but I receive some errors when running dtoc
DTOC C spl/dts/dt-platdata.c Traceback (most recent call last): File "./tools/dtoc/dtoc", line 116, in <module> options.include_disabled, options.output) File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 833, in run_steps plat.generate_tables() File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 794, in generate_tables self.output_node(node) File "/home/wlozano/u-boot/tiny-dm/tools/dtoc/../dtoc/dtb_platdata.py", line 722, in output_node (val)) ValueError: Cant' find driver for compatible '['sandbox_serial']
Er yes, I moved over to a real board halfway through, and sandbox stopped working. Will fix it at some point.
One more thought that has been rattling around is that we could try a more moderate idea. Tiny-DM is sort-of the nuclear approach - minimum possible size at the cost of a somewhat parallel setup, at least in terms of code.
Another idea would be to reduce the size of the DM data structures by having two versions of them - a tiny one and a full one. We might be able handle it automatically. E.g drop the platdata sizes (or compress them), drop the pre-probe options and other things used only on i2c and more complicated things. Use singly-linked lists. Use 16-bit offsets for memory allocation (and perhaps even other things) instead of pointers. It would trade off speed for space.
The benefit would be that it would look more like DM on the surface, with less driver infrastructure to modify. Walter's series shows what can be done by automatic processing with very little user effort.
The cost, of course, is more complexity, mostly under the hood. It would inevitably produce more code as well, although perhaps not much.
Regards, Simon
participants (3)
-
Simon Glass -
Tom Rini -
Walter Lozano