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

[Patrick DELAUNAY]

Hi Marek,

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