include: devicetree: Generate defines for map property

[soburi]

This change introduces generating definitions corresponding to
*-map property, which was currently discarded.

The *-map data is like a two-dimensional array, so it is difficult to
handle with the existing APIs, so we will also provide new APIs.

---

Common utilities have been separated into a separate PR.
Please take a look at this first. #84190

---

I noticed this problem while developing https://github.com/zephyrproject-rtos/gsoc-2022-arduino-core.

Since map is used to associate with the physical layout, there is a need to know the "physical pin names".

Since the mapping process is performed during the generation of devicetree-generated.h, I don't think there are many cases where information about the mapping source is needed. However, there are such cases, however rare, and it is not desirable for the information written in the devicetree to be discarded, so I would like to introduce this.

[mbolivar]

@soburi thank you for your update. I think there is enough in here for me to give this a good review, which I will do as soon as I can. (I am traveling this week on a work trip so my availability will be a bit less than it was last week, but I want to prioritize your PR among other DT review requests I have.)

As an overall design question, I'm not sure that I think that ControllerAndData is an appropriate class to be using to represent the maps.

The zephyr phandle-array type is used when we have a list of (phandle, specifier) pairs. But in the case of generic maps, we don't have pairs, we have triples: (child_specifier, specifier_parent_phandle, parent_specifier). And in the case of interrupt maps, we have 5-tuples: (child_unit_address, child_interrupt_specifier, interrupt_parent, parent_unit_address, parent_interrupt_specifier).

I think that neither of these types of information is really a ControllerAndData due to the additional "child -> parent" information.

I am currently thinking it would make more sense to introduce two new types:

1. NexusMapEntry: for the table entries contained in a <specifier>-map as documented in DTSpec section 2.5.1. This would contain attributes named child_specifier, parent, parent_specifier.
2. InterruptMapEntry: similar, except for the interrupt-map property table entries in DTSpec 2.4.3.

This would match the same pattern we have done with the Range type used to represent elements of the standard ranges property. Experience has shown from that case that picking type names and designing APIs to match the specification leads to future-proof work.

What do you think about this idea?

[mbolivar]

I'm not sure that I think that ControllerAndData

#87595 (review)

cc @dottspina for his perspective as a consumer of this API

[soburi]

@mbolivar

I think that neither of these types of information is really a ControllerAndData due to the additional "child -> parent" information.

I am currently thinking it would make more sense to introduce two new types:

1. `NexusMapEntry`: for the table entries contained in a `<specifier>-map` as documented in DTSpec section 2.5.1. This would contain attributes named `child_specifier`, `parent`, `parent_specifier`.

2. `InterruptMapEntry`: similar, except for the `interrupt-map` property table entries in DTSpec 2.4.3.

This would match the same pattern we have done with the Range type used to represent elements of the standard ranges property. Experience has shown from that case that picking type names and designing APIs to match the specification leads to future-proof work.

What do you think about this idea?

This was possible with the current structure, so I did so, but
as mentioned in the comments, I think it would be clearer to define a new type.

There may be some room for debate as to whether to use NexusMapEntry/InterruptMapEntry or use MapEntry to make the interrupt property optional as needed for.

I'll move forward with it, but I might not have enough time this week.

[mbolivar]

There may be some room for debate as to whether to use NexusMapEntry/InterruptMapEntry or use MapEntry to make the interrupt property optional as needed for.

I agree there's room for debate. We could also use class inheritance to capture the specialization, perhaps. Thanks and looking forward to your next update.

[soburi]

@mbolivar

Update done.

There may be some room for debate as to whether to use NexusMapEntry/InterruptMapEntry or use MapEntry to make the interrupt property optional as needed for.

I decided to use MapEntry because there are no 'behavior' differences between these, so I think no need to split into a class hierarchy.
The difference is whether the length of the ADDRESS information is 0 or not.

I agree there's room for debate. We could also use class inheritance to capture the specialization, perhaps. Thanks and looking forward to your next update.

As a result, I updated macro generation rules and the APIs. It is mainly just a cleanup, but the structure makes it simple and reduces the code.

[rruuaanng]

I noticed that in this file, some macros that require entry_idx include xxx_BY_IDX in their names, while others don't. They take the same parameters, but the suffixes used are not consistent, I think it would be better to make this consistent, e.g.:

#define DT_MAP_ENTRY_CHILD_ADDRESS_LEN(node_id, prop, entry_idx)                                   \
	DT_CAT6(node_id, _P_, prop, _MAP_ENTRY_, entry_idx, _CHILD_ADDRESS_LEN)

#define DT_MAP_HAS_ENTRY_BY_IDX(node_id, prop, entry_idx)                                          \
	IS_ENABLED(DT_CAT6(node_id, _P_, prop, _MAP_ENTRY_, entry_idx, _EXISTS))

And, it seems that when using these macros, you have to specify entry_idx even when it's 0. Therefore, I suggest adding wrapper macros similar to the existing ones, e.g.:

#define DT_REG_SIZE(node_id) DT_REG_SIZE_BY_IDX(node_id, 0)

#define DT_IRQ_HAS_CELL(node_id, cell) DT_IRQ_HAS_CELL_AT_IDX(node_id, 0, cell)

WDYT?

[soburi]

This is done to prevent BY_IDX for map entries and
BY_IDX for lists of child/parent specifiers to appear simultaneously.
IMO, since "two-dimensional arrays" have not existed in devicetree until now,
applying the rules as is would make the code more difficult to read.

[soburi]

And, it seems that when using these macros, you have to specify entry_idx even when it's 0. Therefore, I suggest adding wrapper macros similar to the existing ones, e.g.:

This has also been addressed.

[rruuaanng]

Looks good, let's be patient, we are almost done :)

[sonarqubecloud]

Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[soburi]

@mbolivar

Update done.
Could you take a look, again?