Re: [RFC PATCH 3/7] dt-bindings: bus: add STM32MP15 ETZPC firewall bus bindings
From: Gatien CHEVALLIER
Date: Mon Jan 09 2023 - 06:55:32 EST
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@xxxxxxxxxxx>
---
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