[2/2] USB: EHCI: make ehci-orion a separate driver

Message ID 1360966349-1242560-2-git-send-email-arnd@arndb.de
State Accepted
Commit 6ed3c43d05f6d0d55f17947bc287f35318fd96f8
Headers show

Commit Message

Arnd Bergmann Feb. 15, 2013, 10:12 p.m.
From: Manjunath Goudar <manjunath.goudar@linaro.org>

With the multiplatform changes in arm-soc tree, it becomes
possible to enable the mvebu platform (which uses
ehci-orion) at the same time as other platforms that require
a conflicting EHCI bus glue. At the moment, this results
in a warning like

drivers/usb/host/ehci-hcd.c:1297:0: warning: "PLATFORM_DRIVER" redefined [enabled by default]
drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the previous definition
drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver' defined but not used [-Wunused-variable]

and an ehci driver that only works on one of them.

With the infrastructure added by Alan Stern in patch 3e0232039
"USB: EHCI: prepare to make ehci-hcd a library module", we can
avoid this problem by turning a bus glue into a separate
module, as we do here for the orion bus glue.

Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
---
 drivers/usb/host/Kconfig      |  8 ++++
 drivers/usb/host/Makefile     |  1 +
 drivers/usb/host/ehci-hcd.c   |  6 +--
 drivers/usb/host/ehci-orion.c | 90 ++++++++++++++++++++-----------------------
 4 files changed, 52 insertions(+), 53 deletions(-)

Comments

Jason Cooper Feb. 15, 2013, 10:38 p.m. | #1
On Fri, Feb 15, 2013 at 11:12:29PM +0100, Arnd Bergmann wrote:
> From: Manjunath Goudar <manjunath.goudar@linaro.org>
> 
> With the multiplatform changes in arm-soc tree, it becomes
> possible to enable the mvebu platform (which uses
> ehci-orion) at the same time as other platforms that require
> a conflicting EHCI bus glue. At the moment, this results
> in a warning like
> 
> drivers/usb/host/ehci-hcd.c:1297:0: warning: "PLATFORM_DRIVER" redefined [enabled by default]
> drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the previous definition
> drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver' defined but not used [-Wunused-variable]
> 
> and an ehci driver that only works on one of them.
> 
> With the infrastructure added by Alan Stern in patch 3e0232039
> "USB: EHCI: prepare to make ehci-hcd a library module", we can
> avoid this problem by turning a bus glue into a separate
> module, as we do here for the orion bus glue.
> 
> Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/usb/host/Kconfig      |  8 ++++
>  drivers/usb/host/Makefile     |  1 +
>  drivers/usb/host/ehci-hcd.c   |  6 +--
>  drivers/usb/host/ehci-orion.c | 90 ++++++++++++++++++++-----------------------
>  4 files changed, 52 insertions(+), 53 deletions(-)

This looks sane to me, but I unfortunately don't have time to test atm.
So if Arnd has been working with you:

Acked-by: Jason Cooper <jason@lakedaemon.net>

thx,

Jason.
Andrew Lunn Feb. 16, 2013, 8:42 a.m. | #2
On Fri, Feb 15, 2013 at 11:12:29PM +0100, Arnd Bergmann wrote:
> From: Manjunath Goudar <manjunath.goudar@linaro.org>
> 
> With the multiplatform changes in arm-soc tree, it becomes
> possible to enable the mvebu platform (which uses
> ehci-orion) at the same time as other platforms that require
> a conflicting EHCI bus glue. At the moment, this results
> in a warning like
> 
> drivers/usb/host/ehci-hcd.c:1297:0: warning: "PLATFORM_DRIVER" redefined [enabled by default]
> drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the previous definition
> drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver' defined but not used [-Wunused-variable]
> 
> and an ehci driver that only works on one of them.
> 
> With the infrastructure added by Alan Stern in patch 3e0232039
> "USB: EHCI: prepare to make ehci-hcd a library module", we can
> avoid this problem by turning a bus glue into a separate
> module, as we do here for the orion bus glue.
> 
> Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/usb/host/Kconfig      |  8 ++++
>  drivers/usb/host/Makefile     |  1 +
>  drivers/usb/host/ehci-hcd.c   |  6 +--
>  drivers/usb/host/ehci-orion.c | 90 ++++++++++++++++++++-----------------------
>  4 files changed, 52 insertions(+), 53 deletions(-)

Tested-by: Andrew Lunn <andrew@lunn.ch>

I booted with this patch on a Kirkwood based QNAP NAS box. The USB bus
enumerated as expected.

	   Andrew
Ezequiel Garcia Feb. 16, 2013, 2:19 p.m. | #3
Hi Arnd,

