zephyrproject-rtos / zephyr

Primary Git Repository for the Zephyr Project. Zephyr is a new generation, scalable, optimized, secure RTOS for multiple hardware architectures.
https://docs.zephyrproject.org
Apache License 2.0
10.7k stars 6.54k forks source link

Adding support for multi-functional-devices #48934

Open bjarki-andreasen opened 2 years ago

bjarki-andreasen commented 2 years ago

Introduction

Currently, only one API can be mapped to each instance of a struct device. This proposal provides two solutions which together allow for multiple APIs pr device instance, and the creation of devices which consist of multiple sub devices.

Problem description

Some devices provide multiple functions, which require multiple APIs pr single device instance. Some devices consist of multiple internal devices, which all require their own handles and APIs. There is no general design guideline for creating such devices, and creating devices with multiple APIs pr instance is not currently possible. This is preventing the creation of proper drivers for cellular and GNSS modems which do provide many varying features which can not be efficiently covered by a single API, since the functionalities are ever expanding and vary highly between devices.

Proposed change

The first solution is simply a design guideline for devices which consist of multiple sub devices. The change includes adding a documentation page describing this guideline, and adding some macros to help keep the driver design consistent. The specific docs and macros are contained in this PR https://github.com/zephyrproject-rtos/zephyr/pull/48932

   bus {
       parent {
          child1 {
          }; 
          child2 {
          };
          child3 {
          };
       };
   };

The second solution creates one struct device for each API implemented by a single device instance. The struct devices share all data except for the API pointer. Their names reflect the API they implement, and they are referenced using a function similar to DEVICE_DT_GET(node_id), called DEVICE_DT_API_GET(node_id, api_type), which returns the struct device which implements the specified API type if it has been defined by a driver, using DEVICE_DT_API_DEFINE(api_type, node_id, api_ptr)

struct api_foo;
struct api_bar;

DEVICE_DT_DEFINE(node_id, ... NULL)
DEVICE_DT_API_DEFINE(foo, node_id, &api_foo)
DEVICE_DT_API_DEFINE(bar, node_id, &api_bar)
struct device *foo_dev = DEVICE_DT_API_GET(foo, node_id);
struct device *bar_dev = DEVICE_DT_API_GET(bar, node_id);

The API struct devices are stored in ROM, grouped by API type, allowing for iterating through all devices which support a specific API type, which is handy for the shell for example.

Solution 2 requires no changes to the existing API headers, adds no overhead to the usage of API wrappers, while allowing for multiple APIs pr device instance

int ret = foo_var_get(&foo_dev);
ret = bar_dev_set(&bar_dev);

Detailed RFC

The first solution requires little extra explaining, since it is literally a docs page :)

The second solution requires updating the linker files to add sections for the API struct devices, and an update to the device.h file to include the new macros for defining and getting struct devices. Adding convenience macros to the driver API headers like LOCATION_DEVICE_DT_API_DEFINE(node_id, api_ptr) should also be done, but is not required.

struct api_loc;

DEVICE_DT_DEFINE(node_id, ... NULL)
LOCATION_DEVICE_DT_API_DEFINE(node_id, &api_loc)

Proposed change (Detailed)

See https://github.com/zephyrproject-rtos/zephyr/pull/48817 for example implementation of an API using solution 2

  1. Add documentation and macros for solution 1
  2. Add linker file which will contain API iterable sections
  3. Update device.h to include new macros for solution 2
  4. Add convenience macros to driver API headers
  5. Start transition period from old solution to these solutions
  6. Update in-tree drivers to use DEVICE_DT_API_DEFINE for APIs, and set DEVICE_DT_DEFINE api_ptr to NULL
  7. Transition period ends
  8. Remove api_ptr from DEVICE_DT_DEFINE
  9. Use api ptr in struct device from DEVICE_DT_DEFINE as init function
  10. update linker and device.c to use struct devices with init instead of an additional list as is does now
  11. Success

Dependencies

The only dependency which we can not control is the out-of-tree drivers which must be updated in the transition period.

Concerns and Unresolved Questions

Alternatives

Come forth with your alternatives here :)

galak commented 2 years ago

Want to make sure we reconcile various cases that exist in tree today:

gmarull commented 2 years ago

Adding one use case to the discussion.

STM32 RCC / GD32 RCU (Reset and clock control unit) is a single block of hardware that controls both clocks and reset lines for multiple peripherals. From a Devicetree perspective, it is described as a single node:

