| Message ID | 87sj5pbgwr.fsf@ashishki-desk.ger.corp.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jan 25, 2013 at 02:12:20PM +0200, Alexander Shishkin wrote: > Peter Chen <peter.chen@freescale.com> writes: > > > On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote: > >> Peter Chen <peter.chen@freescale.com> writes: > >> > >> > - Create init/destroy API for probe and remove > >> > - start/stop API are only used otg id switch process > >> > - Create the gadget at ci_hdrc_probe if the gadget is supported > >> > at that port, the main purpose for this is to avoid gadget module > >> > load fail at init.rc > >> > @@ -402,6 +402,12 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr, > if (ret) > return ret; > > + /* > + * there won't be an interrupt in case of manual switching, > + * so we need to check vbus session manually > + */ > + ci_handle_vbus_change(ci); > + It may not be used as there will be a vbus interrupt. > return count; > } > > } > dbg_remove_files(&ci->gadget.dev); > device_unregister(&ci->gadget.dev); > - /* my kobject is dynamic, I swear! */ > - memset(&ci->gadget, 0, sizeof(ci->gadget)); Any reason to delete it, another fix? > } > > /** > @@ -1842,13 +1839,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci) > if (!rdrv) > return -ENOMEM; > > - rdrv->init = udc_start; > rdrv->start = udc_id_switch_for_device; > rdrv->stop = udc_id_switch_for_host; > - rdrv->destroy = udc_stop; Where we call udc_start and udc_stop? And the udc_start should only be called one time. > > Which is shorter (32 insertions(+), 53 deletions(-)) and makes the main > probe easier on the eyes. What do you think? OK, not matter how to change it, it needs to cover below things: (Covers last email) - Gadget needs to be only initialized one time. - Host/device function should be OK when the device on it or the usb cable connects to host during the boots up. - When the OTG port is at host mode, it should not call any register writing operations at gadget's API.
Peter Chen <peter.chen@freescale.com> writes: > On Fri, Jan 25, 2013 at 02:12:20PM +0200, Alexander Shishkin wrote: >> Peter Chen <peter.chen@freescale.com> writes: >> >> > On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote: >> >> Peter Chen <peter.chen@freescale.com> writes: >> >> >> >> > - Create init/destroy API for probe and remove >> >> > - start/stop API are only used otg id switch process >> >> > - Create the gadget at ci_hdrc_probe if the gadget is supported >> >> > at that port, the main purpose for this is to avoid gadget module >> >> > load fail at init.rc >> >> >> @@ -402,6 +402,12 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr, >> if (ret) >> return ret; >> >> + /* >> + * there won't be an interrupt in case of manual switching, >> + * so we need to check vbus session manually >> + */ >> + ci_handle_vbus_change(ci); >> + > It may not be used as there will be a vbus interrupt. Not if you write gadget to "role" file. >> return count; >> } >> >> } >> dbg_remove_files(&ci->gadget.dev); >> device_unregister(&ci->gadget.dev); >> - /* my kobject is dynamic, I swear! */ >> - memset(&ci->gadget, 0, sizeof(ci->gadget)); > Any reason to delete it, another fix? It is currently like this because start and stop functions are called multiple times during the devices lifetime. This patch converts the driver to only call start at driver's probe() and stop at driver's remove(). >> } >> >> /** >> @@ -1842,13 +1839,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci) >> if (!rdrv) >> return -ENOMEM; >> >> - rdrv->init = udc_start; >> rdrv->start = udc_id_switch_for_device; >> rdrv->stop = udc_id_switch_for_host; >> - rdrv->destroy = udc_stop; > Where we call udc_start and udc_stop? And the udc_start should only be called > one time. ci_hdrc_gadget_init() and ci_hdrc_gadget_destroy(). Look closer at the patch. >> >> Which is shorter (32 insertions(+), 53 deletions(-)) and makes the main >> probe easier on the eyes. What do you think? > OK, not matter how to change it, it needs to cover below things: > (Covers last email) > - Gadget needs to be only initialized one time. Yes, that's what we agreed on and that's what I'm suggesting in the above patch. > - Host/device function should be OK when the device on it or > the usb cable connects to host during the boots up. Should still work. > - When the OTG port is at host mode, it should not call any > register writing operations at gadget's API. Furthermore, there shouldn't be any calls to the gadget api. Regards, -- Alex
On Tue, Jan 29, 2013 at 11:37:22AM +0200, Alexander Shishkin wrote: > Peter Chen <peter.chen@freescale.com> writes: > > > On Fri, Jan 25, 2013 at 02:12:20PM +0200, Alexander Shishkin wrote: > >> Peter Chen <peter.chen@freescale.com> writes: > >> > >> > On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote: > >> >> Peter Chen <peter.chen@freescale.com> writes: > >> >> > >> >> > - Create init/destroy API for probe and remove > >> >> > - start/stop API are only used otg id switch process > >> >> > - Create the gadget at ci_hdrc_probe if the gadget is supported > >> >> > at that port, the main purpose for this is to avoid gadget module > >> >> > load fail at init.rc > >> >> > >> @@ -402,6 +402,12 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr, > >> if (ret) > >> return ret; > >> > >> + /* > >> + * there won't be an interrupt in case of manual switching, > >> + * so we need to check vbus session manually > >> + */ > >> + ci_handle_vbus_change(ci); > >> + > > It may not be used as there will be a vbus interrupt. > > Not if you write gadget to "role" file. Let me see, we will only use it for standard-A receptacle port soldered at OTG port, there is no ID manual switch, and ID pin is grounded. When the user wants to use it at gadget mode, eg, update image. 1. Plug cable, then write "gadget" to "role" file It will work as there will be ci_handle_vbus_change(ci) at role_start, just like we insmod gadget mode when the cable is connecting to host. 2. Write "gadget" to "role" file first, then plug cable After stopping host, it should close vbus. After gadget role starts, it will enable vbus interrupt. Then, when we plug in A-to-A cable, there will be a vbus interrupt, then active gadget. > >> return -ENOMEM; > >> > >> - rdrv->init = udc_start; > >> rdrv->start = udc_id_switch_for_device; > >> rdrv->stop = udc_id_switch_for_host; > >> - rdrv->destroy = udc_stop; > > Where we call udc_start and udc_stop? And the udc_start should only be called > > one time. > > ci_hdrc_gadget_init() and ci_hdrc_gadget_destroy(). Look closer at the > patch. I am sorry, I still not see udc_stop at ci_hdrc_gadget_destroy(). It doesn't matter. I know what you want to do, let me summery - Call udc_start at ci_hdrc_gadget_init, but vbus enable function should only be called if the role is gadget, or vbus interrupt will occur when the host switches gadget (vbus is off), besides, the ci->vbus_active will be set at host mode, and ci13xxx_start will operate register with function hw_device_state(ci, ci->ep0out->qh.dma) if we load module at host mode. Besides, if we set REG_SHARED, it will reset controller. - Create two destroy functions for gadget and host, and call them at ci remove. Anything I forget, or do you think anything else I need to change? If you agree, I will send patch for above change. Do you want this change is on the top of my v5 patch or just a new v6 patch with above change? > > - When the OTG port is at host mode, it should not call any > > register writing operations at gadget's API. > > Furthermore, there shouldn't be any calls to the gadget api. The user may load gadget module when it is at host mode, eg, at their init script.
diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h index 342b430..36f495a 100644 --- a/drivers/usb/chipidea/ci.h +++ b/drivers/usb/chipidea/ci.h @@ -67,18 +67,14 @@ enum ci_role { /** * struct ci_role_driver - host/gadget role driver - * init: init this role (used at module probe) * start: start this role (used at id switch) * stop: stop this role (used at id switch) - * destroy: destroy this role (used at module remove) * irq: irq handler for this role * name: role name string (host/gadget) */ struct ci_role_driver { - int (*init)(struct ci13xxx *); int (*start)(struct ci13xxx *); void (*stop)(struct ci13xxx *); - void (*destroy)(struct ci13xxx *); irqreturn_t (*irq)(struct ci13xxx *); const char *name; }; @@ -212,17 +208,6 @@ static inline void ci_role_stop(struct ci13xxx *ci) ci->roles[role]->stop(ci); } -static inline void ci_role_destroy(struct ci13xxx *ci) -{ - enum ci_role role = ci->role; - - if (role == CI_ROLE_END) - return; - - ci->role = CI_ROLE_END; - - ci->roles[role]->destroy(ci); -} /****************************************************************************** * REGISTERS *****************************************************************************/ diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index a61e759..83f35fa 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -402,6 +402,12 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr, if (ret) return ret; + /* + * there won't be an interrupt in case of manual switching, + * so we need to check vbus session manually + */ + ci_handle_vbus_change(ci); + return count; } @@ -592,30 +598,6 @@ static int ci_hdrc_probe(struct platform_device *pdev) otgsc = hw_read(ci, OP_OTGSC, ~0); - /* - * If the gadget is supported, call its init unconditionally, - * We need to support load gadget module at init.rc. - */ - if (ci->roles[CI_ROLE_GADGET]) { - ret = ci->roles[CI_ROLE_GADGET]->init(ci); - if (ret) { - dev_err(dev, "can't init %s role, ret=%d\n", - ci_role(ci)->name, ret); - ret = -ENODEV; - goto rm_wq; - } - } - - if (ci->role == CI_ROLE_HOST) { - ret = ci->roles[CI_ROLE_HOST]->init(ci); - if (ret) { - dev_err(dev, "can't init %s role, ret=%d\n", - ci_role(ci)->name, ret); - ret = -ENODEV; - goto rm_wq; - } - } - platform_set_drvdata(pdev, ci); ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name, ci); @@ -626,6 +608,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) if (ret) goto rm_attr; + ci_role_start(ci, ci->role); + /* Handle the situation that usb device at the MicroB to A cable */ if (ci->is_otg && !(otgsc & OTGSC_ID)) queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(500)); @@ -651,7 +635,8 @@ static int ci_hdrc_remove(struct platform_device *pdev) destroy_workqueue(ci->wq); device_remove_file(ci->dev, &dev_attr_role); free_irq(ci->irq, ci); - ci_role_destroy(ci); + ci_hdrc_gadget_destroy(ci); + ci_hdrc_host_destroy(ci); return 0; } diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index 7f4538c..ead3158 100644 --- a/drivers/usb/chipidea/host.c +++ b/drivers/usb/chipidea/host.c @@ -95,10 +95,8 @@ int ci_hdrc_host_init(struct ci13xxx *ci) if (!rdrv) return -ENOMEM; - rdrv->init = host_start; rdrv->start = host_start; rdrv->stop = host_stop; - rdrv->destroy = host_stop; rdrv->irq = host_irq; rdrv->name = "host"; ci->roles[CI_ROLE_HOST] = rdrv; @@ -107,3 +105,9 @@ int ci_hdrc_host_init(struct ci13xxx *ci) return 0; } + +void ci_hdrc_host_destroy(struct ci13xxx *ci) +{ + if (ci->role == CI_ROLE_HOST) + host_stop(ci); +} diff --git a/drivers/usb/chipidea/host.h b/drivers/usb/chipidea/host.h index 761fb1f..2e529be 100644 --- a/drivers/usb/chipidea/host.h +++ b/drivers/usb/chipidea/host.h @@ -4,6 +4,7 @@ #ifdef CONFIG_USB_CHIPIDEA_HOST int ci_hdrc_host_init(struct ci13xxx *ci); +void ci_hdrc_host_destroy(struct ci13xxx *ci); #else @@ -12,6 +13,10 @@ static inline int ci_hdrc_host_init(struct ci13xxx *ci) return -ENXIO; } +static void ci_hdrc_host_destroy(struct ci13xxx *ci) +{ +} + #endif #endif /* __DRIVERS_USB_CHIPIDEA_HOST_H */ diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 38446de..38b06ac 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -1750,10 +1750,10 @@ static int udc_start(struct ci13xxx *ci) * - Enable vbus detect * - If it has already connected to host, notify udc */ - if (ci->role == CI_ROLE_GADGET) { + //if (ci->role == CI_ROLE_GADGET) { ci_enable_otg_interrupt(ci, OTGSC_BSVIE); ci_handle_vbus_change(ci); - } + //} return retval; @@ -1798,11 +1798,10 @@ static void udc_id_switch_for_host(struct ci13xxx *ci) } /** - * udc_remove: parent remove must call this to remove UDC - * - * No interrupts active, the IRQ has been released + * ci_hdrc_gadget_init - initialize device related bits + * ci: the controller */ -static void udc_stop(struct ci13xxx *ci) +void ci_hdrc_gadget_destroy(struct ci13xxx *ci) { if (ci == NULL) return; @@ -1821,8 +1820,6 @@ static void udc_stop(struct ci13xxx *ci) } dbg_remove_files(&ci->gadget.dev); device_unregister(&ci->gadget.dev); - /* my kobject is dynamic, I swear! */ - memset(&ci->gadget, 0, sizeof(ci->gadget)); } /** @@ -1842,13 +1839,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci) if (!rdrv) return -ENOMEM; - rdrv->init = udc_start; rdrv->start = udc_id_switch_for_device; rdrv->stop = udc_id_switch_for_host; - rdrv->destroy = udc_stop; rdrv->irq = udc_irq; rdrv->name = "gadget"; ci->roles[CI_ROLE_GADGET] = rdrv; - return 0; + return udc_start(ci); } diff --git a/drivers/usb/chipidea/udc.h b/drivers/usb/chipidea/udc.h index 4ff2384d..12fd7cf 100644 --- a/drivers/usb/chipidea/udc.h +++ b/drivers/usb/chipidea/udc.h @@ -80,6 +80,7 @@ struct ci13xxx_req { #ifdef CONFIG_USB_CHIPIDEA_UDC int ci_hdrc_gadget_init(struct ci13xxx *ci); +void ci_hdrc_gadget_destroy(struct ci13xxx *ci); #else @@ -88,6 +89,10 @@ static inline int ci_hdrc_gadget_init(struct ci13xxx *ci) return -ENXIO; } +static void ci_hdrc_gadget_destroy(struct ci13xxx *ci) +{ +} + #endif #endif /* __DRIVERS_USB_CHIPIDEA_UDC_H */