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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAMuHMdX1b5ZZaO+G1h=E8uv+WV7oS8xg8Hx=_+uORO7Qu33eNw@mail.gmail.com>
Date: Tue, 8 Oct 2024 16:57:43 +0200
From: Geert Uytterhoeven <geert@...ux-m68k.org>
To: Claudiu <claudiu.beznea@...on.dev>
Cc: vkoul@...nel.org, kishon@...nel.org, robh@...nel.org, krzk+dt@...nel.org, 
	conor+dt@...nel.org, p.zabel@...gutronix.de, geert+renesas@...der.be, 
	magnus.damm@...il.com, gregkh@...uxfoundation.org, mturquette@...libre.com, 
	sboyd@...nel.org, yoshihiro.shimoda.uh@...esas.com, 
	biju.das.jz@...renesas.com, ulf.hansson@...aro.org, 
	linux-phy@...ts.infradead.org, devicetree@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org, 
	linux-usb@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-clk@...r.kernel.org, linux-pm@...r.kernel.org, 
	Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 10/16] phy: renesas: rcar-gen3-usb2: Add support to
 initialize the bus

Hi Claudiu,

On Thu, Aug 22, 2024 at 5:28 PM Claudiu <claudiu.beznea@...on.dev> wrote:
>
> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>
> 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@...renesas.com>

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@wanadoo.fr

> @@ -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@...ux-m68k.org

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