| Message ID | 1360966349-1242560-2-git-send-email-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 6ed3c43d05f6d0d55f17947bc287f35318fd96f8 |
| Headers | show |
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.
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
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,
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
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
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
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
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
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
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
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
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
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
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.
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
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
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");