Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Sat, Sep 06, 2014 at 12:09:22AM +, Peter Chen wrote: On Fri, Sep 05, 2014 at 08:44:08AM +0800, Peter Chen wrote: On Thu, Sep 04, 2014 at 11:04:38AM -0400, Alan Stern wrote: On Thu, 4 Sep 2014, Peter Chen wrote: Hi Felipe Alan, how about using below steps for this reset/vbus/pullup changes? It mainly uses Alan's suggestion. Step 1: The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. the -reset is mandatory for all gadget drivers (currently, only four, they are composite, configfs, gadgetfs , dbgp. Step 2: Add routines to udc-core to handle Vbus changes, function activation changes, and resets. It will include three sub-steps, from easy to hard: reset handler, vbus handler, and activation handler. Perhaps the activation handler can be delayed until step 4. It won't get used until composite.c is modified to call it. Step 3: Modify all UDCs to call udc-core's reset and vbus handler, delete usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers, and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver Step 4: Modify the composite.c to disable pullup for adding every function, and enable pullup when necessary. Actually, composite.c should be modified to call the activation handler in udc-core, and then _that_ routine would adjust the pullup as necessary. This plan is okay with me. OK, let's put the function activation change to the last. If the Felipe has no other comments, I will send the patch for step one next Monday. Please do, the thread already has too much information and we better put all that in code. Keep in mind that me and Alan have resurected an old patchset adding -reset() callback to the gadget framework. If you want to take a look it's at [1] https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=gadget-add- reset-method Thanks, Felipe. It still takes gadget driver's -reset as optional, but we want to take it as mandatory per our discussion. right, I was going to change that but since you're already working on it I figured you might prefer doing it all yourself :-) cheers -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, Sep 05, 2014 at 08:44:08AM +0800, Peter Chen wrote: On Thu, Sep 04, 2014 at 11:04:38AM -0400, Alan Stern wrote: On Thu, 4 Sep 2014, Peter Chen wrote: Hi Felipe Alan, how about using below steps for this reset/vbus/pullup changes? It mainly uses Alan's suggestion. Step 1: The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. the -reset is mandatory for all gadget drivers (currently, only four, they are composite, configfs, gadgetfs , dbgp. Step 2: Add routines to udc-core to handle Vbus changes, function activation changes, and resets. It will include three sub-steps, from easy to hard: reset handler, vbus handler, and activation handler. Perhaps the activation handler can be delayed until step 4. It won't get used until composite.c is modified to call it. Step 3: Modify all UDCs to call udc-core's reset and vbus handler, delete usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers, and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver Step 4: Modify the composite.c to disable pullup for adding every function, and enable pullup when necessary. Actually, composite.c should be modified to call the activation handler in udc-core, and then _that_ routine would adjust the pullup as necessary. This plan is okay with me. OK, let's put the function activation change to the last. If the Felipe has no other comments, I will send the patch for step one next Monday. Please do, the thread already has too much information and we better put all that in code. Keep in mind that me and Alan have resurected an old patchset adding -reset() callback to the gadget framework. If you want to take a look it's at [1] https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=gadget-add-reset-method -- balbi signature.asc Description: Digital signature
RE: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, Sep 05, 2014 at 08:44:08AM +0800, Peter Chen wrote: On Thu, Sep 04, 2014 at 11:04:38AM -0400, Alan Stern wrote: On Thu, 4 Sep 2014, Peter Chen wrote: Hi Felipe Alan, how about using below steps for this reset/vbus/pullup changes? It mainly uses Alan's suggestion. Step 1: The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. the -reset is mandatory for all gadget drivers (currently, only four, they are composite, configfs, gadgetfs , dbgp. Step 2: Add routines to udc-core to handle Vbus changes, function activation changes, and resets. It will include three sub-steps, from easy to hard: reset handler, vbus handler, and activation handler. Perhaps the activation handler can be delayed until step 4. It won't get used until composite.c is modified to call it. Step 3: Modify all UDCs to call udc-core's reset and vbus handler, delete usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers, and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver Step 4: Modify the composite.c to disable pullup for adding every function, and enable pullup when necessary. Actually, composite.c should be modified to call the activation handler in udc-core, and then _that_ routine would adjust the pullup as necessary. This plan is okay with me. OK, let's put the function activation change to the last. If the Felipe has no other comments, I will send the patch for step one next Monday. Please do, the thread already has too much information and we better put all that in code. Keep in mind that me and Alan have resurected an old patchset adding -reset() callback to the gadget framework. If you want to take a look it's at [1] https://git.kernel.org/cgit/linux/kernel/git/balbi/usb.git/log/?h=gadget-add- reset-method Thanks, Felipe. It still takes gadget driver's -reset as optional, but we want to take it as mandatory per our discussion. Peter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Thu, 4 Sep 2014, Peter Chen wrote: Hi Felipe Alan, how about using below steps for this reset/vbus/pullup changes? It mainly uses Alan's suggestion. Step 1: The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. the -reset is mandatory for all gadget drivers (currently, only four, they are composite, configfs, gadgetfs , dbgp. Step 2: Add routines to udc-core to handle Vbus changes, function activation changes, and resets. It will include three sub-steps, from easy to hard: reset handler, vbus handler, and activation handler. Perhaps the activation handler can be delayed until step 4. It won't get used until composite.c is modified to call it. Step 3: Modify all UDCs to call udc-core's reset and vbus handler, delete usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers, and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver Step 4: Modify the composite.c to disable pullup for adding every function, and enable pullup when necessary. Actually, composite.c should be modified to call the activation handler in udc-core, and then _that_ routine would adjust the pullup as necessary. This plan is okay with me. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Thu, Sep 04, 2014 at 11:04:38AM -0400, Alan Stern wrote: On Thu, 4 Sep 2014, Peter Chen wrote: Hi Felipe Alan, how about using below steps for this reset/vbus/pullup changes? It mainly uses Alan's suggestion. Step 1: The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. the -reset is mandatory for all gadget drivers (currently, only four, they are composite, configfs, gadgetfs , dbgp. Step 2: Add routines to udc-core to handle Vbus changes, function activation changes, and resets. It will include three sub-steps, from easy to hard: reset handler, vbus handler, and activation handler. Perhaps the activation handler can be delayed until step 4. It won't get used until composite.c is modified to call it. Step 3: Modify all UDCs to call udc-core's reset and vbus handler, delete usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers, and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver Step 4: Modify the composite.c to disable pullup for adding every function, and enable pullup when necessary. Actually, composite.c should be modified to call the activation handler in udc-core, and then _that_ routine would adjust the pullup as necessary. This plan is okay with me. OK, let's put the function activation change to the last. If the Felipe has no other comments, I will send the patch for step one next Monday. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Wed, 3 Sep 2014, Peter Chen wrote: PS: I also have an old patch that adds a reset callback to g-mass-storage. Peter asked for this around the same time the other work was done. The idea was that disconnect must flush the buffers to the backing storage device, whereas a reset could avoid flushing anything -- Peter found that the flushing was taking so long, the gadget might not be able to carry out a reset quickly enough if it used the disconnect callback. The version I have of this patch is incomplete; it requires a reset callback to be added to the composite driver. Peter, do you still want to make this change to g-mass-storage? Alan, this problem seems not to be existed at g_mass_storage, g_mass_storage has not .disconnect API and only fsg_disable will be called when bus reset occurs. That sounds like a bug. Shouldn't there be a disconnect callback? Don't we want to flush the dirty buffers when a disconnect occurs? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Wed, 3 Sep 2014, Peter Chen wrote: Okay. Let's start by adding the reset field to struct usb_gadget_driver. The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. (At some later time we can add a -reset callback to struct usb_composite_driver; then the reset handler in composite.c can invoke that callback for all function drivers that define it.) Next, add routines to udc-core to handle Vbus changes, function activation changes, and resets. The Vbus routine will invoke the gadget driver's -disconnect callback when Vbus goes off and then call usb_gadget_disconnect. The activation routine will call usb_gadget_disconnect if any functions are deactivated, and usb_gadget_connect if all the functions are activated (basically, do whatever composite.c would do). And the reset routine will invoke the gadget driver's -reset callback. I suppose we'll also have to add fields to struct usb_gadget, to store the current Vbus and activation statuses. If the usb_gadget_disconnect will be not called at gadget driver's -disconnect, it has no much meaning we still have gadget driver's -reset, since the content of its -disconnect and -reset are the same if we don't consider to add function's reset handler. No, they aren't always the same. For example, g_mass_storage ought to flush its dirty buffers when a disconnect occurs, but it doesn't have to flush them when a reset occurs. Then modify all the UDC drivers; make them invoke the new udc-core routines whenever there's a change in Vbus status or a reset, instead of invoking the gadget driver's callbacks directly. At the same time we can remove the code that turns off the pullup when Vbus goes down, because now the udc-core routine will take care of it. Why we need to record the reset at udc-core? We don't really need to. But it seems like a good idea to have events that affect the entire device (including connect, disconnect, reset, and probably also bus suspend and bus resume) all pass through udc-core. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Wed, Sep 03, 2014 at 01:45:32PM -0400, Alan Stern wrote: On Wed, 3 Sep 2014, Peter Chen wrote: PS: I also have an old patch that adds a reset callback to g-mass-storage. Peter asked for this around the same time the other work was done. The idea was that disconnect must flush the buffers to the backing storage device, whereas a reset could avoid flushing anything -- Peter found that the flushing was taking so long, the gadget might not be able to carry out a reset quickly enough if it used the disconnect callback. The version I have of this patch is incomplete; it requires a reset callback to be added to the composite driver. Peter, do you still want to make this change to g-mass-storage? Alan, this problem seems not to be existed at g_mass_storage, g_mass_storage has not .disconnect API and only fsg_disable will be called when bus reset occurs. That sounds like a bug. Shouldn't there be a disconnect callback? Don't we want to flush the dirty buffers when a disconnect occurs? Maybe the dirty buffers are expected to discard if it is a sudden disconnect. I tried at add debug message before fsg_lun_fsync_sub at f_mass_storage, below are some findings: Windows7: - click cancel button during the transfer: [ 766.500854] do_prevent_allow:1383 [ 766.569848] do_prevent_allow:1383 [ 766.587965] do_synchronize_cache:957 - sudden disconnect fsg_lun_fsync_sub is not called linux pc (Linux version 3.16.0-031600-generic) - click cancel button during the transfer: fsg_lun_fsync_sub is not called - sudden disconnect fsg_lun_fsync_sub is not called I think Linux should be improved for the behaviour when click cancel during the transfer. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Wed, Sep 03, 2014 at 01:53:23PM -0400, Alan Stern wrote: On Wed, 3 Sep 2014, Peter Chen wrote: Okay. Let's start by adding the reset field to struct usb_gadget_driver. The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. (At some later time we can add a -reset callback to struct usb_composite_driver; then the reset handler in composite.c can invoke that callback for all function drivers that define it.) Next, add routines to udc-core to handle Vbus changes, function activation changes, and resets. The Vbus routine will invoke the gadget driver's -disconnect callback when Vbus goes off and then call usb_gadget_disconnect. The activation routine will call usb_gadget_disconnect if any functions are deactivated, and usb_gadget_connect if all the functions are activated (basically, do whatever composite.c would do). And the reset routine will invoke the gadget driver's -reset callback. I suppose we'll also have to add fields to struct usb_gadget, to store the current Vbus and activation statuses. If the usb_gadget_disconnect will be not called at gadget driver's -disconnect, it has no much meaning we still have gadget driver's -reset, since the content of its -disconnect and -reset are the same if we don't consider to add function's reset handler. No, they aren't always the same. For example, g_mass_storage ought to flush its dirty buffers when a disconnect occurs, but it doesn't have to flush them when a reset occurs. Then modify all the UDC drivers; make them invoke the new udc-core routines whenever there's a change in Vbus status or a reset, instead of invoking the gadget driver's callbacks directly. At the same time we can remove the code that turns off the pullup when Vbus goes down, because now the udc-core routine will take care of it. Why we need to record the reset at udc-core? We don't really need to. But it seems like a good idea to have events that affect the entire device (including connect, disconnect, reset, and probably also bus suspend and bus resume) all pass through udc-core. Hi Felipe Alan, how about using below steps for this reset/vbus/pullup changes? It mainly uses Alan's suggestion. Step 1: The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. the -reset is mandatory for all gadget drivers (currently, only four, they are composite, configfs, gadgetfs , dbgp. Step 2: Add routines to udc-core to handle Vbus changes, function activation changes, and resets. It will include three sub-steps, from easy to hard: reset handler, vbus handler, and activation handler. Step 3: Modify all UDCs to call udc-core's reset and vbus handler, delete usb_gadget_connect/usb_gadget_disconnect operation at all UDC drivers, and delete invoking usb_gadget_connect unconditional at udc_bind_to_driver Step 4: Modify the composite.c to disable pullup for adding every function, and enable pullup when necessary. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Thu, 4 Sep 2014, Peter Chen wrote: Alan, this problem seems not to be existed at g_mass_storage, g_mass_storage has not .disconnect API and only fsg_disable will be called when bus reset occurs. That sounds like a bug. Shouldn't there be a disconnect callback? Don't we want to flush the dirty buffers when a disconnect occurs? Maybe the dirty buffers are expected to discard if it is a sudden disconnect. I tried at add debug message before fsg_lun_fsync_sub at f_mass_storage, below are some findings: Windows7: - click cancel button during the transfer: [ 766.500854] do_prevent_allow:1383 [ 766.569848] do_prevent_allow:1383 [ 766.587965] do_synchronize_cache:957 - sudden disconnect fsg_lun_fsync_sub is not called linux pc (Linux version 3.16.0-031600-generic) - click cancel button during the transfer: fsg_lun_fsync_sub is not called - sudden disconnect fsg_lun_fsync_sub is not called I think Linux should be improved for the behaviour when click cancel during the transfer. Maybe, but that means changing the host code, not the gadget code. In general, I think Linux tends to flush caches less often than Windows (at least, for VFAT filesystems). fsg_lun_fsync_sub is probably quite fast if there are no dirty buffers, so adding extra calls won't hurt. The real question is what should the gadget do if the cable gets unplugged _during_ a transfer. I think in that case we really do want to flush the dirty buffers. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, 29 Aug 2014, Felipe Balbi wrote: I still have some old patch files lying around, adding reset callback support to dummy-hcd, net2280, and net2272. Would you like me to post them? Please do :-) let's get all of that sorted out soon. Patches coming up. These were written about two years ago, and although they have been forward-ported, I haven't tested them since they were written. They are based on a patch you posted on August 16, 2012 (usb: gadget: add reset method to struct usb_gadget_driver). Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Tue, Sep 02, 2014 at 11:32:52AM -0400, Alan Stern wrote: On Fri, 29 Aug 2014, Felipe Balbi wrote: I still have some old patch files lying around, adding reset callback support to dummy-hcd, net2280, and net2272. Would you like me to post them? Please do :-) let's get all of that sorted out soon. Patches coming up. These were written about two years ago, and although they have been forward-ported, I haven't tested them since they were written. They are based on a patch you posted on August 16, 2012 (usb: gadget: add reset method to struct usb_gadget_driver). alright, I still have my branch with that patch together with musb and dwc3 implementation. The problem I see, though, is that all three of your patches and my dwc3 and musb implementation conditionally calls -disconnect() if the gadget driver doesn't driver doesn't implement -reset(). If we're talking about usb_gadget_disconnect() from -disconnect(), than all 5 patches will cause regressions. cheers -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Tue, Sep 02, 2014 at 10:43:37AM -0500, Felipe Balbi wrote:
On Tue, Sep 02, 2014 at 11:32:52AM -0400, Alan Stern wrote:
On Fri, 29 Aug 2014, Felipe Balbi wrote:
I still have some old patch files lying around, adding reset callback
support to dummy-hcd, net2280, and net2272. Would you like me to post
them?
Please do :-) let's get all of that sorted out soon.
Patches coming up. These were written about two years ago, and
although they have been forward-ported, I haven't tested them since
they were written. They are based on a patch you posted on August 16,
2012 (usb: gadget: add reset method to struct usb_gadget_driver).
alright, I still have my branch with that patch together with musb and
dwc3 implementation. The problem I see, though, is that all three of
your patches and my dwc3 and musb implementation conditionally calls
-disconnect() if the gadget driver doesn't driver doesn't implement
-reset(). If we're talking about usb_gadget_disconnect() from
-disconnect(), than all 5 patches will cause regressions.
and in fact, the way we discussed way back, -reset() was supposed to be
optinal:
commit e95e07648e892bfe811ce2e68ba6e0dc64ae458d
Author: Felipe Balbi ba...@ti.com
Date: Wed Aug 15 20:41:40 2012 +0300
usb: gadget: add reset method to struct usb_gadget_driver
Some gadget drivers might benefit from having
separate -disconnect and -reset notifications.
For those gadget drivers which don't care, they
can implement only -disconnect and UDC driver
is required to call -disconnect case -reset
isn't valid.
Signed-off-by: Felipe Balbi ba...@ti.com
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index c3a6185..7b0661a 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -810,6 +810,10 @@ static inline int usb_gadget_disconnect(struct usb_gadget
*gadget)
* when the host is disconnected. May be called in_interrupt; this
* may not sleep. Some devices can't detect disconnect, so this might
* not be called except as part of controller shutdown.
+ * @reset: Invoked from reset interrupt handler. This call may not sleep.
+ * Some gadget drivers might benefit from having a different @reset
+ * and @disconnect methods. For those which don't provide @reset,
+ * UDC driver is expected to call @disconnect.
* @bind: the driver's bind callback
* @unbind: Invoked when the driver is unbound from a gadget,
* usually from rmmod (after a disconnect is reported).
@@ -871,6 +875,7 @@ struct usb_gadget_driver {
int (*setup)(struct usb_gadget *,
const struct usb_ctrlrequest *);
void(*disconnect)(struct usb_gadget *);
+ void(*reset)(struct usb_gadget *);
void(*suspend)(struct usb_gadget *);
void(*resume)(struct usb_gadget *);
are we changing that too ?
--
balbi
signature.asc
Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Tue, 2 Sep 2014, Felipe Balbi wrote: On Tue, Sep 02, 2014 at 11:32:52AM -0400, Alan Stern wrote: On Fri, 29 Aug 2014, Felipe Balbi wrote: I still have some old patch files lying around, adding reset callback support to dummy-hcd, net2280, and net2272. Would you like me to post them? Please do :-) let's get all of that sorted out soon. Patches coming up. These were written about two years ago, and although they have been forward-ported, I haven't tested them since they were written. They are based on a patch you posted on August 16, 2012 (usb: gadget: add reset method to struct usb_gadget_driver). alright, I still have my branch with that patch together with musb and dwc3 implementation. The problem I see, though, is that all three of your patches and my dwc3 and musb implementation conditionally calls -disconnect() if the gadget driver doesn't driver doesn't implement -reset(). If we're talking about usb_gadget_disconnect() from -disconnect(), than all 5 patches will cause regressions. That needn't be a problem. If Peter updates the four gadget drivers, adding reset callbacks, then we can remove the parts of our patches that invoke the disconnect callback if there is no reset callback. In other words, we can make reset callbacks mandatory for gadget drivers. Alan Stern PS: I also have an old patch that adds a reset callback to g-mass-storage. Peter asked for this around the same time the other work was done. The idea was that disconnect must flush the buffers to the backing storage device, whereas a reset could avoid flushing anything -- Peter found that the flushing was taking so long, the gadget might not be able to carry out a reset quickly enough if it used the disconnect callback. The version I have of this patch is incomplete; it requires a reset callback to be added to the composite driver. Peter, do you still want to make this change to g-mass-storage? -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
Hi, On Tue, Sep 02, 2014 at 12:02:03PM -0400, Alan Stern wrote: On Tue, 2 Sep 2014, Felipe Balbi wrote: On Tue, Sep 02, 2014 at 11:32:52AM -0400, Alan Stern wrote: On Fri, 29 Aug 2014, Felipe Balbi wrote: I still have some old patch files lying around, adding reset callback support to dummy-hcd, net2280, and net2272. Would you like me to post them? Please do :-) let's get all of that sorted out soon. Patches coming up. These were written about two years ago, and although they have been forward-ported, I haven't tested them since they were written. They are based on a patch you posted on August 16, 2012 (usb: gadget: add reset method to struct usb_gadget_driver). alright, I still have my branch with that patch together with musb and dwc3 implementation. The problem I see, though, is that all three of your patches and my dwc3 and musb implementation conditionally calls -disconnect() if the gadget driver doesn't driver doesn't implement -reset(). If we're talking about usb_gadget_disconnect() from -disconnect(), than all 5 patches will cause regressions. That needn't be a problem. If Peter updates the four gadget drivers, adding reset callbacks, then we can remove the parts of our patches that invoke the disconnect callback if there is no reset callback. In other words, we can make reset callbacks mandatory for gadget drivers. alright, but we need to do this in steps to avoid regressions or a non-bisectable tree. So maybe we add -reset as an optional method, implement support for it to all UDC drivers, patch all gadget drivers to implement reset, make reset mandatory. Does that work ? -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Tue, 2 Sep 2014, Felipe Balbi wrote: That needn't be a problem. If Peter updates the four gadget drivers, adding reset callbacks, then we can remove the parts of our patches that invoke the disconnect callback if there is no reset callback. In other words, we can make reset callbacks mandatory for gadget drivers. alright, but we need to do this in steps to avoid regressions or a non-bisectable tree. So maybe we add -reset as an optional method, implement support for it to all UDC drivers, patch all gadget drivers to implement reset, make reset mandatory. Does that work ? It would be okay, but doing things in a different order would be a little better: Add the reset callback to the usb_gadget_driver structure, implement it in all four gadget drivers, and then implement it in the UDC drivers. That way we don't have to make a second round of modifications to the UDC drivers, removing the fallback calls to -disconnect for when -reset is missing. The kerneldoc could state that some UDC drivers may call -disconnect when a reset occurs, instead of calling -reset. Then we won't have to fix up all the UDC drivers at once. If you want, you could add a remark to the kerneldoc saying that a disconnect handler generally should do everything that a reset handler does plus perhaps a few additional things. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
Hi,
On Tue, Sep 02, 2014 at 01:15:18PM -0400, Alan Stern wrote:
On Tue, 2 Sep 2014, Felipe Balbi wrote:
That needn't be a problem. If Peter updates the four gadget drivers,
adding reset callbacks, then we can remove the parts of our patches
that invoke the disconnect callback if there is no reset callback. In
other words, we can make reset callbacks mandatory for gadget drivers.
alright, but we need to do this in steps to avoid regressions or a
non-bisectable tree. So maybe we add -reset as an optional method,
implement support for it to all UDC drivers, patch all gadget drivers to
implement reset, make reset mandatory. Does that work ?
It would be okay, but doing things in a different order would be a
little better: Add the reset callback to the usb_gadget_driver
structure, implement it in all four gadget drivers, and then implement
it in the UDC drivers. That way we don't have to make a second round
it can't be only in these four gadget drivers. It needs to be
implemented in all of them even if:
diff --git a/drivers/usb/gadget/legacy/dbgp.c b/drivers/usb/gadget/legacy/dbgp.c
index 986fc51..2210289 100644
--- a/drivers/usb/gadget/legacy/dbgp.c
+++ b/drivers/usb/gadget/legacy/dbgp.c
@@ -409,6 +409,7 @@ static __refdata struct usb_gadget_driver dbgp_driver = {
.unbind = dbgp_unbind,
.setup = dbgp_setup,
.disconnect = dbgp_disconnect,
+ .reset = dbgp_disconnect,
.driver = {
.owner = THIS_MODULE,
.name = dbgp
otherwise -reset() will be optional and that means UDC will have to:
if (gadget_driver-reset)
gadget_driver-reset(g);
else if (gadget_driver-disconnect g.speed != UNKNOWN)
gadget_driver-disconnect(g);
and then we end up with a possibility of disconnecting from the host
when we don't want to. Bottom line, we _must_ tell the gadget driver
about a reset IRQ, so it can reset its internal state.
of modifications to the UDC drivers, removing the fallback calls to
-disconnect for when -reset is missing.
-reset() can't be missing if it were to be mandatory, right ?
The kerneldoc could state that some UDC drivers may call -disconnect
when a reset occurs, instead of calling -reset. Then we won't have to
fix up all the UDC drivers at once.
we won't be able to do that because the discussion on this thread is
that -disconnect() should call usb_gadget_disconnect().
If you want, you could add a remark to the kerneldoc saying that a
disconnect handler generally should do everything that a reset handler
does plus perhaps a few additional things.
yeah, that additional thing is usb_gadget_disconnect() and we don't want
to call that from a simple USB reset.
--
balbi
signature.asc
Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Tue, 2 Sep 2014, Felipe Balbi wrote:
alright, but we need to do this in steps to avoid regressions or a
non-bisectable tree. So maybe we add -reset as an optional method,
implement support for it to all UDC drivers, patch all gadget drivers to
implement reset, make reset mandatory. Does that work ?
It would be okay, but doing things in a different order would be a
little better: Add the reset callback to the usb_gadget_driver
structure, implement it in all four gadget drivers, and then implement
it in the UDC drivers. That way we don't have to make a second round
it can't be only in these four gadget drivers. It needs to be
implemented in all of them even if:
There _are_ only four gadget drivers: dbgp, gadgetfs, configfs, and
composite. (Unless some are hiding outside of drivers/usb/gadget -- I
didn't bother to check the entire kernel tree.)
diff --git a/drivers/usb/gadget/legacy/dbgp.c
b/drivers/usb/gadget/legacy/dbgp.c
index 986fc51..2210289 100644
--- a/drivers/usb/gadget/legacy/dbgp.c
+++ b/drivers/usb/gadget/legacy/dbgp.c
@@ -409,6 +409,7 @@ static __refdata struct usb_gadget_driver dbgp_driver = {
.unbind = dbgp_unbind,
.setup = dbgp_setup,
.disconnect = dbgp_disconnect,
+ .reset = dbgp_disconnect,
.driver = {
.owner = THIS_MODULE,
.name = dbgp
otherwise -reset() will be optional and that means UDC will have to:
if (gadget_driver-reset)
gadget_driver-reset(g);
else if (gadget_driver-disconnect g.speed != UNKNOWN)
gadget_driver-disconnect(g);
and then we end up with a possibility of disconnecting from the host
when we don't want to. Bottom line, we _must_ tell the gadget driver
about a reset IRQ, so it can reset its internal state.
So make reset mandatory from the start.
of modifications to the UDC drivers, removing the fallback calls to
-disconnect for when -reset is missing.
-reset() can't be missing if it were to be mandatory, right ?
Right.
The kerneldoc could state that some UDC drivers may call -disconnect
when a reset occurs, instead of calling -reset. Then we won't have to
fix up all the UDC drivers at once.
we won't be able to do that because the discussion on this thread is
that -disconnect() should call usb_gadget_disconnect().
I had forgotten that little detail. :-(
Well, strictly speaking, the disconnect handler doesn't have to call
usb_gadget_disconnect if the UDC driver takes care of it. And
currently all of the UDC drivers do.
Still, in the end it would be cleaner to allow disconnect handlers to
call usb_gadget_disconnect. And that means every UDC driver has to
support -reset callbacks. I don't know how practical that is -- in
some cases there may not be anybody capable of making the necessary
changes.
If you want, you could add a remark to the kerneldoc saying that a
disconnect handler generally should do everything that a reset handler
does plus perhaps a few additional things.
yeah, that additional thing is usb_gadget_disconnect() and we don't want
to call that from a simple USB reset.
That's not the only additional thing. The gadget driver also should
remember that the host isn't connected, so that it won't call
usb_gadget_connect when all the userspace components become ready.
You know, more and more it seems like this ought to be handled by the
UDC core. There are two conditions for turning on the pullup: Vbus
must be active, and the gadget's functions must all be activated. The
UDC driver knows about the first and the gadget driver knows about the
second -- so the UDC core is where those two pieces of knowledge should
be brought together.
That might be more of a change than you want to make, however...
Alan Stern
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Tue, Sep 02, 2014 at 02:11:52PM -0400, Alan Stern wrote:
On Tue, 2 Sep 2014, Felipe Balbi wrote:
alright, but we need to do this in steps to avoid regressions or a
non-bisectable tree. So maybe we add -reset as an optional method,
implement support for it to all UDC drivers, patch all gadget drivers to
implement reset, make reset mandatory. Does that work ?
It would be okay, but doing things in a different order would be a
little better: Add the reset callback to the usb_gadget_driver
structure, implement it in all four gadget drivers, and then implement
it in the UDC drivers. That way we don't have to make a second round
it can't be only in these four gadget drivers. It needs to be
implemented in all of them even if:
There _are_ only four gadget drivers: dbgp, gadgetfs, configfs, and
composite. (Unless some are hiding outside of drivers/usb/gadget -- I
heh, good point :-)
didn't bother to check the entire kernel tree.)
diff --git a/drivers/usb/gadget/legacy/dbgp.c
b/drivers/usb/gadget/legacy/dbgp.c
index 986fc51..2210289 100644
--- a/drivers/usb/gadget/legacy/dbgp.c
+++ b/drivers/usb/gadget/legacy/dbgp.c
@@ -409,6 +409,7 @@ static __refdata struct usb_gadget_driver dbgp_driver =
{
.unbind = dbgp_unbind,
.setup = dbgp_setup,
.disconnect = dbgp_disconnect,
+ .reset = dbgp_disconnect,
.driver = {
.owner = THIS_MODULE,
.name = dbgp
otherwise -reset() will be optional and that means UDC will have to:
if (gadget_driver-reset)
gadget_driver-reset(g);
else if (gadget_driver-disconnect g.speed != UNKNOWN)
gadget_driver-disconnect(g);
and then we end up with a possibility of disconnecting from the host
when we don't want to. Bottom line, we _must_ tell the gadget driver
about a reset IRQ, so it can reset its internal state.
So make reset mandatory from the start.
Sure thing, that can be done :-)
of modifications to the UDC drivers, removing the fallback calls to
-disconnect for when -reset is missing.
-reset() can't be missing if it were to be mandatory, right ?
Right.
The kerneldoc could state that some UDC drivers may call -disconnect
when a reset occurs, instead of calling -reset. Then we won't have to
fix up all the UDC drivers at once.
we won't be able to do that because the discussion on this thread is
that -disconnect() should call usb_gadget_disconnect().
I had forgotten that little detail. :-(
Well, strictly speaking, the disconnect handler doesn't have to call
usb_gadget_disconnect if the UDC driver takes care of it. And
currently all of the UDC drivers do.
Still, in the end it would be cleaner to allow disconnect handlers to
call usb_gadget_disconnect. And that means every UDC driver has to
support -reset callbacks. I don't know how practical that is -- in
some cases there may not be anybody capable of making the necessary
changes.
If you want, you could add a remark to the kerneldoc saying that a
disconnect handler generally should do everything that a reset handler
does plus perhaps a few additional things.
yeah, that additional thing is usb_gadget_disconnect() and we don't want
to call that from a simple USB reset.
That's not the only additional thing. The gadget driver also should
remember that the host isn't connected, so that it won't call
usb_gadget_connect when all the userspace components become ready.
You know, more and more it seems like this ought to be handled by the
UDC core. There are two conditions for turning on the pullup: Vbus
must be active, and the gadget's functions must all be activated. The
UDC driver knows about the first and the gadget driver knows about the
second -- so the UDC core is where those two pieces of knowledge should
be brought together.
I'll agree with this, 100%.
That might be more of a change than you want to make, however...
I wouldn't say that. As long as regressions can be avoided, I have no
issues modifying the framework and every single driver :-) I guess we
won't be able to do everything in one go, however, so we might need to
split this accross two merge windows to give people time to test all the
changes.
--
balbi
signature.asc
Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Tue, 2 Sep 2014, Felipe Balbi wrote: You know, more and more it seems like this ought to be handled by the UDC core. There are two conditions for turning on the pullup: Vbus must be active, and the gadget's functions must all be activated. The UDC driver knows about the first and the gadget driver knows about the second -- so the UDC core is where those two pieces of knowledge should be brought together. I'll agree with this, 100%. That might be more of a change than you want to make, however... I wouldn't say that. As long as regressions can be avoided, I have no issues modifying the framework and every single driver :-) I guess we won't be able to do everything in one go, however, so we might need to split this accross two merge windows to give people time to test all the changes. Do we need a -connect callback in the gadget drivers? If the UDC core does all the work of tracking the connection status, maybe we don't. For now, I'll assume we don't need it. Okay. Let's start by adding the reset field to struct usb_gadget_driver. The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. (At some later time we can add a -reset callback to struct usb_composite_driver; then the reset handler in composite.c can invoke that callback for all function drivers that define it.) Next, add routines to udc-core to handle Vbus changes, function activation changes, and resets. The Vbus routine will invoke the gadget driver's -disconnect callback when Vbus goes off and then call usb_gadget_disconnect. The activation routine will call usb_gadget_disconnect if any functions are deactivated, and usb_gadget_connect if all the functions are activated (basically, do whatever composite.c would do). And the reset routine will invoke the gadget driver's -reset callback. I suppose we'll also have to add fields to struct usb_gadget, to store the current Vbus and activation statuses. Then modify all the UDC drivers; make them invoke the new udc-core routines whenever there's a change in Vbus status or a reset, instead of invoking the gadget driver's callbacks directly. At the same time we can remove the code that turns off the pullup when Vbus goes down, because now the udc-core routine will take care of it. They don't all have to be updated at once. Unchanged UDC drivers will continue to work exactly as before, because they will bypass the new code in udc-core. How does that sound? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Tue, Sep 02, 2014 at 12:02:03PM -0400, Alan Stern wrote: On Tue, 2 Sep 2014, Felipe Balbi wrote: On Tue, Sep 02, 2014 at 11:32:52AM -0400, Alan Stern wrote: On Fri, 29 Aug 2014, Felipe Balbi wrote: I still have some old patch files lying around, adding reset callback support to dummy-hcd, net2280, and net2272. Would you like me to post them? Please do :-) let's get all of that sorted out soon. Patches coming up. These were written about two years ago, and although they have been forward-ported, I haven't tested them since they were written. They are based on a patch you posted on August 16, 2012 (usb: gadget: add reset method to struct usb_gadget_driver). alright, I still have my branch with that patch together with musb and dwc3 implementation. The problem I see, though, is that all three of your patches and my dwc3 and musb implementation conditionally calls -disconnect() if the gadget driver doesn't driver doesn't implement -reset(). If we're talking about usb_gadget_disconnect() from -disconnect(), than all 5 patches will cause regressions. That needn't be a problem. If Peter updates the four gadget drivers, adding reset callbacks, then we can remove the parts of our patches that invoke the disconnect callback if there is no reset callback. In other words, we can make reset callbacks mandatory for gadget drivers. Alan Stern PS: I also have an old patch that adds a reset callback to g-mass-storage. Peter asked for this around the same time the other work was done. The idea was that disconnect must flush the buffers to the backing storage device, whereas a reset could avoid flushing anything -- Peter found that the flushing was taking so long, the gadget might not be able to carry out a reset quickly enough if it used the disconnect callback. The version I have of this patch is incomplete; it requires a reset callback to be added to the composite driver. Peter, do you still want to make this change to g-mass-storage? Alan, this problem seems not to be existed at g_mass_storage, g_mass_storage has not .disconnect API and only fsg_disable will be called when bus reset occurs. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Tue, Sep 02, 2014 at 03:25:05PM -0400, Alan Stern wrote: On Tue, 2 Sep 2014, Felipe Balbi wrote: You know, more and more it seems like this ought to be handled by the UDC core. There are two conditions for turning on the pullup: Vbus must be active, and the gadget's functions must all be activated. The UDC driver knows about the first and the gadget driver knows about the second -- so the UDC core is where those two pieces of knowledge should be brought together. I'll agree with this, 100%. That might be more of a change than you want to make, however... I wouldn't say that. As long as regressions can be avoided, I have no issues modifying the framework and every single driver :-) I guess we won't be able to do everything in one go, however, so we might need to split this accross two merge windows to give people time to test all the changes. Do we need a -connect callback in the gadget drivers? If the UDC core does all the work of tracking the connection status, maybe we don't. For now, I'll assume we don't need it. Okay. Let's start by adding the reset field to struct usb_gadget_driver. The initial implementation in the four gadget drivers can be very simple: It calls the disconnect handler. (At some later time we can add a -reset callback to struct usb_composite_driver; then the reset handler in composite.c can invoke that callback for all function drivers that define it.) Next, add routines to udc-core to handle Vbus changes, function activation changes, and resets. The Vbus routine will invoke the gadget driver's -disconnect callback when Vbus goes off and then call usb_gadget_disconnect. The activation routine will call usb_gadget_disconnect if any functions are deactivated, and usb_gadget_connect if all the functions are activated (basically, do whatever composite.c would do). And the reset routine will invoke the gadget driver's -reset callback. I suppose we'll also have to add fields to struct usb_gadget, to store the current Vbus and activation statuses. If the usb_gadget_disconnect will be not called at gadget driver's -disconnect, it has no much meaning we still have gadget driver's -reset, since the content of its -disconnect and -reset are the same if we don't consider to add function's reset handler. Then modify all the UDC drivers; make them invoke the new udc-core routines whenever there's a change in Vbus status or a reset, instead of invoking the gadget driver's callbacks directly. At the same time we can remove the code that turns off the pullup when Vbus goes down, because now the udc-core routine will take care of it. Why we need to record the reset at udc-core? They don't all have to be updated at once. Unchanged UDC drivers will continue to work exactly as before, because they will bypass the new code in udc-core. How does that sound? Alan Stern -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, Aug 29, 2014 at 11:14:23AM -0400, Alan Stern wrote: On Fri, 29 Aug 2014, Peter Chen wrote: Felipe Alan, thanks for your comments for these patches, I think I may need to list these issues again to make things clearer. The purpose of these patchsets are to fix two issues: - Some udcs failures at USB-IF certification Back-Voltage test, it needs the dp is not pulled up when the vbus is not there, usb 2.0 spec 7.2.1 and 7.1.5 require it. - Some function drivers (f_uvc, f_obex, and android functions later) do not want to pull up dp at the udc_start, they want it on demand, like app has run. So, we can't pull up dp unconditionally at udc-core, I proposal the strategy for pulling up dp after below two conditions are satisfied [1]: - If the udc is ready to pull up dp - all functions at gadget driver want to pullup dp, it will pull up dp. The first condition depends on UDCs, Some UDCs(UDC-A) doesn't need to clear software pull up(run/stop) bit after vbus has disconnected, the possible reason may these kinds of UDC have hardware logic to disable pullup at dp when the vbus is not there, then it can pass back-voltage test, so for these kinds of UDCs, they are always ready to pull up dp. But for other UDCs(UDC-B, like chipidea), it needs to clear pullup bit after vbus has disconnected to pass back-voltage test, for these kinds of UDCs, they are only ready to pull up dp when the vbus is there. The second condition are the same for all UDCs, the function/gadget driver determine it. The patchset[1] has implemented above idea, UDC-A is always ready to pull up dp, it will set ready (connect) bit at the end of udc_start, and UDC-B will be ready when the vbus is there.[2], and the dp control strategy is decided by two conditions[3]. Alan concerns why the .connect API has **connect** argument, the reason is we need to call it at vbus handler to disable pullup bit when the vbus is not there. It is indeed strange we have **connect** argument at .connect, but current .disconnect API does not call usb_gadget_disconnect, so we have no way to pull down dp when the vbus is not there, that is the story we have this patchset and discussion. What's the next step for me? First, change the API so that the disconnect API _does_ call usb_gadget_disconnect. Then change the gadget drivers so that they tell the UDC driver when to turn the pullup on or off. This should happen whenever a function is activated or deactivated or whenever a connect or disconnect occurs. (In theory the gadget driver does not need to tell the UDC driver to turn off the pullup when a disconnect occurs; the UDC driver will turn it off anyway. But the gadget driver still needs to know that there was a disconnect. Otherwise it might tell the UDC driver to turn on the pullup at a time when the device isn't connected.) It is better gadget driver tells the UDC driver (with vbus handler) to turn off the pullup when a disconnect occurs, in that case, we can remove all pull up/down dp code from UDC drivers. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, Aug 29, 2014 at 04:20:20PM -0500, Felipe Balbi wrote: On Fri, Aug 29, 2014 at 05:08:13PM -0400, Alan Stern wrote: On Fri, 29 Aug 2014, Felipe Balbi wrote: First, change the API so that the disconnect API _does_ call usb_gadget_disconnect. that's not what the API was intended for, however. If we're going down that path, we need another of telling the gadget driver to reset all its buffers and teardown all its requests without disconnecting pullups, maybe that's what -reset() would be for. Just so. The disconnect handler should basically do the same thing as the reset handler, plus turn off the pullup. I still have some old patch files lying around, adding reset callback support to dummy-hcd, net2280, and net2272. Would you like me to post them? Please do :-) let's get all of that sorted out soon. Felipe, would you agree with we have a reset API at usb_gadget_driver, and call usb_gadget_disconnet at .disconnect if the flag pullup_on_vbus is set? -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, 29 Aug 2014, Peter Chen wrote: On Thu, 28 Aug 2014, Alan Stern wrote: Okay, so we need to add a vbus_is_on flag to the usb_gadget structure. The gadget driver will set this flag in its connect callback and clear the flag in its disconnect callback. If the UDC driver doesn't have a Vbus handler, it will invoke the connect callback just once during startup, and it will invoke the disconnect callback just once during removal. After more thought, if the vbus_is_on flag is going to be controlled by the gadget driver rather than the UDC driver, then it really belongs in the gadget driver's private data structure and not in struct usb_gadget. Also, a better name for the flag would be is_connected. Yes, we should have some places to change is_connected, that is one of thing we are discussing. The gadget driver should include an adjust_pullup() routine. It should get called whenever a function is activated or deactivated, and also by the connect and disconnect callbacks. The logic is simple: If (any functions are deactivated) turn off the pullup else if (gadget-vbus_is_on) turn on the pullup else turn off the pullup This logic can be compressed a little: if (!is_connected || any functions are deactivated) turn off the pullup else turn on the pullup Thanks, Alan. I have a similar implementation at below, http://www.spinics.net/lists/linux-usb/msg111976.html Yes, that seems to be moving in the right direction. You still need to change the other gadget drivers, and the API should be changed from composite_connect(gadget, flag) to composite_connect(gadget) and composite_disconnect(gadget). On Thu, 28 Aug 2014, Felipe Balbi wrote: you shouldn't call usb_gadget_disconnect() from -disconnect(). It's the other way around. You get a disconnect IRQ, then you call gadget driver's -disconnect(), but gadget driver doesn't need to ask UDC to remove pullup at that time. It's true that the gadget driver shouldn't have to ask for this, but doing so doesn't hurt. On the other hand, the gadget driver definitely _does_ have to tell the UDC driver to enable the pullup when a connect occurs. Otherwise the UDC driver won't know if the gadget's userspace components are ready. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, 29 Aug 2014, Peter Chen wrote: Felipe Alan, thanks for your comments for these patches, I think I may need to list these issues again to make things clearer. The purpose of these patchsets are to fix two issues: - Some udcs failures at USB-IF certification Back-Voltage test, it needs the dp is not pulled up when the vbus is not there, usb 2.0 spec 7.2.1 and 7.1.5 require it. - Some function drivers (f_uvc, f_obex, and android functions later) do not want to pull up dp at the udc_start, they want it on demand, like app has run. So, we can't pull up dp unconditionally at udc-core, I proposal the strategy for pulling up dp after below two conditions are satisfied [1]: - If the udc is ready to pull up dp - all functions at gadget driver want to pullup dp, it will pull up dp. The first condition depends on UDCs, Some UDCs(UDC-A) doesn't need to clear software pull up(run/stop) bit after vbus has disconnected, the possible reason may these kinds of UDC have hardware logic to disable pullup at dp when the vbus is not there, then it can pass back-voltage test, so for these kinds of UDCs, they are always ready to pull up dp. But for other UDCs(UDC-B, like chipidea), it needs to clear pullup bit after vbus has disconnected to pass back-voltage test, for these kinds of UDCs, they are only ready to pull up dp when the vbus is there. The second condition are the same for all UDCs, the function/gadget driver determine it. The patchset[1] has implemented above idea, UDC-A is always ready to pull up dp, it will set ready (connect) bit at the end of udc_start, and UDC-B will be ready when the vbus is there.[2], and the dp control strategy is decided by two conditions[3]. Alan concerns why the .connect API has **connect** argument, the reason is we need to call it at vbus handler to disable pullup bit when the vbus is not there. It is indeed strange we have **connect** argument at .connect, but current .disconnect API does not call usb_gadget_disconnect, so we have no way to pull down dp when the vbus is not there, that is the story we have this patchset and discussion. What's the next step for me? First, change the API so that the disconnect API _does_ call usb_gadget_disconnect. Then change the gadget drivers so that they tell the UDC driver when to turn the pullup on or off. This should happen whenever a function is activated or deactivated or whenever a connect or disconnect occurs. (In theory the gadget driver does not need to tell the UDC driver to turn off the pullup when a disconnect occurs; the UDC driver will turn it off anyway. But the gadget driver still needs to know that there was a disconnect. Otherwise it might tell the UDC driver to turn on the pullup at a time when the device isn't connected.) Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, Aug 29, 2014 at 11:14:23AM -0400, Alan Stern wrote: On Fri, 29 Aug 2014, Peter Chen wrote: Felipe Alan, thanks for your comments for these patches, I think I may need to list these issues again to make things clearer. The purpose of these patchsets are to fix two issues: - Some udcs failures at USB-IF certification Back-Voltage test, it needs the dp is not pulled up when the vbus is not there, usb 2.0 spec 7.2.1 and 7.1.5 require it. - Some function drivers (f_uvc, f_obex, and android functions later) do not want to pull up dp at the udc_start, they want it on demand, like app has run. So, we can't pull up dp unconditionally at udc-core, I proposal the strategy for pulling up dp after below two conditions are satisfied [1]: - If the udc is ready to pull up dp - all functions at gadget driver want to pullup dp, it will pull up dp. The first condition depends on UDCs, Some UDCs(UDC-A) doesn't need to clear software pull up(run/stop) bit after vbus has disconnected, the possible reason may these kinds of UDC have hardware logic to disable pullup at dp when the vbus is not there, then it can pass back-voltage test, so for these kinds of UDCs, they are always ready to pull up dp. But for other UDCs(UDC-B, like chipidea), it needs to clear pullup bit after vbus has disconnected to pass back-voltage test, for these kinds of UDCs, they are only ready to pull up dp when the vbus is there. The second condition are the same for all UDCs, the function/gadget driver determine it. The patchset[1] has implemented above idea, UDC-A is always ready to pull up dp, it will set ready (connect) bit at the end of udc_start, and UDC-B will be ready when the vbus is there.[2], and the dp control strategy is decided by two conditions[3]. Alan concerns why the .connect API has **connect** argument, the reason is we need to call it at vbus handler to disable pullup bit when the vbus is not there. It is indeed strange we have **connect** argument at .connect, but current .disconnect API does not call usb_gadget_disconnect, so we have no way to pull down dp when the vbus is not there, that is the story we have this patchset and discussion. What's the next step for me? First, change the API so that the disconnect API _does_ call usb_gadget_disconnect. that's not what the API was intended for, however. If we're going down that path, we need another of telling the gadget driver to reset all its buffers and teardown all its requests without disconnecting pullups, maybe that's what -reset() would be for. -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, 29 Aug 2014, Felipe Balbi wrote: First, change the API so that the disconnect API _does_ call usb_gadget_disconnect. that's not what the API was intended for, however. If we're going down that path, we need another of telling the gadget driver to reset all its buffers and teardown all its requests without disconnecting pullups, maybe that's what -reset() would be for. Just so. The disconnect handler should basically do the same thing as the reset handler, plus turn off the pullup. I still have some old patch files lying around, adding reset callback support to dummy-hcd, net2280, and net2272. Would you like me to post them? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Fri, Aug 29, 2014 at 05:08:13PM -0400, Alan Stern wrote: On Fri, 29 Aug 2014, Felipe Balbi wrote: First, change the API so that the disconnect API _does_ call usb_gadget_disconnect. that's not what the API was intended for, however. If we're going down that path, we need another of telling the gadget driver to reset all its buffers and teardown all its requests without disconnecting pullups, maybe that's what -reset() would be for. Just so. The disconnect handler should basically do the same thing as the reset handler, plus turn off the pullup. I still have some old patch files lying around, adding reset callback support to dummy-hcd, net2280, and net2272. Would you like me to post them? Please do :-) let's get all of that sorted out soon. Thanks Alan -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Wed, Aug 27, 2014 at 02:37:35PM -0500, Felipe Balbi wrote: Hi, On Tue, Aug 26, 2014 at 03:30:32PM +0800, Peter Chen wrote: On Mon, Aug 25, 2014 at 11:27:47AM -0400, Alan Stern wrote: On Mon, 25 Aug 2014, Peter Chen wrote: Hi Felipe Alan, It is the follow-up for: http://www.spinics.net/lists/linux-usb/msg112193.html This patchset adds reset API at usb_gadget_driver, the UDC driver can call it at bus_reset handler instead of calling disconnect. The benefits of this patchset are: - We can let the gadget driver do different things for bus reset. and host is disconnected, eg, whether pull down dp or not. - Match kernel doc for disconnect API, it says it is invoked when the host is disconnected. - Will be more match for the real things the gadget driver does for connect and disconnect, when we introduce connect API later. The reason for I mark RFC is I don't add the modification for mass UDC driver changes, if you consider this patchset is ok, I will add them without RFC later. This looks good. In patches 2, 4, and 5, shouldn't you call usb_gadget_disconnect() _before_ the gadget driver's original disconnect handler, instead of _after_? That's how we do it now. Hi Alan Felipe, During the changing UDC drivers process, I find we can't simply move usb_gadget_disconnect to usb_gadget_driver.disconnect, some UDC drivers (like dwc3) will be no chance to set software pullup bit (dp control always means software dp pullup bit if no specific at below) again between disconnection and re-connection. sorry, can you rephrase this a bit, I really can't get what you mean. DWC3 doesn't really have a pullup bit. It's got a RUN/STOP bit. Sure, it controls the pullups but it also kills the internal DMA and quite a bit of other internal blocks. It is the same with chipidea, it has RUN/STOP bit at usbcmd, and its function is most like dwc3's. If the RUN/STOP bit is cleared at usb_gadget_driver.disconnect when the cable is disconnected, when we set it again after cable is connected? The UDC drivers who has vbus handler can set run/stop bit at .vbus_session, but if the UDC drivers do not have vbus handler, where we can set it? The way the code is today, we will have a pullup connected when a gadget driver is loaded and not before that. There are two kinds of UDCs, dp is always pullup(UDC-A), pullup dp relies on vbus (UDC-B), so we may need to introduce a flag like pullup_on_vbus at struct usb_gadget, UDC-B needs to set this flag at .udc_start. For the whole gadget life cycle, the dp change for the two kinds of UDC like below: Process dp at UDC-A dp at UDC-B insmod 1 0 connect 1 1 bus_reset 1 1 disconnect 1 0 rmmod 0 0 For insmod/rmmod gadget driver, we can control dp at udc-core relies on flag pullup_on_vbus. For other three stages (no need to control at bus_reset), we can control dp at two gadget driver's API relies on flag pullup_on_vbus. I also can't get what you mean here. If the UDC driver doesn't want to pull up dp at insmod gadget driver, it can pull up dp when the cable is connected. Do you agree above? If you agree, the first patchset for adding reset API at usb_gadget_driver may do less thing, and the reset API implementation at each gadget driver is the same with disconnect. that's why we never had a -reset() callback so far. From a gadget driver perspective, it's the same as disconnect followed by a connect. Yes, it is the current design, but if we want to call usb_gadget_disconnect at -disconnect callback, things will be different, besides, the kernel doc says it is invoked when when the host is disconnected. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Wed, Aug 27, 2014 at 04:20:25PM -0400, Alan Stern wrote: On Wed, 27 Aug 2014, Felipe Balbi wrote: Hi Alan Felipe, During the changing UDC drivers process, I find we can't simply move usb_gadget_disconnect to usb_gadget_driver.disconnect, some UDC drivers (like dwc3) will be no chance to set software pullup bit (dp control always means software dp pullup bit if no specific at below) again between disconnection and re-connection. sorry, can you rephrase this a bit, I really can't get what you mean. DWC3 doesn't really have a pullup bit. It's got a RUN/STOP bit. Sure, it controls the pullups but it also kills the internal DMA and quite a bit of other internal blocks. The way the code is today, we will have a pullup connected when a gadget driver is loaded and not before that. There are two kinds of UDCs, dp is always pullup(UDC-A), pullup dp relies on vbus (UDC-B), so we may need to introduce a flag like pullup_on_vbus at struct usb_gadget, UDC-B needs to set this flag at .udc_start. For the whole gadget life cycle, the dp change for the two kinds of UDC like below: Process dp at UDC-A dp at UDC-B insmod1 0 connect 1 1 bus_reset 1 1 disconnect1 0 rmmod 0 0 For insmod/rmmod gadget driver, we can control dp at udc-core relies on flag pullup_on_vbus. For other three stages (no need to control at bus_reset), we can control dp at two gadget driver's API relies on flag pullup_on_vbus. I also can't get what you mean here. I think Peter is saying that some UDC hardware (which he calls UDC-A) automatically turns off the pullup when Vbus is absent, whereas other hardware (UDC-B) relies on software to do this. I'm not sure why this matters, however. Does anything go wrong if the driver tells UDC-A hardware to turn off the pullup when the cable is unplugged and Vbus goes away? Like I replied at last email, the UDC-A has no chance to pullup dp again after cable has connected if there is no vbus handler to do it (.vbus_session). If we don't no pullup dp, there will be no interrupt, the line state is SE0 after cable has connected. Do you agree above? If you agree, the first patchset for adding reset API at usb_gadget_driver may do less thing, and the reset API implementation at each gadget driver is the same with disconnect. that's why we never had a -reset() callback so far. From a gadget driver perspective, it's the same as disconnect followed by a connect. At the second patchset, we introduce the flag pullup_on_vbus, connect API at usb_gadget_driver, change the disconnect implementation at each gadget driver, and add control dp through function driver. IIRC, only mass storage gadget was showing a need for a -reset() callback, why would you need to modify every gadget driver ? The mass-storage gadget is a function driver, not a gadget driver. Even g_mass_storage.c is simply a single-function wrapper; it still relies on the composite layer. There are only four gadget drivers in the tree: composite, configfs, gadgetfs, and dbgp. Alan Stern -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Wed, Aug 27, 2014 at 03:29:20PM -0500, Felipe Balbi wrote: On Wed, Aug 27, 2014 at 04:20:25PM -0400, Alan Stern wrote: On Wed, 27 Aug 2014, Felipe Balbi wrote: Hi Alan Felipe, During the changing UDC drivers process, I find we can't simply move usb_gadget_disconnect to usb_gadget_driver.disconnect, some UDC drivers (like dwc3) will be no chance to set software pullup bit (dp control always means software dp pullup bit if no specific at below) again between disconnection and re-connection. that's why we never had a -reset() callback so far. From a gadget driver perspective, it's the same as disconnect followed by a connect. At the second patchset, we introduce the flag pullup_on_vbus, connect API at usb_gadget_driver, change the disconnect implementation at each gadget driver, and add control dp through function driver. IIRC, only mass storage gadget was showing a need for a -reset() callback, why would you need to modify every gadget driver ? The mass-storage gadget is a function driver, not a gadget driver. Even g_mass_storage.c is simply a single-function wrapper; it still relies on the composite layer. There are only four gadget drivers in the tree: composite, configfs, gadgetfs, and dbgp. right, but those are the only ones which should be modified. For all gadget drivers which are composed of function drivers (even single function drivers) they should rely on the function knowing what to do, otherwise we might still mistakenly connect to the host when some userspace daemon isn't ready yet. Yes, like Alan said, we only need to modify above four gadget drivers for changing dp pullup strategies, other legacy drivers will rely on their function drivers. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
Hi, On Thu, Aug 28, 2014 at 05:08:08PM +0800, Peter Chen wrote: On Wed, Aug 27, 2014 at 02:37:35PM -0500, Felipe Balbi wrote: Hi, On Tue, Aug 26, 2014 at 03:30:32PM +0800, Peter Chen wrote: On Mon, Aug 25, 2014 at 11:27:47AM -0400, Alan Stern wrote: On Mon, 25 Aug 2014, Peter Chen wrote: Hi Felipe Alan, It is the follow-up for: http://www.spinics.net/lists/linux-usb/msg112193.html This patchset adds reset API at usb_gadget_driver, the UDC driver can call it at bus_reset handler instead of calling disconnect. The benefits of this patchset are: - We can let the gadget driver do different things for bus reset. and host is disconnected, eg, whether pull down dp or not. - Match kernel doc for disconnect API, it says it is invoked when the host is disconnected. - Will be more match for the real things the gadget driver does for connect and disconnect, when we introduce connect API later. The reason for I mark RFC is I don't add the modification for mass UDC driver changes, if you consider this patchset is ok, I will add them without RFC later. This looks good. In patches 2, 4, and 5, shouldn't you call usb_gadget_disconnect() _before_ the gadget driver's original disconnect handler, instead of _after_? That's how we do it now. Hi Alan Felipe, During the changing UDC drivers process, I find we can't simply move usb_gadget_disconnect to usb_gadget_driver.disconnect, some UDC drivers (like dwc3) will be no chance to set software pullup bit (dp control always means software dp pullup bit if no specific at below) again between disconnection and re-connection. sorry, can you rephrase this a bit, I really can't get what you mean. DWC3 doesn't really have a pullup bit. It's got a RUN/STOP bit. Sure, it controls the pullups but it also kills the internal DMA and quite a bit of other internal blocks. It is the same with chipidea, it has RUN/STOP bit at usbcmd, and its function is most like dwc3's. If the RUN/STOP bit is cleared at usb_gadget_driver.disconnect when the cable is disconnected, when we set it again after cable is connected? The UDC drivers who has vbus handler can set run/stop bit at .vbus_session, but if the UDC drivers do not have vbus handler, where we can set it? some UDC drivers don't have a VBUS handler because they don't have internal VBUS comparators and there's no way to implement that for that UDC. The way the code is today, we will have a pullup connected when a gadget driver is loaded and not before that. There are two kinds of UDCs, dp is always pullup(UDC-A), pullup dp relies on vbus (UDC-B), so we may need to introduce a flag like pullup_on_vbus at struct usb_gadget, UDC-B needs to set this flag at .udc_start. For the whole gadget life cycle, the dp change for the two kinds of UDC like below: Process dp at UDC-A dp at UDC-B insmod1 0 connect 1 1 bus_reset 1 1 disconnect1 0 rmmod 0 0 For insmod/rmmod gadget driver, we can control dp at udc-core relies on flag pullup_on_vbus. For other three stages (no need to control at bus_reset), we can control dp at two gadget driver's API relies on flag pullup_on_vbus. I also can't get what you mean here. If the UDC driver doesn't want to pull up dp at insmod gadget driver, it can pull up dp when the cable is connected. how will UDC know that cable was connected if its pullups aren't connected ? Do you agree above? If you agree, the first patchset for adding reset API at usb_gadget_driver may do less thing, and the reset API implementation at each gadget driver is the same with disconnect. that's why we never had a -reset() callback so far. From a gadget driver perspective, it's the same as disconnect followed by a connect. Yes, it is the current design, but if we want to call usb_gadget_disconnect at -disconnect callback, things will be different, besides, the kernel doc says it is invoked when when the host is disconnected. you shouldn't call usb_gadget_disconnect() from -disconnect(). It's the other way around. You get a disconnect IRQ, then you call gadget driver's -disconnect(), but gadget driver doesn't need to ask UDC to remove pullup at that time. This has been getting more and more confusing of a problem. I'll go curl down in a corner and think about it :-) -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Thu, 28 Aug 2014, Peter Chen wrote: DWC3 doesn't really have a pullup bit. It's got a RUN/STOP bit. Sure, it controls the pullups but it also kills the internal DMA and quite a bit of other internal blocks. It is the same with chipidea, it has RUN/STOP bit at usbcmd, and its function is most like dwc3's. If the RUN/STOP bit is cleared at usb_gadget_driver.disconnect when the cable is disconnected, when we set it again after cable is connected? The UDC drivers who has vbus handler can set run/stop bit at .vbus_session, but if the UDC drivers do not have vbus handler, where we can set it? Okay, so we need to add a vbus_is_on flag to the usb_gadget structure. The gadget driver will set this flag in its connect callback and clear the flag in its disconnect callback. If the UDC driver doesn't have a Vbus handler, it will invoke the connect callback just once during startup, and it will invoke the disconnect callback just once during removal. The gadget driver should include an adjust_pullup() routine. It should get called whenever a function is activated or deactivated, and also by the connect and disconnect callbacks. The logic is simple: If (any functions are deactivated) turn off the pullup else if (gadget-vbus_is_on) turn on the pullup else turn off the pullup How does that sound? Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Thu, 28 Aug 2014, Alan Stern wrote: Okay, so we need to add a vbus_is_on flag to the usb_gadget structure. The gadget driver will set this flag in its connect callback and clear the flag in its disconnect callback. If the UDC driver doesn't have a Vbus handler, it will invoke the connect callback just once during startup, and it will invoke the disconnect callback just once during removal. After more thought, if the vbus_is_on flag is going to be controlled by the gadget driver rather than the UDC driver, then it really belongs in the gadget driver's private data structure and not in struct usb_gadget. Also, a better name for the flag would be is_connected. The gadget driver should include an adjust_pullup() routine. It should get called whenever a function is activated or deactivated, and also by the connect and disconnect callbacks. The logic is simple: If (any functions are deactivated) turn off the pullup else if (gadget-vbus_is_on) turn on the pullup else turn off the pullup This logic can be compressed a little: if (!is_connected || any functions are deactivated) turn off the pullup else turn on the pullup Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Thu, Aug 28, 2014 at 09:56:06AM -0500, Felipe Balbi wrote: Hi, For other three stages (no need to control at bus_reset), we can control dp at two gadget driver's API relies on flag pullup_on_vbus. I also can't get what you mean here. If the UDC driver doesn't want to pull up dp at insmod gadget driver, it can pull up dp when the cable is connected. how will UDC know that cable was connected if its pullups aren't connected ? Through vbus handler, these kinds of udc has flag pullup_on_vbus. Do you agree above? If you agree, the first patchset for adding reset API at usb_gadget_driver may do less thing, and the reset API implementation at each gadget driver is the same with disconnect. that's why we never had a -reset() callback so far. From a gadget driver perspective, it's the same as disconnect followed by a connect. Yes, it is the current design, but if we want to call usb_gadget_disconnect at -disconnect callback, things will be different, besides, the kernel doc says it is invoked when when the host is disconnected. you shouldn't call usb_gadget_disconnect() from -disconnect(). It's the other way around. You get a disconnect IRQ, then you call gadget driver's -disconnect(), but gadget driver doesn't need to ask UDC to remove pullup at that time. Not every UDC driver has disconnect IRQ. This has been getting more and more confusing of a problem. I'll go curl down in a corner and think about it :-) Felipe Alan, thanks for your comments for these patches, I think I may need to list these issues again to make things clearer. The purpose of these patchsets are to fix two issues: - Some udcs failures at USB-IF certification Back-Voltage test, it needs the dp is not pulled up when the vbus is not there, usb 2.0 spec 7.2.1 and 7.1.5 require it. - Some function drivers (f_uvc, f_obex, and android functions later) do not want to pull up dp at the udc_start, they want it on demand, like app has run. So, we can't pull up dp unconditionally at udc-core, I proposal the strategy for pulling up dp after below two conditions are satisfied [1]: - If the udc is ready to pull up dp - all functions at gadget driver want to pullup dp, it will pull up dp. The first condition depends on UDCs, Some UDCs(UDC-A) doesn't need to clear software pull up(run/stop) bit after vbus has disconnected, the possible reason may these kinds of UDC have hardware logic to disable pullup at dp when the vbus is not there, then it can pass back-voltage test, so for these kinds of UDCs, they are always ready to pull up dp. But for other UDCs(UDC-B, like chipidea), it needs to clear pullup bit after vbus has disconnected to pass back-voltage test, for these kinds of UDCs, they are only ready to pull up dp when the vbus is there. The second condition are the same for all UDCs, the function/gadget driver determine it. The patchset[1] has implemented above idea, UDC-A is always ready to pull up dp, it will set ready (connect) bit at the end of udc_start, and UDC-B will be ready when the vbus is there.[2], and the dp control strategy is decided by two conditions[3]. Alan concerns why the .connect API has **connect** argument, the reason is we need to call it at vbus handler to disable pullup bit when the vbus is not there. It is indeed strange we have **connect** argument at .connect, but current .disconnect API does not call usb_gadget_disconnect, so we have no way to pull down dp when the vbus is not there, that is the story we have this patchset and discussion. What's the next step for me? [1] http://www.spinics.net/lists/linux-usb/msg111969.html [2] http://www.spinics.net/lists/linux-usb/msg111973.html [3] http://www.spinics.net/lists/linux-usb/msg111976.html -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Thu, 28 Aug 2014, Alan Stern wrote: Okay, so we need to add a vbus_is_on flag to the usb_gadget structure. The gadget driver will set this flag in its connect callback and clear the flag in its disconnect callback. If the UDC driver doesn't have a Vbus handler, it will invoke the connect callback just once during startup, and it will invoke the disconnect callback just once during removal. After more thought, if the vbus_is_on flag is going to be controlled by the gadget driver rather than the UDC driver, then it really belongs in the gadget driver's private data structure and not in struct usb_gadget. Also, a better name for the flag would be is_connected. Yes, we should have some places to change is_connected, that is one of thing we are discussing. The gadget driver should include an adjust_pullup() routine. It should get called whenever a function is activated or deactivated, and also by the connect and disconnect callbacks. The logic is simple: If (any functions are deactivated) turn off the pullup else if (gadget-vbus_is_on) turn on the pullup else turn off the pullup This logic can be compressed a little: if (!is_connected || any functions are deactivated) turn off the pullup else turn on the pullup Thanks, Alan. I have a similar implementation at below, http://www.spinics.net/lists/linux-usb/msg111976.html Peter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
Hi, On Tue, Aug 26, 2014 at 03:30:32PM +0800, Peter Chen wrote: On Mon, Aug 25, 2014 at 11:27:47AM -0400, Alan Stern wrote: On Mon, 25 Aug 2014, Peter Chen wrote: Hi Felipe Alan, It is the follow-up for: http://www.spinics.net/lists/linux-usb/msg112193.html This patchset adds reset API at usb_gadget_driver, the UDC driver can call it at bus_reset handler instead of calling disconnect. The benefits of this patchset are: - We can let the gadget driver do different things for bus reset. and host is disconnected, eg, whether pull down dp or not. - Match kernel doc for disconnect API, it says it is invoked when the host is disconnected. - Will be more match for the real things the gadget driver does for connect and disconnect, when we introduce connect API later. The reason for I mark RFC is I don't add the modification for mass UDC driver changes, if you consider this patchset is ok, I will add them without RFC later. This looks good. In patches 2, 4, and 5, shouldn't you call usb_gadget_disconnect() _before_ the gadget driver's original disconnect handler, instead of _after_? That's how we do it now. Hi Alan Felipe, During the changing UDC drivers process, I find we can't simply move usb_gadget_disconnect to usb_gadget_driver.disconnect, some UDC drivers (like dwc3) will be no chance to set software pullup bit (dp control always means software dp pullup bit if no specific at below) again between disconnection and re-connection. sorry, can you rephrase this a bit, I really can't get what you mean. DWC3 doesn't really have a pullup bit. It's got a RUN/STOP bit. Sure, it controls the pullups but it also kills the internal DMA and quite a bit of other internal blocks. The way the code is today, we will have a pullup connected when a gadget driver is loaded and not before that. There are two kinds of UDCs, dp is always pullup(UDC-A), pullup dp relies on vbus (UDC-B), so we may need to introduce a flag like pullup_on_vbus at struct usb_gadget, UDC-B needs to set this flag at .udc_start. For the whole gadget life cycle, the dp change for the two kinds of UDC like below: Process dp at UDC-A dp at UDC-B insmod1 0 connect 1 1 bus_reset 1 1 disconnect1 0 rmmod 0 0 For insmod/rmmod gadget driver, we can control dp at udc-core relies on flag pullup_on_vbus. For other three stages (no need to control at bus_reset), we can control dp at two gadget driver's API relies on flag pullup_on_vbus. I also can't get what you mean here. Do you agree above? If you agree, the first patchset for adding reset API at usb_gadget_driver may do less thing, and the reset API implementation at each gadget driver is the same with disconnect. that's why we never had a -reset() callback so far. From a gadget driver perspective, it's the same as disconnect followed by a connect. At the second patchset, we introduce the flag pullup_on_vbus, connect API at usb_gadget_driver, change the disconnect implementation at each gadget driver, and add control dp through function driver. IIRC, only mass storage gadget was showing a need for a -reset() callback, why would you need to modify every gadget driver ? -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Wed, 27 Aug 2014, Felipe Balbi wrote: Hi Alan Felipe, During the changing UDC drivers process, I find we can't simply move usb_gadget_disconnect to usb_gadget_driver.disconnect, some UDC drivers (like dwc3) will be no chance to set software pullup bit (dp control always means software dp pullup bit if no specific at below) again between disconnection and re-connection. sorry, can you rephrase this a bit, I really can't get what you mean. DWC3 doesn't really have a pullup bit. It's got a RUN/STOP bit. Sure, it controls the pullups but it also kills the internal DMA and quite a bit of other internal blocks. The way the code is today, we will have a pullup connected when a gadget driver is loaded and not before that. There are two kinds of UDCs, dp is always pullup(UDC-A), pullup dp relies on vbus (UDC-B), so we may need to introduce a flag like pullup_on_vbus at struct usb_gadget, UDC-B needs to set this flag at .udc_start. For the whole gadget life cycle, the dp change for the two kinds of UDC like below: Process dp at UDC-A dp at UDC-B insmod 1 0 connect 1 1 bus_reset 1 1 disconnect 1 0 rmmod 0 0 For insmod/rmmod gadget driver, we can control dp at udc-core relies on flag pullup_on_vbus. For other three stages (no need to control at bus_reset), we can control dp at two gadget driver's API relies on flag pullup_on_vbus. I also can't get what you mean here. I think Peter is saying that some UDC hardware (which he calls UDC-A) automatically turns off the pullup when Vbus is absent, whereas other hardware (UDC-B) relies on software to do this. I'm not sure why this matters, however. Does anything go wrong if the driver tells UDC-A hardware to turn off the pullup when the cable is unplugged and Vbus goes away? Do you agree above? If you agree, the first patchset for adding reset API at usb_gadget_driver may do less thing, and the reset API implementation at each gadget driver is the same with disconnect. that's why we never had a -reset() callback so far. From a gadget driver perspective, it's the same as disconnect followed by a connect. At the second patchset, we introduce the flag pullup_on_vbus, connect API at usb_gadget_driver, change the disconnect implementation at each gadget driver, and add control dp through function driver. IIRC, only mass storage gadget was showing a need for a -reset() callback, why would you need to modify every gadget driver ? The mass-storage gadget is a function driver, not a gadget driver. Even g_mass_storage.c is simply a single-function wrapper; it still relies on the composite layer. There are only four gadget drivers in the tree: composite, configfs, gadgetfs, and dbgp. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Wed, Aug 27, 2014 at 04:20:25PM -0400, Alan Stern wrote: On Wed, 27 Aug 2014, Felipe Balbi wrote: Hi Alan Felipe, During the changing UDC drivers process, I find we can't simply move usb_gadget_disconnect to usb_gadget_driver.disconnect, some UDC drivers (like dwc3) will be no chance to set software pullup bit (dp control always means software dp pullup bit if no specific at below) again between disconnection and re-connection. sorry, can you rephrase this a bit, I really can't get what you mean. DWC3 doesn't really have a pullup bit. It's got a RUN/STOP bit. Sure, it controls the pullups but it also kills the internal DMA and quite a bit of other internal blocks. The way the code is today, we will have a pullup connected when a gadget driver is loaded and not before that. There are two kinds of UDCs, dp is always pullup(UDC-A), pullup dp relies on vbus (UDC-B), so we may need to introduce a flag like pullup_on_vbus at struct usb_gadget, UDC-B needs to set this flag at .udc_start. For the whole gadget life cycle, the dp change for the two kinds of UDC like below: Process dp at UDC-A dp at UDC-B insmod1 0 connect 1 1 bus_reset 1 1 disconnect1 0 rmmod 0 0 For insmod/rmmod gadget driver, we can control dp at udc-core relies on flag pullup_on_vbus. For other three stages (no need to control at bus_reset), we can control dp at two gadget driver's API relies on flag pullup_on_vbus. I also can't get what you mean here. I think Peter is saying that some UDC hardware (which he calls UDC-A) automatically turns off the pullup when Vbus is absent, whereas other hardware (UDC-B) relies on software to do this. I'm not sure why this matters, however. Does anything go wrong if the driver tells UDC-A hardware to turn off the pullup when the cable is unplugged and Vbus goes away? nope, nothing bad should happen. Do you agree above? If you agree, the first patchset for adding reset API at usb_gadget_driver may do less thing, and the reset API implementation at each gadget driver is the same with disconnect. that's why we never had a -reset() callback so far. From a gadget driver perspective, it's the same as disconnect followed by a connect. At the second patchset, we introduce the flag pullup_on_vbus, connect API at usb_gadget_driver, change the disconnect implementation at each gadget driver, and add control dp through function driver. IIRC, only mass storage gadget was showing a need for a -reset() callback, why would you need to modify every gadget driver ? The mass-storage gadget is a function driver, not a gadget driver. Even g_mass_storage.c is simply a single-function wrapper; it still relies on the composite layer. There are only four gadget drivers in the tree: composite, configfs, gadgetfs, and dbgp. right, but those are the only ones which should be modified. For all gadget drivers which are composed of function drivers (even single function drivers) they should rely on the function knowing what to do, otherwise we might still mistakenly connect to the host when some userspace daemon isn't ready yet. cheers -- balbi signature.asc Description: Digital signature
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Mon, Aug 25, 2014 at 11:27:47AM -0400, Alan Stern wrote: On Mon, 25 Aug 2014, Peter Chen wrote: Hi Felipe Alan, It is the follow-up for: http://www.spinics.net/lists/linux-usb/msg112193.html This patchset adds reset API at usb_gadget_driver, the UDC driver can call it at bus_reset handler instead of calling disconnect. The benefits of this patchset are: - We can let the gadget driver do different things for bus reset. and host is disconnected, eg, whether pull down dp or not. - Match kernel doc for disconnect API, it says it is invoked when the host is disconnected. - Will be more match for the real things the gadget driver does for connect and disconnect, when we introduce connect API later. The reason for I mark RFC is I don't add the modification for mass UDC driver changes, if you consider this patchset is ok, I will add them without RFC later. This looks good. In patches 2, 4, and 5, shouldn't you call usb_gadget_disconnect() _before_ the gadget driver's original disconnect handler, instead of _after_? That's how we do it now. Hi Alan Felipe, During the changing UDC drivers process, I find we can't simply move usb_gadget_disconnect to usb_gadget_driver.disconnect, some UDC drivers (like dwc3) will be no chance to set software pullup bit (dp control always means software dp pullup bit if no specific at below) again between disconnection and re-connection. There are two kinds of UDCs, dp is always pullup(UDC-A), pullup dp relies on vbus (UDC-B), so we may need to introduce a flag like pullup_on_vbus at struct usb_gadget, UDC-B needs to set this flag at .udc_start. For the whole gadget life cycle, the dp change for the two kinds of UDC like below: Process dp at UDC-A dp at UDC-B insmod 1 0 connect 1 1 bus_reset 1 1 disconnect 1 0 rmmod 0 0 For insmod/rmmod gadget driver, we can control dp at udc-core relies on flag pullup_on_vbus. For other three stages (no need to control at bus_reset), we can control dp at two gadget driver's API relies on flag pullup_on_vbus. Do you agree above? If you agree, the first patchset for adding reset API at usb_gadget_driver may do less thing, and the reset API implementation at each gadget driver is the same with disconnect. At the second patchset, we introduce the flag pullup_on_vbus, connect API at usb_gadget_driver, change the disconnect implementation at each gadget driver, and add control dp through function driver. -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
On Mon, 25 Aug 2014, Peter Chen wrote: Hi Felipe Alan, It is the follow-up for: http://www.spinics.net/lists/linux-usb/msg112193.html This patchset adds reset API at usb_gadget_driver, the UDC driver can call it at bus_reset handler instead of calling disconnect. The benefits of this patchset are: - We can let the gadget driver do different things for bus reset. and host is disconnected, eg, whether pull down dp or not. - Match kernel doc for disconnect API, it says it is invoked when the host is disconnected. - Will be more match for the real things the gadget driver does for connect and disconnect, when we introduce connect API later. The reason for I mark RFC is I don't add the modification for mass UDC driver changes, if you consider this patchset is ok, I will add them without RFC later. This looks good. In patches 2, 4, and 5, shouldn't you call usb_gadget_disconnect() _before_ the gadget driver's original disconnect handler, instead of _after_? That's how we do it now. Alan Stern -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
Hi Felipe Alan, It is the follow-up for: http://www.spinics.net/lists/linux-usb/msg112193.html This patchset adds reset API at usb_gadget_driver, the UDC driver can call it at bus_reset handler instead of calling disconnect. The benefits of this patchset are: - We can let the gadget driver do different things for bus reset. and host is disconnected, eg, whether pull down dp or not. - Match kernel doc for disconnect API, it says it is invoked when the host is disconnected. - Will be more match for the real things the gadget driver does for connect and disconnect, when we introduce connect API later. The reason for I mark RFC is I don't add the modification for mass UDC driver changes, if you consider this patchset is ok, I will add them without RFC later. This looks good. In patches 2, 4, and 5, shouldn't you call usb_gadget_disconnect() _before_ the gadget driver's original disconnect handler, instead of _after_? That's how we do it now. Thanks, Alan. It is my careless. Peter -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC PATCH 0/7] usb: gadget: add reset API at usb_gadget_driver
Hi Felipe Alan, It is the follow-up for: http://www.spinics.net/lists/linux-usb/msg112193.html This patchset adds reset API at usb_gadget_driver, the UDC driver can call it at bus_reset handler instead of calling disconnect. The benefits of this patchset are: - We can let the gadget driver do different things for bus reset. and host is disconnected, eg, whether pull down dp or not. - Match kernel doc for disconnect API, it says it is invoked when the host is disconnected. - Will be more match for the real things the gadget driver does for connect and disconnect, when we introduce connect API later. The reason for I mark RFC is I don't add the modification for mass UDC driver changes, if you consider this patchset is ok, I will add them without RFC later. Peter Chen (7): usb: gadget: add reset API at usb_gadget_driver usb: gadget: composite: add reset API at usb_gadget_driver usb: gadget: configfs: add reset API at usb_gadget_driver usb: gadget: gadgetfs: add reset API at usb_gadget_driver usb: gadget: dbgp: add reset API at usb_gadget_driver usb: gadget: udc-core: delete usb_gadget_disconnect at usb_gadget_remove_driver usb: gadget: udc-core: call gadget driver's disconnect at soft disconnect drivers/usb/gadget/composite.c| 13 - drivers/usb/gadget/configfs.c |1 + drivers/usb/gadget/legacy/dbgp.c | 14 +- drivers/usb/gadget/legacy/inode.c | 16 +++- drivers/usb/gadget/udc/udc-core.c |3 +-- include/linux/usb/composite.h |1 + include/linux/usb/gadget.h|2 ++ 7 files changed, 45 insertions(+), 5 deletions(-) -- 1.7.9.5 -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
