[U-Boot] [PATCH] usb: dwc2: Add driver for Synopsis DWC2 USB IP block

[Pavel Machek]

Hi!

> This is the USB host controller used on the Altera SoCFPGA and Raspbery Pi.
> 
> This code has three checkpatch warnings, but to make sure it stays at least
> readable and clear, these are not fixed. These bugs are in the USB request
> handling combinatorial logic, so any abstracting of those is out of question.
> 
> Tested on DENX MCV (Altera SoCFPGA 5CSFXC6C6U23C8N) and RPi B+
> (BCM2835).

The code also has ton of unused defines. You said you want them to
document hardware, but do we really need

DWC2_HAINTMSK_CH7 _and_ DWC2_HAINTMSK_CH7_OFFSET, both unused? They
both contain same information...

> NOTE: Unless there are no objections, I would like to apply this.

Parse error :-).

> diff --git a/README b/README
> index 46def00..e243e4b 100644
> --- a/README
> +++ b/README
> @@ -1459,6 +1459,9 @@ The following options need to be configured:
>  		CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
>  		txfilltuning field in the EHCI controller on reset.
>  
> +		CONFIG_USB_DWC2_REG_ADDR the physical CPU address of the DWC2
> +		HW module registers

Missing . at end of sentence.

> index 0000000..cff36f8
> --- /dev/null
> +++ b/drivers/usb/host/dwc2.c
> @@ -0,0 +1,1055 @@
> +/*
> + * Copyright (C) 2012 Oleksandr Tymoshenko <gonzo at freebsd.org>
> + * Copyright (C) 2014 Marek Vasut <marex at denx.de>
> + *
> + * SPDX-License-Identifier:     GPL-2.0
> + */

Is this supposed to be "GPL-2.0+"?

> +	switch (cmd->requesttype & ~USB_DIR_IN) {
> +	case 0:
> +		*(uint16_t *)buffer = cpu_to_le16(1);
> +		len = 2;
> +		break;
> +	case USB_RECIP_INTERFACE:
> +		*(uint16_t *)buffer = cpu_to_le16(0);
> +		len = 2;
> +		break;
> +	case USB_RECIP_ENDPOINT:
> +		*(uint16_t *)buffer = cpu_to_le16(0);
> +		len = 2;
> +		break;
> +	case USB_TYPE_CLASS:
> +		*(uint32_t *)buffer = cpu_to_le32(0);
> +		len = 4;
> +		break;

You can get rid of endianness conversion for zeros.

And can use same code for USB_RECIP_INTERFACE and USB_RECIP_ENDPOINT.

> +	switch (cmd->requesttype & ~USB_DIR_IN) {
> +	case 0:
> +		switch (wValue & 0xff00) {
> +		case 0x0100:	/* device descriptor */
> +			len = min3(txlen, sizeof(root_hub_dev_des), wLength);
> +			memcpy(buffer, root_hub_dev_des, len);
> +			break;
> +		case 0x0200:	/* configuration descriptor */
> +			len = min3(txlen, sizeof(root_hub_config_des), wLength);
> +			memcpy(buffer, root_hub_config_des, len);
> +			break;
> +		case 0x0300:	/* string descriptors */
> +			switch (wValue & 0xff) {
> +			case 0x00:
> +				len = min3(txlen, sizeof(root_hub_str_index0),
> +					   wLength);
> +				memcpy(buffer, root_hub_str_index0, len);
> +				break;
> +			case 0x01:
> +				len = min3(txlen, sizeof(root_hub_str_index1),
> +					   wLength);
> +				memcpy(buffer, root_hub_str_index1, len);
> +				break;
> +			}
> +			break;

Helper function that takes root_hub_str_index0 or similar, then does
len and memcpy?

Otherwise looks good to me, 

Acked-by: Pavel Machek <pavel at denx.de>

> +#define DWC2_HAINT_CH0					(1 << 0)
> +#define DWC2_HAINT_CH0_OFFSET				0
> +#define DWC2_HAINT_CH1					(1 << 1)
> +#define DWC2_HAINT_CH1_OFFSET				1
> +#define DWC2_HAINT_CH2					(1 << 2)
> +#define DWC2_HAINT_CH2_OFFSET				2
> +#define DWC2_HAINT_CH3					(1 << 3)
> +#define DWC2_HAINT_CH3_OFFSET				3
> +#define DWC2_HAINT_CH4					(1 << 4)
> +#define DWC2_HAINT_CH4_OFFSET				4
> +#define DWC2_HAINT_CH5					(1 << 5)
> +#define DWC2_HAINT_CH5_OFFSET				5
> +#define DWC2_HAINT_CH6					(1 << 6)
> +#define DWC2_HAINT_CH6_OFFSET				6
> +#define DWC2_HAINT_CH7					(1 << 7)
> +#define DWC2_HAINT_CH7_OFFSET				7
> +#define DWC2_HAINT_CH8					(1 << 8)
> +#define DWC2_HAINT_CH8_OFFSET				8
> +#define DWC2_HAINT_CH9					(1 << 9)
> +#define DWC2_HAINT_CH9_OFFSET				9
> +#define DWC2_HAINT_CH10					(1 << 10)
> +#define DWC2_HAINT_CH10_OFFSET				10
> +#define DWC2_HAINT_CH11					(1 << 11)
> +#define DWC2_HAINT_CH11_OFFSET				11
> +#define DWC2_HAINT_CH12					(1 << 12)
> +#define DWC2_HAINT_CH12_OFFSET				12
> +#define DWC2_HAINT_CH13					(1 << 13)
> +#define DWC2_HAINT_CH13_OFFSET				13
> +#define DWC2_HAINT_CH14					(1 << 14)
> +#define DWC2_HAINT_CH14_OFFSET				14
> +#define DWC2_HAINT_CH15					(1 << 15)
> +#define DWC2_HAINT_CH15_OFFSET				15

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html