RE: [PATCH 4/7] mx28: add usb host phy functions

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> -----Original Message-----
> From: Sascha Hauer [mailto:s.hauer@xxxxxxxxxxxxxx]
> Sent: Thursday, July 21, 2011 3:26 AM
> To: Lin Tony-B19295
> Cc: linux-usb@xxxxxxxxxxxxxxx; koen.beel.barco@xxxxxxxxx; balbi@xxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> Subject: Re: [PATCH 4/7] mx28: add usb host phy functions
> 
> 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
Ok, I'll consider all of your recommendations, thanks

> 
> 
> --
> 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


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux