[PATCH v2 0/3] clk: rockchip: Fixed some incorrect commits

[PATCH v2 0/3] clk: rockchip: Fixed some incorrect commits

[Elaine Zhang]

Change in V2:
[PATCH v2 0/3]: NO change
[PATCH v2 1/3]: Update commit message
[PATCH v2 2/3]: Update commit message

Elaine Zhang (3):
  Revert "clk: rockchip: Set parent rate for DCLK_VOP clock on RK3228"
  Revert "arm64: dts: rockchip: Increase VOP clk rate on RK3328"
  arm64: dts: rockchip: Increase VOP clk rate on RK3328

 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 10 ++++++----
 drivers/clk/rockchip/clk-rk3228.c        |  2 +-
 2 files changed, 7 insertions(+), 5 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] Revert "clk: rockchip: Set parent rate for DCLK_VOP clock on RK3228"

[Elaine Zhang]

This reverts commit 1d34b9757523c1ad547bd6d040381f62d74a3189.

RK3228 Only GPLL and CPLL, GPLL is a common clock, does not allow
dclk_vop to change its frequency, CPLL is used by GMAC,
if dclk_vop use CLK_SET_RATE_PARENT and CLK_SET_RATE_NO_REPARENT flags will
affect the GMAC function.

If the client application does not use GMAC and CPLL is free, make this
change on the local branch.

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 drivers/clk/rockchip/clk-rk3228.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/clk/rockchip/clk-rk3228.c b/drivers/clk/rockchip/clk-rk3228.c
index ed602c27b624..9c0284607766 100644
--- a/drivers/clk/rockchip/clk-rk3228.c
+++ b/drivers/clk/rockchip/clk-rk3228.c
@@ -409,7 +409,7 @@ static struct rockchip_clk_branch rk3228_clk_branches[] __initdata = {
 			RK2928_CLKSEL_CON(29), 0, 3, DFLAGS),
 	DIV(0, "sclk_vop_pre", "sclk_vop_src", 0,
 			RK2928_CLKSEL_CON(27), 8, 8, DFLAGS),
-	MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT,
+	MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, 0,
 			RK2928_CLKSEL_CON(27), 1, 1, MFLAGS),
 
 	FACTOR(0, "xin12m", "xin24m", 0, 1, 2),
-- 
2.17.1

Re: [PATCH v2 1/3] Revert "clk: rockchip: Set parent rate for DCLK_VOP clock on RK3228"

[Alex Bee]

Hi Elaine,

Am 25.01.25 um 02:15 schrieb Elaine Zhang:
> This reverts commit 1d34b9757523c1ad547bd6d040381f62d74a3189.
> 
> RK3228 Only GPLL and CPLL, GPLL is a common clock, does not allow
> dclk_vop to change its frequency, CPLL is used by GMAC,
> if dclk_vop use CLK_SET_RATE_PARENT and CLK_SET_RATE_NO_REPARENT flags will
> affect the GMAC function.
> 
how do you come to this conclusion?

On the RK3228, hdmiphy is the default parent of dclk_vop, a clock that is
not even generated by the CRU but is the output of an external HDMI PHY.
The CLK_SET_RATE_NO_REPARENT flag ensures that the parent of dclk_vop never
changes to sclk_vop_pre (and thus never uses CPLL or GPLL). With
CLK_SET_RATE_PARENT we only ensure that we can use all rates of [0] since
there is no divider between dclk_vop and hdmiphy. Overall it is pretty much
the same situation as for RK3328, which handles these clocks in the same
way (see dclk_lcdc for RK3328).

Regards,
Alex