https://github.com/zephyrproject-rtos/zephyr/blob/9a7e4b162d4650520532808f9c94fca257bb631b/dts/arm/st/h7/stm32h7.dtsi#L114-L118

or in Linux:

        rcc: reset-clock-controller@58024400 {
            compatible = "st,stm32h743-rcc", "st,stm32-rcc";
            reg = <0x58024400 0x400>;
            #clock-cells = <1>;
            #reset-cells = <1>;
            clocks = <&clk_hse>, <&clk_lse>, <&clk_i2s>;
            st,syscfg = <&pwrcfg>;
        };

ref. https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/stm32h743.dtsi#L537

It is used like this:

https://github.com/zephyrproject-rtos/zephyr/blob/fd1423620e392c7dcd78d54649060f312c01022d/dts/arm/st/h7/stm32h7.dtsi#L369-L378

or in Linux:

        spi2: spi@40003800 {
            #address-cells = <1>;
            #size-cells = <0>;
            compatible = "st,stm32h7-spi";
            reg = <0x40003800 0x400>;
            interrupts = <36>;
            resets = <&rcc STM32H7_APB1L_RESET(SPI2)>;
            clocks = <&rcc SPI2_CK>;
            status = "disabled";
        };

ref. https://elixir.bootlin.com/linux/latest/source/arch/arm/boot/dts/stm32h743.dtsi#L107

Note that Zephyr doesn't have a reset driver for STM32 yet, so we can't find any resets = ... in Zephyr. And... here comes the problem. In the current device model, we have 1 device per DT node. While it makes sense in most cases, this is an arbitrary decision forced by the Zephyr device model. That means, DEVICE_DT_GET(DT_NODELABEL(rcc)) can only resolve to a single device instance (either clock or reset, but not both). If one expects to maintain DT compatibility with Linux, Zephyr device model doesn't work for this case.

One could argue that RCC is a multi-function device (mfd), like e.g. syscon, meaning it could have 2 subnodes one for clock control and the other for reset control. Something like:

rcc: reset-clock-controller@58024400 {
      compatible = "st,stm32-rcc";
      reg = <0x58024400 0x400>;

      rcc_clk: clock-controller {
            compatible = "st,stm32-rcc-clock";
            #clock-cells = <1>;
            clocks = <&clk_hse>, <&clk_lse>, <&clk_i2s>;
      };

      rcc_reset: reset-controller {
            compatible = "st,stm32-rcc-reset";
            #reset-cells = <1>;
      };
};

spi2: spi@40003800 {
      #address-cells = <1>;
      #size-cells = <0>;
      compatible = "st,stm32h7-spi";
      reg = <0x40003800 0x400>;
      interrupts = <36>;
      resets = <&rcc_reset STM32H7_APB1L_RESET(SPI2)>;
      clocks = <&rcc_clock SPI2_CK>;
      status = "disabled";
};

