| Message ID | 1467276056-19357-1-git-send-email-rogerq@ti.com |
|---|---|
| State | New |
| Headers | show |
Hi Felipe, On 30/06/16 13:14, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@ti.com> writes: >> NOP_USB_XCEIV is used not only by gadget drivers but by >> host drivers as well e.g. EHCI_OMAP. >> >> commit 5a8d651a2bde ("usb: gadget: move gadget API functions to udc-core") >> made it so that NOP_USB_XCEIV can't be built-in if USB_GADGET is 'm'. >> But this prevents EHCI_OMAP to be built-in if USB_GADGET is 'm'. >> >> Fix this undesired behaviour by moving usb_gadget_vbus_connect/disconnect() >> to usb/gadget.h so that NOP_USB_XCEIV has no build dependency >> on USB_GADGET. >> >> Retain the original Kconfig behaviour i.e. NOP_USB_XCEIV is selected >> by drivers that need it. > > no, this is the wrong way to fix this. NOP _has_ a dependency on the > Gadget API if it calls Gadget API functions. Dependencies are proper. > > Maybe the reason for the problem is that we ended up adding far too much > code to phy-generic.c itself. Maybe it shouldn't know about clks and > interrupts. The original idea of that driver was to simply satisfy a > requirement to have a valid transceiver by some platforms. Maybe we > should fix that instead. Moving functions around to workaround a problem > is not the way to go, sorry. > OK but something that was working all these years is broken by your patch. Do you mind fixing it please? Or at least let me know how you want to get it fixed. cheers, -roger
On 01/07/16 10:57, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@ti.com> writes: >>>> NOP_USB_XCEIV is used not only by gadget drivers but by >>>> host drivers as well e.g. EHCI_OMAP. >>>> >>>> commit 5a8d651a2bde ("usb: gadget: move gadget API functions to udc-core") >>>> made it so that NOP_USB_XCEIV can't be built-in if USB_GADGET is 'm'. >>>> But this prevents EHCI_OMAP to be built-in if USB_GADGET is 'm'. >>>> >>>> Fix this undesired behaviour by moving usb_gadget_vbus_connect/disconnect() >>>> to usb/gadget.h so that NOP_USB_XCEIV has no build dependency >>>> on USB_GADGET. >>>> >>>> Retain the original Kconfig behaviour i.e. NOP_USB_XCEIV is selected >>>> by drivers that need it. >>> >>> no, this is the wrong way to fix this. NOP _has_ a dependency on the >>> Gadget API if it calls Gadget API functions. Dependencies are proper. >>> >>> Maybe the reason for the problem is that we ended up adding far too much >>> code to phy-generic.c itself. Maybe it shouldn't know about clks and >>> interrupts. The original idea of that driver was to simply satisfy a >>> requirement to have a valid transceiver by some platforms. Maybe we >>> should fix that instead. Moving functions around to workaround a problem >>> is not the way to go, sorry. >>> >> OK but something that was working all these years is broken by your >> patch. >> Do you mind fixing it please? Or at least let me know how you want to >> get it fixed. > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig > index 2e710a4cca52..89fd095ca33d 100644 > --- a/drivers/usb/host/Kconfig > +++ b/drivers/usb/host/Kconfig > @@ -180,7 +180,7 @@ config USB_EHCI_MXC > config USB_EHCI_HCD_OMAP > tristate "EHCI support for OMAP3 and later chips" > depends on ARCH_OMAP > - depends on NOP_USB_XCEIV > + depends on USB_PHY > default y > ---help--- > Enables support for the on-chip EHCI controller on > > This doesn't fix the real problem. i.e. we can't have NFS root on pandaboard and omap5_uevm because network device is on USB EHCI, unless we have both USB_GADGET and NOP_USB_XCEVI built-in, which is not really that desirable. cheers, -roger
On 01/07/16 11:15, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@ti.com> writes: >>>>>> NOP_USB_XCEIV is used not only by gadget drivers but by >>>>>> host drivers as well e.g. EHCI_OMAP. >>>>>> >>>>>> commit 5a8d651a2bde ("usb: gadget: move gadget API functions to udc-core") >>>>>> made it so that NOP_USB_XCEIV can't be built-in if USB_GADGET is 'm'. >>>>>> But this prevents EHCI_OMAP to be built-in if USB_GADGET is 'm'. >>>>>> >>>>>> Fix this undesired behaviour by moving usb_gadget_vbus_connect/disconnect() >>>>>> to usb/gadget.h so that NOP_USB_XCEIV has no build dependency >>>>>> on USB_GADGET. >>>>>> >>>>>> Retain the original Kconfig behaviour i.e. NOP_USB_XCEIV is selected >>>>>> by drivers that need it. >>>>> >>>>> no, this is the wrong way to fix this. NOP _has_ a dependency on the >>>>> Gadget API if it calls Gadget API functions. Dependencies are proper. >>>>> >>>>> Maybe the reason for the problem is that we ended up adding far too much >>>>> code to phy-generic.c itself. Maybe it shouldn't know about clks and >>>>> interrupts. The original idea of that driver was to simply satisfy a >>>>> requirement to have a valid transceiver by some platforms. Maybe we >>>>> should fix that instead. Moving functions around to workaround a problem >>>>> is not the way to go, sorry. >>>>> >>>> OK but something that was working all these years is broken by your >>>> patch. >>>> Do you mind fixing it please? Or at least let me know how you want to >>>> get it fixed. >>> >>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig >>> index 2e710a4cca52..89fd095ca33d 100644 >>> --- a/drivers/usb/host/Kconfig >>> +++ b/drivers/usb/host/Kconfig >>> @@ -180,7 +180,7 @@ config USB_EHCI_MXC >>> config USB_EHCI_HCD_OMAP >>> tristate "EHCI support for OMAP3 and later chips" >>> depends on ARCH_OMAP >>> - depends on NOP_USB_XCEIV >>> + depends on USB_PHY >>> default y >>> ---help--- >>> Enables support for the on-chip EHCI controller on >>> >>> >> This doesn't fix the real problem. i.e. we can't have NFS root on pandaboard >> and omap5_uevm because network device is on USB EHCI, unless we have >> both USB_GADGET and NOP_USB_XCEVI built-in, which is not really that >> desirable. > > Well, NOP calls a symbol that belongs to the Gadget API. So NOP cannot > be built-in if gadget API is a module. Satisfy the dependencies and > everything will work for you. > > Don't get me wrong, I see what you mean; but you _are_ relying in a > driver that calls into the Gadget API. Consider if it was the other way > around: say NOP was calling something from usbcore for whatever reason > and MUSB depended on NOP. You wouldn't have MUSB built-in unless NOP and > usbcore were also built-in, right? > I agree with you. As far as EHCI_OMAP is concerned we're only interested in configuring the PHY clocks and issuing a GPIO reset. As of now because of dependency issues with GADGET, NOP_USB_XCEIV doesn't seem to be the right place. I'm OK with using some other driver that doesn't have such dependency, even if it breaks DT compatibility with some OMAP boards. Or I'll just let EHCI_OMAP do it all by itself. Unless Tony has any objections. cheers, -roger
On 01/07/16 12:41, Felipe Balbi wrote: > > Hi, > > Roger Quadros <rogerq@ti.com> writes: >>>>>>>> NOP_USB_XCEIV is used not only by gadget drivers but by >>>>>>>> host drivers as well e.g. EHCI_OMAP. >>>>>>>> >>>>>>>> commit 5a8d651a2bde ("usb: gadget: move gadget API functions to udc-core") >>>>>>>> made it so that NOP_USB_XCEIV can't be built-in if USB_GADGET is 'm'. >>>>>>>> But this prevents EHCI_OMAP to be built-in if USB_GADGET is 'm'. >>>>>>>> >>>>>>>> Fix this undesired behaviour by moving usb_gadget_vbus_connect/disconnect() >>>>>>>> to usb/gadget.h so that NOP_USB_XCEIV has no build dependency >>>>>>>> on USB_GADGET. >>>>>>>> >>>>>>>> Retain the original Kconfig behaviour i.e. NOP_USB_XCEIV is selected >>>>>>>> by drivers that need it. >>>>>>> >>>>>>> no, this is the wrong way to fix this. NOP _has_ a dependency on the >>>>>>> Gadget API if it calls Gadget API functions. Dependencies are proper. >>>>>>> >>>>>>> Maybe the reason for the problem is that we ended up adding far too much >>>>>>> code to phy-generic.c itself. Maybe it shouldn't know about clks and >>>>>>> interrupts. The original idea of that driver was to simply satisfy a >>>>>>> requirement to have a valid transceiver by some platforms. Maybe we >>>>>>> should fix that instead. Moving functions around to workaround a problem >>>>>>> is not the way to go, sorry. >>>>>>> >>>>>> OK but something that was working all these years is broken by your >>>>>> patch. >>>>>> Do you mind fixing it please? Or at least let me know how you want to >>>>>> get it fixed. >>>>> >>>>> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig >>>>> index 2e710a4cca52..89fd095ca33d 100644 >>>>> --- a/drivers/usb/host/Kconfig >>>>> +++ b/drivers/usb/host/Kconfig >>>>> @@ -180,7 +180,7 @@ config USB_EHCI_MXC >>>>> config USB_EHCI_HCD_OMAP >>>>> tristate "EHCI support for OMAP3 and later chips" >>>>> depends on ARCH_OMAP >>>>> - depends on NOP_USB_XCEIV >>>>> + depends on USB_PHY >>>>> default y >>>>> ---help--- >>>>> Enables support for the on-chip EHCI controller on >>>>> >>>>> >>>> This doesn't fix the real problem. i.e. we can't have NFS root on pandaboard >>>> and omap5_uevm because network device is on USB EHCI, unless we have >>>> both USB_GADGET and NOP_USB_XCEVI built-in, which is not really that >>>> desirable. >>> >>> Well, NOP calls a symbol that belongs to the Gadget API. So NOP cannot >>> be built-in if gadget API is a module. Satisfy the dependencies and >>> everything will work for you. >>> >>> Don't get me wrong, I see what you mean; but you _are_ relying in a >>> driver that calls into the Gadget API. Consider if it was the other way >>> around: say NOP was calling something from usbcore for whatever reason >>> and MUSB depended on NOP. You wouldn't have MUSB built-in unless NOP and >>> usbcore were also built-in, right? >>> >> I agree with you. As far as EHCI_OMAP is concerned we're only interested >> in configuring the PHY clocks and issuing a GPIO reset. >> As of now because of dependency issues with GADGET, NOP_USB_XCEIV doesn't >> seem to be the right place. I'm OK with using some other driver >> that doesn't have such dependency, even if it breaks DT compatibility with >> some OMAP boards. Or I'll just let EHCI_OMAP do it all by itself. >> >> Unless Tony has any objections. > > I don't have memory of why EHCI-omap is using PHY, actually. Is it > really resetting a PHY? > on omap3-beagle, omap4-panda and omap5-uevm, we have an external USB PHY for the EHCI host. That PHY has it's reset line tied to a GPIO and CLK line tied to one of the SoC clock pins. cheers, -roger
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index ff8685e..687828d 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -538,37 +538,6 @@ out: EXPORT_SYMBOL_GPL(usb_gadget_clear_selfpowered); /** - * usb_gadget_vbus_connect - Notify controller that VBUS is powered - * @gadget:The device which now has VBUS power. - * Context: can sleep - * - * This call is used by a driver for an external transceiver (or GPIO) - * that detects a VBUS power session starting. Common responses include - * resuming the controller, activating the D+ (or D-) pullup to let the - * host detect that a USB device is attached, and starting to draw power - * (8mA or possibly more, especially after SET_CONFIGURATION). - * - * Returns zero on success, else negative errno. - */ -int usb_gadget_vbus_connect(struct usb_gadget *gadget) -{ - int ret = 0; - - if (!gadget->ops->vbus_session) { - ret = -EOPNOTSUPP; - goto out; - } - - ret = gadget->ops->vbus_session(gadget, 1); - -out: - trace_usb_gadget_vbus_connect(gadget, ret); - - return ret; -} -EXPORT_SYMBOL_GPL(usb_gadget_vbus_connect); - -/** * usb_gadget_vbus_draw - constrain controller's VBUS power usage * @gadget:The device whose VBUS usage is being described * @mA:How much current to draw, in milliAmperes. This should be twice @@ -601,35 +570,6 @@ out: EXPORT_SYMBOL_GPL(usb_gadget_vbus_draw); /** - * usb_gadget_vbus_disconnect - notify controller about VBUS session end - * @gadget:the device whose VBUS supply is being described - * Context: can sleep - * - * This call is used by a driver for an external transceiver (or GPIO) - * that detects a VBUS power session ending. Common responses include - * reversing everything done in usb_gadget_vbus_connect(). - * - * Returns zero on success, else negative errno. - */ -int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) -{ - int ret = 0; - - if (!gadget->ops->vbus_session) { - ret = -EOPNOTSUPP; - goto out; - } - - ret = gadget->ops->vbus_session(gadget, 0); - -out: - trace_usb_gadget_vbus_disconnect(gadget, ret); - - return ret; -} -EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect); - -/** * usb_gadget_connect - software-controlled connect to USB host * @gadget:the peripheral being connected * diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 2e710a4..d8f5674 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -180,7 +180,7 @@ config USB_EHCI_MXC config USB_EHCI_HCD_OMAP tristate "EHCI support for OMAP3 and later chips" depends on ARCH_OMAP - depends on NOP_USB_XCEIV + select NOP_USB_XCEIV default y ---help--- Enables support for the on-chip EHCI controller on diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig index b9c409a..fe94bbc 100644 --- a/drivers/usb/phy/Kconfig +++ b/drivers/usb/phy/Kconfig @@ -45,7 +45,7 @@ config ISP1301_OMAP config KEYSTONE_USB_PHY tristate "Keystone USB PHY Driver" depends on ARCH_KEYSTONE || COMPILE_TEST - depends on NOP_USB_XCEIV + select NOP_USB_XCEIV help Enable this to support Keystone USB phy. This driver provides interface to interact with USB 2.0 and USB 3.0 PHY that is part @@ -53,7 +53,6 @@ config KEYSTONE_USB_PHY config NOP_USB_XCEIV tristate "NOP USB Transceiver Driver" - depends on USB_GADGET || !USB_GADGET # if USB_GADGET=m, NOP can't be built-in select USB_PHY help This driver is to be used by all the usb transceiver which are either @@ -66,7 +65,7 @@ config AM335X_CONTROL_USB config AM335X_PHY_USB tristate "AM335x USB PHY Driver" depends on ARM || COMPILE_TEST - depends on NOP_USB_XCEIV + select NOP_USB_XCEIV select USB_PHY select AM335X_CONTROL_USB select USB_COMMON diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 612dbdf..305d81e 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -508,14 +508,50 @@ static inline int gadget_is_otg(struct usb_gadget *g) /*-------------------------------------------------------------------------*/ +/** + * usb_gadget_vbus_connect - Notify controller that VBUS is powered + * @gadget:The device which now has VBUS power. + * Context: can sleep + * + * This call is used by a driver for an external transceiver (or GPIO) + * that detects a VBUS power session starting. Common responses include + * resuming the controller, activating the D+ (or D-) pullup to let the + * host detect that a USB device is attached, and starting to draw power + * (8mA or possibly more, especially after SET_CONFIGURATION). + * + * Returns zero on success, else negative errno. + */ +static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget) +{ + if (!gadget->ops->vbus_session) + return -EOPNOTSUPP; + return gadget->ops->vbus_session(gadget, 1); +} + +/** + * usb_gadget_vbus_disconnect - notify controller about VBUS session end + * @gadget:the device whose VBUS supply is being described + * Context: can sleep + * + * This call is used by a driver for an external transceiver (or GPIO) + * that detects a VBUS power session ending. Common responses include + * reversing everything done in usb_gadget_vbus_connect(). + * + * Returns zero on success, else negative errno. + */ +static inline int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) +{ + if (!gadget->ops->vbus_session) + return -EOPNOTSUPP; + return gadget->ops->vbus_session(gadget, 0); +} + #if IS_ENABLED(CONFIG_USB_GADGET) int usb_gadget_frame_number(struct usb_gadget *gadget); int usb_gadget_wakeup(struct usb_gadget *gadget); int usb_gadget_set_selfpowered(struct usb_gadget *gadget); int usb_gadget_clear_selfpowered(struct usb_gadget *gadget); -int usb_gadget_vbus_connect(struct usb_gadget *gadget); int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA); -int usb_gadget_vbus_disconnect(struct usb_gadget *gadget); int usb_gadget_connect(struct usb_gadget *gadget); int usb_gadget_disconnect(struct usb_gadget *gadget); int usb_gadget_deactivate(struct usb_gadget *gadget); @@ -529,12 +565,8 @@ static inline int usb_gadget_set_selfpowered(struct usb_gadget *gadget) { return 0; } static inline int usb_gadget_clear_selfpowered(struct usb_gadget *gadget) { return 0; } -static inline int usb_gadget_vbus_connect(struct usb_gadget *gadget) -{ return 0; } static inline int usb_gadget_vbus_draw(struct usb_gadget *gadget, unsigned mA) { return 0; } -static inline int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) -{ return 0; } static inline int usb_gadget_connect(struct usb_gadget *gadget) { return 0; } static inline int usb_gadget_disconnect(struct usb_gadget *gadget)
NOP_USB_XCEIV is used not only by gadget drivers but by host drivers as well e.g. EHCI_OMAP. commit 5a8d651a2bde ("usb: gadget: move gadget API functions to udc-core") made it so that NOP_USB_XCEIV can't be built-in if USB_GADGET is 'm'. But this prevents EHCI_OMAP to be built-in if USB_GADGET is 'm'. Fix this undesired behaviour by moving usb_gadget_vbus_connect/disconnect() to usb/gadget.h so that NOP_USB_XCEIV has no build dependency on USB_GADGET. Retain the original Kconfig behaviour i.e. NOP_USB_XCEIV is selected by drivers that need it. Fixes: 5a8d651a2bde ("usb: gadget: move gadget API functions to udc-core") Signed-off-by: Roger Quadros <rogerq@ti.com> --- drivers/usb/gadget/udc/core.c | 60 ------------------------------------------- drivers/usb/host/Kconfig | 2 +- drivers/usb/phy/Kconfig | 5 ++-- include/linux/usb/gadget.h | 44 ++++++++++++++++++++++++++----- 4 files changed, 41 insertions(+), 70 deletions(-) -- 2.7.4