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 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.
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 explicitly about overrides. (Whether this
information is transmitted via platform data or by some other method
seems to me like an unimportant implementation detail.)
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.
> > #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 ?
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.
> 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);
> }
...
> > /*-------------------------------------------------------------------------*/
> >
> > +/*
> > + * 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;
> }
Let's see if this can be improved...
Each time your ehci_$field() runs, it tests ehci->ops->$field. Let's
eliminate the test by storing a pointer to ehci_$field in
ehci->ops->$field (whenever it's not overridden) and then always
calling ehci->ops->$field instead of ehci_$field or
ehci->driver->$field.
Now what do we have? ehci->ops is a large table of function
pointers... It looks an awful lot like struct hc_driver. The
ehci->ops table for the ehci-pci platform driver, for example, would be
almost exactly the same as the ehci_pci_hc_driver structure that is
defined in the current ehci-pci.c file.
There only two differences. One is that your ops table may contain
function pointers that don't exist in the hc_driver structure. The
other is that your ops table has entries that may be set dynamically
(by sticking platform_data stuff into the table) instead of
statically.
I don't think the difference between static and dynamic initialization
is all that important. One may be easier to read than the other, one
may be easier to change than the other. But the table is set up only
once, when the module is loaded, so the time and space spent doing the
initialization don't matter.
This leaves the matter of ops table entries that don't exist as
function pointers in the hc_driver structure. (Things like the
ehci_read_frame_pointer() routine you pointed out above.) It seems to
me that these things can always be added or changed in a separate set
of patches after this conversion to multiple platform-specific modules
is done. So I don't think they are really relevant to the conversion.
(I also don't think there will be more than a couple of them.)
On Tue, 2 Oct 2012, Felipe Balbi wrote:
> > +static int __init ehci_pci_init(void)
> > +{
> > + if (usb_disabled())
> > + return -ENODEV;
> > +
> > + pr_info("%s: " DRIVER_DESC "\n", hcd_name);
> > +
> > + /* Copy the generic driver, then add our own functions and overrides */
> > + ehci_pci_hc_driver = ehci_hc_driver;
> > + ehci_pci_hc_driver.description = hcd_name;
> > + ehci_pci_hc_driver.reset = ehci_pci_setup;
> > + ehci_pci_hc_driver.pci_suspend = ehci_pci_suspend;
> > + ehci_pci_hc_driver.pci_resume = ehci_pci_resume;
> > + ehci_pci_hc_driver.update_device = ehci_update_device;
>
> 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.
One is like the code above. Another is the way the existing code does
it (which would require all those exports you hate).
Maybe we can compromise on a third way. It turns out that almost all
the fields in the hc_driver structure are the same across all the
platform drivers (if you ignore ehci-tegra). Here are the exceptions,
gleaned from a quick scan through the driver source files:
Every driver has its own .product_desc string.
ehci-fsl and ehci-spear override hcd_priv_size so that they
can add their own data alongside the ehci_hcd structure.
We should make this easy to do because some other drivers
could stand to use the same technique.
Most drivers override the .reset pointer. I hope that in
time this will become less common, but for now we have to
live with it.
ehci-pmcmsp overrides .irq. I'm not sure that this is
really necessary; it doesn't look like it.
ehci-pci sets .pci_suspend and .pci_resume.
ehci-pci and ehci-vt8500 set .update_device.
ehci-xilinx explicitly avoids setting .relinquish_port and
.port_handed_over. The driver would work perfectly well with
those two pointers set, particularly if the two routines
checked the has_tt flag (since Xilinx's controller does have an
integrated TT and those routines matter only for controllers
with a FS/LS companion).
This suggests each platform driver should define a static table with
entries for product_desc, hcd_priv_size, reset, pci_suspend,
pci_resume, and update_device. The EHCI core could then take this
static table and merge it with the generic ehci_hc_driver to create a
platform-specific hc_driver. Very little would need to be exported.
Or if you prefer, I could leave pci_suspend and pci_resume out of the
table. ehci-pci could add them to the hc_driver just like in the code
above.
> PCI suspend and resume shouldn't be
> exposed to ehci core layer, btw. If we make ehci-pci's device a parent
> to ehci core's device (should we add one), PM core will already make
> sure PCI's suspend is only called after EHCI is suspended, so we can
> properly split EHCI-suspend from PCI-suspend. In fact, looking at the
> code, there's nothing PCI-specific about ehci_pci_suspend(). It just
> calls ehci_suspend(), which shouldn't be exported. In that case, we
> don't need pci-suspend/pci-resume at all.
The .pci_suspend and .pci_resume members seem especially strange
because you're not familiar with how they are used. (Regarding your
second-to-last sentence: ehci_suspend() is one example of the routines
I mentioned above, where a platform driver needs to call the generic
core. But you're right that ehci_pci_suspend() itself could be
eliminated and replaced with a pointer to ehci_suspend().)
It's true that these two members of hc_driver are oddities. They don't
really belong there -- they are explicitly specific to a single bus
type, as you can see from their names. In principle they could be
removed, although doing this would make some other things more awkward.
As for splitting EHCI-suspend from PCI-suspend... I suppose we could.
But I don't see any real advantage in it. And there is a definite
disadvantage in that it would change the sysfs device hierarchy, which
could annoy people (it might mess up powertop, for example).
> ps: as a bonus to changing as suggested above, we would be able to use
> module_pci_driver() :-p
True... But that doesn't sway the balance, I'm afraid. :-)
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
