On Saturday 04 November 2006 10:42 pm, Benjamin Herrenschmidt wrote:
> The issue is that those have an EHCI/OHCI pair which has big endian
> registers but little manipulates DMA data structures in little endian
> form (some would say they are only half broken :-)
I don't actually understand why chip vendors do this kind of stuff:
needlessly byteswapping PCI reads/writes. Just to annoy people who
want to use portable drivers?
> The OHCI change is trivial.
... since the driver already has support for this type of chip braindamage.
Well, _similar_ braindamage.
> The EHCI change is more invasive as EHCI
> didn't have any support for big-endian cells so far, but it's also
> pretty much on the trivial side of things, mostly replacing readl/writel
> with ehci_readl/ehci_writel.
Right. In general I have no issues with this, except the OHCI bit noted
below. It's basically following the model we used for similar OHCI issues,
and the OHCI issue relates to some cleanup I've not yet pushed.
But you'd make Andrew Morton a bit happier if you were switching
readl (...)
to
ehci_readl(ehci, ...)
That is, remove whitespace-for-readability in the "new" code.
> --- linux-cell.orig/drivers/usb/host/ehci-fsl.c 2006-10-21
> 16:37:03.000000000 +1000
> +++ linux-cell/drivers/usb/host/ehci-fsl.c 2006-11-05 17:31:44.000000000
> +1100
> @@ -177,7 +177,7 @@
> case FSL_USB2_PHY_NONE:
> break;
> }
> - writel(portsc, &ehci->regs->port_status[port_offset]);
> + ehci_writel(ehci, portsc, &ehci->regs->port_status[port_offset]);
Here's an example of a vendor which implemented EHCI in a big-endian
environment and didn't feel compelled to add gratuitous byteswapping
for the standard register set ... even though, as non-PCI silicon,
it would be more natural to do it there.
Just sayin', you know. :)
> --- linux-cell.orig/drivers/usb/host/ohci-pci.c 2006-10-21
> 16:37:04.000000000 +1000
> +++ linux-cell/drivers/usb/host/ohci-pci.c 2006-11-05 17:31:44.000000000
> +1100
> @@ -25,6 +25,22 @@
> {
> struct ohci_hcd *ohci = hcd_to_ohci (hcd);
>
> +#ifdef CONFIG_USB_OHCI_BIG_ENDIAN_MMIO
> + if (hcd->self.controller) {
> + struct pci_dev *pdev = to_pci_dev(hcd->self.controller);
It'd actually be a BUG() if self->controller is null, so don't
bother to test it ...
> + /* for TOSHIBA CELLEB
> + *
> + *
> + */
> + if (pdev->vendor == PCI_VENDOR_ID_TOSHIBA_2
> + && pdev->device == 0x01b6) {
> + ohci->flags |= OHCI_BIG_ENDIAN_MMIO;
> + ohci_dbg (ohci,
> + "enabled OHCI_BIG_ENDIAN_MMIO\n");
> + }
> + }
> +#endif
> +
> ohci_hcd_init (ohci);
> return ohci_init (ohci);
> }
And that OHCI bit is the thing I'd rather not see done that way.
Appended is a patch that shows how I'd like to start handling new
OHCI quirks. It's not quite ready to merge as-is, for two reasons:
- ISTR it doesn't apply to the latest kernels;
- That _different_ Toshiba-specific quirk (ahem) isn't resolved
by the patch, for some reason yet tbd.
So, instead of adding more if/else/... please start building this
kind of table-based infrastructure. The other quirks can start
to switch over to this approach later.
- Dave
Add a more formal quirk infrastructure to the OHCI driver, using pci_device_id
and associated utilities.
Use it for a quirk specific to the Portege 4000, listed in osdl buzilla as
http://bugzilla.kernel.org/show_bug.cgi?id=6723 ... the other quirks can be
moved over to use this infrastructure later.
Index: g26/drivers/usb/host/ohci-pci.c
===================================================================
--- g26.orig/drivers/usb/host/ohci-pci.c 2006-10-13 15:30:36.000000000
-0700
+++ g26/drivers/usb/host/ohci-pci.c 2006-10-13 15:53:26.000000000 -0700
@@ -20,13 +20,42 @@
/*-------------------------------------------------------------------------*/
+static void broken_suspend(struct ohci_hcd *ohci)
+{
+ device_init_wakeup(&ohci_to_hcd(ohci)->self.root_hub->dev, 0);
+}
+
+static const struct pci_device_id quirks[] = { {
+ /* Toshiba portege 4000 */
+ .vendor = PCI_VENDOR_ID_AL,
+ .device = 0x5237,
+ .subvendor = PCI_VENDOR_ID_TOSHIBA_2,
+ .subdevice = 0x0004,
+ .driver_data = (unsigned long) broken_suspend,
+}, {
+ /* end: all zeroes */
+} };
+
static int
ohci_pci_reset (struct usb_hcd *hcd)
{
- struct ohci_hcd *ohci = hcd_to_ohci (hcd);
+ struct ohci_hcd *ohci = hcd_to_ohci (hcd);
+ int retval;
+ struct pci_dev *pdev;
+ const struct pci_device_id *id;
ohci_hcd_init (ohci);
- return ohci_init (ohci);
+ retval = ohci_init (ohci);
+
+ pdev = to_pci_dev(hcd->self.controller);
+ id = pci_match_id(quirks, pdev);
+ if (retval == 0 && id) {
+ void (*quirk)(struct ohci_hcd *ohci);
+ quirk = (void *) id->driver_data;
+ quirk(ohci);
+ }
+
+ return retval;
}
static int __devinit
@@ -228,7 +257,7 @@ static struct pci_driver ohci_pci_driver
.id_table = pci_ids,
.probe = usb_hcd_pci_probe,
- .remove = usb_hcd_pci_remove,
+ .remove = __devexit_p(usb_hcd_pci_remove),
#ifdef CONFIG_PM
.suspend = usb_hcd_pci_suspend,
-------------------------------------------------------------------------
Using Tomcat but need to do more? Need to support web services, security?
Get stuff done quickly with pre-integrated technology to make your job easier
Download IBM WebSphere Application Server v.1.0.1 based on Apache Geronimo
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=120709&bid=263057&dat=121642
_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
