Hi Lucas,
On 08/19/12 19:08, Lucas Stach wrote:
> This adds the required code to set up a ULPI USB port. It is
> mostly a port of the Linux ULPI setup code with some tweaks
> added for more correctness, discovered along the way of
> debugging this.
Can you share which tweaks for correctness are there?
>
> To use this both CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT
> have to be set in the board configuration file.
>
> Signed-off-by: Lucas Stach <d...@lynxeye.de>
> ---
> arch/arm/cpu/armv7/tegra20/usb.c | 131
> +++++++++++++++++++++++++++++---
> arch/arm/include/asm/arch-tegra20/usb.h | 29 +++++--
> 2 Dateien geändert, 145 Zeilen hinzugefügt(+), 15 Zeilen entfernt(-)
>
> diff --git a/arch/arm/cpu/armv7/tegra20/usb.c
> b/arch/arm/cpu/armv7/tegra20/usb.c
> index 77966e5..2ae1244 100644
> --- a/arch/arm/cpu/armv7/tegra20/usb.c
> +++ b/arch/arm/cpu/armv7/tegra20/usb.c
> @@ -32,9 +32,17 @@
> #include <asm/arch/sys_proto.h>
> #include <asm/arch/uart.h>
> #include <asm/arch/usb.h>
> +#include <usb/ulpi.h>
> #include <libfdt.h>
> #include <fdtdec.h>
>
> +#ifdef CONFIG_USB_ULPI
> + #ifndef CONFIG_USB_ULPI_VIEWPORT
> + #error "To use CONFIG_USB_ULPI on Tegra Boards you have to also \
> + define CONFIG_USB_ULPI_VIEWPORT"
there is a mix of tabs and spaces in the above line, please make it only tabs
> + #endif
> +#endif
> +
> enum {
> USB_PORTS_MAX = 4, /* Maximum ports we allow */
> };
> @@ -68,11 +76,13 @@ enum dr_mode {
> struct fdt_usb {
> struct usb_ctlr *reg; /* address of registers in physical memory */
> unsigned utmi:1; /* 1 if port has external tranceiver, else 0 */
> + unsigned ulpi:1; /* 1 if port has external ULPI transceiver */
> unsigned enabled:1; /* 1 to enable, 0 to disable */
> unsigned has_legacy_mode:1; /* 1 if this port has legacy mode */
> enum dr_mode dr_mode; /* dual role mode */
> enum periph_id periph_id;/* peripheral id */
> struct fdt_gpio_state vbus_gpio; /* GPIO for vbus enable */
> + struct fdt_gpio_state phy_reset_gpio; /* GPIO to reset ULPI phy */
> };
>
> static struct fdt_usb port[USB_PORTS_MAX]; /* List of valid USB ports */
> @@ -187,8 +197,8 @@ static void usbf_reset_controller(struct fdt_usb *config,
> */
> }
>
> -/* set up the USB controller with the parameters provided */
> -static int init_usb_controller(struct fdt_usb *config,
> +/* set up the UTMI USB controller with the parameters provided */
> +static int init_utmi_usb_controller(struct fdt_usb *config,
> struct usb_ctlr *usbctlr, const u32 timing[])
> {
> u32 val;
> @@ -300,6 +310,83 @@ static int init_usb_controller(struct fdt_usb *config,
> return 0;
> }
>
> +#ifdef CONFIG_USB_ULPI
> +/* set up the ULPI USB controller with the parameters provided */
> +static int init_ulpi_usb_controller(struct fdt_usb *config,
> + struct usb_ctlr *usbctlr)
> +{
> + u32 val;
> + int loop_count;
> + struct ulpi_regs *ulpi_reg = (struct ulpi_regs *)0;
> + struct ulpi_viewport ulpi_vp;
> +
> + /* reset ULPI phy */
> + if (fdt_gpio_isvalid(&config->phy_reset_gpio)) {
> + fdtdec_setup_gpio(&config->phy_reset_gpio);
> + gpio_direction_output(config->phy_reset_gpio.gpio, 0);
> + mdelay(5);
> + gpio_set_value(config->phy_reset_gpio.gpio, 1);
> + }
> +
> + /* Reset the usb controller */
> + clock_enable(config->periph_id);
> + usbf_reset_controller(config, usbctlr);
> +
> + /* enable pinmux bypass */
> + setbits_le32(&usbctlr->ulpi_timing_ctrl_0,
> + ULPI_CLKOUT_PINMUX_BYP | ULPI_OUTPUT_PINMUX_BYP);
> +
> + /* Select ULPI parallel interface */
> + clrsetbits_le32(&usbctlr->port_sc1, PTS_MASK, PTS_ULPI << PTS_SHIFT);
> +
> + /* enable ULPI transceiver */
> + setbits_le32(&usbctlr->susp_ctrl, ULPI_PHY_ENB);
> +
> + /* configure ULPI transceiver timings */
> + val = 0;
> + writel(val, &usbctlr->ulpi_timing_ctrl_1);
> +
> + val |= ULPI_DATA_TRIMMER_SEL(4);
> + val |= ULPI_STPDIRNXT_TRIMMER_SEL(4);
> + val |= ULPI_DIR_TRIMMER_SEL(4);
> + writel(val, &usbctlr->ulpi_timing_ctrl_1);
> + udelay(10);
> +
> + val |= ULPI_DATA_TRIMMER_LOAD;
> + val |= ULPI_STPDIRNXT_TRIMMER_LOAD;
> + val |= ULPI_DIR_TRIMMER_LOAD;
> + writel(val, &usbctlr->ulpi_timing_ctrl_1);
> +
> + /* set up phy for host operation with external vbus supply */
> + ulpi_vp.port_num = 0;
> + ulpi_vp.viewport_addr = (u32)&usbctlr->ulpi_viewport;
> +
> + if (ulpi_init(&ulpi_vp)) {
> + debug("Tegra ULPI viewport init failed\n");
This looks like an error, right?
So why hide it under debug?
> + return -1;
> + }
> +
> + ulpi_write(&ulpi_vp, &ulpi_reg->iface_ctrl_set, ULPI_IFACE_PASSTHRU);
The implementation for the indicator setup is currently missing
from the generic framework.
Have you thought about adding it?
> + ulpi_write(&ulpi_vp, &ulpi_reg->otg_ctrl_set, ULPI_OTG_EXTVBUSIND);
Can ulpi_set_vbus(&ulpi_vp, 1, 1, 1) be used here?
> +
> + /* enable wakeup events */
> + setbits_le32(&usbctlr->port_sc1, WKCN | WKDS | WKOC);
> +
> + setbits_le32(&usbctlr->susp_ctrl, USB_SUSP_CLR);
> + /* Wait for the phy clock to become valid in 100 ms */
> + for (loop_count = 100000; loop_count != 0; loop_count--) {
> + if (readl(&usbctlr->susp_ctrl) & USB_PHY_CLK_VALID)
> + break;
> + udelay(1);
> + }
> + if (!loop_count)
> + return -1;
> + clrbits_le32(&usbctlr->susp_ctrl, USB_SUSP_CLR);
> +
> + return 0;
> +}
> +#endif
> +
> static void power_up_port(struct usb_ctlr *usbctlr)
> {
> /* Deassert power down state */
> @@ -331,11 +418,13 @@ static int add_port(struct fdt_usb *config, const u32
> timing[])
> USB_PORTS_MAX);
> return -1;
> }
> - if (init_usb_controller(config, usbctlr, timing)) {
> - debug("tegrausb: Cannot init port\n");
> - return -1;
> - }
> +
> if (config->utmi) {
> + if (init_utmi_usb_controller(config, usbctlr, timing)) {
> + debug("tegrausb: Cannot init port\n");
This also looks like an error...
So why debug()?
> + return -1;
> + }
> +
> /* Disable ICUSB FS/LS transceiver */
> clrbits_le32(&usbctlr->icusb_ctrl, IC_ENB1);
>
> @@ -345,6 +434,24 @@ static int add_port(struct fdt_usb *config, const u32
> timing[])
> clrbits_le32(&usbctlr->port_sc1, STS);
> power_up_port(usbctlr);
> }
> +
> + if (config->ulpi) {
> +#ifdef CONFIG_USB_ULPI
> + /* set up 24MHz ULPI reference clock on pllp_out4 */
> + clock_enable(PERIPH_ID_DEV2_OUT);
> + clock_set_pllout(CLOCK_ID_PERIPH, PLL_OUT4, 24000000);
Wouldn't it be clearer if:
1) you put the above inside the init_ulpi_usb_controller() function
2) Provide a !CONFIG_USB_ULPI implementation of the same function
technically having only the code under #else below inside.
So then here instead of the whole #ifdef, you will have something like:
if (config->ulpi) {
if (init_ulpi_usb_controller(config, usbctlr)) {
printf("tegrausb: Cannot init port\n");
return -1;
}
}
> +
> + if (init_ulpi_usb_controller(config, usbctlr)) {
> + debug("tegrausb: Cannot init port\n");
> + return -1;
> + }
> +#else
> + debug("No code to set up ULPI controller, please enable"
> + "CONFIG_USB_ULPI and CONFIG_USB_ULPI_VIEWPORT");
> + return -1;
> +#endif
> + }
> +
> port[port_count++] = *config;
>
> return 0;
[...]
--
Regards,
Igor.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
