Re: [PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq

From: Felipe Balbi
Date: Wed Sep 02 2015 - 10:43:51 EST

Next message: Arnaldo Carvalho de Melo: "Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel"
Previous message: Dimitri Sivanich: "Re: [PATCH 1/4] drivers/misc/sgi-gru: remove unused variable"
In reply to: Roger Quadros: "[PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq"
Next in thread: Roger Quadros: "Re: [PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi,

On Wed, Sep 02, 2015 at 05:24:20PM +0300, Roger Quadros wrote:
> If the ID pin event is not available over extcon
> then we rely on the OTG controller to provide us ID and VBUS
> information.
>
> We still don't support any OTG features but just
> dual-role operation.
>
> Signed-off-by: Roger Quadros <rogerq@xxxxxx>
> ---
> drivers/usb/dwc3/core.c | 217 ++++++++++++++++++++++++++++++++++++++++++++----
> drivers/usb/dwc3/core.h | 3 +
> 2 files changed, 205 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 38b31df..632ee53 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -704,6 +704,63 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
> return 0;
> }
>
> +/* Get OTG events and sync it to OTG fsm */
> +static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
> +{
> + u32 reg;
> + int id, vbus;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_OSTS);
> + dev_dbg(dwc->dev, "otgstatus 0x%x\n", reg);
> +
> + id = !!(reg & DWC3_OSTS_CONIDSTS);
> + vbus = !!(reg & DWC3_OSTS_BSESVLD);
> +
> + if (id != dwc->fsm->id || vbus != dwc->fsm->b_sess_vld) {
> + dev_dbg(dwc->dev, "id %d vbus %d\n", id, vbus);
> + dwc->fsm->id = id;
> + dwc->fsm->b_sess_vld = vbus;
> + usb_otg_sync_inputs(dwc->fsm);
> + }

this guy shouldn't try to filter events here. That's what the FSM should
be doing.

> +}
> +
> +static irqreturn_t dwc3_otg_thread_irq(int irq, void *_dwc)
> +{
> + struct dwc3 *dwc = _dwc;
> + unsigned long flags;
> + irqreturn_t ret = IRQ_NONE;

this IRQ will be disabled pretty quickly. You *always* return IRQ_NONE

> + spin_lock_irqsave(&dwc->lock, flags);

if you cache current OSTS in dwc3, you can use that instead and change
this to a spin_lock() instead of disabling IRQs here. This device's IRQs
are already masked anyway.

> + dwc3_otg_fsm_sync(dwc);
> + /* unmask interrupts */
> + dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
> + spin_unlock_irqrestore(&dwc->lock, flags);
> +
> + return ret;
> +}
> +
> +static irqreturn_t dwc3_otg_irq(int irq, void *_dwc)
> +{
> + struct dwc3 *dwc = _dwc;
> + irqreturn_t ret = IRQ_NONE;
> + u32 reg;
> +
> + spin_lock(&dwc->lock);

this seems unnecessary, we're already in hardirq with IRQs disabled.
What sort of race could we have ? (in fact, this also needs change in
dwc3/gadget.c).

> +
> + reg = dwc3_readl(dwc->regs, DWC3_OEVT);
> + if (reg) {
> + dwc3_writel(dwc->regs, DWC3_OEVT, reg);
> + /* mask interrupts till processed */
> + dwc->oevten = dwc3_readl(dwc->regs, DWC3_OEVTEN);
> + dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
> + ret = IRQ_WAKE_THREAD;
> + }
> +
> + spin_unlock(&dwc->lock);
> +
> + return ret;
> +}
> +
> /* --------------------- Dual-Role management ------------------------------- */
>
> static void dwc3_drd_fsm_sync(struct dwc3 *dwc)
> @@ -728,15 +785,44 @@ static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
> {
> struct device *dev = usb_otg_fsm_to_dev(fsm);
> struct dwc3 *dwc = dev_get_drvdata(dev);
> + u32 reg;
>
> dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
> + if (dwc->edev) {
> + if (on) {
> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> + /* start the HCD */
> + usb_otg_start_host(fsm, true);
> + } else {
> + /* stop the HCD */
> + usb_otg_start_host(fsm, false);
> + }

if (on)
dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
usb_otg_start_host(fsm, on);

> +
> + return 0;
> + }
> +
> + /* switch OTG core */
> if (on) {
> - dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
> + /* OCTL.PeriMode = 0 */
> + reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> + reg &= ~DWC3_OCTL_PERIMODE;
> + dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> + /* unconditionally turn on VBUS */
> + reg |= DWC3_OCTL_PRTPWRCTL;
> + dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> /* start the HCD */
> usb_otg_start_host(fsm, true);
> } else {
> /* stop the HCD */
> usb_otg_start_host(fsm, false);
> + /* turn off VBUS */
> + reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> + reg &= ~DWC3_OCTL_PRTPWRCTL;
> + dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> + /* OCTL.PeriMode = 1 */
> + reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> + reg |= DWC3_OCTL_PERIMODE;
> + dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> }

it looks like you're not really following the fluxchart from SNPS
documentation, see Figure 11-4 on section 11.1.4.5

> @@ -746,15 +832,48 @@ static int dwc3_drd_start_gadget(struct otg_fsm *fsm, int on)
> {
> struct device *dev = usb_otg_fsm_to_dev(fsm);
> struct dwc3 *dwc = dev_get_drvdata(dev);
> + u32 reg;
>
> dev_dbg(dwc->dev, "%s: %d\n", __func__, on);
> - if (on) {
> - dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> + if (on)
> dwc3_event_buffers_setup(dwc);
>
> + if (dwc->edev) {
> + if (on) {
> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
> + usb_otg_start_gadget(fsm, true);
> + } else {
> + usb_otg_start_gadget(fsm, false);
> + }
> +
> + return 0;
> + }
> +
> + /* switch OTG core */
> + if (on) {
> + /* OCTL.PeriMode = 1 */
> + reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> + reg |= DWC3_OCTL_PERIMODE;
> + dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> + /* GUSB2PHYCFG0.SusPHY = 1 */
> + if (!dwc->dis_u2_susphy_quirk) {
> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> + reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> + }
> /* start the UDC */
> usb_otg_start_gadget(fsm, true);
> } else {
> + /* GUSB2PHYCFG0.SusPHY=0 */
> + if (!dwc->dis_u2_susphy_quirk) {
> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> + reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> + }
> + /* OCTL.PeriMode = 1 */
> + reg = dwc3_readl(dwc->regs, DWC3_OCTL);
> + reg |= DWC3_OCTL_PERIMODE;
> + dwc3_writel(dwc->regs, DWC3_OCTL, reg);
> /* stop the UDC */
> usb_otg_start_gadget(fsm, false);
> }
> @@ -777,10 +896,30 @@ static int dwc3_drd_notifier(struct notifier_block *nb,
> return NOTIFY_DONE;
> }
>
> +static int dwc3_drd_register(struct dwc3 *dwc)
> +{
> + int ret;
> +
> + /* register parent as DRD device with OTG core */
> + dwc->fsm = usb_otg_register(dwc->dev, &dwc->otg_config);
> + if (IS_ERR(dwc->fsm)) {
> + ret = PTR_ERR(dwc->fsm);
> + if (ret == -ENOTSUPP)
> + dev_err(dwc->dev, "CONFIG_USB_OTG needed for dual-role\n");
> + else
> + dev_err(dwc->dev, "Failed to register with OTG core\n");
> +
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> static int dwc3_drd_init(struct dwc3 *dwc)
> {
> int ret, id, vbus;
> struct usb_otg_caps *otgcaps = &dwc->otg_config.otg_caps;
> + u32 reg;
>
> otgcaps->otg_rev = 0;
> otgcaps->hnp_support = false;
> @@ -788,9 +927,10 @@ static int dwc3_drd_init(struct dwc3 *dwc)
> otgcaps->adp_support = false;
> dwc->otg_config.fsm_ops = &dwc3_drd_ops;
>
> + /* If extcon device is not present we rely on OTG core for ID event */
> if (!dwc->edev) {
> - dev_err(dwc->dev, "No extcon device found for OTG mode\n");
> - return -ENODEV;
> + dev_dbg(dwc->dev, "No extcon device found for OTG mode\n");
> + goto try_otg_core;
> }
>
> dwc->otg_nb.notifier_call = dwc3_drd_notifier;
> @@ -818,17 +958,9 @@ static int dwc3_drd_init(struct dwc3 *dwc)
> goto fail;
> }
>
> - /* register as DRD device with OTG core */
> - dwc->fsm = usb_otg_register(dwc->dev, &dwc->otg_config);
> - if (IS_ERR(dwc->fsm)) {
> - ret = PTR_ERR(dwc->fsm);
> - if (ret == -ENOTSUPP)
> - dev_err(dwc->dev, "CONFIG_USB_OTG needed for dual-role\n");
> - else
> - dev_err(dwc->dev, "Failed to register with OTG core\n");
> -
> + ret = dwc3_drd_register(dwc);
> + if (ret)
> goto fail;
> - }
>
> dwc3_drd_fsm_sync(dwc);
>
> @@ -839,6 +971,61 @@ extcon_fail:
> extcon_unregister_notifier(dwc->edev, EXTCON_USB, &dwc->otg_nb);
>
> return ret;
> +
> +try_otg_core:
> + ret = dwc3_drd_register(dwc);
> + if (ret)
> + return ret;
> +
> + /* disable all irqs */
> + dwc3_writel(dwc->regs, DWC3_OEVTEN, 0);
> + /* clear all events */
> + dwc3_writel(dwc->regs, DWC3_OEVT, ~0);
> +
> + ret = request_threaded_irq(dwc->otg_irq, dwc3_otg_irq,
> + dwc3_otg_thread_irq,
> + IRQF_SHARED, "dwc3-otg", dwc);
> + if (ret) {
> + dev_err(dwc->dev, "failed to request irq #%d --> %d\n",
> + dwc->otg_irq, ret);
> + ret = -ENODEV;
> + goto error;
> + }
> +
> + /* we need to set OTG to get events from OTG core */
> + dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
> + /* GUSB2PHYCFG0.SusPHY=0 */
> + if (!dwc->dis_u2_susphy_quirk) {
> + reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> + reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
> + dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> + }
> +
> + /* Initialize OTG registers */
> + /*
> + * Prevent host/device reset from resetting OTG core.
> + * If we don't do this then xhci_reset (USBCMD.HCRST) will reset
> + * the signal outputs sent to the PHY, the OTG FSM logic of the
> + * core and also the resets to the VBUS filters inside the core.
> + */
> + reg = DWC3_OCFG_SFTRSTMASK;
> + dwc3_writel(dwc->regs, DWC3_OCFG, reg);
> + /* Enable ID event interrupt */
> + dwc3_writel(dwc->regs, DWC3_OEVTEN, DWC3_OEVTEN_CONIDSTSCHNGEN |
> + DWC3_OEVTEN_BDEVVBUSCHNGE |
> + DWC3_OEVTEN_BDEVSESSVLDDETEN);
> + /* OCTL.PeriMode = 1 */
> + dwc3_writel(dwc->regs, DWC3_OCTL, DWC3_OCTL_PERIMODE);
> +
> + dwc3_otg_fsm_sync(dwc);
> + usb_otg_sync_inputs(dwc->fsm);
> +
> + return 0;
> +
> +error:
> + usb_otg_unregister(dwc->dev);
> +
> + return ret;
> }
>
> static void dwc3_drd_exit(struct dwc3 *dwc)
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index bd32cb2..129ef37 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -736,6 +736,7 @@ struct dwc3_scratchpad_array {
> * @gadget_driver: pointer to the gadget driver
> * @regs: base address for our registers
> * @regs_size: address space size
> + * @oevten: otg interrupt enable mask copy
> * @nr_scratch: number of scratch buffers
> * @num_event_buffers: calculated number of event buffers
> * @u1u2: only used on revisions <1.83a for workaround
> @@ -858,6 +859,8 @@ struct dwc3 {
> u32 dcfg;
> u32 gctl;
>
> + u32 oevten;
> +
> u32 nr_scratch;
> u32 num_event_buffers;
> u32 u1u2;
> --
> 2.1.4
>

--
balbi

Attachment: signature.asc
Description: Digital signature

Next message: Arnaldo Carvalho de Melo: "Re: [PATCH] perf tools: Don't write to evsel if parser doesn't collect evsel"
Previous message: Dimitri Sivanich: "Re: [PATCH 1/4] drivers/misc/sgi-gru: remove unused variable"
In reply to: Roger Quadros: "[PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq"
Next in thread: Roger Quadros: "Re: [PATCH v4 5/9] usb: dwc3: core: make dual-role work with OTG irq"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]