ulpi: add generic ULPI functionality

PATCH New Jana Rapava 4 participants 29 messages Nov 5, 2011 – Dec 1, 2011

Subsystems (2)

USB Kbuild

lore

Share & tools

lore

Jana Rapava

author

2011-11-05 20:50 UTC #0 lore raw

PATCH 1/1 ulpi: add generic ULPI functionality

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

Signed-off-by Jana Rapava <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

---
 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   44 ++++++++++++++
 drivers/usb/ulpi/ulpi-viewport.c |   87 +++++++++++++++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  123 ++++++++++++++++++++++++++++++++++++++
 include/usb/ulpi.h               |   16 +++++
 5 files changed, 271 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

Makefile +1 -0

index 571c3eb..a475cb9 100644
@@ -283,6 +283,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

drivers/usb/ulpi/Makefile new file +44 -0

44 lines not on this page · raw message

drivers/usb/ulpi/ulpi-viewport.c new file +87 -0

87 lines not on this page · raw message

drivers/usb/ulpi/ulpi.c new file +123 -0

123 lines not on this page · raw message

include/usb/ulpi.h +16 -0

index 1519cc5..a8625a1 100644
@@ -15,6 +15,8 @@
 #ifndef __USB_ULPI_H
 #define __USB_ULPI_H
 
+#include <usb/ehci-fsl.h>
+
 #define ULPI_ID_REGS_COUNT	4
 #define ULPI_TEST_VALUE		0x55
 #define ULPI_TIMEOUT		1000 /* some reasonable value */

@@ -25,6 +27,11 @@
 #define ULPI_RWRUN	(1 << 30)
 #define ULPI_RWCTRL	(1 << 29)
 
+/* ULPI access modes */
+#define WRITE	0x00
+#define SET	0x01
+#define CLEAR	0x02
+
 struct ulpi_regs {
 	/* Vendor ID and Product ID: 0x00 - 0x03 Read-only */
 	u8	vendor_id_low;

@@ -201,4 +208,13 @@ struct ulpi_regs {
 void ulpi_write(struct usb_ehci *ehci, u32 reg, u32 value);
 u32 ulpi_read(struct usb_ehci *ehci, u32 reg);
 
+void ulpi_init(struct usb_ehci *ehci, struct ulpi_regs *ulpi);
+
+void ulpi_otg_ctrl_flags
+	(struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags);
+void ulpi_function_ctrl_flags
+	(struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags);
+void ulpi_iface_ctrl_flags
+	(struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags);
+
 #endif /* __USB_ULPI_H */

Signature · 1 line

1.7.6.3

No, Linux has offset-based access to ULPI registers, some structure otg_transceiver, where the driver sets the bits which it wants to be set in ULPI registers (if I understand it well) and family of functions, which set bits according to informations in otg_transceiver.

Marek Vasut

2011-11-05 23:13 UTC #3 lore raw

PATCH 1/1 ulpi: add generic ULPI functionality

Quoted 9 lines

2011/11/5 Marek Vasut <[email protected]>
Quoted 3 lines
Quoted 1 line
+int ulpi_wait(struct usb_ehci *ehci, u32 ulpi_value, u32 ulpi_mask)
So this works only with EHCI? How generic is it then ?
I thought until now that ULPI is EHCI-dependent, but isn't... Ok, what else should be supported? OHCI? I haven't any hardware to test it, but I could give it a try.

What about xHCI? I have no idea about OHCI, but why won't you be able to have OHCI and ULPI PHY?

Quoted 23 lines

Quoted 6 lines

Review

Quoted 3 lines

(quoted diff) +2 -0

+void ulpi_iface_ctrl_flags
+     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode,

u32

Quoted 16 lines

Quoted 4 lines

flags) +{

drivers/usb/ulpi/ulpi.c new file +3 -0

+     switch (access_mode) {
+     case WRITE:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_write, flags);

drivers/usb/ulpi/ulpi.c line 108

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+             break;
+     case SET:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_set, flags);
+             break;
+     case CLEAR:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_clear, flags);
+             break;
+     }
+
+}

Is this crap from linux or something?

Quoted 4 lines

No, Linux has offset-based access to ULPI registers, some structure otg_transceiver, where the driver sets the bits which it wants to be set in ULPI registers (if I understand it well) and family of functions, which set bits according to informations in otg_transceiver.

Ok, you have writel() functions, why do you need this switch stuff ?

Jana Rapava

author

2011-11-05 23:32 UTC #4 lore raw

PATCH 1/1 ulpi: add generic ULPI functionality

2011/11/6 Marek Vasut <[email protected]>

Quoted 6 lines

Quoted 6 lines
2011/11/5 Marek Vasut <[email protected]>
Quoted 3 lines
Quoted 1 line
+int ulpi_wait(struct usb_ehci *ehci, u32 ulpi_value, u32 ulpi_mask)
So this works only with EHCI? How generic is it then ?

Quoted 9 lines

Quoted 1 line
I thought until now that ULPI is EHCI-dependent, but isn't... Ok, what
else
Quoted 2 lines
should be supported? OHCI? I haven't any hardware to test it, but I could give it a try.
What about xHCI? I have no idea about OHCI, but why won't you be able to have
OHCI and ULPI PHY?

I'll look at it.

Quoted 24 lines

Quoted 3 lines

Quoted 3 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+void ulpi_iface_ctrl_flags
+     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int

access_mode,

Quoted 20 lines

Quoted 19 lines

u32

Quoted 4 lines

flags) +{

drivers/usb/ulpi/ulpi.c new file +3 -0

+     switch (access_mode) {
+     case WRITE:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_write, flags);

drivers/usb/ulpi/ulpi.c line 108

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+             break;
+     case SET:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_set, flags);
+             break;
+     case CLEAR:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_clear, flags);
+             break;
+     }
+
+}

Is this crap from linux or something?

Quoted 9 lines

Quoted 2 lines
No, Linux has offset-based access to ULPI registers, some structure otg_transceiver, where the driver sets the bits which it wants to be set
in
Quoted 1 line
ULPI registers (if I understand it well) and family of functions, which
set
Quoted 1 line
bits according to informations in otg_transceiver.
Ok, you have writel() functions, why do you need this switch stuff ?

I tried to design some interface, which would allow its user care only about register, access mode and flags and not about the exact way this huge bunch od u8 fields called "ulpi_regs" is implemented.

Marek Vasut

2011-11-05 23:35 UTC #5 lore raw

PATCH 1/1 ulpi: add generic ULPI functionality

Quoted 56 lines

2011/11/6 Marek Vasut <[email protected]>
Quoted 9 lines
Quoted 8 lines
2011/11/5 Marek Vasut <[email protected]>
Quoted 4 lines
Quoted 2 lines
+int ulpi_wait(struct usb_ehci *ehci, u32 ulpi_value, u32 ulpi_mask)
So this works only with EHCI? How generic is it then ?
I thought until now that ULPI is EHCI-dependent, but isn't... Ok, what
Quoted 8 lines
else
Quoted 2 lines
should be supported? OHCI? I haven't any hardware to test it, but I could give it a try.
What about xHCI? I have no idea about OHCI, but why won't you be able to have
OHCI and ULPI PHY?
I'll look at it.
Quoted 34 lines
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+void ulpi_iface_ctrl_flags
+     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int
access_mode,
Quoted 19 lines
Quoted 18 lines
u32
Quoted 11 lines
flags) +{
drivers/usb/ulpi/ulpi.c new file +3 -0
+     switch (access_mode) {
+     case WRITE:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_write,
flags); + break;
drivers/usb/ulpi/ulpi.c new file +5 -0
+     case SET:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_set, flags);
+             break;
+     case CLEAR:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_clear,
flags); + break;
drivers/usb/ulpi/ulpi-viewport.c line 51
Quoted 3 lines
drivers/usb/ulpi/ulpi-viewport.c new file +3 -0
+     }
+
+}
Is this crap from linux or something?
Quoted 3 lines
No, Linux has offset-based access to ULPI registers, some structure otg_transceiver, where the driver sets the bits which it wants to be set
in
Quoted 1 line
ULPI registers (if I understand it well) and family of functions, which
set

Quoted 7 lines

Quoted 3 lines
Quoted 1 line
bits according to informations in otg_transceiver.
Ok, you have writel() functions, why do you need this switch stuff ?
I tried to design some interface, which would allow its user care only about register, access mode and flags and not about the exact way this huge bunch od u8 fields called "ulpi_regs" is implemented.

I'd assume the interface will expose something like:

* ULPI power up port
* ULPI power down power

maybe something else, no?

Igor Grinberg

2011-11-08 11:08 UTC #6 lore raw

PATCH 1/1 ulpi: add generic ULPI functionality

In reply to Jana Rapava

Quoted 49 lines · Jana Rapava

2011/11/6 Marek Vasut <marek.vasut at gmail.com <mailto:[email protected]>>
Quoted 9 lines
2011/11/5 Marek Vasut <marek.vasut at gmail.com <mailto:[email protected]>>
Quoted 3 lines
Quoted 1 line
+int ulpi_wait(struct usb_ehci *ehci, u32 ulpi_value, u32 ulpi_mask)
So this works only with EHCI? How generic is it then ?
I thought until now that ULPI is EHCI-dependent, but isn't... Ok, what else should be supported? OHCI? I haven't any hardware to test it, but I could give it a try.
What about xHCI? I have no idea about OHCI, but why won't you be able to have OHCI and ULPI PHY?
I'll look at it.
Quoted 23 lines
Quoted 6 lines
Review
Quoted 3 lines
(quoted diff) +2 -0
+void ulpi_iface_ctrl_flags
+     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode,
u32
Quoted 16 lines
Quoted 4 lines
flags) +{
drivers/usb/ulpi/ulpi.c new file +3 -0
+     switch (access_mode) {
+     case WRITE:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_write, flags);
drivers/usb/ulpi/ulpi.c line 108
Quoted 10 lines
drivers/usb/ulpi/ulpi.c new file +10 -0
+             break;
+     case SET:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_set, flags);
+             break;
+     case CLEAR:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_clear, flags);
+             break;
+     }
+
+}
Is this crap from linux or something?
Quoted 4 lines
No, Linux has offset-based access to ULPI registers, some structure otg_transceiver, where the driver sets the bits which it wants to be set in ULPI registers (if I understand it well) and family of functions, which set bits according to informations in otg_transceiver.

Quoted 4 lines

Ok, you have writel() functions, why do you need this switch stuff ?
I tried to design some interface, which would allow its user care only about register, access mode and flags and not about the exact way this huge bunch od u8 fields called "ulpi_regs" is implemented.

User should neither care about the register, nor the access mode. User should ask the driver to do "something useful" (e.g. put the phy to some mode, enable vbus, etc.),
and that can be done with flags and functional API.

Signature · 2 lines

Regards,
Igor.

Igor Grinberg

2011-11-08 11:33 UTC #7 lore raw

PATCH 1/1 ulpi: add generic ULPI functionality

In reply to Jana Rapava

Hi Jana,

Thanks for porting this.
Several comments below.

Quoted 53 lines · Jana Rapava

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

Signed-off-by Jana Rapava <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

---
 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   44 ++++++++++++++
 drivers/usb/ulpi/ulpi-viewport.c |   87 +++++++++++++++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  123 ++++++++++++++++++++++++++++++++++++++
 include/usb/ulpi.h               |   16 +++++
 5 files changed, 271 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

Makefile +1 -0

index 571c3eb..a475cb9 100644
@@ -283,6 +283,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

drivers/usb/ulpi/Makefile new file +16 -0

new file mode 100644
index 0000000..f7b6e20
@@ -0,0 +1,44 @@
+#
+# Copyright (C) 2011 Jana Rapava <[email protected]>
+# 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

drivers/usb/ulpi/Makefile line 26

Quoted 10 lines

drivers/usb/ulpi/Makefile new file +10 -0

+# 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 ulpi-viewport.o

This switch is fine for now, but not all viewport implementations can use the ulpi-viewport.c file. Please split, so we can have:
COBJS-$(CONFIG_USB_ULPI) += ulpi.o
which is generic and:
COBJS-$(CONFIG_USB_ULPI_VIEWPORT) += ulpi-viewport.o which is more hardware specific.

Quoted 49 lines

drivers/usb/ulpi/Makefile new file +18 -0

+
+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
+
+#########################################################################

drivers/usb/ulpi/ulpi-viewport.c new file +25 -0

new file mode 100644
index 0000000..a0c213e
@@ -0,0 +1,87 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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 <watchdog.h>
+#include <asm/io.h>
+#include <usb/ulpi.h>
+#include <usb/ehci-fsl.h>
+
+/* ULPI viewport control bits */

drivers/usb/ulpi/ulpi-viewport.c line 35

Quoted 10 lines

drivers/usb/ulpi/ulpi-viewport.c new file +10 -0

+#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)
+
+int ulpi_wait(struct usb_ehci *ehci, u32 ulpi_value, u32 ulpi_mask)

As Marek already stated, ULPI can be used with various interfaces (e.g. EHCI, OHCI, XHCI), so here I'd suggest having:
u32 *ulpi_viewpoint
which is currently generic enough instead of:
struct usb_ehci *ehci
which is EHCI specific, in all ulpi_*() functions

Also, if this function is not used outside of this file, please mark it static.

drivers/usb/ulpi/ulpi-viewport.c line 48

Quoted 13 lines

drivers/usb/ulpi/ulpi-viewport.c new file +13 -0

+{
+	int timeout = ULPI_TIMEOUT;
+	u32 tmp;
+
+	writel(ulpi_value, &ehci->ulpi_viewpoint);
+
+	/* Wait for the bits in ulpi_mask to become zero. */
+	while (--timeout) {
+		tmp = readl(&ehci->ulpi_viewpoint);
+		if (!(tmp & ulpi_mask))
+			break;
+		WATCHDOG_RESET();
+	}

This loop does not have any timing constraints, so basically it depends on the system frequency and some arbitrary timeout value, which can be right/wrong on variable operational frequencies? This is not good.
You need to use one of the ?delay() functions here.

drivers/usb/ulpi/ulpi-viewport.c line 53

Quoted 5 lines

drivers/usb/ulpi/ulpi-viewport.c new file +5 -0
+
+	return !timeout;
+}
+
+int ulpi_wakeup(struct usb_ehci *ehci)

same here

drivers/usb/ulpi/ulpi-viewport.c line 56

Quoted 3 lines

drivers/usb/ulpi/ulpi-viewport.c new file +3 -0

+{
+	if (readl(&ehci->ulpi_viewpoint) & ULPI_SS)
+		return 0; /* already awake */

Please, empty line here.

Quoted 18 lines

drivers/usb/ulpi/ulpi-viewport.c new file +18 -0

+	return ulpi_wait(ehci, ULPI_WU, ULPI_WU);
+}
+
+void ulpi_write(struct usb_ehci *ehci, u32 reg, u32 value)
+{
+	u32 tmp;
+	if (ulpi_wakeup(ehci)) {
+		printf("ULPI wakeup timed out\n");
+		return;
+	}
+
+	tmp = ulpi_wait(ehci, 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(struct usb_ehci *ehci, u32 reg)

drivers/usb/ulpi/ulpi-viewport.c line 84

Quoted 10 lines

drivers/usb/ulpi/ulpi-viewport.c new file +10 -0

+{
+	if (ulpi_wakeup(ehci)) {
+		printf("ULPI wakeup timed out\n");
+		return 0;
+	}
+
+	if (ulpi_wait(ehci, ULPI_RWRUN | reg << ULPI_ADDR_SHIFT, ULPI_RWRUN)) {
+		printf("ULPI read timed out\n");
+		return 0;
+	}

Is 0 the best esoteric value we can return on failure? Can't it be a real read value?
Also, shouldn't we define it as ULPI_ERROR or something?

Quoted 26 lines

drivers/usb/ulpi/ulpi-viewport.c new file +3 -0

+
+	return ulpi_read_mask(readl(&ehci->ulpi_viewpoint));
+}

drivers/usb/ulpi/ulpi.c new file +17 -0

new file mode 100644
index 0000000..b1c482a
@@ -0,0 +1,123 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * Based on:
+ * linux/drivers/usb/otg/ulpi.c
+ * Generic ULPI USB transceiver support
+ *
+ * Original Copyright follow:
+ * Copyright (C) 2009 Daniel Mack <[email protected]>
+ *
+ * Based on sources from
+ *
+ *   Sascha Hauer <[email protected]>
+ *   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

drivers/usb/ulpi/ulpi.c line 27

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+ * (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., 675 Mass Ave, Cambridge, MA 02139, USA.

Are you sure that it is the right address?
May be just remove the address...

drivers/usb/ulpi/ulpi.c line 31

Quoted 4 lines

drivers/usb/ulpi/ulpi.c new file +4 -0
+ */
+
+#include <usb/ulpi.h>
+#include <usb/ehci-fsl.h>

Should not include EHCI specific stuff here.

drivers/usb/ulpi/ulpi.c line 40

Quoted 9 lines

drivers/usb/ulpi/ulpi.c new file +9 -0

+#include <exports.h>
+
+#ifdef DEBUG
+#define debug(fmt, args...)	printf(fmt, ##args)
+#else
+#define debug(fmt, args...)
+#endif /* DEBUG */
+
+void ulpi_integrity_check(struct usb_ehci *ehci, struct ulpi_regs *ulpi)

Should not be EHCI specific.

Quoted 8 lines

drivers/usb/ulpi/ulpi.c new file +8 -0

+{
+	u32 tmp = 0;
+	int i;
+	for (i = 0; i < 2; i++) {
+		ulpi_write(ehci, (u32)&ulpi->scratch_write,
+			ULPI_TEST_VALUE << i);
+		tmp = ulpi_read(ehci, (u32)&ulpi->scratch_write);
+

drivers/usb/ulpi/ulpi.c line 58

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+		if (tmp != (ULPI_TEST_VALUE << i)) {
+			printf("ULPI integrity check failed\n");
+			return;
+		}
+	}
+}
+
+/*
+ * This is a family of wrapper functions which sets bits in ULPI registers.
+ * Access mode could be WRITE, SET or CLEAR.

What about READ?
I know it can be done from any of those registers, but it is confusing.

Quoted 40 lines

drivers/usb/ulpi/ulpi.c new file +40 -0

+ * For further informations see ULPI 1.1 specification.
+ */
+void ulpi_otg_ctrl_flags
+	(struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags)
+{
+	switch (access_mode) {
+	case WRITE:
+		ulpi_write(ehci, (u32)&ulpi->otg_ctrl_write, flags);
+		break;
+	case SET:
+		ulpi_write(ehci, (u32)&ulpi->otg_ctrl_set, flags);
+		break;
+	case CLEAR:
+		ulpi_write(ehci, (u32)&ulpi->otg_ctrl_clear, flags);
+		break;
+	}
+}
+
+void ulpi_function_ctrl_flags
+	(struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags)
+{
+	switch (access_mode) {
+	case WRITE:
+		ulpi_write(ehci, (u32)&ulpi->function_ctrl_write, flags);
+		break;
+	case SET:
+		ulpi_write(ehci, (u32)&ulpi->function_ctrl_set, flags);
+		break;
+	case CLEAR:
+		ulpi_write(ehci, (u32)&ulpi->function_ctrl_clear, flags);
+		break;
+	}
+}
+
+void ulpi_iface_ctrl_flags
+	(struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags)
+{
+	switch (access_mode) {
+	case WRITE:
+		ulpi_write(ehci, (u32)&ulpi->iface_ctrl_write, flags);

drivers/usb/ulpi/ulpi.c line 108

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+		break;
+	case SET:
+		ulpi_write(ehci, (u32)&ulpi->iface_ctrl_set, flags);
+		break;
+	case CLEAR:
+		ulpi_write(ehci, (u32)&ulpi->iface_ctrl_clear, flags);
+		break;
+	}
+
+}

All the above functions are just making users hard to understand what they should do and what information they should know and supply to the driver.
More function oriented API would be much more useful here.

drivers/usb/ulpi/ulpi.c line 120

Quoted 12 lines

drivers/usb/ulpi/ulpi.c new file +12 -0

+
+void ulpi_init(struct usb_ehci *ehci, struct ulpi_regs *ulpi)
+{
+	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(ehci, reg) << (reg * 8);
+
+	/* Split ID into vendor and product ID. */
+	debug("Found ULPI TX, ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);

What is TX? Is it transceiver or xceiver or both in one?

include/usb/ulpi.h line 19

Quoted 13 lines

drivers/usb/ulpi/ulpi.c new file +3 -0

+
+	ulpi_integrity_check(ehci, ulpi);
+}

include/usb/ulpi.h +2 -0

index 1519cc5..a8625a1 100644
@@ -15,6 +15,8 @@
 #ifndef __USB_ULPI_H
 #define __USB_ULPI_H
 
+#include <usb/ehci-fsl.h>
+

This should not be here.

include/usb/ulpi.h line 33

Quoted 11 lines

include/usb/ulpi.h +4 -0

 #define ULPI_ID_REGS_COUNT	4
 #define ULPI_TEST_VALUE		0x55
 #define ULPI_TIMEOUT		1000 /* some reasonable value */

@@ -25,6 +27,11 @@
 #define ULPI_RWRUN	(1 << 30)
 #define ULPI_RWCTRL	(1 << 29)
 
+/* ULPI access modes */
+#define WRITE	0x00
+#define SET	0x01
+#define CLEAR	0x02

Even if we go that way (which I think is wrong), the names are too generic.
It should be at least prefixed with ULPI_*.

Quoted 18 lines

include/usb/ulpi.h +10 -0

+
 struct ulpi_regs {
 	/* Vendor ID and Product ID: 0x00 - 0x03 Read-only */
 	u8	vendor_id_low;

@@ -201,4 +208,13 @@ struct ulpi_regs {
 void ulpi_write(struct usb_ehci *ehci, u32 reg, u32 value);
 u32 ulpi_read(struct usb_ehci *ehci, u32 reg);
 
+void ulpi_init(struct usb_ehci *ehci, struct ulpi_regs *ulpi);
+
+void ulpi_otg_ctrl_flags
+	(struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags);
+void ulpi_function_ctrl_flags
+	(struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags);
+void ulpi_iface_ctrl_flags
+	(struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags);
+
 #endif /* __USB_ULPI_H */

Signature · 2 lines

Regards,
Igor.

Jana Rapava

author

2011-11-12 01:09 UTC #8 lore raw

PATCH 1/1 ulpi: add generic ULPI functionality

2011/11/8 Igor Grinberg <[email protected]>

Quoted 59 lines

Quoted 3 lines

+/*
+ * This is a family of wrapper functions which sets bits in ULPI

registers.

Quoted 1 line

+ * Access mode could be WRITE, SET or CLEAR.

What about READ?
I know it can be done from any of those registers, but it is confusing.

Review

Quoted 4 lines

+ * For further informations see ULPI 1.1 specification.
+ */
(quoted diff) +2 -0
+void ulpi_otg_ctrl_flags
+     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode,

u32 flags)

Quoted 6 lines

drivers/usb/ulpi/ulpi.c new file +6 -0

+{
+     switch (access_mode) {
+     case WRITE:
+             ulpi_write(ehci, (u32)&ulpi->otg_ctrl_write, flags);
+             break;
+     case SET:

drivers/usb/ulpi/ulpi.c

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+             ulpi_write(ehci, (u32)&ulpi->otg_ctrl_set, flags);
+             break;
+     case CLEAR:
+             ulpi_write(ehci, (u32)&ulpi->otg_ctrl_clear, flags);
+             break;
+     }
+}
+
+void ulpi_function_ctrl_flags
+     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode,

u32 flags)

Quoted 6 lines

drivers/usb/ulpi/ulpi.c new file +6 -0

+{
+     switch (access_mode) {
+     case WRITE:
+             ulpi_write(ehci, (u32)&ulpi->function_ctrl_write, flags);
+             break;
+     case SET:

drivers/usb/ulpi/ulpi.c

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+             ulpi_write(ehci, (u32)&ulpi->function_ctrl_set, flags);
+             break;
+     case CLEAR:
+             ulpi_write(ehci, (u32)&ulpi->function_ctrl_clear, flags);
+             break;
+     }
+}
+
+void ulpi_iface_ctrl_flags
+     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode,

u32 flags)

Quoted 11 lines

drivers/usb/ulpi/ulpi.c new file +11 -0

+{
+     switch (access_mode) {
+     case WRITE:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_write, flags);
+             break;
+     case SET:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_set, flags);
+             break;
+     case CLEAR:
+             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_clear, flags);
+             break;

Quoted 10 lines

drivers/usb/ulpi/ulpi-viewport.c line 51
Quoted 3 lines
drivers/usb/ulpi/ulpi-viewport.c new file +3 -0
+     }
+
+}
All the above functions are just making users hard to understand what they should do and what information they should know and supply to the driver.
More function oriented API would be much more useful here.

