Hi,
On Fri, Sep 02, 2011 at 10:52:55AM -0400, Alan Stern wrote:
> > let udev do its thing:
> >
> > - arch code will add a platform_device to the driver model, e.g.:
> > platform_device_register(&omap_ehci_device);
> >
> > - based on the device add uevent, udev will load the correct module (in
> > this case ehci-omap.ko)
> >
> > - ehci-omap.ko instantiates and add EHCI-core platform_device:
> > ehci = platform_device_alloc("ehci", -1);
> > platform_device_add(ehci);
> >
> > - based on the device add uevent, udev will load the correct module (in
> > this case ehci-hcd.ko)
>
> The end result of this would be:
>
> usbcore <--> usb_hcd <--> ehci <--> ehci_omap <--> omap-usb-host
ehci-omap can vanish... omap-usb-host handles the platform-specific
part.
> > Just look into the host stack and you should be able to see all the
> > problems I have listed:
> >
> > . too many exported functions
>
> What exported functions?
>
> $ cd drivers/usb/host
> $ grep -l EXPORT *.c
> octeon2-common.c
> pci-quirks.c
> sl811-hcd.c
> $
>
> None of those seem particularly bad. And your proposal wouldn't
> remove them.
oh... it's uglier than I thought actually. Just now I noticed that
ehci-hcd is including the C files... So, my approach would get rid of
that too.
> > . lots of ifdefs
>
> How would you feel if those #ifdefs were moved from ehci-hcd.c into
> a .h file? Many header files are littered with #ifdefs.
that doesn't _fix_ the problem. It's just pushing it to another
location.
> > . every architecture needs to add some ?hci-<arch>.c which does
> > essentially the same.
>
> In principle, some architectures could share this code. But how much
> of it really is the same? Most of the commonality lies in the probe
> routines. The drivers locate the necessary resources and register
> their hcd structures.
>
> Now, locating resources is clearly platform-specific. Registering
> the hcd and defining the hc_driver structure are the only truly
> redundant parts.
the EHCI-part is the same for all of them. Ok, I'll took ehci-atmel and
here's a sample patch removing all things which are generic. Of course
this won't work but it shows the amount of duplicated code on that file
alone:
diff --git a/drivers/usb/host/ehci-atmel.c b/drivers/usb/host/ehci-atmel.c
index a5a3ef1..b98b9eb 100644
--- a/drivers/usb/host/ehci-atmel.c
+++ b/drivers/usb/host/ehci-atmel.c
@@ -80,94 +80,16 @@ static int ehci_atmel_setup(struct usb_hcd *hcd)
return retval;
}
-static const struct hc_driver ehci_atmel_hc_driver = {
- .description = hcd_name,
- .product_desc = "Atmel EHCI UHP HS",
- .hcd_priv_size = sizeof(struct ehci_hcd),
-
- /* generic hardware linkage */
- .irq = ehci_irq,
- .flags = HCD_MEMORY | HCD_USB2,
-
- /* basic lifecycle operations */
- .reset = ehci_atmel_setup,
- .start = ehci_run,
- .stop = ehci_stop,
- .shutdown = ehci_shutdown,
-
- /* managing i/o requests and associated device resources */
- .urb_enqueue = ehci_urb_enqueue,
- .urb_dequeue = ehci_urb_dequeue,
- .endpoint_disable = ehci_endpoint_disable,
- .endpoint_reset = ehci_endpoint_reset,
-
- /* scheduling support */
- .get_frame_number = ehci_get_frame,
-
- /* root hub support */
- .hub_status_data = ehci_hub_status_data,
- .hub_control = ehci_hub_control,
- .bus_suspend = ehci_bus_suspend,
- .bus_resume = ehci_bus_resume,
- .relinquish_port = ehci_relinquish_port,
- .port_handed_over = ehci_port_handed_over,
-
- .clear_tt_buffer_complete = ehci_clear_tt_buffer_complete,
-};
-
static int __devinit ehci_atmel_drv_probe(struct platform_device *pdev)
{
- struct usb_hcd *hcd;
- const struct hc_driver *driver = &ehci_atmel_hc_driver;
- struct resource *res;
- int irq;
- int retval;
+ struct platform_device *ehci;
+ int retval;
if (usb_disabled())
return -ENODEV;
pr_debug("Initializing Atmel-SoC USB Host Controller\n");
- irq = platform_get_irq(pdev, 0);
- if (irq <= 0) {
- dev_err(&pdev->dev,
- "Found HC with no IRQ. Check %s setup!\n",
- dev_name(&pdev->dev));
- retval = -ENODEV;
- goto fail_create_hcd;
- }
-
- hcd = usb_create_hcd(driver, &pdev->dev, dev_name(&pdev->dev));
- if (!hcd) {
- retval = -ENOMEM;
- goto fail_create_hcd;
- }
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(&pdev->dev,
- "Found HC with no register addr. Check %s setup!\n",
- dev_name(&pdev->dev));
- retval = -ENODEV;
- goto fail_request_resource;
- }
- hcd->rsrc_start = res->start;
- hcd->rsrc_len = resource_size(res);
-
- if (!request_mem_region(hcd->rsrc_start, hcd->rsrc_len,
- driver->description)) {
- dev_dbg(&pdev->dev, "controller already in use\n");
- retval = -EBUSY;
- goto fail_request_resource;
- }
-
- hcd->regs = ioremap_nocache(hcd->rsrc_start, hcd->rsrc_len);
- if (hcd->regs == NULL) {
- dev_dbg(&pdev->dev, "error mapping memory\n");
- retval = -EFAULT;
- goto fail_ioremap;
- }
-
iclk = clk_get(&pdev->dev, "ehci_clk");
if (IS_ERR(iclk)) {
dev_err(&pdev->dev, "Error getting interface clock\n");
@@ -181,25 +103,38 @@ static int __devinit ehci_atmel_drv_probe(struct platform_device *pdev)
goto fail_get_fclk;
}
- atmel_start_ehci(pdev);
+ ehci = platform_device_alloc("ehci", -1);
+ if (!ehci) {
+ dev_err(&pdev->dev, "Error allocating EHCI device\n");
+ retval = -ENOMEM;
+ goto fail_alloc_ehci;
+ }
- retval = usb_add_hcd(hcd, irq, IRQF_SHARED);
- if (retval)
- goto fail_add_hcd;
+ retval = platform_device_add_resources(ehci, pdev->resource,
+ pdev->num_resources);
+ if (retval) {
+ dev_err(&pdev->dev, "Error copying resources\n");
+ goto fail_add_res;
+ }
+
+ retval = platform_device_add(ehci);
+ if (retval) {
+ dev_err(&pdev->dev, "Error adding EHCI device\n");
+ goto fail_add_res;
+ }
+
+ atmel_start_ehci(pdev);
return retval;
fail_add_hcd:
atmel_stop_ehci(pdev);
+fail_add_res:
+ platform_device_put(ehci);
+fail_alloc_ehci:
clk_put(fclk);
fail_get_fclk:
clk_put(iclk);
-fail_get_iclk:
- iounmap(hcd->regs);
-fail_ioremap:
- release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
-fail_request_resource:
- usb_put_hcd(hcd);
fail_create_hcd:
dev_err(&pdev->dev, "init %s fail, %d\n",
dev_name(&pdev->dev), retval);
@@ -211,12 +146,6 @@ static int __devexit ehci_atmel_drv_remove(struct platform_device *pdev)
{
struct usb_hcd *hcd = platform_get_drvdata(pdev);
- ehci_shutdown(hcd);
- usb_remove_hcd(hcd);
- iounmap(hcd->regs);
- release_mem_region(hcd->rsrc_start, hcd->rsrc_len);
- usb_put_hcd(hcd);
-
atmel_stop_ehci(pdev);
clk_put(fclk);
clk_put(iclk);
@@ -231,3 +160,14 @@ static struct platform_driver ehci_atmel_driver = {
.shutdown = usb_hcd_platform_shutdown,
.driver.name = "atmel-ehci",
};
+
+static int __init ehci_atmel_init(void)
+{
+ return platform_driver_register(&ehci_atmel_driver);
+}
+module_init(ehci_atmel_init);
+
+static void __exit ehci_atmel_exit(void)
+{
+ platform_driver_unregister(&ehci_atmel_driver);
+}
> > . ?hci-hcd stacks are PCI centric
>
> In what way? The only PCI-centric part of ehci-hcd is ehci-pci.c,
$ git grep -e "msi" drivers/usb/host
$ git grep -e "pci\|PCI" drivers/usb/host/ | grep -v xhci-pci | \
grep -v ehci-pci | grep -v ohci-pci | grep -v uhci-pci \
| grep -v isp1760 | grep -v pci-quirks | grep -v oxu210
> which is pretty understandable. Likewise for ohci-hcd and uhci-hcd.
>
> True, xhci-hcd is currently very PCI-centric. That's because until
> very recently there was no non-PCI xHCI hardware available.
Still, all other ?hci got implementations on embedded, so it would be at
least a good bet to make a driver which doesn't depend on PCI.
> Aside from xhci-hcd, AFAICT the only PCI-centric part in the entire USB
> stack is the pci_suspend and pci_resume fields in usb_hcd, and they can
see above.
> be eliminated easily enough. (hcd-pci.c doesn't count; it's only
> common routines shared among [eoux]hcd-pci.c. Likewise for
> pci-quirks.c -- neither of those files gets compiled if CONFIG_PCI is
> disabled.)
correct.
> > . ?hci-hcd PM handling is coupled with the underlying architecture
>
> Are you talking about the bus_suspend/bus_resume methods? The methods
> themselves are in the common code; only the method pointers are
> duplicated in the hc_driver structures.
It comes down to the other comment I had: if it's common code, why do
all *-$arch.c need to pass it ?
> > . single module available for a particular system, making "hybrid
> > systems" (let's say, and ARM-SoC with internal EHCI and PCIe
> > controller with discrete EHCI component) very difficult
>
> That's not right. If the kernel config defines both PCI and a
> platform driver, then both ehci-pci.c and the appropriate platform glue
> file will be #included in ehci-hcd.c. The resulting module will be
> able to drive both the internal EHCI and the external PCIe controller.
yes, you are right here.
> > . lack of pm_runtime on ?hci-hcd (it could be used for e.g.
> > bus_suspend/bus_resume)
>
> No, the runtime PM support _is_ present, but it is in usbcore instead
> in the host controller drivers.
We can use device hierarchy for that, no ? Instead of adding methods to
struct hc_driver, implement things on dev_pm_ops.
> > . ARCH needs to know about EHCI and EHCI needs to know about ARCH (not
> > very object orientation friendly)
> > . It doesn't really model the underlying HW
>
> It's possible to go too far in modelling the hardware. We don't need
> to introduce a data structure for every transistor, or even for every
> IP component.
not to every thing, but in this situation, modelling the PCI bridge and
the ?HCI-stack on separate devices seem to pay off.
> > I have no problems, as of today, with how EHCI stack talks to usbcore.
> > What I see as wrong is how ehci-<arch>.c is talking to EHCI stack.
>
> Here's a different approach which addresses all the problems you listed
> above -- and does so without adding extra layers to the device model.
> It's a lot like what you're suggesting, but without a bunch of
> extraneous stuff.
>
> Make each ehci-<arch>.c into a standalone module. Remove all the
> duplicated hc_drivers structures from these files, put a single
> publicly-available ehci_hc_driver in ehci-hcd.c, and make ehci-hcd a
> separate module containing just the core routines.
why making ehci_hc_driver publicly available ? It doesn't have to be.
If it's publicly available you could allow (now, the following is really
farfetched) someone to write a ehci-bomb.c file which passes bogus
function pointers to ehci-stack. And it will get executed.
If, on the other hand, ehci_hc_driver is static (and, if possible,
const) that will not happen. The point is, if it's
common/generic/standardized/whatever, why do you need to publicize it ?
> To make this work, we have to find a way to handle all the individual
> differences in the various hc_driver structures. Most of the
> differences lie in the product_desc and reset fields.
you can pass platform_data to ehci-stack core driver.
> We can get rid of product_desc altogether, and in its place use the
> parent controller's driver name. I'm not sure exactly what would be
yes, that's a good point. Make something like:
"%s Host Controller", dev_name(dev->parent)
> needed for the reset routines, but it mostly looks like we just need to
> refactor some of the startup code.
correct.
> A few drivers, like ehci-tegra.c, will be more troublesome. For such
> cases it might be better to have a private hc_driver structure with
> most of the entries copied from the public ehci_hc_driver.
I don't agree here. If it's only a few fields, you can pass them on a
struct ehci_platform_ops structure by using platform_data pointer on
struct device. Then do something like so:
ehci->ops = dev->platform_data->ops;
...
on reset you could:
if (ehci->ops->reset)
return ehci->ops->reset(dev);
return ehci_default_reset(ehci);
on platform ->reset implementation we could:
static int ehci_omap_reset(struct device *child)
{
struct ehci_omap *omap = dev_get_drvdata(child->parent);
do_ehci_omap_reset(omap);
return 0;
}
--
balbi
Attachment:
signature.asc
Description: Digital signature
