Re: [PATCH v3] phy: renesas: rcar-gen3-usb2: Keep OTG VBUS disabled until role is known

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] phy: renesas: rcar-gen3-usb2: probe fails with -EOPNOTSUPP for fixed VBUS
--

commit 35522c5116f0f9b3c5a656060b08b2a8e45e1ec6
Author: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

phy: renesas: rcar-gen3-usb2: Keep OTG VBUS disabled until role is known

This commit changes the VBUS regulator acquisition helper on OTG channels
without ADP control so that it holds a single regulator enable reference and
calls regulator_hardware_enable(channel->vbus, false) at probe time to keep
VBUS off until the port role is determined.

> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index b5ba751805aae..3481505dea2e8 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -1114,8 +1114,7 @@ static void rcar_gen3_phy_usb2_vbus_disable_action(void *data)
>  	regulator_disable(vbus);
>  }
>  
> -static int rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable(struct rcar_gen3_chan *channel,
> -								  bool enable)
> +static int rcar_gen3_phy_usb2_vbus_regulator_get_exclusive(struct rcar_gen3_chan *channel)
>  {
>  	struct device *dev = channel->dev;
>  	int ret;
> @@ -1133,17 +1132,35 @@ static int rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable(struct rcar_ge
>  		return 0;
>  	}
>  
> -	if (enable) {
> +	ret = regulator_is_enabled(channel->vbus);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (!ret) {
>  		ret = regulator_enable(channel->vbus);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	if (regulator_is_enabled(channel->vbus))
> -		return devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_vbus_disable_action,
> -						channel->vbus);
> +	/*
> +	 * An exclusive get of an enabled regulator already holds a reference, so
> +	 * add a cleanup action to disable it when the device is released.
> +	 */
> +	ret = devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_vbus_disable_action,
> +				       channel->vbus);
> +	if (ret)
> +		return ret;
>  
> -	return 0;
> +	/*
> +	 * The regulator is enabled only so that the regulator core holds a
> +	 * reference to it. Do not drive VBUS until the port's role is known;
> +	 * rcar_gen3_enable_vbus_ctrl() switches it from then on.
> +	 *
> +	 * The VBUS regulator used on these SoCs implement the .enable/.disable
> +	 * ops, so regulator_hardware_enable() cannot fail with -EOPNOTSUPP here
> +	 * and no special handling for it is needed.
> +	 */
> +	return regulator_hardware_enable(channel->vbus, false);
>  }

[Severity: High]
As reported by Sashiko on v2 [1], will this cause rcar_gen3_phy_usb2_probe()
to fail with -EOPNOTSUPP on boards that use a GPIO-controlled fixed VBUS
regulator?

In rcar_gen3_phy_usb2_probe(), channel->is_otg_channel is set whenever
rcar_gen3_get_dr_mode() returns a mode other than USB_DR_MODE_UNKNOWN
(including dr_mode = "host" or "otg"). When channel->phy_data->no_adp_ctrl
is also true, probe calls:

  rcar_gen3_phy_usb2_probe()
    -> rcar_gen3_phy_usb2_vbus_regulator_register()
      -> rcar_gen3_phy_usb2_vbus_regulator_get_exclusive()
        -> regulator_hardware_enable(channel->vbus, false)

However, in drivers/regulator/core.c, regulator_hardware_enable() checks that
the regulator implements .enable and .disable ops:

  int regulator_hardware_enable(struct regulator *regulator, bool enable)
  {
      ...
      if (!rdev->exclusive || !ops || !ops->enable || !ops->disable)
          return ret; /* -EOPNOTSUPP */

For GPIO-backed fixed regulators (compatible = "regulator-fixed" in
drivers/regulator/fixed.c), the enable GPIO is passed to the regulator core
via cfg.ena_gpiod (rdev->ena_pin) and fixed_voltage_ops defines neither
.enable nor .disable:

  static const struct regulator_ops fixed_voltage_ops = {
  };

Several in-tree RZ/G2L-family device trees (where no_adp_ctrl is true) set
dr_mode = "host" on the USB controller and wire vbus-supply on the PHY node
to a GPIO-controlled regulator-fixed supply, such as usb2_phy0 in
arch/arm64/boot/dts/renesas/rzg2l-hummingboard-iiot-common.dtsi and
usb2_phy1 in arch/arm64/boot/dts/renesas/r9a07g044c2-hummingboard-ripple.dts.

Before this change, when no child vbus-regulator node was present, enable was
false and rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable() returned 0
without calling regulator_enable() or regulator_hardware_enable(), allowing
rcar_gen3_phy_usb2_power_on() and rcar_gen3_phy_usb2_power_off() to switch
channel->vbus via regulator_enable() and regulator_disable(). With this
change, regulator_enable() is called unconditionally followed by
regulator_hardware_enable(channel->vbus, false), which returns -EOPNOTSUPP
and aborts probe on those boards.

Should this enable and regulator_hardware_enable() sequence be restricted to
channel->otg_internal_reg (or handle regulators that do not support
regulator_hardware_enable())?

[1] https://lore.kernel.org/r/sashiko-outbox-163308@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102104907.16007-1-prabhakar.mahadev-lad.rj@bp.renesas.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy