Peter Chen <peter.chen@xxxxxxxxxxxxx> writes:
> 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.
This looks good to me in general. Few comments below.
> - 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.
Well, it can, only you need to reload the gadget module for that, but
that's more current gadget framework's shortcoming...
> 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
...but I do agree that this is much better.
One thing we need to address here is CI13XXX_PULLUP_ON_VBUS. Since we
now handle vbus session in the core driver, we should just retire this
flag. And this vbus_disconnect() method won't work if PULLUP_ON_VBUS is
not set anyway, since vbus_session is a nop in that case.
> - Add vbus connect and disconnect to core interrupt handler, it can
> notify udc driver by calling
> usb_gadget_vbus_disconnect/usb_gadget_vbus_connect.
>
> - Add otg.c to implement struct usb_otg, in that case, calling otg_set_peripheral
> will not be failed at udc.c. Besides, we enable id interrupt at
> ci_hdrc_otg_init.
Can you make this a separate patch?
> - Add special case handling, like connecting to host during boot up at device
> mode, usb device at the MicroB to A cable at host mode, etc.
This can probably be a separate patch too, since it's kind of a separate
issue.
> Signed-off-by: Peter Chen <peter.chen@xxxxxxxxxxxxx>
> ---
> drivers/usb/chipidea/Makefile | 2 +-
> drivers/usb/chipidea/bits.h | 10 ++
> drivers/usb/chipidea/ci.h | 6 +
> drivers/usb/chipidea/core.c | 217 +++++++++++++++++++++++++++++++++++++----
> drivers/usb/chipidea/otg.c | 60 +++++++++++
> drivers/usb/chipidea/otg.h | 6 +
> drivers/usb/chipidea/udc.c | 2 +
> 7 files changed, 284 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/chipidea/Makefile b/drivers/usb/chipidea/Makefile
> index d92ca32..11f513c 100644
> --- a/drivers/usb/chipidea/Makefile
> +++ b/drivers/usb/chipidea/Makefile
> @@ -2,7 +2,7 @@ ccflags-$(CONFIG_USB_CHIPIDEA_DEBUG) := -DDEBUG
>
> obj-$(CONFIG_USB_CHIPIDEA) += ci_hdrc.o
>
> -ci_hdrc-y := core.o
> +ci_hdrc-y := core.o otg.o
> ci_hdrc-$(CONFIG_USB_CHIPIDEA_UDC) += udc.o
> ci_hdrc-$(CONFIG_USB_CHIPIDEA_HOST) += host.o
> ci_hdrc-$(CONFIG_USB_CHIPIDEA_DEBUG) += debug.o
> diff --git a/drivers/usb/chipidea/bits.h b/drivers/usb/chipidea/bits.h
> index 050de85..4b6ae3e 100644
> --- a/drivers/usb/chipidea/bits.h
> +++ b/drivers/usb/chipidea/bits.h
> @@ -65,11 +65,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 (BIT(24) | BIT(25) | BIT(26) \
> + | BIT(27) | BIT(28) | BIT(29) \
> + | BIT(30))
> +#define OTGSC_INT_STATUS_BITS (BIT(16) | BIT(17) | BIT(18) \
> + | BIT(19) | BIT(20) | BIT(21) \
> + | BIT(22))
Why not use the OTGSC_* defines instead of bit numbers?
>
> /* USBMODE */
> #define USBMODE_CM (0x03UL << 0)
> diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> index d738603..d32f932 100644
> --- a/drivers/usb/chipidea/ci.h
> +++ b/drivers/usb/chipidea/ci.h
> @@ -139,6 +139,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;
> @@ -164,6 +165,11 @@ struct ci13xxx {
> bool global_phy;
> struct usb_phy *transceiver;
> struct usb_hcd *hcd;
> + /* events handled at ci_role_work */
> +#define ID 0
> +#define B_SESS_VALID 1
> + unsigned long events;
I would just use bools instead.
> + struct usb_otg otg;
> };
>
> static inline struct ci_role_driver *ci_role(struct ci13xxx *ci)
> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c
> index f69d029..b50b77a 100644
> --- a/drivers/usb/chipidea/core.c
> +++ b/drivers/usb/chipidea/core.c
> @@ -73,6 +73,7 @@
> #include "bits.h"
> #include "host.h"
> #include "debug.h"
> +#include "otg.h"
>
> /* Controller register map */
> static uintptr_t ci_regs_nolpm[] = {
> @@ -264,25 +265,138 @@ static enum ci_role ci_otg_role(struct ci13xxx *ci)
> return role;
> }
>
> +#define CI_WAIT_VBUS_STABLE_TIMEOUT 500
> /**
> - * ci_role_work - perform role changing based on ID pin
> - * @work: work struct
> + * ci_wait_vbus_stable
> + * When usb role switches, we need to turn on/off internal vbus
> + * regulaor, sometimes, the real vbus value may not lower fast
> + * enough due to capacitance, and we do want the vbus lower
> + * than 0.8v if it is device mode, and higher than 4.4v, if it
> + * is host mode.
> + *
> + * @low: true, Wait vbus lower than B_SESSION_VALID
> + * : false, Wait vbus higher than A_VBUS_VALID
Instead of this you could just pass otgsc bit mask here (OTGSC_BSV or
OTGSC_AVV), would make it shorter and easier to read from the caller.
> */
> -static void ci_role_work(struct work_struct *work)
> +static void ci_wait_vbus_stable(struct ci13xxx *ci, bool low)
> +{
> + unsigned long timeout;
> + u32 otgsc = hw_read(ci, OP_OTGSC, ~0);
> +
> + timeout = jiffies + CI_WAIT_VBUS_STABLE_TIMEOUT;
> +
> + if (low) {
> + while (otgsc & OTGSC_BSV) {
> + if (time_after(jiffies, timeout)) {
> + dev_err(ci->dev, "wait vbus lower than\
> + B_SESS_VALID timeout!\n");
> + return;
> + }
> + msleep(20);
> + otgsc = hw_read(ci, OP_OTGSC, ~0);
> + }
> + } else {
> + while (!(otgsc & OTGSC_AVV)) {
> + if (time_after(jiffies, timeout)) {
> + dev_err(ci->dev, "wait vbus higher than\
> + A_VBUS_VALID timeout!\n");
> + return;
> + }
> + msleep(20);
> + otgsc = hw_read(ci, OP_OTGSC, ~0);
> + }
> +
> + }
> +}
> +
> +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);
>
> - ci_role_stop(ci);
> - ci_role_start(ci, role);
> - enable_irq(ci->irq);
> + /* 1. Finish the current role */
> + if (ci->role == CI_ROLE_GADGET) {
> + usb_gadget_vbus_disconnect(&ci->gadget);
> + /* host doesn't care B_SESSION_VALID event */
> + hw_write(ci, OP_OTGSC, OTGSC_BSVIE, ~OTGSC_BSVIE);
> + hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
> + ci->role = CI_ROLE_END;
> + /* reset controller */
> + hw_write(ci, OP_USBCMD, USBCMD_RST, USBCMD_RST);
> + while (hw_read(ci, OP_USBCMD, USBCMD_RST))
> + udelay(10);
I would prefer we use hw_device_reset() everywhere where we reset the
controller. Actually, same goes for run/stop bit.
> + } else if (ci->role == CI_ROLE_HOST) {
> + ci_role_stop(ci);
> + /* reset controller */
> + hw_write(ci, OP_USBCMD, USBCMD_RST, USBCMD_RST);
> + while (hw_read(ci, OP_USBCMD, USBCMD_RST))
> + udelay(10);
> + }
> +
> + /* 2. Turn on/off vbus according to coming role */
> + if (ci_otg_role(ci) == CI_ROLE_GADGET) {
> + otg_set_vbus(&ci->otg, false);
> + ci_wait_vbus_stable(ci, true);
> + } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
> + otg_set_vbus(&ci->otg, true);
> + ci_wait_vbus_stable(ci, false);
> + }
> +
> + /* 3. Begin the new role */
> + if (ci_otg_role(ci) == CI_ROLE_GADGET) {
> + ci->role = role;
> + hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
> + hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
> + } else if (ci_otg_role(ci) == CI_ROLE_HOST) {
> + ci_role_start(ci, role);
> + }
> }
> }
>
> +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 (test_bit(ID, &ci->events)) {
> + clear_bit(ID, &ci->events);
> + ci_handle_id_switch(ci);
> + } else if (test_bit(B_SESS_VALID, &ci->events)) {
> + clear_bit(B_SESS_VALID, &ci->events);
> + 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);
> + u32 otgsc = hw_read(ci, OP_OTGSC, ~0);
> +
> + /* Handle the situation that usb device at the MicroB to A cable */
> + if (ci->is_otg && !(otgsc & OTGSC_ID))
> + otg_set_vbus(&ci->otg, true);
> +
> +}
> +
> static ssize_t show_role(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> @@ -321,19 +435,36 @@ static irqreturn_t ci_irq(int irq, void *data)
> irqreturn_t ret = IRQ_NONE;
> u32 otgsc = 0;
>
> - if (ci->is_otg)
> - otgsc = hw_read(ci, OP_OTGSC, ~0);
> + otgsc = hw_read(ci, OP_OTGSC, ~0);
>
> - if (ci->role != CI_ROLE_END)
> - ret = ci_role(ci)->irq(ci);
> -
> - if (ci->is_otg && (otgsc & OTGSC_IDIS)) {
> + /*
> + * Handle id change interrupt, it indicates device/host function
> + * switch.
> + */
> + if (ci->is_otg && (otgsc & OTGSC_IDIE) && (otgsc & OTGSC_IDIS)) {
> + set_bit(ID, &ci->events);
> hw_write(ci, OP_OTGSC, OTGSC_IDIS, OTGSC_IDIS);
> disable_irq_nosync(ci->irq);
> queue_work(ci->wq, &ci->work);
> - ret = IRQ_HANDLED;
> + return IRQ_HANDLED;
> }
>
> + /*
> + * Handle vbus change interrupt, it indicates device connection
> + * and disconnection events.
> + */
> + if ((otgsc & OTGSC_BSVIE) && (otgsc & OTGSC_BSVIS)) {
> + set_bit(B_SESS_VALID, &ci->events);
> + hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
> + disable_irq_nosync(ci->irq);
> + queue_work(ci->wq, &ci->work);
> + return IRQ_HANDLED;
> + }
> +
> + /* Handle device/host interrupt */
> + if (ci->role != CI_ROLE_END)
> + ret = ci_role(ci)->irq(ci);
> +
> return ret;
> }
>
> @@ -397,6 +528,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> struct resource *res;
> void __iomem *base;
> int ret;
> + u32 otgsc;
>
> if (!dev->platform_data) {
> dev_err(dev, "platform data missing\n");
> @@ -421,6 +553,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> return -ENOMEM;
> }
>
> + ci->events = 0;
> ci->dev = dev;
> ci->platdata = dev->platform_data;
> if (ci->platdata->phy)
> @@ -442,12 +575,20 @@ static int __devinit 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");
> return -ENODEV;
> }
> + /* Disable all interrupts bits */
> + hw_write(ci, OP_USBINTR, 0xffffffff, 0);
> + hw_write(ci, OP_OTGSC, OTGSC_INT_EN_BITS, ~OTGSC_INT_EN_BITS);
hw_write(ci, OP_OTGSC, OTGSC_INT_EN_BITS, 0);
will work to the same effect and is a bit easier on the eyes.
> +
> + /* Clear all interrupts status bits*/
> + hw_write(ci, OP_USBSTS, 0xffffffff, 0xffffffff);
> + hw_write(ci, OP_OTGSC, OTGSC_INT_STATUS_BITS, OTGSC_INT_STATUS_BITS);
>
> /* initialize role(s) before the interrupt is requested */
> ret = ci_hdrc_host_init(ci);
> @@ -465,6 +606,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> }
>
> if (ci->roles[CI_ROLE_HOST] && ci->roles[CI_ROLE_GADGET]) {
> + dev_info(dev, "support otg\n");
Looks more like a dev_dbg() to me.
> ci->is_otg = true;
> /* ID pin needs 1ms debouce time, we delay 2ms for safe */
> mdelay(2);
> @@ -475,13 +617,53 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> : CI_ROLE_GADGET;
> }
>
> + if (ci->is_otg) {
> + /* if otg is supported, create struct usb_otg */
> + ci_hdrc_otg_init(ci);
> +
> + /*
> + * If it is host role at otg port, start gadget role first
> + * as we need to allocate device struct.
> + */
> + if (ci->role == CI_ROLE_HOST) {
> +
> + ret = ci_role_start(ci, CI_ROLE_GADGET);
This is kind of a hack. In the worst case you could move gadegt
initialization to the role's init method. Or we could add a role
destructor method and then
1) allocate gadget stuff on first call to role start
2) make role stop a nop
3) deallocate gadget stuff on role destroy()
4) call role's destroy() in device remove path instead of role stop
5) call ci_role_start/ci_role_stop from ci_handle_id_switch()
unconditionally.
Does this make sense to you?
> + if (ret) {
> + dev_err(dev, "can't start gadget role, ret=%d\n",
> + ret);
> + ret = -ENODEV;
> + goto rm_wq;
> + }
> +
> + usb_gadget_vbus_disconnect(&ci->gadget);
> + /* host doesn't care B_SESSION_VALID event */
> + hw_write(ci, OP_OTGSC, OTGSC_BSVIE, ~OTGSC_BSVIE);
> + hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
> + ci->role = CI_ROLE_HOST;
> + }
> + msleep(20);
> + }
> +
> ret = ci_role_start(ci, ci->role);
> if (ret) {
> - dev_err(dev, "can't start %s role\n", ci_role(ci)->name);
> + dev_err(dev, "can't start %s role, ret=%d\n",
> + ci_role(ci)->name, ret);
> ret = -ENODEV;
> goto rm_wq;
> }
>
> + otgsc = hw_read(ci, OP_OTGSC, ~0);
> + /*
> + * if it is device mode:
> + * - Enable vbus detect
> + * - If it has already connected to host, notify udc
> + */
> + if (ci->role == CI_ROLE_GADGET) {
> + hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
> + if (otgsc & OTGSC_BSV)
> + usb_gadget_vbus_connect(&ci->gadget);
This looks a lot like ci_handle_vbus_change(), doesn't it?
> + }
> +
> platform_set_drvdata(pdev, ci);
> ret = request_irq(ci->irq, ci_irq, IRQF_SHARED, ci->platdata->name,
> ci);
> @@ -492,8 +674,7 @@ static int __devinit ci_hdrc_probe(struct platform_device *pdev)
> if (ret)
> goto rm_attr;
>
> - if (ci->is_otg)
> - hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
> + queue_delayed_work(ci->wq, &ci->dwork, msecs_to_jiffies(500));
CI_WAIT_VBUS_STABLE_TIMEOUT?
>
> return ret;
>
> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> new file mode 100644
> index 0000000..cc2864f
> --- /dev/null
> +++ b/drivers/usb/chipidea/otg.c
> @@ -0,0 +1,60 @@
> +/*
> + * otg.c - ChipIdea USB IP core OTG driver
> + *
> + * Copyright 2012 Freescale Semiconductor, Inc.
> + *
> + * Author: Peter Chen
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * 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/chipidea.h>
> +
> +#include "ci.h"
> +#include "udc.h"
> +#include "bits.h"
> +#include "host.h"
> +#include "debug.h"
> +
> +static int ci_otg_set_peripheral(struct usb_otg *otg,
> + struct usb_gadget *periph)
> +{
> + otg->gadget = periph;
> +
> + return 0;
> +}
> +
> +static int ci_otg_set_host(struct usb_otg *otg, struct usb_bus *host)
> +{
> + otg->host = host;
> +
> + return 0;
> +}
> +
> +/**
> + * ci_hdrc_otg_init - initialize device related bits
> + * ci: the controller
> + *
> + * This function create otg struct, if the device can switch between
> + * device and host.
> + */
> +int ci_hdrc_otg_init(struct ci13xxx *ci)
> +{
> + /* Useless at current */
> + ci->otg.set_peripheral = ci_otg_set_peripheral;
> + ci->otg.set_host = ci_otg_set_host;
> + ci->transceiver->otg = &ci->otg;
This will blow up if we don't have a transceiver.
> + hw_write(ci, OP_OTGSC, OTGSC_IDIE, OTGSC_IDIE);
> +
> + return 0;
> +}
> diff --git a/drivers/usb/chipidea/otg.h b/drivers/usb/chipidea/otg.h
> new file mode 100644
> index 0000000..b4c6b3e
> --- /dev/null
> +++ b/drivers/usb/chipidea/otg.h
> @@ -0,0 +1,6 @@
> +#ifndef __DRIVERS_USB_CHIPIDEA_OTG_H
> +#define __DRIVERS_USB_CHIPIDEA_OTG_H
> +
> +int ci_hdrc_otg_init(struct ci13xxx *ci);
> +
> +#endif /* __DRIVERS_USB_CHIPIDEA_OTG_H */
> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c
> index d214448..b52cb10 100644
> --- a/drivers/usb/chipidea/udc.c
> +++ b/drivers/usb/chipidea/udc.c
> @@ -1371,6 +1371,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 +1379,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");
> }
> }
>
> --
> 1.7.0.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