On Fri, Feb 15, 2013 at 11:12:29PM +0100, Arnd Bergmann wrote:
> From: Manjunath Goudar <manjunath.goudar@linaro.org>
> 
> With the multiplatform changes in arm-soc tree, it becomes
> possible to enable the mvebu platform (which uses
> ehci-orion) at the same time as other platforms that require
> a conflicting EHCI bus glue. At the moment, this results
> in a warning like
> 
> drivers/usb/host/ehci-hcd.c:1297:0: warning: "PLATFORM_DRIVER" redefined [enabled by default]
> drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the previous definition
> drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver' defined but not used [-Wunused-variable]
> 
> and an ehci driver that only works on one of them.
> 
> With the infrastructure added by Alan Stern in patch 3e0232039
> "USB: EHCI: prepare to make ehci-hcd a library module", we can
> avoid this problem by turning a bus glue into a separate
> module, as we do here for the orion bus glue.
> 
> Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Cc: Jason Cooper <jason@lakedaemon.net>
> Cc: Andrew Lunn <andrew@lunn.ch>
> ---
>  drivers/usb/host/Kconfig      |  8 ++++
>  drivers/usb/host/Makefile     |  1 +
>  drivers/usb/host/ehci-hcd.c   |  6 +--
>  drivers/usb/host/ehci-orion.c | 90 ++++++++++++++++++++-----------------------
>  4 files changed, 52 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index d77e028..7ac6f48 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -162,6 +162,14 @@ config USB_EHCI_HCD_OMAP
>  	  Enables support for the on-chip EHCI controller on
>  	  OMAP3 and later chips.
>  
> +config USB_EHCI_HCD_ORION
> +        tristate  "Support for Marvell Orion on-chip EHCI USB controller"
> +        depends on USB_EHCI_HCD && PLAT_ORION
> +        default y
> +        ---help---
> +          Enables support for the on-chip EHCI controller on
> +          Morvell Orion chips.

s/Morvell/Marvell

Just this tiny nitpick.

Thanks,
Alan Stern Feb. 18, 2013, 10:34 p.m. | #4
On Fri, 15 Feb 2013, Arnd Bergmann wrote:

> From: Manjunath Goudar <manjunath.goudar@linaro.org>
> 
> With the multiplatform changes in arm-soc tree, it becomes
> possible to enable the mvebu platform (which uses
> ehci-orion) at the same time as other platforms that require
> a conflicting EHCI bus glue. At the moment, this results
> in a warning like
> 
> drivers/usb/host/ehci-hcd.c:1297:0: warning: "PLATFORM_DRIVER" redefined [enabled by default]
> drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the previous definition
> drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver' defined but not used [-Wunused-variable]
> 
> and an ehci driver that only works on one of them.
> 
> With the infrastructure added by Alan Stern in patch 3e0232039
> "USB: EHCI: prepare to make ehci-hcd a library module", we can
> avoid this problem by turning a bus glue into a separate
> module, as we do here for the orion bus glue.

> --- a/drivers/usb/host/Makefile
> +++ b/drivers/usb/host/Makefile
> @@ -30,6 +30,7 @@ obj-$(CONFIG_USB_EHCI_MXC)	+= ehci-mxc.o
>  
>  obj-$(CONFIG_USB_OXU210HP_HCD)	+= oxu210hp-hcd.o
>  obj-$(CONFIG_USB_EHCI_HCD_VT8500)+= ehci-vt8500.o
> +obj-$(CONFIG_USB_EHCI_HCD_ORION)+= ehci-orion.o

Both of these two new lines should be formatted like the other lines in
this file (i.e., with tabs at the corresponding places), and they
should come before the OXU210HP_HCD entry so that they are next to the
other EHCI-related lines.

> --- a/drivers/usb/host/ehci-orion.c
> +++ b/drivers/usb/host/ehci-orion.c
> @@ -17,6 +17,13 @@
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/of_irq.h>
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +#include <linux/io.h>
> +#include <linux/dma-mapping.h>

Is this line really needed?

> @@ -34,6 +41,17 @@
>  #define USB_PHY_IVREF_CTRL	0x440
>  #define USB_PHY_TST_GRP_CTRL	0x450
>  
> +#define DRIVER_DESC "EHCI orion driver"
> +
> +static const char hcd_name[] = "ehci-orion";
> +
> +static struct hc_driver __read_mostly ehci_orion_hc_driver;
> +
> +static const struct ehci_driver_overrides orion_overrides __initdata = {
> +	.reset = ehci_setup,
> +};

This is not necessary; ehci_setup is the default value anyway.  This 
structure can be omitted.

> @@ -323,8 +296,6 @@ static int __exit ehci_orion_drv_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> -MODULE_ALIAS("platform:orion-ehci");
> -
>  static const struct of_device_id ehci_orion_dt_ids[] = {
>  	{ .compatible = "marvell,orion-ehci", },
>  	{},
> @@ -336,8 +307,31 @@ static struct platform_driver ehci_orion_driver = {
>  	.remove		= __exit_p(ehci_orion_drv_remove),
>  	.shutdown	= usb_hcd_platform_shutdown,
>  	.driver = {
> -		.name	= "orion-ehci",
> +		.name	= hcd_name,

Is this really what you want -- changing the driver name from 
"orion-ehci" to "ehci-orion"?  Is that liable to cause trouble?

> +MODULE_DESCRIPTION(DRIVER_DESC);
> +MODULE_ALIAS("platform:ehci-orion");

And is this really what you want -- changing the alias from 
"platform:orion-ehci" to "platform:ehci-orion"?

Alan Stern
manjunath.goudar@linaro.org Feb. 19, 2013, 5:07 a.m. | #5
On 19 February 2013 04:04, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Fri, 15 Feb 2013, Arnd Bergmann wrote:
>
> > From: Manjunath Goudar <manjunath.goudar@linaro.org>
> >
> > With the multiplatform changes in arm-soc tree, it becomes
> > possible to enable the mvebu platform (which uses
> > ehci-orion) at the same time as other platforms that require
> > a conflicting EHCI bus glue. At the moment, this results
> > in a warning like
> >
> > drivers/usb/host/ehci-hcd.c:1297:0: warning: "PLATFORM_DRIVER" redefined
> [enabled by default]
> > drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the
> previous definition
> > drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver'
> defined but not used [-Wunused-variable]
> >
> > and an ehci driver that only works on one of them.
> >
> > With the infrastructure added by Alan Stern in patch 3e0232039
> > "USB: EHCI: prepare to make ehci-hcd a library module", we can
> > avoid this problem by turning a bus glue into a separate
> > module, as we do here for the orion bus glue.
>
> > --- a/drivers/usb/host/Makefile
> > +++ b/drivers/usb/host/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_USB_EHCI_MXC)  += ehci-mxc.o
> >
> >  obj-$(CONFIG_USB_OXU210HP_HCD)       += oxu210hp-hcd.o
> >  obj-$(CONFIG_USB_EHCI_HCD_VT8500)+= ehci-vt8500.o
> > +obj-$(CONFIG_USB_EHCI_HCD_ORION)+= ehci-orion.o
>
> Both of these two new lines should be formatted like the other lines in
> this file (i.e., with tabs at the corresponding places), and they
> should come before the OXU210HP_HCD entry so that they are next to the
> other EHCI-related lines.
>
> > --- a/drivers/usb/host/ehci-orion.c
> > +++ b/drivers/usb/host/ehci-orion.c
> > @@ -17,6 +17,13 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_irq.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/hcd.h>
> > +#include <linux/io.h>
> > +#include <linux/dma-mapping.h>
>
> Is this line really needed?
>
> > @@ -34,6 +41,17 @@
> >  #define USB_PHY_IVREF_CTRL   0x440
> >  #define USB_PHY_TST_GRP_CTRL 0x450
> >
> > +#define DRIVER_DESC "EHCI orion driver"
> > +
> > +static const char hcd_name[] = "ehci-orion";
> > +
> > +static struct hc_driver __read_mostly ehci_orion_hc_driver;
> > +
> > +static const struct ehci_driver_overrides orion_overrides __initdata = {
> > +     .reset = ehci_setup,
> > +};
>
> This is not necessary; ehci_setup is the default value anyway.  This
> structure can be omitted.
>
> > @@ -323,8 +296,6 @@ static int __exit ehci_orion_drv_remove(struct
> platform_device *pdev)
> >       return 0;
> >  }
> >
> > -MODULE_ALIAS("platform:orion-ehci");
> > -
> >  static const struct of_device_id ehci_orion_dt_ids[] = {
> >       { .compatible = "marvell,orion-ehci", },
> >       {},
> > @@ -336,8 +307,31 @@ static struct platform_driver ehci_orion_driver = {
> >       .remove         = __exit_p(ehci_orion_drv_remove),
> >       .shutdown       = usb_hcd_platform_shutdown,
> >       .driver = {
> > -             .name   = "orion-ehci",
> > +             .name   = hcd_name,
>
> Is this really what you want -- changing the driver name from
> "orion-ehci" to "ehci-orion"?  Is that liable to cause trouble?
>
> > +MODULE_DESCRIPTION(DRIVER_DESC);
> > +MODULE_ALIAS("platform:ehci-orion");
>
> And is this really what you want -- changing the alias from
> "platform:orion-ehci" to "platform:ehci-orion"?
>
> Alan Stern
>
> Hi Alan,

Thanks for identified unrelated changes in my patches,in next version patch
I will correct those changes.

Thanks
Manjunath
Andrew Lunn Feb. 19, 2013, 7:24 a.m. | #6
On Mon, Feb 18, 2013 at 05:34:35PM -0500, Alan Stern wrote:
> On Fri, 15 Feb 2013, Arnd Bergmann wrote:
> 
> > From: Manjunath Goudar <manjunath.goudar@linaro.org>
> > 
> > With the multiplatform changes in arm-soc tree, it becomes
> > possible to enable the mvebu platform (which uses
> > ehci-orion) at the same time as other platforms that require
> > a conflicting EHCI bus glue. At the moment, this results
> > in a warning like
> > 
> > drivers/usb/host/ehci-hcd.c:1297:0: warning: "PLATFORM_DRIVER" redefined [enabled by default]
> > drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the previous definition
> > drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver' defined but not used [-Wunused-variable]
> > 
> > and an ehci driver that only works on one of them.
> > 
> > With the infrastructure added by Alan Stern in patch 3e0232039
> > "USB: EHCI: prepare to make ehci-hcd a library module", we can
> > avoid this problem by turning a bus glue into a separate
> > module, as we do here for the orion bus glue.
> 
> > --- a/drivers/usb/host/Makefile
> > +++ b/drivers/usb/host/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_USB_EHCI_MXC)	+= ehci-mxc.o
> >  
> >  obj-$(CONFIG_USB_OXU210HP_HCD)	+= oxu210hp-hcd.o
> >  obj-$(CONFIG_USB_EHCI_HCD_VT8500)+= ehci-vt8500.o
> > +obj-$(CONFIG_USB_EHCI_HCD_ORION)+= ehci-orion.o
> 
> Both of these two new lines should be formatted like the other lines in
> this file (i.e., with tabs at the corresponding places), and they
> should come before the OXU210HP_HCD entry so that they are next to the
> other EHCI-related lines.
> 
> > --- a/drivers/usb/host/ehci-orion.c
> > +++ b/drivers/usb/host/ehci-orion.c
> > @@ -17,6 +17,13 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_irq.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/hcd.h>
> > +#include <linux/io.h>
> > +#include <linux/dma-mapping.h>
> 
> Is this line really needed?
> 
> > @@ -34,6 +41,17 @@
> >  #define USB_PHY_IVREF_CTRL	0x440
> >  #define USB_PHY_TST_GRP_CTRL	0x450
> >  
> > +#define DRIVER_DESC "EHCI orion driver"
> > +
> > +static const char hcd_name[] = "ehci-orion";
> > +
> > +static struct hc_driver __read_mostly ehci_orion_hc_driver;
> > +
> > +static const struct ehci_driver_overrides orion_overrides __initdata = {
> > +	.reset = ehci_setup,
> > +};
> 
> This is not necessary; ehci_setup is the default value anyway.  This 
> structure can be omitted.
> 
> > @@ -323,8 +296,6 @@ static int __exit ehci_orion_drv_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >  
> > -MODULE_ALIAS("platform:orion-ehci");
> > -
> >  static const struct of_device_id ehci_orion_dt_ids[] = {
> >  	{ .compatible = "marvell,orion-ehci", },
> >  	{},
> > @@ -336,8 +307,31 @@ static struct platform_driver ehci_orion_driver = {
> >  	.remove		= __exit_p(ehci_orion_drv_remove),
> >  	.shutdown	= usb_hcd_platform_shutdown,
> >  	.driver = {
> > -		.name	= "orion-ehci",
> > +		.name	= hcd_name,
> 
> Is this really what you want -- changing the driver name from 
> "orion-ehci" to "ehci-orion"?  Is that liable to cause trouble?
> 
> > +MODULE_DESCRIPTION(DRIVER_DESC);
> > +MODULE_ALIAS("platform:ehci-orion");
> 
> And is this really what you want -- changing the alias from 
> "platform:orion-ehci" to "platform:ehci-orion"?

Hi Manjunath

I can confirm that this breaks non DT based kirkwood systems. The
driver does not get loaded.

Sorry for not testing and finding this case earlier, i just tested a
DT based system.

GregKH: Please can you drop this patch from usb-next. It breaks more
than it fixes.

   Andrew
Arnaud Patard (Rtp Feb. 19, 2013, 8:32 a.m. | #7
Andrew Lunn <andrew@lunn.ch> writes:
Hi,

[...]

>> > +
>> > +static const char hcd_name[] = "ehci-orion";

[...]

>> >  }
>> >  
>> > -MODULE_ALIAS("platform:orion-ehci");
>> > -
>> >  static const struct of_device_id ehci_orion_dt_ids[] = {
>> >  	{ .compatible = "marvell,orion-ehci", },

orion-ehci here ...

>> >  	{},
>> > @@ -336,8 +307,31 @@ static struct platform_driver ehci_orion_driver = {
>> >  	.remove		= __exit_p(ehci_orion_drv_remove),
>> >  	.shutdown	= usb_hcd_platform_shutdown,
>> >  	.driver = {
>> > -		.name	= "orion-ehci",
>> > +		.name	= hcd_name,

... and ehci-orion here. This would explain why only DT case seems to
work. I'm wondering why it has not been changed given that it has been
changed everywhere else, breaking stuff.

>> 
>> Is this really what you want -- changing the driver name from 
>> "orion-ehci" to "ehci-orion"?  Is that liable to cause trouble?
>> 
>> > +MODULE_DESCRIPTION(DRIVER_DESC);
>> > +MODULE_ALIAS("platform:ehci-orion");
>> 
>> And is this really what you want -- changing the alias from 
>> "platform:orion-ehci" to "platform:ehci-orion"?
>
> Hi Manjunath
>
> I can confirm that this breaks non DT based kirkwood systems. The
> driver does not get loaded.
>
> Sorry for not testing and finding this case earlier, i just tested a
> DT based system.

Maybe doing a mass renaming from orion-ehci to ehci-orion in
arch/arm/* files would be enough ? [ well, not necessary everywhere, I'm
not sure if changing the clock name in mach-kirkwood/board-dt.c would be
a good idea given the of node didn't change its name ]

Arnaud
manjunath.goudar@linaro.org Feb. 19, 2013, 9:26 a.m. | #8
On 19 February 2013 14:02, Arnaud Patard <arnaud.patard@rtp-net.org> wrote:

> Andrew Lunn <andrew@lunn.ch> writes:
> Hi,
>
> [...]
>
> >> > +
> >> > +static const char hcd_name[] = "ehci-orion";
>
> [...]
>
> >> >  }
> >> >
> >> > -MODULE_ALIAS("platform:orion-ehci");
> >> > -
> >> >  static const struct of_device_id ehci_orion_dt_ids[] = {
> >> >    { .compatible = "marvell,orion-ehci", },
>
> orion-ehci here ...
>
> >> >    {},
> >> > @@ -336,8 +307,31 @@ static struct platform_driver ehci_orion_driver
> = {
> >> >    .remove         = __exit_p(ehci_orion_drv_remove),
> >> >    .shutdown       = usb_hcd_platform_shutdown,
> >> >    .driver = {
> >> > -          .name   = "orion-ehci",
> >> > +          .name   = hcd_name,
>
> ... and ehci-orion here. This would explain why only DT case seems to
> work. I'm wondering why it has not been changed given that it has been
> changed everywhere else, breaking stuff.
>
> >>
> >> Is this really what you want -- changing the driver name from
> >> "orion-ehci" to "ehci-orion"?  Is that liable to cause trouble?
> >>
> >> > +MODULE_DESCRIPTION(DRIVER_DESC);
> >> > +MODULE_ALIAS("platform:ehci-orion");
> >>
> >> And is this really what you want -- changing the alias from
> >> "platform:orion-ehci" to "platform:ehci-orion"?
> >
> > Hi Manjunath
> >
> > I can confirm that this breaks non DT based kirkwood systems. The
> > driver does not get loaded.
> >
> > Sorry for not testing and finding this case earlier, i just tested a
> > DT based system.
>
> Maybe doing a mass renaming from orion-ehci to ehci-orion in
> arch/arm/* files would be enough ? [ well, not necessary everywhere, I'm
> not sure if changing the clock name in mach-kirkwood/board-dt.c would be
> a good idea given the of node didn't change its name ]
>
> Arnaud
>

Hi Arnaud,

Thank you for suggestion,in next version patch I will fix that issue.

Thanks
Manjunath Goudar
manjunath.goudar@linaro.org Feb. 19, 2013, 10:18 a.m. | #9
On 19 February 2013 04:04, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Fri, 15 Feb 2013, Arnd Bergmann wrote:
>
> > From: Manjunath Goudar <manjunath.goudar@linaro.org>
> >
> > With the multiplatform changes in arm-soc tree, it becomes
> > possible to enable the mvebu platform (which uses
> > ehci-orion) at the same time as other platforms that require
> > a conflicting EHCI bus glue. At the moment, this results
> > in a warning like
> >
> > drivers/usb/host/ehci-hcd.c:1297:0: warning: "PLATFORM_DRIVER" redefined
> [enabled by default]
> > drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the
> previous definition
> > drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver'
> defined but not used [-Wunused-variable]
> >
> > and an ehci driver that only works on one of them.
> >
> > With the infrastructure added by Alan Stern in patch 3e0232039
> > "USB: EHCI: prepare to make ehci-hcd a library module", we can
> > avoid this problem by turning a bus glue into a separate
> > module, as we do here for the orion bus glue.
>
> > --- a/drivers/usb/host/Makefile
> > +++ b/drivers/usb/host/Makefile
> > @@ -30,6 +30,7 @@ obj-$(CONFIG_USB_EHCI_MXC)  += ehci-mxc.o
> >
> >  obj-$(CONFIG_USB_OXU210HP_HCD)       += oxu210hp-hcd.o
> >  obj-$(CONFIG_USB_EHCI_HCD_VT8500)+= ehci-vt8500.o
> > +obj-$(CONFIG_USB_EHCI_HCD_ORION)+= ehci-orion.o
>
> Both of these two new lines should be formatted like the other lines in
> this file (i.e., with tabs at the corresponding places), and they
> should come before the OXU210HP_HCD entry so that they are next to the
> other EHCI-related lines.
>
> > --- a/drivers/usb/host/ehci-orion.c
> > +++ b/drivers/usb/host/ehci-orion.c
> > @@ -17,6 +17,13 @@
> >  #include <linux/of.h>
> >  #include <linux/of_device.h>
> >  #include <linux/of_irq.h>
> > +#include <linux/usb.h>
> > +#include <linux/usb/hcd.h>
> > +#include <linux/io.h>
> > +#include <linux/dma-mapping.h>
>
> Is this line really needed?
>
> > @@ -34,6 +41,17 @@
> >  #define USB_PHY_IVREF_CTRL   0x440
> >  #define USB_PHY_TST_GRP_CTRL 0x450
> >
> > +#define DRIVER_DESC "EHCI orion driver"
> > +
> > +static const char hcd_name[] = "ehci-orion";
> > +
> > +static struct hc_driver __read_mostly ehci_orion_hc_driver;
> > +
> > +static const struct ehci_driver_overrides orion_overrides __initdata = {
> > +     .reset = ehci_setup,
> > +};
>
> This is not necessary; ehci_setup is the default value anyway.  This
> structure can be omitted.
>

ehci_init_driver function we are passing orion_overrides,if we are removing
above struct initialization, what should we need to pass in
the  ehci_init_driver function.


> > @@ -323,8 +296,6 @@ static int __exit ehci_orion_drv_remove(struct
> platform_device *pdev)
> >       return 0;
> >  }
> >
> > -MODULE_ALIAS("platform:orion-ehci");
> > -
> >  static const struct of_device_id ehci_orion_dt_ids[] = {
> >       { .compatible = "marvell,orion-ehci", },
> >       {},
> > @@ -336,8 +307,31 @@ static struct platform_driver ehci_orion_driver = {
> >       .remove         = __exit_p(ehci_orion_drv_remove),
> >       .shutdown       = usb_hcd_platform_shutdown,
> >       .driver = {
> > -             .name   = "orion-ehci",
> > +             .name   = hcd_name,
>
> Is this really what you want -- changing the driver name from
> "orion-ehci" to "ehci-orion"?  Is that liable to cause trouble?
>
> > +MODULE_DESCRIPTION(DRIVER_DESC);
> > +MODULE_ALIAS("platform:ehci-orion");
>
> And is this really what you want -- changing the alias from
> "platform:orion-ehci" to "platform:ehci-orion"?
>
> Alan Stern
>
> Thank you for suggesting ,next version patch I will do this modification.

Thanks
Manjunath Goudar
Arnd Bergmann Feb. 19, 2013, 10:56 a.m. | #10
On Tuesday 19 February 2013, Manjunath Goudar wrote:
> > This is not necessary; ehci_setup is the default value anyway.  This
> > structure can be omitted.
> >
> 
> ehci_init_driver function we are passing orion_overrides,if we are removing
> above struct initialization, what should we need to pass in
> the  ehci_init_driver function.
> 

ehci_init_driver already checks if you pass an override in there, and does
not override anything in that case, so you can simply pass NULL as the
argument.

	Arnd
Alan Stern Feb. 19, 2013, 3:26 p.m. | #11
On Tue, 19 Feb 2013, Arnd Bergmann wrote:

> On Tuesday 19 February 2013, Manjunath Goudar wrote:
> > > This is not necessary; ehci_setup is the default value anyway.  This
> > > structure can be omitted.
> > >
> > 
> > ehci_init_driver function we are passing orion_overrides,if we are removing
> > above struct initialization, what should we need to pass in
> > the  ehci_init_driver function.
> > 
> 
> ehci_init_driver already checks if you pass an override in there, and does
> not override anything in that case, so you can simply pass NULL as the
> argument.

That's right.  Manjunath, you can answer at lot of questions like this 
for yourself very easily, simply by reading the source code.

Alan Stern
Alan Stern Feb. 20, 2013, 4:02 p.m. | #12
On Fri, 15 Feb 2013, Arnd Bergmann wrote:

> From: Manjunath Goudar <manjunath.goudar@linaro.org>
> 
> With the multiplatform changes in arm-soc tree, it becomes
> possible to enable the mvebu platform (which uses
> ehci-orion) at the same time as other platforms that require
> a conflicting EHCI bus glue. At the moment, this results
> in a warning like
> 
> drivers/usb/host/ehci-hcd.c:1297:0: warning: "PLATFORM_DRIVER" redefined [enabled by default]
> drivers/usb/host/ehci-hcd.c:1277:0: note: this is the location of the previous definition
> drivers/usb/host/ehci-orion.c:334:31: warning: 'ehci_orion_driver' defined but not used [-Wunused-variable]
> 
> and an ehci driver that only works on one of them.
> 
> With the infrastructure added by Alan Stern in patch 3e0232039
> "USB: EHCI: prepare to make ehci-hcd a library module", we can
> avoid this problem by turning a bus glue into a separate
> module, as we do here for the orion bus glue.

One more comment on this patch...

> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -162,6 +162,14 @@ config USB_EHCI_HCD_OMAP
>  	  Enables support for the on-chip EHCI controller on
>  	  OMAP3 and later chips.
>  
> +config USB_EHCI_HCD_ORION
> +        tristate  "Support for Marvell Orion on-chip EHCI USB controller"
> +        depends on USB_EHCI_HCD && PLAT_ORION
> +        default y
> +        ---help---
> +          Enables support for the on-chip EHCI controller on
> +          Morvell Orion chips.

Currently there is no Kconfig option to control specifically whether
the ehci-orion driver gets built; it always gets built whenever
CONFIG_PLAT_ORION and CONFIG_USB_EHCI_HCD are both enabled.

Do you think it is a good idea to add an option for this?  Should it at
least be non-interactive, so that the driver always gets built under
the same conditions as currently?  A later patch can make it 
interactive, if desired.

Alan Stern
Arnd Bergmann Feb. 20, 2013, 4:27 p.m. | #13
On Wednesday 20 February 2013, Alan Stern wrote:
> Currently there is no Kconfig option to control specifically whether
> the ehci-orion driver gets built; it always gets built whenever
> CONFIG_PLAT_ORION and CONFIG_USB_EHCI_HCD are both enabled.
> 
> Do you think it is a good idea to add an option for this?  Should it at
> least be non-interactive, so that the driver always gets built under
> the same conditions as currently?  A later patch can make it 
> interactive, if desired.

I think it's good to have it interactive, because we are now building for
multiplatform with Orion being one of many. It's useful to be able to
build EHCI_HCD=y AND EHCD_HCD_ORION=m.

However, you are right that this is a change that was not mentioned in
the patch description and would better have been kept separate or
at least explicitly spelled out.

	Arnd
Jason Cooper Feb. 20, 2013, 4:54 p.m. | #14
Alan,

On Wed, Feb 20, 2013 at 04:27:04PM +0000, Arnd Bergmann wrote:
> On Wednesday 20 February 2013, Alan Stern wrote:
> > Currently there is no Kconfig option to control specifically whether
> > the ehci-orion driver gets built; it always gets built whenever
> > CONFIG_PLAT_ORION and CONFIG_USB_EHCI_HCD are both enabled.
> > 
> > Do you think it is a good idea to add an option for this?  Should it at
> > least be non-interactive, so that the driver always gets built under
> > the same conditions as currently?  A later patch can make it 
> > interactive, if desired.
> 
> I think it's good to have it interactive, because we are now building for
> multiplatform with Orion being one of many. It's useful to be able to
> build EHCI_HCD=y AND EHCD_HCD_ORION=m.

Yes, explicit is preferable.  On kirkwood usb has it's own gateable
clock.  The user should be able to conserve power by unloading the
module or building without it.

thx,

Jason.
Greg KH Feb. 20, 2013, 6:28 p.m. | #15
On Tue, Feb 19, 2013 at 08:24:38AM +0100, Andrew Lunn wrote:
> 
> GregKH: Please can you drop this patch from usb-next. It breaks more
> than it fixes.

I've now reverted both of these, and the follow-on patch that Arnd sent
in.

Arnd, sorry, but please be more careful, especially when you ask for
patches to be rushed into the tree...

greg k-h
Arnd Bergmann Feb. 20, 2013, 6:44 p.m. | #16
On Wednesday 20 February 2013, Greg KH wrote:
> On Tue, Feb 19, 2013 at 08:24:38AM +0100, Andrew Lunn wrote:
> > 
> > GregKH: Please can you drop this patch from usb-next. It breaks more
> > than it fixes.
> 
> I've now reverted both of these, and the follow-on patch that Arnd sent
> in.
> 
> Arnd, sorry, but please be more careful, especially when you ask for
> patches to be rushed into the tree...

Yes, I understand. I'll try to come up with a less invasive way to
deal with the build error in 3.9 once the ARM multiplatform branch
gets pulled.

I guess we could either forbid the combinations that are now broken
using Kconfig dependencies, or add just add separate registriation
paths to ehci_hcd_init() and ehci_hcd_cleanup() as we have for
powerpc, although Alan did not like that too much last time I
suggested it.

	Arnd

Patch

diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index d77e028..7ac6f48 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -162,6 +162,14 @@  config USB_EHCI_HCD_OMAP
 	  Enables support for the on-chip EHCI controller on
 	  OMAP3 and later chips.
 
+config USB_EHCI_HCD_ORION
+        tristate  "Support for Marvell Orion on-chip EHCI USB controller"
+        depends on USB_EHCI_HCD && PLAT_ORION
+        default y
+        ---help---
+          Enables support for the on-chip EHCI controller on
+          Morvell Orion chips.
+
 config USB_EHCI_HCD_VT8500
         tristate "Support for VT8500 on-chip EHCI USB controller"
         depends on USB_EHCI_HCD && ARCH_VT8500
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index b0da9cf..4db3f01 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -30,6 +30,7 @@  obj-$(CONFIG_USB_EHCI_MXC)	+= ehci-mxc.o
 
 obj-$(CONFIG_USB_OXU210HP_HCD)	+= oxu210hp-hcd.o
 obj-$(CONFIG_USB_EHCI_HCD_VT8500)+= ehci-vt8500.o
+obj-$(CONFIG_USB_EHCI_HCD_ORION)+= ehci-orion.o
 obj-$(CONFIG_USB_ISP116X_HCD)	+= isp116x-hcd.o
 obj-$(CONFIG_USB_ISP1362_HCD)	+= isp1362-hcd.o
 obj-$(CONFIG_USB_OHCI_HCD)	+= ohci-hcd.o
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index 487ebb8..26154f0 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -1272,11 +1272,6 @@  MODULE_LICENSE ("GPL");
 #define XILINX_OF_PLATFORM_DRIVER	ehci_hcd_xilinx_of_driver
 #endif
 
-#ifdef CONFIG_PLAT_ORION
-#include "ehci-orion.c"
-#define	PLATFORM_DRIVER		ehci_orion_driver
-#endif
-
 #ifdef CONFIG_USB_W90X900_EHCI
 #include "ehci-w90x900.c"
 #define	PLATFORM_DRIVER		ehci_hcd_w90x900_driver
@@ -1343,6 +1338,7 @@  MODULE_LICENSE ("GPL");
 	!IS_ENABLED(CONFIG_USB_EHCI_MXC) && \
 	!defined(PLATFORM_DRIVER) && \
 	!IS_ENABLED(CONFIG_ARCH_VT8500) && \
+	!IS_ENABLED(CONFIG_PLAT_ORION) && \
 	!defined(PS3_SYSTEM_BUS_DRIVER) && \
 	!defined(OF_PLATFORM_DRIVER) && \
 	!defined(XILINX_OF_PLATFORM_DRIVER)
diff --git a/drivers/usb/host/ehci-orion.c b/drivers/usb/host/ehci-orion.c
index 914a3ec..cfc4803 100644
--- a/drivers/usb/host/ehci-orion.c
+++ b/drivers/usb/host/ehci-orion.c
@@ -17,6 +17,13 @@ 
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/of_irq.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+#include <linux/io.h>
+#include <linux/dma-mapping.h>
+
+#include "ehci.h"
+
 
 #define rdl(off)	__raw_readl(hcd->regs + (off))
 #define wrl(off, val)	__raw_writel((val), hcd->regs + (off))
@@ -34,6 +41,17 @@ 
 #define USB_PHY_IVREF_CTRL	0x440
 #define USB_PHY_TST_GRP_CTRL	0x450
 
+#define DRIVER_DESC "EHCI orion driver"
+
+static const char hcd_name[] = "ehci-orion";
+
+static struct hc_driver __read_mostly ehci_orion_hc_driver;
+
+static const struct ehci_driver_overrides orion_overrides __initdata = {
+	.reset = ehci_setup,
+};
+
+
 /*
  * Implement Orion USB controller specification guidelines
  */
@@ -104,51 +122,6 @@  static void orion_usb_phy_v1_setup(struct usb_hcd *hcd)
 	wrl(USB_MODE, 0x13);
 }
 
-static const struct hc_driver ehci_orion_hc_driver = {
-	.description = hcd_name,
-	.product_desc = "Marvell Orion EHCI",
-	.hcd_priv_size = sizeof(struct ehci_hcd),
-
-	/*
-	 * generic hardware linkage
-	 */
-	.irq = ehci_irq,
-	.flags = HCD_MEMORY | HCD_USB2,
-
-	/*
-	 * basic lifecycle operations
-	 */
-	.reset = ehci_setup,
-	.start = ehci_run,
-	.stop = ehci_stop,
-	.shutdown = ehci_shutdown,
-
-	/*
-	 * managing i/o requests and associated device resources
-	 */
-	.urb_enqueue = ehci_urb_enqueue,
-	.urb_dequeue = ehci_urb_dequeue,
-	.endpoint_disable = ehci_endpoint_disable,
-	.endpoint_reset = ehci_endpoint_reset,
-
-	/*
-	 * scheduling support
-	 */
-	.get_frame_number = ehci_get_frame,
-
-	/*
-	 * root hub support
-	 */
-	.hub_status_data = ehci_hub_status_data,
-	.hub_control = ehci_hub_control,
-	.bus_suspend = ehci_bus_suspend,
-	.bus_resume = ehci_bus_resume,
-	.relinquish_port = ehci_relinquish_port,
-	.port_handed_over = ehci_port_handed_over,
-
-	.clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
-};
-
 static void
 ehci_orion_conf_mbus_windows(struct usb_hcd *hcd,
 			     const struct mbus_dram_target_info *dram)
@@ -323,8 +296,6 @@  static int __exit ehci_orion_drv_remove(struct platform_device *pdev)
 	return 0;
 }
 
-MODULE_ALIAS("platform:orion-ehci");
-
 static const struct of_device_id ehci_orion_dt_ids[] = {
 	{ .compatible = "marvell,orion-ehci", },
 	{},
@@ -336,8 +307,31 @@  static struct platform_driver ehci_orion_driver = {
 	.remove		= __exit_p(ehci_orion_drv_remove),
 	.shutdown	= usb_hcd_platform_shutdown,
 	.driver = {
-		.name	= "orion-ehci",
+		.name	= hcd_name,
 		.owner  = THIS_MODULE,
 		.of_match_table = of_match_ptr(ehci_orion_dt_ids),
 	},
 };
+
+static int __init ehci_orion_init(void)
+{
+	if (usb_disabled())
+		return -ENODEV;
+
+	pr_info("%s: " DRIVER_DESC "\n", hcd_name);
+
+	ehci_init_driver(&ehci_orion_hc_driver, &orion_overrides);
+	return platform_driver_register(&ehci_orion_driver);
+}
+module_init(ehci_orion_init);
+
+static void __exit ehci_orion_cleanup(void)
+{
+	platform_driver_unregister(&ehci_orion_driver);
+}
+module_exit(ehci_orion_cleanup);
+
+MODULE_DESCRIPTION(DRIVER_DESC);
+MODULE_ALIAS("platform:ehci-orion");
+MODULE_AUTHOR("Tzachi Perelstein");
+MODULE_LICENSE("GPL");