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 <alexthr...@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 <jo...@kwiboo.se>
>> Signed-off-by: Alex Shumsky <alexthr...@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 <jo...@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