This would work with Zephyr device model because we have again 1 device per DT node. But it breaks DT compatibility with Linux (note that for STM32 it's not compatible nowadays, e.g. for pinctrl).

gmarull commented 2 years ago

Some more notes I think can be useful to the discussion.

In Linux one can find MFD (multi-function devices):

Multi-Function Devices (MFD)

These devices comprise a nexus for heterogeneous hardware blocks containing
more than one non-unique yet varying hardware functionality.

A typical MFD can be:

- A mixed signal ASIC on an external bus, sometimes a PMIC (Power Management
  Integrated Circuit) that is manufactured in a lower technology node (rough
  silicon) that handles analog drivers for things like audio amplifiers, LED
  drivers, level shifters, PHY (physical interfaces to things like USB or
  ethernet), regulators etc.

- A range of memory registers containing "miscellaneous system registers" also
  known as a system controller "syscon" or any other memory range containing a
  mix of unrelated hardware devices.

Optional properties:

- compatible : "simple-mfd" - this signifies that the operating system should
  consider all subnodes of the MFD device as separate devices akin to how
  "simple-bus" indicates when to see subnodes as children for a simple
  memory-mapped bus. For more complex devices, when the nexus driver has to
  probe registers to figure out what child devices exist etc, this should not
  be used. In the latter case the child devices will be determined by the
  operating system.

- ranges: Describes the address mapping relationship to the parent. Should set
  the child's base address to 0, the physical address within parent's address
  space, and the length of the address map.

- #address-cells: Specifies the number of cells used to represent physical base
  addresses. Must be present if ranges is used.

- #size-cells: Specifies the number of cells used to represent the size of an
  address. Must be present if ranges is used.

Example:

foo@1000 {
    compatible = "syscon", "simple-mfd";
    reg = <0x01000 0x1000>;

    led@8.0 {
        compatible = "register-bit-led";
        offset = <0x08>;
        mask = <0x01>;
        label = "myled";
        default-state = "on";
    };
};

ref. https://elixir.bootlin.com/linux/latest/source/Documentation/devicetree/bindings/mfd/mfd.txt

An example:

    #include <dt-bindings/clock/bcm-nsp.h>
    cru-bus@1800c100 {
        compatible = "brcm,ns-cru", "simple-mfd";
        reg = <0x1800c100 0x1d0>;
        ranges;
        #address-cells = <1>;
        #size-cells = <1>;

        clock-controller@100 {
            #clock-cells = <1>;
            compatible = "brcm,nsp-lcpll0";
            reg = <0x100 0x14>;
            clocks = <&osc>;
            clock-output-names = "lcpll0", "pcie_phy", "sdio", "ddr_phy";
        };

        clock-controller@140 {
            #clock-cells = <1>;
            compatible = "brcm,nsp-genpll";
            reg = <0x140 0x24>;
            clocks = <&osc>;
            clock-output-names = "genpll", "phy", "ethernetclk", "usbclk",
                                 "iprocfast", "sata1", "sata2";
        };

        phy@164 {
            compatible = "brcm,ns-usb2-phy";
            reg = <0x164 0x4>;
            brcm,syscon-clkset = <&clkset>;
            clocks = <&genpll BCM_NSP_GENPLL_USB_PHY_REF_CLK>;
            clock-names = "phy-ref-clk";
            #phy-cells = <0>;
        };

        clkset: syscon@180 {
            compatible = "brcm,cru-clkset", "syscon";
            reg = <0x180 0x4>;
        };

        pinctrl@1c0 {
            compatible = "brcm,bcm4708-pinmux";
            reg = <0x1c0 0x24>;
            reg-names = "cru_gpio_control";
        };

        thermal@2c0 {
            compatible = "brcm,ns-thermal";
            reg = <0x2c0 0x10>;
            #thermal-sensor-cells = <0>;
        };
    };

So, it looks like the modem case described a few days ago would fall into this category. In practice, this means that we could simply have multiple regular devices, each one implementing a particular API, so no need to have devices with multiple APIs.

Another point is power management. Some MFD devices may allow turning off its units independently (again, may because I have no idea, if assumption is wrong forget about this). If that's true, a single device with multiple APIs would be problematic because the PM API operates on a device basis.

bjarki-andreasen commented 2 years ago

@gmarull The point of having both solutions is exactly because some multi functional devices are better described as independent nodes, and some devices are single nodes, but require a multitude of APIs to fully be utilized.

The solution 1 fits perfectly with Linux MFDs. The second solution is for devices like cellular modems, creating a single API for such a device is impractical, since some support GPS, SMS, Networking, Cellular properties like imei etc. The second solution allows for creating tailored composite APIs for a device, like a C# class inheriting a bunch of interfaces.

gmarull commented 2 years ago

@gmarull The point of having both solutions is exactly because some multi functional devices are better described as independent nodes, and some devices are single nodes, but require a multitude of APIs to fully be utilized.

So if they're independent nodes, why can't we have a device per node?

The solution 1 fits perfectly with Linux MFDs. The second solution is for devices like cellular modems, creating a single API for such a device is impractical, since some support GPS, SMS, Networking, Cellular properties like imei etc. The second solution allows for creating tailored composite APIs for a device, like a C# class inheriting a bunch of interfaces.

Btw, it would be useful if you could share the datasheet of such modem.

bjarki-andreasen commented 2 years ago

@gmarull The point of having both solutions is exactly because some multi functional devices are better described as independent nodes, and some devices are single nodes, but require a multitude of APIs to fully be utilized.

So if they're independent nodes, why can't we have a device per node? We can?

The solution 1 fits perfectly with Linux MFDs. The second solution is for devices like cellular modems, creating a single API for such a device is impractical, since some support GPS, SMS, Networking, Cellular properties like imei etc. The second solution allows for creating tailored composite APIs for a device, like a C# class inheriting a bunch of interfaces.

Btw, it would be useful if you could share the datasheet of such modem. You can look to the driver for the simcom7080 modem, specifically the amount of APIs that have to be in a custom header since the device is limited to one zephyr/drivers/modem/simcom7080

bjarki-andreasen commented 2 years ago

The working implementation of the (possible) future of the device model :) https://github.com/zephyrproject-rtos/zephyr/pull/49374 Please review it at your leisure :) Automated tests are should be done before next dev meeting

