[v7,0/4] riscv: sophgo: add clock support for sg2042

  From: Chen Wang <unicorn_wang@outlook.com>

This series adds clock controller support for sophgo sg2042.

Thanks,
Chen

---

Changes in v7:
  The patch series is based on v6.7. You can simply review or test the
  patches at the link [8].
  - fixed initval issue.
  - fixed pll clk crash issue.
  - fixed warning reported by <lkp@intel.com>
  - code optimization as per review comments.
  - code cleanup and style improvements as per review comments and checkpatch
    with "--strict"

Changes in v6:
  The patch series is based on v6.7-rc1. You can simply review or test the
  patches at the link [7].
  - fixed some warnings/errors reported by kernel test robot <lkp@intel.com>.

Changes in v5:
  The patch series is based on v6.7-rc1. You can simply review or test the
  patches at the link [6].
  - dt-bindings: improved yaml, such as:
    - add vendor prefix for system-ctrl property for clock generator.
    - Add explanation for system-ctrl property.
  - move sophgo,sg2042-clkgen.yaml to directly under clock folder.
  - fixed bugs for driver Makefile/Kconfig
  - continue cleaning-up debug print for driver code.

Changes in v4:
  The patch series is based on v6.7-rc1. You can simply review or test the
  patches at the link [5].
  - dt-bindings: fixed a dt_binding_check error.

Changes in v3:
  The patch series is based on v6.7-rc1. You can simply review or test the
  patches at the link [3].
  - DTS: don't use syscon but define sg2042 specific system control node. More
    background info can read [4].
  - Updating minor issues in dt-bindings as per input from reviews.

Changes in v2:
  The patch series is based on v6.7-rc1. You can simply review or test the
  patches at the link [2].
  - Squashed the patch adding clock definitions with the patch adding the
    binding for the clock controller.
  - Updating dt-binding for syscon, remove oneOf for property compatible;
    define clock controller as child of syscon.
  - DTS changes: merge sg2042-clock.dtsi into sg2042.dtsi; move clock-frequency
    property of osc to board devicethree due to the oscillator is outside the
    SoC.
  - Fixed some bugs in driver code during testing, including removing warnings
    for rv32_defconfig.
  - Updated MAINTAINERS info.

Changes in v1:
  The patch series is based on v6.7-rc1. You can simply review or test the
  patches at the link [1].

Link: https://github.com/unicornx/linux-riscv/commits/upstream-sg2042-clock-v1 [1]
Link: https://github.com/unicornx/linux-riscv/commits/upstream-sg2042-clock-v2 [2]
Link: https://github.com/unicornx/linux-riscv/commits/upstream-sg2042-clock-v3 [3]
Link: https://lore.kernel.org/linux-riscv/MA0P287MB03329AE180378E1A2E034374FE82A@MA0P287MB0332.INDP287.PROD.OUTLOOK.COM/ [4]
Link: https://github.com/unicornx/linux-riscv/commits/upstream-sg2042-clock-v4 [5]
Link: https://github.com/unicornx/linux-riscv/commits/upstream-sg2042-clock-v5 [6]
Link: https://github.com/unicornx/linux-riscv/commits/upstream-sg2042-clock-v6 [7]
Link: https://github.com/unicornx/linux-riscv/commits/upstream-sg2042-clock-v7 [8]

---

