On 7/12/23 20:30, Tim Harvey wrote:
On Wed, Jul 12, 2023 at 10:25 AM Marek Vasut marex@denx.de wrote:
On 7/12/23 18:29, Tim Harvey wrote:
On Wed, Jul 12, 2023 at 8:10 AM Tim Harvey tharvey@gateworks.com wrote:
On Wed, Jul 12, 2023 at 2:15 AM Marek Vasut marex@denx.de wrote:
On 7/11/23 22:13, Tim Harvey wrote:
On Mon, Jul 10, 2023 at 4:18 PM Marek Vasut marex@denx.de wrote: > > On 7/11/23 00:49, 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 tharvey@gateworks.com >> Reviewed-by: Adam Ford aford173@gmail.com >> Reviewed-by: Marek Vasut marex@denx.de >> --- >> 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 | 39 ++++++++++++++++++++++++++++++++++-- >> 1 file changed, 37 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/phy/phy-imx8mq-usb.c b/drivers/phy/phy-imx8mq-usb.c >> index 69f01de55538..6cbcb2338cfe 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) >> @@ -81,6 +83,9 @@ struct imx8mq_usb_phy { >> #endif >> void __iomem *base; >> enum imx8mpq_phy_type type; >> +#if CONFIG_IS_ENABLED(DM_REGULATOR) >> + struct udevice *vbus_supply; >> +#endif >> }; >> >> static const struct udevice_id imx8mq_usb_phy_of_match[] = { >> @@ -172,10 +177,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(&imx_phy->phy_clk); >> if (ret) { >> printf("Failed to enable usb phy clock\n"); >> @@ -183,18 +188,31 @@ 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_if_allowed(imx_phy->vbus_supply, true); >> + if (ret && ret != -ENOSYS) { >> + dev_err(dev, "Failed to enable VBUS regulator: %d\n", ret); >> + goto err; >> + } >> + } >> + >> /* Disable rx term override */ >> value = readl(imx_phy->base + PHY_CTRL6); >> value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL; >> 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); >> + __maybe_unused int ret; >> u32 value; >> >> /* Override rx term to be 0 */ >> @@ -206,6 +224,14 @@ 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) { >> + 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 +250,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 +259,6 @@ int imx8mq_usb_phy_probe(struct udevice *dev) >> return -EINVAL; >> >> #if CONFIG_IS_ENABLED(CLK) >> - int ret; > > I _think_ if you were to convert this #if CONFIG...() to if > (CONFIG...()), you would be able to drop the __maybe_unused . Separate > conversion patch would be better though.
Hi Marek,
Hi,
Yes, I thought about that as well and felt it should be a separate followup cleanup patch.
Can you please send such a follow up patch ?
Yes, I'll send a follow-up today. It looks like Stefano is picking this up from the imx tree... is that where it should be picked up or should this be through you and the usb tree?
Marek,
It seems Stefano picked up my v2 version of this patch in his master/next tree he's working on right now [1].
Likely this patch should be dropped from imx tree.
Stefano,
please drop from your imx/next tree: commit e445a1d7ffbd ("phy: phy-imx8mq-usb: add vbus regulator support"). Marek will take this through the usb tree when it's ready as it looks like I need a v5.
Also, I noticed my v4 patch here fails building if DM_REGULATOR is disabled. You can't wrap the struct members with macro's to save space if you're doing runtime comparisons.
For example:
struct imx8mq_usb_phy { #if CONFIG_IS_ENABLED(CLK) struct clk phy_clk; #endif void __iomem *base; enum imx8mpq_phy_type type; #if CONFIG_IS_ENABLED(DM_REGULATOR) struct udevice *vbus_supply; #endif };
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; } }
This fails if DM_REGULATOR is not enabled. Similarly I would assume the '#if CONFIG_IS_ENABLED(CLK)' is incorrect as well but I can't build for my board without CLK defined to prove it.
So I'm not quite sure how to proceed at this time.
Should I post a followup after Stefano's tree is ready for pull and merged that does the following?
- change pr_err to dev_err (from my v3)
- add fail path to disable clock if regulator enabled failed (from my v3)
- cleanup the clk code (remove #if CONFIG_IS_ENABLED(CLK) around clk
struct member and use 'if (CONFIG_IS_ENABLED(CLK))'
Does #ifdef work for the structure members ? I think it should do the trick.
Marek,
Wrapping vbus_supply with '#ifdef CONFIG_DM_REGULATOR' doesn't help. If you want to use the pattern 'if (CONFIG_IS_ENABLED(foo))' then that code gets compiled in and executed at runtime thus anything it's referencing needs to be there.
Is there a board using drivers/phy/phy-imx8mq-usb.c that doesn't require CLK? If so, I think you'll find that even before my patch disabling CLK will cause 'phy_clk' member not found issues due to: struct imx8mq_usb_phy { #if CONFIG_IS_ENABLED(CLK) struct clk phy_clk; #endif void __iomem *base; enum imx8mpq_phy_type type; };
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;
#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; } #endif
/* Disable rx term override */ value = readl(imx_phy->base + PHY_CTRL6); value &= ~PHY_CTRL6_RXTERM_OVERRIDE_SEL; writel(value, imx_phy->base + PHY_CTRL6); return 0;}
Personally, I find all of the CONFIG_IS_ENABLED stuff pretty confusing. I'm not sure who the expert is on those constructs... perhaps Simon or Tom?
Tom, yeah.
But then , if there is no board which does disable CLK or PHY, then ... just "depends" on those in Kconfig and zap all the if CONFIG ... goo and problem solved even better ?