[PATCH] phy: phy-imx8mq-usb: add vbus regulator support

[Adam Ford]

On Thu, Jun 8, 2023 at 8:29 PM Tim Harvey <tharvey at gateworks.com> wrote:
>
> Add support for enabling and disabling vbus-supply regulator found
> on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes.
>
> Without this I suspect U-Boot usb does not work on the following:
>  - imx8mp-beacon-kit

The Host-only port works on the Beacon board, but the dual-role, usb
type-c port doesn't appear to have been impacted.  I am guessing it's
due to the lack of a proper type-c driver. Despite that,  I think it's
the right thing to do for this platform, and I'll give some feedback
below.

>  - imx8mp-msc-sm2s
>  - imx8mp-verdin-wifi-dev
>

Reviewed-by: Adam Ford <aford173 at gmail.com>

> Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> ---
>  drivers/phy/phy-imx8mq-usb.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c
> index 69f01de55538..eed9c07848f4 100644
> --- a/drivers/phy/phy-imx8mq-usb.c
> +++ b/drivers/phy/phy-imx8mq-usb.c
> @@ -14,6 +14,7 @@
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <clk.h>
> +#include <power/regulator.h>
>
>  #define PHY_CTRL0                      0x0
>  #define PHY_CTRL0_REF_SSP_EN           BIT(2)
> @@ -81,6 +82,7 @@ struct imx8mq_usb_phy {
>  #endif
>         void __iomem *base;
>         enum imx8mpq_phy_type type;
> +       struct udevice *vbus_supply;
>  };
>
>  static const struct udevice_id imx8mq_usb_phy_of_match[] = {
> @@ -173,9 +175,9 @@ static int imx8mq_usb_phy_power_on(struct phy *usb_phy)
>         struct udevice *dev = usb_phy->dev;
>         struct imx8mq_usb_phy *imx_phy = dev_get_priv(dev);
>         u32 value;
> +       int ret;

In the unlikely event that neither CONFIG_CLK nor CONFIG_DM_REGULATOR
is configured, will defining ret here throw a compiler warning that
it's unused? I wonder if __maybe_unused attribute would be permissible
here or if this should be inside an #if statement.

>
>  #if CONFIG_IS_ENABLED(CLK)
> -       int ret;
>         ret = clk_enable(&imx_phy->phy_clk);
>         if (ret) {
>                 printf("Failed to enable usb phy clock\n");
> @@ -183,6 +185,12 @@ static int imx8mq_usb_phy_power_on(struct phy *usb_phy)
>         }
>  #endif
>
> +       if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) {
> +               ret = regulator_set_enable(imx_phy->vbus_supply, true);
> +               if (ret)

I am personally a fan of error messages.  There is an error message if
the clock fails, so unless there is an objection, can we have one here
too?  One was also added to the probe function.

> +                       return ret;
> +       }
> +
>         /* Disable rx term override */
>         value = readl(imx_phy->base + PHY_CTRL6);
>         value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL;
> @@ -206,6 +214,9 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy)
>         clk_disable(&imx_phy->phy_clk);
>  #endif
>
> +       if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply)
> +               return regulator_set_enable(imx_phy->vbus_supply, false);
> +
>         return 0;
>  }
>
> @@ -224,6 +235,7 @@ struct phy_ops imx8mq_usb_phy_ops = {
>  int imx8mq_usb_phy_probe(struct udevice *dev)
>  {
>         struct imx8mq_usb_phy *priv = dev_get_priv(dev);
> +       int ret;

Same comment as above regarding whether or not this might be unused.
>
>         priv->type = dev_get_driver_data(dev);
>         priv->base = dev_read_addr_ptr(dev);
> @@ -232,7 +244,6 @@ int imx8mq_usb_phy_probe(struct udevice *dev)
>                 return -EINVAL;
>
>  #if CONFIG_IS_ENABLED(CLK)
> -       int ret;
>
>         /* Assigned clock already set clock */
>         ret = clk_get_by_name(dev, "phy", &priv->phy_clk);
> @@ -242,6 +253,15 @@ int imx8mq_usb_phy_probe(struct udevice *dev)
>         }
>  #endif
>
> +       if (CONFIG_IS_ENABLED(DM_REGULATOR)) {
> +               ret = device_get_supply_regulator(dev, "vbus-supply",
> +                                                 &priv->vbus_supply);
> +               if (ret && ret != -ENOENT) {
> +                       pr_err("Failed to get PHY regulator\n");
> +                       return ret;
> +               }
> +       }
> +
>         return 0;
>  }
>
> --
> 2.25.1
>