Re: [PATCH 1/4] dt-bindings: stm32: add clocks and reset binding for stm32mp25 platform

From: Gabriel FERNANDEZ
Date: Wed Nov 08 2023 - 04:14:35 EST

Hi Krzysztof,

Thank you very much for your review.


On 11/7/23 08:27, Krzysztof Kozlowski wrote:
On 06/11/2023 15:18, gabriel.fernandez@xxxxxxxxxxx wrote:
From: Gabriel Fernandez <gabriel.fernandez@xxxxxxxxxxx>

Adds clock and reset binding entries for STM32MP25 SoC family

Signed-off-by: Gabriel Fernandez <gabriel.fernandez@xxxxxxxxxxx>
This was based on some very old tree. Please work on latest release,
maintainer's tree or linux-next. Otherwise we need to point the same
issues we already fixed. This would be quite a waste of time, don't you
think?

To make sure I understood the problem and wouldn't repeat it, I pushed my series starting from the tag 'next-20231031.

Or is it related to the content of the YAML file?



---
.../bindings/clock/st,stm32mp25-rcc.yaml | 116 +++++
include/dt-bindings/clock/stm32mp25-clks.h | 492 ++++++++++++++++++
include/dt-bindings/reset/stm32mp25-resets.h | 167 ++++++
3 files changed, 775 insertions(+)
create mode 100644 Documentation/devicetree/bindings/clock/st,stm32mp25-rcc.yaml
create mode 100644 include/dt-bindings/clock/stm32mp25-clks.h
create mode 100644 include/dt-bindings/reset/stm32mp25-resets.h

diff --git a/Documentation/devicetree/bindings/clock/st,stm32mp25-rcc.yaml b/Documentation/devicetree/bindings/clock/st,stm32mp25-rcc.yaml
new file mode 100644
index 000000000000..27c60f3231ba
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/st,stm32mp25-rcc.yaml
@@ -0,0 +1,116 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/clock/st,stm32mp25-rcc.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: STM32MP25 Reset Clock Controller
+
+maintainers:
+ - Gabriel Fernandez <gabriel.fernandez@xxxxxxxxxxx>
+
+description: |
+ The RCC IP is both a reset and a clock controller.
+ RCC makes also power management (resume/supend and wakeup interrupt).
+ Please also refer to reset.txt for common reset controller binding usage.
Sorry, what TXT?

+
+ This binding uses common clock bindings
+ Documentation/devicetree/bindings/clock/clock-bindings.txt
Please drop all unrelevant, 5 year old links.

+
+ Specifying clocks
+ =================
+
+ All available clocks are defined as preprocessor macros in
+ dt-bindings/clock/stm32mp25-clks.h header and can be used in device
Not even proper path :/

+ tree sources.
+ This file implements defines like:
+ #define CK_BUS_SDMMC1 245
+ #define CK_KER_SDMMC1 313
Open other bindings to see how it is done. We expect full path only.
Drop all this irrelevant parts.

+
+ Specifying softreset control of devices
+ =======================================
+
+ Device nodes should specify the reset channel required in their "resets"
+ property, containing a phandle to the reset device node and an index
+ specifying which channel to use.
Are you now describing how DT and Linux work? Drop.

+ The index is the bit number within the RCC registers bank, starting from RCC
+ base address.
No, it should not be. Use IDs. You will get NAK below anyway.

+ It is calculated as: index = register_offset / 4 * 32 + bit_offset.
+ Where bit_offset is the bit offset within the register.
+
+ For example on STM32MP25, for LTDC reset:
+ ltdc = RCC_LTDCCFGR offset / 4 * 32 + LTDC_bit_offset
+ = 0x840 / 4 * 32 + 0 = 16896
+
+ The list of valid indices for STM32MP25 is available in:
+ include/dt-bindings/reset-controller/stm32mp25-resets.h
+
+ This file implements defines like:
+ #define LTDC_R 16896
? I have no clue what you are saying here.

