* [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback
@ 2023-04-17 16:56 Patrice Chotard
2023-04-17 17:00 ` Marek Vasut
0 siblings, 1 reply; 9+ messages in thread
From: Patrice Chotard @ 2023-04-17 16:56 UTC (permalink / raw)
To: u-boot
Cc: Patrice CHOTARD, Patrick DELAUNAY, U-Boot STM32, Fabrice Gasnier,
Marek Vasut
In case USB hub regulator is shared, unexpected behavior occurs.
On stm32mp135f-dk, stm32mp157c-ev1 and stm32mp157x-dkx, regulator
v3v3 is shared between several IP/devices (USB, panel, ethernet phy,
camera, ...).
Running command "usb stop", v3v3 regulator is switched off and
the splashscreen content disappear.
v3v3 shouldn't be disabled on usb_onboard_hub_remove() callback.
Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
common/usb_onboard_hub.c | 13 -------------
1 file changed, 13 deletions(-)
diff --git a/common/usb_onboard_hub.c b/common/usb_onboard_hub.c
index 89e18a2ddad..8a83f7877ef 100644
--- a/common/usb_onboard_hub.c
+++ b/common/usb_onboard_hub.c
@@ -34,18 +34,6 @@ static int usb_onboard_hub_probe(struct udevice *dev)
return ret;
}
-static int usb_onboard_hub_remove(struct udevice *dev)
-{
- struct onboard_hub *hub = dev_get_priv(dev);
- int ret;
-
- ret = regulator_set_enable_if_allowed(hub->vdd, false);
- if (ret)
- dev_err(dev, "can't disable vdd-supply: %d\n", ret);
-
- return ret;
-}
-
static const struct udevice_id usb_onboard_hub_ids[] = {
/* Use generic usbVID,PID dt-bindings (usb-device.yaml) */
{ .compatible = "usb424,2514" }, /* USB2514B USB 2.0 */
@@ -56,7 +44,6 @@ U_BOOT_DRIVER(usb_onboard_hub) = {
.name = "usb_onboard_hub",
.id = UCLASS_USB_HUB,
.probe = usb_onboard_hub_probe,
- .remove = usb_onboard_hub_remove,
.of_match = usb_onboard_hub_ids,
.priv_auto = sizeof(struct onboard_hub),
};
--
2.25.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback
2023-04-17 16:56 [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback Patrice Chotard
@ 2023-04-17 17:00 ` Marek Vasut
2023-04-18 7:21 ` Patrice CHOTARD
0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2023-04-17 17:00 UTC (permalink / raw)
To: Patrice Chotard, u-boot; +Cc: Patrick DELAUNAY, U-Boot STM32, Fabrice Gasnier
On 4/17/23 18:56, Patrice Chotard wrote:
> In case USB hub regulator is shared, unexpected behavior occurs.
> On stm32mp135f-dk, stm32mp157c-ev1 and stm32mp157x-dkx, regulator
> v3v3 is shared between several IP/devices (USB, panel, ethernet phy,
> camera, ...).
> Running command "usb stop", v3v3 regulator is switched off and
> the splashscreen content disappear.
>
> v3v3 shouldn't be disabled on usb_onboard_hub_remove() callback.
Isn't the regulator enable/disable refcounted ?
If not, it should be, that would be the correct fix.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback
2023-04-17 17:00 ` Marek Vasut
@ 2023-04-18 7:21 ` Patrice CHOTARD
2023-04-18 8:25 ` Marek Vasut
2023-04-18 11:49 ` Fabio Estevam
0 siblings, 2 replies; 9+ messages in thread
From: Patrice CHOTARD @ 2023-04-18 7:21 UTC (permalink / raw)
To: Marek Vasut, u-boot; +Cc: Patrick DELAUNAY, U-Boot STM32, Fabrice Gasnier
Hi Marek
On 4/17/23 19:00, Marek Vasut wrote:
> On 4/17/23 18:56, Patrice Chotard wrote:
>> In case USB hub regulator is shared, unexpected behavior occurs.
>> On stm32mp135f-dk, stm32mp157c-ev1 and stm32mp157x-dkx, regulator
>> v3v3 is shared between several IP/devices (USB, panel, ethernet phy,
>> camera, ...).
>> Running command "usb stop", v3v3 regulator is switched off and
>> the splashscreen content disappear.
>>
>> v3v3 shouldn't be disabled on usb_onboard_hub_remove() callback.
>
> Isn't the regulator enable/disable refcounted ?
There is no refcount on regulator that's why we let regulator enable.
> If not, it should be, that would be the correct fix.
Thanks
Patrice
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback
2023-04-18 7:21 ` Patrice CHOTARD
@ 2023-04-18 8:25 ` Marek Vasut
2023-04-18 9:14 ` Patrice CHOTARD
2023-04-18 11:49 ` Fabio Estevam
1 sibling, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2023-04-18 8:25 UTC (permalink / raw)
To: Patrice CHOTARD, u-boot; +Cc: Patrick DELAUNAY, U-Boot STM32, Fabrice Gasnier
On 4/18/23 09:21, Patrice CHOTARD wrote:
> Hi Marek
Hello Patrice,
> On 4/17/23 19:00, Marek Vasut wrote:
>> On 4/17/23 18:56, Patrice Chotard wrote:
>>> In case USB hub regulator is shared, unexpected behavior occurs.
>>> On stm32mp135f-dk, stm32mp157c-ev1 and stm32mp157x-dkx, regulator
>>> v3v3 is shared between several IP/devices (USB, panel, ethernet phy,
>>> camera, ...).
>>> Running command "usb stop", v3v3 regulator is switched off and
>>> the splashscreen content disappear.
>>>
>>> v3v3 shouldn't be disabled on usb_onboard_hub_remove() callback.
>>
>> Isn't the regulator enable/disable refcounted ?
>
> There is no refcount on regulator that's why we let regulator enable.
Can we add that, instead of hacking around the missing refcount in every
driver ?
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback
2023-04-18 8:25 ` Marek Vasut
@ 2023-04-18 9:14 ` Patrice CHOTARD
2023-04-18 11:28 ` Marek Vasut
0 siblings, 1 reply; 9+ messages in thread
From: Patrice CHOTARD @ 2023-04-18 9:14 UTC (permalink / raw)
To: Marek Vasut, u-boot; +Cc: Patrick DELAUNAY, U-Boot STM32, Fabrice Gasnier
Hi Marek
On 4/18/23 10:25, Marek Vasut wrote:
> On 4/18/23 09:21, Patrice CHOTARD wrote:
>> Hi Marek
>
> Hello Patrice,
>
>> On 4/17/23 19:00, Marek Vasut wrote:
>>> On 4/17/23 18:56, Patrice Chotard wrote:
>>>> In case USB hub regulator is shared, unexpected behavior occurs.
>>>> On stm32mp135f-dk, stm32mp157c-ev1 and stm32mp157x-dkx, regulator
>>>> v3v3 is shared between several IP/devices (USB, panel, ethernet phy,
>>>> camera, ...).
>>>> Running command "usb stop", v3v3 regulator is switched off and
>>>> the splashscreen content disappear.
>>>>
>>>> v3v3 shouldn't be disabled on usb_onboard_hub_remove() callback.
>>>
>>> Isn't the regulator enable/disable refcounted ?
>>
>> There is no refcount on regulator that's why we let regulator enable.
>
> Can we add that, instead of hacking around the missing refcount in every driver ?
I agree, it should a nice solution but, there is a but ;-)
_ it's the same situation for clocks and resets, there is no ref counter.
_ what about side effects by adding ref counter where some platforms does
several enable on the same regulator (or even clock or reset) and only one
disable on it thinking that it's off ? or introduce new regulator_shared_xxx() API
to avoid side-effect with existing code.
_ unfortunately, currently, i didn't get enough bandwidth to address these points :-(.
Patrice
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback
2023-04-18 9:14 ` Patrice CHOTARD
@ 2023-04-18 11:28 ` Marek Vasut
0 siblings, 0 replies; 9+ messages in thread
From: Marek Vasut @ 2023-04-18 11:28 UTC (permalink / raw)
To: Patrice CHOTARD, u-boot
Cc: Patrick DELAUNAY, U-Boot STM32, Fabrice Gasnier, Kunihiko Hayashi
On 4/18/23 11:14, Patrice CHOTARD wrote:
> Hi Marek
Hello Patrice,
> On 4/18/23 10:25, Marek Vasut wrote:
>> On 4/18/23 09:21, Patrice CHOTARD wrote:
>>> Hi Marek
>>
>> Hello Patrice,
>>
>>> On 4/17/23 19:00, Marek Vasut wrote:
>>>> On 4/17/23 18:56, Patrice Chotard wrote:
>>>>> In case USB hub regulator is shared, unexpected behavior occurs.
>>>>> On stm32mp135f-dk, stm32mp157c-ev1 and stm32mp157x-dkx, regulator
>>>>> v3v3 is shared between several IP/devices (USB, panel, ethernet phy,
>>>>> camera, ...).
>>>>> Running command "usb stop", v3v3 regulator is switched off and
>>>>> the splashscreen content disappear.
>>>>>
>>>>> v3v3 shouldn't be disabled on usb_onboard_hub_remove() callback.
>>>>
>>>> Isn't the regulator enable/disable refcounted ?
>>>
>>> There is no refcount on regulator that's why we let regulator enable.
>>
>> Can we add that, instead of hacking around the missing refcount in every driver ?
>
> I agree, it should a nice solution but, there is a but ;-)
> _ it's the same situation for clocks and resets, there is no ref counter.
Those should indeed be fixed too. Hayashi-san from Socionext ran into
the same problem recently IIRC.
> _ what about side effects by adding ref counter where some platforms does
> several enable on the same regulator (or even clock or reset) and only one
> disable on it thinking that it's off ? or introduce new regulator_shared_xxx() API
> to avoid side-effect with existing code.
Such platforms should be fixed if they misuse the regulators.
> _ unfortunately, currently, i didn't get enough bandwidth to address these points :-(.
I also don't want to perpetuate these workarounds, can you find someone
to add the refcounting ? It shouldn't be that hard, probably a new
regulator struct member and ++/-- in enable/disable , maybe with some
underflow check here and there.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback
2023-04-18 7:21 ` Patrice CHOTARD
2023-04-18 8:25 ` Marek Vasut
@ 2023-04-18 11:49 ` Fabio Estevam
2023-04-18 11:53 ` Marek Vasut
1 sibling, 1 reply; 9+ messages in thread
From: Fabio Estevam @ 2023-04-18 11:49 UTC (permalink / raw)
To: Patrice CHOTARD
Cc: Marek Vasut, u-boot, Patrick DELAUNAY, U-Boot STM32, Fabrice Gasnier
Hi Patrice and Marek,
On Tue, Apr 18, 2023 at 4:22 AM Patrice CHOTARD
<patrice.chotard@foss.st.com> wrote:
> > Isn't the regulator enable/disable refcounted ?
>
> There is no refcount on regulator that's why we let regulator enable.
There is a recent patch from Eugen that adds regulator refcount support:
https://lore.kernel.org/u-boot/20230331091549.149095-1-eugen.hristev@collabora.com/
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback
2023-04-18 11:49 ` Fabio Estevam
@ 2023-04-18 11:53 ` Marek Vasut
2023-04-18 15:05 ` Patrice CHOTARD
0 siblings, 1 reply; 9+ messages in thread
From: Marek Vasut @ 2023-04-18 11:53 UTC (permalink / raw)
To: Fabio Estevam, Patrice CHOTARD
Cc: u-boot, Patrick DELAUNAY, U-Boot STM32, Fabrice Gasnier
On 4/18/23 13:49, Fabio Estevam wrote:
> Hi Patrice and Marek,
>
> On Tue, Apr 18, 2023 at 4:22 AM Patrice CHOTARD
> <patrice.chotard@foss.st.com> wrote:
>
>>> Isn't the regulator enable/disable refcounted ?
>>
>> There is no refcount on regulator that's why we let regulator enable.
>
> There is a recent patch from Eugen that adds regulator refcount support:
>
> https://lore.kernel.org/u-boot/20230331091549.149095-1-eugen.hristev@collabora.com/
That's nice !
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback
2023-04-18 11:53 ` Marek Vasut
@ 2023-04-18 15:05 ` Patrice CHOTARD
0 siblings, 0 replies; 9+ messages in thread
From: Patrice CHOTARD @ 2023-04-18 15:05 UTC (permalink / raw)
To: Marek Vasut, Fabio Estevam
Cc: u-boot, Patrick DELAUNAY, U-Boot STM32, Fabrice Gasnier
Hi
On 4/18/23 13:53, Marek Vasut wrote:
> On 4/18/23 13:49, Fabio Estevam wrote:
>> Hi Patrice and Marek,
>>
>> On Tue, Apr 18, 2023 at 4:22 AM Patrice CHOTARD
>> <patrice.chotard@foss.st.com> wrote:
>>
>>>> Isn't the regulator enable/disable refcounted ?
>>>
>>> There is no refcount on regulator that's why we let regulator enable.
>>
>> There is a recent patch from Eugen that adds regulator refcount support:
>>
>> https://lore.kernel.org/u-boot/20230331091549.149095-1-eugen.hristev@collabora.com/
>
> That's nice !
Planets are perfectly aligned ;-)
I cancel this patch, and will test the pointed series when it will be merged.
Thanks
Patrice
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2023-04-18 15:05 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-04-17 16:56 [PATCH] usb: onboard-hub: Don't disable regulator in remove() callback Patrice Chotard
2023-04-17 17:00 ` Marek Vasut
2023-04-18 7:21 ` Patrice CHOTARD
2023-04-18 8:25 ` Marek Vasut
2023-04-18 9:14 ` Patrice CHOTARD
2023-04-18 11:28 ` Marek Vasut
2023-04-18 11:49 ` Fabio Estevam
2023-04-18 11:53 ` Marek Vasut
2023-04-18 15:05 ` Patrice CHOTARD
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.