[U-Boot] [PATCH v7] USB: Add generic ULPI layer and a viewport
Igor Grinberg
grinberg at compulab.co.il
Wed Dec 7 14:38:24 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.
--
Regards,
Igor.
More information about the U-Boot
mailing list