[0] [1] 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c?h=v6.13#n293
> If the client application does not use GMAC and CPLL is free, make this
> change on the local branch.
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
>   drivers/clk/rockchip/clk-rk3228.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/clk/rockchip/clk-rk3228.c b/drivers/clk/rockchip/clk-rk3228.c
> index ed602c27b624..9c0284607766 100644
> --- a/drivers/clk/rockchip/clk-rk3228.c
> +++ b/drivers/clk/rockchip/clk-rk3228.c
> @@ -409,7 +409,7 @@ static struct rockchip_clk_branch rk3228_clk_branches[] __initdata = {
>   			RK2928_CLKSEL_CON(29), 0, 3, DFLAGS),
>   	DIV(0, "sclk_vop_pre", "sclk_vop_src", 0,
>   			RK2928_CLKSEL_CON(27), 8, 8, DFLAGS),
> -	MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT,
> +	MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, 0,
>   			RK2928_CLKSEL_CON(27), 1, 1, MFLAGS),
>   
>   	FACTOR(0, "xin12m", "xin24m", 0, 1, 2),

Re: [PATCH v2 1/3] Revert "clk: rockchip: Set parent rate for DCLK_VOP clock on RK3228"

[Jonas Karlman]

Hi Elaine,

On 2025-01-25 06:38, Alex Bee wrote:
> Hi Elaine,
> 
> Am 25.01.25 um 02:15 schrieb Elaine Zhang:
>> This reverts commit 1d34b9757523c1ad547bd6d040381f62d74a3189.
>>
>> RK3228 Only GPLL and CPLL, GPLL is a common clock, does not allow
>> dclk_vop to change its frequency, CPLL is used by GMAC,
>> if dclk_vop use CLK_SET_RATE_PARENT and CLK_SET_RATE_NO_REPARENT flags will
>> affect the GMAC function.
>>
> how do you come to this conclusion?
> 
> On the RK3228, hdmiphy is the default parent of dclk_vop, a clock that is
> not even generated by the CRU but is the output of an external HDMI PHY.
> The CLK_SET_RATE_NO_REPARENT flag ensures that the parent of dclk_vop never
> changes to sclk_vop_pre (and thus never uses CPLL or GPLL). With
> CLK_SET_RATE_PARENT we only ensure that we can use all rates of [0] since
> there is no divider between dclk_vop and hdmiphy. Overall it is pretty much
> the same situation as for RK3328, which handles these clocks in the same
> way (see dclk_lcdc for RK3328).

I came to similar conclusion for v1 of this patch, see [2].

Maybe we should assign clk parent in DT similar to rk3328.dtsi to make
it extra clear that hdmiphy should be used as parent?

  assigned-clocks = <&cru DCLK_VOP>;
  assigned-clock-parents = <&cru SCLK_HDMI_PHY>;

Also for next revert patch you send, please include the patch author in
the recipient list :-)

[2] https://lore.kernel.org/all/d95d317c-5f6e-42bd-94a9-e1a6c7685e2f@kwiboo.se/

Regards,
Jonas

