From: Jonas Karlman <jonas@kwiboo.se>
To: Eugen Hristev <eugen.hristev@collabora.com>
Cc: u-boot-amlogic@groups.io, kever.yang@rock-chips.com,
neil.armstrong@linaro.org, jagan@edgeble.ai, xdrudis@tinet.cat,
anarsoul@gmail.com, u-boot@lists.denx.de
Subject: Re: [PATCH v4 3/5] phy: add support for phy-supply
Date: Wed, 12 Apr 2023 11:24:26 +0000 (UTC) [thread overview]
Message-ID: <d9f1875f-dd1c-3cc9-37da-58872b436229@kwiboo.se> (raw)
In-Reply-To: <18455f38-123c-12de-b72d-a83444cdd26a@collabora.com>
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@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",
>>
>
next prev parent reply other threads:[~2023-04-12 11:24 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-04-04 14:11 [PATCH v4 1/5] ARM: dts: rockchip: rk3588-rock-5b-u-boot: add USB 2.0 host Eugen Hristev
2023-04-04 14:11 ` [PATCH v4 2/5] configs: rockchip: rock5b-rk3588: enable USB and regulators Eugen Hristev
2023-04-04 14:11 ` [PATCH v4 3/5] phy: add support for phy-supply Eugen Hristev
2023-04-12 6:53 ` Jonas Karlman
2023-04-12 7:45 ` Eugen Hristev
2023-04-12 11:24 ` Jonas Karlman [this message]
2023-04-04 14:11 ` [PATCH v4 4/5] phy: remove phy-supply related code Eugen Hristev
2023-04-04 14:11 ` [PATCH v4 5/5] phy: rockchip-inno-usb2: add initial support for rk3588 PHY Eugen Hristev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d9f1875f-dd1c-3cc9-37da-58872b436229@kwiboo.se \
--to=jonas@kwiboo.se \
--cc=anarsoul@gmail.com \
--cc=eugen.hristev@collabora.com \
--cc=jagan@edgeble.ai \
--cc=kever.yang@rock-chips.com \
--cc=neil.armstrong@linaro.org \
--cc=u-boot-amlogic@groups.io \
--cc=u-boot@lists.denx.de \
--cc=xdrudis@tinet.cat \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).