| Message ID | 20230404141155.283987-3-eugen.hristev@collabora.com |
|---|---|
| State | Superseded |
| Delegated to: | Kever Yang |
| Headers | show |
| Series | [v4,1/5] ARM: dts: rockchip: rk3588-rock-5b-u-boot: add USB 2.0 host | expand |
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.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",
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", >
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", >> >
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",
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. Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com> --- drivers/phy/phy-uclass.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)