[U-Boot] [PATCH 02/15][v2] imx: usb: ehci-mx7 add usb driver for i.MX7D

[Alonso Adrian]

Hi Marek,

Comments inline.

> -----Original Message-----
> From: Marek Vasut [mailto:marex at denx.de]
> Sent: Monday, July 20, 2015 8:56 PM
> To: Alonso Lazcano Adrian-B38018
> Cc: u-boot at lists.denx.de; sbabic at denx.de; otavio at 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 at freescale.com>
> > Signed-off-by: Ye.Li <B37916 at freescale.com>
> > Signed-off-by: Peng Fan <Peng.Fan at 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 at 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