tegra: seaboard: Initialize multiple USB controllers at once

PATCH New Jim Lin 3 participants 3 messages Jun 14, 2012

Subsystems (4)

Arm (32-bit) USB Tegra architecture support USB ehci driver

lore

Share & tools

lore

Stephen Warren

2012-06-14 18:01 UTC #1 lore raw

PATCH 1/1 tegra: seaboard: Initialize multiple USB controllers at once

Quoted 3 lines · Jim Lin

Add support for command line "usb reset" or "usb start" to initialize , "usb stop" to stop multiple USB controllers at once. Other commands like "usb tree" also support multiple controllers.

These patches also need to be sent to the USB maintainer since they touch core USB code. (Now CC'd)

Rather than one mega-patch that touch the USB core, and the Tegra driver, can't the patch be split up a bit so that one patch adds the ability to support multiple controllers, and then another patch modifies the Tegra USB driver to support this, etc. Many smaller patches are much easier to review.

This patch adds a huge number of ifdefs. I'm sure many of them can be removed completely. For example, in the following code, why not /always/ get the rootdev from dev->controller->rootdev, and remove the global variable. I would guess that the code size increase would be extremely minimal, but it would make the code a lot more maintainable by removing all the ifdefs.

Quoted 12 lines

submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
int length, struct devrequest *setup)
{

drivers/usb/host/ehci-hcd.c +3 -0

+#ifdef CONFIG_USB_INIT_MULTI
+       struct ehci_ctrl *ctrl = dev->controller;
+#endif
 
        if (usb_pipetype(pipe) != PIPE_CONTROL) {
                debug("non-control pipe (type=%lu)", usb_pipetype(pipe));
                return -1;
        }

drivers/usb/host/ehci-hcd.c int · line 1094

Quoted 10 lines

drivers/usb/host/ehci-hcd.c +5 -0

+#ifdef CONFIG_USB_INIT_MULTI
+       if (usb_pipedevice(pipe) == ctrl->rootdev) {
+               if (ctrl->rootdev == 0)
+#else
        if (usb_pipedevice(pipe) == rootdev) {
                if (rootdev == 0)
+#endif
                        dev->speed = USB_SPEED_HIGH;
                return ehci_submit_root(dev, pipe, buffer, length, setup);
        }

The patch above removes the use of the global variable "rootdev", but I don't see anywhere that ifdef's that variable out of existence. Not doing so would allow code to accidentally use the global when it should be using the per-device value; how can you be sure you've patched all the places in the code that you need to? What about future changes to the code by people who aren't aware of the USB_INIT_MULTI feature?

Similarly, why not just outright change the prototype of tegrausb_stop_port() so that it always takes a port number; the existing calls can hard-code a port ID of 0 for compatibility:

Quoted 12 lines

drivers/usb/host/ehci-tegra.c +7 -0

+#ifdef CONFIG_USB_INIT_MULTI
+int ehci_hcd_stop(int index)
+{
+       tegrausb_stop_port(index);
+       return 0;
+}
+#else
 int ehci_hcd_stop(void)
 {
        tegrausb_stop_port();
        return 0;
 }

Quoted 1 line

Quoted 1 line
+#endif

In the end, I think if you rework the patch to remove all/most of the ifdefs, the only thing that's left will be that the loop to initialize USB would loop over either just device 0 or devices 0..n-1, and even that wouldn't need to be ifdef'd...

Marek Vasut

2012-06-14 18:47 UTC #2 lore raw

PATCH 1/1 tegra: seaboard: Initialize multiple USB controllers at once

Dear Stephen Warren,

Quoted 7 lines · Jim Lin

Quoted 3 lines
Add support for command line "usb reset" or "usb start" to initialize , "usb stop" to stop multiple USB controllers at once. Other commands like "usb tree" also support multiple controllers.
These patches also need to be sent to the USB maintainer since they touch core USB code. (Now CC'd)

Ok, waiting for smaller patches indeed,

Best regards,
Marek Vasut