Chen Wang (4):
  dt-bindings: soc: sophgo: Add Sophgo system control module
  dt-bindings: clock: sophgo: support SG2042
  clk: sophgo: Add SG2042 clock generator driver
  riscv: dts: add clock generator for Sophgo SG2042 SoC

 .../bindings/clock/sophgo,sg2042-clkgen.yaml  |   53 +
 .../soc/sophgo/sophgo,sg2042-sysctrl.yaml     |   34 +
 MAINTAINERS                                   |    7 +
 .../boot/dts/sophgo/sg2042-milkv-pioneer.dts  |    4 +
 arch/riscv/boot/dts/sophgo/sg2042.dtsi        |   23 +
 drivers/clk/Kconfig                           |    1 +
 drivers/clk/Makefile                          |    1 +
 drivers/clk/sophgo/Kconfig                    |    8 +
 drivers/clk/sophgo/Makefile                   |    2 +
 drivers/clk/sophgo/clk-sophgo-sg2042.c        | 1316 +++++++++++++++++
 drivers/clk/sophgo/clk-sophgo-sg2042.h        |  236 +++
 .../dt-bindings/clock/sophgo,sg2042-clkgen.h  |  169 +++
 12 files changed, 1854 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
 create mode 100644 Documentation/devicetree/bindings/soc/sophgo/sophgo,sg2042-sysctrl.yaml
 create mode 100644 drivers/clk/sophgo/Kconfig
 create mode 100644 drivers/clk/sophgo/Makefile
 create mode 100644 drivers/clk/sophgo/clk-sophgo-sg2042.c
 create mode 100644 drivers/clk/sophgo/clk-sophgo-sg2042.h
 create mode 100644 include/dt-bindings/clock/sophgo,sg2042-clkgen.h


base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
  

On 08/01/2024 07:49, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Add bindings for the clock generator on the SG2042 RISC-V SoC.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../bindings/clock/sophgo,sg2042-clkgen.yaml  |  53 ++++++
>  .../dt-bindings/clock/sophgo,sg2042-clkgen.h  | 169 ++++++++++++++++++
>  2 files changed, 222 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
>  create mode 100644 include/dt-bindings/clock/sophgo,sg2042-clkgen.h
> 
> diff --git a/Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml b/Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
> new file mode 100644
> index 000000000000..f9935e66fc95
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
> @@ -0,0 +1,53 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/clock/sophgo,sg2042-clkgen.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Sophgo SG2042 Clock Generator
> +
> +maintainers:
> +  - Chen Wang <unicorn_wang@outlook.com>
> +
> +properties:
> +  compatible:
> +    const: sophgo,sg2042-clkgen
> +
> +  reg:
> +    maxItems: 1
> +
> +  sophgo,system-ctrl:
> +    $ref: /schemas/types.yaml#/definitions/phandle
> +    description:
> +      Phandle to SG2042 System Controller node. On SG2042, part of control
> +      registers of Clock Controller are defined in System controller. Clock
> +      driver will use this phandle to get the register map base to plus the
> +      offset of the registers to access them.

Do not describe the driver, but hardware. What registers are in
system-ctrl? What are their purpose? Why this hardware needs them?



Best regards,
Krzysztof
  

Hey,

On Wed, Jan 10, 2024 at 08:53:42AM +0800, Chen Wang wrote:
> On 2024/1/8 15:04, Krzysztof Kozlowski wrote:
> > On 08/01/2024 07:49, Chen Wang wrote:
> > > From: Chen Wang <unicorn_wang@outlook.com>
> > > 
> > > Add bindings for the clock generator on the SG2042 RISC-V SoC.
> > > 
> > > Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > ---
> > >   .../bindings/clock/sophgo,sg2042-clkgen.yaml  |  53 ++++++
> > >   .../dt-bindings/clock/sophgo,sg2042-clkgen.h  | 169 ++++++++++++++++++
> > >   2 files changed, 222 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
> > >   create mode 100644 include/dt-bindings/clock/sophgo,sg2042-clkgen.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml b/Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
> > > new file mode 100644
> > > index 000000000000..f9935e66fc95
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
> > > @@ -0,0 +1,53 @@
> > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/clock/sophgo,sg2042-clkgen.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Sophgo SG2042 Clock Generator
> > > +
> > > +maintainers:
> > > +  - Chen Wang <unicorn_wang@outlook.com>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    const: sophgo,sg2042-clkgen
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  sophgo,system-ctrl:
> > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > +    description:
> > > +      Phandle to SG2042 System Controller node. On SG2042, part of control
> > > +      registers of Clock Controller are defined in System controller. Clock
> > > +      driver will use this phandle to get the register map base to plus the
> > > +      offset of the registers to access them.
> > Do not describe the driver, but hardware. What registers are in
> > system-ctrl? What are their purpose? Why this hardware needs them?
> Understood, will fix the words in revision, thanks.

