scripts: extract_dts_includes: Add simple defines for i2c/spi children

[galak]

To simply and reduce what one has to do in a fixup file for i2c & spi
devices, we generate aliased defines based on just the compatible name.
For now we make the assumption that there is only one sensor of a given
compatible in the system (which is how all I2C & SPI client drivers are
currently).

Signed-off-by: Kumar Gala [email protected]

[galak]

Here's a WIP commit to add generation of the defines for SPI & I2C devices. This is in relationship to the discussion in #11737

[galak]

@pabigot patch needs cleaning up, better comments, etc. But it should at least accomplish the job on the generation side.

[pabigot]

I have absolutely no idea how these changes do what they do, but the generated_dts_board.h looks right. Thanks!

[codecov-io]

Codecov Report

Merging #11886 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11886   +/-   ##
=======================================
  Coverage   48.27%   48.27%           
=======================================
  Files         295      295           
  Lines       44281    44281           
  Branches    10601    10601           
=======================================
  Hits        21376    21376           
  Misses      18636    18636           
  Partials     4269     4269

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d5e5603...651eb8e. Read the comment docs.

[erwango]

Seems fine to me, but maybe could be used to address my concern about #11854: we miss a way to instruct, with info coming from dts/bindings, if driver should be compiled with SPI or I2C version.
Since we don't need to care about sensor instance yet, it could be a boolean like:
#define DT_AMS_CCS811_BUS_I2C/SPI 1
So sensor driver could be compiled either for I2C or SPI variant.

[mnkp]

I would like to review this PR, please give me some time to do it.

[pabigot]

@erwango agreed; that's what was proposed when we talked. It's not required for my short-term needs but for completeness it should be there if it's easy to do.

[mnkp]

This feels like we start going down the rabbit hole. It's a relatively dirty hack on top of another relatively dirty hack (the decorated alias defines). And it's likely going to be difficult to untangle it later.

What's stopping us from providing a clean solution?

[pabigot]

I'm not sure what a "clean solution" is and if other people have an idea there's certainly no consensus on it. So that's stopping us.

I don't see this as breaking anything that's already in master: it adds some defines to the generated header. If it, after being augmented with the ability to determine parent bus, gets merged then I can convert some sensor drivers to something closer to the goal state: device instances tied to configuration object instances, manually populated now but ultimately done automatically.

In those PRs people would have a chance to see what it would look like eventually, which is really hard to perceive at this state. If there isn't consensus that this is an improvement, then the extra defines can be dropped and another approach tried.

[pabigot]

This is entirely decoupled from alias, if that wasn't clear. Essentially only compatible appears, and only for nodes subordinate to a bus.

[galak]

This feels like we start going down the rabbit hole. It's a relatively dirty hack on top of another relatively dirty hack (the decorated alias defines). And it's likely going to be difficult to untangle it later.

What's stopping us from providing a clean solution?

The cleaner solution is code generation, but that will be post 1.14. We need something simpler than the full code generation solution for 1.14 that makes it so that dts_fixup.h isn't need for a I2C/SPI client drivers.

Since all the I2C/SPI client drivers only handle single instances figured this was a reasonable stop-gap.

I'm open to any other ideas on how to not require modifications to dts_fixup.h for the I2C/SPI client drivers.

[galak]

@pabigot can you look at tests/drivers/i2c/i2c_slave_api. Seems like we have a case of 2 of the same device.

[pabigot]

@galak indeed, two instances of the same EEPROM on different buses. They happen to have different addresses too, but that's not a reliable distinguisher. This is the case we didn't think we had to handle. It works today because they have different label properties and the code uses the legacy old aliases which incorporate that text.

If the only instance of problem is in test code we could work around it. One approach is to detect the collision and, if one exists, emit a diagnostic and drop both alias names, requiring the implementation to use the full identifiers. To do that you'd need an alias dictionary that covered all nodes. It might be worth having that anyway, as getting the diagnostic from the generator could be more informative than "redefined macro" at compile time.

If we're not willing to work around it, then compatible is already insufficient as a unique identifying prefix and we have to use either an alias or the label property.

[galak]

@galak indeed, two instances of the same EEPROM on different buses. They happen to have different addresses too, but that's not a reliable distinguisher. This is the case we didn't think we had to handle. It works today because they have different label properties and the code uses the legacy old aliases which incorporate that text.

If the only instance of problem is in test code we could work around it. One approach is to detect the collision and, if one exists, emit a diagnostic and drop both alias names, requiring the implementation to use the full identifiers. To do that you'd need an alias dictionary that covered all nodes. It might be worth having that anyway, as getting the diagnostic from the generator could be more informative than "redefined macro" at compile time.

If we're not willing to work around it, then compatible is already insufficient as a unique identifying prefix and we have to use either an alias or the label property.

I have no issue if you want to figure out a working around in the sample.

[pabigot]

I could do that to the test case but the only way is to make the devices have different compatibility strings. Then I'd have to change my review to reject the patch because it's a valid test providing circumstantial evidence that multiple instances of the same device on different buses is a real thing. If we merged the change the diagnostic would just show up on those applications when their maintainers updated Zephyr.

My Python skills are rusty and I don't understand the extract scripts well enough to implement the global registry prefix deconflict workaround instead.

So it looks like relying on compatible alone won't work and we have to stick with either alias or the label property. After I get some other things done I'll return to this and see what can be done along those lines. AFAICT we generate no prefixes based on the label property; for an alias node we currently generate:

#define CCS811_LABEL			DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL
#define DT_AMS_CCS811_CCS811_LABEL	DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL

and tacking on yet another just to get:

#define DT_CCS811_LABEL			DT_NORDIC_NRF_I2C_40003000_AMS_CCS811_5A_LABEL

is gross so I'd probably just use the old alias prefix CCS811_x; unless we can get revert part of a7c430f and remove the compatibility component in the "new" aliases, which several of us want gone anyway (cf post-merge comments in #11180).

[pabigot]

Further experimentation suggests this approach is not unique, and has a good chance of breaking existing applications without significant work described in other comments.

[mnkp]

I'm not sure what a "clean solution" is and if other people have an idea there's certainly no consensus on it.
I don't see this as breaking anything that's already in master: it adds some defines to the generated header.

That's the main problem with this PR, we're going for a quick win without working out the concept. Even if this PR worked it would introduce inconsistency. A few more PRs like this one and soon no one would understand which defines are generated when and what for. And DTS is a critical part of the infrastructure.

This is entirely decoupled from alias, if that wasn't clear.

This PR is (re-)using part of the code which is responsible for generating aliases, i.e. it's relying on the fact that defines generated for aliases are prefixed with compatible string. If we changed that this PR would break.

[pabigot]

That's the main problem with this PR, we're going for a quick win without working out the concept.

I don't disagree. Lacking sufficient information on the detailed design for code generation my goal is short-term relief of the dts_fixup pain we're already experiencing, so at least we get some shared experience with where we're heading before the tooling shows up.

This PR is (re-)using part of the code which is responsible for generating aliases, i.e. it's relying on the fact that defines generated for aliases are prefixed with compatible string. If we changed that this PR would break.

No, this PR proposed adding a third prefix that did not include alias, just compatible. Since it won't work, we'd have to fall back to one of the two solutions that are already in the baseline: the legacy alias-only prefix that doesn't include DT_, or the newer DT_ + compatible + alias one. Which I would really like to see changed to remove compatible, as both you and I have proposed.

[pabigot]

I dislike the re-use here of cell-index, which is a non-standard property still used in Linux primarily in Freescale device tree bindings, serving a poorly defined and possibly inconsistent role. Its use here to identify a device instance is too opaque. Use a property name that makes sense, like device-instance, or one that's clearly documented somewhere official to provide the information that's needed (if such a property exists).

Support for multiple instances should be consistent, not added in an ad-hoc manner to specific devices based on whether we've seen more than one instance. Whatever property name is used it should default to zero, be present in the base device YAML file rather than added only to specific devices, and the generated symbols should always include the device instance.

