[PATCH v2 0/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[PATCH v2 0/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Chukun Pan]

Changes in v2:
  Matches the '-ENOENT' error.
  Use devm_reset_control_get_exclusive.

Chukun Pan (1):
  phy: rockchip: naneng-combphy: compatible reset with old DT

 drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
2.25.1

[PATCH v2 1/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Chukun Pan]

The device tree of RK3568 did not specify reset-names before.
So add fallback to old behaviour to be compatible with old DT.

Fixes: fbcbffbac994 ("phy: rockchip: naneng-combphy: fix phy reset")
Cc: Jianfeng Liu <liujianfeng1994@gmail.com>
Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
---
 drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
index a1532ef8bbe9..8c3ce57f8915 100644
--- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
+++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
@@ -324,7 +324,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");
+	priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
+	/* fallback to old behaviour */
+	if (PTR_ERR(priv->phy_rst) == -ENOENT)
+		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");
 
-- 
2.25.1

Re: [PATCH v2 1/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Jonas Karlman]

Hi Vinod,

On 2025-01-06 11:00, Chukun Pan wrote:
> The device tree of RK3568 did not specify reset-names before.
> So add fallback to old behaviour to be compatible with old DT.
> 
> Fixes: fbcbffbac994 ("phy: rockchip: naneng-combphy: fix phy reset")

The commit fbcbffbac994 ("phy: rockchip: naneng-combphy: fix phy reset")
broke backwards compatibly with old DTs and has already been backported
breaking i.e. PCIe and USB on Rockchip RK356x boards on stable and
longterm kernels.

E.g with v6.12.9 on a Radxa ROCK 3A (RK3568) board:

  rockchip-naneng-combphy fe830000.phy: error -ENOENT: failed to get phy reset
  rockchip-naneng-combphy fe830000.phy: probe with driver rockchip-naneng-combphy failed with error -2
  rockchip-naneng-combphy fe840000.phy: error -ENOENT: failed to get phy reset
  rockchip-naneng-combphy fe840000.phy: probe with driver rockchip-naneng-combphy failed with error -2
  rockchip-naneng-combphy fe820000.phy: error -ENOENT: failed to get phy reset
  rockchip-naneng-combphy fe820000.phy: probe with driver rockchip-naneng-combphy failed with error -2

  # cat /sys/kernel/debug/devices_deferred
  3c0000000.pcie  platform: supplier fe840000.phy not ready
  fcc00000.usb    platform: supplier fe820000.phy not ready
  fd000000.usb    platform: supplier fe830000.phy not ready

Please revert the commit fbcbffbac994 ("phy: rockchip: naneng-combphy:
fix phy reset") or merge this patch to restore PCIe and USB RK356x.

> Cc: Jianfeng Liu <liujianfeng1994@gmail.com>
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>

With this patch PCIe and USB start working again on same board.

Reviewed-by: Jonas Karlman <jonas@kwiboo.se>

Regards,
Jonas

> ---
>  drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index a1532ef8bbe9..8c3ce57f8915 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -324,7 +324,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");
> +	priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
> +	/* fallback to old behaviour */
> +	if (PTR_ERR(priv->phy_rst) == -ENOENT)
> +		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");
>  

Re: [PATCH v2 1/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Chukun Pan]

Hi,
> The commit fbcbffbac994 ("phy: rockchip: naneng-combphy: fix phy reset")
> broke backwards compatibly with old DTs and has already been backported
> breaking i.e. PCIe and USB on Rockchip RK356x boards on stable and
> longterm kernels.

The dts changes have been merged into v6.13-rc7,
maybe it can be sent to linux-stable.

Thanks,
Chukun

-- 
2.25.1

Re: [PATCH v2 1/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Jonas Karlman]

Hi Chukun,

On 2025-01-15 05:00, Chukun Pan wrote:
> Hi,
>> The commit fbcbffbac994 ("phy: rockchip: naneng-combphy: fix phy reset")
>> broke backwards compatibly with old DTs and has already been backported
>> breaking i.e. PCIe and USB on Rockchip RK356x boards on stable and
>> longterm kernels.
> 
> The dts changes have been merged into v6.13-rc7,
> maybe it can be sent to linux-stable.

The original commit broke ABI and does not allow upgrading Linux using
an existing device tree, even if the DT patch adding reset-names props
also is backported you still have uses in the wild e.g. U-Boot v2024.10
and newer that comes bundled with an unpatched DT.

Trying to boot into Linux v6.1.123+, v6.6.69+ and v6.12.8+ using the 
U-Boot bundled DT for RK356x boards now result in broken PCIe, SATA and
USB thanks to the offending commit fbcbffbac994 ("phy: rockchip:
naneng-combphy: fix phy reset").

Regards,
Jonas

> 
> Thanks,
> Chukun
> 

Re: [PATCH v2 1/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Vinod Koul]

On 06-01-25, 18:00, Chukun Pan wrote:
> The device tree of RK3568 did not specify reset-names before.
> So add fallback to old behaviour to be compatible with old DT.
> 
> Fixes: fbcbffbac994 ("phy: rockchip: naneng-combphy: fix phy reset")
> Cc: Jianfeng Liu <liujianfeng1994@gmail.com>
> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
> ---
>  drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> index a1532ef8bbe9..8c3ce57f8915 100644
> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
> @@ -324,7 +324,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");
> +	priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
> +	/* fallback to old behaviour */
> +	if (PTR_ERR(priv->phy_rst) == -ENOENT)
> +		priv->phy_rst = devm_reset_control_array_get_exclusive(dev);

That is not *same* behaviour on fallback, why _exclusive() for the
fallback?

>  	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

-- 
~Vinod

Re: [PATCH v2 1/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Jonas Karlman]

Hi Vinod,

On 2025-01-30 06:57, Vinod Koul wrote:
> On 06-01-25, 18:00, Chukun Pan wrote:
>> The device tree of RK3568 did not specify reset-names before.
>> So add fallback to old behaviour to be compatible with old DT.
>>
>> Fixes: fbcbffbac994 ("phy: rockchip: naneng-combphy: fix phy reset")
>> Cc: Jianfeng Liu <liujianfeng1994@gmail.com>
>> Signed-off-by: Chukun Pan <amadeus@jmu.edu.cn>
>> ---
>>  drivers/phy/rockchip/phy-rockchip-naneng-combphy.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>> index a1532ef8bbe9..8c3ce57f8915 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-combphy.c
>> @@ -324,7 +324,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");
>> +	priv->phy_rst = devm_reset_control_get_exclusive(dev, "phy");
>> +	/* fallback to old behaviour */
>> +	if (PTR_ERR(priv->phy_rst) == -ENOENT)
>> +		priv->phy_rst = devm_reset_control_array_get_exclusive(dev);
> 
> That is not *same* behaviour on fallback, why _exclusive() for the
> fallback?

Prior to the commit fd3ac6e80497 ("dt-bindings: phy: rockchip: rk3588
has two reset lines") the dt-binding did not require reset-names and it
still does not for the rockchip,rk3568-naneng-combphy compatible. The
binding documentation also mentioned "exclusive PHY reset line" for the
resets prop.

The commit fbcbffbac994 ("phy: rockchip: naneng-combphy: fix phy reset")
broke backward compatibility with the rockchip,rk3568-naneng-combphy
compatible by requiring existence of a "phy" in reset-names.

This patch restores the old behavior prior to fbcbffbac994 ("phy:
rockchip: naneng-combphy: fix phy reset") when
devm_reset_control_array_get_exclusive() was used to restore
compatibility with the rockchip,rk3568-naneng-combphy dt-binding.

Regards,
Jonas

> 
>>  	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 v2 0/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Vinod Koul]

On Mon, 06 Jan 2025 18:00:00 +0800, Chukun Pan wrote:
> Changes in v2:
>   Matches the '-ENOENT' error.
>   Use devm_reset_control_get_exclusive.
> 
> Chukun Pan (1):
>   phy: rockchip: naneng-combphy: compatible reset with old DT
> 
> [...]

Applied, thanks!

[1/1] phy: rockchip: naneng-combphy: compatible reset with old DT
      commit: bff68d44135ce6714107e2f72069a79476c8073d

Best regards,
-- 
~Vinod

Re: [PATCH v2 0/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Valentin Kleibel]

Hi,

>> Chukun Pan (1):
>>    phy: rockchip: naneng-combphy: compatible reset with old DT
>>
>> [...]
> 
> Applied, thanks!
> 
> [1/1] phy: rockchip: naneng-combphy: compatible reset with old DT
>        commit: bff68d44135ce6714107e2f72069a79476c8073d

Thanks for your work!
We found your patch after NVMes stopped working on a rock 3A with newer 
kernels and successfully applied it to kernel 6.1.128 (currently in debian).

As the commit breaking NVMe functionality on rk3568 platforms 
(fd3ac6e80497 "phy: rockchip: naneng-combphy: fix phy reset") was 
already backported to 6.1.y, 6.6.y, 6.12.y it would be great to have 
this fix in these releases as well.

Best regards,
Valentin

Re: [PATCH v2 0/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 642 bytes --]

On Tue Feb 11, 2025 at 2:03 PM CET, Valentin Kleibel wrote:
>>> Chukun Pan (1):
>>>    phy: rockchip: naneng-combphy: compatible reset with old DT
>> 
>> Applied, thanks!
>> 
>> [1/1] phy: rockchip: naneng-combphy: compatible reset with old DT
>>        commit: bff68d44135ce6714107e2f72069a79476c8073d
>
> Thanks for your work!
> We found your patch after NVMes stopped working on a rock 3A with newer 
> kernels and successfully applied it to kernel 6.1.128 (currently in debian).

FTR: I've reported it in Debian here: https://bugs.debian.org/1098250

I confirmed it (also) broke on kernel 6.12.8-1.

Cheers,
  Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Salvatore Bonaccorso]

Hi,

On Tue, Feb 18, 2025 at 12:45:34PM +0100, Diederik de Haas wrote:
> On Tue Feb 11, 2025 at 2:03 PM CET, Valentin Kleibel wrote:
> >>> Chukun Pan (1):
> >>>    phy: rockchip: naneng-combphy: compatible reset with old DT
> >> 
> >> Applied, thanks!
> >> 
> >> [1/1] phy: rockchip: naneng-combphy: compatible reset with old DT
> >>        commit: bff68d44135ce6714107e2f72069a79476c8073d
> >
> > Thanks for your work!
> > We found your patch after NVMes stopped working on a rock 3A with newer 
> > kernels and successfully applied it to kernel 6.1.128 (currently in debian).
> 
> FTR: I've reported it in Debian here: https://bugs.debian.org/1098250
> 
> I confirmed it (also) broke on kernel 6.12.8-1.

FWIW, we have several users in Debian reporting the problem, so if it
can be applied to mainline and then flow down to one of the next round
of stable series down to 6.1.y that would be highly appreicated.

Diederik, if you were able to test the fix, you might contribute a
Tested-by (although as I undestsand its not strictly needed at this
point as commit should be on way to mainline and stable series)?

Regards,
Salvatore

Re: [PATCH v2 0/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 1786 bytes --]

On Fri Feb 21, 2025 at 8:18 AM CET, Salvatore Bonaccorso wrote:
> On Tue, Feb 18, 2025 at 12:45:34PM +0100, Diederik de Haas wrote:
>> On Tue Feb 11, 2025 at 2:03 PM CET, Valentin Kleibel wrote:
>> >>> Chukun Pan (1):
>> >>>    phy: rockchip: naneng-combphy: compatible reset with old DT
>> >> 
>> >> Applied, thanks!
>> >> 
>> >> [1/1] phy: rockchip: naneng-combphy: compatible reset with old DT
>> >>        commit: bff68d44135ce6714107e2f72069a79476c8073d
>> >
>> > Thanks for your work!
>> > We found your patch after NVMes stopped working on a rock 3A with newer 
>> > kernels and successfully applied it to kernel 6.1.128 (currently in debian).
>> 
>> FTR: I've reported it in Debian here: https://bugs.debian.org/1098250
>> 
>> I confirmed it (also) broke on kernel 6.12.8-1.
>
> FWIW, we have several users in Debian reporting the problem, so if it
> can be applied to mainline and then flow down to one of the next round
> of stable series down to 6.1.y that would be highly appreicated.
>
> Diederik, if you were able to test the fix, you might contribute a
> Tested-by (although as I undestsand its not strictly needed at this
> point as commit should be on way to mainline and stable series)?

AFAIK a Tested-by could increase the chances of the maintainer accepting
the patch, but that has already happened (commit date: 2025-02-04).
The patch was already verified to fix the problem before submission:
https://lore.kernel.org/all/20250103171109.2726312-1-liujianfeng1994@gmail.com/

So AFAICT another Tested-by would either have no effect or a delaying
effect (adding it would change a bunch of commit IDs probably triggering
various CIs to run again).
I'm not aware of anything (else) I could do wrt this patch.

Cheers,
  Diederik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 0/1] phy: rockchip: naneng-combphy: compatible reset with old DT

[Heiko Stübner]

Am Freitag, 21. Februar 2025, 09:57:14 MEZ schrieb Diederik de Haas:
> On Fri Feb 21, 2025 at 8:18 AM CET, Salvatore Bonaccorso wrote:
> > On Tue, Feb 18, 2025 at 12:45:34PM +0100, Diederik de Haas wrote:
> >> On Tue Feb 11, 2025 at 2:03 PM CET, Valentin Kleibel wrote:
> >> >>> Chukun Pan (1):
> >> >>>    phy: rockchip: naneng-combphy: compatible reset with old DT
> >> >> 
> >> >> Applied, thanks!
> >> >> 
> >> >> [1/1] phy: rockchip: naneng-combphy: compatible reset with old DT
> >> >>        commit: bff68d44135ce6714107e2f72069a79476c8073d
> >> >
> >> > Thanks for your work!
> >> > We found your patch after NVMes stopped working on a rock 3A with newer 
> >> > kernels and successfully applied it to kernel 6.1.128 (currently in debian).
> >> 
> >> FTR: I've reported it in Debian here: https://bugs.debian.org/1098250
> >> 
> >> I confirmed it (also) broke on kernel 6.12.8-1.
> >
> > FWIW, we have several users in Debian reporting the problem, so if it
> > can be applied to mainline and then flow down to one of the next round
> > of stable series down to 6.1.y that would be highly appreicated.
> >
> > Diederik, if you were able to test the fix, you might contribute a
> > Tested-by (although as I undestsand its not strictly needed at this
> > point as commit should be on way to mainline and stable series)?
> 
> AFAIK a Tested-by could increase the chances of the maintainer accepting
> the patch, but that has already happened (commit date: 2025-02-04).
> The patch was already verified to fix the problem before submission:
> https://lore.kernel.org/all/20250103171109.2726312-1-liujianfeng1994@gmail.com/
> 
> So AFAICT another Tested-by would either have no effect or a delaying
> effect (adding it would change a bunch of commit IDs probably triggering
> various CIs to run again).
> I'm not aware of anything (else) I could do wrt this patch.

the patch already is in the fixes-branch of the phy-tree [0], sho should
make its way into 6.14-rc shortly.

Heiko

[0] https://git.kernel.org/pub/scm/linux/kernel/git/phy/linux-phy.git/log/?h=fixes