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

[Tim Harvey]

On Thu, Jun 8, 2023 at 8:12 PM Adam Ford <aford173 at gmail.com> wrote:
>
> 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.

Adam,

Is perhaps the pca6416 gpio0 pulled up externally or defaults to
driving high? If you put a print in regulator_set_enable() you'll see
that reg_usb1_host_vbus never gets called without this patch so I
don't see how a device on that host would work.

Right - the type-c doesn't work because there is no driver for the
ptn5110. Even if there were I'm not clear what the right mechanism is
for calling out from the dwc3 driver to see what role the usb
controller should be. I don't believe U-Boot already has any common
method for usb host controllers to ask for role other than reading the
dr_mode prop. Perhaps there needs to be a usb connector uclass added
with a global function to check the role that various typec controller
drivers could use. I have some imx8mp boards that have OTG connectors
where I need to check the USB_ID gpio for the role which can be
handled that way as well.

If you did want to default your type-c to host mode until such support
exists you can add 'dr_mode = "host"' to the usb_dwc3_0 node in your
imx8mp-beacon-kit-u-boot.dtsi:

>
> >  - 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.
>

good point, I'll add for v2

> >
> >  #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.

ok, I'll add this for v2

>
> > +                       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.

yep - will address.

Thanks for the review!

Tim