ohci and ehci-platform clks, phy and dt support

Add support for ohci-platform instantiation from devicetree, including optionally getting clks and a phy from devicetree, and enabling / disabling those on power_on / off.

This should allow using ohci-platform from devicetree in various cases. Specifically after this commit it can be used for the ohci controller found on Allwinner sunxi SoCs.

Signed-off-by Hans de Goede <[email protected]>

---
  .../devicetree/bindings/usb/mmio-ohci.txt          |  22 +++
  drivers/usb/host/ohci-platform.c                   | 164 ++++++++++++++++++---
  2 files changed, 162 insertions(+), 24 deletions(-)

create mode 100644 Documentation/devicetree/bindings/usb/mmio-ohci.txt

Documentation/devicetree/bindings/usb/mmio-ohci.txt new file +22 -0

new file mode 100644
index 0000000..9c776ed
@@ -0,0 +1,22 @@
+Generic MMIO OHCI controller
+
+Required properties:
+- compatible : "mmio-ohci"
+- reg : ohci controller register range (address and length)
+- interrupts : ohci controller interrupt
+
+Optional properties:
+- clocks : a list of phandle + clock specifier pairs
+- phys : phandle + phy specifier pair
+- phy-names : "usb"
+
+Example:
+
+	ohci0: ohci at 0x01c14400 {
+		compatible = "mmio-ohci";
+		reg = <0x01c14400 0x100>;
+		interrupts = <64>;
+		clocks = <&usb_clk 6>, <&ahb_gates 2>;
+		phys = <&usbphy 1>;
+		phy-names = "usb";
+	};

... <snip> ....

Documentation/devicetree/bindings/usb/mmio-ohci.txt new file +20 -0

@@ -83,17 +156,40 @@ static int ohci_platform_probe(struct platform_device *dev)
  		return -ENXIO;
  	}
 