Please, could you be more specific? I would be thankful, because I know only Linux kernel ULPI API, where driver has to fill otg_transciever structure with bits it needs to set (if I understand it well) and I would like to use something more lightweight here.

Quoted 6 lines

drivers/usb/ulpi/ulpi.c new file +6 -0

+
+void ulpi_init(struct usb_ehci *ehci, struct ulpi_regs *ulpi)
+{
+     u32 tmp = 0;
+     int reg;
+

Quoted 10 lines

drivers/usb/ulpi/ulpi.c line 120

Quoted 6 lines

drivers/usb/ulpi/ulpi.c new file +6 -0

+     /* Assemble ID from four ULPI ID registers (8 bits each). */
+     for (reg = ULPI_ID_REGS_COUNT - 1; reg >= 0; reg--)
+             tmp |= ulpi_read(ehci, reg) << (reg * 8);
+
+     /* Split ID into vendor and product ID. */
+     debug("Found ULPI TX, ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);

What is TX? Is it transceiver or xceiver or both in one?

According to ULPI 1.1 specification, it is transceiver.

Quoted 3 lines

Regards,
Igor.

Regards,
Jana Rapava

Igor Grinberg

2011-11-14 07:40 UTC #9 lore raw

PATCH 1/1 ulpi: add generic ULPI functionality

Hi Jana,

Quoted 69 lines · Jana Rapava

2011/11/8 Igor Grinberg <grinberg at compulab.co.il <mailto:[email protected]>>

Quoted 3 lines

+/*
+ * This is a family of wrapper functions which sets bits in ULPI registers.
+ * Access mode could be WRITE, SET or CLEAR.

    What about READ?
    I know it can be done from any of those registers, but it is confusing.

    > + * For further informations see ULPI 1.1 specification.
    > + */
    > +void ulpi_otg_ctrl_flags
    > +     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags)
    > +{
    > +     switch (access_mode) {
    > +     case WRITE:
    > +             ulpi_write(ehci, (u32)&ulpi->otg_ctrl_write, flags);
    > +             break;
    > +     case SET:
    > +             ulpi_write(ehci, (u32)&ulpi->otg_ctrl_set, flags);
    > +             break;
    > +     case CLEAR:
    > +             ulpi_write(ehci, (u32)&ulpi->otg_ctrl_clear, flags);
    > +             break;
    > +     }
    > +}
    > +
    > +void ulpi_function_ctrl_flags
    > +     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags)
    > +{
    > +     switch (access_mode) {
    > +     case WRITE:
    > +             ulpi_write(ehci, (u32)&ulpi->function_ctrl_write, flags);
    > +             break;
    > +     case SET:
    > +             ulpi_write(ehci, (u32)&ulpi->function_ctrl_set, flags);
    > +             break;
    > +     case CLEAR:
    > +             ulpi_write(ehci, (u32)&ulpi->function_ctrl_clear, flags);
    > +             break;
    > +     }
    > +}
    > +
    > +void ulpi_iface_ctrl_flags
    > +     (struct usb_ehci *ehci, struct ulpi_regs *ulpi, int access_mode, u32 flags)
    > +{
    > +     switch (access_mode) {
    > +     case WRITE:
    > +             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_write, flags);
    > +             break;
    > +     case SET:
    > +             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_set, flags);
    > +             break;
    > +     case CLEAR:
    > +             ulpi_write(ehci, (u32)&ulpi->iface_ctrl_clear, flags);
    > +             break;
    > +     }
    > +
    > +}

    All the above functions are just making users hard to understand
    what they should do and what information they should know and
    supply to the driver.
    More function oriented API would be much more useful here.

Quoted 1 line

Please, could you be more specific? I would be thankful, because I know only Linux kernel ULPI API, where driver has to fill otg_transciever structure with bits it needs to set (if I understand it well) and I would like to use something more lightweight here.

Please, don't write long long lines, they are extremely hard to read.

Now regarding the API:
User needs to use the *functionality* without having the need to look into the ULPI spec to figure out the bits that are needed for that functionality (I'm not sure it is possible, but we should try). What I suggest is to look into your use case with mx5, identify the functionality that is needed for it to function properly and implement it.

Currently, in your use case, I can identify following functionalities:
1) selecting the transceiver type
2) selecting vbus indicator type
3) Dp/Dm pull downs enable/disable
4) selecting operation mode
5) suspend/resume - can be useful for usb start/stop commands
6) reset
7) drive vbus internal/external
8) ...

You are writing a generic driver, right?
Then you should also go through the ULPI spec and see what functionalities it provides.
Of course you don't have to implement all the functionalities described by the spec (although it would be very nice of you), but at least those that your board uses and in a generic way.

Quoted 17 lines

drivers/usb/ulpi/ulpi.c line 120

Quoted 13 lines

drivers/usb/ulpi/ulpi.c new file +12 -0

+
+void ulpi_init(struct usb_ehci *ehci, struct ulpi_regs *ulpi)
+{
+     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(ehci, reg) << (reg * 8);
+
+     /* Split ID into vendor and product ID. */
+     debug("Found ULPI TX, ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);

What is TX? Is it transceiver or xceiver or both in one?

Quoted 1 line

According to ULPI 1.1 specification, it is transceiver.

then can it be transceiver instead of TX?

Signature · 2 lines

Regards,
Igor.

Jana Rapava

author

2011-11-12 17:29 UTC #10 lore raw

PATCH v2 1/1 ulpi: add generic ULPI functionality

In reply to Jana Rapava

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

Signed-off-by Jana Rapava <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

---
This patch is a prerequisite for Efika USB patchset.

Changes for v2:
- make code EHCI-independent
- use udelay() in waiting loop
- mark static functions as static
- naming changes
I haven't rewritten the ULPI registers interface, because I haven't any idea how to implement it in some better way.

 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   45 +++++++++++++++
 drivers/usb/ulpi/ulpi-viewport.c |   87 +++++++++++++++++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  113 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 246 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

Makefile +1 -0

index dfe939f..70a1e1e 100644
@@ -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

drivers/usb/ulpi/Makefile new file +45 -0

45 lines not on this page · raw message

drivers/usb/ulpi/ulpi-viewport.c new file +87 -0

87 lines not on this page · raw message

drivers/usb/ulpi/ulpi.c new file +113 -0

113 lines not on this page · raw message

Signature · 1 line

1.7.6.3

Igor Grinberg

2011-11-14 08:13 UTC #11 lore raw

PATCH v2 1/1 ulpi: add generic ULPI functionality

Hi Jana,

Quoted 64 lines · Jana Rapava

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

Signed-off-by Jana Rapava <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

---
This patch is a prerequisite for Efika USB patchset.

 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   45 +++++++++++++++
 drivers/usb/ulpi/ulpi-viewport.c |   87 +++++++++++++++++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  113 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 246 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

Makefile +1 -0

index dfe939f..70a1e1e 100644
@@ -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

drivers/usb/ulpi/Makefile new file +17 -0

new file mode 100644
index 0000000..64561af
@@ -0,0 +1,45 @@
+#
+# Copyright (C) 2011 Jana Rapava <[email protected]>
+# 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

drivers/usb/ulpi/Makefile line 27

Quoted 10 lines

drivers/usb/ulpi/Makefile new file +10 -0

+# 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

It would be nice to align the above, but only if you want (you don't have to).

Quoted 205 lines

drivers/usb/ulpi/Makefile new file +18 -0

+
+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
+
+#########################################################################

drivers/usb/ulpi/ulpi-viewport.c new file +87 -0

new file mode 100644
index 0000000..9250b09
@@ -0,0 +1,87 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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 = 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)
+{
+	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 0;
+	}
+
+	if (ulpi_wait(ulpi_viewport, ULPI_RWRUN | reg << ULPI_ADDR_SHIFT, ULPI_RWRUN)) {
+		printf("ULPI read timed out\n");
+		return 0;
+	}
+
+	return ulpi_read_mask(readl(ulpi_viewport));
+}

drivers/usb/ulpi/ulpi.c new file +88 -0

new file mode 100644
index 0000000..c17bb73
@@ -0,0 +1,113 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * Based on:
+ * linux/drivers/usb/otg/ulpi.c
+ * Generic ULPI USB transceiver support
+ *
+ * Original Copyright follow:
+ * Copyright (C) 2009 Daniel Mack <[email protected]>
+ *
+ * Based on sources from
+ *
+ *   Sascha Hauer <[email protected]>
+ *   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 void ulpi_integrity_check(u32 ulpi_viewport, struct ulpi_regs *ulpi)
+{
+	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;
+		}
+	}
+}
+
+/*
+ * This is a family of wrapper functions which sets bits in ULPI registers.
+ * Access mode could be WRITE, SET or CLEAR.
+ * For further informations see ULPI 1.1 specification.
+ */
+void ulpi_otg_ctrl_flags(u32 ulpi_viewport,
+	struct ulpi_regs *ulpi, int access_mode, u32 flags)
+{
+	switch (access_mode) {
+	case ULPI_WRITE:
+		ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl, flags);
+		break;
+	case ULPI_SET:
+		ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set, flags);
+		break;
+	case ULPI_CLEAR:
+		ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_clear, flags);
+		break;
+	}
+}
+
+void ulpi_function_ctrl_flags(u32 ulpi_viewport,
+	struct ulpi_regs *ulpi, int access_mode, u32 flags)
+{
+	switch (access_mode) {
+	case ULPI_WRITE:
+		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl, flags);
+		break;
+	case ULPI_SET:
+		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set, flags);
+		break;
+	case ULPI_CLEAR:
+		ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_clear, flags);
+		break;
+	}
+}
+
+void ulpi_iface_ctrl_flags(u32 ulpi_viewport,
+	struct ulpi_regs *ulpi, int access_mode, u32 flags)
+{
+	switch (access_mode) {
+	case ULPI_WRITE:
+		ulpi_write(ulpi_viewport, (u32)&ulpi->iface_ctrl, flags);

drivers/usb/ulpi/ulpi.c line 98

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+		break;
+	case ULPI_SET:
+		ulpi_write(ulpi_viewport, (u32)&ulpi->iface_ctrl_set, flags);
+		break;
+	case ULPI_CLEAR:
+		ulpi_write(ulpi_viewport, (u32)&ulpi->iface_ctrl_clear, flags);
+		break;
+	}
+
+}

All this access mode crap should not be needed
once there is a functional API.

Quoted 15 lines

drivers/usb/ulpi/ulpi.c new file +15 -0

+
+void ulpi_init(u32 ulpi_viewport, struct ulpi_regs *ulpi)
+{
+	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 TX, ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);
+
+	ulpi_integrity_check(ulpi_viewport, ulpi);
+}

Signature · 2 lines

Regards,
Igor.

Jana Rapava

author

2011-11-24 12:22 UTC #12 lore raw

PATCH v3 1/1 ulpi: add generic ULPI functionality

In reply to Jana Rapava

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

Signed-off-by Jana Rapava <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

 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

Makefile +1 -0

index dfe939f..70a1e1e 100644
@@ -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

drivers/usb/ulpi/Makefile new file +45 -0

45 lines not on this page · raw message

drivers/usb/ulpi/ulpi-viewport.c new file +88 -0

88 lines not on this page · raw message

drivers/usb/ulpi/ulpi.c new file +199 -0

199 lines not on this page · raw message

include/usb/ulpi.h new file +222 -0

222 lines not on this page · raw message

Signature · 1 line

1.7.6.3

Igor Grinberg

2011-11-24 13:26 UTC #13 lore raw

PATCH v3 1/1 ulpi: add generic ULPI functionality

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.

Quoted 149 lines · Jana Rapava

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

Signed-off-by Jana Rapava <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

 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

Makefile +1 -0

index dfe939f..70a1e1e 100644
@@ -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

drivers/usb/ulpi/Makefile new file +45 -0

new file mode 100644
index 0000000..a1a2244
@@ -0,0 +1,45 @@
+#
+# Copyright (C) 2011 Jana Rapava <[email protected]>
+# 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
+
+#########################################################################

drivers/usb/ulpi/ulpi-viewport.c new file +50 -0

new file mode 100644
index 0000000..9a1f59f
@@ -0,0 +1,88 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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;
+}

drivers/usb/ulpi/ulpi-viewport.c line 60

Quoted 10 lines

drivers/usb/ulpi/ulpi-viewport.c new file +10 -0

+
+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.

Quoted 67 lines

drivers/usb/ulpi/ulpi-viewport.c new file +28 -0

+{
+	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));
+}

drivers/usb/ulpi/ulpi.c new file +33 -0

new file mode 100644
index 0000000..2d66e86
@@ -0,0 +1,199 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * Based on:
+ * linux/drivers/usb/otg/ulpi.c
+ * Generic ULPI USB transceiver support
+ *
+ * Original Copyright follow:
+ * Copyright (C) 2009 Daniel Mack <[email protected]>
+ *
+ * Based on sources from
+ *
+ *   Sascha Hauer <[email protected]>
+ *   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)
+{

drivers/usb/ulpi/ulpi.c line 43

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+	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.

drivers/usb/ulpi/ulpi.c line 55

Quoted 12 lines

drivers/usb/ulpi/ulpi.c new file +12 -0

+		}
+	}
+}
+
+/*
+ * 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.

Quoted 7 lines

drivers/usb/ulpi/ulpi.c new file +7 -0

+{
+	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:

drivers/usb/ulpi/ulpi.c line 72

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+		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.

Quoted 45 lines

drivers/usb/ulpi/ulpi.c new file +45 -0

+		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.

drivers/usb/ulpi/ulpi.c line 127

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+ */
+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)?

Quoted 41 lines

drivers/usb/ulpi/ulpi.c new file +41 -0

+
+/*
+ * 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)

drivers/usb/ulpi/ulpi.c line 178

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+{
+	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?

drivers/usb/ulpi/ulpi.c line 184

Quoted 6 lines

drivers/usb/ulpi/ulpi.c new file +6 -0

+
+/* 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.

drivers/usb/ulpi/ulpi.c line 186

Quoted 2 lines

drivers/usb/ulpi/ulpi.c new file +2 -0
+
+void ulpi_init(u32 ulpi_viewport)

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

Quoted 231 lines

drivers/usb/ulpi/ulpi.c new file +13 -0

+{
+	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);
+}

include/usb/ulpi.h new file +212 -0

new file mode 100644
index 0000000..834b5e8
@@ -0,0 +1,222 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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);

include/usb/ulpi.h line 222

Quoted 10 lines

include/usb/ulpi.h new file +10 -0

+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.

Signature · 2 lines

Regards,
Igor.

Marek Vasut

2011-11-24 14:21 UTC #14 lore raw

PATCH v3 1/1 ulpi: add generic ULPI functionality

Quoted 5 lines

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.

Jana Rapava

author

2011-11-25 18:39 UTC #15 lore raw

PATCH v3 1/1 ulpi: add generic ULPI functionality

In reply to Igor Grinberg

2011/11/24 Igor Grinberg <[email protected]>

Quoted 29 lines

Quoted 2 lines

+/*
+ * If enable is 0, pull-down resistor not connected to D+, else

pull-down

Quoted 26 lines

+ * resistor connected to D+.
+ * Default behaviour is as for enable equal to 1.
+ */

drivers/usb/ulpi/ulpi.c new file +23 -0

+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);

Quoted 10 lines

Quoted 1 line
+}
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

Igor Grinberg

2011-11-27 07:50 UTC #16 lore raw

PATCH v3 1/1 ulpi: add generic ULPI functionality

Quoted 42 lines · Jana Rapava

2011/11/24 Igor Grinberg <grinberg at compulab.co.il <mailto:[email protected]>>

Quoted 19 lines

drivers/usb/ulpi/ulpi.c new file +19 -0

+/*
+ * 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.

drivers/usb/ulpi/ulpi.c line 127

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+ */
+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);
+}

Quoted 4 lines

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.

Signature · 2 lines

Regards,
Igor.

Jana Rapava

author

2011-11-25 20:05 UTC #17 lore raw

PATCH v4 1/1 ulpi: add generic ULPI functionality

In reply to Jana Rapava

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

Signed-off-by Jana Rapava <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   45 +++++++
 drivers/usb/ulpi/ulpi-viewport.c |   91 ++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  256 ++++++++++++++++++++++++++++++++++++++
 include/usb/ulpi.h               |  221 ++++++++++++++++++++++++++++++++
 5 files changed, 614 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

Makefile +1 -0

index c9e2624..da7352c 100644
@@ -283,6 +283,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

drivers/usb/ulpi/Makefile new file +45 -0

45 lines not on this page · raw message

drivers/usb/ulpi/ulpi-viewport.c new file +91 -0

91 lines not on this page · raw message

drivers/usb/ulpi/ulpi.c new file +256 -0

256 lines not on this page · raw message

include/usb/ulpi.h new file +221 -0

221 lines not on this page · raw message

Signature · 1 line

1.7.6.3

Simon Glass

2011-11-27 04:00 UTC #18 lore raw

PATCH v4 1/1 ulpi: add generic ULPI functionality

Hi Jana,

I am interested in this patch. It seems you could tidy the code a litte - sorry if I am too late with comments.

Quoted 126 lines · Jana Rapava

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

Signed-off-by Jana Rapava <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

---
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 Changes for v4:
? ? ? ?- add error-checking
? ? ? ?- add waiting for completion into ulpi_reset() function

?Makefile ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?drivers/usb/ulpi/Makefile ? ? ? ?| ? 45 +++++++
?drivers/usb/ulpi/ulpi-viewport.c | ? 91 ++++++++++++++
?drivers/usb/ulpi/ulpi.c ? ? ? ? ?| ?256 ++++++++++++++++++++++++++++++++++++++
?include/usb/ulpi.h ? ? ? ? ? ? ? | ?221 ++++++++++++++++++++++++++++++++

?5 files changed, 614 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

Makefile +0 -0

index c9e2624..da7352c 100644
@@ -283,6 +283,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

drivers/usb/ulpi/Makefile new file +45 -0

new file mode 100644
index 0000000..a1a2244
@@ -0,0 +1,45 @@
+#
+# Copyright (C) 2011 Jana Rapava <[email protected]>
+# 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
+
+#########################################################################

drivers/usb/ulpi/ulpi-viewport.c new file +24 -0

new file mode 100644
index 0000000..8f7d94c
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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 */

drivers/usb/ulpi/ulpi-viewport.c line 34

Quoted 10 lines

drivers/usb/ulpi/ulpi-viewport.c new file +10 -0

+#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)

How about a comment as to what this function does, params and return values?

Also perhaps if you pass the operation to this function it can print the error message (perhaps with debug()) rather than ever calling having to do it? Or at least decide whether messages should be in bottom-level functions or in top-level - and perhaps considering using debug() instead of printf() at lower levels.

drivers/usb/ulpi/ulpi-viewport.c line 39

Quoted 5 lines

drivers/usb/ulpi/ulpi-viewport.c new file +5 -0

+{
+ ? ? ? int timeout = CONFIG_USB_ULPI_TIMEOUT;
+ ? ? ? u32 tmp;
+
+ ? ? ? writel(ulpi_value, ulpi_viewport);

Why do the write here? Should it be done in the write call below? At the very least you should rename this function.

drivers/usb/ulpi/ulpi-viewport.c line 45

Quoted 6 lines

drivers/usb/ulpi/ulpi-viewport.c new file +6 -0

+
+ ? ? ? /* Wait for the bits in ulpi_mask to become zero. */
+ ? ? ? while (--timeout) {
+ ? ? ? ? ? ? ? tmp = readl(ulpi_viewport);
+ ? ? ? ? ? ? ? if (!(tmp & ulpi_mask))
+ ? ? ? ? ? ? ? ? ? ? ? break;

return 0?

drivers/usb/ulpi/ulpi-viewport.c line 49

Quoted 4 lines

drivers/usb/ulpi/ulpi-viewport.c new file +4 -0
+ ? ? ? ? ? ? ? udelay(1);
+ ? ? ? }
+
+ ? ? ? return !timeout;

return 1 or -1? Or ULPI_ERROR?

drivers/usb/ulpi/ulpi-viewport.c line 60

Quoted 11 lines

drivers/usb/ulpi/ulpi-viewport.c new file +11 -0

+}
+
+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);
+}
+
+u32 ulpi_write(u32 ulpi_viewport, u32 reg, u32 value)

It seems that every caller requires a cast for the second parameter. Perhaps you could avoid this if you make the reg parameter u8 * instead?

drivers/usb/ulpi/ulpi-viewport.c line 62

Quoted 2 lines

drivers/usb/ulpi/ulpi-viewport.c new file +2 -0
+{
+ ? ? ? u32 tmp;

blank line after declarations

drivers/usb/ulpi/ulpi-viewport.c line 66

Quoted 4 lines

drivers/usb/ulpi/ulpi-viewport.c new file +4 -0

+ ? ? ? if (ulpi_wakeup(ulpi_viewport)) {
+ ? ? ? ? ? ? ? printf("ULPI wakeup timed out\n");
+ ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? }

Shouldn't you do a writel() here?

Quoted 61 lines

drivers/usb/ulpi/ulpi-viewport.c new file +25 -0

+
+ ? ? ? 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");
+ ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? }
+ ? ? ? return 0;
+}
+
+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));
+}

drivers/usb/ulpi/ulpi.c new file +30 -0

new file mode 100644
index 0000000..83a8038
@@ -0,0 +1,256 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * Based on:
+ * linux/drivers/usb/otg/ulpi.c
+ * Generic ULPI USB transceiver support
+ *
+ * Original Copyright follow:
+ * Copyright (C) 2009 Daniel Mack <[email protected]>
+ *
+ * Based on sources from
+ *
+ * ? Sascha Hauer <[email protected]>
+ * ? 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;

