No subject


Mon Jun 27 16:47:34 EDT 2011


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 |



More information about the linux-arm-kernel mailing list