| Message ID | 20200825021120.4926-2-peter.chen@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: cdns3: add runtime pm support | expand |
Peter Chen <peter.chen@nxp.com> writes: > Since we have both USB2 and USB3 PHYs for cdns3 controller, it is > better we have unity APIs to handle both USB2 and USB3's power, it > could simplify code for error handling and further power management > implementation. > > Reviewed-by: Pawel Laszczak <pawell@cadence.com> > Reviewed-by: Jun Li <jun.li@nxp.com> > Signed-off-by: Peter Chen <peter.chen@nxp.com> > --- > drivers/usb/cdns3/core.c | 43 ++++++++++++++++++++++++++-------------- > 1 file changed, 28 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > index 5c1586ec7824..e56dbb6a898c 100644 > --- a/drivers/usb/cdns3/core.c > +++ b/drivers/usb/cdns3/core.c > @@ -371,6 +371,27 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role) > return ret; > } > > +static int set_phy_power_on(struct cdns3 *cdns) care to keep the cdns3_ prefix? It's useful when you want to use e.g. ftrace and function_graph trace > +{ > + int ret; > + > + ret = phy_power_on(cdns->usb2_phy); > + if (ret) > + return ret; > + > + ret = phy_power_on(cdns->usb3_phy); let's say we fail here > + if (ret) > + phy_power_off(cdns->usb2_phy); usb2_phy will be powered off.. > + return ret; > +} > + > +static void set_phy_power_off(struct cdns3 *cdns) > +{ > + phy_power_off(cdns->usb3_phy); > + phy_power_off(cdns->usb2_phy); Won't this cause a problem?
On 20-08-27 16:09:36, Felipe Balbi wrote: > Peter Chen <peter.chen@nxp.com> writes: > > > Since we have both USB2 and USB3 PHYs for cdns3 controller, it is > > better we have unity APIs to handle both USB2 and USB3's power, it > > could simplify code for error handling and further power management > > implementation. > > > > Reviewed-by: Pawel Laszczak <pawell@cadence.com> > > Reviewed-by: Jun Li <jun.li@nxp.com> > > Signed-off-by: Peter Chen <peter.chen@nxp.com> > > --- > > drivers/usb/cdns3/core.c | 43 ++++++++++++++++++++++++++-------------- > > 1 file changed, 28 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c > > index 5c1586ec7824..e56dbb6a898c 100644 > > --- a/drivers/usb/cdns3/core.c > > +++ b/drivers/usb/cdns3/core.c > > @@ -371,6 +371,27 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role) > > return ret; > > } > > > > +static int set_phy_power_on(struct cdns3 *cdns) > > care to keep the cdns3_ prefix? It's useful when you want to use > e.g. ftrace and function_graph trace I original added it, but Greg suggested it is not needed. https://www.spinics.net/lists/linux-usb/msg197076.html > > > +{ > > + int ret; > > + > > + ret = phy_power_on(cdns->usb2_phy); > > + if (ret) > > + return ret; > > + > > + ret = phy_power_on(cdns->usb3_phy); > > let's say we fail here > > > + if (ret) > > + phy_power_off(cdns->usb2_phy); > > usb2_phy will be powered off.. If usb3_phy's power_on has failed, it powers off usb2 phy's power which is powered on before, and return fail. > > > + return ret; > > +} > > + > > +static void set_phy_power_off(struct cdns3 *cdns) > > +{ > > + phy_power_off(cdns->usb3_phy); > > + phy_power_off(cdns->usb2_phy); > > Won't this cause a problem? > What's the problem you have seen?
diff --git a/drivers/usb/cdns3/core.c b/drivers/usb/cdns3/core.c index 5c1586ec7824..e56dbb6a898c 100644 --- a/drivers/usb/cdns3/core.c +++ b/drivers/usb/cdns3/core.c @@ -371,6 +371,27 @@ static int cdns3_role_set(struct usb_role_switch *sw, enum usb_role role) return ret; } +static int set_phy_power_on(struct cdns3 *cdns) +{ + int ret; + + ret = phy_power_on(cdns->usb2_phy); + if (ret) + return ret; + + ret = phy_power_on(cdns->usb3_phy); + if (ret) + phy_power_off(cdns->usb2_phy); + + return ret; +} + +static void set_phy_power_off(struct cdns3 *cdns) +{ + phy_power_off(cdns->usb3_phy); + phy_power_off(cdns->usb2_phy); +} + /** * cdns3_probe - probe for cdns3 core device * @pdev: Pointer to cdns3 core platform device @@ -463,14 +484,10 @@ static int cdns3_probe(struct platform_device *pdev) if (ret) goto err1; - ret = phy_power_on(cdns->usb2_phy); + ret = set_phy_power_on(cdns); if (ret) goto err2; - ret = phy_power_on(cdns->usb3_phy); - if (ret) - goto err3; - sw_desc.set = cdns3_role_set; sw_desc.get = cdns3_role_get; sw_desc.allow_userspace_control = true; @@ -482,16 +499,16 @@ static int cdns3_probe(struct platform_device *pdev) if (IS_ERR(cdns->role_sw)) { ret = PTR_ERR(cdns->role_sw); dev_warn(dev, "Unable to register Role Switch\n"); - goto err4; + goto err3; } ret = cdns3_drd_init(cdns); if (ret) - goto err5; + goto err4; ret = cdns3_core_init_role(cdns); if (ret) - goto err5; + goto err4; device_set_wakeup_capable(dev, true); pm_runtime_set_active(dev); @@ -508,14 +525,11 @@ static int cdns3_probe(struct platform_device *pdev) dev_dbg(dev, "Cadence USB3 core: probe succeed\n"); return 0; -err5: +err4: cdns3_drd_exit(cdns); usb_role_switch_unregister(cdns->role_sw); -err4: - phy_power_off(cdns->usb3_phy); - err3: - phy_power_off(cdns->usb2_phy); + set_phy_power_off(cdns); err2: phy_exit(cdns->usb3_phy); err1: @@ -539,8 +553,7 @@ static int cdns3_remove(struct platform_device *pdev) pm_runtime_put_noidle(&pdev->dev); cdns3_exit_roles(cdns); usb_role_switch_unregister(cdns->role_sw); - phy_power_off(cdns->usb2_phy); - phy_power_off(cdns->usb3_phy); + set_phy_power_off(cdns); phy_exit(cdns->usb2_phy); phy_exit(cdns->usb3_phy); return 0;