Re: [RFC PATCH 3/7] dt-bindings: bus: add STM32MP15 ETZPC firewall bus bindings

From: Gatien CHEVALLIER
Date: Mon Jan 16 2023 - 09:21:43 EST

Hello Krzysztof,

On 1/11/23 13:32, Krzysztof Kozlowski wrote:
On 09/01/2023 12:54, Gatien CHEVALLIER wrote:
Then why do you define them in bindings? Use raw numbers. Do you see
anywhere in arm/arm64 bindings for GIC_SPI interrupt numbers?


What would you think of simply removing the comments that state that IDs
are reserved, mimicking the way it is for qcom bindings? Fundamentally,
they are indeed only IDs and could be raw numbers.

If these are IDs then there are no reserved numbers and they are
continuous from 0 to X. Without gaps.

IMO, this makes reading the device tree harder. Because you'd have to
look what the raw number corresponds to.

Sure, but that's not the reason to put numbers to the bindings... You
mix defines with bindings.

To take an example, it has already been done for SCMI clocks and I find
it eases comprehension.

You need to be a bit more specific...

Please see include/dt-bindings/clock/stm32mp1-clks.h, where there are
various clock IDs defined, some of them not contiguous.

These are pretty often added to accommodate space for exposing these
clocks in the future. IOW, these might be IDs just not all are shared
via header. There are such platforms and it is OK.


Errata: for SCMI clocks they are indeed contiguous but not clock IDs.


Anyway, IDs should be placed in bindings. Some mapping of
internal/hardware ports, registers, offsets, values - usually not.

I don't know where exactly your case fits, but when some IDs are
reserved it is a clear sign that these are not IDs (again - IDs start
from 0 and go incrementally by one, without gaps).


I do agree with your statement that IDs should not be reserved.

I think I've missed something to better highlight my point of view: It
would be perfectly fine using numbers that are not described in this
bindings file. It would just not correspond to an ID of a peripheral
described in the SoC reference manual, thus making no sense to use them.
Stating that they are reserved was incorrect, it's just that peripherals
get a firewall ID, depending on the platform.

Why peripheral ID should be put into the bindings? Why bindings is a
place for it? Interrupt numbers, GPIO indices/numbers, register offsets,
IOMMU ports - none of these are suitable for bindings.


I think it should be okay not describing IDs that are not relevant, what
do you think? I found that in include/dt-bindings/arm/qcom,ids.h, IDs
are not continuous. Not mentioning an ID could be used for deprecation.

These are not IDs of clocks. These are unique identifiers assigned by
vendor and used by different pieces: firmware/bootloaders, DTS and Linux
driver. We have no control of them but they exist. They also do not
represent any hardware number.

You bring some examples as an argument, but these examples are not
always related to your case. To be clear - we talk here about bindings,
so they bind different interfaces of software components (e.g. Linux
kernel with DTS). Now, what is the different interface here in your
case? If you say your peripheral hardware ID, then answer is no - this
is not software interface.

I see what you want to avoid,

These bindings are indeed presented as pure helpers here. They are not used by the firewall bus driver on Linux except for the value that they represent, thus your comment.
However, they will be shared between different boot chain components. I do not have an upstreamed example to give but please see that we might use them in OP-TEE:

[1] https://github.com/STMicroelectronics/optee_os/blob/3.16.0-stm32mp/core/include/dt-bindings/soc/stm32mp13-etzpc.h

They could be used and used differently depending on the software component (e.g: lock of secure configuration for a particular peripheral, ...). This change is here for consistency between those.


Best regards,
Krzysztof


Best regards,
Gatien