Re: [PATCH v2 3/4] usb: dwc3: add dual-role support

[John Youn]

On 03/30/2017 02:27 AM, Felipe Balbi wrote:
>
> Hi
>
> Roger Quadros  writes:
> For something that simple, we wouldn't even need to use OTG FSM layer
> because that brings no benefit for such a simple requirement.

 no no. I think you got it wrong. I'm not using the OTG FSM layer at all :).
>>>
>>> what are all the otg_fsm mentions then? Also you have:
>>>
>>>  +  struct usb_otg  otg;
>>>  +  struct otg_fsm  otg_fsm;
>>>
>>
>> I'll get rid of them. They aren't really needed.
>> Now I see why you thought I was using the otg_fsm layer. :)
>
> fair enough
>
> Can you either confirm or refute the statement above?

 The real problem was that if host adapter was removed during a system 
 suspend
 then while resuming XHCI was locking up like below. This is probably 
 because
 we're trying to remove/Halt the HCD in the otg_irq_handler before XHCI 
 driver has resumed?

 How can we ensure that we call dwc3_host_exit() only *after* XHCI driver 
 has resumed?
>>>
>>> Well, xHCI is our child, so driver model should *already* be
>>> guaranteeing that, no? Specially since you're calling this from
>>> ->complete() which happens after ->resume() has been called for the
>>> entire tree. It's true, however, that dwc3's ->complete() will be called
>>> before xhci's ->complete(). Is this the problem you're pointing at? Or
>>> do you mean xHCI is runtime suspended (or runtime resuming) while you
>>> call dwc3_host_exit()? If that's the case, then there's a bug in xHCI
>>> itself.
>>
>> Yes dwc3->complete() being called before xhci->complete(), and so HCD being
>> removed before xhci->complete() causes the lockup.
>>
>> It could be a bug in xHCI driver as well.
>
> I see...
>
 We need a way to mask the OTG events without loosing them while they are 
 masked.
 Do you know how that could be achieved?
>>>
>>> masking doesn't clear events. It just masks them. Look at gadget.c for
>>> how we do it. Basically, the code we have here is racy, really racy and
>>> will cause hard-to-debug lockups. Your code can only work with
>>> IRQF_ONESHOT, which we don't want to add back.
>>>
>>> In any case, to mask events, you set BIT 31 in GEVNTSIZ register. Just
>>> copy it from gadget.c ;-)
>>
>> Isn't GEVNTSIZ specific to device side? We're talking about the OTG block 
>> here.
>
> that's true, sorry.
>
>> Are you sure that setting bit 31 of GEVNTSIZ will mask OTG_irq? I don't 
>> think so.
>>
>> Note that OTG_IRQ is a separate IRQ line than PERIPHERAL_IRQ.
>
> man, there's really no way to mask OTG events. This can be bad :-)
>
> John, can you confirm if there's really no way of masking OTG events
> without loosing them?

Not sure. I'll have to verify.

If the IP version is 3.00a or higher you could experiment with using
interrupt moderation as a global mask.

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

Re: [PATCH v2 3/4] usb: dwc3: add dual-role support

[Felipe Balbi]

Hi

Roger Quadros  writes:
 For something that simple, we wouldn't even need to use OTG FSM layer
 because that brings no benefit for such a simple requirement.
>>>
>>> no no. I think you got it wrong. I'm not using the OTG FSM layer at all :).
>> 
>> what are all the otg_fsm mentions then? Also you have:
>> 
>>  +   struct usb_otg  otg;
>>  +   struct otg_fsm  otg_fsm;
>> 
>
> I'll get rid of them. They aren't really needed.
> Now I see why you thought I was using the otg_fsm layer. :)

fair enough

 Can you either confirm or refute the statement above?
>>>
>>> The real problem was that if host adapter was removed during a system 
>>> suspend
>>> then while resuming XHCI was locking up like below. This is probably because
>>> we're trying to remove/Halt the HCD in the otg_irq_handler before XHCI 
>>> driver has resumed?
>>>
>>> How can we ensure that we call dwc3_host_exit() only *after* XHCI driver 
>>> has resumed?
>> 
>> Well, xHCI is our child, so driver model should *already* be
>> guaranteeing that, no? Specially since you're calling this from
>> ->complete() which happens after ->resume() has been called for the
>> entire tree. It's true, however, that dwc3's ->complete() will be called
>> before xhci's ->complete(). Is this the problem you're pointing at? Or
>> do you mean xHCI is runtime suspended (or runtime resuming) while you
>> call dwc3_host_exit()? If that's the case, then there's a bug in xHCI
>> itself.
>
> Yes dwc3->complete() being called before xhci->complete(), and so HCD being
> removed before xhci->complete() causes the lockup.
>
> It could be a bug in xHCI driver as well.

I see...

>>> We need a way to mask the OTG events without loosing them while they are 
>>> masked.
>>> Do you know how that could be achieved?
>> 
>> masking doesn't clear events. It just masks them. Look at gadget.c for
>> how we do it. Basically, the code we have here is racy, really racy and
>> will cause hard-to-debug lockups. Your code can only work with
>> IRQF_ONESHOT, which we don't want to add back.
>> 
>> In any case, to mask events, you set BIT 31 in GEVNTSIZ register. Just
>> copy it from gadget.c ;-)
>
> Isn't GEVNTSIZ specific to device side? We're talking about the OTG block 
> here.

that's true, sorry.

> Are you sure that setting bit 31 of GEVNTSIZ will mask OTG_irq? I don't think 
> so.
>
> Note that OTG_IRQ is a separate IRQ line than PERIPHERAL_IRQ.

man, there's really no way to mask OTG events. This can be bad :-)

John, can you confirm if there's really no way of masking OTG events
without loosing them?

> + /* OEVTEN = 0 */
> + dwc3_otg_disable_events(dwc, DWC3_OTG_ALL_EVENTS);
> + /* OEVTEN.ConIDStsChngEn = 1. Instead we enable all events */
> + dwc3_otg_enable_events(dwc, DWC3_OTG_ALL_EVENTS);

 oh, disable everything and enable everything right after. What gives?
>>>
>>> I did this following Fig 11.4. But there there don't enable all events,
>>> so it was a good idea to be on a clean slate by disabling all events first
>>> and then only enabling selected events.
>>>
>>> In any case I think it is good practice. i.e. clear before OR operation?
>>> FYI. dwc3_otg_enable_events doesn't clear the events that are not enabled so
>>> if some old event not part of enable_mask was enabled it will stay enabled.
>> 
>> can't this result in IRQ triggering forever and us not handling it? ;-)
>
> Why should enabling events trigger IRQ? IRQ will trigger only when the
> event actually happens no?

heh, right :-) What I mean is that you might enable an interrupt event
which you don't clear, because you don't support it or don't handle it,
or whatever.

Reserved bits might become non-reserved in the future and so on.

> + /* start the xHCI host driver */
> + if (!skip) {

 when would skip be true?

>>>
>>> only during system resume.
>> 
>> hmmm, is there a reason for that? I mean, could we live without it for
>> the time being? Seems like all this achieves is avoiding reenumeration
>> of some devices during resume. Do we care from a starting
>> implementation?
>
> At least on AM43x, it was required. without that USB devices plugged in
> before a system suspend were lost after resume.
>
> I agree on dropping this for now and adding it later.

looks like we have another problem which needs to be investigated ;-)

> + dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> + /* start the Peripheral driver  */
> + if (dwc->gadget_driver) {
> + __dwc3_gadget_start(dwc);
> + if (dwc->gadget_pullup)
> + dwc3_gadget_run_stop(dwc, true, false);

 why don't you add/remove the UDC just like you do for the HCD? (you
 wouldn't add/remove a device, but rather call
 usb_del_gadget_udc()/usb_add_gadget_udc() directly. Would that clean up
 some of this?
>>>
>>> It causes more problems than solving 

Re: [PATCH v2 3/4] usb: dwc3: add dual-role support

[Roger Quadros]

Hi,

On 29/03/17 16:15, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros  writes:
 @@ -839,6 +841,505 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
return 0;
  }
  
 +static int dwc3_drd_start_host(struct dwc3 *dwc, int on, bool skip);
 +static int dwc3_drd_start_gadget(struct dwc3 *dwc, int on);
 +
 +/* dwc->lock must be held */
 +static void dwc3_drd_statemachine(struct dwc3 *dwc, int id, int vbus)
 +{
 +  enum usb_otg_state new_state;
 +  int protocol;
 +
 +  if (id == dwc->otg_fsm.id && vbus == dwc->otg_fsm.b_sess_vld)
 +  return;
 +
 +  dwc->otg_fsm.id = id;
 +  dwc->otg_fsm.b_sess_vld = vbus;
 +
 +  if (!id) {
 +  new_state = OTG_STATE_A_HOST;
 +  } else{
 +  if (vbus)
 +  new_state = OTG_STATE_B_PERIPHERAL;
 +  else
 +  new_state = OTG_STATE_B_IDLE;
 +  }
 +
 +  if (dwc->otg.state == new_state)
 +  return;
 +
 +  protocol = dwc->otg_fsm.protocol;
 +  switch (new_state) {
 +  case OTG_STATE_B_IDLE:
 +  if (protocol == PROTO_GADGET)
 +  dwc3_drd_start_gadget(dwc, 0);
 +  else if (protocol == PROTO_HOST)
 +  dwc3_drd_start_host(dwc, 0, 0);
 +  dwc->otg_fsm.protocol = PROTO_UNDEF;
 +  break;
 +  case OTG_STATE_B_PERIPHERAL:
 +  if (protocol == PROTO_HOST)
 +  dwc3_drd_start_host(dwc, 0, 0);
 +
 +  if (protocol != PROTO_GADGET) {
 +  dwc->otg_fsm.protocol = PROTO_GADGET;
 +  dwc3_drd_start_gadget(dwc, 1);
 +  }
 +  break;
 +  case OTG_STATE_A_HOST:
 +  if (protocol == PROTO_GADGET)
 +  dwc3_drd_start_gadget(dwc, 0);
 +
 +  if (protocol != PROTO_HOST) {
 +  dwc->otg_fsm.protocol = PROTO_HOST;
 +  dwc3_drd_start_host(dwc, 1, 0);
 +  }
 +  break;
 +  default:
 +  dev_err(dwc->dev, "drd: invalid usb-drd state: %s\n",
 +  usb_otg_state_string(new_state));
 +  return;
 +  }
 +
 +  dwc->otg.state = new_state;
 +}
>>>
>>> I think I've mentioned this before. Why don't we start with the simplest
>>> possible implementation? Something that *just* allows us to get ID pin
>>> value and set the mode. After that's stable, then we add more pieces to
>>> the mix.
>>
>> That is exactly what I'm doing. Maybe the switch case is making it look 
>> complicated.
>> dwc3_drd_statemachine() has only 2 inputs VBUS and ID.
>>
>> I can change it to if/else if you prefer that. I like the way it is cause
>> we can define 3 states IDLE, PERIPHERAL and HOST.
> 
> Right, I like the three states, but somehow the code looks really
> complex :-s
> 
>>> For something that simple, we wouldn't even need to use OTG FSM layer
>>> because that brings no benefit for such a simple requirement.
>>
>> no no. I think you got it wrong. I'm not using the OTG FSM layer at all :).
> 
> what are all the otg_fsm mentions then? Also you have:
> 
>  +struct usb_otg  otg;
>  +struct otg_fsm  otg_fsm;
> 

I'll get rid of them. They aren't really needed.
Now I see why you thought I was using the otg_fsm layer. :)

 +/* dwc->lock must be held */
 +static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
 +{
 +  u32 reg;
 +  int id, vbus;
 +
 +  /*
 +   * calling dwc3_otg_fsm_sync() during resume breaks host
 +   * if adapter was removed during suspend as xhci driver
 +   * is not prepared to see hcd removal before xhci_resume.
 +   */
 +  if (dwc->otg_prevent_sync)
 +  return;
 +
 +  reg = dwc3_readl(dwc->regs, DWC3_OSTS);
 +  id = !!(reg & DWC3_OSTS_CONIDSTS);
 +  vbus = !!(reg & DWC3_OSTS_BSESVLD);
 +  dwc3_drd_statemachine(dwc, id, vbus);
 +}
>>>
>>> Just consider this for a moment. Consider the steps taken to get here.
>>>
>>> - User plugs cable
>>> - Hardirq handler run
>>> - read register
>>> - if (reg) return IRQ_WAKE_THREAD;
>>> - schedule bottom half handler to sometime in the future
>>> - scheduler runs our threaded handler
>>> - lock dwc3
>>> - if (host)
>>> - configure register
>>> - add xHCI device
>>> - else
>>> - otg_fsm_sync()
>>> - drd_statemachine()
>>> - configure registers
>>> - reimplement gadget initialization (same thing we do
>>> when registering UDC
>>>
>>> I mean, just looking at this we can already see that it's really overly
>>> complex. Right now we need simple, dead simple. This will limit the
>>> possibility of errors.
>>
>> OK. I can probably get rid of the state 

Re: [PATCH v2 3/4] usb: dwc3: add dual-role support

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>>> @@ -839,6 +841,505 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>>> return 0;
>>>  }
>>>  
>>> +static int dwc3_drd_start_host(struct dwc3 *dwc, int on, bool skip);
>>> +static int dwc3_drd_start_gadget(struct dwc3 *dwc, int on);
>>> +
>>> +/* dwc->lock must be held */
>>> +static void dwc3_drd_statemachine(struct dwc3 *dwc, int id, int vbus)
>>> +{
>>> +   enum usb_otg_state new_state;
>>> +   int protocol;
>>> +
>>> +   if (id == dwc->otg_fsm.id && vbus == dwc->otg_fsm.b_sess_vld)
>>> +   return;
>>> +
>>> +   dwc->otg_fsm.id = id;
>>> +   dwc->otg_fsm.b_sess_vld = vbus;
>>> +
>>> +   if (!id) {
>>> +   new_state = OTG_STATE_A_HOST;
>>> +   } else{
>>> +   if (vbus)
>>> +   new_state = OTG_STATE_B_PERIPHERAL;
>>> +   else
>>> +   new_state = OTG_STATE_B_IDLE;
>>> +   }
>>> +
>>> +   if (dwc->otg.state == new_state)
>>> +   return;
>>> +
>>> +   protocol = dwc->otg_fsm.protocol;
>>> +   switch (new_state) {
>>> +   case OTG_STATE_B_IDLE:
>>> +   if (protocol == PROTO_GADGET)
>>> +   dwc3_drd_start_gadget(dwc, 0);
>>> +   else if (protocol == PROTO_HOST)
>>> +   dwc3_drd_start_host(dwc, 0, 0);
>>> +   dwc->otg_fsm.protocol = PROTO_UNDEF;
>>> +   break;
>>> +   case OTG_STATE_B_PERIPHERAL:
>>> +   if (protocol == PROTO_HOST)
>>> +   dwc3_drd_start_host(dwc, 0, 0);
>>> +
>>> +   if (protocol != PROTO_GADGET) {
>>> +   dwc->otg_fsm.protocol = PROTO_GADGET;
>>> +   dwc3_drd_start_gadget(dwc, 1);
>>> +   }
>>> +   break;
>>> +   case OTG_STATE_A_HOST:
>>> +   if (protocol == PROTO_GADGET)
>>> +   dwc3_drd_start_gadget(dwc, 0);
>>> +
>>> +   if (protocol != PROTO_HOST) {
>>> +   dwc->otg_fsm.protocol = PROTO_HOST;
>>> +   dwc3_drd_start_host(dwc, 1, 0);
>>> +   }
>>> +   break;
>>> +   default:
>>> +   dev_err(dwc->dev, "drd: invalid usb-drd state: %s\n",
>>> +   usb_otg_state_string(new_state));
>>> +   return;
>>> +   }
>>> +
>>> +   dwc->otg.state = new_state;
>>> +}
>> 
>> I think I've mentioned this before. Why don't we start with the simplest
>> possible implementation? Something that *just* allows us to get ID pin
>> value and set the mode. After that's stable, then we add more pieces to
>> the mix.
>
> That is exactly what I'm doing. Maybe the switch case is making it look 
> complicated.
> dwc3_drd_statemachine() has only 2 inputs VBUS and ID.
>
> I can change it to if/else if you prefer that. I like the way it is cause
> we can define 3 states IDLE, PERIPHERAL and HOST.

Right, I like the three states, but somehow the code looks really
complex :-s

>> For something that simple, we wouldn't even need to use OTG FSM layer
>> because that brings no benefit for such a simple requirement.
>
> no no. I think you got it wrong. I'm not using the OTG FSM layer at all :).

what are all the otg_fsm mentions then? Also you have:

 +  struct usb_otg  otg;
 +  struct otg_fsm  otg_fsm;

