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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The newly introduced PHY reset is deasserted too late (in `rtk_phy_init()`), causing fatal MMIO register accesses during `probe()` if the static configuration omits explicit initializers.
--

--- Patch [4]: [PATCH v7 4/6] phy: realtek: usb2: introduce reset controller struct ---
commit f14473a9b55e70977d451ec39af7f337818751bd
Author: Rustam Adilov <[email protected]>

phy: realtek: usb2: introduce reset controller struct

This commit introduces a reset_control struct to the driver to handle
deasserting the usb2 phy reset line using 
devm_reset_control_get_optional_exclusive().

> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
> index 5e483c853f02f..2ddad7be73534 100644
> --- a/drivers/phy/realtek/phy-rtk-usb2.c
> +++ b/drivers/phy/realtek/phy-rtk-usb2.c

[ ... ]

> @@ -676,6 +678,15 @@ static int rtk_phy_init(struct phy *phy)
>  	if (!rtk_phy)
>  		return -EINVAL;
>  
> +	if (rtk_phy->phy_rst) {
> +		ret = reset_control_deassert(rtk_phy->phy_rst);
> +
> +		if (ret)
> +			return ret;
> +
> +		usleep_range(5000, 6000);
> +	}
> +

[ ... ]

> @@ -1055,6 +1066,11 @@ static int rtk_usb2phy_probe(struct platform_device *pdev)
>  
>  	rtk_phy->num_phy = phy_cfg->num_phy;
>  
> +	rtk_phy->phy_rst = devm_reset_control_get_optional_exclusive(dev, NULL);
> +	if (IS_ERR(rtk_phy->phy_rst))
> +		return dev_err_probe(dev, PTR_ERR(rtk_phy->phy_rst),
> +				     "usb2 phy reset is not working\n");
> +
>  	ret = parse_phy_data(rtk_phy);

[Severity: High]
Can this cause a bus hang or read timeout during probe()?

Since the newly introduced reset is only deasserted later when consumers
call rtk_phy_init(), the IP is still held in reset here.

However, parse_phy_data() can perform direct MMIO reads against the PHY to
cache default values if the static configuration lacks explicit initializers:

drivers/phy/realtek/phy-rtk-usb2.c:update_dc_driving_level() {
    ...
    if (!phy_cfg->page0[4].addr) {
        rtk_phy_set_page(phy_reg, 0);
        phy_cfg->page0[4].addr = PAGE0_0XE4;
        phy_cfg->page0[4].data = rtk_phy_read(phy_reg, PAGE0_0XE4);
    }
    ...
}

Will this cause issues when interacting with the PHY registers while the
device is still in reset?

>  	if (ret)
>  		goto err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy