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

[Marek Vasut]

On Saturday, July 18, 2015 at 12:52:58 AM, LEMIEUX, SYLVAIN wrote:
> Hi Mark,

Hi!

> Thanks for the feedback;
> 
> > From: U-Boot [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Marek
> > Vasut Sent: 17-Jul-15 6:09 PM
> > 
> > On Friday, July 17, 2015 at 10:48:54 PM, slemieux.tyco at gmail.com wrote:
> > > From: Sylvain Lemieux <slemieux at tycoint.com>

[...]

> > > +   /* 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;
> > > +}
> 
> The initial posting was a porting effort only;
> no problem, I will do the rework.

Cool, thanks!

> > 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.
> 
> I can look at this; but, I will not be able to do it before the following
> week. I will ensure to post my update before the merge window close.

No worries, thanks!

> There is already a LPC32xx I2C available in u-boot;
> minor change will be require (ex. source clock is different for the USB
> I2C).

Then that's even better, you'll have less code to take care of.

> > [...]
> > 
> > > +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.
> 
> Will do as part of the rework to improve the result of this porting effort.

Thanks!