Hi Igor,
Looks good - a few comments from me.
On Mon, Dec 5, 2011 at 1:07 AM, Igor Grinberg grinberg@compulab.co.il wrote:
From: Jana Rapava fermata7@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@gmail.com Signed-off-by: Igor Grinberg grinberg@compulab.co.il Cc: Remy Bohmer linux@bohmer.net Cc: Stefano Babic sbabic@denx.de Cc: Wolfgang Grandegger wg@denx.de Cc: Simon Glass sjg@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@gmail.com
- Copyright (C) 2011 CompuLab, Ltd. <www.compulab.co.il>
- Authors: Jana Rapava fermata7@gmail.com
- Igor Grinberg grinberg@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@caiaq.de
- Based on sources from
- Sascha Hauer s.hauer@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