I hope that I am not misunderstanding things, but I got a bit suspicious
of this binding and look at the driver, and saw that there are clocks
registered like:

| static int sg2042_clk_register_gates(struct sg2042_clk_data *clk_data,
| 				     const struct sg2042_gate_clock gate_clks[],
| 				     int num_gate_clks)
| {
| 	struct clk_hw *hw;
| 	const struct sg2042_gate_clock *gate;
| 	int i, ret = 0;
| 	void __iomem *reg;
| 
| 	for (i = 0; i < num_gate_clks; i++) {
| 		gate = &gate_clks[i];
| 		if (gate->flag_sysctrl)
| 			reg = clk_data->iobase_syscon + gate->offset_enable;
| 		else
| 			reg = clk_data->iobase + gate->offset_enable;

iobase_syscon is the base address of the system controller that this
property points at & iobase is the base address of the clock controller
itself.

| 		hw = clk_hw_register_gate(NULL,
| 					  gate->name,
| 					  gate->parent_name,
| 					  gate->flags,
| 					  reg,
| 					  gate->bit_idx,
| 					  0,
| 					  &sg2042_clk_lock);

As far as I can tell, in this particular case, for any gate clock that
flag_sysctrl is set, none of the registers actually lie inside the
clkgen region, but instead are entirely contained in the sysctrl region.

I think that this is because your devicetree does not correctly define
the relationship between clocks, and these clocks are actually provided
by the system controller block and are inputs to the clkgen block.

| 		if (IS_ERR(hw)) {
| 			pr_err("failed to register clock %s\n", gate->name);
| 			ret = PTR_ERR(hw);
| 			break;
| 		}
| 
| 		clk_data->onecell_data.hws[gate->id] = hw;
| 	}
| 
| 	/* leave unregister to outside if failed */
| 	return ret;
| }

I had a much briefer look at the `sg2042_pll_clock`s that make use of
the regmap, and it doesn't seem like they "mix and match" registers
between both blocks, and instead only have registers in the system
controller? If so, it doesn't seem like this clkgen block should be
providing the PLL clocks either, but instead be taking them as inputs.

Reading stuff like
https://github.com/sophgo/sophgo-doc/blob/main/SG2042/TRM/source/system-control.rst#pll_stat-offset-0x0c0
(and onwards) makes it seem like those PLLs are fully contained within
the system controller register space.

It seems like
https://github.com/sophgo/sophgo-doc/blob/main/SG2042/TRM/source/clock-reg.rst
is the register map for the clkgen region? It seems like that region
only contains gates and divider clocks, but no PLLs.

Am I missing something, or is this description of the clock controllers
on the soc incomplete?

Cheers,
Conor.
  

