[U-Boot] [PATCH v6 5/5] usb: lpc32xx: add host USB driver

[LEMIEUX, SYLVAIN]

Hi Vladimir and Marek,

> -----Original Message-----
> From: Vladimir Zapolskiy [mailto:vz at mleia.com]
> Sent: 12-Aug-15 1:55 PM
>
> Hi Sylvain,
>
> On 10.08.2015 15:16, slemieux.tyco at gmail.com wrote:
> > From: Sylvain Lemieux <slemieux at 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 at tycoint.com>
> > ---

[...]

> > +
> > +static int wait_for_bit(void *reg, const u32 mask, bool set)
> > +{
>
> (set == false) argument is not in use, and hence there is a piece of
> dead code in the function.
>
See comment in other response; I prefer to keep this way.
It will make it easier to introduce a generic function.

> > +   u32 val;
> > +   unsigned long start = get_timer(0);
> > +
> > +   while (1) {
> > +           val = readl(reg);
> > +           if (!set)
> > +                   val = ~val;
> > +
> > +           if ((val & mask) == mask)
> > +                   return 0;
> > +
> > +           if (get_timer(start) > CONFIG_SYS_HZ)
> > +                   break;
> > +
> > +           udelay(1);
> > +   }
> > +
> > +   debug("%s: Timeout (reg=%p mask=%08x wait_set=%i)\n",
> > +         __func__, reg, mask, set);
>
> I would recommend on error path always to display this message to a user.
>
OK

[...]

> > +
> > +   /* enable I2C clock in OTG block if it isn't */
> > +   if ((readl(&otg->otg_clk_sts) & OTG_CLK_I2C_EN) != OTG_CLK_I2C_EN) {
>
> if (!(readl(&otg->otg_clk_sts) & OTG_CLK_I2C_EN))
>
> > +           writel(OTG_CLK_I2C_EN, &otg->otg_clk_ctrl);
> > +
> > +           ret = wait_for_bit(&otg->otg_clk_sts, OTG_CLK_I2C_EN, 1);
> > +           if (ret)
> > +                   return ret;
>
> Or actually the embracing check may be dropped and
> writel(OTG_CLK_I2C_EN) / wait_for_bit() are done unconditionally.

I will change to always write the register and perform the wait operation.

>
> > +   }
> > +
> > +   /* Configure ISP1301 */
> > +   isp1301_configure();
>
> Not sure if there are any LPC32xx boards with USB host/device, but
> without ISP1301 phy. Let it be here until such a board emerges, if Marek
> does not object.
>
> In general I wonder, won't drivers/usb/phy/ be a better place for this
> driver or bigger part of it?

The Embedded Artists LPC3250 OEM Board is having the USB with the ISP1301;
http://www.embeddedartists.com/products/kits/lpc3250_kit.php
The legacy NXP LPC32xx BSP included board support files for it.

[...]
> >
>
> Feel free to add my
>
> Tested-by: Vladimir Zapolskiy <vz at mleia.com>
>
> --
> With best wishes,
> Vladimir

________________________________

This e-mail contains privileged and confidential information intended for the use of the addressees named above. If you are not the intended recipient of this e-mail, you are hereby notified that you must not disseminate, copy or take any action in respect of any information contained in it. If you have received this e-mail in error, please notify the sender immediately by e-mail and immediately destroy this e-mail and its attachments.