drivers/usb/ulpi/ulpi.c line 40

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+
+/*
+ * This is a generic ULPI interface implemenatation.
+ * All functions return 0 in the case of success, ULPI_ERROR otherwise.
+ */
+
+static int ulpi_integrity_check(u32 ulpi_viewport)
+{
+ ? ? ? u32 tmp = 0;
+ ? ? ? int i;

blank line after decls

drivers/usb/ulpi/ulpi.c line 41

Quoted 1 line

drivers/usb/ulpi/ulpi.c new file +1 -0
+ ? ? ? for (i = 0; i < 2; i++) {

Why 2? Do you need a comment here?

Quoted 32 lines

drivers/usb/ulpi/ulpi.c new file +32 -0

+ ? ? ? ? ? ? ? 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 ULPI_ERROR;
+ ? ? ? ? ? ? ? }
+ ? ? ? }
+ ? ? ? return 0;
+}
+
+/*
+ * 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.
+ */
+int ulpi_select_transceiver(u32 ulpi_viewport, int speed)
+{
+ ? ? ? switch (speed) {
+ ? ? ? case ULPI_FC_HIGH_SPEED:
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_FC_HIGH_SPEED))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? ? ? ? ? break;
+ ? ? ? case ULPI_FC_FULL_SPEED:
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_FC_FULL_SPEED))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? ? ? ? ? break;

drivers/usb/ulpi/ulpi.c line 83

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+ ? ? ? case ULPI_FC_LOW_SPEED:
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_FC_LOW_SPEED))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? ? ? ? ? break;
+ ? ? ? case ULPI_FC_FS4LS:
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_FC_FS4LS))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? ? ? ? ? break;

Am I missing something?:

+ ? ? ? case ULPI_FC_HIGH_SPEED:
+ ? ? ? case ULPI_FC_FULL_SPEED:
+ ? ? ? case ULPI_FC_LOW_SPEED:
+ ? ? ? case ULPI_FC_FS4LS:
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? speed))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? ? ? ? ? break;

Quoted 9 lines

drivers/usb/ulpi/ulpi.c new file +9 -0

+ ? ? ? default:
+ ? ? ? ? ? ? ? printf("ulpi_select_transceiver: unknown transceiver speed\n");
+ ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? }
+ ? ? ? return 0;
+}
+
+/*
+ * Signals that 5V is driven to VBUS.

drivers/usb/ulpi/ulpi.c

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+ * 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.
+ */
+int ulpi_drive_vbus(u32 ulpi_viewport,
+ ? ? ? int ext_power_supply, int use_ext_indicator)
+{
+ ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+ ? ? ? ? ? ? ? ULPI_OTG_DRVVBUS))
+ ? ? ? ? ? ? ? return ULPI_ERROR;

You could have a variable which is set to either of the 3 options, then call the function once.

Quoted 22 lines

drivers/usb/ulpi/ulpi.c new file +22 -0

+
+ ? ? ? if (ext_power_supply) {
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_OTG_DRVVBUS_EXT))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? }
+
+ ? ? ? if (use_ext_indicator) {
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_OTG_EXTVBUSIND))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? }
+ ? ? ? return 0;
+}
+
+/*
+ * 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.
+ */
+int ulpi_dp_pulldown(u32 ulpi_viewport, int enable)
+{

drivers/usb/ulpi/ulpi.c line 134

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+ ? ? ? if (enable) {
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_OTG_DP_PULLDOWN))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? } else {
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_clear,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_OTG_DP_PULLDOWN))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? }
+ ? ? ? return 0;

Is something like this better?

return ulpi_write(ulpi_viewport, enable ? (u32)&ulpi->otg_ctrl_set
: (u32)&ulpi->otg_ctrl_clear,
? ? ? ? ? ? ? ? ? ? ? ULPI_OTG_DP_PULLDOWN);

Quoted 8 lines

drivers/usb/ulpi/ulpi.c new file +8 -0

+}
+
+/*
+ * 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.
+ */
+int ulpi_dm_pulldown(u32 ulpi_viewport, int enable)

drivers/usb/ulpi/ulpi.c

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+{
+ ? ? ? if (enable) {
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_OTG_DM_PULLDOWN))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? } else {
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_clear,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_OTG_DM_PULLDOWN))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? }

as above

Quoted 23 lines

drivers/usb/ulpi/ulpi.c new file +23 -0

+ ? ? ? return 0;
+}
+
+/*
+ * 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.
+ */
+int ulpi_select_opmode(u32 ulpi_viewport, int style)
+{
+ ? ? ? switch (style) {
+ ? ? ? case ULPI_FC_OPMODE_NORMAL:
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_FC_OPMODE_NORMAL))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? ? ? ? ? break;
+ ? ? ? case ULPI_FC_OPMODE_NONDRIVING:
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_FC_OPMODE_NONDRIVING))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? ? ? ? ? break;

drivers/usb/ulpi/ulpi.c line 185

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+ ? ? ? case ULPI_FC_OPMODE_DISABLE_NRZI:
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_FC_OPMODE_DISABLE_NRZI))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? ? ? ? ? break;
+ ? ? ? case ULPI_FC_OPMODE_NOSYNC_NOEOP:
+ ? ? ? ? ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ? ? ? ? ULPI_FC_OPMODE_NOSYNC_NOEOP))
+ ? ? ? ? ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? ? ? ? ? break;

as above

Quoted 42 lines

drivers/usb/ulpi/ulpi.c new file +42 -0

+ ? ? ? default:
+ ? ? ? ? ? ? ? printf("ulpi_select_opmode: unknown bit encoding style\n");
+ ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? }
+ ? ? ? return 0;
+}
+
+/* Put PHY into low power mode. */
+int ulpi_suspend(u32 ulpi_viewport)
+{
+ ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_clear,
+ ? ? ? ? ? ? ? ULPI_FC_SUSPENDM))
+ ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? return 0;
+}
+
+/* Put PHY out of low power mode. */
+int ulpi_resume(u32 ulpi_viewport)
+{
+ ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ULPI_FC_SUSPENDM))
+ ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? return 0;
+}
+
+static int reset_wait(u32 ulpi_viewport)
+{
+ ? ? ? int timeout = CONFIG_USB_ULPI_TIMEOUT;
+ ? ? ? u32 tmp;
+
+ ? ? ? if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+ ? ? ? ? ? ? ? ULPI_FC_RESET))
+ ? ? ? ? ? ? ? return ULPI_ERROR;
+
+ ? ? ? /* Wait for the RESET bit to become zero. */
+ ? ? ? while (--timeout) {
+ ? ? ? ? ? ? ? tmp = ulpi_read(ulpi_viewport, (u32)&ulpi->function_ctrl);
+ ? ? ? ? ? ? ? if (!(tmp & ULPI_FC_RESET))
+ ? ? ? ? ? ? ? ? ? ? ? break;
+ ? ? ? ? ? ? ? udelay(1);
+ ? ? ? }
+

drivers/usb/ulpi/ulpi.c line 237

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+ ? ? ? return !timeout;
+}
+
+/* Reset transceiver, does not reset ULPI interface or ULPI register set. */
+int ulpi_reset(u32 ulpi_viewport)
+{
+ ? ? ? /* Have to wait for reset to complete */
+ ? ? ? if (reset_wait(ulpi_viewport))
+ ? ? ? ? ? ? ? return ULPI_ERROR;
+ ? ? ? return 0;

Why not just

   return reset_wait(ulpi_viewport);

Quoted 226 lines

drivers/usb/ulpi/ulpi.c new file +19 -0

+}
+
+int ulpi_init(u32 ulpi_viewport)
+{
+ ? ? ? 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);
+
+ ? ? ? if (ulpi_integrity_check(ulpi_viewport))
+ ? ? ? ? ? ? ? return ULPI_ERROR;
+
+ ? ? ? return 0;
+}

include/usb/ulpi.h new file +201 -0

new file mode 100644
index 0000000..806fef7
@@ -0,0 +1,221 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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*/

include/usb/ulpi.h line 211

Quoted 10 lines

include/usb/ulpi.h new file +10 -0

+#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)
+
+u32 ulpi_write(u32 ulpi_viewport, u32 reg, u32 value);
+u32 ulpi_read(u32 ulpi_viewport, u32 reg);
+
+int ulpi_init(u32 ulpi_viewport);
+

It would be nice to have some comments on this if you want people to understand / use the API/

Quoted 18 lines

include/usb/ulpi.h new file +10 -1

+int ulpi_select_transceiver(u32 ulpi_viewport, int speed);
+int ulpi_drive_vbus(u32 ulpi_viewport,
+ ? ? ? int ext_power_supply, int use_ext_indicator);
+int ulpi_dp_pulldown(u32 ulpi_viewport, int enable);
+int ulpi_dm_pulldown(u32 ulpi_viewport, int enable);
+int ulpi_select_opmode(u32 ulpi_viewport, int style);
+int ulpi_suspend(u32 ulpi_viewport);
+int ulpi_resume(u32 ulpi_viewport);
+int ulpi_reset(u32 ulpi_viewport);
+#endif /* __USB_ULPI_H */
--

1.7.6.3

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Igor Grinberg

2011-11-27 08:08 UTC #19 lore raw

PATCH v4 1/1 ulpi: add generic ULPI functionality

Hi Simon,

Quoted 4 lines · Simon Glass

Hi Jana,
I am interested in this patch. It seems you could tidy the code a litte - sorry if I am too late with comments.

It is never too late, but please consider the following:
there are several patch series depending on this one, so how about we make only a "no go" changes, so other patch series could go in and then we can fix the rest?

Quoted 141 lines · Jana Rapava

Quoted 126 lines

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

Signed-off-by Jana Rapava <[email protected]>

Cc Marek Vasut <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   45 +++++++
 drivers/usb/ulpi/ulpi-viewport.c |   91 ++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  256 ++++++++++++++++++++++++++++++++++++++
 include/usb/ulpi.h               |  221 ++++++++++++++++++++++++++++++++
 5 files changed, 614 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

Makefile +1 -0

index c9e2624..da7352c 100644
@@ -283,6 +283,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

drivers/usb/ulpi/Makefile new file +45 -0

new file mode 100644
index 0000000..a1a2244
@@ -0,0 +1,45 @@
+#
+# Copyright (C) 2011 Jana Rapava <[email protected]>
+# 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
+
+#########################################################################

drivers/usb/ulpi/ulpi-viewport.c new file +24 -0

new file mode 100644
index 0000000..8f7d94c
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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 */

drivers/usb/ulpi/ulpi-viewport.c line 34

Quoted 10 lines

drivers/usb/ulpi/ulpi-viewport.c new file +10 -0

+#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)

How about a comment as to what this function does, params and return values?

Quoted 5 lines

Also perhaps if you pass the operation to this function it can print the error message (perhaps with debug()) rather than ever calling having to do it? Or at least decide whether messages should be in bottom-level functions or in top-level - and perhaps considering using debug() instead of printf() at lower levels.

I would like to consider this as a verbosity fix/cleanup and have this fixed later, can we?

Quoted 9 lines

drivers/usb/ulpi/ulpi-viewport.c line 39
Quoted 6 lines
drivers/usb/ulpi/ulpi-viewport.c new file +5 -0
+{
+       int timeout = CONFIG_USB_ULPI_TIMEOUT;
+       u32 tmp;
+
+       writel(ulpi_value, ulpi_viewport);
Why do the write here? Should it be done in the write call below? At the very least you should rename this function.

If you move this writel() from here, then you need to add
it to every function calling this one (it is not just the write below). It can be done, but again can we do this later?

Quoted 8 lines

drivers/usb/ulpi/ulpi-viewport.c line 45

Quoted 7 lines

drivers/usb/ulpi/ulpi-viewport.c new file +6 -0

+
+       /* Wait for the bits in ulpi_mask to become zero. */
+       while (--timeout) {
+               tmp = readl(ulpi_viewport);
+               if (!(tmp & ulpi_mask))
+                       break;

Quoted 8 lines

return 0?
drivers/usb/ulpi/ulpi-viewport.c line 49
Quoted 4 lines
drivers/usb/ulpi/ulpi-viewport.c new file +4 -0
+               udelay(1);
+       }
+
+       return !timeout;
return 1 or -1? Or ULPI_ERROR?

As this is a static function, can we fix this later?

Quoted 16 lines

drivers/usb/ulpi/ulpi-viewport.c line 60
Quoted 12 lines
drivers/usb/ulpi/ulpi-viewport.c new file +11 -0
+}
+
+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);
+}
+
+u32 ulpi_write(u32 ulpi_viewport, u32 reg, u32 value)
It seems that every caller requires a cast for the second parameter. Perhaps you could avoid this if you make the reg parameter u8 * instead?

