Hi Eugen, On 2023-04-04 16:11, Eugen Hristev wrote: > Some phys require a phy-supply property that is a phandle to a regulator > that needs to be enabled for phy operations. > Implement basic supply lookup, enable and disabling, if DM_REGULATOR is > available.
Thanks for looking at this, I have now had some time to test this and it turned out there was some minor issues. First, the regulator is enabled in power_on and disabled in exit. It should probably be disabled after power_off ops has been called. Second, the return value is ignored, this will change the meson code to continue with the call to the power_on ops, before it would have stopped and returned early with an error from the power_on ops. Third, there seem to be a possibility that the counts in regulator core can end up uneven when any of init/exit/power_on/power_off ops is NULL. I created "fixup! phy: add support for phy-supply" and "phy: Keep balance of counts when ops is missing" at [1] during my testing, please feel free to fixup/squash/rework any code :-) Tested with USB (multiple usb start/reset/stop cycles) and PCIe (multiple pci enum) on RK3568 together with your "regulator: implement basic reference counter". [1] https://github.com/Kwiboo/u-boot-rockchip/commits/rk3568-usb-v1 Regards, Jonas > > Signed-off-by: Eugen Hristev <eugen.hris...@collabora.com> > --- > drivers/phy/phy-uclass.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/phy/phy-uclass.c b/drivers/phy/phy-uclass.c > index 3fef5135a9cb..475ac285df05 100644 > --- a/drivers/phy/phy-uclass.c > +++ b/drivers/phy/phy-uclass.c > @@ -12,6 +12,7 @@ > #include <dm/devres.h> > #include <generic-phy.h> > #include <linux/list.h> > +#include <power/regulator.h> > > /** > * struct phy_counts - Init and power-on counts of a single PHY port > @@ -29,12 +30,14 @@ > * without a matching generic_phy_exit() afterwards > * @list: Handle for a linked list of these structures corresponding to > * ports of the same PHY provider > + * @supply: Handle to a phy-supply device > */ > struct phy_counts { > unsigned long id; > int power_on_count; > int init_count; > struct list_head list; > + struct udevice *supply; > }; > > static inline struct phy_ops *phy_dev_ops(struct udevice *dev) > @@ -224,6 +227,12 @@ int generic_phy_init(struct phy *phy) > return 0; > } > > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > + device_get_supply_regulator(phy->dev, "phy-supply", &counts->supply); > + if (IS_ERR(counts->supply)) > + dev_dbg(phy->dev, "no phy-supply property found\n"); > +#endif > + > ret = ops->init(phy); > if (ret) > dev_err(phy->dev, "PHY: Failed to init %s: %d.\n", > @@ -272,6 +281,12 @@ int generic_phy_exit(struct phy *phy) > return 0; > } > > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > + if (!IS_ERR_OR_NULL(counts->supply)) { > + ret = regulator_set_enable(counts->supply, false); > + dev_dbg(phy->dev, "supply disable status: %d\n", ret); > + } > +#endif > ret = ops->exit(phy); > if (ret) > dev_err(phy->dev, "PHY: Failed to exit %s: %d.\n", > @@ -300,6 +315,13 @@ int generic_phy_power_on(struct phy *phy) > return 0; > } > > +#if CONFIG_IS_ENABLED(DM_REGULATOR) > + if (!IS_ERR_OR_NULL(counts->supply)) { > + ret = regulator_set_enable(counts->supply, true); > + dev_dbg(phy->dev, "supply enable status: %d\n", ret); > + } > +#endif > + > ret = ops->power_on(phy); > if (ret) > dev_err(phy->dev, "PHY: Failed to power on %s: %d.\n",