[PATCH v4] phy: phy-imx8mq-usb: add vbus regulator support
Tim Harvey
tharvey at gateworks.com
Wed Jul 12 20:30:58 CEST 2023
On Wed, Jul 12, 2023 at 10:25 AM Marek Vasut <marex at denx.de> wrote:
>
> On 7/12/23 18:29, Tim Harvey wrote:
> > On Wed, Jul 12, 2023 at 8:10 AM Tim Harvey <tharvey at gateworks.com> wrote:
> >>
> >> On Wed, Jul 12, 2023 at 2:15 AM Marek Vasut <marex at denx.de> wrote:
> >>>
> >>> On 7/11/23 22:13, Tim Harvey wrote:
> >>>> On Mon, Jul 10, 2023 at 4:18 PM Marek Vasut <marex at 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 at gateworks.com>
> >>>>>> Reviewed-by: Adam Ford <aford173 at gmail.com>
> >>>>>> Reviewed-by: Marek Vasut <marex at 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?
best regards,
Tim
More information about the U-Boot
mailing list