Message-ID:
 <MA0P287MB282224A6097B4FCCF721C89AFE682@MA0P287MB2822.INDP287.PROD.OUTLOOK.COM>
Date: Thu, 11 Jan 2024 16:00:04 +0800
From: Chen Wang <unicorn_wang@...look.com>
To: Conor Dooley <conor@...nel.org>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
 Chen Wang <unicornxw@...il.com>, aou@...s.berkeley.edu, chao.wei@...hgo.com,
 krzysztof.kozlowski+dt@...aro.org, mturquette@...libre.com,
 palmer@...belt.com, paul.walmsley@...ive.com, richardcochran@...il.com,
 robh+dt@...nel.org, sboyd@...nel.org, devicetree@...r.kernel.org,
 linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-riscv@...ts.infradead.org, haijiao.liu@...hgo.com,
 xiaoguang.xing@...hgo.com, guoren@...nel.org, jszhang@...nel.org,
 inochiama@...look.com, samuel.holland@...ive.com,
 Conor Dooley <conor.dooley@...rochip.com>
Subject: Re: [PATCH v7 2/4] dt-bindings: clock: sophgo: support SG2042

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@...look.com>
>>>>
>>>> Add bindings for the clock generator on the SG2042 RISC-V SoC.
>>>>
>>>> Signed-off-by: Chen Wang <unicorn_wang@...look.com>
>>>> Reviewed-by: Conor Dooley <conor.dooley@...rochip.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@...look.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@...0010000 {
     compatible = "sophgo,sg2042-sysctrl";
     reg = <0x70 0x30010000 0x0 0x1000>;

     pllclk: clock-controller {
         compatible = "sophgo,sg2042-pll";
         #clock-cells = <1>;
         clocks = <&cgi>;
     };
};

clkgen: clock-controller@...0012000 {
     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@...0010000 {
     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@...0012000 {
     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.

Powered by blists - more mailing lists