[RFC,1/2] USB: OHCI: prepare to make ohci-hcd a library module

Message ID 516d2f1e.62ae440a.26b5.ffff86eb@mx.google.com
State New
Headers show

Commit Message

manjunath.goudar@linaro.org April 16, 2013, 10:58 a.m.
From: Manjunath Goudar <manjunath.goudar@linaro.org>

This patch prepares ohci-hcd for being split up into a core
library and separate platform driver modules.  A generic
ohci_hc_driver structure is created, containing all the "standard"
values, and a new mechanism is added whereby a driver module can
specify a set of overrides to those values.  In addition the
ohci_init(),ohci_restart(),ohci_hcd_init(),ohci_run() ohci_stop(),
ohci_suspend() and ohci_resume() routines need to be EXPORTed
for use by the drivers.

Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Greg KH <greg@kroah.com>
Cc: Alan Stern <stern@rowland.harvard.edu>
Cc: linux-usb@vger.kernel.org
---
 drivers/usb/host/ohci-hcd.c |   87 ++++++++++++++++++++++++++++++++++++-------
 drivers/usb/host/ohci-mem.c |    3 +-
 drivers/usb/host/ohci.h     |   39 +++++++++++++++++++
 3 files changed, 115 insertions(+), 14 deletions(-)

Comments

Arnd Bergmann April 17, 2013, 12:01 p.m. | #1
On Tuesday 16 April 2013, manjunath.goudar@linaro.org wrote:
> From: Manjunath Goudar <manjunath.goudar@linaro.org>
> 
> This patch prepares ohci-hcd for being split up into a core
> library and separate platform driver modules.  A generic
> ohci_hc_driver structure is created, containing all the "standard"
> values, and a new mechanism is added whereby a driver module can
> specify a set of overrides to those values.  In addition the
> ohci_init(),ohci_restart(),ohci_hcd_init(),ohci_run() ohci_stop(),
> ohci_suspend() and ohci_resume() routines need to be EXPORTed
> for use by the drivers.
> 
> Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Greg KH <greg@kroah.com>
> Cc: Alan Stern <stern@rowland.harvard.edu>
> Cc: linux-usb@vger.kernel.org

Hi Manjunath,

This looks nice I think, but let's see what Alan has to say about it.

I did not see any information in your description about how you
tested the changes. Are you able to use any USB-1.1 devices with
this on a PC, using ohci-pci as a loadable module?

> +
> +#if defined(CONFIG_PM) || defined(CONFIG_PCI)
> +extern int ohci_restart (struct ohci_hcd *ohci);
> +#else
> +static inline int ohci_restart (struct ohci_hcd *ohci)
> +{
> +	return;
> +}
> +#endif

I looked at who calls ohci_restart, and I don't think the #else clause
is actually needed here, or the "return" for that matter.

Just declare the function unconditionally.

I would also remove the space after the ohci_restart identifier, since
that is the common coding style.

	Arnd
manjunath.goudar@linaro.org April 18, 2013, 6:32 a.m. | #2
On 17 April 2013 17:31, Arnd Bergmann <arnd@arndb.de> wrote:

> On Tuesday 16 April 2013, manjunath.goudar@linaro.org wrote:
> > From: Manjunath Goudar <manjunath.goudar@linaro.org>
> >
> > This patch prepares ohci-hcd for being split up into a core
> > library and separate platform driver modules.  A generic
> > ohci_hc_driver structure is created, containing all the "standard"
> > values, and a new mechanism is added whereby a driver module can
> > specify a set of overrides to those values.  In addition the
> > ohci_init(),ohci_restart(),ohci_hcd_init(),ohci_run() ohci_stop(),
> > ohci_suspend() and ohci_resume() routines need to be EXPORTed
> > for use by the drivers.
> >
> > Signed-off-by: Manjunath Goudar <manjunath.goudar@linaro.org>
> > Cc: Arnd Bergmann <arnd@arndb.de>
> > Cc: Greg KH <greg@kroah.com>
> > Cc: Alan Stern <stern@rowland.harvard.edu>
> > Cc: linux-usb@vger.kernel.org
>
> Hi Manjunath,
>
> This looks nice I think, but let's see what Alan has to say about it.
>
> I did not see any information in your description about how you
> tested the changes. Are you able to use any USB-1.1 devices with
> this on a PC, using ohci-pci as a loadable module?
>
> I did not test, I applied only Alan EHCI concept here.

> > +
> > +#if defined(CONFIG_PM) || defined(CONFIG_PCI)
> > +extern int ohci_restart (struct ohci_hcd *ohci);
> > +#else
> > +static inline int ohci_restart (struct ohci_hcd *ohci)
> > +{
> > +     return;
> > +}
> > +#endif
>
> I looked at who calls ohci_restart, and I don't think the #else clause
> is actually needed here, or the "return" for that matter.
>
> Just declare the function unconditionally.
>
> I would also remove the space after the ohci_restart identifier, since
> that is the common coding style.
>
>
I think it is not necessary that's why I will remove in next version.

Manjunath Goudar
Alan Stern April 18, 2013, 6:46 p.m. | #3
On Tue, 16 Apr 2013 manjunath.goudar@linaro.org wrote:

> From: Manjunath Goudar <manjunath.goudar@linaro.org>
> 
> This patch prepares ohci-hcd for being split up into a core
> library and separate platform driver modules.  A generic
> ohci_hc_driver structure is created, containing all the "standard"
> values, and a new mechanism is added whereby a driver module can
> specify a set of overrides to those values.  In addition the
> ohci_init(),ohci_restart(),ohci_hcd_init(),ohci_run() ohci_stop(),
> ohci_suspend() and ohci_resume() routines need to be EXPORTed
> for use by the drivers.

This is a good start, but it goes too far by exporting routines that
ought to be private.  ohci_hcd_init, ohci_run, and ohci_stop should not
be called by bus glue modules:

	ohci_hcd_init doesn't need to be called by glue modules at all,
	since ohci_init already calls it.

	Glue modules should not override ohci_hc_driver.start.  Stuff 
	like the code at the start of ohci_pci_start can be moved to
	the end of the module's reset routine.	

	Likewise, glue modules should not need to call ohci_stop.
	If ohci_run fails, it should call ohci_stop itself.

Lastly, I agree with Arnd that sb800_prefetch should be moved to the 
pci-quirks.c file.

Alan Stern
manjunath.goudar@linaro.org April 23, 2013, 10:14 a.m. | #4
On 19 April 2013 00:16, Alan Stern <stern@rowland.harvard.edu> wrote:

> On Tue, 16 Apr 2013 manjunath.goudar@linaro.org wrote:
>
> > From: Manjunath Goudar <manjunath.goudar@linaro.org>
> >
> > This patch prepares ohci-hcd for being split up into a core
> > library and separate platform driver modules.  A generic
> > ohci_hc_driver structure is created, containing all the "standard"
> > values, and a new mechanism is added whereby a driver module can
> > specify a set of overrides to those values.  In addition the
> > ohci_init(),ohci_restart(),ohci_hcd_init(),ohci_run() ohci_stop(),
> > ohci_suspend() and ohci_resume() routines need to be EXPORTed
> > for use by the drivers.
>
> This is a good start, but it goes too far by exporting routines that
> ought to be private.  ohci_hcd_init, ohci_run, and ohci_stop should not
> be called by bus glue modules:
>

Yes I agree,even I made  ohci_hcd_init, ohci_run, and ohci_stop as static
function.

for ohci controller initialization written generic function as ohci_setup.

int ohci_setup(struct usb_hcd *hcd)
{
        struct ohci_hcd *ohci = hcd_to_ohci(hcd);
        int retval;

        ohci->sbrn = HCD_USB11;

        /* data structure init */
        retval = ohci_init(ohci);
        if (retval)
                return retval;
        ohci_usb_reset(ohci);
        return 0;
}


>         ohci_hcd_init doesn't need to be called by glue modules at all,
>         since ohci_init already calls it.
>
> In all ohci blus glue called ohci_hcd_init in probe function,I am planing
to remove ohci_hcd_init function from ohci.mem.c file
 and moving the below statements to ohci_init function.this is my approach.
        ohci->next_statechange = jiffies;
        spin_lock_init (&ohci->lock);
        INIT_LIST_HEAD (&ohci->pending);

What about your suggestion.

        Glue modules should not override ohci_hc_driver.start.  Stuff
>         like the code at the start of ohci_pci_start can be moved to
>         the end of the module's reset routine.
>

I agree and now I am not  override the "start" function.

        Likewise, glue modules should not need to call ohci_stop.
>         If ohci_run fails, it should call ohci_stop itself.
>
> Lastly, I agree with Arnd that sb800_prefetch should be moved to the
> pci-quirks.c file.
>


>
> Alan Stern
>
>

Patch

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
index 9e6de95..fb869fc 100644
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -79,12 +79,6 @@  static const char	hcd_name [] = "ohci_hcd";
 #include "pci-quirks.h"
 
 static void ohci_dump (struct ohci_hcd *ohci, int verbose);
-static int ohci_init (struct ohci_hcd *ohci);
-static void ohci_stop (struct usb_hcd *hcd);
-
-#if defined(CONFIG_PM) || defined(CONFIG_PCI)
-static int ohci_restart (struct ohci_hcd *ohci);
-#endif
 
 #ifdef CONFIG_PCI
 static void sb800_prefetch(struct ohci_hcd *ohci, int on);
@@ -520,7 +514,7 @@  done:
 
 /* init memory, and kick BIOS/SMM off */
 
-static int ohci_init (struct ohci_hcd *ohci)
+int ohci_init(struct ohci_hcd *ohci)
 {
 	int ret;
 	struct usb_hcd *hcd = ohci_to_hcd(ohci);
@@ -590,6 +584,7 @@  static int ohci_init (struct ohci_hcd *ohci)
 
 	return ret;
 }
+EXPORT_SYMBOL_GPL(ohci_init);
 
 /*-------------------------------------------------------------------------*/
 
@@ -597,7 +592,7 @@  static int ohci_init (struct ohci_hcd *ohci)
  * resets USB and controller
  * enable interrupts
  */
-static int ohci_run (struct ohci_hcd *ohci)
+int ohci_run(struct ohci_hcd *ohci)
 {
 	u32			mask, val;
 	int			first = ohci->fminterval == 0;
@@ -767,6 +762,7 @@  retry:
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ohci_run);
 
 /*-------------------------------------------------------------------------*/
 
@@ -914,7 +910,7 @@  static irqreturn_t ohci_irq (struct usb_hcd *hcd)
 
 /*-------------------------------------------------------------------------*/
 
-static void ohci_stop (struct usb_hcd *hcd)
+void ohci_stop(struct usb_hcd *hcd)
 {
 	struct ohci_hcd		*ohci = hcd_to_ohci (hcd);
 
@@ -943,13 +939,14 @@  static void ohci_stop (struct usb_hcd *hcd)
 		ohci->hcca_dma = 0;
 	}
 }
+EXPORT_SYMBOL_GPL(ohci_stop);
 
 /*-------------------------------------------------------------------------*/
 
 #if defined(CONFIG_PM) || defined(CONFIG_PCI)
 
 /* must not be called from interrupt context */
-static int ohci_restart (struct ohci_hcd *ohci)
+int ohci_restart(struct ohci_hcd *ohci)
 {
 	int temp;
 	int i;
@@ -1008,12 +1005,13 @@  static int ohci_restart (struct ohci_hcd *ohci)
 	ohci_dbg(ohci, "restart complete\n");
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ohci_restart);
 
 #endif
 
 #ifdef CONFIG_PM
 
-static int __maybe_unused ohci_suspend(struct usb_hcd *hcd, bool do_wakeup)
+int ohci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 {
 	struct ohci_hcd	*ohci = hcd_to_ohci (hcd);
 	unsigned long	flags;
@@ -1031,9 +1029,9 @@  static int __maybe_unused ohci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ohci_suspend);
 
-
-static int __maybe_unused ohci_resume(struct usb_hcd *hcd, bool hibernated)
+int ohci_resume(struct usb_hcd *hcd, bool hibernated)
 {
 	struct ohci_hcd		*ohci = hcd_to_ohci(hcd);
 	int			port;
@@ -1081,11 +1079,74 @@  static int __maybe_unused ohci_resume(struct usb_hcd *hcd, bool hibernated)
 
 	return 0;
 }
+EXPORT_SYMBOL_GPL(ohci_resume);
 
 #endif
 
 /*-------------------------------------------------------------------------*/
 
