Re: [PATCH v2 01/11] dt-bindings: clock: Add StarFive JH7110 System-Top-Group clock and reset generator

From: Xingyu Wu
Date: Thu Mar 02 2023 - 22:14:17 EST

Next message: Yangtao Li: "[PATCH] erofs: mark z_erofs_lzma_init/erofs_pcpubuf_init w/ __init"
Previous message: Joel Fernandes: "Re: [PATCH] kheaders: Use array declaration instead of char"
In reply to: Emil Renner Berthing: "Re: [PATCH v2 01/11] dt-bindings: clock: Add StarFive JH7110 System-Top-Group clock and reset generator"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 2023/3/2 23:05, Emil Renner Berthing wrote:
> On Tue, 21 Feb 2023 at 09:37, Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx> wrote:
>> Add bindings for the System-Top-Group clock and reset generator (STGCRG)
>> on the JH7110 RISC-V SoC by StarFive Ltd.
>>
>> Signed-off-by: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
>> ---
>> .../clock/starfive,jh7110-stgcrg.yaml | 82 +++++++++++++++++++
>> MAINTAINERS | 1 +
>> .../dt-bindings/clock/starfive,jh7110-crg.h | 34 ++++++++
>> .../dt-bindings/reset/starfive,jh7110-crg.h | 28 +++++++
>> 4 files changed, 145 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/clock/starfive,jh7110-stgcrg.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/clock/starfive,jh7110-stgcrg.yaml b/Documentation/devicetree/bindings/clock/starfive,jh7110-stgcrg.yaml
>> new file mode 100644
>> index 000000000000..b64ccd84200a
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/clock/starfive,jh7110-stgcrg.yaml
>> @@ -0,0 +1,82 @@
>> +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/clock/starfive,jh7110-stgcrg.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: StarFive JH7110 System-Top-Group Clock and Reset Generator
>> +
>> +maintainers:
>> + - Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
>> +
>> +properties:
>> + compatible:
>> + const: starfive,jh7110-stgcrg
>> +
>> + reg:
>> + maxItems: 1
>> +
>> + clocks:
>> + items:
>> + - description: Main Oscillator (24 MHz)
>> + - description: HIFI4 core
>> + - description: STG AXI/AHB
>> + - description: USB (125 MHz)
>> + - description: CPU Bus
>> + - description: HIFI4 Axi
>> + - description: NOC STG Bus
>> + - description: APB Bus
>> +
>> + clock-names:
>> + items:
>> + - const: osc
>> + - const: hifi4_core
>> + - const: stg_axiahb
>> + - const: usb_125m
>> + - const: cpu_bus
>> + - const: hifi4_axi
>> + - const: nocstg_bus
>> + - const: apb_bus
>> +
>> + '#clock-cells':
>> + const: 1
>> + description:
>> + See <dt-bindings/clock/starfive,jh7110-crg.h> for valid indices.
>> +
>> + '#reset-cells':
>> + const: 1
>> + description:
>> + See <dt-bindings/reset/starfive,jh7110-crg.h> for valid indices.
>> +
>> +required:
>> + - compatible
>> + - reg
>> + - clocks
>> + - clock-names
>> + - '#clock-cells'
>> + - '#reset-cells'
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> + - |
>> + #include <dt-bindings/clock/starfive,jh7110-crg.h>
>> +
>> + stgcrg: clock-controller@10230000 {
>> + compatible = "starfive,jh7110-stgcrg";
>> + reg = <0x10230000 0x10000>;
>> + clocks = <&osc>,
>> + <&syscrg JH7110_SYSCLK_HIFI4_CORE>,
>> + <&syscrg JH7110_SYSCLK_STG_AXIAHB>,
>> + <&syscrg JH7110_SYSCLK_USB_125M>,
>> + <&syscrg JH7110_SYSCLK_CPU_BUS>,
>> + <&syscrg JH7110_SYSCLK_HIFI4_AXI>,
>> + <&syscrg JH7110_SYSCLK_NOCSTG_BUS>,
>> + <&syscrg JH7110_SYSCLK_APB_BUS>;
>> + clock-names = "osc", "hifi4_core",
>> + "stg_axiahb", "usb_125m",
>> + "cpu_bus", "hifi4_axi",
>> + "nocstg_bus", "apb_bus";
>> + #clock-cells = <1>;
>> + #reset-cells = <1>;
>> + };
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 93eb504c3b21..2e70c9f21989 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -19914,6 +19914,7 @@ F: arch/riscv/boot/dts/starfive/
>> STARFIVE JH71X0 CLOCK DRIVERS
>> M: Emil Renner Berthing <kernel@xxxxxxxx>
>> M: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
>> +M: Xingyu Wu <xingyu.wu@xxxxxxxxxxxxxxxx>
>> S: Maintained
>> F: Documentation/devicetree/bindings/clock/starfive,jh71*.yaml
>> F: drivers/clk/starfive/clk-starfive-jh71*
>> diff --git a/include/dt-bindings/clock/starfive,jh7110-crg.h b/include/dt-bindings/clock/starfive,jh7110-crg.h
>> index 5e4f21ca0642..5ac8a4d90a7a 100644
>> --- a/include/dt-bindings/clock/starfive,jh7110-crg.h
>> +++ b/include/dt-bindings/clock/starfive,jh7110-crg.h
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: GPL-2.0 OR MIT */
>> /*
>> * Copyright 2022 Emil Renner Berthing <kernel@xxxxxxxx>
>> + * Copyright 2022 StarFive Technology Co., Ltd.
>> */
>>
>> #ifndef __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__
>> @@ -222,4 +223,37 @@
>>
>> #define JH7110_AONCLK_END 14
>
> Hi Xingyu,
>
> The clock and reset names below have been shortened from the very long
> names in the documentation. I see you've come to the same shortened
> names as I used in the first STGCRG driver I pushed, which is great,
> but I find it highly unlikely to have happened without looking at /
> copying my code like you did for the SYSCRG and AONCRG drivers Hal has
> posted. Unfortunately the commit message above doesn't reflect that,
> so please add a
> Co-developed-by: Emil Renner Berthing <kernel@xxxxxxxx>
> Signed-off-by: Emil Renner Berthing <kernel@xxxxxxxx>