I'm growing even more sympathetic with @mnkp's comment that we "keep going for a quick win without working out the concept". I don't like this "let's try this; no that doesn't work, how about this" development model. All of this should have been thought through before it started getting merged, or if that wasn't possible there should have been a commitment to and programmatic oversight of rapid evolution to an acceptable long-term solution.

On that basis, I'm inclined to stop trying to fix things and just live with the dts_fixup.h files, disgusting as they are. At this time I won't approve or reject this, but will leave it to others to comment to see if a consensus can emerge.

[pabigot]

This should default to 0 so that if a device (like eeprom) that supports multiple instances is used on a board that only populates one instance we don't have to fuss with mapping the unqualified names to the ones suffixed by the instance.

[galak]

What exactly do you mean by default to 0. Are you saying that for cell-index 0 we should NOT do 0 in the defines?

[pabigot]

I mean move the property to the SPI and I2C base YAML descriptions, and either make it required so people have to specify the instance number, or allow absence of an instance to behave as if it was provided with a value 0.

[galak]

Ok, will add cell-index to the i2c-device & spi-device yaml files. That makes sense to me. If there is something more than that let me know.

Outdated early review

[galak]

This feels like we start going down the rabbit hole. It's a relatively dirty hack on top of another relatively dirty hack (the decorated alias defines). And it's likely going to be difficult to untangle it later.

What's stopping us from providing a clean solution?

The time for LTS, the clean solution is code generation that removes the need for the define generation and fixups. However we want to put something in place for the LTS/1.14 release. I agree this is paper taping over the issue.

[galak]

@erwango can you review this again. I want some solution for this release until we move to code gen after.

[mnkp]

If we are talking about creating defines following the pattern

DT_<compatible>_<index>_<property_name>

that's actually a very interesting proposal. It's scalable, consistent and not that much different to how binary blobs work. I believe we do not need to introduce cell-index property though. The script is aware of how many nodes with a given compatible property are present in DT. It can then generate a unique, consecutive number starting at 0 for every such node.

Such defines could also be used by SoC drivers. We know that at the moment we misuse aliases. With the above approach we wouldn't need to.

One difference to our current way of working with DT is that it will not be possible any more to enable driver instances in Kconfig since there will be no direct relation between instance number as defined in datasheet and the define generated for this instance. This is actually for the better.

[pabigot]

I believe we do not need to introduce cell-index property though. The script is aware of how many nodes with a given compatible property are present in DT. It can then generate a unique, consecutive number starting at 0 for every such node.

This would have to be done very carefully to be robust in the case of somebody refactoring the device-tree source, e.g. moving the include for an overlay that describes one instance above or below a node that describes another instance.

The instance number really should be explicit, with a default of 0 inferred when the corresponding property is absent which will be the most common case for SPI/I2C peripherals.

We can also bike-shed the property name; I don't like cell-index because it's opaque and is used in Linux in ways that aren't uniformly consistent with this usage (e.g. as the ordinal of a 4 KiBy page in a SOC memory region). device-instance, zephyr,device-instance, and zephyr,instance are candidates (I'm backing away from zephyr,index that I mentioned in another context).

[galak]

This would have to be done very carefully to be robust in the case of somebody refactoring the device-tree source, e.g. moving the include for an overlay that describes one instance above or below a node that describes another instance.

Curious, why would moving / refactoring the DTS source impact what the ID was?

The instance number really should be explicit, with a default of 0 inferred when the corresponding property is absent which will be the most common case for SPI/I2C peripherals.

If I understand what @mnkp is suggesting, that we just enumerate the instances of a given compatible starting at 0. Let's say there are 3 instances for "i2c,eeprom". It should not mater which of those instances is 0, 1, or 2.

Is there some case you can think of where it would mater?