On Thu, Jan 11, 2024 at 04:00:04PM +0800, Chen Wang wrote:
> Resent and fixed some format issue in last email.
> 
> On 2024/1/10 22:42, Conor Dooley wrote:
> > Hey,
> > 
> > On Wed, Jan 10, 2024 at 08:53:42AM +0800, Chen Wang wrote:
> > > On 2024/1/8 15:04, Krzysztof Kozlowski wrote:
> > > > On 08/01/2024 07:49, Chen Wang wrote:
> > > > > From: Chen Wang <unicorn_wang@outlook.com>
> > > > > 
> > > > > Add bindings for the clock generator on the SG2042 RISC-V SoC.
> > > > > 
> > > > > Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> > > > > Reviewed-by: Conor Dooley <conor.dooley@microchip.com>
> > > > > ---
> > > > >    .../bindings/clock/sophgo,sg2042-clkgen.yaml  |  53 ++++++
> > > > >    .../dt-bindings/clock/sophgo,sg2042-clkgen.h  | 169 ++++++++++++++++++
> > > > >    2 files changed, 222 insertions(+)
> > > > >    create mode 100644 Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
> > > > >    create mode 100644 include/dt-bindings/clock/sophgo,sg2042-clkgen.h
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml b/Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..f9935e66fc95
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/clock/sophgo,sg2042-clkgen.yaml
> > > > > @@ -0,0 +1,53 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/clock/sophgo,sg2042-clkgen.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Sophgo SG2042 Clock Generator
> > > > > +
> > > > > +maintainers:
> > > > > +  - Chen Wang <unicorn_wang@outlook.com>
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    const: sophgo,sg2042-clkgen
> > > > > +
> > > > > +  reg:
> > > > > +    maxItems: 1
> > > > > +
> > > > > +  sophgo,system-ctrl:
> > > > > +    $ref: /schemas/types.yaml#/definitions/phandle
> > > > > +    description:
> > > > > +      Phandle to SG2042 System Controller node. On SG2042, part of control
> > > > > +      registers of Clock Controller are defined in System controller. Clock
> > > > > +      driver will use this phandle to get the register map base to plus the
> > > > > +      offset of the registers to access them.
> > > > Do not describe the driver, but hardware. What registers are in
> > > > system-ctrl? What are their purpose? Why this hardware needs them?
> > > Understood, will fix the words in revision, thanks.
> > I hope that I am not misunderstanding things, but I got a bit suspicious
> > of this binding and look at the driver, and saw that there are clocks
> > registered like:
> > 
> > | static int sg2042_clk_register_gates(struct sg2042_clk_data *clk_data,
> > | 				     const struct sg2042_gate_clock gate_clks[],
> > | 				     int num_gate_clks)
> > | {
> > | 	struct clk_hw *hw;
> > | 	const struct sg2042_gate_clock *gate;
> > | 	int i, ret = 0;
> > | 	void __iomem *reg;
> > |
> > | 	for (i = 0; i < num_gate_clks; i++) {
> > | 		gate = &gate_clks[i];
> > | 		if (gate->flag_sysctrl)
> > | 			reg = clk_data->iobase_syscon + gate->offset_enable;
> > | 		else
> > | 			reg = clk_data->iobase + gate->offset_enable;
> > 
> > iobase_syscon is the base address of the system controller that this
> > property points at & iobase is the base address of the clock controller
> > itself.
> > 
> > | 		hw = clk_hw_register_gate(NULL,
> > | 					  gate->name,
> > | 					  gate->parent_name,
> > | 					  gate->flags,
> > | 					  reg,
> > | 					  gate->bit_idx,
> > | 					  0,
> > | 					  &sg2042_clk_lock);
> > 
> > As far as I can tell, in this particular case, for any gate clock that
> > flag_sysctrl is set, none of the registers actually lie inside the
> > clkgen region, but instead are entirely contained in the sysctrl region.
> > 
> > I think that this is because your devicetree does not correctly define
> > the relationship between clocks, and these clocks are actually provided
> > by the system controller block and are inputs to the clkgen block.
> > 
> > | 		if (IS_ERR(hw)) {
> > | 			pr_err("failed to register clock %s\n", gate->name);
> > | 			ret = PTR_ERR(hw);
> > | 			break;
> > | 		}
> > |
> > | 		clk_data->onecell_data.hws[gate->id] = hw;
> > | 	}
> > |
> > | 	/* leave unregister to outside if failed */
> > | 	return ret;
> > | }
> > 
> > I had a much briefer look at the `sg2042_pll_clock`s that make use of
> > the regmap, and it doesn't seem like they "mix and match" registers
> > between both blocks, and instead only have registers in the system
> > controller? If so, it doesn't seem like this clkgen block should be
> > providing the PLL clocks either, but instead be taking them as inputs.
> > 
> > Reading stuff like
> > https://github.com/sophgo/sophgo-doc/blob/main/SG2042/TRM/source/system-control.rst#pll_stat-offset-0x0c0
> > (and onwards) makes it seem like those PLLs are fully contained within
> > the system controller register space.
> > 
> > It seems like
> > https://github.com/sophgo/sophgo-doc/blob/main/SG2042/TRM/source/clock-reg.rst
> > is the register map for the clkgen region? It seems like that region
> > only contains gates and divider clocks, but no PLLs.
> > 
> > Am I missing something, or is this description of the clock controllers
> > on the soc incomplete?
> hi，Conor，
> 
> There are four types of clocks for SG2042 and following are where their
> control registers are defined in:
> 
> PLL：all in SYS_CTRL
> DIV: all in CLOCK
> GATE: some are in SYS_CTRL, some others are in CLOCK

