Hi,
On Mon, Oct 01, 2012 at 04:21:31PM -0400, Alan Stern wrote:
> Felipe:
>
> We've been talking about this for a long time. Here at last is an
> initial attempt at splitting ehci-hcd up into a library module and
> multiple platform driver modules.
>
> The first patch is just preparation. It moves a few items between .c
> and .h files, adds a generic ehci_hc_driver structure, and exports a
> bunch of formerly private routines.
>
> The second patch converts ehci-pci.c to a separate module, as an
> example to show how the whole thing is meant to work. The same
> approach should be usable for almost all the other platform drivers
> (ehci-tegra will be problematic). I did ehci-pci first because that's
> the only one I can test. :-)
>
> This isn't exactly what you had in mind because it doesn't introduce
> another struct device layer. Still, the end result is what you want --
> we will be able to build all the EHCI platform drivers into a single
> kernel.
>
> What do you think?
>
> Alan Stern
>
>
>
> Index: usb-3.6/drivers/usb/host/ehci.h
> ===================================================================
> --- usb-3.6.orig/drivers/usb/host/ehci.h
> +++ usb-3.6/drivers/usb/host/ehci.h
> @@ -761,26 +761,73 @@ static inline u32 hc32_to_cpup (const st
>
> /*-------------------------------------------------------------------------*/
>
> -#ifdef CONFIG_PCI
> -
> -/* For working around the MosChip frame-index-register bug */
> -static unsigned ehci_read_frame_index(struct ehci_hcd *ehci);
> +#define ehci_dbg(ehci, fmt, args...) \
> + dev_dbg(ehci_to_hcd(ehci)->self.controller , fmt , ## args )
> +#define ehci_err(ehci, fmt, args...) \
> + dev_err(ehci_to_hcd(ehci)->self.controller , fmt , ## args )
> +#define ehci_info(ehci, fmt, args...) \
> + dev_info(ehci_to_hcd(ehci)->self.controller , fmt , ## args )
> +#define ehci_warn(ehci, fmt, args...) \
> + dev_warn(ehci_to_hcd(ehci)->self.controller , fmt , ## args )
>
> +#ifdef VERBOSE_DEBUG
> +#define ehci_vdbg ehci_dbg
How about:
#define ehci_vdbg(ehci, fmt, args...) \
dev_vdbg(ehci_to_hcd(ehci)->self.controller , fmt , ## args )
> #else
> -
> -static inline unsigned ehci_read_frame_index(struct ehci_hcd *ehci)
> -{
> - return ehci_readl(ehci, &ehci->regs->frame_index);
> -}
> -
> + static inline void ehci_vdbg(struct ehci_hcd *ehci, ...) {}
> #endif
>
> -/*-------------------------------------------------------------------------*/
> -
> #ifndef DEBUG
> #define STUB_DEBUG_FILES
> #endif /* DEBUG */
>
> /*-------------------------------------------------------------------------*/
>
> +/* Declarations of things exported for use by ehci platform drivers */
> +
> +extern const struct hc_driver ehci_hc_driver;
> +
> +extern irqreturn_t ehci_irq(struct usb_hcd *hcd);
> +
> +extern int ehci_setup(struct usb_hcd *hcd);
> +extern int ehci_run(struct usb_hcd *hcd);
> +extern void ehci_stop(struct usb_hcd *hcd);
> +extern void ehci_shutdown(struct usb_hcd *hcd);
> +
> +extern int ehci_urb_enqueue(struct usb_hcd *hcd, struct urb *urb,
> + gfp_t mem_flags);
> +extern int ehci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb,
> + int status);
> +extern void ehci_endpoint_disable(struct usb_hcd *hcd,
> + struct usb_host_endpoint *ep);
> +extern void ehci_endpoint_reset(struct usb_hcd *hcd,
> + struct usb_host_endpoint *ep);
> +
> +extern int ehci_get_frame(struct usb_hcd *hcd);
> +
> +extern int ehci_hub_status_data(struct usb_hcd *hcd, char *buf);
> +extern int ehci_hub_control(struct usb_hcd *hcd, u16 typeReq,
> + u16 wValue, u16 wIndex,
> + char *buf, u16 wLength);
> +extern void ehci_relinquish_port(struct usb_hcd *hcd, int portnum);
> +extern int ehci_port_handed_over(struct usb_hcd *hcd, int portnum);
> +extern void ehci_clear_tt_buffer_complete(struct usb_hcd *hcd,
> + struct usb_host_endpoint *ep);
> +
> +extern void ehci_port_power(struct ehci_hcd *ehci, int is_on);
> +
> +#ifdef CONFIG_PM
> +extern int ehci_suspend(struct usb_hcd *hcd, bool do_wakeup);
> +extern int ehci_resume(struct usb_hcd *hcd, bool hibernated);
> +extern int ehci_bus_suspend(struct usb_hcd *hcd);
> +extern int ehci_bus_resume(struct usb_hcd *hcd);
> +
> +#else
> +
> +#define ehci_bus_suspend NULL
> +#define ehci_bus_resume NULL
> +#endif /* CONFIG_PM */
> +
> +extern int ehci_lpm_set_da(struct ehci_hcd *ehci, int dev_addr, int port_num);
> +extern int ehci_lpm_check(struct ehci_hcd *ehci, int port);
this is one thing I don't like. Exposing these implementation details to
another unrelated driver (ehci-{pci,omap,tegra,etc}.c is just a small
bridge driver which should only be preparing the platform (be it an SoC
or a Desktop) and passing correct resources for ehci core driver.
This is why I would prefer to have the extra struct device for ehci core
with a parent device on the bridge drivers (pci, omap, tegra, etc).
We wouldn't have to expose all these details. Even though there's no
difference technically, I still think it easier to understand that way.
And, like I suggested before, in cases where platform needs a special
callback for e.g. workaround implementation, we can allow those to be
overwritten if ehci-$platform.c passes a function pointer via
platform_data to ehci core driver.
> #endif /* __LINUX_EHCI_HCD_H */
> Index: usb-3.6/drivers/usb/host/ehci-hcd.c
> ===================================================================
> --- usb-3.6.orig/drivers/usb/host/ehci-hcd.c
> +++ usb-3.6/drivers/usb/host/ehci-hcd.c
> @@ -118,9 +118,24 @@ MODULE_PARM_DESC(hird, "host initiated r
> /*-------------------------------------------------------------------------*/
>
> #include "ehci.h"
> -#include "ehci-dbg.c"
> #include "pci-quirks.h"
>
> +#ifdef CONFIG_PCI
> +
> +/* For working around the MosChip frame-index-register bug */
> +static unsigned ehci_read_frame_index(struct ehci_hcd *ehci);
> +
> +#else
> +
> +static inline unsigned ehci_read_frame_index(struct ehci_hcd *ehci)
> +{
> + return ehci_readl(ehci, &ehci->regs->frame_index);
> +}
> +
> +#endif
See this is one example. What if two other different plaforms need to
work around silicon bugs on different cases ? Will we start sprinkling
ifdefs in this driver again ?
I would much rather have something like:
static const struct ehci_platform_data moschip_pci_platform_data __devinitconst = {
.frame_index = moschip_pci_frame_index,
[ ... ]
};
static int ehci_pci_probe(struct pci_device *pci)
{
struct platform_device *ehci;
ehci = platform_device_alloc(....);
[ ... ]
/* check if MosChip */
ret = platform_device_add_data(ehci, &moschip_pci_platform_data,
sizeof(struct ehci_platform_data));
[ ... ]
return 0;
}
Then on ehci-core, instead of adding ifdefs all over, you could:
static inline unsigned ehci_read_frame_index(struct ehci_hcd *ehci)
{
if (unlikely(ehci->ops->frame_index))
return ehci->ops->frame_index(ehci_to_dev(ehci));
return ehci_readl(ehci, &ehci->regs->frame_index);
}
> +#include "ehci-dbg.c"
> +
> /*-------------------------------------------------------------------------*/
>
> /*
> @@ -338,7 +353,7 @@ static void ehci_silence_controller(stru
> * This forcibly disables dma and IRQs, helping kexec and other cases
> * where the next system software may expect clean state.
> */
> -static void ehci_shutdown(struct usb_hcd *hcd)
> +void ehci_shutdown(struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>
> @@ -352,8 +367,9 @@ static void ehci_shutdown(struct usb_hcd
>
> hrtimer_cancel(&ehci->hrtimer);
> }
> +EXPORT_SYMBOL_GPL(ehci_shutdown);
>
> -static void ehci_port_power (struct ehci_hcd *ehci, int is_on)
> +void ehci_port_power(struct ehci_hcd *ehci, int is_on)
> {
> unsigned port;
>
> @@ -370,6 +386,7 @@ static void ehci_port_power (struct ehci
> ehci_readl(ehci, &ehci->regs->command);
> msleep(20);
> }
> +EXPORT_SYMBOL_GPL(ehci_port_power);
>
> /*-------------------------------------------------------------------------*/
>
> @@ -411,7 +428,7 @@ static void ehci_work (struct ehci_hcd *
> /*
> * Called when the ehci_hcd module is removed.
> */
> -static void ehci_stop (struct usb_hcd *hcd)
> +void ehci_stop(struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
>
> @@ -451,6 +468,7 @@ static void ehci_stop (struct usb_hcd *h
> dbg_status (ehci, "ehci_stop completed",
> ehci_readl(ehci, &ehci->regs->status));
> }
> +EXPORT_SYMBOL_GPL(ehci_stop);
>
> /* one-time init, only for memory state */
> static int ehci_init(struct usb_hcd *hcd)
> @@ -575,7 +593,7 @@ static int ehci_init(struct usb_hcd *hcd
> }
>
> /* start HC running; it's halted, ehci_init() has been run (once) */
> -static int ehci_run (struct usb_hcd *hcd)
> +int ehci_run(struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> u32 temp;
> @@ -659,8 +677,9 @@ static int ehci_run (struct usb_hcd *hcd
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(ehci_run);
>
> -static int ehci_setup(struct usb_hcd *hcd)
> +int ehci_setup(struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> int retval;
> @@ -691,10 +710,11 @@ static int ehci_setup(struct usb_hcd *hc
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(ehci_setup);
>
> /*-------------------------------------------------------------------------*/
>
> -static irqreturn_t ehci_irq (struct usb_hcd *hcd)
> +irqreturn_t ehci_irq(struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> u32 status, masked_status, pcd_status = 0, cmd;
> @@ -842,6 +862,7 @@ dead:
> usb_hcd_poll_rh_status(hcd);
> return IRQ_HANDLED;
> }
> +EXPORT_SYMBOL_GPL(ehci_irq);
>
> /*-------------------------------------------------------------------------*/
>
> @@ -857,7 +878,7 @@ dead:
> * NOTE: control, bulk, and interrupt share the same code to append TDs
> * to a (possibly active) QH, and the same QH scanning code.
> */
> -static int ehci_urb_enqueue (
> +int ehci_urb_enqueue(
> struct usb_hcd *hcd,
> struct urb *urb,
> gfp_t mem_flags
> @@ -893,12 +914,12 @@ static int ehci_urb_enqueue (
> return sitd_submit (ehci, urb, mem_flags);
> }
> }
> +EXPORT_SYMBOL_GPL(ehci_urb_enqueue);
>
> /* remove from hardware lists
> * completions normally happen asynchronously
> */
> -
> -static int ehci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
> +int ehci_urb_dequeue(struct usb_hcd *hcd, struct urb *urb, int status)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> struct ehci_qh *qh;
> @@ -963,13 +984,13 @@ done:
> spin_unlock_irqrestore (&ehci->lock, flags);
> return rc;
> }
> +EXPORT_SYMBOL_GPL(ehci_urb_dequeue);
>
> /*-------------------------------------------------------------------------*/
>
> // bulk qh holds the data toggle
>
> -static void
> -ehci_endpoint_disable (struct usb_hcd *hcd, struct usb_host_endpoint *ep)
> +void ehci_endpoint_disable(struct usb_hcd *hcd, struct usb_host_endpoint *ep)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> unsigned long flags;
> @@ -1040,9 +1061,9 @@ idle_timeout:
> ep->hcpriv = NULL;
> spin_unlock_irqrestore (&ehci->lock, flags);
> }
> +EXPORT_SYMBOL_GPL(ehci_endpoint_disable);
>
> -static void
> -ehci_endpoint_reset(struct usb_hcd *hcd, struct usb_host_endpoint *ep)
> +void ehci_endpoint_reset(struct usb_hcd *hcd, struct usb_host_endpoint *ep)
> {
> struct ehci_hcd *ehci = hcd_to_ehci(hcd);
> struct ehci_qh *qh;
> @@ -1081,12 +1102,14 @@ ehci_endpoint_reset(struct usb_hcd *hcd,
> }
> spin_unlock_irqrestore(&ehci->lock, flags);
> }
> +EXPORT_SYMBOL_GPL(ehci_endpoint_reset);
>
> -static int ehci_get_frame (struct usb_hcd *hcd)
> +int ehci_get_frame(struct usb_hcd *hcd)
> {
> struct ehci_hcd *ehci = hcd_to_ehci (hcd);
> return (ehci_read_frame_index(ehci) >> 3) % ehci->periodic_size;
> }
> +EXPORT_SYMBOL_GPL(ehci_get_frame);
>
> /*-------------------------------------------------------------------------*/
>
> @@ -1096,7 +1119,7 @@ static int ehci_get_frame (struct usb_hc
>
> /* These routines handle the generic parts of controller suspend/resume */
>
> -static int __maybe_unused ehci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> +int ehci_suspend(struct usb_hcd *hcd, bool do_wakeup)
> {
> struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>
> @@ -1119,9 +1142,10 @@ static int __maybe_unused ehci_suspend(s
>
> return 0;
> }
> +EXPORT_SYMBOL_GPL(ehci_suspend);
>
> /* Returns 0 if power was preserved, 1 if power was lost */
> -static int __maybe_unused ehci_resume(struct usb_hcd *hcd, bool hibernated)
> +int ehci_resume(struct usb_hcd *hcd, bool hibernated)
> {
> struct ehci_hcd *ehci = hcd_to_ehci(hcd);
>
> @@ -1182,11 +1206,64 @@ static int __maybe_unused ehci_resume(st
>
> return 1;
> }
> +EXPORT_SYMBOL_GPL(ehci_resume);
>
> #endif
>
> /*-------------------------------------------------------------------------*/
>
> +/*
> + * Generic structure; platform drivers can copy this and override individual
> + * entries as needed.
> + */
> +
> +const struct hc_driver ehci_hc_driver = {
IMHO, this should be static. If we want to allow platform bridge drivers
to override some of these fields (due to some silicon errata, for
instance), then they would have to pass proper function pointers and we
would always:
static int ehci_$field(struct ehci_hcd *ehci)
{
if (unlikely(ehci->ops->$field))
return ehci->ops->$field(ehci_to_dev(ehci);
/* otherwise, fallback to default implementation */
return 0;
}
--
balbi
Attachment:
signature.asc
Description: Digital signature
