Re: [PATCH v4 3/5] phy: add support for phy-supply

On 4/12/23 09:53, Jonas Karlman wrote:

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.

While indeed there is a change, I do not want to stop when there is an error with phy supply. First, the enabling of the regulator could return EALREADY on enable, or EBUSY on disable, which is not really an error, so this has to be treated accordingly Second, all this time the code has lived without phy supply except for some few drivers, so, if now we stop and break on phy supply not working, we may break a lot of other boards which simply ignored phy supply until today. Because of that, my whole patch has treated phy supply as basically an optional property which is requested, but not mandatory to continue without.

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 :-)

do you want to add this second patch to my series ?

and squash the first into my patches with you as co-author ? but I will change a bit what you wrote, namely handling EBUSY and EALREADY e.g.

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.hristev@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",