diff mbox series

[v1,3/3] riscv: dts: starfive: jh7110: Add PLL clock node

Message ID	20230221141147.303642-4-xingyu.wu@starfivetech.com
State	New
Headers	Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; From: Xingyu Wu <xingyu.wu@starfivetech.com> To: <linux-riscv@lists.infradead.org>, <devicetree@vger.kernel.org>, "Michael Turquette" <mturquette@baylibre.com>, Stephen Boyd <sboyd@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Philipp Zabel <p.zabel@pengutronix.de>, Emil Renner Berthing <kernel@esmil.dk> CC: Rob Herring <robh+dt@kernel.org>, Conor Dooley <conor@kernel.org>, "Paul Walmsley" <paul.walmsley@sifive.com>, Palmer Dabbelt <palmer@dabbelt.com>, Albert Ou <aou@eecs.berkeley.edu>, Hal Feng <hal.feng@starfivetech.com>, Xingyu Wu <xingyu.wu@starfivetech.com>, <linux-kernel@vger.kernel.org>, <linux-clk@vger.kernel.org> Subject: [PATCH v1 3/3] riscv: dts: starfive: jh7110: Add PLL clock node Date: Tue, 21 Feb 2023 22:11:47 +0800 Message-ID: <20230221141147.303642-4-xingyu.wu@starfivetech.com> In-Reply-To: <20230221141147.303642-1-xingyu.wu@starfivetech.com> References: <20230221141147.303642-1-xingyu.wu@starfivetech.com> MIME-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 8BIT Precedence: bulk
Series	Add PLL clocks driver for StarFive JH7110 \| [v1,0/3] Add PLL clocks driver for StarFive JH7110 [v1,1/3] dt-bindings: clock: Add StarFive JH7110 PLL clock generator [v1,2/3] clk: starfive: Add StarFive JH7110 PLL clock driver [v1,3/3] riscv: dts: starfive: jh7110: Add PLL clock node

Commit Message

Xingyu Wu Feb. 21, 2023, 2:11 p.m. UTC

  Add the PLL clock node for the Starfive JH7110 SoC and
modify the SYSCRG node to add PLL clocks.

Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
---
 arch/riscv/boot/dts/starfive/jh7110.dtsi | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski Feb. 22, 2023, 9:09 a.m. UTC | #1

On 21/02/2023 15:11, Xingyu Wu wrote:
> Add the PLL clock node for the Starfive JH7110 SoC and
> modify the SYSCRG node to add PLL clocks.
> 
> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
> ---
>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> index b6612c53d0d2..0cb8d86ebce5 100644
> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
> @@ -461,12 +461,16 @@ syscrg: clock-controller@13020000 {
>  				 <&gmac1_rgmii_rxin>,
>  				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
>  				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
> -				 <&tdm_ext>, <&mclk_ext>;
> +				 <&tdm_ext>, <&mclk_ext>,
> +				 <&pllclk JH7110_CLK_PLL0_OUT>,
> +				 <&pllclk JH7110_CLK_PLL1_OUT>,
> +				 <&pllclk JH7110_CLK_PLL2_OUT>;
>  			clock-names = "osc", "gmac1_rmii_refin",
>  				      "gmac1_rgmii_rxin",
>  				      "i2stx_bclk_ext", "i2stx_lrck_ext",
>  				      "i2srx_bclk_ext", "i2srx_lrck_ext",
> -				      "tdm_ext", "mclk_ext";
> +				      "tdm_ext", "mclk_ext",
> +				      "pll0_out", "pll1_out", "pll2_out";
>  			#clock-cells = <1>;
>  			#reset-cells = <1>;
>  		};
> @@ -476,6 +480,13 @@ sys_syscon: syscon@13030000 {
>  			reg = <0x0 0x13030000 0x0 0x1000>;
>  		};
>  
> +		pllclk: pll-clock-controller {

Does not look like you tested the DTS against bindings. Please run `make
dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
for instructions). You should see here warnings of mixing non-MMIO nodes
in MMIO-bus.

Best regards,
Krzysztof

Xingyu Wu Feb. 23, 2023, 8:47 a.m. UTC | #2

On 2023/2/22 17:09, Krzysztof Kozlowski wrote:
> On 21/02/2023 15:11, Xingyu Wu wrote:
>> Add the PLL clock node for the Starfive JH7110 SoC and
>> modify the SYSCRG node to add PLL clocks.
>> 
>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>> ---
>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 15 +++++++++++++--
>>  1 file changed, 13 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> index b6612c53d0d2..0cb8d86ebce5 100644
>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>> @@ -461,12 +461,16 @@ syscrg: clock-controller@13020000 {
>>  				 <&gmac1_rgmii_rxin>,
>>  				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
>>  				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
>> -				 <&tdm_ext>, <&mclk_ext>;
>> +				 <&tdm_ext>, <&mclk_ext>,
>> +				 <&pllclk JH7110_CLK_PLL0_OUT>,
>> +				 <&pllclk JH7110_CLK_PLL1_OUT>,
>> +				 <&pllclk JH7110_CLK_PLL2_OUT>;
>>  			clock-names = "osc", "gmac1_rmii_refin",
>>  				      "gmac1_rgmii_rxin",
>>  				      "i2stx_bclk_ext", "i2stx_lrck_ext",
>>  				      "i2srx_bclk_ext", "i2srx_lrck_ext",
>> -				      "tdm_ext", "mclk_ext";
>> +				      "tdm_ext", "mclk_ext",
>> +				      "pll0_out", "pll1_out", "pll2_out";
>>  			#clock-cells = <1>;
>>  			#reset-cells = <1>;
>>  		};
>> @@ -476,6 +480,13 @@ sys_syscon: syscon@13030000 {
>>  			reg = <0x0 0x13030000 0x0 0x1000>;
>>  		};
>>  
>> +		pllclk: pll-clock-controller {
> 
> Does not look like you tested the DTS against bindings. Please run `make
> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
> for instructions). You should see here warnings of mixing non-MMIO nodes
> in MMIO-bus.
> 

