Re: [PATCH v2] phy: phy-imx8mq-usb: add vbus regulator support
Hi Tim, On 11.07.23 21:42, sba...@denx.de wrote: Add support for enabling and disabling vbus-supply regulator found on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes. Signed-off-by: Tim Harvey Reviewed-by: Adam Ford Applied to u-boot-imx, master, thanks ! I had applied V4, but after talking with Marek and seen V5, I have drop it from u-boot-imx. Regards, Stefano -- = DENX Software Engineering GmbH,Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de =
[PATCH v2] phy: phy-imx8mq-usb: add vbus regulator support
> Add support for enabling and disabling vbus-supply regulator found > on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes. > Signed-off-by: Tim Harvey > Reviewed-by: Adam Ford Applied to u-boot-imx, master, thanks ! Best regards, Stefano Babic -- = DENX Software Engineering GmbH,Managing Director: Erika Unter HRB 165235 Munich, Office: Kirchenstr.5, 82194 Groebenzell, Germany Phone: +49-8142-66989-53 Fax: +49-8142-66989-80 Email: sba...@denx.de =
Re: [PATCH v2] phy: phy-imx8mq-usb: add vbus regulator support
On Mon, Jul 10, 2023 at 9:33 AM Marek Vasut wrote: > > On 7/10/23 17:58, Tim Harvey wrote: > > On Mon, Jul 10, 2023 at 8:44 AM Marek Vasut wrote: > >> > >> On 7/10/23 17:26, Tim Harvey wrote: > >>> On Sat, Jul 8, 2023 at 1:55 PM Marek Vasut wrote: > > On 6/9/23 19:28, Tim Harvey wrote: > > Add support for enabling and disabling vbus-supply regulator found > > on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes. > > > > Signed-off-by: Tim Harvey > > Reviewed-by: Adam Ford > > --- > > v2: > > - protect ret with __maybe_unused in case CONFIG_CLK and > > CONFIG_DM_REGULATOR not defined > > - add error prints on failures > > - add Adam's rb tag > > --- > > drivers/phy/phy-imx8mq-usb.c | 32 ++-- > > 1 file changed, 30 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c > > index 69f01de55538..53099436b04b 100644 > > --- a/drivers/phy/phy-imx8mq-usb.c > > +++ b/drivers/phy/phy-imx8mq-usb.c > > @@ -14,6 +14,7 @@ > > #include > > #include > > #include > > +#include > > > > #define PHY_CTRL0 0x0 > > #define PHY_CTRL0_REF_SSP_ENBIT(2) > > @@ -81,6 +82,7 @@ struct imx8mq_usb_phy { > > #endif > > void __iomem *base; > > enum imx8mpq_phy_type type; > > Shouldn't this be in #if CONFIG_IS_ENABLED(DM_REGULATOR) ? > > > + struct udevice *vbus_supply; > >>> > >>> Hi Marek, > >>> > >>> No, the usage of it is within an 'if (CONFIG_IS_ENABLED(DM_REGULATOR) > >>> && imx_phy->vbus_supply)' statement > >> > >> Except if this is not ifdef'd out, the structure is larger for no good > >> reason if DM_REGULATOR is DISABLED. > > > > ok, I see your point. > > > >> > > }; > > > > static const struct udevice_id imx8mq_usb_phy_of_match[] = { > > @@ -172,10 +174,10 @@ 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); > > + __maybe_unused int ret; > > u32 value; > > > > #if CONFIG_IS_ENABLED(CLK) > > - int ret; > > ret = clk_enable(_phy->phy_clk); > > if (ret) { > > printf("Failed to enable usb phy clock\n"); > > @@ -183,6 +185,14 @@ 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) { > > + pr_err("Failed to enable VBUS regulator: %d\n", > > ret); > > You do need to disable clock enabled above. > >>> > >>> it does get disabled in imx8mq_usb_phy_power_off() > >> > >> If that called if power_on returns -ERRNO ? > > > > sorry, I don't quite understand. > > > > Are you asking me to add a regulator disable in a remove function to > > make sure it's disabled if removed or are you saying that I shouldn't > > return the error from regulator_set_enable from > > imx8mq_usb_phy_power_on? > > Actually, neither. There is clk_enable() a bit higher up in this > power_on function. You need to stop the clock if this > regulator_set_enable() return -ERRNO. > > This likely needs a fail path with "err: ... return ret;" > >>> > >>> there are no further conditional return paths in this function. > >> > >> I suspect there should be. > > > > again, I don't understand. The only thing remaining in this function > > is disable rx term override below so I don't see the point in adding a > > goto around that if that is what you are suggesting. > > > > I don't really know what 'rx term override' is either... perhaps the > > regulator_set_enable should go after that? > > Its only a matter of disabling the clock if the regulator enable fails > and we bail out, that's all. Does it make sense now ? Yes - that makes sense. Thanks for explaining... not sure how I misunderstood that. You said 'clock' and I kept reading 'regulator' :) Thanks, Tim
Re: [PATCH v2] phy: phy-imx8mq-usb: add vbus regulator support
On 7/10/23 17:58, Tim Harvey wrote: On Mon, Jul 10, 2023 at 8:44 AM Marek Vasut wrote: On 7/10/23 17:26, Tim Harvey wrote: On Sat, Jul 8, 2023 at 1:55 PM Marek Vasut wrote: On 6/9/23 19:28, Tim Harvey wrote: Add support for enabling and disabling vbus-supply regulator found on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes. Signed-off-by: Tim Harvey Reviewed-by: Adam Ford --- v2: - protect ret with __maybe_unused in case CONFIG_CLK and CONFIG_DM_REGULATOR not defined - add error prints on failures - add Adam's rb tag --- drivers/phy/phy-imx8mq-usb.c | 32 ++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c index 69f01de55538..53099436b04b 100644 --- a/drivers/phy/phy-imx8mq-usb.c +++ b/drivers/phy/phy-imx8mq-usb.c @@ -14,6 +14,7 @@ #include #include #include +#include #define PHY_CTRL0 0x0 #define PHY_CTRL0_REF_SSP_ENBIT(2) @@ -81,6 +82,7 @@ struct imx8mq_usb_phy { #endif void __iomem *base; enum imx8mpq_phy_type type; Shouldn't this be in #if CONFIG_IS_ENABLED(DM_REGULATOR) ? + struct udevice *vbus_supply; Hi Marek, No, the usage of it is within an 'if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply)' statement Except if this is not ifdef'd out, the structure is larger for no good reason if DM_REGULATOR is DISABLED. ok, I see your point. }; static const struct udevice_id imx8mq_usb_phy_of_match[] = { @@ -172,10 +174,10 @@ 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); + __maybe_unused int ret; u32 value; #if CONFIG_IS_ENABLED(CLK) - int ret; ret = clk_enable(_phy->phy_clk); if (ret) { printf("Failed to enable usb phy clock\n"); @@ -183,6 +185,14 @@ 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) { + pr_err("Failed to enable VBUS regulator: %d\n", ret); You do need to disable clock enabled above. it does get disabled in imx8mq_usb_phy_power_off() If that called if power_on returns -ERRNO ? sorry, I don't quite understand. Are you asking me to add a regulator disable in a remove function to make sure it's disabled if removed or are you saying that I shouldn't return the error from regulator_set_enable from imx8mq_usb_phy_power_on? Actually, neither. There is clk_enable() a bit higher up in this power_on function. You need to stop the clock if this regulator_set_enable() return -ERRNO. This likely needs a fail path with "err: ... return ret;" there are no further conditional return paths in this function. I suspect there should be. again, I don't understand. The only thing remaining in this function is disable rx term override below so I don't see the point in adding a goto around that if that is what you are suggesting. I don't really know what 'rx term override' is either... perhaps the regulator_set_enable should go after that? Its only a matter of disabling the clock if the regulator enable fails and we bail out, that's all. Does it make sense now ?
Re: [PATCH v2] phy: phy-imx8mq-usb: add vbus regulator support
On Mon, Jul 10, 2023 at 8:44 AM Marek Vasut wrote: > > On 7/10/23 17:26, Tim Harvey wrote: > > On Sat, Jul 8, 2023 at 1:55 PM Marek Vasut wrote: > >> > >> On 6/9/23 19:28, Tim Harvey wrote: > >>> Add support for enabling and disabling vbus-supply regulator found > >>> on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes. > >>> > >>> Signed-off-by: Tim Harvey > >>> Reviewed-by: Adam Ford > >>> --- > >>> v2: > >>>- protect ret with __maybe_unused in case CONFIG_CLK and > >>> CONFIG_DM_REGULATOR not defined > >>>- add error prints on failures > >>>- add Adam's rb tag > >>> --- > >>>drivers/phy/phy-imx8mq-usb.c | 32 ++-- > >>>1 file changed, 30 insertions(+), 2 deletions(-) > >>> > >>> diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c > >>> index 69f01de55538..53099436b04b 100644 > >>> --- a/drivers/phy/phy-imx8mq-usb.c > >>> +++ b/drivers/phy/phy-imx8mq-usb.c > >>> @@ -14,6 +14,7 @@ > >>>#include > >>>#include > >>>#include > >>> +#include > >>> > >>>#define PHY_CTRL0 0x0 > >>>#define PHY_CTRL0_REF_SSP_ENBIT(2) > >>> @@ -81,6 +82,7 @@ struct imx8mq_usb_phy { > >>>#endif > >>>void __iomem *base; > >>>enum imx8mpq_phy_type type; > >> > >> Shouldn't this be in #if CONFIG_IS_ENABLED(DM_REGULATOR) ? > >> > >>> + struct udevice *vbus_supply; > > > > Hi Marek, > > > > No, the usage of it is within an 'if (CONFIG_IS_ENABLED(DM_REGULATOR) > > && imx_phy->vbus_supply)' statement > > Except if this is not ifdef'd out, the structure is larger for no good > reason if DM_REGULATOR is DISABLED. ok, I see your point. > > >>>}; > >>> > >>>static const struct udevice_id imx8mq_usb_phy_of_match[] = { > >>> @@ -172,10 +174,10 @@ 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); > >>> + __maybe_unused int ret; > >>>u32 value; > >>> > >>>#if CONFIG_IS_ENABLED(CLK) > >>> - int ret; > >>>ret = clk_enable(_phy->phy_clk); > >>>if (ret) { > >>>printf("Failed to enable usb phy clock\n"); > >>> @@ -183,6 +185,14 @@ 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) { > >>> + pr_err("Failed to enable VBUS regulator: %d\n", > >>> ret); > >> > >> You do need to disable clock enabled above. > > > > it does get disabled in imx8mq_usb_phy_power_off() > > If that called if power_on returns -ERRNO ? sorry, I don't quite understand. Are you asking me to add a regulator disable in a remove function to make sure it's disabled if removed or are you saying that I shouldn't return the error from regulator_set_enable from imx8mq_usb_phy_power_on? > > >> This likely needs a fail path with "err: ... return ret;" > > > > there are no further conditional return paths in this function. > > I suspect there should be. again, I don't understand. The only thing remaining in this function is disable rx term override below so I don't see the point in adding a goto around that if that is what you are suggesting. I don't really know what 'rx term override' is either... perhaps the regulator_set_enable should go after that? best regards, Tim > > >>> + return ret; > >>> + } > >>> + } > >>> + > >>>/* Disable rx term override */ > >>>value = readl(imx_phy->base + PHY_CTRL6); > >>>value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL; > >>> @@ -195,6 +205,7 @@ static int imx8mq_usb_phy_power_off(struct phy > >>> *usb_phy) > >>>{ > >>>struct udevice *dev = usb_phy->dev; > >>>struct imx8mq_usb_phy *imx_phy = dev_get_priv(dev); > >>> + __maybe_unused int ret; > >>>u32 value; > >>> > >>>/* Override rx term to be 0 */ > >>> @@ -206,6 +217,14 @@ static int imx8mq_usb_phy_power_off(struct phy > >>> *usb_phy) > >>>clk_disable(_phy->phy_clk); > >>>#endif > >>> > >>> + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { > >>> + ret = regulator_set_enable(imx_phy->vbus_supply, false); > >>> + if (ret) { > >>> + pr_err("Failed to disable VBUS regulator: %d\n", > >>> ret); > >> > >> btw with DM, these can be dev_err() . > > > > Ok, I will send a v3 with that. > > Thanks !
Re: [PATCH v2] phy: phy-imx8mq-usb: add vbus regulator support
On 7/10/23 17:26, Tim Harvey wrote: On Sat, Jul 8, 2023 at 1:55 PM Marek Vasut wrote: On 6/9/23 19:28, Tim Harvey wrote: Add support for enabling and disabling vbus-supply regulator found on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes. Signed-off-by: Tim Harvey Reviewed-by: Adam Ford --- v2: - protect ret with __maybe_unused in case CONFIG_CLK and CONFIG_DM_REGULATOR not defined - add error prints on failures - add Adam's rb tag --- drivers/phy/phy-imx8mq-usb.c | 32 ++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c index 69f01de55538..53099436b04b 100644 --- a/drivers/phy/phy-imx8mq-usb.c +++ b/drivers/phy/phy-imx8mq-usb.c @@ -14,6 +14,7 @@ #include #include #include +#include #define PHY_CTRL0 0x0 #define PHY_CTRL0_REF_SSP_ENBIT(2) @@ -81,6 +82,7 @@ struct imx8mq_usb_phy { #endif void __iomem *base; enum imx8mpq_phy_type type; Shouldn't this be in #if CONFIG_IS_ENABLED(DM_REGULATOR) ? + struct udevice *vbus_supply; Hi Marek, No, the usage of it is within an 'if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply)' statement Except if this is not ifdef'd out, the structure is larger for no good reason if DM_REGULATOR is DISABLED. }; static const struct udevice_id imx8mq_usb_phy_of_match[] = { @@ -172,10 +174,10 @@ 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); + __maybe_unused int ret; u32 value; #if CONFIG_IS_ENABLED(CLK) - int ret; ret = clk_enable(_phy->phy_clk); if (ret) { printf("Failed to enable usb phy clock\n"); @@ -183,6 +185,14 @@ 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) { + pr_err("Failed to enable VBUS regulator: %d\n", ret); You do need to disable clock enabled above. it does get disabled in imx8mq_usb_phy_power_off() If that called if power_on returns -ERRNO ? This likely needs a fail path with "err: ... return ret;" there are no further conditional return paths in this function. I suspect there should be. + return ret; + } + } + /* Disable rx term override */ value = readl(imx_phy->base + PHY_CTRL6); value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL; @@ -195,6 +205,7 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) { struct udevice *dev = usb_phy->dev; struct imx8mq_usb_phy *imx_phy = dev_get_priv(dev); + __maybe_unused int ret; u32 value; /* Override rx term to be 0 */ @@ -206,6 +217,14 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) clk_disable(_phy->phy_clk); #endif + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { + ret = regulator_set_enable(imx_phy->vbus_supply, false); + if (ret) { + pr_err("Failed to disable VBUS regulator: %d\n", ret); btw with DM, these can be dev_err() . Ok, I will send a v3 with that. Thanks !
Re: [PATCH v2] phy: phy-imx8mq-usb: add vbus regulator support
On Sat, Jul 8, 2023 at 1:55 PM Marek Vasut wrote: > > On 6/9/23 19:28, Tim Harvey wrote: > > Add support for enabling and disabling vbus-supply regulator found > > on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes. > > > > Signed-off-by: Tim Harvey > > Reviewed-by: Adam Ford > > --- > > v2: > > - protect ret with __maybe_unused in case CONFIG_CLK and > > CONFIG_DM_REGULATOR not defined > > - add error prints on failures > > - add Adam's rb tag > > --- > > drivers/phy/phy-imx8mq-usb.c | 32 ++-- > > 1 file changed, 30 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c > > index 69f01de55538..53099436b04b 100644 > > --- a/drivers/phy/phy-imx8mq-usb.c > > +++ b/drivers/phy/phy-imx8mq-usb.c > > @@ -14,6 +14,7 @@ > > #include > > #include > > #include > > +#include > > > > #define PHY_CTRL0 0x0 > > #define PHY_CTRL0_REF_SSP_ENBIT(2) > > @@ -81,6 +82,7 @@ struct imx8mq_usb_phy { > > #endif > > void __iomem *base; > > enum imx8mpq_phy_type type; > > Shouldn't this be in #if CONFIG_IS_ENABLED(DM_REGULATOR) ? > > > + struct udevice *vbus_supply; Hi Marek, No, the usage of it is within an 'if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply)' statement > > }; > > > > static const struct udevice_id imx8mq_usb_phy_of_match[] = { > > @@ -172,10 +174,10 @@ 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); > > + __maybe_unused int ret; > > u32 value; > > > > #if CONFIG_IS_ENABLED(CLK) > > - int ret; > > ret = clk_enable(_phy->phy_clk); > > if (ret) { > > printf("Failed to enable usb phy clock\n"); > > @@ -183,6 +185,14 @@ 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) { > > + pr_err("Failed to enable VBUS regulator: %d\n", ret); > > You do need to disable clock enabled above. it does get disabled in imx8mq_usb_phy_power_off() > This likely needs a fail path with "err: ... return ret;" there are no further conditional return paths in this function. > > > + return ret; > > + } > > + } > > + > > /* Disable rx term override */ > > value = readl(imx_phy->base + PHY_CTRL6); > > value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL; > > @@ -195,6 +205,7 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) > > { > > struct udevice *dev = usb_phy->dev; > > struct imx8mq_usb_phy *imx_phy = dev_get_priv(dev); > > + __maybe_unused int ret; > > u32 value; > > > > /* Override rx term to be 0 */ > > @@ -206,6 +217,14 @@ static int imx8mq_usb_phy_power_off(struct phy > > *usb_phy) > > clk_disable(_phy->phy_clk); > > #endif > > > > + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { > > + ret = regulator_set_enable(imx_phy->vbus_supply, false); > > + if (ret) { > > + pr_err("Failed to disable VBUS regulator: %d\n", ret); > > btw with DM, these can be dev_err() . Ok, I will send a v3 with that. Best regards, Tim > > > + return ret; > > + } > > + } > > + > > return 0; > > } > [...]
Re: [PATCH v2] phy: phy-imx8mq-usb: add vbus regulator support
On 6/9/23 19:28, Tim Harvey wrote: Add support for enabling and disabling vbus-supply regulator found on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes. Signed-off-by: Tim Harvey Reviewed-by: Adam Ford --- v2: - protect ret with __maybe_unused in case CONFIG_CLK and CONFIG_DM_REGULATOR not defined - add error prints on failures - add Adam's rb tag --- drivers/phy/phy-imx8mq-usb.c | 32 ++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c index 69f01de55538..53099436b04b 100644 --- a/drivers/phy/phy-imx8mq-usb.c +++ b/drivers/phy/phy-imx8mq-usb.c @@ -14,6 +14,7 @@ #include #include #include +#include #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; Shouldn't this be in #if CONFIG_IS_ENABLED(DM_REGULATOR) ? + struct udevice *vbus_supply; }; static const struct udevice_id imx8mq_usb_phy_of_match[] = { @@ -172,10 +174,10 @@ 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); + __maybe_unused int ret; u32 value; #if CONFIG_IS_ENABLED(CLK) - int ret; ret = clk_enable(_phy->phy_clk); if (ret) { printf("Failed to enable usb phy clock\n"); @@ -183,6 +185,14 @@ 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) { + pr_err("Failed to enable VBUS regulator: %d\n", ret); You do need to disable clock enabled above. This likely needs a fail path with "err: ... return ret;" + return ret; + } + } + /* Disable rx term override */ value = readl(imx_phy->base + PHY_CTRL6); value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL; @@ -195,6 +205,7 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) { struct udevice *dev = usb_phy->dev; struct imx8mq_usb_phy *imx_phy = dev_get_priv(dev); + __maybe_unused int ret; u32 value; /* Override rx term to be 0 */ @@ -206,6 +217,14 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) clk_disable(_phy->phy_clk); #endif + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { + ret = regulator_set_enable(imx_phy->vbus_supply, false); + if (ret) { + pr_err("Failed to disable VBUS regulator: %d\n", ret); btw with DM, these can be dev_err() . + return ret; + } + } + return 0; } [...]
[PATCH v2] phy: phy-imx8mq-usb: add vbus regulator support
Add support for enabling and disabling vbus-supply regulator found on several imx8mp boards in the usb3_phy0 and usb3_phy1 nodes. Signed-off-by: Tim Harvey Reviewed-by: Adam Ford --- v2: - protect ret with __maybe_unused in case CONFIG_CLK and CONFIG_DM_REGULATOR not defined - add error prints on failures - add Adam's rb tag --- drivers/phy/phy-imx8mq-usb.c | 32 ++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c index 69f01de55538..53099436b04b 100644 --- a/drivers/phy/phy-imx8mq-usb.c +++ b/drivers/phy/phy-imx8mq-usb.c @@ -14,6 +14,7 @@ #include #include #include +#include #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[] = { @@ -172,10 +174,10 @@ 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); + __maybe_unused int ret; u32 value; #if CONFIG_IS_ENABLED(CLK) - int ret; ret = clk_enable(_phy->phy_clk); if (ret) { printf("Failed to enable usb phy clock\n"); @@ -183,6 +185,14 @@ 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) { + pr_err("Failed to enable VBUS regulator: %d\n", ret); + return ret; + } + } + /* Disable rx term override */ value = readl(imx_phy->base + PHY_CTRL6); value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL; @@ -195,6 +205,7 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) { struct udevice *dev = usb_phy->dev; struct imx8mq_usb_phy *imx_phy = dev_get_priv(dev); + __maybe_unused int ret; u32 value; /* Override rx term to be 0 */ @@ -206,6 +217,14 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) clk_disable(_phy->phy_clk); #endif + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { + ret = regulator_set_enable(imx_phy->vbus_supply, false); + if (ret) { + pr_err("Failed to disable VBUS regulator: %d\n", ret); + return ret; + } + } + return 0; } @@ -224,6 +243,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); + __maybe_unused int ret; priv->type = dev_get_driver_data(dev); priv->base = dev_read_addr_ptr(dev); @@ -232,7 +252,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", >phy_clk); @@ -242,6 +261,15 @@ int imx8mq_usb_phy_probe(struct udevice *dev) } #endif + if (CONFIG_IS_ENABLED(DM_REGULATOR)) { + ret = device_get_supply_regulator(dev, "vbus-supply", + >vbus_supply); + if (ret && ret != -ENOENT) { + pr_err("Failed to get VBUS regulator: %d\n", ret); + return ret; + } + } + return 0; } -- 2.25.1