On Mon, May 1, 2023 at 1:12 AM Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/30/23 21:21, Jonas Karlman wrote:
Hi Tim, On 2023-04-28 18:36, Tim Harvey wrote:
On Fri, Apr 28, 2023 at 12:39 AM Eugen Hristev eugen.hristev@collabora.com wrote:
On 4/28/23 02:39, Tim Harvey wrote:
On Wed, Apr 19, 2023 at 6:45 AM Eugen Hristev eugen.hristev@collabora.com wrote:
Some devices share a regulator supply, when the first one will request regulator disable, the second device will have it's supply cut off before graciously shutting down. Hence there will be timeouts and other failed operations. Implement a reference counter mechanism similar with what is done in Linux, to keep track of enable and disable requests, and only disable the regulator when the last of the consumers has requested shutdown.
Signed-off-by: Eugen Hristev eugen.hristev@collabora.com Reviewed-by: Simon Glass sjg@chromium.org
Changes in v5:
- none
Changes in v4:
- add documentation for error codes
Changes in v3:
- add error return codes
Changes in v2:
- add info in header regarding the function
drivers/power/regulator/regulator_common.c | 22 ++++++++++++++++++++++ drivers/power/regulator/regulator_common.h | 21 +++++++++++++++++++++ 2 files changed, 43 insertions(+)
diff --git a/drivers/power/regulator/regulator_common.c b/drivers/power/regulator/regulator_common.c index 93d8196b381e..484a4fc31ef7 100644 --- a/drivers/power/regulator/regulator_common.c +++ b/drivers/power/regulator/regulator_common.c @@ -73,6 +73,23 @@ int regulator_common_set_enable(const struct udevice *dev, return 0; }
/* If previously enabled, increase count */if (enable && dev_pdata->enable_count > 0) {dev_pdata->enable_count++;return -EALREADY;}if (!enable) {if (dev_pdata->enable_count > 1) {/* If enabled multiple times, decrease count */dev_pdata->enable_count--;return -EBUSY;} else if (!dev_pdata->enable_count) {/* If already disabled, do nothing */return -EALREADY;}}Eugen,
Thank you for working on this series!
Hi Tim,
Thank you for testing and having a look on my patches.
I wonder if instead of returning a failure you should print an error here but return 0 in order to not break unbalanced regulator calls
Initially I did that, but Simon forced me to return error as you see now.
Ok, I see that discussion here: https://patchwork.ozlabs.org/project/uboot/patch/20230328130127.20279-1-euge...
(like what is done in clk_disable()). I see that you have another patch in this series which handles htis for regulator_set_enable_if_allowed() but that doesn't cover drivers that call regulator_common_set_enable() directly such as drivers/power/regulator/fixed.c and drivers/power/regulator/gpio-regulator.c.
I believe Jonas (in CC) fixed those with a patch, but at the moment I am lost in providing you a link to it
Are you thinking of "pci: pcie_dw_rockchip: Use regulator_set_enable_if_allowed" https://patchwork.ozlabs.org/project/uboot/patch/20230422181943.889436-3-jon... ?
I added some debug prints to regulator_set_enable and see: starting USB... Bus usb@32e40000: regulator_set_enable regulator-usb-otg1 enable regulator_set_enable regulator-usb-otg1 enable ret=0 Bus usb@32e50000: regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=0 regulator_set_enable regulator-usb-otg2 enable regulator_set_enable regulator-usb-otg2 enable ret=-114 Error enabling VBUS supply (ret=-114) regulator_set_enable regulator-usb-otg2 disable regulator_set_enable regulator-usb-otg2 disable ret=-16 probe failed, error -114
So clearly something is trying to enable regulator-usb-otg2 when it's already enabled but I don't think that should be considered an error and cause a failure.
Simon, is this what you were intending with your feedback?
I know there is an unbalanced call currently on imx8mm that this patch causes a failure where it previously did not: u-boot=> usb start && usb tree starting USB... Bus usb@32e40000: Bus usb@32e50000: Error enabling VBUS supply (ret=-114) probe failed, error -114
That's a good catch. I expected such things would happen if I would return error in those cases, so it might be that this is not the only case. If you are able to test that board, do you wish me to send you a patch that changes the code to using regulator_set_enable_if_allowed() ?
Yes, I can test. Are you thinking changing the calls to regulator_common_set_enable (used in drivers/power/regulator/fixed{gpio-regulator,regulator_common} to a wrapper calling regulator_common_set_enable_if_allowed instead? I think that would just be the same thing as removing the error as no callers of that function would be left.
This is nothing that should be changed in the fixed/gpio/common regulator code. Such change should happen in the drivers that call the regulator_set_enable function. In your case the usb driver that tries to enable/disable the regulator.
Your series solves a necessary issue where a regulator_disable call would disable a regulator that was still being used by another consumer but I don't think it should break calls to regulator_enable just because another consumer is also using it.
With this patch at least fixed/gpio regulators have a basic reference counter and because of this the regulator_set_enable may now return a more detailed errno, "0 on success or -errno val if fails".
The call to regulator_set_enable can be replaced with a call to regulator_set_enable_if_allowed when the caller requires less strict errors, "0 on success or if enabling is not supported or -errno val if fails."
I think that the best option is to replace calls to regulator_set_enable with regulator_set_enable_if_allowed.
Tim, what is the driver that is now broken ? I will try to create a patch and send it your way so you can test ?
The breakage I saw was from drivers/usb/host/ehci-mx6.c with several calls to enable the vbus_supply. I assume there are many other drivers that would have the same problem as there are just over 100 calls to regulator_set_enable throughout U-Boot. I'm not sure I understand what cases there are where a call to enable a regulator should fail just because another consumer still needs it.
Best Regards,
Tim
Another option could be to also relax the errors returned by regulator_set_enable, and instead print an error message for the two new errno.
Regards, Jonas
Best Regards,
Tim