From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Thu, 22 Jan 2015 08:05:53 +0100
Subject: [U-Boot] [PATCH 2/4] usb: add 'bcm_udc_otg' support
In-Reply-To: <1421793730-8179-3-git-send-email-srae@broadcom.com>
References: <1421793730-8179-1-git-send-email-srae@broadcom.com>
	<1421793730-8179-3-git-send-email-srae@broadcom.com>
Message-ID: <201501220805.53148.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Tuesday, January 20, 2015 at 11:42:08 PM, Steve Rae wrote:
> Implement the UDC support for the USB OTG interface.
> 
> Signed-off-by: Steve Rae <srae@broadcom.com>
> ---

General question -- this bcm controller you're adding here isn't by
any chance a DWC2 controller, or is it ? There's already a driver
for DWC2 in drivers/usb/gadget/s3c_udc_otg.c . This driver should really
be properly renamed though ;-/

If this is not DWC2, do you know what controller this is please ?

[...]

> +#define FASTBOOT_INTERFACE_CLASS	0xff
> +#define FASTBOOT_INTERFACE_SUB_CLASS	0x42
> +#define FASTBOOT_INTERFACE_PROTOCOL	0x03
> +
> +#define wfld_set(addr, fld_val, fld_mask) \
> +		(writel(((readl(addr) & ~(fld_mask)) | (fld_val)), (addr)))
> +#define wfld_clear(addr, fld_mask) \
> +		(writel((readl(addr) & ~(fld_mask)), (addr)))

The same functionality is implemented by clrsetbits32() and friends.

> +#define DEVICE_STRING_LANGUAGE_ID	0x0409 /* English (United States) */
> +
> +/*
> + * In high speed mode rx packets are 512
> + * In full speed mode rx packets are 64
> + */
> +#define RX_ENDPOINT_MAXIMUM_PACKET_SIZE		(0x0200)
> +#define TX_ENDPOINT_MAXIMUM_PACKET_SIZE		(0x0040)

No need for the brackets here.

> +#ifndef CONFIG_USB_BOARDNAME
> +#define CONFIG_USB_BOARDNAME "Board"
> +#endif
> +
> +#ifndef CONFIG_USB_CONFIGURATION
> +#define CONFIG_USB_CONFIGURATION "Fastboot"
> +#endif

What is all this stuff doing in generic USB UDC driver please ?
Or is this not a generic UDC driver ?

[...]

> +static void usb_turn_off_vdp(void)
> +{
> +	/* Check if it is standard host port (SHP) */
> +	if (readl(HSOTG_CTRL_BASE_ADDR + HSOTG_CTRL_STATUS_OFFSET) &
> +			HSOTG_CTRL_STATUS_SHP_MASK) {
> +		udelay(60000);	/* 50 ms + 20 % */

mdelay(60), this should be fixed all over the place please.

> +		/*
> +		 * force turn off VDP, enable sw_ovwr_set to take over the
> +		 * bc11 switches directly
> +		 */
> +		wfld_set(HSOTG_CTRL_BASE_ADDR + HSOTG_CTRL_CFG_OFFSET,
> +			 BC11_CFG_VDP_OFF,
> +			 HSOTG_CTRL_CFG_OVWR_KEY_MASK |
> +			 HSOTG_CTRL_CFG_SW_OVWR_EN_MASK |
> +			 HSOTG_CTRL_CFG_OVWR_SET_M0_MASK |
> +			 HSOTG_CTRL_CFG_OVWR_SET_P0_MASK);
> +
> +		udelay(160);	/* Allow time for switches to disengage */
> +	} else {
> +		udelay(120000);	/* 100 ms + 20 % */
> +	}
> +}

[...]