When you say "some", do you meant some entire clocks are in SYS_CTRL and
some entire clocks are in the CLOCKS? Or do you meant that for a given
clock, some registers are in SYS_CTRL and some are in CLOCK? It's the
first option, right?

> MUX: all in CLOCK
> 
> For PLLs, yes, they are all controlled by registers defined in SYS_CTRL.
> About what you said "it doesn't seem like this clkgen block should be
> providing the PLL clocks either, but instead be taking them as inputs.", I
> am not very sure what your meaning of "inputs". I try to write DTS with my
> undrstadning, please help me see if it fits what you mean.
> 
> ```dts
> 
> sys_ctrl: system-controller@7030010000 {
>     compatible = "sophgo,sg2042-sysctrl";
>     reg = <0x70 0x30010000 0x0 0x1000>;
> 
>     pllclk: clock-controller {
>         compatible = "sophgo,sg2042-pll";
>         #clock-cells = <1>;
>         clocks = <&cgi>;
>     };
> };
> 
> clkgen: clock-controller@7030012000 {
>     compatible = "sophgo,sg2042-clkgen";
>     reg = <0x70 0x30012000 0x0 0x1000>;
>     #clock-cells = <1>;
>     clocks = <&pllclk MPLL_CLK>,
>         <&pllclk FPLL_CLK>,
>         <&pllclk DPLL0_CLK>,
>         <&pllclk DPLL1_CLK>;
>     clock-names = "cgi", "mpll", "fpll", "dpll0", "dpll1";
> };
> 
> ```
> 
> With this change, we describe the plls defined in system control as pllclk,
> as a child node of system controller. clkgen will use pllclk as "input"
> because pll clocks are parent of div clocks .
> 
> But there is another remaining question about the gate clock. For those gate
> clocks controlled by CLOCK, no problem we will provide then in clkgen, but 
> for those gate clocks controlled by registers in SYS_CTRL, they are child
> gate of the "clk_gate_rp_cpu_normal", which is a gate clock provided by
> clkgen. If I extracted those SYS_CTRL gate clocks and define them in system
> controller dts node, I may have to use "clk_gate_rp_cpu_normal" as their
> input, it looks a bit wierd becasue there are cases where each other serves
> as input. I try to draft below DTS to explan what I meant. I'm not sure if
> it can work and I'd love to hear your guidance.

I'm not sure how this sort of circular relationship works for probing
works either. Stephen etc would know more than me here.

