[U-Boot] [PATCH] ulpi: add generic ULPI functionality
Igor Grinberg
grinberg at compulab.co.il
Tue Nov 8 12:33:45 CET 2011
Hi Jana,
Thanks for porting this.
Several comments below.
On 11/05/11 22:50, Jana Rapava wrote:
> Add generic functions for ULPI init and setting bits in
> ULPI registers.
>
> Signed-off-by: Jana Rapava <fermata7 at gmail.com>
> Cc: Marek Vasut <marek.vasut at gmail.com>
> Cc: Remy Bohmer <linux at bohmer.net>
> Cc: Stefano Babic <sbabic at denx.de>
> Cc: Igor Grinberg <grinberg at compulab.co.il>
> ---
> 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
>
> diff --git a/Makefile b/Makefile
> index 571c3eb..a475cb9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -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
> diff --git a/drivers/usb/ulpi/Makefile b/drivers/usb/ulpi/Makefile
> new file mode 100644
> index 0000000..f7b6e20
> --- /dev/null
> +++ b/drivers/usb/ulpi/Makefile
> @@ -0,0 +1,44 @@
> +#
> +# Copyright (C) 2011 Jana Rapava <fermata7 at gmail.com>
> +# See file CREDITS for list of people who contributed to this
> +# project.
> +#
> +# This program is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU General Public License as
> +# published by the Free Software Foundation; either version 2 of
> +# the License, or (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> +# MA 02111-1307 USA
> +#
> +
> +include $(TOPDIR)/config.mk
> +
> +LIB := $(obj)libusb_ulpi.o
> +
> +COBJS-$(CONFIG_USB_ULPI) += ulpi.o 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.
> +
> +COBJS := $(COBJS-y)
> +SRCS := $(COBJS:.o=.c)
> +OBJS := $(addprefix $(obj),$(COBJS))
> +
> +all: $(LIB)
> +
> +$(LIB): $(obj).depend $(OBJS)
> + $(call cmd_link_o_target, $(OBJS))
> +
> +#########################################################################
> +
> +# defines $(obj).depend target
> +include $(SRCTREE)/rules.mk
> +
> +sinclude $(obj).depend
> +
> +#########################################################################
> diff --git a/drivers/usb/ulpi/ulpi-viewport.c b/drivers/usb/ulpi/ulpi-viewport.c
> new file mode 100644
> index 0000000..a0c213e
> --- /dev/null
> +++ b/drivers/usb/ulpi/ulpi-viewport.c
> @@ -0,0 +1,87 @@
> +/*
> + * Copyright (C) 2011 Jana Rapava <fermata7 at gmail.com>
> + * Based on:
> + * linux/drivers/usb/otg/ulpi_viewport.c
> + *
> + * Original Copyright follow:
> + * Copyright (C) 2011 Google, Inc.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <watchdog.h>
> +#include <asm/io.h>
> +#include <usb/ulpi.h>
> +#include <usb/ehci-fsl.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)
> +
> +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.
> +{
> + 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.
> +
> + return !timeout;
> +}
> +
> +int ulpi_wakeup(struct usb_ehci *ehci)
same here
> +{
> + if (readl(&ehci->ulpi_viewpoint) & ULPI_SS)
> + return 0; /* already awake */
Please, empty line here.
> + 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)
> +{
> + 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?
> +
> + return ulpi_read_mask(readl(&ehci->ulpi_viewpoint));
> +}
> diff --git a/drivers/usb/ulpi/ulpi.c b/drivers/usb/ulpi/ulpi.c
> new file mode 100644
> index 0000000..b1c482a
> --- /dev/null
> +++ b/drivers/usb/ulpi/ulpi.c
> @@ -0,0 +1,123 @@
> +/*
> + * Copyright (C) 2011 Jana Rapava <fermata7 at gmail.com>
> + * Based on:
> + * linux/drivers/usb/otg/ulpi.c
> + * Generic ULPI USB transceiver support
> + *
> + * Original Copyright follow:
> + * Copyright (C) 2009 Daniel Mack <daniel at caiaq.de>
> + *
> + * Based on sources from
> + *
> + * Sascha Hauer <s.hauer at pengutronix.de>
> + * Freescale Semiconductors
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * 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...
> + */
> +
> +#include <usb/ulpi.h>
> +#include <usb/ehci-fsl.h>
Should not include EHCI specific stuff here.
> +#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.
> +{
> + 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);
> +
> + 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.
> + * 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.
> +
> +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?
> +
> + ulpi_integrity_check(ehci, ulpi);
> +}
> diff --git a/include/usb/ulpi.h b/include/usb/ulpi.h
> index 1519cc5..a8625a1 100644
> --- a/include/usb/ulpi.h
> +++ b/include/usb/ulpi.h
> @@ -15,6 +15,8 @@
> #ifndef __USB_ULPI_H
> #define __USB_ULPI_H
>
> +#include <usb/ehci-fsl.h>
> +
This should not be here.
> #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_*.
> +
> 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 */
--
Regards,
Igor.
More information about the U-Boot
mailing list