We can also bike-shed the property name; I don't like cell-index because it's opaque and is used in Linux in ways that aren't uniformly consistent with this usage (e.g. as the ordinal of a 4 KiBy page in a SOC memory region). device-instance, zephyr,device-instance, and zephyr,instance are candidates (I'm backing away from zephyr,index that I mentioned in another context).

Yeah, I'm open to whatever we want to call it if we go this direction.

[pabigot]

If I understand what @mnkp is suggesting, that we just enumerate the instances of a given compatible starting at 0. Let's say there are 3 instances for "i2c,eeprom". It should not mater which of those instances is 0, 1, or 2.

Is there some case you can think of where it would mater?

I can't think of a case where it wouldn't, unless there's some other indicator I can use in applications and board setup to determine which instance I'm using for a given operation. I am assuming that part of the product of this scheme would be DT_I2C_EEPROM_0_LABEL.

For EEPROM, I might have a 4 kb one for configuration data, and a 64 Mb one for logs.

For sensors I might have one that's on the board, and one that's connected through a cable.

In either case, they are not interchangeable.

Am I missing something here?

[erwango]

I guess starting at 0 is not mandatory, is it?

[erwango]

I'm fine with the change as it is. I don't think we should enumerate available instances because index should not be changed if .dts is reorganised (first instance in the file would get _0 while I expected _1 in my code) or as mentioned by @pabigot some overlay is used or not.
So zephyr,index is good to me and actually I think we should not provide a constraint on the numbering scheme (why should it start from 0?).

[erwango]

I've been a bit fast on the approval: I've one minor concern which is is the constraint on numbering.

[galak]

I can't think of a case where it wouldn't, unless there's some other indicator I can use in applications and board setup to determine which instance I'm using for a given operation. I am assuming that part of the product of this scheme would be DT_I2C_EEPROM_0_LABEL.

For EEPROM, I might have a 4 kb one for configuration data, and a 64 Mb one for logs.

For sensors I might have one that's on the board, and one that's connected through a cable.

In either case, they are not interchangeable.

Am I missing something here?

Are we mixing how an application determines which device instance it wants from how a driver instantiates those instances?

Let's use an example with the eeprom so we are talking about something concrete:

Here's what the DTS would look like:

i2c@40005400 {
		compatible = "st,stm32-i2c-v2";
		label = "I2C_1";

        ep_0: eeprom@54 {
                compatible = "i2c,eeprom";
                reg = <0x54>;
                label = "EEPROM_SLAVE_4K";
                size = <4096>;
        };

        ep_1: eeprom@56 {
                compatible = "i2c,eeprom";
                reg = <0x56>;
                label = "EEPROM_SLAVE_64M";
                size = <64*1024*1024>;
        };
};

Here's what we'd generate:

#define DT_I2C_EEPROM_0_BASE_ADDRESS 0x54
#define DT_I2C_EEPROM_0_BUS_NAME "I2C_1"
#define DT_I2C_EEPROM_0_LABEL "EEPROM_SLAVE_4K"
#define DT_I2C_EEPROM_0_SIZE 4096
#define DT_I2C_EEPROM_0 1

#define DT_I2C_EEPROM_1_BASE_ADDRESS 0x56
#define DT_I2C_EEPROM_1_BUS_NAME "I2C_1"
#define DT_I2C_EEPROM_1_LABEL "EEPROM_SLAVE_64M"
#define DT_I2C_EEPROM_1_SIZE 64*1024*1024
#define DT_I2C_EEPROM_1 1

Driver would look like:

#ifdef DT_I2C_EEPROM_1

static struct i2c_eeprom_slave_data i2c_eeprom_slave_1_dev_data;

static u8_t i2c_eeprom_slave_1_buffer[(DT_I2C_EEPROM_1_SIZE * 1024)];

static const struct i2c_eeprom_slave_config i2c_eeprom_slave_1_cfg = {
        .controller_dev_name = DT_I2C_EEPROM_1_BUS_NAME,
        .address = DT_I2C_EEPROM_1_BASE_ADDRESS,
        .buffer_size = (DT_I2C_EEPROM_1_SIZE * 1024),
        .buffer = i2c_eeprom_slave_1_buffer
};

