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

Jonas Karlman jonas at kwiboo.se
Wed Apr 12 13:24:26 CEST 2023


Hi Eugen,

On 2023-04-12 09:45, Eugen Hristev wrote:
> 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.

Understandable, and I was expecting that regulator_set_enable_if_allowed
handled that, it return -ENOSYS when it is unsupported or 0 on anything
that could be treated like a success and only an error if there is an
error while changing the regulator.

Looking at the phy-supply defined in device trees in u-boot it is mostly
used for ethernet or usb on imx, meson, rockchip and sunxi. So hopefully
a relaxed error handling should not affect that much and could help
keeping the regulator enable_count balanced.

> 
>>
>> 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.

Please pick/squash as you see best fit, no need for co-author :-)

The counts balance change was inspired by linux drivers/phy/phy-core.c

Regards,
Jonas

> 
>> 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 at 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",
>>
> 



More information about the U-Boot mailing list