From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Mon, 3 Aug 2015 23:27:50 +0200
Subject: [U-Boot] [PATCH v3 5/5] usb: lpc32xx: add host USB driver
In-Reply-To: <1438631269-31670-6-git-send-email-slemieux.tyco@gmail.com>
References: <1438631269-31670-6-git-send-email-slemieux.tyco@gmail.com>
Message-ID: <201508032327.50424.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 Monday, August 03, 2015 at 09:47:49 PM, slemieux.tyco at gmail.com wrote:
> From: Sylvain Lemieux <slemieux@tycoint.com>
> 
> Incorporate USB driver from legacy LPCLinux NXP BSP.
> The files taken from the legacy patch are:
> - lpc32xx USB driver
> - lpc3250 header file USB registers definition.
> 
> The legacy driver was updated and clean-up as part
> of the integration with the latest u-boot.
> 
> Signed-off-by: Sylvain Lemieux <slemieux@tycoint.com>
> ---

Looks good, so I'm only nitpicking :)

[...]

> diff --git a/drivers/usb/host/ohci-lpc32xx.c
> b/drivers/usb/host/ohci-lpc32xx.c new file mode 100644
> index 0000000..b1a9e9e
> --- /dev/null
> +++ b/drivers/usb/host/ohci-lpc32xx.c

[...]

> +static int usbpll_setup(void)
> +{
> +	unsigned long start;
> +
> +	/* make sure clocks are disabled */
> +	clrbits_le32(&clk_pwr->usb_ctrl,
> +		     CLK_USBCTRL_CLK_EN1 | CLK_USBCTRL_CLK_EN2);
> +
> +	/* start PLL clock input */
> +	setbits_le32(&clk_pwr->usb_ctrl, CLK_USBCTRL_CLK_EN1);
> +
> +	/* Setup PLL. */
> +	setbits_le32(&clk_pwr->usb_ctrl,
> +		     CLK_USBCTRL_FDBK_PLUS1(192 - 1));
> +	setbits_le32(&clk_pwr->usb_ctrl, CLK_USBCTRL_POSTDIV_2POW(0x01));
> +	setbits_le32(&clk_pwr->usb_ctrl, CLK_USBCTRL_PLL_PWRUP);
> +
> +	start = get_timer(0);
> +	while (1) {
> +		if ((readl(clk_pwr->usb_ctrl) & CLK_USBCTRL_PLL_STS) != 0)
> +			break;
> +
> +		if (get_timer(start) > 10000) {
> +			printf("usbpll_setup: ERROR PLL doesn't lock\n");
> +			return -1;
> +		}
> +
> +		udelay(1);
> +	}

Maybe you can pick drivers/usb/host/dwc2.c wait_for_bit() and copy it into this 
driver, since you're having two such timeouts in this driver. Extra point for
implementing generic wait_for_bit() :)

> +	/* enable PLL output */
> +	setbits_le32(&clk_pwr->usb_ctrl, CLK_USBCTRL_CLK_EN2);
> +
> +	return 0;
> +}
> +
> +int usb_cpu_init(void)
> +{
> +	unsigned long start;
> +
> +	/* USB pins routing setup is done by "lpc32xx_usb_init()" and should
> +	 * be call by board "board_init()" or "misc_init_r()" functions. */
> +
> +	/* 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);
> +
> +		start = get_timer(0);
> +		while (1) {
> +			if (readl(&otg->otg_clk_sts) == OTG_CLK_I2C_EN)
> +				break;
> +
> +			if (get_timer(start) > 100)
> +				return -1;
> +
> +			udelay(1);
> +		}
> +	}
> +
> +	/* Configure ISP1301 */
> +	isp1301_configure();
> +
> +	/* setup USB clocks and PLL */
> +	if (usbpll_setup() == -1)
> +		return -1;
> +
> +	/* enable usb_host_need_clk */
> +	setbits_le32(&clk_pwr->usb_ctrl, CLK_USBCTRL_USBHSTND_EN);
> +
> +	/* enable all needed USB clocks */
> +	writel(OTG_CLK_AHB_EN | OTG_CLK_OTG_EN |
> +	       OTG_CLK_I2C_EN | OTG_CLK_HOST_EN,
> +	       &otg->otg_clk_ctrl);
> +
> +	start = get_timer(0);
> +	while (1) {
> +		if ((readl(&otg->otg_clk_ctrl) &
> +		     (OTG_CLK_AHB_EN | OTG_CLK_OTG_EN |
> +		      OTG_CLK_I2C_EN | OTG_CLK_HOST_EN)) ==
> +		    (OTG_CLK_AHB_EN | OTG_CLK_OTG_EN |
> +		     OTG_CLK_I2C_EN | OTG_CLK_HOST_EN))
> +			break;

You can introduce a variable here, const u32 mask = x | y | z | w; and possibly
another one for the reg and then do:
reg = readl();
if (reg & mask == mask)
    break;

That's a bit clearer :)

> +		if (get_timer(start) > 100)
> +			return -1;
> +
> +		udelay(1);
> +	}
> +
> +	setbits_le32(&otg->otg_sts_ctrl, OTG_HOST_EN);
> +	isp1301_set_value(ISP1301_I2C_OTG_CONTROL_1_SET, OTG1_VBUS_DRV);
> +
> +	return 0;
> +}
> +
> +int usb_cpu_stop(void)
> +{
> +	/* vbus off */
> +	isp1301_set_value(ISP1301_I2C_OTG_CONTROL_1_SET, OTG1_VBUS_DRV);
> +
> +	clrbits_le32(&otg->otg_sts_ctrl, OTG_HOST_EN);
> +
> +	clrbits_le32(&clk_pwr->usb_ctrl, CLK_USBCTRL_HCLK_EN);
> +
> +	return 0;
> +}
> +
> +int usb_cpu_init_fail(void)
> +{
> +	return usb_cpu_stop();
> +}