diff mbox series

[v7,2/4] dt-bindings: clock: sophgo: support SG2042

Message ID 925d99d5b4ece01337cb3389aaea4b631894dd1d.1704694903.git.unicorn_wang@outlook.com (mailing list archive)
State Changes Requested, archived
Headers show
Series riscv: sophgo: add clock support for sg2042 | expand

Commit Message

Chen Wang Jan. 8, 2024, 6:49 a.m. UTC
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

Comments

Krzysztof Kozlowski Jan. 8, 2024, 7:04 a.m. UTC | #1
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
Chen Wang Jan. 10, 2024, 12:53 a.m. UTC | #2
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.
>
>
>
> Best regards,
> Krzysztof
>
Conor Dooley Jan. 10, 2024, 2:42 p.m. UTC | #3
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.
Chen Wang Jan. 11, 2024, 7:51 a.m. UTC | #4
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
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.

```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?

Thanks

Chen

> Cheers,
> Conor.
Chen Wang Jan. 11, 2024, 8 a.m. UTC | #5
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
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.

```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?

Thanks

Chen
>
> Cheers,
> Conor.
Conor Dooley Jan. 11, 2024, 4:58 p.m. UTC | #6
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.
Chen Wang Jan. 12, 2024, 12:08 a.m. UTC | #7
On 2024/1/12 0:58, Conor Dooley wrote:
> On Thu, Jan 11, 2024 at 04:00:04PM +0800, Chen Wang wrote:
>> 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?

It's the first option.

>> MUX: all in CLOCK
Conor Dooley Jan. 12, 2024, 7:42 a.m. UTC | #8
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.
Chen Wang Jan. 12, 2024, 8:27 a.m. UTC | #9
On 2024/1/12 15:42, Conor Dooley wrote:
> 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.

Um, seems that we need to define two clock-controllers to output their 
own clocks respectively. Thank you for your patient guidance, let me 
re-cook the code.

Regards,

Chen

> Cheers,
> Conor.
Chen Wang Jan. 12, 2024, 8:35 a.m. UTC | #10
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?

On 2024/1/12 15:42, Conor Dooley wrote:
> 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.
Krzysztof Kozlowski Jan. 12, 2024, 8:38 a.m. UTC | #11
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
Samuel Holland Jan. 12, 2024, 7:35 p.m. UTC | #12
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
Chen Wang Jan. 13, 2024, 1:15 a.m. UTC | #13
On 2024/1/13 3:35, Samuel Holland wrote:
> 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
Learned and thank you.
>
> Regards,
> Samuel
>
diff mbox series