Thanks. But these STG/ISP/VOUT drivers are transplanted from 7110-SDK wrote
by myself one year ago and followed 7100 clock drivers framework and structure
at that time. And I haven't seen your STGCRG driver before. I improved these
to follow these SYSCRG and AONCRG drivers' framework. So you could look like copying
your code but these are still little different like these clock and reset name.

But I don't know if I am right. I follow your code framework to write it and
I should add 'Co-developed-by' and 'Signed-off-by' to reflect that. It that right?

>
> I do have some updated suggestions for short names below though:
>
>> +/* STGCRG clocks */
>> +#define JH7110_STGCLK_HIFI4_CLK_CORE 0
>> +#define JH7110_STGCLK_USB0_APB 1
>> +#define JH7110_STGCLK_USB0_UTMI_APB 2 unli
>> +#define JH7110_STGCLK_USB0_AXI 3
>> +#define JH7110_STGCLK_USB0_LPM 4
>> +#define JH7110_STGCLK_USB0_STB 5
>> +#define JH7110_STGCLK_USB0_APP_125 6
>> +#define JH7110_STGCLK_USB0_REFCLK 7
>> +#define JH7110_STGCLK_PCIE0_AXI_MST0 8
>> +#define JH7110_STGCLK_PCIE0_APB 9
>> +#define JH7110_STGCLK_PCIE0_TL 10
>> +#define JH7110_STGCLK_PCIE1_AXI_MST0 11
>> +#define JH7110_STGCLK_PCIE1_APB 12
>> +#define JH7110_STGCLK_PCIE1_TL 13
>> +#define JH7110_STGCLK_PCIE01_SLV_DEC_MAINCLK 14
>
> Does PCIE01 here mean that the clock is used by both pcie0 and pcie1?
> If so then maybe just call it JH7110_PCIE_SLV_MAIN

Yes, it is used by both pcie0 and pcie1. Will modify it.

>
>> +#define JH7110_STGCLK_SEC_HCLK 15
>
> For other clocks I think "hclk" means ahb clock, so maybe JH7110_STGCLK_SEC_AHB

Will modify it.

>
>> +#define JH7110_STGCLK_SEC_MISCAHB 16
>
> I find something like JH7110_STGCLK_SEC_MISC_AHB a little easier to read.

Will modify it.