>>> +/* dwc->lock must be held */
>>> +static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
>>> +{
>>> +   u32 reg;
>>> +   int id, vbus;
>>> +
>>> +   /*
>>> +* calling dwc3_otg_fsm_sync() during resume breaks host
>>> +* if adapter was removed during suspend as xhci driver
>>> +* is not prepared to see hcd removal before xhci_resume.
>>> +*/
>>> +   if (dwc->otg_prevent_sync)
>>> +   return;
>>> +
>>> +   reg = dwc3_readl(dwc->regs, DWC3_OSTS);
>>> +   id = !!(reg & DWC3_OSTS_CONIDSTS);
>>> +   vbus = !!(reg & DWC3_OSTS_BSESVLD);
>>> +   dwc3_drd_statemachine(dwc, id, vbus);
>>> +}
>> 
>> Just consider this for a moment. Consider the steps taken to get here.
>> 
>>  - User plugs cable
>> - Hardirq handler run
>>  - read register
>> - if (reg) return IRQ_WAKE_THREAD;
>> - schedule bottom half handler to sometime in the future
>> - scheduler runs our threaded handler
>> - lock dwc3
>> - if (host)
>>  - configure register
>> - add xHCI device
>>  - else
>>  - otg_fsm_sync()
>> - drd_statemachine()
>>  - configure registers
>> - reimplement gadget initialization (same thing we do
>>  when registering UDC
>> 
>> I mean, just looking at this we can already see that it's really overly
>> complex. Right now we need simple, dead simple. This will limit the
>> possibility of errors.
>
> OK. I can probably get rid of the state machine model and just use if/else?
> Anything else you want me to get rid of?

The workqueue, unless it's really, really necessary and it appears like
it shouldn't be.

We _can_ keep the switch statement. The problem is not the switch

Re: [PATCH v2 3/4] usb: dwc3: add dual-role support

[Roger Quadros]

Hi,

On 28/03/17 14:07, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros  writes:
>> If dr_mode is "otg" then support dual role mode of operation.
>>
>> Get ID and VBUS information from the OTG controller
>> and put the controller in the appropriate state.
>>
>> This is our dual-role state table.
>>
>> ID   VBUSdual-role state
>> --   ---
>> 0x   A_HOST - Host controller active
>> 10   B_IDLE - Both Host and Gadget controllers inactive
>> 11   B_PERIPHERAL - Gadget controller active
>>
>> Calling dwc3_otg_fsm_sync() directly from dwc3_complete() can
>> lock up the system at xHCI add/remove so we use a work queue for it.
>>
>> Signed-off-by: Roger Quadros 
> 
> it still seems to me that you're adding too much code for something that
> should be darn simple. Please, read on.

Sure, I'm all ears for simplification :).

> 
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 369bab1..619fa7c 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -27,6 +27,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
> 
> as a cosmetic change, it would be nicer to have a drd.c or otg.c which
> exposes dwc3_otg_start()/stop() like we do for gadget.c and host.c

Agreed.
> 
>> @@ -107,6 +108,7 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>>  reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>>  reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>>  reg |= DWC3_GCTL_PRTCAPDIR(mode);
>> +dwc->current_mode = mode;
>>  dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>>  }
>>  
>> @@ -839,6 +841,505 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>>  return 0;
>>  }
>>  
>> +static int dwc3_drd_start_host(struct dwc3 *dwc, int on, bool skip);
>> +static int dwc3_drd_start_gadget(struct dwc3 *dwc, int on);
>> +
>> +/* dwc->lock must be held */
>> +static void dwc3_drd_statemachine(struct dwc3 *dwc, int id, int vbus)
>> +{
>> +enum usb_otg_state new_state;
>> +int protocol;
>> +
>> +if (id == dwc->otg_fsm.id && vbus == dwc->otg_fsm.b_sess_vld)
>> +return;
>> +
>> +dwc->otg_fsm.id = id;
>> +dwc->otg_fsm.b_sess_vld = vbus;
>> +
>> +if (!id) {
>> +new_state = OTG_STATE_A_HOST;
>> +} else{
>> +if (vbus)
>> +new_state = OTG_STATE_B_PERIPHERAL;
>> +else
>> +new_state = OTG_STATE_B_IDLE;
>> +}
>> +
>> +if (dwc->otg.state == new_state)
>> +return;
>> +
>> +protocol = dwc->otg_fsm.protocol;
>> +switch (new_state) {
>> +case OTG_STATE_B_IDLE:
>> +if (protocol == PROTO_GADGET)
>> +dwc3_drd_start_gadget(dwc, 0);
>> +else if (protocol == PROTO_HOST)
>> +dwc3_drd_start_host(dwc, 0, 0);
>> +dwc->otg_fsm.protocol = PROTO_UNDEF;
>> +break;
>> +case OTG_STATE_B_PERIPHERAL:
>> +if (protocol == PROTO_HOST)
>> +dwc3_drd_start_host(dwc, 0, 0);
>> +
>> +if (protocol != PROTO_GADGET) {
>> +dwc->otg_fsm.protocol = PROTO_GADGET;
>> +dwc3_drd_start_gadget(dwc, 1);
>> +}
>> +break;
>> +case OTG_STATE_A_HOST:
>> +if (protocol == PROTO_GADGET)
>> +dwc3_drd_start_gadget(dwc, 0);
>> +
>> +if (protocol != PROTO_HOST) {
>> +dwc->otg_fsm.protocol = PROTO_HOST;
>> +dwc3_drd_start_host(dwc, 1, 0);
>> +}
>> +break;
>> +default:
>> +dev_err(dwc->dev, "drd: invalid usb-drd state: %s\n",
>> +usb_otg_state_string(new_state));
>> +return;
>> +}
>> +
>> +dwc->otg.state = new_state;
>> +}
> 
> I think I've mentioned this before. Why don't we start with the simplest
> possible implementation? Something that *just* allows us to get ID pin
> value and set the mode. After that's stable, then we add more pieces to
> the mix.