Patch

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.
+
+  clocks:
+    items:
+      - description: Clock Generation IC (25 MHz)
+
+  '#clock-cells':
+    const: 1
+    description:
+      See <dt-bindings/clock/sophgo,sg2042-clkgen.h> for valid indices.
+
+required:
+  - compatible
+  - reg
+  - sophgo,system-ctrl
+  - clocks
+  - '#clock-cells'
+
+additionalProperties: false
+
+examples:
+  - |
+    clock-controller@30012000 {
+        compatible = "sophgo,sg2042-clkgen";
+        reg = <0x30012000 0x1000>;
+        sophgo,system-ctrl = <&sys_ctrl>;
+        clocks = <&cgi>;
+        #clock-cells = <1>;
+    };
diff --git a/include/dt-bindings/clock/sophgo,sg2042-clkgen.h b/include/dt-bindings/clock/sophgo,sg2042-clkgen.h
new file mode 100644
index 000000000000..78cd3c3efdb6
--- /dev/null
+++ b/include/dt-bindings/clock/sophgo,sg2042-clkgen.h
@@ -0,0 +1,169 @@ 
+/* SPDX-License-Identifier: GPL-2.0 OR BSD-2-Clause */
+/*
+ * Copyright (C) 2023 Sophgo Technology Inc. All rights reserved.
+ */
+
+#ifndef __DT_BINDINGS_CLOCK_SOPHGO_SG2042_H__
+#define __DT_BINDINGS_CLOCK_SOPHGO_SG2042_H__
+
+/* Divider clocks */
+#define DIV_CLK_MPLL_RP_CPU_NORMAL_0	0
+#define DIV_CLK_MPLL_AXI_DDR_0		1
+#define DIV_CLK_FPLL_DDR01_1		2
+#define DIV_CLK_FPLL_DDR23_1		3
+#define DIV_CLK_FPLL_RP_CPU_NORMAL_1	4
+#define DIV_CLK_FPLL_50M_A53		5
+#define DIV_CLK_FPLL_TOP_RP_CMN_DIV2	6
+#define DIV_CLK_FPLL_UART_500M		7
+#define DIV_CLK_FPLL_AHB_LPC		8
+#define DIV_CLK_FPLL_EFUSE		9
+#define DIV_CLK_FPLL_TX_ETH0		10
+#define DIV_CLK_FPLL_PTP_REF_I_ETH0	11
+#define DIV_CLK_FPLL_REF_ETH0		12
+#define DIV_CLK_FPLL_EMMC		13
+#define DIV_CLK_FPLL_SD			14
+#define DIV_CLK_FPLL_TOP_AXI0		15
+#define DIV_CLK_FPLL_TOP_AXI_HSPERI	16
+#define DIV_CLK_FPLL_AXI_DDR_1		17
+#define DIV_CLK_FPLL_DIV_TIMER1		18
+#define DIV_CLK_FPLL_DIV_TIMER2		19
+#define DIV_CLK_FPLL_DIV_TIMER3		20
+#define DIV_CLK_FPLL_DIV_TIMER4		21
+#define DIV_CLK_FPLL_DIV_TIMER5		22
+#define DIV_CLK_FPLL_DIV_TIMER6		23
+#define DIV_CLK_FPLL_DIV_TIMER7		24
+#define DIV_CLK_FPLL_DIV_TIMER8		25
+#define DIV_CLK_FPLL_100K_EMMC		26
+#define DIV_CLK_FPLL_100K_SD		27
+#define DIV_CLK_FPLL_GPIO_DB		28
+#define DIV_CLK_DPLL0_DDR01_0		29
+#define DIV_CLK_DPLL1_DDR23_0		30
+
+/* Gate clocks */
+#define GATE_CLK_RP_CPU_NORMAL_DIV0	31
+#define GATE_CLK_AXI_DDR_DIV0		32
+
+#define GATE_CLK_RP_CPU_NORMAL_DIV1	33
+#define GATE_CLK_A53_50M		34
+#define GATE_CLK_TOP_RP_CMN_DIV2	35
+#define GATE_CLK_HSDMA			36
+#define GATE_CLK_EMMC_100M		37
+#define GATE_CLK_SD_100M		38
+#define GATE_CLK_TX_ETH0		39
+#define GATE_CLK_PTP_REF_I_ETH0		40
+#define GATE_CLK_REF_ETH0		41
+#define GATE_CLK_UART_500M		42
+#define GATE_CLK_EFUSE			43
+
+#define GATE_CLK_AHB_LPC		44
+#define GATE_CLK_AHB_ROM		45
+#define GATE_CLK_AHB_SF			46
+
+#define GATE_CLK_APB_UART		47
+#define GATE_CLK_APB_TIMER		48
+#define GATE_CLK_APB_EFUSE		49
+#define GATE_CLK_APB_GPIO		50
+#define GATE_CLK_APB_GPIO_INTR		51
+#define GATE_CLK_APB_SPI		52
+#define GATE_CLK_APB_I2C		53
+#define GATE_CLK_APB_WDT		54
+#define GATE_CLK_APB_PWM		55
+#define GATE_CLK_APB_RTC		56
+
+#define GATE_CLK_AXI_PCIE0		57
+#define GATE_CLK_AXI_PCIE1		58
+#define GATE_CLK_SYSDMA_AXI		59
+#define GATE_CLK_AXI_DBG_I2C		60
+#define GATE_CLK_AXI_SRAM		61
+#define GATE_CLK_AXI_ETH0		62
+#define GATE_CLK_AXI_EMMC		63
+#define GATE_CLK_AXI_SD			64
+#define GATE_CLK_TOP_AXI0		65
+#define GATE_CLK_TOP_AXI_HSPERI		66
+
+#define GATE_CLK_TIMER1			67
+#define GATE_CLK_TIMER2			68
+#define GATE_CLK_TIMER3			69
+#define GATE_CLK_TIMER4			70
+#define GATE_CLK_TIMER5			71
+#define GATE_CLK_TIMER6			72
+#define GATE_CLK_TIMER7			73
+#define GATE_CLK_TIMER8			74
+#define GATE_CLK_100K_EMMC		75
+#define GATE_CLK_100K_SD		76
+#define GATE_CLK_GPIO_DB		77
+
+#define GATE_CLK_AXI_DDR_DIV1		78
+#define GATE_CLK_DDR01_DIV1		79
+#define GATE_CLK_DDR23_DIV1		80
+/* DPLL0 */
+#define GATE_CLK_DDR01_DIV0		81
+/* DPLL1 */
+#define GATE_CLK_DDR23_DIV0		82
+
+#define GATE_CLK_DDR01			83
+#define GATE_CLK_DDR23			84
+#define GATE_CLK_RP_CPU_NORMAL		85
+#define GATE_CLK_AXI_DDR		86
+#define GATE_CLK_RXU0			87
+#define GATE_CLK_RXU1			88
+#define GATE_CLK_RXU2			89
+#define GATE_CLK_RXU3			90
+#define GATE_CLK_RXU4			91
+#define GATE_CLK_RXU5			92
+#define GATE_CLK_RXU6			93
+#define GATE_CLK_RXU7			94
+#define GATE_CLK_RXU8			95
+#define GATE_CLK_RXU9			96
+#define GATE_CLK_RXU10			97
+#define GATE_CLK_RXU11			98
+#define GATE_CLK_RXU12			99
+#define GATE_CLK_RXU13			100
+#define GATE_CLK_RXU14			101
+#define GATE_CLK_RXU15			102
+#define GATE_CLK_RXU16			103
+#define GATE_CLK_RXU17			104
+#define GATE_CLK_RXU18			105
+#define GATE_CLK_RXU19			106
+#define GATE_CLK_RXU20			107
+#define GATE_CLK_RXU21			108
+#define GATE_CLK_RXU22			109
+#define GATE_CLK_RXU23			110
+#define GATE_CLK_RXU24			111
+#define GATE_CLK_RXU25			112
+#define GATE_CLK_RXU26			113
+#define GATE_CLK_RXU27			114
+#define GATE_CLK_RXU28			115
+#define GATE_CLK_RXU29			116
+#define GATE_CLK_RXU30			117
+#define GATE_CLK_RXU31			118
+#define GATE_CLK_MP0			119
+#define GATE_CLK_MP1			120
+#define GATE_CLK_MP2			121
+#define GATE_CLK_MP3			122
+#define GATE_CLK_MP4			123
+#define GATE_CLK_MP5			124
+#define GATE_CLK_MP6			125
+#define GATE_CLK_MP7			126
+#define GATE_CLK_MP8			127
+#define GATE_CLK_MP9			128
+#define GATE_CLK_MP10			129
+#define GATE_CLK_MP11			130
+#define GATE_CLK_MP12			131
+#define GATE_CLK_MP13			132
+#define GATE_CLK_MP14			133
+#define GATE_CLK_MP15			134
+
+/* MUX clocks */
+#define MUX_CLK_DDR01			135
+#define MUX_CLK_DDR23			136
+#define MUX_CLK_RP_CPU_NORMAL		137
+#define MUX_CLK_AXI_DDR			138
+
+/* PLL clocks */
+#define MPLL_CLK			139
+#define FPLL_CLK			140
+#define DPLL0_CLK			141
+#define DPLL1_CLK			142
+
+#endif /* __DT_BINDINGS_CLOCK_SOPHGO_SG2042_H__ */