DEVICE_AND_API_INIT(i2c_eeprom_slave_1, DT_I2C_EEPROM_1_LABEL,
                    &i2c_eeprom_slave_init,
                    &i2c_eeprom_slave_1_dev_data, &i2c_eeprom_slave_1_cfg,
                    POST_KERNEL, CONFIG_I2C_SLAVE_INIT_PRIORITY,
                    &api_funcs);

#endif /* DT_I2C_EEPROM_1 */

Your application would do something like:

struct device *dev = device_get_binding("EEPROM_SLAVE_4K");

[galak]

If in my example the generator looks like this: (change which instance was 0 and 1)

#define DT_I2C_EEPROM_0_BASE_ADDRESS 0x56
#define DT_I2C_EEPROM_0_BUS_NAME "I2C_1"
#define DT_I2C_EEPROM_0_LABEL "EEPROM_SLAVE_64M"
#define DT_I2C_EEPROM_0_SIZE 64*1024*1024
#define DT_I2C_EEPROM_0 1

#define DT_I2C_EEPROM_1_BASE_ADDRESS 0x54
#define DT_I2C_EEPROM_1_BUS_NAME "I2C_1"
#define DT_I2C_EEPROM_1_LABEL "EEPROM_SLAVE_4K"
#define DT_I2C_EEPROM_1_SIZE 4096
#define DT_I2C_EEPROM_1 1

The driver code and application code stays identical and things work exactly the same.

[mnkp]

@galak was faster providing the code sample. I'll repeat the same in words (since I already wrote it).

If we follow approach DT_<compatible>_<index>_<property_name> it doesn't matter any more which specific module instance is assigned which index as long as indexes are unique and do not exceed number of instances defined in the driver. If user enables in DT i2c_3 instance (as defined in the datasheet) it will be assigned index 0. If later they decide to use also i2c_1 instance (as defined in the datasheet) or maybe some .overlay file will enable it then i2c_3 will be assigned index 1. That's fine. Driver code is agnostic to it.

Since the application gets handle to the driver instance via _LABEL property it is always able to get the specific instance it needs.

[galak]

I've been a bit fast on the approval: I've one minor concern which is is the constraint on numbering.

what do you mean by constraint on numbering?

[pabigot]

@galak you're right; I've been working from an idiom where I identified resources based on aliases, so I'd have done:

aliases {
   logeeprom = &ep_1;
}

and referenced it by DT_LOGEEPROM_LABEL. (Mostly because I dislike using ASCIIZ strings as device identifiers in embedded systems, and this path is more futureproof.)

So I agree there's no reason to try to make sure the instance number is persistent. Then it isn't clear why we'd need an instance number in the bindings (source) at all so we might as well get rid of zephyr,index too.

Unless I'm also wrong about #12226 (comment), though, we should try to avoid the concern described there (FOO and FOO_1 vs FOO_0 and FOO_1).

[erwango]

what do you mean by constraint on numbering?

Seems I was confused as well between application and driver. You and @mnkp made it clear now.
Let's simplify the change and spare the zephyr-index.

[galak]

Agreed, we don't need an explicit index

Seems I was confused as well between application and driver. You and @mnkp made it clear now.
Let's simplify the change and spare the zephyr-index.

Agreed.

[galak]

Unless I'm also wrong about #12226 (comment), though, we should try to avoid the concern described there (FOO and FOO_ vs FOO_0 and FOO_).

Agreed, regardless of the number of instances, we would always just have FOO_0

Wait next iteration

[erwango]

Agreed, regardless of the number of instances, we would always just have FOO_0

So does it mean that all current bus slaves drivers should be reworked to use _0 instance then?

[pabigot]

So does it mean that all current bus slaves drivers should be reworked to use _0 instance then?

I think "yes". When this is available I'll update #11977 so we can see how it looks in a couple cases before going through and updating everything.

[carlescufi]

cc @anangl

[galak]

I pushed a new PR with the solution discussed here. The new PR is #12426.