Oh I cherry-pick the commit of syscon node and it also include the MMC node.
I will remove the MMC node. 
I used dtbs_check and get the error 'should not be valid under {'type': 'object'}',
If I move this node out of the 'soc' node, the dtbs_check will be pass.
Is it OK to move the PLL node out of the 'soc' node? Thanks.

Best regards,
Xingyu Wu

Krzysztof Kozlowski Feb. 23, 2023, 8:52 a.m. UTC | #3

On 23/02/2023 09:47, Xingyu Wu wrote:
> On 2023/2/22 17:09, Krzysztof Kozlowski wrote:
>> On 21/02/2023 15:11, Xingyu Wu wrote:
>>> Add the PLL clock node for the Starfive JH7110 SoC and
>>> modify the SYSCRG node to add PLL clocks.
>>>
>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>> ---
>>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 15 +++++++++++++--
>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> index b6612c53d0d2..0cb8d86ebce5 100644
>>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>> @@ -461,12 +461,16 @@ syscrg: clock-controller@13020000 {
>>>  				 <&gmac1_rgmii_rxin>,
>>>  				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
>>>  				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
>>> -				 <&tdm_ext>, <&mclk_ext>;
>>> +				 <&tdm_ext>, <&mclk_ext>,
>>> +				 <&pllclk JH7110_CLK_PLL0_OUT>,
>>> +				 <&pllclk JH7110_CLK_PLL1_OUT>,
>>> +				 <&pllclk JH7110_CLK_PLL2_OUT>;
>>>  			clock-names = "osc", "gmac1_rmii_refin",
>>>  				      "gmac1_rgmii_rxin",
>>>  				      "i2stx_bclk_ext", "i2stx_lrck_ext",
>>>  				      "i2srx_bclk_ext", "i2srx_lrck_ext",
>>> -				      "tdm_ext", "mclk_ext";
>>> +				      "tdm_ext", "mclk_ext",
>>> +				      "pll0_out", "pll1_out", "pll2_out";
>>>  			#clock-cells = <1>;
>>>  			#reset-cells = <1>;
>>>  		};
>>> @@ -476,6 +480,13 @@ sys_syscon: syscon@13030000 {
>>>  			reg = <0x0 0x13030000 0x0 0x1000>;
>>>  		};
>>>  
>>> +		pllclk: pll-clock-controller {
>>
>> Does not look like you tested the DTS against bindings. Please run `make
>> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
>> for instructions). You should see here warnings of mixing non-MMIO nodes
>> in MMIO-bus.
>>
> 
> Oh I cherry-pick the commit of syscon node and it also include the MMC node.
> I will remove the MMC node. 
> I used dtbs_check and get the error 'should not be valid under {'type': 'object'}',
> If I move this node out of the 'soc' node, the dtbs_check will be pass.
> Is it OK to move the PLL node out of the 'soc' node? Thanks.

Shall it be out side of soc? How it can then do anything with registers?
This does not look like correct representation of hardware.

Best regards,
Krzysztof

Xingyu Wu Feb. 23, 2023, 9:03 a.m. UTC | #4

On 2023/2/23 16:52, Krzysztof Kozlowski wrote:
> On 23/02/2023 09:47, Xingyu Wu wrote:
>> On 2023/2/22 17:09, Krzysztof Kozlowski wrote:
>>> On 21/02/2023 15:11, Xingyu Wu wrote:
>>>> Add the PLL clock node for the Starfive JH7110 SoC and
>>>> modify the SYSCRG node to add PLL clocks.
>>>>
>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>>> ---
>>>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 15 +++++++++++++--
>>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>> index b6612c53d0d2..0cb8d86ebce5 100644
>>>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>> @@ -461,12 +461,16 @@ syscrg: clock-controller@13020000 {
>>>>  				 <&gmac1_rgmii_rxin>,
>>>>  				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
>>>>  				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
>>>> -				 <&tdm_ext>, <&mclk_ext>;
>>>> +				 <&tdm_ext>, <&mclk_ext>,
>>>> +				 <&pllclk JH7110_CLK_PLL0_OUT>,
>>>> +				 <&pllclk JH7110_CLK_PLL1_OUT>,
>>>> +				 <&pllclk JH7110_CLK_PLL2_OUT>;
>>>>  			clock-names = "osc", "gmac1_rmii_refin",
>>>>  				      "gmac1_rgmii_rxin",
>>>>  				      "i2stx_bclk_ext", "i2stx_lrck_ext",
>>>>  				      "i2srx_bclk_ext", "i2srx_lrck_ext",
>>>> -				      "tdm_ext", "mclk_ext";
>>>> +				      "tdm_ext", "mclk_ext",
>>>> +				      "pll0_out", "pll1_out", "pll2_out";
>>>>  			#clock-cells = <1>;
>>>>  			#reset-cells = <1>;
>>>>  		};
>>>> @@ -476,6 +480,13 @@ sys_syscon: syscon@13030000 {
>>>>  			reg = <0x0 0x13030000 0x0 0x1000>;
>>>>  		};
>>>>  
>>>> +		pllclk: pll-clock-controller {
>>>
>>> Does not look like you tested the DTS against bindings. Please run `make
>>> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
>>> for instructions). You should see here warnings of mixing non-MMIO nodes
>>> in MMIO-bus.
>>>
>> 
>> Oh I cherry-pick the commit of syscon node and it also include the MMC node.
>> I will remove the MMC node. 
>> I used dtbs_check and get the error 'should not be valid under {'type': 'object'}',
>> If I move this node out of the 'soc' node, the dtbs_check will be pass.
>> Is it OK to move the PLL node out of the 'soc' node? Thanks.
> 
> Shall it be out side of soc? How it can then do anything with registers?
> This does not look like correct representation of hardware.

