| Message ID | 20230417185558.1.If7ed2ccb5a1c1a84637d29d763cc1935d9b8815e@changeid |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | usb: onboard-hub: Don't disable regulator in remove() callback | expand |
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.
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
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 ?
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
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.
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/
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 !
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
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), };
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(-)