On Wed, Jul 20, 2011 at 07:08:23PM +0800, Tony Lin wrote:
> add usb phy register definitions and functions
> usb host driver will use these functions to initialize
> usb phy and change phy working mode
>
>
> obj-y += devices/
> diff --git a/arch/arm/mach-mxs/regs-usbphy-mx28.h b/arch/arm/mach-mxs/regs-usbphy-mx28.h
> new file mode 100644
> index 0000000..a367927
> --- /dev/null
> +++ b/arch/arm/mach-mxs/regs-usbphy-mx28.h
> @@ -0,0 +1,323 @@
> +/*
> + * Freescale USBPHY Register Definitions
> + *
> + * Copyright 2008-2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * 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
> + *
> + * This file is created by xml file. Don't Edit it.
Please remove this comment. We can and will change this header if need
arises.
> + *
> + * Xml Revision: 1.52
> + * Template revision: 26195
> + */
> +
> +#ifndef __ARCH_ARM___USBPHY_H
> +#define __ARCH_ARM___USBPHY_H
> +
> +
> +#define HW_USBPHY_PWD (0x00000000)
> +#define HW_USBPHY_PWD_SET (0x00000004)
> +#define HW_USBPHY_PWD_CLR (0x00000008)
> +#define HW_USBPHY_PWD_TOG (0x0000000c)
> +
> +#define BP_USBPHY_PWD_RSVD2 21
> +#define BM_USBPHY_PWD_RSVD2 0xFFE00000
> +#define BF_USBPHY_PWD_RSVD2(v) \
> + (((v) << 21) & BM_USBPHY_PWD_RSVD2)
> +#define BM_USBPHY_PWD_RXPWDRX 0x00100000
> +#define BM_USBPHY_PWD_RXPWDDIFF 0x00080000
> +#define BM_USBPHY_PWD_RXPWD1PT1 0x00040000
> +#define BM_USBPHY_PWD_RXPWDENV 0x00020000
> +#define BP_USBPHY_PWD_RSVD1 13
> +#define BM_USBPHY_PWD_RSVD1 0x0001E000
> +#define BF_USBPHY_PWD_RSVD1(v) \
> + (((v) << 13) & BM_USBPHY_PWD_RSVD1)
Please remove all these reserved bits. They only reduce readability
> diff --git a/arch/arm/mach-mxs/usb_h1.c b/arch/arm/mach-mxs/usb_h1.c
>From what I know the i.MX28 has two identical USB phys. There shouldn't
be a file handling only one of these.
> new file mode 100644
> index 0000000..9ca5236
> --- /dev/null
> +++ b/arch/arm/mach-mxs/usb_h1.c
> @@ -0,0 +1,230 @@
> +/*
> + * Copyright (C) 2009-2011 Freescale Semiconductor, Inc. All Rights Reserved.
> + *
> + * 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.,
> + * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/types.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/platform_device.h>
> +#include <linux/io.h>
> +#include <linux/err.h>
> +#include <linux/fsl_devices.h>
> +#include <linux/gpio.h>
> +#include <asm/mach-types.h>
> +#include <asm/mach/arch.h>
> +#include <mach/irqs.h>
> +#include <mach/mx28.h>
> +#include "regs-usbphy-mx28.h"
> +
> +/* EHCI registers: */
> +#define UOG_USBCMD (0x140)/* USB command register */
> +#define UOG_PORTSC1 (0x184)/* port status and control */
> +/* x_PORTSCx */
> +#define PORTSC_PTS_MASK (3 << 30)/* parallel xcvr mask */
> +#define PORTSC_PTS_UTMI (0 << 30)/* UTMI/UTMI+ */
> +#define PORTSC_PTW (1 << 28)/* UTMI width */
> +/* USBCMD */
> +#define UCMD_RUN_STOP (1 << 0)/* controller run/stop */
> +#define UCMD_RESET (1 << 1)/* controller reset */
> +
> +#define HOSTPHY_CONNECT_STATE (1 << 3)
> +
> +static struct clk *usb_clk;
> +static struct clk *usb_phy_clk;
> +static int internal_phy_clk_already_on;
Please get rid fo these static variables. The i.MX28 has two USB phys
and two EHCI cores. All of these should be devices and the context
should be stored in data private to the driver.
> +
> +/* The dmamask must be set for EHCI to work */
> +static u64 ehci_dmamask = ~(u32) 0;
> +static int instance_id = ~(u32) 0;
> +
> +static int fsl_platform_get_usb_conn_status(void)
> +{
> + u32 status;
> +
> + status = __raw_readl(MX28_IO_ADDRESS(MX28_USBPHY1_BASE_ADDR) + \
> + HW_USBPHY_STATUS);
> + return ((status & HOSTPHY_CONNECT_STATE) == 0);
> +}
A function which takes absolutely no context looks suspicious.
> +
> +/* enable/disable high-speed disconnect detector of phy ctrl */
> +static void fsl_platform_set_usb_phy_dis(int enable)
> +{
> + if (enable)
> + __raw_writel(BM_USBPHY_CTRL_ENHOSTDISCONDETECT,
> + MX28_IO_ADDRESS(MX28_USBPHY1_BASE_ADDR) + HW_USBPHY_CTRL_SET);
> + else
> + __raw_writel(BM_USBPHY_CTRL_ENHOSTDISCONDETECT,
> + MX28_IO_ADDRESS(MX28_USBPHY1_BASE_ADDR) + HW_USBPHY_CTRL_CLR);
> +}
> +
> +static int usb_phy_enable(struct mxc_usbh_platform_data *pdata)
> +{
> + u32 tmp;
> + void __iomem *phy_reg = MX28_IO_ADDRESS(MX28_USBPHY1_BASE_ADDR);
> + void __iomem *usb_reg = MX28_IO_ADDRESS(MX28_USBCTRL1_BASE_ADDR);
This function takes device specific context and still it uses hardcoded
usbphy1 registers.
> + void __iomem *usbcmd, *phy_ctrl, *portsc;
> + /* Reset USB IP */
> + usbcmd = usb_reg + UOG_USBCMD;
> + tmp = __raw_readl(usbcmd); /* usb command */
> + tmp &= ~UCMD_RUN_STOP;
> + __raw_writel(tmp, usbcmd);
> + while (__raw_readl(usbcmd) & UCMD_RUN_STOP)
> + ;
No potentially endless loops please.
> + tmp |= UCMD_RESET;
> + __raw_writel(tmp, usbcmd);
> + while (__raw_readl(usbcmd) & UCMD_RESET)
> + ;
> + mdelay(10);
> + /* Reset USBPHY module */
> + phy_ctrl = phy_reg + HW_USBPHY_CTRL;
> + tmp = __raw_readl(phy_ctrl);
> + tmp |= BM_USBPHY_CTRL_SFTRST;
> + __raw_writel(tmp, phy_ctrl);
> + udelay(10);
> + /* Remove CLKGATE and SFTRST */
> + tmp = __raw_readl(phy_ctrl);
> + tmp &= ~(BM_USBPHY_CTRL_CLKGATE | BM_USBPHY_CTRL_SFTRST);
> + __raw_writel(tmp, phy_ctrl);
> + udelay(10);
> + /* set UTMI xcvr */
> + /* Workaround an IC issue for ehci driver:
> + * when turn off root hub port power, EHCI set
> + * PORTSC reserved bits to be 0, but PTW with 0
> + * means 8 bits tranceiver width, here change
> + * it back to be 16 bits and do PHY diable and
> + * then enable.
> + */
> + portsc = usb_reg + UOG_PORTSC1;
> + tmp = __raw_readl(portsc);
> + tmp &= ~PORTSC_PTS_MASK;
> + tmp |= (PORTSC_PTS_UTMI | PORTSC_PTW);
> + __raw_writel(tmp, portsc);
> + /* Power up the PHY */
> + __raw_writel(0, phy_reg + HW_USBPHY_PWD);
> + return 0;
> +}
> +
> +static int fsl_usb_host_init(struct platform_device *pdev)
> +{
> + struct mxc_usbh_platform_data *pdata = pdev->dev.platform_data;
> + void __iomem *phy_reg = MX28_IO_ADDRESS(MX28_USBPHY1_BASE_ADDR);
> + u32 tmp;
> +
> + usb_phy_enable(pdata);
> + /* enable FS/LS device */
> + tmp = __raw_readl(phy_reg + HW_USBPHY_CTRL);
> + tmp |= (BM_USBPHY_CTRL_ENUTMILEVEL2 | BM_USBPHY_CTRL_ENUTMILEVEL3);
> + __raw_writel(tmp, phy_reg + HW_USBPHY_CTRL);
> +
> + return 0;
> +}
> +
> +static void usbh1_internal_phy_clock_gate(bool on)
> +{
> + u32 tmp;
> + void __iomem *phy_reg = MX28_IO_ADDRESS(MX28_USBPHY1_BASE_ADDR);
> +
> + if (on) {
> + internal_phy_clk_already_on += 1;
> + if (internal_phy_clk_already_on == 1) {
> + tmp = BM_USBPHY_CTRL_SFTRST | BM_USBPHY_CTRL_CLKGATE;
> + __raw_writel(tmp, phy_reg + HW_USBPHY_CTRL_CLR);
> + }
> + } else {
> + internal_phy_clk_already_on -= 1;
> + if (internal_phy_clk_already_on == 0) {
> + tmp = BM_USBPHY_CTRL_CLKGATE;
> + __raw_writel(tmp, phy_reg + HW_USBPHY_CTRL_SET);
> + }
> + }
> + if (internal_phy_clk_already_on < 0)
> + printk(KERN_ERR "please check phy clock ON/OFF sequence\n");
> +}
> +static int fsl_usb_host_init_ext(struct platform_device *pdev)
> +{
> + usb_clk = clk_get(NULL, "usb1");
> + clk_enable(usb_clk);
> + clk_put(usb_clk);
> + usb_phy_clk = clk_get(NULL, "usb1_phy");
> + clk_enable(usb_phy_clk);
> + clk_put(usb_phy_clk);
> +
> + usbh1_internal_phy_clock_gate(true);
> + return fsl_usb_host_init(pdev);
> +}
> +
> +static int fsl_usb_host_uninit_ext(struct platform_device *pdev)
> +{
> + usbh1_internal_phy_clock_gate(false);
> + clk_disable(usb_phy_clk);
> + clk_disable(usb_clk);
> + return 0;
> +}
> +
> +static struct mxc_usbh_platform_data usbh1_config = {
> + .init = fsl_usb_host_init_ext,
> + .exit = fsl_usb_host_uninit_ext,
> + .portsc = MXC_EHCI_MODE_ULPI,
> + .otg = NULL,
> + .plt_get_usb_connect_status = fsl_platform_get_usb_conn_status,
> + .plt_usb_disconnect_detect = fsl_platform_set_usb_phy_dis,
> +};
> +
> +/* The resources for kinds of usb devices */
> +static struct resource usbh1_resources[] = {
> + [0] = {
> + .start = (u32) (MX28_USBCTRL1_BASE_ADDR),
> + .end = (u32) (MX28_USBCTRL1_BASE_ADDR + 0x1ff),
> + .flags = IORESOURCE_MEM,
> + },
> + [1] = {
> + .start = MX28_INT_USB1,
> + .flags = IORESOURCE_IRQ,
> + },
> +};
> +
> +static int __init usbh1_init(void)
> +{
> + struct platform_device *pdev;
> + int rc;
> +
> + if (!cpu_is_mx28())
> + return 0;
> +
> + pdev = platform_device_register_simple("mxc-ehci",
> + instance_id, usbh1_resources, ARRAY_SIZE(usbh1_resources));
What is this global instance_id you use here? This function is clearly
called only once, yet it uses a variable which is incremented below
and then never used again.
> + if (IS_ERR(pdev)) {
> + pr_debug("can't register Host, %ld\n",
> + PTR_ERR(pdev));
> + return -EFAULT;
> + }
> +
> + pdev->dev.coherent_dma_mask = 0xffffffff;
> + pdev->dev.dma_mask = &ehci_dmamask;
> +
> + rc = platform_device_add_data(pdev, &usbh1_config,
> + sizeof(struct mxc_usbh_platform_data));
I doubt it's allowed to add platform data after registering a device.
The driver could already be probed after register and bailed out
because of missing platform data.
Overall this needs major rework.
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
