[U-Boot] [PATCH v7] USB: Add generic ULPI layer and a viewport
Marek Vasut
marek.vasut at gmail.com
Wed Dec 7 18:27:48 CET 2011
> 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 <grinberg at compulab.co.il>
wrote:
> >> From: Jana Rapava <fermata7 at 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 <fermata7 at gmail.com>
> >> Signed-off-by: Igor Grinberg <grinberg at compulab.co.il>
> >> Cc: Remy Bohmer <linux at bohmer.net>
> >> Cc: Stefano Babic <sbabic at denx.de>
> >> Cc: Wolfgang Grandegger <wg at denx.de>
> >> Cc: Simon Glass <sjg at 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 <fermata7 at gmail.com>
> >> + * Copyright (C) 2011 CompuLab, Ltd. <www.compulab.co.il>
> >> + *
> >> + * Authors: Jana Rapava <fermata7 at gmail.com>
> >> + * Igor Grinberg <grinberg at compulab.co.il>
> >> + *
> >> + * Based on:
> >> + * linux/drivers/usb/otg/ulpi.c
> >> + * Generic ULPI USB transceiver support
> >> + *
> >> + * Original Copyright follow:
> >> + * Copyright (C) 2009 Daniel Mack <daniel at caiaq.de>
> >> + *
> >> + * Based on sources from
> >> + *
> >> + * Sascha Hauer <s.hauer at 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
More information about the U-Boot
mailing list