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].
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))'
Tim [1] https://source.denx.de/u-boot/custodians/u-boot-imx/-/commits/master-next