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

[Marek Vasut]

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.

[...]