| Message ID | 1437437826-3171-1-git-send-email-aalonso@freescale.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
On Tuesday, July 21, 2015 at 02:17:06 AM, Adrian Alonso wrote: > * Add support for usb driver for i.MX7D SoC > > Signed-off-by: Adrian Alonso <aalonso@freescale.com> > Signed-off-by: Ye.Li <B37916@freescale.com> > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> Hi! Do we really need yet-another-driver for i.MX ? > --- > Changes for V2: Resend > > drivers/usb/host/Makefile | 1 + > drivers/usb/host/ehci-mx7.c | 103 > ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 104 > insertions(+) > create mode 100644 drivers/usb/host/ehci-mx7.c > > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile > index 4d35d3e..7267160 100644 > --- a/drivers/usb/host/Makefile > +++ b/drivers/usb/host/Makefile > @@ -36,6 +36,7 @@ obj-$(CONFIG_USB_EHCI_MXC) += ehci-mxc.o > obj-$(CONFIG_USB_EHCI_MXS) += ehci-mxs.o > obj-$(CONFIG_USB_EHCI_MX5) += ehci-mx5.o > obj-$(CONFIG_USB_EHCI_MX6) += ehci-mx6.o > +obj-$(CONFIG_USB_EHCI_MX7) += ehci-mx7.o > obj-$(CONFIG_USB_EHCI_OMAP) += ehci-omap.o > obj-$(CONFIG_USB_EHCI_PPC4XX) += ehci-ppc4xx.o > obj-$(CONFIG_USB_EHCI_MARVELL) += ehci-marvell.o > diff --git a/drivers/usb/host/ehci-mx7.c b/drivers/usb/host/ehci-mx7.c > new file mode 100644 > index 0000000..7429d62 > --- /dev/null > +++ b/drivers/usb/host/ehci-mx7.c > @@ -0,0 +1,103 @@ > +/* > + * Copyright (c) 2009 Daniel Mack <daniel@caiaq.de> > + * Copyright (C) 2010-2015 Freescale Semiconductor, Inc. > + * > + * SPDX-License-Identifier: GPL-2.0+ > + */ > + > +#include <common.h> > +#include <usb.h> > +#include <errno.h> > +#include <linux/compiler.h> > +#include <usb/ehci-fsl.h> > +#include <asm/io.h> > +#include <asm/arch/crm_regs.h> > +#include <asm/arch/clock.h> > +#include <asm/imx-common/iomux-v3.h> > + > +#include "ehci.h" > + > +#define USB_NC_OFFSET 0x200 > + > +#define UCTRL_PM (1 << 9) /* OTG Power Mask */ > +#define UCTRL_OVER_CUR_POL (1 << 8) /* OTG Polarity of Overcurrent */ > +#define UCTRL_OVER_CUR_DIS (1 << 7) /* Disable OTG Overcurrent Detection > */ + Are these bits EHCI spiecific or MX7 Chipidea controller specific ? > +/* USBCMD */ > +#define UCMD_RUN_STOP (1 << 0) /* controller run/stop */ > +#define UCMD_RESET (1 << 1) /* controller reset */ > + > +/* Base address for this IP block is 0x02184800 */ > +struct usbnc_regs { > + u32 ctrl1; > + u32 ctrl2; Please stop mixing tab and space, use tab :) > + u32 reserve1[11]; > + u32 phy_ctrl2; > + u32 reserve2[6]; > + u32 adp_cfg1; > + u32 reserve3; > + u32 adp_status; > +}; > + > +static void usb_oc_config(int index) > +{ > + struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR + > + (0x10000 * index) + USB_NC_OFFSET); > + void __iomem *ctrl = (void __iomem *)(&usbnc->ctrl1); > + u32 val; > + > + val = __raw_readl(ctrl); > + val |= UCTRL_OVER_CUR_POL; > + __raw_writel(val, ctrl); setbits_le32() here. > + val = __raw_readl(ctrl); > + val |= (UCTRL_OVER_CUR_DIS | UCTRL_PM); > + __raw_writel(val, ctrl); Here as well. > +} > + > +int __weak board_ehci_hcd_init(int port) > +{ > + return 0; > +} > + > +int __weak board_ehci_power(int port, int on) > +{ > + return 0; > +} Can you add kerneldoc style comments to the functions please ? > +int ehci_hcd_init(int index, enum usb_init_type init, > + struct ehci_hccr **hccr, struct ehci_hcor **hcor) > +{ > + struct usb_ehci *ehci = (struct usb_ehci *)(USB_BASE_ADDR + > + (0x10000 * index)); > + > + if (index > 3) > + return -EINVAL; > + enable_usboh3_clk(1); > + mdelay(1); > + > + /* Do board specific initialization */ > + board_ehci_hcd_init(index); > + > + usb_oc_config(index); > + > + *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); > + *hcor = (struct ehci_hcor *)((uint32_t)*hccr + > + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); > + > + board_ehci_power(index, (init == USB_INIT_DEVICE) ? 0 : 1); > + if (init == USB_INIT_DEVICE) > + return 0; > + setbits_le32(&ehci->usbmode, CM_HOST); > + __raw_writel(CONFIG_MXC_USB_PORTSC, &ehci->portsc); Just use writel(). > + setbits_le32(&ehci->portsc, USB_EN); > + > + mdelay(10); > + > + return 0; > +} > + > +int ehci_hcd_stop(int index) > +{ You cannot stop the controller at all once you start it ? That looks like yet another hardware flub :-( > + return 0; > +} Best regards, Marek Vasut
Hi Marek, Comments inline. > -----Original Message----- > From: Marek Vasut [mailto:marex@denx.de] > Sent: Monday, July 20, 2015 8:56 PM > To: Alonso Lazcano Adrian-B38018 > Cc: u-boot@lists.denx.de; sbabic@denx.de; otavio@ossystems.com.br; > Estevam Fabio-R49496; Li Frank-B20596; Garg Nitin-B37173 > Subject: Re: [PATCH 02/15][v2] imx: usb: ehci-mx7 add usb driver for i.MX7D > > On Tuesday, July 21, 2015 at 02:17:06 AM, Adrian Alonso wrote: > > * Add support for usb driver for i.MX7D SoC > > > > Signed-off-by: Adrian Alonso <aalonso@freescale.com> > > Signed-off-by: Ye.Li <B37916@freescale.com> > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > > Hi! > > Do we really need yet-another-driver for i.MX ? > [Adrian] USB support for iMX could be re-implemented following DM model, At the moment ehci-mx7 handles the SoC specific settings for USB support. > > --- > > Changes for V2: Resend > > > > drivers/usb/host/Makefile | 1 + > > drivers/usb/host/ehci-mx7.c | 103 > > ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, > 104 > > insertions(+) > > create mode 100644 drivers/usb/host/ehci-mx7.c > > > > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile > > index 4d35d3e..7267160 100644 > > --- a/drivers/usb/host/Makefile > > +++ b/drivers/usb/host/Makefile > > @@ -36,6 +36,7 @@ obj-$(CONFIG_USB_EHCI_MXC) += ehci-mxc.o > > obj-$(CONFIG_USB_EHCI_MXS) += ehci-mxs.o > > obj-$(CONFIG_USB_EHCI_MX5) += ehci-mx5.o > > obj-$(CONFIG_USB_EHCI_MX6) += ehci-mx6.o > > +obj-$(CONFIG_USB_EHCI_MX7) += ehci-mx7.o > > obj-$(CONFIG_USB_EHCI_OMAP) += ehci-omap.o > > obj-$(CONFIG_USB_EHCI_PPC4XX) += ehci-ppc4xx.o > > obj-$(CONFIG_USB_EHCI_MARVELL) += ehci-marvell.o diff --git > > a/drivers/usb/host/ehci-mx7.c b/drivers/usb/host/ehci-mx7.c new file > > mode 100644 index 0000000..7429d62 > > --- /dev/null > > +++ b/drivers/usb/host/ehci-mx7.c > > @@ -0,0 +1,103 @@ > > +/* > > + * Copyright (c) 2009 Daniel Mack <daniel@caiaq.de> > > + * Copyright (C) 2010-2015 Freescale Semiconductor, Inc. > > + * > > + * SPDX-License-Identifier: GPL-2.0+ > > + */ > > + > > +#include <common.h> > > +#include <usb.h> > > +#include <errno.h> > > +#include <linux/compiler.h> > > +#include <usb/ehci-fsl.h> > > +#include <asm/io.h> > > +#include <asm/arch/crm_regs.h> > > +#include <asm/arch/clock.h> > > +#include <asm/imx-common/iomux-v3.h> > > + > > +#include "ehci.h" > > + > > +#define USB_NC_OFFSET 0x200 > > + > > +#define UCTRL_PM (1 << 9) /* OTG Power Mask */ > > +#define UCTRL_OVER_CUR_POL (1 << 8) /* OTG Polarity of Overcurrent > */ > > +#define UCTRL_OVER_CUR_DIS (1 << 7) /* Disable OTG Overcurrent > Detection > > */ + > > Are these bits EHCI spiecific or MX7 Chipidea controller specific ? [Adrian] They are iMX7 specific > > > +/* USBCMD */ > > +#define UCMD_RUN_STOP (1 << 0) /* controller run/stop */ > > +#define UCMD_RESET (1 << 1) /* controller reset */ > > + > > +/* Base address for this IP block is 0x02184800 */ struct usbnc_regs > > +{ > > + u32 ctrl1; > > + u32 ctrl2; > > Please stop mixing tab and space, use tab :) > > > + u32 reserve1[11]; > > + u32 phy_ctrl2; > > + u32 reserve2[6]; > > + u32 adp_cfg1; > > + u32 reserve3; > > + u32 adp_status; > > +}; > > + > > +static void usb_oc_config(int index) > > +{ > > + struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR + > > + (0x10000 * index) + USB_NC_OFFSET); > > + void __iomem *ctrl = (void __iomem *)(&usbnc->ctrl1); > > + u32 val; > > + > > + val = __raw_readl(ctrl); > > + val |= UCTRL_OVER_CUR_POL; > > + __raw_writel(val, ctrl); > > setbits_le32() here. > > > + val = __raw_readl(ctrl); > > + val |= (UCTRL_OVER_CUR_DIS | UCTRL_PM); > > + __raw_writel(val, ctrl); > > Here as well. > > > +} > > + > > +int __weak board_ehci_hcd_init(int port) { > > + return 0; > > +} > > + > > +int __weak board_ehci_power(int port, int on) { > > + return 0; > > +} > > Can you add kerneldoc style comments to the functions please ? [Adrian] All change suggestion to be addressed in a new patch version. > > > +int ehci_hcd_init(int index, enum usb_init_type init, > > + struct ehci_hccr **hccr, struct ehci_hcor **hcor) { > > + struct usb_ehci *ehci = (struct usb_ehci *)(USB_BASE_ADDR + > > + (0x10000 * index)); > > + > > + if (index > 3) > > + return -EINVAL; > > + enable_usboh3_clk(1); > > + mdelay(1); > > + > > + /* Do board specific initialization */ > > + board_ehci_hcd_init(index); > > + > > + usb_oc_config(index); > > + > > + *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); > > + *hcor = (struct ehci_hcor *)((uint32_t)*hccr + > > + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); > > + > > + board_ehci_power(index, (init == USB_INIT_DEVICE) ? 0 : 1); > > + if (init == USB_INIT_DEVICE) > > + return 0; > > + setbits_le32(&ehci->usbmode, CM_HOST); > > + __raw_writel(CONFIG_MXC_USB_PORTSC, &ehci->portsc); > > Just use writel(). > > > + setbits_le32(&ehci->portsc, USB_EN); > > + > > + mdelay(10); > > + > > + return 0; > > +} > > + > > +int ehci_hcd_stop(int index) > > +{ > > You cannot stop the controller at all once you start it ? That looks like yet > another hardware flub :-( > > > + return 0; > > +} > > Best regards, > Marek Vasut
On Wednesday, July 22, 2015 at 11:15:31 PM, Alonso Adrian wrote: > Hi Marek, > > Comments inline. > > > -----Original Message----- > > From: Marek Vasut [mailto:marex@denx.de] > > Sent: Monday, July 20, 2015 8:56 PM > > To: Alonso Lazcano Adrian-B38018 > > Cc: u-boot@lists.denx.de; sbabic@denx.de; otavio@ossystems.com.br; > > Estevam Fabio-R49496; Li Frank-B20596; Garg Nitin-B37173 > > Subject: Re: [PATCH 02/15][v2] imx: usb: ehci-mx7 add usb driver for > > i.MX7D > > > > On Tuesday, July 21, 2015 at 02:17:06 AM, Adrian Alonso wrote: > > > * Add support for usb driver for i.MX7D SoC > > > > > > Signed-off-by: Adrian Alonso <aalonso@freescale.com> > > > Signed-off-by: Ye.Li <B37916@freescale.com> > > > Signed-off-by: Peng Fan <Peng.Fan@freescale.com> > > > > Hi! > > > > Do we really need yet-another-driver for i.MX ? > > [Adrian] USB support for iMX could be re-implemented following DM model, > At the moment ehci-mx7 handles the SoC specific settings for USB support. But do we really need yet another driver for i.MX ? We already have one for MX23/28, MX5, MX6 and one for Vybrid . > > > --- > > > Changes for V2: Resend > > > > > > drivers/usb/host/Makefile | 1 + > > > drivers/usb/host/ehci-mx7.c | 103 > > > > > > ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, > > > > 104 > > > > > insertions(+) > > > > > > create mode 100644 drivers/usb/host/ehci-mx7.c > > > > > > diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile > > > index 4d35d3e..7267160 100644 > > > --- a/drivers/usb/host/Makefile > > > +++ b/drivers/usb/host/Makefile > > > @@ -36,6 +36,7 @@ obj-$(CONFIG_USB_EHCI_MXC) += ehci-mxc.o > > > > > > obj-$(CONFIG_USB_EHCI_MXS) += ehci-mxs.o > > > obj-$(CONFIG_USB_EHCI_MX5) += ehci-mx5.o > > > obj-$(CONFIG_USB_EHCI_MX6) += ehci-mx6.o > > > > > > +obj-$(CONFIG_USB_EHCI_MX7) += ehci-mx7.o > > > > > > obj-$(CONFIG_USB_EHCI_OMAP) += ehci-omap.o > > > obj-$(CONFIG_USB_EHCI_PPC4XX) += ehci-ppc4xx.o > > > obj-$(CONFIG_USB_EHCI_MARVELL) += ehci-marvell.o diff --git > > > > > > a/drivers/usb/host/ehci-mx7.c b/drivers/usb/host/ehci-mx7.c new file > > > mode 100644 index 0000000..7429d62 > > > --- /dev/null > > > +++ b/drivers/usb/host/ehci-mx7.c > > > @@ -0,0 +1,103 @@ > > > +/* > > > + * Copyright (c) 2009 Daniel Mack <daniel@caiaq.de> > > > + * Copyright (C) 2010-2015 Freescale Semiconductor, Inc. > > > + * > > > + * SPDX-License-Identifier: GPL-2.0+ > > > + */ > > > + > > > +#include <common.h> > > > +#include <usb.h> > > > +#include <errno.h> > > > +#include <linux/compiler.h> > > > +#include <usb/ehci-fsl.h> > > > +#include <asm/io.h> > > > +#include <asm/arch/crm_regs.h> > > > +#include <asm/arch/clock.h> > > > +#include <asm/imx-common/iomux-v3.h> > > > + > > > +#include "ehci.h" > > > + > > > +#define USB_NC_OFFSET 0x200 > > > + > > > +#define UCTRL_PM (1 << 9) /* OTG Power Mask */ > > > +#define UCTRL_OVER_CUR_POL (1 << 8) /* OTG Polarity of Overcurrent > > > > */ > > > > > +#define UCTRL_OVER_CUR_DIS (1 << 7) /* Disable OTG Overcurrent > > > > Detection > > > > > */ + > > > > Are these bits EHCI spiecific or MX7 Chipidea controller specific ? > > [Adrian] They are iMX7 specific OK [...] > > > +} > > > + > > > +int __weak board_ehci_hcd_init(int port) { > > > + return 0; > > > +} > > > + > > > +int __weak board_ehci_power(int port, int on) { > > > + return 0; > > > +} > > > > Can you add kerneldoc style comments to the functions please ? > > [Adrian] All change suggestion to be addressed in a new patch version. Thanks!
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile index 4d35d3e..7267160 100644 --- a/drivers/usb/host/Makefile +++ b/drivers/usb/host/Makefile @@ -36,6 +36,7 @@ obj-$(CONFIG_USB_EHCI_MXC) += ehci-mxc.o obj-$(CONFIG_USB_EHCI_MXS) += ehci-mxs.o obj-$(CONFIG_USB_EHCI_MX5) += ehci-mx5.o obj-$(CONFIG_USB_EHCI_MX6) += ehci-mx6.o +obj-$(CONFIG_USB_EHCI_MX7) += ehci-mx7.o obj-$(CONFIG_USB_EHCI_OMAP) += ehci-omap.o obj-$(CONFIG_USB_EHCI_PPC4XX) += ehci-ppc4xx.o obj-$(CONFIG_USB_EHCI_MARVELL) += ehci-marvell.o diff --git a/drivers/usb/host/ehci-mx7.c b/drivers/usb/host/ehci-mx7.c new file mode 100644 index 0000000..7429d62 --- /dev/null +++ b/drivers/usb/host/ehci-mx7.c @@ -0,0 +1,103 @@ +/* + * Copyright (c) 2009 Daniel Mack <daniel@caiaq.de> + * Copyright (C) 2010-2015 Freescale Semiconductor, Inc. + * + * SPDX-License-Identifier: GPL-2.0+ + */ + +#include <common.h> +#include <usb.h> +#include <errno.h> +#include <linux/compiler.h> +#include <usb/ehci-fsl.h> +#include <asm/io.h> +#include <asm/arch/crm_regs.h> +#include <asm/arch/clock.h> +#include <asm/imx-common/iomux-v3.h> + +#include "ehci.h" + +#define USB_NC_OFFSET 0x200 + +#define UCTRL_PM (1 << 9) /* OTG Power Mask */ +#define UCTRL_OVER_CUR_POL (1 << 8) /* OTG Polarity of Overcurrent */ +#define UCTRL_OVER_CUR_DIS (1 << 7) /* Disable OTG Overcurrent Detection */ + +/* USBCMD */ +#define UCMD_RUN_STOP (1 << 0) /* controller run/stop */ +#define UCMD_RESET (1 << 1) /* controller reset */ + +/* Base address for this IP block is 0x02184800 */ +struct usbnc_regs { + u32 ctrl1; + u32 ctrl2; + u32 reserve1[11]; + u32 phy_ctrl2; + u32 reserve2[6]; + u32 adp_cfg1; + u32 reserve3; + u32 adp_status; +}; + +static void usb_oc_config(int index) +{ + struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR + + (0x10000 * index) + USB_NC_OFFSET); + void __iomem *ctrl = (void __iomem *)(&usbnc->ctrl1); + u32 val; + + val = __raw_readl(ctrl); + val |= UCTRL_OVER_CUR_POL; + __raw_writel(val, ctrl); + + val = __raw_readl(ctrl); + val |= (UCTRL_OVER_CUR_DIS | UCTRL_PM); + __raw_writel(val, ctrl); +} + +int __weak board_ehci_hcd_init(int port) +{ + return 0; +} + +int __weak board_ehci_power(int port, int on) +{ + return 0; +} + +int ehci_hcd_init(int index, enum usb_init_type init, + struct ehci_hccr **hccr, struct ehci_hcor **hcor) +{ + struct usb_ehci *ehci = (struct usb_ehci *)(USB_BASE_ADDR + + (0x10000 * index)); + + if (index > 3) + return -EINVAL; + enable_usboh3_clk(1); + mdelay(1); + + /* Do board specific initialization */ + board_ehci_hcd_init(index); + + usb_oc_config(index); + + *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); + *hcor = (struct ehci_hcor *)((uint32_t)*hccr + + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); + + board_ehci_power(index, (init == USB_INIT_DEVICE) ? 0 : 1); + if (init == USB_INIT_DEVICE) + return 0; + setbits_le32(&ehci->usbmode, CM_HOST); + __raw_writel(CONFIG_MXC_USB_PORTSC, &ehci->portsc); + setbits_le32(&ehci->portsc, USB_EN); + + mdelay(10); + + return 0; +} + +int ehci_hcd_stop(int index) +{ + return 0; +}