[U-Boot] [RESEND PATCH 1/5] usb: host: dwc2: add phy support

[Patrick DELAUNAY]

Hi,

> From: Marek Vasut <marex at 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