Hi,
On Tue, Aug 23, 2011 at 10:27:58AM -0400, Alan Stern wrote:
> On Mon, 22 Aug 2011, Sarah Sharp wrote:
>
> > On Mon, Aug 15, 2011 at 04:51:27PM +0200, Sebastian Andrzej Siewior wrote:
> > > usb_hcd_pci_pm_ops provides run-time functionality for the USB HCDs.
> > > It expects that driver's data is a struct usb_hcd. This does not work
> > > if we create the HCD device is a platform device and its parent is the
> > > PCI device. We create the pci_dev from device and usb_hcd from parent's
> > > driver data.
> > > This patch is preparation to let the xhci core work under a platform
> > > device.
> >
> > This patch looks a bit messy, with all the nearly-duplicate function
> > names like check_root_hub_suspended and _check_root_hub_suspended.
> >
> > I think the code might be more readable if check_root_hub_suspended was
> > renamed to check_common_root_hub_suspended and you added two new
> > functions: the check_plat_pci_root_hub_suspended function you already
> > have, and another function called check_pci_root_hub_suspended.
> > You could do similar things with all the functions in this patch.
> >
> > Alan, what do you think?
>
> There's no doubt that the patch is messy.
>
> This is because it tries to cater to two different approaches
> simultaneously: the old (use the PCI device directly) and the new
> (interpose a fake platform device below the PCI device). It would be
I wouldn't call it fake. You _do_ have an IP core with a PCIe bridge
around and that's what the patch tries to model. If it was the best
implementation, then we can discuss but the fact that we have TWO
devices on such PCIe cards is irrefutable. Same goes for EHCI/OHCI/UHCI
cores and those need to be changed too.
> better to settle on a single approach.
>
> Personally, I think it's more than a little odd to have the fake
> platform device in the middle like that. It also runs the risk of
> messing up existing userspace programs that do power management on the
> controller device; they might easily contain references to paths like
> /sys/bus/usb/devices/usb3/../power/. Certainly I have done things like
> that in the past.
This would mean those userspace programs are actually broken if they
have hard-coded paths instead of trying to find the correct devices.
All in all, we are trying to prevent that big pile of ifdeferry we have
on e.g. ehci-hcd.c. This patchset will also allow for a more
object-oriented approach to driver development:
the PCI glue layer will be a parent device to xhci-core and, for
example, if we talk about PM, xhci-core only needs to handle
XHCI-specific PM and PCI glue only handles PCI-specific PM.
> Is there a better way to accomplish the desired effect? Not just for
> this one patch but for the entire series?
add the same ifdef hell we have on ehci-hcd.c
> For example, maybe the core parts of xhci-hcd could be made into a
> standalone module, containing routines that would be called by separate
> driver modules (one for xhci-pci and others for various platform
> variants). There would be a disadvantage, because now two modules
> would have to be loaded to manage an xHCI controller, but overall it
> would be cleaner.
I don't think so. I think your approach is inverted. Why should a PCI
glue/bridge call into XHCI ? All XHCI needs is a memory base and an IRQ
number (well, quirks, PM, etc) and that can be easily passed by the "bus
integration context" which I'm calling PCI glue/bridge on this mail.
> And of course, the same approach could be applied to the other joint
> PCI/platform HCDs.
you're only thinking about PCI and for embedded platforms we don't have
PCI. Most will have some OCP, or AXI/AHB bus and if we don't decouple
PCI from the host stack we will have the current ifdef mess we're into.
Let's say we have just achieve single zImage ARM kernel and we're all
standing in awe to the usefulness of having a single ARM kernel, until
we look into USB Host side:
<snippet of ehci-hcd.c>
| #ifdef CONFIG_USB_EHCI_FSL
| #include "ehci-fsl.c"
|
| #ifdef CONFIG_USB_EHCI_MXC
| #include "ehci-mxc.c"
| #define PLATFORM_DRIVER ehci_mxc_driver
| #endif
|
| #ifdef CONFIG_USB_EHCI_SH
| #include "ehci-sh.c"
| #define PLATFORM_DRIVER ehci_hcd_sh_driver
| #endif
|
| #ifdef CONFIG_SOC_AU1200
| #include "ehci-au1xxx.c"
| #define PLATFORM_DRIVER ehci_hcd_au1xxx_driver
| #endif
|
| #ifdef CONFIG_USB_EHCI_HCD_OMAP
| #include "ehci-omap.c"
| #define PLATFORM_DRIVER ehci_hcd_omap_driver
| #endif
|
| #ifdef CONFIG_PLAT_ORION
| #include "ehci-orion.c"
| #define PLATFORM_DRIVER ehci_orion_driver
| #endif
|
| #ifdef CONFIG_ARCH_IXP4XX
| #include "ehci-ixp4xx.c"
| #define PLATFORM_DRIVER ixp4xx_ehci_driver
| #endif
|
| #ifdef CONFIG_USB_W90X900_EHCI
| #include "ehci-w90x900.c"
| #define PLATFORM_DRIVER ehci_hcd_w90x900_driver
| #endif
|
| #ifdef CONFIG_ARCH_AT91
| #include "ehci-atmel.c"
| #define PLATFORM_DRIVER ehci_atmel_driver
| #endif
|
| #ifdef CONFIG_USB_OCTEON_EHCI
| #include "ehci-octeon.c"
| #define PLATFORM_DRIVER ehci_octeon_driver
| #endif
|
| #ifdef CONFIG_USB_CNS3XXX_EHCI
| #include "ehci-cns3xxx.c"
| #define PLATFORM_DRIVER cns3xxx_ehci_driver
| #endif
|
| #ifdef CONFIG_ARCH_VT8500
| #include "ehci-vt8500.c"
| #define PLATFORM_DRIVER vt8500_ehci_driver
| #endif
|
| #ifdef CONFIG_PLAT_SPEAR
| #include "ehci-spear.c"
| #define PLATFORM_DRIVER spear_ehci_hcd_driver
| #endif
|
| #ifdef CONFIG_USB_EHCI_MSM
| #include "ehci-msm.c"
| #define PLATFORM_DRIVER ehci_msm_driver
| #endif
|
| #ifdef CONFIG_USB_EHCI_HCD_PMC_MSP
| #include "ehci-pmcmsp.c"
| #define PLATFORM_DRIVER ehci_hcd_msp_driver
| #endif
|
| #ifdef CONFIG_USB_EHCI_TEGRA
| #include "ehci-tegra.c"
| #define PLATFORM_DRIVER tegra_ehci_driver
| #endif
|
| #ifdef CONFIG_USB_EHCI_S5P
| #include "ehci-s5p.c"
| #define PLATFORM_DRIVER s5p_ehci_driver
| #endif
|
| #ifdef CONFIG_USB_EHCI_ATH79
| #include "ehci-ath79.c"
| #define PLATFORM_DRIVER ehci_ath79_driver
| #endif
|
| #ifdef CONFIG_SPARC_LEON
| #include "ehci-grlib.c"
| #define PLATFORM_DRIVER ehci_grlib_driver
| #endif
</snippet>
Granted, not all of the above are ARM-based, but clearly we will have to
choose which one of the platforms will have EHCI supported. With the
approach we're proposing e.g. ehci-omap.c will become something like:
pdev = platform_device_alloc("ehci-omap", -1);
platform_device_add_resources(res, ARRAY_SIZE(res));
platform_device_add(pdev);
and ehci-hcd.c would have its resources/irqs which would allow it to
work.
If we don't stop coupling the USB stack with PCI, Linux will always be
very difficult for the embedded folks. Ok, I'm biased because I'm one of
the embedded folks and I want less work on my side, but decoupling Host
side from PCI will not create any extra maintenance burden for you
and/or Sarah, so why not ?
--
balbi
Attachment:
signature.asc
Description: Digital signature
