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

[Marek Vasut]

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.

-- 
Best regards,
Marek Vasut