>
>> +#define JH7110_STGCLK_GRP0_MAIN 17
>> +#define JH7110_STGCLK_GRP0_BUS 18
>> +#define JH7110_STGCLK_GRP0_STG 19
>> +#define JH7110_STGCLK_GRP1_MAIN 20
>> +#define JH7110_STGCLK_GRP1_BUS 21
>> +#define JH7110_STGCLK_GRP1_STG 22
>> +#define JH7110_STGCLK_GRP1_HIFI 23
>> +#define JH7110_STGCLK_E2_RTC 24
>> +#define JH7110_STGCLK_E2_CORE 25
>> +#define JH7110_STGCLK_E2_DBG 26
>> +#define JH7110_STGCLK_DMA1P_AXI 27
>> +#define JH7110_STGCLK_DMA1P_AHB 28
>> +
>> +#define JH7110_STGCLK_END 29
>> +
>> #endif /* __DT_BINDINGS_CLOCK_STARFIVE_JH7110_CRG_H__ */
>> diff --git a/include/dt-bindings/reset/starfive,jh7110-crg.h b/include/dt-bindings/reset/starfive,jh7110-crg.h
>> index d78e38690ceb..4a865ded78b8 100644
>> --- a/include/dt-bindings/reset/starfive,jh7110-crg.h
>> +++ b/include/dt-bindings/reset/starfive,jh7110-crg.h
>> @@ -1,6 +1,7 @@
>> /* SPDX-License-Identifier: GPL-2.0 OR MIT */
>> /*
>> * Copyright (C) 2022 Emil Renner Berthing <kernel@xxxxxxxx>
>> + * Copyright (C) 2022 StarFive Technology Co., Ltd.
>> */
>>
>> #ifndef __DT_BINDINGS_RESET_STARFIVE_JH7110_CRG_H__
>> @@ -151,4 +152,31 @@
>>
>> #define JH7110_AONRST_END 8
>>
>> +/* STGCRG resets */
>> +#define JH7110_STGRST_SYSCON 0
>> +#define JH7110_STGRST_HIFI4_CORE 1
>> +#define JH7110_STGRST_HIFI4_AXI 2
>> +#define JH7110_STGRST_SEC_TOP_HRESETN 3
>
> JH7110_STGRST_SEC_AHB to match the clock above.

Will modify it.

>
>> +#define JH7110_STGRST_E24_CORE 4
>> +#define JH7110_STGRST_DMA1P_AXI 5
>> +#define JH7110_STGRST_DMA1P_AHB 6
>> +#define JH7110_STGRST_USB0_AXI 7
>> +#define JH7110_STGRST_USB0_APB 8
>> +#define JH7110_STGRST_USB0_UTMI_APB 9
>> +#define JH7110_STGRST_USB0_PWRUP 10
>> +#define JH7110_STGRST_PCIE0_AXI_MST0 11
>> +#define JH7110_STGRST_PCIE0_AXI_SLV0 12
>> +#define JH7110_STGRST_PCIE0_AXI_SLV 13
>> +#define JH7110_STGRST_PCIE0_BRG 14
>> +#define JH7110_STGRST_PCIE0_CORE 15
>> +#define JH7110_STGRST_PCIE0_APB 16
>> +#define JH7110_STGRST_PCIE1_AXI_MST0 17
>> +#define JH7110_STGRST_PCIE1_AXI_SLV0 18
>> +#define JH7110_STGRST_PCIE1_AXI_SLV 19
>> +#define JH7110_STGRST_PCIE1_BRG 20
>> +#define JH7110_STGRST_PCIE1_CORE 21
>> +#define JH7110_STGRST_PCIE1_APB 22
>> +
>> +#define JH7110_STGRST_END 23
>> +
>> #endif /* __DT_BINDINGS_RESET_STARFIVE_JH7110_CRG_H__ */
>> --
>> 2.25.1
>>
>>
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@xxxxxxxxxxxxxxxxxxx
>> http://lists.infradead.org/mailman/listinfo/linux-riscv

Best regards

Next message: Yangtao Li: "[PATCH] erofs: mark z_erofs_lzma_init/erofs_pcpubuf_init w/ __init"
Previous message: Joel Fernandes: "Re: [PATCH] kheaders: Use array declaration instead of char"
In reply to: Emil Renner Berthing: "Re: [PATCH v2 01/11] dt-bindings: clock: Add StarFive JH7110 System-Top-Group clock and reset generator"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]