> On 12/07/11 03:42, Simon Glass wrote: > > 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? > > Not really... > > > How about just unsigned? > > Sounds sane > > > I think this adds a mask to the call = larger code size. You check the > > arg with the switch() below anyway. > > Haven't thought about that... > I don't know how is this exactly works, but I don't see > any reason why shouldn't I use unsigned here. > > >> +{ > >> + 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 > > Ok > > >> +{ > >> + 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. > > Yep, missed this... it also applies in some other places. > > >> + > >> + 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. > > Me too, that's exactly the reason I haven't documented them all around - > I don't like the stupid copy/paste work... > > > Just out of interest, is it possible to test this? How would I plumb it > > in? > > Well, from my experience with ULPI hardware, > I think the controller specific glue looks like the right place > for putting the ULPI layer calls in. > > In general, the controller code knows which PHYs it supports > and board code knows which PHY is assembled on the board, > so it is not that straight simple to find the right place. > > I think, Marek has patches that supposed to use this framework on efikamx > board.
I tried using the interface, but the design seems completely wrong :-( Jana was supposed to design it mainly for the efikamx board, but this interface is unusable there. I had to fall back to basic ulpi_read()/ulpi_write() calls :-( I'm afraid we won't make it for .12 release window with this patches ... very bad :-( I'll try talking to WD if he can push the release window to allow this (or redesigned version) in, but I don't know if that's a good idea. M _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot