Re: [PATCH v2 4/6] phy: realtek: usb2: introduce reset controller struct

[Rustam Adilov]

On 2026-03-30 21:39, Vladimir Oltean wrote:
> On Fri, Mar 27, 2026 at 09:06:36PM +0500, Rustam Adilov wrote:
>> In RTL9607C, there is so called "IP Enable Controller" which resemble
>> reset controller with reset lines and is used for various things like
>> USB, PCIE, GMAC and such.
>> 
>> Introduce the reset_control struct to this driver to handle deasserting
>> usb2 phy reset line.
>> 
>> Make use of the function devm_reset_control_array_get_optional_exclusive()
>> function to get the reset controller and since existing RTD SoCs don't
>> specify the resets we can have a cleaner code.
>> 
>> Co-developed-by: Michael Zavertkin <misha.zavertkin@xxxxxxx>
>> Signed-off-by: Michael Zavertkin <misha.zavertkin@xxxxxxx>
>> Signed-off-by: Rustam Adilov <adilov@xxxxxxxxxxx>
>> ---
>>  drivers/phy/realtek/phy-rtk-usb2.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>> 
>> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
>> index e65b8525b88b..070cba1e0e0a 100644
>> --- a/drivers/phy/realtek/phy-rtk-usb2.c
>> +++ b/drivers/phy/realtek/phy-rtk-usb2.c
>> @@ -17,6 +17,7 @@
>>  #include <linux/sys_soc.h>
>>  #include <linux/mfd/syscon.h>
>>  #include <linux/phy/phy.h>
>> +#include <linux/reset.h>
>>  #include <linux/usb.h>
>>  
>>  /* GUSB2PHYACCn register */
>> @@ -130,6 +131,7 @@ struct rtk_phy {
>>  	struct phy_cfg *phy_cfg;
>>  	int num_phy;
>>  	struct phy_parameter *phy_parameter;
>> +	struct reset_control *phy_rst;
>>  
>>  	struct dentry *debug_dir;
>>  };
>> @@ -602,6 +604,10 @@ static int do_rtk_phy_init(struct rtk_phy *rtk_phy, int index)
>>  	phy_parameter = &((struct phy_parameter *)rtk_phy->phy_parameter)[index];
>>  	phy_reg = &phy_parameter->phy_reg;
>>  
>> +	reset_control_deassert(rtk_phy->phy_rst);
> 
> LLM review says:
> 
> (less important)
> Can reset_control_deassert() fail here? If there is a hardware communication
> error with the reset controller, should this check the return value and
> propagate the error up instead of proceeding to configure the PHY?
> Additionally, since the exclusive reset line is deasserted here, does this
> code need a corresponding reset_control_assert() in the driver's teardown
> or exit path? Leaving the IP block permanently enabled after shutdown could
> lead to power leaks and prevent proper hardware re-initialization.

It realistically shouldn't fail. But I can add the return error for this.
And no, it doesn't need the reset_control_assert.

>> +
>> +	mdelay(5);
> 
> (more important)
> This code unnecessarily penalizes existing platforms. If rtk_phy->phy_rst
> is NULL (as on older platforms where the optional reset is not defined), the
> delay still executes.
> 
> Also, since PHY initialization callbacks run in a sleepable context, would it
> be better to use a sleep-based delay like usleep_range(5000, 6000) to yield
> the CPU instead of busy-waiting with mdelay(5)?

I can change mdelay to msleep and wrap it around something like if (rtk_phy->phy_rst).

>> +
>>  	if (phy_cfg->use_default_parameter) {
>>  		dev_dbg(rtk_phy->dev, "%s phy#%d use default parameter\n",
>>  			__func__, index);
>> @@ -1069,6 +1075,12 @@ static int rtk_usb2phy_probe(struct platform_device *pdev)
>>  
>>  	rtk_phy->num_phy = phy_cfg->num_phy;
>>  
>> +	rtk_phy->phy_rst = devm_reset_control_array_get_optional_exclusive(dev);
>> +	if (IS_ERR(rtk_phy->phy_rst)) {
>> +		dev_err(dev, "usb2 phy resets are not working\n");
>> +		return PTR_ERR(rtk_phy->phy_rst);
>> +	}
>> +
> 
> (still LLM review)
> If the reset controller driver is not yet ready, this will return
> -EPROBE_DEFER and print an error message to the kernel log.
> Should this use dev_err_probe() to silently handle probe deferral while
> correctly logging actual errors?

I can change it to dev_err_probe, no problem with that.

>>  	ret = parse_phy_data(rtk_phy);
>>  	if (ret)
>>  		goto err;
>> -- 
>> 2.53.0
>> 
>>