> ```dts
> 
> sys_ctrl: system-controller@7030010000 {
>     compatible = "sophgo,sg2042-sysctrl";
>     reg = <0x70 0x30010000 0x0 0x1000>;
> 
>     pllclk: clock-controller {
>         compatible = "sophgo,sg2042-pll";
>         #clock-cells = <1>;
>         clocks = <&cgi>;
>     };
> 
>     somegateclk: clock-controller2 {
>         compatible = "sophgo,sg2042-somegate";
>         #clock-cells = <1>;
>         clocks = <&clkgen GATE_CLK_RP_CPU_NORMAL>;
>         clock-names = "clk_gate_rp_cpu_normal";
>     };
> };
> 
> clkgen: clock-controller@7030012000 {
>     compatible = "sophgo,sg2042-clkgen";
>     reg = <0x70 0x30012000 0x0 0x1000>;
>     #clock-cells = <1>;
>     clocks =      <&pllclk MPLL_CLK>,
>              <&pllclk FPLL_CLK>,
>              <&pllclk DPLL0_CLK>,
>              <&pllclk DPLL1_CLK>,;
>     clock-names = "cgi", "mpll", "fpll", "dpll0", "dpll1";
> };
> 
> ```
> 
> So, can we put all gate clocks in clkgen to simplify this?

The dts should describe how the hardware actually looks, even if that is
not really convenient for the operating system.
  

On Fri, Jan 12, 2024 at 08:08:15AM +0800, Chen Wang wrote:
> On 2024/1/12 0:58, Conor Dooley wrote:
> > On Thu, Jan 11, 2024 at 04:00:04PM +0800, Chen Wang wrote:

> > > There are four types of clocks for SG2042 and following are where their
> > > control registers are defined in:
> > > 
> > > PLL：all in SYS_CTRL
> > > DIV: all in CLOCK
> > > GATE: some are in SYS_CTRL, some others are in CLOCK
> > When you say "some", do you meant some entire clocks are in SYS_CTRL and
> > some entire clocks are in the CLOCKS? Or do you meant that for a given
> > clock, some registers are in SYS_CTRL and some are in CLOCK? It's the
> > first option, right?
> 
> It's the first option.

Then the gate clocks that are fully contained within SYS_CTRL are
outputs of SYS_CTRL and gate clocks fully contained within CLOCK are
outputs of CLOCK. You should not use a phandle to SYS_CTRL from the
CLOCKS node so that you can pretend they are part of CLOCKS just because
that makes writing your driver easier. That said, obviously you can
share the routines for turning the gates on and off etc.

Cheers,
Conor.
  

On 12/01/2024 09:35, Chen Wang wrote:
> Conor and Krzysztof,
> 
> Just a quick question, due to I am planning to change the binding files 
> you have reviewed,  should I remain your signature of “Reviewed-by" or 
> remove it in next patchset?

Depends on the amount of changes. If you remove or add properties, then
please drop the tag. Whenever you drop a tag or skip it (so do not add
after receiving), explain this in the changelog. changelog goes under ---.

Best regards,
Krzysztof
  

Hi Conor, Chen,

On 2024-01-11 10:58 AM, Conor Dooley wrote:
> On Thu, Jan 11, 2024 at 04:00:04PM +0800, Chen Wang wrote:
>> With this change, we describe the plls defined in system control as pllclk,
>> as a child node of system controller. clkgen will use pllclk as "input"
>> because pll clocks are parent of div clocks .
>>
>> But there is another remaining question about the gate clock. For those gate
>> clocks controlled by CLOCK, no problem we will provide then in clkgen, but 
>> for those gate clocks controlled by registers in SYS_CTRL, they are child
>> gate of the "clk_gate_rp_cpu_normal", which is a gate clock provided by
>> clkgen. If I extracted those SYS_CTRL gate clocks and define them in system
>> controller dts node, I may have to use "clk_gate_rp_cpu_normal" as their
>> input, it looks a bit wierd becasue there are cases where each other serves
>> as input. I try to draft below DTS to explan what I meant. I'm not sure if
>> it can work and I'd love to hear your guidance.
> 
> I'm not sure how this sort of circular relationship works for probing
> works either. Stephen etc would know more than me here.

It generally works fine. The common clock framework can handle the child clock
being registered before its parent, even when using a DT (fw_name) reference.
See for example clk_core_fill_parent_index() and
clk_core_reparent_orphans_nolock() in drivers/clk/clk.c

Regards,
Samuel