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

From: Gatien CHEVALLIER <gatien.chevallier@foss.st.com>
To: Krzysztof Kozlowski <krzk@kernel.org>,
	<alexandre.torgue@foss.st.com>, <robh+dt@kernel.org>,
	<Oleksii_Moisieiev@epam.com>, <linus.walleij@linaro.org>,
	<gregkh@linuxfoundation.org>
Cc: <linux-stm32@st-md-mailman.stormreply.com>,
	<linux-arm-kernel@lists.infradead.org>,
	<linux-kernel@vger.kernel.org>, <loic.pallardy@st.com>,
	<devicetree@vger.kernel.org>, <mark.rutland@arm.com>,
	<arnd@arndb.de>
Subject: Re: [RFC PATCH 3/7] dt-bindings: bus: add STM32MP15 ETZPC firewall bus bindings
Date: Mon, 16 Jan 2023 15:06:06 +0100	[thread overview]
Message-ID: <b90e4b78-d84d-3a63-1aef-e7214d4b29d9@foss.st.com> (raw)
In-Reply-To: <f169d05a-7a07-aedf-bad2-30cb4a88fc16@kernel.org>

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel