diff mbox series

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

Message ID 20221221173055.11719-4-gatien.chevallier@foss.st.com (mailing list archive)
State New, archived
Headers show
Series Introduce STM32 system bus | expand

Commit Message

Gatien CHEVALLIER Dec. 21, 2022, 5:30 p.m. UTC
Adds the list of peripherals IDs under firewall bus on STM32MP15.

Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
---
 include/dt-bindings/bus/stm32mp15_sys_bus.h | 98 +++++++++++++++++++++
 1 file changed, 98 insertions(+)
 create mode 100644 include/dt-bindings/bus/stm32mp15_sys_bus.h

Comments

Krzysztof Kozlowski Dec. 22, 2022, 10:26 a.m. UTC | #1
On 21/12/2022 18:30, Gatien Chevallier wrote:
> Adds the list of peripherals IDs under firewall bus on STM32MP15.
> 
> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
> ---
>  include/dt-bindings/bus/stm32mp15_sys_bus.h | 98 +++++++++++++++++++++
>  1 file changed, 98 insertions(+)
>  create mode 100644 include/dt-bindings/bus/stm32mp15_sys_bus.h
> 
> diff --git a/include/dt-bindings/bus/stm32mp15_sys_bus.h b/include/dt-bindings/bus/stm32mp15_sys_bus.h
> new file mode 100644
> index 000000000000..97eacc7b5f16
> --- /dev/null
> +++ b/include/dt-bindings/bus/stm32mp15_sys_bus.h

That's wrong in multiple ways:
1. No underscores
2. Missing vendor prefix
3. Name not matching compatible.

> @@ -0,0 +1,98 @@
> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
> +/*
> + * Copyright (C) STMicroelectronics 2022 - All Rights Reserved
> + */
> +#ifndef _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
> +#define _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
> +
> +/* ETZPC IDs */
> +#define STM32MP1_ETZPC_STGENC_ID	0
> +#define STM32MP1_ETZPC_BKPSRAM_ID	1
> +#define STM32MP1_ETZPC_IWDG1_ID		2
> +#define STM32MP1_ETZPC_USART1_ID	3
> +#define STM32MP1_ETZPC_SPI6_ID		4
> +#define STM32MP1_ETZPC_I2C4_ID		5
> +/* ID 6 reserved */

Reserved why? These are IDs so they start from 0 and go by 0. Don't
hard-code some register offsets.

Best regards,
Krzysztof
Gatien CHEVALLIER Dec. 22, 2022, 1:51 p.m. UTC | #2
Hello,

On 12/22/22 11:26, Krzysztof Kozlowski wrote:
> On 21/12/2022 18:30, Gatien Chevallier wrote:
>> Adds the list of peripherals IDs under firewall bus on STM32MP15.
>>
>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>> ---
>>   include/dt-bindings/bus/stm32mp15_sys_bus.h | 98 +++++++++++++++++++++
>>   1 file changed, 98 insertions(+)
>>   create mode 100644 include/dt-bindings/bus/stm32mp15_sys_bus.h
>>
>> diff --git a/include/dt-bindings/bus/stm32mp15_sys_bus.h b/include/dt-bindings/bus/stm32mp15_sys_bus.h
>> new file mode 100644
>> index 000000000000..97eacc7b5f16
>> --- /dev/null
>> +++ b/include/dt-bindings/bus/stm32mp15_sys_bus.h
> 
> That's wrong in multiple ways:
> 1. No underscores
> 2. Missing vendor prefix
> 3. Name not matching compatible.

Sure, will comply in V3.

> 
>> @@ -0,0 +1,98 @@
>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>> +/*
>> + * Copyright (C) STMicroelectronics 2022 - All Rights Reserved
>> + */
>> +#ifndef _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>> +#define _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>> +
>> +/* ETZPC IDs */
>> +#define STM32MP1_ETZPC_STGENC_ID	0
>> +#define STM32MP1_ETZPC_BKPSRAM_ID	1
>> +#define STM32MP1_ETZPC_IWDG1_ID		2
>> +#define STM32MP1_ETZPC_USART1_ID	3
>> +#define STM32MP1_ETZPC_SPI6_ID		4
>> +#define STM32MP1_ETZPC_I2C4_ID		5
>> +/* ID 6 reserved */
> 
> Reserved why? These are IDs so they start from 0 and go by 0. Don't
> hard-code some register offsets.

Here, I do define IDs. Some appear as reserved based on what I've seen 
in the SoC datasheet that states these as "indexes"

Please see the table 94 in chapter 15.6 (ETZPC) of the STM32MP157 
Reference manual:
[1] https://www.st.com/resource/en/reference_manual/DM00327659-.pdf

> 
> Best regards,
> Krzysztof
> 

Best regards,
Gatien
Krzysztof Kozlowski Dec. 22, 2022, 1:57 p.m. UTC | #3
On 22/12/2022 14:51, Gatien CHEVALLIER wrote:
> Hello,
> 
> On 12/22/22 11:26, Krzysztof Kozlowski wrote:
>> On 21/12/2022 18:30, Gatien Chevallier wrote:
>>> Adds the list of peripherals IDs under firewall bus on STM32MP15.
>>>
>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>> ---
>>>   include/dt-bindings/bus/stm32mp15_sys_bus.h | 98 +++++++++++++++++++++
>>>   1 file changed, 98 insertions(+)
>>>   create mode 100644 include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>
>>> diff --git a/include/dt-bindings/bus/stm32mp15_sys_bus.h b/include/dt-bindings/bus/stm32mp15_sys_bus.h
>>> new file mode 100644
>>> index 000000000000..97eacc7b5f16
>>> --- /dev/null
>>> +++ b/include/dt-bindings/bus/stm32mp15_sys_bus.h
>>
>> That's wrong in multiple ways:
>> 1. No underscores
>> 2. Missing vendor prefix
>> 3. Name not matching compatible.
> 
> Sure, will comply in V3.
> 
>>
>>> @@ -0,0 +1,98 @@
>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>> +/*
>>> + * Copyright (C) STMicroelectronics 2022 - All Rights Reserved
>>> + */
>>> +#ifndef _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>>> +#define _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>>> +
>>> +/* ETZPC IDs */
>>> +#define STM32MP1_ETZPC_STGENC_ID	0
>>> +#define STM32MP1_ETZPC_BKPSRAM_ID	1
>>> +#define STM32MP1_ETZPC_IWDG1_ID		2
>>> +#define STM32MP1_ETZPC_USART1_ID	3
>>> +#define STM32MP1_ETZPC_SPI6_ID		4
>>> +#define STM32MP1_ETZPC_I2C4_ID		5
>>> +/* ID 6 reserved */
>>
>> Reserved why? These are IDs so they start from 0 and go by 0. Don't
>> hard-code some register offsets.
> 
> Here, I do define IDs. Some appear as reserved based on what I've seen 
> in the SoC datasheet that states these as "indexes"
> 
> Please see the table 94 in chapter 15.6 (ETZPC) of the STM32MP157 
> Reference manual:
> [1] https://www.st.com/resource/en/reference_manual/DM00327659-.pdf

Then why do you define them in bindings? Use raw numbers. Do you see
anywhere in arm/arm64 bindings for GIC_SPI interrupt numbers?

Best regards,
Krzysztof
Gatien CHEVALLIER Jan. 4, 2023, 1:43 p.m. UTC | #4
Hello Krzysztof,

On 12/22/22 14:57, Krzysztof Kozlowski wrote:
> On 22/12/2022 14:51, Gatien CHEVALLIER wrote:
>> Hello,
>>
>> On 12/22/22 11:26, Krzysztof Kozlowski wrote:
>>> On 21/12/2022 18:30, Gatien Chevallier wrote:
>>>> Adds the list of peripherals IDs under firewall bus on STM32MP15.
>>>>
>>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>>> ---
>>>>    include/dt-bindings/bus/stm32mp15_sys_bus.h | 98 +++++++++++++++++++++
>>>>    1 file changed, 98 insertions(+)
>>>>    create mode 100644 include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>>
>>>> diff --git a/include/dt-bindings/bus/stm32mp15_sys_bus.h b/include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>> new file mode 100644
>>>> index 000000000000..97eacc7b5f16
>>>> --- /dev/null
>>>> +++ b/include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>
>>> That's wrong in multiple ways:
>>> 1. No underscores
>>> 2. Missing vendor prefix
>>> 3. Name not matching compatible.
>>
>> Sure, will comply in V3.
>>
>>>
>>>> @@ -0,0 +1,98 @@
>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>> +/*
>>>> + * Copyright (C) STMicroelectronics 2022 - All Rights Reserved
>>>> + */
>>>> +#ifndef _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>>>> +#define _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>>>> +
>>>> +/* ETZPC IDs */
>>>> +#define STM32MP1_ETZPC_STGENC_ID	0
>>>> +#define STM32MP1_ETZPC_BKPSRAM_ID	1
>>>> +#define STM32MP1_ETZPC_IWDG1_ID		2
>>>> +#define STM32MP1_ETZPC_USART1_ID	3
>>>> +#define STM32MP1_ETZPC_SPI6_ID		4
>>>> +#define STM32MP1_ETZPC_I2C4_ID		5
>>>> +/* ID 6 reserved */
>>>
>>> Reserved why? These are IDs so they start from 0 and go by 0. Don't
>>> hard-code some register offsets.
>>
>> Here, I do define IDs. Some appear as reserved based on what I've seen
>> in the SoC datasheet that states these as "indexes"
>>
>> Please see the table 94 in chapter 15.6 (ETZPC) of the STM32MP157
>> Reference manual:
>> [1] https://www.st.com/resource/en/reference_manual/DM00327659-.pdf
> 
> 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.

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

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

Best regards,
Gatien

> Best regards,
> Krzysztof
>
Krzysztof Kozlowski Jan. 5, 2023, 9:53 p.m. UTC | #5
On 04/01/2023 14:43, Gatien CHEVALLIER wrote:
> Hello Krzysztof,
> 
> On 12/22/22 14:57, Krzysztof Kozlowski wrote:
>> On 22/12/2022 14:51, Gatien CHEVALLIER wrote:
>>> Hello,
>>>
>>> On 12/22/22 11:26, Krzysztof Kozlowski wrote:
>>>> On 21/12/2022 18:30, Gatien Chevallier wrote:
>>>>> Adds the list of peripherals IDs under firewall bus on STM32MP15.
>>>>>
>>>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>>>> ---
>>>>>    include/dt-bindings/bus/stm32mp15_sys_bus.h | 98 +++++++++++++++++++++
>>>>>    1 file changed, 98 insertions(+)
>>>>>    create mode 100644 include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>>>
>>>>> diff --git a/include/dt-bindings/bus/stm32mp15_sys_bus.h b/include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>>> new file mode 100644
>>>>> index 000000000000..97eacc7b5f16
>>>>> --- /dev/null
>>>>> +++ b/include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>>
>>>> That's wrong in multiple ways:
>>>> 1. No underscores
>>>> 2. Missing vendor prefix
>>>> 3. Name not matching compatible.
>>>
>>> Sure, will comply in V3.
>>>
>>>>
>>>>> @@ -0,0 +1,98 @@
>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>>> +/*
>>>>> + * Copyright (C) STMicroelectronics 2022 - All Rights Reserved
>>>>> + */
>>>>> +#ifndef _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>>>>> +#define _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>>>>> +
>>>>> +/* ETZPC IDs */
>>>>> +#define STM32MP1_ETZPC_STGENC_ID	0
>>>>> +#define STM32MP1_ETZPC_BKPSRAM_ID	1
>>>>> +#define STM32MP1_ETZPC_IWDG1_ID		2
>>>>> +#define STM32MP1_ETZPC_USART1_ID	3
>>>>> +#define STM32MP1_ETZPC_SPI6_ID		4
>>>>> +#define STM32MP1_ETZPC_I2C4_ID		5
>>>>> +/* ID 6 reserved */
>>>>
>>>> Reserved why? These are IDs so they start from 0 and go by 0. Don't
>>>> hard-code some register offsets.
>>>
>>> Here, I do define IDs. Some appear as reserved based on what I've seen
>>> in the SoC datasheet that states these as "indexes"
>>>
>>> Please see the table 94 in chapter 15.6 (ETZPC) of the STM32MP157
>>> Reference manual:
>>> [1] https://www.st.com/resource/en/reference_manual/DM00327659-.pdf
>>
>> 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...

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).


Best regards,
Krzysztof
Gatien CHEVALLIER Jan. 9, 2023, 11:54 a.m. UTC | #6
On 1/5/23 22:53, Krzysztof Kozlowski wrote:
> On 04/01/2023 14:43, Gatien CHEVALLIER wrote:
>> Hello Krzysztof,
>>
>> On 12/22/22 14:57, Krzysztof Kozlowski wrote:
>>> On 22/12/2022 14:51, Gatien CHEVALLIER wrote:
>>>> Hello,
>>>>
>>>> On 12/22/22 11:26, Krzysztof Kozlowski wrote:
>>>>> On 21/12/2022 18:30, Gatien Chevallier wrote:
>>>>>> Adds the list of peripherals IDs under firewall bus on STM32MP15.
>>>>>>
>>>>>> Signed-off-by: Gatien Chevallier <gatien.chevallier@foss.st.com>
>>>>>> ---
>>>>>>     include/dt-bindings/bus/stm32mp15_sys_bus.h | 98 +++++++++++++++++++++
>>>>>>     1 file changed, 98 insertions(+)
>>>>>>     create mode 100644 include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>>>>
>>>>>> diff --git a/include/dt-bindings/bus/stm32mp15_sys_bus.h b/include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>>>> new file mode 100644
>>>>>> index 000000000000..97eacc7b5f16
>>>>>> --- /dev/null
>>>>>> +++ b/include/dt-bindings/bus/stm32mp15_sys_bus.h
>>>>>
>>>>> That's wrong in multiple ways:
>>>>> 1. No underscores
>>>>> 2. Missing vendor prefix
>>>>> 3. Name not matching compatible.
>>>>
>>>> Sure, will comply in V3.
>>>>
>>>>>
>>>>>> @@ -0,0 +1,98 @@
>>>>>> +/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
>>>>>> +/*
>>>>>> + * Copyright (C) STMicroelectronics 2022 - All Rights Reserved
>>>>>> + */
>>>>>> +#ifndef _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>>>>>> +#define _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
>>>>>> +
>>>>>> +/* ETZPC IDs */
>>>>>> +#define STM32MP1_ETZPC_STGENC_ID	0
>>>>>> +#define STM32MP1_ETZPC_BKPSRAM_ID	1
>>>>>> +#define STM32MP1_ETZPC_IWDG1_ID		2
>>>>>> +#define STM32MP1_ETZPC_USART1_ID	3
>>>>>> +#define STM32MP1_ETZPC_SPI6_ID		4
>>>>>> +#define STM32MP1_ETZPC_I2C4_ID		5
>>>>>> +/* ID 6 reserved */
>>>>>
>>>>> Reserved why? These are IDs so they start from 0 and go by 0. Don't
>>>>> hard-code some register offsets.
>>>>
>>>> Here, I do define IDs. Some appear as reserved based on what I've seen
>>>> in the SoC datasheet that states these as "indexes"
>>>>
>>>> Please see the table 94 in chapter 15.6 (ETZPC) of the STM32MP157
>>>> Reference manual:
>>>> [1] https://www.st.com/resource/en/reference_manual/DM00327659-.pdf
>>>
>>> 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.

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.

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.

> 
> Best regards,
> Krzysztof
> 

Best regards,
Gatien
Krzysztof Kozlowski Jan. 11, 2023, 12:32 p.m. UTC | #7
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.

Best regards,
Krzysztof
Gatien CHEVALLIER Jan. 16, 2023, 2:06 p.m. UTC | #8
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
diff mbox series

Patch

diff --git a/include/dt-bindings/bus/stm32mp15_sys_bus.h b/include/dt-bindings/bus/stm32mp15_sys_bus.h
new file mode 100644
index 000000000000..97eacc7b5f16
--- /dev/null
+++ b/include/dt-bindings/bus/stm32mp15_sys_bus.h
@@ -0,0 +1,98 @@ 
+/* SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) */
+/*
+ * Copyright (C) STMicroelectronics 2022 - All Rights Reserved
+ */
+#ifndef _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
+#define _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H
+
+/* ETZPC IDs */
+#define STM32MP1_ETZPC_STGENC_ID	0
+#define STM32MP1_ETZPC_BKPSRAM_ID	1
+#define STM32MP1_ETZPC_IWDG1_ID		2
+#define STM32MP1_ETZPC_USART1_ID	3
+#define STM32MP1_ETZPC_SPI6_ID		4
+#define STM32MP1_ETZPC_I2C4_ID		5
+/* ID 6 reserved */
+#define STM32MP1_ETZPC_RNG1_ID		7
+#define STM32MP1_ETZPC_HASH1_ID		8
+#define STM32MP1_ETZPC_CRYP1_ID		9
+#define STM32MP1_ETZPC_DDRCTRL_ID	10
+#define STM32MP1_ETZPC_DDRPHYC_ID	11
+#define STM32MP1_ETZPC_I2C6_ID		12
+/* IDs 13-15 reserved */
+#define STM32MP1_ETZPC_TIM2_ID		16
+#define STM32MP1_ETZPC_TIM3_ID		17
+#define STM32MP1_ETZPC_TIM4_ID		18
+#define STM32MP1_ETZPC_TIM5_ID		19
+#define STM32MP1_ETZPC_TIM6_ID		20
+#define STM32MP1_ETZPC_TIM7_ID		21
+#define STM32MP1_ETZPC_TIM12_ID		22
+#define STM32MP1_ETZPC_TIM13_ID		23
+#define STM32MP1_ETZPC_TIM14_ID		24
+#define STM32MP1_ETZPC_LPTIM1_ID	25
+#define STM32MP1_ETZPC_WWDG1_ID		26
+#define STM32MP1_ETZPC_SPI2_ID		27
+#define STM32MP1_ETZPC_SPI3_ID		28
+#define STM32MP1_ETZPC_SPDIFRX_ID	29
+#define STM32MP1_ETZPC_USART2_ID	30
+#define STM32MP1_ETZPC_USART3_ID	31
+#define STM32MP1_ETZPC_UART4_ID		32
+#define STM32MP1_ETZPC_UART5_ID		33
+#define STM32MP1_ETZPC_I2C1_ID		34
+#define STM32MP1_ETZPC_I2C2_ID		35
+#define STM32MP1_ETZPC_I2C3_ID		36
+#define STM32MP1_ETZPC_I2C5_ID		37
+#define STM32MP1_ETZPC_CEC_ID		38
+#define STM32MP1_ETZPC_DAC_ID		39
+#define STM32MP1_ETZPC_UART7_ID		40
+#define STM32MP1_ETZPC_UART8_ID		41
+/* IDs 42-43 reserved */
+#define STM32MP1_ETZPC_MDIOS_ID		44
+/* IDs 45-47 reserved */
+#define STM32MP1_ETZPC_TIM1_ID		48
+#define STM32MP1_ETZPC_TIM8_ID		49
+/* ID 50 reserved */
+#define STM32MP1_ETZPC_USART6_ID	51
+#define STM32MP1_ETZPC_SPI1_ID		52
+#define STM32MP1_ETZPC_SPI4_ID		53
+#define STM32MP1_ETZPC_TIM15_ID		54
+#define STM32MP1_ETZPC_TIM16_ID		55
+#define STM32MP1_ETZPC_TIM17_ID		56
+#define STM32MP1_ETZPC_SPI5_ID		57
+#define STM32MP1_ETZPC_SAI1_ID		58
+#define STM32MP1_ETZPC_SAI2_ID		59
+#define STM32MP1_ETZPC_SAI3_ID		60
+#define STM32MP1_ETZPC_DFSDM_ID		61
+#define STM32MP1_ETZPC_TT_FDCAN_ID	62
+/* IDs 63 reserved */
+#define STM32MP1_ETZPC_LPTIM2_ID	64
+#define STM32MP1_ETZPC_LPTIM3_ID	65
+#define STM32MP1_ETZPC_LPTIM4_ID	66
+#define STM32MP1_ETZPC_LPTIM5_ID	67
+#define STM32MP1_ETZPC_SAI4_ID		68
+#define STM32MP1_ETZPC_VREFBUF_ID	69
+#define STM32MP1_ETZPC_DCMI_ID		70
+#define STM32MP1_ETZPC_CRC2_ID		71
+#define STM32MP1_ETZPC_ADC_ID		72
+#define STM32MP1_ETZPC_HASH2_ID		73
+#define STM32MP1_ETZPC_RNG2_ID		74
+#define STM32MP1_ETZPC_CRYP2_ID		75
+/* IDs 76-79 reserved */
+#define STM32MP1_ETZPC_SRAM1_ID		80
+#define STM32MP1_ETZPC_SRAM2_ID		81
+#define STM32MP1_ETZPC_SRAM3_ID		82
+#define STM32MP1_ETZPC_SRAM4_ID		83
+#define STM32MP1_ETZPC_RETRAM_ID	84
+#define STM32MP1_ETZPC_OTG_ID		85
+#define STM32MP1_ETZPC_SDMMC3_ID	86
+#define STM32MP1_ETZPC_DLYBSD3_ID	87
+#define STM32MP1_ETZPC_DMA1_ID		88
+#define STM32MP1_ETZPC_DMA2_ID		89
+#define STM32MP1_ETZPC_DMAMUX_ID	90
+#define STM32MP1_ETZPC_FMC_ID		91
+#define STM32MP1_ETZPC_QSPI_ID		92
+#define STM32MP1_ETZPC_DLYBQ_ID		93
+#define STM32MP1_ETZPC_ETH_ID		94
+/* ID 95 reserved */
+
+#endif /* _DT_BINDINGS_BUS_STM32MP15_SYS_BUS_H */