Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Requirements for driver devices generation using device tree #10904

Closed
erwango opened this issue Oct 29, 2018 · 51 comments
Closed

Requirements for driver devices generation using device tree #10904

erwango opened this issue Oct 29, 2018 · 51 comments
Assignees
Labels
Enhancement Changes/Updates/Additions to existing features

Comments

@erwango
Copy link
Member

erwango commented Oct 29, 2018

Aim of this issue is to settle on requirements for code generation solution that should be used to generate devices code for a driver configured by device tree. Most of the elements that will be listed here were discussed during OpenIoT 2018 in Edinburgh last week.

  • User's acceptance/Ease of use:
    • Code generation solution should be easy to read and use. User should identify main elements and understand behavior with minimum effort.
    • User should be able to easily map elements from the EDTS file to the code generation template
    • Ideally, syntax should be easy to edit in IDE
  • Versatility:
    • Code generation solution should fit to most of zephyr drivers, and at least in a first iteration work with all drivers using DEVICE_AND_API_INIT macro.
  • Ease of review:
    • Solution should not bring significant overhead to the review process. It should be constrained and sufficiently defined so we don't loose time to comment on various possible ways on using the solution for a particular driver.
  • Maintainability:
    • Code generation provided should be easy to maintain.
  • Doc:
    • Solution should be fully documented
  • Implementation basics:
    • Input:
      • EDTS Python library API (cf script/dts: Generate Extended Device Tree database #9876, and zephyr/topic-EDTS branch) exclusively. No use of the edts.json directly in order to avoid reliance on the JSON formatting
      • Clean, non-templated i2c_foo.c file with the driver implementation (not used by the templating engine, here for completeness)
      • Single template file i2c_foo_devices.h.in with the templated device structures.
    • Output:
      • Generated file : i2c_foo_devices.h in build/zephyr/drivers/<driver>/
      • Included in i2c_foo.c( #include i2c_foo_devices.h)
    • Invocation: Solution should be invoked from one liner, simple cmake macro:
      zephyr_library_sources_codegen_ifdef(CONFIG_I2C_STM32 i2c_ll_stm32.c)
    • Rendering
      • Time: Needs to be rendered at ninja time
      • Performance: Needs to render files one by one in parallel
  • Standalone:
    • The tooling to take a device tree & yaml, plus some template description should not be zephyr specific or zephyr aware (ie any invocation of the tools should be able to exist and run outside of Zephyr)
    • The integration (cmake) and templates helpers can be zephyr aware.

Note:
While being targeted in the long term, pinmux generation using device tree input is not addressed here. It is likely this is one element of the discussion, but aim of the ticket is to sum up what we know/want on driver devices.

@erwango
Copy link
Member Author

erwango commented Oct 29, 2018

@b0661 , @evenl

@erwango erwango self-assigned this Oct 29, 2018
@erwango
Copy link
Member Author

erwango commented Oct 30, 2018

@galak , @carlescufi, @mbolivar, @MaureenHelm, @nashif, feel free editing these lines.
Then, I think it is important that we're able to freeze a version of it.

@galak
Copy link
Collaborator

galak commented Oct 30, 2018

@b0661 mentioned use for out of tree drivers, so wondering what requirements that drives

@galak
Copy link
Collaborator

galak commented Oct 30, 2018

One template file per driver .c file

(if I have, i2c_foo.c, than just i2c_foo.h_template)

@galak
Copy link
Collaborator

galak commented Oct 30, 2018

Should be standalone from Zephyr (usable by other projects).

@galak
Copy link
Collaborator

galak commented Oct 30, 2018

Performance of rendering (Are we invoking python once for all files that need to be rendered or per file)?

@SebastianBoe
Copy link
Collaborator

Performance of rendering (Are we invoking python once for all files that need to be rendered or per file)?

Once per file in parallel. Note that this has already been implemented.

@carlescufi
Copy link
Member

Performance of rendering (Are we invoking python once for all files that need to be rendered or per file)?

Once per file in parallel. Note that this has already been implemented.

Thanks, updated the description now

@erwango
Copy link
Member Author

erwango commented Oct 30, 2018

Should be standalone from Zephyr (usable by other projects).

This is complex.
This means that the solution should provide modules, or templating approach. Codegen provides this with each module providing an API for a particular template. I don't know if jinja2 could provide the same approach.
It also mean that invocation could be decoupled from Cmake.

@erwango
Copy link
Member Author

erwango commented Oct 30, 2018

Added information about driver <> compatible binding:
* Driver <> EDTS binding should be done using dts 'compatible' property.

@b0661
Copy link
Collaborator

b0661 commented Oct 30, 2018

EDTS should be decoupled from extract_dts_includes.py. It should have it´s own extraction function that directly extracts from the compiled DTS file (see #10885).

Benefit:

  • Smooth transistion from defines to code generation without impacting each other
  • Standardized EDTS extraction becomes part of the database and enables the database to be used standalone. This is also a prerequisite to make - sometime - codegen standalone.
  • Without coupling to define generation new binding extraction can be introduced without risk to the current code base and also with potential other solutions.

@b0661
Copy link
Collaborator

b0661 commented Oct 30, 2018

Driver <> EDTS binding should be done using dts 'compatible' property.

Must be detailed. 'compatible' is the same for all instances of a device.

  • How shall the instances of a device be bound? - by 'label', by 'address', by ???

@b0661
Copy link
Collaborator

b0661 commented Oct 30, 2018

Code generation provided should be easy to maintain.

Does that mean:

  • low maintenance burden because maintained/ improved outside of Zephyr or
  • low maintenance burden because maintained/ improved inside of Zephyr with full control and no extra effort to upstream?

@b0661
Copy link
Collaborator

b0661 commented Oct 30, 2018

Clean, non-templated i2c_foo.c file with the driver implementation (not used by the templating engine, here for completeness)
Single template file i2c_foo_devices.h.in with the templated device structures.

These are rules how to structure a driver. Code generation should not be restricted to this model. Code genration should be versatile and generate any file type (not only *.h.in -> *.h).

@b0661
Copy link
Collaborator

b0661 commented Oct 30, 2018

Template faults shall be easy to debug with a clear indication of the fault location in the template.

@b0661
Copy link
Collaborator

b0661 commented Oct 31, 2018

Note:
While being targeted in the long term, pinmux generation using device tree input is not addressed here. It is likely this is one element of the discussion, but aim of the ticket is to sum up what we know/want on driver devices.

Excluding a major set of known use cases from the requirments analysis is the wrong method. By this you do not identify the shortcomings of the tools. In a tool evaluation you still may decide to use the tool just because you are only targeting the limited scope - but then you know about the implications.

The use cases I ran in so far:

  • Write a driver for a pin controller device that initializes pins at startup with values taken from the DTS.
  • Write a driver with shared interrupts
  • Write a driver that uses DMA and where DMA channels can not be exclusively attached to a driver instance
  • Write an average driver (whatever average is)

The requirements will only cover the last use case. Shortcomings or possible solutions for the first 3 will not be identified.

@tbursztyka
Copy link
Collaborator

tbursztyka commented Oct 31, 2018

Should be standalone from Zephyr (usable by other projects).

I don't get this requirement. Have there been actual real interest from other projects to reuse what's being done by @b0661 and @erwango ? (This is something deeply tied to zephyr, making it "generic" will just add-up a big amount of work).

@carlescufi
Copy link
Member

The use cases I ran in so far:

  • Write a driver for a pin controller device that initializes pins at startup with values taken from the DTS.
  • Write a driver with shared interrupts
  • Write a driver that uses DMA and where DMA channels can not be exclusively attached to a driver instance
  • Write an average driver (whatever average is)

@b0661 so are you saying that these usecases are not covered by the requirements because we will need code generation inside the .c files, that having a generated .h won't be enough?

@carlescufi
Copy link
Member

These are rules how to structure a driver. Code generation should not be restricted to this model. Code genration should be versatile and generate any file type (not only *.h.in -> *.h).

We are trying to limit the scope here, solving one problem at a time to try and make progress. Code generation itself is limitless, but we do want to try and work on a subset of issues that we need to fix now. All of the code generation options proposed are flexible enough in the end to generate any file types.

@carlescufi
Copy link
Member

@galak I've removed the "should be standalone" requirement because it's not clear what exactly you mean by that, as pointed out by several people on this thread.

@galak
Copy link
Collaborator

galak commented Oct 31, 2018

I don't get this requirement. Have there been actual real interest from other projects to reuse what's being done by @b0661 and @erwango ? (This is something deeply tied to zephyr, making it "generic" will just add-up a big amount of work).

Yes, there's real interesting other projects like the TFM (https://www.trustedfirmware.org/). The CMake integration is obviously going to be Zephyr specific, but going from a set of YAML files & dts, plus whatever template generation solution should be able to be used by any project.

@evenl
Copy link
Collaborator

evenl commented Nov 1, 2018

So what I think we are a missing is a different issue where we can discuss high level requirements for code generation in general. Because some of the statements in this issue is very generic while others are very concrete/specific to the driver instantiation use-case. To avoid that the high level requirements diverges between different code generation use-case discussion, all of these discussions should refer to this high level issue.

@b0661
Copy link
Collaborator

b0661 commented Nov 1, 2018

The use cases I ran in so far:

  • Write a driver for a pin controller device that initializes pins at startup with values taken from the DTS.
  • Write a driver with shared interrupts
  • Write a driver that uses DMA and where DMA channels can not be exclusively attached to a driver instance
  • Write an average driver (whatever average is)

@b0661 so are you saying that these usecases are not covered by the requirements because we will need code generation inside the .c files, that having a generated .h won't be enough?

No, I don´t say that. For the C compiler it is regardless where the C code comes from. The way you are forcing *.h files is just a matter of taste no technical requirement.

What I´m saying is that by restricting the scope before elaborating requirement makes you miss the requirements that are related to the use cases that you left out because of the restriction.

@nashif nashif added the Enhancement Changes/Updates/Additions to existing features label Nov 2, 2018
@erwango
Copy link
Member Author

erwango commented Nov 6, 2018

Driver <> EDTS binding should be done using dts 'compatible' property.

Must be detailed. 'compatible' is the same for all instances of a device.

  • How shall the instances of a device be bound? - by 'label', by 'address', by ???

I assume you guys mean how we figure out what one would pass to device_get_binding()? Or are we thinking about something else?

My purpose is how do we establish the list of devices (or instances of device) that should be generated against a particular driver template. In Linux, the equivalent operation is done using 'compatible' field at run time. Even if we're doing this operation at pre-compilation, we should use the same approach.

@b0661
Copy link
Collaborator

b0661 commented Nov 6, 2018

Driver <> EDTS binding should be done using dts 'compatible' property.

Must be detailed. 'compatible' is the same for all instances of a device.

  • How shall the instances of a device be bound? - by 'label', by 'address', by ???

I assume you guys mean how we figure out what one would pass to device_get_binding()? Or are we thinking about something else?

My purpose is how do we establish the list of devices (or instances of device) that should be generated against a particular driver template. In Linux, the equivalent operation is done using 'compatible' field at run time. Even if we're doing this operation at pre-compilation, we should use the same approach.

For device_get_binding you always have to use the 'compatible' property.

The question is how do you identify a specific instance of a device type (e.g. UART0 of all the UARTs of a SOC that are of the same type and therefor have the same 'compatible')?

I propose to use the 'label' property to identify a specific instance of a device type - would be 'UART0' in the example above.

The 'compatible' does not provide an unique identification because it is shared by all device instances of the same type. The 'label' should be unique for a SOC device instance (no other device instance is called 'UART0' on the same SOC).

So the 'label' alone is already unique and sufficient to identify the device instance.

@erwango
Copy link
Member Author

erwango commented Nov 7, 2018

The 'compatible' does not provide an unique identification because it is shared by all device instances of the same type. The 'label' should be unique for a SOC device instance (no other device instance is called 'UART0' on the same SOC).

I haven't managed to make myself clear enough.
My point is how to list the instances that will match the device template provided by a driver. We should make clear this should done thanks to 'compatible'.

You're raising another question.

@b0661
Copy link
Collaborator

b0661 commented Nov 7, 2018

of the same type. The 'label' should be unique for a SOC device instance (no other device instance is called 'UART0' on the same SOC).

I haven't managed to make myself clear enough.
My point is how to list the instances that will match the device template provided by a driver. We should make clear this should done thanks to 'compatible'

So lets make it clear:

  • How to identify all instances of a device type that match the device template provided by a driver?

    • by 'comaptible'
  • How to identify a unique instance of a device type that match the device template provided by a driver?

    • by 'label'

@pabigot
Copy link
Collaborator

pabigot commented Dec 1, 2018

I'm not thrilled with this use of the label property which is standardized as "The label property defines a human readable string describing a device." Normally this would be something like "green:inet" for an LED or "Volume Down" for a button. For zephyr something like "High-accuracy accelerometer" or "On-board temperature sensor" would be useful, but not available if we co-opt that property.

The specification does say "The binding specifies the exact meaning for the device", but Zephyr's YAML files all have no more than "(used by Zephyr for the API name)".

I think we should use a different property, such as binding-name, so the tooling might automatically convert something like:

    binding-name = "UART1";

to DT_UART1_BINDING_NAME for driver and application use.

Second, I think each binding description should clearly document the format for the binding-name property values it expects. This would isolation cross-platform application code from having to deal with one board that names its first UART "UART_0" while another names it "uart0" or "USART0" or....

@erwango
Copy link
Member Author

erwango commented Dec 18, 2018

@pabigot , IIUC your recent comments, you changed your mind about use of binding-name.
Can you update your though in this PR, so we don't carry ideas that are not up to date anymore?

@pabigot
Copy link
Collaborator

pabigot commented Dec 18, 2018

@erwango No, I didn't change my mind about binding-name; I still think label is a misuse of an existing property name, inconsistent with other uses already present in Zephyr. But @galak told me label was too entrenched to replace, and that its mis-use would go away when the tooling is complete. Not sure what that implies in the context of this issue and how the requirements and their elicitation are being tracked.

@pabigot
Copy link
Collaborator

pabigot commented Dec 20, 2018

It's very hard to see what the requirements are in this issue, but I think this one is missing:

There are chips that provide features that are covered by multiple subsystem driver APIs. An example is the SX1509B IO extender, which is currently in Zephyr as a GPIO device, but also has an LED controller capability that appears to be compatible with PWM devices. The nrf52_pca20020 has LEDs that are normally driven through the extender. So:

The device tree infrastructure must support multiple distinct device instances associated with a single node.

Or an alternative solution to this problem must be proposed. This issue also arises wrt #12097 though there I prefer the opposite approach (single device instance that interacts with multiple nodes).

At this time I expect to manage by using string token catenation in both the driver and the source, something like:

struct device *pwm = device_get_binding(DT_IOX_LABEL ".PWM");

which is not sustainable.

@b0661
Copy link
Collaborator

b0661 commented Dec 20, 2018

The device tree infrastructure must support multiple distinct device instances associated with a single node.

I think this is not about "multiple distinct device instances associated with a single node" but about a device that combines different "sub-devices" of different type. This can already be described by creating a sub-node for each sub-device. A combi-driver still can handle all the sub-devices.

@pabigot
Copy link
Collaborator

pabigot commented Dec 20, 2018

@b0661 example? In zephyr or in linux? Note that this might not naturally be thought of as multiple devices bundled into one IC. The PWM operations use the same IO signals as the GPIO, so using them in one blocks them from use in the other.

@b0661
Copy link
Collaborator

b0661 commented Dec 20, 2018

The PWM operations use the same IO signals as the GPIO, so using them in one blocks them from use in the other.

Yes. If you blindly activate both you might have a clash. But there is the pinctrl-[x] directive to do fine grained control on the usage of pins. pinctrl-[x] should be used for the pin allocation in such situations.

Linux does have the pin control subsystem where you can manage pin allocation and can detect such clashes at initialisation time or run time. Zephyr is currently missing it.

My conclusion: DTS provides ways to manage the combi-devices. We should not invent our own way of DTS definition but extend Zephyr to handle it completely.

@pabigot
Copy link
Collaborator

pabigot commented Dec 20, 2018

My conclusion: DTS provides ways to manage the combi-devices. We should not invent our own way of DTS definition but extend Zephyr to handle it completely.

Yes. So part of the requirements for extending Zephyr, which is the point of this issue, will be to identify what it means to handle it completely.

The pin case might be handled if there were a plan beyond "we should" to support pinmux in Zephyr. What about a PMIC that provides battery voltage, for which the SENSOR api would be the way to provide it? Maybe we'd do something like:

&i2c0 {
      max17043:max170xx@36 {
        label = "max17043_0";
        compatible = "maxim,max170xx";
        status = "ok";
        fuelgauge;
        reg = <0x36>;
        max17043-voltage {
           compatible="sensor,fuelgauge";
           label="something";
        };
    };
};

Regardless of how it's expressed in the device tree, the tooling has to provide both the drivers and the application with information on named resources. I think there's a general concept of how to deal with "device on a bus" which is currently trending to "use aliases", but not for "apis within a device on a bus". I just want to make sure that gets handled. I don't (at this point) care how.

@b0661
Copy link
Collaborator

b0661 commented Dec 20, 2018

The pin case might be handled if there were a plan beyond "we should" to support pinmux in Zephyr.

The initial reason why I suggested code genereration was to enable pin control using DTS (for pin initialisation and during runtime). The "plan beyond "we should"" is already implemented and working but needs code generation to be upstream.

@kubiaj
Copy link
Contributor

kubiaj commented Jun 27, 2019

Hi I would like to jump into the discussion just to be sure that this issue is also solving my problem or not. As Zephyr is moving everything to the DT then the kernel peripheral driver need to know if his peripheral is enabled or not. For example I have a 3x I2C peripherals so I write one driver which will initialize each peripheral if is is enabled in DT. However in current DT generation scheme (at 1.14.0) there is no stable define which I can use. DT is using relative indexing so when I enable only i2c-1 then the DT generate something like DT_I2C_0_xxxx . Currently there is only one partially stable point in the DT and that is DT_I2C_BASE_ADDRES_xyz however the peripheral address is not the same across various MCU from the same MCU series so it is more like workaround.

Will this issue solve this problem or I should start new issue?

@erwango
Copy link
Member Author

erwango commented Jun 28, 2019

@kubiaj, the issue you mention is common across various archs and socs, it is indeed taken into account.

@kubiaj
Copy link
Contributor

kubiaj commented Jul 10, 2019

Hi,
as this issue is opened from Oct 2018 is there any progress?

@mbolivar-nordic
Copy link
Contributor

@erwango in this comment, I think you are saying you are not in favor of codegen anymore:

#8499 (comment)

Can you close this issue if that's right?

@erwango
Copy link
Member Author

erwango commented Oct 16, 2020

Closing since the initial goals have been achieved (available in code base today).

@erwango erwango closed this as completed Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Changes/Updates/Additions to existing features
Projects
None yet
Development

No branches or pull requests