| Message ID | 1459865117-7032-10-git-send-email-rogerq@ti.com |
|---|---|
| State | New |
| Headers | show |
On 18/04/16 09:59, Peter Chen wrote: > On Tue, Apr 05, 2016 at 05:05:14PM +0300, Roger Quadros wrote: >> The OTG state machine needs a mechanism to start and >> stop the gadget controller. Add usb_gadget_start() >> and usb_gadget_stop(). >> >> 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 mutexbefore calling > > ...mutex before... OK. > >> 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 | 166 +++++++++++++++++++++++++++++++++++--- >> include/linux/usb/gadget.h | 4 + >> 2 files changed, 161 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c >> index 4151597..9b9702f 100644 >> --- a/drivers/usb/gadget/udc/udc-core.c >> +++ b/drivers/usb/gadget/udc/udc-core.c >> @@ -28,6 +28,10 @@ >> #include <linux/usb/ch9.h> >> #include <linux/usb/gadget.h> >> #include <linux/usb.h> >> +#include <linux/usb/otg.h> >> + >> +#include <linux/of.h> >> +#include <linux/of_platform.h> >> >> /** >> * struct usb_udc - describes one usb device controller >> @@ -304,6 +308,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); >> */ >> static inline int usb_gadget_udc_start(struct usb_udc *udc) >> { >> + dev_dbg(&udc->dev, "%s\n", __func__); >> return udc->gadget->ops->udc_start(udc->gadget, udc->driver); >> } > > You may delete the debug message next time. OK for this and all the next comments to remove debug messages. > >> >> @@ -321,10 +326,81 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) >> */ >> static inline void usb_gadget_udc_stop(struct usb_udc *udc) >> { >> + dev_dbg(&udc->dev, "%s\n", __func__); >> udc->gadget->ops->udc_stop(udc->gadget); >> } > > The same. > >> >> /** >> + * usb_gadget_start - start the usb gadget controller and connect to bus >> + * @gadget: the gadget device to start >> + * >> + * This is external API for use by OTG core. >> + * >> + * Start the usb device controller and connect to bus (enable pull). >> + */ >> +static int usb_gadget_start(struct usb_gadget *gadget) >> +{ >> + int ret; >> + struct usb_udc *udc = NULL; >> + >> + dev_dbg(&gadget->dev, "%s\n", __func__); > > The same > >> + mutex_lock(&udc_lock); >> + list_for_each_entry(udc, &udc_list, list) >> + if (udc->gadget == gadget) >> + goto found; >> + >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >> + __func__); >> + mutex_unlock(&udc_lock); >> + return -EINVAL; >> + >> +found: >> + ret = usb_gadget_udc_start(udc); >> + if (ret) >> + dev_err(&udc->dev, "USB Device Controller didn't start: %d\n", >> + ret); >> + else >> + usb_udc_connect_control(udc); >> + >> + mutex_unlock(&udc_lock); >> + >> + return ret; >> +} >> + >> +/** >> + * usb_gadget_stop - disconnect from bus and stop the usb gadget >> + * @gadget: The gadget device we want to stop >> + * >> + * This is external API for use by OTG core. >> + * >> + * Disconnect from the bus (disable pull) and stop the >> + * gadget controller. >> + */ >> +static int usb_gadget_stop(struct usb_gadget *gadget) >> +{ >> + struct usb_udc *udc = NULL; >> + >> + dev_dbg(&gadget->dev, "%s\n", __func__); > > The same > >> + mutex_lock(&udc_lock); >> + list_for_each_entry(udc, &udc_list, list) >> + if (udc->gadget == gadget) >> + goto found; >> + >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >> + __func__); >> + mutex_unlock(&udc_lock); >> + return -EINVAL; > > The above finding gadget code is the same with usb_gadget_start, can we > use one common API to instead of it? Sure. > >> + >> +found: >> + usb_gadget_disconnect(udc->gadget); >> + udc->driver->disconnect(udc->gadget); >> + usb_gadget_udc_stop(udc); >> + mutex_unlock(&udc_lock); >> + >> + return 0; >> +} >> + >> +/** >> * usb_udc_release - release the usb_udc struct >> * @dev: the dev member within usb_udc >> * >> @@ -486,6 +562,48 @@ 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) { >> + struct device_node *np; >> + struct platform_device *pdev; >> + >> + np = of_parse_phandle(parent->of_node, "otg-controller", 0); >> + if (!np) { >> + dev_err(parent, >> + "otg_dev is NULL or no otg-controller property in DT\n"); >> + return -EINVAL; >> + } >> + >> + pdev = of_find_device_by_node(np); >> + of_node_put(np); >> + if (!pdev) { >> + dev_err(parent, "couldn't get otg-controller device\n"); >> + return -ENODEV; >> + } >> + >> + gadget->otg_dev = &pdev->dev; >> + } else { >> + gadget->otg_dev = otg_dev; >> + } > > The above the code is duplicated with hcd's. Can we do something common > at usb-otg.c, eg define an API get_usb_otg_dev(struct usb_gadget *gadget, struct > usb_hcd *hcd), in this API we can try to get otg_dev for both gadget and > hcd, and we can call it at controller driver during register otg. > Good point. I'll address it in v7. cheers, -roger
Hi, On 21/04/16 09:38, Jun Li wrote: > Hi, > > ... >> >> /** >> + * usb_gadget_start - start the usb gadget controller and connect to >> +bus >> + * @gadget: the gadget device to start >> + * >> + * This is external API for use by OTG core. >> + * >> + * Start the usb device controller and connect to bus (enable pull). >> + */ >> +static int usb_gadget_start(struct usb_gadget *gadget) { >> + int ret; >> + struct usb_udc *udc = NULL; >> + >> + dev_dbg(&gadget->dev, "%s\n", __func__); >> + mutex_lock(&udc_lock); >> + list_for_each_entry(udc, &udc_list, list) >> + if (udc->gadget == gadget) >> + goto found; >> + >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >> + __func__); >> + mutex_unlock(&udc_lock); >> + return -EINVAL; >> + >> +found: >> + ret = usb_gadget_udc_start(udc); >> + if (ret) >> + dev_err(&udc->dev, "USB Device Controller didn't start: %d\n", >> + ret); >> + else >> + usb_udc_connect_control(udc); > > For drd, it's fine, but for real otg, gadget connect should be done > by loc_conn() instead of gadget start. It is upto the OTG state machine to call gadget_start() when it needs to connect to the bus (i.e. loc_conn()). I see no point in calling gadget start before. Do you see any issue in doing so? cheers, -roger > >> + >> + mutex_unlock(&udc_lock); >> + >> + return ret; >> +} >> + >> +/** >> + * usb_gadget_stop - disconnect from bus and stop the usb gadget >> + * @gadget: The gadget device we want to stop >> + * >> + * This is external API for use by OTG core. >> + * >> + * Disconnect from the bus (disable pull) and stop the >> + * gadget controller. >> + */ >> +static int usb_gadget_stop(struct usb_gadget *gadget) { >> + struct usb_udc *udc = NULL; >> + >> + dev_dbg(&gadget->dev, "%s\n", __func__); >> + mutex_lock(&udc_lock); >> + list_for_each_entry(udc, &udc_list, list) >> + if (udc->gadget == gadget) >> + goto found; >> + >> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >> + __func__); >> + mutex_unlock(&udc_lock); >> + return -EINVAL; >> + >> +found: >> + usb_gadget_disconnect(udc->gadget); > > Likewise, gadget disconnect also should be done by loc_conn() > instead of gadget stop. > >> + udc->driver->disconnect(udc->gadget); >> + usb_gadget_udc_stop(udc); >> + mutex_unlock(&udc_lock); >> + >> + return 0; >> +} >> + > > Li Jun >
On 28/04/16 13:23, Jun Li wrote: > Hi > >> -----Original Message----- >> From: Roger Quadros [mailto:rogerq@ti.com] >> Sent: Thursday, April 28, 2016 5:55 PM >> To: Jun Li <jun.li@nxp.com>; stern@rowland.harvard.edu; balbi@kernel.org; >> gregkh@linuxfoundation.org; peter.chen@freescale.com >> Cc: dan.j.williams@intel.com; jun.li@freescale.com; >> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com; >> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org; >> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org >> Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core >> >> Hi, >> >> On 27/04/16 14:22, Roger Quadros wrote: >>> On 26/04/16 03:07, Jun Li wrote: >>>> Hi >>>> >>>>> -----Original Message----- >>>>> From: Roger Quadros [mailto:rogerq@ti.com] >>>>> Sent: Monday, April 25, 2016 10:04 PM >>>>> To: Jun Li <jun.li@nxp.com>; stern@rowland.harvard.edu; >>>>> balbi@kernel.org; gregkh@linuxfoundation.org; >>>>> peter.chen@freescale.com >>>>> Cc: dan.j.williams@intel.com; jun.li@freescale.com; >>>>> mathias.nyman@linux.intel.com; tony@atomide.com; >>>>> Joao.Pinto@synopsys.com; abrestic@chromium.org; >>>>> r.baldyga@samsung.com; linux-usb@vger.kernel.org; >>>>> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org >>>>> Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core >>>>> >>>>> Hi, >>>>> >>>>> On 21/04/16 09:38, Jun Li wrote: >>>>>> Hi, >>>>>> >>>>>> ... >>>>>>> >>>>>>> /** >>>>>>> + * usb_gadget_start - start the usb gadget controller and connect >>>>>>> +to bus >>>>>>> + * @gadget: the gadget device to start >>>>>>> + * >>>>>>> + * This is external API for use by OTG core. >>>>>>> + * >>>>>>> + * Start the usb device controller and connect to bus (enable pull). >>>>>>> + */ >>>>>>> +static int usb_gadget_start(struct usb_gadget *gadget) { >>>>>>> + int ret; >>>>>>> + struct usb_udc *udc = NULL; >>>>>>> + >>>>>>> + dev_dbg(&gadget->dev, "%s\n", __func__); >>>>>>> + mutex_lock(&udc_lock); >>>>>>> + list_for_each_entry(udc, &udc_list, list) >>>>>>> + if (udc->gadget == gadget) >>>>>>> + goto found; >>>>>>> + >>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >>>>>>> + __func__); >>>>>>> + mutex_unlock(&udc_lock); >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> +found: >>>>>>> + ret = usb_gadget_udc_start(udc); >>>>>>> + if (ret) >>>>>>> + dev_err(&udc->dev, "USB Device Controller didn't >> start: %d\n", >>>>>>> + ret); >>>>>>> + else >>>>>>> + usb_udc_connect_control(udc); >>>>>> >>>>>> For drd, it's fine, but for real otg, gadget connect should be done >>>>>> by >>>>>> loc_conn() instead of gadget start. >>>>> >>>>> It is upto the OTG state machine to call gadget_start() when it >>>>> needs to connect to the bus (i.e. loc_conn()). I see no point in >>>>> calling gadget start before. >>>>> >>>>> Do you see any issue in doing so? >>>> >>>> This is what OTG state machine does: >>>> 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; >> >> On second thoughts, after seen the OTG state machine. >> otg_set_protocol(fsm, PROTO_GADGET); is always followed by >> otg_loc_conn(otg, 1); And whenever protocol changes to anything other the >> PROTO_GADGET, we use otg_loc_conn(otg, 0); >> >> So otg_loc_conn seems redundant. Can we just get rid of it? >> >> usb_gadget_start() implies that gadget controller starts up and enables >> pull. >> usb_gadget_stop() implies that gadget controller disables pull and stops. >> >> >> Can you please explain why just these 2 APIs are not sufficient for full >> OTG? >> >> Do we want anything to happen between gadget controller start/stop and >> pull on/off? > > "loc_conn" is a standard output parameter in OTG spec, it deserves > a separate api, yes, current implementation of OTG state machine code > seems allow you to combine the 2 things into one, but don't do that, > because they do not always happen together, e.g. for peripheral only > B device (also a part OTG spec: section 7.3), will be fixed in gadget > mode, but it will do gadget connect and disconnect in its diff states, > so, to make the framework common, let's keep them separated. I'm sorry but I didn't understand your comment about "it will do gadget connect and disconnect in its diff states" I am reading the OTG v2.0 specification and loc_conn is always true when b_peripheral or a_peripheral is true and false otherwise. loc_conn is just an internal state variable and it corresponds to our gadget_start/stop() state. As per 7.4.2.3 "loc_conn The “local connect” (loc_conn) variable is TRUE when the local device has signaled that it is connected to the bus. This variable is FALSE when the local device has signaled that it is disconnected from the bus" Can you please point me in the specification if there is any place where loc_conn is false and b_peripheral/a_peripheral is true? cheers, -roger
Hi, On 03/05/16 10:06, Jun Li wrote: > Hi > >>>>>>>>> /** >>>>>>>>> + * usb_gadget_start - start the usb gadget controller and >>>>>>>>> +connect to bus >>>>>>>>> + * @gadget: the gadget device to start >>>>>>>>> + * >>>>>>>>> + * This is external API for use by OTG core. >>>>>>>>> + * >>>>>>>>> + * Start the usb device controller and connect to bus (enable >> pull). >>>>>>>>> + */ >>>>>>>>> +static int usb_gadget_start(struct usb_gadget *gadget) { >>>>>>>>> + int ret; >>>>>>>>> + struct usb_udc *udc = NULL; >>>>>>>>> + >>>>>>>>> + dev_dbg(&gadget->dev, "%s\n", __func__); >>>>>>>>> + mutex_lock(&udc_lock); >>>>>>>>> + list_for_each_entry(udc, &udc_list, list) >>>>>>>>> + if (udc->gadget == gadget) >>>>>>>>> + goto found; >>>>>>>>> + >>>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >>>>>>>>> + __func__); >>>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>>> + return -EINVAL; >>>>>>>>> + >>>>>>>>> +found: >>>>>>>>> + ret = usb_gadget_udc_start(udc); >>>>>>>>> + if (ret) >>>>>>>>> + dev_err(&udc->dev, "USB Device Controller didn't >>>> start: %d\n", >>>>>>>>> + ret); >>>>>>>>> + else >>>>>>>>> + usb_udc_connect_control(udc); >>>>>>>> >>>>>>>> For drd, it's fine, but for real otg, gadget connect should be >>>>>>>> done by >>>>>>>> loc_conn() instead of gadget start. >>>>>>> >>>>>>> It is upto the OTG state machine to call gadget_start() when it >>>>>>> needs to connect to the bus (i.e. loc_conn()). I see no point in >>>>>>> calling gadget start before. >>>>>>> >>>>>>> Do you see any issue in doing so? >>>>>> >>>>>> This is what OTG state machine does: >>>>>> 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; >>>> >>>> On second thoughts, after seen the OTG state machine. >>>> otg_set_protocol(fsm, PROTO_GADGET); is always followed by >>>> otg_loc_conn(otg, 1); And whenever protocol changes to anything other >>>> the PROTO_GADGET, we use otg_loc_conn(otg, 0); >>>> >>>> So otg_loc_conn seems redundant. Can we just get rid of it? >>>> >>>> usb_gadget_start() implies that gadget controller starts up and >>>> enables pull. >>>> usb_gadget_stop() implies that gadget controller disables pull and >> stops. >>>> >>>> >>>> Can you please explain why just these 2 APIs are not sufficient for >>>> full OTG? >>>> >>>> Do we want anything to happen between gadget controller start/stop >>>> and pull on/off? >>> >>> "loc_conn" is a standard output parameter in OTG spec, it deserves a >>> separate api, yes, current implementation of OTG state machine code >>> seems allow you to combine the 2 things into one, but don't do that, >>> because they do not always happen together, e.g. for peripheral only B >>> device (also a part OTG spec: section 7.3), will be fixed in gadget >>> mode, but it will do gadget connect and disconnect in its diff states, >>> so, to make the framework common, let's keep them separated. >> >> I'm sorry but I didn't understand your comment about "it will do gadget >> connect and disconnect in its diff states" > > Gadget connect means loc_conn(1). > >> >> I am reading the OTG v2.0 specification and loc_conn is always true when >> b_peripheral or a_peripheral is true and false otherwise. > > If you only talk about these 2 states, yes, loc_conn is ture. > >> >> loc_conn is just an internal state variable and it corresponds to our >> gadget_start/stop() state. > > It's not an internal variable, there are OTG state machine > parameters tables(table 7-x) in OTG spec which have clear lists > which are "internal variable", which are "input", which are "output"... > > Those APIs are driven directly from OTG spec, easily understood so > code reader can know what's those APIs for. For real OTG, I don't > see the benefit if get rid of it. OK, no issues if we don't get rid of it. But I am still in favor of doing a connect in usb_gadget_start(), because 1) If we split connect/disconnect() and usb_gadget_start/stop() then there is additional overhead of keeping track whether connect was called or not during usb_gadget_stop(). Plus we need to take care that users don't call connect/disconnect outside of start/stop. It is just complicating things. 2) for many controllers there is no difference between run/stop and connect/disconnect. i.e. a single register bit controls both. 3) it fits well with the OTG specification. OTG specification says that loc_conn *variable* must be true *after* the device has signalled a connect. So OTG state machine can safely set loc_conn variable to true after doing otg_set_protocol(fsm, PROTO_GADGET); and set it to false otherwise. Note, OTG specification does not say to take any action based on loc_conn. It is just a connect indicator variable. So we might have to fix this in the OTG state machine. My suggestion is to keep it simple for now. Try the OTG implementation, and later if we find issues then extend it as required. cheers, -roger > >> >> As per 7.4.2.3 >> "loc_conn >> The "local connect" (loc_conn) variable is TRUE when the local device has >> signaled that it is connected to the bus. This variable is FALSE when the >> local device has signaled that it is disconnected from the bus" >> >> Can you please point me in the specification if there is any place where >> loc_conn is false and b_peripheral/a_peripheral is true? >> >> cheers, >> -roger
Peter, On 04/05/16 06:35, Peter Chen wrote: > On Tue, May 03, 2016 at 06:44:46PM +0300, Roger Quadros wrote: >> Hi, >> >> On 03/05/16 10:06, Jun Li wrote: >>> Hi >>> >>>>>>>>>>> /** >>>>>>>>>>> + * usb_gadget_start - start the usb gadget controller and >>>>>>>>>>> +connect to bus >>>>>>>>>>> + * @gadget: the gadget device to start >>>>>>>>>>> + * >>>>>>>>>>> + * This is external API for use by OTG core. >>>>>>>>>>> + * >>>>>>>>>>> + * Start the usb device controller and connect to bus (enable >>>> pull). >>>>>>>>>>> + */ >>>>>>>>>>> +static int usb_gadget_start(struct usb_gadget *gadget) { >>>>>>>>>>> + int ret; >>>>>>>>>>> + struct usb_udc *udc = NULL; >>>>>>>>>>> + >>>>>>>>>>> + dev_dbg(&gadget->dev, "%s\n", __func__); >>>>>>>>>>> + mutex_lock(&udc_lock); >>>>>>>>>>> + list_for_each_entry(udc, &udc_list, list) >>>>>>>>>>> + if (udc->gadget == gadget) >>>>>>>>>>> + goto found; >>>>>>>>>>> + >>>>>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", >>>>>>>>>>> + __func__); >>>>>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>>>>> + return -EINVAL; >>>>>>>>>>> + >>>>>>>>>>> +found: >>>>>>>>>>> + ret = usb_gadget_udc_start(udc); >>>>>>>>>>> + if (ret) >>>>>>>>>>> + dev_err(&udc->dev, "USB Device Controller didn't >>>>>> start: %d\n", >>>>>>>>>>> + ret); >>>>>>>>>>> + else >>>>>>>>>>> + usb_udc_connect_control(udc); >>>>>>>>>> >>>>>>>>>> For drd, it's fine, but for real otg, gadget connect should be >>>>>>>>>> done by >>>>>>>>>> loc_conn() instead of gadget start. >>>>>>>>> >>>>>>>>> It is upto the OTG state machine to call gadget_start() when it >>>>>>>>> needs to connect to the bus (i.e. loc_conn()). I see no point in >>>>>>>>> calling gadget start before. >>>>>>>>> >>>>>>>>> Do you see any issue in doing so? >>>>>>>> >>>>>>>> This is what OTG state machine does: >>>>>>>> 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; >>>>>> >>>>>> On second thoughts, after seen the OTG state machine. >>>>>> otg_set_protocol(fsm, PROTO_GADGET); is always followed by >>>>>> otg_loc_conn(otg, 1); And whenever protocol changes to anything other >>>>>> the PROTO_GADGET, we use otg_loc_conn(otg, 0); >>>>>> >>>>>> So otg_loc_conn seems redundant. Can we just get rid of it? >>>>>> >>>>>> usb_gadget_start() implies that gadget controller starts up and >>>>>> enables pull. >>>>>> usb_gadget_stop() implies that gadget controller disables pull and >>>> stops. >>>>>> >>>>>> >>>>>> Can you please explain why just these 2 APIs are not sufficient for >>>>>> full OTG? >>>>>> >>>>>> Do we want anything to happen between gadget controller start/stop >>>>>> and pull on/off? >>>>> >>>>> "loc_conn" is a standard output parameter in OTG spec, it deserves a >>>>> separate api, yes, current implementation of OTG state machine code >>>>> seems allow you to combine the 2 things into one, but don't do that, >>>>> because they do not always happen together, e.g. for peripheral only B >>>>> device (also a part OTG spec: section 7.3), will be fixed in gadget >>>>> mode, but it will do gadget connect and disconnect in its diff states, >>>>> so, to make the framework common, let's keep them separated. >>>> >>>> I'm sorry but I didn't understand your comment about "it will do gadget >>>> connect and disconnect in its diff states" >>> >>> Gadget connect means loc_conn(1). >>> >>>> >>>> I am reading the OTG v2.0 specification and loc_conn is always true when >>>> b_peripheral or a_peripheral is true and false otherwise. >>> >>> If you only talk about these 2 states, yes, loc_conn is ture. >>> >>>> >>>> loc_conn is just an internal state variable and it corresponds to our >>>> gadget_start/stop() state. >>> >>> It's not an internal variable, there are OTG state machine >>> parameters tables(table 7-x) in OTG spec which have clear lists >>> which are "internal variable", which are "input", which are "output"... >>> >>> Those APIs are driven directly from OTG spec, easily understood so >>> code reader can know what's those APIs for. For real OTG, I don't >>> see the benefit if get rid of it. >> >> OK, no issues if we don't get rid of it. But I am still in favor of >> doing a connect in usb_gadget_start(), because >> >> 1) If we split connect/disconnect() and usb_gadget_start/stop() then there is >> additional overhead of keeping track whether connect was called or not during >> usb_gadget_stop(). Plus we need to take care that users don't call connect/disconnect >> outside of start/stop. It is just complicating things. >> >> 2) for many controllers there is no difference between run/stop and >> connect/disconnect. i.e. a single register bit controls both. >> >> 3) it fits well with the OTG specification. OTG specification says >> that loc_conn *variable* must be true *after* the device has signalled a connect. >> So OTG state machine can safely set loc_conn variable to true after doing >> otg_set_protocol(fsm, PROTO_GADGET); and set it to false otherwise. >> >> Note, OTG specification does not say to take any action based on loc_conn. >> It is just a connect indicator variable. So we might have to fix this in the >> OTG state machine. >> >> My suggestion is to keep it simple for now. Try the OTG implementation, >> and later if we find issues then extend it as required. >> > > Just talked with Jun, he is worried if loc_conn != pullup_dp at some > situations. So, how about only calling start gadget at usb_start_gadget, Which situations? > and pullup_dp at drd_set_state (see below). > > > static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state new_state) > { > struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm); > > if (otg->state == new_state) > return; > > fsm->state_changed = 1; > dev_dbg(otg->dev, "otg: set state: %s\n", > usb_otg_state_string(new_state)); > switch (new_state) { > case OTG_STATE_B_IDLE: > + usb_udc_vbus_handler(gadget, false); This is redundant, When we switch from PROTO_GADGET to PROTO_UNDEF we do a usb_gadget_stop(), and a usb_gadget_disconnect() is done there. > drd_set_protocol(fsm, PROTO_UNDEF); > otg_drv_vbus(otg, 0); > break; > case OTG_STATE_B_PERIPHERAL: > drd_set_protocol(fsm, PROTO_GADGET); > + usb_udc_vbus_handler(gadget, true); This is redundant as well since usb_gadget_start() is doing a usb_gadget_connect(). > otg_drv_vbus(otg, 0); > break; > ...... > }; > > } > > When the OTG FSM is added to this framework, it can keep usb_fsm->ops->loc_conn, > and using the current FSM. > I have no strong opinions for or against usb_fsm->ops->loc_conn. Although strictly speaking, we shouldn't take any action based on loc_conn. It is just a state variable indicator. cheers, -roger
On 04/05/16 11:03, Jun Li wrote: > Hi > >> -----Original Message----- >> From: Roger Quadros [mailto:rogerq@ti.com] >> Sent: Wednesday, May 04, 2016 2:37 PM >> To: Peter Chen <hzpeterchen@gmail.com> >> Cc: Jun Li <jun.li@nxp.com>; stern@rowland.harvard.edu; balbi@kernel.org; >> gregkh@linuxfoundation.org; peter.chen@freescale.com; >> dan.j.williams@intel.com; jun.li@freescale.com; >> mathias.nyman@linux.intel.com; tony@atomide.com; Joao.Pinto@synopsys.com; >> abrestic@chromium.org; r.baldyga@samsung.com; linux-usb@vger.kernel.org; >> linux-kernel@vger.kernel.org; linux-omap@vger.kernel.org >> Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core >> >> Peter, >> >> On 04/05/16 06:35, Peter Chen wrote: >>> On Tue, May 03, 2016 at 06:44:46PM +0300, Roger Quadros wrote: >>>> Hi, >>>> >>>> On 03/05/16 10:06, Jun Li wrote: >>>>> Hi >>>>> >>>>>>>>>>>>> /** >>>>>>>>>>>>> + * usb_gadget_start - start the usb gadget controller and >>>>>>>>>>>>> +connect to bus >>>>>>>>>>>>> + * @gadget: the gadget device to start >>>>>>>>>>>>> + * >>>>>>>>>>>>> + * This is external API for use by OTG core. >>>>>>>>>>>>> + * >>>>>>>>>>>>> + * Start the usb device controller and connect to bus >>>>>>>>>>>>> +(enable >>>>>> pull). >>>>>>>>>>>>> + */ >>>>>>>>>>>>> +static int usb_gadget_start(struct usb_gadget *gadget) { >>>>>>>>>>>>> + int ret; >>>>>>>>>>>>> + struct usb_udc *udc = NULL; >>>>>>>>>>>>> + >>>>>>>>>>>>> + dev_dbg(&gadget->dev, "%s\n", __func__); >>>>>>>>>>>>> + mutex_lock(&udc_lock); >>>>>>>>>>>>> + list_for_each_entry(udc, &udc_list, list) >>>>>>>>>>>>> + if (udc->gadget == gadget) >>>>>>>>>>>>> + goto found; >>>>>>>>>>>>> + >>>>>>>>>>>>> + dev_err(gadget->dev.parent, "%s: gadget not >> registered.\n", >>>>>>>>>>>>> + __func__); >>>>>>>>>>>>> + mutex_unlock(&udc_lock); >>>>>>>>>>>>> + return -EINVAL; >>>>>>>>>>>>> + >>>>>>>>>>>>> +found: >>>>>>>>>>>>> + ret = usb_gadget_udc_start(udc); >>>>>>>>>>>>> + if (ret) >>>>>>>>>>>>> + dev_err(&udc->dev, "USB Device Controller didn't >>>>>>>> start: %d\n", >>>>>>>>>>>>> + ret); >>>>>>>>>>>>> + else >>>>>>>>>>>>> + usb_udc_connect_control(udc); >>>>>>>>>>>> >>>>>>>>>>>> For drd, it's fine, but for real otg, gadget connect should >>>>>>>>>>>> be done by >>>>>>>>>>>> loc_conn() instead of gadget start. >>>>>>>>>>> >>>>>>>>>>> It is upto the OTG state machine to call gadget_start() when >>>>>>>>>>> it needs to connect to the bus (i.e. loc_conn()). I see no >>>>>>>>>>> point in calling gadget start before. >>>>>>>>>>> >>>>>>>>>>> Do you see any issue in doing so? >>>>>>>>>> >>>>>>>>>> This is what OTG state machine does: >>>>>>>>>> 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; >>>>>>>> >>>>>>>> On second thoughts, after seen the OTG state machine. >>>>>>>> otg_set_protocol(fsm, PROTO_GADGET); is always followed by >>>>>>>> otg_loc_conn(otg, 1); And whenever protocol changes to anything >>>>>>>> other the PROTO_GADGET, we use otg_loc_conn(otg, 0); >>>>>>>> >>>>>>>> So otg_loc_conn seems redundant. Can we just get rid of it? >>>>>>>> >>>>>>>> usb_gadget_start() implies that gadget controller starts up and >>>>>>>> enables pull. >>>>>>>> usb_gadget_stop() implies that gadget controller disables pull >>>>>>>> and >>>>>> stops. >>>>>>>> >>>>>>>> >>>>>>>> Can you please explain why just these 2 APIs are not sufficient >>>>>>>> for full OTG? >>>>>>>> >>>>>>>> Do we want anything to happen between gadget controller >>>>>>>> start/stop and pull on/off? >>>>>>> >>>>>>> "loc_conn" is a standard output parameter in OTG spec, it deserves >>>>>>> a separate api, yes, current implementation of OTG state machine >>>>>>> code seems allow you to combine the 2 things into one, but don't >>>>>>> do that, because they do not always happen together, e.g. for >>>>>>> peripheral only B device (also a part OTG spec: section 7.3), will >>>>>>> be fixed in gadget mode, but it will do gadget connect and >>>>>>> disconnect in its diff states, so, to make the framework common, >> let's keep them separated. >>>>>> >>>>>> I'm sorry but I didn't understand your comment about "it will do >>>>>> gadget connect and disconnect in its diff states" >>>>> >>>>> Gadget connect means loc_conn(1). >>>>> >>>>>> >>>>>> I am reading the OTG v2.0 specification and loc_conn is always true >>>>>> when b_peripheral or a_peripheral is true and false otherwise. >>>>> >>>>> If you only talk about these 2 states, yes, loc_conn is ture. >>>>> >>>>>> >>>>>> loc_conn is just an internal state variable and it corresponds to >>>>>> our >>>>>> gadget_start/stop() state. >>>>> >>>>> It's not an internal variable, there are OTG state machine >>>>> parameters tables(table 7-x) in OTG spec which have clear lists >>>>> which are "internal variable", which are "input", which are "output"... >>>>> >>>>> Those APIs are driven directly from OTG spec, easily understood so >>>>> code reader can know what's those APIs for. For real OTG, I don't >>>>> see the benefit if get rid of it. >>>> >>>> OK, no issues if we don't get rid of it. But I am still in favor of >>>> doing a connect in usb_gadget_start(), because >>>> >>>> 1) If we split connect/disconnect() and usb_gadget_start/stop() then >>>> there is additional overhead of keeping track whether connect was >>>> called or not during usb_gadget_stop(). Plus we need to take care >>>> that users don't call connect/disconnect outside of start/stop. It is >> just complicating things. >>>> >>>> 2) for many controllers there is no difference between run/stop and >>>> connect/disconnect. i.e. a single register bit controls both. >>>> >>>> 3) it fits well with the OTG specification. OTG specification says >>>> that loc_conn *variable* must be true *after* the device has signalled >> a connect. >>>> So OTG state machine can safely set loc_conn variable to true after >>>> doing otg_set_protocol(fsm, PROTO_GADGET); and set it to false >> otherwise. >>>> >>>> Note, OTG specification does not say to take any action based on >> loc_conn. >>>> It is just a connect indicator variable. So we might have to fix this >>>> in the OTG state machine. >>>> >>>> My suggestion is to keep it simple for now. Try the OTG >>>> implementation, and later if we find issues then extend it as required. >>>> >>> >>> Just talked with Jun, he is worried if loc_conn != pullup_dp at some >>> situations. So, how about only calling start gadget at >>> usb_start_gadget, >> >> Which situations? > > When to pull-up DP is decided by application (while vbus is on), > not only by driver state machine. So when OTG state is B_PERIPHERAL or A_PERIPHERAL we still want DP pull-up to be disabled? > >> >>> and pullup_dp at drd_set_state (see below). >>> >>> >>> static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state >>> new_state) { >>> struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm); >>> >>> if (otg->state == new_state) >>> return; >>> >>> fsm->state_changed = 1; >>> dev_dbg(otg->dev, "otg: set state: %s\n", >>> usb_otg_state_string(new_state)); >>> switch (new_state) { >>> case OTG_STATE_B_IDLE: >>> + usb_udc_vbus_handler(gadget, false); >> >> This is redundant, When we switch from PROTO_GADGET to PROTO_UNDEF we do a >> usb_gadget_stop(), and a usb_gadget_disconnect() is done there. >> >>> drd_set_protocol(fsm, PROTO_UNDEF); >>> otg_drv_vbus(otg, 0); >>> break; >>> case OTG_STATE_B_PERIPHERAL: >>> drd_set_protocol(fsm, PROTO_GADGET); >>> + usb_udc_vbus_handler(gadget, true); >> >> This is redundant as well since usb_gadget_start() is doing a >> usb_gadget_connect(). >> >>> otg_drv_vbus(otg, 0); >>> break; >>> ...... >>> }; >>> >>> } >>> >>> When the OTG FSM is added to this framework, it can keep >>> usb_fsm->ops->loc_conn, and using the current FSM. >>> >> >> I have no strong opinions for or against usb_fsm->ops->loc_conn. >> Although strictly speaking, we shouldn't take any action based on loc_conn. > > Strictly speaking(OTG spec), all you does is for loc_conn, but you think > it's start_gadget. > >> It is just a state variable indicator. > > Nobody check this "indicator". > > Of cos, this is not a big deal, you can define the new API as is, > do udc_start() + gadget_connect() in one shot, it's up to user to > decide if use your usb_otg_start_gadget(), in case of udc_start() > followed by gadget_connect() is not wanted, user can/need do udc_start() > and something else before do gadget_connect. Please don't get me wrong. I want the full OTG state machine to be able to use usb_otg_start/stop_gadget(). Just trying to understand the real problem. cheers, -roger
diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c index 4151597..9b9702f 100644 --- a/drivers/usb/gadget/udc/udc-core.c +++ b/drivers/usb/gadget/udc/udc-core.c @@ -28,6 +28,10 @@ #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb.h> +#include <linux/usb/otg.h> + +#include <linux/of.h> +#include <linux/of_platform.h> /** * struct usb_udc - describes one usb device controller @@ -304,6 +308,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); */ static inline int usb_gadget_udc_start(struct usb_udc *udc) { + dev_dbg(&udc->dev, "%s\n", __func__); return udc->gadget->ops->udc_start(udc->gadget, udc->driver); } @@ -321,10 +326,81 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) */ static inline void usb_gadget_udc_stop(struct usb_udc *udc) { + dev_dbg(&udc->dev, "%s\n", __func__); udc->gadget->ops->udc_stop(udc->gadget); } /** + * usb_gadget_start - start the usb gadget controller and connect to bus + * @gadget: the gadget device to start + * + * This is external API for use by OTG core. + * + * Start the usb device controller and connect to bus (enable pull). + */ +static int usb_gadget_start(struct usb_gadget *gadget) +{ + int ret; + struct usb_udc *udc = NULL; + + dev_dbg(&gadget->dev, "%s\n", __func__); + mutex_lock(&udc_lock); + list_for_each_entry(udc, &udc_list, list) + if (udc->gadget == gadget) + goto found; + + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", + __func__); + mutex_unlock(&udc_lock); + return -EINVAL; + +found: + ret = usb_gadget_udc_start(udc); + if (ret) + dev_err(&udc->dev, "USB Device Controller didn't start: %d\n", + ret); + else + usb_udc_connect_control(udc); + + mutex_unlock(&udc_lock); + + return ret; +} + +/** + * usb_gadget_stop - disconnect from bus and stop the usb gadget + * @gadget: The gadget device we want to stop + * + * This is external API for use by OTG core. + * + * Disconnect from the bus (disable pull) and stop the + * gadget controller. + */ +static int usb_gadget_stop(struct usb_gadget *gadget) +{ + struct usb_udc *udc = NULL; + + dev_dbg(&gadget->dev, "%s\n", __func__); + mutex_lock(&udc_lock); + list_for_each_entry(udc, &udc_list, list) + if (udc->gadget == gadget) + goto found; + + dev_err(gadget->dev.parent, "%s: gadget not registered.\n", + __func__); + mutex_unlock(&udc_lock); + return -EINVAL; + +found: + usb_gadget_disconnect(udc->gadget); + udc->driver->disconnect(udc->gadget); + usb_gadget_udc_stop(udc); + mutex_unlock(&udc_lock); + + return 0; +} + +/** * usb_udc_release - release the usb_udc struct * @dev: the dev member within usb_udc * @@ -486,6 +562,48 @@ 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) { + struct device_node *np; + struct platform_device *pdev; + + np = of_parse_phandle(parent->of_node, "otg-controller", 0); + if (!np) { + dev_err(parent, + "otg_dev is NULL or no otg-controller property in DT\n"); + return -EINVAL; + } + + pdev = of_find_device_by_node(np); + of_node_put(np); + if (!pdev) { + dev_err(parent, "couldn't get otg-controller device\n"); + return -ENODEV; + } + + gadget->otg_dev = &pdev->dev; + } 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 +611,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 +656,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 +667,12 @@ EXPORT_SYMBOL_GPL(usb_del_gadget_udc); /* ------------------------------------------------------------------------- */ +struct otg_gadget_ops otg_gadget_intf = { + .start = usb_gadget_start, + .stop = usb_gadget_stop, +}; + +/* udc_lock must be held */ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver) { int ret; @@ -553,12 +687,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 +802,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 1878ae1..061f09e4 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -1160,6 +1160,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. Add usb_gadget_start() and usb_gadget_stop(). 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 mutexbefore 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 | 166 +++++++++++++++++++++++++++++++++++--- include/linux/usb/gadget.h | 4 + 2 files changed, 161 insertions(+), 9 deletions(-) -- 2.5.0