[PATCH 0/2] USB: Rework OHCI PPC OF driver to support new bindings

[PATCH 0/2] USB: Rework OHCI PPC OF driver to support new bindings

[Valentine Barshak]

These patches update ohci-ppc-of and the dts files for the new bindings.
The "compatible" is set to "usb-ohci" and the "big-endian" quirkiness is 
expressed by a property, not by the "compatible" name.
Also user-selectable USB_OHCI_HCD_PPC_OF_BE/USB_OHCI_HCD_PPC_OF_LE config
options are removed, since the USB_OHCI_BIG_ENDIAN/USB_OHCI_LITTLE_ENDIAN
selection is made by default for PPC/PCI OHCI controllers.

[PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[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.

Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
---
 drivers/usb/host/Kconfig       |   17 +++--------------
 drivers/usb/host/ohci-ppc-of.c |   38 ++++++--------------------------------
 2 files changed, 9 insertions(+), 46 deletions(-)

diff -pruN linux-2.6.orig/drivers/usb/host/Kconfig linux-2.6/drivers/usb/host/Kconfig
--- linux-2.6.orig/drivers/usb/host/Kconfig	2007-10-24 18:44:25.000000000 +0400
+++ linux-2.6/drivers/usb/host/Kconfig	2007-10-24 19:37:18.000000000 +0400
@@ -128,23 +128,12 @@ config USB_OHCI_HCD_PPC_OF
 	bool "OHCI support for PPC USB controller on OF platform bus"
 	depends on USB_OHCI_HCD && PPC_OF
 	default y
+	select USB_OHCI_BIG_ENDIAN_DESC
+	select USB_OHCI_BIG_ENDIAN_MMIO
 	---help---
 	  Enables support for the USB controller PowerPC present on the
 	  OpenFirmware platform bus.
 
-config USB_OHCI_HCD_PPC_OF_BE
-	bool "Support big endian HC"
-	depends on USB_OHCI_HCD_PPC_OF
-	default y
-	select USB_OHCI_BIG_ENDIAN_DESC
-	select USB_OHCI_BIG_ENDIAN_MMIO
-
-config USB_OHCI_HCD_PPC_OF_LE
-	bool "Support little endian HC"
-	depends on USB_OHCI_HCD_PPC_OF
-	default n
-	select USB_OHCI_LITTLE_ENDIAN
-
 config USB_OHCI_HCD_PCI
 	bool "OHCI support for PCI-bus USB controllers"
 	depends on USB_OHCI_HCD && PCI && (STB03xxx || PPC_MPC52xx || USB_OHCI_HCD_PPC_OF)
@@ -180,7 +169,7 @@ config USB_OHCI_BIG_ENDIAN_MMIO
 config USB_OHCI_LITTLE_ENDIAN
 	bool
 	depends on USB_OHCI_HCD
-	default n if STB03xxx || PPC_MPC52xx
+	default n if STB03xxx || PPC_MPC52xx || USB_OHCI_HCD_PPC_OF
 	default y
 
 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
--- linux-2.6.orig/drivers/usb/host/ohci-ppc-of.c	2007-10-24 18:44:25.000000000 +0400
+++ linux-2.6/drivers/usb/host/ohci-ppc-of.c	2007-10-24 19:32:21.000000000 +0400
@@ -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 *
 	}
 
 	ohci = hcd_to_ohci(hcd);
-	if (is_bigendian) {
+
+	if (of_get_property(dn, "big-endian", NULL)) {
 		ohci->flags |= OHCI_QUIRK_BE_MMIO | OHCI_QUIRK_BE_DESC;
-		if (of_device_is_compatible(dn, "mpc5200-ohci"))
+		if (of_device_is_compatible(dn, "mpc5200-usb-ohci"))
 			ohci->flags |= OHCI_QUIRK_FRAME_NO;
 	}
 
@@ -187,35 +183,13 @@ static int ohci_hcd_ppc_of_shutdown(stru
 
 
 static struct of_device_id ohci_hcd_ppc_of_match[] = {
-#ifdef CONFIG_USB_OHCI_HCD_PPC_OF_BE
-	{
-		.name = "usb",
-		.compatible = "ohci-bigendian",
-	},
-	{
-		.name = "usb",
-		.compatible = "ohci-be",
-	},
-#endif
-#ifdef CONFIG_USB_OHCI_HCD_PPC_OF_LE
-	{
-		.name = "usb",
-		.compatible = "ohci-littledian",
-	},
 	{
-		.name = "usb",
-		.compatible = "ohci-le",
+		.compatible = "usb-ohci",
 	},
-#endif
 	{},
 };
 MODULE_DEVICE_TABLE(of, ohci_hcd_ppc_of_match);
 
-#if	!defined(CONFIG_USB_OHCI_HCD_PPC_OF_BE) && \
-	!defined(CONFIG_USB_OHCI_HCD_PPC_OF_LE)
-#error "No endianess selected for ppc-of-ohci"
-#endif
-
 
 static struct of_platform_driver ohci_hcd_ppc_of_driver = {
 	.name		= "ppc-of-ohci",

[PATCH 2/2] PowerPC: Update USB OHCI DTS entires for new bindings

[Valentine Barshak]

Adjust mpc52xx DTS entries to support reworked ohci-ppc-of driver.
Use compatible "usb-ohci" and an empty big-endian property for 
USB_OHCI_BIG_ENDIAN_DESC and USB_OHCI_BIG_ENDIAN_MMIO support.
This also adds OHCI DTS entry for Sequoia PowerPC 440EPx board.

Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
---
 arch/powerpc/boot/dts/lite5200.dts  |    5 +++--
 arch/powerpc/boot/dts/lite5200b.dts |    5 +++--
 arch/powerpc/boot/dts/sequoia.dts   |    8 ++++++++
 3 files changed, 14 insertions(+), 4 deletions(-)

diff -pruN linux-2.6.orig/arch/powerpc/boot/dts/lite5200b.dts linux-2.6/arch/powerpc/boot/dts/lite5200b.dts
--- linux-2.6.orig/arch/powerpc/boot/dts/lite5200b.dts	2007-10-24 18:44:00.000000000 +0400
+++ linux-2.6/arch/powerpc/boot/dts/lite5200b.dts	2007-10-24 19:20:31.000000000 +0400
@@ -183,8 +183,9 @@
 		};
 
 		usb@1000 {
-			device_type = "usb-ohci-be";
-			compatible = "mpc5200b-ohci","mpc5200-ohci","ohci-be";
+			device_type = "usb-ohci";
+			compatible = "mpc5200b-usb-ohci","mpc5200-usb-ohci","usb-ohci";
+			big-endian;
 			reg = <1000 ff>;
 			interrupts = <2 6 0>;
 			interrupt-parent = <&mpc5200_pic>;
diff -pruN linux-2.6.orig/arch/powerpc/boot/dts/lite5200.dts linux-2.6/arch/powerpc/boot/dts/lite5200.dts
--- linux-2.6.orig/arch/powerpc/boot/dts/lite5200.dts	2007-10-24 18:44:00.000000000 +0400
+++ linux-2.6/arch/powerpc/boot/dts/lite5200.dts	2007-10-24 19:21:57.000000000 +0400
@@ -183,8 +183,9 @@
 		};
 
 		usb@1000 {
-			device_type = "usb-ohci-be";
-			compatible = "mpc5200-ohci","ohci-be";
+			device_type = "usb-ohci";
+			compatible = "mpc5200-usb-ohci","usb-ohci";
+			big-endian;
 			reg = <1000 ff>;
 			interrupts = <2 6 0>;
 			interrupt-parent = <&mpc5200_pic>;
diff -pruN linux-2.6.orig/arch/powerpc/boot/dts/sequoia.dts linux-2.6/arch/powerpc/boot/dts/sequoia.dts
--- linux-2.6.orig/arch/powerpc/boot/dts/sequoia.dts	2007-10-24 18:44:00.000000000 +0400
+++ linux-2.6/arch/powerpc/boot/dts/sequoia.dts	2007-10-24 19:30:36.000000000 +0400
@@ -122,6 +122,14 @@
 			interrupt-map-mask = <ffffffff>;
 		};
 
+		USB1: usb@e0000400 {
+			compatible = "usb-ohci-440epx", "usb-ohci";
+			reg = <0 e0000400 60>;
+			big-endian;
+			interrupt-parent = <&UIC0>;
+			interrupts = <15 8>;
+		};
+
 		POB0: opb {
 		  	compatible = "ibm,opb-440epx", "ibm,opb";
 			#address-cells = <1>;

Re: [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Matt Sealey]

Valentine Barshak wrote:
> 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 <vbarshak@ru.mvista.com>
> ---

[snip]

>  
>  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
> --- linux-2.6.orig/drivers/usb/host/ohci-ppc-of.c	2007-10-24 18:44:25.000000000 +0400
> +++ linux-2.6/drivers/usb/host/ohci-ppc-of.c	2007-10-24 19:32:21.000000000 +0400
> @@ -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 *
>  	}
>  
>  	ohci = hcd_to_ohci(hcd);
> -	if (is_bigendian) {
> +
> +	if (of_get_property(dn, "big-endian", NULL)) {
>  		ohci->flags |= OHCI_QUIRK_BE_MMIO | OHCI_QUIRK_BE_DESC;
> -		if (of_device_is_compatible(dn, "mpc5200-ohci"))
> +		if (of_device_is_compatible(dn, "mpc5200-usb-ohci"))
>  			ohci->flags |= OHCI_QUIRK_FRAME_NO;
>  	}

Just a note, this is a fairly destructive change and will stop the Efika
from having it's USB ports detected.

I've updated the Efika Device Tree Supplement script internally, but I
would really rather not have users be forced to update their kernel and
firmware quite so often just for what is, here, a merely aesthetic
change.

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

At least work in 'mpc5200-ohci' for the endian check (it's always big
endian, but our device tree has no big-endian property by default and
does not contain mpc5200-usb-ohci or usb-ohci properties) otherwise we are
going to have users complain. To you guys.

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[David Brownell]

On Wednesday 24 October 2007, Matt Sealey wrote:
> 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...

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Grant Likely]

On 10/24/07, David Brownell <david-b@pacbell.net> wrote:
> On Wednesday 24 October 2007, Matt Sealey wrote:
> > 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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Valentine Barshak]

Grant Likely wrote:
> On 10/24/07, David Brownell <david-b@pacbell.net> wrote:
>> On Wednesday 24 October 2007, Matt Sealey wrote:
>>> 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.
> 

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.

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Grant Likely]

On 10/25/07, Valentine Barshak <vbarshak@ru.mvista.com> wrote:
> Grant Likely wrote:
> > On 10/24/07, David Brownell <david-b@pacbell.net> wrote:
> >> On Wednesday 24 October 2007, Matt Sealey wrote:
> >>> 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.
> >
>
> 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?

> 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.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Valentine Barshak]

Grant Likely wrote:
> On 10/25/07, Valentine Barshak <vbarshak@ru.mvista.com> wrote:
>> Grant Likely wrote:
>>> On 10/24/07, David Brownell <david-b@pacbell.net> wrote:
>>>> On Wednesday 24 October 2007, Matt Sealey wrote:
>>>>> 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.
>>>
>> 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.

> 
> We probably need a formal guideline here.  ie. When is it okay to drop
> compatibility with old dts files?
> 
>> 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.
> 

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Matt Sealey]

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..

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Valentine Barshak wrote:
> Grant Likely wrote:
>> On 10/24/07, David Brownell <david-b@pacbell.net> wrote:
>>> On Wednesday 24 October 2007, Matt Sealey wrote:
>>>> 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.
>>
> 
> 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.

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Matt Sealey]

Grant Likely wrote:
> On 10/25/07, Valentine Barshak <vbarshak@ru.mvista.com> wrote:
 >
> If it doesn't, it can be added during prom_init.c  We're already doing
> a bunch of efika fixups there anyway.

I want those to go away. Far, far away.

http://www.powerdeveloper.org/platforms/efika/devicetree

Not the most elegant solution right now, but it works (kinda, a few bugs
to sort out).

Note that Domen's ethernet driver plus a bunch of Sylvain's stuff if
it is ever cleaned up (deep sleep etc.) will not work without these
device tree changes. You should realise that if we plugged every tiny
thing into prom_init.c we would double the size of the file just for
Efika fixes.

And that's dumb.

Compatibility with old device trees should go away after there is a
production firmware people can download - like the x86 hardware monitor
drivers in lmsensors report "please upgrade your BIOS" if they have
been disabled, users will happily update their BIOS to an updated
version if it is available.

For Efika, right now, it is not.

And for Efika, right now, I fear the stupidity some of the device tree
design (mandated by a text file..) means any new firmware update
will have far more strings and reporting than it should ever truly
need.

Although you can restrict Linux kernels from running on firmwares below
a certain version, we can't knowingly restrict the board firmware to only
running Linux kernels above a certain version.

Therefore, this is an exercise in not pissing people off, not really
of any technical merit. We already had Pegasos keyboard support disappear
because someone decided the device tree usage needed to be changed. Given
the size of the fix in nvramrc, it's harmless, given that Pegasos is
a dead platform, it's harmless. Efika is still in production.

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Valentine Barshak]

Matt Sealey wrote:
> 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.
> 

OK, I'll submit a new patch in a bit.
Thanks,
Valentine.

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Matt Sealey]

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.

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Valentine Barshak wrote:
> Grant Likely wrote:
>> On 10/25/07, Valentine Barshak <vbarshak@ru.mvista.com> wrote:
>>> Grant Likely wrote:
>>>> On 10/24/07, David Brownell <david-b@pacbell.net> wrote:
>>>>> On Wednesday 24 October 2007, Matt Sealey wrote:
>>>>>> 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.
>>>>
>>> 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.
> 
>>
>> We probably need a formal guideline here.  ie. When is it okay to drop
>> compatibility with old dts files?
>>
>>> 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.
>>
> 

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Valentine Barshak]

Matt Sealey wrote:
> 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.
>

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

> 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"

>     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

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

> 
> 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..
> 
:)

Thanks,
Valentine.

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Matt Sealey]

Valentine Barshak wrote:
> Matt Sealey wrote:
>> Compatible property on /builtin@F0000000/usb@F0001000 is
> 
> We should also keep "ohci-bigendian" and "ohci-be" in the match table.

Eh.. maybe.

>> 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"

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.

>> 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..

-- 
Matt Sealey <matt@genesi-usa.com>
Genesi, Manager, Developer Relations

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Valentine Barshak]

Matt Sealey wrote:
> Valentine Barshak wrote:
>> Matt Sealey wrote:
>>> Compatible property on /builtin@F0000000/usb@F0001000 is
>>
>> We should also keep "ohci-bigendian" and "ohci-be" in the match table.
> 
> Eh.. maybe.
> 
>>> 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"
> 
> 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.

The device-type thing shouldn't be used by Linux kernel.
Please, take a look at this discussion:
http://patchwork.ozlabs.org/linuxppc/patch?order=date&id=13514
Thanks,
Valentine.

> 
>>> 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..
> 

[PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[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.
The compatible "mpc5200-ohci" property is kept for old bindings support.

Signed-off-by: Valentine Barshak <vbarshak@ru.mvista.com>
---
 drivers/usb/host/Kconfig       |   17 ++--------------
 drivers/usb/host/ohci-ppc-of.c |   42 ++++++++++-------------------------------
 2 files changed, 14 insertions(+), 45 deletions(-)

diff -pruN linux-2.6.orig/drivers/usb/host/Kconfig linux-2.6/drivers/usb/host/Kconfig
--- linux-2.6.orig/drivers/usb/host/Kconfig	2007-10-25 19:20:12.000000000 +0400
+++ linux-2.6/drivers/usb/host/Kconfig	2007-10-25 22:59:34.000000000 +0400
@@ -128,23 +128,12 @@ config USB_OHCI_HCD_PPC_OF
 	bool "OHCI support for PPC USB controller on OF platform bus"
 	depends on USB_OHCI_HCD && PPC_OF
 	default y
+	select USB_OHCI_BIG_ENDIAN_DESC
+	select USB_OHCI_BIG_ENDIAN_MMIO
 	---help---
 	  Enables support for the USB controller PowerPC present on the
 	  OpenFirmware platform bus.
 
-config USB_OHCI_HCD_PPC_OF_BE
-	bool "Support big endian HC"
-	depends on USB_OHCI_HCD_PPC_OF
-	default y
-	select USB_OHCI_BIG_ENDIAN_DESC
-	select USB_OHCI_BIG_ENDIAN_MMIO
-
-config USB_OHCI_HCD_PPC_OF_LE
-	bool "Support little endian HC"
-	depends on USB_OHCI_HCD_PPC_OF
-	default n
-	select USB_OHCI_LITTLE_ENDIAN
-
 config USB_OHCI_HCD_PCI
 	bool "OHCI support for PCI-bus USB controllers"
 	depends on USB_OHCI_HCD && PCI && (STB03xxx || PPC_MPC52xx || USB_OHCI_HCD_PPC_OF)
@@ -180,7 +169,7 @@ config USB_OHCI_BIG_ENDIAN_MMIO
 config USB_OHCI_LITTLE_ENDIAN
 	bool
 	depends on USB_OHCI_HCD
-	default n if STB03xxx || PPC_MPC52xx
+	default n if STB03xxx || PPC_MPC52xx || USB_OHCI_HCD_PPC_OF
 	default y
 
 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
--- linux-2.6.orig/drivers/usb/host/ohci-ppc-of.c	2007-10-25 19:20:12.000000000 +0400
+++ linux-2.6/drivers/usb/host/ohci-ppc-of.c	2007-10-25 23:08:14.000000000 +0400
@@ -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,10 +129,14 @@ ohci_hcd_ppc_of_probe(struct of_device *
 	}
 
 	ohci = hcd_to_ohci(hcd);
-	if (is_bigendian) {
+
+	if (of_get_property(dn, "big-endian", NULL))
 		ohci->flags |= OHCI_QUIRK_BE_MMIO | OHCI_QUIRK_BE_DESC;
-		if (of_device_is_compatible(dn, "mpc5200-ohci"))
-			ohci->flags |= OHCI_QUIRK_FRAME_NO;
+
+	if (of_device_is_compatible(dn, "mpc5200-usb-ohci") ||
+	    of_device_is_compatible(dn, "mpc5200-ohci")) {
+		ohci->flags |= OHCI_QUIRK_BE_MMIO | OHCI_QUIRK_BE_DESC |
+			       OHCI_QUIRK_FRAME_NO;
 	}
 
 	ohci_hcd_init(ohci);
@@ -187,35 +186,16 @@ static int ohci_hcd_ppc_of_shutdown(stru
 
 
 static struct of_device_id ohci_hcd_ppc_of_match[] = {
-#ifdef CONFIG_USB_OHCI_HCD_PPC_OF_BE
 	{
-		.name = "usb",
-		.compatible = "ohci-bigendian",
+		.compatible = "mpc5200-ohci",
 	},
 	{
-		.name = "usb",
-		.compatible = "ohci-be",
-	},
-#endif
-#ifdef CONFIG_USB_OHCI_HCD_PPC_OF_LE
-	{
-		.name = "usb",
-		.compatible = "ohci-littledian",
+		.compatible = "usb-ohci",
 	},
-	{
-		.name = "usb",
-		.compatible = "ohci-le",
-	},
-#endif
 	{},
 };
 MODULE_DEVICE_TABLE(of, ohci_hcd_ppc_of_match);
 
-#if	!defined(CONFIG_USB_OHCI_HCD_PPC_OF_BE) && \
-	!defined(CONFIG_USB_OHCI_HCD_PPC_OF_LE)
-#error "No endianess selected for ppc-of-ohci"
-#endif
-
 
 static struct of_platform_driver ohci_hcd_ppc_of_driver = {
 	.name		= "ppc-of-ohci",

Re: [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[tnt]

> Valentine Barshak wrote:
>> 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 ...


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

I tend to agree with Matt here.

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

Re: [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Valentine Barshak]

tnt@blacksnow.net wrote:
>>> 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 ...

If I have just a pci controller, and don't expect to to find/use the 
built-in one, why should I need the ohci-ppc-of driver then?
I'm not aware of a soc that uses little endian, but I think that LE/BE 
still shouldn't be selected manually. It should be autoselected then, 
the same way we select LE for PCI for example.

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

I've resubmitted the patches with the old mpc52xx bindings support.
Keeping old ohci-be/le stuff along with the new bindings doesn't look 
good to me. However, I think we can live with the old ones and maybe 
find a better way for OF ohci support later.

> 
> 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.

OK

>     Sylvain
> 

Thanks,
Valentine.

Re: [linux-usb-devel] [PATCH 1/2] USB: Rework OHCI PPC OF for new bindings

[Dale Farnsworth]

Sylvain Munaut wrote:
> > Valentine Barshak wrote:
> >> 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 ...

USB_OHCI_LITTLE_ENDIAN and USB_OHCI_BIG_ENDIAN are useful and I
don't see anyone wanting to remove them.  However, if all of the
chips supported by the ohci-ppc-of driver are big-endian, then
USB_OHCI_HCD_PPC_OF_LE and USB_OHCI_HCD_PPC_OF_BE are not needed.
Just select USB_OHCI_BIG_ENDIAN when the ohci-ppc-of driver is
selected, in the same way that we always select USB_OHCI_LITTLE_ENDIAN
when the ohci-pci driver is selected.

-Dale