gmarull commented 2 years ago

So I've been thinking about this use case, and I have the following suggestion that requires no modifications to the current device model.

First for DT:

&uart0 {
    vnd-modem@.... {
        compatible = "vnd,modem";
        modem-prop = <0xff>;
        ...
        gps {
            compatible = "vnd,modem-gps";
            gps-prop = <0xee>;
            ...
        };

        gpio-controller {
            compatible = "vnd,modem-gpio";
            gpio-controller;
            #gpio-cells = <2>;
            ...
        };
    };
};

Then, you can create one device per each MFD entry and still share parent driver's data/config:

/* modem data/config (will be used by all child devices) */
struct vnd_modem_data {
    /* potentially arbitration (mutex/sem) for bus access */
    ...
};

struct vnd_modem_config {
    const struct device *uart;
    int modem_prop;
};

/* gps data/config */
struct vnd_modem_gps_data {
    ...
};

struct vnd_modem_gps_config {
    /* reference to parent modem device */
    const struct device *modem;
    int gps_prop;
};

/* gpio data/config */
struct vnd_modem_gpio_data {
    ...
};

struct vnd_modem_gpio_config {
    /* reference to parent modem device */
    const struct device *modem;
};

/* modem init function */
static int vnd_modem_init(const struct device *dev)
{
    const struct vnd_modem_config *config = dev->config;

    if (!device_is_ready(config->uart)) {
        return -ENODEV;
    }

    /* any common init code/sequence here */
}

/* modem GPS init function */
static int vnd_modem_gps_init(const struct device *dev)
{
    const struct vnd_modem_gps_config *config = dev->config;

    if (!device_is_ready(config->modem)) {
        return -ENODEV;
    }

    /* any GPS init code/sequence here */
    /* we can access `config->modem` data/config! */
}

/* modem GPIO init function */
static int vnd_modem_gpio_init(const struct device *dev)
{
    const struct vnd_modem_gpio_config *config = dev->config;

    if (!device_is_ready(config->modem)) {
        return -ENODEV;
    }

    /* any GPIO init code/sequence here */
    /* we can access `config->modem` data/config! */
}

/* modem init function */
static int vnd_modem_init(const struct device *dev)
{
    const struct vnd_modem_config *config = dev->config;

    if (!device_is_ready(config->uart)) {
        return -ENODEV;
    }

    /* any common init code/sequence here */
}

