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