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

Message ID 1441203864-15786-6-git-send-email-rogerq@ti.com
State New
Headers show

Commit Message

Roger Quadros Sept. 2, 2015, 2:24 p.m.
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@ti.com>
---
 drivers/usb/dwc3/core.c | 217 ++++++++++++++++++++++++++++++++++++++++++++----
 drivers/usb/dwc3/core.h |   3 +
 2 files changed, 205 insertions(+), 15 deletions(-)

Comments

Felipe Balbi Sept. 2, 2015, 2:43 p.m. | #1
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@ti.com>
> ---
>  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
>
Roger Quadros Sept. 3, 2015, 1:52 p.m. | #2
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 02/09/15 17:43, Felipe Balbi wrote:
> 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@ti.com>
>> ---
>>  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.

OK. I'll remove the if condition.

> 
>> +}
>> +
>> +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.

OK.

> 
>> +	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).

You're right. Will fix at both places.
> 
>> +
>> +	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);
> 

OK.

>> +
>> +		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

Did you mean that I'm ignoring all OTG bits (HNP/SRP/ADP)?

> 
>> @@ -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;
>>  }
>>  

- --
cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6FCCAAoJENJaa9O+djCTbaYQAME771phZpgr2Xtj1ejnPE8H
Bl84Sam/gWjy4+mqCUw+mQaCuF8M24ExVugHypQ0fF+9w6UMNrDrg+g+kZtQusCt
BTFkvS7g/6LJHEJowIoRZc5y/5bhnLa4Udcw5pYdhZHG7yIUsTs98WePROdOPk6z
i6OXA/wPC9ZJxeavew42HDmNj2IjJppU7bLDo+uMj/vz35dElq/B5w5mAXhshJ9A
R2IbxDevP4SiBYPfx1uFYKO5v9YVHnB3wk+3i3MjKuwO2CqfAVjzt9qWpM1iNThx
hOh+9vOenvttn7WHXP0scZAdBjmp3kKRAlfSELaAowy79X/3QRseZ75yJA8/tz+y
GT0x69fDQDxu0ffC961CY8p0a0F3ByVAqXBmsrCXPj0KxfutOkB8xE1BXY6+oUg/
ciqe0geXabmD9mu+3z8AXWOsFBnyFzsgSa2Dx5CRJ4/w5jhYOIg9/l8GGDlP6p1R
kHfiGYC2OzyxM4IgKYvc5p/VbAA4Ub5aQsWBdMYahAbs+l1xQ7zUEALf5S8c0KHK
k8jJo+oo+ghWzm6ikMfn96Ko/0vQuKG+uZZtzBDVp0uHBEW135GmZV5PCOh89M2k
yMuZQnbcTpEaANvWYJDkH3Can6Afcuki/i9kOK8bDib4Exo3IBijZNsLzpUzeS0m
vnsEqLL9IDRJR54ibQOC
=5n96
-----END PGP SIGNATURE-----
--
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
Felipe Balbi Sept. 3, 2015, 3:51 p.m. | #3
Hi,

On Thu, Sep 03, 2015 at 04:52:02PM +0300, Roger Quadros wrote:
> >>  	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
> 
> Did you mean that I'm ignoring all OTG bits (HNP/SRP/ADP)?

yes and no :-)  There's a rather complex flux chart which details how we
switch from host to peripheral and vice versa. We need to follow that to
the smallest details since that's what IP provider considers to be
correct. If we deviate from that we should have very strong reasons for
doing so and we also want big, fat, long comments in source code
detailing why and how we're deviating :-)
Roger Quadros Sept. 4, 2015, 9:13 a.m. | #4
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On 03/09/15 18:51, Felipe Balbi wrote:
> Hi,
> 
> On Thu, Sep 03, 2015 at 04:52:02PM +0300, Roger Quadros wrote:
>>>>  	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
>>
>> Did you mean that I'm ignoring all OTG bits (HNP/SRP/ADP)?
> 
> yes and no :-)  There's a rather complex flux chart which details how we
> switch from host to peripheral and vice versa. We need to follow that to
> the smallest details since that's what IP provider considers to be
> correct. If we deviate from that we should have very strong reasons for
> doing so and we also want big, fat, long comments in source code
> detailing why and how we're deviating :-)
> 
Understood. I'll update this accordingly.

- --
cheers,
- -roger
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQIcBAEBCAAGBQJV6WC4AAoJENJaa9O+djCTWSEP/jq0jX2KJVjqKeSk60wyDIEH
AIGXpaupz4XCFRsLFHlEomAoWHEzmbdwQlcI5gyB4N6IgF6xfxX71AZnytw4k4ja
sFxQq/qNSPWa4yiEY9MFbIPuiRMFcLOB5VbemGpcZq4n31yaKlbLtHMDEC8zPmdY
yfnUz7raY2tID/+wOASAM+nDXVS6nFztWXCrz+4TxwDaH0dgwl9OAoD7nvTil3MT
UTuR34GbWcb9rCxJVYgneht1sLbkKGCkwjLRcqzhOajg87GuW38irqD9OOF+pLCm
srhYsntQyBJlvArDQKMZIyGEYHvjICWzJUo/IPA8DFS/qOrRLyrLvVHskb6pr9/y
KZL/1N+5Nbh0N0XGWKwxzVh69DqmQPuY32SN2r1xqCDzzZAnvrl9cz6ixsRq1JGK
9EvYNJlRWsdypfA8tdhPtCgXua+wYSLNewgHQSko10qeAgiaIU5/bQ/9Eb2Ys9QV
6wkhr0Gulf/hnMKXHupErhUsvSBJk2MGaaDIfUSt3mWMTRsthp2NedRE9li9bWW9
gSXcTxs8785Y7FgxFDvvqQ05rbGWNqfxkeq54ziMtH3mZ93inE7me1HbqL1jgEuh
NFrNsAKYc000syvnqunzl8uZMRMsICaY1aKC0riQTdpgagHgGMfadKtWXb8OdcVa
gICj2/o8MpPpv3FEniDz
=L39b
-----END PGP SIGNATURE-----
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/
Peter Chen Sept. 6, 2015, 2:20 a.m. | #5
On Wed, Sep 02, 2015 at 09:43:38AM -0500, Felipe Balbi wrote:
> Hi,
> 
> > +
> > +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).
> 

Is it possible the kernel process is accessing the content you will 
access?
Roger Quadros Sept. 15, 2015, 2:46 p.m. | #6
On 06/09/15 05:20, Peter Chen wrote:
> On Wed, Sep 02, 2015 at 09:43:38AM -0500, Felipe Balbi wrote:
>> Hi,
>>
>>> +
>>> +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).
>>
> 
> Is it possible the kernel process is accessing the content you will 
> access?
> 
When kernel process accesses the data we'll never reach here
as we're protecting it with spinlock_irqsave(), spinunlock_irqrestore().

cheers,
-roger
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

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);
+	}
+}
+
+static irqreturn_t dwc3_otg_thread_irq(int irq, void *_dwc)
+{
+	struct dwc3 *dwc = _dwc;
+	unsigned long flags;
+	irqreturn_t ret = IRQ_NONE;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	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);
+
+	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);
+		}
+
+		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);
 	}
 
 	return 0;
@@ -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;