That is exactly what I'm doing. Maybe the switch case is making it look 
complicated.
dwc3_drd_statemachine() has only 2 inputs VBUS and ID.

I can change it to if/else if you prefer that. I like the way it is cause
we can define 3 states IDLE, PERIPHERAL and HOST.

> 
> For something that simple, we wouldn't even need to use OTG FSM layer
> because that brings no benefit for such a simple requirement.

no no. I think you got it wrong. I'm not using the OTG FSM layer at all :).

> 
> Once there's a *real* need for OTG FSM, then we can add support for it,
> but then we add support to something we know to be working.
> 
>> +/* dwc->lock must be held */
>> +static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
>> +{
>> +u32 reg;
>> +int id, vbus;
>> +
>> +/*
>> + * calling dwc3_otg_fsm_sync() during resume breaks host
>> + * if adapter was removed during suspend as xhci driver
>> + * is not 

Re: [PATCH v2 3/4] usb: dwc3: add dual-role support

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> If dr_mode is "otg" then support dual role mode of operation.
>
> Get ID and VBUS information from the OTG controller
> and put the controller in the appropriate state.
>
> This is our dual-role state table.
>
> IDVBUSdual-role state
> -----
> 0 x   A_HOST - Host controller active
> 1 0   B_IDLE - Both Host and Gadget controllers inactive
> 1 1   B_PERIPHERAL - Gadget controller active
>
> Calling dwc3_otg_fsm_sync() directly from dwc3_complete() can
> lock up the system at xHCI add/remove so we use a work queue for it.
>
> Signed-off-by: Roger Quadros 

it still seems to me that you're adding too much code for something that
should be darn simple. Please, read on.

> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 369bab1..619fa7c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -27,6 +27,7 @@
>  #include 
>  #include 
>  #include 
> +#include 

as a cosmetic change, it would be nicer to have a drd.c or otg.c which
exposes dwc3_otg_start()/stop() like we do for gadget.c and host.c

> @@ -107,6 +108,7 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
>   reg = dwc3_readl(dwc->regs, DWC3_GCTL);
>   reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
>   reg |= DWC3_GCTL_PRTCAPDIR(mode);
> + dwc->current_mode = mode;
>   dwc3_writel(dwc->regs, DWC3_GCTL, reg);
>  }
>  
> @@ -839,6 +841,505 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>   return 0;
>  }
>  
> +static int dwc3_drd_start_host(struct dwc3 *dwc, int on, bool skip);
> +static int dwc3_drd_start_gadget(struct dwc3 *dwc, int on);
> +
> +/* dwc->lock must be held */
> +static void dwc3_drd_statemachine(struct dwc3 *dwc, int id, int vbus)
> +{
> + enum usb_otg_state new_state;
> + int protocol;
> +
> + if (id == dwc->otg_fsm.id && vbus == dwc->otg_fsm.b_sess_vld)
> + return;
> +
> + dwc->otg_fsm.id = id;
> + dwc->otg_fsm.b_sess_vld = vbus;
> +
> + if (!id) {
> + new_state = OTG_STATE_A_HOST;
> + } else{
> + if (vbus)
> + new_state = OTG_STATE_B_PERIPHERAL;
> + else
> + new_state = OTG_STATE_B_IDLE;
> + }
> +
> + if (dwc->otg.state == new_state)
> + return;
> +
> + protocol = dwc->otg_fsm.protocol;
> + switch (new_state) {
> + case OTG_STATE_B_IDLE:
> + if (protocol == PROTO_GADGET)
> + dwc3_drd_start_gadget(dwc, 0);
> + else if (protocol == PROTO_HOST)
> + dwc3_drd_start_host(dwc, 0, 0);
> + dwc->otg_fsm.protocol = PROTO_UNDEF;
> + break;
> + case OTG_STATE_B_PERIPHERAL:
> + if (protocol == PROTO_HOST)
> + dwc3_drd_start_host(dwc, 0, 0);
> +
> + if (protocol != PROTO_GADGET) {
> + dwc->otg_fsm.protocol = PROTO_GADGET;
> + dwc3_drd_start_gadget(dwc, 1);
> + }
> + break;
> + case OTG_STATE_A_HOST:
> + if (protocol == PROTO_GADGET)
> + dwc3_drd_start_gadget(dwc, 0);
> +
> + if (protocol != PROTO_HOST) {
> + dwc->otg_fsm.protocol = PROTO_HOST;
> + dwc3_drd_start_host(dwc, 1, 0);
> + }
> + break;
> + default:
> + dev_err(dwc->dev, "drd: invalid usb-drd state: %s\n",
> + usb_otg_state_string(new_state));
> + return;
> + }
> +
> + dwc->otg.state = new_state;
> +}

I think I've mentioned this before. Why don't we start with the simplest
possible implementation? Something that *just* allows us to get ID pin
value and set the mode. After that's stable, then we add more pieces to
the mix.

For something that simple, we wouldn't even need to use OTG FSM layer
because that brings no benefit for such a simple requirement.

Once there's a *real* need for OTG FSM, then we can add support for it,
but then we add support to something we know to be working.

> +/* dwc->lock must be held */
> +static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
> +{
> + u32 reg;
> + int id, vbus;
> +
> + /*
> +  * calling dwc3_otg_fsm_sync() during resume breaks host
> +  * if adapter was removed during suspend as xhci driver
> +  * is not prepared to see hcd removal before xhci_resume.
> +  */
> + if (dwc->otg_prevent_sync)
> + return;
> +
> + reg = dwc3_readl(dwc->regs, DWC3_OSTS);
> + id = !!(reg & DWC3_OSTS_CONIDSTS);
> + vbus = !!(reg & DWC3_OSTS_BSESVLD);
> + dwc3_drd_statemachine(dwc, id, vbus);
> +}

Just consider this for a moment. Consider the steps taken to get here.

- User plugs cable
- Hardirq handler run
- read register
- if (reg) return IRQ_WAKE_THREAD;
- schedule bottom half 

[PATCH v2 3/4] usb: dwc3: add dual-role support

[Roger Quadros]

If dr_mode is "otg" then support dual role mode of operation.

Get ID and VBUS information from the OTG controller
and put the controller in the appropriate state.

This is our dual-role state table.

ID  VBUSdual-role state
--  ---
0   x   A_HOST - Host controller active
1   0   B_IDLE - Both Host and Gadget controllers inactive
1   1   B_PERIPHERAL - Gadget controller active

Calling dwc3_otg_fsm_sync() directly from dwc3_complete() can
lock up the system at xHCI add/remove so we use a work queue for it.

Signed-off-by: Roger Quadros 
---
 drivers/usb/dwc3/core.c   | 595 --
 drivers/usb/dwc3/core.h   |  44 +++-
 drivers/usb/dwc3/gadget.c |  18 +-
 3 files changed, 633 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 369bab1..619fa7c 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -27,6 +27,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -107,6 +108,7 @@ void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
reg = dwc3_readl(dwc->regs, DWC3_GCTL);
reg &= ~(DWC3_GCTL_PRTCAPDIR(DWC3_GCTL_PRTCAP_OTG));
reg |= DWC3_GCTL_PRTCAPDIR(mode);
+   dwc->current_mode = mode;
dwc3_writel(dwc->regs, DWC3_GCTL, reg);
 }
 
@@ -839,6 +841,505 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
return 0;
 }
 
+static int dwc3_drd_start_host(struct dwc3 *dwc, int on, bool skip);
+static int dwc3_drd_start_gadget(struct dwc3 *dwc, int on);
+
+/* dwc->lock must be held */
+static void dwc3_drd_statemachine(struct dwc3 *dwc, int id, int vbus)
+{
+   enum usb_otg_state new_state;
+   int protocol;
+
+   if (id == dwc->otg_fsm.id && vbus == dwc->otg_fsm.b_sess_vld)
+   return;
+
+   dwc->otg_fsm.id = id;
+   dwc->otg_fsm.b_sess_vld = vbus;
+
+   if (!id) {
+   new_state = OTG_STATE_A_HOST;
+   } else{
+   if (vbus)
+   new_state = OTG_STATE_B_PERIPHERAL;
+   else
+   new_state = OTG_STATE_B_IDLE;
+   }
+
+   if (dwc->otg.state == new_state)
+   return;
+
+   protocol = dwc->otg_fsm.protocol;
+   switch (new_state) {
+   case OTG_STATE_B_IDLE:
+   if (protocol == PROTO_GADGET)
+   dwc3_drd_start_gadget(dwc, 0);
+   else if (protocol == PROTO_HOST)
+   dwc3_drd_start_host(dwc, 0, 0);
+   dwc->otg_fsm.protocol = PROTO_UNDEF;
+   break;
+   case OTG_STATE_B_PERIPHERAL:
+   if (protocol == PROTO_HOST)
+   dwc3_drd_start_host(dwc, 0, 0);
+
+   if (protocol != PROTO_GADGET) {
+   dwc->otg_fsm.protocol = PROTO_GADGET;
+   dwc3_drd_start_gadget(dwc, 1);
+   }
+   break;
+   case OTG_STATE_A_HOST:
+   if (protocol == PROTO_GADGET)
+   dwc3_drd_start_gadget(dwc, 0);
+
+   if (protocol != PROTO_HOST) {
+   dwc->otg_fsm.protocol = PROTO_HOST;
+   dwc3_drd_start_host(dwc, 1, 0);
+   }
+   break;
+   default:
+   dev_err(dwc->dev, "drd: invalid usb-drd state: %s\n",
+   usb_otg_state_string(new_state));
+   return;
+   }
+
+   dwc->otg.state = new_state;
+}
+
+/* dwc->lock must be held */
+static void dwc3_otg_fsm_sync(struct dwc3 *dwc)
+{
+   u32 reg;
+   int id, vbus;
+
+   /*
+* calling dwc3_otg_fsm_sync() during resume breaks host
+* if adapter was removed during suspend as xhci driver
+* is not prepared to see hcd removal before xhci_resume.
+*/
+   if (dwc->otg_prevent_sync)
+   return;
+
+   reg = dwc3_readl(dwc->regs, DWC3_OSTS);
+   id = !!(reg & DWC3_OSTS_CONIDSTS);
+   vbus = !!(reg & DWC3_OSTS_BSESVLD);
+   dwc3_drd_statemachine(dwc, id, vbus);
+}
+
+static void dwc3_drd_work(struct work_struct *work)
+{
+   struct dwc3 *dwc = container_of(work, struct dwc3,
+   otg_work);
+
+   spin_lock(>lock);
+   dwc3_otg_fsm_sync(dwc);
+   spin_unlock(>lock);
+}
+
+static void dwc3_otg_disable_events(struct dwc3 *dwc, u32 disable_mask)
+{
+   dwc->oevten &= ~(disable_mask);
+   dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
+}
+
+static void dwc3_otg_enable_events(struct dwc3 *dwc, u32 enable_mask)
+{
+   dwc->oevten |= (enable_mask);
+   dwc3_writel(dwc->regs, DWC3_OEVTEN, dwc->oevten);
+}
+
+#define DWC3_OTG_ALL_EVENTS(DWC3_OEVTEN_XHCIRUNSTPSETEN | \
+   DWC3_OEVTEN_DEVRUNSTPSETEN | DWC3_OEVTEN_HIBENTRYEN | \
+   DWC3_OEVTEN_CONIDSTSCHNGEN | DWC3_OEVTEN_HRRCONFNOTIFEN | \
+