SL811 problem on mach-pxa

SL811 problem on mach-pxa

[Frank Buss]

I've tried to configure the SL811 driver with 2.6.11 for mach-pxa 
platform, but it doesn't work: The hub was recognized, but no device 
(I've tested it with a USB mouse and keyboard). The hub is visible in 
proc/bus/usb after mounting it.

I've tried to find the bug, but perhaps I'm wrong. This is what I've 
found:

For me it looks like the SL11H_INTMASK_INSRMV interupt in 
drivers/usb/host/sl811-hcd.c is not enabled. In other drivers it looks 
like it is enabled in the start function, like in ohci-pxa27x.c in 
pxa27x_start_hc. After adding a port_power(sl811, 1) call at the end of 
sl811h_start at least the driver gets the interupt, if a mouse is 
connected: it crashes in the "start" function, because ep->hep is NULL. I 
fixed this by setting ep->hep=hep in sl811h_urb_enqueue. But the driver 
still doesn't work. Now it doesn't crash, but I'll get some errors and 
the device is not recognized.

What can I do to find the problem? I wonder if the driver is working at 
all. In Linux 2.4 the driver worked on the same hardware, but looks like 
the driver in Linux 2.6 is rewritten from scratch. But I'm really lost 
when it comes to kernel programming, so maybe it is trivial to fix the 
problem and it is no driver bug.

Another question (perhaps this is related to my problem): Where do I have 
to provide the sl811_platform_data data and what values and functions are 
needed?

-- 
Frank Buß, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de

RE: SL811 problem on mach-pxa

[Frank Buss]

Now the driver is working, at least on my platform. Currently I'm using
version 2.6.11-rc1 as my base, but the diff output is only this:

--- linux-2.6.11-rc1.orig/drivers/usb/host/sl811-hcd.c  Wed Jan 12 05:00:38
2005
+++ linux-2.6.11-rc4.orig/drivers/usb/host/sl811-hcd.c  Sun Feb 13 04:05:51
2005
@@ -1042,7 +1042,7 @@

        usb_put_dev(ep->udev);
        kfree(ep);
-       hep->hcpriv = 0;
+       hep->hcpriv = NULL;
 }

 static int


Below is the patch. Comments are included for the changed parts. I don't
know, if Outlook preserves tabs, so you can download the patch here:

http://www.frank-buss.de/tmp/sl811-hcd.c-patch.txt

and the whole changed file:

http://www.frank-buss.de/tmp/sl811-hcd.c

There is still an important error: When a device is plugged, then opened and
then unplugged while open, it looks like the process freezes, which opened
the device (I've tried "cat /dev/input/mice" and I can't break it after
unplugged). After plugging the device again, it is not recognized any more.
When the device is not open or after closing the device, unlugging and
plugging again is no problem.


--- linux-2.6.11-rc1.orig/drivers/usb/host/sl811-hcd.c	Wed Jan 12 05:00:38
2005
+++ linux-2.6.11-rc1/drivers/usb/host/sl811-hcd.c	Thu Feb 17 04:37:55
2005
@@ -83,8 +83,9 @@
  */
 #define	DISABLE_ISO
 
-// #define	QUIRK2
-#define	QUIRK3
+/* with other QUIRK combinations it crashes */
+#define	QUIRK2
+//#define	QUIRK3
 
 static const char hcd_name[] = "sl811-hcd";
 
@@ -831,8 +832,11 @@
 #endif
 
 	/* avoid all allocations within spinlocks */
-	if (!hep->hcpriv)
+	if (!hep->hcpriv) {
 		ep = kcalloc(1, sizeof *ep, mem_flags);
+		/* set hep, otherwise sl811h_ep	*start(struct sl811 *sl811,
u8 bank) crashes */
+		ep->hep = hep;
+	}
 
 	spin_lock_irqsave(&sl811->lock, flags);
 
@@ -952,7 +956,11 @@
 		retval = 0;
 		goto fail;
 	}
-	urb->hcpriv = hep;
+	
+	/* when uncommented, causes this error on mouse open: 
+		drivers/usb/input/usbmouse.c: can't resubmit intr,
sl811-hcd-1/input0, status -22
+		urb->hcpriv = hep;  */
+	
 	spin_unlock(&urb->lock);
 
 	start_transfer(sl811);
@@ -1037,6 +1045,9 @@
 	/* assume we'd just wait for the irq */
 	if (!list_empty(&hep->urb_list))
 		msleep(3);
+
+	/* this error occurs, when the device is connected, opened and then
disconnected;
+	   after this warning, the process freezes */
 	if (!list_empty(&hep->urb_list))
 		WARN("ep %p not empty?\n", ep);
 
