| Message ID | 20200629021350.21262-2-peng.fan@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | [1/7] usb: ehci-mx6: Add powerup_fixup implementation | expand |
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. > --- 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 > /* 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. > + 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. [...] > 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. [...]
> 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. > > [...]
On 6/29/20 10:24 AM, Peng Fan wrote: [...] >>> 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. I understand that, but the 64bit fixes can be pulled into separate patch to make it easier to review just the OTG support without having the fixes mixed in the same patch. [...] >>> 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]" [...]
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 1c374a7bd8..f48547caa0 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -139,8 +139,8 @@ config USB_EHCI_MX5 Enables support for the on-chip EHCI controller on i.MX5 SoCs. config USB_EHCI_MX6 - bool "Support for i.MX6/i.MX7ULP on-chip EHCI USB controller" - depends on ARCH_MX6 || ARCH_MX7ULP + bool "Support for i.MX6/i.MX7ULP/i.MX8 on-chip EHCI USB controller" + depends on ARCH_MX6 || ARCH_MX7ULP || ARCH_IMX8 default y ---help--- Enables support for the on-chip EHCI controller on i.MX6 SoCs. diff --git a/drivers/usb/host/ehci-mx6.c b/drivers/usb/host/ehci-mx6.c index e654595683..191d619220 100644 --- 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) + /* 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) + 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); + } + + if (timeout <= 0) { + /* If timeout, we power down the pll */ + writel(PLL_USB_PWR_MASK, + &usbphy->usb1_pll_480_ctrl_clr); + return; + } + } + + /* Clear the bypass */ + writel(PLL_USB_BYPASS_MASK, &usbphy->usb1_pll_480_ctrl_clr); + + /* Enable the PLL clock out to USB */ + writel((PLL_USB_EN_USB_CLKS_MASK | PLL_USB_ENABLE_MASK), + &usbphy->usb1_pll_480_ctrl_set); + #else struct anatop_regs __iomem *anatop = (struct anatop_regs __iomem *)ANATOP_BASE_ADDR; @@ -212,6 +267,20 @@ struct usbnc_regs { u32 reserve0[2]; u32 hsic_ctrl; }; +#elif defined(CONFIG_IMX8) +struct usbnc_regs { + u32 ctrl1; + u32 ctrl2; + u32 reserve1[10]; + u32 phy_cfg1; + u32 phy_cfg2; + u32 reserve2; + u32 phy_status; + u32 reserve3[4]; + u32 adp_cfg1; + u32 adp_cfg2; + u32 adp_status; +}; #else /* Base address for this IP block is 0x02184800 */ struct usbnc_regs { @@ -240,7 +309,7 @@ struct usbnc_regs { 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 + (0x10000 * index) + USBNC_OFFSET); void __iomem *phy_cfg2 = (void __iomem *)(&usbnc->phy_cfg2); @@ -253,7 +322,7 @@ static void usb_power_config(int index) int usb_phy_mode(int port) { - struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR + + struct usbnc_regs *usbnc = (struct usbnc_regs *)(ulong)(USB_BASE_ADDR + (0x10000 * port) + USBNC_OFFSET); void __iomem *status = (void __iomem *)(&usbnc->phy_status); u32 val; @@ -292,8 +361,8 @@ static void usb_oc_config(int index) struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR + USB_OTHERREGS_OFFSET); void __iomem *ctrl = (void __iomem *)(&usbnc->ctrl[index]); -#elif defined(CONFIG_MX7) || defined(CONFIG_MX7ULP) - struct usbnc_regs *usbnc = (struct usbnc_regs *)(USB_BASE_ADDR + +#elif defined(CONFIG_MX7) || defined(CONFIG_MX7ULP) || defined(CONFIG_IMX8) + struct usbnc_regs *usbnc = (struct usbnc_regs *)(ulong)(USB_BASE_ADDR + (0x10000 * index) + USBNC_OFFSET); void __iomem *ctrl = (void __iomem *)(&usbnc->ctrl1); #endif @@ -376,7 +445,7 @@ int ehci_mx6_common_init(struct usb_ehci *ehci, int index) usb_power_config(index); usb_oc_config(index); -#if defined(CONFIG_MX6) || defined(CONFIG_MX7ULP) +#if defined(CONFIG_MX6) || defined(CONFIG_MX7ULP) || defined(CONFIG_IMX8) usb_internal_phy_clock_gate(index, 1); usb_phy_enable(index, ehci); #endif @@ -395,10 +464,10 @@ int ehci_hcd_init(int index, enum usb_init_type init, enum usb_init_type type; #if defined(CONFIG_MX6) u32 controller_spacing = 0x200; -#elif defined(CONFIG_MX7) || defined(CONFIG_MX7ULP) +#elif defined(CONFIG_MX7) || defined(CONFIG_MX7ULP) || defined(CONFIG_IMX8) u32 controller_spacing = 0x10000; #endif - struct usb_ehci *ehci = (struct usb_ehci *)(USB_BASE_ADDR + + struct usb_ehci *ehci = (struct usb_ehci *)(ulong)(USB_BASE_ADDR + (controller_spacing * index)); int ret; @@ -422,8 +491,8 @@ int ehci_hcd_init(int index, enum usb_init_type init, type = board_usb_phy_mode(index); if (hccr && hcor) { - *hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); - *hcor = (struct ehci_hcor *)((uint32_t)*hccr + + *hccr = (struct ehci_hccr *)((ulong)&ehci->caplength); + *hcor = (struct ehci_hcor *)((ulong)*hccr + HC_LENGTH(ehci_readl(&(*hccr)->cr_capbase))); } @@ -509,7 +578,7 @@ static int ehci_usb_phy_mode(struct udevice *dev) * About fsl,usbphy, Refer to * Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt. */ - if (is_mx6() || is_mx7ulp()) { + if (is_mx6() || is_mx7ulp() || is_imx8()) { phy_off = fdtdec_lookup_phandle(blob, offset, "fsl,usbphy"); @@ -655,8 +724,8 @@ static int ehci_usb_probe(struct udevice *dev) mdelay(10); - hccr = (struct ehci_hccr *)((uint32_t)&ehci->caplength); - hcor = (struct ehci_hcor *)((uint32_t)hccr + + hccr = (struct ehci_hccr *)((ulong)&ehci->caplength); + hcor = (struct ehci_hcor *)((ulong)hccr + HC_LENGTH(ehci_readl(&(hccr)->cr_capbase))); return ehci_register(dev, hccr, hcor, &mx6_ehci_ops, 0, priv->init_type);