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_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.)
That's right.
> 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()
> > > #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.
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) ?
> > 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.
maybe we don't need to add all fields now and only add the ones which
are currently being overwritten.
> 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.
fair enough.
> 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.)
Ok, sounds good
> 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.
not really, because that's something ehci core should be passing to
usbcore, that's the language they talk. If we need to change the
implementation of what of EHCI specification says, we need to do it
between ehci core and ehci-$platform.
> One is like the code above. Another is the way the existing code does
> it (which would require all those exports you hate).
I "hate" those for a reason. My experience with the MUSB driver taught
me that the more core functionality we expose, the more it will be
abused by users. It also taught me a different way of doing things
considering how the HW is actually layed out (a 'generic' IP with a
'wrapper' IP around it, so two HW entities), which turned out to give
a lot of functionality for free (PM encapsulation, address space
'isolation'[1], easier to read code with proper split between 'generic'
and company-specific details, etc).
Look at drivers/usb/dwc3/ and you will clearly see what I mean. What
core functionality is built into dwc3.ko and dwc3-$plat.ko is just a
shim layer which prepares the 'wrapper' to make dwc3 work.
[1] by isolation I mean that e.g. EHCI's registers shouldn't be accessed
by ehci-omap.c (ideally)
> 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,
this is what I mean, actually :-) Most of them just re-use generic ehci
routines.
> 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.
this could also be done differently. If EHCI gets its own device, that
won't be necessary and ehci-spear/fsl will be able to
platform_set_drvdata(pdev, spear/fsl), which will give them a way to
manage their own data without polluting EHCI's internal data.
> 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.
I looked into those already, most of them have one form of:
ehci->caps = hcd->regs + offset; /* offset can be zero */
hcd->has_tt = true;
ehci_setup();
ehci_port_power();
a few of them will have some extra bits and pieces here and there. OMAP,
for instance does a PHY reset, which shouldn't be done there anyway, it
should be done by a PHY driver (drivers/usb/phy/) and ehci core should
just call usb_phy_reset(); or something similar.
> ehci-pmcmsp overrides .irq. I'm not sure that this is
> really necessary; it doesn't look like it.
you're right here. As long as ehci properly checks the STS register, we
should be fine, right ?
> ehci-pci sets .pci_suspend and .pci_resume.
at least ehci_pci_suspend isn't needed. It just call the generic
ehci_suspend().
> 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).
that's right.
> 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.
It looks to me that update_device could be made generic. The underlying
platform doesn't need to know about a new device being addressed, ehci
core does. Am I missing something ?
> 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.
Or maybe just convert pci_resume/pci_suspend into generic resume/suspend
and let pci pass its own implementation there ?
> > 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).
Ok, then I withdraw my comments about pci_suspend/pci_resume. They can
either be left as they are, or do what you suggested above.
> > 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. :-)
that's too bad, I really thought I'd convince you with that :-p heh
--
balbi
Attachment:
signature.asc
Description: Digital signature