This is what I would call a "no go"...
Jana, can you, please deal with this?
And if you do, then you can deal with *most* of other comments as well.

Quoted 4 lines

drivers/usb/ulpi/ulpi-viewport.c line 62
Quoted 3 lines
drivers/usb/ulpi/ulpi-viewport.c new file +2 -0
+{
+       u32 tmp;

Quoted 8 lines

blank line after declarations
drivers/usb/ulpi/ulpi-viewport.c line 66
Quoted 4 lines
drivers/usb/ulpi/ulpi-viewport.c new file +4 -0
+       if (ulpi_wakeup(ulpi_viewport)) {
+               printf("ULPI wakeup timed out\n");
+               return ULPI_ERROR;
+       }
Shouldn't you do a writel() here?

if so, then also in read and wakeup...

Quoted 124 lines

Quoted 62 lines

drivers/usb/ulpi/ulpi-viewport.c new file +25 -0

+
+       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");
+               return ULPI_ERROR;
+       }
+       return 0;
+}
+
+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));
+}

drivers/usb/ulpi/ulpi.c new file +30 -0

new file mode 100644
index 0000000..83a8038
@@ -0,0 +1,256 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * Based on:
+ * linux/drivers/usb/otg/ulpi.c
+ * Generic ULPI USB transceiver support
+ *
+ * Original Copyright follow:
+ * Copyright (C) 2009 Daniel Mack <[email protected]>
+ *
+ * Based on sources from
+ *
+ *   Sascha Hauer <[email protected]>
+ *   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;

drivers/usb/ulpi/ulpi.c line 40

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+
+/*
+ * This is a generic ULPI interface implemenatation.
+ * All functions return 0 in the case of success, ULPI_ERROR otherwise.
+ */
+
+static int ulpi_integrity_check(u32 ulpi_viewport)
+{
+       u32 tmp = 0;
+       int i;

blank line after decls

drivers/usb/ulpi/ulpi.c line 41

Quoted 1 line

drivers/usb/ulpi/ulpi.c new file +1 -0
+       for (i = 0; i < 2; i++) {

Why 2? Do you need a comment here?

Quoted 32 lines

drivers/usb/ulpi/ulpi.c new file +32 -0

+               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 ULPI_ERROR;
+               }
+       }
+       return 0;
+}
+
+/*
+ * 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.
+ */
+int ulpi_select_transceiver(u32 ulpi_viewport, int speed)
+{
+       switch (speed) {
+       case ULPI_FC_HIGH_SPEED:
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+                       ULPI_FC_HIGH_SPEED))
+                       return ULPI_ERROR;
+               break;
+       case ULPI_FC_FULL_SPEED:
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+                       ULPI_FC_FULL_SPEED))
+                       return ULPI_ERROR;
+               break;

drivers/usb/ulpi/ulpi.c line 83

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+       case ULPI_FC_LOW_SPEED:
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+                       ULPI_FC_LOW_SPEED))
+                       return ULPI_ERROR;
+               break;
+       case ULPI_FC_FS4LS:
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+                       ULPI_FC_FS4LS))
+                       return ULPI_ERROR;
+               break;

Am I missing something?:

drivers/usb/ulpi/ulpi.c line 82

Quoted 7 lines

drivers/usb/ulpi/ulpi.c new file +7 -0

+       case ULPI_FC_HIGH_SPEED:
+       case ULPI_FC_FULL_SPEED:
+       case ULPI_FC_LOW_SPEED:
+       case ULPI_FC_FS4LS:
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+                       speed))
+                       return ULPI_ERROR;

Just:
return ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set, speed);

Quoted 17 lines

+ break;

Quoted 14 lines

drivers/usb/ulpi/ulpi.c new file +14 -0

+       default:
+               printf("ulpi_select_transceiver: unknown transceiver speed\n");
+               return ULPI_ERROR;
+       }
+       return 0;
+}
+
+/*
+ * 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.
+ */
+int ulpi_drive_vbus(u32 ulpi_viewport,

Quoted 8 lines

drivers/usb/ulpi/ulpi.c line 102
Quoted 5 lines
drivers/usb/ulpi/ulpi.c new file +5 -0
+       int ext_power_supply, int use_ext_indicator)
+{
+       if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+               ULPI_OTG_DRVVBUS))
+               return ULPI_ERROR;
You could have a variable which is set to either of the 3 options, then call the function once.

Correct,
return ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set, flags);

Quoted 36 lines

Quoted 23 lines

drivers/usb/ulpi/ulpi.c new file +22 -0

+
+       if (ext_power_supply) {
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+                       ULPI_OTG_DRVVBUS_EXT))
+                       return ULPI_ERROR;
+       }
+
+       if (use_ext_indicator) {
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+                       ULPI_OTG_EXTVBUSIND))
+                       return ULPI_ERROR;
+       }
+       return 0;
+}
+
+/*
+ * 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.
+ */
+int ulpi_dp_pulldown(u32 ulpi_viewport, int enable)
+{

drivers/usb/ulpi/ulpi.c line 134

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+       if (enable) {
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+                       ULPI_OTG_DP_PULLDOWN))
+                       return ULPI_ERROR;
+       } else {
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_clear,
+                       ULPI_OTG_DP_PULLDOWN))
+                       return ULPI_ERROR;
+       }
+       return 0;

Is something like this better?

Quoted 3 lines

   return ulpi_write(ulpi_viewport, enable ? (u32)&ulpi->otg_ctrl_set
: (u32)&ulpi->otg_ctrl_clear,
                       ULPI_OTG_DP_PULLDOWN);

I don't think so - it is unreadable, but:

u32 reg = enable ? (u32)&ulpi->otg_ctrl_set

		 : (u32)&ulpi->otg_ctrl_clear;

return ulpi_write(ulpi_viewport, reg, ULPI_OTG_DP_PULLDOWN);

I think will do better.

Quoted 13 lines

drivers/usb/ulpi/ulpi.c new file +12 -0

+}
+
+/*
+ * 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.
+ */
+int ulpi_dm_pulldown(u32 ulpi_viewport, int enable)
+{
+       if (enable) {
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_set,
+                       ULPI_OTG_DM_PULLDOWN))

Quoted 11 lines

drivers/usb/ulpi/ulpi.c line 152

Quoted 6 lines

drivers/usb/ulpi/ulpi.c new file +6 -0

+                       return ULPI_ERROR;
+       } else {
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->otg_ctrl_clear,
+                       ULPI_OTG_DM_PULLDOWN))
+                       return ULPI_ERROR;
+       }

as above

Quoted 2 lines

drivers/usb/ulpi/ulpi.c new file +2 -0
+       return 0;
+}

Both above functions should be unite into one.

Quoted 9 lines

drivers/usb/ulpi/ulpi.c line 163

Quoted 9 lines

drivers/usb/ulpi/ulpi.c new file +9 -0

+
+/*
+ * 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.
+ */
+int ulpi_select_opmode(u32 ulpi_viewport, int style)

I think it will make more sense:
s/style/opmode/.

Quoted 23 lines

Quoted 12 lines

drivers/usb/ulpi/ulpi.c new file +12 -0

+{
+       switch (style) {
+       case ULPI_FC_OPMODE_NORMAL:
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+                       ULPI_FC_OPMODE_NORMAL))
+                       return ULPI_ERROR;
+               break;
+       case ULPI_FC_OPMODE_NONDRIVING:
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+                       ULPI_FC_OPMODE_NONDRIVING))
+                       return ULPI_ERROR;
+               break;

drivers/usb/ulpi/ulpi.c line 185

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+       case ULPI_FC_OPMODE_DISABLE_NRZI:
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+                       ULPI_FC_OPMODE_DISABLE_NRZI))
+                       return ULPI_ERROR;
+               break;
+       case ULPI_FC_OPMODE_NOSYNC_NOEOP:
+               if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+                       ULPI_FC_OPMODE_NOSYNC_NOEOP))
+                       return ULPI_ERROR;
+               break;

Quoted 1 line

as above

and
return ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set, opmode);

Quoted 15 lines

Quoted 5 lines

drivers/usb/ulpi/ulpi.c new file +4 -0

+       default:
+               printf("ulpi_select_opmode: unknown bit encoding style\n");
+               return ULPI_ERROR;
+       }

drivers/usb/ulpi/ulpi.c line 199

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+       return 0;
+}
+
+/* Put PHY into low power mode. */
+int ulpi_suspend(u32 ulpi_viewport)
+{
+       if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_clear,
+               ULPI_FC_SUSPENDM))
+               return ULPI_ERROR;
+       return 0;

return ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_clear,

		ULPI_FC_SUSPENDM);

Quoted 9 lines

drivers/usb/ulpi/ulpi.c line 208

Quoted 9 lines

drivers/usb/ulpi/ulpi.c new file +9 -0

+}
+
+/* Put PHY out of low power mode. */
+int ulpi_resume(u32 ulpi_viewport)
+{
+       if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+               ULPI_FC_SUSPENDM))
+               return ULPI_ERROR;
+       return 0;

same here

Quoted 10 lines

drivers/usb/ulpi/ulpi.c line 218

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+}
+
+static int reset_wait(u32 ulpi_viewport)
+{
+       int timeout = CONFIG_USB_ULPI_TIMEOUT;
+       u32 tmp;
+
+       if (ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set,
+               ULPI_FC_RESET))
+               return ULPI_ERROR;

here, make it:
ret = ulpi_write(ulpi_viewport, (u32)&ulpi->function_ctrl_set, ULPI_FC_RESET) if (ret)

	return ret;

so the value can propagate.

Quoted 22 lines

Quoted 9 lines

drivers/usb/ulpi/ulpi.c new file +9 -0

+
+       /* Wait for the RESET bit to become zero. */
+       while (--timeout) {
+               tmp = ulpi_read(ulpi_viewport, (u32)&ulpi->function_ctrl);
+               if (!(tmp & ULPI_FC_RESET))
+                       break;
+               udelay(1);
+       }
+

drivers/usb/ulpi/ulpi.c line 237

Quoted 10 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+       return !timeout;
+}
+
+/* Reset transceiver, does not reset ULPI interface or ULPI register set. */
+int ulpi_reset(u32 ulpi_viewport)
+{
+       /* Have to wait for reset to complete */
+       if (reset_wait(ulpi_viewport))
+               return ULPI_ERROR;
+       return 0;

Why not just

Quoted 1 line

   return reset_wait(ulpi_viewport);

Or even more: why not make it a single function ulpi_reset()? instead of having a wrapper that does nothing.

Quoted 11 lines

drivers/usb/ulpi/ulpi.c new file +10 -0

+}
+
+int ulpi_init(u32 ulpi_viewport)
+{
+       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);

Quoted 8 lines

drivers/usb/ulpi/ulpi.c line 255

Quoted 8 lines

drivers/usb/ulpi/ulpi.c new file +8 -0

+
+       /* Split ID into vendor and product ID. */
+       debug("Found ULPI transceiver ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);
+
+       if (ulpi_integrity_check(ulpi_viewport))
+               return ULPI_ERROR;
+
+       return 0;

Also, here, you can:
return ulpi_integrity_check(ulpi_viewport);

Quoted 213 lines

drivers/usb/ulpi/ulpi.c new file +1 -0

+}

include/usb/ulpi.h new file +206 -0

new file mode 100644
index 0000000..806fef7
@@ -0,0 +1,221 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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)
+

Quoted 8 lines

include/usb/ulpi.h line 211
Quoted 5 lines
include/usb/ulpi.h new file +5 -0
+u32 ulpi_write(u32 ulpi_viewport, u32 reg, u32 value);
+u32 ulpi_read(u32 ulpi_viewport, u32 reg);
+
+int ulpi_init(u32 ulpi_viewport);
+
It would be nice to have some comments on this if you want people to understand / use the API/

If documenting takes too long - can be done later in a follow up patch. What is important is to define it well -
the parameters comment should be fixed.

Quoted 24 lines

Quoted 14 lines
include/usb/ulpi.h new file +10 -1
+int ulpi_select_transceiver(u32 ulpi_viewport, int speed);
+int ulpi_drive_vbus(u32 ulpi_viewport,
+       int ext_power_supply, int use_ext_indicator);
+int ulpi_dp_pulldown(u32 ulpi_viewport, int enable);
+int ulpi_dm_pulldown(u32 ulpi_viewport, int enable);
+int ulpi_select_opmode(u32 ulpi_viewport, int style);
+int ulpi_suspend(u32 ulpi_viewport);
+int ulpi_resume(u32 ulpi_viewport);
+int ulpi_reset(u32 ulpi_viewport);
+#endif /* __USB_ULPI_H */
--
1.7.6.3
Quoted 5 lines
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Signature · 2 lines

Regards,
Igor.

Jana Rapava

author

2011-11-27 22:37 UTC #20 lore raw

PATCH v4 1/1 ulpi: add generic ULPI functionality

2011/11/27 Igor Grinberg <[email protected]>

Quoted 11 lines

Quoted 9 lines
drivers/usb/ulpi/ulpi-viewport.c line 39
Quoted 6 lines
drivers/usb/ulpi/ulpi-viewport.c new file +5 -0
+{
+       int timeout = CONFIG_USB_ULPI_TIMEOUT;
+       u32 tmp;
+
+       writel(ulpi_value, ulpi_viewport);
Why do the write here? Should it be done in the write call below? At the very least you should rename this function.

Quoted 5 lines

If you move this writel() from here, then you need to add
it to every function calling this one (it is not just the write below). It can be done, but again can we do this later?

If you would really want, I can do it later, but I think that rename this function
and document it properly is better way.

Quoted 18 lines

Quoted 17 lines
drivers/usb/ulpi/ulpi-viewport.c line 60
Quoted 13 lines
drivers/usb/ulpi/ulpi-viewport.c new file +11 -0
+}
+
+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);
+}
+
+u32 ulpi_write(u32 ulpi_viewport, u32 reg, u32 value)
It seems that every caller requires a cast for the second parameter. Perhaps you could avoid this if you make the reg parameter u8 * instead?

Quoted 5 lines

This is what I would call a "no go"...
Jana, can you, please deal with this?
And if you do, then you can deal with *most* of other comments as well.

Ok, I'll do so.
Regards,
Jana Rapava

Simon Glass

2011-11-27 23:34 UTC #21 lore raw

PATCH v4 1/1 ulpi: add generic ULPI functionality

In reply to Igor Grinberg

Hi Igor,

Quoted 5 lines · Igor Grinberg

Hi Simon,
Quoted 2 lines · Simon Glass
Hi Jana,

Quoted 7 lines

Quoted 2 lines
I am interested in this patch. It seems you could tidy the code a litte - sorry if I am too late with comments.
It is never too late, but please consider the following:
there are several patch series depending on this one, so how about we make only a "no go" changes, so other patch series could go in and then we can fix the rest?

Of course. My comments were only about code style and only suggestions. Please feel free to ignore any and all of them :-)

Regards,
Simon

Quoted 5 lines

--
Regards,
Igor.

Jana Rapava

author

2011-11-27 22:30 UTC #22 lore raw

PATCH v4 1/1 ulpi: add generic ULPI functionality

In reply to Simon Glass

2011/11/27 Simon Glass <[email protected]>

Quoted 12 lines

Quoted 1 line
+static int ulpi_wait(u32 ulpi_viewport, u32 ulpi_value, u32 ulpi_mask)
How about a comment as to what this function does, params and return values?
Also perhaps if you pass the operation to this function it can print the error message (perhaps with debug()) rather than ever calling having to do it? Or at least decide whether messages should be in bottom-level functions or in top-level - and perhaps considering using debug() instead of printf() at lower levels.

Quoted 10 lines

drivers/usb/ulpi/ulpi-viewport.c line 39
Quoted 5 lines
drivers/usb/ulpi/ulpi-viewport.c new file +5 -0
+{
+       int timeout = CONFIG_USB_ULPI_TIMEOUT;
+       u32 tmp;
+
+       writel(ulpi_value, ulpi_viewport);
Why do the write here? Should it be done in the write call below? At the very least you should rename this function.

This function issues ULPI wakeup or read-write request, writes the request into ULPI viewport register and waits until ULPI interface signalizes completion
of this request.
With error messages is the situation a bit difficult, because this function only knows
whether the request was for wakeup or for read-write.
I think more specific error messages are more helpful, so I decided to
put them into
functions which knows what we've attempted to do(ulpi_wakeup()/ulpi_read()/ulpi-write()).
If you really think that debug() is more appropriate in these functions, I
can change this,
but Igor doesn't consider this a reason for this patch not to be accepted, and I agree with him.

And I'll rename this function into ulpi_request(), ulpi_wait() is really a bit deceiveing.
Thanks for your comments.
Regards,
Jana Rapava

Jana Rapava

author

2011-11-28 00:19 UTC #23 lore raw

PATCH v5 1/1 ulpi: add generic ULPI functionality

In reply to Jana Rapava

Add generic functions for reading, writing and setting bits in ULPI registers.

Signed-off-by Jana Rapava <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

Cc Simon Glass <[email protected]>

---
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
Changes for v4:
- add error-checking
- add waiting for completion into ulpi_reset() function
Changes for v5:
- CodingStyle changes
- add comments
- simplify implemenation of the ULPI interface functions

 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   45 +++++++
 drivers/usb/ulpi/ulpi-viewport.c |  114 ++++++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  158 +++++++++++++++++++++++++
 include/usb/ulpi.h               |  238 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 556 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

Makefile +1 -0

index dfe939f..70a1e1e 100644
@@ -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

drivers/usb/ulpi/Makefile new file +45 -0

45 lines not on this page · raw message

drivers/usb/ulpi/ulpi-viewport.c new file +114 -0

114 lines not on this page · raw message

drivers/usb/ulpi/ulpi.c new file +158 -0

158 lines not on this page · raw message

include/usb/ulpi.h new file +238 -0

238 lines not on this page · raw message

Signature · 1 line

1.7.6.3

Igor Grinberg

2011-11-28 07:39 UTC #24 lore raw

PATCH v5 1/1 ulpi: add generic ULPI functionality

Quoted 8 lines · Jana Rapava

Add generic functions for reading, writing and setting bits in ULPI registers.
Signed-off-by Jana Rapava <[email protected]>
Cc Remy Bohmer <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Wolfgang Grandegger <[email protected]>
Cc Simon Glass <[email protected]>

Several improvements can still be made (mostly internal to that driver), but this can be done by a follow up patch, meanwhile
patch sets that depend on this one can be merged.

Acked-by Igor Grinberg <[email protected]>

Quoted 619 lines

 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   45 +++++++
 drivers/usb/ulpi/ulpi-viewport.c |  114 ++++++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  158 +++++++++++++++++++++++++
 include/usb/ulpi.h               |  238 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 556 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

Makefile +1 -0

index dfe939f..70a1e1e 100644
@@ -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

drivers/usb/ulpi/Makefile new file +45 -0

new file mode 100644
index 0000000..a1a2244
@@ -0,0 +1,45 @@
+#
+# Copyright (C) 2011 Jana Rapava <[email protected]>
+# 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
+
+#########################################################################

drivers/usb/ulpi/ulpi-viewport.c new file +114 -0

new file mode 100644
index 0000000..7764939
@@ -0,0 +1,114 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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)
+
+/* This function issues ULPI wakeup/read-write request
+ * and waits until ULPI interface signalizes completion.
+ * 'ulpi_value' is properly constructed ULPI request,
+ * 'ulpi_mask' is ULPI_WU for wakeup, ULPI_RWRUN for read or write.
+ */
+static int ulpi_request(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))
+			return 0;
+		udelay(1);
+	}
+
+	return ULPI_ERROR;
+}
+
+/* If ULPI bus isn't already awake, issue wakeup request via ulpi_request(). */
+static int ulpi_wakeup(u32 ulpi_viewport)
+{
+	if (readl(ulpi_viewport) & ULPI_SS)
+		return 0; /* already awake */
+
+	return ulpi_request(ulpi_viewport, ULPI_WU, ULPI_WU);
+}
+
+/*
+ * Wakeup ULPI interface and, if successful, create and issue
+ * ULPI write request via ulpi_request().
+ * 'reg' is pointer to member of structure ulpi_regs
+ * 'value' is value to be written.
+ */
+u32 ulpi_write(u32 ulpi_viewport, u8 *reg, u32 value)
+{
+	u32 tmp;
+
+	if (ulpi_wakeup(ulpi_viewport)) {
+		printf("ULPI wakeup timed out\n");
+		return ULPI_ERROR;
+	}
+
+	tmp = ulpi_request(ulpi_viewport, ULPI_RWRUN | ULPI_RWCTRL |
+		(u32)reg << ULPI_ADDR_SHIFT | ulpi_write_mask(value),
+		ULPI_RWRUN);
+	if (tmp)
+		printf("ULPI write timed out\n");
+
+	return tmp;
+}
+
+/*
+ * Wakeup ULPI interface and, if successful, create and issue
+ * ULPI read request via ulpi_request().
+ * 'reg' is pointer to member of structure ulpi_regs.
+ * Return value is value which was read, or ULPI_ERROR if wakeup or read
+ * request timed out.
+ */
+u32 ulpi_read(u32 ulpi_viewport, u8 *reg)
+{
+	u32 tmp;
+
+	if (ulpi_wakeup(ulpi_viewport)) {
+		printf("ULPI wakeup timed out\n");
+		return ULPI_ERROR;
+	}
+
+	tmp = ulpi_request(ulpi_viewport, ULPI_RWRUN |
+		(u32)reg << ULPI_ADDR_SHIFT, ULPI_RWRUN);
+	if (tmp) {
+		printf("ULPI read timed out\n");
+		return tmp;
+	}
+
+	return ulpi_read_mask(readl(ulpi_viewport));
+}

drivers/usb/ulpi/ulpi.c new file +158 -0

