Re: [RFC 1/2] Convert ehci-hcd to a library
On Wed, 3 Oct 2012, Felipe Balbi wrote:
> > There will be a few cases where the platform code needs to call a core
> > routine. So we'd still need to export a few routines, but not nearly
> > as many as I did here.
>
> I think there shouldn't be.. well, unless we need to apply some very
> specific workaround like OMAP's port suspend workaround where we need to
> switch a clock parent. In that case we need to add code to hub_control()
> and the best way is to do our workaround thing, then call the generic
> hub_control()
Specifically, I was thinking of ehci_suspend, ehci_resume, ehci_setup,
and ehci_port_power.
Adding another layer to the device hierarchy might allow us to avoid
exporting ehci_suspend and ehci_resume, but doing that would be a
major change visible to userspace and I'm not convinced it would be
for the best. (Also, there is the special call to ehci_pci_reinit in
ehci_pci_resume _after_ ehci_resume runs -- that would be difficult to
handle. Other platform drivers may have similar requirements.)
Some drivers need to change a few values in the ehci_hcd structure
after ehci_setup runs. Exporting it seems to be the easiest way to
accomplish this.
The port power stuff almost certainly isn't needed. The EHCI core
driver and usbcore are responsible for managing port power anyway; the
platform drivers shouldn't be concerned with it. I'm planning on
removing it from the platform drivers at some point -- but for now
they use it.
> > > > +#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 ?
> >
> > This isn't as significant as it appears. The "#ifdef CONFIG_PCI" part
> > is just a minor optimization, because this particular quirk affects
> > only PCI controllers. We could get rid of the inline routine entirely
> > and use the out-of-line ehci_read_frame_index() routine on all
> > platforms.
>
> that'll prevent certain compiler optimizations to happen, no ? I mean,
> do you think GCC would still inline the cases where it's really just a
> register read (which is just one instruction on ARM) ?
No; you're right.
> > > 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);
> > > }
That also is more than a single instruction, because it involves a
test. But the scheme could be made to work easily enough for all
platforms, although I would do it slightly differently:
static inline unsigned ehci_read_frame_index(struct ehci_hcd *ehci)
{
if (unlikely(ehci->frame_index_bug))
return ehci_moschip_read_frame_index(ehci);
return ehci_readl(ehci, &ehci->regs->frame_index);
}
This, together with moving the ehci_info() etc. macros from ehci-dbg.c
to ehci.h are simple cleanups that can be done before the real
conversion.
Another such cleanup involves the Link Power Management code. I don't
know why ehci-lpm.c is always included in the build even though it
gets used only by ehci-pci.c and ehci-vt8500.c. On the other hand, it
doesn't seem to be at all system specific; LPM is part of the official
spec. The ehci_update_device routine in ehci-pci.c, for example,
would work on any system (although most systems don't implement LPM
and so would set ehci->has_lpm to 0). Done properly, we wouldn't need
to export any of the routines in ehci-lpm.c.
> > > I would rather see ehci-pci.c passing a "ops" structure with a set of
> > > function pointers to the core layer.
> >
> > As pointed out above, ehci_pci_hc_driver essentially _is_ such an "ops"
> > structure. I can think of a few ways to initialize these structures.
>
> not really, because that's som
Re: [RFC 1/2] Convert ehci-hcd to a library
Hi,
On Tue, Oct 02, 2012 at 04:07:08PM -0400, Alan Stern wrote:
> On Tue, 2 Oct 2012, Felipe Balbi wrote:
>
> > > --- 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 )
>
> That can be changed separately if anybody cares. This was just the
> result of simply moving code from ehci-dbg.c to ehci.h.
agreed.
> > > #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_driverehci_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
> > over
Re: [RFC 1/2] Convert ehci-hcd to a library
On Tue, 2 Oct 2012, Felipe Balbi wrote:
> > --- 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 )
That can be changed separately if anybody cares. This was just the
result of simply moving code from ehci-dbg.c to ehci.h.
> > #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 voidehci_stop(struct usb_hcd *hcd);
> > +extern voidehci_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 voidehci_endpoint_disable(struct usb_hcd *hcd,
> > + struct usb_host_endpoint *ep);
> > +extern voidehci_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 voidehci_relinquish_port(struct usb_hcd *hcd, int portnum);
> > +extern int ehci_port_handed_over(struct usb_hcd *hcd, int portnum);
> > +extern voidehci_clear_tt_buffer_complete(struct usb_hcd *hcd,
> > + struct usb_host_endpoint *ep);
> > +
> > +extern voidehci_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_resumeNULL
> > +#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.
Let's see if I understand all this. You don't want the platform
drivers to define their own hc_driver structures; instead you want the
hc_driver structure to be set up by the core and have the platform
drivers inform the core exp
Re: [RFC 1/2] Convert ehci-hcd to a library
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_driverehci_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
