-
Notifications
You must be signed in to change notification settings - Fork 8.2k
drivers: use codegen for device instantiation #9163
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
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9163 +/- ##
=======================================
Coverage 52.15% 52.15%
=======================================
Files 212 212
Lines 25916 25916
Branches 5582 5582
=======================================
Hits 13517 13517
Misses 10149 10149
Partials 2250 2250Continue to review full report at Codecov.
|
af9a3cf to
e527a87
Compare
drivers/spi/spi_ll_stm32.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it strictly necessary to remove this header for this change ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it is not necessary. It just happened over the various adaptations to the changing requirements to codegen.
drivers/spi/Kconfig.stm32
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this would be great, but this should go in a separate changeset
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Of course it can go to a different changeset. This PR includes various changes that I applied to make the drivers work with codegen (or better work). Currently it is a massive maintenance/ rebase effort that increases with every changeset to be managed. If codegen is accepted I can restructure the driver changes.
drivers/spi/spi_ll_stm32.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I maybe missed a bit, but where do you declare the device compatible ? How do you link with the DT node and it's attributes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I declare the driver names. Device compatible is then read from the device tree. Linkage to the device tree attributes is done by the placeholders - e.g ${reg/0/address/0} relates to the device tree reg property address 0 of register 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you clarify link between driver names and DT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The driver name (aka. the label property from DTS) is used to identify the driver instance and to retrieve all the data from the extended DTS database. The DTS database is searched for a device where the label property matches the driver name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it a Zephyr policy to use label for that ?
In the normal DT world (Linux, U-Boot, ...) we use the compatible and the aliases to determine the driver and the id of the device, the "label" property is only used to give a label on the device, not to link device<->driver.
AFAIK we used the compatible to generate the defines (ST_STM32_I2C_V2_) and we used the label to generate the config (CONFIG_I2C_1_). Then we used the fixup to link the two.
Why can't we generate the fixups using codegen ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@b0661 , I agree with @superna9999 on the usual use of 'compatible'. It should be sufficient to link driver to the nodes. Using 'label', you might face issues, for instance when it is not numbered, but use letters for enumeration, or with only one instance.
@superna9999 , indeed they can be generated. But ideally fixups should not be needed and we should be able to generate directly the device init code with values from dt, w/o using intermediate defines. Btw, in #8561, I'm demonstrating an generation API, also based on codegen, and using 'compatible' as the link between driver and dts file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 'label', you might face issues, for instance when it is not numbered, but use letters for enumeration, or with only one instance.
This is not a problem as the driver writer knows about the labels and can create whatever list of labels is necessary. This is not restricted to numbers.
The driver writer can also derive the labels from the 'compatible' devices as you do in your device_declare API. As this is a raw API it is just made visible. The real benefit is that you do not have to assume/ impose that all config values are of the form CONFIG_[label] but can be of any form as long as they are listen in the same sequence as the related labels.
You always need a link between the driver instance and the DTS instance. There are only two practical properties, the label and the base address. Also in your device_declare API you have choosen to link the DTS by label. It is just hidden behind the API. The label is first searched by the 'compatible' property and then used for the CONFIG_[label] value.
To have full flexibilty for the raw version I am using the label for the linkage to the DTS. How these labels are found/ defined is up to a more sophisticated API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. and we used the label to generate the config (CONFIG_I2C_1_).
This is a linkage done by the label.
As explained before the linkage by label is a minimal low level interface that provides unique linkage between a driver instance and the related DTS instance. It provides maximum flexibility to build sopisticated device_declare APIs (as @erwango ´s) on top. A base interface should not impose restrictions where restrictions are not necessary. Using 'compatible' would impose such unnecessary restrictions. @erwango ´s API implementation shows that you can implement 'comaptible' based linkage on top of the raw interface.
drivers/i2c/i2c_ll_stm32.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this go in a separate changeset ? Please stick to codegen only changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to do this change to use the codegen devicedeclare easily. The selction of clock source by config define and base address in i2c_stm32_runtime_configure was not possible the way it was after conversion to codegen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @superna9999 , difficult to review and comment on mix of several new additions.
Please remove the clock source part and introduce it in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is about introducing codegen for driver intiantiation. The PR includes various changes that I applied to make the drivers work with codegen (or better work). Currently it is a massive maintenance/ rebase effort that increases with every changeset to be managed. If codegen is accepted I can restructure the driver changes.
erwango
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented, I'm not in favor of this raw device driver instantiation generation.
Main point is generated code should stick with available dts properties and we should be able to detec and warn when a property is missing and not generate bogus code.
This is why code generation requires some 'magic' that could be provided by a more elaborated generation function.
doc/subsystems/codegen/modules.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As discussed earlier, there should be no need to provide the number of available IPs.
Besides, it should be updated when a new SoC with a n+1 instance is introduced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. Will also work if you replace the upper limit by e.g. 100. Should be enough for Most SoCs.
doc/subsystems/codegen/modules.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here (instance numbering range should not be needed).
Besides, couldn't we use dts property 'label' instead of yet another instance name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above. The driver writer can choose an upper limit that covers all future SoCs. As only the configured and activated devices will be intiantiated the upper limit can be any big number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides, couldn't we use dts property 'label' instead of yet another instance name.
The driver name is the same as the 'label' property. In the INIT macro it is called driver_name so I stick with it. The extended DTS database is searched for a matching 'label' property to retrieve the DTS properties of the device instance with the given driver name.
drivers/can/stm32_can.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #ifdef should not be available in driver instantiation template (and this is why I'm not in favor of full 'raw' templates).
In this example, when generating this code with a F0 dts file, F0 dts file does not provide rx, tx interrupts properties.
So, either generation will break because properties are not available.
Either, you're able to fill missing property with '0', generating bogus code (which is not clean even if won't be compiled). And it also means you're not able to generate a warning/error when a missing property occurs in the F0 case (which is, I think, required).
This is why I think driver instances code generation requires some magic and could not be fully raw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the raw version of device declare as I explained in several places. Your sophisticated version of device_declare can go without #ifdefs. The raw version shall just show how easily the non codegen code can be transformed to use codegen, and how readability and maintainability is given.
I transfered your device_declare API to the devicedeclare codegen module. Please see how this uses the raw API to provide the sopisticated API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think #ifdef should not be available in driver instantiation template (and this is why I'm not in favor of full 'raw' templates).
In your sopisticated device_declare API you are using #ifdef behind the scenes for the interrupt flag. The raw version sould support to be used to implement your more sophisticated API. Also the raw version is only for cases where the sophisticated device declare API does not provide enough control on the driver info template. Adding any limitations to the usage of C code in the template would render the extended template control worthless.
So, either generation will break because properties are not available.
Generation will not break - but compilation will break if DTS properties are not available because the placeholder will not be replaced in this case.
And it also means you're not able to generate a warning/error when a missing property occurs in the F0 case (which is, I think, required).
Compiler error should be enough to provide guidance for the driver writer/ tester. There is no undetected invalid code.
drivers/i2c/i2c_ll_stm32.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree with @superna9999 , difficult to review and comment on mix of several new additions.
Please remove the clock source part and introduce it in another PR
|
Can‘t remove, won‘t complle. |
|
FYI: @nordic-krch, @anangl |
As I transfered your device_declare API to the codegen devicedeclare module every driver author can choose to use your more sopisticated API. I also added this to the documentation suggesting to use the raw API only in case the sopisticated API does not provide enough control on the driver info template.
The generated code just does not compile if a DTS property is not available. Template placeholders (like e.g. ${label}) are not replaced if the property is not available. The C compiler will complain about the literal (e.g. ${label}).
The magic for detection and warning in case of missing DTS properties is already build into the template placeholder replacement. |
e527a87 to
df95729
Compare
|
@b0661 , motivation for this PR is a bit clearer now. I understand this is to demonstrate capability to provide other API to use codegen, and demonstrated one is raw, which indeed could be useful in some cases. |
As explained above the linkage by 'label' is a low level/ raw interface. Sophisticated device declare APIs (as yours) are based on the raw interface. The global linkage by 'compatible' is also using the linkage by 'label' to have a unique driver instance to DTS instance mapping behind the scenes. Quotation from above: The driver writer can also derive the labels from the 'compatible' devices as you do in your device_declare API. As this is a raw API it is just made visible. The real benefit is that you do not have to assume/ impose that all config values are of the form CONFIG_[label] but can be of any form as long as they are listen in the same sequence as the related labels. You always need a link between the driver instance and the DTS instance. There are only two practical properties, the label and the base address. Also in your device_declare API you have choosen to link the DTS by label. It is just hidden behind the API. The label is first searched by the 'compatible' property and then used for the CONFIG_[label] value. To have full flexibilty for the raw version I am using the label for the linkage to the DTS. How these labels are found/ defined is up to a more sophisticated API like yours. |
df95729 to
85a11c0
Compare
doc/subsystems/codegen/codegen.rst
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, I suspect this should read:
The lines immediately before the ``@code{.codeins}@endcode`` comment
are the output from the generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The output is filled between @endcode{.codegen} and @code{.codeins}@endcode. There is no output currently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a silly question, but I mention this because I'm wondering how careful folks need to be about using the exact layout you give in your examples, and what happens if they add comments around these constructs. For example, what will the output look like if I were to add some more comments after the @Endcode{.codegen} or I futz around with the @code{.codeins}@Endcode comment block? Are my comments preserved in the generated output?
/* This is my C file. */
...
/**
* @code{.codegen}
* fnames = ['DoSomething', 'DoAnotherThing', 'DoLastThing']
* for fn in fnames:
* codegen.outl("void %s();" % fn)
* @endcode{.codegen}
* Here are some more comments about the codegen block
*/
/**
* And here is another comment
* @code{.codeins}
* @endcode
*/
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what will the output look like if I were to add some more comments after the @Endcode{.codegen} or I futz around with the @code{.codeins}@Endcode comment block? Are my comments preserved in the generated output?
Everything between @endcode{.codegen} and @code{.codeins}@endcode will be replaced by the generated code in the generated file. There will be no changes in the source file itself - it is preserved there.
You should not add comments after @endcode{.codegen} and/ or before @code{.codeins}@endcode. You should also not split the @code{.codeins}@endcode marker into two lines.
I did not test all the possible fuzz around. If some standard fuzzing patterns will emerge the parser will have to be made more robust. Currently it assumes some well defined marker structure.
This will probably work to comment the codegen block in the source file. It will be replaced in the generated file. So it does not make much sense.
/* This is my C file. */
...
/**
* @code{.codegen}
* fnames = ['DoSomething', 'DoAnotherThing', 'DoLastThing']
* for fn in fnames:
* codegen.outl("void %s();" % fn)
* @endcode{.codegen}
*/
/* Here are some more comments about the codegen block
*/
/* And here is another comment
*/
Some other woodoo.
/** @code{.codeins}@endcode */
...
85a11c0 to
f097ca5
Compare
f097ca5 to
6962534
Compare
b40838b to
7aa0de8
Compare
| processed by Codegen and the generated source files are written to the CMake | ||
| binary directory. | ||
|
|
||
| The inlined Python snippets can contain any Python code, they are regular |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have security implications with ANY Python code allowed? I suppose thorough code reviews would catch something malicious being added, and I suspect it's hard to limit the allowed Python snippets.
Codegen is a file generation tool. It uses pieces of Python code in
the source file as generators to generate whatever text is needed.
If a source file is added by zephyr_sources_codegen(..) or
zephyr_sources_codegen_ifdef(..) codegen will be executed and
the generated file stored to the build directory. Compilation
will run on the generated file only.
Codegen is based on Cog. All files taken from Cog are modified.
The files are marked by the Cog author's copyright:
Copyright 2004-2016, Ned Batchelder.
http://nedbatchelder.com/code/cog
SPDX-License-Identifier: MIT
Origin: https://pypi.python.org/pypi/cogapp
Status: 2.5.1
Description: A code generator for executing Python snippets in source
files.
Dependencies: python
URL: https://pypi.python.org/pypi/cogapp
commit: 2.5.1
Maintained-by: External
License: MIT
License Link:
URL: https://pypi.python.org/pypi/cogapp
Signed-off-by: Bobby Noelte <[email protected]>
Document the inline code generation tool codegen and its integration in the CMake build process. Signed-off-by: Bobby Noelte <[email protected]>
Create extended DTS database to be used by codegen and others. Signed-off-by: Bobby Noelte <[email protected]>
Extract compatible info and make it available by EDTS. This way the compatible info of all activated devices can be used in drivers. Signed-off-by: Bobby Noelte <[email protected]>
Extend reg directive handling to fill the EDTS datanbase. Signed-off-by: Bobby Noelte <[email protected]>
Extend interrupts directive handling to fill the EDTS database. Signed-off-by: Bobby Noelte <[email protected]>
Extend clocks directive handling to fill the EDTS database. Signed-off-by: Bobby Noelte <[email protected]>
Extend default directive handling to fill the EDTS database. Signed-off-by: Bobby Noelte <[email protected]>
Extend directive handling to use heuristics to fill the EDTS database. Signed-off-by: Bobby Noelte <[email protected]>
modules/devicedeclare.py: Device declaration functions. Signed-off-by: Bobby Noelte <[email protected]>
Document the modules that can be imported and the templates that can be included by inline code generation. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by codegen solution. Extend binding to allow DTS extraction for device info. Remove defines from dts.fixup that are now taken from DTS. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by codegen solution. Extend bindings to allow DTS extract for device info. Correct DTS to be used in SPI driver. Remove defines from dts.fixup that are now taken from DTS. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by codegen solution. Extend binding to allow DTS extraction for device info. Remove defines from dts.fixup that are now taken from DTS. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by codegen solution. Extend binding to allow DTS extract for device info. Correct DTS to be used in CAN driver. Remove defines from dts.fixup that are now taken from DTS. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by codegen solution. Extend binding to allow DTS extraction for device info. Remove defines from dts.fixup that are now taken from DTS. Adapt samples. Signed-off-by: Bobby Noelte <[email protected]> drivers: pwm: stm32: use codegen driver instantiation (SQUASH#1) Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by codegen solution. Add binding to allow DTS extract for device info. Add DTS to be used in EXTI driver. Signed-off-by: Bobby Noelte <[email protected]>
The NUCLEO FO91RC, F070RB, F030R8 boards use SPI_1 for loopback test. Signed-off-by: Bobby Noelte <[email protected]>
7aa0de8 to
fe86193
Compare
Overview
Use codegen #6762 for device instantiation.
This PR includes PR #6762.
Motivation for or Use Case
Demonstrate the usage of codegen and the included devicedeclare module for device instantiation.
Design Details
The following drivers are adapted to use codegen for device instantiation:
Codegen #6762 provides two device declaration functions in it's devicedeclare module.
Provides a sophisticated device declaration API. It is transferred from @erwango 's work in Generated driver instantiation prototype #8561.
Provides a more raw device declaration API that allows for a fine grained control of the device instantiation template. It is the basis also for devicedeclare.device_declare.
All drivers above except INT are adapted to use the devicedeclare.device_declare_multi API.
The interrupt driver uses the devicedeclare.device_declare API. It is a minimal conversion to show the feasibility. For elaborated examples of the devciedeclare.device_declare API see #8561.
Codegen's device declaration is done using a device info template with placeholders that is used for every device instance. The placeholders are replaced by the respective device tree, or Kconfig, or device instance values:
Test Strategy
This PR extends the SPI loopback test for the NUCLEO F091RC board. It was tested using this test and some
sample applications.
Signed-off-by: Bobby Noelte [email protected]