+	hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev,
+			dev_name(&dev->dev));
+	if (!hcd)
+		return -ENOMEM;
+
+	platform_set_drvdata(dev, hcd);
+	dev->dev.platform_data = pdata;
+	priv = hcd_to_ohci_priv(hcd);
+
+	if (pdata == &ohci_platform_defaults && dev->dev.of_node) {
+		priv->phy = devm_phy_get(&dev->dev, "usb");
+		if (IS_ERR(priv->phy)) {
+			err = PTR_ERR(priv->phy);
+			if (err == -EPROBE_DEFER)
+				goto err_put_hcd;
+			priv->phy = NULL;
+		}
+
+		for (clk = 0; clk < OHCI_MAX_CLKS; clk++) {
+			priv->clks[clk] = of_clk_get(dev->dev.of_node, clk);

Quoted 73 lines · Hans de Goede

Add support for ohci-platform instantiation from devicetree, including optionally getting clks and a phy from devicetree, and enabling / disabling those on power_on / off.

This should allow using ohci-platform from devicetree in various cases. Specifically after this commit it can be used for the ohci controller found on Allwinner sunxi SoCs.

Signed-off-by Hans de Goede <[email protected]>

---
  .../devicetree/bindings/usb/mmio-ohci.txt          |  22 +++
  drivers/usb/host/ohci-platform.c                   | 164 ++++++++++++++++++---
  2 files changed, 162 insertions(+), 24 deletions(-)

create mode 100644 Documentation/devicetree/bindings/usb/mmio-ohci.txt

Documentation/devicetree/bindings/usb/mmio-ohci.txt new file +22 -0

new file mode 100644
index 0000000..9c776ed
@@ -0,0 +1,22 @@
+Generic MMIO OHCI controller
+
+Required properties:
+- compatible : "mmio-ohci"
+- reg : ohci controller register range (address and length)
+- interrupts : ohci controller interrupt
+
+Optional properties:
+- clocks : a list of phandle + clock specifier pairs
+- phys : phandle + phy specifier pair
+- phy-names : "usb"
+
+Example:
+
+    ohci0: ohci at 0x01c14400 {
+        compatible = "mmio-ohci";
+        reg = <0x01c14400 0x100>;
+        interrupts = <64>;
+        clocks = <&usb_clk 6>, <&ahb_gates 2>;
+        phys = <&usbphy 1>;
+        phy-names = "usb";
+    };

... <snip> ....

Documentation/devicetree/bindings/usb/mmio-ohci.txt new file +24 -0

@@ -83,17 +156,40 @@ static int ohci_platform_probe(struct platform_device *dev)
          return -ENXIO;
      }
+    hcd = usb_create_hcd(&ohci_platform_hc_driver, &dev->dev,
+            dev_name(&dev->dev));
+    if (!hcd)
+        return -ENOMEM;
+
+    platform_set_drvdata(dev, hcd);
+    dev->dev.platform_data = pdata;
+    priv = hcd_to_ohci_priv(hcd);
+
+    if (pdata == &ohci_platform_defaults && dev->dev.of_node) {
+        priv->phy = devm_phy_get(&dev->dev, "usb");
+        if (IS_ERR(priv->phy)) {
+            err = PTR_ERR(priv->phy);
+            if (err == -EPROBE_DEFER)
+                goto err_put_hcd;
+            priv->phy = NULL;
+        }
+
+        for (clk = 0; clk < OHCI_MAX_CLKS; clk++) {
+            priv->clks[clk] = of_clk_get(dev->dev.of_node, clk);
+            if (IS_ERR(priv->clks[clk])) {
+                err = PTR_ERR(priv->clks[clk]);
+                if (err == -EPROBE_DEFER)
+                    goto err_put_clks;

Documentation/devicetree/bindings/usb/mmio-ohci.txt static int ohci_platform_probe(struct platform_device *dev) · line 187

Quoted 6 lines

Documentation/devicetree/bindings/usb/mmio-ohci.txt new file +5 -0

+                priv->clks[clk] = NULL;
+                break;
+            }
+        }
+    }
 

Given that you have limited the clock parsing to OHCI_MAX_CLKS, there should be some kind of warning/error message if the DT contains more than OHCI_MAX_CLKS - otherwise you have silent failures when the clocks aren't configured.

There is also no note in the binding to indicate this limitation and you shouldn't expect people writing DT files to go digging through the source to check for this.

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set, this version should be 100% ready for merging upstream.

Changes since v3:

(quoted diff) +0 -3

-Address all of Alan Stern's review comments
-Properly use put_clk in probe error path
-Don't do hcd_put until after power_off as power_off may use private hcd data

Regards,

Hans

Hello.

Quoted 2 lines · Hans de Goede

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set, this version should be 100% ready for merging upstream.

I see you've decided to completely ignore my opinion. NAK, FWIW.

Quoted 4 lines · Sergei Shtylyov

Hello.

On 01/11/2014 01:46 AM, Hans de Goede wrote:

Quoted 4 lines

Quoted 2 lines

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set, this version should be 100% ready for merging upstream.

I see you've decided to completely ignore my opinion. NAK, FWIW.

I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING, everyone except you seems to be fine with mmio and and one point in time we need to make a decision and move forward.

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set, this version should be 100% ready for merging upstream.

I see you've decided to completely ignore my opinion. NAK, FWIW.

I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING, everyone except you seems to be fine with mmio and and one point in time we need to make a decision and move forward.

FWIW I agree with your decision - not that I think that matters to anyone :-)

But it would have been nice if you summarized the discussion in the cover letter, including the reason behind your decision. Just so that

Sergei and other reviewers would see that you didn't actually ignore any opinion. You just didn't agree, and you had your reasons to do so.

Bj?rn

Quoted 2 lines

FWIW I agree with your decision - not that I think that matters to anyone :-)

It matters at least to me, potential user of this stuff.

Quoted 4 lines

Quoted 2 lines

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set, this version should be 100% ready for merging upstream.

I see you've decided to completely ignore my opinion. NAK, FWIW.

I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING, everyone except you seems to be fine with mmio

and and one point in time we
need to make a decision and move forward.

Regards,

Hans

Quoted 7 lines · Hans de Goede

Quoted 4 lines

Quoted 2 lines

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set, this version should be 100% ready for merging upstream.

I see you've decided to completely ignore my opinion. NAK, FWIW.

I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING, everyone except you seems to be fine with mmio

Everyone being couple of people reading this thread? Of which only one being familiar with the real [EO]HCI hardware? :-)

The name is just plain stupid if you want to know my real opinion.

And you've posted v3 with this name used first only what, 1.5 days ago?

Quoted 2 lines

and and one point in time we
need to make a decision and move forward.

I see the first version of your patches posted on January 6th, today is 11th (just starting here). You must be in a very big hurry to get this merged, I see. :-)

    Serious changes like this are not really done at such pace. Only the stuff that someone else has to painfully fix up later... :-(

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

Quoted 2 lines

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set, this version should be 100% ready for merging upstream.

I see you've decided to completely ignore my opinion. NAK, FWIW.

I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING,
everyone except you seems to be fine with mmio

Quoted 2 lines

Everyone being couple of people reading this thread? Of which only one
being familiar with the real [EO]HCI hardware? :-)

You are aware that I've worked on qemu's USB emulation for 3 years full-time, including things like making the ehci emulation Windows XP compatible and many other hcd emulation fixes?

And Alan Stern's credentials speak for themselves.

Comments like this do not really help to get your own comments taken serious.

Quoted 1 line

The name is just plain stupid if you want to know my real opinion.

Your opinion has been noted, but so far your entire objection seems to be you not liking the name and now calling it stupid, while there have been actually *technical* arguments against the other prefixes.

Quoted 8 lines

And you've posted v3 with this name used first only what, 1.5 days ago?

Quoted 2 lines

and and one point in time we
need to make a decision and move forward.

I see the first version of your patches posted on January 6th, today is
11th (just starting here). You must be in a very big hurry to get this merged, I see. :-)

This is not about being in a hurry, this is about not wanting to waste time on what is quickly turning into pure bike-shedding.

Quoted 2 lines

Serious changes like this are not really done at such pace. Only the
stuff that someone else has to painfully fix up later... :-(

If you're suggestion that the mmio- prefix will cause breakage later now is your time to actually make an actual technical argument. "just plain stupid" does not help to get your comments taken serious.

Regards,

Hans

Hi,

Quoted 4 lines · Sergei Shtylyov

Hello.

On 01/11/2014 01:46 AM, Hans de Goede wrote:

Quoted 4 lines

Quoted 2 lines

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set, this version should be 100% ready for merging upstream.

I see you've decided to completely ignore my opinion. NAK, FWIW.

I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING, everyone except you seems to be fine with mmio and and one point in time we need to make a decision and move forward.

Quoted 10 lines · Hans de Goede

Hi,

Quoted 7 lines · Sergei Shtylyov

Hello.

Quoted 2 lines · Hans de Goede

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set, this version should be 100% ready for merging upstream.

I see you've decided to completely ignore my opinion. NAK, FWIW.

Quoted 3 lines

I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING, everyone except you seems to be fine with mmio and and one point in time we need to make a decision and move forward.

If this isn't beating a dead horse... Maybe everyone can agree on a name like ohci-generic or generic-ohci. That seems like a pretty good description of the hardware that the platform driver can handle.

Alan Stern

Quoted 18 lines · Alan Stern

Quoted 16 lines · Hans de Goede

Hi,

Quoted 8 lines · Sergei Shtylyov

Hello.

Quoted 3 lines · Hans de Goede

Here is v4 of my ohci and ehci-platform clks, phy and dt support patch-set,
this version should be 100% ready for merging upstream.

I see you've decided to completely ignore my opinion. NAK, FWIW.

I'm sorry but the whole prefix thing has become a thing of -ETOOMUCHBIKESHEDDING,
everyone except you seems to be fine with mmio and and one point in time we
need to make a decision and move forward.

Quoted 5 lines

If this isn't beating a dead horse... Maybe everyone can agree on a name like ohci-generic or generic-ohci. That seems like a pretty good description of the hardware that the platform driver can handle.

Alan Stern

I prefer the -generic option, although generic- is equally fine - Having said that, I don't really care if it's called mmio either (although this does seem less 'descriptive').

Hi,

[Cc'ing DT maintainers directly]

Quoted 4 lines

Quoted 1 line

Alan Stern Wrote:

I prefer the -generic option, although generic- is equally fine - Having said that, I don't really care if it's called mmio either (although this does seem less 'descriptive').

Grepping over existing dts files, I can find several occurrences of "usb-ehci" compatible string:

at91sam9g45.dtsi:                       compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
at91sam9x5.dtsi:                        compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
omap3.dtsi:                             compatible = "ti,ehci-omap", "usb-ehci";
omap4.dtsi:                             compatible = "ti,ehci-omap", "usb-ehci";
omap5.dtsi:                             compatible = "ti,ehci-omap", "usb-ehci";
sama5d3.dtsi:                   compatible = "atmel,at91sam9g45-ehci", "usb-ehci";
spear13xx.dtsi:                 compatible = "st,spear600-ehci", "usb-ehci";
spear13xx.dtsi:                 compatible = "st,spear600-ehci", "usb-ehci";
spear3xx.dtsi:                  compatible = "st,spear600-ehci", "usb-ehci";
spear600.dtsi:                  compatible = "st,spear600-ehci", "usb-ehci";
spear600.dtsi:                  compatible = "st,spear600-ehci", "usb-ehci";
tegra114.dtsi:          compatible = "nvidia,tegra30-ehci", "usb-ehci";
tegra114.dtsi:          compatible = "nvidia,tegra30-ehci", "usb-ehci";
tegra20.dtsi:           compatible = "nvidia,tegra20-ehci", "usb-ehci";
tegra20.dtsi:           compatible = "nvidia,tegra20-ehci", "usb-ehci";
tegra20.dtsi:           compatible = "nvidia,tegra20-ehci", "usb-ehci";
tegra30.dtsi:           compatible = "nvidia,tegra30-ehci", "usb-ehci";
tegra30.dtsi:           compatible = "nvidia,tegra30-ehci", "usb-ehci";
tegra30.dtsi:           compatible = "nvidia,tegra30-ehci", "usb-ehci";

Same for "usb-ohci":

arch/arm/boot/dts/at91rm9200.dtsi:                      compatible = "atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9260.dtsi:                     compatible = "atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9263.dtsi:                     compatible = "atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9g45.dtsi:                     compatible = "atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9n12.dtsi:                     compatible = "atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/at91sam9x5.dtsi:                      compatible = "atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/lpc32xx.dtsi:                 compatible = "nxp,ohci-nxp", "usb-ohci";
arch/arm/boot/dts/omap3.dtsi:                           compatible = "ti,ohci-omap3", "usb-ohci";
arch/arm/boot/dts/omap4.dtsi:                           compatible = "ti,ohci-omap3", "usb-ohci";
arch/arm/boot/dts/omap5.dtsi:                           compatible = "ti,ohci-omap3", "usb-ohci";
arch/arm/boot/dts/sama5d3.dtsi:                 compatible = "atmel,at91rm9200-ohci", "usb-ohci";
arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear13xx.dtsi:                       compatible = "st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear3xx.dtsi:                        compatible = "st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear3xx.dtsi:                        compatible = "st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ohci", "usb-ohci";
arch/arm/boot/dts/spear600.dtsi:                        compatible = "st,spear600-ohci", "usb-ohci";

For "usb-ehci" there is even a documentation file [1], while "usb-ohci" seems to be undocumented.

[1] Documentation/devicetree/bindings/usb/usb-ehci.txt

Aren't they both something that should be accounted for in this series?

I agree that usb-Xhci would be the best compatible strings to use.

The problem with usb-ehci is that there already is a ppc specific driver binding to that compatible string, doing various ppc specific controller initialization.

Thinking more about this, there is one possible solution though, the ehci-ppc-of.c is guarded in Kconfig with:

depends on PPC_OF

If we add an inverted check to the Kconfig option for platform-ehci.c, ie:

config USB_EHCI_HCD_PLATFORM
tristate "Generic EHCI driver for a platform device"
depends on !PPC_OF

Then we can be certain that we don't end up with 2 drivers claiming the usb-ehci compatible on ppc platforms. I've done some quick research and it seems that ehci-platform.c is only used on arm and mips devices, so excluding its use on ppc should not be an issue.

Then later on someone, who has the actual hardware to test, can merge the ppc specific quirk handling into ehci-platform,c and ehci-ppc-of.c can go away entirely.

Alan, if you agree this is the best way forward, I'll do a v5 with the proposed changes.