Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thursday 28 August 2008, Arnd Bergmann wrote: +/*- + Gadget driver register and unregister. + --*/ +int usb_gadget_register_driver(struct usb_gadget_driver *driver) +EXPORT_SYMBOL(usb_gadget_register_driver); + +int usb_gadget_unregister_driver(struct usb_gadget_driver *driver) +EXPORT_SYMBOL(usb_gadget_unregister_driver); 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, 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. Gadget drivers are what talk *to* that abstract interface. you can never build a kernel that includes more than one gadget driver. Even if the drivers are all built as modules, simply loading one of them prevents loading another one. 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thursday 28 August 2008, Arnd Bergmann wrote: 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. 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. ;) ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Mon, Sep 1, 2008 at 9:52 PM, Anton Vorontsov
[EMAIL PROTECTED] wrote:
On Thu, Aug 28, 2008 at 05:43:33PM +0800, Li Yang wrote:
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 [EMAIL PROTECTED]
Signed-off-by: Li Yang [EMAIL PROTECTED]
---
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.
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
Already done to address comment from Arnd.
- 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?
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
is no point to make the speed of a USB device changable.
- 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).
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
has the full functionality.
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 DRIVER_AUTHOR Xie XiaoBo
#define DRIVER_VERSION 1.0
@@ -2490,6 +2482,7 @@ static int __devinit qe_udc_probe(struct of_device
*ofdev,
const struct of_device_id *match)
{
struct device_node *np = ofdev-node;
+ enum qe_clock clk;
struct qe_ep *ep;
unsigned int ret = 0;
unsigned int i;
@@ -2499,6 +2492,16 @@ static int __devinit qe_udc_probe(struct of_device
*ofdev,
if (!prop || strcmp(prop, peripheral))
return -ENODEV;
+ prop = of_get_property(np, fsl,fullspeed-clock, NULL);
+ if (!prop)
+ return -EINVAL;
+
+ clk = qe_clock_source(prop);
+ if (clk == QE_CLK_NONE || clk == QE_CLK_DUMMY)
+ return -EINVAL;
+
+ qe_usb_clock_set(clk, USB_CLOCK);
+
/* Initialize the udc structure including QH member and other member */
udc_controller = qe_udc_config(ofdev);
if (!udc_controller) {
@@ -2689,7 +2692,7 @@ static int __devexit qe_udc_remove(struct of_device
*ofdev)
/*-*/
static struct of_device_id qe_udc_match[] = {
{
- .compatible = fsl,mpc8360-qe-usb,
+ .compatible = fsl,mpc8323-qe-usb,
},
{
.compatible = fsl,mpc8272-cpm-usb,
diff --git a/drivers/usb/gadget/fsl_qe_udc.h b/drivers/usb/gadget/fsl_qe_udc.h
index e2e9639..0ff9c00 100644
--- a/drivers/usb/gadget/fsl_qe_udc.h
+++ b/drivers/usb/gadget/fsl_qe_udc.h
@@ -17,6 +17,8 @@
#ifndef __FSL_QE_UDC_H
#define __FSL_QE_UDC_H
+#define USB_CLOCK 4800
+
#define USB_MAX_ENDPOINTS 4
#define USB_MAX_PIPES USB_MAX_ENDPOINTS
#define USB_EP0_MAX_SIZE 64
--
To unsubscribe from this
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Tue, Sep 2, 2008 at 1:11 AM, Anton Vorontsov
[EMAIL PROTECTED] wrote:
On Thu, Aug 28, 2008 at 05:43:33PM +0800, Li Yang wrote:
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 [EMAIL PROTECTED]
Signed-off-by: Li Yang [EMAIL PROTECTED]
---
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.
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] [3438] 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()?
It's possible but wasteful. We don't know which ep will be used and
what's the buffer size needed at probe time.
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), GFP_ATOMIC);
if (ep-rxframe == NULL) {
dev_err(ep-udc-dev, malloc rxframe failed\n);
return -ENOMEM;
@@ -447,7 +447,7 @@ static int qe_ep_rxbd_update(struct qe_ep *ep)
bdring_len = USB_BDRING_LEN;
size = (ep-ep.maxpacket + USB_CRC_SIZE + 2) * (bdring_len + 1);
- ep-rxbuffer = kzalloc(size, GFP_KERNEL);
+ ep-rxbuffer = kzalloc(size, GFP_ATOMIC);
if (ep-rxbuffer == NULL) {
dev_err(ep-udc-dev, malloc rxbuffer failed,size=%d\n,
size);
@@ -680,7 +680,7 @@ static int qe_ep_init(struct qe_udc *udc,
}
if ((ep-tm == USBP_TM_CTL) || (ep-dir == USB_DIR_IN)) {
- ep-txframe = kmalloc(sizeof(*ep-txframe), GFP_KERNEL);
+ ep-txframe = kmalloc(sizeof(*ep-txframe), GFP_ATOMIC);
if (ep-txframe == NULL) {
dev_err(udc-dev, malloc txframe failed\n);
goto en_done;
@@ -1930,7 +1930,9 @@ static int reset_queues(struct qe_udc *udc)
udc_reset_ep_queue(udc, pipe);
/* report disconnect; the driver is already quiesced */
+ spin_unlock(udc-lock);
udc-driver-disconnect(udc-gadget);
+ spin_lock(udc-lock);
Good catch.
return 0;
}
--
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Tue, 2008-09-02 at 15:35 +0800, Li Yang wrote: On Tue, Sep 2, 2008 at 1:11 AM, Anton Vorontsov [EMAIL PROTECTED] wrote: On Thu, Aug 28, 2008 at 05:43:33PM +0800, Li Yang wrote: 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 [EMAIL PROTECTED] Signed-off-by: Li Yang [EMAIL PROTECTED] --- 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. Just caught this: g_ether gadget: high speed config #1: CDC Ethernet (ECM) Does RNDIS work too? If not, is it possible to add or doesn't the HW support it? Jocke ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thu, Aug 28, 2008 at 05:43:33PM +0800, Li Yang wrote:
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 [EMAIL PROTECTED]
Signed-off-by: Li Yang [EMAIL PROTECTED]
---
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.
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 DRIVER_AUTHOR Xie XiaoBo
#define DRIVER_VERSION 1.0
@@ -2490,6 +2482,7 @@ static int __devinit qe_udc_probe(struct of_device *ofdev,
const struct of_device_id *match)
{
struct device_node *np = ofdev-node;
+ enum qe_clock clk;
struct qe_ep *ep;
unsigned int ret = 0;
unsigned int i;
@@ -2499,6 +2492,16 @@ static int __devinit qe_udc_probe(struct of_device
*ofdev,
if (!prop || strcmp(prop, peripheral))
return -ENODEV;
+ prop = of_get_property(np, fsl,fullspeed-clock, NULL);
+ if (!prop)
+ return -EINVAL;
+
+ clk = qe_clock_source(prop);
+ if (clk == QE_CLK_NONE || clk == QE_CLK_DUMMY)
+ return -EINVAL;
+
+ qe_usb_clock_set(clk, USB_CLOCK);
+
/* Initialize the udc structure including QH member and other member */
udc_controller = qe_udc_config(ofdev);
if (!udc_controller) {
@@ -2689,7 +2692,7 @@ static int __devexit qe_udc_remove(struct of_device
*ofdev)
/*-*/
static struct of_device_id qe_udc_match[] = {
{
- .compatible = fsl,mpc8360-qe-usb,
+ .compatible = fsl,mpc8323-qe-usb,
},
{
.compatible = fsl,mpc8272-cpm-usb,
diff --git a/drivers/usb/gadget/fsl_qe_udc.h b/drivers/usb/gadget/fsl_qe_udc.h
index e2e9639..0ff9c00 100644
--- a/drivers/usb/gadget/fsl_qe_udc.h
+++ b/drivers/usb/gadget/fsl_qe_udc.h
@@ -17,6 +17,8 @@
#ifndef __FSL_QE_UDC_H
#define __FSL_QE_UDC_H
+#define USB_CLOCK 4800
+
#define USB_MAX_ENDPOINTS 4
#define USB_MAX_PIPES USB_MAX_ENDPOINTS
#define USB_EP0_MAX_SIZE 64
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thu, Aug 28, 2008 at 05:43:33PM +0800, Li Yang wrote:
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 [EMAIL PROTECTED]
Signed-off-by: Li Yang [EMAIL PROTECTED]
---
Just found a recursive locking bug:
[...]
+static int reset_queues(struct qe_udc *udc)
+{
Note: this function is called from the IRQ, the IRQ handler
grabs udc-lock spinlock..
+ u8 pipe;
+
+ for (pipe = 0; pipe USB_MAX_ENDPOINTS; pipe++)
+ udc_reset_ep_queue(udc, pipe);
+
+ /* report disconnect; the driver is already quiesced */
+ udc-driver-disconnect(udc-gadget);
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: [EMAIL PROTECTED]
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thu, Aug 28, 2008 at 05:43:33PM +0800, Li Yang wrote:
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 [EMAIL PROTECTED]
Signed-off-by: Li Yang [EMAIL PROTECTED]
---
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.
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] [3438] 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), GFP_ATOMIC);
if (ep-rxframe == NULL) {
dev_err(ep-udc-dev, malloc rxframe failed\n);
return -ENOMEM;
@@ -447,7 +447,7 @@ static int qe_ep_rxbd_update(struct qe_ep *ep)
bdring_len = USB_BDRING_LEN;
size = (ep-ep.maxpacket + USB_CRC_SIZE + 2) * (bdring_len + 1);
- ep-rxbuffer = kzalloc(size, GFP_KERNEL);
+ ep-rxbuffer = kzalloc(size, GFP_ATOMIC);
if (ep-rxbuffer == NULL) {
dev_err(ep-udc-dev, malloc rxbuffer failed,size=%d\n,
size);
@@ -680,7 +680,7 @@ static int qe_ep_init(struct qe_udc *udc,
}
if ((ep-tm == USBP_TM_CTL) || (ep-dir == USB_DIR_IN)) {
- ep-txframe = kmalloc(sizeof(*ep-txframe), GFP_KERNEL);
+ ep-txframe = kmalloc(sizeof(*ep-txframe), GFP_ATOMIC);
if (ep-txframe == NULL) {
dev_err(udc-dev, malloc txframe failed\n);
goto en_done;
@@ -1930,7 +1930,9 @@ static int reset_queues(struct qe_udc *udc)
udc_reset_ep_queue(udc, pipe);
/* report disconnect; the driver is already quiesced */
+ spin_unlock(udc-lock);
udc-driver-disconnect(udc-gadget);
+ spin_lock(udc-lock);
return 0;
}
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thu, 2008-08-28 at 17:04 +0200, Arnd Bergmann wrote:
On Thursday 28 August 2008, Li Yang wrote:
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.
Looks pretty good, just a few comments on the driver:
+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
Why do you need the two config options, not just one?
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
to compiled as module.
+#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
...
+static int qe_ep_cmd_restarttx(struct qe_ep *ep)
+{
+ u8 ep_num;
+#ifdef CONFIG_CPM2
+ u32 command;
+ u8 opcode;
+
+ ep_num = ep-epnum CPM_USB_EP_SHIFT;
+ command = CPM_USB_RESTART_TX | (u32)ep_num;
+ opcode = CPM_USB_RESTART_TX_OPCODE;
+ cpm_command(command, opcode);
+#else
+ ep_num = ep-epnum;
+ qe_issue_cmd(QE_USB_RESTART_TX, QE_CR_SUBBLOCK_USB, ep_num, 0);
+#endif
+ return 0;
+}
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
here would be broken in that case if you try to run it on QE.
Ok.
+static void setup_received_handle(struct qe_udc *udc,
+ struct usb_ctrlrequest *setup);
+static int qe_ep_rxframe_handle(struct qe_ep *ep);
+static void ep0_req_complete(struct qe_udc *udc, struct qe_req *req);
Better try to avoid static forward declarations by reordering your
functions in call order. That is the common coding style and makes
drivers easier to read when you're used to it.
+
+ tasklet_schedule(udc-rx_tasklet);
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
a workqueue here.
Is this truly a trend? Work queue is more flexible but it has higher
latency. Why are work queues preferred?
- Leo
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Friday 29 August 2008, Li Yang wrote: 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 a workqueue here. Is this truly a trend? Work queue is more flexible but it has higher latency. Why are work queues preferred? 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Fri, 29 Aug 2008, Arnd Bergmann wrote: Does building a kernel image that can run on different hardware without rebuilding also violate the relevant standards? 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 time. 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. That's right. The standard requires that there can only be one protocol handler per physical interface, which is a reasonable limitation. No, you've got it exactly backward. There can be multiple protocol handlers per physical interface, but there must be only one physical interface per device. 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. Not at all -- it is an implementation of the constraint that there be only one physical interface. 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. There is no such rule. Alan Stern ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Friday 29 August 2008, Alan Stern wrote:
The standard requires that there can only be one protocol handler
per physical interface, which is a reasonable limitation.
No, you've got it exactly backward. There can be multiple protocol
handlers per physical interface, but there must be only one physical
interface per device.
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,
making it impossible to load more than one simultaneously.
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.
Not at all -- it is an implementation of the constraint that there be
only one physical interface.
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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Friday 29 August 2008, Alan Stern wrote:
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.
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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Fri, 29 Aug 2008, Arnd Bergmann wrote:
On Friday 29 August 2008, Alan Stern wrote:
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.
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.
All right, then we're in agreement. :-)
Alan Stern
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thu, Aug 28, 2008 at 05:43:33PM +0800, Li Yang wrote: +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. How can you say m to something that is not a tristate? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thu, 28 Aug 2008, Arnd Bergmann wrote: 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 loading one of them prevents loading another one. 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thu, 28 Aug 2008, Scott Wood wrote: Alan Stern wrote: This was done deliberately. The relevant standards state that a USB device can have no more than one peripheral interface. Does building a kernel image that can run on different hardware without rebuilding also violate the relevant standards? 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 time. 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? :-) 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Thursday 28 August 2008, Alan Stern wrote: On Thu, 28 Aug 2008, Scott Wood wrote: Alan Stern wrote: This was done deliberately. The relevant standards state that a USB device can have no more than one peripheral interface. Does building a kernel image that can run on different hardware without rebuilding also violate the relevant standards? 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 time. 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
Alan Stern wrote: This was done deliberately. The relevant standards state that a USB device can have no more than one peripheral interface. 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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Wed, Aug 06, 2008 at 03:16:40PM +0800, Li Yang wrote:
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 [EMAIL PROTECTED]
Signed-off-by: Li Yang [EMAIL PROTECTED]
Hi,
Just want to report that the driver works here on MPC8360E-MDS,
with few issues though.
+/*-
+ Gadget driver register and unregister.
+ --*/
+int usb_gadget_register_driver(struct usb_gadget_driver *driver)
+{
+ int retval;
+ unsigned long flags = 0;
+
+ /* standard operations */
+ if (!udc_controller)
+ return -ENODEV;
+
+ if (!driver || (driver-speed != USB_SPEED_FULL
+ driver-speed != USB_SPEED_HIGH)
+ || !driver-bind || !driver-unbind ||
+ !driver-disconnect || !driver-setup)
Usually unbind is assigned via __exit_p() macro, thus the driver
will not able to work with g_ether when it is built in.
Plus I got this with various debugging enabled:
g_ether gadget: high speed config #1: CDC Ethernet (ECM)
BUG: spinlock recursion on CPU#0, swapper/0
lock: cf846c50, .magic: dead4ead, .owner: swapper/0, .owner_cpu: 0
Call Trace:
[c0361cb0] [c0008a90] show_stack+0x4c/0x16c (unreliable)
[c0361cf0] [c0175b84] spin_bug+0x7c/0xc4
[c0361d10] [c0175ea8] _raw_spin_lock+0xb4/0xb8
[c0361d20] [c027e5dc] _spin_lock_irqsave+0x30/0x48
[c0361d40] [c01c8450] qe_ep_init+0x80/0x3a4
[c0361d60] [c01c8828] qe_ep_enable+0xb4/0xe4
[c0361d80] [c01cb210] ecm_set_alt+0x88/0x12c
[c0361d90] [c01cbb10] set_config+0xc8/0x1bc
[c0361db0] [c01cbf90] composite_setup+0x20c/0x3a8
[c0361de0] [c01c7f88] setup_received_handle+0x1e4/0x26c
[c0361e00] [c01c807c] ep0_setup_handle+0x6c/0x74
[c0361e10] [c01c81f0] qe_ep0_rx+0x16c/0x17c
[c0361e30] [c01c9140] rx_irq+0x88/0xc0
[c0361e40] [c01c98c8] qe_udc_irq+0x10c/0x134
[c0361e60] [c006083c] handle_IRQ_event+0x5c/0xb0
[c0361e80] [c0062964] handle_level_irq+0xa8/0x144
[c0361ea0] [c0029a9c] qe_ic_cascade_low_ipic+0x58/0x90
[c0361eb0] [c0006820] do_IRQ+0xa4/0xc8
[c0361ec0] [c0013e88] ret_from_except+0x0/0x14
--- Exception: 501 at cpu_idle+0xa0/0x104
LR = cpu_idle+0xa0/0x104
[c0361f80] [c0009d88] cpu_idle+0x50/0x104 (unreliable)
[c0361fa0] [c027f07c] 0xc027f07c
[c0361fc0] [c030d91c] start_kernel+0x1ac/0x230
[c0361ff0] [3438] 0x3438
BUG: spinlock lockup on CPU#0, swapper/0, cf846c50
Call Trace:
[c0361ca0] [c0008a90] show_stack+0x4c/0x16c (unreliable)
[c0361ce0] [c0175df0] __spin_lock_debug+0xf4/0xf8
[c0361d10] [c0175e80] _raw_spin_lock+0x8c/0xb8
[c0361d20] [c027e5dc] _spin_lock_irqsave+0x30/0x48
[c0361d40] [c01c8450] qe_ep_init+0x80/0x3a4
[c0361d60] [c01c8828] qe_ep_enable+0xb4/0xe4
[c0361d80] [c01cb210] ecm_set_alt+0x88/0x12c
[c0361d90] [c01cbb10] set_config+0xc8/0x1bc
[c0361db0] [c01cbf90] composite_setup+0x20c/0x3a8
[c0361de0] [c01c7f88] setup_received_handle+0x1e4/0x26c
[c0361e00] [c01c807c] ep0_setup_handle+0x6c/0x74
[c0361e10] [c01c81f0] qe_ep0_rx+0x16c/0x17c
[c0361e30] [c01c9140] rx_irq+0x88/0xc0
[c0361e40] [c01c98c8] qe_udc_irq+0x10c/0x134
[c0361e60] [c006083c] handle_IRQ_event+0x5c/0xb0
[c0361e80] [c0062964] handle_level_irq+0xa8/0x144
[c0361ea0] [c0029a9c] qe_ic_cascade_low_ipic+0x58/0x90
[c0361eb0] [c0006820] do_IRQ+0xa4/0xc8
[c0361ec0] [c0013e88] ret_from_except+0x0/0x14
--- Exception: 501 at cpu_idle+0xa0/0x104
LR = cpu_idle+0xa0/0x104
[c0361f80] [c0009d88] cpu_idle+0x50/0x104 (unreliable)
[c0361fa0] [c027f07c] 0xc027f07c
[c0361fc0] [c030d91c] start_kernel+0x1ac/0x230
[c0361ff0] [3438] 0x3438
Thanks,
--
Anton Vorontsov
email: [EMAIL PROTECTED]
irc://irc.freenode.net/bd2
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] usb: add Freescale QE/CPM USB peripheral controller driver
On Wed, Aug 6, 2008 at 2:16 AM, Li Yang [EMAIL PROTECTED] wrote: +/*- + * Mask definitions for usb BD * + **/ +#define QE_SIZEOF_BD sizeof(struct qe_bd) + +#define BD_BUFFER_ARG(bd) (((struct qe_bd *)bd)-buf) +#define BD_BUFFER_CLEAR(bd) out_be32((BD_BUFFER_ARG(bd)), 0); +#define BD_BUFFER(bd) in_be32((BD_BUFFER_ARG(bd))) +#define BD_STATUS_AND_LENGTH_SET(bd, val) out_be32((u32 *)bd, val) +#define BD_STATUS_AND_LENGTH(bd)in_be32((u32 *)bd) +#define BD_BUFFER_SET(bd, buffer) out_be32((BD_BUFFER_ARG(bd)), \ + (u32)(buffer)) Delete all of these. Don't use these silly macros at all. Reference the structure fields directly, and use the in_ and out_ functions directly. Using macros like these encourages unnecessary typecasts. struct qe_bd has been defined, so you should use it. -- Timur Tabi Linux kernel developer at Freescale ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