The error appears to be due to a lack of reg base about PLL node. PLL do something with register
by 'sys_syscon' node and the syscon node is in the soc node.

Best regards,
Xingyu Wu

Krzysztof Kozlowski Feb. 23, 2023, 9:04 a.m. UTC | #5

On 23/02/2023 10:03, Xingyu Wu wrote:
> On 2023/2/23 16:52, Krzysztof Kozlowski wrote:
>> On 23/02/2023 09:47, Xingyu Wu wrote:
>>> On 2023/2/22 17:09, Krzysztof Kozlowski wrote:
>>>> On 21/02/2023 15:11, Xingyu Wu wrote:
>>>>> Add the PLL clock node for the Starfive JH7110 SoC and
>>>>> modify the SYSCRG node to add PLL clocks.
>>>>>
>>>>> Signed-off-by: Xingyu Wu <xingyu.wu@starfivetech.com>
>>>>> ---
>>>>>  arch/riscv/boot/dts/starfive/jh7110.dtsi | 15 +++++++++++++--
>>>>>  1 file changed, 13 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>>> index b6612c53d0d2..0cb8d86ebce5 100644
>>>>> --- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>>> +++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
>>>>> @@ -461,12 +461,16 @@ syscrg: clock-controller@13020000 {
>>>>>  				 <&gmac1_rgmii_rxin>,
>>>>>  				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
>>>>>  				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
>>>>> -				 <&tdm_ext>, <&mclk_ext>;
>>>>> +				 <&tdm_ext>, <&mclk_ext>,
>>>>> +				 <&pllclk JH7110_CLK_PLL0_OUT>,
>>>>> +				 <&pllclk JH7110_CLK_PLL1_OUT>,
>>>>> +				 <&pllclk JH7110_CLK_PLL2_OUT>;
>>>>>  			clock-names = "osc", "gmac1_rmii_refin",
>>>>>  				      "gmac1_rgmii_rxin",
>>>>>  				      "i2stx_bclk_ext", "i2stx_lrck_ext",
>>>>>  				      "i2srx_bclk_ext", "i2srx_lrck_ext",
>>>>> -				      "tdm_ext", "mclk_ext";
>>>>> +				      "tdm_ext", "mclk_ext",
>>>>> +				      "pll0_out", "pll1_out", "pll2_out";
>>>>>  			#clock-cells = <1>;
>>>>>  			#reset-cells = <1>;
>>>>>  		};
>>>>> @@ -476,6 +480,13 @@ sys_syscon: syscon@13030000 {
>>>>>  			reg = <0x0 0x13030000 0x0 0x1000>;
>>>>>  		};
>>>>>  
>>>>> +		pllclk: pll-clock-controller {
>>>>
>>>> Does not look like you tested the DTS against bindings. Please run `make
>>>> dtbs_check` (see Documentation/devicetree/bindings/writing-schema.rst
>>>> for instructions). You should see here warnings of mixing non-MMIO nodes
>>>> in MMIO-bus.
>>>>
>>>
>>> Oh I cherry-pick the commit of syscon node and it also include the MMC node.
>>> I will remove the MMC node. 
>>> I used dtbs_check and get the error 'should not be valid under {'type': 'object'}',
>>> If I move this node out of the 'soc' node, the dtbs_check will be pass.
>>> Is it OK to move the PLL node out of the 'soc' node? Thanks.
>>
>> Shall it be out side of soc? How it can then do anything with registers?
>> This does not look like correct representation of hardware.
> 
> The error appears to be due to a lack of reg base about PLL node. PLL do something with register
> by 'sys_syscon' node and the syscon node is in the soc node.

Again: And how is this correct representation of hardware?

Best regards,
Krzysztof

diff mbox series

Patch

diff --git a/arch/riscv/boot/dts/starfive/jh7110.dtsi b/arch/riscv/boot/dts/starfive/jh7110.dtsi
index b6612c53d0d2..0cb8d86ebce5 100644
--- a/arch/riscv/boot/dts/starfive/jh7110.dtsi
+++ b/arch/riscv/boot/dts/starfive/jh7110.dtsi
@@ -461,12 +461,16 @@  syscrg: clock-controller@13020000 {
 				 <&gmac1_rgmii_rxin>,
 				 <&i2stx_bclk_ext>, <&i2stx_lrck_ext>,
 				 <&i2srx_bclk_ext>, <&i2srx_lrck_ext>,
-				 <&tdm_ext>, <&mclk_ext>;
+				 <&tdm_ext>, <&mclk_ext>,
+				 <&pllclk JH7110_CLK_PLL0_OUT>,
+				 <&pllclk JH7110_CLK_PLL1_OUT>,
+				 <&pllclk JH7110_CLK_PLL2_OUT>;
 			clock-names = "osc", "gmac1_rmii_refin",
 				      "gmac1_rgmii_rxin",
 				      "i2stx_bclk_ext", "i2stx_lrck_ext",
 				      "i2srx_bclk_ext", "i2srx_lrck_ext",
-				      "tdm_ext", "mclk_ext";
+				      "tdm_ext", "mclk_ext",
+				      "pll0_out", "pll1_out", "pll2_out";
 			#clock-cells = <1>;
 			#reset-cells = <1>;
 		};
@@ -476,6 +480,13 @@  sys_syscon: syscon@13030000 {
 			reg = <0x0 0x13030000 0x0 0x1000>;
 		};
 
+		pllclk: pll-clock-controller {
+			compatible = "starfive,jh7110-pll";
+			clocks = <&osc>;
+			#clock-cells = <1>;
+			starfive,sysreg = <&sys_syscon>;
+		};
+
 		sysgpio: pinctrl@13040000 {
 			compatible = "starfive,jh7110-sys-pinctrl";
 			reg = <0x0 0x13040000 0x0 0x10000>;