[U-Boot] [PATCH v3] Add USB support for Efika
Stefano Babic
sbabic at denx.de
Mon Sep 19 10:43:14 CEST 2011
On 09/18/2011 04:19 AM, Jana Rapava wrote:
> From: Marek Vasut <marek.vasut at gmail.com>
>
> This commit adds USB support for EfikaMX and EfikaSB.
>
> Signed-off-by: Marek Vasut <marek.vasut at gmail.com>
> Signed-off-by: Jana Rapava <fermata7 at gmail.com>
> ---
> Changes for v2:
> - changed to proper patch
> Changes for v3:
> - merged other USB patches from u-boot-pxa/efikasb
> - offset-based access changed to struct-based access
> - use {clrset,clr,set}bits_le32() calls
> - CodingStyle and naming cleanup
Hi Jana,
you change several files, some of them in the general USB support. You
must then split your patch into a patchset, and each patch must address
a single issue. Really with your patch you do not only add USB support
to the EfikaMX board, but you want to add support for MX5 Soc and maybe
fix some issues. And if you change some general USB files, you should
add in CC the USB maintainer (Remy, I have already added him in my answer).
> diff --git a/board/efikamx/Makefile b/board/efikamx/Makefile
> index ee4a16e..860e4d2 100644
> --- a/board/efikamx/Makefile
> +++ b/board/efikamx/Makefile
> @@ -28,6 +28,9 @@ include $(TOPDIR)/config.mk
> LIB = $(obj)lib$(BOARD).o
>
> COBJS := efikamx.o
> +ifdef CONFIG_CMD_USB
> +COBJS += efikamx-usb.o
> +endif
>
> SRCS := $(SOBJS:.o=.S) $(COBJS:.o=.c)
> OBJS := $(addprefix $(obj),$(COBJS))
> diff --git a/board/efikamx/efikamx-usb.c b/board/efikamx/efikamx-usb.c
> new file mode 100644
> index 0000000..19227d4
> --- /dev/null
> +++ b/board/efikamx/efikamx-usb.c
> @@ -0,0 +1,349 @@
> +#include <common.h>
> +#include <usb.h>
> +#include <asm/io.h>
> +#include <asm/arch/imx-regs.h>
> +#include <asm/arch/mx5x_pins.h>
> +#include <asm/arch/iomux.h>
> +#include <asm/gpio.h>
> +#include <usb/ehci-fsl.h>
> +#include <errno.h>
> +
> +#include "../../drivers/usb/host/ehci.h"
> +#include "../../drivers/usb/host/ehci-core.h"
This seems to me pretty nasty - but it is not the only example in u-boot
including files from drivers/. Can we imagine to move these files into
the include directory ?
> +
> + /*
> + * Configure USBH1 control pads
> + */
> +
> + /* USB PHY reset */
> + mxc_request_iomux(MX51_PIN_EIM_D27, IOMUX_CONFIG_ALT1);
> + mxc_iomux_set_pad(MX51_PIN_EIM_D27, PAD_CTL_PKE_ENABLE |
> + PAD_CTL_SRE_FAST | PAD_CTL_DRV_HIGH);
> +
> + /* USB HUB reset */
> + mxc_request_iomux(MX51_PIN_GPIO1_5, IOMUX_CONFIG_ALT0);
> + mxc_iomux_set_pad(MX51_PIN_GPIO1_5, PAD_CTL_PKE_ENABLE |
> + PAD_CTL_SRE_FAST | PAD_CTL_DRV_HIGH);
> +
> +
> +#ifdef CONFIG_MACH_EFIKASB
Have I missed something ? In U-Boot I see only MACH_TYPE_MX51_EFIKAMX.
And I cannot find any patch on the ML for the EfikaSB board.
If you add support for a EfikaMX variant, you should add this variant in
your patchset, else this stuff is only dead code and must be removed.
> + /* WIFI EN (act low) */
> + mxc_request_iomux(MX51_PIN_EIM_A22, IOMUX_CONFIG_GPIO);
> + mxc_iomux_set_pad(MX51_PIN_EIM_A22, 0);
I think it is more readable if you exchange the fix "0" value with the
constants we have already defined, using maybe clrset* function for
PAD_CTL_PKE_ENABLE and PAD_CTL_PUE_KEEPER (the bits you want to clear).
> + /* WIFI RESET */
> + mxc_request_iomux(MX51_PIN_EIM_A16, IOMUX_CONFIG_GPIO);
> + mxc_iomux_set_pad(MX51_PIN_EIM_A16, 0);
Ditto, check globally.
> +/*
> + * Enable devices connected to USB BUSes
> + */
> +void efika_usb_enable_devices(void)
> +{
> + /* Enable Bluetooth */
> + gpio_direction_output(IOMUX_TO_GPIO(MX51_PIN_EIM_A17), 0);
> + udelay(10000);
> + gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_EIM_A17), 1);
> +
> + /* Enable WiFi */
> + gpio_direction_output(IOMUX_TO_GPIO(MX51_PIN_EIM_A22), 1);
> + udelay(10000);
> +
> + /* Reset the WiFi chip */
> + gpio_direction_output(IOMUX_TO_GPIO(MX51_PIN_EIM_A16), 0);
> + udelay(10000);
> + gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_EIM_A16), 1);
> +}
Is it ok to wait in this function 30mSec or the value can be tuned ? It
seems to me quite high. In the rest of code you wait for 1mSec. Is it ok ?
> +
> +/*
> + * Reset USB PHY (or PHYs on EfikaSB)
As already said: I do not see support for EfikaSB. Please explain or add
support for this variant of the board.
> + */
> +void efika_usb_phy_reset(void)
> +{
> + /* SMSC 3317 PHY reset */
> + gpio_direction_output(IOMUX_TO_GPIO(MX51_PIN_EIM_D27), 0);
> + udelay(1000);
> + gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_EIM_D27), 1);
> +}
Here you wait 1mSec...
> +
> +/*
> + * Configure control registers of the USB controller
> + */
> +void control_regs_setup(struct usb_control_regs *control)
> +{
> + clrsetbits_le32(&control->usbctrl,
> + (MXX1_OTG_WUE | MXX1_OTG_PM | MX51_H1_ULPI_IE | MX51_H1_WUE),
> + MX51_H1_PM);
Because these bits are valid for both MX5 and MX3, maybe you can call
them MXC_OTG_WUE, MXC_OTG_PM, and so on. MXX1_ let me think there is a
MXX1 Soc.
> +
> + clrsetbits_le32(&control->phyctrl0,
> + MX51_OTG_OVERCURD,
> + MX51_EHCI_POWERPINSE);
> +
> + clrsetbits_le32(&control->phyctrl1,
> + MX51_SYSCLOCK_MASK,
> + MX51_SYSCLOCK_24_MHZ);
> +
> + setbits_le32(&control->usbctrl1, MX51_H1_EXTCLKE);
> +
> + clrsetbits_le32(&control->uh2ctrl,
> + (MX51_H2_ULPI_IE | MX51_H2_WUE),
> + MX51_H2_PM);
> +
> + udelay(10000);
> +}
Please explain all these magic delays. If they are really needed, add a
comment to explain why it is necessary. And again, 10mSec seems to me a
lot of time.
> +
> +#define ULPI_ADDR_SHIFT 16
> +#define ulpi_write_mask(value) ((value) & 0xff)
> +#define ulpi_read_mask(value) (((value) >> 8) & 0xff)
> +
> +
> +void ulpi_write(u32 reg, u32 value, struct usb_ehci *ehci)
> +{
> + if (!(readl(&ehci->ulpi_viewpoint) & ULPI_SS)) {
Why is writing into this structure so specific to the efikamx ? It seems
to me this could be general code.
> + writel(ULPI_WU, &ehci->ulpi_viewpoint);
> + while (readl(&ehci->ulpi_viewpoint) & ULPI_WU)
> + ;
This is an endless loop. It is better to add a timeout (even with a
counter) and print something if the timeout elapses. This should be
fixed globally.
> +u32 ulpi_read(u32 reg, struct usb_ehci *ehci)
A ulpi_read() as ulpi_write() is not strictly related to the Efika board.
> +
> +void ehciX_init(u32 base, struct ulpi_regs *ulpi, struct usb_ehci *ehci)
What is the meaning of X ?
> +{
> + u32 tmp = 0;
> + int reg, i;
> +
> + /* get ID from ULPI immediate registers*/
> + for (reg = ULPI_ID_REGS_COUNT - 1; reg >= 0; reg--)
> + tmp |= ulpi_read(reg, ehci) << (reg * 8);
> + /* split into vendor and product ID */
> + debug("Found ULPI TX, ID %04x:%04x\n", tmp >> 16, tmp & 0xffff);
IMHO we should add this stuff globally. It is taken (or very similar) to
the OTG support under Linux - why do not sync it with the kernel drivers ?
> +
> + /* ULPI check integrity */
> + for (i = 0; i < 2; i++) {
> + ulpi_write(&ulpi->scratch_write, 0x55 << i, ehci);
> + tmp = ulpi_read(&ulpi->scratch_write, ehci);
> +
> + if (tmp != (0x55 << i)) {
> + printf("ULPI integrity check failed\n");
> + return;
> + }
> + }
Also this stuff is general code - why do we not take otg/ulpi.c from
Linux ?
> +
> + /* ULPI set flags */
> + ulpi_write(&ulpi->otg_ctrl_write,
> + ULPI_OTG_EXT_VBUS_IND | ULPI_OTG_DM_PULLDOWN | ULPI_OTG_DP_PULLDOWN, ehci);
> + ulpi_write(&ulpi->function_ctrl_write,
> + ULPI_FC_XCVR_SELECT | ULPI_FC_OPMODE_NORMAL | ULPI_FC_SUSPENDM_PWRED, ehci);
> + ulpi_write(&ulpi->iface_ctrl_write, 0, ehci);
> + ulpi_write(&ulpi->otg_ctrl_set,
> + ULPI_OTG_DRV_VBUS | ULPI_OTG_DRV_VBUS_EXT, ehci);
> +
> + /*
> + * NOTE: This violates USB specification, but otherwise, USB on Efika
> + * doesn't work.
> + */
> + ulpi_write(&ulpi->otg_ctrl_set, ULPI_OTG_CHRG_VBUS, ehci);
Ok - I understand it does not work. It is not explained why. Is it not
the workaround we find in the kernel ?
"* Workaround: b_host can't driver
* vbus, but PP in PORTSC needs to
* be 1 for host to work.
* So we set drv_vbus bit in
* transceiver to 0 thru ULPI."
> +}
> +
> +void ehci0_init(struct usb_ehci *ehci)
> +{
> + setbits_le32(&ehci->portsc, MX51_16BIT_UTMI);
> +}
> +
> +void ehci1_init(struct usb_ehci *ehci, struct ulpi_regs *ulpi)
> +{
> + mxc_request_iomux(MX51_PIN_USBH1_STP, IOMUX_CONFIG_ALT2);
> + mxc_iomux_set_pad(MX51_PIN_USBH1_STP, PAD_CTL_DRV_HIGH |
> + PAD_CTL_PKE_ENABLE | PAD_CTL_SRE_FAST);
> + gpio_direction_output(IOMUX_TO_GPIO(MX51_PIN_USBH1_STP), 0);
> + udelay(1000);
> + gpio_set_value(IOMUX_TO_GPIO(MX51_PIN_USBH1_STP), 1);
> + udelay(1000);
> +
> + mxc_request_iomux(MX51_PIN_USBH1_STP, IOMUX_CONFIG_ALT0);
> + mxc_iomux_set_pad(MX51_PIN_USBH1_STP, USB_PAD_CONFIG);
> + udelay(10000);
> +
> + clrbits_le32(&ehci->usbcmd, MX51_ITC_IMMEDIATE_MASK);
> + udelay(10000);
> +
> + writel(MX51_ULPI_MODE_MASK, &ehci->portsc);
> + udelay(10000);
> +
> + ehciX_init(OTG_BASE_ADDR + MX51_UH1_ID, ulpi, ehci);
> +}
I do not see any magic delays in kernel, and the driver is working, too.
> +int ehci_hcd_init(void)
> +{
> + struct usb_ehci *ehci;
> + struct usb_control_regs *control;
> + struct ulpi_regs *ulpi;
> +
> + /* Init iMX51 EHCI */
> + efika_usb_phy_reset();
> + efika_usb_hub_reset();
> + efika_usb_enable_devices();
> +
> + control = (struct usb_control_regs *)(OTG_BASE_ADDR +
> + MX51_CTRL_REGS);
> + control_regs_setup(control);
> +
> + ulpi = (struct ulpi_regs *)(0);
> + /* Init EHCI core */
> + ehci = (struct usb_ehci *)(OTG_BASE_ADDR +
> + (MX51_REGISTER_LAYOUT_LENGTH * CONFIG_MXC_USB_PORT));
> + hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength);
> + hcor = (struct ehci_hcor *)((uint32_t) hccr +
> + HC_LENGTH(ehci_readl(&hccr->cr_capbase)));
> + setbits_le32(&ehci->usbmode, CM_HOST);
> + setbits_le32(&ehci->control, USB_EN);
> +
> + switch (CONFIG_MXC_USB_PORT) {
> + case 0:
> + ehci0_init(ehci);
> + case 1:
> + ehci1_init(ehci, ulpi);
> +#ifdef MACH_EFIKASB
Already explained: MACH_EFIKASB does not (yet ?) exist. Why do we have a
"special" ehci_hcd_init and do we not fix the functions in ehci-mxc.c ?
>
> diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
> index 2197119..52b98c2 100644
> --- a/drivers/usb/host/ehci-hcd.c
> +++ b/drivers/usb/host/ehci-hcd.c
> @@ -26,6 +26,7 @@
> #include <asm/io.h>
> #include <malloc.h>
> #include <watchdog.h>
> +#include <usb/ehci-fsl.h>
>
> #include "ehci.h"
>
> @@ -709,8 +710,26 @@ ehci_submit_root(struct usb_device *dev, unsigned long pipe, void *buffer,
> * usb 2.0 specification say 50 ms resets on
> * root
I do not see this code in ML file. Does your patch apply ?
> */
> + /* wait_ms(50); */
> +
> +#if 1
There must be no #if 1 or #if 0 in mainline code.
> + struct usb_ehci *ehci = (struct usb_ehci *)(OTG_BASE_ADDR +
> + (MX51_REGISTER_LAYOUT_LENGTH));
> + extern u32 ulpi_read(u32 reg, struct usb_ehci *ehci);
> + extern void ulpi_write(u32 reg, u32 value, struct usb_ehci *ehci);
> + #define XXBASE 0x73f80200
You are mixing some general code with some MX specific code. XXBASE
makes no sense on most architectures. maybe you need a callback, but it
seems to me wrong to add this code here.
> diff --git a/drivers/usb/host/ehci-mxc.c b/drivers/usb/host/ehci-mxc.c
> index a0cfbb7..de7ed00 100644
> --- a/drivers/usb/host/ehci-mxc.c
> +++ b/drivers/usb/host/ehci-mxc.c
> @@ -37,9 +37,6 @@
> #endif
>
> #ifdef CONFIG_MX31
> -#define MX31_OTG_SIC_SHIFT 29
> -#define MX31_OTG_SIC_MASK (0x3 << MX31_OTG_SIC_SHIFT)
> -#define MX31_OTG_PM_BIT (1 << 24)
>
> #define MX31_H2_SIC_SHIFT 21
> #define MX31_H2_SIC_MASK (0x3 << MX31_H2_SIC_SHIFT)
> @@ -66,11 +63,11 @@ static int mxc_set_usbcontrol(int port, unsigned int flags)
>
> switch (port) {
> case 0: /* OTG port */
> - v &= ~(MX31_OTG_SIC_MASK | MX31_OTG_PM_BIT);
> + v &= ~(MXX1_OTG_SIC_MASK | MXX1_OTG_PM_BIT);
Agree the bit must be general and not SOC specific, I find only the
chosen name not particulary good.
> diff --git a/include/configs/efikamx.h b/include/configs/efikamx.h
> index b90e342..9cbb024 100644
> --- a/include/configs/efikamx.h
> +++ b/include/configs/efikamx.h
> @@ -40,6 +40,10 @@
>
> #define CONFIG_SYS_TEXT_BASE 0x97800000
>
> +#define CONFIG_L2_OFF
> +#define CONFIG_SYS_ICACHE_OFF
> +#define CONFIG_SYS_DCACHE_OFF
> +
> /*
> * Bootloader Components Configuration
> */
> @@ -49,6 +53,8 @@
> #define CONFIG_CMD_FAT
> #define CONFIG_CMD_EXT2
> #define CONFIG_CMD_IDE
> +#define CONFIG_CMD_NET
This has nothing to do with the goal of your patch. If you commit
message is "add USB", it is odd to find you add network support. Please
split your patch.
> +#define CONFIG_CMD_DATE
Ditto. This has nothing to do with USB. You coul add a separate patch in
your patchset for the efikamx.h, explaining you update the configuration
for the board.
>
> /*
> * Miscellaneous configurable options
> diff --git a/include/usb/ehci-fsl.h b/include/usb/ehci-fsl.h
> index 67600ed..9360ba5 100644
> --- a/include/usb/ehci-fsl.h
> +++ b/include/usb/ehci-fsl.h
> @@ -169,6 +169,78 @@
> #define CONFIG_SYS_FSL_USB_ADDR CONFIG_SYS_MPC512x_USB_ADDR
> #endif
>
> +
> +#ifdef CONFIG_MX51 || CONFIG_MX31
I do not know, but do we need this #ifdef ? Does not apply for MX53, for
example ?
Best regards,
Stefano Babic
--
=====================================================================
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: office at denx.de
=====================================================================
More information about the U-Boot
mailing list