Hello Simon,
On 05/12/2024 at 10:56:44 -07, Simon Glass <s...@chromium.org> wrote:
> Hi Miquel,
>
> On Thu, 5 Dec 2024 at 06:54, Miquel Raynal <miquel.ray...@bootlin.com> wrote:
>>
>> It is very surprising that such an uclass, specifically designed to
>> handle resources that may be shared by different devices, is not keeping
>> the count of the number of times a power domain has been
>> enabled/disabled to avoid shutting it down unexpectedly or disabling it
>> several times.
>>
>> Doing this causes troubles on eg. i.MX8MP because disabling power
>> domains can be done in a recursive loops were the same power domain
>> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
>> timings to respect and it is easy to produce a race condition that puts
>> the power domains in an unstable state, leading to ADB400 errors and
>> later crashes in Linux.
>>
>> Signed-off-by: Miquel Raynal <miquel.ray...@bootlin.com>
>> ---
>> drivers/power/domain/power-domain-uclass.c | 37
>> ++++++++++++++++++++++++++++--
>> include/power-domain.h | 4 ++--
>> 2 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/power/domain/power-domain-uclass.c
>> b/drivers/power/domain/power-domain-uclass.c
>> index
>> 938bd8cbc9ffd1ba2109d702f886b6a99288d063..55a3d95d83c5aaac31acbd877199c87f8f6fb880
>> 100644
>> --- a/drivers/power/domain/power-domain-uclass.c
>> +++ b/drivers/power/domain/power-domain-uclass.c
>> @@ -12,6 +12,10 @@
>> #include <power-domain-uclass.h>
>> #include <dm/device-internal.h>
>>
>> +struct power_domain_priv {
>> + int on_count;
>> +};
>> +
>> static inline struct power_domain_ops *power_domain_dev_ops(struct udevice
>> *dev)
>> {
>> return (struct power_domain_ops *)dev->driver->ops;
>> @@ -110,19 +114,47 @@ int power_domain_free(struct power_domain
>> *power_domain)
>> int power_domain_on(struct power_domain *power_domain)
>> {
>> struct power_domain_ops *ops =
>> power_domain_dev_ops(power_domain->dev);
>> + struct power_domain_priv *priv =
>> dev_get_uclass_priv(power_domain->dev);
>> + int ret;
>>
>> debug("%s(power_domain=%p)\n", __func__, power_domain);
>>
>> - return ops->on ? ops->on(power_domain) : 0;
>> + if (priv->on_count++ > 0)
>> + return 0;
>
> -EALREADY - see drivers/power/regulator/regulator-uclass.c
Actually I don't see the point in returning this? What is the purpose?
Users should not care nor know about this, it is all internal to the
uclass, we do _not_ want users to mess with this.
If someone ever needs it, they can add a layer of "hiding" for all users
but them, but as said just above, I really think it is a bad idea to
propagate this outside of the uclass.
(same in the _off path).
Thanks,
Miquèl
