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

On Thu, Jun 8, 2023 at 8:12 PM Adam Ford aford173@gmail.com wrote:

On Thu, Jun 8, 2023 at 8:29 PM Tim Harvey tharvey@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@gmail.com

...

Signed-off-by: Tim Harvey tharvey@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