Adds the list of peripherals IDs under firewall bus on STM32MP15.
Signed-off-by: Gatien Chevallier <[email protected]>
---
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
@@ -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 */
--
2.25.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 <[email protected]>
> ---
> 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
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 <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>>> ---
>>>> 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
>
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 <[email protected]>
>>>>> ---
>>>>> 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
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 <[email protected]>
>>>>>> ---
>>>>>> 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
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
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