Devicetree: Child node of node on SPI bus itself needs reg property - Bug?

[arludwig]

Disclaimer
I am not sure this is actually a bug - it might just be that I have misunderstood some details about the devicetree. But it might also be a bug.

Describe the bug
I am trying to split the wiring and the protocol information of an spi slave device into two nodes. This would allow me to have a physical connector, like a zif-connector, on my board, hardwired to a specific chip select gpio; further more I would be able to specify the actual target device, like an epaper display, in an overlay file, specifying device dependent information, like the max frequency, clock configuration or word size. This way, I could support devices with different requirements by simply changing the overlay.

To achieve this, I define an spi-target binding for a node on an spi bus, that takes the actual device as child node. Following example is simplified:

description: SPI Target

compatible: spi-target

include: base.yaml

on-bus: spi

properties:
  reg:
    required: true

child-binding:
  description: SPI Device

  properties:
    spi-max-frequency:
      type: int
      required: true
      description: Maximum clock frequency of device's SPI interface in Hz
    
    label:
      type: string
      required: true
      description: Human readable string describing the device (used as device_get_binding() argument)

(Obviously I will have to write a custom driver for this, but I that is ok and not an issue here.)

I tested this with the following overlay file for a nrf52840dk_nrf52840 board:

&spi3 {
    cs-gpios = <&arduino_header 16 GPIO_ACTIVE_LOW>; // D10

    spi_target@0 {
        compatible = "spi-target";
        reg = <0>;

        my_spi_dev_0 {
            label = "MY_DEVICE";
            spi-max-frequency = <10000000>;
        };
    };
};

When I try to apply this, I get the following error:

[cmake] Traceback (most recent call last):
[cmake]   File "C:\zephyrproject\zephyr\scripts\dts\gen_legacy_defines.py", line 827, in <module>
[cmake]     main()
[cmake]   File "C:\zephyrproject\zephyr\scripts\dts\gen_legacy_defines.py", line 55, in main
[cmake]     write_spi_dev(node)
[cmake]   File "C:\zephyrproject\zephyr\scripts\dts\gen_legacy_defines.py", line 534, in write_spi_dev
[cmake]     cs_gpio = node.spi_cs_gpio
[cmake]   File "C:\zephyrproject\zephyr\scripts\dts\edtlib.py", line 936, in spi_cs_gpio
[cmake]     _err("{!r} needs a 'reg' property, to look up the chip select index "
[cmake]   File "C:\zephyrproject\zephyr\scripts\dts\edtlib.py", line 2258, in _err
[cmake]     raise EDTError(msg)
[cmake] edtlib.EDTError: <Node /soc/spi@4002f000/spi_con@0/my_spi_dev_0 in 'nrf52840dk_nrf52840.dts.pre.tmp', binding .../dts/bindings\spi-target.yaml> needs a 'reg' property, to look up the chip select index for SPI

Seems zephyr requires the child node itself to have a reg property, which kind of defeats my intentions here.

To Reproduce

1. Define binding as above
2. Set overlay as above
3. CMake-configure the project
4. Read CMake output / error message

Expected behavior
When I have a device on the spi bus, then this device should be able to have itself children. So I would have expected my aproach to work. This would be a nice way to modularize devicetree information using overlays for mine and related cases.

Impact
This is more of an annoyance as a show stopper, because I can work around using non-related nodes and referring from the device to the spi-target via a phandle property. But from a devicetree structural point of view it would be a lot nicer (and I think also safer / less error prone) and clearer if a hierarchical structure could be used.

Environment (please complete the following information):

• Windows 10
• CMake 3.17.2
• Zephyr 2.4 (c2a0b0f)
• GNU ARM Embedded Toolchain (9 2020-q2-update)

[arludwig]

OK, so I found that it works if I put it like this:

&spi3 {
    cs-gpios = <&arduino_header 16 GPIO_ACTIVE_LOW>; // D10

    spi_target@0 {
        compatible = "spi-target";
        reg = <0>;
        #address-cells = <1>;
        #size-cells = <0>;

        my_spi_dev@0 {
            reg = <0>;
            label = "MY_DEVICE";
            spi-max-frequency = <10000000>;
        };
    };
};

Just have to assign the child node a reg value on the spi-target. I set the #address-cells and #size-cells properties for convenient reg values (defaults are 2 and 1). (And of course I added reg to the child-binding properties of the binding.)

Thus it just became only a minor inconvenience, because the work-around is not that much of a game changer.

But I would still like to know whether this is a bug in the zephyr device tree scripts, or in my understanding of the device tree 😕

[mbolivar-nordic]

I can't rule out a bug from reviewing the details description. Will take a more detailed look.

[mbolivar-nordic]

@arludwig I can reproduce the issue on v2.4.0 as given in your report, but it doesn't reproduce on master.

Bisection shows that 8165008, which removed legacy macro support, also avoids the error. Since this commit doesn't cherry-pick cleanly onto v2.4.0, here is a patch which just turns off legacy define generation in the build system, that is on top of v2.4.0:

$ cat 0001-dts.cmake-disable-legacy-macro-generation.patch 
From 508267d8fd9ffa76d5309201baee6c81f1fcdad2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Mart=C3=AD=20Bol=C3=ADvar?= <[email protected]>
Date: Thu, 10 Dec 2020 10:39:36 -0800
Subject: [PATCH] dts.cmake: disable legacy macro generation
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Hotfix for issue
https://github.com/zephyrproject-rtos/zephyr/issues/30423.

Signed-off-by: Martí Bolívar <[email protected]>
---
 cmake/dts.cmake | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cmake/dts.cmake b/cmake/dts.cmake
index 1bed174a93..76e2e4b88a 100644
--- a/cmake/dts.cmake
+++ b/cmake/dts.cmake
@@ -249,6 +249,7 @@ if(SUPPORTS_DTS)
     message(STATUS "Generated devicetree_unfixed.h: ${DEVICETREE_UNFIXED_H}")
   endif()
 
+if (FALSE) # disable legacy macros support
   execute_process(
     COMMAND ${CMD_LEGACY_EXTRACT}
     WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
@@ -257,6 +258,7 @@ if(SUPPORTS_DTS)
   if(NOT "${ret}" STREQUAL "0")
     message(FATAL_ERROR "gen_legacy_defines.py failed with return code: ${ret}")
   endif()
+endif()
 
   # A file that used to be generated by 'dtc'. zephyr.dts is the new
   # equivalent. Will be removed in Zephyr 2.3.
-- 
2.29.2

Also available here:

https://github.com/mbolivar-nordic/zephyr/tree/issue-30423-no-legacy-macros-hotfix

As long as you are using the new devicetree API and not the legacy macros in your application, this should suffice to avoid the error.

If you're using the legacy macros, I think you will need to move to the new API first, before you can use this patch. Sorry for any inconvenience.

Please reopen if I've misunderstood anything or this won't work for you, and thank you for taking the time to file a detailed report.

[arludwig]

Yes, this fixes the issue. Thank you!