Hello,
attached you can find new version of my patch for PXA27x UDC driver
support against kernel 2.6.22-rc3 (but it applies also ro rc6).
I'd like to know what I have to do in order to prepare this patch for
kernel inclusion. It's time PXA27x has its UDC driver into linux! :)
Thanks for your suggestions,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
> -----Original Message-----
> From: [email protected]
> [mailto:[email protected]] On Behalf Of Rodolfo
Giometti
> Sent: Thursday, June 28, 2007 6:36 PM
> To: [email protected]
> Cc: [email protected]; Andrew Morton
> Subject: [PATCH] PXA27x UDC driver.
>
> Hello,
>
> attached you can find new version of my patch for PXA27x UDC driver
> support against kernel 2.6.22-rc3 (but it applies also ro rc6).
>
> I'd like to know what I have to do in order to prepare this patch for
> kernel inclusion. It's time PXA27x has its UDC driver into linux! :)
>
> Thanks for your suggestions,
You should probably also cc: [email protected] and
David Brownell for UDC matters.
- Leo
On Thu, Jun 28, 2007 at 07:15:06PM +0800, Li Yang-r58472 wrote:
> You should probably also cc: [email protected] and
> David Brownell for UDC matters.
As suggest by Leo let me propose to you my new patch for PXA27x UDC
support.
Please, let me know what I have to do for kernel inclusion. :)
Thanks,
Rodolfo
P.S. Please, in the replay also add:
[email protected], [email protected]
and Andrew Morton <[email protected]> where I already sent the
patch.
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
On Thu, 28 Jun 2007 16:12:07 +0200
Rodolfo Giometti <[email protected]> wrote:
> On Thu, Jun 28, 2007 at 07:15:06PM +0800, Li Yang-r58472 wrote:
>
> > You should probably also cc: [email protected] and
> > David Brownell for UDC matters.
>
> As suggest by Leo let me propose to you my new patch for PXA27x UDC
> support.
>
> Please, let me know what I have to do for kernel inclusion. :)
>
Please feed it through the current version of scripts/checkpatch.pl and
think about fixing the large amount of trivia spew which ensues.
> diff --git a/arch/arm/mach-pxa/generic.c b/arch/arm/mach-pxa/generic.c
> index 64b08b7..7f390fd 100644
> --- a/arch/arm/mach-pxa/generic.c
> +++ b/arch/arm/mach-pxa/generic.c
> @@ -282,7 +282,11 @@ static struct resource pxa2xx_udc_resources[] = {
> static u64 udc_dma_mask = ~(u32)0;
>
> static struct platform_device udc_device = {
> +#ifdef CONFIG_PXA27x
> + .name = "pxa27x-udc",
> +#else
> .name = "pxa2xx-udc",
> +#endif
This looks odd. The same driver presents itself under a different name
according to a config option?
> @@ -0,0 +1,2395 @@
> +/*
> + * linux/drivers/usb/gadget/pxa27x_udc.c
> + * Intel PXA2xx and IXP4xx on-chip full speed USB device controllers
> + *
> + * Copyright (C) 2002 Intrinsyc, Inc. (Frank Becker)
> + * Copyright (C) 2003 Robert Schwebel, Pengutronix
> + * Copyright (C) 2003 Benedikt Spranger, Pengutronix
> + * Copyright (C) 2003 David Brownell
> + * Copyright (C) 2003 Joshua Wise
> + * Copyright (C) 2004 Intel Corporation
> + * Copyright (C) 2007 Rodolfo Giometti <[email protected]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#undef DEBUG
Why? -DDEBUG is normally provided on the command line. If the user _did_
go and set DEBUG, he presumably wants DEBUG.
> +/* #define VERBOSE DBG_VERBOSE */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/ioport.h>
> +#include <linux/types.h>
> +#include <linux/version.h>
> +#include <linux/errno.h>
> +#include <linux/delay.h>
> +#include <linux/sched.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/timer.h>
> +#include <linux/list.h>
> +#include <linux/interrupt.h>
> +#include <linux/proc_fs.h>
> +#include <linux/mm.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/platform_device.h>
> +
> +#include <asm/byteorder.h>
> +#include <asm/dma.h>
> +#include <asm/io.h>
> +#include <asm/irq.h>
> +#include <asm/system.h>
> +#include <asm/mach-types.h>
> +#include <asm/unaligned.h>
> +#include <asm/hardware.h>
> +#include <asm/arch/pxa-regs.h>
> +
> +#include <linux/usb/ch9.h>
> +#include <linux/usb_gadget.h>
> +
> +#include <asm/arch/udc.h>
> +
> +/*
> + * This driver handles the USB Device Controller (UDC) in Intel's PXA 27x
> + * series processors.
> + * Such controller drivers work with a gadget driver. The gadget driver
> + * returns descriptors, implements configuration and data protocols used
> + * by the host to interact with this device, and allocates endpoints to
> + * the different protocol interfaces. The controller driver virtualizes
> + * usb hardware so that the gadget drivers will be more portable.
> + *
> + * This UDC hardware wants to implement a bit too much USB protocol, so
> + * it constrains the sorts of USB configuration change events that work.
> + * The errata for these chips are misleading; some "fixed" bugs from
> + * pxa250 a0/a1 b0/b1/b2 sure act like they're still there.
> + */
> +
> +#define DRIVER_VERSION "28-Jun-2007"
> +#define DRIVER_DESC "PXA 27x USB Device Controller driver"
> +
> +static const char driver_name[] = "pxa27x_udc";
> +
> +static const char ep0name[] = "ep0";
> +
> +#undef USE_DMA
So DMA is busted?
> +#undef DISABLE_TEST_MODE
enabling DISABLE_TEST_MODE seens to enable test mode. Confused.
> +#ifdef CONFIG_PROC_FS
> +#define UDC_PROC_FILE
> +#endif
> +
> +#include "pxa27x_udc.h"
> +
> +#ifdef CONFIG_EMBEDDED
> +/* few strings, and little code to use them */
> +#undef DEBUG
> +#undef UDC_PROC_FILE
> +#endif
gag, this looks like a mess. Thise sort of logic should be implemented in
Kconfig, not in .c.
> +#ifdef USE_DMA
> +static int use_dma = 1;
> +module_param(use_dma, bool, 0);
> +MODULE_PARM_DESC(use_dma, "true to use dma");
> +
> +static void dma_nodesc_handler(int dmach, void *_ep);
> +static void kick_dma(struct pxa27x_ep *ep, struct pxa27x_request *req);
> +
> +#define DMASTR " (dma support)"
> +
> +#else /* !USE_DMA */
> +#define DMASTR " (pio only)"
> +#endif
> +
> +#ifdef CONFIG_USB_PXA27X_SMALL
> +#define SIZE_STR " (small)"
> +#else
> +#define SIZE_STR ""
> +#endif
> +
> +#ifdef DISABLE_TEST_MODE
> +/* (mode == 0) == no undocumented chip tweaks
> + * (mode & 1) == double buffer bulk IN
> + * (mode & 2) == double buffer bulk OUT
> + * ... so mode = 3 (or 7, 15, etc) does it for both
> + */
> +static ushort fifo_mode = 0;
Unneeded initialisation (checkpatch should catch this)
Use u16, not ushort. Or just "int".
> +module_param(fifo_mode, ushort, 0);
> +MODULE_PARM_DESC(fifo_mode, "pxa27x udc fifo mode");
> +#endif
> +
> +#define UDCISR0_IR0 0x3
> +#define UDCISR_INT_MASK (UDC_INT_FIFOERROR | UDC_INT_PACKETCMP)
> +#define UDCICR_INT_MASK UDCISR_INT_MASK
> +
> +#define UDCCSR_MASK (UDCCSR_FST | UDCCSR_DME)
> +/* ---------------------------------------------------------------------------
> + * endpoint related parts of the api to the usb controller hardware,
> + * used by gadget driver; and the inner talker-to-hardware core.
> + * ---------------------------------------------------------------------------
> + */
kernel comments normally look like
/*
* endpoint related parts of the api to the usb controller hardware,
* used by gadget driver; and the inner talker-to-hardware core.
*
*/
> +
> +static void pxa27x_ep_fifo_flush(struct usb_ep *ep);
> +static void nuke(struct pxa27x_ep *, int status);
> +
> +/* one GPIO should control a D+ pullup, so host sees this device (or not) */
> +static void pullup_off(void)
> +{
> + struct pxa2xx_udc_mach_info *mach = the_controller->mach;
> +
> + if (mach->gpio_pullup)
> + udc_gpio_set(mach->gpio_pullup, 0);
> + else if (mach->udc_command)
> + mach->udc_command(PXA2XX_UDC_CMD_DISCONNECT);
> +}
> +
> +static void pullup_on(void)
> +{
> + struct pxa2xx_udc_mach_info *mach = the_controller->mach;
> +
> + if (mach->gpio_pullup)
> + udc_gpio_set(mach->gpio_pullup, 1);
> + else if (mach->udc_command)
> + mach->udc_command(PXA2XX_UDC_CMD_CONNECT);
> +}
> +
> +static void pio_irq_enable(int ep_num)
> +{
> + if (ep_num < 16)
> + UDCICR0 |= 3 << (ep_num * 2);
> + else {
> + ep_num -= 16;
> + UDCICR1 |= 3 << (ep_num * 2);
> + }
> +}
We'd normally put braces around the "if" clause if there are braces around
the "else" clause. What you have there looks a bit funny.
> +static void pio_irq_disable(int ep_num)
> +{
> + ep_num &= 0xf;
> + if (ep_num < 16)
> + UDCICR0 &= ~(3 << (ep_num * 2));
> + else {
> + ep_num -= 16;
> + UDCICR1 &= ~(3 << (ep_num * 2));
> + }
> +}
Ditto
> + if (ep->ep_type != USB_ENDPOINT_XFER_BULK
> + && desc->bmAttributes != USB_ENDPOINT_XFER_INT) {
We'd normally lay this out as
if (ep->ep_type != USB_ENDPOINT_XFER_BULK &&
desc->bmAttributes != USB_ENDPOINT_XFER_INT) {
but what you have there is pretty common.
> + DMSG("%s, %s type mismatch\n", __FUNCTION__, _ep->name);
> + return -EINVAL;
> + }
> +
> + /* hardware _could_ do smaller, but driver doesn't */
> + if ((desc->bmAttributes == USB_ENDPOINT_XFER_BULK
> + && le16_to_cpu(desc->wMaxPacketSize)
> + != BULK_FIFO_SIZE)
> + || !desc->wMaxPacketSize) {
that expression is quite hard to read.
/* hardware _could_ do smaller, but driver doesn't */
if ((desc->bmAttributes == USB_ENDPOINT_XFER_BULK &&
le16_to_cpu(desc->wMaxPacketSize) != BULK_FIFO_SIZE) ||
!desc->wMaxPacketSize) {
or something like that?
> + DMSG("%s, bad %s maxpacket\n", __FUNCTION__, _ep->name);
> + return -ERANGE;
> + }
> +
> + dev = ep->dev;
> + if (!dev->driver || dev->gadget.speed == USB_SPEED_UNKNOWN) {
> + DMSG("%s, bogus device state\n", __FUNCTION__);
> + return -ESHUTDOWN;
> + }
> +
> + ep->desc = desc;
> + ep->dma = -1;
> + ep->stopped = 0;
> + ep->pio_irqs = ep->dma_irqs = 0;
> + ep->ep.maxpacket = le16_to_cpu(desc->wMaxPacketSize);
> +
> + /* flush fifo (mostly for OUT buffers) */
> + pxa27x_ep_fifo_flush(_ep);
> +
> + /* ... reset halt state too, if we could ... */
> +
> +#ifdef USE_DMA
> + /* for (some) bulk and ISO endpoints, try to get a DMA channel and
> + * bind it to the endpoint. otherwise use PIO.
> + */
> + DMSG("%s: called attributes=%d\n", __FUNCTION__, ep->ep_type);
> + switch (ep->ep_type) {
> + case USB_ENDPOINT_XFER_ISOC:
> + if (le16_to_cpu(desc->wMaxPacketSize) % 32)
> + break;
> + /* fall through */
> + case USB_ENDPOINT_XFER_BULK:
> + if (!use_dma || !ep->reg_drcmr)
> + break;
> + ep->dma = pxa_request_dma((char *)_ep->name,
> + (le16_to_cpu(desc->wMaxPacketSize) > 64)
> + ? DMA_PRIO_MEDIUM /* some iso */
> + : DMA_PRIO_LOW,
> + dma_nodesc_handler, ep);
> + if (ep->dma >= 0) {
> + *ep->reg_drcmr = DRCMR_MAPVLD | ep->dma;
> + DMSG("%s using dma%d\n", _ep->name, ep->dma);
> + }
> + default:
> + break;
> + }
> +#endif
> + DBG(DBG_VERBOSE, "enabled %s\n", _ep->name);
> + return 0;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/* for the pxa27x, these can just wrap kmalloc/kfree. gadget drivers
> + * must still pass correctly initialized endpoints, since other controller
> + * drivers may care about how it's currently set up (dma issues etc).
> + */
> +
> +/*
> + * pxa27x_ep_alloc_request - allocate a request data structure
> + */
> +static struct usb_request *pxa27x_ep_alloc_request(struct usb_ep *_ep,
> + unsigned int gfp_flags)
> +{
> + struct pxa27x_request *req;
> +
> + req = kmalloc(sizeof *req, gfp_flags);
> + if (!req)
> + return 0;
> +
> + memset(req, 0, sizeof *req);
use kzalloc()
> + INIT_LIST_HEAD(&req->queue);
> + return &req->req;
> +}
> +
>
>
> +
> +static int
> +write_packet(volatile u32 * uddr, struct pxa27x_request *req, unsigned max)
Please review Documentation/volatile-considered-harmful.txt
> +{
> + u32 *buf;
> + int length, count, remain;
> +
> + buf = (u32 *) (req->req.buf + req->req.actual);
> + prefetch(buf);
> +
> + /* how big will this packet be? */
> + length = min(req->req.length - req->req.actual, max);
> + req->req.actual += length;
> +
> + remain = length & 0x3;
> + count = length & ~(0x3);
> +
> + while (likely(count)) {
> + *uddr = *buf++;
> + count -= 4;
> + }
> +
> + if (remain) {
> + volatile u8 *reg = (u8 *) uddr;
> + char *rd = (u8 *) buf;
> +
> + while (remain--) {
> + *reg = *rd++;
> + }
> + }
> +
> + return length;
> +}
> +
>
> ...
>
> +
> +static void dma_nodesc_handler(int dmach, void *_ep, struct pt_regs *r)
> +{
> + struct pxa27x_ep *ep = _ep;
> + struct pxa27x_request *req, *req_next;
> + u32 dcsr, tmp, completed;
> +
> + local_irq_disable();
this looks fishy. local_irq_disable() only provides cpu-local protection.
Is this code SMP-correct? What's happening here? A comment is needed
explaining this, at least.
> +static inline void validate_fifo_size(struct pxa27x_ep *pxa_ep, u8 bmAttributes)
> +{
> + switch (bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
> + case USB_ENDPOINT_XFER_CONTROL:
> + pxa_ep->fifo_size = EP0_FIFO_SIZE;
> + break;
> + case USB_ENDPOINT_XFER_ISOC:
> + pxa_ep->fifo_size = ISO_FIFO_SIZE;
> + break;
> + case USB_ENDPOINT_XFER_BULK:
> + pxa_ep->fifo_size = BULK_FIFO_SIZE;
> + break;
> + case USB_ENDPOINT_XFER_INT:
> + pxa_ep->fifo_size = INT_FIFO_SIZE;
> + break;
> + default:
> + break;
> + }
> +}
There's no point in inlining this. With only one callsite the compiler
will inline it anyway. If we grow a second callsite, this fucntion is too
large to inline.
This is general truth, so please generally avoid inline.
> + ep->maxpacket = min((ushort) pxa_ep->fifo_size, desc->wMaxPacketSize);
Use min_t, not an open-coded cast.
Use u16.
> +#ifdef UDC_PROC_FILE
> +
> +static const char proc_node_name[] = "driver/udc";
/proc is for process-related things. Driver-related things go in /sys
> +
> +#define create_proc_files() \
> + create_proc_read_entry(proc_node_name, 0, NULL, udc_proc_read, dev)
> +#define remove_proc_files() \
> + remove_proc_entry(proc_node_name, NULL)
> +
> +#else /* !UDC_PROC_FILE */
> +#define create_proc_files() do {} while (0)
> +#define remove_proc_files() do {} while (0)
Please only use macros when you *must* use macros. When for some reason
you cannot use C. This is not such a case. IOW, convert to static
inlines.
> +#endif /* UDC_PROC_FILE */
> +
> + DMSG("registered gadget driver '%s'\n", driver->driver.name);
> + udc_enable(dev);
> + dump_state(dev);
> +
> + return 0;
> +
> + create_file_error:
> + driver->unbind(&dev->gadget);
> +
> + device_bind_error:
> + device_del(&dev->gadget.dev);
> +
> + device_add_error:
> + dev->driver = 0;
> + dev->gadget.dev.driver = 0;
> +
> + return retval;
> +}
We normally indent labels with zero or one spaces, not six.
> +}
> +
> +EXPORT_SYMBOL(usb_gadget_unregister_driver);
We normally leave zero blank lines between the } and the EXPORT_SYMBOL
(checkpatch should report this)
> +#ifndef enable_disconnect_irq
> +#define enable_disconnect_irq() do {} while (0)
> +#define disable_disconnect_irq() do {} while (0)
> +#endif
hm, OK, I guess this is somewhere where a macro is appropriate.
> +/*-------------------------------------------------------------------------*/
> +
> +static inline void clear_ep_state(struct pxa27x_udc *dev)
> +{
> + unsigned i;
> +
> + /* hardware SET_{CONFIGURATION,INTERFACE} automagic resets endpoint
> + * fifos, and pending transactions mustn't be continued in any case.
> + */
> + for (i = 1; i < UDC_EP_NUM; i++)
> + nuke(&dev->ep[i], -ECONNABORTED);
> +}
Probably too big to inline.
Has no callers.
> + if (i < 0) {
> + /* hardware automagic preventing STALL... */
> + if (dev->req_config) {
> + /* hardware sometimes neglects to tell
> + * tell us about config change events,
> + * so later ones may fail...
> + */
> + WARN("config change %02x fail %d?\n",
> + u.r.bRequest, i);
> + return;
> + /* TODO experiment: if has_cfr,
> + * hardware didn't ACK; maybe we
> + * could actually STALL!
> + */
> + }
> + DBG(DBG_VERBOSE, "protocol STALL, "
> + "%02x err %d\n", UDCCSR0, i);
> + stall:
what's that label doing all the way over there. It makes it rather hard to
find.
> + /* the watchdog timer helps deal with cases
> + * where udc seems to clear FST wrongly, and
> + * then NAKs instead of STALLing.
> + */
> + ep0start(dev, UDCCSR0_FST | UDCCSR0_FTF,
> + "stall");
> + start_watchdog(dev);
> + dev->ep0state = EP0_STALL;
> + LED_EP0_OFF;
> +
> + /* deferred i/o == no response yet */
> + } else if (dev->req_pending) {
> + if (likely(dev->ep0state == EP0_IN_DATA_PHASE
> + || dev->req_std || u.r.wLength))
> + ep0start(dev, 0, "defer");
> + else
> + ep0start(dev, UDCCSR0_IPR, "defer/IPR");
> + }
> +
> + /* expect at least one data or status stage irq */
> + return;
Burying returns deep inside large functions is unpopular: it can easily
lead to resource leaks and locking errors as the code evolves. It makes
review and auditing harder.
> + } else {
> + /* some random early IRQ:
> + * - we acked FST
> + * - IPR cleared
> + * - OPC got set, without SA (likely status stage)
> + */
> + UDCCSR0 = udccsr0 & (UDCCSR0_SA | UDCCSR0_OPC);
> + }
> + break;
> + case EP0_IN_DATA_PHASE: /* GET_DESCRIPTOR etc */
> + if (udccsr0 & UDCCSR0_OPC) {
> + UDCCSR0 = UDCCSR0_OPC | UDCCSR0_FTF;
> + DBG(DBG_VERBOSE, "ep0in premature status\n");
> + if (req)
> + done(ep, req, 0);
> + ep0_idle(dev);
> + } else { /* irq was IPR clearing */
> +
> + if (req) {
> + /* this IN packet might finish the request */
> + (void)write_ep0_fifo(ep, req);
> + } /* else IN token before response was written */
> + }
> + break;
> + case EP0_OUT_DATA_PHASE: /* SET_DESCRIPTOR etc */
> + if (udccsr0 & UDCCSR0_OPC) {
> + if (req) {
> + /* this OUT packet might finish the request */
> + if (read_ep0_fifo(ep, req))
> + done(ep, req, 0);
> + /* else more OUT packets expected */
> + } /* else OUT token before read was issued */
> + } else { /* irq was IPR clearing */
> +
> + DBG(DBG_VERBOSE, "ep0out premature status\n");
> + if (req)
> + done(ep, req, 0);
> + ep0_idle(dev);
> + }
> + break;
> + case EP0_STALL:
> + UDCCSR0 = UDCCSR0_FST;
> + break;
> + }
> + UDCISR0 = UDCISR_INT(0, UDCISR_INT_MASK);
> +}
> +
>
> ...
>
> +static void udc_init_ep(struct pxa27x_udc *dev)
> +{
> + int i;
> +
> + INIT_LIST_HEAD(&dev->gadget.ep_list);
> + INIT_LIST_HEAD(&dev->gadget.ep0->ep_list);
> +
> + for (i = 0; i < UDC_EP_NUM; i++) {
> + struct pxa27x_ep *ep = &dev->ep[i];
> +
> + ep->dma = -1;
> + if (i != 0) {
> + memset(ep, 0, sizeof(*ep));
> + }
unneeded braces
> + INIT_LIST_HEAD(&ep->queue);
> + }
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static void nop_release(struct device *dev)
> +{
> + DMSG("%s %s\n", __FUNCTION__, dev->bus_id);
> +}
> +
> +/* this uses load-time allocation and initialization (instead of
> + * doing it at run-time) to save code, eliminate fault paths, and
> + * be more obviously correct.
> + */
> +static struct pxa27x_udc memory = {
> + .gadget = {
> + .ops = &pxa27x_udc_ops,
> + .ep0 = &memory.ep[0].ep,
> + .name = driver_name,
> + .dev = {
> + .bus_id = "gadget",
> + .release = nop_release,
> + },
> + },
> +
> + /* control endpoint */
> + .ep[0] = {
> + .ep = {
> + .name = ep0name,
> + .ops = &pxa27x_ep_ops,
> + .maxpacket = EP0_FIFO_SIZE,
> + },
> + .dev = &memory,
> + .reg_udccsr = &UDCCSR0,
> + .reg_udcdr = &UDCDR0,
whitespace broke
> + }
> +};
> +
> +#define CP15R0_VENDOR_MASK 0xffffe000
> +
> +#define CP15R0_XSCALE_VALUE 0x69054000 /* intel/arm/xscale */
> +
> +/*
> + * probe - binds to the platform device
> + */
> +static int __init pxa27x_udc_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct pxa27x_udc *udc = &memory;
> + int irq, retval;
> + u32 chiprev;
> +
> + /* insist on Intel/ARM/XScale */
> + asm("mrc%? p15, 0, %0, c0, c0":"=r"(chiprev));
whitespace
> + if ((chiprev & CP15R0_VENDOR_MASK) != CP15R0_XSCALE_VALUE) {
> + printk(KERN_ERR "%s: not XScale!\n", driver_name);
> + return -ENODEV;
> + }
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return -ENODEV;
> + pr_debug("%s: IRQ %d\n", driver_name, irq);
> +
> + /* other non-static parts of init */
> + udc->dev = dev;
> + udc->mach = dev->platform_data;
> +
> + /* Disable irq, erase old events and disable the pull up on the bus */
> + UDCICR0 = 0x00000000;
> + UDCICR1 = 0x00000000;
> + UDCISR0 = 0xffffffff;
> + UDCISR1 = 0xffffffff;
> + if (udc->mach->gpio_pullup)
> + udc_gpio_init_pullup(udc->mach->gpio_pullup);
> +
> + init_timer(&udc->timer);
> + udc->timer.function = udc_watchdog;
> + udc->timer.data = (unsigned long)udc;
> +
> + device_initialize(&udc->gadget.dev);
> + udc->gadget.dev.parent = dev;
> + udc->gadget.dev.dma_mask = dev->dma_mask;
> +
> + the_controller = udc;
> + dev_set_drvdata(dev, udc);
> +
> + udc_disable(udc);
> + udc_init_ep(udc);
> + udc_reinit(udc);
> +
> + /* irq setup after old hardware state is cleaned up */
> + retval = request_irq(irq, pxa27x_udc_irq, 0, driver_name, udc);
> + if (retval != 0) {
> + printk(KERN_ERR "%s: can't get irq %i, err %d\n",
> + driver_name, irq, retval);
> + return -EBUSY;
> + }
> + udc->got_irq = 1;
> +
> + create_proc_files();
> +
> + return 0;
> +}
> +
>
> ...
>
> + /* UDCCSR = UDC Control/Status Register for this EP
> + * UBCR = UDC Byte Count Remaining (contents of OUT fifo)
> + * UDCDR = UDC Endpoint Data Register (the fifo)
> + * UDCCR = UDC Endpoint Configuration Registers
> + * DRCM = DMA Request Channel Map
> + */
> + volatile u32 *reg_udccsr;
> + volatile u32 *reg_udcbcr;
> + volatile u32 *reg_udcdr;
> + volatile u32 *reg_udccr;
more volatiles
> +#ifdef USE_DMA
> + volatile u32 *reg_drcmr;
> +#define drcmr(n) .reg_drcmr = & DRCMR ## n ,
> +#else
> +#define drcmr(n)
> +#endif
> +
> +#ifdef CONFIG_PM
> + unsigned udccsr_value;
> + unsigned udccr_value;
> +#endif
> +};
> +
>
> ...
>
> +
> +#define EP0_FIFO_SIZE ((unsigned)16)
> +#define BULK_FIFO_SIZE ((unsigned)64)
> +#define ISO_FIFO_SIZE ((unsigned)256)
> +#define INT_FIFO_SIZE ((unsigned)8)
#define INT_FIFO_SIZE 8U
would be nicer.
> +struct pxa27x_udc {
> + struct usb_gadget gadget;
> + struct usb_gadget_driver *driver;
> +
> + enum ep0_state ep0state;
> + struct udc_stats stats;
> + unsigned got_irq : 1,
> + got_disc : 1,
> + has_cfr : 1,
> + req_pending : 1,
> + req_std : 1,
> + req_config : 1;
> +
> +#define start_watchdog(dev) mod_timer(&dev->timer, jiffies + (HZ/200))
write it in C or, better, just open-code it at the callsite.
> + struct timer_list timer;
> +
> + struct device *dev;
> + struct pxa2xx_udc_mach_info *mach;
> + u64 dma_mask;
> + struct pxa27x_ep ep [UDC_EP_NUM];
> +
> + unsigned configuration,
> + interface,
> + alternate;
> +#ifdef CONFIG_PM
> + unsigned udccsr0;
> +#endif
> +};
>
> ...
>
> +#define HEX_DISPLAY2(n) do { \
> + if (machine_is_mainstone()) \
> + { MST_LEDDAT2 = (n); } \
> + } while(0)
See, this references "n"
> +
> +/* LEDs are only for debug */
> +#ifndef HEX_DISPLAY
> +#define HEX_DISPLAY(n) do {} while(0)
> +#endif
but this doesn't. So we risk getting unused-var warnings in callers
depending upon config options. Writing this in C rather than cpp (the
general rule) will fix this, as well as lots of other stuff.
HEX_DISPLAY2 gets different treatment from HEX_DISPLAY here.
> +#ifndef LED_CONNECTED_ON
> +#define LED_CONNECTED_ON do {} while(0)
> +#define LED_CONNECTED_OFF do {} while(0)
> +#endif
> +#ifndef LED_EP0_ON
> +#define LED_EP0_ON do {} while (0)
> +#define LED_EP0_OFF do {} while (0)
> +#endif
> +
> +static struct pxa27x_udc *the_controller;
eep, you can't do that! We'll get separate instances of the_controller in
each C file which includes this header.
> +/*-------------------------------------------------------------------------*/
> +
> +/*
> + * Debugging support vanishes in non-debug builds. DBG_NORMAL should be
> + * mostly silent during normal use/testing, with no timing side-effects.
> + */
> +#define DBG_NORMAL 1 /* error paths, device state transitions */
> +#define DBG_VERBOSE 2 /* add some success path trace info */
> +#define DBG_NOISY 3 /* ... even more: request level */
> +#define DBG_VERY_NOISY 4 /* ... even more: packet level */
> +
> +#ifdef DEBUG
> +
> +static const char *state_name[] = {
> + "EP0_IDLE",
> + "EP0_IN_DATA_PHASE", "EP0_OUT_DATA_PHASE",
> + "EP0_END_XFER", "EP0_STALL"
> +};
> +
> +#define DMSG(stuff...) printk(KERN_ERR "udc: " stuff)
> +
> +#ifdef VERBOSE
> +# define UDC_DEBUG DBG_VERBOSE
> +#else
> +# define UDC_DEBUG DBG_NORMAL
> +#endif
> +
> +static void __attribute__ ((__unused__))
Use __maybe_unused
> +dump_udccr(const char *label)
> +{
> + u32 udccr = UDCCR;
> + DMSG("%s 0x%08x =%s%s%s%s%s%s%s%s%s%s, con=%d,inter=%d,altinter=%d\n",
> + label, udccr,
> + (udccr & UDCCR_OEN) ? " oen":"",
> + (udccr & UDCCR_AALTHNP) ? " aalthnp":"",
> + (udccr & UDCCR_AHNP) ? " rem" : "",
> + (udccr & UDCCR_BHNP) ? " rstir" : "",
> + (udccr & UDCCR_DWRE) ? " dwre" : "",
> + (udccr & UDCCR_SMAC) ? " smac" : "",
> + (udccr & UDCCR_EMCE) ? " emce" : "",
> + (udccr & UDCCR_UDR) ? " udr" : "",
> + (udccr & UDCCR_UDA) ? " uda" : "",
> + (udccr & UDCCR_UDE) ? " ude" : "",
> + (udccr & UDCCR_ACN) >> UDCCR_ACN_S,
> + (udccr & UDCCR_AIN) >> UDCCR_AIN_S,
> + (udccr & UDCCR_AAISN)>> UDCCR_AAISN_S );
> +}
> +
> +static void __attribute__ ((__unused__))
ditto
> +dump_udccsr0(const char *label)
> +{
> + u32 udccsr0 = UDCCSR0;
> +
> + DMSG("%s %s 0x%08x =%s%s%s%s%s%s%s\n",
> + label, state_name[the_controller->ep0state], udccsr0,
> + (udccsr0 & UDCCSR0_SA) ? " sa" : "",
> + (udccsr0 & UDCCSR0_RNE) ? " rne" : "",
> + (udccsr0 & UDCCSR0_FST) ? " fst" : "",
> + (udccsr0 & UDCCSR0_SST) ? " sst" : "",
> + (udccsr0 & UDCCSR0_DME) ? " dme" : "",
> + (udccsr0 & UDCCSR0_IPR) ? " ipr" : "",
> + (udccsr0 & UDCCSR0_OPC) ? " opr" : "");
> +}
> +
> +static void __attribute__ ((__unused__))
ditto
> +dump_state(struct pxa27x_udc *dev)
> +{
> + unsigned i;
> +
> + DMSG("%s, udcicr %02X.%02X, udcsir %02X.%02x, udcfnr %02X\n",
> + state_name[dev->ep0state],
> + UDCICR1, UDCICR0, UDCISR1, UDCISR0, UDCFNR);
> + dump_udccr("udccr");
> +
> + if (!dev->driver) {
> + DMSG("no gadget driver bound\n");
> + return;
> + } else
> + DMSG("ep0 driver '%s'\n", dev->driver->driver.name);
> +
> +
> + dump_udccsr0 ("udccsr0");
> + DMSG("ep0 IN %lu/%lu, OUT %lu/%lu\n",
> + dev->stats.write.bytes, dev->stats.write.ops,
> + dev->stats.read.bytes, dev->stats.read.ops);
> +
> + for (i = 1; i < UDC_EP_NUM; i++) {
> + if (dev->ep [i].desc == 0)
> + continue;
> + DMSG ("udccs%d = %02x\n", i, *dev->ep->reg_udccsr);
> + }
> +}
> +
>
> ..
>
> +
> +#define WARN(stuff...) printk(KERN_WARNING "udc: " stuff)
> +#define INFO(stuff...) printk(KERN_INFO "udc: " stuff)
hrm. Why does every driver in the tree need to invent its own boilerplate
infrastructure?
can we use dev_warn() here or something?
On Thursday 28 June 2007, Rodolfo Giometti wrote:
> As suggest by Leo let me propose to you my new patch for PXA27x UDC
> support.
>
> Please, let me know what I have to do for kernel inclusion. :)
Let's start with *JUST* a driver, not trying to update everything
else in the USB Gadget stack so that it looks like it's designed
specifically to handle all of Intel's design botches related to
endpoint config ... and work worse for essentially everything else.
(Unlike pretty much every other vendor, Intel wanted hardware config
management. It was unusably buggy in pxa21x/25x/26x, and not much
better in pxa27x.)
So in technical terms, and to repeat what I've said before: just
configure it to act more like a PXA 25x chip (no altsettings) and
get it so it passes all the tests [1], modulo errata which have no
workarounds ... then submit that. No epautoconfig updates, no
patches to every gadget driver to cope with updated autoconfig.
Once there's a basic working no-frills version merged, then we can
talk about whether things in the rest of the stack should change
to accomodate the bizarre concepts of this controller.
- Dave
[1] http://www.linux-usb.org/usbtest/
By the way, there are three or four versions of a pxa27x UDC
driver floating around; which one is this? One you updated?
It looks like it forked from the pxa2xx driver several years ago
but never got cleaned up ... e.g. it's a different controller,
so none of the comments relevant to earlier controller versions
(pxa250 rev a0 etc) could possibly apply, or even later ones
(like pxa255, effectively end-of-the-line for that UDC).
Quite a lot of the code issues in this driver are inherited from
some rather ancient code dating to ... 2.4.19-rmk7 kernels, if
I don't mis-recall. Some of them are even specific to the now
obsolete "Lubbock" reference hardware.
In short: whatever version of a pxa27x_udc driver gets submitted,
much cleanup seems *STILL* to be needed.
On Thursday 28 June 2007, Andrew Morton wrote:
> > --- a/arch/arm/mach-pxa/generic.c
> > +++ b/arch/arm/mach-pxa/generic.c
> > @@ -282,7 +282,11 @@ static struct resource pxa2xx_udc_resources[] = {
> > static u64 udc_dma_mask = ~(u32)0;
> >
> > static struct platform_device udc_device = {
> > +#ifdef CONFIG_PXA27x
> > + .name = "pxa27x-udc",
> > +#else
> > .name = "pxa2xx-udc",
> > +#endif
>
> This looks odd. The same driver presents itself under a different name
> according to a config option?
The PXA 27x platform devices should really have been in a
different file ... the PXA platform has been rather neglected,
since it effectively has no maintainer.
>
> > + * This UDC hardware wants to implement a bit too much USB protocol, so
> > + * it constrains the sorts of USB configuration change events that work.
> > + * The errata for these chips are misleading; some "fixed" bugs from
> > + * pxa250 a0/a1 b0/b1/b2 sure act like they're still there.
And *STILL* people carry around comments from the pxa2xx_udc
code (for "x" in 1, 6, and mostly 5).
ISTR that every time someone has submitted a pxa27x driver I've
had to ask that obsolete comments be removed. Still waiting...
> > +#undef USE_DMA
>
> So DMA is busted?
This driver was largely cut'n'paste from pxa2xx_udc, which
had to cope with errata which mostly meant that it couldn't
work. Then there were *DESIGN BUGS* in the DMA, making it
not worth using in the most significant cases ... even in
the later chip revisions where DMA would allegedly work.
(Which reminds me, maybe I should just rip DMA out of the
pxa2xx_udc code ... nobody seems to have picked it up and
finished it, so there's no point in keeping that around.)
I'm told that DMA works better in pxa27x, and that at least
one version of the pxa27x_udc driver enabled it by default.
(For TX, if not RX where it's most useful...) This would
seem not to be that version.
> > +#ifdef CONFIG_EMBEDDED
> > +/* few strings, and little code to use them */
> > +#undef DEBUG
> > +#undef UDC_PROC_FILE
> > +#endif
>
> gag, this looks like a mess. Thise sort of logic should be implemented in
> Kconfig, not in .c.
The debug file stuff *DOES* have a Kconfig hook...
> > +#ifndef enable_disconnect_irq
> > +#define enable_disconnect_irq() do {} while (0)
> > +#define disable_disconnect_irq() do {} while (0)
> > +#endif
>
> hm, OK, I guess this is somewhere where a macro is appropriate.
Not really. It's an artifact of a rather bizarre FPGA design
on the PXA25x reference design ("Lubbock"), which was very
gratuitously different from what Intel docs recommended. The
normal case is that there is a single IRQ.
That stuff is long gone even from pxa2xx_udc ... and in any case,
the VBUS irqs should not be called "connect/disconnect", it's
just confusing to do that.
> > + /* hardware sometimes neglects to tell
> > + * tell us about config change events,
> > + * so later ones may fail...
> > + */
Curious ... did that pxa255 erratum transfer somehow to the
newer pxa27x silicon?
> > + WARN("config change %02x fail %d?\n",
> > + u.r.bRequest, i);
> > + return;
> > + /* TODO experiment: if has_cfr,
> > + * hardware didn't ACK; maybe we
> > + * could actually STALL!
> > + */
Another comment that shouldn't be relevant on this driver.
ISTR that all PXA 27x chips have CFR. It was new in PXA255,
which is why the pxa2xx_udc had to cope with its absence.
> HEX_DISPLAY2 gets different treatment from HEX_DISPLAY here.
Yeah, but all that stuff was specific to the Lubbock platform.
Best to just remove it.
> > +#define WARN(stuff...) printk(KERN_WARNING "udc: " stuff)
> > +#define INFO(stuff...) printk(KERN_INFO "udc: " stuff)
>
> hrm. Why does every driver in the tree need to invent its own boilerplate
> infrastructure?
>
> can we use dev_warn() here or something?
That stuff dates from 2.4.19-rmk7 kernel support, which
significantly predates dev_warn(). ;)
- Dave
On Thu, Jun 28, 2007 at 02:53:22PM -0700, David Brownell wrote:
>
> Let's start with *JUST* a driver, not trying to update everything
> else in the USB Gadget stack so that it looks like it's designed
> specifically to handle all of Intel's design botches related to
> endpoint config ... and work worse for essentially everything else.
>
> (Unlike pretty much every other vendor, Intel wanted hardware config
> management. It was unusably buggy in pxa21x/25x/26x, and not much
> better in pxa27x.)
>
>
> So in technical terms, and to repeat what I've said before: just
> configure it to act more like a PXA 25x chip (no altsettings) and
> get it so it passes all the tests [1], modulo errata which have no
> workarounds ... then submit that. No epautoconfig updates, no
> patches to every gadget driver to cope with updated autoconfig.
This looks interesting... as you alredy told this driver derives from
an older one, I just maintained it till now.
If I well understand I should remove usb_ep_autoconfig() and program
into the controller only one (the default) configuration. Is that
right?
Currently I tested the driver only with ether gadget, but if I do as
above, should I get the driver working with all gadgets automagically?
:)
Thanks a lot,
Rodolfo
--
GNU/Linux Solutions e-mail: [email protected]
Linux Device Driver [email protected]
Embedded Systems [email protected]
UNIX programming phone: +39 349 2432127
David Brownell wrote:
> On Thursday 28 June 2007, Rodolfo Giometti wrote:
>
>> As suggest by Leo let me propose to you my new patch for PXA27x UDC
>> support.
>>
>> Please, let me know what I have to do for kernel inclusion. :)
>
> Let's start with *JUST* a driver, not trying to update everything
> else in the USB Gadget stack so that it looks like it's designed
> specifically to handle all of Intel's design botches related to
> endpoint config ... and work worse for essentially everything else.
>
> (Unlike pretty much every other vendor, Intel wanted hardware config
> management. It was unusably buggy in pxa21x/25x/26x, and not much
> better in pxa27x.)
>
>
> So in technical terms, and to repeat what I've said before: just
> configure it to act more like a PXA 25x chip (no altsettings) and
> get it so it passes all the tests [1], modulo errata which have no
> workarounds
Other options are:
1. pre-program endpoints so the setting covers all gadget
configurations, it seems it's feasible. The only appreciable
change is, CDC Ethernet config number should be 3 instead of 1.
It should not break anything.
2. Implement a FAKE call of GET_CONFIGURATION command so upon
gadget binding you can issue the command and program endpoints
according to the received gadget configuration.
Also, considering that PXA3XX processors include PXA27x-compatible
USB device controller it makes sense to develop a driver that
will support both processor families. Hopefully PXA3XX arch
support will be merged some day (the arch support has been already
submitted here, but I don't know about its current status).
Regards,
Dmitry
> ... then submit that. No epautoconfig updates, no
> patches to every gadget driver to cope with updated autoconfig.
>
> Once there's a basic working no-frills version merged, then we can
> talk about whether things in the rest of the stack should change
> to accomodate the bizarre concepts of this controller.
>
> - Dave
>
>
> [1] http://www.linux-usb.org/usbtest/
>
Rodolfo Giometti wrote:
> On Thu, Jun 28, 2007 at 02:53:22PM -0700, David Brownell wrote:
>> Let's start with *JUST* a driver, not trying to update everything
>> else in the USB Gadget stack so that it looks like it's designed
>> specifically to handle all of Intel's design botches related to
>> endpoint config ... and work worse for essentially everything else.
>>
>> (Unlike pretty much every other vendor, Intel wanted hardware config
>> management. It was unusably buggy in pxa21x/25x/26x, and not much
>> better in pxa27x.)
>>
>>
>> So in technical terms, and to repeat what I've said before: just
>> configure it to act more like a PXA 25x chip (no altsettings) and
>> get it so it passes all the tests [1], modulo errata which have no
>> workarounds ... then submit that. No epautoconfig updates, no
>> patches to every gadget driver to cope with updated autoconfig.
>
> This looks interesting... as you alredy told this driver derives from
> an older one, I just maintained it till now.
>
> If I well understand I should remove usb_ep_autoconfig() and program
> into the controller only one (the default) configuration. Is that
> right?
You should leave the usb_ep_autoconfig() as is, i.e. do not introduce
any new parameters to it.
>
> Currently I tested the driver only with ether gadget,
Why have you submitted the limited driver then? You should test
the driver with all gadgets. Also, please report how it works
in DMA and PIO modes. And what transfer rate the driver achives
in both modes.
> but if I do as
> above, should I get the driver working with all gadgets automagically?
Probably no, but technically there is no reason to not support
all gadgets.
BTW, I suggest that you wait until PXA3XX arch support will be
merged into mainline kernel, then, pxa27x_udc-compatible driver
will probably be submitted too. Taking into account that
pxa27_udc have not been merged for years, I think extra
2-3 month is ok.
Regards,
Dmitry
> :)
>
> Thanks a lot,
>
> Rodolfo
>
Thanks for taking the lead on this! I can't wait to have a sane PXA27x
gadget driver in mainline.
On Thu, Jun 28, 2007 at 12:36:20PM +0200, Rodolfo Giometti wrote:
> +config USB_GADGET_PXA27X
> + boolean "PXA 27x"
> + depends on ARCH_PXA && PXA27x
> + help
> + Intel's PXA 27x series XScale processors include an integrated
XScale is a Marvell product now, not Intel. How about
XScale PXA 27x processors (from Marvell, formerly Intel) include ...
> + Say "y" to link the driver statically, or "m" to build a
> + dynamically linked module called "pxa2xx_udc" and force all
> + gadget drivers to also be dynamically linked.
Please copy the boilerplate on this:
To compile this driver as a module, choose M here: the
module will be called pxa27x_udc.
(Note the fixed module name, too.)
> + if (!_ep || !ep->desc) {
> + DMSG("%s, %s not enabled\n", __FUNCTION__,
> + _ep ? ep->ep.name : NULL);
I wouldn't pass NULL to a printf-format-string-using function, and ':'
would be a more traditional separator than ','. (Many instances of the
latter.)
> + if (dcsr & DCSR_BUSERR) {
> + DCSR(dmach) = DCSR_BUSERR;
> + printk(KERN_ERR " Buss Error\n");
Extra space after ", and Bus is misspelled. This printk should include
as much information about the error as reasonable.
> +static int pxa27x_ep_fifo_status(struct usb_ep *_ep)
> +{
> + struct pxa27x_ep *ep;
> +
> + ep = container_of(_ep, struct pxa27x_ep, ep);
No reason not to write
struct pxa27x_ep *ep = container_of(_ep, struct pxa27x_ep, ep);
> + while (((*ep->reg_udccsr) & UDCCSR_BNE) != 0)
> + (void)*ep->reg_udcdr;
That looks funky. On x86 I think you'd want a cpu_relax() in the loop
body, but I don't know if that's necessary on ARM. At the very least,
there's no reason to waste every other volatile read, so you should
remove the ->reg_udcdr read outside of the condition. Instead, the
standard seems to be
while (((*ep->reg_udccsr) & UDCCSR_BNE) != 0)
;
> + *ep->reg_udccsr = UDCCSR_PC | UDCCSR_FST | UDCCSR_TRN
> + | (ep->ep_type == USB_ENDPOINT_XFER_ISOC)
> + ? 0 : UDCCSR_SST;
More parentheses and/or indentation to make it clear how the ?: nests
with |.
> +#define NAME_SIZE 18
If this code isn't killed by David Brownell's comments, please either
make this a local variable or move the define to the top of the file
(and give it a name that describes what name it's the size of).
> + DMSG("udccsr=0x%8x, udcbcr=0x%8x, udcdr=0x%8x,"
> + "udccr0=0x%8x\n",
> + (unsigned)pxa_ep->reg_udccsr,
> + (unsigned)pxa_ep->reg_udcbcr,
> + (unsigned)pxa_ep->reg_udcdr, (unsigned)pxa_ep->reg_udccr);
Print pointers using %p.
> +#if 0
> + tmp |= (pxa_ep->interface << UDCCONR_IN_S) & UDCCONR_IN;
> + tmp |= (pxa_ep->aisn << UDCCONR_AISN_S) & UDCCONR_AISN;
> +#else
> + tmp |= (0 << UDCCONR_IN_S) & UDCCONR_IN;
> + tmp |= (0 << UDCCONR_AISN_S) & UDCCONR_AISN;
> +#endif
This stuff is wierd. It would be slightly more sane to just have the
code commented out, with a comment explaining why.
> + name = kmalloc(NAME_SIZE, GFP_KERNEL);
> + if (!name) {
> + printk(KERN_ERR "%s: Error\n", __FUNCTION__);
Useless printk. Should probably propagate ERR_PTR(-ENOMEM) back up the
call tree instead. (Several instances.)
> +udc_proc_read(char *page, char **start, off_t off, int count,
> + int *eof, void *_dev)
> +{
[snip]
> + t = scnprintf(next, size,
> + "uicr %02X.%02X, usir %02X.%02x, ufnr %02X\n",
> + UDCICR1, UDCICR0, UDCISR1, UDCISR0, UDCFNR);
> + size -= t;
> + next += t;
This code will get a lot simpler if you use seq_printf instead.
> +#define create_proc_files() \
> + create_proc_read_entry(proc_node_name, 0, NULL, udc_proc_read, dev)
> +#define remove_proc_files() \
> + remove_proc_entry(proc_node_name, NULL)
> +#else /* !UDC_PROC_FILE */
> +#define create_proc_files() do {} while (0)
> +#define remove_proc_files() do {} while (0)
> +#endif /* UDC_PROC_FILE */
The create_proc_files()/remove_proc_files() macros are nasty, and will
become unnecessary if you move the proc stuff to a separate file and use
Kconfig to optionally build it.
> +static DEVICE_ATTR(function, S_IRUGO, show_function, NULL);
Huh? This isn't used AFAICS.
> +static void udc_reinit(struct pxa27x_udc *dev)
> +{
> + u32 i;
No reason for this to be u32, use int. (Unless there's a significant
benefit in the generated code on ARM, perhaps.)
> + if (UDCCR & UDCCR_EMCE) {
> + printk(KERN_ERR
> + ": There are error in configuration, udc disabled\n");
Add the device name or some other identifier here. And there's probably
a missing return or goto.
> +#define CP15R0_VENDOR_MASK 0xffffe000
> +
> +#define CP15R0_XSCALE_VALUE 0x69054000 /* intel/arm/xscale */
These belong in a header file.
> +struct pxa27x_udc {
> + struct usb_gadget gadget;
> + struct usb_gadget_driver *driver;
> +
> + enum ep0_state ep0state;
> + struct udc_stats stats;
> + unsigned got_irq : 1,
> + got_disc : 1,
> + has_cfr : 1,
> + req_pending : 1,
> + req_std : 1,
> + req_config : 1;
> +
> +#define start_watchdog(dev) mod_timer(&dev->timer, jiffies + (HZ/200))
This define definitely doesn't belong here, and I don't think it belongs
in this header file at all -- or if it does, it needs a less generic
name.
> + struct timer_list timer;
> +
> + struct device *dev;
> + struct pxa2xx_udc_mach_info *mach;
> + u64 dma_mask;
> + struct pxa27x_ep ep [UDC_EP_NUM];
No space ^ here.
-andy
On Friday 29 June 2007, Dmitry Krivoschekov wrote:
> David Brownell wrote:
> > On Thursday 28 June 2007, Rodolfo Giometti wrote:
> >
> >> As suggest by Leo let me propose to you my new patch for PXA27x UDC
> >> support.
> >>
> >> Please, let me know what I have to do for kernel inclusion. :)
> >
> > Let's start with *JUST* a driver, not trying to update everything
> > else in the USB Gadget stack so that it looks like it's designed
> > specifically to handle all of Intel's design botches related to
> > endpoint config ... and work worse for essentially everything else.
> >
> > (Unlike pretty much every other vendor, Intel wanted hardware config
> > management. It was unusably buggy in pxa21x/25x/26x, and not much
> > better in pxa27x.)
> >
> >
> > So in technical terms, and to repeat what I've said before: just
> > configure it to act more like a PXA 25x chip (no altsettings) and
> > get it so it passes all the tests [1], modulo errata which have no
> > workarounds
>
> Other options are:
>
> 1. pre-program endpoints so the setting covers all gadget
> configurations, it seems it's feasible. The only appreciable
> change is, CDC Ethernet config number should be 3 instead of 1.
> It should not break anything.
ISTR looking at this in some detail a few years back, and
concluding that it wouldn't quite work. That was before
started to hear back from people that the pxa27x hardware
didn't really match its documentation, too ...
> 2. Implement a FAKE call of GET_CONFIGURATION command so upon
> gadget binding you can issue the command and program endpoints
> according to the received gadget configuration.
That would involve *multiple* such fake calls. Not the most
elegant of solutions, but ISTR using it in some other context.
But again, that presumes sane hardware, or at least hardware
that matches the docs. And people who've looked at this in
more pain than I have are consistent in telling me that pxa27x
endpoint configuration has problems that the docs and errata
do not describe. (If it were just one person, rather than four
different developers, I'd be somewhat skeptical.) Hence my
eventual conclusion (and advice) to just stop trying to use
that misfeature.
> Also, considering that PXA3XX processors include PXA27x-compatible
> USB device controller it makes sense to develop a driver that
> will support both processor families. Hopefully PXA3XX arch
> support will be merged some day (the arch support has been already
> submitted here, but I don't know about its current status).
Has someone actually signed up to develop and maintain such a
controller driver? If so, that would be a Fine Thing, but not
one I've heard rumored before. I've assumed that the best we'd
have is someone (Rodolfo?!) finally cleaning up a pxa27x version
so it could be merged.
- Dave
>
>
> Regards,
> Dmitry
>
> > ... then submit that. No epautoconfig updates, no
> > patches to every gadget driver to cope with updated autoconfig.
> >
> > Once there's a basic working no-frills version merged, then we can
> > talk about whether things in the rest of the stack should change
> > to accomodate the bizarre concepts of this controller.
> >
> > - Dave
> >
> >
> > [1] http://www.linux-usb.org/usbtest/
> >
>
On Friday 29 June 2007, Rodolfo Giometti wrote:
> On Thu, Jun 28, 2007 at 02:53:22PM -0700, David Brownell wrote:
> >
> > Let's start with *JUST* a driver, not trying to update everything
> > else in the USB Gadget stack so that it looks like it's designed
> > specifically to handle all of Intel's design botches related to
> > endpoint config ... and work worse for essentially everything else.
> >
> > (Unlike pretty much every other vendor, Intel wanted hardware config
> > management. It was unusably buggy in pxa21x/25x/26x, and not much
> > better in pxa27x.)
> >
> >
> > So in technical terms, and to repeat what I've said before: just
> > configure it to act more like a PXA 25x chip (no altsettings) and
> > get it so it passes all the tests [1], modulo errata which have no
> > workarounds ... then submit that. No epautoconfig updates, no
> > patches to every gadget driver to cope with updated autoconfig.
>
> This looks interesting... as you alredy told this driver derives from
> an older one, I just maintained it till now.
>
> If I well understand I should remove usb_ep_autoconfig() and program
> into the controller only one (the default) configuration. Is that
> right?
Other than the fact that "configuration" means something very specific
in USB terms, so there would be three of them ... yes. That's what the
PXA2[156]x UDC hardware does.
> Currently I tested the driver only with ether gadget, but if I do as
> above, should I get the driver working with all gadgets automagically?
> :)
There's not much point in merging a driver that only works with one
of what are currently six gadget drivers ... others are on the way.
Yes, the point of working more like PXA2[156]x hardware is that we
know that kind of hardware setup works with everything ... while we
know that every attempt to use the PXA27x magic has failed on some
of those drivers. (By all reports, because of hardware bugs.)
- Dave
On Thursday 28 June 2007, Andrew Morton wrote:
> > +#undef DISABLE_TEST_MODE
>
> enabling DISABLE_TEST_MODE seens to enable test mode. Confused.
Blame it on Intel. ISTR that early pxa2[156]x silicon had
something called "test mode". And a boatload of errata, with
a common thread in workarounds: using the "test mode" helped
many things work better, although it was neither necessary nor
sufficient. If one were to #define DISABLE_TEST_MODE, software
could experiment with other workarounds ... and maybe be able
to get the documented "double buffering" feature to work.
Later silicon stopped documenting "test mode" as such, effectively
making certain workarounds become the standard way to use the chip.
Which doesn't explain why pxa270 code has pxa2[156]x devel hooks,
but explains why pxa2[156]x code wants the hardware "test mode"
to be enabled ... except during developer experiments.
- Dave