From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Sat, 18 Jul 2015 00:09:12 +0200
Subject: [U-Boot] [PATCH 3/3] usb: lpc32xx: add host USB driver
In-Reply-To: <1437166134-21204-4-git-send-email-slemieux.tyco@gmail.com>
References: <1437166134-21204-4-git-send-email-slemieux.tyco@gmail.com>
Message-ID: <201507180009.12140.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 Friday, July 17, 2015 at 10:48:54 PM, slemieux.tyco at gmail.com wrote:
> From: Sylvain Lemieux <slemieux@tycoint.com>

Hi!

> Incorporate DMA driver from legacy LPCLinux NXP BSP.
> The files taken from the legacy patch are:
> - lpc32xx USB driver
> - lpc3250 header file USB registers definition.
> 
> Updated driver to integrate with the latest u-boot:
> 1) Fixed checkpatch script output in legacy code.
> 2) Use LPC32xx definition from "cpu.h" and "clk.h".
> 3) Incorporate USB specific register definition from "lpc3250.h" header
> file from legacy BSP patch from LPCLinux.
> 4) Use u-boot API for register access to remove the use of volatile in
>    register definition taken from "lpc3250.h" header file.
> 5) Update driver for latest u-boot USB interface (board_usb init &
> board_usb_cleanup). 6) Use the peripheral clock to compute the I2C
> divider.


[...]

> +static int i2c_wait_reset(int timeout)
> +{
> +	while (timeout > 0 && (readl(&otg->otg_i2c.otg_i2c_ctrl) & I2C_RESET)) {
> +		udelay(1000);
> +		timeout--;
> +	}
> +
> +	return timeout <= 0;
> +}
> +
> +static int isp1301_set_value(int reg, int value)
> +{
> +	int n = 0;
> +
> +	/* send isp1301 address */
> +	writel((ISP1301_I2C_ADDR << 1) | I2C_START_BIT,
> +	       &otg->otg_i2c.otg_i2c_txrx);
> +
> +	/* offset to write to */
> +	writel((reg & 0xff) | I2C_WRITE, &otg->otg_i2c.otg_i2c_txrx);
> +	/* value to write */
> +	writel((value & 0xff) | I2C_STOP_BIT, &otg->otg_i2c.otg_i2c_txrx);
> +
> +	/* wait for transmit done (TDI) */
> +	while (((readl(&otg->otg_i2c.otg_i2c_stat) & I2C_TDI) != I2C_TDI) &&
> +	       n++ < 100000)
> +		;

Can you please rework the timeouts to use get_timer() ? It looks like this:

start = get_timer(0);
do {
	if (readl(...) == FOO)
		break;
	udelay(1);	/* Don't do tightloop and poke WDT */
} while (get_timer(start) < DELAY_IN_MSECS);

> +	if (n >= 100000) {
> +		printf("isp1301_set_value: ERROR TDI not set\n");
> +		return -1;
> +	}
> +
> +	/* clear TDI */
> +	setbits_le32(&otg->otg_i2c.otg_i2c_stat, I2C_TDI);
> +
> +	return 0;
> +}

Don't you want to split this code out and make it into an actual i2c controller?
Then, the USB driver would use regular i2c_*() calls to configure the device on
the other end of the i2c bus. That'd be really cool and convenient.

[...]

> +int board_usb_init(int index, enum usb_init_type init)
> +{
> +	/* Remove warning. */
> +	(void)index;
> +
> +	if (init != USB_INIT_HOST)
> +		return -1;
> +
> +	/* enable AHB slave USB clock */
> +	setbits_le32(&clk_pwr->usb_ctrl,
> +		     CLK_USBCTRL_HCLK_EN | CLK_USBCTRL_BUS_KEEPER);
> +
> +	/* enable I2C clock in OTG block if it isn't */
> +	if ((readl(&otg->otg_clk_sts) & OTG_CLK_I2C_EN) != OTG_CLK_I2C_EN) {
> +		writel(OTG_CLK_I2C_EN, &otg->otg_clk_ctrl);
> +
> +		while (readl(&otg->otg_clk_sts) != OTG_CLK_I2C_EN)
> +			;

Ew..., such ugly infinite loops are a big no-no, please add a timeout here.

[...]