| Message ID | 20191014080025.11245-2-patrick.delaunay@st.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | usb: host: dwc2: use driver model for PHY and CLOCK | expand |
On 10/14/19 10:00 AM, Patrick Delaunay wrote: > Use generic phy to initialize the PHY associated to the PHY and USB are abbreviations, should be in capitals. > DWC2 device and available in the device tree. [...] General question -- is the PHY subsystem a mandatory dependency of this driver now or will it work without the PHY subsystem still ? > +static int dwc2_setup_phy(struct udevice *dev) > +{ > + struct dwc2_priv *priv = dev_get_priv(dev); > + int ret; > + > + ret = generic_phy_get_by_index(dev, 0, &priv->phy); > + if (ret) { > + if (ret != -ENOENT) { > + dev_err(dev, "failed to get usb phy\n"); Sentence starts with capital letter, USB and PHY are in capitals. Fix globally please. It would be useful to print the $ret value too. > + return ret; > + } > + return 0; > + } > + > + ret = generic_phy_init(&priv->phy); > + if (ret) { > + dev_err(dev, "failed to init usb phy\n"); > + return ret; > + } > + > + ret = generic_phy_power_on(&priv->phy); > + if (ret) { > + dev_err(dev, "failed to power on usb phy\n"); > + return generic_phy_exit(&priv->phy); > + } > + > + return 0; > +} > + > +static int dwc2_shutdown_phy(struct udevice *dev) > +{ > + struct dwc2_priv *priv = dev_get_priv(dev); > + int ret; > + > + if (!generic_phy_valid(&priv->phy)) > + return 0; > + > + ret = generic_phy_power_off(&priv->phy); > + if (ret) { > + dev_err(dev, "failed to power off usb phy\n"); > + return ret; > + } > + > + ret = generic_phy_exit(&priv->phy); > + if (ret) { > + dev_err(dev, "failed to power off usb phy\n"); Shouldn't all those error prints be produced by the PHY subsystem ? > + return ret; [...] > @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev) > if (ret) > return ret; > > + dwc2_shutdown_phy(dev); This function returns a return value, but it's ignored here ? > dwc2_uninit_common(priv->regs); > > reset_release_bulk(&priv->resets); > [...]
Hi Marek, > From: Marek Vasut <marex@denx.de> > Sent: mardi 15 octobre 2019 01:27 First sorry for my late answer.... > On 10/14/19 10:00 AM, Patrick Delaunay wrote: > > Use generic phy to initialize the PHY associated to the > > PHY and USB are abbreviations, should be in capitals. > > > DWC2 device and available in the device tree. > > [...] > > General question -- is the PHY subsystem a mandatory dependency of this driver > now or will it work without the PHY subsystem still ? Normally it is working as all the generic_phy_XXX fucntions are stubbed in include/generic-phy.h - generic_phy_get_by_index() return 0 and phy->dev = NULL - all other function return 0 - generic_phy_valid return FALSE (phy->dev = NULL) > > +static int dwc2_setup_phy(struct udevice *dev) { > > + struct dwc2_priv *priv = dev_get_priv(dev); > > + int ret; > > + > > + ret = generic_phy_get_by_index(dev, 0, &priv->phy); > > + if (ret) { > > + if (ret != -ENOENT) { > > + dev_err(dev, "failed to get usb phy\n"); > > Sentence starts with capital letter, USB and PHY are in capitals. Fix globally > please. > It would be useful to print the $ret value too. Yes, in V2 > > > + return ret; > > + } > > + return 0; > > + } > > + > > + ret = generic_phy_init(&priv->phy); > > + if (ret) { > > + dev_err(dev, "failed to init usb phy\n"); > > + return ret; > > + } > > + > > + ret = generic_phy_power_on(&priv->phy); > > + if (ret) { > > + dev_err(dev, "failed to power on usb phy\n"); > > + return generic_phy_exit(&priv->phy); > > + } > > + > > + return 0; > > +} > > + > > +static int dwc2_shutdown_phy(struct udevice *dev) { > > + struct dwc2_priv *priv = dev_get_priv(dev); > > + int ret; > > + > > + if (!generic_phy_valid(&priv->phy)) > > + return 0; > > + > > + ret = generic_phy_power_off(&priv->phy); > > + if (ret) { > > + dev_err(dev, "failed to power off usb phy\n"); > > + return ret; > > + } > > + > > + ret = generic_phy_exit(&priv->phy); > > + if (ret) { > > + dev_err(dev, "failed to power off usb phy\n"); > > Shouldn't all those error prints be produced by the PHY subsystem ? Perhaps... but it is not done today in phy u-class (only call ops). I make the same level of trace than ./drivers/usb/dwc3/core.c as copy initially the phy support from this driver. > > + return ret; > > [...] > > > @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev) > > if (ret) > > return ret; > > > > + dwc2_shutdown_phy(dev); > > This function returns a return value, but it's ignored here ? Yes, even if the shutdown of the USB PHY failed, the USB dwc2 driver continues the procedure to release other ressources... And the driver expects that the USB PHY will be available for next request/probe (recovery with phy reset for example). I use the same logic than dwc3 driver in : source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove() drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove() > > > dwc2_uninit_common(priv->regs); > > > > reset_release_bulk(&priv->resets); > > > > [...] Regards Patrick
On 11/6/19 6:40 PM, Patrick DELAUNAY wrote: > Hi Marek, Hi, [...] >>> +static int dwc2_shutdown_phy(struct udevice *dev) { >>> + struct dwc2_priv *priv = dev_get_priv(dev); >>> + int ret; >>> + >>> + if (!generic_phy_valid(&priv->phy)) >>> + return 0; >>> + >>> + ret = generic_phy_power_off(&priv->phy); >>> + if (ret) { >>> + dev_err(dev, "failed to power off usb phy\n"); >>> + return ret; >>> + } >>> + >>> + ret = generic_phy_exit(&priv->phy); >>> + if (ret) { >>> + dev_err(dev, "failed to power off usb phy\n"); >> >> Shouldn't all those error prints be produced by the PHY subsystem ? > > Perhaps... but it is not done today in phy u-class (only call ops). > > I make the same level of trace than ./drivers/usb/dwc3/core.c > as copy initially the phy support from this driver. So this starts the duplication. Can you move it to the PHY subsystem instead ? >>> + return ret; >> >> [...] >> >>> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev) >>> if (ret) >>> return ret; >>> >>> + dwc2_shutdown_phy(dev); >> >> This function returns a return value, but it's ignored here ? > > Yes, even if the shutdown of the USB PHY failed, the USB dwc2 > driver continues the procedure to release other ressources... How can you safely release the rest of the resources if the PHY driver didn't shut down? I suspect this might lead to some resource corruption, no? > And the driver expects that the USB PHY will be available for next > request/probe (recovery with phy reset for example). > > I use the same logic than dwc3 driver in : > source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove() > drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove() dwc3_shutdown_phy() only ever returns 0 though.
Hi, > From: Marek Vasut <marex@denx.de> > > On 11/6/19 6:40 PM, Patrick DELAUNAY wrote: > > Hi Marek, > > Hi, > > [...] > > >>> +static int dwc2_shutdown_phy(struct udevice *dev) { > >>> + struct dwc2_priv *priv = dev_get_priv(dev); > >>> + int ret; > >>> + > >>> + if (!generic_phy_valid(&priv->phy)) > >>> + return 0; > >>> + > >>> + ret = generic_phy_power_off(&priv->phy); > >>> + if (ret) { > >>> + dev_err(dev, "failed to power off usb phy\n"); > >>> + return ret; > >>> + } > >>> + > >>> + ret = generic_phy_exit(&priv->phy); > >>> + if (ret) { > >>> + dev_err(dev, "failed to power off usb phy\n"); > >> > >> Shouldn't all those error prints be produced by the PHY subsystem ? > > > > Perhaps... but it is not done today in phy u-class (only call ops). > > > > I make the same level of trace than ./drivers/usb/dwc3/core.c as copy > > initially the phy support from this driver. > > So this starts the duplication. Can you move it to the PHY subsystem instead ? Yes I can, in v2 I will change dev_err to dev_dbg And I will sent a other serie to change the generic phy (add printf or dev_err) and also remove the dev_err for all the caller to avoid duplicated trace. This generic error is already done in some U-Boot uclass, - clock (clk_enable) But sometime only the caller, the driver, knows if it is a error or a warning, and it is not done for others uclass, for example: - Reset: reset_assert/ reset_deassert reset_assert_bulk/ reset_deassert_bulk - Regulator: regulator_set_enable > >>> + return ret; > >> > >> [...] > >> > >>> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev) > >>> if (ret) > >>> return ret; > >>> > >>> + dwc2_shutdown_phy(dev); > >> > >> This function returns a return value, but it's ignored here ? > > > > Yes, even if the shutdown of the USB PHY failed, the USB dwc2 driver > > continues the procedure to release other ressources... > > How can you safely release the rest of the resources if the PHY driver didn't shut > down? I suspect this might lead to some resource corruption, no? Yes...and that depends of the PHY driver. What it is better stategy: - try to continue to release the resources after the first error and the next probe could works / the error is masked Or - stopped the release procedure => the next procedure could failed (resource not available) > > And the driver expects that the USB PHY will be available for next > > request/probe (recovery with phy reset for example). > > > > I use the same logic than dwc3 driver in : > > source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove() > > drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove() > > dwc3_shutdown_phy() only ever returns 0 though. Yes, but in dwc3_shutdown_phy, the phy operation can have errors and the "remove" procedure continue (even if ret is never retruned) ret = generic_phy_power_off(&usb_phys[i]); ret |= generic_phy_exit(&usb_phys[i]); if (ret) { pr_err("Can't shutdown USB PHY%d for %s\n", i, dev->name); } Anyway I will treat error in v2, it should be more clear in dw2c code. + ret= dwc2_shutdown_phy(dev); + if (ret) { + dev_dbg(dev, "Failed to shutdown USB PHY: %d.\n": ret); + return ret; + } > -- > Best regards, > Marek Vasut Regards Patrick
On 11/8/19 2:25 PM, Patrick DELAUNAY wrote: Hi, [...] >>>>> +static int dwc2_shutdown_phy(struct udevice *dev) { >>>>> + struct dwc2_priv *priv = dev_get_priv(dev); >>>>> + int ret; >>>>> + >>>>> + if (!generic_phy_valid(&priv->phy)) >>>>> + return 0; >>>>> + >>>>> + ret = generic_phy_power_off(&priv->phy); >>>>> + if (ret) { >>>>> + dev_err(dev, "failed to power off usb phy\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + ret = generic_phy_exit(&priv->phy); >>>>> + if (ret) { >>>>> + dev_err(dev, "failed to power off usb phy\n"); >>>> >>>> Shouldn't all those error prints be produced by the PHY subsystem ? >>> >>> Perhaps... but it is not done today in phy u-class (only call ops). >>> >>> I make the same level of trace than ./drivers/usb/dwc3/core.c as copy >>> initially the phy support from this driver. >> >> So this starts the duplication. Can you move it to the PHY subsystem instead ? > > Yes I can, in v2 I will change dev_err to dev_dbg > > And I will sent a other serie to change the generic phy (add printf or dev_err) > and also remove the dev_err for all the caller to avoid duplicated trace. Thanks [...] >>>>> @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev) >>>>> if (ret) >>>>> return ret; >>>>> >>>>> + dwc2_shutdown_phy(dev); >>>> >>>> This function returns a return value, but it's ignored here ? >>> >>> Yes, even if the shutdown of the USB PHY failed, the USB dwc2 driver >>> continues the procedure to release other ressources... >> >> How can you safely release the rest of the resources if the PHY driver didn't shut >> down? I suspect this might lead to some resource corruption, no? > > Yes...and that depends of the PHY driver. Does it ? > What it is better stategy: > - try to continue to release the resources after the first error and the next probe could works / the error is masked > Or > - stopped the release procedure => the next procedure could failed (resource not available) Stop the release procedure, because then at least the dynamically allocated data are still tracked by allocator and if the driver so decided to use them, they will still exist. If you were to release them, then the driver could either misbehave and/or cause memory corruption. >>> And the driver expects that the USB PHY will be available for next >>> request/probe (recovery with phy reset for example). >>> >>> I use the same logic than dwc3 driver in : >>> source/drivers/usb/dwc3/dwc3-generic.c::dwc3_generic_remove() >>> drivers/usb/host/xhci-dwc3.c::xhci_dwc3_remove() >> >> dwc3_shutdown_phy() only ever returns 0 though. > > Yes, but in dwc3_shutdown_phy, the phy operation can have errors > and the "remove" procedure continue (even if ret is never retruned) > > ret = generic_phy_power_off(&usb_phys[i]); > ret |= generic_phy_exit(&usb_phys[i]); > if (ret) { > pr_err("Can't shutdown USB PHY%d for %s\n", i, dev->name); > } > > Anyway I will treat error in v2, it should be more clear in dw2c code. That doesn't sound right. [...]
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 350d820a6e..eb1026effc 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -7,6 +7,7 @@ #include <common.h> #include <dm.h> #include <errno.h> +#include <generic-phy.h> #include <usb.h> #include <malloc.h> #include <memalign.h> @@ -35,6 +36,7 @@ struct dwc2_priv { #ifdef CONFIG_DM_REGULATOR struct udevice *vbus_supply; #endif + struct phy phy; #else uint8_t *aligned_buffer; uint8_t *status_buffer; @@ -1320,13 +1322,70 @@ static int dwc2_usb_ofdata_to_platdata(struct udevice *dev) return 0; } +static int dwc2_setup_phy(struct udevice *dev) +{ + struct dwc2_priv *priv = dev_get_priv(dev); + int ret; + + ret = generic_phy_get_by_index(dev, 0, &priv->phy); + if (ret) { + if (ret != -ENOENT) { + dev_err(dev, "failed to get usb phy\n"); + return ret; + } + return 0; + } + + ret = generic_phy_init(&priv->phy); + if (ret) { + dev_err(dev, "failed to init usb phy\n"); + return ret; + } + + ret = generic_phy_power_on(&priv->phy); + if (ret) { + dev_err(dev, "failed to power on usb phy\n"); + return generic_phy_exit(&priv->phy); + } + + return 0; +} + +static int dwc2_shutdown_phy(struct udevice *dev) +{ + struct dwc2_priv *priv = dev_get_priv(dev); + int ret; + + if (!generic_phy_valid(&priv->phy)) + return 0; + + ret = generic_phy_power_off(&priv->phy); + if (ret) { + dev_err(dev, "failed to power off usb phy\n"); + return ret; + } + + ret = generic_phy_exit(&priv->phy); + if (ret) { + dev_err(dev, "failed to power off usb phy\n"); + return ret; + } + + return 0; +} + static int dwc2_usb_probe(struct udevice *dev) { struct dwc2_priv *priv = dev_get_priv(dev); struct usb_bus_priv *bus_priv = dev_get_uclass_priv(dev); + int ret; bus_priv->desc_before_addr = true; + ret = dwc2_setup_phy(dev); + if (ret) + return ret; + return dwc2_init_common(dev, priv); } @@ -1339,6 +1398,8 @@ static int dwc2_usb_remove(struct udevice *dev) if (ret) return ret; + dwc2_shutdown_phy(dev); + dwc2_uninit_common(priv->regs); reset_release_bulk(&priv->resets);
Use generic phy to initialize the PHY associated to the DWC2 device and available in the device tree. Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- drivers/usb/host/dwc2.c | 61 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 61 insertions(+)