[U-Boot,v3] ulpi: add generic ULPI functionality

Add generic functions for ULPI init and setting bits in
ULPI registers.

Signed-off-by: Jana Rapava <fermata7@gmail.com>
Cc: Marek Vasut <marek.vasut@gmail.com>
Cc: Remy Bohmer <linux@bohmer.net>
Cc: Stefano Babic <sbabic@denx.de>
Cc: Igor Grinberg <grinberg@compulab.co.il>
Cc: Wolfgang Grandegger <wg@denx.de>
---
Changes for v2:
       - make code EHCI-independent
       - use udelay() in waiting loop
       - mark static functions as static
       - naming changes
Changes for v3:
	- merge with patch ulpi: add generic ULPI support header file
	- rewrite ULPI interface in more functionality-oriented way

 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   45 ++++++++
 drivers/usb/ulpi/ulpi-viewport.c |   88 +++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  199 ++++++++++++++++++++++++++++++++++
 include/usb/ulpi.h               |  222 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 555 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

Hi Jana,

Good job!
IMO, this version looks much better
(although Marek is not happy with its schedule).
Some final neats below and we're done.

On 11/24/11 14:22, Jana Rapava wrote:
> Add generic functions for ULPI init and setting bits in
> ULPI registers.
> 
> Signed-off-by: Jana Rapava <fermata7@gmail.com>
> Cc: Marek Vasut <marek.vasut@gmail.com>
> Cc: Remy Bohmer <linux@bohmer.net>
> Cc: Stefano Babic <sbabic@denx.de>
> Cc: Igor Grinberg <grinberg@compulab.co.il>
> Cc: Wolfgang Grandegger <wg@denx.de>
> ---
> Changes for v2:
>        - make code EHCI-independent
>        - use udelay() in waiting loop
>        - mark static functions as static
>        - naming changes
> Changes for v3:
> 	- merge with patch ulpi: add generic ULPI support header file
> 	- rewrite ULPI interface in more functionality-oriented way
> 
>  Makefile                         |    1 +
>  drivers/usb/ulpi/Makefile        |   45 ++++++++
>  drivers/usb/ulpi/ulpi-viewport.c |   88 +++++++++++++++
>  drivers/usb/ulpi/ulpi.c          |  199 ++++++++++++++++++++++++++++++++++
>  include/usb/ulpi.h               |  222 ++++++++++++++++++++++++++++++++++++++
>  5 files changed, 555 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/Makefile b/Makefile
> index dfe939f..70a1e1e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -272,6 +272,7 @@ LIBS += drivers/usb/gadget/libusb_gadget.o
>  LIBS += drivers/usb/host/libusb_host.o
>  LIBS += drivers/usb/musb/libusb_musb.o
>  LIBS += drivers/usb/phy/libusb_phy.o
> +LIBS += drivers/usb/ulpi/libusb_ulpi.o
>  LIBS += drivers/video/libvideo.o
>  LIBS += drivers/watchdog/libwatchdog.o
>  LIBS += common/libcommon.o
> diff --git a/drivers/usb/ulpi/Makefile b/drivers/usb/ulpi/Makefile
> new file mode 100644
> index 0000000..a1a2244
> --- /dev/null
> +++ b/drivers/usb/ulpi/Makefile
> @@ -0,0 +1,45 @@
> +#
> +# Copyright (C) 2011 Jana Rapava <fermata7@gmail.com>
> +# See file CREDITS for list of people who contributed to this
> +# project.
> +#
> +# 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.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> +# MA 02111-1307 USA
> +#
> +
> +include $(TOPDIR)/config.mk
> +
> +LIB	:= $(obj)libusb_ulpi.o
> +
> +COBJS-$(CONFIG_USB_ULPI)		+= ulpi.o
> +COBJS-$(CONFIG_USB_ULPI_VIEWPORT)	+= ulpi-viewport.o
> +
> +COBJS	:= $(COBJS-y)
> +SRCS	:= $(COBJS:.o=.c)
> +OBJS	:= $(addprefix $(obj),$(COBJS))
> +
> +all:	$(LIB)
> +
> +$(LIB):	$(obj).depend $(OBJS)
> +	$(call cmd_link_o_target, $(OBJS))
> +
> +#########################################################################
> +
> +# defines $(obj).depend target
> +include $(SRCTREE)/rules.mk
> +
> +sinclude $(obj).depend
> +
> +#########################################################################
> diff --git a/drivers/usb/ulpi/ulpi-viewport.c b/drivers/usb/ulpi/ulpi-viewport.c
> new file mode 100644
> index 0000000..9a1f59f
> --- /dev/null
> +++ b/drivers/usb/ulpi/ulpi-viewport.c
> @@ -0,0 +1,88 @@
> +/*
> + * Copyright (C) 2011 Jana Rapava <fermata7@gmail.com>
> + * Based on:
> + * linux/drivers/usb/otg/ulpi_viewport.c
> + *
> + * Original Copyright follow:
> + * Copyright (C) 2011 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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 <asm/io.h>
> +#include <usb/ulpi.h>
> +
> +/* ULPI viewport control bits */
> +#define ULPI_WU		(1 << 31)
> +#define ULPI_SS		(1 << 27)
> +#define ULPI_RWRUN	(1 << 30)
> +#define ULPI_RWCTRL	(1 << 29)
> +
> +#define ULPI_ADDR_SHIFT		16
> +#define ulpi_write_mask(value)	((value) & 0xff)
> +#define ulpi_read_mask(value)	(((value) >> 8) & 0xff)
> +
> +static int ulpi_wait(u32 ulpi_viewport, u32 ulpi_value, u32 ulpi_mask)
> +{
> +	int timeout = CONFIG_USB_ULPI_TIMEOUT;
> +	u32 tmp;
> +
> +	writel(ulpi_value, ulpi_viewport);
> +
> +	/* Wait for the bits in ulpi_mask to become zero. */
> +	while (--timeout) {
> +		tmp = readl(ulpi_viewport);
> +		if (!(tmp & ulpi_mask))
> +			break;
> +		udelay(1);
> +	}
> +
> +	return !timeout;
> +}
> +
> +static int ulpi_wakeup(u32 ulpi_viewport)
> +{
> +	if (readl(ulpi_viewport) & ULPI_SS)
> +		return 0; /* already awake */
> +
> +	return ulpi_wait(ulpi_viewport, ULPI_WU, ULPI_WU);
> +}
> +
> +void ulpi_write(u32 ulpi_viewport, u32 reg, u32 value)

Can this function also return an error value in case it fails (times out)?
This way the error can propagate to the user code.

> +{
> +	u32 tmp;
> +	if (ulpi_wakeup(ulpi_viewport)) {
> +		printf("ULPI wakeup timed out\n");
> +		return;
> +	}
> +
> +	tmp = ulpi_wait(ulpi_viewport, ULPI_RWRUN | ULPI_RWCTRL |
> +		reg << ULPI_ADDR_SHIFT | ulpi_write_mask(value), ULPI_RWRUN);
> +	if (tmp)
> +		printf("ULPI write timed out\n");
> +}
> +
> +u32 ulpi_read(u32 ulpi_viewport, u32 reg)
> +{
> +	if (ulpi_wakeup(ulpi_viewport)) {
> +		printf("ULPI wakeup timed out\n");
> +		return ULPI_ERROR;
> +	}
> +
> +	if (ulpi_wait(ulpi_viewport, ULPI_RWRUN | reg << ULPI_ADDR_SHIFT,
> +		ULPI_RWRUN)) {
> +			printf("ULPI read timed out\n");
> +			return ULPI_ERROR;
> +	}
> +
> +	return ulpi_read_mask(readl(ulpi_viewport));
> +}
> diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
> new file mode 100644
> index 0000000..2d66e86
> --- /dev/null
> +++ b/drivers/usb/ulpi/ulpi.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright (C) 2011 Jana Rapava <fermata7@gmail.com>
> + * 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>
> +
> +static struct ulpi_regs *ulpi = (struct ulpi_regs *)0;
> +
> +static void ulpi_integrity_check(u32 ulpi_viewport)
> +{
> +	u32 tmp = 0;
> +	int i;
> +	for (i = 0; i < 2; i++) {
> +		ulpi_write(ulpi_viewport, (u32)&ulpi->scratch,
> +			ULPI_TEST_VALUE << i);
> +		tmp = ulpi_read(ulpi_viewport, (u32)&ulpi->scratch);
> +
> +		if (tmp != (ULPI_TEST_VALUE << i)) {
> +			printf("ULPI integrity check failed\n");
> +			return;

I would expect this function to return a value, so it can be
propagated to the code using it.

> +		}
> +	}
> +}
> +
> +/*
> + * This function is used to select transceiver speed.
> + * Accepted parameter values are:
> + * ULPI_FC_HIGH_SPEED, ULPI_FC_FULL_SPEED, ULPI_FC_LOW_SPEED, ULPI_FC_FS4LS
> + * (FS transceiver for LS packets).
> + * Default value is ULPI_FC_FULL_SPEED.
> + */
> +void ulpi_select_transceiver(u32 ulpi_viewport, int speed)

same here... let the error propagate.
Please, fix it globally.

> +{
> +	switch (speed) {
> +	case ULPI_FC_HIGH_SPEED:
> +		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
> +			ULPI_FC_HIGH_SPEED);
> +		break;
> +	case ULPI_FC_FULL_SPEED:
> +		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
> +			ULPI_FC_FULL_SPEED);
> +		break;
> +	case ULPI_FC_LOW_SPEED:
> +		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
> +			ULPI_FC_LOW_SPEED);
> +		break;
> +	case ULPI_FC_FS4LS:
> +		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
> +			ULPI_FC_FS4LS);

each of the above ulpi_write() calls can fail...
Please, fix globally.

> +		break;
> +	default:
> +		printf("ulpi_select_transceiver: unknown transceiver speed\n");
> +	}
> +}
> +
> +/*
> + * Signals that 5V is driven to VBUS.
> + * Ext_power_supply is 1, if we use external supply instead of
> + * internal charge pump(which is default).
> + * Use_ext_indicator is 1, if we use external VBUS over-current indicator.
> + */
> +void ulpi_drive_vbus(u32 ulpi_viewport,
> +	int ext_power_supply, int use_ext_indicator)
> +{
> +	ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set, ULPI_OTG_DRVVBUS);
> +
> +	if (ext_power_supply)
> +		ulpi_write(ulpi_viewport,
> +			(u32)&ulpi->otg_ctrl_set, ULPI_OTG_DRVVBUS_EXT);
> +
> +	if (use_ext_indicator)
> +		ulpi_write(ulpi_viewport,
> +			(u32)&ulpi->otg_ctrl_set, ULPI_OTG_EXTVBUSIND);
> +}
> +
> +/*
> + * If enable is 0, pull-down resistor not connected to D+, else pull-down
> + * resistor connected to D+.
> + * Default behaviour is as for enable equal to 1.
> + */
> +void ulpi_dp_pulldown(u32 ulpi_viewport, int enable)
> +{
> +	if (enable)
> +		ulpi_write(ulpi_viewport,
> +			(u32)&ulpi->otg_ctrl_set, ULPI_OTG_DP_PULLDOWN);
> +	else
> +		ulpi_write(ulpi_viewport,
> +			(u32)&ulpi->otg_ctrl_clear, ULPI_OTG_DP_PULLDOWN);
> +}
> +
> +/*
> + * If enable is 0, pull-down resistor not connected to D- else pull-down
> + * resistor connected to D-.
> + * Default behaviour is as for enable equal to 1.
> + */
> +void ulpi_dm_pulldown(u32 ulpi_viewport, int enable)
> +{
> +	if (enable)
> +		ulpi_write(ulpi_viewport,
> +			(u32)&ulpi->otg_ctrl_set, ULPI_OTG_DM_PULLDOWN);
> +	else
> +		ulpi_write(ulpi_viewport,
> +			(u32)&ulpi->otg_ctrl_clear, ULPI_OTG_DM_PULLDOWN);
> +}

Correct me if I'm wrong, but I don't think there is a use for
the above functions in separate and the user will have to
call them both.
So, can these two functions be united in one,
say ulpi_pulldown(u32 ..., int enable)?

> +
> +/*
> + * This function is used to select bit encoding style.
> + * Accepted parameter values are:
> + * ULPI_FC_OPMODE_NORMAL, ULPI_FC_OPMODE_NONDRIVING,
> + * ULPI_FC_OPMODE_DISABLE_NRZI, ULPI_FC_OPMODE_NOSYNC_NOEOP.
> + * Default value is ULPI_FC_OPMODE_NORMAL.
> + */
> +void ulpi_select_opmode(u32 ulpi_viewport, int style)
> +{
> +	switch (style) {
> +	case ULPI_FC_OPMODE_NORMAL:
> +		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
> +			ULPI_FC_OPMODE_NORMAL);
> +		break;
> +	case ULPI_FC_OPMODE_NONDRIVING:
> +		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
> +			ULPI_FC_OPMODE_NONDRIVING);
> +		break;
> +	case ULPI_FC_OPMODE_DISABLE_NRZI:
> +		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
> +			ULPI_FC_OPMODE_DISABLE_NRZI);
> +		break;
> +	case ULPI_FC_OPMODE_NOSYNC_NOEOP:
> +		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
> +			ULPI_FC_OPMODE_NOSYNC_NOEOP);
> +		break;
> +	default:
> +		printf("ulpi_select_opmode: unknown bit encoding style\n");
> +	}
> +}
> +
> +/* Put PHY into low power mode. */
> +void ulpi_suspend(u32 ulpi_viewport)
> +{
> +	ulpi_write(ulpi_viewport,
> +		(u32)&ulpi->function_ctrl_clear, ULPI_FC_SUSPENDM);
> +}
> +
> +/* Put PHY out of low power mode. */
> +void ulpi_resume(u32 ulpi_viewport)
> +{
> +	ulpi_write(ulpi_viewport,
> +		(u32)&ulpi->function_ctrl_set, ULPI_FC_SUSPENDM);
> +}
> +
> +/* Reset transceiver, does not reset ULPI interface or ULPI register set. */
> +void ulpi_reset(u32 ulpi_viewport)
> +{
> +	ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set, ULPI_FC_RESET);
> +}

In two functions above, don't you need to wait for the operation to complete?

> +
> +/* Charge VBUS through a resistor */
> +void ulpi_charge_vbus(u32 ulpi_viewport)
> +{
> +	ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set, ULPI_OTG_CHRGVBUS);
> +}

CHRGVBUS bit is used for SRP, and the above function does not implement
SRP by the spec. so it is wrong. Also, I don't think we need SRP in U-Boot,
but may be I'm wrong...

Now, I know you need to set this bit to workaround the efika bug...
But it does not belong to the generic driver!
What I suggest is, remove this function from here, and use ulpi_write()
directly, in places where you need to workaround that bug.

> +
> +void ulpi_init(u32 ulpi_viewport)

same here, how does the user code know, if init has succeeded?

> +{
> +	u32 tmp = 0;
> +	int reg;
> +
> +	/* Assemble ID from four ULPI ID registers (8 bits each). */
> +	for (reg = ULPI_ID_REGS_COUNT - 1; reg >= 0; reg--)
> +		tmp |= ulpi_read(ulpi_viewport, reg) << (reg * 8);
> +
> +	/* Split ID into vendor and product ID. */
> +	debug("Found ULPI transceiver ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);
> +
> +	ulpi_integrity_check(ulpi_viewport);
> +}
> diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h
> new file mode 100644
> index 0000000..834b5e8
> --- /dev/null
> +++ b/include/usb/ulpi.h
> @@ -0,0 +1,222 @@
> +/*
> + * Copyright (C) 2011 Jana Rapava <fermata7@gmail.com>
> + * Based on:
> + * linux/include/linux/usb/ulpi.h
> + * ULPI defines and function prototypes
> + *
> + * Original Copyrights follow:
> + * Copyright (C) 2010 Nokia Corporation
> + *
> + * This software is distributed under the terms of the GNU General
> + * Public License ("GPL") as published by the Free Software Foundation,
> + * version 2 of that License.
> + */
> +
> +#ifndef __USB_ULPI_H
> +#define __USB_ULPI_H
> +
> +#define ULPI_ID_REGS_COUNT	4
> +#define ULPI_TEST_VALUE		0x55
> +/* value greater than 0xff indicates failure */
> +#define ULPI_ERROR		(1 << 8)
> +
> +/* ULPI viewport control bits */
> +#define ULPI_WU		(1 << 31)
> +#define ULPI_SS		(1 << 27)
> +#define ULPI_RWRUN	(1 << 30)
> +#define ULPI_RWCTRL	(1 << 29)
> +
> +/* ULPI access modes */
> +#define ULPI_WRITE	0x00
> +#define ULPI_SET	0x01
> +#define ULPI_CLEAR	0x02
> +
> +struct ulpi_regs {
> +	/* Vendor ID and Product ID: 0x00 - 0x03 Read-only */
> +	u8	vendor_id_low;
> +	u8	vendor_id_high;
> +	u8	product_id_low;
> +	u8	product_id_high;
> +	/* Function Control: 0x04 - 0x06 Read */
> +	u8	function_ctrl;		/* 0x04 Write */
> +	u8	function_ctrl_set;	/* 0x05 Set */
> +	u8	function_ctrl_clear;	/* 0x06 Clear */
> +	/* Interface Control: 0x07 - 0x09 Read */
> +	u8	iface_ctrl;		/* 0x07 Write */
> +	u8	iface_ctrl_set;		/* 0x08 Set */
> +	u8	iface_ctrl_clear;	/* 0x09 Clear */
> +	/* OTG Control: 0x0A - 0x0C Read */
> +	u8	otg_ctrl;		/* 0x0A Write */
> +	u8	otg_ctrl_set;		/* 0x0B Set */
> +	u8	otg_ctrl_clear;		/* 0x0C Clear */
> +	/* USB Interrupt Enable Rising: 0x0D - 0x0F Read */
> +	u8	usb_ie_rising;		/* 0x0D Write */
> +	u8	usb_ie_rising_set;	/* 0x0E Set */
> +	u8	usb_ie_rising_clear;	/* 0x0F Clear */
> +	/* USB Interrupt Enable Falling: 0x10 - 0x12 Read */
> +	u8	usb_ie_falling;		/* 0x10 Write */
> +	u8	usb_ie_falling_set;	/* 0x11 Set */
> +	u8	usb_ie_falling_clear;	/* 0x12 Clear */
> +	/* USB Interrupt Status: 0x13 Read-only */
> +	u8	usb_int_status;
> +	/* USB Interrupt Latch: 0x14 Read-only with auto-clear */
> +	u8	usb_int_latch;
> +	/* Debug: 0x15 Read-only */
> +	u8	debug;
> +	/* Scratch Register: 0x16 - 0x18 Read */
> +	u8	scratch;		/* 0x16 Write */
> +	u8	scratch_set;		/* 0x17 Set */
> +	u8	scratch_clear;		/* 0x18 Clear */
> +	/*
> +	 * Optional Carkit registers
> +	 */
> +	/* Carkit Control: 0x19 - 0x1B Read */
> +	u8	carkit_ctrl;		/* 0x19 Write */
> +	u8	carkit_ctrl_set;	/* 0x1A Set */
> +	u8	carkit_ctrl_clear;	/* 0x1B Clear */
> +	/* Carkit Interrupt Delay: 0x1C Read, Write */
> +	u8	carkit_int_delay;
> +	/* Carkit Interrupt Enable: 0x1D - 0x1F Read */
> +	u8	carkit_ie;		/* 0x1D Write */
> +	u8	carkit_ie_set;		/* 0x1E Set */
> +	u8	carkit_ie_clear;	/* 0x1F Clear */
> +	/* Carkit Interrupt Status: 0x20 Read-only */
> +	u8	carkit_int_status;
> +	/* Carkit Interrupt Latch: 0x21 Read-only with auto-clear */
> +	u8	carkit_int_latch;
> +	/* Carkit Pulse Control: 0x22 - 0x24 Read */
> +	u8	carkit_pulse_ctrl;		/* 0x22 Write */
> +	u8	carkit_pulse_ctrl_set;		/* 0x23 Set */
> +	u8	carkit_pulse_ctrl_clear;	/* 0x24 Clear */
> +	/*
> +	 * Other optional registers
> +	 */
> +	/* Transmit Positive Width: 0x25 Read, Write */
> +	u8	transmit_pos_width;
> +	/* Transmit Negative Width: 0x26 Read, Write */
> +	u8	transmit_neg_width;
> +	/* Receive Polarity Recovery: 0x27 Read, Write */
> +	u8	recv_pol_recovery;
> +	/*
> +	 * Addresses 0x28 - 0x2E are reserved, so we use offsets
> +	 * for immediate registers with higher addresses
> +	 */
> +};
> +
> +/* Access Extended Register Set (indicator) */
> +#define ACCESS_EXT_REGS_OFFSET	0x2f	/* read-write */
> +/* Vendor-specific */
> +#define VENDOR_SPEC_OFFSET	0x30
> +
> +/*
> + * Extended Register Set
> + *
> + * Addresses 0x00-0x3F map directly to Immediate Register Set.
> + * Addresses 0x40-0x7F are reserved.
> + * Addresses 0x80-0xff are vendor-specific.
> + */
> +#define EXT_VENDOR_SPEC_OFFSET	0x80
> +
> +/*
> + * Register Bits
> + */
> +
> +/* Function Control */
> +#define ULPI_FC_XCVRSEL			(1 << 0)
> +#define ULPI_FC_XCVRSEL_MASK		(3 << 0)
> +#define ULPI_FC_HIGH_SPEED		(0 << 0)
> +#define ULPI_FC_FULL_SPEED		(1 << 0)
> +#define ULPI_FC_LOW_SPEED		(2 << 0)
> +#define ULPI_FC_FS4LS			(3 << 0)
> +#define ULPI_FC_TERMSELECT		(1 << 2)
> +#define ULPI_FC_OPMODE			(1 << 3)
> +#define ULPI_FC_OPMODE_MASK		(3 << 3)
> +#define ULPI_FC_OPMODE_NORMAL		(0 << 3)
> +#define ULPI_FC_OPMODE_NONDRIVING	(1 << 3)
> +#define ULPI_FC_OPMODE_DISABLE_NRZI	(2 << 3)
> +#define ULPI_FC_OPMODE_NOSYNC_NOEOP	(3 << 3)
> +#define ULPI_FC_RESET			(1 << 5)
> +#define ULPI_FC_SUSPENDM		(1 << 6)
> +
> +/* Interface Control */
> +#define ULPI_IFACE_6_PIN_SERIAL_MODE	(1 << 0)
> +#define ULPI_IFACE_3_PIN_SERIAL_MODE	(1 << 1)
> +#define ULPI_IFACE_CARKITMODE		(1 << 2)
> +#define ULPI_IFACE_CLOCKSUSPENDM	(1 << 3)
> +#define ULPI_IFACE_AUTORESUME		(1 << 4)
> +#define ULPI_IFACE_EXTVBUS_COMPLEMENT	(1 << 5)
> +#define ULPI_IFACE_PASSTHRU		(1 << 6)
> +#define ULPI_IFACE_PROTECT_IFC_DISABLE	(1 << 7)
> +
> +/* OTG Control */
> +#define ULPI_OTG_ID_PULLUP		(1 << 0)
> +#define ULPI_OTG_DP_PULLDOWN		(1 << 1)
> +#define ULPI_OTG_DM_PULLDOWN		(1 << 2)
> +#define ULPI_OTG_DISCHRGVBUS		(1 << 3)
> +#define ULPI_OTG_CHRGVBUS		(1 << 4)
> +#define ULPI_OTG_DRVVBUS		(1 << 5)
> +#define ULPI_OTG_DRVVBUS_EXT		(1 << 6)
> +#define ULPI_OTG_EXTVBUSIND		(1 << 7)
> +
> +/*
> + * USB Interrupt Enable Rising,
> + * USB Interrupt Enable Falling,
> + * USB Interrupt Status and
> + * USB Interrupt Latch
> + */
> +#define ULPI_INT_HOST_DISCONNECT		(1 << 0)
> +#define ULPI_INT_VBUS_VALID			(1 << 1)
> +#define ULPI_INT_SESS_VALID			(1 << 2)
> +#define ULPI_INT_SESS_END			(1 << 3)
> +#define ULPI_INT_IDGRD				(1 << 4)
> +
> +/* Debug */
> +#define ULPI_DEBUG_LINESTATE0			(1 << 0)
> +#define ULPI_DEBUG_LINESTATE1			(1 << 1)
> +
> +/* Carkit Control */
> +#define ULPI_CARKIT_CTRL_CARKITPWR		(1 << 0)
> +#define ULPI_CARKIT_CTRL_IDGNDDRV		(1 << 1)
> +#define ULPI_CARKIT_CTRL_TXDEN			(1 << 2)
> +#define ULPI_CARKIT_CTRL_RXDEN			(1 << 3)
> +#define ULPI_CARKIT_CTRL_SPKLEFTEN		(1 << 4)
> +#define ULPI_CARKIT_CTRL_SPKRIGHTEN		(1 << 5)
> +#define ULPI_CARKIT_CTRL_MICEN			(1 << 6)
> +
> +/* Carkit Interrupt Enable */
> +#define ULPI_CARKIT_INT_EN_IDFLOAT_RISE		(1 << 0)
> +#define ULPI_CARKIT_INT_EN_IDFLOAT_FALL		(1 << 1)
> +#define ULPI_CARKIT_INT_EN_CARINTDET		(1 << 2)
> +#define ULPI_CARKIT_INT_EN_DP_RISE		(1 << 3)
> +#define ULPI_CARKIT_INT_EN_DP_FALL		(1 << 4)
> +
> +/*
> + * Carkit Interrupt Status and
> + * Carkit Interrupt Latch
> + */
> +#define ULPI_CARKIT_INT_IDFLOAT			(1 << 0)
> +#define ULPI_CARKIT_INT_CARINTDET		(1 << 1)
> +#define ULPI_CARKIT_INT_DP			(1 << 2)
> +
> +/* Carkit Pulse Control*/
> +#define ULPI_CARKIT_PLS_CTRL_TXPLSEN		(1 << 0)
> +#define ULPI_CARKIT_PLS_CTRL_RXPLSEN		(1 << 1)
> +#define ULPI_CARKIT_PLS_CTRL_SPKRLEFT_BIASEN	(1 << 2)
> +#define ULPI_CARKIT_PLS_CTRL_SPKRRIGHT_BIASEN	(1 << 3)
> +
> +void ulpi_write(u32 ulpi_viewport, u32 reg, u32 value);
> +u32 ulpi_read(u32 ulpi_viewport, u32 reg);
> +
> +void ulpi_init(u32 ulpi_viewport);
> +
> +void ulpi_select_transceiver(u32 ulpi_viewport, int speed);
> +void ulpi_drive_vbus(u32 ulpi_viewport,
> +	int ext_power_supply, int use_ext_indicator);
> +void ulpi_dp_pulldown(u32 ulpi_viewport, int enable);
> +void ulpi_dm_pulldown(u32 ulpi_viewport, int enable);
> +void ulpi_select_opmode(u32 ulpi_viewport, int style);
> +void ulpi_suspend(u32 ulpi_viewport);
> +void ulpi_resume(u32 ulpi_viewport);
> +void ulpi_reset(u32 ulpi_viewport);
> +void ulpi_charge_vbus(u32 ulpi_viewport);
> +#endif /* __USB_ULPI_H */

Overall, the API is useful, the patch looks good and almost ready
for submission.
Please, make those final changes and we're done.

Thanks for doing this.

> Hi Jana,
> 
> Good job!
> IMO, this version looks much better
> (although Marek is not happy with its schedule).

Yes, you're not getting ACK on my side for this. Ever. I'm not reviewing this 
shit anymore.

2011/11/24 Igor Grinberg <grinberg@compulab.co.il>

> > +/*
> > + * If enable is 0, pull-down resistor not connected to D+, else
> pull-down
> > + * resistor connected to D+.
> > + * Default behaviour is as for enable equal to 1.
> > + */
> > +void ulpi_dp_pulldown(u32 ulpi_viewport, int enable)
> > +{
> > +     if (enable)
> > +             ulpi_write(ulpi_viewport,
> > +                     (u32)&ulpi->otg_ctrl_set, ULPI_OTG_DP_PULLDOWN);
> > +     else
> > +             ulpi_write(ulpi_viewport,
> > +                     (u32)&ulpi->otg_ctrl_clear, ULPI_OTG_DP_PULLDOWN);
> > +}
> > +
> > +/*
> > + * If enable is 0, pull-down resistor not connected to D- else pull-down
> > + * resistor connected to D-.
> > + * Default behaviour is as for enable equal to 1.
> > + */
> > +void ulpi_dm_pulldown(u32 ulpi_viewport, int enable)
> > +{
> > +     if (enable)
> > +             ulpi_write(ulpi_viewport,
> > +                     (u32)&ulpi->otg_ctrl_set, ULPI_OTG_DM_PULLDOWN);
> > +     else
> > +             ulpi_write(ulpi_viewport,
> > +                     (u32)&ulpi->otg_ctrl_clear, ULPI_OTG_DM_PULLDOWN);
> > +}
>
>
> Correct me if I'm wrong, but I don't think there is a use for
> the above functions in separate and the user will have to
> call them both.
> So, can these two functions be united in one,
> say ulpi_pulldown(u32 ..., int enable)?
>
>
If I understand ULPI specification well, the overall effect is the same
when both bits are set to 1 as when they are set to 0.
And default setting is for both bits to be 1, so I don't think that have
one function for both bits make sense.

Regards,
Jana Rapava

On 11/25/11 20:39, Jana Rapava wrote:
> 
> 
> 2011/11/24 Igor Grinberg <grinberg@compulab.co.il <mailto:grinberg@compulab.co.il>>
> 
>     > +/*
>     > + * If enable is 0, pull-down resistor not connected to D+, else pull-down
>     > + * resistor connected to D+.
>     > + * Default behaviour is as for enable equal to 1.
>     > + */
>     > +void ulpi_dp_pulldown(u32 ulpi_viewport, int enable)
>     > +{
>     > +     if (enable)
>     > +             ulpi_write(ulpi_viewport,
>     > +                     (u32)&ulpi->otg_ctrl_set, ULPI_OTG_DP_PULLDOWN);
>     > +     else
>     > +             ulpi_write(ulpi_viewport,
>     > +                     (u32)&ulpi->otg_ctrl_clear, ULPI_OTG_DP_PULLDOWN);
>     > +}
>     > +
>     > +/*
>     > + * If enable is 0, pull-down resistor not connected to D- else pull-down
>     > + * resistor connected to D-.
>     > + * Default behaviour is as for enable equal to 1.
>     > + */
>     > +void ulpi_dm_pulldown(u32 ulpi_viewport, int enable)
>     > +{
>     > +     if (enable)
>     > +             ulpi_write(ulpi_viewport,
>     > +                     (u32)&ulpi->otg_ctrl_set, ULPI_OTG_DM_PULLDOWN);
>     > +     else
>     > +             ulpi_write(ulpi_viewport,
>     > +                     (u32)&ulpi->otg_ctrl_clear, ULPI_OTG_DM_PULLDOWN);
>     > +}
> 
> 
>     Correct me if I'm wrong, but I don't think there is a use for
>     the above functions in separate and the user will have to
>     call them both.
>     So, can these two functions be united in one,
>     say ulpi_pulldown(u32 ..., int enable)?
> 
> 
> If I understand ULPI specification well, the overall effect is the same
> when both bits are set to 1 as when they are set to 0.
> And default setting is for both bits to be 1, so I don't think that have
> one function for both bits make sense.

It has nothing to do with the ULPI spec, but with the USB (or USB2.0) spec.
This technique is used for speed identification, so for both, Low and Full
speed devices to function properly, both resistors must be enabled.

So for sake of correctness, please, provide one unite function.