Re: [PATCH 10/16] phy: renesas: rcar-gen3-usb2: Add support to initialize the bus

[Geert Uytterhoeven]

Hi Claudiu,

On Thu, Aug 22, 2024 at 5:28 PM Claudiu <claudiu.beznea@xxxxxxxxx> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>
>
> The Renesas RZ/G3S need to initialize the USB BUS before transferring data
> due to hardware limitation. As the register that need to be touched for
> this is in the address space of the USB PHY, and the UBS PHY need to be
> initialized before any other USB drivers handling data transfer, add
> support to initialize the USB BUS.
>
> As the USB PHY is probed before any other USB drivers that enables
> clocks and de-assert the reset signals and the BUS initialization is done
> in the probe phase, we need to add code to de-assert reset signal and
> runtime resume the device (which enables its clocks) before accessing
> the registers.
>
> As the reset signals are not required by the USB PHY driver for the other
> USB PHY hardware variants, the reset signals and runtime PM was handled
> only in the function that initialize the USB BUS.
>
> The PHY initialization was done right after runtime PM enable to have
> all in place when the PHYs are registered.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@xxxxxxxxxxxxxx>

Thanks for your patch, which is now commit 4eae16375357a2a7 ("phy:
renesas: rcar-gen3-usb2: Add support to initialize the bus") in
v6.12-rc1.

> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -650,6 +658,35 @@ static enum usb_dr_mode rcar_gen3_get_dr_mode(struct device_node *np)
>         return candidate;
>  }
>
> +static int rcar_gen3_phy_usb2_init_bus(struct rcar_gen3_chan *channel)
> +{
> +       struct device *dev = channel->dev;
> +       int ret;
> +       u32 val;
> +
> +       channel->rstc = devm_reset_control_array_get_shared(dev);
> +       if (IS_ERR(channel->rstc))
> +               return PTR_ERR(channel->rstc);
> +
> +       ret = pm_runtime_resume_and_get(dev);
> +       if (ret)
> +               return ret;
> +
> +       ret = reset_control_deassert(channel->rstc);
> +       if (ret)
> +               goto rpm_put;
> +
> +       val = readl(channel->base + USB2_AHB_BUS_CTR);
> +       val &= ~USB2_AHB_BUS_CTR_MBL_MASK;
> +       val |= USB2_AHB_BUS_CTR_MBL_INCR4;
> +       writel(val, channel->base + USB2_AHB_BUS_CTR);
> +
> +rpm_put:
> +       pm_runtime_put(dev);
> +
> +       return ret;
> +}
> +
>  static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>  {
>         const struct rcar_gen3_phy_drv_data *phy_data;
> @@ -703,6 +740,15 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>                 goto error;
>         }
>
> +       platform_set_drvdata(pdev, channel);
> +       channel->dev = dev;

Unrelated change?

> +
> +       if (phy_data->init_bus) {
> +               ret = rcar_gen3_phy_usb2_init_bus(channel);
> +               if (ret)
> +                       goto error;
> +       }
> +
>         channel->soc_no_adp_ctrl = phy_data->no_adp_ctrl;
>         if (phy_data->no_adp_ctrl)
>                 channel->obint_enable_bits = USB2_OBINT_IDCHG_EN;
> @@ -733,9 +779,6 @@ static int rcar_gen3_phy_usb2_probe(struct platform_device *pdev)
>                 channel->vbus = NULL;
>         }
>
> -       platform_set_drvdata(pdev, channel);
> -       channel->dev = dev;
> -
>         provider = devm_of_phy_provider_register(dev, rcar_gen3_phy_usb2_xlate);
>         if (IS_ERR(provider)) {
>                 dev_err(dev, "Failed to register PHY provider\n");

The reset is not asserted in the error path, only in .remove().

Oh, Christophe already sent a fix for that...
"[PATCH v3] phy: renesas: rcar-gen3-usb2: Fix an error handling path
in rcar_gen3_phy_usb2_probe()"
https://lore.kernel.org/all/290b25827e3f0742808940719455ff0c5cb9d01d.1726329925.git.christophe.jaillet@xxxxxxxxxx

> @@ -762,6 +805,7 @@ static void rcar_gen3_phy_usb2_remove(struct platform_device *pdev)
>         if (channel->is_otg_channel)
>                 device_remove_file(&pdev->dev, &dev_attr_role);
>
> +       reset_control_assert(channel->rstc);
>         pm_runtime_disable(&pdev->dev);
>  };

The rest LGTM.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds