login
Search
Some of Freescale SoC chips have a QE or CPM co-processor which supports full speed USB. The driver adds device mode support of both QE and CPM USB controller to Linux USB gadget. The driver is tested with MPC8360 and MPC8272, and should work with other models having QE/CPM given minor tweaks. Signed-off-by: Xie Xiaobo <X.Xie@freescale.com> Signed-off-by: Li Yang <leoli@freescale.com> --- This is the second submission of the driver. This version addressed: Comments from Anton Vorontsov. A lot of cosmetic problem. Sparse and various kernel DEBUG warnings. drivers/usb/gadget/Kconfig | 20 + drivers/usb/gadget/Makefile | 1 + drivers/usb/gadget/fsl_qe_udc.c | 2731 +++++++++++++++++++++++++++++++++++++ drivers/usb/gadget/fsl_qe_udc.h | 422 ++++++ drivers/usb/gadget/gadget_chips.h | 9 + 5 files changed, 3183 insertions(+), 0 deletions(-) create mode 100644 drivers/usb/gadget/fsl_qe_udc.c create mode 100644 drivers/usb/gadget/fsl_qe_udc.h diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index acc95b2..4dbf622 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -150,6 +150,26 @@ config USB_FSL_USB2 default USB_GADGET select USB_GADGET_SELECTED +config USB_GADGET_FSL_QE + boolean "Freescale QE/CPM USB Device Controller" + depends on FSL_SOC && (QUICC_ENGINE || CPM) + help + Some of Freescale PowerPC processors have a Full Speed + QE/CPM2 USB controller, which support device mode with 4 + programmable endpoints. This driver supports the + controller in the MPC8360 and MPC8272, and should work with + controllers having QE or CPM2, given minor tweaks. + + Say "y" to link the driver statically, or "m" to build a + dynamically linked module called "fsl_qe_udc" and force all + gadget drivers to also be dynamically linked. + +config USB_FSL_QE + tristate + depends on USB_GADGET_FSL_QE + default USB_GADGET + select USB_GADGET_SELECTED + config USB_GADGET_NET2280 ...
This part doesn't look good, you should try to avoid hardcoding the specific type of chip (QE or CPM2) here. AFAICT, you can build a multiplatform kernel that supports both QE and CPM2, but your code Better try to avoid static forward declarations by reordering your functions in call order. That is the common coding style and makes Not a problem, but an observation: Most new code uses work queues instead of tasklets these days, which gives you more predictable real time latencies. If you don't have a specific reason to prefer a tasklet, just use Not addressing this driver in particular, but the USB gadget layer in general: This is a horrible interface, since every gadget driver exports the same symbols, you can never build a kernel that includes more than one gadget driver. Even if the drivers are all built as modules, simply You should not assume a specific layout of the reg property but rather use the simpler of_iomap function to get the registers, Since they are evidently different implementations, you should fill out the data field in the device id as the easiest way please use the standard dev_debug and pr_debug macros instead of defining your own. Arnd <>< --
This was done deliberately. The relevant standards state that a USB device can have no more than one peripheral interface. Now, I don't claim to fully support this decision. But there is a reason behind it; it's not just a case of bad design. Alan Stern --
Does building a kernel image that can run on different hardware without rebuilding also violate the "relevant standards"? And who's to say that there aren't multiple USB devices on a single board, that just happen to share a CPU and memory? :-) -Scott --
No. That isn't what Arnd was concerned about. He noted that even if you did build multiple modules, only one of them could be loaded at any That's why I don't fully support this decision. But I wanted to point out that there _was_ a conscious decision, as opposed to bad programming through sheer carelessness. Alan Stern --
Well, actually it was exactly what I was concerned about ;-) The way I understand the code, it is layered into the hardware specific part and the protocol specific part, which are connected through the interfaces I pointed out. The standard requires that there can only be one protocol handler per physical interface, which is a reasonable limitation. However, what the Linux implementation actually enforces is that there can only be one hardware specific driver built or loaded into the kernel, which just looks like an arbitrary restriction that does not actually help. If the gadget hardware drivers were registering the device with a gadget_bus_type, you could still enforce the "only one protocol" rule by binding every protocol to every device in that bus type. Arnd <>< --
No, you've got it exactly backward. There can be multiple protocol handlers per physical interface, but there must be only one physical Not at all -- it is an implementation of the constraint that there be There is no such rule. Alan Stern --
Maybe I am using wrong terminology, but I still don't see how that
fits together. Let me try to explain what I have understood so far:
The physical device is identified by a struct usb_gadget is defined
statically in the driver that exports the
usb_gadget_{un,}register_driver() functions. Obviously there can only
be one physical interface per physical device, I was not arguing
against that.
The protocol handler is identified by a usb_gadget_driver and defined
in a driver that calls usb_gadget_register_driver().
You say that there can be multiple protocol handlers, which I don't
understand because the protocol handlers all call set_gadget_data,
which overwrites the previous driver_data field in struct usb_gadget,
How that? Taking drivers/usb/gadget/fsl_usb2_udc.c as an example, it will
create a new fsl_udc structure for each matching platform_device it finds
(though it will leak every one except the last one), so the interface
does not limit the number of physical interfaces at all to this point,
the implementation of the every gadget hardware driver does this.
The real problem is that you cannot build a kernel that has both
fsl_usb2_udc.c and fsl_qe_udc.c built in. Having both drivers loaded
would not violate the one-interface rule, because only one of them
would find hardware to bind to on a given system, just like you can
load both the uhci and ohci drivers without them interfering.
Arnd <><
--
I thought you _were_ arguing against that. Unless I misunderstood,
your original complaint was that since each peripheral controller
driver defines usb_gadget_{un}register_driver, there can be only one
controller driver loaded at a time.
It's a new feature, only recently added to the kernel and not fully
implemented yet. The protocol handlers are encapsulated as "function"
drivers, where multiple functions can be combined at link time into a
fsl_usb2_udc isn't a good example; its behavior is not typical. Other
peripheral hardware drivers behave differently -- they define a single
static structure (or a dynamic singleton structure) and expect the
platform to contain no more than one matching device. Look at
I see your point. Even worse, you can't build both of them as modules.
Personally I wouldn't mind relaxing the "one interface per device"
rule. But I'm not the Gadget maintainer...
(There is another problem. If you have multiple peripheral interfaces,
how would you specify which one a particular gadget driver should bind
to?)
Alan Stern
--
That's probably where the misunderstanding came from: I did not expect more than one driver to actually be useful on a given system, but that should IMHO not prevent you from loading the drivers using modprobe, or building them all into the kernel. Arnd <>< --
All right, then we're in agreement. :-) Alan Stern --
And you'd have to rewrite all the gadget drivers ("protocol") to work with multiple upstream ports. That gets messy with e.g. the Ethernet links ... each would need to be configured with unique ethernet address pairs. Likewise with serial numbers. I've learned to just accept complaints in this area as sort of a price for existing. It's all complaints, no patches. So obviously the complaints don't have any requirements backing them. ;) --
This is common for udc drivers. I guess this measure is used to make the selection of udc drivers a choice list while still make it possible Is this truly a trend? Work queue is more flexible but it has higher latency. Why are work queues preferred? - Leo --
Most drivers don't need the low irq to bottom half latency. As I said, not a problem at all, just my observation. In 2.6.27, we have around three times more new workqueue usages than new tasklet usages. My feeling from doing reviews was that it would be closer to a factor of ten, but I guess I was wrong in that. Arnd <>< --
Bad terminology. Gadget drivers are what sit on TOP of peripheral controller drivers ... only peripheral controller drivers touch the actual hardware registers. They export an abstract "gadget" interface. That's never been a particular requirement. Systems won't get USB branding if they have more than one USB peripheral (upstream) port. Supporting more than one type of controller hardware is at best a pretty esoteric configuration. If you really want to see such stuff ... -ENOPATCH. :) - Dave --
How can you say "m" to something that is not a tristate? -Scott --
Well, I copied these from other udc drivers. Udc drivers can be compiled as module, through the CONFIG_USB_GADGET option. Though, the description here is not accurate. - Leo --
You might want to consider some of the following changes (they're tested with http://ozlabs.org/pipermail/linuxppc-dev/2008-September/062360.html). - remove qe_muram -> cpm_muram defines - configure clocks per device tree via qe_usb_clock_set(). To use this function we should select QE_USB. NOTE: currently this means that CPM build will break. :-( We should probably implement generic cpm_clock_source() and cpm_usb_clock_set(). Or.. we can move this to the board file (Do we ever need to change the clocks in this driver? For the Host driver we need this, since it can switch between low and full speed). What do you think? - change 8360 compatible to 8323 (8323 is the first chip with QE USB, device trees for 8360 boards should specify fsl,mpc8323-qe-usb as a compatible). diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig index 4dbf622..e57c298 100644 --- a/drivers/usb/gadget/Kconfig +++ b/drivers/usb/gadget/Kconfig @@ -153,6 +153,7 @@ config USB_FSL_USB2 config USB_GADGET_FSL_QE boolean "Freescale QE/CPM USB Device Controller" depends on FSL_SOC && (QUICC_ENGINE || CPM) + select QE_USB help Some of Freescale PowerPC processors have a Full Speed QE/CPM2 USB controller, which support device mode with 4 diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c index e448610..c1a0beb 100644 --- a/drivers/usb/gadget/fsl_qe_udc.c +++ b/drivers/usb/gadget/fsl_qe_udc.c @@ -36,20 +36,12 @@ #include <linux/usb/ch9.h> #include <linux/usb/gadget.h> #include <linux/usb/otg.h> +#include <asm/cpm.h> #include <asm/qe.h> #include <asm/dma.h> #include <asm/reg.h> #include "fsl_qe_udc.h" -#ifdef CONFIG_CPM2 -#include <asm/cpm.h> - -#define qe_muram_addr cpm_muram_addr -#define qe_muram_offset cpm_muram_offset -#define qe_muram_alloc cpm_muram_alloc -#define qe_muram_free cpm_muram_free -#endif - #define DRIVER_DESC "Freescale QE/CPM USB Device Controller driver" #define ...
On Mon, Sep 1, 2008 at 9:52 PM, Anton Vorontsov I'd prefer to leave the clock setting for USB gadget mode in platform code. There is no need to change the clock in the udc driver. There Well. IIRC, 8360 came out earlier than 8323. Whatsoever, 8360 is a better representative for the PowerQUICC 2 pro family with a QE as it --
Just found a recursive locking bug: Note: this function is called from the IRQ, the IRQ handler In the disconnect(), g_ether driver will immediately call qe_ep_disable() function which will try to grab &udc->lock spinlock once again.. Not sure how to fix this properly... :-/ p.s. the same bug exists in omap_udc.c, pxa27x_udc.c and probably other drivers as well... The only reason why it does not exploit in most cases is that the spin_lock_irqsave for !SMP case turns into simple local_irq_save(). -- Anton Vorontsov email: cbouatmailru@gmail.com irc://irc.freenode.net/bd2 --
Just caught this:
g_ether gadget: high speed config #1: CDC Ethernet (ECM)
BUG: sleeping function called from invalid context at mm/page_alloc.c:1450
in_atomic():1, irqs_disabled():1
Call Trace:
[c03a9c30] [c0008a90] show_stack+0x4c/0x16c (unreliable)
[c03a9c70] [c002cd28] __might_sleep+0xe4/0x114
[c03a9c80] [c006e550] __alloc_pages_internal+0x328/0x42c
[c03a9d00] [c006e67c] __get_free_pages+0x28/0x68
[c03a9d10] [c0090efc] __kmalloc+0xc0/0xe4
[c03a9d30] [c01d8efc] qe_ep_rxbd_update+0x98/0x1e0
[c03a9d50] [c01d90f0] qe_ep_init+0xac/0x37c
[c03a9d70] [c01d9458] qe_ep_enable+0x98/0xb8
[c03a9d80] [c01db268] gether_connect+0xa0/0x178
[c03a9da0] [c01dbba8] ecm_set_alt+0x108/0x12c
[c03a9db0] [c01dc988] composite_setup+0x2ec/0x3a8
[c03a9de0] [c01d8bec] setup_received_handle+0x1e4/0x26c
[c03a9e00] [c01d8ce0] ep0_setup_handle+0x6c/0x74
[c03a9e10] [c01d8e54] qe_ep0_rx+0x16c/0x17c
[c03a9e30] [c01d9db8] rx_irq+0x88/0xc0
[c03a9e40] [c01da32c] qe_udc_irq+0x10c/0x134
[c03a9e60] [c00631f4] handle_IRQ_event+0x5c/0xb0
[c03a9e80] [c006526c] handle_level_irq+0xa8/0x144
[c03a9ea0] [c002a5d0] qe_ic_cascade_low_ipic+0x58/0x70
[c03a9eb0] [c0006820] do_IRQ+0xa4/0xc8
[c03a9ec0] [c0013e14] ret_from_except+0x0/0x14
--- Exception: 501 at cpu_idle+0xa0/0x104
LR = cpu_idle+0xa0/0x104
[c03a9f80] [c0009d88] cpu_idle+0x50/0x104 (unreliable)
[c03a9fa0] [c029307c] _etext+0x7c/0x90
[c03a9fc0] [c035491c] start_kernel+0x1ac/0x230
[c03a9ff0] [00003438] 0x3438
The workaround is obvious (below), but should we really allocate
things in interrupts? Can we just allocate everything in probe()
and free in remove()?
diff --git a/drivers/usb/gadget/fsl_qe_udc.c b/drivers/usb/gadget/fsl_qe_udc.c
index c1a0beb..c54863b 100644
--- a/drivers/usb/gadget/fsl_qe_udc.c
+++ b/drivers/usb/gadget/fsl_qe_udc.c
@@ -433,7 +433,7 @@ static int qe_ep_rxbd_update(struct qe_ep *ep)
bd = ep->rxbase;
- ep->rxframe = kmalloc(sizeof(*ep->rxframe), GFP_KERNEL);
+ ep->rxframe = kmalloc(sizeof(*ep->rxframe), ...On Tue, Sep 2, 2008 at 1:11 AM, Anton Vorontsov It's possible but wasteful. We don't know which ep will be used and --
Does RNDIS work too? If not, is it possible to add or doesn't the HW support it? Jocke --
RNDIS is a gadget(protocol) level thing. I believe it can work with this driver although not tested myself. Noted: AFAIK, RNDIS gadget in Linux doesn't interoperate with windows well enough to be production level. Use at your own risk. - Leo --
I see. If one wants to connect with CDC to Windows, what drivers are there for Windows that works well with Linux? Jocke --
I believe MCCI has some. It also has drivers for a CDC subset, pretty much the same one Linux has used forever except wrapped with a few extra descriptors to make it practical to identify that "SAFE" (!) subset without needing vendor and product IDs. - Dave --
It should, so long as the QE hardware doesn't try to manage too many USB protocol details. Examples would be caring at all about "config change events" -- SET_CONFIGURATION picking between one of N configurations, and SET_INTERFACE Something in the past few releases broke so it may not be operating much at all, for that matter... However, it's a fair point that the interoperation requirements for RNDIS are so badly defined that non-Microsoft code has a very hard time being robust. Somewhere in the linux-usb archives is someone's fairly careful summary of a dozen extremely rude ways the MSFT RNDIS stack misbehaves. It'll do things like deciding to stop forwarding packets to certain applications for a while ... then continuing after a few minutes as if nothing was wrong. Blue screens are a possibility. Best to wait a few minutes between plugging and unplugging, while the races between the USB and NET stacks sort themselves out. - Dave --
