[PATCH v2] rockchip: rockchip-inno-usb2: Fix Synchronous Abort on usb start

[Jonas Karlman]

Hi Alex,

On 7/3/2025 8:29 AM, Alex Shumsky wrote:
> Hi Jonas,
> 
> This time I had difficulties to reproduce Synchronous Abort on a real board, but
> then I spotted a difference with my previous tests.
> "usb start" abort happen when I boot upstream/current u-boot proper from
> SD-card by older SPL/TPL installed on eMMC (stock/proprietary u-boot from 2019
> installed by STB manufacturer).
> There is no Sync Abort with upstream SPL/TPL. Looks like NULL dereference are
> still there but some SPL/TPL init code prevents it from aborting.

Thanks for taking a closer look at this and use of different TPL, SPL
and/or TF-A versions is very likely the reasons why we initially had
different results (a sync abort vs none).

I only have one small remark on this,

> 
> On Thu, Jul 3, 2025 at 9:05 AM Alex Shumsky <alexthreed at gmail.com> wrote:
>>
>> Fix NULL pointer dereference that happen when rockchip-inno-usb2 clock
>> enabled before device probe. This early clock enable call happen in process
>> of parent clock activation added in ac30d90f3367.
>>
>> Fixes: 229218373c22 ("phy: rockchip-inno-usb2: Add support for clkout_ctl_phy").
>> Fixes: ac30d90f3367 ("clk: Ensure the parent clocks are enabled while reparenting")
>> Co-authored-by: Jonas Karlman <jonas at kwiboo.se>
>> Signed-off-by: Alex Shumsky <alexthreed at gmail.com>
>> ---
>>
>> Changes in v2:
>> - rework patch (add NULL check to code instead of dts modification)
>>
>>  drivers/phy/rockchip/phy-rockchip-inno-usb2.c | 19 ++++++++++++++-----
>>  1 file changed, 14 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>> index 88b33de1b2..3cc5956aed 100644
>> --- a/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>> +++ b/drivers/phy/rockchip/phy-rockchip-inno-usb2.c
>> @@ -167,20 +167,27 @@ static struct phy_ops rockchip_usb2phy_ops = {
>>         .of_xlate = rockchip_usb2phy_of_xlate,
>>  };
>>
>> -static void rockchip_usb2phy_clkout_ctl(struct clk *clk, struct regmap **base,
>> -                                       const struct usb2phy_reg **clkout_ctl)
>> +static int rockchip_usb2phy_clkout_ctl(struct clk *clk, struct regmap **base,
>> +                                      const struct usb2phy_reg **clkout_ctl)
>>  {
>>         struct udevice *parent = dev_get_parent(clk->dev);
>>         struct rockchip_usb2phy *priv = dev_get_priv(parent);
>>         const struct rockchip_usb2phy_cfg *phy_cfg = priv->phy_cfg;
>>
>> -       if (priv->phy_cfg->clkout_ctl_phy.enable) {
>> +       // phy_cfg can be NULL if this function called before probe (when parent
>> +       // clocks are enabled)

Please use /* block comment style */ here, with that fixed this is:

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

Regards,
Jonas

>> +       if (!phy_cfg)
>> +               return -EINVAL;
>> +
>> +       if (phy_cfg->clkout_ctl_phy.enable) {
>>                 *base = priv->phy_base;
>>                 *clkout_ctl = &phy_cfg->clkout_ctl_phy;
>>         } else {
>>                 *base = priv->reg_base;
>>                 *clkout_ctl = &phy_cfg->clkout_ctl;
>>         }
>> +
>> +       return 0;
>>  }
>>
>>  /**
>> @@ -206,7 +213,8 @@ int rockchip_usb2phy_clk_enable(struct clk *clk)
>>         const struct usb2phy_reg *clkout_ctl;
>>         struct regmap *base;
>>
>> -       rockchip_usb2phy_clkout_ctl(clk, &base, &clkout_ctl);
>> +       if (rockchip_usb2phy_clkout_ctl(clk, &base, &clkout_ctl))
>> +               return -ENOSYS;
>>
>>         /* turn on 480m clk output if it is off */
>>         if (!property_enabled(base, clkout_ctl)) {
>> @@ -230,7 +238,8 @@ int rockchip_usb2phy_clk_disable(struct clk *clk)
>>         const struct usb2phy_reg *clkout_ctl;
>>         struct regmap *base;
>>
>> -       rockchip_usb2phy_clkout_ctl(clk, &base, &clkout_ctl);
>> +       if (rockchip_usb2phy_clkout_ctl(clk, &base, &clkout_ctl))
>> +               return -ENOSYS;
>>
>>         /* turn off 480m clk output */
>>         property_enable(base, clkout_ctl, false);
>> --
>> 2.43.0
>>
>> base-commit: 7027b445cc0bfb86204ecb1f1fe596f5895048d9
>> branch: fix-rk3328-usb2phy-clock-v2