-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
drivers: use codegen for device instantiation #10888
Conversation
Codecov Report
@@ Coverage Diff @@
## master #10888 +/- ##
=======================================
Coverage 48.23% 48.23%
=======================================
Files 279 279
Lines 43291 43291
Branches 10359 10359
=======================================
Hits 20881 20881
Misses 18270 18270
Partials 4140 4140 Continue to review full report at Codecov.
|
5ef6ec1
to
603140f
Compare
fb1031c
to
45746f3
Compare
@b0661 some drivers of QMSI uses DEVICE_DEFINE() instead of DEVICE_AND_API_INIT() macro to initialize drivers providing power management support via the PM control function. The codgen by default substitues DEVICE_AND_API_INIT(). Is there any way to specify codegen to substitute DEVICE_DEFINE() macro instead of DEVICE_AND_API_INIT() macro? |
Not implemented at the moment. But should not be a problem to switch between DEVICE_DEFINE() and DEVICE_AND_API_INIT() based on pm_control value. device_declare_multi() may become
device_pm_controls maybe a list of pm_control functions or a single pm_control function or None. |
* for x in range(1, 11)] + \ | ||
* ['CONFIG_UART_STM32_LPUART_{}'.format(x) \ | ||
* for x in range(1, 2)] | ||
* driver_names = ['UART_{}'.format(x) for x in range(1, 11)] + \ |
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 don't understand why you do this, you already have the label field that you can use? What if the order of devices in the DTS data is not from low to high? Is there any guarantee that the device order will be the same in the DTS database as in the 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.
I don't understand why you do this, you already have the label field that you can use?
Later on I will search for device where the label value fits to the ones that I set up here.
What if the order of devices in the DTS data is not from low to high?
That does not matter. I just search for a device with the given label value.
Is there any guarantee that the device order will be the same in the DTS database as in the dts file?
This is not necessary - see above.
scripts/codegen/cmake.py
Outdated
from pathlib import Path | ||
|
||
|
||
class CMakeCacheEntry: |
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 do you need the cache information for?
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.
For driver writing.
scripts/codegen/edts.py
Outdated
if extract: | ||
# File produced by the DT compiler | ||
board = self.cmake_variable("BOARD") | ||
dts_compiled = Path(prj_bin_dir, '{}.dts_compiled'.format(board)) |
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.
Why isn't the edts.json file containing the same information as the dts_compiled file? It should not be necessary for the code generator to aggregate information from multiple sources.
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 EDTS database class that I am using has it´s own extract function. It does not depend on extract_dts_includes to fill the database. It the file is already available it just loads the json file. You comment on the etxract part if no edts.json file is available.
device_api, | ||
device_info, | ||
device_defaults = {}): | ||
device_configured = codegen.config_property(device_config, '<not-set>') |
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.
Isn't 'not-set' the default?
return irq_bootstrap_info | ||
|
||
|
||
def device_declare(compatibles, init_prio_flag, kernel_level, irq_func, |
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.
Wouldn't it be easier to just pass a object, for instance a dictionary? I get the chills when I see this many parameters to a function.
init_prio_flag and kernel_level can probably also default to 'CONFIG_KERNEL_INIT_PRIORITY_DEVICE' and 'POST_KERNEL' as it seems that most drivers use this values.
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.
@erwango, can you explain. It is your device_declare variant.
config_struct = _device_generate_struct('config', config_struct) | ||
data_struct = _device_generate_struct('data', data_struct) | ||
if api is None: | ||
api = "(*(const int *)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.
Shouldn't the template trust that the target toolchain has a sane NULL value?
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.
Pleas rebase on topic-EDTS branch.
Same comments as always on the need for higher level device_declare API
* codegen.import_module('devicedeclare') | ||
* | ||
* device_configs = ['CONFIG_CAN_1', ] | ||
* driver_names = ['CAN_1', ] |
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 mentioned several times, this should not be present in API.
This information should be extracted from device tree and drivers <> EDTS binding should be done by 'compatible' property. This is literally its main and only use.
cf #8561
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 mentionend several times to identify a specific driver instance the compatible is not enough. There are usual several driver instantiations for one compatible. So you need something more to identify a single one and use the 'label' property for that. But if you use the 'label' property in addition to the 'compatible' you find out that the 'label' alone is unique und you really do not need the 'compatible'. Taking the 'compatible' and the 'label' is just a waste of computing.
If you would look into the code of your device_declare variant you could easily identify that in the end only the 'label' is used.
* static void ${device-config-irq}(CAN_TypeDef *can) | ||
* { | ||
* LOG_DBG("Enable ${driver-name} IRQ"); | ||
* #ifdef CONFIG_SOC_SERIES_STM32F0X |
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.
'compatible' should be used to identify and provide variations of the same driver.
#ifdef CONFIG_SOC_SERIES_STM32F0X
should not be present in templated code.
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.
Hey this is a proof of concept and not a full blown driver rework, I expect the CAN driver author to do the rework in the end when there is a code generation facility. My focus was the extraction of device tree data not to make that work without some ifdefs.
* device_info = \ | ||
* """ | ||
* DEVICE_DECLARE(${device-name}); | ||
* static void ${device-config-irq}(CAN_TypeDef *can) |
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 whole piece of code, IRQ configuration functions, should be extracted by API.
API should be able to detect if node specifies IRQs, and generated this code if needed, cf #8561
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 know your opinion. This is just to show the principle. You can use your device_declare variant here also. Your variant will fail with STM32F0 UART drivers. A lot of shared interrupts where you definitely need a more sofisticated way to generate interrupt routines. The driver is OOT but I can tell it definitely did not work with the driver_declare interrupt configuration - your one and also not the simple one that I have here.
@@ -20,6 +21,23 @@ LOG_MODULE_REGISTER(i2c_ll_stm32); | |||
|
|||
#include "i2c-priv.h" | |||
|
|||
#if defined(CONFIG_SOC_SERIES_STM32F3X) || defined(CONFIG_SOC_SERIES_STM32F0X) |
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 changes are not related to code generation. Remove from there
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.
In these drivers I added changes whenever CI failed. As already mentioned it is a proof of concept. I do not claim that every driver is right but I want CI to pass. I can not demonstrate the viability of Codegen without CI to pass.
*/ | ||
LL_RCC_GetSystemClocksFreq(&rcc_clocks); | ||
clock = rcc_clocks.SYSCLK_Frequency; | ||
if (cfg->ll_clock_source != LL_I2C_NO_CLOCKSOURCE) { |
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.
Remove from 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.
There were compile errors in CI before !???! Is this gone?
@@ -173,32 +197,10 @@ static int i2c_stm32_init(struct device *dev) | |||
clock_control_on(clock, (clock_control_subsys_t *) &cfg->pclken); | |||
|
|||
#if defined(CONFIG_SOC_SERIES_STM32F3X) || defined(CONFIG_SOC_SERIES_STM32F0X) | |||
/* |
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.
Remove from this change
@@ -338,12 +355,12 @@ static int spi_stm32_configure(struct device *dev, | |||
spi_context_cs_configure(&data->ctx); | |||
|
|||
LOG_DBG("Installed config %p: freq %uHz (div = %u)," | |||
" mode %u/%u/%u, slave %u", |
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.
Why modifying this?
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.
Not by intention. Seems to have happened when SYS_LOG was changed to LOG and I updated.
* codegen.import_module('devicedeclare') | ||
* | ||
* device_configs = ['CONFIG_SPI_{}'.format(x) for x in range(1, 7)] | ||
* driver_names = ['SPI_{}'.format(x) for x in range(1, 7)] |
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.
Cf comments on CAN driver
We've been trying to formalize requirements for driver devices generation here: #10904 (still open for debate, if you want to). To my view, what is missing to the various PR you've been proposing is adherence to above requirements (which were not written down until now, I hope having a formalized version makes things clearer now). This is why Iv'e been pushing #8561. Though, I don't want to push this despite you, so if you feel interested, feel free to push your own version of a codegen solution that could work with requirements. |
Thank you for the comments. I am not quite shure what to do with the comments that relate to driver functions and not to code generation from DTS.
My goal for all drivers is to show Codegen works and the driver passes CI and sometimes some other tests. |
@b0661 thanks for the PR. As mentioned by @erwango this should be rebased on top of the topic branch. During my discussions with the rest of Zephyr TSC members, most of us expressed a dislike for having the generated code embedded in the actual .c file. Instead we would prefer to have a clean separation between generated code (i.e. instantiation) and the actual driver code. |
Yes it is possible. For codegen you simply define the template file and the destination file. It the template file happens to be called my_file.h.in and the destination file my_file.h codegen will produce it the way you are expecting it. |
Will do - but I have to finish some work yet. Sorry - EDIT: What about the build system integration of codegen (btw. as you asked that: I wrote that - not @SebastianBoe). @evenl somehow hijacked the codegen build system integration - will that be changed? EDIT2: Just saw - already done. Thank you @evenl. |
f62ec73
to
953c4b5
Compare
@erwango, @galak, @carlescufi, @evenl Codegen now supports inlined template "snippets" written in Python or Jinja2. Also files that are one template snippet as a whole (without snippet markers) are supported. @evenl, for Jinja2 I copied the device_declare.jinja2 file but renamed it to devicedeclare.jinja2 (no underscore) to make it distinct from jinja2gen. This way codegen and jinja2gen can coexist. The STM32 SPI driver is (mis-)used to show all the different ways to generate code that are supported by codegen:
Compilation is done for the SPI source file that includes the codegen Python generated header file. The generated code (from Python and Jinja2 snippets) in the source file is guarded by #if 0 and not compiled. Both Jinja2 and Python templates have access to the same codegen functions using codegen.funcX() function signatures. Codegen is now also made (hopefully) completely independent from Zephyr. It integrates the EDTS database and can extract from the DTS compiled file and bindings. All relevant info and data can now be provided by options specified on invocation. Codegen should now be usable also standalone. The integration into the Zephyr build system is changed accordingly. The Jinja2 and codegen Python templates used for header creation generate the same code !-) with slightly different indentation. The templates are: Jinja2:
Codegen Python:
|
953c4b5
to
861ee96
Compare
These are generic bindings that the extraction function of the EDTS database can work with. The bindings can be overriden by providing a same named file to cogeno within a directory that is given in the --bindings option. Signed-off-by: Bobby Noelte <[email protected]>
Modules and templates for Zephyr device declaration using code generation by cogeno. devicedeclare.py: a python module to support codegen python templates and cogeno inline templates devicedeclare.jinja2: a jinja2 template to support cogeno jinja2 templates and codegen inline templates Signed-off-by: Bobby Noelte <[email protected]>
Integrate code generation by cogeno into Zephyr build system. If a source file is added by zephyr_sources_codegen(..) or zephyr_sources_codegen_ifdef(..) cogeno will be executed and the generated file stored to the build directory. Compilation will run on the generated file only. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by codegen solution. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by code generation solution. For testing purposes all different ways of code generation are done for this driver: - inline template - cogeno and Jinja2 snippets - generates a source file - Jinja2 template - generates a header file - codegen template - generates a header file Compilation is done for the unmodified source file including the codegen generated header file. Additionally the following adaptations are done: - Extend bindings to allow DTS extract for device info. - Correct DTS to be used in SPI driver. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by code generation solution. Extend binding to allow DTS extraction for device info. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by code generation solution. Extend binding to allow DTS extract for device info. Correct DTS to be used in CAN driver. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by code generation solution. Extend binding to allow DTS extraction for device info. Adapt samples. Signed-off-by: Bobby Noelte <[email protected]>
Replace device instances code by code generation 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]>
Incorrect naming prevents compilation. Change names: - CONFIG_xx_NAME -> DT_xx_NAME - spi_sam0_xx -> spi_sam_xx Signed-off-by: Bobby Noelte <[email protected]>
78c6a1e
to
6350298
Compare
Overview
Use Codegen #10885 for device instantiation.
This PR includes parts of PR #10885. It is the successor to #9163.
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 #10885 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 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]