[RFC] USB: EHCI: hot-fix OMAP and Orion multiplatform config

Message ID 201303151700.27231.arnd@arndb.de
State New
Headers show

Commit Message

Arnd Bergmann March 15, 2013, 5 p.m.
On Friday 15 March 2013, Alan Stern wrote:
> Roger just submitted my patch to split out ehci-omap.

Ok, if we can merge that for 3.9, the main issue will be resolved, because
an OMAP+Orion config is much more interesting in practice than adding
VT8500 to the mix.

An alternative for VT8500 would be to remove drivers/usb/host/ehci-vt8500.c
completely and use the generic ehci-platform.c bus glue with added DT
support, as the patch below that I just hacked up. Not sure if that
anyone would prefer that over the hot-fix for 3.9, but it's possibly
a better solution in the long run.

> What happened to Manjunath Goudar?  I haven't heard from him since his 
> first round of submissions a month ago.  Fixing them up shouldn't be 
> too much work.

He's still around and working on his patches for 3.10. I'm currently working
with him to resolve a few of the remaining problems in his series and he
will post the lot soon.

> As far as I can tell, your scheme should be okay for now.  It would be 
> good to hear from other people, though.
> 
> I'd guess that the only reason it wasn't done this way from the
> beginning is that nobody considered the possibility of building a
> multi-platform driver.

Right. It's been a long way until it became possible on ARM. The four
PowerPC bus glues have already been mutually exclusive like this all
the time.

	Arnd


---

Comments

Alan Stern March 15, 2013, 5:40 p.m. | #1
On Fri, 15 Mar 2013, Arnd Bergmann wrote:

> On Friday 15 March 2013, Alan Stern wrote:
> > Roger just submitted my patch to split out ehci-omap.
> 
> Ok, if we can merge that for 3.9, the main issue will be resolved, because
> an OMAP+Orion config is much more interesting in practice than adding
> VT8500 to the mix.

I don't know if the patch will get into 3.9.  That's up to Greg; IIRC 
Roger did not ask for it.

> An alternative for VT8500 would be to remove drivers/usb/host/ehci-vt8500.c
> completely and use the generic ehci-platform.c bus glue with added DT
> support, as the patch below that I just hacked up. Not sure if that
> anyone would prefer that over the hot-fix for 3.9, but it's possibly
> a better solution in the long run.

Getting rid of driver files is always worthwhile.

> @@ -89,12 +90,17 @@ static int ehci_platform_probe(struct platform_device *dev)
>  		return -ENXIO;
>  	}
>  
> -	if (pdata->power_on) {
> +	if (pdata && pdata->power_on) {
>  		err = pdata->power_on(dev);
>  		if (err < 0)
>  			return err;
>  	}

Instead of adding these tests for non-NULL pdata all over the place, 
you could define a static structure with default settings and store a 
pointer to it if pdata wasn't set initially.

Alan Stern
Alan Stern March 15, 2013, 7:19 p.m. | #2
On Fri, 15 Mar 2013, Arnd Bergmann wrote:

> On Friday 15 March 2013, Alan Stern wrote:
> > Roger just submitted my patch to split out ehci-omap.
> 
> Ok, if we can merge that for 3.9, the main issue will be resolved, because
> an OMAP+Orion config is much more interesting in practice than adding
> VT8500 to the mix.

I just got an automated message from Greg:

--------------------------------------------------------------------------
This is a note to let you know that I've just added the patch titled

    USB: EHCI: split ehci-omap out to a separate driver

to my usb git tree which can be found at
    git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
in the usb-next branch.
--------------------------------------------------------------------------

Unless something is changed, this patch won't get into 3.9-final.  
Do you want Greg to move it to his usb-linus branch?

Alan Stern
Greg Kroah-Hartman March 15, 2013, 7:25 p.m. | #3
On Fri, Mar 15, 2013 at 03:19:08PM -0400, Alan Stern wrote:
> On Fri, 15 Mar 2013, Arnd Bergmann wrote:
> 
> > On Friday 15 March 2013, Alan Stern wrote:
> > > Roger just submitted my patch to split out ehci-omap.
> > 
> > Ok, if we can merge that for 3.9, the main issue will be resolved, because
> > an OMAP+Orion config is much more interesting in practice than adding
> > VT8500 to the mix.
> 
> I just got an automated message from Greg:
> 
> --------------------------------------------------------------------------
> This is a note to let you know that I've just added the patch titled
> 
>     USB: EHCI: split ehci-omap out to a separate driver
> 
> to my usb git tree which can be found at
>     git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> in the usb-next branch.
> --------------------------------------------------------------------------
> 
> Unless something is changed, this patch won't get into 3.9-final.  
> Do you want Greg to move it to his usb-linus branch?

It's a bit too "big" for 3.9-final, sorry.

greg k-h
Arnd Bergmann March 15, 2013, 8:15 p.m. | #4
On Friday 15 March 2013, Alan Stern wrote:
> I just got an automated message from Greg:
> 
> --------------------------------------------------------------------------
> This is a note to let you know that I've just added the patch titled
> 
>     USB: EHCI: split ehci-omap out to a separate driver
> 
> to my usb git tree which can be found at
>     git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
> in the usb-next branch.
> --------------------------------------------------------------------------
> 
> Unless something is changed, this patch won't get into 3.9-final.  
> Do you want Greg to move it to his usb-linus branch?

I don't care much which way we do it, but I'd certainly like to
see one of these two solutions for the bug:

a) Apply Roger's patch for 3.9, and add my "USB: EHCI: DT support
   for generic bus glue" patch on top after it has been tested
b) Apply my "USB: EHCI: hot-fix OMAP and Orion multiplatform config"
   when I send a version based on the proper tree for 3.9, and leave
   the other ones to do it properly for 3.10.

	Arnd
Arnd Bergmann March 15, 2013, 9:11 p.m. | #5
On Friday 15 March 2013, Sergei Shtylyov wrote:
> >
> > a) Apply Roger's patch for 3.9, and add my "USB: EHCI: DT support
> >     for generic bus glue" patch on top after it has been tested
> 
>     Arnd, sorry, where can I find this patch? Archive search doesn't 
> turn up anything...

It's in this email thread, in one of my earlier replies to Alan. Since
Greg already said that he won't take Roger's patch for 3.9, taking
this one would be pointless as well. I can resubmit it for 3.10
once we know what to do about my hot-fix.

	Arnd
Arnd Bergmann March 15, 2013, 9:13 p.m. | #6
On Friday 15 March 2013, Greg Kroah-Hartman wrote:
> > Unless something is changed, this patch won't get into 3.9-final.  
> > Do you want Greg to move it to his usb-linus branch?
> 
> It's a bit too "big" for 3.9-final, sorry.

Would you consider the hot fix at the start of this thread for 3.9
then? I can resubmit it as a patch for inclusion with the missing
hunk added in if there are no objections.

	Arnd
Sergei Shtylyov March 15, 2013, 10:08 p.m. | #7
Hello.

On 03/15/2013 11:15 PM, Arnd Bergmann wrote:

> On Friday 15 March 2013, Alan Stern wrote:
>> I just got an automated message from Greg:
>>
>> --------------------------------------------------------------------------
>> This is a note to let you know that I've just added the patch titled
>>
>>      USB: EHCI: split ehci-omap out to a separate driver
>>
>> to my usb git tree which can be found at
>>      git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git
>> in the usb-next branch.
>> --------------------------------------------------------------------------
>>
>> Unless something is changed, this patch won't get into 3.9-final.
>> Do you want Greg to move it to his usb-linus branch?
> I don't care much which way we do it, but I'd certainly like to
> see one of these two solutions for the bug:
>
> a) Apply Roger's patch for 3.9, and add my "USB: EHCI: DT support
>     for generic bus glue" patch on top after it has been tested

    Arnd, sorry, where can I find this patch? Archive search doesn't 
turn up anything...

WBR, Sergei
Sergei Shtylyov March 15, 2013, 10:24 p.m. | #8
Hello.

On 03/16/2013 12:11 AM, Arnd Bergmann wrote:

> On Friday 15 March 2013, Sergei Shtylyov wrote:
>>> a) Apply Roger's patch for 3.9, and add my "USB: EHCI: DT support
>>>      for generic bus glue" patch on top after it has been tested
>>      Arnd, sorry, where can I find this patch? Archive search doesn't
>> turn up anything...
> It's in this email thread, in one of my earlier replies to Alan.

    Ah, found it now. I thought however that it already handles the DT 
properties for the EHCI platform data.
Of course, that can be added later...

WBR, Sergei
Greg Kroah-Hartman March 16, 2013, 12:09 a.m. | #9
On Fri, Mar 15, 2013 at 09:13:52PM +0000, Arnd Bergmann wrote:
> On Friday 15 March 2013, Greg Kroah-Hartman wrote:
> > > Unless something is changed, this patch won't get into 3.9-final.  
> > > Do you want Greg to move it to his usb-linus branch?
> > 
> > It's a bit too "big" for 3.9-final, sorry.
> 
> Would you consider the hot fix at the start of this thread for 3.9
> then? I can resubmit it as a patch for inclusion with the missing
> hunk added in if there are no objections.

Feel free to resend, but remember, I am _very_ leery of pushing this
type of patch in at the moment, given my past history with it...

greg k-h
Arnd Bergmann March 18, 2013, 3:45 p.m. | #10
On Saturday 16 March 2013, Greg Kroah-Hartman wrote:
> On Fri, Mar 15, 2013 at 09:13:52PM +0000, Arnd Bergmann wrote:
> > On Friday 15 March 2013, Greg Kroah-Hartman wrote:
> > > > Unless something is changed, this patch won't get into 3.9-final.  
> > > > Do you want Greg to move it to his usb-linus branch?
> > > 
> > > It's a bit too "big" for 3.9-final, sorry.
> > 
> > Would you consider the hot fix at the start of this thread for 3.9
> > then? I can resubmit it as a patch for inclusion with the missing
> > hunk added in if there are no objections.
> 
> Feel free to resend, but remember, I am very leery of pushing this
> type of patch in at the moment, given my past history with it...

Sure, it's your decision. I just don't want to be accused of abandoning the
issue that I caused. It's not strictly a regression because no configuration
that was working in 3.8 is broken in 3.9, and embedded systems won't normally
run a multiplatform kernel anyway. Also, the patch is really an ugly hack,
which is probably enough reason not to take it. ;-)

The only situation I can think of that is broken without the hotfix is
distribution kernels that were already running multiplatform on Armada XP
(mvebu/orion) and now want to add OMAP support without breaking the
platforms that are already working. Peter Robinson is doing this on for
Fedora right now[1] and I assume the Debian/Ubuntu/OpenSUSE/Gentoo
people will be in a similar situation, but they can all apply the patch
manually if it doesn't make it into 3.9. It won't be the only patch they
need to get there, it's just the only one for 3.9 we already know about.

	Arnd

[1] https://plus.google.com/117898952501074015902/posts/VgnDhJBxbU4
Arnaud Patard (Rtp) March 18, 2013, 4:53 p.m. | #11
Arnd Bergmann <arnd@arndb.de> writes:

Hi,

> On Saturday 16 March 2013, Greg Kroah-Hartman wrote:
>> On Fri, Mar 15, 2013 at 09:13:52PM +0000, Arnd Bergmann wrote:
>> > On Friday 15 March 2013, Greg Kroah-Hartman wrote:
>> > > > Unless something is changed, this patch won't get into 3.9-final.  
>> > > > Do you want Greg to move it to his usb-linus branch?
>> > > 
>> > > It's a bit too "big" for 3.9-final, sorry.
>> > 
>> > Would you consider the hot fix at the start of this thread for 3.9
>> > then? I can resubmit it as a patch for inclusion with the missing
>> > hunk added in if there are no objections.
>> 
>> Feel free to resend, but remember, I am very leery of pushing this
>> type of patch in at the moment, given my past history with it...
>
> Sure, it's your decision. I just don't want to be accused of abandoning the
> issue that I caused. It's not strictly a regression because no configuration
> that was working in 3.8 is broken in 3.9, and embedded systems won't normally
> run a multiplatform kernel anyway. Also, the patch is really an ugly hack,
> which is probably enough reason not to take it. ;-)

Given that the omap patch has been accepted and that the patch is an
"ugly hack", how hard it would be to provide a working patch for mvebu
(well, orion usb driver) ?

>
> The only situation I can think of that is broken without the hotfix is
> distribution kernels that were already running multiplatform on Armada XP
> (mvebu/orion) and now want to add OMAP support without breaking the

well, for omap, this patch may be needed too
http://marc.info/?l=linux-usb&m=136248269928419&w=2 imho (I've not
checked if it's merged now).

> platforms that are already working. Peter Robinson is doing this on for
> Fedora right now[1] and I assume the Debian/Ubuntu/OpenSUSE/Gentoo
> people will be in a similar situation, but they can all apply the
> patch

fwiw, I've mentionned this usb issue few days ago when talking about
multiplatform on the debian-arm ml.

> manually if it doesn't make it into 3.9. It won't be the only patch they

it can applied in the debian kernel, as long as the patch is merged
upstream.

Arnaud
Arnd Bergmann March 18, 2013, 5:32 p.m. | #12
On Monday 18 March 2013, Arnaud Patard wrote:
> Arnd Bergmann <arnd@arndb.de> writes:

> > Sure, it's your decision. I just don't want to be accused of abandoning the
> > issue that I caused. It's not strictly a regression because no configuration
> > that was working in 3.8 is broken in 3.9, and embedded systems won't normally
> > run a multiplatform kernel anyway. Also, the patch is really an ugly hack,
> > which is probably enough reason not to take it. ;-)
> 
> Given that the omap patch has been accepted and that the patch is an
> "ugly hack", how hard it would be to provide a working patch for mvebu
> (well, orion usb driver) ?

The OMAP patch was accepted for 3.10, and I'm sure we will have good
patches for the other bus glues as well, but the bug is also present on
3.9, and the proper patches are too invasive now to get merged there.

> > platforms that are already working. Peter Robinson is doing this on for
> > Fedora right now[1] and I assume the Debian/Ubuntu/OpenSUSE/Gentoo
> > people will be in a similar situation, but they can all apply the
> > patch
> 
> fwiw, I've mentionned this usb issue few days ago when talking about
> multiplatform on the debian-arm ml.

Yes, we've known about it for a long time, but failed to get the proper
patches out in time :(

> > manually if it doesn't make it into 3.9. It won't be the only patch they
> 
> it can applied in the debian kernel, as long as the patch is merged
> upstream.

It won't be merged in 3.10, but in that case Debian can backport the proper
patches from 3.10.

	Arnd

Patch

diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index ca75063..2e4ddd4 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -18,11 +18,13 @@ 
  *
  * Licensed under the GNU/GPL. See COPYING for details.
  */
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/kernel.h>
 #include <linux/hrtimer.h>
 #include <linux/io.h>
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/usb.h>
 #include <linux/usb/hcd.h>
@@ -41,17 +43,21 @@  static int ehci_platform_reset(struct usb_hcd *hcd)
 	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
 	int retval;
 
-	hcd->has_tt = pdata->has_tt;
-	ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug;
-	ehci->big_endian_desc = pdata->big_endian_desc;
-	ehci->big_endian_mmio = pdata->big_endian_mmio;
+	ehci->caps = hcd->regs;
+
+	if (pdata) {
+		hcd->has_tt = pdata->has_tt;
+		ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug;
+		ehci->big_endian_desc = pdata->big_endian_desc;
+		ehci->big_endian_mmio = pdata->big_endian_mmio;
+	 	ehci->caps += pdata->caps_offset;
+	}
 
-	ehci->caps = hcd->regs + pdata->caps_offset;
 	retval = ehci_setup(hcd);
 	if (retval)
 		return retval;
 
-	if (pdata->no_io_watchdog)
+	if (pdata && pdata->no_io_watchdog)
 		ehci->need_io_watchdog = 0;
 	return 0;
 }
@@ -70,11 +76,6 @@  static int ehci_platform_probe(struct platform_device *dev)
 	int irq;
 	int err = -ENOMEM;
 
-	if (!pdata) {
-		WARN_ON(1);
-		return -ENODEV;
-	}
-
 	if (usb_disabled())
 		return -ENODEV;
 
@@ -89,12 +90,17 @@  static int ehci_platform_probe(struct platform_device *dev)
 		return -ENXIO;
 	}
 
-	if (pdata->power_on) {
+	if (pdata && pdata->power_on) {
 		err = pdata->power_on(dev);
 		if (err < 0)
 			return err;
 	}
 
+	if (!dev->dev.dma_mask) {
+		dev->dev.dma_mask = &dev->dev.coherent_dma_mask;
+		dev->dev.coherent_dma_mask = DMA_BIT_MASK(32);
+	}
+
 	hcd = usb_create_hcd(&ehci_platform_hc_driver, &dev->dev,
 			     dev_name(&dev->dev));
 	if (!hcd) {
@@ -121,7 +127,7 @@  static int ehci_platform_probe(struct platform_device *dev)
 err_put_hcd:
 	usb_put_hcd(hcd);
 err_power:
-	if (pdata->power_off)
+	if (pdata && pdata->power_off)
 		pdata->power_off(dev);
 
 	return err;
@@ -136,7 +142,7 @@  static int ehci_platform_remove(struct platform_device *dev)
 	usb_put_hcd(hcd);
 	platform_set_drvdata(dev, NULL);
 
-	if (pdata->power_off)
+	if (pdata && pdata->power_off)
 		pdata->power_off(dev);
 
 	return 0;
@@ -155,7 +161,7 @@  static int ehci_platform_suspend(struct device *dev)
 
 	ret = ehci_suspend(hcd, do_wakeup);
 
-	if (pdata->power_suspend)
+	if (pdata && pdata->power_suspend)
 		pdata->power_suspend(pdev);
 
 	return ret;
@@ -168,7 +174,7 @@  static int ehci_platform_resume(struct device *dev)
 	struct platform_device *pdev =
 		container_of(dev, struct platform_device, dev);
 
-	if (pdata->power_on) {
+	if (pdata && pdata->power_on) {
 		int err = pdata->power_on(pdev);
 		if (err < 0)
 			return err;
@@ -183,6 +189,12 @@  static int ehci_platform_resume(struct device *dev)
 #define ehci_platform_resume	NULL
 #endif /* CONFIG_PM */
 
+static const struct of_device_id vt8500_ehci_ids[] = {
+	{ .compatible = "via,vt8500-ehci", },
+	{ .compatible = "wm,prizm-ehci", },
+	{}
+};
+
 static const struct platform_device_id ehci_platform_table[] = {
 	{ "ehci-platform", 0 },
 	{ }
@@ -203,6 +215,7 @@  static struct platform_driver ehci_platform_driver = {
 		.owner	= THIS_MODULE,
 		.name	= "ehci-platform",
 		.pm	= &ehci_platform_pm_ops,
+		.of_match_table = of_match_ptr(vt8500_ehci_ids),
 	}
 };