Hi,
From: Marek Vasut marex@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