Re: [PATCH v3] usb: chipidea: add role switch class support
Hi, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [cannot apply to v5.3-rc4 next-20190814] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/jun-li-nxp-com/usb-chipidea-add-role-switch-class-support/20190815-040028 config: arm-multi_v5_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=arm If you fix the issue, kindly add following tag Reported-by: kbuild test robot All errors (new ones prefixed by >>): drivers/usb/chipidea/core.o: In function `ci_hdrc_remove': >> core.c:(.text+0xec4): undefined reference to `usb_role_switch_unregister' drivers/usb/chipidea/core.o: In function `ci_hdrc_probe': core.c:(.text+0x1328): undefined reference to `usb_role_switch_unregister' core.c:(.text+0x14c8): undefined reference to `usb_role_switch_register' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
RE: [PATCH v3] usb: chipidea: add role switch class support
> > Changes for v3: > - Remove the patch usb: chipidea: replace ci_role with usb_role > as the existing ci->role usage can't map to usb_role. > - Use the suggested ci_hdrc_cable for reuse current role change > handling. > - Fix build robot warning by add usb_role head file. > You may need to add "select USB_ROLE_SWITCH" at chipidea Kconfig to fix build error. > +static int ci_usb_role_switch_set(struct device *dev, enum usb_role > +role) { > + struct ci_hdrc *ci = dev_get_drvdata(dev); > + struct ci_hdrc_cable *cable = NULL; > + enum usb_role current_role = ci_role_to_usb_role(ci); > + > + if (current_role == role) > + return 0; > + > + /* Stop current role */ > + if (current_role == USB_ROLE_DEVICE) > + cable = &ci->platdata->vbus_extcon; > + else if (current_role == USB_ROLE_HOST) > + cable = &ci->platdata->id_extcon; > + > + if (cable) { > + cable->changed = true; > + cable->connected = false; > + ci_irq(ci->irq, ci); > + } > + > + cable = NULL; > + > + /* Start target role */ > + if (role == USB_ROLE_DEVICE) > + cable = &ci->platdata->vbus_extcon; > + else if (role == USB_ROLE_HOST) > + cable = &ci->platdata->id_extcon; > + > + if (cable) { > + if (ci->wq) > + flush_workqueue(ci->wq); > + cable->changed = true; > + cable->connected = true; > + ci_irq(ci->irq, ci); > + } > + > + return 0; > +} > + You may add spin_lock_irqsave in .set API either and pay attention to flush_workqueue. And you may improve low power management support for it later, other things are ok for me. I have tested it at imx8qm mek by adding .allow_userspace_control = true. Peter > +static struct usb_role_switch_desc ci_role_switch = { > + .set = ci_usb_role_switch_set, > + .get = ci_usb_role_switch_get, > +}; > + > static int ci_get_platdata(struct device *dev, > struct ci_hdrc_platform_data *platdata) { @@ -726,6 +784,9 @@ > static int ci_get_platdata(struct device *dev, > cable->connected = false; > } > > + if (device_property_read_bool(dev, "usb-role-switch")) > + ci_role_switch.fwnode = dev->fwnode; > + > platdata->pctl = devm_pinctrl_get(dev); > if (!IS_ERR(platdata->pctl)) { > struct pinctrl_state *p; > @@ -1051,6 +1112,15 @@ static int ci_hdrc_probe(struct platform_device *pdev) > } > } > > + if (ci_role_switch.fwnode) { > + ci->role_switch = usb_role_switch_register(dev, > + &ci_role_switch); > + if (IS_ERR(ci->role_switch)) { > + ret = PTR_ERR(ci->role_switch); > + goto deinit_otg; > + } > + } > + > if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) { > if (ci->is_otg) { > ci->role = ci_otg_role(ci); > @@ -1115,6 +1185,9 @@ static int ci_hdrc_probe(struct platform_device *pdev) > remove_debug: > dbg_remove_files(ci); > stop: > + if (ci->role_switch) > + usb_role_switch_unregister(ci->role_switch); > +deinit_otg: > if (ci->is_otg && ci->roles[CI_ROLE_GADGET]) > ci_hdrc_otg_destroy(ci); > deinit_gadget: > @@ -1133,6 +1206,9 @@ static int ci_hdrc_remove(struct platform_device *pdev) > { > struct ci_hdrc *ci = platform_get_drvdata(pdev); > > + if (ci->role_switch) > + usb_role_switch_unregister(ci->role_switch); > + > if (ci->supports_runtime_pm) { > pm_runtime_get_sync(&pdev->dev); > pm_runtime_disable(&pdev->dev); > diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c index > f25d482..fbfb02e 100644 > --- a/drivers/usb/chipidea/otg.c > +++ b/drivers/usb/chipidea/otg.c > @@ -35,7 +35,7 @@ u32 hw_read_otgsc(struct ci_hdrc *ci, u32 mask) >* detection overwrite OTGSC register value >*/ > cable = &ci->platdata->vbus_extcon; > - if (!IS_ERR(cable->edev)) { > + if (!IS_ERR(cable->edev) || ci->role_switch) { > if (cable->changed) > val |= OTGSC_BSVIS; > else > @@ -53,7 +53,7 @@ u32 hw_read_otgsc(struct ci_hdrc *ci, u32 mask) > } > > cable = &ci->platdata->id_extcon; > - if (!IS_ERR(cable->edev)) { > + if (!IS_ERR(cable->edev) || ci->role_switch) { > if (cable->changed) > val |= OTGSC_IDIS; > else > @@ -83,7 +83,7 @@ void hw_write_otgsc(struct ci_hdrc *ci, u32 mask, u32 data) > struct ci_hdrc_cable *cable; > > cable = &ci->platdata->vbus_extcon; > - if (!IS_ERR(cable->edev)) { > + if (!IS_ERR(cable->edev) || ci->role_switch) { > if (data & mask & OTGSC_BSVIS) > cable->changed = false; > > @@
Re: [PATCH v3] usb: chipidea: add role switch class support
Hi, Thank you for the patch! Yet something to improve: [auto build test ERROR on linus/master] [cannot apply to v5.3-rc4 next-20190814] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/jun-li-nxp-com/usb-chipidea-add-role-switch-class-support/20190815-040028 config: arm-multi_v5_defconfig (attached as .config) compiler: arm-linux-gnueabi-gcc (GCC) 7.4.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.4.0 make.cross ARCH=arm If you fix the issue, kindly add following tag Reported-by: kbuild test robot All errors (new ones prefixed by >>): drivers/usb/chipidea/core.o: In function `ci_hdrc_remove': core.c:(.text+0xec4): undefined reference to `usb_role_switch_unregister' drivers/usb/chipidea/core.o: In function `ci_hdrc_probe': core.c:(.text+0x1328): undefined reference to `usb_role_switch_unregister' >> core.c:(.text+0x14c8): undefined reference to `usb_role_switch_register' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: application/gzip
[PATCH v3] usb: chipidea: add role switch class support
From: Li Jun USB role is fully controlled by usb role switch consumer(e.g. typec), usb port can be at host mode(USB_ROLE_HOST), device mode connected to host(USB_ROLE_DEVICE), or not connecting any partner(USB_ROLE_NONE). Signed-off-by: Li Jun --- Changes for v3: - Remove the patch usb: chipidea: replace ci_role with usb_role as the existing ci->role usage can't map to usb_role. - Use the suggested ci_hdrc_cable for reuse current role change handling. - Fix build robot warning by add usb_role head file. Change for v2: - Support USB_ROLE_NONE, which for usb port not connecting any device or host, and will be in low power mode. drivers/usb/chipidea/ci.h | 12 +++ drivers/usb/chipidea/core.c | 76 + drivers/usb/chipidea/otg.c | 8 ++--- 3 files changed, 92 insertions(+), 4 deletions(-) diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h index 6a2cc5c..6911aef 100644 --- a/drivers/usb/chipidea/ci.h +++ b/drivers/usb/chipidea/ci.h @@ -16,6 +16,7 @@ #include #include #include +#include #include /** @@ -217,6 +218,7 @@ struct ci_hdrc { ktime_t hr_timeouts[NUM_OTG_FSM_TIMERS]; unsignedenabled_otg_timer_bits; enum otg_fsm_timer next_otg_timer; + struct usb_role_switch *role_switch; struct work_struct work; struct workqueue_struct *wq; @@ -290,6 +292,16 @@ static inline void ci_role_stop(struct ci_hdrc *ci) ci->roles[role]->stop(ci); } +static inline enum usb_role ci_role_to_usb_role(struct ci_hdrc *ci) +{ + if (ci->role == CI_ROLE_HOST) + return USB_ROLE_HOST; + else if (ci->role == CI_ROLE_GADGET && ci->vbus_active) + return USB_ROLE_DEVICE; + else + return USB_ROLE_NONE; +} + /** * hw_read_id_reg: reads from a identification register * @ci: the controller diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index 26062d6..c3300a1 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -600,6 +600,64 @@ static int ci_cable_notifier(struct notifier_block *nb, unsigned long event, return NOTIFY_DONE; } +static enum usb_role ci_usb_role_switch_get(struct device *dev) +{ + struct ci_hdrc *ci = dev_get_drvdata(dev); + enum usb_role role; + unsigned long flags; + + spin_lock_irqsave(&ci->lock, flags); + role = ci_role_to_usb_role(ci); + spin_unlock_irqrestore(&ci->lock, flags); + + return role; +} + +static int ci_usb_role_switch_set(struct device *dev, enum usb_role role) +{ + struct ci_hdrc *ci = dev_get_drvdata(dev); + struct ci_hdrc_cable *cable = NULL; + enum usb_role current_role = ci_role_to_usb_role(ci); + + if (current_role == role) + return 0; + + /* Stop current role */ + if (current_role == USB_ROLE_DEVICE) + cable = &ci->platdata->vbus_extcon; + else if (current_role == USB_ROLE_HOST) + cable = &ci->platdata->id_extcon; + + if (cable) { + cable->changed = true; + cable->connected = false; + ci_irq(ci->irq, ci); + } + + cable = NULL; + + /* Start target role */ + if (role == USB_ROLE_DEVICE) + cable = &ci->platdata->vbus_extcon; + else if (role == USB_ROLE_HOST) + cable = &ci->platdata->id_extcon; + + if (cable) { + if (ci->wq) + flush_workqueue(ci->wq); + cable->changed = true; + cable->connected = true; + ci_irq(ci->irq, ci); + } + + return 0; +} + +static struct usb_role_switch_desc ci_role_switch = { + .set = ci_usb_role_switch_set, + .get = ci_usb_role_switch_get, +}; + static int ci_get_platdata(struct device *dev, struct ci_hdrc_platform_data *platdata) { @@ -726,6 +784,9 @@ static int ci_get_platdata(struct device *dev, cable->connected = false; } + if (device_property_read_bool(dev, "usb-role-switch")) + ci_role_switch.fwnode = dev->fwnode; + platdata->pctl = devm_pinctrl_get(dev); if (!IS_ERR(platdata->pctl)) { struct pinctrl_state *p; @@ -1051,6 +1112,15 @@ static int ci_hdrc_probe(struct platform_device *pdev) } } + if (ci_role_switch.fwnode) { + ci->role_switch = usb_role_switch_register(dev, + &ci_role_switch); + if (IS_ERR(ci->role_switch)) { + ret = PTR_ERR(ci->role_switch); + goto deinit_otg; + } + } + if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {