| Message ID | 1463133808-10630-14-git-send-email-rogerq@ti.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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 >>>>>
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
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 >>>>>>>
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
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(-) -- 2.7.4