> 
> Regards,
> Alex
> 
> [0] [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/rockchip/phy-rockchip-inno-hdmi.c?h=v6.13#n293
>> If the client application does not use GMAC and CPLL is free, make this
>> change on the local branch.
>>
>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>> ---
>>   drivers/clk/rockchip/clk-rk3228.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/clk/rockchip/clk-rk3228.c b/drivers/clk/rockchip/clk-rk3228.c
>> index ed602c27b624..9c0284607766 100644
>> --- a/drivers/clk/rockchip/clk-rk3228.c
>> +++ b/drivers/clk/rockchip/clk-rk3228.c
>> @@ -409,7 +409,7 @@ static struct rockchip_clk_branch rk3228_clk_branches[] __initdata = {
>>   			RK2928_CLKSEL_CON(29), 0, 3, DFLAGS),
>>   	DIV(0, "sclk_vop_pre", "sclk_vop_src", 0,
>>   			RK2928_CLKSEL_CON(27), 8, 8, DFLAGS),
>> -	MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, CLK_SET_RATE_PARENT | CLK_SET_RATE_NO_REPARENT,
>> +	MUX(DCLK_VOP, "dclk_vop", mux_dclk_vop_p, 0,
>>   			RK2928_CLKSEL_CON(27), 1, 1, MFLAGS),
>>   
>>   	FACTOR(0, "xin12m", "xin24m", 0, 1, 2),
> 
> 

[PATCH v2 2/3] Revert "arm64: dts: rockchip: Increase VOP clk rate on RK3328"

[Elaine Zhang]

This reverts commit 0f2ddb128fa20f8441d903285632f2c69e90fae1.

Before changing the PLL frequency, in order to avoid overclocking the
child clock, set the child clock to a large div first, and then set the
CLK as required after the PLL is set.

Fixes: 0f2ddb128fa2 ("arm64: dts: rockchip: Increase VOP clk rate on RK3328")

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index 7d992c3c01ce..f3ef8cbfbdae 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -852,8 +852,8 @@
 			<0>, <24000000>,
 			<24000000>, <24000000>,
 			<15000000>, <15000000>,
-			<300000000>, <100000000>,
-			<400000000>, <100000000>,
+			<100000000>, <100000000>,
+			<100000000>, <100000000>,
 			<50000000>, <100000000>,
 			<100000000>, <100000000>,
 			<50000000>, <50000000>,
-- 
2.17.1

Re: [PATCH v2 2/3] Revert "arm64: dts: rockchip: Increase VOP clk rate on RK3328"

[Jonas Karlman]

Hi Elaine,

On 2025-01-25 02:15, Elaine Zhang wrote:
> This reverts commit 0f2ddb128fa20f8441d903285632f2c69e90fae1.
> 
> Before changing the PLL frequency, in order to avoid overclocking the
> child clock, set the child clock to a large div first, and then set the
> CLK as required after the PLL is set.

This commit message does not match what this patch does. In this patch
you revert a change and in next patch you re-introduce same thing
slightly different.

As mentioned in v1, see [1], you should merge both patches as a single
fix, if a fix really is needed.

Testing on a rk3328-rock64 I see no difference before or after these
changes. Please describe what this fixes because clk_summary show same
clock tree and rates before and after this fix.

Also for next revert patch you send, please include the patch author in
the recipient list :-)

[1] https://lore.kernel.org/all/cae9cb0a-1500-4fbc-bbf4-a6266549bcb9@kwiboo.se/

Regards,
Jonas

> 
> Fixes: 0f2ddb128fa2 ("arm64: dts: rockchip: Increase VOP clk rate on RK3328")
> 
> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> index 7d992c3c01ce..f3ef8cbfbdae 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> @@ -852,8 +852,8 @@
>  			<0>, <24000000>,
>  			<24000000>, <24000000>,
>  			<15000000>, <15000000>,
> -			<300000000>, <100000000>,
> -			<400000000>, <100000000>,
> +			<100000000>, <100000000>,
> +			<100000000>, <100000000>,
>  			<50000000>, <100000000>,
>  			<100000000>, <100000000>,
>  			<50000000>, <50000000>,

Re: [PATCH v2 2/3] Revert "arm64: dts: rockchip: Increase VOP clk rate on RK3328"

[zhangqing]

在 2025/1/26 5:39, Jonas Karlman 写道:
> Hi Elaine,
>
> On 2025-01-25 02:15, Elaine Zhang wrote:
>> This reverts commit 0f2ddb128fa20f8441d903285632f2c69e90fae1.
>>
>> Before changing the PLL frequency, in order to avoid overclocking the
>> child clock, set the child clock to a large div first, and then set the
>> CLK as required after the PLL is set.
> This commit message does not match what this patch does. In this patch
> you revert a change and in next patch you re-introduce same thing
> slightly different.
>
> As mentioned in v1, see [1], you should merge both patches as a single
> fix, if a fix really is needed.
Ok, This will be fixed in the next release.
>
> Testing on a rk3328-rock64 I see no difference before or after these
> changes. Please describe what this fixes because clk_summary show same
> clock tree and rates before and after this fix.
The clock tree might see the same result.

It is not safe or correct to set the child clock frequency before 
setting the PLL frequency, and the parent clock PLL frequency may not be 
the final expected value at this time.
Therefore, the correct setting of the child clock frequency should be 
after the PLL frequency is set.

