[U-Boot] OMAP4 u-boot broken (was [PATCH] usb: align usb_endpoint_descriptor to 16-bit boundary)

[Stefan Kristiansson]

Hi Aneesh,
On Tue, Dec 13, 2011 at 06:59:45PM +0530, Aneesh V wrote:
> OMAP4 U-Boot is broken in the mainline. U-Boot wouldn't boot up on any
> OMAP4 platforms. I suspect this will be the case with any ARM platform
> that has enabled USB tty code. I git-bisected the issue to this patch.
> I did some analysis on it and here are my findings.
> 
> aligned(2) indeed makes the sizeof(struct usb_endpoint_descriptor) ==
> 8. But that doesn't seem to be enough. struct acm_config_desc embeds
> fields of type usb_endpoint_descriptor and has the attribute 'packed'.
> As a result, these usb_endpoint_descriptor structures in
> acm_config_desc may have odd addresses and consequently wMaxPacketSize
> also has odd address. As far as I can see, this is not a new issue. But
> your patch fortunately or unfortunately brought it out. Here is how:
> 
> When 'usb_endpoint_descriptor' didn't have the 'aligned' attribute,
> compiler took extra care while accessing wMaxPacketSize. It did it
> using two byte reads and combining them to make a 16-bit half-word.
> When aligned was added compiler replaced it with a 'ldrh' (load
> half-word) instruction that resulted in an abort due the odd address.
> 

How unpleasent, nice that you were able to pinpoint where and how it fails
however.

> Now, I am not sure how to solve this problem. I tried the following and
> the boot issue is gone.
> 
> diff --git a/drivers/serial/usbtty.c b/drivers/serial/usbtty.c
> index e2e87fe..2a961e9 100644
> --- a/drivers/serial/usbtty.c
> +++ b/drivers/serial/usbtty.c
> @@ -151,7 +151,7 @@ struct acm_config_desc {
>  	/* Slave Interface */
>  	struct usb_interface_descriptor data_class_interface;
>  	struct usb_endpoint_descriptor data_endpoints[NUM_ENDPOINTS-1];
> -} __attribute__((packed));
> +};
> 
>  static struct acm_config_desc
> acm_configuration_descriptors[NUM_CONFIGS] = {
>  	{
> 
> I am not a USB expert, so not sure whether this works. Does that
> structure really have to be 'packed'? It will be great if USB experts
> can look into this and come up with a permanent solution at the
> earliest because quite a few platforms will be broken until then.
> 

I am neither a USB expert, just the moron that apperantly broke a lot
of OMAP4 boards.

The way I see it there are 3 options at hand here:
1) revert my patch and look over the places where wMaxPacketSize is used
and wrap them in get/set_unaligned() (at least in non-arch specific code)
2) go forward with your approach
3) stop using usb_endpoint_descriptor in arrays

I think option nr 1 is the safest one here, but yes,
input from USB experts would be great.

Stefan