Hi,
> From: Marek Vasut <[email protected]>
>
> 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
_______________________________________________
U-Boot mailing list
[email protected]
https://lists.denx.de/listinfo/u-boot
