| Message ID | 20230713004003.3502504-1-tharvey@gateworks.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [v5] phy: phy-imx8mq-usb: add vbus regulator support | expand |
On 7/13/23 02:40, 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. > > While we are at it replace '#if CONFIG_IS_ENABLED(CLK)' with > 'if (CONFIG_IS_ENABLED(CLK))' as a cleanup. > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > Reviewed-by: Adam Ford <aford173@gmail.com> > Reviewed-by: Marek Vasut <marex@denx.de> > --- > v5: > - remove #if CONFIG_IS_ENABLED around struct members > (you can not conditionally include if using if (CONFIG_IS_ENABLED(foo)) ) > - replace clk '#if CONFIG_IS_ENABLED(CLK)' with 'if (CONFIG_IS_ENABLED(CLK))' > > v4: > - use regulator_set_enable_if_allowed to handle regulator reference > counting > errors > - added Marek's rb tag > > v3: > - change pr_err to dev_err > - add #if CONFIG_IS_ENABLED around vbus_supply in struct > - add fail path to disable clock if regulator enable failed > > 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 | 68 +++++++++++++++++++++++++----------- > 1 file changed, 48 insertions(+), 20 deletions(-) > > diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c > index 69f01de55538..532b0235e2bc 100644 > --- a/drivers/phy/phy-imx8mq-usb.c > +++ b/drivers/phy/phy-imx8mq-usb.c > @@ -14,6 +14,8 @@ > #include <linux/delay.h> > #include <linux/err.h> > #include <clk.h> > +#include <dm/device_compat.h> > +#include <power/regulator.h> > > #define PHY_CTRL0 0x0 > #define PHY_CTRL0_REF_SSP_EN BIT(2) > @@ -76,11 +78,10 @@ enum imx8mpq_phy_type { > }; > > struct imx8mq_usb_phy { > -#if CONFIG_IS_ENABLED(CLK) > struct clk phy_clk; > -#endif > void __iomem *base; > enum imx8mpq_phy_type type; > + struct udevice *vbus_supply; > }; > > static const struct udevice_id imx8mq_usb_phy_of_match[] = { > @@ -172,16 +173,24 @@ 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); > + int ret; > u32 value; > int ret; goes below u32 value (nitpick) > -#if CONFIG_IS_ENABLED(CLK) > - int ret; > - ret = clk_enable(&imx_phy->phy_clk); > - if (ret) { > - printf("Failed to enable usb phy clock\n"); > - return ret; > + if (CONFIG_IS_ENABLED(CLK)) { > + ret = clk_enable(&imx_phy->phy_clk); > + if (ret) { > + printf("Failed to enable usb phy clock\n"); > + return ret; > + } > + } > + > + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { > + ret = regulator_set_enable_if_allowed(imx_phy->vbus_supply, true); > + if (ret && ret != -ENOSYS) { > + dev_err(dev, "Failed to enable VBUS regulator: %d\n", ret); > + goto err; > + } > } > -#endif > > /* Disable rx term override */ > value = readl(imx_phy->base + PHY_CTRL6); > @@ -189,12 +198,17 @@ static int imx8mq_usb_phy_power_on(struct phy *usb_phy) > writel(value, imx_phy->base + PHY_CTRL6); > > return 0; > + > +err: > + clk_disable(&imx_phy->phy_clk); > + return ret; > } > > 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); > + int ret; > u32 value; DTTO > /* Override rx term to be 0 */ > @@ -202,9 +216,16 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) > value |= PHY_CTRL6_RXTERM_OVERRIDE_SEL; > writel(value, imx_phy->base + PHY_CTRL6); > > -#if CONFIG_IS_ENABLED(CLK) > - clk_disable(&imx_phy->phy_clk); > -#endif > + if (CONFIG_IS_ENABLED(CLK)) > + clk_disable(&imx_phy->phy_clk); It would really be good to have this in two patches, i.e. the clock clean up first, and the regulator addition second.
On Wed, Jul 12, 2023 at 6:55 PM Marek Vasut <marex@denx.de> wrote: > > On 7/13/23 02:40, 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. > > > > While we are at it replace '#if CONFIG_IS_ENABLED(CLK)' with > > 'if (CONFIG_IS_ENABLED(CLK))' as a cleanup. > > > > Signed-off-by: Tim Harvey <tharvey@gateworks.com> > > Reviewed-by: Adam Ford <aford173@gmail.com> > > Reviewed-by: Marek Vasut <marex@denx.de> > > --- > > v5: > > - remove #if CONFIG_IS_ENABLED around struct members > > (you can not conditionally include if using if (CONFIG_IS_ENABLED(foo)) ) > > - replace clk '#if CONFIG_IS_ENABLED(CLK)' with 'if (CONFIG_IS_ENABLED(CLK))' > > > > v4: > > - use regulator_set_enable_if_allowed to handle regulator reference > > counting > > errors > > - added Marek's rb tag > > > > v3: > > - change pr_err to dev_err > > - add #if CONFIG_IS_ENABLED around vbus_supply in struct > > - add fail path to disable clock if regulator enable failed > > > > 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 | 68 +++++++++++++++++++++++++----------- > > 1 file changed, 48 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c > > index 69f01de55538..532b0235e2bc 100644 > > --- a/drivers/phy/phy-imx8mq-usb.c > > +++ b/drivers/phy/phy-imx8mq-usb.c > > @@ -14,6 +14,8 @@ > > #include <linux/delay.h> > > #include <linux/err.h> > > #include <clk.h> > > +#include <dm/device_compat.h> > > +#include <power/regulator.h> > > > > #define PHY_CTRL0 0x0 > > #define PHY_CTRL0_REF_SSP_EN BIT(2) > > @@ -76,11 +78,10 @@ enum imx8mpq_phy_type { > > }; > > > > struct imx8mq_usb_phy { > > -#if CONFIG_IS_ENABLED(CLK) > > struct clk phy_clk; > > -#endif > > void __iomem *base; > > enum imx8mpq_phy_type type; > > + struct udevice *vbus_supply; > > }; > > > > static const struct udevice_id imx8mq_usb_phy_of_match[] = { > > @@ -172,16 +173,24 @@ 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); > > + int ret; > > u32 value; > > > > int ret; goes below u32 value (nitpick) > > > -#if CONFIG_IS_ENABLED(CLK) > > - int ret; > > - ret = clk_enable(&imx_phy->phy_clk); > > - if (ret) { > > - printf("Failed to enable usb phy clock\n"); > > - return ret; > > + if (CONFIG_IS_ENABLED(CLK)) { > > + ret = clk_enable(&imx_phy->phy_clk); > > + if (ret) { > > + printf("Failed to enable usb phy clock\n"); > > + return ret; > > + } > > + } > > + > > + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { > > + ret = regulator_set_enable_if_allowed(imx_phy->vbus_supply, true); > > + if (ret && ret != -ENOSYS) { > > + dev_err(dev, "Failed to enable VBUS regulator: %d\n", ret); > > + goto err; > > + } > > } > > -#endif > > > > /* Disable rx term override */ > > value = readl(imx_phy->base + PHY_CTRL6); > > @@ -189,12 +198,17 @@ static int imx8mq_usb_phy_power_on(struct phy *usb_phy) > > writel(value, imx_phy->base + PHY_CTRL6); > > > > return 0; > > + > > +err: > > + clk_disable(&imx_phy->phy_clk); > > + return ret; > > } > > > > 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); > > + int ret; > > u32 value; > > DTTO > > > /* Override rx term to be 0 */ > > @@ -202,9 +216,16 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) > > value |= PHY_CTRL6_RXTERM_OVERRIDE_SEL; > > writel(value, imx_phy->base + PHY_CTRL6); > > > > -#if CONFIG_IS_ENABLED(CLK) > > - clk_disable(&imx_phy->phy_clk); > > -#endif > > + if (CONFIG_IS_ENABLED(CLK)) > > + clk_disable(&imx_phy->phy_clk); > > It would really be good to have this in two patches, i.e. the clock > clean up first, and the regulator addition second. ok, I'll submit a v5 with 2 patches then. Note that the __maybe_unused from previous revisions will not be there as is not needed as if (CONFIG_IS_ENABLED(foo)) is always compiled in. All of these changes from '#ifdef' to 'if (CONFIG_IS_ENABLED)' is not shrinking code.... I hope everyone understands that. best regards, Tim
On 7/13/23 18:55, Tim Harvey wrote: > On Wed, Jul 12, 2023 at 6:55 PM Marek Vasut <marex@denx.de> wrote: >> >> On 7/13/23 02:40, 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. >>> >>> While we are at it replace '#if CONFIG_IS_ENABLED(CLK)' with >>> 'if (CONFIG_IS_ENABLED(CLK))' as a cleanup. >>> >>> Signed-off-by: Tim Harvey <tharvey@gateworks.com> >>> Reviewed-by: Adam Ford <aford173@gmail.com> >>> Reviewed-by: Marek Vasut <marex@denx.de> >>> --- >>> v5: >>> - remove #if CONFIG_IS_ENABLED around struct members >>> (you can not conditionally include if using if (CONFIG_IS_ENABLED(foo)) ) >>> - replace clk '#if CONFIG_IS_ENABLED(CLK)' with 'if (CONFIG_IS_ENABLED(CLK))' >>> >>> v4: >>> - use regulator_set_enable_if_allowed to handle regulator reference >>> counting >>> errors >>> - added Marek's rb tag >>> >>> v3: >>> - change pr_err to dev_err >>> - add #if CONFIG_IS_ENABLED around vbus_supply in struct >>> - add fail path to disable clock if regulator enable failed >>> >>> 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 | 68 +++++++++++++++++++++++++----------- >>> 1 file changed, 48 insertions(+), 20 deletions(-) >>> >>> diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c >>> index 69f01de55538..532b0235e2bc 100644 >>> --- a/drivers/phy/phy-imx8mq-usb.c >>> +++ b/drivers/phy/phy-imx8mq-usb.c >>> @@ -14,6 +14,8 @@ >>> #include <linux/delay.h> >>> #include <linux/err.h> >>> #include <clk.h> >>> +#include <dm/device_compat.h> >>> +#include <power/regulator.h> >>> >>> #define PHY_CTRL0 0x0 >>> #define PHY_CTRL0_REF_SSP_EN BIT(2) >>> @@ -76,11 +78,10 @@ enum imx8mpq_phy_type { >>> }; >>> >>> struct imx8mq_usb_phy { >>> -#if CONFIG_IS_ENABLED(CLK) >>> struct clk phy_clk; >>> -#endif >>> void __iomem *base; >>> enum imx8mpq_phy_type type; >>> + struct udevice *vbus_supply; >>> }; >>> >>> static const struct udevice_id imx8mq_usb_phy_of_match[] = { >>> @@ -172,16 +173,24 @@ 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); >>> + int ret; >>> u32 value; >>> >> >> int ret; goes below u32 value (nitpick) >> >>> -#if CONFIG_IS_ENABLED(CLK) >>> - int ret; >>> - ret = clk_enable(&imx_phy->phy_clk); >>> - if (ret) { >>> - printf("Failed to enable usb phy clock\n"); >>> - return ret; >>> + if (CONFIG_IS_ENABLED(CLK)) { >>> + ret = clk_enable(&imx_phy->phy_clk); >>> + if (ret) { >>> + printf("Failed to enable usb phy clock\n"); >>> + return ret; >>> + } >>> + } >>> + >>> + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { >>> + ret = regulator_set_enable_if_allowed(imx_phy->vbus_supply, true); >>> + if (ret && ret != -ENOSYS) { >>> + dev_err(dev, "Failed to enable VBUS regulator: %d\n", ret); >>> + goto err; >>> + } >>> } >>> -#endif >>> >>> /* Disable rx term override */ >>> value = readl(imx_phy->base + PHY_CTRL6); >>> @@ -189,12 +198,17 @@ static int imx8mq_usb_phy_power_on(struct phy *usb_phy) >>> writel(value, imx_phy->base + PHY_CTRL6); >>> >>> return 0; >>> + >>> +err: >>> + clk_disable(&imx_phy->phy_clk); >>> + return ret; >>> } >>> >>> 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); >>> + int ret; >>> u32 value; >> >> DTTO >> >>> /* Override rx term to be 0 */ >>> @@ -202,9 +216,16 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) >>> value |= PHY_CTRL6_RXTERM_OVERRIDE_SEL; >>> writel(value, imx_phy->base + PHY_CTRL6); >>> >>> -#if CONFIG_IS_ENABLED(CLK) >>> - clk_disable(&imx_phy->phy_clk); >>> -#endif >>> + if (CONFIG_IS_ENABLED(CLK)) >>> + clk_disable(&imx_phy->phy_clk); >> >> It would really be good to have this in two patches, i.e. the clock >> clean up first, and the regulator addition second. > > ok, I'll submit a v5 with 2 patches then. > > Note that the __maybe_unused from previous revisions will not be there > as is not needed as if (CONFIG_IS_ENABLED(foo)) is always compiled in. > All of these changes from '#ifdef' to 'if (CONFIG_IS_ENABLED)' is not > shrinking code.... I hope everyone understands that. I believe we are all in agreement, yes.
diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c index 69f01de55538..532b0235e2bc 100644 --- a/drivers/phy/phy-imx8mq-usb.c +++ b/drivers/phy/phy-imx8mq-usb.c @@ -14,6 +14,8 @@ #include <linux/delay.h> #include <linux/err.h> #include <clk.h> +#include <dm/device_compat.h> +#include <power/regulator.h> #define PHY_CTRL0 0x0 #define PHY_CTRL0_REF_SSP_EN BIT(2) @@ -76,11 +78,10 @@ enum imx8mpq_phy_type { }; struct imx8mq_usb_phy { -#if CONFIG_IS_ENABLED(CLK) struct clk phy_clk; -#endif void __iomem *base; enum imx8mpq_phy_type type; + struct udevice *vbus_supply; }; static const struct udevice_id imx8mq_usb_phy_of_match[] = { @@ -172,16 +173,24 @@ 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); + int ret; u32 value; -#if CONFIG_IS_ENABLED(CLK) - int ret; - ret = clk_enable(&imx_phy->phy_clk); - if (ret) { - printf("Failed to enable usb phy clock\n"); - return ret; + if (CONFIG_IS_ENABLED(CLK)) { + ret = clk_enable(&imx_phy->phy_clk); + if (ret) { + printf("Failed to enable usb phy clock\n"); + return ret; + } + } + + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { + ret = regulator_set_enable_if_allowed(imx_phy->vbus_supply, true); + if (ret && ret != -ENOSYS) { + dev_err(dev, "Failed to enable VBUS regulator: %d\n", ret); + goto err; + } } -#endif /* Disable rx term override */ value = readl(imx_phy->base + PHY_CTRL6); @@ -189,12 +198,17 @@ static int imx8mq_usb_phy_power_on(struct phy *usb_phy) writel(value, imx_phy->base + PHY_CTRL6); return 0; + +err: + clk_disable(&imx_phy->phy_clk); + return ret; } 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); + int ret; u32 value; /* Override rx term to be 0 */ @@ -202,9 +216,16 @@ static int imx8mq_usb_phy_power_off(struct phy *usb_phy) value |= PHY_CTRL6_RXTERM_OVERRIDE_SEL; writel(value, imx_phy->base + PHY_CTRL6); -#if CONFIG_IS_ENABLED(CLK) - clk_disable(&imx_phy->phy_clk); -#endif + if (CONFIG_IS_ENABLED(CLK)) + clk_disable(&imx_phy->phy_clk); + + if (CONFIG_IS_ENABLED(DM_REGULATOR) && imx_phy->vbus_supply) { + ret = regulator_set_enable_if_allowed(imx_phy->vbus_supply, false); + if (ret && ret != -ENOSYS) { + dev_err(dev, "Failed to disable VBUS regulator: %d\n", ret); + return ret; + } + } return 0; } @@ -224,6 +245,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; priv->type = dev_get_driver_data(dev); priv->base = dev_read_addr_ptr(dev); @@ -231,16 +253,22 @@ int imx8mq_usb_phy_probe(struct udevice *dev) if (!priv->base) return -EINVAL; -#if CONFIG_IS_ENABLED(CLK) - int ret; + if (CONFIG_IS_ENABLED(CLK)) { + ret = clk_get_by_name(dev, "phy", &priv->phy_clk); + if (ret) { + printf("Failed to get usb phy clock\n"); + return ret; + } + } - /* Assigned clock already set clock */ - ret = clk_get_by_name(dev, "phy", &priv->phy_clk); - if (ret) { - printf("Failed to get usb phy clock\n"); - return ret; + if (CONFIG_IS_ENABLED(DM_REGULATOR)) { + ret = device_get_supply_regulator(dev, "vbus-supply", + &priv->vbus_supply); + if (ret && ret != -ENOENT) { + dev_err(dev, "Failed to get VBUS regulator: %d\n", ret); + return ret; + } } -#endif return 0; }