+
+properties:
+ "#clock-cells":
+ const: 1
+
+ "#reset-cells":
+ const: 1
+
+ compatible:
+ items:
+ - enum:
+ - st,stm32mp25-rcc
Compatible is always first.

+ clocks: true
+ clock-names: true
NAK, missing constraints.

This does not look at all like any decent bindings. Start from scratch
from recently reviewed bindings.

+
+ reg:
+ maxItems: 1
+
+required:
+ - "#clock-cells"
+ - "#reset-cells"
+ - compatible
+ - reg
+
+if:
+ properties:
+ compatible:
+ contains:
+ enum:
+ - st,stm32mp25-rcc
+then:
+ properties:
+ clocks:
+ description: Specifies oscillators.
+ maxItems: 5
+
+ clock-names:
+ items:
+ - const: hse
+ - const: hsi
+ - const: msi
+ - const: lse
+ - const: lsi
+ required:
+ - clocks
+ - clock-names
+
+additionalProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/stm32mp25-clks.h>
+ #include <dt-bindings/reset/stm32mp25-resets.h>
+
+ rcc: rcc@44200000 {
+ compatible = "st,stm32mp25-rcc";
+ reg = <0x44200000 0x10000>;
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ clock-names = "hse", "hsi", "msi", "lse", "lsi";
+ clocks = <&scmi_clk CK_SCMI_HSE>,
+ <&scmi_clk CK_SCMI_HSI>,
+ <&scmi_clk CK_SCMI_MSI>,
+ <&scmi_clk CK_SCMI_LSE>,
+ <&scmi_clk CK_SCMI_LSI>;
+ };
+...
diff --git a/include/dt-bindings/clock/stm32mp25-clks.h b/include/dt-bindings/clock/stm32mp25-clks.h
new file mode 100644
index 000000000000..9876ee0dd1e4
--- /dev/null
+++ b/include/dt-bindings/clock/stm32mp25-clks.h
Same filename as bindings.

@@ -0,0 +1,492 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause */
+/*
+ * Copyright (C) STMicroelectronics 2023 - All Rights Reserved
+ * Author: Gabriel Fernandez <gabriel.fernandez@xxxxxxxxxxx>
+ */
+
+#ifndef _DT_BINDINGS_STM32MP25_CLKS_H_
+#define _DT_BINDINGS_STM32MP25_CLKS_H_
+

...

+#endif /* _DT_BINDINGS_STM32MP25_CLKS_H_ */
diff --git a/include/dt-bindings/reset/stm32mp25-resets.h b/include/dt-bindings/reset/stm32mp25-resets.h
new file mode 100644
index 000000000000..3a4a9eef6a95
--- /dev/null
+++ b/include/dt-bindings/reset/stm32mp25-resets.h
Filename matching compatible format.

Okay, I will fix all the remarks mentioned above


@@ -0,0 +1,167 @@
+/* SPDX-License-Identifier: GPL-2.0-only OR BSD-3-Clause */
+/*
+ * Copyright (C) STMicroelectronics 2023 - All Rights Reserved
+ * Author(s): Gabriel Fernandez <gabriel.fernandez@xxxxxxxxxxx>
+ */
+
+#ifndef _DT_BINDINGS_STM32MP25_RESET_H_
+#define _DT_BINDINGS_STM32MP25_RESET_H_
+
+#define SYS_R 8192
NAK, don't put register values into the bindings. There is no single
need of it. Use IDs (which start from 0 and are incremented by 1) or
drop it.

My STM32MP25 driver is based on the same binding as the STM32MP13, which is already upstreamed last year.

I will update also  the YAML file of the STM32MP13 for the above remarks.

But should I update the binding values of the STM32MP13 and try a solution about backward compatible ?

The idea was to have the same reset driver for all STM32MP platforms

Best regards

Gabriel.



Best regards,
Krzysztof