Hi Igor, Looks good - a few comments from me.
On Mon, Dec 5, 2011 at 1:07 AM, Igor Grinberg <grinb...@compulab.co.il> wrote: > From: Jana Rapava <ferma...@gmail.com> > > Add partial ULPI specification implementation that should be enough to > interface the ULPI PHYs in the boot loader context. > Add a viewport implementation for Chipidea/ARC based controllers. > > Signed-off-by: Jana Rapava <ferma...@gmail.com> > Signed-off-by: Igor Grinberg <grinb...@compulab.co.il> > Cc: Remy Bohmer <li...@bohmer.net> > Cc: Stefano Babic <sba...@denx.de> > Cc: Wolfgang Grandegger <w...@denx.de> > Cc: Simon Glass <s...@chromium.org> > --- > Makefile | 1 + > drivers/usb/ulpi/Makefile | 44 ++++++ > drivers/usb/ulpi/ulpi-viewport.c | 118 +++++++++++++++ > drivers/usb/ulpi/ulpi.c | 227 +++++++++++++++++++++++++++++ > include/usb/ulpi.h | 298 > ++++++++++++++++++++++++++++++++++++++ This would benefit from additions to the README describing the two new CONFIGs you add. > 5 files changed, 688 insertions(+), 0 deletions(-) > create mode 100644 drivers/usb/ulpi/Makefile > create mode 100644 drivers/usb/ulpi/ulpi-viewport.c > create mode 100644 drivers/usb/ulpi/ulpi.c > create mode 100644 include/usb/ulpi.h > > diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c > new file mode 100644 > index 0000000..805e29d > --- /dev/null > +++ b/drivers/usb/ulpi/ulpi.c > @@ -0,0 +1,227 @@ > +/* > + * Copyright (C) 2011 Jana Rapava <ferma...@gmail.com> > + * Copyright (C) 2011 CompuLab, Ltd. <www.compulab.co.il> > + * > + * Authors: Jana Rapava <ferma...@gmail.com> > + * Igor Grinberg <grinb...@compulab.co.il> > + * > + * Based on: > + * linux/drivers/usb/otg/ulpi.c > + * Generic ULPI USB transceiver support > + * > + * Original Copyright follow: > + * Copyright (C) 2009 Daniel Mack <dan...@caiaq.de> > + * > + * Based on sources from > + * > + * Sascha Hauer <s.ha...@pengutronix.de> > + * Freescale Semiconductors > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License as published by > + * the Free Software Foundation; either version 2 of the License, or > + * (at your option) any later version. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <common.h> > +#include <exports.h> > +#include <usb/ulpi.h> > + > +#define ULPI_ID_REGS_COUNT 4 > +#define ULPI_TEST_VALUE 0x55 /* 0x55 == 0b01010101 */ > + > +static struct ulpi_regs *ulpi = (struct ulpi_regs *)0; > + > +static int ulpi_integrity_check(u32 ulpi_viewport) > +{ > + u32 err, val, tval = ULPI_TEST_VALUE; > + int i; > + > + /* Use the 'special' test value to check all bits */ > + for (i = 0; i < 2; i++, tval <<= 1) { > + err = ulpi_write(ulpi_viewport, &ulpi->scratch, tval); > + if (err) > + return err; > + > + val = ulpi_read(ulpi_viewport, &ulpi->scratch); > + if (val != tval) { > + printf("ULPI integrity check failed\n"); > + return val; > + } > + } > + > + return 0; > +} > + > +int ulpi_init(u32 ulpi_viewport) > +{ > + u32 val, id = 0; > + u8 *reg = &ulpi->product_id_high; > + int i; > + > + /* Assemble ID from four ULPI ID registers (8 bits each). */ > + for (i = 0; i < ULPI_ID_REGS_COUNT; i++) { > + val = ulpi_read(ulpi_viewport, reg - i); > + if (val == ULPI_ERROR) > + return val; > + > + id = (id << 8) | val; > + } > + > + /* Split ID into vendor and product ID. */ > + debug("ULPI transceiver ID 0x%04x:0x%04x\n", id >> 16, id & 0xffff); > + > + return ulpi_integrity_check(ulpi_viewport); > +} > + > +int ulpi_select_transceiver(u32 ulpi_viewport, u8 speed) Is there any point in making the argument u8? How about just unsigned? I think this adds a mask to the call = larger code size. You check the arg with the switch() below anyway. > +{ > + u8 tspeed = ULPI_FC_FULL_SPEED; > + u32 val; > + > + switch (speed) { > + case ULPI_FC_HIGH_SPEED: > + case ULPI_FC_FULL_SPEED: > + case ULPI_FC_LOW_SPEED: > + case ULPI_FC_FS4LS: > + tspeed = speed; > + break; > + default: > + printf("ULPI: %s: wrong transceiver speed specified, " > + "falling back to full speed\n", __func__); > + } > + > + val = ulpi_read(ulpi_viewport, &ulpi->function_ctrl); > + if (val == ULPI_ERROR) > + return val; > + > + /* clear the previous speed setting */ > + val = (val & ~ULPI_FC_XCVRSEL_MASK) | tspeed; > + > + return ulpi_write(ulpi_viewport, &ulpi->function_ctrl, val); > +} > + > +int ulpi_set_vbus(u32 ulpi_viewport, int on, int ext_power, int ext_ind) > +{ > + u32 flags = ULPI_OTG_DRVVBUS; > + u8 *reg = on ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear; > + > + if (ext_power) > + flags |= ULPI_OTG_DRVVBUS_EXT; > + if (ext_ind) > + flags |= ULPI_OTG_EXTVBUSIND; > + > + return ulpi_write(ulpi_viewport, reg, flags); > +} > + > +int ulpi_set_pd(u32 ulpi_viewport, int enable) > +{ > + u32 val = ULPI_OTG_DP_PULLDOWN | ULPI_OTG_DM_PULLDOWN; > + u8 *reg = enable ? &ulpi->otg_ctrl_set : &ulpi->otg_ctrl_clear; > + > + return ulpi_write(ulpi_viewport, reg, val); > +} > + > +int ulpi_opmode_sel(u32 ulpi_viewport, u8 opmode) suggest unsigned for arg instead of u8 > +{ > + u8 topmode = ULPI_FC_OPMODE_NORMAL; > + u32 val; > + > + switch (opmode) { > + case ULPI_FC_OPMODE_NORMAL: > + case ULPI_FC_OPMODE_NONDRIVING: > + case ULPI_FC_OPMODE_DISABLE_NRZI: > + case ULPI_FC_OPMODE_NOSYNC_NOEOP: > + topmode = opmode; > + break; > + default: > + printf("ULPI: %s: wrong OpMode specified, " > + "falling back to OpMode Normal\n", __func__); > + } > + > + val = ulpi_read(ulpi_viewport, &ulpi->function_ctrl); > + if (val == ULPI_ERROR) > + return val; > + > + /* clear the previous opmode setting */ > + val = (val & ~ULPI_FC_OPMODE_MASK) | topmode; > + > + return ulpi_write(ulpi_viewport, &ulpi->function_ctrl, val); > +} > + > +int ulpi_serial_mode_enable(u32 ulpi_viewport, u8 smode) and again > +{ > + switch (smode) { > + case ULPI_IFACE_6_PIN_SERIAL_MODE: > + case ULPI_IFACE_3_PIN_SERIAL_MODE: > + break; > + default: > + printf("ULPI: %s: unrecognized Serial Mode specified\n", > + __func__); > + return ULPI_ERROR; > + } > + > + return ulpi_write(ulpi_viewport, &ulpi->iface_ctrl_set, smode); > +} > + > +int ulpi_suspend(u32 ulpi_viewport) > +{ > + u32 err; This function returns int but err is u32. Which do you want? I think function return values should be native types, int or unsigned in this case. > + > + err = ulpi_write(ulpi_viewport, &ulpi->function_ctrl_clear, > + ULPI_FC_SUSPENDM); > + if (err) > + printf("ULPI: %s: failed writing the suspend bit\n", > __func__); > + > + return err; > +} > + > +/* > + * Wait for ULPI PHY reset to complete. > + * Actual wait for reset must be done in a view port specific way, > + * because it involves checking the DIR line. > + */ > +static int __ulpi_reset_wait(u32 ulpi_viewport) > +{ > + u32 val; > + int timeout = CONFIG_USB_ULPI_TIMEOUT; > + > + /* Wait for the RESET bit to become zero */ > + while (--timeout) { > + /* > + * This function is generic and suppose to work > + * with any viewport, so we cheat here and don't check > + * for the error of ulpi_read(), if there is one, then > + * there will be a timeout. > + */ > + val = ulpi_read(ulpi_viewport, &ulpi->function_ctrl); > + if (!(val & ULPI_FC_RESET)) > + return 0; > + > + udelay(1); > + } > + > + printf("ULPI: %s: reset timed out\n", __func__); > + > + return ULPI_ERROR; > +} > +int ulpi_reset_wait(u32) __attribute__((weak, alias("__ulpi_reset_wait"))); > + > +int ulpi_reset(u32 ulpi_viewport) > +{ > + u32 err; same as above function > + > + err = ulpi_write(ulpi_viewport, > + &ulpi->function_ctrl_set, ULPI_FC_RESET); > + if (err) { > + printf("ULPI: %s: failed writing reset bit\n", __func__); > + return err; > + } > + > + return ulpi_reset_wait(ulpi_viewport); > +} In general if you have time you could check that you document all the args in your comments rather than just some. But I think in all cases it is pretty clear anyway. Just out of interest, is it possible to test this? How would I plumb it in? Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot