Evidently not yet ready to go upstream ... even though 2.6.25-rc1
has the OMAP patches, they don't include 34xx support.
So there's plenty of time to clean this up.
Review comments below.
- Dave
On Monday 04 February 2008, Pandita, Vikram wrote:
> This patch adds OMAP EHCI glue layer and Menu config option
> Incorporated Paul Walmsley review comments on usage of cm_{read,write}_mod_reg()
>
> Signed-off-by: Vikram Pandita <vikram.pandita@xxxxxx>
> Acked-by: Kamat, Nishant <nskamat@xxxxxx>
>
> ---
> drivers/usb/Kconfig | 1
> drivers/usb/host/Kconfig | 19 +
> drivers/usb/host/ehci-hcd.c | 5
> drivers/usb/host/ehci-omap.c | 560 +++++++++++++++++++++++++++++++++++++++++++
> drivers/usb/host/ehci-omap.h | 116 ++++++++
"quilt refresh --sort ..." is a bit nicer, FWIW.
> 5 files changed, 701 insertions(+)
>
> Index: omap-submit/drivers/usb/host/ehci-omap.c
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ omap-submit/drivers/usb/host/ehci-omap.c 2008-02-04 17:07:14.000000000 +0530
> @@ -0,0 +1,560 @@
> +/*
> + * ehci-omap.c - driver for USBHOST on OMAP 34xx processor
> + *
> + * Bus Glue for OMAP34xx USBHOST 3 port EHCI controller
> + * Tested on OMAP3430 ES2.0 SDP
> + *
> + * Copyright (C) 2007-2008 Texas Instruments, Inc.
> + * Copyright (C) 2007-2008 Vikram Pandita <vikram.pandita@xxxxxx>
See comment later about work-for-hire and US copyright law.
> + * Based on "ehci-fsl.c" by David Brownell and
No, not authored by me at all. I'm about to send a patch to
remove that false attribution from ehci-fsl.c; please fix that
in this bus glue.
> + * "ehci-au1xxx.c" by K.Boge <karsten.boge@xxxxxxx>
> + *
> + * 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 <linux/platform_device.h>
> +#include <linux/clk.h>
> +#include <asm/arch/gpio.h>
> +
> +#include "ehci-omap.h"
> +
> +
> +#ifdef CONFIG_OMAP_EHCI_PHY_MODE
> +/* EHCI connected to External PHY */
Any reason this needs to be visible in Kconfig, rather
than being listed in board-specific platform_data that's
passed to this driver?
> +
> +/* External USB connectivity board: 750-2083-001
> + * Connected to OMAP3430 SDP
> + * The board has Port1 and Port2 connected to ISP1504 in 12-pin ULPI mode
> + */
> +
> +/* ISSUE1:
> + * ISP1504 for input clocking mode needs special reset handling
> + * Hold the PHY in reset by asserting RESET_N signal
> + * Then start the 60Mhz clock input to PHY
> + * Release the reset after a delay -
> + * to get the PHY state machine in working state
> + */
> +#define EXTERNAL_PHY_RESET
> +#define EXT_PHY_RESET_GPIO_PORT1 (57)
> +#define EXT_PHY_RESET_GPIO_PORT2 (61)
> +#define EXT_PHY_RESET_DELAY (10)
Pass this stuff in platform_data. Would it make sense to
put this type of PHY stuff into callbacks? If not, you
should at least make these three
> +
> +/* ISSUE2:
> + * USBHOST supports External charge pump PHYs only
> + * Use the VBUS from Port1 to power VBUS of Port2 externally
> + * So use Port2 as the working ULPI port
> + */
> +#define VBUS_INTERNAL_CHARGEPUMP_HACK
This explains nothing to me. Doesn't each port have
its own PHY? Doesn't each PHY have its own chage pump?
Again, this looks like something that should live in
the platform_data. It's evidently specific to one
board ... so it does not belong in this file.
> +
> +#endif /* CONFIG_OMAP_EHCI_PHY_MODE */
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/* Define USBHOST clocks for clock management */
This structure, and the associated #defines, should be in your
header file.
However, the structure should be handled differently. All over
this bus glue you're doing pointer arithmetic -- wrong idea.
Instead, just stick the ehci_hcd at the front of this struct,
and pick a better name for it.
That way you'll still be doing a bit of type punning, but you
can avoid the pointer arithmetic.
> +struct ehci_omap_clock_defs {
> + struct clk *usbhost_ick_clk;
> + struct clk *usbhost2_120m_fck_clk;
> + struct clk *usbhost1_48m_fck_clk;
> + struct clk *usbtll_fck_clk;
> + struct clk *usbtll_ick_clk;
> +};
> +
> +/* Clock names as per clock framework: May change so keep as #defs */
> +#define USBHOST_ICKL "usbhost_l4_ick"
> +#define USBHOST_120M_FCLK "usbhost_120m_fck"
> +#define USBHOST_48M_FCLK "usbhost_48m_fck"
> +#define USBHOST_TLL_ICKL "usbtll_ick"
> +#define USBHOST_TLL_FCLK "usbtll_fck"
> +/*-------------------------------------------------------------------------*/
> +
> +/* omap_start_ehc
> + * - Start the TI USBHOST controller
> + */
> +static int omap_start_ehc(struct platform_device *dev, struct usb_hcd *hcd)
This does more than start the controller. It also does PHY setup,
which IMO should be abstracted more ... much of it is clearly
board-specific.
> +{
> + struct ehci_omap_clock_defs *ehci_clocks;
> +
> + dev_dbg(hcd->self.controller, ": starting TI EHCI USB Controller\n");
> +
> + ehci_clocks = (struct ehci_omap_clock_defs *)(
> + ((char *)hcd_to_ehci(hcd)) +
> + sizeof(struct ehci_hcd));
Casts that ugly should be avoided -- strongly.
The problem is that instead of defining and allocating
a struct with the EHCI data up front and these clocks
at the end, you're playing games. Don't.
> +
> + /* Start DPLL5 Programming:
> + * Clock Framework is not doing this now:
> + * This will be done in clock framework later
Glad to know that the clock framework will do this.
This kind of code really doesn't belong here ...
And by the way, isn't "13 MHz" a board-specific value,
hence something that belongs in platform_data?
> + */
> + /* Enable DPLL 5 : Based on Input of 13Mhz*/
> + cm_write_mod_reg((12 << OMAP3430ES2_PERIPH2_DPLL_DIV_SHIFT)|
> + (120 << OMAP3430ES2_PERIPH2_DPLL_MULT_SHIFT),
> + PLL_MOD, OMAP3430ES2_CM_CLKSEL4);
> +
> + cm_write_mod_reg(1 << OMAP3430ES2_DIV_120M_SHIFT,
> + PLL_MOD, OMAP3430ES2_CM_CLKSEL5);
> +
> + cm_write_mod_reg((7 << OMAP3430ES2_PERIPH2_DPLL_FREQSEL_SHIFT) |
> + (7 << OMAP3430ES2_EN_PERIPH2_DPLL_SHIFT),
> + PLL_MOD, OMAP3430ES2_CM_CLKEN2);
> +
> + while (!(cm_read_mod_reg(PLL_MOD, CM_IDLEST2) &
> + OMAP3430_ST_PERIPH2_CLK))
> + dev_dbg(hcd->self.controller,
> + "idlest2 = 0x%x\n",
> + cm_read_mod_reg(PLL_MOD, CM_IDLEST2));
> + /* End DPLL5 programming */
> +
> +
> + /* PRCM settings for USBHOST:
> + * Interface clk un-related to domain transition
> + */
> + cm_write_mod_reg(0 << OMAP3430ES2_AUTO_USBHOST_SHIFT,
> + OMAP3430ES2_USBHOST_MOD, CM_AUTOIDLE);
> +
> + /* Disable sleep dependency with MPU and IVA */
> + cm_write_mod_reg((0 << OMAP3430ES2_EN_MPU_SHIFT) |
> + (0 << OMAP3430ES2_EN_IVA2_SHIFT),
> + OMAP3430ES2_USBHOST_MOD, OMAP3430_CM_SLEEPDEP);
> +
> + /* Disable Automatic transition of clock */
> + cm_write_mod_reg(0 << OMAP3430ES2_CLKTRCTRL_USBHOST_SHIFT,
> + OMAP3430ES2_USBHOST_MOD, CM_CLKSTCTRL);
... end of stuff that should live in clock framework, I guess.
> +
> + /* Enable Clocks for USBHOST */
> + ehci_clocks->usbhost_ick_clk = clk_get(&dev->dev,
> + USBHOST_ICKL);
> + if (IS_ERR(ehci_clocks->usbhost_ick_clk))
> + return PTR_ERR(ehci_clocks->usbhost_ick_clk);
> + clk_enable(ehci_clocks->usbhost_ick_clk);
> +
> +
> + ehci_clocks->usbhost2_120m_fck_clk = clk_get(&dev->dev,
> + USBHOST_120M_FCLK);
> + if (IS_ERR(ehci_clocks->usbhost2_120m_fck_clk))
> + return PTR_ERR(ehci_clocks->usbhost2_120m_fck_clk);
> + clk_enable(ehci_clocks->usbhost2_120m_fck_clk);
> +
> + ehci_clocks->usbhost1_48m_fck_clk = clk_get(&dev->dev,
> + USBHOST_48M_FCLK);
> + if (IS_ERR(ehci_clocks->usbhost1_48m_fck_clk))
> + return PTR_ERR(ehci_clocks->usbhost1_48m_fck_clk);
> + clk_enable(ehci_clocks->usbhost1_48m_fck_clk);
> +
> +
> +#ifdef EXTERNAL_PHY_RESET
> + /* Refer: ISSUE1 */
> + omap_request_gpio(EXT_PHY_RESET_GPIO_PORT1);
> + omap_set_gpio_direction(EXT_PHY_RESET_GPIO_PORT1, 0);
> + omap_request_gpio(EXT_PHY_RESET_GPIO_PORT2);
> + omap_set_gpio_direction(EXT_PHY_RESET_GPIO_PORT2, 0);
> + omap_set_gpio_dataout(EXT_PHY_RESET_GPIO_PORT1, 0);
> + omap_set_gpio_dataout(EXT_PHY_RESET_GPIO_PORT2, 0);
I'd rather see the generic GPIO calls used for this stuff.
Direction "0" is opaque at best; and there's no good reason
to use OMAP-only GPIO calls for this level stuff. (Except,
pending some patches that need updating, gpio_request.)
I'd also have this PHY reset logic abstracted into a function
that's passed in the platform_data. :)
> + /* Hold the PHY in RESET for enough time till DIR is high */
> + udelay(EXT_PHY_RESET_DELAY);
> +#endif
I must have missed taking the PHY *out* of reset... no, it's
way way down below. With a second udelay ... surely you only
need one of them?
And for this next bit: color me confused. ULPI is for
talking to transceivers; a "TransceiverLess Link" (TLL)
is for, well, talking to not-transceivers. The comment
is accordingly not correct. Is the situation just that
some clocks and modes are misnamed? Or that ULPI setup
involves a "TLL" stage? Or what?
> +
> + /* Configure TLL for 60Mhz clk for ULPI */
> + ehci_clocks->usbtll_fck_clk = clk_get(&dev->dev, USBHOST_TLL_FCLK);
> + if (IS_ERR(ehci_clocks->usbtll_fck_clk))
> + return PTR_ERR(ehci_clocks->usbtll_fck_clk);
> + clk_enable(ehci_clocks->usbtll_fck_clk);
> +
> + ehci_clocks->usbtll_ick_clk = clk_get(&dev->dev, USBHOST_TLL_ICKL);
> + if (IS_ERR(ehci_clocks->usbtll_ick_clk))
> + return PTR_ERR(ehci_clocks->usbtll_ick_clk);
> + clk_enable(ehci_clocks->usbtll_ick_clk);
> +
> + /* Disable Auto Idle of USBTLL */
> + cm_write_mod_reg((0 << OMAP3430_AUTO_USBTLL_SHIFT),
> + CORE_MOD, OMAP3430_CM_AUTOIDLE3_CORE);
> +
> + /* Wait for TLL to be Active */
> + while ((cm_read_mod_reg(CORE_MOD, OMAP3430_CM_IDLEST3_CORE) &
> + (1 << OMAP3430_ST_USBTLL_SHIFT)));
> +
> + /* perform TLL soft reset, and wait until reset is complete */
> + /* (1<<3) = no idle mode only for initial debugging */
> + omap_writel((1 << OMAP_USBTLL_SYSCONFIG_SOFTRESET_SHIFT) |
> + (1 << OMAP_USBTLL_SYSCONFIG_ENAWAKEUP_SHIFT) |
> + (1 << OMAP_USBTLL_SYSCONFIG_SIDLEMODE_SHIFT) |
> + (1 << OMAP_USBTLL_SYSCONFIG_CACTIVITY_SHIFT),
> + OMAP_USBTLL_SYSCONFIG);
> + /* Wait for TLL reset to complete */
> + while (!(omap_readl(OMAP_USBTLL_SYSSTATUS) &
> + (1 << OMAP_USBTLL_SYSSTATUS_RESETDONE_SHIFT)));
> +
> + dev_dbg(hcd->self.controller, "\n TLL RESET DONE");
Presumably this next bit would make sense if I knew what
a UHH was, or the docs became public so I could find out...
meanwhile, please explain the acronym somewhere.
> +
> + /* Put UHH in NoIdle/NoStandby mode */
> + omap_writel((0 << OMAP_UHH_SYSCONFIG_AUTOIDLE_SHIFT) |
> + (1 << OMAP_UHH_SYSCONFIG_ENAWAKEUP_SHIFT) |
> + (1 << OMAP_UHH_SYSCONFIG_SIDLEMODE_SHIFT) |
> + (1 << OMAP_UHH_SYSCONFIG_CACTIVITY_SHIFT) |
> + (1 << OMAP_UHH_SYSCONFIG_MIDLEMODE_SHIFT),
> + OMAP_UHH_SYSCONFIG);
> +
Again, I'd really prefer to see board-specific stuff like this
next bit stay out of Kconfig. Maybe there should be some code
in arch/arm/mach-omap2 that this calls ... certainly #ifdefs in
the body of a function are to be avoided, and an "EHCI" driver
should avoid knowing so many ULPI details. Are those ports
only used by EHCI?
> +#ifdef CONFIG_OMAP_EHCI_PHY_MODE
> + /* Bypass the TLL module for PHY mode operation */
> + omap_writel((0 << OMAP_UHH_HOSTCONFIG_ULPI_BYPASS_SHIFT),
> + OMAP_UHH_HOSTCONFIG);
> + /* Ensure that BYPASS is set */
> + while (omap_readl(OMAP_UHH_HOSTCONFIG) &
> + (1 << OMAP_UHH_HOSTCONFIG_ULPI_BYPASS_SHIFT));
> +
> + dev_dbg(hcd->self.controller, "Entered ULPI PHY MODE: success");
> +
> +#else
> + /* Use UTMI Ports of TLL */
> + omap_writel((1 << OMAP_UHH_HOSTCONFIG_ULPI_BYPASS_SHIFT),
> + OMAP_UHH_HOSTCONFIG);
> + /* Enusre bit is set */
> + while (!(omap_readl(OMAP_UHH_HOSTCONFIG) &
> + (1 << OMAP_UHH_HOSTCONFIG_ULPI_BYPASS_SHIFT)));
> +
> + dev_dbg(hcd->self.controller, "Entered UTMI MODE: success");
> +
> + /* Program the 3 TLL channels upfront */
How about if the board doesn't use all three channels?
> +
> + /* CHANNEL-1 */
> + /* Disable AutoIdle */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(0)) &
> + ~(1<<OMAP_TLL_CHANNEL_CONF_UTMIAUTOIDLE_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(0));
> + /* Disable BitStuffing */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(0)) &
> + ~(1<<OMAP_TLL_CHANNEL_CONF_ULPINOBITSTUFF_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(0));
> + /* SDR Mode */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(0)) &
> + ~(1<<OMAP_TLL_CHANNEL_CONF_ULPIDDRMODE_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(0));
> +
> + /* CHANNEL-2 */
> + /* Disable AutoIdle */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(1)) &
> + ~(1<<OMAP_TLL_CHANNEL_CONF_UTMIAUTOIDLE_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(1));
> + /* Disable BitStuffing */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(1)) &
> + ~(1<<OMAP_TLL_CHANNEL_CONF_ULPINOBITSTUFF_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(1));
> + /* SDR Mode */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(1)) &
> + ~(1<<OMAP_TLL_CHANNEL_CONF_ULPIDDRMODE_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(1));
> +
> + /* CHANNEL-3 */
> + /* Disable AutoIdle */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(2)) &
> + ~(1<<OMAP_TLL_CHANNEL_CONF_UTMIAUTOIDLE_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(2));
> + /* Disable BitStuffing */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(2)) &
> + ~(1<<OMAP_TLL_CHANNEL_CONF_ULPINOBITSTUFF_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(2));
> + /* SDR Mode */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(2)) &
> + ~(1<<OMAP_TLL_CHANNEL_CONF_ULPIDDRMODE_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(2));
> +
> + /* Program Common TLL register */
> + omap_writel((1 << OMAP_TLL_SHARED_CONF_FCLK_IS_ON_SHIFT) |
> + (1 << OMAP_TLL_SHARED_CONF_USB_DIVRATION_SHIFT) |
> + (1 << OMAP_TLL_SHARED_CONF_USB_180D_SDR_EN_SHIFT) |
> + (1 << OMAP_TLL_SHARED_CONF_USB_90D_DDR_EN_SHFT),
> + OMAP_TLL_SHARED_CONF);
> +
> + /* Enable All 3 channels now */
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(0)) |
> + (1<<OMAP_TLL_CHANNEL_CONF_CHANEN_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(0));
> +
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(1)) |
> + (1<<OMAP_TLL_CHANNEL_CONF_CHANEN_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(1));
> +
> + omap_writel(omap_readl(OMAP_TLL_CHANNEL_CONF(2)) |
> + (1<<OMAP_TLL_CHANNEL_CONF_CHANEN_SHIFT),
> + OMAP_TLL_CHANNEL_CONF(2));
> +
> + /* test writing to ulpi scratch register */
> + omap_writeb(0xBE, OMAP_TLL_ULPI_SCRATCH_REGISTER);
> + dev_dbg(hcd->self.controller, "\nULPI_SCRATCH_REG 0x%02x\n",
> + omap_readb(OMAP_TLL_ULPI_SCRATCH_REGISTER));
> +
> +#endif
> +
> +#ifdef EXTERNAL_PHY_RESET
> + /* Refer ISSUE1:
> + * Hold the PHY in RESET for enough time till PHY is settled and ready
> + */
> + udelay(EXT_PHY_RESET_DELAY);
> + omap_set_gpio_dataout(EXT_PHY_RESET_GPIO_PORT1, 1);
> + omap_set_gpio_dataout(EXT_PHY_RESET_GPIO_PORT2, 1);
> +#endif
> +
> +#ifdef VBUS_INTERNAL_CHARGEPUMP_HACK
> + /* Refer ISSUE2: LINK assumes external charge pump */
> +
> + /* use Port1 VBUS to charge externally Port2:
> + * So for PHY mode operation use Port2 only
> + */
> + omap_writel((0xA << EHCI_INSNREG05_ULPI_REGADD_SHIFT) |/* OTG ctrl reg*/
> + (2 << EHCI_INSNREG05_ULPI_OPSEL_SHIFT) |/* Write */
> + (1 << EHCI_INSNREG05_ULPI_PORTSEL_SHIFT) |/* Port1 */
> + (1 << EHCI_INSNREG05_ULPI_CONTROL_SHIFT) |/* Start */
> + (0x26),
> + EHCI_INSNREG05_ULPI);
> +
> + while (!(omap_readl(EHCI_INSNREG05_ULPI) &
> + (1<<EHCI_INSNREG05_ULPI_CONTROL_SHIFT)));
> +
> +#endif
Yeah, this routine is almost entirely board-specific stuff that
should be abstracted out of this driver.
> +
> + return 0;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +static void omap_stop_ehc(struct platform_device *dev, struct usb_hcd *hcd)
> +{
> + struct ehci_omap_clock_defs *ehci_clocks;
> +
> + ehci_clocks = (struct ehci_omap_clock_defs *)
> + (((char *)hcd_to_ehci(hcd)) + sizeof(struct ehci_hcd));
> +
> + dev_dbg(hcd->self.controller, ": stopping TI EHCI USB Controller\n");
> +
> + /* Reset OMAP modules for insmod/rmmod to work */
> + omap_writel((1<<1), OMAP_UHH_SYSCONFIG);
> + while (!(omap_readl(OMAP_UHH_SYSSTATUS) & (1<<0)));
> + while (!(omap_readl(OMAP_UHH_SYSSTATUS) & (1<<1)));
> + while (!(omap_readl(OMAP_UHH_SYSSTATUS) & (1<<2)));
> + dev_dbg(hcd->self.controller,
> + "UHH RESET DONE OMAP_UHH_SYSSTATUS %x !!\n",
> + omap_readl(OMAP_UHH_SYSSTATUS));
> +
> + omap_writel((1<<1), OMAP_USBTLL_SYSCONFIG);
> + while (!(omap_readl(OMAP_USBTLL_SYSSTATUS) & (1<<0)));
> + dev_dbg(hcd->self.controller, ":TLL RESEET DONE");
> +
> + if (ehci_clocks->usbtll_fck_clk != NULL) {
> + clk_disable(ehci_clocks->usbtll_fck_clk);
> + clk_put(ehci_clocks->usbtll_fck_clk);
> + ehci_clocks->usbtll_fck_clk = NULL;
> + }
> +
> + if (ehci_clocks->usbhost_ick_clk != NULL) {
> + clk_disable(ehci_clocks->usbhost_ick_clk);
> + clk_put(ehci_clocks->usbhost_ick_clk);
> + ehci_clocks->usbhost_ick_clk = NULL;
> + }
> +
> + if (ehci_clocks->usbhost1_48m_fck_clk != NULL) {
> + clk_disable(ehci_clocks->usbhost1_48m_fck_clk);
> + clk_put(ehci_clocks->usbhost1_48m_fck_clk);
> + ehci_clocks->usbhost1_48m_fck_clk = NULL;
> + }
> +
> + if (ehci_clocks->usbhost2_120m_fck_clk != NULL) {
> + clk_disable(ehci_clocks->usbhost2_120m_fck_clk);
> + clk_put(ehci_clocks->usbhost2_120m_fck_clk);
> + ehci_clocks->usbhost2_120m_fck_clk = NULL;
> + }
> +
> + if (ehci_clocks->usbtll_ick_clk != NULL) {
> + clk_disable(ehci_clocks->usbtll_ick_clk);
> + clk_put(ehci_clocks->usbtll_ick_clk);
> + ehci_clocks->usbtll_ick_clk = NULL;
> + }
> +
> +
> +#ifdef EXTERNAL_PHY_RESET
> + omap_free_gpio(EXT_PHY_RESET_GPIO_PORT1);
> + omap_free_gpio(EXT_PHY_RESET_GPIO_PORT2);
> +#endif
> +
> + dev_dbg(hcd->self.controller,
> + ": Clock to USB host has been disabled\n");
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +/* Forward declaration */
> +extern const struct hc_driver ehci_omap_hc_driver;
Forward decls ... yeech. Why not just put that struct
decllaration here? And it should *NOT* be "extern".
Nothing defined in this file should be anything but
"static", near as I can tell.
> +
> +/*-------------------------------------------------------------------------*/
> +/* configure so an HC device and id are always provided */
> +/* always called with process context; sleeping is OK */
Why are you copying these interface descriptions?
Especially when you effectively duplicate one with the
"Context:" kerneldoc?
> +
> +/**
> + * ehci_hcd_omap_drv_probe - initialize TI-based HCDs
> + * Context: !in_interrupt()
> + *
> + * Allocates basic resources for this USB host controller, and
> + * then invokes the start() method for the HCD associated with it
> + * through the hotplug entry's driver_data.
> + *
> + */
> +static int ehci_hcd_omap_drv_probe(struct platform_device *dev)
Convention: name platform_device parameters "pdev".
That helps avoid ugliness like the "&dev->dev" stuff below...
> +{
> + int retval = 0;
> + struct usb_hcd *hcd;
> + struct ehci_hcd *ehci;
> +
> + dev_dbg(&dev->dev, "ehci_hcd_omap_drv_probe()");
> +
> + if (usb_disabled())
> + return -ENODEV;
> +
> + if (dev->resource[1].flags != IORESOURCE_IRQ) {
> + dev_dbg(&dev->dev, "resource[1] is not IORESOURCE_IRQ");
> + retval = -ENOMEM;
> + }
Just use platform_get_irq() and fail if the IRQ is missing.
> +
> + hcd = usb_create_hcd(&ehci_omap_hc_driver, &dev->dev, dev->dev.bus_id);
> + if (!hcd)
> + return -ENOMEM;
> +
> + retval = omap_start_ehc(dev, hcd);
> + if (retval)
> + return retval;
> +
> + hcd->rsrc_start = 0;
> + hcd->rsrc_len = 0;
> + hcd->rsrc_start = dev->resource[0].start;
> + hcd->rsrc_len = dev->resource[0].end - dev->resource[0].start + 1;
> +
> + hcd->regs = (void __iomem *) (int) IO_ADDRESS(hcd->rsrc_start);
... and platform_get_resource(pdev, IORESOURCE_MEM, 0).
Notice how you didn't check the type of resource[0]??
Why do that for IRQ but not MEM? Better, why not
let the platform_get_*() code handle that?
> +
> + ehci = hcd_to_ehci(hcd);
> + ehci->caps = hcd->regs;
> +
> + ehci->regs = hcd->regs + HC_LENGTH(readl(&ehci->caps->hc_capbase));
> + /* cache this readonly data; minimize chip reads */
> + ehci->hcs_params = readl(&ehci->caps->hcs_params);
> +
> + /* SET 1 micro-frame Interrupt interval */
> + writel(readl(&ehci->regs->command) | (1<<16), &ehci->regs->command);
I'm pretty sure that at least some of that will be done by
generic EHCI driver code. And it'll use the IRQ interval
that's configured by the module parameter ...
> +
> + retval = usb_add_hcd(hcd, dev->resource[1].start,
> + IRQF_DISABLED | IRQF_SHARED);
> + if (retval == 0)
> + return retval;
> +
> + dev_dbg(hcd->self.controller, "ERR: add_hcd");
> + omap_stop_ehc(dev, hcd);
> +
> + usb_put_hcd(hcd);
> + return retval;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +
> +/* may be called without controller electrically present */
> +/* may be called with controller, bus, and devices active */
Again, don't bother copying those comments. I was unaware
that OMAP3 was a modular processor, for example, where one
could physically remove the EHCI controller without a FIB
or similar apparatus. ;)
> +
> +/**
> + * ehci_hcd_omap_drv_remove - shutdown processing for EHCI HCDs
> + * @dev: USB Host Controller being removed
> + * Context: !in_interrupt()
> + *
> + * Reverses the effect of usb_ehci_hcd_omap_probe(), first invoking
> + * the HCD's stop() method. It is always called from a thread
> + * context, normally "rmmod", "apmd", or something similar.
> + *
> + */
> +static int ehci_hcd_omap_drv_remove(struct platform_device *dev)
> +{
> + struct usb_hcd *hcd = platform_get_drvdata(dev);
> +
> + dev_dbg(&dev->dev, "ehci_hcd_omap_drv_remove()");
> +
> + usb_remove_hcd(hcd);
> + usb_put_hcd(hcd);
> + omap_stop_ehc(dev, hcd);
> +
> + return 0;
> +}
> +
> +/*-------------------------------------------------------------------------*/
> +#ifdef CONFIG_PM
> +static int omap_ehci_bus_suspend(struct usb_hcd *hcd)
> +{
> + return ehci_bus_suspend(hcd);
> +}
> +
> +static int omap_ehci_bus_resume(struct usb_hcd *hcd)
> +{
> + return ehci_bus_resume(hcd);
> +}
> +#endif
Ah ... why do you have a NOP layer here?
I'm guessing they're a placeholder for something which
does sane stuff with the clocks. If so, stick in some
comment along those lines ("FIXME" etc).
> +/*-------------------------------------------------------------------------*/
> +
> +const struct hc_driver ehci_omap_hc_driver = {
> + .description = hcd_name,
> + .product_desc = "OMAP-EHCI Host Controller",
> + .hcd_priv_size = sizeof(struct ehci_hcd)
> + + sizeof(struct ehci_omap_clock_defs),
Again: it'd be a lot better if you had a structure that held both
those, so that you could use typesafe casting in various places
(instead of error-prone pointer arithmetic).
> +
> + /*
> + * generic hardware linkage
> + */
> + .irq = ehci_irq,
> + .flags = HCD_MEMORY | HCD_USB2,
> +
> + /*
> + * basic lifecycle operations
> + */
> + .reset = ehci_init,
You surely have more one-time init than ehci_init does...
compare what ehci_pci_setup() does to what you do.
I suspect that some of the nuances there -- like needing
a reinit path for system suspend states which power off
this controller, instead of assuming there's only a kind
of retention mode that can issue wakeup events -- will
apply to OMAP3 too.
> + .start = ehci_run,
> + .stop = ehci_stop,
> + .shutdown = ehci_shutdown,
> +
> + /*
> + * managing i/o requests and associated device resources
> + */
> + .urb_enqueue = ehci_urb_enqueue,
> + .urb_dequeue = ehci_urb_dequeue,
> + .endpoint_disable = ehci_endpoint_disable,
> +
> + /*
> + * scheduling support
> + */
> + .get_frame_number = ehci_get_frame,
> +
> + /*
> + * root hub support
> + */
> + .hub_status_data = ehci_hub_status_data,
> + .hub_control = ehci_hub_control,
> +#ifdef CONFIG_PM
> + .bus_suspend = omap_ehci_bus_suspend,
> + .bus_resume = omap_ehci_bus_resume,
> +#endif
Best to avoid #ifdefs. In this case you've already got one (above),
so you can just add an #else there with #defines for those symbols
(as NULL).
> +};
> +
> +/*-------------------------------------------------------------------------*/
> +MODULE_ALIAS("omap-ehci");
Courtesy of a backwards-incompatible and incomplete (== broken)
patch a while back, this should have a "platform:" prefix.
And note that it doesn't match the "ehci-omap" below, so there's
no way this module could properly hotplug...
> +static struct platform_driver ehci_hcd_omap_driver = {
> + .probe = ehci_hcd_omap_drv_probe,
> + .remove = ehci_hcd_omap_drv_remove,
> + .shutdown = usb_hcd_platform_shutdown,
> + /*.suspend = ehci_hcd_omap_drv_suspend, */
> + /*.resume = ehci_hcd_omap_drv_resume, */
If you're going to need those, then don't comment them out.
Else don't even have them commented out.
> + .driver = {
> + .name = "ehci-omap",
> + .bus = &platform_bus_type
NEVER pass a bus_type in a driver initializer. That's what
the platform-specific driver registration code is for.
> + }
> +};
> Index: omap-submit/drivers/usb/host/ehci-hcd.c
> ===================================================================
> --- omap-submit.orig/drivers/usb/host/ehci-hcd.c 2008-02-04 17:06:27.000000000 +0530
> +++ omap-submit/drivers/usb/host/ehci-hcd.c 2008-02-04 17:07:14.000000000 +0530
> @@ -954,6 +954,11 @@
> #define PLATFORM_DRIVER ehci_hcd_au1xxx_driver
> #endif
>
> +#ifdef CONFIG_ARCH_OMAP34XX
> +#include "ehci-omap.c"
> +#define PLATFORM_DRIVER ehci_hcd_omap_driver
> +#endif
> +
> #ifdef CONFIG_PPC_PS3
> #include "ehci-ps3.c"
> #define PS3_SYSTEM_BUS_DRIVER ps3_ehci_driver
> Index: omap-submit/drivers/usb/host/ehci-omap.h
> ===================================================================
> --- /dev/null 1970-01-01 00:00:00.000000000 +0000
> +++ omap-submit/drivers/usb/host/ehci-omap.h 2008-02-04 17:07:14.000000000 +0530
> @@ -0,0 +1,116 @@
> +/*
> + * ehci-omap.h - register definitions for USBHOST in OMAP 34xx
> + *
> + * Copyright (C) 2007-2008 Texas Instruments, Inc.
> + * Copyright (C) 2007-2008 Vikram Pandita <vikram.pandita@xxxxxx>
As I understand U.S. copyright law, you don't actually hold a
separate copyright unless this is not "work for hire". And if
you collect a paycheck from TI, that's all but certainly what
this code is ...
> + *
> + * 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
> + *
> + */
> +
> +#ifndef __EHCI_OMAP_H
> +#define __EHCI_OMAP_H
> +
> +#include <asm/hardware.h>
> +#include "../../../arch/arm/mach-omap2/cm.h"
> +#include "../../../arch/arm/mach-omap2/cm_regbits_34xx.h"
Broken. If these headers need to be accessed outside of
the mach-omap2 tree, they belong in <asm-arm/arch-omap/...>
header files.
> +
> +/*
> + * OMAP USBHOST Register addresses: PHYSICAL ADDRESSES
> + * Use omap_readl()/omap_writel() functions
> + */
Yes, having controller-specific registers associated with the
controller's driver is the way to go. Good!
> +
> +/* USBHOST: TLL, UUH, OHCI, EHCI */
> +#define OMAP_USBHOST_BASE (L4_34XX_BASE + 0x60000)
> +
> +/* TLL Register Set */
> +#define OMAP_USBHOST_TLL_BASE (OMAP_USBHOST_BASE + 0x2000)
> +#define OMAP_USBTLL_REVISION (OMAP_USBHOST_TLL_BASE + 0x00)
> +#define OMAP_USBTLL_SYSCONFIG (OMAP_USBHOST_TLL_BASE + 0x10)
> + #define OMAP_USBTLL_SYSCONFIG_CACTIVITY_SHIFT 8
> + #define OMAP_USBTLL_SYSCONFIG_SIDLEMODE_SHIFT 3
> + #define OMAP_USBTLL_SYSCONFIG_ENAWAKEUP_SHIFT 2
> + #define OMAP_USBTLL_SYSCONFIG_SOFTRESET_SHIFT 1
> + #define OMAP_USBTLL_SYSCONFIG_AUTOIDLE_SHIFT 0
> +#define OMAP_USBTLL_SYSSTATUS (OMAP_USBHOST_TLL_BASE + 0x14)
> + #define OMAP_USBTLL_SYSSTATUS_RESETDONE_SHIFT 0
> +#define OMAP_USBTLL_IRQSTATUS (OMAP_USBHOST_TLL_BASE + 0x18)
> +#define OMAP_USBTLL_IRQENABLE (OMAP_USBHOST_TLL_BASE + 0x1C)
> +
> +#define OMAP_TLL_SHARED_CONF (OMAP_USBHOST_TLL_BASE + 0x30)
> + #define OMAP_TLL_SHARED_CONF_USB_90D_DDR_EN_SHFT 6
> + #define OMAP_TLL_SHARED_CONF_USB_180D_SDR_EN_SHIFT 5
> + #define OMAP_TLL_SHARED_CONF_USB_DIVRATION_SHIFT 2
> + #define OMAP_TLL_SHARED_CONF_FCLK_REQ_SHIFT 1
> + #define OMAP_TLL_SHARED_CONF_FCLK_IS_ON_SHIFT 0
> +
> +#define OMAP_TLL_CHANNEL_CONF(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x040 + 0x004 * num))
> + #define OMAP_TLL_CHANNEL_CONF_ULPINOBITSTUFF_SHIFT 11
> + #define OMAP_TLL_CHANNEL_CONF_ULPI_ULPIAUTOIDLE_SHIFT 10
> + #define OMAP_TLL_CHANNEL_CONF_UTMIAUTOIDLE_SHIFT 9
> + #define OMAP_TLL_CHANNEL_CONF_ULPIDDRMODE_SHIFT 8
> + #define OMAP_TLL_CHANNEL_CONF_CHANEN_SHIFT 0
> +
> +#define OMAP_TLL_ULPI_FUNCTION_CTRL(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x804 + 0x100 * num))
> +#define OMAP_TLL_ULPI_INTERFACE_CTRL(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x807 + 0x100 * num))
> +#define OMAP_TLL_ULPI_OTG_CTRL(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x80A + 0x100 * num))
> +#define OMAP_TLL_ULPI_INT_EN_RISE(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x80D + 0x100 * num))
> +#define OMAP_TLL_ULPI_INT_EN_FALL(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x810 + 0x100 * num))
> +#define OMAP_TLL_ULPI_INT_STATUS(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x813 + 0x100 * num))
> +#define OMAP_TLL_ULPI_INT_LATCH(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x814 + 0x100 * num))
> +#define OMAP_TLL_ULPI_DEBUG(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x815 + 0x100 * num))
> +#define OMAP_TLL_ULPI_SCRATCH_REGISTER(num)\
> + (OMAP_USBHOST_TLL_BASE + (0x8165 + 0x100 * num))
> +
> +/* UHH Register Set */
> +#define OMAP_USBHOST_UHH_BASE (OMAP_USBHOST_BASE + 0x4000)
> +#define OMAP_UHH_REVISION (OMAP_USBHOST_UHH_BASE + 0x00)
> +#define OMAP_UHH_SYSCONFIG (OMAP_USBHOST_UHH_BASE + 0x10)
> + #define OMAP_UHH_SYSCONFIG_MIDLEMODE_SHIFT 12
> + #define OMAP_UHH_SYSCONFIG_CACTIVITY_SHIFT 8
> + #define OMAP_UHH_SYSCONFIG_SIDLEMODE_SHIFT 3
> + #define OMAP_UHH_SYSCONFIG_ENAWAKEUP_SHIFT 2
> + #define OMAP_UHH_SYSCONFIG_SOFTRESET_SHIFT 1
> + #define OMAP_UHH_SYSCONFIG_AUTOIDLE_SHIFT 0
> +
> +#define OMAP_UHH_SYSSTATUS (OMAP_USBHOST_UHH_BASE + 0x14)
> +#define OMAP_UHH_HOSTCONFIG (OMAP_USBHOST_UHH_BASE + 0x40)
> + #define OMAP_UHH_HOSTCONFIG_ULPI_BYPASS_SHIFT 0
> +
> +#define OMAP_UHH_DEBUG_CSR (OMAP_USBHOST_UHH_BASE + 0x44)
> +
> +/* EHCI Register Set */
> +#define OMAP_USBHOST_EHCI_BASE (OMAP_USBHOST_BASE + 0x4800)
> +#define EHCI_INSNREG05_ULPI (OMAP_USBHOST_EHCI_BASE + 0xA4)
> + #define EHCI_INSNREG05_ULPI_CONTROL_SHIFT 31
> + #define EHCI_INSNREG05_ULPI_PORTSEL_SHIFT 24
> + #define EHCI_INSNREG05_ULPI_OPSEL_SHIFT 22
> + #define EHCI_INSNREG05_ULPI_REGADD_SHIFT 16
> + #define EHCI_INSNREG05_ULPI_EXTREGADD_SHIFT 8
> + #define EHCI_INSNREG05_ULPI_WRDATA_SHIFT 0
EHCI doesn't have ULPI registers. You should drop at
least the EHCI_ prefix for those.
> +
> +/* OHCI Register Set */
> +#define OMAP_USBHOST_OHCI_BASE (OMAP_USBHOST_BASE + 0x4400)
Umm, what's an OHCI register definition doing *HERE*?
That should instead be an addition to ohci-omap.c bus glue ...
> +
> +#endif/* __EHCI_OMAP_H*/
> Index: omap-submit/drivers/usb/Kconfig
> ===================================================================
> --- omap-submit.orig/drivers/usb/Kconfig 2008-02-04 17:06:27.000000000 +0530
> +++ omap-submit/drivers/usb/Kconfig 2008-02-04 17:07:14.000000000 +0530
> @@ -49,6 +49,7 @@
> boolean
> default y if PPC_83xx
> default y if SOC_AU1200
> + default y if ARCH_OMAP34XX
> default PCI
>
> # ARM SA1111 chips have a non-PCI based "OHCI-compatible" USB host interface.
> Index: omap-submit/drivers/usb/host/Kconfig
> ===================================================================
> --- omap-submit.orig/drivers/usb/host/Kconfig 2008-02-04 17:06:27.000000000 +0530
> +++ omap-submit/drivers/usb/host/Kconfig 2008-02-04 17:07:14.000000000 +0530
> @@ -28,6 +28,25 @@
>
> To compile this driver as a module, choose M here: the
> module will be called ehci-hcd.
> +choice
> + prompt "PHY/TLL mode"
> + depends on USB_EHCI_HCD && EXPERIMENTAL && ARCH_OMAP34XX
> + ---help---
> + Choose PHY or TLL mode of operation
Again, this PHY stuff should be in platform data, not Kconfig.
> +
> +config OMAP_EHCI_PHY_MODE
> + bool "PHY mode: ISP1504 on Port1/2 (NEW 3430ES2.0)"
> + depends on USB_EHCI_HCD && EXPERIMENTAL && ARCH_OMAP34XX
> + ---help---
> + EHCI PHY mode. Port1 and Port2 are connected to ISP1504 transcievers
> +
> +config OMAP_EHCI_TLL_MODE
> + bool "TLL mode: (EXPERIMENTAL)"
> + depends on USB_EHCI_HCD && EXPERIMENTAL && ARCH_OMAP34XX
> + ---help---
> + OMAP EHCI controller has TLL mode of operation for all 3 ports.
> + Use this mode when no transciever is present
> +endchoice
>
> config USB_EHCI_SPLIT_ISO
> bool "Full speed ISO transactions (EXPERIMENTAL)"
>
-
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
