Messages in this thread | | | Date | Wed, 8 Nov 2023 10:13:54 +0100 | Subject | Re: [PATCH 1/4] dt-bindings: stm32: add clocks and reset binding for stm32mp25 platform | From | Gabriel FERNANDEZ <> |
| |
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@foss.st.com wrote: >> From: Gabriel Fernandez <gabriel.fernandez@foss.st.com> >> >> Adds clock and reset binding entries for STM32MP25 SoC family >> >> Signed-off-by: Gabriel Fernandez <gabriel.fernandez@foss.st.com> > 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@foss.st.com> >> + >> +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@foss.st.com> >> + */ >> + >> +#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@foss.st.com> >> + */ >> + >> +#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 >
| |