USB: Rework OHCI PPC OF driver to support new bindings

Rework ohci-ppc-of driver to use big-endian property instead of ohci-be/ohci-le compatible strings. Also remove unnecessary user-selectable USB_OHCI_HCD_PPC_OF_LE/BE stuff, because USB_OHCI_BIG_ENDIAN_DESC/MMIO should always be enabled for ppc and USB_OHCI_LITTLE_ENDIAN is selected for USB_OHCI_HCD_PCI by default.

Signed-off-by Valentine Barshak <[email protected]>

---

config USB_UHCI_HCD
diff -pruN linux-2.6.orig/drivers/usb/host/ohci-ppc-of.c linux-2.6/drivers/usb/host/ohci-ppc-of.c

drivers/usb/host/ohci-ppc-of.c +2 -7

@@ -15,8 +15,8 @@
 
 #include <linux/signal.h>
 
-#include <asm/of_platform.h>
-#include <asm/prom.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 
 
 static int __devinit

@@ -91,15 +91,10 @@ ohci_hcd_ppc_of_probe(struct of_device *
 	int irq;
 
 	int rv;
-	int is_bigendian;
 
 	if (usb_disabled())
 		return -ENODEV;
 
-	is_bigendian =
-		of_device_is_compatible(dn, "ohci-bigendian") ||
-		of_device_is_compatible(dn, "ohci-be");
-
 	dev_dbg(&op->dev, "initializing PPC-OF USB Controller\n");
 
 	rv = of_address_to_resource(dn, 0, &res);

@@ -134,9 +129,10 @@ ohci_hcd_ppc_of_probe(struct of_device *
 	}

Can we just make sure real quickly that the changing of compatibles doesn't break existing, not-easily-flashable firmwares?

Quoted 2 lines · Matt Sealey

Can we just make sure real quickly that the changing of compatibles doesn't break existing, not-easily-flashable firmwares?

Yeah, I'm not keen on such breakage either...

Quoted 3 lines · David Brownell

Quoted 2 lines · Matt Sealey

Can we just make sure real quickly that the changing of compatibles doesn't break existing, not-easily-flashable firmwares?

Quoted 1 line

Yeah, I'm not keen on such breakage either...

Add my voice to the chorus. It's okay to change the binding, but make sure the old binding is still supported.

Cheers,
g.

Quoted 9 lines · Grant Likely

Quoted 4 lines · David Brownell

Quoted 2 lines · Matt Sealey

Can we just make sure real quickly that the changing of compatibles doesn't break existing, not-easily-flashable firmwares?

Yeah, I'm not keen on such breakage either...

Add my voice to the chorus. It's okay to change the binding, but make sure the old binding is still supported.

Quoted 3 lines

Cheers,
g.

Actually, I thought that changing the DTS stuff for mpc52xx boards would suffice. Sorry, I was unaware of Efika firmware here. I'll keep old bindings as well.

Does the device tree have "ohci-bigendian" or "ohci-be" compatible property on Efika?

Quoted 12 lines · Valentine Barshak

Quoted 11 lines · Grant Likely

Quoted 4 lines · David Brownell

Quoted 2 lines · Matt Sealey

Can we just make sure real quickly that the changing of compatibles doesn't break existing, not-easily-flashable firmwares?

Yeah, I'm not keen on such breakage either...

Add my voice to the chorus. It's okay to change the binding, but make sure the old binding is still supported.

Cheers,
g.

Quoted 3 lines

Actually, I thought that changing the DTS stuff for mpc52xx boards would suffice. Sorry, I was unaware of Efika firmware here. I'll keep old bindings as well.

Even if that were the case; I'm nervous about breaking compatibility with old device trees.

We probably need a formal guideline here. ie. When is it okay to drop compatibility with old dts files?

Quoted 2 lines

Does the device tree have "ohci-bigendian" or "ohci-be" compatible property on Efika?

If it doesn't, it can be added during prom_init.c We're already doing a bunch of efika fixups there anyway.

Cheers,
g.

Quoted 13 lines · Grant Likely

Quoted 12 lines · Valentine Barshak

Quoted 11 lines · Grant Likely

Quoted 4 lines · David Brownell

Quoted 2 lines · Matt Sealey

Can we just make sure real quickly that the changing of compatibles doesn't break existing, not-easily-flashable firmwares?

Yeah, I'm not keen on such breakage either...

Add my voice to the chorus. It's okay to change the binding, but make sure the old binding is still supported.

Cheers,
g.

Quoted 6 lines

Quoted 3 lines

Actually, I thought that changing the DTS stuff for mpc52xx boards would suffice. Sorry, I was unaware of Efika firmware here. I'll keep old bindings as well.

Even if that were the case; I'm nervous about breaking compatibility with old device trees.

If we keep the old bindings intact in the driver code then the old dts files should work fine. But I'm starting to doubt we really need any new bindings for this if we still have to keep the old ones. BTW, does anybody know of any ohci-le devices on OF bus?
Thanks,
Valentine.

Quoted 13 lines

We probably need a formal guideline here. ie. When is it okay to drop compatibility with old dts files?

Quoted 2 lines

Does the device tree have "ohci-bigendian" or "ohci-be" compatible property on Efika?

If it doesn't, it can be added during prom_init.c We're already doing a bunch of efika fixups there anyway.

Cheers,
g.

Valentine,

Please do the very minimal required to keep supporting the Efika.

As for an little endian OHCI controller on an OF bus, I would not consider it an impossibility. But, not having the big-endian property fixes this; OHCI is little-endian by default. You need only report "difference" in device trees, overzealous naming of a billion kinds of 99.99999% compatible controllers is just a waste of space.

I prefer the new binding to a degree. I like the big-endian property and I like the reporting of a standard controller type (usb-ohci rather than building in chip names). However by making the driver support only the recommending binding, we break old platforms for the sake of making new ones cleaner.

I wish someone would have sat down and defined the 5200 device tree in a design committee rather than a peer review post-commit system. In fact, that is a great idea, we can start this off with the MPC5121E right now, and get the damn thing RIGHT.

On 10/25/07, Valentine Barshak <[email protected]> wrote:

If it doesn't, it can be added during prom_init.c We're already doing a bunch of efika fixups there anyway.

Quoted 4 lines · Grant Likely

Quoted 3 lines · David Brownell

Quoted 2 lines · Matt Sealey

Can we just make sure real quickly that the changing of compatibles doesn't break existing, not-easily-flashable firmwares?

Quoted 8 lines

Quoted 1 line

Yeah, I'm not keen on such breakage either...

Add my voice to the chorus. It's okay to change the binding, but make sure the old binding is still supported.

Cheers,
g.

Actually, I thought that changing the DTS stuff for mpc52xx boards would suffice. Sorry, I was unaware of Efika firmware here. I'll keep old bindings as well. Does the device tree have "ohci-bigendian" or "ohci-be" compatible property on Efika?
Thanks,
Valentine.

Compatible property on /builtin@F0000000/usb@F0001000 is

ohci-bigendian
ohci-be
mpc5200-ohci
mpc5200-usb

device_type is "usb", model is "mpc5200-ohci".

Although I worry about cluttering up the cleanup, it is probably just adding an "if property(big-endian) OR compatible(mpc5200-ohci)" to that small big-endian check there.

I am currently moving on the assumption that the "correct" device tree for the Efika (notwithstanding the above) would be

usb@F0001000 {
device-type = "usb-ohci"
compatible = "mpc5200-ohci,mpc5200-usb-ohci"

big-endian
}

Or some variation including all the relevant checked-for properties.

I don't like the old "ohci-bigendian" and "ohci-be" properties. Picking out "ohci-bigendian" and "ohci-be" was someone's drunken idea, I'm sure, so I am happy to let them die a horrible death and never rear up ever again.

Using mpc5200-ohci out is by far the safest idea, although it leaves in a rather platform-specific fix, I prefer singling out that platform and potentially causing nasty looks towards the direction of Genesi/bplan, than having ohci-bigendian continue to exist for the sake of it :D

There is another solution; change the properties in the Linux device tree fixups, but I would loathe that solution as it adds yet another part of the kernel to track.

Unfortunately the current device tree is a complete, stupid mess, a result of a bunch of guys not looking at the problem, and I have said this before (rant mode :) - I think device_type, compatible should report the KIND of device it is, and the model property should be used to pick out the particular quirks of the chipset. We could have had a nice system where "usb" is paired with compatible "ohci", and model is "mpc5200". No dashes or spaces or 10 strings to compare..

Quoted 1 line · Matt Sealey

Compatible property on /builtin@F0000000/usb@F0001000 is

We should also keep "ohci-bigendian" and "ohci-be" in the match table.

Quoted 6 lines

I am currently moving on the assumption that the "correct" device tree for the Efika (notwithstanding the above) would be

usb@F0001000 {
device-type = "usb-ohci"
compatible = "mpc5200-ohci,mpc5200-usb-ohci"

It should also have compatible "usb-ohci" entry as a more general one. Others are for device-specific quirks:
compatible = "mpc5200-usb-ohci","usb-ohci"

Quoted 5 lines

Using mpc5200-ohci out is by far the safest idea, although it leaves in a rather platform-specific fix, I prefer singling out that platform and potentially causing nasty looks towards the direction of Genesi/bplan, than having ohci-bigendian continue to exist for the sake of it :D

So, do you suggest to use "mpc5200-ohci" instead of "ohci-be" in the match table?

Quoted 4 lines · Valentine Barshak

Quoted 1 line · Matt Sealey

Compatible property on /builtin@F0000000/usb@F0001000 is

We should also keep "ohci-bigendian" and "ohci-be" in the match table.

Eh.. maybe.

Quoted 7 lines

Quoted 6 lines

I am currently moving on the assumption that the "correct" device tree for the Efika (notwithstanding the above) would be

usb@F0001000 {
device-type = "usb-ohci"
compatible = "mpc5200-ohci,mpc5200-usb-ohci"

Quoted 3 lines

It should also have compatible "usb-ohci" entry as a more general one. Others are for device-specific quirks:
compatible = "mpc5200-usb-ohci","usb-ohci"

Why? It's in the device_type. You don't need to duplicate it as compatible with the same value as in the device_type.

Quoted 8 lines

Quoted 6 lines

Using mpc5200-ohci out is by far the safest idea, although it leaves in a rather platform-specific fix, I prefer singling out that platform and potentially causing nasty looks towards the direction of Genesi/bplan, than having ohci-bigendian continue to exist for the sake of it :D

So, do you suggest to use "mpc5200-ohci" instead of "ohci-be" in the match table?

Yes. I think ohci-be and ohci-bigendian should die. After all, it might get mixed with Firewire if you are not being careful.

If we had to start again, device-type of "usb" (that just makes it easier all round, it allows a system based on the MPC5200B alone to make the assumption of OHCI), compatibles of "usb-ohci" (since this makes it very specific that it is not just USB, but the OHCI spec) and big-endian property would be all there would be.

Model property would give the "mpc5200-ohci" value. Since nothing checks model (and this is not set on the firmware here), figuring on "mpc5200-ohci" as a compatible entry is good enough. Device-specific quirks should (Segher? Clarify please) never be futzed into compatible properties. At least the IEEE 1275 spec makes it clear that the model property is meant to clarify the particular device in question and is for information, I think defining a device as "USB", then subordinately as "OHCI flavor of USB" and particularly "the USB controller on an MPC5200 chip" (model) is all we need here, and in fact in any device.

You could say the same about any other device - why is the current standard to give each node a unique name based on chip docs? 5200 device tree spec says, use "gpt" as the name for the MPC5200 general purpose timers. Why not "timer" as the name, with "fsl,gpt" in the device_type or compatible property, and "mpc5200-gpt" in the model property? or "fsl,slt" compatible and "mpc5200-slt" model? Or "dma-controller" with a *model* of "bestcomm"?

Some of this makes me grind my teeth so much..

Quoted 5 lines · Valentine Barshak

Rework ohci-ppc-of driver to use big-endian property instead of ohci-be/ohci-le compatible strings. Also remove unnecessary user-selectable USB_OHCI_HCD_PPC_OF_LE/BE stuff, because USB_OHCI_BIG_ENDIAN_DESC/MMIO should always be enabled for ppc and USB_OHCI_LITTLE_ENDIAN is selected for USB_OHCI_HCD_PCI by default.

Can we just make sure real quickly that the changing of compatibles doesn't break existing, not-easily-flashable firmwares?

Quoted 2 lines

Quoted 2 lines

USB_OHCI_BIG_ENDIAN_DESC/MMIO should always be enabled for ppc and USB_OHCI_LITTLE_ENDIAN is selected for USB_OHCI_HCD_PCI by default.

I don't find those options useless. If you think the defauts are not the best change them but I find these options relevant. You don't always want the support for BE on PPC ... if the only controller you have is pci ... or if you're on a soc that use little endian ...

Do we really need to stop supporting the old values right now ?

IMHO, Changing to the usb-ohci with a property sounds fine to me if you really want to but don't drop support for the old values. They are not "bad". Change the dts to reflect the fact that the new "way" is preferred and a comment somewhere in the code and that should be fine.

Sylvain

Quoted 6 lines

Quoted 5 lines · Valentine Barshak

Rework ohci-ppc-of driver to use big-endian property instead of ohci-be/ohci-le compatible strings. Also remove unnecessary user-selectable USB_OHCI_HCD_PPC_OF_LE/BE stuff, because USB_OHCI_BIG_ENDIAN_DESC/MMIO should always be enabled for ppc and USB_OHCI_LITTLE_ENDIAN is selected for USB_OHCI_HCD_PCI by default.

I don't find those options useless. If you think the defauts are not the best change them but I find these options relevant. You don't always want the support for BE on PPC ... if the only controller you have is pci ... or if you're on a soc that use little endian ...