@@ -1580,6 +1591,14 @@
 	if (sl811->board && sl811->board->power)
 		hub_set_power_budget(udev, sl811->board->power * 2);
 
+	// enable power and interupts	
+	port_power(sl811, 1);
+
+	/* reset USB (without this the devices were not detected at boot,
only after plugging) */
+	sl811_write(sl811, SL11H_CTLREG1, 0x08);
+	mdelay(20);
+	sl811_write(sl811, SL11H_CTLREG1, 0);
+
 	return 0;
 }
 
@@ -1639,11 +1658,11 @@
 	free_irq(hcd->irq, hcd);
 
 	iounmap(sl811->data_reg);
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	release_mem_region(res->start, 1);
 
 	iounmap(sl811->addr_reg);
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res = platform_get_resource(pdev, IORESOURCE_IO, 0);
 	release_mem_region(res->start, 1);
 
 	usb_put_hcd(hcd);
@@ -1674,8 +1693,28 @@
 	if (pdev->num_resources < 3)
 		return -ENODEV;
 
-	addr = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	data = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	/* IORESOURCE_IO, because I think it is IO access. The following
+		resources in the platform description file works:
+		static struct resource usb_resources[] = {
+			[0] = {
+				.start	= YOUR_PLATFORM_USB_PHYS,
+				.end	= YOUR_PLATFORM_USB_PHYS + 256
+				.flags	= IORESOURCE_IO,
+			},
+			[1] = {
+				.start	= YOUR_PLATFORM_USB_PHYS + 0x010000,
+				.end	= YOUR_PLATFORM_USB_PHYS + 0x010000
+ 256
+				.flags	= IORESOURCE_MEM,
+			},
+			[2] = {
+				.start	= USB_IRQ,
+				.end	= USB_IRQ,
+				.flags	= IORESOURCE_IRQ,
+			},
+		};*/
+
+	addr = platform_get_resource(pdev, IORESOURCE_IO, 0);
+	data = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	irq = platform_get_irq(pdev, 0);
 	if (!addr || !data || irq < 0)
 		return -ENODEV;
@@ -1700,7 +1739,7 @@
 		retval = -EBUSY;
 		goto err3;
 	}
-	data_reg = ioremap(data->start, resource_len(addr));
+	data_reg = ioremap(data->start, resource_len(data));
 	if (data_reg == NULL) {
 		retval = -ENOMEM;
 		goto err4;
@@ -1728,7 +1767,6 @@
 	sl811->timer.data = (unsigned long) sl811;
 	sl811->addr_reg = addr_reg;
 	sl811->data_reg = data_reg;
-
 	spin_lock_irq(&sl811->lock);
 	port_power(sl811, 0);
 	spin_unlock_irq(&sl811->lock);





-- 
Frank Buß, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de

Re: SL811 problem on mach-pxa

[David Brownell]

On Wednesday 16 February 2005 7:51 pm, Frank Buss wrote:
> 
> http://www.frank-buss.de/tmp/sl811-hcd.c-patch.txt

Some of that looks reasonable, not all.  In particular, don't
change the convention on resources (memory to i/o), or expect
that the two regions involve more than one byte each ... the
hardware only has two single-byte registers!

I'll look at the ep->hep stuff ... I could believe rc1 got a
bug added there.  The urb->hcpriv bit looks wrong though.
It may take a little time for me to check it out though. 


> There is still an important error: When a device is plugged, then opened and
> then unplugged while open, it looks like the process freezes, which opened
> the device (I've tried "cat /dev/input/mice" and I can't break it after
> unplugged). After plugging the device again, it is not recognized any more.
> When the device is not open or after closing the device, unlugging and
> plugging again is no problem.

That seems pretty odd; I certainly tested that (on 2.6.almost-10)
as part of the initial development, and nothing in that area should
have changed either in the sl811 driver or usbcore.  I suspect the
issue is one of the other changes you made.


> -// #define	QUIRK2
> -#define	QUIRK3
> +/* with other QUIRK combinations it crashes */
> +#define	QUIRK2
> +//#define	QUIRK3

Also very odd.  It was tested with _both_ workarounds for IRQ issues;
and nobody else has reported any need for #2 any more (now that the
IRQs are acked selectively, unlike the predecessors to this driver).

If there's a crash there, don't paper it over like this.


> @@ -1580,6 +1591,14 @@
>  	if (sl811->board && sl811->board->power)
>  		hub_set_power_budget(udev, sl811->board->power * 2);
>  
> +	// enable power and interupts	
> +	port_power(sl811, 1);
> +
> +	/* reset USB (without this the devices were not detected at boot,
> only after plugging) */
> +	sl811_write(sl811, SL11H_CTLREG1, 0x08);
> +	mdelay(20);
> +	sl811_write(sl811, SL11H_CTLREG1, 0);
> +
>  	return 0;
>  }
>  

Hmm, what platform were you using?  I've had reports that one of the
KARO boards has that issue.  That looks like the sort of thing that
should be done in the reset() routine rather than start(); and it should
certainly use a symbolic constant not 0x08.

- Dave

RE: SL811 problem on mach-pxa

[Frank Buss]

> Some of that looks reasonable, not all.  In particular, don't
> change the convention on resources (memory to i/o), or expect
> that the two regions involve more than one byte each ... the
> hardware only has two single-byte registers!

ok, perhaps I've misunderstood the meaning of IORESOURCE_IO and
IORESOURCE_MEM. Is IORESOURCE_IO for "outb" and "inb" (Intel assembler,
don't know the Arm aquivalent)? Then you are right, it should be
IORESOURCE_MEM, only, because anything is accessed as like accessing normal
memory. I didn't found much documention of such low-level kernel
programming.

> I'll look at the ep->hep stuff ... I could believe rc1 got a
> bug added there.  The urb->hcpriv bit looks wrong though.
> It may take a little time for me to check it out though. 

ok, thanks. If you have a new patch, I'll try it on my platform.

> > There is still an important error: When a device is 
> > plugged, then opened and
> > then unplugged while open, it looks like the process 
> > freezes, which opened the device
> 
> That seems pretty odd; I certainly tested that (on 2.6.almost-10)
> as part of the initial development, and nothing in that area should
> have changed either in the sl811 driver or usbcore.  I suspect the
> issue is one of the other changes you made.

perhaps you are right, I don't understand the interactions between the
driver and the USB framework in detail.

> Hmm, what platform were you using?  I've had reports that one of the
> KARO boards has that issue.  

The platform was developed by a company I'm working for as a freelancer for
a product the company sells.

> That looks like the sort of thing that
> should be done in the reset() routine rather than start(); 
> and it should
> certainly use a symbolic constant not 0x08.

do you mean sl811->board->reset? I don't know, where I have to setup the
function pointer, but looks ok for me to reset the controller (and all
plugged USB devices) in the probe function.

-- 
Frank Buß, fb@frank-buss.de
http://www.frank-buss.de, http://www.it4-systems.de

Re: SL811 problem on mach-pxa

[David Brownell]

On Thursday 17 February 2005 11:11 am, Frank Buss wrote:
> > Some of that looks reasonable, not all.  In particular, don't
> > change the convention on resources (memory to i/o), or expect
> > that the two regions involve more than one byte each ... the
> > hardware only has two single-byte registers!
> 
> ok, perhaps I've misunderstood the meaning of IORESOURCE_IO and
> IORESOURCE_MEM. Is IORESOURCE_IO for "outb" and "inb" (Intel assembler,
> don't know the Arm aquivalent)?

Yes, and there _is_ no ARM equivalent.  Modern CPUs generaly won't
bother with a separate physical I/O space, and special instructions
to access it.  So Linux drivers use macros that always boil down to
normal memory-mapped I/O accessors ... that's what ARM does, and
oddly enough all current users of this driver are on PXA hardware.

I'm not sure what the CFU1U(*) CF/PCMCIA support will need; if that
card maps those registers to I/O space, this driver will need some
minor tweaks (in addition to the PCMCIA framework glue that registers
a new platform_device).  That is, the registers would normally be
memory-mapped, but on some boards they might need to be in I/O space.

All your platform should need to do is initialize a platform_device
with the three resources (two memory, one IRQ), and platform_data
initialized to match the structure defined for that purpose.


> > That looks like the sort of thing that
> > should be done in the reset() routine rather than start(); 
> > and it should
> > certainly use a symbolic constant not 0x08.
> 
> do you mean sl811->board->reset? 

If your board is wired to support a separate chip reset (maybe
through a GPIO or something), yes; otherwise, I suppose in this
case I meant the port_power() routine should be handling that.

This driver doesn't have a separate hc_driver.reset() entry, it
does the analagous stuff as part of powering up the chip ... but
as you've noted, that approach is a bit troublesome on boards
that don't have that level of control over the hardware.

It's best to ask questions about USB drivers on linux-usb-devel,
not all USB developers make time to swim through LKML.

- Dave

(*) http://www.ratocsystems.com/english/products/subpages/cfu1u.html
    CompactFlash single-port USB host adapter, for PDAs, using
    the SL811HS chip.