[U-Boot] [RFC USB PATCH] USB ehci fix and test on ixp4xx hardware

[michael <trimarchi@gandalf.sssup.it>]

Hi,

Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 11:00 Fri 12 Dec     , Michael Trimarchi wrote:
>   
>> EHCI fix code and ixp4xx test.
>> USB ehci configuration parameter:
>>
>> #define CONFIG_CMD_USB          1
>> #define CONFIG_USB_STORAGE      1
>> #define CONFIG_USB_EHCI
>> #define CONFIG_USB_EHCI_IXP4XX          1
>> #define CONFIG_EHCI_IS_TDI              1
>> #define CONFIG_EHCI_DESC_BIG_ENDIAN     1
>> #define CONFIG_EHCI_MMIO_BIG_ENDIAN     1
>> #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 2
>> #define CONFIG_LEGACY_USB_INIT_SEQ      1
>>
>> 2 USB Device(s) found
>>        scanning bus for storage devices... 0 Storage Device(s) found
>> => usb tree
>>
>> Device Tree:
>>   1  Hub (1.5MBit/s, 0mA)
>>   |  u-boot EHCI Host Controller
>>   |
>>   |+-2  Mass Storage (12MBit/s, 100mA)
>>        Sony Storage Media 0C07040930296
>>
>> =>
>>
>> Signed-off-by: Michael Trimarchi <trimarchimichael@yahoo.it>
>>
>> ---
>>  drivers/usb/usb_ehci.h      |   49 ++++++++++--------
>>  drivers/usb/usb_ehci_core.c |  122 ++++++++++++++++++++++++++----------------
>>  2 files changed, 102 insertions(+), 69 deletions(-)
>>
>> diff --git a/drivers/usb/usb_ehci.h b/drivers/usb/usb_ehci.h
>> index 90b137a..9e8e7b2 100644
>> --- a/drivers/usb/usb_ehci.h
>> +++ b/drivers/usb/usb_ehci.h
>> @@ -1,5 +1,6 @@
>>  /*-
>>   * Copyright (c) 2007-2008, Juniper Networks, Inc.
>> + * Copyright (c) 2008, Michael Trimarchi <trimarchimichael@yahoo.it>
>>     
> please remove you do not do enough modification to claim the copytight
>   
Ok, I do few changes before, but if you think that they aren't a lot, 
I'll remove it.
>>   * All rights reserved.
>>   *
>>   * This program is free software; you can redistribute it and/or
>> @@ -21,6 +22,10 @@
>>  #ifndef USB_EHCI_H
>>  #define USB_EHCI_H
>>  
>> +#if !defined(CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS)
>> +#error "USB EHCI define MAX_ROOT_PORTS"
>> +#endif
>>     
> I'll prefer this
> #if !defined(CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS)
> #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS	2
> #endif
>
> other wise you must update everyone
>   
ok
>> +
>>  /* (shifted) direction/type/recipient from the USB 2.0 spec, table 9.2 */
>>  #define DeviceRequest \
>>  	((USB_DIR_IN | USB_TYPE_STANDARD | USB_RECIP_DEVICE) << 8)
>> @@ -45,10 +50,12 @@ struct ehci_hccr {
>>  #define HC_LENGTH(p)		(((p) >> 0) & 0x00ff)
>>  #define HC_VERSION(p)		(((p) >> 16) & 0xffff)
>>  	uint32_t cr_hcsparams;
>> +#define HCS_PPC(p)		((p) & (1 << 4))
>> +#define HCS_INDICATOR(p)        ((p) & (1 << 16)) /* Port indicators */
>>     
> 			   ^^^^^^^^
> whitespace please fix
>   
ok

>>  #define HCS_N_PORTS(p)		(((p) >> 0) & 0xf)
>>  	uint32_t cr_hccparams;
>>  	uint8_t cr_hcsp_portrt[8];
>> -};
>> +} __attribute__ ((packed));
>>  
>>  struct ehci_hcor {
>>  	uint32_t or_usbcmd;
>> @@ -71,9 +78,9 @@ struct ehci_hcor {
>>  	uint32_t _reserved_[9];
>>  	uint32_t or_configflag;
>>  #define FLAG_CF		(1 << 0)	/* true:  we'll support "high speed" */
>> -	uint32_t or_portsc[2];
>> +	uint32_t or_portsc[CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS];
>>  	uint32_t or_systune;
>> -};
>> +} __attribute__ ((packed));
>>  
>>  #define USBMODE		0x68		/* USB Device mode */
>>  #define USBMODE_SDIS	(1 << 3)	/* Stream disable */
>> @@ -123,26 +130,24 @@ struct usb_linux_config_descriptor {
>>  #define cpu_to_hc32(x)		cpu_to_le32((x))
>>  #endif
>>  
>> -#define EHCI_PS_WKOC_E		0x00400000	/* RW wake on over current */
>> -#define EHCI_PS_WKDSCNNT_E	0x00200000	/* RW wake on disconnect */
>> -#define EHCI_PS_WKCNNT_E	0x00100000	/* RW wake on connect */
>> -#define EHCI_PS_PTC		0x000f0000	/* RW port test control */
>> -#define EHCI_PS_PIC		0x0000c000	/* RW port indicator control */
>> -#define EHCI_PS_PO		0x00002000	/* RW port owner */
>> -#define EHCI_PS_PP		0x00001000	/* RW,RO port power */
>> -#define EHCI_PS_LS		0x00000c00	/* RO line status */
>> -#define EHCI_PS_PR		0x00000100	/* RW port reset */
>> -#define EHCI_PS_SUSP		0x00000080	/* RW suspend */
>> -#define EHCI_PS_FPR		0x00000040	/* RW force port resume */
>> -#define EHCI_PS_OCC		0x00000020	/* RWC over current change */
>> -#define EHCI_PS_OCA		0x00000010	/* RO over current active */
>> -#define EHCI_PS_PEC		0x00000008	/* RWC port enable change */
>> -#define EHCI_PS_PE		0x00000004	/* RW port enable */
>> -#define EHCI_PS_CSC		0x00000002	/* RWC connect status change */
>> -#define EHCI_PS_CS		0x00000001	/* RO connect status */
>> +#define EHCI_PS_WKOC_E		(1 << 22)	/* RW wake on over current */
>> +#define EHCI_PS_WKDSCNNT_E	(1 << 21)	/* RW wake on disconnect */
>> +#define EHCI_PS_WKCNNT_E	(1 << 20)	/* RW wake on connect */
>> +#define EHCI_PS_PO		(1 << 13)	/* RW port owner */
>> +#define EHCI_PS_PP		(1 << 12)	/* RW,RO port power */
>> +#define EHCI_PS_LS		(3 << 10)	/* RO line status */
>> +#define EHCI_PS_PR		(1 << 8)	/* RW port reset */
>> +#define EHCI_PS_SUSP		(1 << 7)	/* RW suspend */
>> +#define EHCI_PS_FPR		(1 << 6)	/* RW force port resume */
>> +#define EHCI_PS_OCC		(1 << 5)	/* RWC over current change */
>> +#define EHCI_PS_OCA		(1 << 4)	/* RO over current active */
>> +#define EHCI_PS_PEC		(1 << 3)	/* RWC port enable change */
>> +#define EHCI_PS_PE		(1 << 2)	/* RW port enable */
>> +#define EHCI_PS_CSC		(1 << 1)	/* RWC connect status change */
>> +#define EHCI_PS_CS		(1 << 0)	/* RO connect status */
>>  #define EHCI_PS_CLEAR		(EHCI_PS_OCC | EHCI_PS_PEC | EHCI_PS_CSC)
>>  
>> -#define EHCI_PS_IS_LOWSPEED(x)	(((x) & EHCI_PS_LS) == 0x00000400)
>> +#define EHCI_PS_IS_LOWSPEED(x)	(((x) & EHCI_PS_LS) == (1 << 10))
>>     
> IMHP It's personnal preference
>   
I think that it is more readable.
>>  
>>  /*
>>   * Schedule Interface Space.
>> @@ -178,7 +183,7 @@ struct QH {
>>  };
>>  
>>  /* Low level intit functions */
>> -
>>  int ehci_hcd_init(void);
>>  int ehci_hcd_stop(void);
>> +
>>     
> ditto
>   
ok
>>  #endif /* USB_EHCI_H */
>> diff --git a/drivers/usb/usb_ehci_core.c b/drivers/usb/usb_ehci_core.c
>> index ec50874..167867e 100644
>> --- a/drivers/usb/usb_ehci_core.c
>> +++ b/drivers/usb/usb_ehci_core.c
>> @@ -1,6 +1,7 @@
>>  /*-
>>   * Copyright (c) 2007-2008, Juniper Networks, Inc.
>>   * Copyright (c) 2008, Excito Elektronik i Sk?ne AB
>> + * Copyright (c) 2008, Michael Trimarchi <trimarchimichael@yahoo.it>
>>   * All rights reserved.
>>   *
>>   * This program is free software; you can redistribute it and/or
>> @@ -18,7 +19,6 @@
>>   * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
>>   * MA 02111-1307 USA
>>   */
>> -#define DEBUG
>>  #include <common.h>
>>  #include <asm/byteorder.h>
>>  #include <usb.h>
>> @@ -99,6 +99,12 @@ static struct descriptor {
>>  	},
>>  };
>>  
>> +#if defined CONFIG_EHCI_IS_TDI
>>     
> please use ifdef or if defined()
>   
>> +#define ehci_is_TDI()	1
>> +#else
>> +#define ehci_is_TDI()	0
>> +#endif
>>     
> please replace with (1) and (0)
>   
>> +
>>     
>
> Best Regards,
> J.
>
>   
I send another version with your comment fixed.

Regards Michael