+/*
+ * Generic structure: This gets copied for platform drivers so that
+ * individual entries can be overridden as needed.
+ */
+
+static const struct hc_driver ohci_hc_driver = {
+	.description =          hcd_name,
+	.product_desc =         "OHCI Host Controller",
+	.hcd_priv_size =        sizeof(struct ohci_hcd),
+
+	/*
+	 * generic hardware linkage
+	*/
+	.irq =                  ohci_irq,
+	.flags =                HCD_MEMORY | HCD_USB11,
+
+	/*
+	* basic lifecycle operations
+	*/
+	.start =                ohci_run,
+	.stop =                 ohci_stop,
+	.shutdown =             ohci_shutdown,
+
+	/*
+	 * managing i/o requests and associated device resources
+	*/
+	.urb_enqueue =          ohci_urb_enqueue,
+	.urb_dequeue =          ohci_urb_dequeue,
+	.endpoint_disable =     ohci_endpoint_disable,
+
+	/*
+	* scheduling support
+	*/
+	.get_frame_number =     ohci_get_frame,
+
+	/*
+	* root hub support
+	*/
+	.hub_status_data =      ohci_hub_status_data,
+	.hub_control =          ohci_hub_control,
+	.bus_suspend =          ohci_bus_suspend,
+	.bus_resume =           ohci_bus_resume,
+	.start_port_reset =	ohci_start_port_reset,
+};
+
+void ohci_init_driver(struct hc_driver *drv,
+		const struct ohci_driver_overrides *over)
+{
+	/* Copy the generic table to drv and then apply the overrides */
+	*drv = ohci_hc_driver;
+
+	drv->product_desc = over->product_desc;
+	drv->hcd_priv_size += over->extra_priv_size;
+	if (over->reset)
+		drv->reset = over->reset;
+	if (over->start)
+		drv->start = over->start;
+}
+EXPORT_SYMBOL_GPL(ohci_init_driver);
+
+/*-------------------------------------------------------------------------*/
+
 MODULE_AUTHOR (DRIVER_AUTHOR);
 MODULE_DESCRIPTION(DRIVER_DESC);
 MODULE_LICENSE ("GPL");
diff --git a/drivers/usb/host/ohci-mem.c b/drivers/usb/host/ohci-mem.c
index 2f20d3d..1a4dede 100644
--- a/drivers/usb/host/ohci-mem.c
+++ b/drivers/usb/host/ohci-mem.c
@@ -23,12 +23,13 @@ 
 
 /*-------------------------------------------------------------------------*/
 
-static void ohci_hcd_init (struct ohci_hcd *ohci)
+void ohci_hcd_init (struct ohci_hcd *ohci)
 {
 	ohci->next_statechange = jiffies;
 	spin_lock_init (&ohci->lock);
 	INIT_LIST_HEAD (&ohci->pending);
 }
+EXPORT_SYMBOL_GPL(ohci_hcd_init);
 
 /*-------------------------------------------------------------------------*/
 
diff --git a/drivers/usb/host/ohci.h b/drivers/usb/host/ohci.h
index d329914..12c750a 100644
--- a/drivers/usb/host/ohci.h
+++ b/drivers/usb/host/ohci.h
@@ -718,3 +718,42 @@  static inline u32 roothub_status (struct ohci_hcd *hc)
 	{ return ohci_readl (hc, &hc->regs->roothub.status); }
 static inline u32 roothub_portstatus (struct ohci_hcd *hc, int i)
 	{ return read_roothub (hc, portstatus [i], 0xffe0fce0); }
+
+/* Declarations of things exported for use by ohci platform drivers */
+
+struct ohci_driver_overrides {
+	const char	*product_desc;
+	size_t		extra_priv_size;
+	int		(*reset)(struct usb_hcd *hcd);
+	int		(*start)(struct usb_hcd *hcd);
+};
+
+extern void	ohci_init_driver(struct hc_driver *drv,
+				const struct ohci_driver_overrides *over);
+extern int ohci_init (struct ohci_hcd *ohci);
+extern void ohci_stop (struct usb_hcd *hcd);
+extern void ohci_hcd_init (struct ohci_hcd *ohci);
+extern int ohci_run (struct ohci_hcd *ohci);
+
+#if defined(CONFIG_PM) || defined(CONFIG_PCI)
+extern int ohci_restart (struct ohci_hcd *ohci);
+#else
+static inline int ohci_restart (struct ohci_hcd *ohci)
+{
+	return;
+}
+#endif
+
+#ifdef CONFIG_PM
+extern int ohci_suspend(struct usb_hcd *hcd, bool do_wakeup);
+extern int ohci_resume(struct usb_hcd *hcd, bool hibernated);
+#else
+static inline int ohci_suspend(struct usb_hcd *hcd, bool do_wakeup)
+{
+	return 0;
+}
+static inline int ohci_resume(struct usb_hcd *hcd, bool hibernated)
+{
+	return 0;
+}
+#endif