Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On 28/04/16 04:54, Peter Chen wrote:
> On Wed, Apr 27, 2016 at 01:59:44PM +0300, Roger Quadros wrote:
>> Hi,
>>
>> On 27/04/16 06:15, Peter Chen wrote:
>>> On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
>>>> On Tue, Apr 26, 2016 at 07:00:22AM +, Jun Li wrote:
>>>>> Hi
>>>>>
>>>>>> -Original Message-
>>>>>> From: Peter Chen [mailto:hzpeterc...@gmail.com]
>>>>>> Sent: Tuesday, April 26, 2016 2:28 PM
>>>>>> To: Jun Li
>>>>>> Cc: Roger Quadros ; st...@rowland.harvard.edu;
>>>>>> ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
>>>>>> dan.j.willi...@intel.com; jun...@freescale.com;
>>>>>> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
>>>>>> abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
>>>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>>>
>>>>>> On Tue, Apr 26, 2016 at 05:11:36AM +, Jun Li wrote:
>>>>>>> Hi
>>>>>>>
>>>>>>>> -Original Message-
>>>>>>>> From: Peter Chen [mailto:hzpeterc...@gmail.com]
>>>>>>>> Sent: Tuesday, April 26, 2016 11:47 AM
>>>>>>>> To: Jun Li
>>>>>>>> Cc: Roger Quadros ; st...@rowland.harvard.edu;
>>>>>>>> ba...@kernel.org; gre...@linuxfoundation.org;
>>>>>>>> peter.c...@freescale.com; dan.j.willi...@intel.com;
>>>>>>>> jun...@freescale.com; mathias.ny...@linux.intel.com;
>>>>>>>> t...@atomide.com; joao.pi...@synopsys.com; abres...@chromium.org;
>>>>>>>> r.bald...@samsung.com; linux-...@vger.kernel.org;
>>>>>>>> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
>>>>>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>>>>>
>>>>>>>> On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
>>>>>>>>>> +struct usb_otg *usb_otg_register(struct device *dev,
>>>>>>>>>> + struct usb_otg_config *config) {
>>>>>>>>>> +struct usb_otg *otg;
>>>>>>>>>> +struct otg_wait_data *wait;
>>>>>>>>>> +int ret = 0;
>>>>>>>>>> +
>>>>>>>>>> +if (!dev || !config || !config->fsm_ops)
>>>>>>>>>> +return ERR_PTR(-EINVAL);
>>>>>>>>>> +
>>>>>>>>>> +/* already in list? */
>>>>>>>>>> +mutex_lock(&otg_list_mutex);
>>>>>>>>>> +if (usb_otg_get_data(dev)) {
>>>>>>>>>> +dev_err(dev, "otg: %s: device already in otg list\n",
>>>>>>>>>> +__func__);
>>>>>>>>>> +ret = -EINVAL;
>>>>>>>>>> +goto unlock;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +/* allocate and add to list */
>>>>>>>>>> +otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>>>>>>>>>> +if (!otg) {
>>>>>>>>>> +ret = -ENOMEM;
>>>>>>>>>> +goto unlock;
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>> +otg->dev = dev;
>>>>>>>>>> +otg->caps = config->otg_caps;
>>>>>>>>>> +
>>>>>>>>>> +if ((otg->caps->hnp_support || otg->caps->srp_support ||
>>>>>>>>>> + otg->caps->adp_support) && !config->otg_work)
>>>>>>>>>> +dev_info(dev, "otg: limiting to dual-role\n");
>>>>>>>>>
>>>>>>>>> dev_err, this should be an error.
>>>>>>>>
>>>>>>>> The condition may be wrong, but it is an information to show that
>>>>>>>> curr
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Wed, Apr 27, 2016 at 01:59:44PM +0300, Roger Quadros wrote:
> Hi,
>
> On 27/04/16 06:15, Peter Chen wrote:
> > On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
> >> On Tue, Apr 26, 2016 at 07:00:22AM +, Jun Li wrote:
> >>> Hi
> >>>
> >>>> -Original Message-
> >>>> From: Peter Chen [mailto:hzpeterc...@gmail.com]
> >>>> Sent: Tuesday, April 26, 2016 2:28 PM
> >>>> To: Jun Li
> >>>> Cc: Roger Quadros ; st...@rowland.harvard.edu;
> >>>> ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
> >>>> dan.j.willi...@intel.com; jun...@freescale.com;
> >>>> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
> >>>> abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
> >>>> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> >>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> >>>>
> >>>> On Tue, Apr 26, 2016 at 05:11:36AM +, Jun Li wrote:
> >>>>> Hi
> >>>>>
> >>>>>> -Original Message-
> >>>>>> From: Peter Chen [mailto:hzpeterc...@gmail.com]
> >>>>>> Sent: Tuesday, April 26, 2016 11:47 AM
> >>>>>> To: Jun Li
> >>>>>> Cc: Roger Quadros ; st...@rowland.harvard.edu;
> >>>>>> ba...@kernel.org; gre...@linuxfoundation.org;
> >>>>>> peter.c...@freescale.com; dan.j.willi...@intel.com;
> >>>>>> jun...@freescale.com; mathias.ny...@linux.intel.com;
> >>>>>> t...@atomide.com; joao.pi...@synopsys.com; abres...@chromium.org;
> >>>>>> r.bald...@samsung.com; linux-...@vger.kernel.org;
> >>>>>> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> >>>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> >>>>>>
> >>>>>> On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
> >>>>>>>> +struct usb_otg *usb_otg_register(struct device *dev,
> >>>>>>>> + struct usb_otg_config *config) {
> >>>>>>>> +struct usb_otg *otg;
> >>>>>>>> +struct otg_wait_data *wait;
> >>>>>>>> +int ret = 0;
> >>>>>>>> +
> >>>>>>>> +if (!dev || !config || !config->fsm_ops)
> >>>>>>>> +return ERR_PTR(-EINVAL);
> >>>>>>>> +
> >>>>>>>> +/* already in list? */
> >>>>>>>> +mutex_lock(&otg_list_mutex);
> >>>>>>>> +if (usb_otg_get_data(dev)) {
> >>>>>>>> +dev_err(dev, "otg: %s: device already in otg list\n",
> >>>>>>>> +__func__);
> >>>>>>>> +ret = -EINVAL;
> >>>>>>>> +goto unlock;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +/* allocate and add to list */
> >>>>>>>> +otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> >>>>>>>> +if (!otg) {
> >>>>>>>> +ret = -ENOMEM;
> >>>>>>>> +goto unlock;
> >>>>>>>> +}
> >>>>>>>> +
> >>>>>>>> +otg->dev = dev;
> >>>>>>>> +otg->caps = config->otg_caps;
> >>>>>>>> +
> >>>>>>>> +if ((otg->caps->hnp_support || otg->caps->srp_support ||
> >>>>>>>> + otg->caps->adp_support) && !config->otg_work)
> >>>>>>>> +dev_info(dev, "otg: limiting to dual-role\n");
> >>>>>>>
> >>>>>>> dev_err, this should be an error.
> >>>>>>
> >>>>>> The condition may be wrong, but it is an information to show that
> >>>>>> current OTG is dual-role.
> >>>>>
> >>>>> This should not happen in any correct design, I even doubt if we
> >>>>> should try to continue by "downgrade" it to be duel role, currently
> >>>>
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi Jun,
On 26/04/16 05:07, Jun Li wrote:
> Hi Roger
>
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Tuesday, April 05, 2016 10:05 PM
>> To: st...@rowland.harvard.edu; ba...@kernel.org;
>> gre...@linuxfoundation.org; peter.c...@freescale.com
>> Cc: dan.j.willi...@intel.com; jun...@freescale.com;
>> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
>> abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
>> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org; Roger Quadros
>>
>> Subject: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>
>> It provides APIs for the following tasks
>>
>> - Registering an OTG/dual-role capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
>>
>> Provide a dual-role device (DRD) state machine.
>> DRD mode is a reduced functionality OTG mode. In this mode we don't
>> support SRP, HNP and dynamic role-swap.
>>
>> In DRD operation, the controller mode (Host or Peripheral) is decided
>> based on the ID pin status. Once a cable plug (Type-A or Type-B) is
>> attached the controller selects the state and doesn't change till the
>> cable in unplugged and a different cable type is inserted.
>>
>> As we don't need most of the complex OTG states and OTG timers we
>> implement a lean DRD state machine in usb-otg.c.
>> The DRD state machine is only interested in 2 hardware inputs 'id' and
>> 'b_sess_vld'.
>>
>> Signed-off-by: Roger Quadros
>> ---
>
> ...
>
>> +/**
>> + * Register pending host/gadget and remove entry from wait list */
>> +static void usb_otg_flush_wait(struct device *otg_dev) {
>> +struct otg_wait_data *wait;
>> +struct otg_hcd *host;
>> +struct otg_gcd *gadget;
>> +
>> +mutex_lock(&wait_list_mutex);
>> +
>> +wait = usb_otg_get_wait(otg_dev);
>> +if (!wait)
>> +goto done;
>> +
>> +dev_dbg(otg_dev, "otg: registering pending host/gadget\n");
>> +gadget = &wait->gcd;
>> +if (gadget)
>
> If (gadget->gadget)
good catch :)
I'll probably rename the local variables
host to hcd
gadget to gcd.
>
>> +usb_otg_register_gadget(gadget->gadget, gadget->ops);
>> +
>> +host = &wait->primary_hcd;
>> +if (host->hcd)
>> +usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
>> + host->ops);
>> +
>> +host = &wait->shared_hcd;
>> +if (host->hcd)
>> +usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
>> + host->ops);
>> +
>> +list_del(&wait->list);
>> +kfree(wait);
>> +
>> +done:
>> +mutex_unlock(&wait_list_mutex);
>> +}
>> +
>> +/**
>> + * Check if the OTG device is in our OTG list and return
>> + * usb_otg data, else NULL.
>> + *
>> + * otg_list_mutex must be held.
>> + */
>> +static struct usb_otg *usb_otg_get_data(struct device *otg_dev) {
>> +struct usb_otg *otg;
>> +
>> +if (!otg_dev)
>> +return NULL;
>> +
>> +list_for_each_entry(otg, &otg_list, list) {
>> +if (otg->dev == otg_dev)
>> +return otg;
>> +}
>> +
>> +return NULL;
>> +}
>
> Could you export it to be a public API, we may need access usb_otg
> in common host driver for handling of enumeration of otg test device.
We can always do that later. As of now nobody is using it so let's keep it
private.
>
> ...
>
>> +/**
>> + * Called when entering a DRD state.
>> + * fsm->lock must be held.
>> + */
>> +static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state
>> +new_state) {
>> +struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
>> +
>> +if (otg->state == new_state)
>> +return;
>> +
>> +fsm->state_changed = 1;
>> +dev_dbg(otg->dev, "otg: set state: %s\n",
>> +usb_otg_state_string(new_state));
>> +switch (new_state) {
>> +case OTG_STATE_B_IDLE:
>> +drd_set_protocol(fsm, PROTO_UNDEF);
>> +otg_drv_vbus(otg, 0);
>> +break;
>> +case OTG_STATE_B_PERIPHERAL:
>> +drd_set_protocol(fsm, PROTO_GADGET);
>> +otg_drv_vbus(otg, 0);
>> +break;
>> +case OTG_STATE_A_HOST:
>> +drd_set_protocol(fsm, PROTO_HOST);
>> +otg_drv_vbus(otg, 1);
>> +break;
>> +case OTG_STATE_UNDEFINED:
>> +case OTG_STATE_B_SRP_INIT:
>> +case OTG_STATE_B_WAIT_ACON:
>> +case OTG_STATE_B_HOST:
>> +case OTG_STATE_A_IDLE:
>> +case OTG_STATE_A_WAIT_VRISE:
>> +case OTG_STATE_A_WAIT_BCON:
>> +case OTG_STATE_A_SUSPEND:
>> +case OTG_STATE_A_PERIPHERAL:
>> +case OTG_STATE_A_WAIT_VFALL:
>> +case OTG_STATE_A_VBUS_ERR:
>
> Remove above unused states.
OK.
>
>> +default:
>> +dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
>> + __func__, usb_otg_state_string(new_state));
>> +break;
>> +
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
On 27/04/16 06:15, Peter Chen wrote:
> On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
>> On Tue, Apr 26, 2016 at 07:00:22AM +, Jun Li wrote:
>>> Hi
>>>
>>>> -Original Message-
>>>> From: Peter Chen [mailto:hzpeterc...@gmail.com]
>>>> Sent: Tuesday, April 26, 2016 2:28 PM
>>>> To: Jun Li
>>>> Cc: Roger Quadros ; st...@rowland.harvard.edu;
>>>> ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
>>>> dan.j.willi...@intel.com; jun...@freescale.com;
>>>> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
>>>> abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
>>>> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>
>>>> On Tue, Apr 26, 2016 at 05:11:36AM +, Jun Li wrote:
>>>>> Hi
>>>>>
>>>>>> -Original Message-
>>>>>> From: Peter Chen [mailto:hzpeterc...@gmail.com]
>>>>>> Sent: Tuesday, April 26, 2016 11:47 AM
>>>>>> To: Jun Li
>>>>>> Cc: Roger Quadros ; st...@rowland.harvard.edu;
>>>>>> ba...@kernel.org; gre...@linuxfoundation.org;
>>>>>> peter.c...@freescale.com; dan.j.willi...@intel.com;
>>>>>> jun...@freescale.com; mathias.ny...@linux.intel.com;
>>>>>> t...@atomide.com; joao.pi...@synopsys.com; abres...@chromium.org;
>>>>>> r.bald...@samsung.com; linux-...@vger.kernel.org;
>>>>>> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
>>>>>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>>>>>
>>>>>> On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
>>>>>>>> +struct usb_otg *usb_otg_register(struct device *dev,
>>>>>>>> + struct usb_otg_config *config) {
>>>>>>>> + struct usb_otg *otg;
>>>>>>>> + struct otg_wait_data *wait;
>>>>>>>> + int ret = 0;
>>>>>>>> +
>>>>>>>> + if (!dev || !config || !config->fsm_ops)
>>>>>>>> + return ERR_PTR(-EINVAL);
>>>>>>>> +
>>>>>>>> + /* already in list? */
>>>>>>>> + mutex_lock(&otg_list_mutex);
>>>>>>>> + if (usb_otg_get_data(dev)) {
>>>>>>>> + dev_err(dev, "otg: %s: device already in otg list\n",
>>>>>>>> + __func__);
>>>>>>>> + ret = -EINVAL;
>>>>>>>> + goto unlock;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + /* allocate and add to list */
>>>>>>>> + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>>>>>>>> + if (!otg) {
>>>>>>>> + ret = -ENOMEM;
>>>>>>>> + goto unlock;
>>>>>>>> + }
>>>>>>>> +
>>>>>>>> + otg->dev = dev;
>>>>>>>> + otg->caps = config->otg_caps;
>>>>>>>> +
>>>>>>>> + if ((otg->caps->hnp_support || otg->caps->srp_support ||
>>>>>>>> + otg->caps->adp_support) && !config->otg_work)
>>>>>>>> + dev_info(dev, "otg: limiting to dual-role\n");
>>>>>>>
>>>>>>> dev_err, this should be an error.
>>>>>>
>>>>>> The condition may be wrong, but it is an information to show that
>>>>>> current OTG is dual-role.
>>>>>
>>>>> This should not happen in any correct design, I even doubt if we
>>>>> should try to continue by "downgrade" it to be duel role, currently
>>>>> the only example user is dual role, so doing like this can't be tested
>>>>> by real case, this downgrade is not so easy like we image, at least
>>>>> for chipidea otg driver, simply replace a queue worker may not work,
>>>>> as we have much more difference between the 2 configs.
>>>>>
>>>>
>>>>
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
> On Tue, Apr 26, 2016 at 07:00:22AM +, Jun Li wrote:
> > Hi
> >
> > > -Original Message-
> > > From: Peter Chen [mailto:hzpeterc...@gmail.com]
> > > Sent: Tuesday, April 26, 2016 2:28 PM
> > > To: Jun Li
> > > Cc: Roger Quadros ; st...@rowland.harvard.edu;
> > > ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
> > > dan.j.willi...@intel.com; jun...@freescale.com;
> > > mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
> > > abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> > > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > >
> > > On Tue, Apr 26, 2016 at 05:11:36AM +, Jun Li wrote:
> > > > Hi
> > > >
> > > > > -Original Message-
> > > > > From: Peter Chen [mailto:hzpeterc...@gmail.com]
> > > > > Sent: Tuesday, April 26, 2016 11:47 AM
> > > > > To: Jun Li
> > > > > Cc: Roger Quadros ; st...@rowland.harvard.edu;
> > > > > ba...@kernel.org; gre...@linuxfoundation.org;
> > > > > peter.c...@freescale.com; dan.j.willi...@intel.com;
> > > > > jun...@freescale.com; mathias.ny...@linux.intel.com;
> > > > > t...@atomide.com; joao.pi...@synopsys.com; abres...@chromium.org;
> > > > > r.bald...@samsung.com; linux-...@vger.kernel.org;
> > > > > linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> > > > > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > > > >
> > > > > On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
> > > > > > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > > > > > + struct usb_otg_config *config) {
> > > > > > > + struct usb_otg *otg;
> > > > > > > + struct otg_wait_data *wait;
> > > > > > > + int ret = 0;
> > > > > > > +
> > > > > > > + if (!dev || !config || !config->fsm_ops)
> > > > > > > + return ERR_PTR(-EINVAL);
> > > > > > > +
> > > > > > > + /* already in list? */
> > > > > > > + mutex_lock(&otg_list_mutex);
> > > > > > > + if (usb_otg_get_data(dev)) {
> > > > > > > + dev_err(dev, "otg: %s: device already in otg list\n",
> > > > > > > + __func__);
> > > > > > > + ret = -EINVAL;
> > > > > > > + goto unlock;
> > > > > > > + }
> > > > > > > +
> > > > > > > + /* allocate and add to list */
> > > > > > > + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > > > > > + if (!otg) {
> > > > > > > + ret = -ENOMEM;
> > > > > > > + goto unlock;
> > > > > > > + }
> > > > > > > +
> > > > > > > + otg->dev = dev;
> > > > > > > + otg->caps = config->otg_caps;
> > > > > > > +
> > > > > > > + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > > > > > + otg->caps->adp_support) && !config->otg_work)
> > > > > > > + dev_info(dev, "otg: limiting to dual-role\n");
> > > > > >
> > > > > > dev_err, this should be an error.
> > > > >
> > > > > The condition may be wrong, but it is an information to show that
> > > > > current OTG is dual-role.
> > > >
> > > > This should not happen in any correct design, I even doubt if we
> > > > should try to continue by "downgrade" it to be duel role, currently
> > > > the only example user is dual role, so doing like this can't be tested
> > > > by real case, this downgrade is not so easy like we image, at least
> > > > for chipidea otg driver, simply replace a queue worker may not work,
> > > > as we have much more difference between the 2 configs.
> > > >
> > >
> > > Would you show more why chipidea can't work just replace the work item,
> > > and see if anything we still can improve for this framework?
&g
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Tue, Apr 26, 2016 at 07:00:22AM +, Jun Li wrote:
> Hi
>
> > -Original Message-
> > From: Peter Chen [mailto:hzpeterc...@gmail.com]
> > Sent: Tuesday, April 26, 2016 2:28 PM
> > To: Jun Li
> > Cc: Roger Quadros ; st...@rowland.harvard.edu;
> > ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
> > dan.j.willi...@intel.com; jun...@freescale.com;
> > mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
> > abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> >
> > On Tue, Apr 26, 2016 at 05:11:36AM +, Jun Li wrote:
> > > Hi
> > >
> > > > -Original Message-
> > > > From: Peter Chen [mailto:hzpeterc...@gmail.com]
> > > > Sent: Tuesday, April 26, 2016 11:47 AM
> > > > To: Jun Li
> > > > Cc: Roger Quadros ; st...@rowland.harvard.edu;
> > > > ba...@kernel.org; gre...@linuxfoundation.org;
> > > > peter.c...@freescale.com; dan.j.willi...@intel.com;
> > > > jun...@freescale.com; mathias.ny...@linux.intel.com;
> > > > t...@atomide.com; joao.pi...@synopsys.com; abres...@chromium.org;
> > > > r.bald...@samsung.com; linux-...@vger.kernel.org;
> > > > linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> > > > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > > >
> > > > On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
> > > > > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > > > > +struct usb_otg_config *config) {
> > > > > > + struct usb_otg *otg;
> > > > > > + struct otg_wait_data *wait;
> > > > > > + int ret = 0;
> > > > > > +
> > > > > > + if (!dev || !config || !config->fsm_ops)
> > > > > > + return ERR_PTR(-EINVAL);
> > > > > > +
> > > > > > + /* already in list? */
> > > > > > + mutex_lock(&otg_list_mutex);
> > > > > > + if (usb_otg_get_data(dev)) {
> > > > > > + dev_err(dev, "otg: %s: device already in otg list\n",
> > > > > > + __func__);
> > > > > > + ret = -EINVAL;
> > > > > > + goto unlock;
> > > > > > + }
> > > > > > +
> > > > > > + /* allocate and add to list */
> > > > > > + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > > > > + if (!otg) {
> > > > > > + ret = -ENOMEM;
> > > > > > + goto unlock;
> > > > > > + }
> > > > > > +
> > > > > > + otg->dev = dev;
> > > > > > + otg->caps = config->otg_caps;
> > > > > > +
> > > > > > + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > > > > +otg->caps->adp_support) && !config->otg_work)
> > > > > > + dev_info(dev, "otg: limiting to dual-role\n");
> > > > >
> > > > > dev_err, this should be an error.
> > > >
> > > > The condition may be wrong, but it is an information to show that
> > > > current OTG is dual-role.
> > >
> > > This should not happen in any correct design, I even doubt if we
> > > should try to continue by "downgrade" it to be duel role, currently
> > > the only example user is dual role, so doing like this can't be tested
> > > by real case, this downgrade is not so easy like we image, at least
> > > for chipidea otg driver, simply replace a queue worker may not work,
> > > as we have much more difference between the 2 configs.
> > >
> >
> > Would you show more why chipidea can't work just replace the work item,
> > and see if anything we still can improve for this framework?
>
> In real OTG, we need enable AVV irq,
Enable and Handling AVV is platform stuff. In this framework, we are
focus on how otg device manages host and gadget together, and the state
machine when the related otg event occurs.
> but for duel role, nobody care/handle,
> there are much more resource required for OTG: timers, hnp polling,
> otg test device handling...
They are common things for fully OTG fsm, you can move them
to common code (In fact, hnp polling handling is already common code).
>
> with current design, chipidea driver can support real OTG with its own
> queue worker, or DRD with Roger's drd work item if config is correct.
>
> But improve something to work on a *wrong* config will make it complicated
> and does not make much sense IMO.
>
What does above "config" you mean?
If the configure is fully OTG, you can choose different state machine,
eg otg_statemachine, if you find it is hard for chipidea to use this
framework, just list the reason, and see if we can improve.
--
Best Regards,
Peter Chen
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi
> -Original Message-
> From: Peter Chen [mailto:hzpeterc...@gmail.com]
> Sent: Tuesday, April 26, 2016 2:28 PM
> To: Jun Li
> Cc: Roger Quadros ; st...@rowland.harvard.edu;
> ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
> dan.j.willi...@intel.com; jun...@freescale.com;
> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
> abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>
> On Tue, Apr 26, 2016 at 05:11:36AM +, Jun Li wrote:
> > Hi
> >
> > > -Original Message-
> > > From: Peter Chen [mailto:hzpeterc...@gmail.com]
> > > Sent: Tuesday, April 26, 2016 11:47 AM
> > > To: Jun Li
> > > Cc: Roger Quadros ; st...@rowland.harvard.edu;
> > > ba...@kernel.org; gre...@linuxfoundation.org;
> > > peter.c...@freescale.com; dan.j.willi...@intel.com;
> > > jun...@freescale.com; mathias.ny...@linux.intel.com;
> > > t...@atomide.com; joao.pi...@synopsys.com; abres...@chromium.org;
> > > r.bald...@samsung.com; linux-...@vger.kernel.org;
> > > linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> > > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> > >
> > > On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
> > > > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > > > + struct usb_otg_config *config) {
> > > > > + struct usb_otg *otg;
> > > > > + struct otg_wait_data *wait;
> > > > > + int ret = 0;
> > > > > +
> > > > > + if (!dev || !config || !config->fsm_ops)
> > > > > + return ERR_PTR(-EINVAL);
> > > > > +
> > > > > + /* already in list? */
> > > > > + mutex_lock(&otg_list_mutex);
> > > > > + if (usb_otg_get_data(dev)) {
> > > > > + dev_err(dev, "otg: %s: device already in otg list\n",
> > > > > + __func__);
> > > > > + ret = -EINVAL;
> > > > > + goto unlock;
> > > > > + }
> > > > > +
> > > > > + /* allocate and add to list */
> > > > > + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > > > + if (!otg) {
> > > > > + ret = -ENOMEM;
> > > > > + goto unlock;
> > > > > + }
> > > > > +
> > > > > + otg->dev = dev;
> > > > > + otg->caps = config->otg_caps;
> > > > > +
> > > > > + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > > > + otg->caps->adp_support) && !config->otg_work)
> > > > > + dev_info(dev, "otg: limiting to dual-role\n");
> > > >
> > > > dev_err, this should be an error.
> > >
> > > The condition may be wrong, but it is an information to show that
> > > current OTG is dual-role.
> >
> > This should not happen in any correct design, I even doubt if we
> > should try to continue by "downgrade" it to be duel role, currently
> > the only example user is dual role, so doing like this can't be tested
> > by real case, this downgrade is not so easy like we image, at least
> > for chipidea otg driver, simply replace a queue worker may not work,
> > as we have much more difference between the 2 configs.
> >
>
> Would you show more why chipidea can't work just replace the work item,
> and see if anything we still can improve for this framework?
In real OTG, we need enable AVV irq, but for duel role, nobody care/handle,
there are much more resource required for OTG: timers, hnp polling,
otg test device handling...
with current design, chipidea driver can support real OTG with its own
queue worker, or DRD with Roger's drd work item if config is correct.
But improve something to work on a *wrong* config will make it complicated
and does not make much sense IMO.
Li Jun
>
> --
> Best Regards,
> Peter Chen
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Tue, Apr 26, 2016 at 05:11:36AM +, Jun Li wrote:
> Hi
>
> > -Original Message-
> > From: Peter Chen [mailto:hzpeterc...@gmail.com]
> > Sent: Tuesday, April 26, 2016 11:47 AM
> > To: Jun Li
> > Cc: Roger Quadros ; st...@rowland.harvard.edu;
> > ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
> > dan.j.willi...@intel.com; jun...@freescale.com;
> > mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
> > abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
> > linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> > Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> >
> > On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
> > > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > > +struct usb_otg_config *config) {
> > > > + struct usb_otg *otg;
> > > > + struct otg_wait_data *wait;
> > > > + int ret = 0;
> > > > +
> > > > + if (!dev || !config || !config->fsm_ops)
> > > > + return ERR_PTR(-EINVAL);
> > > > +
> > > > + /* already in list? */
> > > > + mutex_lock(&otg_list_mutex);
> > > > + if (usb_otg_get_data(dev)) {
> > > > + dev_err(dev, "otg: %s: device already in otg list\n",
> > > > + __func__);
> > > > + ret = -EINVAL;
> > > > + goto unlock;
> > > > + }
> > > > +
> > > > + /* allocate and add to list */
> > > > + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > > + if (!otg) {
> > > > + ret = -ENOMEM;
> > > > + goto unlock;
> > > > + }
> > > > +
> > > > + otg->dev = dev;
> > > > + otg->caps = config->otg_caps;
> > > > +
> > > > + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > > +otg->caps->adp_support) && !config->otg_work)
> > > > + dev_info(dev, "otg: limiting to dual-role\n");
> > >
> > > dev_err, this should be an error.
> >
> > The condition may be wrong, but it is an information to show that current
> > OTG is dual-role.
>
> This should not happen in any correct design, I even doubt if we
> should try to continue by "downgrade" it to be duel role, currently
> the only example user is dual role, so doing like this can't be
> tested by real case, this downgrade is not so easy like we image,
> at least for chipidea otg driver, simply replace a queue worker may
> not work, as we have much more difference between the 2 configs.
>
Would you show more why chipidea can't work just replace the work item,
and see if anything we still can improve for this framework?
--
Best Regards,
Peter Chen
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi
> -Original Message-
> From: Peter Chen [mailto:hzpeterc...@gmail.com]
> Sent: Tuesday, April 26, 2016 11:47 AM
> To: Jun Li
> Cc: Roger Quadros ; st...@rowland.harvard.edu;
> ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
> dan.j.willi...@intel.com; jun...@freescale.com;
> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
> abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org
> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>
> On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
> > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > + struct usb_otg_config *config) {
> > > + struct usb_otg *otg;
> > > + struct otg_wait_data *wait;
> > > + int ret = 0;
> > > +
> > > + if (!dev || !config || !config->fsm_ops)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + /* already in list? */
> > > + mutex_lock(&otg_list_mutex);
> > > + if (usb_otg_get_data(dev)) {
> > > + dev_err(dev, "otg: %s: device already in otg list\n",
> > > + __func__);
> > > + ret = -EINVAL;
> > > + goto unlock;
> > > + }
> > > +
> > > + /* allocate and add to list */
> > > + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > + if (!otg) {
> > > + ret = -ENOMEM;
> > > + goto unlock;
> > > + }
> > > +
> > > + otg->dev = dev;
> > > + otg->caps = config->otg_caps;
> > > +
> > > + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > > + otg->caps->adp_support) && !config->otg_work)
> > > + dev_info(dev, "otg: limiting to dual-role\n");
> >
> > dev_err, this should be an error.
>
> The condition may be wrong, but it is an information to show that current
> OTG is dual-role.
This should not happen in any correct design, I even doubt if we
should try to continue by "downgrade" it to be duel role, currently
the only example user is dual role, so doing like this can't be
tested by real case, this downgrade is not so easy like we image,
at least for chipidea otg driver, simply replace a queue worker may
not work, as we have much more difference between the 2 configs.
Li Jun
>
> Peter
> >
> > > +
> > > + if (config->otg_work) /* custom otg_work ? */
> > > + INIT_WORK(&otg->work, config->otg_work);
> > > + else
> > > + INIT_WORK(&otg->work, usb_otg_work);
> > > +
> > > + otg->wq = create_singlethread_workqueue("usb_otg");
> > > + if (!otg->wq) {
> > > + dev_err(dev, "otg: %s: can't create workqueue\n",
> > > + __func__);
> > > + ret = -ENOMEM;
> > > + goto err_wq;
> > > + }
> > > +
> > > + /* set otg ops */
> > > + otg->fsm.ops = config->fsm_ops;
> > > +
> > > + mutex_init(&otg->fsm.lock);
> > > +
> > > + list_add_tail(&otg->list, &otg_list);
> > > + mutex_unlock(&otg_list_mutex);
> > > +
> > > + /* were we in wait list? */
> > > + mutex_lock(&wait_list_mutex);
> > > + wait = usb_otg_get_wait(dev);
> > > + mutex_unlock(&wait_list_mutex);
> > > + if (wait) {
> > > + /* register pending host/gadget and flush from list */
> > > + usb_otg_flush_wait(dev);
> > > + }
> > > +
> > > + return otg;
> > > +
> > > +err_wq:
> > > + kfree(otg);
> > > +unlock:
> > > + mutex_unlock(&otg_list_mutex);
> > > + return ERR_PTR(ret);
> > > +}
> > > +EXPORT_SYMBOL_GPL(usb_otg_register);
> > > +
> >
> > --
> > 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
>
> --
> Best Regards,
> Peter Chen
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
> > +struct usb_otg *usb_otg_register(struct device *dev,
> > +struct usb_otg_config *config)
> > +{
> > + struct usb_otg *otg;
> > + struct otg_wait_data *wait;
> > + int ret = 0;
> > +
> > + if (!dev || !config || !config->fsm_ops)
> > + return ERR_PTR(-EINVAL);
> > +
> > + /* already in list? */
> > + mutex_lock(&otg_list_mutex);
> > + if (usb_otg_get_data(dev)) {
> > + dev_err(dev, "otg: %s: device already in otg list\n",
> > + __func__);
> > + ret = -EINVAL;
> > + goto unlock;
> > + }
> > +
> > + /* allocate and add to list */
> > + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > + if (!otg) {
> > + ret = -ENOMEM;
> > + goto unlock;
> > + }
> > +
> > + otg->dev = dev;
> > + otg->caps = config->otg_caps;
> > +
> > + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> > +otg->caps->adp_support) && !config->otg_work)
> > + dev_info(dev, "otg: limiting to dual-role\n");
>
> dev_err, this should be an error.
The condition may be wrong, but it is an information to show
that current OTG is dual-role.
Peter
>
> > +
> > + if (config->otg_work) /* custom otg_work ? */
> > + INIT_WORK(&otg->work, config->otg_work);
> > + else
> > + INIT_WORK(&otg->work, usb_otg_work);
> > +
> > + otg->wq = create_singlethread_workqueue("usb_otg");
> > + if (!otg->wq) {
> > + dev_err(dev, "otg: %s: can't create workqueue\n",
> > + __func__);
> > + ret = -ENOMEM;
> > + goto err_wq;
> > + }
> > +
> > + /* set otg ops */
> > + otg->fsm.ops = config->fsm_ops;
> > +
> > + mutex_init(&otg->fsm.lock);
> > +
> > + list_add_tail(&otg->list, &otg_list);
> > + mutex_unlock(&otg_list_mutex);
> > +
> > + /* were we in wait list? */
> > + mutex_lock(&wait_list_mutex);
> > + wait = usb_otg_get_wait(dev);
> > + mutex_unlock(&wait_list_mutex);
> > + if (wait) {
> > + /* register pending host/gadget and flush from list */
> > + usb_otg_flush_wait(dev);
> > + }
> > +
> > + return otg;
> > +
> > +err_wq:
> > + kfree(otg);
> > +unlock:
> > + mutex_unlock(&otg_list_mutex);
> > + return ERR_PTR(ret);
> > +}
> > +EXPORT_SYMBOL_GPL(usb_otg_register);
> > +
>
> --
> 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
--
Best Regards,
Peter Chen
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi Roger
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Tuesday, April 05, 2016 10:05 PM
> To: st...@rowland.harvard.edu; ba...@kernel.org;
> gre...@linuxfoundation.org; peter.c...@freescale.com
> Cc: dan.j.willi...@intel.com; jun...@freescale.com;
> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
> abres...@chromium.org; r.bald...@samsung.com; linux-...@vger.kernel.org;
> linux-kernel@vger.kernel.org; linux-o...@vger.kernel.org; Roger Quadros
>
> Subject: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>
> It provides APIs for the following tasks
>
> - Registering an OTG/dual-role capable controller
> - Registering Host and Gadget controllers to OTG core
> - Providing inputs to and kicking the OTG state machine
>
> Provide a dual-role device (DRD) state machine.
> DRD mode is a reduced functionality OTG mode. In this mode we don't
> support SRP, HNP and dynamic role-swap.
>
> In DRD operation, the controller mode (Host or Peripheral) is decided
> based on the ID pin status. Once a cable plug (Type-A or Type-B) is
> attached the controller selects the state and doesn't change till the
> cable in unplugged and a different cable type is inserted.
>
> As we don't need most of the complex OTG states and OTG timers we
> implement a lean DRD state machine in usb-otg.c.
> The DRD state machine is only interested in 2 hardware inputs 'id' and
> 'b_sess_vld'.
>
> Signed-off-by: Roger Quadros
> ---
...
> +/**
> + * Register pending host/gadget and remove entry from wait list */
> +static void usb_otg_flush_wait(struct device *otg_dev) {
> + struct otg_wait_data *wait;
> + struct otg_hcd *host;
> + struct otg_gcd *gadget;
> +
> + mutex_lock(&wait_list_mutex);
> +
> + wait = usb_otg_get_wait(otg_dev);
> + if (!wait)
> + goto done;
> +
> + dev_dbg(otg_dev, "otg: registering pending host/gadget\n");
> + gadget = &wait->gcd;
> + if (gadget)
If (gadget->gadget)
> + usb_otg_register_gadget(gadget->gadget, gadget->ops);
> +
> + host = &wait->primary_hcd;
> + if (host->hcd)
> + usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
> + host->ops);
> +
> + host = &wait->shared_hcd;
> + if (host->hcd)
> + usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
> + host->ops);
> +
> + list_del(&wait->list);
> + kfree(wait);
> +
> +done:
> + mutex_unlock(&wait_list_mutex);
> +}
> +
> +/**
> + * Check if the OTG device is in our OTG list and return
> + * usb_otg data, else NULL.
> + *
> + * otg_list_mutex must be held.
> + */
> +static struct usb_otg *usb_otg_get_data(struct device *otg_dev) {
> + struct usb_otg *otg;
> +
> + if (!otg_dev)
> + return NULL;
> +
> + list_for_each_entry(otg, &otg_list, list) {
> + if (otg->dev == otg_dev)
> + return otg;
> + }
> +
> + return NULL;
> +}
Could you export it to be a public API, we may need access usb_otg
in common host driver for handling of enumeration of otg test device.
...
> +/**
> + * Called when entering a DRD state.
> + * fsm->lock must be held.
> + */
> +static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state
> +new_state) {
> + struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
> +
> + if (otg->state == new_state)
> + return;
> +
> + fsm->state_changed = 1;
> + dev_dbg(otg->dev, "otg: set state: %s\n",
> + usb_otg_state_string(new_state));
> + switch (new_state) {
> + case OTG_STATE_B_IDLE:
> + drd_set_protocol(fsm, PROTO_UNDEF);
> + otg_drv_vbus(otg, 0);
> + break;
> + case OTG_STATE_B_PERIPHERAL:
> + drd_set_protocol(fsm, PROTO_GADGET);
> + otg_drv_vbus(otg, 0);
> + break;
> + case OTG_STATE_A_HOST:
> + drd_set_protocol(fsm, PROTO_HOST);
> + otg_drv_vbus(otg, 1);
> + break;
> + case OTG_STATE_UNDEFINED:
> + case OTG_STATE_B_SRP_INIT:
> + case OTG_STATE_B_WAIT_ACON:
> + case OTG_STATE_B_HOST:
> + case OTG_STATE_A_IDLE:
> + case OTG_STATE_A_WAIT_VRISE:
> + case OTG_STATE_A_WAIT_BCON:
> + case OTG_STATE_A_SUSPEND:
> + case OTG_STATE_A_PERIPHERAL:
> + case OTG_STATE_A_WAIT_VFALL:
> + case OTG_STATE_A_VBUS_ERR:
Remove above unused states.
> + default:
> + dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
> + __func__, usb_otg_state_string(new_state));
> + break;
> + }
> +
> + otg->state = new_state;
> +}
> +
> +/**
> + * DRD state change judgement
> + *
> + * For DRD we're only interested in some of the OTG states
> + * i.e. OTG_STATE_B_IDLE: both peripheral and host are stopped
> + * OTG_STATE_B_PERIPHERAL: peripheral active
> + * OTG_STATE_A_HOST: host active
>
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Peter,
On 21/04/16 09:52, Peter Chen wrote:
> On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
>> It provides APIs for the following tasks
>>
>> - Registering an OTG/dual-role capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
>>
>> Provide a dual-role device (DRD) state machine.
>> DRD mode is a reduced functionality OTG mode. In this mode
>> we don't support SRP, HNP and dynamic role-swap.
>>
>> In DRD operation, the controller mode (Host or Peripheral)
>> is decided based on the ID pin status. Once a cable plug (Type-A
>> or Type-B) is attached the controller selects the state
>> and doesn't change till the cable in unplugged and a different
>> cable type is inserted.
>>
>> As we don't need most of the complex OTG states and OTG timers
>> we implement a lean DRD state machine in usb-otg.c.
>> The DRD state machine is only interested in 2 hardware inputs
>> 'id' and 'b_sess_vld'.
>>
>> Signed-off-by: Roger Quadros
>> ---
>> drivers/usb/common/Makefile |2 +-
>> drivers/usb/common/usb-otg.c | 1058
>> ++
>> drivers/usb/common/usb-otg.h | 71 +++
>> drivers/usb/core/Kconfig |2 +-
>> include/linux/usb/gadget.h |2 +
>> include/linux/usb/hcd.h |1 +
>> include/linux/usb/otg-fsm.h |7 +
>> include/linux/usb/otg.h | 154 +-
>> 8 files changed, 1292 insertions(+), 5 deletions(-)
>> create mode 100644 drivers/usb/common/usb-otg.c
>> create mode 100644 drivers/usb/common/usb-otg.h
>>
>> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
>> index f8f2c88..730d928 100644
>> --- a/drivers/usb/common/Makefile
>> +++ b/drivers/usb/common/Makefile
>> @@ -7,5 +7,5 @@ usb-common-y += common.o
>> usb-common-$(CONFIG_USB_LED_TRIG) += led.o
>>
>> obj-$(CONFIG_USB_ULPI_BUS) += ulpi.o
>> -usbotg-y:= usb-otg-fsm.o
>> +usbotg-y:= usb-otg.o usb-otg-fsm.o
>> obj-$(CONFIG_USB_OTG) += usbotg.o
>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>> new file mode 100644
>> index 000..41e762a
>> --- /dev/null
>> +++ b/drivers/usb/common/usb-otg.c
>> @@ -0,0 +1,1058 @@
>> +/**
>> + * drivers/usb/common/usb-otg.c - USB OTG core
>> + *
>> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com
>> + * Author: Roger Quadros
>> + *
>> + * 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.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include "usb-otg.h"
>> +
>> +struct otg_gcd {
>> +struct usb_gadget *gadget;
>> +struct otg_gadget_ops *ops;
>> +};
>> +
>> +/* OTG device list */
>> +LIST_HEAD(otg_list);
>> +static DEFINE_MUTEX(otg_list_mutex);
>> +
>> +/* Hosts and Gadgets waiting for OTG controller */
>> +struct otg_wait_data {
>> +struct device *dev; /* OTG controller device */
>> +
>> +struct otg_hcd primary_hcd;
>> +struct otg_hcd shared_hcd;
>> +struct otg_gcd gcd;
>> +struct list_head list;
>> +};
>> +
>> +LIST_HEAD(wait_list);
>> +static DEFINE_MUTEX(wait_list_mutex);
>> +
>> +static int usb_otg_hcd_is_primary_hcd(struct usb_hcd *hcd)
>> +{
>> +if (!hcd->primary_hcd)
>> +return 1;
>> +return hcd == hcd->primary_hcd;
>> +}
>
> Just find there is already a usb_hcd_is_primary_hcd at hcd.c,
> would you use it directly?
We can't even if we make it public as HCD can be a loadable module
and OTG is always built-in thus leading to undefined reference.
cheers,
-roger
>
> Peter
>> +
>> +/**
>> + * Check if the OTG device is in our wait list and return
>> + * otg_wait_data, else NULL.
>> + *
>> + * wait_list_mutex must be held.
>> + */
>> +static struct otg_wait_data *usb_otg_get_wait(struct device *otg_dev)
>> +{
>> +struct otg_wait_data *wait;
>> +
>> +if (!otg_dev)
>> +return NULL;
>> +
>> +/* is there an entry for this otg_dev ?*/
>> +list_for_each_entry(wait, &wait_list, list) {
>> +if (wait->dev == otg_dev)
>> +return wait;
>> +}
>> +
>> +return NULL;
>> +}
>> +
>> +/**
>> + * Add the hcd to our wait list
>> + */
>> +static int usb_otg_hcd_wait_add(struct device *otg_dev, struct usb_hcd *hcd,
>> +unsigned int irqnum, unsigned long irqflags,
>> +struct otg_hcd_ops *ops)
>> +{
>> +struct otg_wait_data *wait;
>> +int ret = -EINVAL;
>> +
>> +
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Wed, Apr 20, 2016 at 10:03:34AM +0300, Roger Quadros wrote:
> On 20/04/16 08:08, Yoshihiro Shimoda wrote:
> > Hi,
> >
> >> From: Peter Chen
> >> Sent: Tuesday, April 19, 2016 6:18 PM
> >>
> >> On Fri, Apr 15, 2016 at 10:03:16AM +, Yoshihiro Shimoda wrote:
> >>> Hi,
> >>>
> From: Yoshihiro Shimoda
> Sent: Friday, April 15, 2016 6:59 PM
>
> Hi,
>
> > From: Roger Quadros
> > Sent: Thursday, April 14, 2016 8:32 PM
> >
> > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> >> Hi,
> >>
> < snip >
> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> unsigned int irqnum,
> * we're ready only if we have shared HCD
> * or we don't need shared HCD.
> */
> -if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> +if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> +!otg->primary_hcd.hcd->shared_hcd))
> {
> otg->host = hcd_to_bus(hcd);
> /* FIXME: set bus->otg_port if this is true OTG port
> with HNP */
>
> >>>
> >>> These changes look good to me. Thanks.
> >>
> >> Thank you for the comment.
> >> If we change the "needs_companion" place to the otg_config,
> >> do we need to add a flag into the otg, instead of otg->caps?
> >
> > Yes we can add a flag in struct usb_otg.
>
> Thank you for the comment.
>
> I made a fixed patch.
> So, should I send this patch to ML after you sent v7 patches?
> Or, would you apply this patch before you send v7 patches?
> >>>
> >>> Oops, I sent this email without my patch...
> >>>
> >>> ---
> >>> Subject: [PATCH] usb: otg: add hcd companion support
> >>>
> >>> Since some host controller (e.g. EHCI) needs a companion host controller
> >>> (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> >>> core.
> >>>
> >>> Signed-off-by: Yoshihiro Shimoda
> >>> ---
> >>> Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> >>> drivers/usb/common/usb-otg.c | 17 +
> >>> include/linux/usb/otg.h | 7 ++-
> >>> 3 files changed, 22 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/usb/generic.txt
> >>> b/Documentation/devicetree/bindings/usb/generic.txt
> >>> index f6866c1..1db1c33 100644
> >>> --- a/Documentation/devicetree/bindings/usb/generic.txt
> >>> +++ b/Documentation/devicetree/bindings/usb/generic.txt
> >>> @@ -27,6 +27,9 @@ Optional properties:
> >>> - otg-controller: phandle to otg controller. Host or gadget controllers
> >>> can
> >>> contain this property to link it to a particular OTG
> >>> controller.
> >>> + - hcd-needs-companion: must be present if otg controller is dealing with
> >>> + EHCI host controller that needs a companion OHCI host
> >>> + controller.
> >>>
> >>> This is an attribute to a USB controller such as:
> >>>
> >>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> >>> index 41e762a..83c8c96 100644
> >>> --- a/drivers/usb/common/usb-otg.c
> >>> +++ b/drivers/usb/common/usb-otg.c
> >>> @@ -20,6 +20,7 @@
> >>> #include
> >>> #include
> >>> #include
> >>> +#include
> >>> #include
> >>> #include
> >>> #include
> >>> @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> >>> else
> >>> INIT_WORK(&otg->work, usb_otg_work);
> >>>
> >>> + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> >>> + config->hcd_needs_companion)/* needs comanion ? */
> >>
> >> %s/comanion/companion
> >
> > Thank you for pointing it out!
> >
> > Roger, would you fix this in your v7 patch set?
>
> Yes, I'll fix it locally. You don't need to post it again.
>
Roger, I am ok with this patch.
Acked-by: Peter Chen
Peter
> --
> cheers,
> -roger
>
> >
> >> I have a little puzzled with companion controller and shared hcd, let me
> >> post a topic for it.
> >
> > I looked at the email thread.
> > It is very useful information to me! :)
> >
> > Best regards,
> > Yoshihiro Shimoda
> >
> >> Peter
> >>
> >>> + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> >>> +
> >>> otg->wq = create_singlethread_workqueue("usb_otg");
> >>> if (!otg->wq) {
> >>> dev_err(dev, "otg: %s: can't create workqueue\n",
> >>> @@ -823,13 +828,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> >>> unsigned int irqnum,
> >>> /* HCD will be started by OTG fsm when needed */
> >>> mutex_lock(&otg->fsm.lock);
> >>> if (otg->primary_hcd.hcd) {
> >>> - /* probably a shared HCD ? */
> >>> - if (usb_otg_hcd_is_primary_hcd(hcd)) {
> >>> + /* probably a shared HCD or a companion OHCI HCD ? */
> >>> + if (!(otg->flags & OTG_FL
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
> From: Peter Chen
> Sent: Friday, April 22, 2016 12:34 PM
>
> On Fri, Apr 22, 2016 at 09:26:46AM +0800, Peter Chen wrote:
> > On Fri, Apr 15, 2016 at 10:03:16AM +, Yoshihiro Shimoda wrote:
> > > Hi,
> > >
> > > > From: Yoshihiro Shimoda
> > > > Sent: Friday, April 15, 2016 6:59 PM
> > > >
> > > > Hi,
> > > >
> > > > > From: Roger Quadros
> > > > > Sent: Thursday, April 14, 2016 8:32 PM
> > > > >
> > > > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > > > Hi,
> > > > > >
> > > > < snip >
> > > > > >>> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > > > > >>> unsigned int irqnum,
> > > > > >>>* we're ready only if we have shared HCD
> > > > > >>>* or we don't need shared HCD.
> > > > > >>>*/
> > > > > >>> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> > > > > >>> + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> > > > > >>> + !otg->primary_hcd.hcd->shared_hcd))
> > > > > >>> {
> > > > > >>> otg->host = hcd_to_bus(hcd);
> > > > > >>> /* FIXME: set bus->otg_port if this is true OTG port
> > > > > >>> with HNP */
> > > > > >>>
> > > > > >>
> > > > > >> These changes look good to me. Thanks.
> > > > > >
> > > > > > Thank you for the comment.
> > > > > > If we change the "needs_companion" place to the otg_config,
> > > > > > do we need to add a flag into the otg, instead of otg->caps?
> > > > >
> > > > > Yes we can add a flag in struct usb_otg.
> > > >
> > > > Thank you for the comment.
> > > >
> > > > I made a fixed patch.
> > > > So, should I send this patch to ML after you sent v7 patches?
> > > > Or, would you apply this patch before you send v7 patches?
> > >
> > > Oops, I sent this email without my patch...
> > >
> > > ---
> > > Subject: [PATCH] usb: otg: add hcd companion support
> > >
> > > Since some host controller (e.g. EHCI) needs a companion host controller
> > > (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> > > core.
> > >
> > > Signed-off-by: Yoshihiro Shimoda
> > > ---
> > > Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> > > drivers/usb/common/usb-otg.c | 17 +
> > > include/linux/usb/otg.h | 7 ++-
> > > 3 files changed, 22 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/generic.txt
> > > b/Documentation/devicetree/bindings/usb/generic.txt
> > > index f6866c1..1db1c33 100644
> > > --- a/Documentation/devicetree/bindings/usb/generic.txt
> > > +++ b/Documentation/devicetree/bindings/usb/generic.txt
> > > @@ -27,6 +27,9 @@ Optional properties:
> > > - otg-controller: phandle to otg controller. Host or gadget controllers
> > > can
> > > contain this property to link it to a particular OTG
> > > controller.
> > > + - hcd-needs-companion: must be present if otg controller is dealing with
> > > + EHCI host controller that needs a companion OHCI host
> > > + controller.
> > >
> > > This is an attribute to a USB controller such as:
> > >
> > > diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> > > index 41e762a..83c8c96 100644
> > > --- a/drivers/usb/common/usb-otg.c
> > > +++ b/drivers/usb/common/usb-otg.c
> > > @@ -20,6 +20,7 @@
> > > #include
> > > #include
> > > #include
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> > > else
> > > INIT_WORK(&otg->work, usb_otg_work);
> > >
> > > + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> > > + config->hcd_needs_companion)/* needs comanion ? */
> > > + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> > > +
> > > otg->wq = create_singlethread_workqueue("usb_otg");
> > > if (!otg->wq) {
> > > dev_err(dev, "otg: %s: can't create workqueue\n",
> > > @@ -823,13 +828,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > > unsigned int irqnum,
> > > /* HCD will be started by OTG fsm when needed */
> > > mutex_lock(&otg->fsm.lock);
> > > if (otg->primary_hcd.hcd) {
> > > - /* probably a shared HCD ? */
> > > - if (usb_otg_hcd_is_primary_hcd(hcd)) {
> > > + /* probably a shared HCD or a companion OHCI HCD ? */
> > > + if (!(otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION) &&
> > > + usb_otg_hcd_is_primary_hcd(hcd)) {
> > > dev_err(otg_dev, "otg: primary host already
> > > registered\n");
> > > goto err;
> > > }
> > >
> > > - if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> > > + if (otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION ||
> > > + (hcd->shared_hcd == otg->primary_hcd.hcd)) {
> > > if (otg->shared_hcd.hcd) {
> > > dev_err(otg_dev, "otg: shared host alr
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Fri, Apr 22, 2016 at 09:26:46AM +0800, Peter Chen wrote:
> On Fri, Apr 15, 2016 at 10:03:16AM +, Yoshihiro Shimoda wrote:
> > Hi,
> >
> > > From: Yoshihiro Shimoda
> > > Sent: Friday, April 15, 2016 6:59 PM
> > >
> > > Hi,
> > >
> > > > From: Roger Quadros
> > > > Sent: Thursday, April 14, 2016 8:32 PM
> > > >
> > > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > > Hi,
> > > > >
> > > < snip >
> > > > >>> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > > > >>> unsigned int irqnum,
> > > > >>> * we're ready only if we have shared HCD
> > > > >>> * or we don't need shared HCD.
> > > > >>> */
> > > > >>> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> > > > >>> + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> > > > >>> + !otg->primary_hcd.hcd->shared_hcd))
> > > > >>> {
> > > > >>> otg->host = hcd_to_bus(hcd);
> > > > >>> /* FIXME: set bus->otg_port if this is true OTG port
> > > > >>> with HNP */
> > > > >>>
> > > > >>
> > > > >> These changes look good to me. Thanks.
> > > > >
> > > > > Thank you for the comment.
> > > > > If we change the "needs_companion" place to the otg_config,
> > > > > do we need to add a flag into the otg, instead of otg->caps?
> > > >
> > > > Yes we can add a flag in struct usb_otg.
> > >
> > > Thank you for the comment.
> > >
> > > I made a fixed patch.
> > > So, should I send this patch to ML after you sent v7 patches?
> > > Or, would you apply this patch before you send v7 patches?
> >
> > Oops, I sent this email without my patch...
> >
> > ---
> > Subject: [PATCH] usb: otg: add hcd companion support
> >
> > Since some host controller (e.g. EHCI) needs a companion host controller
> > (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> > core.
> >
> > Signed-off-by: Yoshihiro Shimoda
> > ---
> > Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> > drivers/usb/common/usb-otg.c | 17 +
> > include/linux/usb/otg.h | 7 ++-
> > 3 files changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/generic.txt
> > b/Documentation/devicetree/bindings/usb/generic.txt
> > index f6866c1..1db1c33 100644
> > --- a/Documentation/devicetree/bindings/usb/generic.txt
> > +++ b/Documentation/devicetree/bindings/usb/generic.txt
> > @@ -27,6 +27,9 @@ Optional properties:
> > - otg-controller: phandle to otg controller. Host or gadget controllers
> > can
> > contain this property to link it to a particular OTG
> > controller.
> > + - hcd-needs-companion: must be present if otg controller is dealing with
> > + EHCI host controller that needs a companion OHCI host
> > + controller.
> >
> > This is an attribute to a USB controller such as:
> >
> > diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> > index 41e762a..83c8c96 100644
> > --- a/drivers/usb/common/usb-otg.c
> > +++ b/drivers/usb/common/usb-otg.c
> > @@ -20,6 +20,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> > else
> > INIT_WORK(&otg->work, usb_otg_work);
> >
> > + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> > + config->hcd_needs_companion)/* needs comanion ? */
> > + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> > +
> > otg->wq = create_singlethread_workqueue("usb_otg");
> > if (!otg->wq) {
> > dev_err(dev, "otg: %s: can't create workqueue\n",
> > @@ -823,13 +828,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > unsigned int irqnum,
> > /* HCD will be started by OTG fsm when needed */
> > mutex_lock(&otg->fsm.lock);
> > if (otg->primary_hcd.hcd) {
> > - /* probably a shared HCD ? */
> > - if (usb_otg_hcd_is_primary_hcd(hcd)) {
> > + /* probably a shared HCD or a companion OHCI HCD ? */
> > + if (!(otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION) &&
> > + usb_otg_hcd_is_primary_hcd(hcd)) {
> > dev_err(otg_dev, "otg: primary host already
> > registered\n");
> > goto err;
> > }
> >
> > - if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> > + if (otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION ||
> > + (hcd->shared_hcd == otg->primary_hcd.hcd)) {
> > if (otg->shared_hcd.hcd) {
> > dev_err(otg_dev, "otg: shared host already
> > registered\n");
> > goto err;
> > @@ -865,7 +872,9 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
> > int irqnum,
> > * we're ready only if we have shared HCD
>
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Fri, Apr 15, 2016 at 10:03:16AM +, Yoshihiro Shimoda wrote:
> Hi,
>
> > From: Yoshihiro Shimoda
> > Sent: Friday, April 15, 2016 6:59 PM
> >
> > Hi,
> >
> > > From: Roger Quadros
> > > Sent: Thursday, April 14, 2016 8:32 PM
> > >
> > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > Hi,
> > > >
> > < snip >
> > > >>> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > > >>> unsigned int irqnum,
> > > >>>* we're ready only if we have shared HCD
> > > >>>* or we don't need shared HCD.
> > > >>>*/
> > > >>> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> > > >>> + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> > > >>> + !otg->primary_hcd.hcd->shared_hcd))
> > > >>> {
> > > >>> otg->host = hcd_to_bus(hcd);
> > > >>> /* FIXME: set bus->otg_port if this is true OTG port
> > > >>> with HNP */
> > > >>>
> > > >>
> > > >> These changes look good to me. Thanks.
> > > >
> > > > Thank you for the comment.
> > > > If we change the "needs_companion" place to the otg_config,
> > > > do we need to add a flag into the otg, instead of otg->caps?
> > >
> > > Yes we can add a flag in struct usb_otg.
> >
> > Thank you for the comment.
> >
> > I made a fixed patch.
> > So, should I send this patch to ML after you sent v7 patches?
> > Or, would you apply this patch before you send v7 patches?
>
> Oops, I sent this email without my patch...
>
> ---
> Subject: [PATCH] usb: otg: add hcd companion support
>
> Since some host controller (e.g. EHCI) needs a companion host controller
> (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> core.
>
> Signed-off-by: Yoshihiro Shimoda
> ---
> Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> drivers/usb/common/usb-otg.c | 17 +
> include/linux/usb/otg.h | 7 ++-
> 3 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/generic.txt
> b/Documentation/devicetree/bindings/usb/generic.txt
> index f6866c1..1db1c33 100644
> --- a/Documentation/devicetree/bindings/usb/generic.txt
> +++ b/Documentation/devicetree/bindings/usb/generic.txt
> @@ -27,6 +27,9 @@ Optional properties:
> - otg-controller: phandle to otg controller. Host or gadget controllers can
> contain this property to link it to a particular OTG
> controller.
> + - hcd-needs-companion: must be present if otg controller is dealing with
> + EHCI host controller that needs a companion OHCI host
> + controller.
>
> This is an attribute to a USB controller such as:
>
> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> index 41e762a..83c8c96 100644
> --- a/drivers/usb/common/usb-otg.c
> +++ b/drivers/usb/common/usb-otg.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> else
> INIT_WORK(&otg->work, usb_otg_work);
>
> + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> + config->hcd_needs_companion)/* needs comanion ? */
> + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> +
> otg->wq = create_singlethread_workqueue("usb_otg");
> if (!otg->wq) {
> dev_err(dev, "otg: %s: can't create workqueue\n",
> @@ -823,13 +828,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
> int irqnum,
> /* HCD will be started by OTG fsm when needed */
> mutex_lock(&otg->fsm.lock);
> if (otg->primary_hcd.hcd) {
> - /* probably a shared HCD ? */
> - if (usb_otg_hcd_is_primary_hcd(hcd)) {
> + /* probably a shared HCD or a companion OHCI HCD ? */
> + if (!(otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION) &&
> + usb_otg_hcd_is_primary_hcd(hcd)) {
> dev_err(otg_dev, "otg: primary host already
> registered\n");
> goto err;
> }
>
> - if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> + if (otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION ||
> + (hcd->shared_hcd == otg->primary_hcd.hcd)) {
> if (otg->shared_hcd.hcd) {
> dev_err(otg_dev, "otg: shared host already
> registered\n");
> goto err;
> @@ -865,7 +872,9 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
> int irqnum,
>* we're ready only if we have shared HCD
>* or we don't need shared HCD.
>*/
> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> + if (otg->shared_hcd.hcd ||
> + (!(otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION) &&
> + !
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> It provides APIs for the following tasks
>
> - Registering an OTG/dual-role capable controller
> - Registering Host and Gadget controllers to OTG core
> - Providing inputs to and kicking the OTG state machine
>
> Provide a dual-role device (DRD) state machine.
> DRD mode is a reduced functionality OTG mode. In this mode
> we don't support SRP, HNP and dynamic role-swap.
>
> In DRD operation, the controller mode (Host or Peripheral)
> is decided based on the ID pin status. Once a cable plug (Type-A
> or Type-B) is attached the controller selects the state
> and doesn't change till the cable in unplugged and a different
> cable type is inserted.
>
> As we don't need most of the complex OTG states and OTG timers
> we implement a lean DRD state machine in usb-otg.c.
> The DRD state machine is only interested in 2 hardware inputs
> 'id' and 'b_sess_vld'.
>
> Signed-off-by: Roger Quadros
> ---
> drivers/usb/common/Makefile |2 +-
> drivers/usb/common/usb-otg.c | 1058
> ++
> drivers/usb/common/usb-otg.h | 71 +++
> drivers/usb/core/Kconfig |2 +-
> include/linux/usb/gadget.h |2 +
> include/linux/usb/hcd.h |1 +
> include/linux/usb/otg-fsm.h |7 +
> include/linux/usb/otg.h | 154 +-
> 8 files changed, 1292 insertions(+), 5 deletions(-)
> create mode 100644 drivers/usb/common/usb-otg.c
> create mode 100644 drivers/usb/common/usb-otg.h
>
> diff --git a/drivers/usb/common/Makefile b/drivers/usb/common/Makefile
> index f8f2c88..730d928 100644
> --- a/drivers/usb/common/Makefile
> +++ b/drivers/usb/common/Makefile
> @@ -7,5 +7,5 @@ usb-common-y+= common.o
> usb-common-$(CONFIG_USB_LED_TRIG) += led.o
>
> obj-$(CONFIG_USB_ULPI_BUS) += ulpi.o
> -usbotg-y := usb-otg-fsm.o
> +usbotg-y := usb-otg.o usb-otg-fsm.o
> obj-$(CONFIG_USB_OTG)+= usbotg.o
> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> new file mode 100644
> index 000..41e762a
> --- /dev/null
> +++ b/drivers/usb/common/usb-otg.c
> @@ -0,0 +1,1058 @@
> +/**
> + * drivers/usb/common/usb-otg.c - USB OTG core
> + *
> + * Copyright (C) 2016 Texas Instruments Incorporated - http://www.ti.com
> + * Author: Roger Quadros
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "usb-otg.h"
> +
> +struct otg_gcd {
> + struct usb_gadget *gadget;
> + struct otg_gadget_ops *ops;
> +};
> +
> +/* OTG device list */
> +LIST_HEAD(otg_list);
> +static DEFINE_MUTEX(otg_list_mutex);
> +
> +/* Hosts and Gadgets waiting for OTG controller */
> +struct otg_wait_data {
> + struct device *dev; /* OTG controller device */
> +
> + struct otg_hcd primary_hcd;
> + struct otg_hcd shared_hcd;
> + struct otg_gcd gcd;
> + struct list_head list;
> +};
> +
> +LIST_HEAD(wait_list);
> +static DEFINE_MUTEX(wait_list_mutex);
> +
> +static int usb_otg_hcd_is_primary_hcd(struct usb_hcd *hcd)
> +{
> + if (!hcd->primary_hcd)
> + return 1;
> + return hcd == hcd->primary_hcd;
> +}
Just find there is already a usb_hcd_is_primary_hcd at hcd.c,
would you use it directly?
Peter
> +
> +/**
> + * Check if the OTG device is in our wait list and return
> + * otg_wait_data, else NULL.
> + *
> + * wait_list_mutex must be held.
> + */
> +static struct otg_wait_data *usb_otg_get_wait(struct device *otg_dev)
> +{
> + struct otg_wait_data *wait;
> +
> + if (!otg_dev)
> + return NULL;
> +
> + /* is there an entry for this otg_dev ?*/
> + list_for_each_entry(wait, &wait_list, list) {
> + if (wait->dev == otg_dev)
> + return wait;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * Add the hcd to our wait list
> + */
> +static int usb_otg_hcd_wait_add(struct device *otg_dev, struct usb_hcd *hcd,
> + unsigned int irqnum, unsigned long irqflags,
> + struct otg_hcd_ops *ops)
> +{
> + struct otg_wait_data *wait;
> + int ret = -EINVAL;
> +
> + mutex_lock(&wait_list_mutex);
> +
> + wait = usb_otg_get_wait(otg_dev);
> + if (!wait) {
> + /* Not yet in wait list? allocate and add */
> + wait = kzalloc(sizeof(*wait), GFP_KERNEL);
> + if (!wait) {
> + ret = -ENOMEM;
> + g
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Wed, Apr 20, 2016 at 10:02:33AM +0300, Roger Quadros wrote:
> On 19/04/16 11:06, Peter Chen wrote:
> > On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> >> +/**
> >> + * usb_otg_start_host - start/stop the host controller
> >> + * @otg: usb_otg instance
> >> + * @on: true to start, false to stop
> >> + *
> >> + * Start/stop the USB host controller. This function is meant
> >> + * for use by the OTG controller driver.
> >> + */
> >> +int usb_otg_start_host(struct usb_otg *otg, int on)
> >> +{
> >> + struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
> >> +
> >> + dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
> >> + if (!otg->host) {
> >> + WARN_ONCE(1, "otg: fsm running without host\n");
> >> + return 0;
> >> + }
> >> +
> >> + if (on) {
> >> + if (otg->flags & OTG_FLAG_HOST_RUNNING)
> >> + return 0;
> >> +
> >> + otg->flags |= OTG_FLAG_HOST_RUNNING;
> >> +
> >> + /* start host */
> >> + hcd_ops->add(otg->primary_hcd.hcd, otg->primary_hcd.irqnum,
> >> + otg->primary_hcd.irqflags);
> >> + if (otg->shared_hcd.hcd) {
> >> + hcd_ops->add(otg->shared_hcd.hcd,
> >> + otg->shared_hcd.irqnum,
> >> + otg->shared_hcd.irqflags);
> >> + }
> >
> > Check the return value please.
>
> And what should we do on failure?
> Even if things fail, they could potentially start working on next
> remove/add iteration so I didn't bother checking return values.
>
If usb_add_hcd has failed, the hcd may be released (usb_put_hcd is
called), in that case, we can't call usb_remove_hcd, maybe we may
need to add hcd valid check for primary hcd too. Even we can't
stop fsm, we need to show an error message for user.
Chipidea idea have a bug before:
commit 41314fea2ffb6dc716b7e698a47c68b329602fc0
Author: Russell King - ARM Linux
Date: Wed Oct 16 13:45:15 2013 +0100
usb/chipidea: fix oops on memory allocation failure
--
Best Regards,
Peter Chen
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Wed, Apr 20, 2016 at 09:54:49AM +0300, Roger Quadros wrote:
> On 18/04/16 05:09, Peter Chen wrote:
> > On Fri, Apr 15, 2016 at 02:00:46PM +0300, Roger Quadros wrote:
> >> On 15/04/16 12:25, Peter Chen wrote:
> >>> On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> + * @dev: OTG/dual-role controller device.
> + * @config: OTG configuration.
> + *
> + * Registers the OTG/dual-role controller device with the USB OTG core.
> + *
> + * Return: struct usb_otg * if success, ERR_PTR() if error.
> + */
> +struct usb_otg *usb_otg_register(struct device *dev,
> + struct usb_otg_config *config)
> +{
> +struct usb_otg *otg;
> +struct otg_wait_data *wait;
> +int ret = 0;
> +
> +if (!dev || !config || !config->fsm_ops)
> +return ERR_PTR(-EINVAL);
> +
> +/* already in list? */
> +mutex_lock(&otg_list_mutex);
> +if (usb_otg_get_data(dev)) {
> +dev_err(dev, "otg: %s: device already in otg list\n",
> +__func__);
> +ret = -EINVAL;
> +goto unlock;
> +}
> +
> +/* allocate and add to list */
> +otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> +if (!otg) {
> +ret = -ENOMEM;
> +goto unlock;
> +}
> +
> +otg->dev = dev;
> +otg->caps = config->otg_caps;
> +
> +if ((otg->caps->hnp_support || otg->caps->srp_support ||
> + otg->caps->adp_support) && !config->otg_work)
> +dev_info(dev, "otg: limiting to dual-role\n");
> >>>
> >>> What does above mean? Customized otg_work item may be dual-role,
> >>> may be full otg.
> >>
> >> I'm checking for !config->otg_work so we're sure of using the
> >> default dual-role only work function.
> >>
> >
> > I see. But whether it is dual-role or fully otg should depend on
> > otg->caps, the work item may be different according to design.
> > Besides, your code seems to depend on one of the otg capabilities
> > for dual-role.
> >
> Which capability? Id/vbus status?
>
if (otg->caps->hnp_support || otg->caps->srp_support || otg->caps->adp_support)
it is fully otg;
else
it is dual role;
Do you think so?
--
Best Regards,
Peter Chen
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On 20/04/16 08:08, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Peter Chen
>> Sent: Tuesday, April 19, 2016 6:18 PM
>>
>> On Fri, Apr 15, 2016 at 10:03:16AM +, Yoshihiro Shimoda wrote:
>>> Hi,
>>>
From: Yoshihiro Shimoda
Sent: Friday, April 15, 2016 6:59 PM
Hi,
> From: Roger Quadros
> Sent: Thursday, April 14, 2016 8:32 PM
>
> On 14/04/16 14:15, Yoshihiro Shimoda wrote:
>> Hi,
>>
< snip >
@@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
unsigned int irqnum,
* we're ready only if we have shared HCD
* or we don't need shared HCD.
*/
- if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
+ if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
+ !otg->primary_hcd.hcd->shared_hcd))
{
otg->host = hcd_to_bus(hcd);
/* FIXME: set bus->otg_port if this is true OTG port
with HNP */
>>>
>>> These changes look good to me. Thanks.
>>
>> Thank you for the comment.
>> If we change the "needs_companion" place to the otg_config,
>> do we need to add a flag into the otg, instead of otg->caps?
>
> Yes we can add a flag in struct usb_otg.
Thank you for the comment.
I made a fixed patch.
So, should I send this patch to ML after you sent v7 patches?
Or, would you apply this patch before you send v7 patches?
>>>
>>> Oops, I sent this email without my patch...
>>>
>>> ---
>>> Subject: [PATCH] usb: otg: add hcd companion support
>>>
>>> Since some host controller (e.g. EHCI) needs a companion host controller
>>> (e.g. OHCI), this patch adds such a configuration to use it in the OTG
>>> core.
>>>
>>> Signed-off-by: Yoshihiro Shimoda
>>> ---
>>> Documentation/devicetree/bindings/usb/generic.txt | 3 +++
>>> drivers/usb/common/usb-otg.c | 17 +
>>> include/linux/usb/otg.h | 7 ++-
>>> 3 files changed, 22 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/generic.txt
>>> b/Documentation/devicetree/bindings/usb/generic.txt
>>> index f6866c1..1db1c33 100644
>>> --- a/Documentation/devicetree/bindings/usb/generic.txt
>>> +++ b/Documentation/devicetree/bindings/usb/generic.txt
>>> @@ -27,6 +27,9 @@ Optional properties:
>>> - otg-controller: phandle to otg controller. Host or gadget controllers
>>> can
>>> contain this property to link it to a particular OTG
>>> controller.
>>> + - hcd-needs-companion: must be present if otg controller is dealing with
>>> + EHCI host controller that needs a companion OHCI host
>>> + controller.
>>>
>>> This is an attribute to a USB controller such as:
>>>
>>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>>> index 41e762a..83c8c96 100644
>>> --- a/drivers/usb/common/usb-otg.c
>>> +++ b/drivers/usb/common/usb-otg.c
>>> @@ -20,6 +20,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #include
>>> #include
>>> @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
>>> else
>>> INIT_WORK(&otg->work, usb_otg_work);
>>>
>>> + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
>>> + config->hcd_needs_companion)/* needs comanion ? */
>>
>> %s/comanion/companion
>
> Thank you for pointing it out!
>
> Roger, would you fix this in your v7 patch set?
Yes, I'll fix it locally. You don't need to post it again.
--
cheers,
-roger
>
>> I have a little puzzled with companion controller and shared hcd, let me
>> post a topic for it.
>
> I looked at the email thread.
> It is very useful information to me! :)
>
> Best regards,
> Yoshihiro Shimoda
>
>> Peter
>>
>>> + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
>>> +
>>> otg->wq = create_singlethread_workqueue("usb_otg");
>>> if (!otg->wq) {
>>> dev_err(dev, "otg: %s: can't create workqueue\n",
>>> @@ -823,13 +828,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
>>> unsigned int irqnum,
>>> /* HCD will be started by OTG fsm when needed */
>>> mutex_lock(&otg->fsm.lock);
>>> if (otg->primary_hcd.hcd) {
>>> - /* probably a shared HCD ? */
>>> - if (usb_otg_hcd_is_primary_hcd(hcd)) {
>>> + /* probably a shared HCD or a companion OHCI HCD ? */
>>> + if (!(otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION) &&
>>> + usb_otg_hcd_is_primary_hcd(hcd)) {
>>> dev_err(otg_dev, "otg: primary host already
>>> registered\n");
>>> goto err;
>>> }
>>>
>>> - if (hcd->shared_hcd == otg->primary_hcd.hcd) {
>>> + if (otg->flags & OTG_FLAG_HCD_NEEDS_COMPAN
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On 19/04/16 11:06, Peter Chen wrote:
> On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
>> +/**
>> + * usb_otg_start_host - start/stop the host controller
>> + * @otg:usb_otg instance
>> + * @on: true to start, false to stop
>> + *
>> + * Start/stop the USB host controller. This function is meant
>> + * for use by the OTG controller driver.
>> + */
>> +int usb_otg_start_host(struct usb_otg *otg, int on)
>> +{
>> +struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
>> +
>> +dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
>> +if (!otg->host) {
>> +WARN_ONCE(1, "otg: fsm running without host\n");
>> +return 0;
>> +}
>> +
>> +if (on) {
>> +if (otg->flags & OTG_FLAG_HOST_RUNNING)
>> +return 0;
>> +
>> +otg->flags |= OTG_FLAG_HOST_RUNNING;
>> +
>> +/* start host */
>> +hcd_ops->add(otg->primary_hcd.hcd, otg->primary_hcd.irqnum,
>> + otg->primary_hcd.irqflags);
>> +if (otg->shared_hcd.hcd) {
>> +hcd_ops->add(otg->shared_hcd.hcd,
>> + otg->shared_hcd.irqnum,
>> + otg->shared_hcd.irqflags);
>> +}
>
> Check the return value please.
And what should we do on failure?
Even if things fail, they could potentially start working on next
remove/add iteration so I didn't bother checking return values.
>
>> +} else {
>> +if (!(otg->flags & OTG_FLAG_HOST_RUNNING))
>> +return 0;
>> +
>> +otg->flags &= ~OTG_FLAG_HOST_RUNNING;
>> +
>> +/* stop host */
>> +if (otg->shared_hcd.hcd)
>> +hcd_ops->remove(otg->shared_hcd.hcd);
>> +
>> +hcd_ops->remove(otg->primary_hcd.hcd);
>> +}
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_start_host);
>> +
>> +/**
>> + * usb_otg_start_gadget - start/stop the gadget controller
>> + * @otg:usb_otg instance
>> + * @on: true to start, false to stop
>> + *
>> + * Start/stop the USB gadget controller. This function is meant
>> + * for use by the OTG controller driver.
>> + */
>> +int usb_otg_start_gadget(struct usb_otg *otg, int on)
>> +{
>> +struct usb_gadget *gadget = otg->gadget;
>> +
>> +dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
>> +if (!gadget) {
>> +WARN_ONCE(1, "otg: fsm running without gadget\n");
>> +return 0;
>> +}
>> +
>> +if (on) {
>> +if (otg->flags & OTG_FLAG_GADGET_RUNNING)
>> +return 0;
>> +
>> +otg->flags |= OTG_FLAG_GADGET_RUNNING;
>> +otg->gadget_ops->start(otg->gadget);
>
> ditto
>
>> +} else {
>> +if (!(otg->flags & OTG_FLAG_GADGET_RUNNING))
>> +return 0;
>> +
>> +otg->flags &= ~OTG_FLAG_GADGET_RUNNING;
>> +otg->gadget_ops->stop(otg->gadget);
>> +}
>> +
>> +return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(usb_otg_start_gadget);
>> +
>> +/**
>> + * Change USB protocol when there is a protocol change.
>> + * fsm->lock must be held.
>> + */
>> +static int drd_set_protocol(struct otg_fsm *fsm, int protocol)
>> +{
>> +struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
>> +int ret = 0;
>> +
>> +if (fsm->protocol != protocol) {
>> +dev_dbg(otg->dev, "otg: changing role fsm->protocol= %d; new
>> protocol= %d\n",
>> +fsm->protocol, protocol);
>> +/* stop old protocol */
>> +if (fsm->protocol == PROTO_HOST)
>> +ret = otg_start_host(otg, 0);
>> +else if (fsm->protocol == PROTO_GADGET)
>> +ret = otg_start_gadget(otg, 0);
>> +if (ret)
>> +return ret;
>> +
>> +/* start new protocol */
>> +if (protocol == PROTO_HOST)
>> +ret = otg_start_host(otg, 1);
>> +else if (protocol == PROTO_GADGET)
>> +ret = otg_start_gadget(otg, 1);
>> +if (ret)
>> +return ret;
>> +
>> +fsm->protocol = protocol;
>> +return 0;
>> +}
>> +
>> +return 0;
>> +}
>> +
>> +/**
>> + * Called when entering a DRD state.
>> + * fsm->lock must be held.
>> + */
>> +static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state)
>> +{
>> +struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
>> +
>> +if (otg->state == new_state)
>> +return;
>> +
>> +fsm->state_changed = 1;
>> +dev_dbg(otg->dev, "otg: set state: %s\n",
>> +usb_otg_state_string(new_state));
>> +switch (new_state) {
>> +case OTG_STATE_B_IDLE:
>> +drd_set_protocol(fsm, PROTO_UNDEF);
>> +otg_drv_vbus(otg, 0);
>> +break;
>> +case OTG_STATE_B_PERIPHERAL:
>> +drd_set_protocol(fsm, PROTO_GADGET);
>> +
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On 18/04/16 05:09, Peter Chen wrote:
> On Fri, Apr 15, 2016 at 02:00:46PM +0300, Roger Quadros wrote:
>> On 15/04/16 12:25, Peter Chen wrote:
>>> On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
+ * usb_otg_register() - Register the OTG/dual-role device to OTG core
+ * @dev: OTG/dual-role controller device.
+ * @config: OTG configuration.
+ *
+ * Registers the OTG/dual-role controller device with the USB OTG core.
+ *
+ * Return: struct usb_otg * if success, ERR_PTR() if error.
+ */
+struct usb_otg *usb_otg_register(struct device *dev,
+ struct usb_otg_config *config)
+{
+ struct usb_otg *otg;
+ struct otg_wait_data *wait;
+ int ret = 0;
+
+ if (!dev || !config || !config->fsm_ops)
+ return ERR_PTR(-EINVAL);
+
+ /* already in list? */
+ mutex_lock(&otg_list_mutex);
+ if (usb_otg_get_data(dev)) {
+ dev_err(dev, "otg: %s: device already in otg list\n",
+ __func__);
+ ret = -EINVAL;
+ goto unlock;
+ }
+
+ /* allocate and add to list */
+ otg = kzalloc(sizeof(*otg), GFP_KERNEL);
+ if (!otg) {
+ ret = -ENOMEM;
+ goto unlock;
+ }
+
+ otg->dev = dev;
+ otg->caps = config->otg_caps;
+
+ if ((otg->caps->hnp_support || otg->caps->srp_support ||
+ otg->caps->adp_support) && !config->otg_work)
+ dev_info(dev, "otg: limiting to dual-role\n");
>>>
>>> What does above mean? Customized otg_work item may be dual-role,
>>> may be full otg.
>>
>> I'm checking for !config->otg_work so we're sure of using the
>> default dual-role only work function.
>>
>
> I see. But whether it is dual-role or fully otg should depend on
> otg->caps, the work item may be different according to design.
> Besides, your code seems to depend on one of the otg capabilities
> for dual-role.
>
Which capability? Id/vbus status?
Are you OK with this patch?
cheers,
-roger
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
> From: Peter Chen
> Sent: Tuesday, April 19, 2016 6:18 PM
>
> On Fri, Apr 15, 2016 at 10:03:16AM +, Yoshihiro Shimoda wrote:
> > Hi,
> >
> > > From: Yoshihiro Shimoda
> > > Sent: Friday, April 15, 2016 6:59 PM
> > >
> > > Hi,
> > >
> > > > From: Roger Quadros
> > > > Sent: Thursday, April 14, 2016 8:32 PM
> > > >
> > > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > > Hi,
> > > > >
> > > < snip >
> > > > >>> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > > > >>> unsigned int irqnum,
> > > > >>> * we're ready only if we have shared HCD
> > > > >>> * or we don't need shared HCD.
> > > > >>> */
> > > > >>> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> > > > >>> + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> > > > >>> + !otg->primary_hcd.hcd->shared_hcd))
> > > > >>> {
> > > > >>> otg->host = hcd_to_bus(hcd);
> > > > >>> /* FIXME: set bus->otg_port if this is true OTG port
> > > > >>> with HNP */
> > > > >>>
> > > > >>
> > > > >> These changes look good to me. Thanks.
> > > > >
> > > > > Thank you for the comment.
> > > > > If we change the "needs_companion" place to the otg_config,
> > > > > do we need to add a flag into the otg, instead of otg->caps?
> > > >
> > > > Yes we can add a flag in struct usb_otg.
> > >
> > > Thank you for the comment.
> > >
> > > I made a fixed patch.
> > > So, should I send this patch to ML after you sent v7 patches?
> > > Or, would you apply this patch before you send v7 patches?
> >
> > Oops, I sent this email without my patch...
> >
> > ---
> > Subject: [PATCH] usb: otg: add hcd companion support
> >
> > Since some host controller (e.g. EHCI) needs a companion host controller
> > (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> > core.
> >
> > Signed-off-by: Yoshihiro Shimoda
> > ---
> > Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> > drivers/usb/common/usb-otg.c | 17 +
> > include/linux/usb/otg.h | 7 ++-
> > 3 files changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/generic.txt
> > b/Documentation/devicetree/bindings/usb/generic.txt
> > index f6866c1..1db1c33 100644
> > --- a/Documentation/devicetree/bindings/usb/generic.txt
> > +++ b/Documentation/devicetree/bindings/usb/generic.txt
> > @@ -27,6 +27,9 @@ Optional properties:
> > - otg-controller: phandle to otg controller. Host or gadget controllers
> > can
> > contain this property to link it to a particular OTG
> > controller.
> > + - hcd-needs-companion: must be present if otg controller is dealing with
> > + EHCI host controller that needs a companion OHCI host
> > + controller.
> >
> > This is an attribute to a USB controller such as:
> >
> > diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> > index 41e762a..83c8c96 100644
> > --- a/drivers/usb/common/usb-otg.c
> > +++ b/drivers/usb/common/usb-otg.c
> > @@ -20,6 +20,7 @@
> > #include
> > #include
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> > else
> > INIT_WORK(&otg->work, usb_otg_work);
> >
> > + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> > + config->hcd_needs_companion)/* needs comanion ? */
>
> %s/comanion/companion
Thank you for pointing it out!
Roger, would you fix this in your v7 patch set?
> I have a little puzzled with companion controller and shared hcd, let me
> post a topic for it.
I looked at the email thread.
It is very useful information to me! :)
Best regards,
Yoshihiro Shimoda
> Peter
>
> > + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> > +
> > otg->wq = create_singlethread_workqueue("usb_otg");
> > if (!otg->wq) {
> > dev_err(dev, "otg: %s: can't create workqueue\n",
> > @@ -823,13 +828,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > unsigned int irqnum,
> > /* HCD will be started by OTG fsm when needed */
> > mutex_lock(&otg->fsm.lock);
> > if (otg->primary_hcd.hcd) {
> > - /* probably a shared HCD ? */
> > - if (usb_otg_hcd_is_primary_hcd(hcd)) {
> > + /* probably a shared HCD or a companion OHCI HCD ? */
> > + if (!(otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION) &&
> > + usb_otg_hcd_is_primary_hcd(hcd)) {
> > dev_err(otg_dev, "otg: primary host already
> > registered\n");
> > goto err;
> > }
> >
> > - if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> > + if (otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION ||
> > + (hcd->shared_hcd == otg->primary_hcd.hcd)) {
> >
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Fri, Apr 15, 2016 at 10:03:16AM +, Yoshihiro Shimoda wrote:
> Hi,
>
> > From: Yoshihiro Shimoda
> > Sent: Friday, April 15, 2016 6:59 PM
> >
> > Hi,
> >
> > > From: Roger Quadros
> > > Sent: Thursday, April 14, 2016 8:32 PM
> > >
> > > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > > Hi,
> > > >
> > < snip >
> > > >>> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > > >>> unsigned int irqnum,
> > > >>>* we're ready only if we have shared HCD
> > > >>>* or we don't need shared HCD.
> > > >>>*/
> > > >>> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> > > >>> + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> > > >>> + !otg->primary_hcd.hcd->shared_hcd))
> > > >>> {
> > > >>> otg->host = hcd_to_bus(hcd);
> > > >>> /* FIXME: set bus->otg_port if this is true OTG port
> > > >>> with HNP */
> > > >>>
> > > >>
> > > >> These changes look good to me. Thanks.
> > > >
> > > > Thank you for the comment.
> > > > If we change the "needs_companion" place to the otg_config,
> > > > do we need to add a flag into the otg, instead of otg->caps?
> > >
> > > Yes we can add a flag in struct usb_otg.
> >
> > Thank you for the comment.
> >
> > I made a fixed patch.
> > So, should I send this patch to ML after you sent v7 patches?
> > Or, would you apply this patch before you send v7 patches?
>
> Oops, I sent this email without my patch...
>
> ---
> Subject: [PATCH] usb: otg: add hcd companion support
>
> Since some host controller (e.g. EHCI) needs a companion host controller
> (e.g. OHCI), this patch adds such a configuration to use it in the OTG
> core.
>
> Signed-off-by: Yoshihiro Shimoda
> ---
> Documentation/devicetree/bindings/usb/generic.txt | 3 +++
> drivers/usb/common/usb-otg.c | 17 +
> include/linux/usb/otg.h | 7 ++-
> 3 files changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/generic.txt
> b/Documentation/devicetree/bindings/usb/generic.txt
> index f6866c1..1db1c33 100644
> --- a/Documentation/devicetree/bindings/usb/generic.txt
> +++ b/Documentation/devicetree/bindings/usb/generic.txt
> @@ -27,6 +27,9 @@ Optional properties:
> - otg-controller: phandle to otg controller. Host or gadget controllers can
> contain this property to link it to a particular OTG
> controller.
> + - hcd-needs-companion: must be present if otg controller is dealing with
> + EHCI host controller that needs a companion OHCI host
> + controller.
>
> This is an attribute to a USB controller such as:
>
> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> index 41e762a..83c8c96 100644
> --- a/drivers/usb/common/usb-otg.c
> +++ b/drivers/usb/common/usb-otg.c
> @@ -20,6 +20,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
> else
> INIT_WORK(&otg->work, usb_otg_work);
>
> + if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
> + config->hcd_needs_companion)/* needs comanion ? */
%s/comanion/companion
I have a little puzzled with companion controller and shared hcd, let me
post a topic for it.
Peter
> + otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
> +
> otg->wq = create_singlethread_workqueue("usb_otg");
> if (!otg->wq) {
> dev_err(dev, "otg: %s: can't create workqueue\n",
> @@ -823,13 +828,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
> int irqnum,
> /* HCD will be started by OTG fsm when needed */
> mutex_lock(&otg->fsm.lock);
> if (otg->primary_hcd.hcd) {
> - /* probably a shared HCD ? */
> - if (usb_otg_hcd_is_primary_hcd(hcd)) {
> + /* probably a shared HCD or a companion OHCI HCD ? */
> + if (!(otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION) &&
> + usb_otg_hcd_is_primary_hcd(hcd)) {
> dev_err(otg_dev, "otg: primary host already
> registered\n");
> goto err;
> }
>
> - if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> + if (otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION ||
> + (hcd->shared_hcd == otg->primary_hcd.hcd)) {
> if (otg->shared_hcd.hcd) {
> dev_err(otg_dev, "otg: shared host already
> registered\n");
> goto err;
> @@ -865,7 +872,9 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
> int irqnum,
>* we're ready only if we have shared HCD
>* or we don't need shared HCD.
>*/
> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> +/**
> + * usb_otg_start_host - start/stop the host controller
> + * @otg: usb_otg instance
> + * @on: true to start, false to stop
> + *
> + * Start/stop the USB host controller. This function is meant
> + * for use by the OTG controller driver.
> + */
> +int usb_otg_start_host(struct usb_otg *otg, int on)
> +{
> + struct otg_hcd_ops *hcd_ops = otg->hcd_ops;
> +
> + dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
> + if (!otg->host) {
> + WARN_ONCE(1, "otg: fsm running without host\n");
> + return 0;
> + }
> +
> + if (on) {
> + if (otg->flags & OTG_FLAG_HOST_RUNNING)
> + return 0;
> +
> + otg->flags |= OTG_FLAG_HOST_RUNNING;
> +
> + /* start host */
> + hcd_ops->add(otg->primary_hcd.hcd, otg->primary_hcd.irqnum,
> + otg->primary_hcd.irqflags);
> + if (otg->shared_hcd.hcd) {
> + hcd_ops->add(otg->shared_hcd.hcd,
> + otg->shared_hcd.irqnum,
> + otg->shared_hcd.irqflags);
> + }
Check the return value please.
> + } else {
> + if (!(otg->flags & OTG_FLAG_HOST_RUNNING))
> + return 0;
> +
> + otg->flags &= ~OTG_FLAG_HOST_RUNNING;
> +
> + /* stop host */
> + if (otg->shared_hcd.hcd)
> + hcd_ops->remove(otg->shared_hcd.hcd);
> +
> + hcd_ops->remove(otg->primary_hcd.hcd);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_start_host);
> +
> +/**
> + * usb_otg_start_gadget - start/stop the gadget controller
> + * @otg: usb_otg instance
> + * @on: true to start, false to stop
> + *
> + * Start/stop the USB gadget controller. This function is meant
> + * for use by the OTG controller driver.
> + */
> +int usb_otg_start_gadget(struct usb_otg *otg, int on)
> +{
> + struct usb_gadget *gadget = otg->gadget;
> +
> + dev_dbg(otg->dev, "otg: %s %d\n", __func__, on);
> + if (!gadget) {
> + WARN_ONCE(1, "otg: fsm running without gadget\n");
> + return 0;
> + }
> +
> + if (on) {
> + if (otg->flags & OTG_FLAG_GADGET_RUNNING)
> + return 0;
> +
> + otg->flags |= OTG_FLAG_GADGET_RUNNING;
> + otg->gadget_ops->start(otg->gadget);
ditto
> + } else {
> + if (!(otg->flags & OTG_FLAG_GADGET_RUNNING))
> + return 0;
> +
> + otg->flags &= ~OTG_FLAG_GADGET_RUNNING;
> + otg->gadget_ops->stop(otg->gadget);
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_start_gadget);
> +
> +/**
> + * Change USB protocol when there is a protocol change.
> + * fsm->lock must be held.
> + */
> +static int drd_set_protocol(struct otg_fsm *fsm, int protocol)
> +{
> + struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
> + int ret = 0;
> +
> + if (fsm->protocol != protocol) {
> + dev_dbg(otg->dev, "otg: changing role fsm->protocol= %d; new
> protocol= %d\n",
> + fsm->protocol, protocol);
> + /* stop old protocol */
> + if (fsm->protocol == PROTO_HOST)
> + ret = otg_start_host(otg, 0);
> + else if (fsm->protocol == PROTO_GADGET)
> + ret = otg_start_gadget(otg, 0);
> + if (ret)
> + return ret;
> +
> + /* start new protocol */
> + if (protocol == PROTO_HOST)
> + ret = otg_start_host(otg, 1);
> + else if (protocol == PROTO_GADGET)
> + ret = otg_start_gadget(otg, 1);
> + if (ret)
> + return ret;
> +
> + fsm->protocol = protocol;
> + return 0;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * Called when entering a DRD state.
> + * fsm->lock must be held.
> + */
> +static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state)
> +{
> + struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
> +
> + if (otg->state == new_state)
> + return;
> +
> + fsm->state_changed = 1;
> + dev_dbg(otg->dev, "otg: set state: %s\n",
> + usb_otg_state_string(new_state));
> + switch (new_state) {
> + case OTG_STATE_B_IDLE:
> + drd_set_protocol(fsm, PROTO_UNDEF);
> + otg_drv_vbus(otg, 0);
> + break;
> + case OTG_STATE_B_PERIPHERAL:
> + drd_set_protocol(fsm, PROTO_GADGET);
> + otg_drv_vbus(otg, 0);
> + break;
> + case OTG_STATE_A_HOST:
> + drd_set_protocol(fsm, PROTO_HOST);
> + otg_drv_vbus(otg, 1);
> + break;
> + case OTG_STATE_UNDEFINED:
> + case OTG_STATE_B_SRP_INIT:
> +
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Fri, Apr 15, 2016 at 02:00:46PM +0300, Roger Quadros wrote:
> On 15/04/16 12:25, Peter Chen wrote:
> > On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> >> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> >> + * @dev: OTG/dual-role controller device.
> >> + * @config: OTG configuration.
> >> + *
> >> + * Registers the OTG/dual-role controller device with the USB OTG core.
> >> + *
> >> + * Return: struct usb_otg * if success, ERR_PTR() if error.
> >> + */
> >> +struct usb_otg *usb_otg_register(struct device *dev,
> >> + struct usb_otg_config *config)
> >> +{
> >> + struct usb_otg *otg;
> >> + struct otg_wait_data *wait;
> >> + int ret = 0;
> >> +
> >> + if (!dev || !config || !config->fsm_ops)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + /* already in list? */
> >> + mutex_lock(&otg_list_mutex);
> >> + if (usb_otg_get_data(dev)) {
> >> + dev_err(dev, "otg: %s: device already in otg list\n",
> >> + __func__);
> >> + ret = -EINVAL;
> >> + goto unlock;
> >> + }
> >> +
> >> + /* allocate and add to list */
> >> + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> >> + if (!otg) {
> >> + ret = -ENOMEM;
> >> + goto unlock;
> >> + }
> >> +
> >> + otg->dev = dev;
> >> + otg->caps = config->otg_caps;
> >> +
> >> + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> >> + otg->caps->adp_support) && !config->otg_work)
> >> + dev_info(dev, "otg: limiting to dual-role\n");
> >
> > What does above mean? Customized otg_work item may be dual-role,
> > may be full otg.
>
> I'm checking for !config->otg_work so we're sure of using the
> default dual-role only work function.
>
I see. But whether it is dual-role or fully otg should depend on
otg->caps, the work item may be different according to design.
Besides, your code seems to depend on one of the otg capabilities
for dual-role.
--
Best Regards,
Peter Chen
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On 15/04/16 12:25, Peter Chen wrote:
> On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
>> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
>> + * @dev: OTG/dual-role controller device.
>> + * @config: OTG configuration.
>> + *
>> + * Registers the OTG/dual-role controller device with the USB OTG core.
>> + *
>> + * Return: struct usb_otg * if success, ERR_PTR() if error.
>> + */
>> +struct usb_otg *usb_otg_register(struct device *dev,
>> + struct usb_otg_config *config)
>> +{
>> +struct usb_otg *otg;
>> +struct otg_wait_data *wait;
>> +int ret = 0;
>> +
>> +if (!dev || !config || !config->fsm_ops)
>> +return ERR_PTR(-EINVAL);
>> +
>> +/* already in list? */
>> +mutex_lock(&otg_list_mutex);
>> +if (usb_otg_get_data(dev)) {
>> +dev_err(dev, "otg: %s: device already in otg list\n",
>> +__func__);
>> +ret = -EINVAL;
>> +goto unlock;
>> +}
>> +
>> +/* allocate and add to list */
>> +otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>> +if (!otg) {
>> +ret = -ENOMEM;
>> +goto unlock;
>> +}
>> +
>> +otg->dev = dev;
>> +otg->caps = config->otg_caps;
>> +
>> +if ((otg->caps->hnp_support || otg->caps->srp_support ||
>> + otg->caps->adp_support) && !config->otg_work)
>> +dev_info(dev, "otg: limiting to dual-role\n");
>
> What does above mean? Customized otg_work item may be dual-role,
> may be full otg.
I'm checking for !config->otg_work so we're sure of using the
default dual-role only work function.
>
>> +
>> +if (config->otg_work) /* custom otg_work ? */
>> +INIT_WORK(&otg->work, config->otg_work);
>> +else
>> +INIT_WORK(&otg->work, usb_otg_work);
>> +
>> +otg->wq = create_singlethread_workqueue("usb_otg");
>> +if (!otg->wq) {
>> +dev_err(dev, "otg: %s: can't create workqueue\n",
>> +__func__);
>> +ret = -ENOMEM;
>> +goto err_wq;
>> +}
>
> I found a bug that caused by non-freezable workqueue, change it
> as freezable please.
>
> https://marc.ttias.be/linux-stable/2016-03/msg00723.php
OK.
>
> I will review the whole set from next week.
Thanks.
--
cheers,
-roger
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On 15/04/16 12:59, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Roger Quadros
>> Sent: Thursday, April 14, 2016 8:32 PM
>>
>> On 14/04/16 14:15, Yoshihiro Shimoda wrote:
>>> Hi,
>>>
> < snip >
> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> unsigned int irqnum,
>* we're ready only if we have shared HCD
>* or we don't need shared HCD.
>*/
> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> + !otg->primary_hcd.hcd->shared_hcd)) {
> otg->host = hcd_to_bus(hcd);
> /* FIXME: set bus->otg_port if this is true OTG port with HNP */
>
These changes look good to me. Thanks.
>>>
>>> Thank you for the comment.
>>> If we change the "needs_companion" place to the otg_config,
>>> do we need to add a flag into the otg, instead of otg->caps?
>>
>> Yes we can add a flag in struct usb_otg.
>
> Thank you for the comment.
>
> I made a fixed patch.
> So, should I send this patch to ML after you sent v7 patches?
> Or, would you apply this patch before you send v7 patches?
>
Thanks for working on this and testing the series. I will pick your patch
as part of v7.
cheers,
-roger
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
> From: Yoshihiro Shimoda
> Sent: Friday, April 15, 2016 6:59 PM
>
> Hi,
>
> > From: Roger Quadros
> > Sent: Thursday, April 14, 2016 8:32 PM
> >
> > On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > > Hi,
> > >
> < snip >
> > >>> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > >>> unsigned int irqnum,
> > >>> * we're ready only if we have shared HCD
> > >>> * or we don't need shared HCD.
> > >>> */
> > >>> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> > >>> + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> > >>> + !otg->primary_hcd.hcd->shared_hcd))
> > >>> {
> > >>> otg->host = hcd_to_bus(hcd);
> > >>> /* FIXME: set bus->otg_port if this is true OTG port
> > >>> with HNP */
> > >>>
> > >>
> > >> These changes look good to me. Thanks.
> > >
> > > Thank you for the comment.
> > > If we change the "needs_companion" place to the otg_config,
> > > do we need to add a flag into the otg, instead of otg->caps?
> >
> > Yes we can add a flag in struct usb_otg.
>
> Thank you for the comment.
>
> I made a fixed patch.
> So, should I send this patch to ML after you sent v7 patches?
> Or, would you apply this patch before you send v7 patches?
Oops, I sent this email without my patch...
---
Subject: [PATCH] usb: otg: add hcd companion support
Since some host controller (e.g. EHCI) needs a companion host controller
(e.g. OHCI), this patch adds such a configuration to use it in the OTG
core.
Signed-off-by: Yoshihiro Shimoda
---
Documentation/devicetree/bindings/usb/generic.txt | 3 +++
drivers/usb/common/usb-otg.c | 17 +
include/linux/usb/otg.h | 7 ++-
3 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/generic.txt
b/Documentation/devicetree/bindings/usb/generic.txt
index f6866c1..1db1c33 100644
--- a/Documentation/devicetree/bindings/usb/generic.txt
+++ b/Documentation/devicetree/bindings/usb/generic.txt
@@ -27,6 +27,9 @@ Optional properties:
- otg-controller: phandle to otg controller. Host or gadget controllers can
contain this property to link it to a particular OTG
controller.
+ - hcd-needs-companion: must be present if otg controller is dealing with
+ EHCI host controller that needs a companion OHCI host
+ controller.
This is an attribute to a USB controller such as:
diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
index 41e762a..83c8c96 100644
--- a/drivers/usb/common/usb-otg.c
+++ b/drivers/usb/common/usb-otg.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -600,6 +601,10 @@ struct usb_otg *usb_otg_register(struct device *dev,
else
INIT_WORK(&otg->work, usb_otg_work);
+ if (of_find_property(dev->of_node, "hcd-needs-companion", NULL) ||
+ config->hcd_needs_companion)/* needs comanion ? */
+ otg->flags |= OTG_FLAG_HCD_NEEDS_COMPANION;
+
otg->wq = create_singlethread_workqueue("usb_otg");
if (!otg->wq) {
dev_err(dev, "otg: %s: can't create workqueue\n",
@@ -823,13 +828,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
int irqnum,
/* HCD will be started by OTG fsm when needed */
mutex_lock(&otg->fsm.lock);
if (otg->primary_hcd.hcd) {
- /* probably a shared HCD ? */
- if (usb_otg_hcd_is_primary_hcd(hcd)) {
+ /* probably a shared HCD or a companion OHCI HCD ? */
+ if (!(otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION) &&
+ usb_otg_hcd_is_primary_hcd(hcd)) {
dev_err(otg_dev, "otg: primary host already
registered\n");
goto err;
}
- if (hcd->shared_hcd == otg->primary_hcd.hcd) {
+ if (otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION ||
+ (hcd->shared_hcd == otg->primary_hcd.hcd)) {
if (otg->shared_hcd.hcd) {
dev_err(otg_dev, "otg: shared host already
registered\n");
goto err;
@@ -865,7 +872,9 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int
irqnum,
* we're ready only if we have shared HCD
* or we don't need shared HCD.
*/
- if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
+ if (otg->shared_hcd.hcd ||
+ (!(otg->flags & OTG_FLAG_HCD_NEEDS_COMPANION) &&
+!otg->primary_hcd.hcd->shared_hcd)) {
otg->host = hcd_to_bus(hcd);
/* FIXME: set bus->otg_port if this is true OTG port with HNP */
diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
index
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
> From: Roger Quadros
> Sent: Thursday, April 14, 2016 8:32 PM
>
> On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> > Hi,
> >
< snip >
> >>> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> >>> unsigned int irqnum,
> >>>* we're ready only if we have shared HCD
> >>>* or we don't need shared HCD.
> >>>*/
> >>> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> >>> + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> >>> + !otg->primary_hcd.hcd->shared_hcd)) {
> >>> otg->host = hcd_to_bus(hcd);
> >>> /* FIXME: set bus->otg_port if this is true OTG port with HNP */
> >>>
> >>
> >> These changes look good to me. Thanks.
> >
> > Thank you for the comment.
> > If we change the "needs_companion" place to the otg_config,
> > do we need to add a flag into the otg, instead of otg->caps?
>
> Yes we can add a flag in struct usb_otg.
Thank you for the comment.
I made a fixed patch.
So, should I send this patch to ML after you sent v7 patches?
Or, would you apply this patch before you send v7 patches?
Best regards,
Yoshihiro Shimoda
---
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On Tue, Apr 05, 2016 at 05:05:12PM +0300, Roger Quadros wrote:
> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> + * @dev: OTG/dual-role controller device.
> + * @config: OTG configuration.
> + *
> + * Registers the OTG/dual-role controller device with the USB OTG core.
> + *
> + * Return: struct usb_otg * if success, ERR_PTR() if error.
> + */
> +struct usb_otg *usb_otg_register(struct device *dev,
> + struct usb_otg_config *config)
> +{
> + struct usb_otg *otg;
> + struct otg_wait_data *wait;
> + int ret = 0;
> +
> + if (!dev || !config || !config->fsm_ops)
> + return ERR_PTR(-EINVAL);
> +
> + /* already in list? */
> + mutex_lock(&otg_list_mutex);
> + if (usb_otg_get_data(dev)) {
> + dev_err(dev, "otg: %s: device already in otg list\n",
> + __func__);
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + /* allocate and add to list */
> + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> + if (!otg) {
> + ret = -ENOMEM;
> + goto unlock;
> + }
> +
> + otg->dev = dev;
> + otg->caps = config->otg_caps;
> +
> + if ((otg->caps->hnp_support || otg->caps->srp_support ||
> + otg->caps->adp_support) && !config->otg_work)
> + dev_info(dev, "otg: limiting to dual-role\n");
What does above mean? Customized otg_work item may be dual-role,
may be full otg.
> +
> + if (config->otg_work) /* custom otg_work ? */
> + INIT_WORK(&otg->work, config->otg_work);
> + else
> + INIT_WORK(&otg->work, usb_otg_work);
> +
> + otg->wq = create_singlethread_workqueue("usb_otg");
> + if (!otg->wq) {
> + dev_err(dev, "otg: %s: can't create workqueue\n",
> + __func__);
> + ret = -ENOMEM;
> + goto err_wq;
> + }
I found a bug that caused by non-freezable workqueue, change it
as freezable please.
https://marc.ttias.be/linux-stable/2016-03/msg00723.php
I will review the whole set from next week.
Peter
> +
> + /* set otg ops */
> + otg->fsm.ops = config->fsm_ops;
> +
> + mutex_init(&otg->fsm.lock);
> +
> + list_add_tail(&otg->list, &otg_list);
> + mutex_unlock(&otg_list_mutex);
> +
> + /* were we in wait list? */
> + mutex_lock(&wait_list_mutex);
> + wait = usb_otg_get_wait(dev);
> + mutex_unlock(&wait_list_mutex);
> + if (wait) {
> + /* register pending host/gadget and flush from list */
> + usb_otg_flush_wait(dev);
> + }
> +
> + return otg;
> +
> +err_wq:
> + kfree(otg);
> +unlock:
> + mutex_unlock(&otg_list_mutex);
> + return ERR_PTR(ret);
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_register);
> +
> +/**
> + * usb_otg_unregister() - Unregister the OTG/dual-role device from USB OTG
> core
> + * @dev: OTG controller device.
> + *
> + * Unregisters the OTG/dual-role controller device from USB OTG core.
> + * Prevents unregistering till both the associated Host and Gadget
> controllers
> + * have unregistered from the OTG core.
> + *
> + * Return: 0 on success, error value otherwise.
> + */
> +int usb_otg_unregister(struct device *dev)
> +{
> + struct usb_otg *otg;
> +
> + mutex_lock(&otg_list_mutex);
> + otg = usb_otg_get_data(dev);
> + if (!otg) {
> + dev_err(dev, "otg: %s: device not in otg list\n",
> + __func__);
> + mutex_unlock(&otg_list_mutex);
> + return -EINVAL;
> + }
> +
> + /* prevent unregister till both host & gadget have unregistered */
> + if (otg->host || otg->gadget) {
> + dev_err(dev, "otg: %s: host/gadget still registered\n",
> + __func__);
> + return -EBUSY;
> + }
> +
> + /* OTG FSM is halted when host/gadget unregistered */
> + destroy_workqueue(otg->wq);
> +
> + /* remove from otg list */
> + list_del(&otg->list);
> + kfree(otg);
> + mutex_unlock(&otg_list_mutex);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(usb_otg_unregister);
> +
> +/**
> + * start/kick the OTG FSM if we can
> + * fsm->lock must be held
> + */
> +static void usb_otg_start_fsm(struct usb_otg *otg)
> +{
> + struct otg_fsm *fsm = &otg->fsm;
> +
> + if (fsm->running)
> + goto kick_fsm;
> +
> + if (!otg->host) {
> + dev_info(otg->dev, "otg: can't start till host registers\n");
> + return;
> + }
> +
> + if (!otg->gadget) {
> + dev_info(otg->dev, "otg: can't start till gadget registers\n");
> + return;
> + }
> +
> + fsm->running = true;
> +kick_fsm:
> + queue_work(otg->wq, &otg->work);
> +}
> +
> +/**
> + * stop the OTG FSM. Stops Host & Gadget controllers as well.
> + * fsm->lock must be held
> + */
> +static void usb_otg_stop_fsm(struct usb_otg *otg)
> +{
> + struct otg_fsm *fsm = &otg->fsm;
> +
> + if (!fsm->
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On 14/04/16 14:15, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Roger Quadros
>> Sent: Thursday, April 14, 2016 8:00 PM
>>
>> On 14/04/16 11:36, Yoshihiro Shimoda wrote:
>>> Hi,
>>>
> < snip >
>>> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
>>> index e3d0161..8b74715 100644
>>> --- a/drivers/usb/common/common.c
>>> +++ b/drivers/usb/common/common.c
>>> @@ -233,6 +233,8 @@ int of_usb_update_otg_caps(struct device_node *np,
>>> if (of_find_property(np, "adp-disable", NULL) ||
>>> (otg_caps->otg_rev < 0x0200))
>>> otg_caps->adp_support = false;
>>> + if (of_find_property(np, "hcd-needs-companion", NULL))
>>> + otg_caps->needs_companion = true;
>>
>> I'm not sure if otg_caps structure is a right place for this. Maybe Peter
>> can confirm
>> if this is OK or not.
>>
>> I was thinking more about adding this bit in the otg_config structure.
>
> I see. I also think the otg_config is more suitable.
>
>>>
>>> return 0;
>>> }
>>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>>> index 41e762a..e0df839 100644
>>> --- a/drivers/usb/common/usb-otg.c
>>> +++ b/drivers/usb/common/usb-otg.c
>>> @@ -823,13 +823,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
>>> unsigned int irqnum,
>>> /* HCD will be started by OTG fsm when needed */
>>> mutex_lock(&otg->fsm.lock);
>>> if (otg->primary_hcd.hcd) {
>>> - /* probably a shared HCD ? */
>>> - if (usb_otg_hcd_is_primary_hcd(hcd)) {
>>> + /* probably a shared HCD or a companion OHCI HCD ? */
>>> + if (!otg->caps->needs_companion &&
>>> + usb_otg_hcd_is_primary_hcd(hcd)) {
>>> dev_err(otg_dev, "otg: primary host already
>>> registered\n");
>>> goto err;
>>> }
>>>
>>> - if (hcd->shared_hcd == otg->primary_hcd.hcd) {
>>> + if (otg->caps->needs_companion ||
>>> + (hcd->shared_hcd == otg->primary_hcd.hcd)) {
>>> if (otg->shared_hcd.hcd) {
>>> dev_err(otg_dev, "otg: shared host already
>>> registered\n");
>>> goto err;
>>> @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
>>> int irqnum,
>>> * we're ready only if we have shared HCD
>>> * or we don't need shared HCD.
>>> */
>>> - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
>>> + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
>>> + !otg->primary_hcd.hcd->shared_hcd)) {
>>> otg->host = hcd_to_bus(hcd);
>>> /* FIXME: set bus->otg_port if this is true OTG port with HNP */
>>>
>>
>> These changes look good to me. Thanks.
>
> Thank you for the comment.
> If we change the "needs_companion" place to the otg_config,
> do we need to add a flag into the otg, instead of otg->caps?
Yes we can add a flag in struct usb_otg.
cheers,
-roger
>
>>> diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
>>> index b094352..64a7db8 100644
>>> --- a/include/linux/usb/otg.h
>>> +++ b/include/linux/usb/otg.h
>>> @@ -112,12 +112,14 @@ struct usb_otg {
>>> * @hnp_support: Indicates if the device supports HNP.
>>> * @srp_support: Indicates if the device supports SRP.
>>> * @adp_support: Indicates if the device supports ADP.
>>> + * @needs_companion: Indicates if the device needs a companion controller.
>>
>> Description is not exact. How about this.
>> "Indicates if host controller needs a companion controller"
>>
>> Is hcd_needs_companion is better than just needs_companion?
>
> I agree with you.
>
> So, I will modify my local patch and test it tomorrow.
>
> Best regards,
> Yoshihiro Shimoda
>
>>
>>> */
>>> struct usb_otg_caps {
>>> u16 otg_rev;
>>> bool hnp_support;
>>> bool srp_support;
>>> bool adp_support;
>>> + bool needs_companion;
>>> };
>>>
>>> /**
>>>
>>
>> cheers,
>> -roger
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
> From: Roger Quadros
> Sent: Thursday, April 14, 2016 8:00 PM
>
> On 14/04/16 11:36, Yoshihiro Shimoda wrote:
> > Hi,
> >
< snip >
> > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
> > index e3d0161..8b74715 100644
> > --- a/drivers/usb/common/common.c
> > +++ b/drivers/usb/common/common.c
> > @@ -233,6 +233,8 @@ int of_usb_update_otg_caps(struct device_node *np,
> > if (of_find_property(np, "adp-disable", NULL) ||
> > (otg_caps->otg_rev < 0x0200))
> > otg_caps->adp_support = false;
> > + if (of_find_property(np, "hcd-needs-companion", NULL))
> > + otg_caps->needs_companion = true;
>
> I'm not sure if otg_caps structure is a right place for this. Maybe Peter can
> confirm
> if this is OK or not.
>
> I was thinking more about adding this bit in the otg_config structure.
I see. I also think the otg_config is more suitable.
> >
> > return 0;
> > }
> > diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> > index 41e762a..e0df839 100644
> > --- a/drivers/usb/common/usb-otg.c
> > +++ b/drivers/usb/common/usb-otg.c
> > @@ -823,13 +823,15 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > unsigned int irqnum,
> > /* HCD will be started by OTG fsm when needed */
> > mutex_lock(&otg->fsm.lock);
> > if (otg->primary_hcd.hcd) {
> > - /* probably a shared HCD ? */
> > - if (usb_otg_hcd_is_primary_hcd(hcd)) {
> > + /* probably a shared HCD or a companion OHCI HCD ? */
> > + if (!otg->caps->needs_companion &&
> > + usb_otg_hcd_is_primary_hcd(hcd)) {
> > dev_err(otg_dev, "otg: primary host already
> > registered\n");
> > goto err;
> > }
> >
> > - if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> > + if (otg->caps->needs_companion ||
> > + (hcd->shared_hcd == otg->primary_hcd.hcd)) {
> > if (otg->shared_hcd.hcd) {
> > dev_err(otg_dev, "otg: shared host already
> > registered\n");
> > goto err;
> > @@ -865,7 +867,8 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
> > int irqnum,
> > * we're ready only if we have shared HCD
> > * or we don't need shared HCD.
> > */
> > - if (otg->shared_hcd.hcd || !otg->primary_hcd.hcd->shared_hcd) {
> > + if (otg->shared_hcd.hcd || (!otg->caps->needs_companion &&
> > + !otg->primary_hcd.hcd->shared_hcd)) {
> > otg->host = hcd_to_bus(hcd);
> > /* FIXME: set bus->otg_port if this is true OTG port with HNP */
> >
>
> These changes look good to me. Thanks.
Thank you for the comment.
If we change the "needs_companion" place to the otg_config,
do we need to add a flag into the otg, instead of otg->caps?
> > diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
> > index b094352..64a7db8 100644
> > --- a/include/linux/usb/otg.h
> > +++ b/include/linux/usb/otg.h
> > @@ -112,12 +112,14 @@ struct usb_otg {
> > * @hnp_support: Indicates if the device supports HNP.
> > * @srp_support: Indicates if the device supports SRP.
> > * @adp_support: Indicates if the device supports ADP.
> > + * @needs_companion: Indicates if the device needs a companion controller.
>
> Description is not exact. How about this.
> "Indicates if host controller needs a companion controller"
>
> Is hcd_needs_companion is better than just needs_companion?
I agree with you.
So, I will modify my local patch and test it tomorrow.
Best regards,
Yoshihiro Shimoda
>
> > */
> > struct usb_otg_caps {
> > u16 otg_rev;
> > bool hnp_support;
> > bool srp_support;
> > bool adp_support;
> > + bool needs_companion;
> > };
> >
> > /**
> >
>
> cheers,
> -roger
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On 14/04/16 11:36, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Roger Quadros
>> Sent: Monday, April 11, 2016 7:55 PM
>>
>> On 08/04/16 14:22, Yoshihiro Shimoda wrote:
>>> Hi,
>>>
From: Roger Quadros
Sent: Thursday, April 07, 2016 8:45 PM
Hi,
On 07/04/16 11:52, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Roger Quadros
>> Sent: Tuesday, April 05, 2016 11:05 PM
>>> < snip >
> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> index 41e762a..4d7f043 100644
> --- a/drivers/usb/common/usb-otg.c
> +++ b/drivers/usb/common/usb-otg.c
> @@ -825,11 +825,16 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> unsigned int irqnum,
> if (otg->primary_hcd.hcd) {
> /* probably a shared HCD ? */
> if (usb_otg_hcd_is_primary_hcd(hcd)) {
> + if (hcd->driver->flags & HCD_USB11) {
> + dev_info(otg_dev, "this assumes usb 1.1 hc is
> as shared_hcd\n");
> + goto check_shared_hcd;
> + }
> dev_err(otg_dev, "otg: primary host already
> registered\n");
> goto err;
> }
>
> if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> +check_shared_hcd:
> if (otg->shared_hcd.hcd) {
> dev_err(otg_dev, "otg: shared host already
> registered\n");
> goto err;
>
> What do you think this local hack?
Is it guaranteed that EHCI hcd registers before OHCI hcd?
>>>
>>> Thank you for the comment. No, it is not guaranteed.
>>>
If not we need to improve the code still.
We will also need to remove the constraint that primary_hcd must be
registered
first in usb_otg_register_hcd(). I think that constraint is no longer
needed anyways.
>>>
>>> I got it.
>>> So, I made a patch to avoid the constraint using an additional property
>>> "otg-hcds".
>>> The patch is in this end of email. What do you think about the patch?
>>
>> This might only solve the issue for device tree but what about non-device
>> tree?
>> We need to deal with both cases.
>
> Thank you for the comment. I got it.
>
If EHCI & OHCI HCDs register before OTG driver then things will break
unless
we fix usb_otg_hcd_wait_add(). We need to add this HCD_USB11 check there to
populate wait->shared_hcd.
>>>
>>> I see. In my environment, since EHCI & OHCI HCDs need phy driver and phy
>>> driver
>>> will calls usb_otg_register(), the order is right. In other words,
>>> EHCI & OHCI HCDs never register before OTG driver because even if EHCI
>>> driver
>>> is probed first, the driver will be deferred (EHCI driver needs the phy
>>> driver).
>>
>> But still we cannot assume this is true for all platforms. OTG driver can
>> also
>> be a separate entity than PHY driver.
>
> I understood it.
>
> I also wonder if array of hcd may be good for both xHCI and EHCI/OHCI.
> For example of xHCI:
> - otg->hcds[0] = primary_hcd
> - otg->hcds[1] = shared_hcd
>
> For example of EHCI/OHCI:
> - otg->hcds[0] = primary_hcd of EHCI
> - otg->hcds[1] = primary_hcd of OHCI
The bigger problem is that how do we know in the OHCI/EHCI case that we
need to wait
for both of them before starting the OTG FSM?
Some systems might use just OHCI or just EHCI.
There is no guarantee that OTG driver registers before the HCDs so this
piece
of information must come from the HCD itself. i.e. whether it needs a
companion or not.
>>>
>>> I understood these problems. I wonder if my patch can resolve these
>>> problems.
>>>
>>> Best regards,
>>> Yoshihiro Shimoda
>>> ---
>>> diff --git a/Documentation/devicetree/bindings/usb/generic.txt
>>> b/Documentation/devicetree/bindings/usb/generic.txt
>>> index f6866c1..518b9eb 100644
>>> --- a/Documentation/devicetree/bindings/usb/generic.txt
>>> +++ b/Documentation/devicetree/bindings/usb/generic.txt
>>> @@ -27,6 +27,12 @@ Optional properties:
>>> - otg-controller: phandle to otg controller. Host or gadget controllers
>>> can
>>> contain this property to link it to a particular OTG
>>> controller.
>>> + - otg-hcds: phandle to host controller(s). An OTG controller can contain
>>> this
>>> +property. The first one is as "primary", and (optional) the second
>>> +one is as "shared".
>>> +- For example for xHCI:otg-hcds = <&xhci>, <&xhci>;
>>> +- For example for EHCI/OHCI: otg-hcds = <&ehci>, <&ohci>;
>>> +- For example for just EHCI: otg-hcds = <&ehci>;
>>>
>>
>> This is kind of duplicating the information. We are already specifying in the
>> hcd nodes as to which otg controller it is linked to.
>>
>> Instead, how about just adding a boolean property in the otg node s
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
> From: Roger Quadros
> Sent: Monday, April 11, 2016 7:55 PM
>
> On 08/04/16 14:22, Yoshihiro Shimoda wrote:
> > Hi,
> >
> >> From: Roger Quadros
> >> Sent: Thursday, April 07, 2016 8:45 PM
> >>
> >> Hi,
> >>
> >> On 07/04/16 11:52, Yoshihiro Shimoda wrote:
> >>> Hi,
> >>>
> From: Roger Quadros
> Sent: Tuesday, April 05, 2016 11:05 PM
> > < snip >
> >>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> >>> index 41e762a..4d7f043 100644
> >>> --- a/drivers/usb/common/usb-otg.c
> >>> +++ b/drivers/usb/common/usb-otg.c
> >>> @@ -825,11 +825,16 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> >>> unsigned int irqnum,
> >>> if (otg->primary_hcd.hcd) {
> >>> /* probably a shared HCD ? */
> >>> if (usb_otg_hcd_is_primary_hcd(hcd)) {
> >>> + if (hcd->driver->flags & HCD_USB11) {
> >>> + dev_info(otg_dev, "this assumes usb 1.1 hc is
> >>> as shared_hcd\n");
> >>> + goto check_shared_hcd;
> >>> + }
> >>> dev_err(otg_dev, "otg: primary host already
> >>> registered\n");
> >>> goto err;
> >>> }
> >>>
> >>> if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> >>> +check_shared_hcd:
> >>> if (otg->shared_hcd.hcd) {
> >>> dev_err(otg_dev, "otg: shared host already
> >>> registered\n");
> >>> goto err;
> >>>
> >>> What do you think this local hack?
> >>
> >> Is it guaranteed that EHCI hcd registers before OHCI hcd?
> >
> > Thank you for the comment. No, it is not guaranteed.
> >
> >> If not we need to improve the code still.
> >> We will also need to remove the constraint that primary_hcd must be
> >> registered
> >> first in usb_otg_register_hcd(). I think that constraint is no longer
> >> needed anyways.
> >
> > I got it.
> > So, I made a patch to avoid the constraint using an additional property
> > "otg-hcds".
> > The patch is in this end of email. What do you think about the patch?
>
> This might only solve the issue for device tree but what about non-device
> tree?
> We need to deal with both cases.
Thank you for the comment. I got it.
> >> If EHCI & OHCI HCDs register before OTG driver then things will break
> >> unless
> >> we fix usb_otg_hcd_wait_add(). We need to add this HCD_USB11 check there to
> >> populate wait->shared_hcd.
> >
> > I see. In my environment, since EHCI & OHCI HCDs need phy driver and phy
> > driver
> > will calls usb_otg_register(), the order is right. In other words,
> > EHCI & OHCI HCDs never register before OTG driver because even if EHCI
> > driver
> > is probed first, the driver will be deferred (EHCI driver needs the phy
> > driver).
>
> But still we cannot assume this is true for all platforms. OTG driver can also
> be a separate entity than PHY driver.
I understood it.
> >>> I also wonder if array of hcd may be good for both xHCI and EHCI/OHCI.
> >>> For example of xHCI:
> >>> - otg->hcds[0] = primary_hcd
> >>> - otg->hcds[1] = shared_hcd
> >>>
> >>> For example of EHCI/OHCI:
> >>> - otg->hcds[0] = primary_hcd of EHCI
> >>> - otg->hcds[1] = primary_hcd of OHCI
> >>
> >> The bigger problem is that how do we know in the OHCI/EHCI case that we
> >> need to wait
> >> for both of them before starting the OTG FSM?
> >> Some systems might use just OHCI or just EHCI.
> >>
> >> There is no guarantee that OTG driver registers before the HCDs so this
> >> piece
> >> of information must come from the HCD itself. i.e. whether it needs a
> >> companion or not.
> >
> > I understood these problems. I wonder if my patch can resolve these
> > problems.
> >
> > Best regards,
> > Yoshihiro Shimoda
> > ---
> > diff --git a/Documentation/devicetree/bindings/usb/generic.txt
> > b/Documentation/devicetree/bindings/usb/generic.txt
> > index f6866c1..518b9eb 100644
> > --- a/Documentation/devicetree/bindings/usb/generic.txt
> > +++ b/Documentation/devicetree/bindings/usb/generic.txt
> > @@ -27,6 +27,12 @@ Optional properties:
> > - otg-controller: phandle to otg controller. Host or gadget controllers
> > can
> > contain this property to link it to a particular OTG
> > controller.
> > + - otg-hcds: phandle to host controller(s). An OTG controller can contain
> > this
> > +property. The first one is as "primary", and (optional) the second
> > +one is as "shared".
> > +- For example for xHCI:otg-hcds = <&xhci>, <&xhci>;
> > +- For example for EHCI/OHCI: otg-hcds = <&ehci>, <&ohci>;
> > +- For example for just EHCI: otg-hcds = <&ehci>;
> >
>
> This is kind of duplicating the information. We are already specifying in the
> hcd nodes as to which otg controller it is linked to.
>
> Instead, how about just adding a boolean property in the otg node saying that
> HCD needs companion. e.g.
> hcd-needs-compainion: must b
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
On 08/04/16 14:22, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Roger Quadros
>> Sent: Thursday, April 07, 2016 8:45 PM
>>
>> Hi,
>>
>> On 07/04/16 11:52, Yoshihiro Shimoda wrote:
>>> Hi,
>>>
From: Roger Quadros
Sent: Tuesday, April 05, 2016 11:05 PM
> < snip >
>>> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
>>> index 41e762a..4d7f043 100644
>>> --- a/drivers/usb/common/usb-otg.c
>>> +++ b/drivers/usb/common/usb-otg.c
>>> @@ -825,11 +825,16 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
>>> unsigned int irqnum,
>>> if (otg->primary_hcd.hcd) {
>>> /* probably a shared HCD ? */
>>> if (usb_otg_hcd_is_primary_hcd(hcd)) {
>>> + if (hcd->driver->flags & HCD_USB11) {
>>> + dev_info(otg_dev, "this assumes usb 1.1 hc is
>>> as shared_hcd\n");
>>> + goto check_shared_hcd;
>>> + }
>>> dev_err(otg_dev, "otg: primary host already
>>> registered\n");
>>> goto err;
>>> }
>>>
>>> if (hcd->shared_hcd == otg->primary_hcd.hcd) {
>>> +check_shared_hcd:
>>> if (otg->shared_hcd.hcd) {
>>> dev_err(otg_dev, "otg: shared host already
>>> registered\n");
>>> goto err;
>>>
>>> What do you think this local hack?
>>
>> Is it guaranteed that EHCI hcd registers before OHCI hcd?
>
> Thank you for the comment. No, it is not guaranteed.
>
>> If not we need to improve the code still.
>> We will also need to remove the constraint that primary_hcd must be
>> registered
>> first in usb_otg_register_hcd(). I think that constraint is no longer
>> needed anyways.
>
> I got it.
> So, I made a patch to avoid the constraint using an additional property
> "otg-hcds".
> The patch is in this end of email. What do you think about the patch?
This might only solve the issue for device tree but what about non-device tree?
We need to deal with both cases.
>
>> If EHCI & OHCI HCDs register before OTG driver then things will break unless
>> we fix usb_otg_hcd_wait_add(). We need to add this HCD_USB11 check there to
>> populate wait->shared_hcd.
>
> I see. In my environment, since EHCI & OHCI HCDs need phy driver and phy
> driver
> will calls usb_otg_register(), the order is right. In other words,
> EHCI & OHCI HCDs never register before OTG driver because even if EHCI driver
> is probed first, the driver will be deferred (EHCI driver needs the phy
> driver).
But still we cannot assume this is true for all platforms. OTG driver can also
be a separate entity than PHY driver.
>
>>> I also wonder if array of hcd may be good for both xHCI and EHCI/OHCI.
>>> For example of xHCI:
>>> - otg->hcds[0] = primary_hcd
>>> - otg->hcds[1] = shared_hcd
>>>
>>> For example of EHCI/OHCI:
>>> - otg->hcds[0] = primary_hcd of EHCI
>>> - otg->hcds[1] = primary_hcd of OHCI
>>
>> The bigger problem is that how do we know in the OHCI/EHCI case that we need
>> to wait
>> for both of them before starting the OTG FSM?
>> Some systems might use just OHCI or just EHCI.
>>
>> There is no guarantee that OTG driver registers before the HCDs so this piece
>> of information must come from the HCD itself. i.e. whether it needs a
>> companion or not.
>
> I understood these problems. I wonder if my patch can resolve these problems.
>
> Best regards,
> Yoshihiro Shimoda
> ---
> diff --git a/Documentation/devicetree/bindings/usb/generic.txt
> b/Documentation/devicetree/bindings/usb/generic.txt
> index f6866c1..518b9eb 100644
> --- a/Documentation/devicetree/bindings/usb/generic.txt
> +++ b/Documentation/devicetree/bindings/usb/generic.txt
> @@ -27,6 +27,12 @@ Optional properties:
> - otg-controller: phandle to otg controller. Host or gadget controllers can
> contain this property to link it to a particular OTG
> controller.
> + - otg-hcds: phandle to host controller(s). An OTG controller can contain
> this
> + property. The first one is as "primary", and (optional) the second
> + one is as "shared".
> + - For example for xHCI:otg-hcds = <&xhci>, <&xhci>;
> + - For example for EHCI/OHCI: otg-hcds = <&ehci>, <&ohci>;
> + - For example for just EHCI: otg-hcds = <&ehci>;
>
This is kind of duplicating the information. We are already specifying in the
hcd nodes as to which otg controller it is linked to.
Instead, how about just adding a boolean property in the otg node saying that
HCD needs companion. e.g.
hcd-needs-compainion: must be present if otg controller is dealing with
EHCI host controller that needs a companion OHCI host
controller.
This can also be added in struct usb_otg_config so that it can deal with non-DT
cases.
So if this property is true, the OTG core will wait for 2 primary HCDs to be
registered.
cheers,
-roger
>
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
> From: Roger Quadros
> Sent: Thursday, April 07, 2016 8:45 PM
>
> Hi,
>
> On 07/04/16 11:52, Yoshihiro Shimoda wrote:
> > Hi,
> >
> >> From: Roger Quadros
> >> Sent: Tuesday, April 05, 2016 11:05 PM
< snip >
> > diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> > index 41e762a..4d7f043 100644
> > --- a/drivers/usb/common/usb-otg.c
> > +++ b/drivers/usb/common/usb-otg.c
> > @@ -825,11 +825,16 @@ int usb_otg_register_hcd(struct usb_hcd *hcd,
> > unsigned int irqnum,
> > if (otg->primary_hcd.hcd) {
> > /* probably a shared HCD ? */
> > if (usb_otg_hcd_is_primary_hcd(hcd)) {
> > + if (hcd->driver->flags & HCD_USB11) {
> > + dev_info(otg_dev, "this assumes usb 1.1 hc is
> > as shared_hcd\n");
> > + goto check_shared_hcd;
> > + }
> > dev_err(otg_dev, "otg: primary host already
> > registered\n");
> > goto err;
> > }
> >
> > if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> > +check_shared_hcd:
> > if (otg->shared_hcd.hcd) {
> > dev_err(otg_dev, "otg: shared host already
> > registered\n");
> > goto err;
> >
> > What do you think this local hack?
>
> Is it guaranteed that EHCI hcd registers before OHCI hcd?
Thank you for the comment. No, it is not guaranteed.
> If not we need to improve the code still.
> We will also need to remove the constraint that primary_hcd must be registered
> first in usb_otg_register_hcd(). I think that constraint is no longer
> needed anyways.
I got it.
So, I made a patch to avoid the constraint using an additional property
"otg-hcds".
The patch is in this end of email. What do you think about the patch?
> If EHCI & OHCI HCDs register before OTG driver then things will break unless
> we fix usb_otg_hcd_wait_add(). We need to add this HCD_USB11 check there to
> populate wait->shared_hcd.
I see. In my environment, since EHCI & OHCI HCDs need phy driver and phy driver
will calls usb_otg_register(), the order is right. In other words,
EHCI & OHCI HCDs never register before OTG driver because even if EHCI driver
is probed first, the driver will be deferred (EHCI driver needs the phy driver).
> > I also wonder if array of hcd may be good for both xHCI and EHCI/OHCI.
> > For example of xHCI:
> > - otg->hcds[0] = primary_hcd
> > - otg->hcds[1] = shared_hcd
> >
> > For example of EHCI/OHCI:
> > - otg->hcds[0] = primary_hcd of EHCI
> > - otg->hcds[1] = primary_hcd of OHCI
>
> The bigger problem is that how do we know in the OHCI/EHCI case that we need
> to wait
> for both of them before starting the OTG FSM?
> Some systems might use just OHCI or just EHCI.
>
> There is no guarantee that OTG driver registers before the HCDs so this piece
> of information must come from the HCD itself. i.e. whether it needs a
> companion or not.
I understood these problems. I wonder if my patch can resolve these problems.
Best regards,
Yoshihiro Shimoda
---
diff --git a/Documentation/devicetree/bindings/usb/generic.txt
b/Documentation/devicetree/bindings/usb/generic.txt
index f6866c1..518b9eb 100644
--- a/Documentation/devicetree/bindings/usb/generic.txt
+++ b/Documentation/devicetree/bindings/usb/generic.txt
@@ -27,6 +27,12 @@ Optional properties:
- otg-controller: phandle to otg controller. Host or gadget controllers can
contain this property to link it to a particular OTG
controller.
+ - otg-hcds: phandle to host controller(s). An OTG controller can contain this
+property. The first one is as "primary", and (optional) the second
+one is as "shared".
+- For example for xHCI:otg-hcds = <&xhci>, <&xhci>;
+- For example for EHCI/OHCI: otg-hcds = <&ehci>, <&ohci>;
+- For example for just EHCI: otg-hcds = <&ehci>;
This is an attribute to a USB controller such as:
diff --git a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
index 741d9d2..9dcf76ac 100644
--- a/arch/arm64/boot/dts/renesas/r8a7795.dtsi
+++ b/arch/arm64/boot/dts/renesas/r8a7795.dtsi
@@ -1253,6 +1253,7 @@
compatible = "renesas,usb2-phy-r8a7795";
reg = <0 0xee080200 0 0x700>;
interrupts = ;
+ otg-hcds = <&ehci0>, <&ohci0>;
clocks = <&cpg CPG_MOD 703>;
power-domains = <&sysc R8A7795_PD_ALWAYS_ON>;
#phy-cells = <0>;
diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
index 41e762a..9a78482 100644
--- a/drivers/usb/common/usb-otg.c
+++ b/drivers/usb/common/usb-otg.c
@@ -258,7 +258,8 @@ static void usb_otg_flush_wait(struct device *otg_dev)
dev_dbg(otg_dev, "otg: registering pending host/gad
Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
On 07/04/16 11:52, Yoshihiro Shimoda wrote:
> Hi,
>
>> From: Roger Quadros
>> Sent: Tuesday, April 05, 2016 11:05 PM
>>
>> It provides APIs for the following tasks
>>
>> - Registering an OTG/dual-role capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
>>
>> Provide a dual-role device (DRD) state machine.
>> DRD mode is a reduced functionality OTG mode. In this mode
>> we don't support SRP, HNP and dynamic role-swap.
>>
>> In DRD operation, the controller mode (Host or Peripheral)
>> is decided based on the ID pin status. Once a cable plug (Type-A
>> or Type-B) is attached the controller selects the state
>> and doesn't change till the cable in unplugged and a different
>> cable type is inserted.
>>
>> As we don't need most of the complex OTG states and OTG timers
>> we implement a lean DRD state machine in usb-otg.c.
>> The DRD state machine is only interested in 2 hardware inputs
>> 'id' and 'b_sess_vld'.
>>
>> Signed-off-by: Roger Quadros
>> ---
>
> I tried to implement this framework on my environment,
> and it seemed work if I added a local hack to this patch :)
Great. Thanks for testing :).
>
> < snip >
>> +/* HCD will be started by OTG fsm when needed */
>> +mutex_lock(&otg->fsm.lock);
>> +if (otg->primary_hcd.hcd) {
>> +/* probably a shared HCD ? */
>> +if (usb_otg_hcd_is_primary_hcd(hcd)) {
>> +dev_err(otg_dev, "otg: primary host already
>> registered\n");
>
> My environment is arm64 / r8a7795, renesas_usbhs (as a gadget), EHCI and OHCI.
> In this case, OHCI driver is also a primary_hcd. So, this error happened.
> To avoid this, I assumes that the OHCI hcd is as shared_hcd like a patch
> below:
>
> diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
> index 41e762a..4d7f043 100644
> --- a/drivers/usb/common/usb-otg.c
> +++ b/drivers/usb/common/usb-otg.c
> @@ -825,11 +825,16 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
> int irqnum,
> if (otg->primary_hcd.hcd) {
> /* probably a shared HCD ? */
> if (usb_otg_hcd_is_primary_hcd(hcd)) {
> + if (hcd->driver->flags & HCD_USB11) {
> + dev_info(otg_dev, "this assumes usb 1.1 hc is
> as shared_hcd\n");
> + goto check_shared_hcd;
> + }
> dev_err(otg_dev, "otg: primary host already
> registered\n");
> goto err;
> }
>
> if (hcd->shared_hcd == otg->primary_hcd.hcd) {
> +check_shared_hcd:
> if (otg->shared_hcd.hcd) {
> dev_err(otg_dev, "otg: shared host already
> registered\n");
> goto err;
>
> What do you think this local hack?
Is it guaranteed that EHCI hcd registers before OHCI hcd?
If not we need to improve the code still.
We will also need to remove the constraint that primary_hcd must be registered
first in usb_otg_register_hcd(). I think that constraint is no longer
needed anyways.
If EHCI & OHCI HCDs register before OTG driver then things will break unless
we fix usb_otg_hcd_wait_add(). We need to add this HCD_USB11 check there to
populate wait->shared_hcd.
> I also wonder if array of hcd may be good for both xHCI and EHCI/OHCI.
> For example of xHCI:
> - otg->hcds[0] = primary_hcd
> - otg->hcds[1] = shared_hcd
>
> For example of EHCI/OHCI:
> - otg->hcds[0] = primary_hcd of EHCI
> - otg->hcds[1] = primary_hcd of OHCI
The bigger problem is that how do we know in the OHCI/EHCI case that we need to
wait
for both of them before starting the OTG FSM?
Some systems might use just OHCI or just EHCI.
There is no guarantee that OTG driver registers before the HCDs so this piece
of information must come from the HCD itself. i.e. whether it needs a companion
or not.
cheers,
-roger
RE: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
Hi,
> From: Roger Quadros
> Sent: Tuesday, April 05, 2016 11:05 PM
>
> It provides APIs for the following tasks
>
> - Registering an OTG/dual-role capable controller
> - Registering Host and Gadget controllers to OTG core
> - Providing inputs to and kicking the OTG state machine
>
> Provide a dual-role device (DRD) state machine.
> DRD mode is a reduced functionality OTG mode. In this mode
> we don't support SRP, HNP and dynamic role-swap.
>
> In DRD operation, the controller mode (Host or Peripheral)
> is decided based on the ID pin status. Once a cable plug (Type-A
> or Type-B) is attached the controller selects the state
> and doesn't change till the cable in unplugged and a different
> cable type is inserted.
>
> As we don't need most of the complex OTG states and OTG timers
> we implement a lean DRD state machine in usb-otg.c.
> The DRD state machine is only interested in 2 hardware inputs
> 'id' and 'b_sess_vld'.
>
> Signed-off-by: Roger Quadros
> ---
I tried to implement this framework on my environment,
and it seemed work if I added a local hack to this patch :)
< snip >
> + /* HCD will be started by OTG fsm when needed */
> + mutex_lock(&otg->fsm.lock);
> + if (otg->primary_hcd.hcd) {
> + /* probably a shared HCD ? */
> + if (usb_otg_hcd_is_primary_hcd(hcd)) {
> + dev_err(otg_dev, "otg: primary host already
> registered\n");
My environment is arm64 / r8a7795, renesas_usbhs (as a gadget), EHCI and OHCI.
In this case, OHCI driver is also a primary_hcd. So, this error happened.
To avoid this, I assumes that the OHCI hcd is as shared_hcd like a patch below:
diff --git a/drivers/usb/common/usb-otg.c b/drivers/usb/common/usb-otg.c
index 41e762a..4d7f043 100644
--- a/drivers/usb/common/usb-otg.c
+++ b/drivers/usb/common/usb-otg.c
@@ -825,11 +825,16 @@ int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned
int irqnum,
if (otg->primary_hcd.hcd) {
/* probably a shared HCD ? */
if (usb_otg_hcd_is_primary_hcd(hcd)) {
+ if (hcd->driver->flags & HCD_USB11) {
+ dev_info(otg_dev, "this assumes usb 1.1 hc is
as shared_hcd\n");
+ goto check_shared_hcd;
+ }
dev_err(otg_dev, "otg: primary host already
registered\n");
goto err;
}
if (hcd->shared_hcd == otg->primary_hcd.hcd) {
+check_shared_hcd:
if (otg->shared_hcd.hcd) {
dev_err(otg_dev, "otg: shared host already
registered\n");
goto err;
What do you think this local hack?
I also wonder if array of hcd may be good for both xHCI and EHCI/OHCI.
For example of xHCI:
- otg->hcds[0] = primary_hcd
- otg->hcds[1] = shared_hcd
For example of EHCI/OHCI:
- otg->hcds[0] = primary_hcd of EHCI
- otg->hcds[1] = primary_hcd of OHCI
Best regards,
Yoshihiro Shimoda