The partial clock is set before the PLL setting frequency, which is to 
prevent the child clock from overclocking when the PLL is set, so ensure 
that the child clock is at a safe frequency before setting the PLL, and 
reset the child clock frequency after the PLL is set.
>
> Also for next revert patch you send, please include the patch author in
> the recipient list :-)
>
> [1] https://lore.kernel.org/all/cae9cb0a-1500-4fbc-bbf4-a6266549bcb9@kwiboo.se/
>
> Regards,
> Jonas
>
>> Fixes: 0f2ddb128fa2 ("arm64: dts: rockchip: Increase VOP clk rate on RK3328")
>>
>> Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
>> ---
>>   arch/arm64/boot/dts/rockchip/rk3328.dtsi | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> index 7d992c3c01ce..f3ef8cbfbdae 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> @@ -852,8 +852,8 @@
>>   			<0>, <24000000>,
>>   			<24000000>, <24000000>,
>>   			<15000000>, <15000000>,
>> -			<300000000>, <100000000>,
>> -			<400000000>, <100000000>,
>> +			<100000000>, <100000000>,
>> +			<100000000>, <100000000>,
>>   			<50000000>, <100000000>,
>>   			<100000000>, <100000000>,
>>   			<50000000>, <50000000>,
>
-- 
张晴
瑞芯微电子股份有限公司
Rockchip Electronics Co.,Ltd
地址：福建省福州市铜盘路软件大道89号软件园A区21号楼
Add:No.21 Building, A District, No.89 Software Boulevard Fuzhou, Fujian 350003, P.R.China
Tel:+86-0591-83991906-8601
邮编：350003
E-mail:elaine.zhang@rock-chips.com
****************************************************************************
保密提示：本邮件及其附件含有机密信息，仅发送给本邮件所指特定收件人。若非该特定收件人，请勿复制、使用或披露本邮件的任何内容。若误收本邮件，请从系统中永久性删除本邮件及所有附件，并以回复邮件或其他方式即刻告知发件人。福州瑞芯微电子有限公司拥有本邮件信息的著作权及解释权，禁止任何未经授权许可的侵权行为。

IMPORTANT NOTICE: This email is from Fuzhou Rockchip Electronics Co., Ltd .The contents of this email and any attachments may contain information that is privileged, confidential and/or exempt from disclosure under applicable law and relevant NDA. If you are not the intended recipient, you are hereby notified that any disclosure, copying, distribution, or use of the information is STRICTLY PROHIBITED. Please immediately contact the sender as soon as possible and destroy the material in its entirety in any format. Thank you.

****************************************************************************

[PATCH v2 3/3] arm64: dts: rockchip: Increase VOP clk rate on RK3328

[Elaine Zhang]

The VOP on RK3328 needs to run at a higher rate in order to produce
a proper 3840x2160 signal.
Change to use 300MHz for VIO clk and 400MHz for VOP clk.

Fixes: 0f2ddb128fa2 ("arm64: dts: rockchip: Increase VOP clk rate on RK3328")

Signed-off-by: Elaine Zhang <zhangqing@rock-chips.com>
---
 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index f3ef8cbfbdae..0c905f411e92 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -842,7 +842,8 @@
 			<&cru ACLK_BUS_PRE>, <&cru HCLK_BUS_PRE>,
 			<&cru PCLK_BUS_PRE>, <&cru ACLK_PERI_PRE>,
 			<&cru HCLK_PERI>, <&cru PCLK_PERI>,
-			<&cru SCLK_RTC32K>;
+			<&cru SCLK_RTC32K>, <&cru ACLK_VIO_PRE>,
+			<&cru ACLK_VOP_PRE>;
 		assigned-clock-parents =
 			<&cru HDMIPHY>, <&cru PLL_APLL>,
 			<&cru PLL_GPLL>, <&xin24m>,
@@ -863,7 +864,8 @@
 			<150000000>, <75000000>,
 			<75000000>, <150000000>,
 			<75000000>, <75000000>,
-			<32768>;
+			<32768>, <300000000>,
+			<400000000>;
 	};
 
 	usb2phy_grf: syscon@ff450000 {
-- 
2.17.1