#define VND_MODEM_DEFINE(n)                                                    \
    struct vnd_modem_data vnd_modem_data##n;                               \
                                                                               \
    const struct vnd_modem_config modem_config##n = {                      \
        .uart = DT_INST_BUS(n),                                        \
    };                                                                     \
                                                                               \
    DEVICE_DT_INST_DEFINE(n, vnd_modem_init, NULL, &vnd_modem_data##n,     \
                  &vnd_modem_config##n, POST_KERNEL, N, NULL);

#define VND_MODEM_GPS_DEFINE(n)                                                \
    struct vnd_modem_gps_data vnd_modem_gps_data##n;                       \
                                                                               \
    const struct vnd_modem_gps_config modem_gps_config##n = {              \
        .modem = DEVICE_DT_GET(DT_INST_BUS(n)),                        \
        .gps_prop = DT_INST_PROP(n, gps_prop),                         \
    };                                                                     \
                                                                               \
    DEVICE_DT_INST_DEFINE(                                                 \
        n, vnd_modem_gps_init, NULL, &vnd_modem_gps_data##n,           \
        &vnd_modem_gps_config##n, POST_KERNEL, N + 1, &gps_api);

#define VND_MODEM_GPIO_DEFINE(n)                                               \
    struct vnd_modem_gpio_data vnd_modem_gpio_data##n;                     \
                                                                               \
    const struct vnd_modem_gpio_config modem_gpio_config##n = {            \
        .modem = DEVICE_DT_GET(DT_INST_BUS(n)),                        \
    };                                                                     \
                                                                               \
    DEVICE_DT_INST_DEFINE(                                                 \
        n, vnd_modem_gpio_init, NULL, &vnd_modem_gpio_data##n,         \
        &vnd_modem_gpio_config##n, POST_KERNEL, N + 1, &gpio_api);

/* instantiate modem (parent) device */
#define DT_DRV_COMPAT vnd_modem
DT_INST_FOREACH_STATUS_OKAY(VND_MODEM_DEFINE)
#undef DT_DRV_COMPAT

/* instantiate modem gps (child) device */
#define DT_DRV_COMPAT vnd_modem_gps
DT_INST_FOREACH_STATUS_OKAY(VND_MODEM_GPS_DEFINE)
#undef DT_DRV_COMPAT

/* instantiate modem gpio (child) device */
#define DT_DRV_COMPAT vnd_modem_gpio
DT_INST_FOREACH_STATUS_OKAY(VND_MODEM_GPIO_DEFINE)
#undef DT_DRV_COMPAT
bjarki-andreasen commented 2 years ago

@gmarull

This is the nearly identical to the solution proposed in this PR https://github.com/zephyrproject-rtos/zephyr/pull/48932, which does indeed not require any changes.

The multi API model however is solving the usecase for multiple APIs pr instance, this solution solves MFDs, by creating three distinct instances, which share context. The Multi API device model is trying to create C++ style objects, which can derive multiple APIs, like I2C and I3C and PM.

The multi API device model also creates three destinct instances in ROM which share the same context, but they all use the same devicetree node identifier, and since they are declared in properties files, the build system knows to generate macros and externs for them.

gmarull commented 2 years ago

@gmarull

This is the nearly identical to the solution proposed in this PR #48932, which does indeed not require any changes.

The multi API model however is solving the usecase for multiple APIs pr instance, this solution solves MFDs, by creating three distinct instances, which share context.

The example above also solves the same problem. This is important to note, since than means Zephyr can solve your problem, even if not the way your prefer. It also allows us to weight in solutions that involve breaking the device model, including adding runtime overhead. Is it worth doing all these changes for such particular usecase? To me clearly not, unless I’m given a more convincing example.

The Multi API device model is trying to create C++ style objects, which can derive multiple APIs, like I2C and I3C and PM.

Afaik C++ does not work exactly the same way multi-API proposal is in its current state.

The multi API device model also creates three destinct instances in ROM which share the same context

Again, in the code above parent context is also shared.

bjarki-andreasen commented 2 years ago

@gmarull This is the nearly identical to the solution proposed in this PR #48932, which does indeed not require any changes. The multi API model however is solving the usecase for multiple APIs pr instance, this solution solves MFDs, by creating three distinct instances, which share context.

The example above also solves the same problem. This is important to note, since than means Zephyr can solve your problem, even if not the way your prefer. It also allows us to weight in solutions that involve breaking the device model, including adding runtime overhead. Is it worth doing all these changes for such particular usecase? To me clearly not, unless I’m given a more convincing example.

The Multi API device model is trying to create C++ style objects, which can derive multiple APIs, like I2C and I3C and PM.

Afaik C++ does not work exactly the same way multi-API proposal is in its current state.

The multi API device model also creates three destinct instances in ROM which share the same context

Again, in the code above parent context is also shared.

You misunderstood my comment. I am not arguing that any particular implementation of this specific solution is better, I am highlighting that the MFD solutions are essentially the same, and how they differ from the multi API solution. This is a good thing, as it implies agreement.

The PR https://github.com/zephyrproject-rtos/zephyr/pull/48932 contains an addition to the docs describing the solution you suggested, please review it and help formulating the solution.

I am well aware how C++ solves the issue, maybe I should have said "any object oriented programming language"

CraigBurden commented 1 year ago

I feel like solution 2 covers the range of needs a bit better, it more simply allows APIs to share context because of the shared struct data. This is really important for many of the multi-function devices that are not necessarily able to do multiple things at once, despite it making sense to split the APIs based on the different functionality.

For example the SX1276 is a multi-modulation radio, which is currently supported under the LoRa APIs, however this chip is capable of many more. No additional modulations are currently supported but if they were, they cannot be supported at the same time, the device would need to switch configuration. Having them supported within the same device instance but with separate APIs would allow the driver to intelligently switch the device configuration when the different APIs are used. Of course that specific functionality could be omitted, but perhaps it would be valuable to include API hooks for setup and tear down functions for each API, to formalise support for such devices? I can think of many devices that would require this functionality

This is different to the GPS/GSM modem example you have given where both can happen at the same time, or at least the modem deals with the coexistence of the two internally.

arbrauns commented 1 year ago

Related but not yet linked here: there is a discussion with a good overview of the problem and its solutions so far in #50641.