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

[Gabriel FERNANDEZ <gabriel.fernandez@foss.st.com>]

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
>