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.