[PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Chukun Pan]

The reset-names of combphy are missing, add it.

Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 arch/arm64/boot/dts/rockchip/rk3568.dtsi      | 1 +
 arch/arm64/boot/dts/rockchip/rk356x-base.dtsi | 2 ++
 2 files changed, 3 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
index ecaefe208e3e..695cccbdab0f 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
@@ -350,6 +350,7 @@ combphy0: phy@fe820000 {
 		assigned-clocks = <&pmucru CLK_PCIEPHY0_REF>;
 		assigned-clock-rates = <100000000>;
 		resets = <&cru SRST_PIPEPHY0>;
+		reset-names = "phy";
 		rockchip,pipe-grf = <&pipegrf>;
 		rockchip,pipe-phy-grf = <&pipe_phy_grf0>;
 		#phy-cells = <1>;
diff --git a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
index 62be06f3b863..e55390629114 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x-base.dtsi
@@ -1681,6 +1681,7 @@ combphy1: phy@fe830000 {
 		assigned-clocks = <&pmucru CLK_PCIEPHY1_REF>;
 		assigned-clock-rates = <100000000>;
 		resets = <&cru SRST_PIPEPHY1>;
+		reset-names = "phy";
 		rockchip,pipe-grf = <&pipegrf>;
 		rockchip,pipe-phy-grf = <&pipe_phy_grf1>;
 		#phy-cells = <1>;
@@ -1697,6 +1698,7 @@ combphy2: phy@fe840000 {
 		assigned-clocks = <&pmucru CLK_PCIEPHY2_REF>;
 		assigned-clock-rates = <100000000>;
 		resets = <&cru SRST_PIPEPHY2>;
+		reset-names = "phy";
 		rockchip,pipe-grf = <&pipegrf>;
 		rockchip,pipe-phy-grf = <&pipe_phy_grf2>;
 		#phy-cells = <1>;
-- 
2.25.1

[PATCH 2/2] phy: rockchip: naneng-combphy: fix phy reset

[Chukun Pan]

Currently, the USB port via combophy on the RK3528/RK3588 SoC is broken.

  usb usb8-port1: Cannot enable. Maybe the USB cable is bad?

This is due to the combphy of RK3528/RK3588 SoC has multiple resets, but
only "phy resets" need assert and deassert, "apb resets" don't need.
So change the driver to only match the phy resets, which is also what
the vendor kernel does.

Fixes: 7160820d742a ("phy: rockchip: add naneng combo phy for RK3568")
Cc: FUKAUMI Naoki <naoki@radxa.com>
Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index 0a9989e41237..2eb3329ca23f 100644
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
@@ -309,7 +309,7 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
 
 	priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
 
-	priv->phy_rst = devm_reset_control_array_get_exclusive(dev);
+	priv->phy_rst = devm_reset_control_get(dev, "phy");
 	if (IS_ERR(priv->phy_rst))
 		return dev_err_probe(dev, PTR_ERR(priv->phy_rst), "failed to get phy reset\n");
 
-- 
2.25.1

Re: [PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Heiko Stuebner]

Am Freitag, 22. November 2024, 08:30:05 CET schrieb Chukun Pan:
> The reset-names of combphy are missing, add it.
> 
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>

Fixes: fd3ac6e80497 ("dt-bindings: phy: rockchip: rk3588 has two reset lines")

It does looks like, before that patch above, the phy did not use
nor require reset-names?

No need to resend for this, as I think b4 will just pick up this
addition when I grab the series from the list.

Re: [PATCH 2/2] phy: rockchip: naneng-combphy: fix phy reset

[Heiko Stuebner]

Am Freitag, 22. November 2024, 08:30:06 CET schrieb Chukun Pan:
> Currently, the USB port via combophy on the RK3528/RK3588 SoC is broken.
> 
>   usb usb8-port1: Cannot enable. Maybe the USB cable is bad?
> 
> This is due to the combphy of RK3528/RK3588 SoC has multiple resets, but
> only "phy resets" need assert and deassert, "apb resets" don't need.
> So change the driver to only match the phy resets, which is also what
> the vendor kernel does.
> 
> Fixes: 7160820d742a ("phy: rockchip: add naneng combo phy for RK3568")
> Cc: FUKAUMI Naoki <naoki@radxa.com>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
>  drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index 0a9989e41237..2eb3329ca23f 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -309,7 +309,7 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
>  
>  	priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
>  
> -	priv->phy_rst = devm_reset_control_array_get_exclusive(dev);
> +	priv->phy_rst = devm_reset_control_get(dev, "phy");
>  	if (IS_ERR(priv->phy_rst))
>  		return dev_err_probe(dev, PTR_ERR(priv->phy_rst), "failed to get phy reset\n");

That reset-control is deasserted in the driver in a place where
already parts of the phy are supposed to be configured, so it
wiggling the apb_reset at those places (i.e. after running
priv->cfg->combphy_cfg()) probably causes strange effects, so

Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Heiko Stuebner]

Am Freitag, 22. November 2024, 08:56:17 CET schrieb Heiko Stuebner:
> No need to resend for this, as I think b4 will just pick up this
> addition when I grab the series from the list.

Of course, I'll just grab this patch, not the series.
The phy driver change will go through the phy tree :-)

Re: [PATCH 2/2] phy: rockchip: naneng-combphy: fix phy reset

[FUKAUMI Naoki]

Hi,

On 11/22/24 16:30, Chukun Pan wrote:
> Currently, the USB port via combophy on the RK3528/RK3588 SoC is broken.
> 
>    usb usb8-port1: Cannot enable. Maybe the USB cable is bad?
> 
> This is due to the combphy of RK3528/RK3588 SoC has multiple resets, but
> only "phy resets" need assert and deassert, "apb resets" don't need.
> So change the driver to only match the phy resets, which is also what
> the vendor kernel does.
> 
> Fixes: 7160820d742a ("phy: rockchip: add naneng combo phy for RK3568")
> Cc: FUKAUMI Naoki <naoki@radxa.com>
> Cc: Michael Zimmermann <sigmaepsilon92@gmail.com>
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> Reviewed-by: Heiko Stuebner <heiko@sntech.de>

Tested-by: FUKAUMI Naoki <naoki@radxa.com>

Best regards,

--
FUKAUMI Naoki
Radxa Computer (Shenzhen) Co., Ltd.

> ---
>   drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index 0a9989e41237..2eb3329ca23f 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -309,7 +309,7 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
>   
>   	priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
>   
> -	priv->phy_rst = devm_reset_control_array_get_exclusive(dev);
> +	priv->phy_rst = devm_reset_control_get(dev, "phy");
>   	if (IS_ERR(priv->phy_rst))
>   		return dev_err_probe(dev, PTR_ERR(priv->phy_rst), "failed to get phy reset\n");
>   

Re: (subset) [PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Heiko Stuebner]

On Fri, 22 Nov 2024 15:30:05 +0800, Chukun Pan wrote:
> The reset-names of combphy are missing, add it.
> 
> 

Applied, thanks!

[1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy
      commit: 8b9c12757f919157752646faf3821abf2b7d2a64

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>

Re: (subset) [PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Vinod Koul]

On Fri, 22 Nov 2024 15:30:05 +0800, Chukun Pan wrote:
> The reset-names of combphy are missing, add it.
> 
> 

Applied, thanks!

[2/2] phy: rockchip: naneng-combphy: fix phy reset
      commit: fbcbffbac994aca1264e3c14da96ac9bfd90466e

Best regards,
-- 
~Vinod

Re: (subset) [PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Jianfeng Liu]

Hi,

On Sun, 08 Dec 2024 22:28:48 +0530, Vinod Koul wrote:
>Applied, thanks!
>
>[2/2] phy: rockchip: naneng-combphy: fix phy reset
>      commit: fbcbffbac994aca1264e3c14da96ac9bfd90466e

The other patch in this series:
arm64: dts: rockchip: rk3568: add reset-names for combphy
is missing in v6.13-rc5, which will break pcie2 of rk3568 because the phy
driver has changed.

Best regards,
Jianfeng

Re: (subset) [PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Jonas Karlman]

On 2025-01-02 16:16, Jianfeng Liu wrote:
> Hi,
> 
> On Sun, 08 Dec 2024 22:28:48 +0530, Vinod Koul wrote:
>> Applied, thanks!
>>
>> [2/2] phy: rockchip: naneng-combphy: fix phy reset
>>      commit: fbcbffbac994aca1264e3c14da96ac9bfd90466e
> 
> The other patch in this series:
> arm64: dts: rockchip: rk3568: add reset-names for combphy
> is missing in v6.13-rc5, which will break pcie2 of rk3568 because the phy
> driver has changed.

Yes, the commit fbcbffbac994 ("phy: rockchip: naneng-combphy: fix phy
reset") breaks backward compatibility with existing DTs and this commit
should probably be reverted or driver need a fix that fall back to old
behavior in case DT is missing reset-names.

Regards,
Jonas

> 
> Best regards,
> Jianfeng
> 

Re: [PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Chukun Pan]

Hi,
> The other patch in this series:
> arm64: dts: rockchip: rk3568: add reset-names for combphy
> is missing in v6.13-rc5, which will break pcie2 of rk3568
> because the phy driver has changed.

I don't know why the dts patch hasn't been merged into the mainline yet.
Maybe the merge window is different. Can you test the following patch?

```
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
@@ -325,6 +325,10 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
 	priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
 
 	priv->phy_rst = devm_reset_control_get(dev, "phy");
+	/* fallback to old behaviour */
+	if (IS_ERR_OR_NULL(priv->phy_rst))
+		priv->phy_rst = devm_reset_control_array_get_exclusive(dev);
+
 	if (IS_ERR(priv->phy_rst))
 		return dev_err_probe(dev, PTR_ERR(priv->phy_rst), "failed to get phy reset\n");
 
```

Thanks,
Chukun

-- 
2.25.1

Re: [PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Heiko Stübner]

Hi,

Am Freitag, 3. Januar 2025, 04:30:16 CET schrieb Chukun Pan:
> > The other patch in this series:
> > arm64: dts: rockchip: rk3568: add reset-names for combphy
> > is missing in v6.13-rc5, which will break pcie2 of rk3568
> > because the phy driver has changed.
> 
> I don't know why the dts patch hasn't been merged into the mainline yet.

because it was christmas ;-) .

> Maybe the merge window is different. Can you test the following patch?

But in any case, the driver _must_ stay compatible with old devicetrees.
I.e. requiring a DT update is a general no-go.


> ```
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -325,6 +325,10 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
>  	priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
>  
>  	priv->phy_rst = devm_reset_control_get(dev, "phy");
> +	/* fallback to old behaviour */
> +	if (IS_ERR_OR_NULL(priv->phy_rst))
> +		priv->phy_rst = devm_reset_control_array_get_exclusive(dev);
> +

This looks like a reasonable fallback

>  	if (IS_ERR(priv->phy_rst))
>  		return dev_err_probe(dev, PTR_ERR(priv->phy_rst), "failed to get phy reset\n");


Heiko

Re: [PATCH 1/2] arm64: dts: rockchip: rk3568: add reset-names for combphy

[Jianfeng Liu]

Hi,

On Fri,  3 Jan 2025 11:30:16 +0800, Chukun Pan wrote:
>Maybe the merge window is different. Can you test the following patch?
>
>```
>--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>@@ -325,6 +325,10 @@ static int rockchip_combphy_parse_dt(struct device *dev, struct rockchip_combphy
> 	priv->ext_refclk = device_property_present(dev, "rockchip,ext-refclk");
> 
> 	priv->phy_rst = devm_reset_control_get(dev, "phy");
>+	/* fallback to old behaviour */
>+	if (IS_ERR_OR_NULL(priv->phy_rst))
>+		priv->phy_rst = devm_reset_control_array_get_exclusive(dev);
>+
> 	if (IS_ERR(priv->phy_rst))
> 		return dev_err_probe(dev, PTR_ERR(priv->phy_rst), "failed to get phy reset\n");
> 
>```

Tested with rock-3a. This patch will fix dtb without reset-names property.

Best regards,
Jianfeng