Message ID | 20221009160813.776829-2-xu.yang_2@nxp.com |
---|---|
State | Superseded |
Headers | show |
Series | [1/8] usb: chipidea: core: add controller resume support when controller is powered off | expand |
Hi Xu, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on char-misc/char-misc-testing] [also build test WARNING on usb/usb-testing linus/master v6.0 next-20221007] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Xu-Yang/add-power-lost-support-during-system-suspend-resume/20221009-160800 base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 7cd04013fbf3e6dcb67ca6b59aa813269a2ad9ce config: m68k-allyesconfig compiler: m68k-linux-gcc (GCC) 12.1.0 reproduce (this is a W=1 build): wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # https://github.com/intel-lab-lkp/linux/commit/0da9a11a221d24228a1ea15b9d8a2f51cfa377b5 git remote add linux-review https://github.com/intel-lab-lkp/linux git fetch --no-tags linux-review Xu-Yang/add-power-lost-support-during-system-suspend-resume/20221009-160800 git checkout 0da9a11a221d24228a1ea15b9d8a2f51cfa377b5 # save the config file mkdir build_dir && cp config build_dir/.config COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=m68k SHELL=/bin/bash drivers/usb/chipidea/ If you fix the issue, kindly add following tag where applicable | Reported-by: kernel test robot <lkp@intel.com> All warnings (new ones prefixed by >>): >> drivers/usb/chipidea/core.c:681:6: warning: no previous prototype for 'ci_handle_power_lost' [-Wmissing-prototypes] 681 | void ci_handle_power_lost(struct ci_hdrc *ci) | ^~~~~~~~~~~~~~~~~~~~ vim +/ci_handle_power_lost +681 drivers/usb/chipidea/core.c 680 > 681 void ci_handle_power_lost(struct ci_hdrc *ci) 682 { 683 enum ci_role role; 684 685 disable_irq_nosync(ci->irq); 686 if (!ci_otg_is_fsm_mode(ci)) { 687 role = ci_get_role(ci); 688 689 if (ci->role != role) { 690 ci_handle_id_switch(ci); 691 } else if (role == CI_ROLE_GADGET) { 692 if (ci->is_otg && hw_read_otgsc(ci, OTGSC_BSV)) 693 usb_gadget_vbus_connect(&ci->gadget); 694 } 695 } 696 697 enable_irq(ci->irq); 698 } 699
Hi Xu,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on usb/usb-testing linus/master v6.0 next-20221007]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Xu-Yang/add-power-lost-support-during-system-suspend-resume/20221009-160800
base: https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/char-misc.git 7cd04013fbf3e6dcb67ca6b59aa813269a2ad9ce
config: i386-randconfig-s003
compiler: gcc-11 (Debian 11.3.0-5) 11.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.4-39-gce1a6720-dirty
# https://github.com/intel-lab-lkp/linux/commit/0da9a11a221d24228a1ea15b9d8a2f51cfa377b5
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Xu-Yang/add-power-lost-support-during-system-suspend-resume/20221009-160800
git checkout 0da9a11a221d24228a1ea15b9d8a2f51cfa377b5
# save the config file
mkdir build_dir && cp config build_dir/.config
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=i386 SHELL=/bin/bash drivers/usb/chipidea/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> drivers/usb/chipidea/core.c:681:6: sparse: sparse: symbol 'ci_handle_power_lost' was not declared. Should it be static?
diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index ae90fee75a32..3a39eb5e7dca 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; +} + +void ci_handle_power_lost(struct ci_hdrc *ci) +{ + 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);
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@nxp.com> --- 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(-)