Message ID | 1463133808-10630-14-git-send-email-rogerq@ti.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: > + > +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) > +{ > + struct usb_udc *udc; > + > + mutex_lock(&udc_lock); > + udc = usb_gadget_to_udc(gadget); > + if (!udc) { > + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", > + __func__); > + mutex_unlock(&udc_lock); > + return -EINVAL; > + } > + > + if (connect) { > + if (!gadget->connected) > + usb_gadget_connect(udc->gadget); > + } else { > + if (gadget->connected) { > + usb_gadget_disconnect(udc->gadget); > + udc->driver->disconnect(udc->gadget); > + } > + } > + > + mutex_unlock(&udc_lock); > + > + return 0; > +} > + Since this is called for vbus interrupt, why not using usb_udc_vbus_handler directly, and call udc->driver->disconnect at usb_gadget_stop. > return 0; > @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev, > return -EOPNOTSUPP; > } > > + /* In OTG mode we don't support softconnect, but b_bus_req */ > + if (udc->gadget->otg_dev) { > + dev_err(dev, "soft-connect not supported in OTG mode\n"); > + return -EOPNOTSUPP; > + } > + The soft-connect can be supported at dual-role mode currently, we can use b_bus_req entry once it is implemented later. > if (sysfs_streq(buf, "connect")) { > usb_gadget_udc_start(udc); > - usb_gadget_connect(udc->gadget); > + usb_udc_connect_control(udc); This line seems to be not related with this patch.
Hi, On 16/05/16 10:02, Peter Chen wrote: > On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >> + >> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) >> +{ >> + struct usb_udc *udc; >> + >> + mutex_lock(&udc_lock); >> + udc = usb_gadget_to_udc(gadget); >> + if (!udc) { >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >> + __func__); >> + mutex_unlock(&udc_lock); >> + return -EINVAL; >> + } >> + >> + if (connect) { >> + if (!gadget->connected) >> + usb_gadget_connect(udc->gadget); >> + } else { >> + if (gadget->connected) { >> + usb_gadget_disconnect(udc->gadget); >> + udc->driver->disconnect(udc->gadget); >> + } >> + } >> + >> + mutex_unlock(&udc_lock); >> + >> + return 0; >> +} >> + > > Since this is called for vbus interrupt, why not using > usb_udc_vbus_handler directly, and call udc->driver->disconnect > at usb_gadget_stop. We can't assume that this is always called for vbus interrupt so I decided not to call usb_udc_vbus_handler. udc->vbus is really pointless for us. We keep vbus states in our state machine and leave udc->vbus as ture always. Why do you want to move udc->driver->disconnect() to stop? If USB controller disconnected from bus then the gadget driver must be notified about the disconnect immediately. The controller may or may not be stopped by the core. > >> return 0; >> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev, >> return -EOPNOTSUPP; >> } >> >> + /* In OTG mode we don't support softconnect, but b_bus_req */ >> + if (udc->gadget->otg_dev) { >> + dev_err(dev, "soft-connect not supported in OTG mode\n"); >> + return -EOPNOTSUPP; >> + } >> + > > The soft-connect can be supported at dual-role mode currently, we can > use b_bus_req entry once it is implemented later. Soft-connect should be done via sysfs handling within the OTG core. This can be added later. I don't want anything outside the OTG core to handle soft-connect behaviour as it will be hard to keep things in sync. I can update the comment to something like this. /* In OTG/dual-role mode, soft-connect should be handled by OTG core */ > >> if (sysfs_streq(buf, "connect")) { >> usb_gadget_udc_start(udc); >> - usb_gadget_connect(udc->gadget); >> + usb_udc_connect_control(udc); > > This line seems to be not related with this patch. > Right. I'll remove it. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: > Hi, > > On 16/05/16 10:02, Peter Chen wrote: > > On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: > >> + > >> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) > >> +{ > >> + struct usb_udc *udc; > >> + > >> + mutex_lock(&udc_lock); > >> + udc = usb_gadget_to_udc(gadget); > >> + if (!udc) { > >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", > >> + __func__); > >> + mutex_unlock(&udc_lock); > >> + return -EINVAL; > >> + } > >> + > >> + if (connect) { > >> + if (!gadget->connected) > >> + usb_gadget_connect(udc->gadget); > >> + } else { > >> + if (gadget->connected) { > >> + usb_gadget_disconnect(udc->gadget); > >> + udc->driver->disconnect(udc->gadget); > >> + } > >> + } > >> + > >> + mutex_unlock(&udc_lock); > >> + > >> + return 0; > >> +} > >> + > > > > Since this is called for vbus interrupt, why not using > > usb_udc_vbus_handler directly, and call udc->driver->disconnect > > at usb_gadget_stop. > > We can't assume that this is always called for vbus interrupt so > I decided not to call usb_udc_vbus_handler. > > udc->vbus is really pointless for us. We keep vbus states in our > state machine and leave udc->vbus as ture always. > > Why do you want to move udc->driver->disconnect() to stop? > If USB controller disconnected from bus then the gadget driver > must be notified about the disconnect immediately. The controller > may or may not be stopped by the core. > Then, would you give some comments when this API will be used? I was assumed it is only used for drd state machine. > > > >> return 0; > >> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev, > >> return -EOPNOTSUPP; > >> } > >> > >> + /* In OTG mode we don't support softconnect, but b_bus_req */ > >> + if (udc->gadget->otg_dev) { > >> + dev_err(dev, "soft-connect not supported in OTG mode\n"); > >> + return -EOPNOTSUPP; > >> + } > >> + > > > > The soft-connect can be supported at dual-role mode currently, we can > > use b_bus_req entry once it is implemented later. > > Soft-connect should be done via sysfs handling within the OTG core. > This can be added later. I don't want anything outside the OTG core > to handle soft-connect behaviour as it will be hard to keep things > in sync. > > I can update the comment to something like this. > > /* In OTG/dual-role mode, soft-connect should be handled by OTG core */ Ok, let's Felipe decide it. > > > > >> if (sysfs_streq(buf, "connect")) { > >> usb_gadget_udc_start(udc); > >> - usb_gadget_connect(udc->gadget); > >> + usb_udc_connect_control(udc); > > > > This line seems to be not related with this patch. > > > Right. I'll remove it. > > cheers, > -roger
On 16/05/16 12:23, Peter Chen wrote: > On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: >> Hi, >> >> On 16/05/16 10:02, Peter Chen wrote: >>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >>>> + >>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) >>>> +{ >>>> + struct usb_udc *udc; >>>> + >>>> + mutex_lock(&udc_lock); >>>> + udc = usb_gadget_to_udc(gadget); >>>> + if (!udc) { >>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >>>> + __func__); >>>> + mutex_unlock(&udc_lock); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + if (connect) { >>>> + if (!gadget->connected) >>>> + usb_gadget_connect(udc->gadget); >>>> + } else { >>>> + if (gadget->connected) { >>>> + usb_gadget_disconnect(udc->gadget); >>>> + udc->driver->disconnect(udc->gadget); >>>> + } >>>> + } >>>> + >>>> + mutex_unlock(&udc_lock); >>>> + >>>> + return 0; >>>> +} >>>> + >>> >>> Since this is called for vbus interrupt, why not using >>> usb_udc_vbus_handler directly, and call udc->driver->disconnect >>> at usb_gadget_stop. >> >> We can't assume that this is always called for vbus interrupt so >> I decided not to call usb_udc_vbus_handler. >> >> udc->vbus is really pointless for us. We keep vbus states in our >> state machine and leave udc->vbus as ture always. >> >> Why do you want to move udc->driver->disconnect() to stop? >> If USB controller disconnected from bus then the gadget driver >> must be notified about the disconnect immediately. The controller >> may or may not be stopped by the core. >> > > Then, would you give some comments when this API will be used? > I was assumed it is only used for drd state machine. drd_state machine didn't even need this API in the first place :). You guys wanted me to separate out start/stop and connect/disconnect for full OTG case. Won't full OTG state machine want to use this API? If not what would it use? cheers, -roger > >>> >>>> return 0; >>>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev, >>>> return -EOPNOTSUPP; >>>> } >>>> >>>> + /* In OTG mode we don't support softconnect, but b_bus_req */ >>>> + if (udc->gadget->otg_dev) { >>>> + dev_err(dev, "soft-connect not supported in OTG mode\n"); >>>> + return -EOPNOTSUPP; >>>> + } >>>> + >>> >>> The soft-connect can be supported at dual-role mode currently, we can >>> use b_bus_req entry once it is implemented later. >> >> Soft-connect should be done via sysfs handling within the OTG core. >> This can be added later. I don't want anything outside the OTG core >> to handle soft-connect behaviour as it will be hard to keep things >> in sync. >> >> I can update the comment to something like this. >> >> /* In OTG/dual-role mode, soft-connect should be handled by OTG core */ > > Ok, let's Felipe decide it. > >> >>> >>>> if (sysfs_streq(buf, "connect")) { >>>> usb_gadget_udc_start(udc); >>>> - usb_gadget_connect(udc->gadget); >>>> + usb_udc_connect_control(udc); >>> >>> This line seems to be not related with this patch. >>> >> Right. I'll remove it. >> >> cheers, >> -roger > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi > -----Original Message----- > From: Roger Quadros [mailto:rogerq@ti.com] > Sent: Monday, May 16, 2016 5:52 PM > To: Peter Chen <hzpeterchen@gmail.com> > Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; > gregkh@linuxfoundation.org; dan.j.williams@intel.com; > mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; > sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; > grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; > robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; > linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core > > On 16/05/16 12:23, Peter Chen wrote: > > On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: > >> Hi, > >> > >> On 16/05/16 10:02, Peter Chen wrote: > >>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: > >>>> + > >>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, > >>>> +bool connect) { > >>>> + struct usb_udc *udc; > >>>> + > >>>> + mutex_lock(&udc_lock); > >>>> + udc = usb_gadget_to_udc(gadget); > >>>> + if (!udc) { > >>>> + dev_err(gadget->dev.parent, "%s: gadget not > registered.\n", > >>>> + __func__); > >>>> + mutex_unlock(&udc_lock); > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + if (connect) { > >>>> + if (!gadget->connected) > >>>> + usb_gadget_connect(udc->gadget); > >>>> + } else { > >>>> + if (gadget->connected) { > >>>> + usb_gadget_disconnect(udc->gadget); > >>>> + udc->driver->disconnect(udc->gadget); > >>>> + } > >>>> + } > >>>> + > >>>> + mutex_unlock(&udc_lock); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>> > >>> Since this is called for vbus interrupt, why not using > >>> usb_udc_vbus_handler directly, and call udc->driver->disconnect at > >>> usb_gadget_stop. > >> > >> We can't assume that this is always called for vbus interrupt so I > >> decided not to call usb_udc_vbus_handler. > >> > >> udc->vbus is really pointless for us. We keep vbus states in our > >> state machine and leave udc->vbus as ture always. > >> > >> Why do you want to move udc->driver->disconnect() to stop? > >> If USB controller disconnected from bus then the gadget driver must > >> be notified about the disconnect immediately. The controller may or > >> may not be stopped by the core. > >> > > > > Then, would you give some comments when this API will be used? > > I was assumed it is only used for drd state machine. > > drd_state machine didn't even need this API in the first place :). > You guys wanted me to separate out start/stop and connect/disconnect for > full OTG case. > Won't full OTG state machine want to use this API? If not what would it > use? Instead create those new interfaces/symbol here and there just aim to address build problems in diff configures, Could we only allow meaningful combination of those 3 drivers configures? Hcd=y, gadget=y, otg=y or Hcd=m, gadget=m, otg=m Li Jun > > cheers, > -roger > > > > >>> > >>>> return 0; > >>>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct > device *dev, > >>>> return -EOPNOTSUPP; > >>>> } > >>>> > >>>> + /* In OTG mode we don't support softconnect, but b_bus_req */ > >>>> + if (udc->gadget->otg_dev) { > >>>> + dev_err(dev, "soft-connect not supported in OTG mode\n"); > >>>> + return -EOPNOTSUPP; > >>>> + } > >>>> + > >>> > >>> The soft-connect can be supported at dual-role mode currently, we > >>> can use b_bus_req entry once it is implemented later. > >> > >> Soft-connect should be done via sysfs handling within the OTG core. > >> This can be added later. I don't want anything outside the OTG core > >> to handle soft-connect behaviour as it will be hard to keep things in > >> sync. > >> > >> I can update the comment to something like this. > >> > >> /* In OTG/dual-role mode, soft-connect should be handled by OTG core > >> */ > > > > Ok, let's Felipe decide it. > > > >> > >>> > >>>> if (sysfs_streq(buf, "connect")) { > >>>> usb_gadget_udc_start(udc); > >>>> - usb_gadget_connect(udc->gadget); > >>>> + usb_udc_connect_control(udc); > >>> > >>> This line seems to be not related with this patch. > >>> > >> Right. I'll remove it. > >> > >> cheers, > >> -roger > > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 17/05/16 10:38, Jun Li wrote: > Hi > >> -----Original Message----- >> From: Roger Quadros [mailto:rogerq@ti.com] >> Sent: Monday, May 16, 2016 5:52 PM >> To: Peter Chen <hzpeterchen@gmail.com> >> Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; >> gregkh@linuxfoundation.org; dan.j.williams@intel.com; >> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; >> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; >> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; >> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; >> linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; >> devicetree@vger.kernel.org >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core >> >> On 16/05/16 12:23, Peter Chen wrote: >>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 16/05/16 10:02, Peter Chen wrote: >>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >>>>>> + >>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, >>>>>> +bool connect) { >>>>>> + struct usb_udc *udc; >>>>>> + >>>>>> + mutex_lock(&udc_lock); >>>>>> + udc = usb_gadget_to_udc(gadget); >>>>>> + if (!udc) { >>>>>> + dev_err(gadget->dev.parent, "%s: gadget not >> registered.\n", >>>>>> + __func__); >>>>>> + mutex_unlock(&udc_lock); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + if (connect) { >>>>>> + if (!gadget->connected) >>>>>> + usb_gadget_connect(udc->gadget); >>>>>> + } else { >>>>>> + if (gadget->connected) { >>>>>> + usb_gadget_disconnect(udc->gadget); >>>>>> + udc->driver->disconnect(udc->gadget); >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + mutex_unlock(&udc_lock); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>> >>>>> Since this is called for vbus interrupt, why not using >>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect at >>>>> usb_gadget_stop. >>>> >>>> We can't assume that this is always called for vbus interrupt so I >>>> decided not to call usb_udc_vbus_handler. >>>> >>>> udc->vbus is really pointless for us. We keep vbus states in our >>>> state machine and leave udc->vbus as ture always. >>>> >>>> Why do you want to move udc->driver->disconnect() to stop? >>>> If USB controller disconnected from bus then the gadget driver must >>>> be notified about the disconnect immediately. The controller may or >>>> may not be stopped by the core. >>>> >>> >>> Then, would you give some comments when this API will be used? >>> I was assumed it is only used for drd state machine. >> >> drd_state machine didn't even need this API in the first place :). >> You guys wanted me to separate out start/stop and connect/disconnect for >> full OTG case. >> Won't full OTG state machine want to use this API? If not what would it >> use? > > Instead create those new interfaces/symbol here and there just aim to > address build problems in diff configures, Could we only allow meaningful > combination of those 3 drivers configures? > > Hcd=y, gadget=y, otg=y or > Hcd=m, gadget=m, otg=m This is still a limitation. It is perfectly fine to have hcd=m, gadget=y or hcd=y, gadget=m cheers, -roger > > Li Jun > >> >> cheers, >> -roger >> >>> >>>>> >>>>>> return 0; >>>>>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct >> device *dev, >>>>>> return -EOPNOTSUPP; >>>>>> } >>>>>> >>>>>> + /* In OTG mode we don't support softconnect, but b_bus_req */ >>>>>> + if (udc->gadget->otg_dev) { >>>>>> + dev_err(dev, "soft-connect not supported in OTG mode\n"); >>>>>> + return -EOPNOTSUPP; >>>>>> + } >>>>>> + >>>>> >>>>> The soft-connect can be supported at dual-role mode currently, we >>>>> can use b_bus_req entry once it is implemented later. >>>> >>>> Soft-connect should be done via sysfs handling within the OTG core. >>>> This can be added later. I don't want anything outside the OTG core >>>> to handle soft-connect behaviour as it will be hard to keep things in >>>> sync. >>>> >>>> I can update the comment to something like this. >>>> >>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG core >>>> */ >>> >>> Ok, let's Felipe decide it. >>> >>>> >>>>> >>>>>> if (sysfs_streq(buf, "connect")) { >>>>>> usb_gadget_udc_start(udc); >>>>>> - usb_gadget_connect(udc->gadget); >>>>>> + usb_udc_connect_control(udc); >>>>> >>>>> This line seems to be not related with this patch. >>>>> >>>> Right. I'll remove it. >>>> >>>> cheers, >>>> -roger >>> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Roger, > -----Original Message----- > From: Roger Quadros [mailto:rogerq@ti.com] > Sent: Tuesday, May 17, 2016 4:09 PM > To: Jun Li <jun.li@nxp.com>; Peter Chen <hzpeterchen@gmail.com> > Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; > gregkh@linuxfoundation.org; dan.j.williams@intel.com; > mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; > sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; > grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; > robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; > linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core > > On 17/05/16 10:38, Jun Li wrote: > > Hi > > > >> -----Original Message----- > >> From: Roger Quadros [mailto:rogerq@ti.com] > >> Sent: Monday, May 16, 2016 5:52 PM > >> To: Peter Chen <hzpeterchen@gmail.com> > >> Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; > >> gregkh@linuxfoundation.org; dan.j.williams@intel.com; > >> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; > >> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; > >> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; > >> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; > >> linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; > >> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org > >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core > >> > >> On 16/05/16 12:23, Peter Chen wrote: > >>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: > >>>> Hi, > >>>> > >>>> On 16/05/16 10:02, Peter Chen wrote: > >>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: > >>>>>> + > >>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, > >>>>>> +bool connect) { > >>>>>> + struct usb_udc *udc; > >>>>>> + > >>>>>> + mutex_lock(&udc_lock); > >>>>>> + udc = usb_gadget_to_udc(gadget); > >>>>>> + if (!udc) { > >>>>>> + dev_err(gadget->dev.parent, "%s: gadget not > >> registered.\n", > >>>>>> + __func__); > >>>>>> + mutex_unlock(&udc_lock); > >>>>>> + return -EINVAL; > >>>>>> + } > >>>>>> + > >>>>>> + if (connect) { > >>>>>> + if (!gadget->connected) > >>>>>> + usb_gadget_connect(udc->gadget); > >>>>>> + } else { > >>>>>> + if (gadget->connected) { > >>>>>> + usb_gadget_disconnect(udc->gadget); > >>>>>> + udc->driver->disconnect(udc->gadget); > >>>>>> + } > >>>>>> + } > >>>>>> + > >>>>>> + mutex_unlock(&udc_lock); > >>>>>> + > >>>>>> + return 0; > >>>>>> +} > >>>>>> + > >>>>> > >>>>> Since this is called for vbus interrupt, why not using > >>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect at > >>>>> usb_gadget_stop. > >>>> > >>>> We can't assume that this is always called for vbus interrupt so I > >>>> decided not to call usb_udc_vbus_handler. > >>>> > >>>> udc->vbus is really pointless for us. We keep vbus states in our > >>>> state machine and leave udc->vbus as ture always. > >>>> > >>>> Why do you want to move udc->driver->disconnect() to stop? > >>>> If USB controller disconnected from bus then the gadget driver must > >>>> be notified about the disconnect immediately. The controller may or > >>>> may not be stopped by the core. > >>>> > >>> > >>> Then, would you give some comments when this API will be used? > >>> I was assumed it is only used for drd state machine. > >> > >> drd_state machine didn't even need this API in the first place :). > >> You guys wanted me to separate out start/stop and connect/disconnect > >> for full OTG case. > >> Won't full OTG state machine want to use this API? If not what would > >> it use? > > > > Instead create those new interfaces/symbol here and there just aim to > > address build problems in diff configures, Could we only allow > > meaningful combination of those 3 drivers configures? > > > > Hcd=y, gadget=y, otg=y or > > Hcd=m, gadget=m, otg=m > > This is still a limitation. > > It is perfectly fine to have > hcd=m, gadget=y > or > hcd=y, gadget=m I agree it makes sense to have above configs in non-otg case, that is, the 'y' driver can work without 'm' driver loaded. But, in otg enabled(y/m) case, the otherwise config of my list can't make any sense from my point view. That is: some driver is built-in, but it can't work at all if another 'm' driver is not loaded, in another words, the otg driver has to be 'm' if its dependent driver is 'm', correct? Li Jun > > cheers, > -roger > > > > > Li Jun > > > >> > >> cheers, > >> -roger > >> > >>> > >>>>> > >>>>>> return 0; > >>>>>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct > >> device *dev, > >>>>>> return -EOPNOTSUPP; > >>>>>> } > >>>>>> > >>>>>> + /* In OTG mode we don't support softconnect, but b_bus_req */ > >>>>>> + if (udc->gadget->otg_dev) { > >>>>>> + dev_err(dev, "soft-connect not supported in OTG mode\n"); > >>>>>> + return -EOPNOTSUPP; > >>>>>> + } > >>>>>> + > >>>>> > >>>>> The soft-connect can be supported at dual-role mode currently, we > >>>>> can use b_bus_req entry once it is implemented later. > >>>> > >>>> Soft-connect should be done via sysfs handling within the OTG core. > >>>> This can be added later. I don't want anything outside the OTG core > >>>> to handle soft-connect behaviour as it will be hard to keep things > >>>> in sync. > >>>> > >>>> I can update the comment to something like this. > >>>> > >>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG > >>>> core */ > >>> > >>> Ok, let's Felipe decide it. > >>> > >>>> > >>>>> > >>>>>> if (sysfs_streq(buf, "connect")) { > >>>>>> usb_gadget_udc_start(udc); > >>>>>> - usb_gadget_connect(udc->gadget); > >>>>>> + usb_udc_connect_control(udc); > >>>>> > >>>>> This line seems to be not related with this patch. > >>>>> > >>>> Right. I'll remove it. > >>>> > >>>> cheers, > >>>> -roger > >>> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote: > On 16/05/16 12:23, Peter Chen wrote: > > On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: > >> Hi, > >> > >> On 16/05/16 10:02, Peter Chen wrote: > >>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: > >>>> + > >>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) > >>>> +{ > >>>> + struct usb_udc *udc; > >>>> + > >>>> + mutex_lock(&udc_lock); > >>>> + udc = usb_gadget_to_udc(gadget); > >>>> + if (!udc) { > >>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", > >>>> + __func__); > >>>> + mutex_unlock(&udc_lock); > >>>> + return -EINVAL; > >>>> + } > >>>> + > >>>> + if (connect) { > >>>> + if (!gadget->connected) > >>>> + usb_gadget_connect(udc->gadget); > >>>> + } else { > >>>> + if (gadget->connected) { > >>>> + usb_gadget_disconnect(udc->gadget); > >>>> + udc->driver->disconnect(udc->gadget); > >>>> + } > >>>> + } > >>>> + > >>>> + mutex_unlock(&udc_lock); > >>>> + > >>>> + return 0; > >>>> +} > >>>> + > >>> > >>> Since this is called for vbus interrupt, why not using > >>> usb_udc_vbus_handler directly, and call udc->driver->disconnect > >>> at usb_gadget_stop. > >> > >> We can't assume that this is always called for vbus interrupt so > >> I decided not to call usb_udc_vbus_handler. > >> > >> udc->vbus is really pointless for us. We keep vbus states in our > >> state machine and leave udc->vbus as ture always. > >> > >> Why do you want to move udc->driver->disconnect() to stop? > >> If USB controller disconnected from bus then the gadget driver > >> must be notified about the disconnect immediately. The controller > >> may or may not be stopped by the core. > >> > > > > Then, would you give some comments when this API will be used? > > I was assumed it is only used for drd state machine. > > drd_state machine didn't even need this API in the first place :). > You guys wanted me to separate out start/stop and connect/disconnect for full OTG case. > Won't full OTG state machine want to use this API? If not what would it use? > Oh, I meant only drd and fully otg state machine needs it. I am wondering if we need have a new API to do it. Two questions: - Except for vbus interrupt, any chances this API will be used at current logic? - When this API is called but without a coming gadget->stop?
On 17/05/16 11:28, Jun Li wrote: > Hi Roger, > >> -----Original Message----- >> From: Roger Quadros [mailto:rogerq@ti.com] >> Sent: Tuesday, May 17, 2016 4:09 PM >> To: Jun Li <jun.li@nxp.com>; Peter Chen <hzpeterchen@gmail.com> >> Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; >> gregkh@linuxfoundation.org; dan.j.williams@intel.com; >> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; >> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; >> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; >> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; >> linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; >> devicetree@vger.kernel.org >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core >> >> On 17/05/16 10:38, Jun Li wrote: >>> Hi >>> >>>> -----Original Message----- >>>> From: Roger Quadros [mailto:rogerq@ti.com] >>>> Sent: Monday, May 16, 2016 5:52 PM >>>> To: Peter Chen <hzpeterchen@gmail.com> >>>> Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; >>>> gregkh@linuxfoundation.org; dan.j.williams@intel.com; >>>> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; >>>> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; >>>> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; >>>> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; >>>> linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; >>>> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org >>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core >>>> >>>> On 16/05/16 12:23, Peter Chen wrote: >>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: >>>>>> Hi, >>>>>> >>>>>> On 16/05/16 10:02, Peter Chen wrote: >>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >>>>>>>> + >>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, >>>>>>>> +bool connect) { >>>>>>>> + struct usb_udc *udc; >>>>>>>> + >>>>>>>> + mutex_lock(&udc_lock); >>>>>>>> + udc = usb_gadget_to_udc(gadget); >>>>>>>> + if (!udc) { >>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not >>>> registered.\n", >>>>>>>> + __func__); >>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (connect) { >>>>>>>> + if (!gadget->connected) >>>>>>>> + usb_gadget_connect(udc->gadget); >>>>>>>> + } else { >>>>>>>> + if (gadget->connected) { >>>>>>>> + usb_gadget_disconnect(udc->gadget); >>>>>>>> + udc->driver->disconnect(udc->gadget); >>>>>>>> + } >>>>>>>> + } >>>>>>>> + >>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>> >>>>>>> Since this is called for vbus interrupt, why not using >>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect at >>>>>>> usb_gadget_stop. >>>>>> >>>>>> We can't assume that this is always called for vbus interrupt so I >>>>>> decided not to call usb_udc_vbus_handler. >>>>>> >>>>>> udc->vbus is really pointless for us. We keep vbus states in our >>>>>> state machine and leave udc->vbus as ture always. >>>>>> >>>>>> Why do you want to move udc->driver->disconnect() to stop? >>>>>> If USB controller disconnected from bus then the gadget driver must >>>>>> be notified about the disconnect immediately. The controller may or >>>>>> may not be stopped by the core. >>>>>> >>>>> >>>>> Then, would you give some comments when this API will be used? >>>>> I was assumed it is only used for drd state machine. >>>> >>>> drd_state machine didn't even need this API in the first place :). >>>> You guys wanted me to separate out start/stop and connect/disconnect >>>> for full OTG case. >>>> Won't full OTG state machine want to use this API? If not what would >>>> it use? >>> >>> Instead create those new interfaces/symbol here and there just aim to >>> address build problems in diff configures, Could we only allow >>> meaningful combination of those 3 drivers configures? >>> >>> Hcd=y, gadget=y, otg=y or >>> Hcd=m, gadget=m, otg=m >> >> This is still a limitation. >> >> It is perfectly fine to have >> hcd=m, gadget=y >> or >> hcd=y, gadget=m > > I agree it makes sense to have above configs in non-otg case, that is, > the 'y' driver can work without 'm' driver loaded. > > But, > in otg enabled(y/m) case, the otherwise config of my list can't make > any sense from my point view. That is: some driver is built-in, but > it can't work at all if another 'm' driver is not loaded, > > in another words, the otg driver has to be 'm' if its dependent driver > is 'm', correct? If both host and gadget are 'm' then otg can be 'm', but if either host or gadget is built in then we have no choice but to make otg as built-in. I didn't want to have complex Kconfig so decided to have otg as built-in only. What do you want me to change in existing code? and why? cheers, -roger > > Li Jun > >> >> cheers, >> -roger >> >>> >>> Li Jun >>> >>>> >>>> cheers, >>>> -roger >>>> >>>>> >>>>>>> >>>>>>>> return 0; >>>>>>>> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct >>>> device *dev, >>>>>>>> return -EOPNOTSUPP; >>>>>>>> } >>>>>>>> >>>>>>>> + /* In OTG mode we don't support softconnect, but b_bus_req */ >>>>>>>> + if (udc->gadget->otg_dev) { >>>>>>>> + dev_err(dev, "soft-connect not supported in OTG mode\n"); >>>>>>>> + return -EOPNOTSUPP; >>>>>>>> + } >>>>>>>> + >>>>>>> >>>>>>> The soft-connect can be supported at dual-role mode currently, we >>>>>>> can use b_bus_req entry once it is implemented later. >>>>>> >>>>>> Soft-connect should be done via sysfs handling within the OTG core. >>>>>> This can be added later. I don't want anything outside the OTG core >>>>>> to handle soft-connect behaviour as it will be hard to keep things >>>>>> in sync. >>>>>> >>>>>> I can update the comment to something like this. >>>>>> >>>>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG >>>>>> core */ >>>>> >>>>> Ok, let's Felipe decide it. >>>>> >>>>>> >>>>>>> >>>>>>>> if (sysfs_streq(buf, "connect")) { >>>>>>>> usb_gadget_udc_start(udc); >>>>>>>> - usb_gadget_connect(udc->gadget); >>>>>>>> + usb_udc_connect_control(udc); >>>>>>> >>>>>>> This line seems to be not related with this patch. >>>>>>> >>>>>> Right. I'll remove it. >>>>>> >>>>>> cheers, >>>>>> -roger >>>>> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/05/16 06:18, Peter Chen wrote: > On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote: >> On 16/05/16 12:23, Peter Chen wrote: >>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 16/05/16 10:02, Peter Chen wrote: >>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >>>>>> + >>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) >>>>>> +{ >>>>>> + struct usb_udc *udc; >>>>>> + >>>>>> + mutex_lock(&udc_lock); >>>>>> + udc = usb_gadget_to_udc(gadget); >>>>>> + if (!udc) { >>>>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >>>>>> + __func__); >>>>>> + mutex_unlock(&udc_lock); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + if (connect) { >>>>>> + if (!gadget->connected) >>>>>> + usb_gadget_connect(udc->gadget); >>>>>> + } else { >>>>>> + if (gadget->connected) { >>>>>> + usb_gadget_disconnect(udc->gadget); >>>>>> + udc->driver->disconnect(udc->gadget); >>>>>> + } >>>>>> + } >>>>>> + >>>>>> + mutex_unlock(&udc_lock); >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>> >>>>> Since this is called for vbus interrupt, why not using >>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect >>>>> at usb_gadget_stop. >>>> >>>> We can't assume that this is always called for vbus interrupt so >>>> I decided not to call usb_udc_vbus_handler. >>>> >>>> udc->vbus is really pointless for us. We keep vbus states in our >>>> state machine and leave udc->vbus as ture always. >>>> >>>> Why do you want to move udc->driver->disconnect() to stop? >>>> If USB controller disconnected from bus then the gadget driver >>>> must be notified about the disconnect immediately. The controller >>>> may or may not be stopped by the core. >>>> >>> >>> Then, would you give some comments when this API will be used? >>> I was assumed it is only used for drd state machine. >> >> drd_state machine didn't even need this API in the first place :). >> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case. >> Won't full OTG state machine want to use this API? If not what would it use? >> > > Oh, I meant only drd and fully otg state machine needs it. I am > wondering if we need have a new API to do it. Two questions: OK. > > - Except for vbus interrupt, any chances this API will be used at > current logic? I don't think so. But we can't assume caller behaviour for any API. > - When this API is called but without a coming gadget->stop? > Never for DRD case. But we want to catch wrong users. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi > -----Original Message----- > From: Roger Quadros [mailto:rogerq@ti.com] > Sent: Wednesday, May 18, 2016 8:43 PM > To: Jun Li <jun.li@nxp.com>; Peter Chen <hzpeterchen@gmail.com> > Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; > gregkh@linuxfoundation.org; dan.j.williams@intel.com; > mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; > sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; > grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; > robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; > linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core > > On 17/05/16 11:28, Jun Li wrote: > > Hi Roger, > > > >> -----Original Message----- > >> From: Roger Quadros [mailto:rogerq@ti.com] > >> Sent: Tuesday, May 17, 2016 4:09 PM > >> To: Jun Li <jun.li@nxp.com>; Peter Chen <hzpeterchen@gmail.com> > >> Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; > >> gregkh@linuxfoundation.org; dan.j.williams@intel.com; > >> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; > >> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; > >> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; > >> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; > >> linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; > >> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org > >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core > >> > >> On 17/05/16 10:38, Jun Li wrote: > >>> Hi > >>> > >>>> -----Original Message----- > >>>> From: Roger Quadros [mailto:rogerq@ti.com] > >>>> Sent: Monday, May 16, 2016 5:52 PM > >>>> To: Peter Chen <hzpeterchen@gmail.com> > >>>> Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; > >>>> gregkh@linuxfoundation.org; dan.j.williams@intel.com; > >>>> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; > >>>> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; > >>>> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; > >>>> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; > >>>> linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; > >>>> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org > >>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core > >>>> > >>>> On 16/05/16 12:23, Peter Chen wrote: > >>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: > >>>>>> Hi, > >>>>>> > >>>>>> On 16/05/16 10:02, Peter Chen wrote: > >>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: > >>>>>>>> + > >>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget > >>>>>>>> +*gadget, bool connect) { > >>>>>>>> + struct usb_udc *udc; > >>>>>>>> + > >>>>>>>> + mutex_lock(&udc_lock); > >>>>>>>> + udc = usb_gadget_to_udc(gadget); > >>>>>>>> + if (!udc) { > >>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not > >>>> registered.\n", > >>>>>>>> + __func__); > >>>>>>>> + mutex_unlock(&udc_lock); > >>>>>>>> + return -EINVAL; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + if (connect) { > >>>>>>>> + if (!gadget->connected) > >>>>>>>> + usb_gadget_connect(udc->gadget); > >>>>>>>> + } else { > >>>>>>>> + if (gadget->connected) { > >>>>>>>> + usb_gadget_disconnect(udc->gadget); > >>>>>>>> + udc->driver->disconnect(udc->gadget); > >>>>>>>> + } > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + mutex_unlock(&udc_lock); > >>>>>>>> + > >>>>>>>> + return 0; > >>>>>>>> +} > >>>>>>>> + > >>>>>>> > >>>>>>> Since this is called for vbus interrupt, why not using > >>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect > >>>>>>> at usb_gadget_stop. > >>>>>> > >>>>>> We can't assume that this is always called for vbus interrupt so > >>>>>> I decided not to call usb_udc_vbus_handler. > >>>>>> > >>>>>> udc->vbus is really pointless for us. We keep vbus states in our > >>>>>> state machine and leave udc->vbus as ture always. > >>>>>> > >>>>>> Why do you want to move udc->driver->disconnect() to stop? > >>>>>> If USB controller disconnected from bus then the gadget driver > >>>>>> must be notified about the disconnect immediately. The controller > >>>>>> may or may not be stopped by the core. > >>>>>> > >>>>> > >>>>> Then, would you give some comments when this API will be used? > >>>>> I was assumed it is only used for drd state machine. > >>>> > >>>> drd_state machine didn't even need this API in the first place :). > >>>> You guys wanted me to separate out start/stop and > >>>> connect/disconnect for full OTG case. > >>>> Won't full OTG state machine want to use this API? If not what > >>>> would it use? > >>> > >>> Instead create those new interfaces/symbol here and there just aim > >>> to address build problems in diff configures, Could we only allow > >>> meaningful combination of those 3 drivers configures? > >>> > >>> Hcd=y, gadget=y, otg=y or > >>> Hcd=m, gadget=m, otg=m > >> > >> This is still a limitation. > >> > >> It is perfectly fine to have > >> hcd=m, gadget=y > >> or > >> hcd=y, gadget=m > > > > I agree it makes sense to have above configs in non-otg case, that is, > > the 'y' driver can work without 'm' driver loaded. > > > > But, > > in otg enabled(y/m) case, the otherwise config of my list can't make > > any sense from my point view. That is: some driver is built-in, but it > > can't work at all if another 'm' driver is not loaded, > > > > in another words, the otg driver has to be 'm' if its dependent driver > > is 'm', correct? > > If both host and gadget are 'm' then otg can be 'm', but if either host or > gadget is built in then we have no choice but to make otg as built-in. > > I didn't want to have complex Kconfig so decided to have otg as built-in > only. > What do you want me to change in existing code? and why? Remove those stuff which only for pass diff driver config Like every controller driver need a duplicated static struct otg_hcd_ops ci_hcd_ops = { ... } And here is another example, for gadget connect, otg driver can directly call to usb_udc_vbus_handler() in drd state machine, but you create another interface: .connect_control = usb_gadget_connect_control, If the symbol is defined in one driver which is 'm', another driver reference it should be 'm' as well, then there is no this kind of problem as my understanding. Li Jun > > cheers, > -roger > > > > > Li Jun > > > >> > >> cheers, > >> -roger > >> > >>> > >>> Li Jun > >>> > >>>> > >>>> cheers, > >>>> -roger > >>>> > >>>>> > >>>>>>> > >>>>>>>> return 0; > >>>>>>>> @@ -660,9 +830,15 @@ static ssize_t > >>>>>>>> usb_udc_softconn_store(struct > >>>> device *dev, > >>>>>>>> return -EOPNOTSUPP; > >>>>>>>> } > >>>>>>>> > >>>>>>>> + /* In OTG mode we don't support softconnect, but b_bus_req */ > >>>>>>>> + if (udc->gadget->otg_dev) { > >>>>>>>> + dev_err(dev, "soft-connect not supported in OTG mode\n"); > >>>>>>>> + return -EOPNOTSUPP; > >>>>>>>> + } > >>>>>>>> + > >>>>>>> > >>>>>>> The soft-connect can be supported at dual-role mode currently, > >>>>>>> we can use b_bus_req entry once it is implemented later. > >>>>>> > >>>>>> Soft-connect should be done via sysfs handling within the OTG core. > >>>>>> This can be added later. I don't want anything outside the OTG > >>>>>> core to handle soft-connect behaviour as it will be hard to keep > >>>>>> things in sync. > >>>>>> > >>>>>> I can update the comment to something like this. > >>>>>> > >>>>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG > >>>>>> core */ > >>>>> > >>>>> Ok, let's Felipe decide it. > >>>>> > >>>>>> > >>>>>>> > >>>>>>>> if (sysfs_streq(buf, "connect")) { > >>>>>>>> usb_gadget_udc_start(udc); > >>>>>>>> - usb_gadget_connect(udc->gadget); > >>>>>>>> + usb_udc_connect_control(udc); > >>>>>>> > >>>>>>> This line seems to be not related with this patch. > >>>>>>> > >>>>>> Right. I'll remove it. > >>>>>> > >>>>>> cheers, > >>>>>> -roger > >>>>> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/05/16 16:12, Jun Li wrote: > Hi > >> -----Original Message----- >> From: Roger Quadros [mailto:rogerq@ti.com] >> Sent: Wednesday, May 18, 2016 8:43 PM >> To: Jun Li <jun.li@nxp.com>; Peter Chen <hzpeterchen@gmail.com> >> Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; >> gregkh@linuxfoundation.org; dan.j.williams@intel.com; >> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; >> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; >> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; >> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; >> linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; >> devicetree@vger.kernel.org >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core >> >> On 17/05/16 11:28, Jun Li wrote: >>> Hi Roger, >>> >>>> -----Original Message----- >>>> From: Roger Quadros [mailto:rogerq@ti.com] >>>> Sent: Tuesday, May 17, 2016 4:09 PM >>>> To: Jun Li <jun.li@nxp.com>; Peter Chen <hzpeterchen@gmail.com> >>>> Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; >>>> gregkh@linuxfoundation.org; dan.j.williams@intel.com; >>>> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; >>>> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; >>>> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; >>>> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; >>>> linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; >>>> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org >>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core >>>> >>>> On 17/05/16 10:38, Jun Li wrote: >>>>> Hi >>>>> >>>>>> -----Original Message----- >>>>>> From: Roger Quadros [mailto:rogerq@ti.com] >>>>>> Sent: Monday, May 16, 2016 5:52 PM >>>>>> To: Peter Chen <hzpeterchen@gmail.com> >>>>>> Cc: peter.chen@freescale.com; balbi@kernel.org; tony@atomide.com; >>>>>> gregkh@linuxfoundation.org; dan.j.williams@intel.com; >>>>>> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; >>>>>> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; >>>>>> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; >>>>>> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; >>>>>> linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; >>>>>> linux-kernel@vger.kernel.org; devicetree@vger.kernel.org >>>>>> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core >>>>>> >>>>>> On 16/05/16 12:23, Peter Chen wrote: >>>>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: >>>>>>>> Hi, >>>>>>>> >>>>>>>> On 16/05/16 10:02, Peter Chen wrote: >>>>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >>>>>>>>>> + >>>>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget >>>>>>>>>> +*gadget, bool connect) { >>>>>>>>>> + struct usb_udc *udc; >>>>>>>>>> + >>>>>>>>>> + mutex_lock(&udc_lock); >>>>>>>>>> + udc = usb_gadget_to_udc(gadget); >>>>>>>>>> + if (!udc) { >>>>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not >>>>>> registered.\n", >>>>>>>>>> + __func__); >>>>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>>>> + return -EINVAL; >>>>>>>>>> + } >>>>>>>>>> + >>>>>>>>>> + if (connect) { >>>>>>>>>> + if (!gadget->connected) >>>>>>>>>> + usb_gadget_connect(udc->gadget); >>>>>>>>>> + } else { >>>>>>>>>> + if (gadget->connected) { >>>>>>>>>> + usb_gadget_disconnect(udc->gadget); >>>>>>>>>> + udc->driver->disconnect(udc->gadget); >>>>>>>>>> + } >>>>>>>>>> + } >>>>>>>>>> + >>>>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>>>> + >>>>>>>>>> + return 0; >>>>>>>>>> +} >>>>>>>>>> + >>>>>>>>> >>>>>>>>> Since this is called for vbus interrupt, why not using >>>>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect >>>>>>>>> at usb_gadget_stop. >>>>>>>> >>>>>>>> We can't assume that this is always called for vbus interrupt so >>>>>>>> I decided not to call usb_udc_vbus_handler. >>>>>>>> >>>>>>>> udc->vbus is really pointless for us. We keep vbus states in our >>>>>>>> state machine and leave udc->vbus as ture always. >>>>>>>> >>>>>>>> Why do you want to move udc->driver->disconnect() to stop? >>>>>>>> If USB controller disconnected from bus then the gadget driver >>>>>>>> must be notified about the disconnect immediately. The controller >>>>>>>> may or may not be stopped by the core. >>>>>>>> >>>>>>> >>>>>>> Then, would you give some comments when this API will be used? >>>>>>> I was assumed it is only used for drd state machine. >>>>>> >>>>>> drd_state machine didn't even need this API in the first place :). >>>>>> You guys wanted me to separate out start/stop and >>>>>> connect/disconnect for full OTG case. >>>>>> Won't full OTG state machine want to use this API? If not what >>>>>> would it use? >>>>> >>>>> Instead create those new interfaces/symbol here and there just aim >>>>> to address build problems in diff configures, Could we only allow >>>>> meaningful combination of those 3 drivers configures? >>>>> >>>>> Hcd=y, gadget=y, otg=y or >>>>> Hcd=m, gadget=m, otg=m >>>> >>>> This is still a limitation. >>>> >>>> It is perfectly fine to have >>>> hcd=m, gadget=y >>>> or >>>> hcd=y, gadget=m >>> >>> I agree it makes sense to have above configs in non-otg case, that is, >>> the 'y' driver can work without 'm' driver loaded. >>> >>> But, >>> in otg enabled(y/m) case, the otherwise config of my list can't make >>> any sense from my point view. That is: some driver is built-in, but it >>> can't work at all if another 'm' driver is not loaded, >>> >>> in another words, the otg driver has to be 'm' if its dependent driver >>> is 'm', correct? >> >> If both host and gadget are 'm' then otg can be 'm', but if either host or >> gadget is built in then we have no choice but to make otg as built-in. >> >> I didn't want to have complex Kconfig so decided to have otg as built-in >> only. >> What do you want me to change in existing code? and why? > > Remove those stuff which only for pass diff driver config > Like every controller driver need a duplicated > > static struct otg_hcd_ops ci_hcd_ops = { > ... > } This is an exception only. Every controller driver doesn't need to implement hcd_ops. It is implemented in the hcd core. > > And here is another example, for gadget connect, otg driver can > directly call to usb_udc_vbus_handler() in drd state machine, > but you create another interface: > > .connect_control = usb_gadget_connect_control, > > If the symbol is defined in one driver which is 'm', another driver > reference it should be 'm' as well, then there is no this kind of problem > as my understanding. That is fine as long as all are 'm'. but how do you solve the case when Gadget is built in and host is 'm'? OTG has to be built-in and you will need an hcd to gadget interface. Do you have any ideas to solve that case? cheers, -roger >>>>>> >>>>>>> >>>>>>>>> >>>>>>>>>> return 0; >>>>>>>>>> @@ -660,9 +830,15 @@ static ssize_t >>>>>>>>>> usb_udc_softconn_store(struct >>>>>> device *dev, >>>>>>>>>> return -EOPNOTSUPP; >>>>>>>>>> } >>>>>>>>>> >>>>>>>>>> + /* In OTG mode we don't support softconnect, but b_bus_req */ >>>>>>>>>> + if (udc->gadget->otg_dev) { >>>>>>>>>> + dev_err(dev, "soft-connect not supported in OTG mode\n"); >>>>>>>>>> + return -EOPNOTSUPP; >>>>>>>>>> + } >>>>>>>>>> + >>>>>>>>> >>>>>>>>> The soft-connect can be supported at dual-role mode currently, >>>>>>>>> we can use b_bus_req entry once it is implemented later. >>>>>>>> >>>>>>>> Soft-connect should be done via sysfs handling within the OTG core. >>>>>>>> This can be added later. I don't want anything outside the OTG >>>>>>>> core to handle soft-connect behaviour as it will be hard to keep >>>>>>>> things in sync. >>>>>>>> >>>>>>>> I can update the comment to something like this. >>>>>>>> >>>>>>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG >>>>>>>> core */ >>>>>>> >>>>>>> Ok, let's Felipe decide it. >>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>>> if (sysfs_streq(buf, "connect")) { >>>>>>>>>> usb_gadget_udc_start(udc); >>>>>>>>>> - usb_gadget_connect(udc->gadget); >>>>>>>>>> + usb_udc_connect_control(udc); >>>>>>>>> >>>>>>>>> This line seems to be not related with this patch. >>>>>>>>> >>>>>>>> Right. I'll remove it. >>>>>>>> >>>>>>>> cheers, >>>>>>>> -roger >>>>>>> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> >> > >> I didn't want to have complex Kconfig so decided to have otg as > >> built-in only. > >> What do you want me to change in existing code? and why? > > > > Remove those stuff which only for pass diff driver config Like every > > controller driver need a duplicated > > > > static struct otg_hcd_ops ci_hcd_ops = { > > ... > > } > > This is an exception only. Every controller driver doesn't need to > implement hcd_ops. It is implemented in the hcd core. > > > > > And here is another example, for gadget connect, otg driver can > > directly call to usb_udc_vbus_handler() in drd state machine, but you > > create another interface: > > > > .connect_control = usb_gadget_connect_control, > > > > If the symbol is defined in one driver which is 'm', another driver > > reference it should be 'm' as well, then there is no this kind of > > problem as my understanding. > > That is fine as long as all are 'm'. but how do you solve the case when > Gadget is built in and host is 'm'? OTG has to be built-in and you will > need an hcd to gadget interface. Hcd to gadget interface? Or you want to say otg to host interface? I think hcd and gadget are independent each other, now Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol) If you directly call to usb_udc_vbus_handler() in drd state machine Then: Hcd --> otg; and gadget <--> otg, (gadget and otg will refer to symbol of each other) Li Jun > > Do you have any ideas to solve that case? > > cheers, > -roger > > >>>>>> > >>>>>>> > >>>>>>>>> > >>>>>>>>>> return 0; > >>>>>>>>>> @@ -660,9 +830,15 @@ static ssize_t > >>>>>>>>>> usb_udc_softconn_store(struct > >>>>>> device *dev, > >>>>>>>>>> return -EOPNOTSUPP; > >>>>>>>>>> } > >>>>>>>>>> > >>>>>>>>>> + /* In OTG mode we don't support softconnect, but > b_bus_req */ > >>>>>>>>>> + if (udc->gadget->otg_dev) { > >>>>>>>>>> + dev_err(dev, "soft-connect not supported in OTG > mode\n"); > >>>>>>>>>> + return -EOPNOTSUPP; > >>>>>>>>>> + } > >>>>>>>>>> + > >>>>>>>>> > >>>>>>>>> The soft-connect can be supported at dual-role mode currently, > >>>>>>>>> we can use b_bus_req entry once it is implemented later. > >>>>>>>> > >>>>>>>> Soft-connect should be done via sysfs handling within the OTG > core. > >>>>>>>> This can be added later. I don't want anything outside the OTG > >>>>>>>> core to handle soft-connect behaviour as it will be hard to > >>>>>>>> keep things in sync. > >>>>>>>> > >>>>>>>> I can update the comment to something like this. > >>>>>>>> > >>>>>>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG > >>>>>>>> core */ > >>>>>>> > >>>>>>> Ok, let's Felipe decide it. > >>>>>>> > >>>>>>>> > >>>>>>>>> > >>>>>>>>>> if (sysfs_streq(buf, "connect")) { > >>>>>>>>>> usb_gadget_udc_start(udc); > >>>>>>>>>> - usb_gadget_connect(udc->gadget); > >>>>>>>>>> + usb_udc_connect_control(udc); > >>>>>>>>> > >>>>>>>>> This line seems to be not related with this patch. > >>>>>>>>> > >>>>>>>> Right. I'll remove it. > >>>>>>>> > >>>>>>>> cheers, > >>>>>>>> -roger > >>>>>>> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 18/05/16 17:46, Jun Li wrote: > > >>>> >>>> I didn't want to have complex Kconfig so decided to have otg as >>>> built-in only. >>>> What do you want me to change in existing code? and why? >>> >>> Remove those stuff which only for pass diff driver config Like every >>> controller driver need a duplicated >>> >>> static struct otg_hcd_ops ci_hcd_ops = { >>> ... >>> } >> >> This is an exception only. Every controller driver doesn't need to >> implement hcd_ops. It is implemented in the hcd core. >> >>> >>> And here is another example, for gadget connect, otg driver can >>> directly call to usb_udc_vbus_handler() in drd state machine, but you >>> create another interface: >>> >>> .connect_control = usb_gadget_connect_control, >>> >>> If the symbol is defined in one driver which is 'm', another driver >>> reference it should be 'm' as well, then there is no this kind of >>> problem as my understanding. >> >> That is fine as long as all are 'm'. but how do you solve the case when >> Gadget is built in and host is 'm'? OTG has to be built-in and you will >> need an hcd to gadget interface. > > Hcd to gadget interface? Or you want to say otg to host interface? Sorry, I meant to say host to otg interface. > > I think hcd and gadget are independent each other, now > > Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol) It is actually a circular dependency for both. hcd <--> otg and gadget <--> otg hcd -> otg for usb_otg_register/unregister_hcd otg -> hcd for usb_add/remove_hcd, usb_bus_start_enum, usb_control_msg, usb_hub_find_child gadget -> otg for usb_otg_register/unregister_gadget otg -> gadget for usb_gadget_start/stop, usb_udc_vbus_handler Now consider what will happen if I get rid of the otg_hcd and otg_gadget interfaces. 'y' means built-in, 'm' means module. 1) hcd 'y', gadget 'y' otg has to be 'y' for proper build. 2) hcd 'm', gadget 'm' otg has to be 'm' for proper build. 3) hcd 'y', gadget 'm' Build will fail always. If otg is 'y', otg build will fail due to dependency on gadget. If otg is 'm', hcd build will fail due to dependency on otg. 4) hcd 'm', gadget 'y' Build will fail always. If otg is 'y', otg build will fail due to dependency on hcd. If otg is 'm', gadget build will fails due to dependency on otg. So I solve this problem by adding the otg_hcd_ops and otg_gadget_ops to remove otg->hcd and otg->gadget dependency. Now we can address 3) and 4) like so 3) hcd 'y', gadget 'm' otg has to be 'y' for proper build. 4) hcd 'm', gadget 'y' otg has to be 'y' for proper build. > > If you directly call to usb_udc_vbus_handler() in drd state machine > > Then: > > Hcd --> otg; and gadget <--> otg, (gadget and otg will refer to symbol of each other) It's not so easy. We are again creating a circular dependency and all build configurations don't work like I pointed above. The only optimization I could do is make CONFIG_OTG tristate and allow it to be built as 'm' if both hcd and gadget are 'm'. i.e. case (2). cheers, -roger >> >> Do you have any ideas to solve that case? >> >> cheers, >> -roger >> >>>>>>>> >>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> return 0; >>>>>>>>>>>> @@ -660,9 +830,15 @@ static ssize_t >>>>>>>>>>>> usb_udc_softconn_store(struct >>>>>>>> device *dev, >>>>>>>>>>>> return -EOPNOTSUPP; >>>>>>>>>>>> } >>>>>>>>>>>> >>>>>>>>>>>> + /* In OTG mode we don't support softconnect, but >> b_bus_req */ >>>>>>>>>>>> + if (udc->gadget->otg_dev) { >>>>>>>>>>>> + dev_err(dev, "soft-connect not supported in OTG >> mode\n"); >>>>>>>>>>>> + return -EOPNOTSUPP; >>>>>>>>>>>> + } >>>>>>>>>>>> + >>>>>>>>>>> >>>>>>>>>>> The soft-connect can be supported at dual-role mode currently, >>>>>>>>>>> we can use b_bus_req entry once it is implemented later. >>>>>>>>>> >>>>>>>>>> Soft-connect should be done via sysfs handling within the OTG >> core. >>>>>>>>>> This can be added later. I don't want anything outside the OTG >>>>>>>>>> core to handle soft-connect behaviour as it will be hard to >>>>>>>>>> keep things in sync. >>>>>>>>>> >>>>>>>>>> I can update the comment to something like this. >>>>>>>>>> >>>>>>>>>> /* In OTG/dual-role mode, soft-connect should be handled by OTG >>>>>>>>>> core */ >>>>>>>>> >>>>>>>>> Ok, let's Felipe decide it. >>>>>>>>> >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>>> if (sysfs_streq(buf, "connect")) { >>>>>>>>>>>> usb_gadget_udc_start(udc); >>>>>>>>>>>> - usb_gadget_connect(udc->gadget); >>>>>>>>>>>> + usb_udc_connect_control(udc); >>>>>>>>>>> >>>>>>>>>>> This line seems to be not related with this patch. >>>>>>>>>>> >>>>>>>>>> Right. I'll remove it. >>>>>>>>>> >>>>>>>>>> cheers, >>>>>>>>>> -roger >>>>>>>>> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, May 18, 2016 at 03:45:11PM +0300, Roger Quadros wrote: > On 18/05/16 06:18, Peter Chen wrote: > > On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote: > >> On 16/05/16 12:23, Peter Chen wrote: > >>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: > >>>> Hi, > >>>> > >>>> On 16/05/16 10:02, Peter Chen wrote: > >>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: > >>>>>> + > >>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) > >>>>>> +{ > >>>>>> + struct usb_udc *udc; > >>>>>> + > >>>>>> + mutex_lock(&udc_lock); > >>>>>> + udc = usb_gadget_to_udc(gadget); > >>>>>> + if (!udc) { > >>>>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", > >>>>>> + __func__); > >>>>>> + mutex_unlock(&udc_lock); > >>>>>> + return -EINVAL; > >>>>>> + } > >>>>>> + > >>>>>> + if (connect) { > >>>>>> + if (!gadget->connected) > >>>>>> + usb_gadget_connect(udc->gadget); > >>>>>> + } else { > >>>>>> + if (gadget->connected) { > >>>>>> + usb_gadget_disconnect(udc->gadget); > >>>>>> + udc->driver->disconnect(udc->gadget); > >>>>>> + } > >>>>>> + } > >>>>>> + > >>>>>> + mutex_unlock(&udc_lock); > >>>>>> + > >>>>>> + return 0; > >>>>>> +} > >>>>>> + > >>>>> > >>>>> Since this is called for vbus interrupt, why not using > >>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect > >>>>> at usb_gadget_stop. > >>>> > >>>> We can't assume that this is always called for vbus interrupt so > >>>> I decided not to call usb_udc_vbus_handler. > >>>> > >>>> udc->vbus is really pointless for us. We keep vbus states in our > >>>> state machine and leave udc->vbus as ture always. > >>>> > >>>> Why do you want to move udc->driver->disconnect() to stop? > >>>> If USB controller disconnected from bus then the gadget driver > >>>> must be notified about the disconnect immediately. The controller > >>>> may or may not be stopped by the core. > >>>> > >>> > >>> Then, would you give some comments when this API will be used? > >>> I was assumed it is only used for drd state machine. > >> > >> drd_state machine didn't even need this API in the first place :). > >> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case. > >> Won't full OTG state machine want to use this API? If not what would it use? > >> > > > > Oh, I meant only drd and fully otg state machine needs it. I am > > wondering if we need have a new API to do it. Two questions: > > OK. > > > > - Except for vbus interrupt, any chances this API will be used at > > current logic? > > I don't think so. But we can't assume caller behaviour for any API. > > > - When this API is called but without a coming gadget->stop? > > > Never for DRD case. But we want to catch wrong users. > In future, otg_start_gadget will be used for both DRD and fully OTG FSM. There is no otg_loc_conn at current DRD FSM, but there is otg_loc_conn at current OTG FSM, see below. DRD FSM: 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; OTG FSM: case OTG_STATE_B_IDLE: otg_drv_vbus(otg, 0); otg_chrg_vbus(otg, 0); otg_loc_conn(otg, 0); otg_loc_sof(otg, 0); /* * Driver is responsible for starting ADP probing * if ADP sensing times out. */ otg_start_adp_sns(otg); otg_set_protocol(fsm, PROTO_UNDEF); otg_add_timer(otg, B_SE0_SRP); break; case OTG_STATE_B_PERIPHERAL: otg_chrg_vbus(otg, 0); otg_loc_sof(otg, 0); otg_set_protocol(fsm, PROTO_GADGET); otg_loc_conn(otg, 1); break; My original suggestion is to have an API to do pull dp and this API will be used at both DRD and OTG FSM, and called at otg_loc_conn. The (de)initialize is the same for both two FSMs, it both includes init peripheral mode and pull up dp, and can be done by drd_set_protocol(fsm, PROTO_GADGET) otg_loc_conn(otg, 1); What do you think?
Peter, On 20/05/16 04:39, Peter Chen wrote: > On Wed, May 18, 2016 at 03:45:11PM +0300, Roger Quadros wrote: >> On 18/05/16 06:18, Peter Chen wrote: >>> On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote: >>>> On 16/05/16 12:23, Peter Chen wrote: >>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: >>>>>> Hi, >>>>>> >>>>>> On 16/05/16 10:02, Peter Chen wrote: >>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >>>>>>>> + >>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) >>>>>>>> +{ >>>>>>>> + struct usb_udc *udc; >>>>>>>> + >>>>>>>> + mutex_lock(&udc_lock); >>>>>>>> + udc = usb_gadget_to_udc(gadget); >>>>>>>> + if (!udc) { >>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >>>>>>>> + __func__); >>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>> + return -EINVAL; >>>>>>>> + } >>>>>>>> + >>>>>>>> + if (connect) { >>>>>>>> + if (!gadget->connected) >>>>>>>> + usb_gadget_connect(udc->gadget); >>>>>>>> + } else { >>>>>>>> + if (gadget->connected) { >>>>>>>> + usb_gadget_disconnect(udc->gadget); >>>>>>>> + udc->driver->disconnect(udc->gadget); >>>>>>>> + } >>>>>>>> + } >>>>>>>> + >>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>> + >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>> >>>>>>> Since this is called for vbus interrupt, why not using >>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect >>>>>>> at usb_gadget_stop. >>>>>> >>>>>> We can't assume that this is always called for vbus interrupt so >>>>>> I decided not to call usb_udc_vbus_handler. >>>>>> >>>>>> udc->vbus is really pointless for us. We keep vbus states in our >>>>>> state machine and leave udc->vbus as ture always. >>>>>> >>>>>> Why do you want to move udc->driver->disconnect() to stop? >>>>>> If USB controller disconnected from bus then the gadget driver >>>>>> must be notified about the disconnect immediately. The controller >>>>>> may or may not be stopped by the core. >>>>>> >>>>> >>>>> Then, would you give some comments when this API will be used? >>>>> I was assumed it is only used for drd state machine. >>>> >>>> drd_state machine didn't even need this API in the first place :). >>>> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case. >>>> Won't full OTG state machine want to use this API? If not what would it use? >>>> >>> >>> Oh, I meant only drd and fully otg state machine needs it. I am >>> wondering if we need have a new API to do it. Two questions: >> >> OK. >>> >>> - Except for vbus interrupt, any chances this API will be used at >>> current logic? >> >> I don't think so. But we can't assume caller behaviour for any API. >> >>> - When this API is called but without a coming gadget->stop? >>> >> Never for DRD case. But we want to catch wrong users. >> > > In future, otg_start_gadget will be used for both DRD and fully OTG FSM. > There is no otg_loc_conn at current DRD FSM, but there is > otg_loc_conn at current OTG FSM, see below. > > DRD FSM: > 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; > > OTG FSM: > case OTG_STATE_B_IDLE: > otg_drv_vbus(otg, 0); > otg_chrg_vbus(otg, 0); > otg_loc_conn(otg, 0); > otg_loc_sof(otg, 0); > /* > * Driver is responsible for starting ADP probing > * if ADP sensing times out. > */ > otg_start_adp_sns(otg); > otg_set_protocol(fsm, PROTO_UNDEF); > otg_add_timer(otg, B_SE0_SRP); > break; > case OTG_STATE_B_PERIPHERAL: > otg_chrg_vbus(otg, 0); > otg_loc_sof(otg, 0); > otg_set_protocol(fsm, PROTO_GADGET); > otg_loc_conn(otg, 1); > break; > > My original suggestion is to have an API to do pull dp and this API > will be used at both DRD and OTG FSM, and called at otg_loc_conn. The API is usb_gadget_connect_control(); > The (de)initialize is the same for both two FSMs, it both includes > init peripheral mode and pull up dp, and can be done by drd_set_protocol(fsm, PROTO_GADGET) > otg_loc_conn(otg, 1); > > What do you think? > I think loc_conn is a bit confusing to drd users. Another issue I see is that DRD controller drivers will need to explicitly pass .loc_conn ops via the otg_fsm_ops. This is an additional step and totally unnecessary as it can be automatically done via direct DRD -> UDC-core call. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, May 19, 2016 at 10:32:44AM +0300, Roger Quadros wrote: > On 18/05/16 17:46, Jun Li wrote: > > > > > >>>> > >>>> I didn't want to have complex Kconfig so decided to have otg as > >>>> built-in only. > >>>> What do you want me to change in existing code? and why? > >>> > >>> Remove those stuff which only for pass diff driver config Like every > >>> controller driver need a duplicated > >>> > >>> static struct otg_hcd_ops ci_hcd_ops = { > >>> ... > >>> } > >> > >> This is an exception only. Every controller driver doesn't need to > >> implement hcd_ops. It is implemented in the hcd core. > >> > >>> > >>> And here is another example, for gadget connect, otg driver can > >>> directly call to usb_udc_vbus_handler() in drd state machine, but you > >>> create another interface: > >>> > >>> .connect_control = usb_gadget_connect_control, > >>> > >>> If the symbol is defined in one driver which is 'm', another driver > >>> reference it should be 'm' as well, then there is no this kind of > >>> problem as my understanding. > >> > >> That is fine as long as all are 'm'. but how do you solve the case when > >> Gadget is built in and host is 'm'? OTG has to be built-in and you will > >> need an hcd to gadget interface. > > > > Hcd to gadget interface? Or you want to say otg to host interface? > > Sorry, I meant to say host to otg interface. > > > > > I think hcd and gadget are independent each other, now > > > > Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol) > > It is actually a circular dependency for both. > hcd <--> otg and gadget <--> otg > > hcd -> otg for usb_otg_register/unregister_hcd > otg -> hcd for usb_add/remove_hcd, usb_bus_start_enum, usb_control_msg, usb_hub_find_child > > gadget -> otg for usb_otg_register/unregister_gadget > otg -> gadget for usb_gadget_start/stop, usb_udc_vbus_handler > > Now consider what will happen if I get rid of the otg_hcd and otg_gadget interfaces. > 'y' means built-in, 'm' means module. > > 1) hcd 'y', gadget 'y' > otg has to be 'y' for proper build. > > 2) hcd 'm', gadget 'm' > otg has to be 'm' for proper build. > > 3) hcd 'y', gadget 'm' > Build will fail always. > If otg is 'y', otg build will fail due to dependency on gadget. > If otg is 'm', hcd build will fail due to dependency on otg. > > 4) hcd 'm', gadget 'y' > Build will fail always. > If otg is 'y', otg build will fail due to dependency on hcd. > If otg is 'm', gadget build will fails due to dependency on otg. > > So I solve this problem by adding the otg_hcd_ops and otg_gadget_ops > to remove otg->hcd and otg->gadget dependency. > > Now we can address 3) and 4) like so > > 3) hcd 'y', gadget 'm' > otg has to be 'y' for proper build. > > 4) hcd 'm', gadget 'y' > otg has to be 'y' for proper build. > How about this: Moving usb_otg_register/unregister_hcd to host driver to remove dependency hcd->otg. And moving usb_otg_get_data to common.c. Delete the wait queue at usb-otg.c, and if calling usb_otg_get_data returns NULL, the host/device driver's probe return -EPROBE_DEFER. When the otg driver is probed successfully, the host/device will be re-probed again, and usb_otg_register_hcd will be called again. And let OTG depends on HCD && GADGET, and delete otg_hcd_ops and otg_gadget_ops. Below build dependency issues can be fixed. What do you think? > 1) hcd 'y', gadget 'y' > otg has to be 'y' for proper build. > > 2) hcd 'm', gadget 'm' > otg has to be 'm' for proper build. > > 3) hcd 'y', gadget 'm' > Build will fail always. > If otg is 'y', otg build will fail due to dependency on gadget. > If otg is 'm', hcd build will fail due to dependency on otg. > > 4) hcd 'm', gadget 'y' > Build will fail always. > If otg is 'y', otg build will fail due to dependency on hcd. > If otg is 'm', gadget build will fails due to dependency on otg.
On Fri, May 20, 2016 at 10:26:03AM +0300, Roger Quadros wrote: > Peter, > > On 20/05/16 04:39, Peter Chen wrote: > > On Wed, May 18, 2016 at 03:45:11PM +0300, Roger Quadros wrote: > >> On 18/05/16 06:18, Peter Chen wrote: > >>> On Mon, May 16, 2016 at 12:51:53PM +0300, Roger Quadros wrote: > >>>> On 16/05/16 12:23, Peter Chen wrote: > >>>>> On Mon, May 16, 2016 at 11:26:57AM +0300, Roger Quadros wrote: > >>>>>> Hi, > >>>>>> > >>>>>> On 16/05/16 10:02, Peter Chen wrote: > >>>>>>> On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: > >>>>>>>> + > >>>>>>>> +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) > >>>>>>>> +{ > >>>>>>>> + struct usb_udc *udc; > >>>>>>>> + > >>>>>>>> + mutex_lock(&udc_lock); > >>>>>>>> + udc = usb_gadget_to_udc(gadget); > >>>>>>>> + if (!udc) { > >>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", > >>>>>>>> + __func__); > >>>>>>>> + mutex_unlock(&udc_lock); > >>>>>>>> + return -EINVAL; > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + if (connect) { > >>>>>>>> + if (!gadget->connected) > >>>>>>>> + usb_gadget_connect(udc->gadget); > >>>>>>>> + } else { > >>>>>>>> + if (gadget->connected) { > >>>>>>>> + usb_gadget_disconnect(udc->gadget); > >>>>>>>> + udc->driver->disconnect(udc->gadget); > >>>>>>>> + } > >>>>>>>> + } > >>>>>>>> + > >>>>>>>> + mutex_unlock(&udc_lock); > >>>>>>>> + > >>>>>>>> + return 0; > >>>>>>>> +} > >>>>>>>> + > >>>>>>> > >>>>>>> Since this is called for vbus interrupt, why not using > >>>>>>> usb_udc_vbus_handler directly, and call udc->driver->disconnect > >>>>>>> at usb_gadget_stop. > >>>>>> > >>>>>> We can't assume that this is always called for vbus interrupt so > >>>>>> I decided not to call usb_udc_vbus_handler. > >>>>>> > >>>>>> udc->vbus is really pointless for us. We keep vbus states in our > >>>>>> state machine and leave udc->vbus as ture always. > >>>>>> > >>>>>> Why do you want to move udc->driver->disconnect() to stop? > >>>>>> If USB controller disconnected from bus then the gadget driver > >>>>>> must be notified about the disconnect immediately. The controller > >>>>>> may or may not be stopped by the core. > >>>>>> > >>>>> > >>>>> Then, would you give some comments when this API will be used? > >>>>> I was assumed it is only used for drd state machine. > >>>> > >>>> drd_state machine didn't even need this API in the first place :). > >>>> You guys wanted me to separate out start/stop and connect/disconnect for full OTG case. > >>>> Won't full OTG state machine want to use this API? If not what would it use? > >>>> > >>> > >>> Oh, I meant only drd and fully otg state machine needs it. I am > >>> wondering if we need have a new API to do it. Two questions: > >> > >> OK. > >>> > >>> - Except for vbus interrupt, any chances this API will be used at > >>> current logic? > >> > >> I don't think so. But we can't assume caller behaviour for any API. > >> > >>> - When this API is called but without a coming gadget->stop? > >>> > >> Never for DRD case. But we want to catch wrong users. > >> > > > > In future, otg_start_gadget will be used for both DRD and fully OTG FSM. > > There is no otg_loc_conn at current DRD FSM, but there is > > otg_loc_conn at current OTG FSM, see below. > > > > DRD FSM: > > 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; > > > > OTG FSM: > > case OTG_STATE_B_IDLE: > > otg_drv_vbus(otg, 0); > > otg_chrg_vbus(otg, 0); > > otg_loc_conn(otg, 0); > > otg_loc_sof(otg, 0); > > /* > > * Driver is responsible for starting ADP probing > > * if ADP sensing times out. > > */ > > otg_start_adp_sns(otg); > > otg_set_protocol(fsm, PROTO_UNDEF); > > otg_add_timer(otg, B_SE0_SRP); > > break; > > case OTG_STATE_B_PERIPHERAL: > > otg_chrg_vbus(otg, 0); > > otg_loc_sof(otg, 0); > > otg_set_protocol(fsm, PROTO_GADGET); > > otg_loc_conn(otg, 1); > > break; > > > > My original suggestion is to have an API to do pull dp and this API > > will be used at both DRD and OTG FSM, and called at otg_loc_conn. > > The API is usb_gadget_connect_control(); > > > The (de)initialize is the same for both two FSMs, it both includes > > init peripheral mode and pull up dp, and can be done by drd_set_protocol(fsm, PROTO_GADGET) > > otg_loc_conn(otg, 1); > > > > What do you think? > > > > I think loc_conn is a bit confusing to drd users. Another issue I see is that > DRD controller drivers will need to explicitly pass .loc_conn ops via the otg_fsm_ops. > This is an additional step and totally unnecessary as it can be automatically done > via direct DRD -> UDC-core call. > If you are stick to that, let's follow your way if Felipe agree with it too, although it lets the DRD state machine look different with OTG's.
On Sat, May 21, 2016 at 10:29:40AM +0800, Peter Chen wrote: > On Thu, May 19, 2016 at 10:32:44AM +0300, Roger Quadros wrote: > > On 18/05/16 17:46, Jun Li wrote: > > > > > > > > >>>> > > >>>> I didn't want to have complex Kconfig so decided to have otg as > > >>>> built-in only. > > >>>> What do you want me to change in existing code? and why? > > >>> > > >>> Remove those stuff which only for pass diff driver config Like every > > >>> controller driver need a duplicated > > >>> > > >>> static struct otg_hcd_ops ci_hcd_ops = { > > >>> ... > > >>> } > > >> > > >> This is an exception only. Every controller driver doesn't need to > > >> implement hcd_ops. It is implemented in the hcd core. > > >> > > >>> > > >>> And here is another example, for gadget connect, otg driver can > > >>> directly call to usb_udc_vbus_handler() in drd state machine, but you > > >>> create another interface: > > >>> > > >>> .connect_control = usb_gadget_connect_control, > > >>> > > >>> If the symbol is defined in one driver which is 'm', another driver > > >>> reference it should be 'm' as well, then there is no this kind of > > >>> problem as my understanding. > > >> > > >> That is fine as long as all are 'm'. but how do you solve the case when > > >> Gadget is built in and host is 'm'? OTG has to be built-in and you will > > >> need an hcd to gadget interface. > > > > > > Hcd to gadget interface? Or you want to say otg to host interface? > > > > Sorry, I meant to say host to otg interface. > > > > > > > > I think hcd and gadget are independent each other, now > > > > > > Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol) > > > > It is actually a circular dependency for both. > > hcd <--> otg and gadget <--> otg > > > > hcd -> otg for usb_otg_register/unregister_hcd > > otg -> hcd for usb_add/remove_hcd, usb_bus_start_enum, usb_control_msg, usb_hub_find_child > > > > gadget -> otg for usb_otg_register/unregister_gadget > > otg -> gadget for usb_gadget_start/stop, usb_udc_vbus_handler > > > > Now consider what will happen if I get rid of the otg_hcd and otg_gadget interfaces. > > 'y' means built-in, 'm' means module. > > > > 1) hcd 'y', gadget 'y' > > otg has to be 'y' for proper build. > > > > 2) hcd 'm', gadget 'm' > > otg has to be 'm' for proper build. > > > > 3) hcd 'y', gadget 'm' > > Build will fail always. > > If otg is 'y', otg build will fail due to dependency on gadget. > > If otg is 'm', hcd build will fail due to dependency on otg. > > > > 4) hcd 'm', gadget 'y' > > Build will fail always. > > If otg is 'y', otg build will fail due to dependency on hcd. > > If otg is 'm', gadget build will fails due to dependency on otg. > > > > So I solve this problem by adding the otg_hcd_ops and otg_gadget_ops > > to remove otg->hcd and otg->gadget dependency. > > > > Now we can address 3) and 4) like so > > > > 3) hcd 'y', gadget 'm' > > otg has to be 'y' for proper build. > > > > 4) hcd 'm', gadget 'y' > > otg has to be 'y' for proper build. > > > > How about this: > Moving usb_otg_register/unregister_hcd to host driver to remove > dependency hcd->otg. And moving usb_otg_get_data to common.c. > > Delete the wait queue at usb-otg.c, and if calling usb_otg_get_data > returns NULL, the host/device driver's probe return -EPROBE_DEFER. > When the otg driver is probed successfully, the host/device will be > re-probed again, and usb_otg_register_hcd will be called again. > > And let OTG depends on HCD && GADGET, and delete otg_hcd_ops and > otg_gadget_ops. Below build dependency issues can be fixed. > What do you think? > > > 1) hcd 'y', gadget 'y' > > otg has to be 'y' for proper build. > > > > 2) hcd 'm', gadget 'm' > > otg has to be 'm' for proper build. > > > > 3) hcd 'y', gadget 'm' > > Build will fail always. > > If otg is 'y', otg build will fail due to dependency on gadget. > > If otg is 'm', hcd build will fail due to dependency on otg. > > > > 4) hcd 'm', gadget 'y' > > Build will fail always. > > If otg is 'y', otg build will fail due to dependency on hcd. > > If otg is 'm', gadget build will fails due to dependency on otg. > -- > After thinking more, my suggestion can't work. How about moving CONFIG_USB_OTG out of CONFIG_USB, in that case, CONFIG_USB_OTG can only be built in.
On 23/05/16 06:21, Peter Chen wrote: > On Sat, May 21, 2016 at 10:29:40AM +0800, Peter Chen wrote: >> On Thu, May 19, 2016 at 10:32:44AM +0300, Roger Quadros wrote: >>> On 18/05/16 17:46, Jun Li wrote: >>>> >>>> >>>>>>> >>>>>>> I didn't want to have complex Kconfig so decided to have otg as >>>>>>> built-in only. >>>>>>> What do you want me to change in existing code? and why? >>>>>> >>>>>> Remove those stuff which only for pass diff driver config Like every >>>>>> controller driver need a duplicated >>>>>> >>>>>> static struct otg_hcd_ops ci_hcd_ops = { >>>>>> ... >>>>>> } >>>>> >>>>> This is an exception only. Every controller driver doesn't need to >>>>> implement hcd_ops. It is implemented in the hcd core. >>>>> >>>>>> >>>>>> And here is another example, for gadget connect, otg driver can >>>>>> directly call to usb_udc_vbus_handler() in drd state machine, but you >>>>>> create another interface: >>>>>> >>>>>> .connect_control = usb_gadget_connect_control, >>>>>> >>>>>> If the symbol is defined in one driver which is 'm', another driver >>>>>> reference it should be 'm' as well, then there is no this kind of >>>>>> problem as my understanding. >>>>> >>>>> That is fine as long as all are 'm'. but how do you solve the case when >>>>> Gadget is built in and host is 'm'? OTG has to be built-in and you will >>>>> need an hcd to gadget interface. >>>> >>>> Hcd to gadget interface? Or you want to say otg to host interface? >>> >>> Sorry, I meant to say host to otg interface. >>> >>>> >>>> I think hcd and gadget are independent each other, now >>>> >>>> Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol) >>> >>> It is actually a circular dependency for both. >>> hcd <--> otg and gadget <--> otg >>> >>> hcd -> otg for usb_otg_register/unregister_hcd >>> otg -> hcd for usb_add/remove_hcd, usb_bus_start_enum, usb_control_msg, usb_hub_find_child >>> >>> gadget -> otg for usb_otg_register/unregister_gadget >>> otg -> gadget for usb_gadget_start/stop, usb_udc_vbus_handler >>> >>> Now consider what will happen if I get rid of the otg_hcd and otg_gadget interfaces. >>> 'y' means built-in, 'm' means module. >>> >>> 1) hcd 'y', gadget 'y' >>> otg has to be 'y' for proper build. >>> >>> 2) hcd 'm', gadget 'm' >>> otg has to be 'm' for proper build. >>> >>> 3) hcd 'y', gadget 'm' >>> Build will fail always. >>> If otg is 'y', otg build will fail due to dependency on gadget. >>> If otg is 'm', hcd build will fail due to dependency on otg. >>> >>> 4) hcd 'm', gadget 'y' >>> Build will fail always. >>> If otg is 'y', otg build will fail due to dependency on hcd. >>> If otg is 'm', gadget build will fails due to dependency on otg. >>> >>> So I solve this problem by adding the otg_hcd_ops and otg_gadget_ops >>> to remove otg->hcd and otg->gadget dependency. >>> >>> Now we can address 3) and 4) like so >>> >>> 3) hcd 'y', gadget 'm' >>> otg has to be 'y' for proper build. >>> >>> 4) hcd 'm', gadget 'y' >>> otg has to be 'y' for proper build. >>> >> >> How about this: >> Moving usb_otg_register/unregister_hcd to host driver to remove >> dependency hcd->otg. And moving usb_otg_get_data to common.c. >> >> Delete the wait queue at usb-otg.c, and if calling usb_otg_get_data >> returns NULL, the host/device driver's probe return -EPROBE_DEFER. >> When the otg driver is probed successfully, the host/device will be >> re-probed again, and usb_otg_register_hcd will be called again. >> >> And let OTG depends on HCD && GADGET, and delete otg_hcd_ops and >> otg_gadget_ops. Below build dependency issues can be fixed. >> What do you think? >> >>> 1) hcd 'y', gadget 'y' >>> otg has to be 'y' for proper build. >>> >>> 2) hcd 'm', gadget 'm' >>> otg has to be 'm' for proper build. >>> >>> 3) hcd 'y', gadget 'm' >>> Build will fail always. >>> If otg is 'y', otg build will fail due to dependency on gadget. >>> If otg is 'm', hcd build will fail due to dependency on otg. >>> >>> 4) hcd 'm', gadget 'y' >>> Build will fail always. >>> If otg is 'y', otg build will fail due to dependency on hcd. >>> If otg is 'm', gadget build will fails due to dependency on otg. >> -- >> > > After thinking more, my suggestion can't work. How about moving > CONFIG_USB_OTG out of CONFIG_USB, in that case, CONFIG_USB_OTG > can only be built in. > I tried this but it still doesn't resolve 3 and 4. I.e. it can't be automatically set to 'y' when either of hcd/gadget is 'y' and the other is 'm'. I think some kconfig trickery can be done to get what we want. cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi > -----Original Message----- > From: Roger Quadros [mailto:rogerq@ti.com] > Sent: Monday, May 23, 2016 6:12 PM > To: Peter Chen <hzpeterchen@gmail.com> > Cc: Jun Li <jun.li@nxp.com>; peter.chen@freescale.com; balbi@kernel.org; > tony@atomide.com; gregkh@linuxfoundation.org; dan.j.williams@intel.com; > mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; > sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; > grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; > robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; > linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; > devicetree@vger.kernel.org > Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core > > On 23/05/16 06:21, Peter Chen wrote: > > On Sat, May 21, 2016 at 10:29:40AM +0800, Peter Chen wrote: > >> On Thu, May 19, 2016 at 10:32:44AM +0300, Roger Quadros wrote: > >>> On 18/05/16 17:46, Jun Li wrote: > >>>> > >>>> > >>>>>>> > >>>>>>> I didn't want to have complex Kconfig so decided to have otg as > >>>>>>> built-in only. > >>>>>>> What do you want me to change in existing code? and why? > >>>>>> > >>>>>> Remove those stuff which only for pass diff driver config Like > >>>>>> every controller driver need a duplicated > >>>>>> > >>>>>> static struct otg_hcd_ops ci_hcd_ops = { > >>>>>> ... > >>>>>> } > >>>>> > >>>>> This is an exception only. Every controller driver doesn't need to > >>>>> implement hcd_ops. It is implemented in the hcd core. > >>>>> > >>>>>> > >>>>>> And here is another example, for gadget connect, otg driver can > >>>>>> directly call to usb_udc_vbus_handler() in drd state machine, but > >>>>>> you create another interface: > >>>>>> > >>>>>> .connect_control = usb_gadget_connect_control, > >>>>>> > >>>>>> If the symbol is defined in one driver which is 'm', another > >>>>>> driver reference it should be 'm' as well, then there is no this > >>>>>> kind of problem as my understanding. > >>>>> > >>>>> That is fine as long as all are 'm'. but how do you solve the case > >>>>> when Gadget is built in and host is 'm'? OTG has to be built-in > >>>>> and you will need an hcd to gadget interface. > >>>> > >>>> Hcd to gadget interface? Or you want to say otg to host interface? > >>> > >>> Sorry, I meant to say host to otg interface. > >>> > >>>> > >>>> I think hcd and gadget are independent each other, now > >>>> > >>>> Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol) > >>> > >>> It is actually a circular dependency for both. > >>> hcd <--> otg and gadget <--> otg > >>> > >>> hcd -> otg for usb_otg_register/unregister_hcd otg -> hcd for > >>> usb_add/remove_hcd, usb_bus_start_enum, usb_control_msg, > >>> usb_hub_find_child > >>> > >>> gadget -> otg for usb_otg_register/unregister_gadget > >>> otg -> gadget for usb_gadget_start/stop, usb_udc_vbus_handler > >>> > >>> Now consider what will happen if I get rid of the otg_hcd and > otg_gadget interfaces. > >>> 'y' means built-in, 'm' means module. > >>> > >>> 1) hcd 'y', gadget 'y' > >>> otg has to be 'y' for proper build. > >>> > >>> 2) hcd 'm', gadget 'm' > >>> otg has to be 'm' for proper build. > >>> > >>> 3) hcd 'y', gadget 'm' > >>> Build will fail always. > >>> If otg is 'y', otg build will fail due to dependency on gadget. > >>> If otg is 'm', hcd build will fail due to dependency on otg. > >>> > >>> 4) hcd 'm', gadget 'y' > >>> Build will fail always. > >>> If otg is 'y', otg build will fail due to dependency on hcd. > >>> If otg is 'm', gadget build will fails due to dependency on otg. > >>> > >>> So I solve this problem by adding the otg_hcd_ops and otg_gadget_ops > >>> to remove otg->hcd and otg->gadget dependency. > >>> > >>> Now we can address 3) and 4) like so > >>> > >>> 3) hcd 'y', gadget 'm' > >>> otg has to be 'y' for proper build. > >>> > >>> 4) hcd 'm', gadget 'y' > >>> otg has to be 'y' for proper build. > >>> > >> > >> How about this: > >> Moving usb_otg_register/unregister_hcd to host driver to remove > >> dependency hcd->otg. And moving usb_otg_get_data to common.c. > >> > >> Delete the wait queue at usb-otg.c, and if calling usb_otg_get_data > >> returns NULL, the host/device driver's probe return -EPROBE_DEFER. > >> When the otg driver is probed successfully, the host/device will be > >> re-probed again, and usb_otg_register_hcd will be called again. > >> > >> And let OTG depends on HCD && GADGET, and delete otg_hcd_ops and > >> otg_gadget_ops. Below build dependency issues can be fixed. > >> What do you think? > >> > >>> 1) hcd 'y', gadget 'y' > >>> otg has to be 'y' for proper build. > >>> > >>> 2) hcd 'm', gadget 'm' > >>> otg has to be 'm' for proper build. > >>> > >>> 3) hcd 'y', gadget 'm' > >>> Build will fail always. > >>> If otg is 'y', otg build will fail due to dependency on gadget. > >>> If otg is 'm', hcd build will fail due to dependency on otg. > >>> > >>> 4) hcd 'm', gadget 'y' > >>> Build will fail always. > >>> If otg is 'y', otg build will fail due to dependency on hcd. > >>> If otg is 'm', gadget build will fails due to dependency on otg. > >> -- > >> > > > > After thinking more, my suggestion can't work. How about moving > > CONFIG_USB_OTG out of CONFIG_USB, in that case, CONFIG_USB_OTG can > > only be built in. > > > I tried this but it still doesn't resolve 3 and 4. I.e. > it can't be automatically set to 'y' when either of hcd/gadget is 'y' > and the other is 'm'. USB_OTG only can be selected by *user*, can't be automatically set to 'y' like you said, no matter what's hcd/gadget config, USB_OTG can be disabled in any cases. I think Peter's intention is to make USB_OTG to be 'bool', either disabled; or built-in(if enabled), something like your original idea? Li Jun > > I think some kconfig trickery can be done to get what we want. > > cheers, > -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 23/05/16 13:34, Jun Li wrote: > Hi > >> -----Original Message----- >> From: Roger Quadros [mailto:rogerq@ti.com] >> Sent: Monday, May 23, 2016 6:12 PM >> To: Peter Chen <hzpeterchen@gmail.com> >> Cc: Jun Li <jun.li@nxp.com>; peter.chen@freescale.com; balbi@kernel.org; >> tony@atomide.com; gregkh@linuxfoundation.org; dan.j.williams@intel.com; >> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; >> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; >> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; >> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; >> linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; >> devicetree@vger.kernel.org >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core >> >> On 23/05/16 06:21, Peter Chen wrote: >>> On Sat, May 21, 2016 at 10:29:40AM +0800, Peter Chen wrote: >>>> On Thu, May 19, 2016 at 10:32:44AM +0300, Roger Quadros wrote: >>>>> On 18/05/16 17:46, Jun Li wrote: >>>>>> >>>>>> >>>>>>>>> >>>>>>>>> I didn't want to have complex Kconfig so decided to have otg as >>>>>>>>> built-in only. >>>>>>>>> What do you want me to change in existing code? and why? >>>>>>>> >>>>>>>> Remove those stuff which only for pass diff driver config Like >>>>>>>> every controller driver need a duplicated >>>>>>>> >>>>>>>> static struct otg_hcd_ops ci_hcd_ops = { >>>>>>>> ... >>>>>>>> } >>>>>>> >>>>>>> This is an exception only. Every controller driver doesn't need to >>>>>>> implement hcd_ops. It is implemented in the hcd core. >>>>>>> >>>>>>>> >>>>>>>> And here is another example, for gadget connect, otg driver can >>>>>>>> directly call to usb_udc_vbus_handler() in drd state machine, but >>>>>>>> you create another interface: >>>>>>>> >>>>>>>> .connect_control = usb_gadget_connect_control, >>>>>>>> >>>>>>>> If the symbol is defined in one driver which is 'm', another >>>>>>>> driver reference it should be 'm' as well, then there is no this >>>>>>>> kind of problem as my understanding. >>>>>>> >>>>>>> That is fine as long as all are 'm'. but how do you solve the case >>>>>>> when Gadget is built in and host is 'm'? OTG has to be built-in >>>>>>> and you will need an hcd to gadget interface. >>>>>> >>>>>> Hcd to gadget interface? Or you want to say otg to host interface? >>>>> >>>>> Sorry, I meant to say host to otg interface. >>>>> >>>>>> >>>>>> I think hcd and gadget are independent each other, now >>>>>> >>>>>> Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol) >>>>> >>>>> It is actually a circular dependency for both. >>>>> hcd <--> otg and gadget <--> otg >>>>> >>>>> hcd -> otg for usb_otg_register/unregister_hcd otg -> hcd for >>>>> usb_add/remove_hcd, usb_bus_start_enum, usb_control_msg, >>>>> usb_hub_find_child >>>>> >>>>> gadget -> otg for usb_otg_register/unregister_gadget >>>>> otg -> gadget for usb_gadget_start/stop, usb_udc_vbus_handler >>>>> >>>>> Now consider what will happen if I get rid of the otg_hcd and >> otg_gadget interfaces. >>>>> 'y' means built-in, 'm' means module. >>>>> >>>>> 1) hcd 'y', gadget 'y' >>>>> otg has to be 'y' for proper build. >>>>> >>>>> 2) hcd 'm', gadget 'm' >>>>> otg has to be 'm' for proper build. >>>>> >>>>> 3) hcd 'y', gadget 'm' >>>>> Build will fail always. >>>>> If otg is 'y', otg build will fail due to dependency on gadget. >>>>> If otg is 'm', hcd build will fail due to dependency on otg. >>>>> >>>>> 4) hcd 'm', gadget 'y' >>>>> Build will fail always. >>>>> If otg is 'y', otg build will fail due to dependency on hcd. >>>>> If otg is 'm', gadget build will fails due to dependency on otg. >>>>> >>>>> So I solve this problem by adding the otg_hcd_ops and otg_gadget_ops >>>>> to remove otg->hcd and otg->gadget dependency. >>>>> >>>>> Now we can address 3) and 4) like so >>>>> >>>>> 3) hcd 'y', gadget 'm' >>>>> otg has to be 'y' for proper build. >>>>> >>>>> 4) hcd 'm', gadget 'y' >>>>> otg has to be 'y' for proper build. >>>>> >>>> >>>> How about this: >>>> Moving usb_otg_register/unregister_hcd to host driver to remove >>>> dependency hcd->otg. And moving usb_otg_get_data to common.c. >>>> >>>> Delete the wait queue at usb-otg.c, and if calling usb_otg_get_data >>>> returns NULL, the host/device driver's probe return -EPROBE_DEFER. >>>> When the otg driver is probed successfully, the host/device will be >>>> re-probed again, and usb_otg_register_hcd will be called again. >>>> >>>> And let OTG depends on HCD && GADGET, and delete otg_hcd_ops and >>>> otg_gadget_ops. Below build dependency issues can be fixed. >>>> What do you think? >>>> >>>>> 1) hcd 'y', gadget 'y' >>>>> otg has to be 'y' for proper build. >>>>> >>>>> 2) hcd 'm', gadget 'm' >>>>> otg has to be 'm' for proper build. >>>>> >>>>> 3) hcd 'y', gadget 'm' >>>>> Build will fail always. >>>>> If otg is 'y', otg build will fail due to dependency on gadget. >>>>> If otg is 'm', hcd build will fail due to dependency on otg. >>>>> >>>>> 4) hcd 'm', gadget 'y' >>>>> Build will fail always. >>>>> If otg is 'y', otg build will fail due to dependency on hcd. >>>>> If otg is 'm', gadget build will fails due to dependency on otg. >>>> -- >>>> >>> >>> After thinking more, my suggestion can't work. How about moving >>> CONFIG_USB_OTG out of CONFIG_USB, in that case, CONFIG_USB_OTG can >>> only be built in. >>> >> I tried this but it still doesn't resolve 3 and 4. I.e. >> it can't be automatically set to 'y' when either of hcd/gadget is 'y' >> and the other is 'm'. > > USB_OTG only can be selected by *user*, can't be automatically set to > 'y' like you said, no matter what's hcd/gadget config, USB_OTG can be > disabled in any cases. Yes. I am on the same page with that. > > I think Peter's intention is to make USB_OTG to be 'bool', either disabled; > or built-in(if enabled), something like your original idea? I'm fine with my original idea if you guys can confirm :). cheers, -roger > > Li Jun > >> >> I think some kconfig trickery can be done to get what we want. >> >> cheers, >> -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, May 23, 2016 at 01:36:51PM +0300, Roger Quadros wrote: > On 23/05/16 13:34, Jun Li wrote: > > Hi > > > >> -----Original Message----- > >> From: Roger Quadros [mailto:rogerq@ti.com] > >> Sent: Monday, May 23, 2016 6:12 PM > >> To: Peter Chen <hzpeterchen@gmail.com> > >> Cc: Jun Li <jun.li@nxp.com>; peter.chen@freescale.com; balbi@kernel.org; > >> tony@atomide.com; gregkh@linuxfoundation.org; dan.j.williams@intel.com; > >> mathias.nyman@linux.intel.com; Joao.Pinto@synopsys.com; > >> sergei.shtylyov@cogentembedded.com; jun.li@freescale.com; > >> grygorii.strashko@ti.com; yoshihiro.shimoda.uh@renesas.com; > >> robh@kernel.org; nsekhar@ti.com; b-liu@ti.com; linux-usb@vger.kernel.org; > >> linux-omap@vger.kernel.org; linux-kernel@vger.kernel.org; > >> devicetree@vger.kernel.org > >> Subject: Re: [PATCH v8 13/14] usb: gadget: udc: adapt to OTG core > >> > >> On 23/05/16 06:21, Peter Chen wrote: > >>> On Sat, May 21, 2016 at 10:29:40AM +0800, Peter Chen wrote: > >>>> On Thu, May 19, 2016 at 10:32:44AM +0300, Roger Quadros wrote: > >>>>> On 18/05/16 17:46, Jun Li wrote: > >>>>>> > >>>>>> > >>>>>>>>> > >>>>>>>>> I didn't want to have complex Kconfig so decided to have otg as > >>>>>>>>> built-in only. > >>>>>>>>> What do you want me to change in existing code? and why? > >>>>>>>> > >>>>>>>> Remove those stuff which only for pass diff driver config Like > >>>>>>>> every controller driver need a duplicated > >>>>>>>> > >>>>>>>> static struct otg_hcd_ops ci_hcd_ops = { > >>>>>>>> ... > >>>>>>>> } > >>>>>>> > >>>>>>> This is an exception only. Every controller driver doesn't need to > >>>>>>> implement hcd_ops. It is implemented in the hcd core. > >>>>>>> > >>>>>>>> > >>>>>>>> And here is another example, for gadget connect, otg driver can > >>>>>>>> directly call to usb_udc_vbus_handler() in drd state machine, but > >>>>>>>> you create another interface: > >>>>>>>> > >>>>>>>> .connect_control = usb_gadget_connect_control, > >>>>>>>> > >>>>>>>> If the symbol is defined in one driver which is 'm', another > >>>>>>>> driver reference it should be 'm' as well, then there is no this > >>>>>>>> kind of problem as my understanding. > >>>>>>> > >>>>>>> That is fine as long as all are 'm'. but how do you solve the case > >>>>>>> when Gadget is built in and host is 'm'? OTG has to be built-in > >>>>>>> and you will need an hcd to gadget interface. > >>>>>> > >>>>>> Hcd to gadget interface? Or you want to say otg to host interface? > >>>>> > >>>>> Sorry, I meant to say host to otg interface. > >>>>> > >>>>>> > >>>>>> I think hcd and gadget are independent each other, now > >>>>>> > >>>>>> Hcd --> otg; and gadget --> otg, (hcd and gadget use otg's symbol) > >>>>> > >>>>> It is actually a circular dependency for both. > >>>>> hcd <--> otg and gadget <--> otg > >>>>> > >>>>> hcd -> otg for usb_otg_register/unregister_hcd otg -> hcd for > >>>>> usb_add/remove_hcd, usb_bus_start_enum, usb_control_msg, > >>>>> usb_hub_find_child > >>>>> > >>>>> gadget -> otg for usb_otg_register/unregister_gadget > >>>>> otg -> gadget for usb_gadget_start/stop, usb_udc_vbus_handler > >>>>> > >>>>> Now consider what will happen if I get rid of the otg_hcd and > >> otg_gadget interfaces. > >>>>> 'y' means built-in, 'm' means module. > >>>>> > >>>>> 1) hcd 'y', gadget 'y' > >>>>> otg has to be 'y' for proper build. > >>>>> > >>>>> 2) hcd 'm', gadget 'm' > >>>>> otg has to be 'm' for proper build. > >>>>> > >>>>> 3) hcd 'y', gadget 'm' > >>>>> Build will fail always. > >>>>> If otg is 'y', otg build will fail due to dependency on gadget. > >>>>> If otg is 'm', hcd build will fail due to dependency on otg. > >>>>> > >>>>> 4) hcd 'm', gadget 'y' > >>>>> Build will fail always. > >>>>> If otg is 'y', otg build will fail due to dependency on hcd. > >>>>> If otg is 'm', gadget build will fails due to dependency on otg. > >>>>> > >>>>> So I solve this problem by adding the otg_hcd_ops and otg_gadget_ops > >>>>> to remove otg->hcd and otg->gadget dependency. > >>>>> > >>>>> Now we can address 3) and 4) like so > >>>>> > >>>>> 3) hcd 'y', gadget 'm' > >>>>> otg has to be 'y' for proper build. > >>>>> > >>>>> 4) hcd 'm', gadget 'y' > >>>>> otg has to be 'y' for proper build. > >>>>> > >>>> > >>>> How about this: > >>>> Moving usb_otg_register/unregister_hcd to host driver to remove > >>>> dependency hcd->otg. And moving usb_otg_get_data to common.c. > >>>> > >>>> Delete the wait queue at usb-otg.c, and if calling usb_otg_get_data > >>>> returns NULL, the host/device driver's probe return -EPROBE_DEFER. > >>>> When the otg driver is probed successfully, the host/device will be > >>>> re-probed again, and usb_otg_register_hcd will be called again. > >>>> > >>>> And let OTG depends on HCD && GADGET, and delete otg_hcd_ops and > >>>> otg_gadget_ops. Below build dependency issues can be fixed. > >>>> What do you think? > >>>> > >>>>> 1) hcd 'y', gadget 'y' > >>>>> otg has to be 'y' for proper build. > >>>>> > >>>>> 2) hcd 'm', gadget 'm' > >>>>> otg has to be 'm' for proper build. > >>>>> > >>>>> 3) hcd 'y', gadget 'm' > >>>>> Build will fail always. > >>>>> If otg is 'y', otg build will fail due to dependency on gadget. > >>>>> If otg is 'm', hcd build will fail due to dependency on otg. > >>>>> > >>>>> 4) hcd 'm', gadget 'y' > >>>>> Build will fail always. > >>>>> If otg is 'y', otg build will fail due to dependency on hcd. > >>>>> If otg is 'm', gadget build will fails due to dependency on otg. > >>>> -- > >>>> > >>> > >>> After thinking more, my suggestion can't work. How about moving > >>> CONFIG_USB_OTG out of CONFIG_USB, in that case, CONFIG_USB_OTG can > >>> only be built in. > >>> > >> I tried this but it still doesn't resolve 3 and 4. I.e. > >> it can't be automatically set to 'y' when either of hcd/gadget is 'y' > >> and the other is 'm'. > > > > USB_OTG only can be selected by *user*, can't be automatically set to > > 'y' like you said, no matter what's hcd/gadget config, USB_OTG can be > > disabled in any cases. > > Yes. I am on the same page with that. > > > > I think Peter's intention is to make USB_OTG to be 'bool', either disabled; > > or built-in(if enabled), something like your original idea? > > I'm fine with my original idea if you guys can confirm :). > Please do it.
On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: > @@ -530,6 +683,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget) > } > mutex_unlock(&udc_lock); > > + mutex_unlock(&udc_lock); > + Here, you have one more mutex_unlock. Peter > kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); > flush_work(&gadget->work); > device_unregister(&udc->dev); > @@ -539,6 +694,13 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc); > > /* ------------------------------------------------------------------------- */ > > +struct otg_gadget_ops otg_gadget_intf = { > + .start = usb_gadget_start, > + .stop = usb_gadget_stop, > + .connect_control = usb_gadget_connect_control, > +}; > + > +/* udc_lock must be held */ > static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver) > { > int ret; > @@ -553,12 +715,20 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri > ret = driver->bind(udc->gadget, driver); > if (ret) > goto err1; > - ret = usb_gadget_udc_start(udc); > - if (ret) { > - driver->unbind(udc->gadget); > - goto err1; > + > + /* If OTG, the otg core starts the UDC when needed */ > + if (udc->gadget->otg_dev) { > + mutex_unlock(&udc_lock); > + usb_otg_register_gadget(udc->gadget, &otg_gadget_intf); > + mutex_lock(&udc_lock); > + } else { > + ret = usb_gadget_udc_start(udc); > + if (ret) { > + driver->unbind(udc->gadget); > + goto err1; > + } > + usb_udc_connect_control(udc); > } > - usb_udc_connect_control(udc); > > kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); > return 0; > @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev, > return -EOPNOTSUPP; > } > > + /* In OTG mode we don't support softconnect, but b_bus_req */ > + if (udc->gadget->otg_dev) { > + dev_err(dev, "soft-connect not supported in OTG mode\n"); > + return -EOPNOTSUPP; > + } > + > if (sysfs_streq(buf, "connect")) { > usb_gadget_udc_start(udc); > - usb_gadget_connect(udc->gadget); > + usb_udc_connect_control(udc); > } else if (sysfs_streq(buf, "disconnect")) { > usb_gadget_disconnect(udc->gadget); > udc->driver->disconnect(udc->gadget); > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index 3ecfddd..79d654f 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -1162,6 +1162,10 @@ extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget); > extern void usb_del_gadget_udc(struct usb_gadget *gadget); > extern char *usb_get_gadget_udc_name(void); > > +extern int usb_otg_add_gadget_udc(struct device *parent, > + struct usb_gadget *gadget, > + struct device *otg_dev); > + > /*-------------------------------------------------------------------------*/ > > /* utility to simplify dealing with string descriptors */ > -- > 2.7.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-usb" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On 01/06/16 10:38, Peter Chen wrote: > On Fri, May 13, 2016 at 01:03:27PM +0300, Roger Quadros wrote: >> @@ -530,6 +683,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget) >> } >> mutex_unlock(&udc_lock); >> >> + mutex_unlock(&udc_lock); >> + > > Here, you have one more mutex_unlock. Will fix it. Thanks. >> kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); >> flush_work(&gadget->work); >> device_unregister(&udc->dev); >> @@ -539,6 +694,13 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc); >> >> /* ------------------------------------------------------------------------- */ >> >> +struct otg_gadget_ops otg_gadget_intf = { >> + .start = usb_gadget_start, >> + .stop = usb_gadget_stop, >> + .connect_control = usb_gadget_connect_control, >> +}; >> + >> +/* udc_lock must be held */ >> static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver) >> { >> int ret; >> @@ -553,12 +715,20 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri >> ret = driver->bind(udc->gadget, driver); >> if (ret) >> goto err1; >> - ret = usb_gadget_udc_start(udc); >> - if (ret) { >> - driver->unbind(udc->gadget); >> - goto err1; >> + >> + /* If OTG, the otg core starts the UDC when needed */ >> + if (udc->gadget->otg_dev) { >> + mutex_unlock(&udc_lock); >> + usb_otg_register_gadget(udc->gadget, &otg_gadget_intf); >> + mutex_lock(&udc_lock); >> + } else { >> + ret = usb_gadget_udc_start(udc); >> + if (ret) { >> + driver->unbind(udc->gadget); >> + goto err1; >> + } >> + usb_udc_connect_control(udc); >> } >> - usb_udc_connect_control(udc); >> >> kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); >> return 0; >> @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev, >> return -EOPNOTSUPP; >> } >> >> + /* In OTG mode we don't support softconnect, but b_bus_req */ >> + if (udc->gadget->otg_dev) { >> + dev_err(dev, "soft-connect not supported in OTG mode\n"); >> + return -EOPNOTSUPP; >> + } >> + >> if (sysfs_streq(buf, "connect")) { >> usb_gadget_udc_start(udc); >> - usb_gadget_connect(udc->gadget); >> + usb_udc_connect_control(udc); >> } else if (sysfs_streq(buf, "disconnect")) { >> usb_gadget_disconnect(udc->gadget); >> udc->driver->disconnect(udc->gadget); >> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h >> index 3ecfddd..79d654f 100644 >> --- a/include/linux/usb/gadget.h >> +++ b/include/linux/usb/gadget.h >> @@ -1162,6 +1162,10 @@ extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget); >> extern void usb_del_gadget_udc(struct usb_gadget *gadget); >> extern char *usb_get_gadget_udc_name(void); >> >> +extern int usb_otg_add_gadget_udc(struct device *parent, >> + struct usb_gadget *gadget, >> + struct device *otg_dev); >> + >> /*-------------------------------------------------------------------------*/ >> >> /* utility to simplify dealing with string descriptors */ >> -- >> 2.7.4 >> -- cheers, -roger -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c index 4151597..21c85ef 100644 --- a/drivers/usb/gadget/udc/udc-core.c +++ b/drivers/usb/gadget/udc/udc-core.c @@ -28,6 +28,11 @@ #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb.h> +#include <linux/usb/otg.h> +#include <linux/usb/of.h> + +#include <linux/of.h> +#include <linux/of_platform.h> /** * struct usb_udc - describes one usb device controller @@ -325,6 +330,119 @@ static inline void usb_gadget_udc_stop(struct usb_udc *udc) } /** + * usb_gadget_to_udc - get the UDC owning the gadget + * + * udc_lock must be held. + * Returs NULL if UDC is not found. + */ +static struct usb_udc *usb_gadget_to_udc(struct usb_gadget *gadget) +{ + struct usb_udc *udc; + + list_for_each_entry(udc, &udc_list, list) + if (udc->gadget == gadget) + return udc; + + return NULL; +} + +/** + * usb_gadget_start - start the usb gadget controller + * @gadget: the gadget device to start + * + * This is external API for use by OTG core. + * + * Start the usb device controller. Does not connect to the bus. + */ +static int usb_gadget_start(struct usb_gadget *gadget) +{ + int ret; + struct usb_udc *udc; + + mutex_lock(&udc_lock); + udc = usb_gadget_to_udc(gadget); + if (!udc) { + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", + __func__); + mutex_unlock(&udc_lock); + return -EINVAL; + } + + ret = usb_gadget_udc_start(udc); + if (ret) + dev_err(&udc->dev, "USB Device Controller didn't start: %d\n", + ret); + + mutex_unlock(&udc_lock); + + return ret; +} + +/** + * usb_gadget_stop - stop the usb gadget controller + * @gadget: the gadget device we want to stop + * + * This is external API for use by OTG core. + * + * Stop the gadget controller. Does not disconnect from the bus. + * Caller must ensure that gadget has disconnected from the bus + * before calling usb_gadget_stop(). + */ +static int usb_gadget_stop(struct usb_gadget *gadget) +{ + struct usb_udc *udc; + + mutex_lock(&udc_lock); + udc = usb_gadget_to_udc(gadget); + if (!udc) { + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", + __func__); + mutex_unlock(&udc_lock); + return -EINVAL; + } + + if (gadget->connected) { + dev_err(gadget->dev.parent, + "%s: called while still connected\n", __func__); + mutex_unlock(&udc_lock); + return -EINVAL; + } + + usb_gadget_udc_stop(udc); + mutex_unlock(&udc_lock); + + return 0; +} + +static int usb_gadget_connect_control(struct usb_gadget *gadget, bool connect) +{ + struct usb_udc *udc; + + mutex_lock(&udc_lock); + udc = usb_gadget_to_udc(gadget); + if (!udc) { + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", + __func__); + mutex_unlock(&udc_lock); + return -EINVAL; + } + + if (connect) { + if (!gadget->connected) + usb_gadget_connect(udc->gadget); + } else { + if (gadget->connected) { + usb_gadget_disconnect(udc->gadget); + udc->driver->disconnect(udc->gadget); + } + } + + mutex_unlock(&udc_lock); + + return 0; +} + +/** * usb_udc_release - release the usb_udc struct * @dev: the dev member within usb_udc * @@ -486,6 +604,33 @@ int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget) } EXPORT_SYMBOL_GPL(usb_add_gadget_udc); +/** + * usb_otg_add_gadget_udc - adds a new gadget to the udc class driver list + * @parent: the parent device to this udc. Usually the controller + * driver's device. + * @gadget: the gadget to be added to the list + * @otg_dev: the OTG controller device + * + * If otg_dev is NULL then device tree node is checked + * for OTG controller via the otg-controller property. + * Returns zero on success, negative errno otherwise. + */ +int usb_otg_add_gadget_udc(struct device *parent, struct usb_gadget *gadget, + struct device *otg_dev) +{ + if (!otg_dev) { + gadget->otg_dev = of_usb_get_otg(parent->of_node); + if (!gadget->otg_dev) + return -ENODEV; + } else { + gadget->otg_dev = otg_dev; + } + + return usb_add_gadget_udc_release(parent, gadget, NULL); +} +EXPORT_SYMBOL_GPL(usb_otg_add_gadget_udc); + +/* udc_lock must be held */ static void usb_gadget_remove_driver(struct usb_udc *udc) { dev_dbg(&udc->dev, "unregistering UDC driver [%s]\n", @@ -493,10 +638,18 @@ static void usb_gadget_remove_driver(struct usb_udc *udc) kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); - usb_gadget_disconnect(udc->gadget); - udc->driver->disconnect(udc->gadget); + /* If OTG, the otg core ensures UDC is stopped on unregister */ + if (udc->gadget->otg_dev) { + mutex_unlock(&udc_lock); + usb_otg_unregister_gadget(udc->gadget); + mutex_lock(&udc_lock); + } else { + usb_gadget_disconnect(udc->gadget); + udc->driver->disconnect(udc->gadget); + usb_gadget_udc_stop(udc); + } + udc->driver->unbind(udc->gadget); - usb_gadget_udc_stop(udc); udc->driver = NULL; udc->dev.driver = NULL; @@ -530,6 +683,8 @@ void usb_del_gadget_udc(struct usb_gadget *gadget) } mutex_unlock(&udc_lock); + mutex_unlock(&udc_lock); + kobject_uevent(&udc->dev.kobj, KOBJ_REMOVE); flush_work(&gadget->work); device_unregister(&udc->dev); @@ -539,6 +694,13 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc); /* ------------------------------------------------------------------------- */ +struct otg_gadget_ops otg_gadget_intf = { + .start = usb_gadget_start, + .stop = usb_gadget_stop, + .connect_control = usb_gadget_connect_control, +}; + +/* udc_lock must be held */ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver) { int ret; @@ -553,12 +715,20 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri ret = driver->bind(udc->gadget, driver); if (ret) goto err1; - ret = usb_gadget_udc_start(udc); - if (ret) { - driver->unbind(udc->gadget); - goto err1; + + /* If OTG, the otg core starts the UDC when needed */ + if (udc->gadget->otg_dev) { + mutex_unlock(&udc_lock); + usb_otg_register_gadget(udc->gadget, &otg_gadget_intf); + mutex_lock(&udc_lock); + } else { + ret = usb_gadget_udc_start(udc); + if (ret) { + driver->unbind(udc->gadget); + goto err1; + } + usb_udc_connect_control(udc); } - usb_udc_connect_control(udc); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0; @@ -660,9 +830,15 @@ static ssize_t usb_udc_softconn_store(struct device *dev, return -EOPNOTSUPP; } + /* In OTG mode we don't support softconnect, but b_bus_req */ + if (udc->gadget->otg_dev) { + dev_err(dev, "soft-connect not supported in OTG mode\n"); + return -EOPNOTSUPP; + } + if (sysfs_streq(buf, "connect")) { usb_gadget_udc_start(udc); - usb_gadget_connect(udc->gadget); + usb_udc_connect_control(udc); } else if (sysfs_streq(buf, "disconnect")) { usb_gadget_disconnect(udc->gadget); udc->driver->disconnect(udc->gadget); diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 3ecfddd..79d654f 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -1162,6 +1162,10 @@ extern int usb_add_gadget_udc(struct device *parent, struct usb_gadget *gadget); extern void usb_del_gadget_udc(struct usb_gadget *gadget); extern char *usb_get_gadget_udc_name(void); +extern int usb_otg_add_gadget_udc(struct device *parent, + struct usb_gadget *gadget, + struct device *otg_dev); + /*-------------------------------------------------------------------------*/ /* utility to simplify dealing with string descriptors */
The OTG state machine needs a mechanism to start and stop the gadget controller as well as connect/disconnect from the bus. Add usb_gadget_start(), usb_gadget_stop() and usb_gadget_connect_control(). Introduce usb_otg_add_gadget_udc() to allow controller drivers to register a gadget controller that is part of an OTG instance. Register with OTG core when gadget function driver is available and unregister when function driver is unbound. We need to unlock the usb_lock mutex before calling usb_otg_register_gadget() in udc_bind_to_driver() and usb_gadget_remove_driver() else it will cause a circular locking dependency. Ignore softconnect sysfs control when we're in OTG mode as OTG FSM takes care of gadget softconnect using the b_bus_req mechanism. Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/gadget/udc/udc-core.c | 194 ++++++++++++++++++++++++++++++++++++-- include/linux/usb/gadget.h | 4 + 2 files changed, 189 insertions(+), 9 deletions(-)