new file mode 100644
index 0000000..c4dc408
@@ -0,0 +1,158 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * Based on:
+ * linux/drivers/usb/otg/ulpi.c
+ * Generic ULPI USB transceiver support
+ *
+ * Original Copyright follow:
+ * Copyright (C) 2009 Daniel Mack <[email protected]>
+ *
+ * Based on sources from
+ *
+ *   Sascha Hauer <[email protected]>
+ *   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 int ulpi_integrity_check(u32 ulpi_viewport)
+{
+	u32 tmp = 0;
+	int i;
+
+	/*
+	 * Write at and read from 3 parts of the Scratch Register
+	 * (8 bits each).
+	 */
+	for (i = 0; i < 2; i++) {
+		ulpi_write(ulpi_viewport, &ulpi->scratch,
+			ULPI_TEST_VALUE << i);
+		tmp = ulpi_read(ulpi_viewport, &ulpi->scratch);
+
+		if (tmp != (ULPI_TEST_VALUE << i)) {
+			printf("ULPI integrity check failed\n");
+			return ULPI_ERROR;
+		}
+	}
+	return 0;
+}
+
+/* Identify ULPI transceiver, perform integrity check. */
+int ulpi_init(u32 ulpi_viewport)
+{
+	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, (u8 *)reg) << (reg * 8);
+
+	/* Split ID into vendor and product ID. */
+	debug("Found ULPI transceiver ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);
+
+	return ulpi_integrity_check(ulpi_viewport);
+}
+
+/*
+ * This function is used to select transceiver speed.
+ * Accepted 'speed' 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.
+ */
+int ulpi_select_transceiver(u32 ulpi_viewport, int speed)
+{
+	return ulpi_write(ulpi_viewport, &ulpi->function_ctrl_set, speed);
+}
+
+/*
+ * 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.
+ */
+int ulpi_drive_vbus(u32 ulpi_viewport,
+	int ext_power_supply, int use_ext_indicator)
+{
+	int flags = ULPI_OTG_DRVVBUS |
+		(ext_power_supply) ? ULPI_OTG_DRVVBUS_EXT : 0 |
+		(use_ext_indicator) ? ULPI_OTG_EXTVBUSIND : 0;
+
+	return ulpi_write(ulpi_viewport, &ulpi->otg_ctrl_set, flags);
+}
+
+/*
+ * If enable is 0, pull-down resistors not connected to D+ and D-,
+ * else pull-down resistors connected to D+ and D-.
+ * Default behaviour is as for enable equal to 1.
+ */
+int ulpi_pulldown(u32 ulpi_viewport, int enable)
+{
+	u8 *reg = (enable) ? &ulpi->otg_ctrl_set
+		: &ulpi->otg_ctrl_clear;
+
+	return ulpi_write(ulpi_viewport, reg,
+		ULPI_OTG_DP_PULLDOWN | ULPI_OTG_DM_PULLDOWN);
+}
+
+/*
+ * This function is used to select bit encoding style.
+ * Accepted 'opmode' 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.
+ */
+int ulpi_select_opmode(u32 ulpi_viewport, int opmode)
+{
+	return ulpi_write(ulpi_viewport, &ulpi->function_ctrl_set, opmode);
+}
+
+/* Put PHY into low power mode. */
+int ulpi_suspend(u32 ulpi_viewport)
+{
+	return ulpi_write(ulpi_viewport, &ulpi->function_ctrl_clear,
+		ULPI_FC_SUSPENDM);
+}
+
+/* Put PHY out of low power mode. */
+int ulpi_resume(u32 ulpi_viewport)
+{
+	return ulpi_write(ulpi_viewport, &ulpi->function_ctrl_set,
+		ULPI_FC_SUSPENDM);
+}
+
+/* Reset transceiver, does not reset ULPI interface or ULPI register set. */
+int ulpi_reset(u32 ulpi_viewport)
+{
+	int timeout = CONFIG_USB_ULPI_TIMEOUT;
+	u32 tmp;
+	int ret = ulpi_write(ulpi_viewport, &ulpi->function_ctrl_set,
+		ULPI_FC_RESET);
+
+	if (ret)
+		return ret;
+
+	/* Wait for the RESET bit to become zero. */
+	while (--timeout) {
+		tmp = ulpi_read(ulpi_viewport, &ulpi->function_ctrl);
+		if (!(tmp & ULPI_FC_RESET))
+			return 0;
+		udelay(1);
+	}
+
+	return ULPI_ERROR;
+}

include/usb/ulpi.h new file +238 -0

new file mode 100644
index 0000000..d7b37a6
@@ -0,0 +1,238 @@
+/*
+ * Copyright (C) 2011 Jana Rapava <[email protected]>
+ * 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)
+
+/*
+ * This is the generic ULPI interface.
+ * All functions with the exception of ulpi_read() return 0 in the case
+ * of success, ULPI_ERROR otherwise.
+ * 'ulpi_viewport' have to be address of ULPI Viewport register.
+ */
+/*
+ * This function should be called before every other function
+ * to check the ULPI registers.
+ */
+int ulpi_init(u32 ulpi_viewport);
+
+/*
+ * These are wrappers for functionalities, which could be enabled
+ * via ULPI registers.
+ */
+int ulpi_select_transceiver(u32 ulpi_viewport, int speed);
+int ulpi_drive_vbus(u32 ulpi_viewport,
+	int ext_power_supply, int use_ext_indicator);
+int ulpi_pulldown(u32 ulpi_viewport, int enable);
+int ulpi_select_opmode(u32 ulpi_viewport, int style);
+int ulpi_suspend(u32 ulpi_viewport);
+int ulpi_resume(u32 ulpi_viewport);
+int ulpi_reset(u32 ulpi_viewport);
+
+/*
+ * These are functions for direct reading from and writing to
+ * ULPI registers.
+ */
+u32 ulpi_write(u32 ulpi_viewport, u8 *reg, u32 value);
+u32 ulpi_read(u32 ulpi_viewport, u8 *reg);
+#endif /* __USB_ULPI_H */

Signature · 2 lines

Regards,
Igor.

Simon Glass

2011-11-28 17:06 UTC #25 lore raw

PATCH v5 1/1 ulpi: add generic ULPI functionality

In reply to Jana Rapava

Hi,

Quoted 8 lines · Jana Rapava

Add generic functions for reading, writing and setting bits in ULPI registers.
Signed-off-by Jana Rapava <[email protected]>
Cc Remy Bohmer <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Wolfgang Grandegger <[email protected]>
Cc Simon Glass <[email protected]>

Quoted 16 lines

---
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 Changes for v4:
? ? ? - add error-checking
? ? ? - add waiting for completion into ulpi_reset() function Changes for v5:
? ? ? ?- CodingStyle changes
? ? ? ?- add comments
? ? ? ?- simplify implemenation of the ULPI interface functions

I think this is a lot cleaner, thank you.

Quoted 18 lines

?Makefile ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?drivers/usb/ulpi/Makefile ? ? ? ?| ? 45 +++++++
?drivers/usb/ulpi/ulpi-viewport.c | ?114 ++++++++++++++++++
?drivers/usb/ulpi/ulpi.c ? ? ? ? ?| ?158 +++++++++++++++++++++++++
?include/usb/ulpi.h ? ? ? ? ? ? ? | ?238 ++++++++++++++++++++++++++++++++++++++
?5 files changed, 556 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
drivers/usb/ulpi/ulpi.c new file +0 -0
new file mode 100644
index 0000000..c4dc408
@@ -0,0 +1,158 @@

[snip]

drivers/usb/ulpi/ulpi.c

Quoted 6 lines

drivers/usb/ulpi/ulpi.c new file +6 -0

+int ulpi_drive_vbus(u32 ulpi_viewport,
+ ? ? ? int ext_power_supply, int use_ext_indicator)
+{
+ ? ? ? int flags = ULPI_OTG_DRVVBUS |
+ ? ? ? ? ? ? ? (ext_power_supply) ? ULPI_OTG_DRVVBUS_EXT : 0 |
+ ? ? ? ? ? ? ? (use_ext_indicator) ? ULPI_OTG_EXTVBUSIND : 0;

You have a few places where some people might prefer brackets, but not important. This one I think really should be:

int flags = ULPI_OTG_DRVVBUS |
? ? ? ? ? ? ? (ext_power_supply ? ULPI_OTG_DRVVBUS_EXT : 0) | ? ? ? ? ? ? ? (use_ext_indicator ? ULPI_OTG_EXTVBUSIND : 0);

or perhaps if you like:

int flags = ULPI_OTG_DRVVBUS;

if (ext_power_supply)
flags |= ULPI_OTG_DRVVBUS_EXT;
if (use_ext_indicator)
flags |= ULPI_OTG_EXTVBUSIND;

drivers/usb/ulpi/ulpi.c

Quoted 5 lines

drivers/usb/ulpi/ulpi.c new file +5 -0

+
+ ? ? ? return ulpi_write(ulpi_viewport, &ulpi->otg_ctrl_set, flags);
+}
+
+/*

Otherwise:

Acked-by Simon Glass <[email protected]>

Regards,
Simon

Jana Rapava

author

2011-11-28 19:43 UTC #26 lore raw

PATCH v6 1/1 ulpi: add generic ULPI functionality

In reply to Jana Rapava

Add generic functions for reading, writing and setting bits in ULPI registers.

Signed-off-by Jana Rapava <[email protected]>

Cc Remy Bohmer <[email protected]>

Cc Stefano Babic <[email protected]>

Cc Igor Grinberg <[email protected]>

Cc Wolfgang Grandegger <[email protected]>

Cc Simon Glass <[email protected]>

Acked-by Igor Grinberg <[email protected]>

Acked-by Simon Glass <[email protected]>

 Makefile                         |    1 +
 drivers/usb/ulpi/Makefile        |   45 +++++++
 drivers/usb/ulpi/ulpi-viewport.c |  114 ++++++++++++++++++
 drivers/usb/ulpi/ulpi.c          |  161 +++++++++++++++++++++++++
 include/usb/ulpi.h               |  238 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 559 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

Makefile +1 -0

index dfe939f..70a1e1e 100644
@@ -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

drivers/usb/ulpi/Makefile new file +45 -0

45 lines not on this page · raw message

drivers/usb/ulpi/ulpi-viewport.c new file +114 -0

114 lines not on this page · raw message

drivers/usb/ulpi/ulpi.c new file +161 -0

161 lines not on this page · raw message

include/usb/ulpi.h new file +238 -0

238 lines not on this page · raw message

Signature · 1 line

1.7.6.3

Simon Glass

2011-11-28 20:56 UTC #27 lore raw

PATCH v6 1/1 ulpi: add generic ULPI functionality

Quoted 11 lines · Jana Rapava

Add generic functions for reading, writing and setting bits in ULPI registers.
Signed-off-by Jana Rapava <[email protected]>
Cc Remy Bohmer <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Wolfgang Grandegger <[email protected]>
Cc Simon Glass <[email protected]>
Acked-by Igor Grinberg <[email protected]>
Acked-by Simon Glass <[email protected]>

Looks good, thanks. - Simon

Quoted 30 lines

---
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 Changes for v4:
? ? ? - add error-checking
? ? ? - add waiting for completion into ulpi_reset() function Changes for v5:
? ? ? ?- CodingStyle changes
? ? ? ?- add comments
? ? ? ?- simplify implemenation of the ULPI interface functions Changes for v6:
? ? ? ?- cleanup function ulpi_drive_vbus()
?Makefile ? ? ? ? ? ? ? ? ? ? ? ? | ? ?1 +
?drivers/usb/ulpi/Makefile ? ? ? ?| ? 45 +++++++
?drivers/usb/ulpi/ulpi-viewport.c | ?114 ++++++++++++++++++
?drivers/usb/ulpi/ulpi.c ? ? ? ? ?| ?161 +++++++++++++++++++++++++
?include/usb/ulpi.h ? ? ? ? ? ? ? | ?238 ++++++++++++++++++++++++++++++++++++++
?5 files changed, 559 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

Igor Grinberg

2011-12-01 11:12 UTC #28 lore raw

PATCH v6 1/1 ulpi: add generic ULPI functionality

In reply to Jana Rapava

Quoted 21 lines · Jana Rapava

Add generic functions for reading, writing and setting bits in ULPI registers.
Signed-off-by Jana Rapava <[email protected]>
Cc Remy Bohmer <[email protected]>
Cc Stefano Babic <[email protected]>
Cc Igor Grinberg <[email protected]>
Cc Wolfgang Grandegger <[email protected]>
Cc Simon Glass <[email protected]>
Acked-by Igor Grinberg <[email protected]>
Acked-by Simon Glass <[email protected]>
---
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
Changes for v4:

Quoted 8 lines

- add error-checking
- add waiting for completion into ulpi_reset() function
Changes for v5:
- CodingStyle changes
- add comments
- simplify implemenation of the ULPI interface functions
Changes for v6:
- cleanup function ulpi_drive_vbus()

Lately, I've found several flaws in v6.
Those are a "no goes" as for me.
I've got Jana's permission to take over the patch and I will submit a reworked version soon (hopefully tomorrow).

Signature · 2 lines

Regards,
Igor.