[PATCH 2/7] usb: ehci-mx6: Add i.MX8 OTG controller support

[Peng Fan]

> Subject: Re: [PATCH 2/7] usb: ehci-mx6: Add i.MX8 OTG controller support
> 
> On 6/29/20 4:13 AM, Peng Fan wrote:
> 
> Hi,
> 
> > The i.MX8 has two USB controllers: USBOH and USB3. The USBOH reuses
> > previous i.MX6/7. It has same PHY IP as i.MX7ULP but NC registers are
> > same as i.MX7D. So add its support in ehci-mx6 driver.
> >
> > Also the driver is updated to remove build warning for 64 bits CPU.
> 
> Please split the fixes into separate patch.

Sorry for not be clear, but the driver was only built for ARM32 i.MX.
It is after we start supporting i.MX8, there are some warnings, which
was introduced by adding i.MX8 support. It should stay in same patch.

> 
> > --- a/drivers/usb/host/ehci-mx6.c
> > +++ b/drivers/usb/host/ehci-mx6.c
> > @@ -62,12 +62,20 @@ DECLARE_GLOBAL_DATA_PTR;
> >  #define UCTRL_OVER_CUR_POL	(1 << 8) /* OTG Polarity of Overcurrent
> */
> >  #define UCTRL_OVER_CUR_DIS	(1 << 7) /* Disable OTG Overcurrent
> Detection */
> >
> > +#define PLL_USB_EN_USB_CLKS_MASK	(0x01 << 6)
> > +#define PLL_USB_PWR_MASK		(0x01 << 12)
> > +#define PLL_USB_ENABLE_MASK		(0x01 << 13)
> > +#define PLL_USB_BYPASS_MASK		(0x01 << 16)
> > +#define PLL_USB_REG_ENABLE_MASK		(0x01 << 21)
> > +#define PLL_USB_DIV_SEL_MASK		(0x07 << 22)
> > +#define PLL_USB_LOCK_MASK		(0x01 << 31)
> 
> Use BIT() macro

Yes.

> 
> >  /* USBCMD */
> >  #define UCMD_RUN_STOP           (1 << 0) /* controller run/stop */
> >  #define UCMD_RESET		(1 << 1) /* controller reset */
> >
> > -#if defined(CONFIG_MX6) || defined(CONFIG_MX7ULP) -static const
> > unsigned phy_bases[] = {
> > +#if defined(CONFIG_MX6) || defined(CONFIG_MX7ULP) ||
> > +defined(CONFIG_IMX8) static const ulong phy_bases[] = {
> >  	USB_PHY0_BASE_ADDR,
> >  #if defined(USB_PHY1_BASE_ADDR)
> >  	USB_PHY1_BASE_ADDR,
> > @@ -101,6 +109,53 @@ static void usb_power_config(int index)
> >
> >  	scg_enable_usb_pll(true);
> >
> > +#elif defined(CONFIG_IMX8)
> 
> This function should be split into multiple, it's too long, make it a per-SoC
> function.

Ok.

> 
> > +	struct usbphy_regs __iomem *usbphy =
> > +		(struct usbphy_regs __iomem *)USB_PHY0_BASE_ADDR;
> > +	int timeout = 1000000;
> > +
> > +	if (index > 0)
> > +		return;
> > +
> > +	writel(ANADIG_USB2_CHRG_DETECT_EN_B |
> > +		   ANADIG_USB2_CHRG_DETECT_CHK_CHRG_B,
> > +		   &usbphy->usb1_chrg_detect);
> > +
> > +	if (!(readl(&usbphy->usb1_pll_480_ctrl) & PLL_USB_LOCK_MASK)) {
> > +
> > +		/* Enable the regulator first */
> > +		writel(PLL_USB_REG_ENABLE_MASK,
> > +		       &usbphy->usb1_pll_480_ctrl_set);
> > +
> > +		/* Wait at least 25us */
> > +		udelay(25);
> > +
> > +		/* Enable the power */
> > +		writel(PLL_USB_PWR_MASK, &usbphy->usb1_pll_480_ctrl_set);
> > +
> > +		/* Wait lock */
> > +		while (timeout--) {
> > +			if (readl(&usbphy->usb1_pll_480_ctrl) &
> > +			    PLL_USB_LOCK_MASK)
> > +				break;
> > +			udelay(10);
> > +		}
> 
> Use wait_for_bit*() here.

ok.

> 
> [...]
> 
> >  static void usb_power_config(int index)  {
> > -	struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR +
> > +	struct usbnc_regs *usbnc = (struct usbnc_regs
> > +*)(ulong)(USB_BASE_ADDR +
> 
> Is the extra type cast really needed ? If so, then it should be uintptr_t so it
> would work with 64bit addresses.

Will use uintptr_t. there is warning if not.

"warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]"

Thanks,
Peng.
>  
> [...]