| Message ID | 1362563800-16673-4-git-send-email-peter.chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 6 March 2013 11:56, Peter Chen <peter.chen@freescale.com> wrote: > The main design flow is the same with msm otg driver, that is the id and > vbus interrupt are handled at core driver, others are handled by > individual drivers. > > - At former design, when switch usb role from device->host, it will call > udc_stop, it will remove the gadget driver, so when switch role > from host->device, it can't add gadget driver any more. > At new design, when role switch occurs, the gadget just calls > usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as > reset controller, it will not free any device/gadget structure > > - Add vbus connect and disconnect to core interrupt handler, it can > notify udc driver by calling usb_gadget_vbus_disconnect > /usb_gadget_vbus_connect. > > - vbus interrupt needs to be handled when gadget function is enabled > > - Create/destroy the gadget at udc's init and destory function > > - start/stop API are used at otg id switch and probe routine > > - Defer some gadget operations at ci's delayed work queue When I asked you to reorder patches, I didn't mean squash them all into one. Now you have a patch that does 5 different things and none of them properly, because you still need to amend these changes later in the patchset, like the patch 7, where you remove the delayed work, which is added in this patch. At the same time, you have bits in this patch that should be moved to other patches. See comments below. > > Signed-off-by: Peter Chen <peter.chen@freescale.com> > --- > drivers/usb/chipidea/bits.h | 10 ++ > drivers/usb/chipidea/ci.h | 7 +- > drivers/usb/chipidea/ci13xxx_imx.c | 3 +- > drivers/usb/chipidea/core.c | 190 ++++++++++++++++++++++++++++++++---- > drivers/usb/chipidea/host.c | 6 + > drivers/usb/chipidea/host.h | 6 +- > drivers/usb/chipidea/otg.c | 28 ++++-- > drivers/usb/chipidea/otg.h | 3 + > drivers/usb/chipidea/udc.c | 39 ++++++-- > drivers/usb/chipidea/udc.h | 4 + > 10 files changed, 254 insertions(+), 42 deletions(-) > > diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h > index d8ffc2f..58ef56c 100644 > --- a/drivers/usb/chipidea/bits.h > +++ b/drivers/usb/chipidea/bits.h > @@ -77,11 +77,21 @@ > #define OTGSC_ASVIS BIT(18) > #define OTGSC_BSVIS BIT(19) > #define OTGSC_BSEIS BIT(20) > +#define OTGSC_1MSIS BIT(21) > +#define OTGSC_DPIS BIT(22) > #define OTGSC_IDIE BIT(24) > #define OTGSC_AVVIE BIT(25) > #define OTGSC_ASVIE BIT(26) > #define OTGSC_BSVIE BIT(27) > #define OTGSC_BSEIE BIT(28) > +#define OTGSC_1MSIE BIT(29) > +#define OTGSC_DPIE BIT(30) > +#define OTGSC_INT_EN_BITS (OTGSC_IDIE | OTGSC_AVVIE | OTGSC_ASVIE \ > + | OTGSC_BSVIE | OTGSC_BSEIE | OTGSC_1MSIE \ > + | OTGSC_DPIE) > +#define OTGSC_INT_STATUS_BITS (OTGSC_IDIS | OTGSC_AVVIS | OTGSC_ASVIS \ > + | OTGSC_BSVIS | OTGSC_BSEIS | OTGSC_1MSIS \ > + | OTGSC_DPIS) > > /* USBMODE */ > #define USBMODE_CM (0x03UL << 0) > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h > index 697e369..a3777a1 100644 > --- a/drivers/usb/chipidea/ci.h > +++ b/drivers/usb/chipidea/ci.h > @@ -130,6 +130,9 @@ struct hw_bank { > * @transceiver: pointer to USB PHY, if any > * @hcd: pointer to usb_hcd for ehci host driver > * @otg: for otg support > + * @id_event: indicates there is a id event, and handled at ci_otg_work > + * @b_sess_valid_event: indicates there is a vbus event, and handled > + * at ci_otg_work > */ > struct ci13xxx { > struct device *dev; > @@ -140,6 +143,7 @@ struct ci13xxx { > enum ci_role role; > bool is_otg; > struct work_struct work; > + struct delayed_work dwork; > struct workqueue_struct *wq; > > struct dma_pool *qh_pool; > @@ -166,6 +170,8 @@ struct ci13xxx { > struct usb_phy *transceiver; > struct usb_hcd *hcd; > struct usb_otg otg; > + bool id_event; > + bool b_sess_valid_event; > }; > > static inline struct ci_role_driver *ci_role(struct ci13xxx *ci) > @@ -201,7 +207,6 @@ static inline void ci_role_stop(struct ci13xxx *ci) > > ci->roles[role]->stop(ci); > } > - > /****************************************************************************** > * REGISTERS > *****************************************************************************/ > diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c > index 136869b..793fdba 100644 > --- a/drivers/usb/chipidea/ci13xxx_imx.c > +++ b/drivers/usb/chipidea/ci13xxx_imx.c > @@ -110,7 +110,8 @@ static int ci13xxx_imx_probe(struct platform_device *pdev) > pdata->capoffset = DEF_CAPOFFSET; > pdata->flags = CI13XXX_REQUIRE_TRANSCEIVER | > CI13XXX_PULLUP_ON_VBUS | > - CI13XXX_DISABLE_STREAMING; > + CI13XXX_DISABLE_STREAMING | > + CI13XXX_REGS_SHARED; > > pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node); > pdata->dr_mode = of_usb_get_dr_mode(pdev->dev.of_node); > diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c > index c89f2aa..c563e17 100644 > --- a/drivers/usb/chipidea/core.c > +++ b/drivers/usb/chipidea/core.c > @@ -75,6 +75,7 @@ > #include "bits.h" > #include "host.h" > #include "debug.h" > +#include "otg.h" > > /* Controller register map */ > static uintptr_t ci_regs_nolpm[] = { > @@ -201,6 +202,14 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base) > if (ci->hw_ep_max > ENDPT_MAX) > return -ENODEV; > > + /* Disable all interrupts bits */ > + hw_write(ci, OP_USBINTR, 0xffffffff, 0); > + ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS); > + > + /* Clear all interrupts status bits*/ > + hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff); > + ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS); > + > dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n", > ci->hw_bank.lpm, ci->hw_bank.cap, ci->hw_bank.op); > > @@ -302,24 +311,131 @@ static enum ci_role ci_otg_role(struct ci13xxx *ci) > } > > /** > - * ci_role_work - perform role changing based on ID pin > - * @work: work struct > + * hw_wait_reg: wait the register value > + * > + * Sometimes, it needs to wait register value before going on. > + * Eg, when switch to device mode, the vbus value should be lower > + * than OTGSC_BSV before connects to host. > + * > + * @ci: the controller > + * @reg: register index > + * @mask: mast bit > + * @value: the bit value to wait > + * @timeout: timeout to indicate an error > + * > + * This function returns an error code if timeout > */ > -static void ci_role_work(struct work_struct *work) > +static int hw_wait_reg(struct ci13xxx *ci, enum ci13xxx_regs reg, u32 mask, > + u32 value, unsigned long timeout) > +{ > + unsigned long elapse = jiffies + timeout; > + > + while (hw_read(ci, reg, mask) != value) { > + if (time_after(jiffies, elapse)) { > + dev_err(ci->dev, "timeout waiting for %08x in %d\n", > + mask, reg); > + return -ETIMEDOUT; > + } > + msleep(20); > + } > + > + return 0; > +} > + > +/* > + * Since there are some capacitances at vbus, the vbus may not > + * change very quickly when we stop/start internal 5v. > + * Below is a vbus stable timeout value, the routine will quit > + * if the vbus gets the required value, we can think there are some > + * problems for hardware if the vbus can't get the required value > + * within 5 seconds. > + */ > +#define CI_VBUS_STABLE_TIMEOUT 500 > +static void ci_handle_id_switch(struct ci13xxx *ci) > { > - struct ci13xxx *ci = container_of(work, struct ci13xxx, work); > enum ci_role role = ci_otg_role(ci); > > if (role != ci->role) { > dev_dbg(ci->dev, "switching from %s to %s\n", > ci_role(ci)->name, ci->roles[role]->name); > > + /* 1. Finish the current role */ > ci_role_stop(ci); > + hw_device_reset(ci, USBMODE_CM_IDLE); > + > + /* 2. Turn on/off vbus according to coming role */ > + if (role == CI_ROLE_GADGET) { > + otg_set_vbus(&ci->otg, false); > + /* wait vbus lower than OTGSC_BSV */ > + hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0, > + CI_VBUS_STABLE_TIMEOUT); > + } else if (role == CI_ROLE_HOST) { > + otg_set_vbus(&ci->otg, true); > + /* wait vbus higher than OTGSC_AVV */ > + hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV, > + CI_VBUS_STABLE_TIMEOUT); > + } > + > + /* 3. Begin the new role */ > ci_role_start(ci, role); > - enable_irq(ci->irq); > } > } > > +static void ci_handle_vbus_change(struct ci13xxx *ci) > +{ > + u32 otgsc = hw_read(ci, OP_OTGSC, ~0); > + > + if (otgsc & OTGSC_BSV) > + usb_gadget_vbus_connect(&ci->gadget); > + else > + usb_gadget_vbus_disconnect(&ci->gadget); > +} > + > +/** > + * ci_otg_work - perform otg (vbus/id) event handle > + * @work: work struct > + */ > +static void ci_otg_work(struct work_struct *work) > +{ > + struct ci13xxx *ci = container_of(work, struct ci13xxx, work); > + > + if (ci->id_event) { > + ci->id_event = false; > + ci_handle_id_switch(ci); > + } else if (ci->b_sess_valid_event) { > + ci->b_sess_valid_event = false; > + ci_handle_vbus_change(ci); > + } else > + dev_err(ci->dev, "unexpected event occurs at %s\n", __func__); > + > + enable_irq(ci->irq); > +} > + > +static void ci_delayed_work(struct work_struct *work) > +{ > + struct delayed_work *dwork = to_delayed_work(work); > + struct ci13xxx *ci = container_of(dwork, struct ci13xxx, dwork); > + > + if (ci->role == CI_ROLE_GADGET) { > + /* > + * if it is device mode: > + * - Enable vbus detect > + * - If it has already connected to host, notify udc > + */ > + ci_enable_otg_interrupt(ci, OTGSC_BSVIE); > + ci_handle_vbus_change(ci); > + } else if (ci->is_otg && (ci->role == CI_ROLE_HOST)) { > + /* USB Device at the MicroB to A cable */ > + otg_set_vbus(&ci->otg, true); > + } > +} You remove this function in patch 7. Why add it in the first place? > + > +static inline void ci_role_destroy(struct ci13xxx *ci) > +{ > + ci_hdrc_gadget_destroy(ci); > + ci_hdrc_host_destroy(ci); > +} You shouldn't use "inline" here. And the name of the function is also ambiguous, since it destroys all roles. It would be better to just call both _destroys(), like I suggested in the diff I gave you some time ago. > + > static ssize_t show_role(struct device *dev, struct device_attribute *attr, > char *buf) > { > @@ -352,25 +468,50 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr, > > static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role); > > +static bool ci_is_otg_capable(struct ci13xxx *ci) > +{ > + return hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC | DCCPARAMS_HC) > + == (DCCPARAMS_DC | DCCPARAMS_HC); > +} Can't we a) remember this value so that we don't have to read the register at every interrupt b) also take into account the fact that DCCPARAMS doesn't read correctly on all chipideas? (see dr_mode patches) > + > static irqreturn_t ci_irq(int irq, void *data) > { > struct ci13xxx *ci = data; > irqreturn_t ret = IRQ_NONE; > u32 otgsc = 0; > > - if (ci->is_otg) > + if (ci_is_otg_capable(ci)) ci->is_otg is actually the right thing to use, we should instead make sure that it's set properly > otgsc = hw_read(ci, OP_OTGSC, ~0); > > - if (ci->role != CI_ROLE_END) > - ret = ci_role(ci)->irq(ci); > + /* > + * Handle id change interrupt, it indicates device/host function > + * switch. > + */ > + if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) { > + ci->id_event = true; > + ci_clear_otg_interrupt(ci, OTGSC_IDIS); > + disable_irq_nosync(ci->irq); > + queue_work(ci->wq, &ci->work); > + return IRQ_HANDLED; > + } > > - if (ci->is_otg && (otgsc & OTGSC_IDIS)) { > - hw_write(ci, OP_OTGSC, OTGSC_IDIS, OTGSC_IDIS); > + /* > + * Handle vbus change interrupt, it indicates device connection > + * and disconnection events. > + */ > + if (ci_is_otg_capable(ci) && (otgsc & OTGSC_BSVIE) > + && (otgsc & OTGSC_BSVIS)) { > + ci->b_sess_valid_event = true; > + ci_clear_otg_interrupt(ci, OTGSC_BSVIS); > disable_irq_nosync(ci->irq); > queue_work(ci->wq, &ci->work); > - ret = IRQ_HANDLED; > + return IRQ_HANDLED; > } > > + /* Handle device/host interrupt */ > + if (ci->role != CI_ROLE_END) > + ret = ci_role(ci)->irq(ci); > + > return ret; > } > > @@ -481,7 +622,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) > return -ENODEV; > } > > - INIT_WORK(&ci->work, ci_role_work); > + INIT_WORK(&ci->work, ci_otg_work); > + INIT_DELAYED_WORK(&ci->dwork, ci_delayed_work); > ci->wq = create_singlethread_workqueue("ci_otg"); > if (!ci->wq) { > dev_err(dev, "can't create workqueue\n"); > @@ -512,7 +654,10 @@ static int ci_hdrc_probe(struct platform_device *pdev) > } > > if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) { > + dev_dbg(dev, "support otg\n"); > ci->is_otg = true; > + /* if otg is supported, create struct usb_otg */ > + ci_hdrc_otg_init(ci); > /* ID pin needs 1ms debouce time, we delay 2ms for safe */ > mdelay(2); > ci->role = ci_otg_role(ci); I'm thinking that this should be based on dr_mode patches and not the other way around. > @@ -528,7 +673,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) > if (ret) { > dev_err(dev, "can't start %s role\n", ci_role(ci)->name); > ret = -ENODEV; > - goto rm_wq; > + goto free_memory; > } > > platform_set_drvdata(pdev, ci); > @@ -539,17 +684,22 @@ static int ci_hdrc_probe(struct platform_device *pdev) > > ret = device_create_file(dev, &dev_attr_role); > if (ret) > - goto rm_attr; > + goto free_irq; > > - if (ci->is_otg) > - hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE); > + /* Defer some operations */ > + queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(200)); > > return ret; > > -rm_attr: > - device_remove_file(dev, &dev_attr_role); > +free_irq: > + free_irq(ci->irq, ci); > stop: > - ci_role_stop(ci); > + ci_role_destroy(ci); > +free_memory: > + if (ci->roles[CI_ROLE_HOST]) > + devm_kfree(dev, ci->roles[CI_ROLE_HOST]); > + if (ci->roles[CI_ROLE_GADGET]) > + devm_kfree(dev, ci->roles[CI_ROLE_GADGET]); > rm_wq: > flush_workqueue(ci->wq); > destroy_workqueue(ci->wq); > @@ -565,7 +715,7 @@ 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_stop(ci); > + ci_role_destroy(ci); > > return 0; > } > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c > index 8e9d312..ead3158 100644 > --- a/drivers/usb/chipidea/host.c > +++ b/drivers/usb/chipidea/host.c > @@ -105,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..19cc72d 100644 > --- a/drivers/usb/chipidea/host.h > +++ b/drivers/usb/chipidea/host.h > @@ -4,13 +4,17 @@ > #ifdef CONFIG_USB_CHIPIDEA_HOST > > int ci_hdrc_host_init(struct ci13xxx *ci); > - > +void ci_hdrc_host_destroy(struct ci13xxx *ci); > #else > > static inline int ci_hdrc_host_init(struct ci13xxx *ci) > { > return -ENXIO; > } > +static inline void ci_hdrc_host_destroy(struct ci13xxx *ci) > +{ > + > +} > > #endif > > diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c > index 7dea3b3..2986d91 100644 > --- a/drivers/usb/chipidea/otg.c > +++ b/drivers/usb/chipidea/otg.c > @@ -10,21 +10,28 @@ > * published by the Free Software Foundation. > */ > > -#include <linux/platform_device.h> > -#include <linux/module.h> > -#include <linux/io.h> > -#include <linux/irq.h> > -#include <linux/kernel.h> > -#include <linux/slab.h> > -#include <linux/usb/gadget.h> > #include <linux/usb/otg.h> > +#include <linux/usb/gadget.h> > #include <linux/usb/chipidea.h> > > #include "ci.h" > -#include "udc.h" > #include "bits.h" > -#include "host.h" > -#include "debug.h" > + > +void ci_clear_otg_interrupt(struct ci13xxx *ci, u32 bits) > +{ > + /* Only clear request bits */ > + hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, bits); > +} > + > +void ci_enable_otg_interrupt(struct ci13xxx *ci, u32 bits) > +{ > + hw_write(ci, OP_OTGSC, bits, bits); > +} > + > +void ci_disable_otg_interrupt(struct ci13xxx *ci, u32 bits) > +{ > + hw_write(ci, OP_OTGSC, bits, 0); > +} > > static int ci_otg_set_peripheral(struct usb_otg *otg, > struct usb_gadget *periph) > @@ -55,6 +62,7 @@ int ci_hdrc_otg_init(struct ci13xxx *ci) > ci->otg.set_host = ci_otg_set_host; > if (!IS_ERR_OR_NULL(ci->transceiver)) > ci->transceiver->otg = &ci->otg; > + ci_enable_otg_interrupt(ci, OTGSC_IDIE); There should be a counter-action to ci_hdrc_otg_init(). Btw, why can't clear_otg_interrupt() be called from here as well instead of hw_device_init()? This function should only be called for otg capable devices, so it should be safe. And while at it, I think this whole otg.c/otg.h part of this patch should be moved to patch 2. > > return 0; > } > diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h > index b4c6b3e..fa30428 100644 > --- a/drivers/usb/chipidea/otg.h > +++ b/drivers/usb/chipidea/otg.h > @@ -2,5 +2,8 @@ > #define __DRIVERS_USB_CHIPIDEA_OTG_H > > int ci_hdrc_otg_init(struct ci13xxx *ci); > +void ci_clear_otg_interrupt(struct ci13xxx *ci, u32 bits); > +void ci_enable_otg_interrupt(struct ci13xxx *ci, u32 bits); > +void ci_disable_otg_interrupt(struct ci13xxx *ci, u32 bits); > > #endif /* __DRIVERS_USB_CHIPIDEA_OTG_H */ > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index d214448..e82dae4 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -31,6 +31,7 @@ > > #include "ci.h" > #include "udc.h" > +#include "otg.h" > #include "bits.h" > #include "debug.h" > > @@ -1371,6 +1372,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active) > pm_runtime_get_sync(&_gadget->dev); > hw_device_reset(ci, USBMODE_CM_DC); > hw_device_state(ci, ci->ep0out->qh.dma); > + dev_dbg(ci->dev, "Connected to host\n"); > } else { > hw_device_state(ci, 0); > if (ci->platdata->notify_event) > @@ -1378,6 +1380,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active) > CI13XXX_CONTROLLER_STOPPED_EVENT); > _gadget_stop_activity(&ci->gadget); > pm_runtime_put_sync(&_gadget->dev); > + dev_dbg(ci->dev, "Disconnected from host\n"); > } > } > > @@ -1741,7 +1744,12 @@ static int udc_start(struct ci13xxx *ci) > if (!IS_ERR_OR_NULL(ci->transceiver)) { > retval = otg_set_peripheral(ci->transceiver->otg, > &ci->gadget); > - if (retval) > + /* > + * If we implement all USB functions using chipidea drivers, > + * it doesn't need to call above API, meanwhile, if we only > + * use gadget function, calling above API is useless. > + */ > + if (retval && retval != -ENOTSUPP) > goto remove_dbg; > } > > @@ -1778,12 +1786,27 @@ free_qh_pool: > return retval; > } > > +static int udc_id_switch_for_device(struct ci13xxx *ci) I would use _to_ instead of _for_, for clarity. > +{ > + ci_clear_otg_interrupt(ci, OTGSC_BSVIS); > + ci_enable_otg_interrupt(ci, OTGSC_BSVIE); > + > + return 0; > +} > + > +static void udc_id_switch_for_host(struct ci13xxx *ci) Same here. > +{ > + /* host doesn't care B_SESSION_VALID event */ > + ci_clear_otg_interrupt(ci, OTGSC_BSVIS); > + ci_disable_otg_interrupt(ci, OTGSC_BSVIE); > +} > + > /** > - * udc_remove: parent remove must call this to remove UDC > + * ci_hdrc_gadget_destroy: parent remove must call this to remove UDC > * > * No interrupts active, the IRQ has been released > */ > -static void udc_stop(struct ci13xxx *ci) > +void ci_hdrc_gadget_destroy(struct ci13xxx *ci) > { > if (ci == NULL) > return; > @@ -1802,15 +1825,13 @@ 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)); > } > > /** > * ci_hdrc_gadget_init - initialize device related bits > * ci: the controller > * > - * This function enables the gadget role, if the device is "device capable". > + * This function initializes gadget, if the device is "device capable". > */ > int ci_hdrc_gadget_init(struct ci13xxx *ci) > { > @@ -1823,11 +1844,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci) > if (!rdrv) > return -ENOMEM; > > - rdrv->start = udc_start; > - rdrv->stop = udc_stop; > + rdrv->start = udc_id_switch_for_device; > + rdrv->stop = udc_id_switch_for_host; > 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 4ff2384..6553e4d 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 > > @@ -87,7 +88,10 @@ static inline int ci_hdrc_gadget_init(struct ci13xxx *ci) > { > return -ENXIO; > } > +static inline void ci_hdrc_gadget_destroy(struct ci13xxx *ci) > +{ > > +} Newlines are wrong here as well as in the udc.h bit. Which is weird, because they were ok in the original patch I sent you http://www.mail-archive.com/linux-usb@vger.kernel.org/msg13668.html > #endif > > #endif /* __DRIVERS_USB_CHIPIDEA_UDC_H */ > -- > 1.7.0.4 > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 06, 2013 at 07:09:34PM +0200, Alexander Shishkin wrote: > > - start/stop API are used at otg id switch and probe routine > > > > - Defer some gadget operations at ci's delayed work queue > > When I asked you to reorder patches, I didn't mean squash them all > into one. Now you have a patch that does 5 different things and none > of them properly, because you still need to amend these changes later > in the patchset, like the patch 7, where you remove the delayed work, > which is added in this patch. At the same time, you have bits in this > patch that should be moved to other patches. See comments below. > I am sorry to let you review difficult, as I changed/added patch according to function change, but not updated previous patches. I will re-organize all patches according to functionalities. Besides, can you help review all patches during next time, in that case, I can know which patch is ok. > > You remove this function in patch 7. Why add it in the first place? > As it is needed at patch 7 to let the function work > > + > > +static inline void ci_role_destroy(struct ci13xxx *ci) > > +{ > > + ci_hdrc_gadget_destroy(ci); > > + ci_hdrc_host_destroy(ci); > > +} > > You shouldn't use "inline" here. And the name of the function is also > ambiguous, since it destroys all roles. It would be better to just > call both _destroys(), like I suggested in the diff I gave you some > time ago. Will change. > > > + > > static ssize_t show_role(struct device *dev, struct device_attribute *attr, > > char *buf) > > { > > @@ -352,25 +468,50 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr, > > > > static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role); > > > > +static bool ci_is_otg_capable(struct ci13xxx *ci) > > +{ > > + return hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC | DCCPARAMS_HC) > > + == (DCCPARAMS_DC | DCCPARAMS_HC); > > +} > > Can't we > a) remember this value so that we don't have to read the register at > every interrupt Yes. > b) also take into account the fact that DCCPARAMS doesn't read > correctly on all chipideas? (see dr_mode patches) > We have discussed it, but you have not given me exactly answer "Which situation we can read OTGSC"? 1. We Can't read OTGSC if DCCPARAMS shows it is only device-capable 2. DCCPARAMS doesn't correctly for some chips 3. If we depend on dr_mode, OTGSC should be read when dr_mode = "peripheral", in that case, the condition is ci->roles[CI_ROLE_GADGET]. If the dr_mode is not set by DT, the 1 and 3 is conflict. > > > > - if (ci->is_otg) > > + if (ci_is_otg_capable(ci)) > > ci->is_otg is actually the right thing to use, we should instead make > sure that it's set properly > Yes, what is ci->is_otg stands for in your opinion? In my patchset, it means it supportes partial OTG (id switch) function. > > if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) { > > + dev_dbg(dev, "support otg\n"); > > ci->is_otg = true; > > + /* if otg is supported, create struct usb_otg */ > > + ci_hdrc_otg_init(ci); > > /* ID pin needs 1ms debouce time, we delay 2ms for safe */ > > mdelay(2); > > ci->role = ci_otg_role(ci); > > I'm thinking that this should be based on dr_mode patches and not the > other way around. > No, ci->role means which role is currently. And it is otg mode already as (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]). > > > > static int ci_otg_set_peripheral(struct usb_otg *otg, > > struct usb_gadget *periph) > > @@ -55,6 +62,7 @@ int ci_hdrc_otg_init(struct ci13xxx *ci) > > ci->otg.set_host = ci_otg_set_host; > > if (!IS_ERR_OR_NULL(ci->transceiver)) > > ci->transceiver->otg = &ci->otg; > > + ci_enable_otg_interrupt(ci, OTGSC_IDIE); > > There should be a counter-action to ci_hdrc_otg_init(). > Btw, why can't clear_otg_interrupt() be called from here as well > instead of hw_device_init()? What the criterion for reading OTGSC? What means otg capable and how do we know it is otg capable? > This function should only be called for > otg capable devices, so it should be safe. And while at it, I think > this whole otg.c/otg.h part of this patch should be moved to patch 2. > OK. > > > > +static int udc_id_switch_for_device(struct ci13xxx *ci) > > I would use _to_ instead of _for_, for clarity. > > > +{ > > + ci_clear_otg_interrupt(ci, OTGSC_BSVIS); > > + ci_enable_otg_interrupt(ci, OTGSC_BSVIE); > > + > > + return 0; > > +} > > + > > +static void udc_id_switch_for_host(struct ci13xxx *ci) > > Same here. > Will change > > Newlines are wrong here as well as in the udc.h bit. Which is weird, > because they were ok in the original patch I sent you > http://www.mail-archive.com/linux-usb@vger.kernel.org/msg13668.html > Will change
On Thu, Mar 07, 2013 at 01:50:24PM +0800, Peter Chen wrote: > On Wed, Mar 06, 2013 at 07:09:34PM +0200, Alexander Shishkin wrote: > > > - start/stop API are used at otg id switch and probe routine > > > > > > - Defer some gadget operations at ci's delayed work queue > > > > When I asked you to reorder patches, I didn't mean squash them all > > into one. Now you have a patch that does 5 different things and none > > of them properly, because you still need to amend these changes later > > in the patchset, like the patch 7, where you remove the delayed work, > > which is added in this patch. At the same time, you have bits in this > > patch that should be moved to other patches. See comments below. > > > > I am sorry to let you review difficult, as I changed/added patch according > to function change, but not updated previous patches. > > I will re-organize all patches according to functionalities. > Besides, can you help review all patches during next time, in that case, > I can know which patch is ok. > > > > > You remove this function in patch 7. Why add it in the first place? > > > > As it is needed at patch 7 to let the function work > > > > + > > > +static inline void ci_role_destroy(struct ci13xxx *ci) > > > +{ > > > + ci_hdrc_gadget_destroy(ci); > > > + ci_hdrc_host_destroy(ci); > > > +} > > > > You shouldn't use "inline" here. And the name of the function is also > > ambiguous, since it destroys all roles. It would be better to just > > call both _destroys(), like I suggested in the diff I gave you some > > time ago. > > Will change. > > > > > > + > > > static ssize_t show_role(struct device *dev, struct device_attribute *attr, > > > char *buf) > > > { > > > @@ -352,25 +468,50 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr, > > > > > > static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role); > > > > > > +static bool ci_is_otg_capable(struct ci13xxx *ci) > > > +{ > > > + return hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC | DCCPARAMS_HC) > > > + == (DCCPARAMS_DC | DCCPARAMS_HC); > > > +} > > > > Can't we > > a) remember this value so that we don't have to read the register at > > every interrupt > > Yes. > > > b) also take into account the fact that DCCPARAMS doesn't read > > correctly on all chipideas? (see dr_mode patches) > > > > We have discussed it, but you have not given me exactly answer > "Which situation we can read OTGSC"? > > 1. We Can't read OTGSC if DCCPARAMS shows it is only device-capable > 2. DCCPARAMS doesn't correctly for some chips > 3. If we depend on dr_mode, OTGSC should be read when dr_mode = "peripheral", > in that case, the condition is ci->roles[CI_ROLE_GADGET]. > > If the dr_mode is not set by DT, the 1 and 3 is conflict. > > > > > > > > - if (ci->is_otg) > > > + if (ci_is_otg_capable(ci)) > > > > ci->is_otg is actually the right thing to use, we should instead make > > sure that it's set properly > > > > Yes, what is ci->is_otg stands for in your opinion? > In my patchset, it means it supportes partial OTG (id switch) function. > > > > if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) { > > > + dev_dbg(dev, "support otg\n"); > > > ci->is_otg = true; > > > + /* if otg is supported, create struct usb_otg */ > > > + ci_hdrc_otg_init(ci); > > > /* ID pin needs 1ms debouce time, we delay 2ms for safe */ > > > mdelay(2); > > > ci->role = ci_otg_role(ci); > > > > I'm thinking that this should be based on dr_mode patches and not the > > other way around. > > > > No, ci->role means which role is currently. > And it is otg mode already as > (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]). > > > > > > > static int ci_otg_set_peripheral(struct usb_otg *otg, > > > struct usb_gadget *periph) > > > @@ -55,6 +62,7 @@ int ci_hdrc_otg_init(struct ci13xxx *ci) > > > ci->otg.set_host = ci_otg_set_host; > > > if (!IS_ERR_OR_NULL(ci->transceiver)) > > > ci->transceiver->otg = &ci->otg; > > > + ci_enable_otg_interrupt(ci, OTGSC_IDIE); > > > > There should be a counter-action to ci_hdrc_otg_init(). > > Btw, why can't clear_otg_interrupt() be called from here as well > > instead of hw_device_init()? > > What the criterion for reading OTGSC? > What means otg capable and how do we know it is otg capable? > > > This function should only be called for > > otg capable devices, so it should be safe. And while at it, I think > > this whole otg.c/otg.h part of this patch should be moved to patch 2. > > > > OK. > > > > > > > +static int udc_id_switch_for_device(struct ci13xxx *ci) > > > > I would use _to_ instead of _for_, for clarity. > > > > > +{ > > > + ci_clear_otg_interrupt(ci, OTGSC_BSVIS); > > > + ci_enable_otg_interrupt(ci, OTGSC_BSVIE); > > > + > > > + return 0; > > > +} > > > + > > > +static void udc_id_switch_for_host(struct ci13xxx *ci) > > Hi Alan, Would you like to answer my questions, esp for "the situation to read OTGSC", we discussed it lots of times, but seems has no conclusion. Thanks.
diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h index d8ffc2f..58ef56c 100644 --- a/drivers/usb/chipidea/bits.h +++ b/drivers/usb/chipidea/bits.h @@ -77,11 +77,21 @@ #define OTGSC_ASVIS BIT(18) #define OTGSC_BSVIS BIT(19) #define OTGSC_BSEIS BIT(20) +#define OTGSC_1MSIS BIT(21) +#define OTGSC_DPIS BIT(22) #define OTGSC_IDIE BIT(24) #define OTGSC_AVVIE BIT(25) #define OTGSC_ASVIE BIT(26) #define OTGSC_BSVIE BIT(27) #define OTGSC_BSEIE BIT(28) +#define OTGSC_1MSIE BIT(29) +#define OTGSC_DPIE BIT(30) +#define OTGSC_INT_EN_BITS (OTGSC_IDIE | OTGSC_AVVIE | OTGSC_ASVIE \ + | OTGSC_BSVIE | OTGSC_BSEIE | OTGSC_1MSIE \ + | OTGSC_DPIE) +#define OTGSC_INT_STATUS_BITS (OTGSC_IDIS | OTGSC_AVVIS | OTGSC_ASVIS \ + | OTGSC_BSVIS | OTGSC_BSEIS | OTGSC_1MSIS \ + | OTGSC_DPIS) /* USBMODE */ #define USBMODE_CM (0x03UL << 0) diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h index 697e369..a3777a1 100644 --- a/drivers/usb/chipidea/ci.h +++ b/drivers/usb/chipidea/ci.h @@ -130,6 +130,9 @@ struct hw_bank { * @transceiver: pointer to USB PHY, if any * @hcd: pointer to usb_hcd for ehci host driver * @otg: for otg support + * @id_event: indicates there is a id event, and handled at ci_otg_work + * @b_sess_valid_event: indicates there is a vbus event, and handled + * at ci_otg_work */ struct ci13xxx { struct device *dev; @@ -140,6 +143,7 @@ struct ci13xxx { enum ci_role role; bool is_otg; struct work_struct work; + struct delayed_work dwork; struct workqueue_struct *wq; struct dma_pool *qh_pool; @@ -166,6 +170,8 @@ struct ci13xxx { struct usb_phy *transceiver; struct usb_hcd *hcd; struct usb_otg otg; + bool id_event; + bool b_sess_valid_event; }; static inline struct ci_role_driver *ci_role(struct ci13xxx *ci) @@ -201,7 +207,6 @@ static inline void ci_role_stop(struct ci13xxx *ci) ci->roles[role]->stop(ci); } - /****************************************************************************** * REGISTERS *****************************************************************************/ diff --git a/drivers/usb/chipidea/ci13xxx_imx.c b/drivers/usb/chipidea/ci13xxx_imx.c index 136869b..793fdba 100644 --- a/drivers/usb/chipidea/ci13xxx_imx.c +++ b/drivers/usb/chipidea/ci13xxx_imx.c @@ -110,7 +110,8 @@ static int ci13xxx_imx_probe(struct platform_device *pdev) pdata->capoffset = DEF_CAPOFFSET; pdata->flags = CI13XXX_REQUIRE_TRANSCEIVER | CI13XXX_PULLUP_ON_VBUS | - CI13XXX_DISABLE_STREAMING; + CI13XXX_DISABLE_STREAMING | + CI13XXX_REGS_SHARED; pdata->phy_mode = of_usb_get_phy_mode(pdev->dev.of_node); pdata->dr_mode = of_usb_get_dr_mode(pdev->dev.of_node); diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index c89f2aa..c563e17 100644 --- a/drivers/usb/chipidea/core.c +++ b/drivers/usb/chipidea/core.c @@ -75,6 +75,7 @@ #include "bits.h" #include "host.h" #include "debug.h" +#include "otg.h" /* Controller register map */ static uintptr_t ci_regs_nolpm[] = { @@ -201,6 +202,14 @@ static int hw_device_init(struct ci13xxx *ci, void __iomem *base) if (ci->hw_ep_max > ENDPT_MAX) return -ENODEV; + /* Disable all interrupts bits */ + hw_write(ci, OP_USBINTR, 0xffffffff, 0); + ci_disable_otg_interrupt(ci, OTGSC_INT_EN_BITS); + + /* Clear all interrupts status bits*/ + hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff); + ci_clear_otg_interrupt(ci, OTGSC_INT_STATUS_BITS); + dev_dbg(ci->dev, "ChipIdea HDRC found, lpm: %d; cap: %p op: %p\n", ci->hw_bank.lpm, ci->hw_bank.cap, ci->hw_bank.op); @@ -302,24 +311,131 @@ static enum ci_role ci_otg_role(struct ci13xxx *ci) } /** - * ci_role_work - perform role changing based on ID pin - * @work: work struct + * hw_wait_reg: wait the register value + * + * Sometimes, it needs to wait register value before going on. + * Eg, when switch to device mode, the vbus value should be lower + * than OTGSC_BSV before connects to host. + * + * @ci: the controller + * @reg: register index + * @mask: mast bit + * @value: the bit value to wait + * @timeout: timeout to indicate an error + * + * This function returns an error code if timeout */ -static void ci_role_work(struct work_struct *work) +static int hw_wait_reg(struct ci13xxx *ci, enum ci13xxx_regs reg, u32 mask, + u32 value, unsigned long timeout) +{ + unsigned long elapse = jiffies + timeout; + + while (hw_read(ci, reg, mask) != value) { + if (time_after(jiffies, elapse)) { + dev_err(ci->dev, "timeout waiting for %08x in %d\n", + mask, reg); + return -ETIMEDOUT; + } + msleep(20); + } + + return 0; +} + +/* + * Since there are some capacitances at vbus, the vbus may not + * change very quickly when we stop/start internal 5v. + * Below is a vbus stable timeout value, the routine will quit + * if the vbus gets the required value, we can think there are some + * problems for hardware if the vbus can't get the required value + * within 5 seconds. + */ +#define CI_VBUS_STABLE_TIMEOUT 500 +static void ci_handle_id_switch(struct ci13xxx *ci) { - struct ci13xxx *ci = container_of(work, struct ci13xxx, work); enum ci_role role = ci_otg_role(ci); if (role != ci->role) { dev_dbg(ci->dev, "switching from %s to %s\n", ci_role(ci)->name, ci->roles[role]->name); + /* 1. Finish the current role */ ci_role_stop(ci); + hw_device_reset(ci, USBMODE_CM_IDLE); + + /* 2. Turn on/off vbus according to coming role */ + if (role == CI_ROLE_GADGET) { + otg_set_vbus(&ci->otg, false); + /* wait vbus lower than OTGSC_BSV */ + hw_wait_reg(ci, OP_OTGSC, OTGSC_BSV, 0, + CI_VBUS_STABLE_TIMEOUT); + } else if (role == CI_ROLE_HOST) { + otg_set_vbus(&ci->otg, true); + /* wait vbus higher than OTGSC_AVV */ + hw_wait_reg(ci, OP_OTGSC, OTGSC_AVV, OTGSC_AVV, + CI_VBUS_STABLE_TIMEOUT); + } + + /* 3. Begin the new role */ ci_role_start(ci, role); - enable_irq(ci->irq); } } +static void ci_handle_vbus_change(struct ci13xxx *ci) +{ + u32 otgsc = hw_read(ci, OP_OTGSC, ~0); + + if (otgsc & OTGSC_BSV) + usb_gadget_vbus_connect(&ci->gadget); + else + usb_gadget_vbus_disconnect(&ci->gadget); +} + +/** + * ci_otg_work - perform otg (vbus/id) event handle + * @work: work struct + */ +static void ci_otg_work(struct work_struct *work) +{ + struct ci13xxx *ci = container_of(work, struct ci13xxx, work); + + if (ci->id_event) { + ci->id_event = false; + ci_handle_id_switch(ci); + } else if (ci->b_sess_valid_event) { + ci->b_sess_valid_event = false; + ci_handle_vbus_change(ci); + } else + dev_err(ci->dev, "unexpected event occurs at %s\n", __func__); + + enable_irq(ci->irq); +} + +static void ci_delayed_work(struct work_struct *work) +{ + struct delayed_work *dwork = to_delayed_work(work); + struct ci13xxx *ci = container_of(dwork, struct ci13xxx, dwork); + + if (ci->role == CI_ROLE_GADGET) { + /* + * if it is device mode: + * - Enable vbus detect + * - If it has already connected to host, notify udc + */ + ci_enable_otg_interrupt(ci, OTGSC_BSVIE); + ci_handle_vbus_change(ci); + } else if (ci->is_otg && (ci->role == CI_ROLE_HOST)) { + /* USB Device at the MicroB to A cable */ + otg_set_vbus(&ci->otg, true); + } +} + +static inline void ci_role_destroy(struct ci13xxx *ci) +{ + ci_hdrc_gadget_destroy(ci); + ci_hdrc_host_destroy(ci); +} + static ssize_t show_role(struct device *dev, struct device_attribute *attr, char *buf) { @@ -352,25 +468,50 @@ static ssize_t store_role(struct device *dev, struct device_attribute *attr, static DEVICE_ATTR(role, S_IRUSR | S_IWUSR, show_role, store_role); +static bool ci_is_otg_capable(struct ci13xxx *ci) +{ + return hw_read(ci, CAP_DCCPARAMS, DCCPARAMS_DC | DCCPARAMS_HC) + == (DCCPARAMS_DC | DCCPARAMS_HC); +} + static irqreturn_t ci_irq(int irq, void *data) { struct ci13xxx *ci = data; irqreturn_t ret = IRQ_NONE; u32 otgsc = 0; - if (ci->is_otg) + if (ci_is_otg_capable(ci)) otgsc = hw_read(ci, OP_OTGSC, ~0); - if (ci->role != CI_ROLE_END) - ret = ci_role(ci)->irq(ci); + /* + * Handle id change interrupt, it indicates device/host function + * switch. + */ + if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) { + ci->id_event = true; + ci_clear_otg_interrupt(ci, OTGSC_IDIS); + disable_irq_nosync(ci->irq); + queue_work(ci->wq, &ci->work); + return IRQ_HANDLED; + } - if (ci->is_otg && (otgsc & OTGSC_IDIS)) { - hw_write(ci, OP_OTGSC, OTGSC_IDIS, OTGSC_IDIS); + /* + * Handle vbus change interrupt, it indicates device connection + * and disconnection events. + */ + if (ci_is_otg_capable(ci) && (otgsc & OTGSC_BSVIE) + && (otgsc & OTGSC_BSVIS)) { + ci->b_sess_valid_event = true; + ci_clear_otg_interrupt(ci, OTGSC_BSVIS); disable_irq_nosync(ci->irq); queue_work(ci->wq, &ci->work); - ret = IRQ_HANDLED; + return IRQ_HANDLED; } + /* Handle device/host interrupt */ + if (ci->role != CI_ROLE_END) + ret = ci_role(ci)->irq(ci); + return ret; } @@ -481,7 +622,8 @@ static int ci_hdrc_probe(struct platform_device *pdev) return -ENODEV; } - INIT_WORK(&ci->work, ci_role_work); + INIT_WORK(&ci->work, ci_otg_work); + INIT_DELAYED_WORK(&ci->dwork, ci_delayed_work); ci->wq = create_singlethread_workqueue("ci_otg"); if (!ci->wq) { dev_err(dev, "can't create workqueue\n"); @@ -512,7 +654,10 @@ static int ci_hdrc_probe(struct platform_device *pdev) } if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) { + dev_dbg(dev, "support otg\n"); ci->is_otg = true; + /* if otg is supported, create struct usb_otg */ + ci_hdrc_otg_init(ci); /* ID pin needs 1ms debouce time, we delay 2ms for safe */ mdelay(2); ci->role = ci_otg_role(ci); @@ -528,7 +673,7 @@ static int ci_hdrc_probe(struct platform_device *pdev) if (ret) { dev_err(dev, "can't start %s role\n", ci_role(ci)->name); ret = -ENODEV; - goto rm_wq; + goto free_memory; } platform_set_drvdata(pdev, ci); @@ -539,17 +684,22 @@ static int ci_hdrc_probe(struct platform_device *pdev) ret = device_create_file(dev, &dev_attr_role); if (ret) - goto rm_attr; + goto free_irq; - if (ci->is_otg) - hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE); + /* Defer some operations */ + queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(200)); return ret; -rm_attr: - device_remove_file(dev, &dev_attr_role); +free_irq: + free_irq(ci->irq, ci); stop: - ci_role_stop(ci); + ci_role_destroy(ci); +free_memory: + if (ci->roles[CI_ROLE_HOST]) + devm_kfree(dev, ci->roles[CI_ROLE_HOST]); + if (ci->roles[CI_ROLE_GADGET]) + devm_kfree(dev, ci->roles[CI_ROLE_GADGET]); rm_wq: flush_workqueue(ci->wq); destroy_workqueue(ci->wq); @@ -565,7 +715,7 @@ 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_stop(ci); + ci_role_destroy(ci); return 0; } diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index 8e9d312..ead3158 100644 --- a/drivers/usb/chipidea/host.c +++ b/drivers/usb/chipidea/host.c @@ -105,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..19cc72d 100644 --- a/drivers/usb/chipidea/host.h +++ b/drivers/usb/chipidea/host.h @@ -4,13 +4,17 @@ #ifdef CONFIG_USB_CHIPIDEA_HOST int ci_hdrc_host_init(struct ci13xxx *ci); - +void ci_hdrc_host_destroy(struct ci13xxx *ci); #else static inline int ci_hdrc_host_init(struct ci13xxx *ci) { return -ENXIO; } +static inline void ci_hdrc_host_destroy(struct ci13xxx *ci) +{ + +} #endif diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c index 7dea3b3..2986d91 100644 --- a/drivers/usb/chipidea/otg.c +++ b/drivers/usb/chipidea/otg.c @@ -10,21 +10,28 @@ * published by the Free Software Foundation. */ -#include <linux/platform_device.h> -#include <linux/module.h> -#include <linux/io.h> -#include <linux/irq.h> -#include <linux/kernel.h> -#include <linux/slab.h> -#include <linux/usb/gadget.h> #include <linux/usb/otg.h> +#include <linux/usb/gadget.h> #include <linux/usb/chipidea.h> #include "ci.h" -#include "udc.h" #include "bits.h" -#include "host.h" -#include "debug.h" + +void ci_clear_otg_interrupt(struct ci13xxx *ci, u32 bits) +{ + /* Only clear request bits */ + hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, bits); +} + +void ci_enable_otg_interrupt(struct ci13xxx *ci, u32 bits) +{ + hw_write(ci, OP_OTGSC, bits, bits); +} + +void ci_disable_otg_interrupt(struct ci13xxx *ci, u32 bits) +{ + hw_write(ci, OP_OTGSC, bits, 0); +} static int ci_otg_set_peripheral(struct usb_otg *otg, struct usb_gadget *periph) @@ -55,6 +62,7 @@ int ci_hdrc_otg_init(struct ci13xxx *ci) ci->otg.set_host = ci_otg_set_host; if (!IS_ERR_OR_NULL(ci->transceiver)) ci->transceiver->otg = &ci->otg; + ci_enable_otg_interrupt(ci, OTGSC_IDIE); return 0; } diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h index b4c6b3e..fa30428 100644 --- a/drivers/usb/chipidea/otg.h +++ b/drivers/usb/chipidea/otg.h @@ -2,5 +2,8 @@ #define __DRIVERS_USB_CHIPIDEA_OTG_H int ci_hdrc_otg_init(struct ci13xxx *ci); +void ci_clear_otg_interrupt(struct ci13xxx *ci, u32 bits); +void ci_enable_otg_interrupt(struct ci13xxx *ci, u32 bits); +void ci_disable_otg_interrupt(struct ci13xxx *ci, u32 bits); #endif /* __DRIVERS_USB_CHIPIDEA_OTG_H */ diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index d214448..e82dae4 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -31,6 +31,7 @@ #include "ci.h" #include "udc.h" +#include "otg.h" #include "bits.h" #include "debug.h" @@ -1371,6 +1372,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active) pm_runtime_get_sync(&_gadget->dev); hw_device_reset(ci, USBMODE_CM_DC); hw_device_state(ci, ci->ep0out->qh.dma); + dev_dbg(ci->dev, "Connected to host\n"); } else { hw_device_state(ci, 0); if (ci->platdata->notify_event) @@ -1378,6 +1380,7 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active) CI13XXX_CONTROLLER_STOPPED_EVENT); _gadget_stop_activity(&ci->gadget); pm_runtime_put_sync(&_gadget->dev); + dev_dbg(ci->dev, "Disconnected from host\n"); } } @@ -1741,7 +1744,12 @@ static int udc_start(struct ci13xxx *ci) if (!IS_ERR_OR_NULL(ci->transceiver)) { retval = otg_set_peripheral(ci->transceiver->otg, &ci->gadget); - if (retval) + /* + * If we implement all USB functions using chipidea drivers, + * it doesn't need to call above API, meanwhile, if we only + * use gadget function, calling above API is useless. + */ + if (retval && retval != -ENOTSUPP) goto remove_dbg; } @@ -1778,12 +1786,27 @@ free_qh_pool: return retval; } +static int udc_id_switch_for_device(struct ci13xxx *ci) +{ + ci_clear_otg_interrupt(ci, OTGSC_BSVIS); + ci_enable_otg_interrupt(ci, OTGSC_BSVIE); + + return 0; +} + +static void udc_id_switch_for_host(struct ci13xxx *ci) +{ + /* host doesn't care B_SESSION_VALID event */ + ci_clear_otg_interrupt(ci, OTGSC_BSVIS); + ci_disable_otg_interrupt(ci, OTGSC_BSVIE); +} + /** - * udc_remove: parent remove must call this to remove UDC + * ci_hdrc_gadget_destroy: parent remove must call this to remove UDC * * No interrupts active, the IRQ has been released */ -static void udc_stop(struct ci13xxx *ci) +void ci_hdrc_gadget_destroy(struct ci13xxx *ci) { if (ci == NULL) return; @@ -1802,15 +1825,13 @@ 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)); } /** * ci_hdrc_gadget_init - initialize device related bits * ci: the controller * - * This function enables the gadget role, if the device is "device capable". + * This function initializes gadget, if the device is "device capable". */ int ci_hdrc_gadget_init(struct ci13xxx *ci) { @@ -1823,11 +1844,11 @@ int ci_hdrc_gadget_init(struct ci13xxx *ci) if (!rdrv) return -ENOMEM; - rdrv->start = udc_start; - rdrv->stop = udc_stop; + rdrv->start = udc_id_switch_for_device; + rdrv->stop = udc_id_switch_for_host; 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 4ff2384..6553e4d 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 @@ -87,7 +88,10 @@ static inline int ci_hdrc_gadget_init(struct ci13xxx *ci) { return -ENXIO; } +static inline void ci_hdrc_gadget_destroy(struct ci13xxx *ci) +{ +} #endif #endif /* __DRIVERS_USB_CHIPIDEA_UDC_H */
The main design flow is the same with msm otg driver, that is the id and vbus interrupt are handled at core driver, others are handled by individual drivers. - At former design, when switch usb role from device->host, it will call udc_stop, it will remove the gadget driver, so when switch role from host->device, it can't add gadget driver any more. At new design, when role switch occurs, the gadget just calls usb_gadget_vbus_disconnect/usb_gadget_vbus_connect as well as reset controller, it will not free any device/gadget structure - Add vbus connect and disconnect to core interrupt handler, it can notify udc driver by calling usb_gadget_vbus_disconnect /usb_gadget_vbus_connect. - vbus interrupt needs to be handled when gadget function is enabled - Create/destroy the gadget at udc's init and destory function - start/stop API are used at otg id switch and probe routine - Defer some gadget operations at ci's delayed work queue Signed-off-by: Peter Chen <peter.chen@freescale.com> --- drivers/usb/chipidea/bits.h | 10 ++ drivers/usb/chipidea/ci.h | 7 +- drivers/usb/chipidea/ci13xxx_imx.c | 3 +- drivers/usb/chipidea/core.c | 190 ++++++++++++++++++++++++++++++++---- drivers/usb/chipidea/host.c | 6 + drivers/usb/chipidea/host.h | 6 +- drivers/usb/chipidea/otg.c | 28 ++++-- drivers/usb/chipidea/otg.h | 3 + drivers/usb/chipidea/udc.c | 39 ++++++-- drivers/usb/chipidea/udc.h | 4 + 10 files changed, 254 insertions(+), 42 deletions(-)