On 06/13/2012 10:17 PM, Jim Lin wrote:
> 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.
> submit_control_msg(struct usb_device *dev, unsigned long pipe, void *buffer,
> int length, struct devrequest *setup)
> {
> +#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;
> }
>
> +#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:
> +#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;
> }
>> +#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...
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
