Hi Conor,
> -----Original Message-----
> From: Conor Dooley <conor@xxxxxxxxxx>
> Sent: Tuesday, October 25, 2022 9:47 PM
> To: Xu Yang <xu.yang_2@xxxxxxx>
> Cc: peter.chen@xxxxxxxxxx; Jun Li <jun.li@xxxxxxx>; gregkh@xxxxxxxxxxxxxxxxxxx; linux-usb@xxxxxxxxxxxxxxx; dl-linux-
> imx <linux-imx@xxxxxxx>
> Subject: [EXT] Re: [PATCH v2 1/8] usb: chipidea: core: add controller resume support when controller is powered off
>
> Caution: EXT Email
>
> On Thu, Oct 13, 2022 at 11:14:35PM +0800, Xu Yang wrote:
> > For some SoCs, the controler's power will be off during the system
> > suspend, and it needs some recovery operation to let the system back
> > to workable. We add this support in this patch.
> >
> > Signed-off-by: Xu Yang <xu.yang_2@xxxxxxx>
> >
> > ---
> > Changes since v1:
> > - add static modifer for ci_handle_power_lost().
> > ---
> > drivers/usb/chipidea/core.c | 80 ++++++++++++++++++++++++++++---------
> > drivers/usb/chipidea/otg.c | 2 +-
> > drivers/usb/chipidea/otg.h | 1 +
> > 3 files changed, 63 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> > index ae90fee75a32..80267b973c26 100644
> > --- a/drivers/usb/chipidea/core.c
> > +++ b/drivers/usb/chipidea/core.c
> > @@ -637,6 +637,49 @@ static int ci_usb_role_switch_set(struct usb_role_switch *sw,
> > return 0;
> > }
> >
> > +static enum ci_role ci_get_role(struct ci_hdrc *ci)
> > +{
> > + enum ci_role role;
> > +
> > + if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> > + if (ci->is_otg) {
> > + role = ci_otg_role(ci);
> > + hw_write_otgsc(ci, OTGSC_IDIE, OTGSC_IDIE);
> > + } else {
> > + /*
> > + * If the controller is not OTG capable, but support
> > + * role switch, the defalt role is gadget, and the
> > + * user can switch it through debugfs.
> > + */
> > + role = CI_ROLE_GADGET;
> > + }
> > + } else {
> > + role = ci->roles[CI_ROLE_HOST] ? CI_ROLE_HOST
> > + : CI_ROLE_GADGET;
> > + }
> > +
> > + return role;
> > +}
> > +
> > +static void ci_handle_power_lost(struct ci_hdrc *ci)
>
> Hey,
>
> This appears to have landed in -next and is breaking allmodconfig for
> RISC-V:
> ../drivers/usb/chipidea/core.c:664:13: error: 'ci_handle_power_lost' defined but not used [-Werror=unused-function]
> 664 | static void ci_handle_power_lost(struct ci_hdrc *ci)
> | ^~~~~~~~~~~~~~~~~~~~
> CC [M] drivers/media/i2c/mt9t001.o
> CC [M] drivers/net/ethernet/davicom/dm9051.o
> CC [M] drivers/input/touchscreen/ti_am335x_tsc.o
> CC [M] drivers/watchdog/wdt_pci.o
> cc1: all warnings being treated as errors
> make[5]: *** [../scripts/Makefile.build:250: drivers/usb/chipidea/core.o] Error 1
> make[4]: *** [../scripts/Makefile.build:500: drivers/usb/chipidea] Error 2
> make[3]: *** [../scripts/Makefile.build:500: drivers/usb] Error 2
> make[3]: *** Waiting for unfinished jobs....
>
> The only user seems to be wrapped in an #ifdef CONFIG_PM_SLEEP, and
> while I haven't had the chance to investigate further yet that's
> probably where I'd start looking.
> Apologies if it's been reported, I had a quick look on lore but didn't
> see anything.
Since only ci_resume() will call ci_handle_power_lost(), so it should also
be wrapped in #ifdef CONFIG_PM_SLEEP. Thanks for your report.
Thanks,
Xu Yang
>
> Thanks,
> Conor.
>
> > +{
> > + enum ci_role role;
> > +
> > + disable_irq_nosync(ci->irq);
> > + if (!ci_otg_is_fsm_mode(ci)) {
> > + role = ci_get_role(ci);
> > +
> > + if (ci->role != role) {
> > + ci_handle_id_switch(ci);
> > + } else if (role == CI_ROLE_GADGET) {
> > + if (ci->is_otg && hw_read_otgsc(ci, OTGSC_BSV))
> > + usb_gadget_vbus_connect(&ci->gadget);
> > + }
> > + }
> > +
> > + enable_irq(ci->irq);
> > +}
> > +
> > static struct usb_role_switch_desc ci_role_switch = {
> > .set = ci_usb_role_switch_set,
> > .get = ci_usb_role_switch_get,
> > @@ -1134,25 +1177,7 @@ static int ci_hdrc_probe(struct platform_device *pdev)
> > }
> > }
> >
> > - if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> > - if (ci->is_otg) {
> > - ci->role = ci_otg_role(ci);
> > - /* Enable ID change irq */
> > - hw_write_otgsc(ci, OTGSC_IDIE, OTGSC_IDIE);
> > - } else {
> > - /*
> > - * If the controller is not OTG capable, but support
> > - * role switch, the defalt role is gadget, and the
> > - * user can switch it through debugfs.
> > - */
> > - ci->role = CI_ROLE_GADGET;
> > - }
> > - } else {
> > - ci->role = ci->roles[CI_ROLE_HOST]
> > - ? CI_ROLE_HOST
> > - : CI_ROLE_GADGET;
> > - }
> > -
> > + ci->role = ci_get_role(ci);
> > if (!ci_otg_is_fsm_mode(ci)) {
> > /* only update vbus status for peripheral */
> > if (ci->role == CI_ROLE_GADGET) {
> > @@ -1374,8 +1399,16 @@ static int ci_suspend(struct device *dev)
> > static int ci_resume(struct device *dev)
> > {
> > struct ci_hdrc *ci = dev_get_drvdata(dev);
> > + bool power_lost;
> > int ret;
> >
> > + /* Since ASYNCLISTADDR (host mode) and ENDPTLISTADDR (device
> > + * mode) share the same register address. We can check if
> > + * controller resume from power lost based on this address
> > + * due to this register will be reset after power lost.
> > + */
> > + power_lost = !hw_read(ci, OP_ENDPTLISTADDR, ~0);
> > +
> > if (device_may_wakeup(dev))
> > disable_irq_wake(ci->irq);
> >
> > @@ -1383,6 +1416,15 @@ static int ci_resume(struct device *dev)
> > if (ret)
> > return ret;
> >
> > + if (power_lost) {
> > + /* shutdown and re-init for phy */
> > + ci_usb_phy_exit(ci);
> > + ci_usb_phy_init(ci);
> > + }
> > +
> > + if (power_lost)
> > + ci_handle_power_lost(ci);
> > +
> > if (ci->supports_runtime_pm) {
> > pm_runtime_disable(dev);
> > pm_runtime_set_active(dev);
> > diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> > index 7b53274ef966..622c3b68aa1e 100644
> > --- a/drivers/usb/chipidea/otg.c
> > +++ b/drivers/usb/chipidea/otg.c
> > @@ -165,7 +165,7 @@ static int hw_wait_vbus_lower_bsv(struct ci_hdrc *ci)
> > return 0;
> > }
> >
> > -static void ci_handle_id_switch(struct ci_hdrc *ci)
> > +void ci_handle_id_switch(struct ci_hdrc *ci)
> > {
> > enum ci_role role = ci_otg_role(ci);
> >
> > diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
> > index 5e7a6e571dd2..87629b81e03e 100644
> > --- a/drivers/usb/chipidea/otg.h
> > +++ b/drivers/usb/chipidea/otg.h
> > @@ -14,6 +14,7 @@ int ci_hdrc_otg_init(struct ci_hdrc *ci);
> > void ci_hdrc_otg_destroy(struct ci_hdrc *ci);
> > enum ci_role ci_otg_role(struct ci_hdrc *ci);
> > void ci_handle_vbus_change(struct ci_hdrc *ci);
> > +void ci_handle_id_switch(struct ci_hdrc *ci);
> > static inline void ci_otg_queue_work(struct ci_hdrc *ci)
> > {
> > disable_irq_nosync(ci->irq);
> > --
> > 2.34.1
> >
> >
