Skip to contentSkip to conversation

Add driver for ehci in Sunplus SP7021

PATCH Superseded Vincent Shih 3 participants 11 messages Feb 14, 2022 – Feb 22, 2022

Subsystems (3)
Device tree bindings USB USB ehci driver

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Vincent Shih

author
#0 lore raw

cover Add driver for ehci in Sunplus SP7021

This is a patch series for ehci driver for Sunplus SP7021 SoC.

Sunplus SP7021 is an ARM Coretex A7 (4 cores) based SoC. It integrates many peripherals (ex: UART, I2C, SPI, SDIO, eMMC, USB, SD Card and etc.) into a single chip. It is designed for industrial control.

Refer to:
https://sunplus-tibbo.atlassian.net/wiki/spaces/doc/overview
https://tibbo.com/store/plus1.html

Vincent Shih (2):
usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021
dt-bindings: usb: Add bindings doc for Sunplus EHCI driver

 .../bindings/usb/sunplus,sp7021-usb-ehci.yaml      |  69 +++
 MAINTAINERS                                        |   7 +
 drivers/usb/host/Kconfig                           |  11 +
 drivers/usb/host/Makefile                          |   1 +
 drivers/usb/host/ehci-sunplus.c                    | 527 +++++++++++++++++++++
 5 files changed, 615 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml
 create mode 100644 drivers/usb/host/ehci-sunplus.c
Signature · 1 line
2.7.4
author
#1 lore raw

PATCH 1/2 usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021

Add driver for ehci in Sunplus SP7021

Signed-off-by Vincent Shih <[email protected]>
---
 MAINTAINERS                     |   6 +
 drivers/usb/host/Kconfig        |  11 +
 drivers/usb/host/Makefile       |   1 +
 drivers/usb/host/ehci-sunplus.c | 527 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 545 insertions(+)
 create mode 100644 drivers/usb/host/ehci-sunplus.c
MAINTAINERS +6 -0
index 80eebc1..6c2faf3 100644
@@ -17947,6 +17947,12 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS USB EHCI DRIVER
+M:	Vincent Shih <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/usb/host/ehci-sunplus.c
+
 SUPERH
 M:	Yoshinori Sato <[email protected]>
 M:	Rich Felker <[email protected]>
drivers/usb/host/Kconfig +11 -0
index c4736d1..9728d3b 100644
@@ -339,6 +339,17 @@ config USB_OCTEON_EHCI
 	  USB 2.0 device support.  All CN6XXX based chips with USB are
 	  supported.
 
+config USB_EHCI_SUNPLUS
+	tristate "EHCI support for Sunplus USB controller"
+	depends on SOC_SP7021
+	select NVMEM_SUNPLUS_OCOTP
+	help
+	  Enables support for the on-chip EHCI controller in Sunplus
+	  SoCs. It supports 32-bit AHB/AXI config bus and 64-bit AXI
+	  data bus.
+	  This driver can also be built as a module. If so, the module
+	  will be called ehci-sunplus.
+
 endif # USB_EHCI_HCD
 
 config USB_OXU210HP_HCD
drivers/usb/host/Makefile +1 -0
index 171de4d..7e9b5f1 100644
@@ -46,6 +46,7 @@ obj-$(CONFIG_USB_EHCI_HCD_STI)	+= ehci-st.o
 obj-$(CONFIG_USB_EHCI_EXYNOS)	+= ehci-exynos.o
 obj-$(CONFIG_USB_EHCI_HCD_AT91) += ehci-atmel.o
 obj-$(CONFIG_USB_EHCI_BRCMSTB)	+= ehci-brcm.o
+obj-$(CONFIG_USB_EHCI_SUNPLUS)	+= ehci-sunplus.o
 
 obj-$(CONFIG_USB_OXU210HP_HCD)	+= oxu210hp-hcd.o
 obj-$(CONFIG_USB_ISP116X_HCD)	+= isp116x-hcd.o
drivers/usb/host/ehci-sunplus.c new file +527 -0

527 lines not on this page · raw message

Signature · 1 line
2.7.4
maintainer
#2 lore raw

Re: [PATCH v1 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021

Quoted 68 lines · Vincent Shih

Add driver for ehci in Sunplus SP7021

Signed-off-by Vincent Shih <[email protected]>
---
 MAINTAINERS                     |   6 +
 drivers/usb/host/Kconfig        |  11 +
 drivers/usb/host/Makefile       |   1 +
 drivers/usb/host/ehci-sunplus.c | 527 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 545 insertions(+)
 create mode 100644 drivers/usb/host/ehci-sunplus.c
MAINTAINERS +6 -0
index 80eebc1..6c2faf3 100644
@@ -17947,6 +17947,12 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS USB EHCI DRIVER
+M:	Vincent Shih <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/usb/host/ehci-sunplus.c
+
 SUPERH
 M:	Yoshinori Sato <[email protected]>
 M:	Rich Felker <[email protected]>
drivers/usb/host/Kconfig +11 -0
index c4736d1..9728d3b 100644
@@ -339,6 +339,17 @@ config USB_OCTEON_EHCI
 	  USB 2.0 device support.  All CN6XXX based chips with USB are
 	  supported.
 
+config USB_EHCI_SUNPLUS
+	tristate "EHCI support for Sunplus USB controller"
+	depends on SOC_SP7021
+	select NVMEM_SUNPLUS_OCOTP
+	help
+	  Enables support for the on-chip EHCI controller in Sunplus
+	  SoCs. It supports 32-bit AHB/AXI config bus and 64-bit AXI
+	  data bus.
+	  This driver can also be built as a module. If so, the module
+	  will be called ehci-sunplus.
+
 endif # USB_EHCI_HCD
 
 config USB_OXU210HP_HCD
drivers/usb/host/Makefile +1 -0
index 171de4d..7e9b5f1 100644
@@ -46,6 +46,7 @@ obj-$(CONFIG_USB_EHCI_HCD_STI)	+= ehci-st.o
 obj-$(CONFIG_USB_EHCI_EXYNOS)	+= ehci-exynos.o
 obj-$(CONFIG_USB_EHCI_HCD_AT91) += ehci-atmel.o
 obj-$(CONFIG_USB_EHCI_BRCMSTB)	+= ehci-brcm.o
+obj-$(CONFIG_USB_EHCI_SUNPLUS)	+= ehci-sunplus.o
 
 obj-$(CONFIG_USB_OXU210HP_HCD)	+= oxu210hp-hcd.o
 obj-$(CONFIG_USB_ISP116X_HCD)	+= isp116x-hcd.o
drivers/usb/host/ehci-sunplus.c new file +0 -0
new file mode 100644
index 0000000..007923a

drivers/usb/host/ehci-sunplus.c line 6

Quoted 6 lines
drivers/usb/host/ehci-sunplus.c new file +6 -0
@@ -0,0 +1,527 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * The EHCI driver for Sunplus SP7021
+ *
+ * Copyright (C) 2019 Sunplus Technology Inc., All rights reserved.

You have not touched this file since 2019?

Quoted 142 lines
drivers/usb/host/ehci-sunplus.c new file +142 -0
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/usb.h>
+#include <linux/usb/ehci_pdriver.h>
+#include <linux/usb/hcd.h>
+
+#include "ehci.h"
+
+#define RF_MASK_V(mask, val)			(((mask) << 16) | (val))
+#define RF_MASK_V_CLR(mask)			(((mask) << 16) | 0)
+
+#define USB_PORT0_ID				0
+#define USB_PORT1_ID				1
+#define USB_PORT_NUM				2
+
+#define MASK_BITS				0xffff
+
+#define OTP_DISC_LEVEL_DEFAULT			0xd
+#define OTP_DISC_LEVEL_BITS			0x5
+
+// GROUP 140/150 UPHY0/UPHY1
+#define CONFIG1					0x4
+#define J_HS_TX_PWRSAV				BIT(5)
+#define CONFIG3					0xc
+#define J_FORCE_DISC_ON				BIT(5)
+#define J_DEBUG_CTRL_ADDR_MACRO			BIT(0)
+#define CONFIG7					0x1c
+#define J_DISC					0X1f
+#define CONFIG9					0x24
+#define J_ECO_PATH				BIT(6)
+#define CONFIG16				0x40
+#define J_TBCWAIT_MASK				GENMASK(6, 5)
+#define J_TBCWAIT_1P1_MS			FIELD_PREP(J_TBCWAIT_MASK, 0)
+#define J_TVDM_SRC_DIS_MASK			GENMASK(4, 3)
+#define J_TVDM_SRC_DIS_8P2_MS			FIELD_PREP(J_TVDM_SRC_DIS_MASK, 3)
+#define J_TVDM_SRC_EN_MASK			GENMASK(2, 1)
+#define J_TVDM_SRC_EN_1P6_MS			FIELD_PREP(J_TVDM_SRC_EN_MASK, 0)
+#define J_BC_EN					BIT(0)
+#define CONFIG17				0x44
+#define IBG_TRIM0_MASK				GENMASK(7, 5)
+#define IBG_TRIM0_SSLVHT			FIELD_PREP(IBG_TRIM0_MASK, 4)
+#define J_VDATREE_TRIM_MASK			GENMASK(4, 1)
+#define J_VDATREE_TRIM_DEFAULT			FIELD_PREP(J_VDATREE_TRIM_MASK, 9)
+#define CONFIG23				0x5c
+#define PROB_MASK				GENMASK(5, 3)
+#define PROB					FIELD_PREP(PROB_MASK, 7)
+
+// GROUP 0 MOON 0
+#define HARDWARE_RESET_CONTROL2			0x5c
+#define UPHY1_RESET				BIT(14)
+#define UPHY0_RESET				BIT(13)
+#define USBC1_RESET				BIT(11)
+#define USBC0_RESET				BIT(10)
+
+// GROUP 4 MOON 4
+#define USBC_CONTROL				0x44
+#define MO1_USBC1_USB0_SEL			BIT(13)
+#define MO1_USBC1_USB0_CTRL			BIT(12)
+#define MO1_USBC0_USB0_SEL			BIT(5)
+#define MO1_USBC0_USB0_CTRL			BIT(4)
+#define UPHY0_CONTROL0				0x48
+#define UPHY0_CONTROL1				0x4c
+#define UPHY0_CONTROL2				0x50
+#define MO1_UPHY0_RX_CLK_SEL			BIT(6)
+#define UPHY0_CONTROL3				0x54
+#define MO1_UPHY0_PLL_POWER_OFF_SEL		BIT(7)
+#define MO1_UPHY0_PLL_POWER_OFF			BIT(3)
+#define UPHY1_CONTROL0				0x58
+#define UPHY1_CONTROL1				0x5c
+#define UPHY1_CONTROL2				0x60
+#define MO1_UPHY1_RX_CLK_SEL			BIT(6)
+#define UPHY1_CONTROL3				0x64
+#define MO1_UPHY1_PLL_POWER_OFF_SEL		BIT(7)
+#define MO1_UPHY1_PLL_POWER_OFF			BIT(3)
+
+#define hcd_to_sp_ehci_priv(h) \
+		((struct sp_ehci_priv *)hcd_to_ehci(h)->priv)
+
+struct sp_ehci_priv {
+	struct resource *uphy_res_mem[USB_PORT_NUM];
+	struct resource *moon0_res_mem;
+	struct resource *moon4_res_mem;
+	struct clk *uphy_clk[USB_PORT_NUM];
+	struct clk *ehci_clk[USB_PORT_NUM];
+	void __iomem *uphy_regs[USB_PORT_NUM];
+	void __iomem *moon0_regs;
+	void __iomem *moon4_regs;
+};
+
+static int ehci_sunplus_reset(struct usb_hcd *hcd)
+{
+	struct platform_device *pdev = to_platform_device(hcd->self.controller);
+	struct usb_ehci_pdata *pdata = pdev->dev.platform_data;
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+	int retval;
+
+	hcd->has_tt = pdata->has_tt;
+	ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug;
+	ehci->big_endian_desc = pdata->big_endian_desc;
+	ehci->big_endian_mmio = pdata->big_endian_mmio;
+
+	ehci->caps = hcd->regs + pdata->caps_offset;
+	retval = ehci_setup(hcd);
+
+	return retval;
+}
+
+static struct hc_driver __read_mostly ehci_sunplus_driver;
+
+static const struct ehci_driver_overrides ehci_sunplus_overrides __initconst = {
+	.reset =		ehci_sunplus_reset,
+	.extra_priv_size =	sizeof(struct sp_ehci_priv),
+};
+
+static int uphy0_init(struct platform_device *pdev, struct sp_ehci_priv *sp_priv)
+{
+	struct nvmem_cell *cell;
+	char *disc_name = "disc_vol";
+	ssize_t otp_l = 0;
+	char *otp_v;
+	int port = pdev->id - 1;
+	u32 val, set, pll_pwr_on, pll_pwr_off;
+
+	/* Default value modification */
+	writel(RF_MASK_V(MASK_BITS, 0x4002), sp_priv->moon4_regs + UPHY0_CONTROL0);
+	writel(RF_MASK_V(MASK_BITS, 0x8747), sp_priv->moon4_regs + UPHY0_CONTROL1);
+
+	/* PLL power off/on twice */
+	pll_pwr_off = (readl(sp_priv->moon4_regs + UPHY0_CONTROL3) & ~MASK_BITS)
+			| MO1_UPHY0_PLL_POWER_OFF_SEL | MO1_UPHY0_PLL_POWER_OFF;
+	pll_pwr_on = (readl(sp_priv->moon4_regs + UPHY0_CONTROL3) & ~MASK_BITS)
+			| MO1_UPHY0_PLL_POWER_OFF_SEL;
+

drivers/usb/host/ehci-sunplus.c line 158

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY0_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY0_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY0_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY0_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, 0x0), sp_priv->moon4_regs + UPHY0_CONTROL3);
+

Why not just do a read after a write so that you know that is the correct delay? Doing a sleep like this seems slow and possibly wrong.

drivers/usb/host/ehci-sunplus.c line 162

Quoted 4 lines
drivers/usb/host/ehci-sunplus.c new file +4 -0
+	/* reset UPHY0 */
+	writel(RF_MASK_V(UPHY0_RESET, UPHY0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+	writel(RF_MASK_V_CLR(UPHY0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+	mdelay(1);

Again, read()?

Quoted 42 lines
drivers/usb/host/ehci-sunplus.c new file +42 -0
+
+	/* board uphy 0 internal register modification for tid certification */
+	cell = nvmem_cell_get(&pdev->dev, disc_name);
+	if (IS_ERR_OR_NULL(cell)) {
+		if (PTR_ERR(cell) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+	}
+
+	otp_v = nvmem_cell_read(cell, &otp_l);
+	nvmem_cell_put(cell);
+
+	if (otp_v)
+		set = ((*otp_v >> OTP_DISC_LEVEL_BITS) |
+			(*(otp_v + 1) << (sizeof(char) * 8 - OTP_DISC_LEVEL_BITS))) & J_DISC;
+
+	if (!otp_v || set == 0)
+		set = OTP_DISC_LEVEL_DEFAULT;
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG7);
+	val = (val & ~J_DISC) | set;
+	writel(val, sp_priv->uphy_regs[port] + CONFIG7);
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG9);
+	val &= ~(J_ECO_PATH);
+	writel(val, sp_priv->uphy_regs[port] + CONFIG9);
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG1);
+	val &= ~(J_HS_TX_PWRSAV);
+	writel(val, sp_priv->uphy_regs[port] + CONFIG1);
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG23);
+	val = (val & ~PROB) | PROB;
+	writel(val, sp_priv->uphy_regs[port] + CONFIG23);
+
+	/* USBC 0 reset */
+	writel(RF_MASK_V(USBC0_RESET, USBC0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+	writel(RF_MASK_V_CLR(USBC0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+
+	/* port 0 uphy clk fix */
+	writel(RF_MASK_V(MO1_UPHY0_RX_CLK_SEL, MO1_UPHY0_RX_CLK_SEL),
+	       sp_priv->moon4_regs + UPHY0_CONTROL2);
+

drivers/usb/host/ehci-sunplus.c line 214

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+	/* switch to host */
+	writel(RF_MASK_V(MO1_USBC0_USB0_SEL | MO1_USBC0_USB0_CTRL,
+			 MO1_USBC0_USB0_SEL | MO1_USBC0_USB0_CTRL),
+	       sp_priv->moon4_regs + USBC_CONTROL);
+
+	/* battery charger */
+	writel(J_TBCWAIT_1P1_MS | J_TVDM_SRC_DIS_8P2_MS | J_TVDM_SRC_EN_1P6_MS | J_BC_EN,
+	       sp_priv->uphy_regs[port] + CONFIG16);
+	writel(IBG_TRIM0_SSLVHT | J_VDATREE_TRIM_DEFAULT, sp_priv->uphy_regs[port] + CONFIG17);
+	writel(J_FORCE_DISC_ON | J_DEBUG_CTRL_ADDR_MACRO, sp_priv->uphy_regs[port] + CONFIG3);

What about all of these writes, are you sure they made it to the hardware ok? No need to read?

Same for elsewhere in the file.

thanks,

greg k-h

author
#3 lore raw

Re: [PATCH v1 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021

Hi, Greg
About this issue, my colleague Hammer Hsieh has explained it to
you recently in the mail of "[PATCH v7 2/2] serial: sunplus-uart: Add Sunplus
SoC UART Driver". The ehci driver and uart one are in the same Sunplus Soc.
Thanks for your review.

Greg KH <[email protected]> 於 2022年2月14日 週一 下午4:38寫道：

Quoted 304 lines · Vincent Shih
Quoted 68 lines

Add driver for ehci in Sunplus SP7021

Signed-off-by Vincent Shih <[email protected]>
---
 MAINTAINERS                     |   6 +
 drivers/usb/host/Kconfig        |  11 +
 drivers/usb/host/Makefile       |   1 +
 drivers/usb/host/ehci-sunplus.c | 527 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 545 insertions(+)
 create mode 100644 drivers/usb/host/ehci-sunplus.c
MAINTAINERS +6 -0
index 80eebc1..6c2faf3 100644
@@ -17947,6 +17947,12 @@ L:   [email protected]
 S:   Maintained
 F:   drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS USB EHCI DRIVER
+M:   Vincent Shih <[email protected]>
+L:   [email protected]
+S:   Maintained
+F:   drivers/usb/host/ehci-sunplus.c
+
 SUPERH
 M:   Yoshinori Sato <[email protected]>
 M:   Rich Felker <[email protected]>
drivers/usb/host/Kconfig +11 -0
index c4736d1..9728d3b 100644
@@ -339,6 +339,17 @@ config USB_OCTEON_EHCI
        USB 2.0 device support.  All CN6XXX based chips with USB are
        supported.
 
+config USB_EHCI_SUNPLUS
+     tristate "EHCI support for Sunplus USB controller"
+     depends on SOC_SP7021
+     select NVMEM_SUNPLUS_OCOTP
+     help
+       Enables support for the on-chip EHCI controller in Sunplus
+       SoCs. It supports 32-bit AHB/AXI config bus and 64-bit AXI
+       data bus.
+       This driver can also be built as a module. If so, the module
+       will be called ehci-sunplus.
+
 endif # USB_EHCI_HCD
 
 config USB_OXU210HP_HCD
drivers/usb/host/Makefile +1 -0
index 171de4d..7e9b5f1 100644
@@ -46,6 +46,7 @@ obj-$(CONFIG_USB_EHCI_HCD_STI)      += ehci-st.o
 obj-$(CONFIG_USB_EHCI_EXYNOS)        += ehci-exynos.o
 obj-$(CONFIG_USB_EHCI_HCD_AT91) += ehci-atmel.o
 obj-$(CONFIG_USB_EHCI_BRCMSTB)       += ehci-brcm.o
+obj-$(CONFIG_USB_EHCI_SUNPLUS)       += ehci-sunplus.o
 
 obj-$(CONFIG_USB_OXU210HP_HCD)       += oxu210hp-hcd.o
 obj-$(CONFIG_USB_ISP116X_HCD)        += isp116x-hcd.o
drivers/usb/host/ehci-sunplus.c new file +0 -0
new file mode 100644
index 0000000..007923a

drivers/usb/host/ehci-sunplus.c line 6

Quoted 6 lines
drivers/usb/host/ehci-sunplus.c new file +6 -0
@@ -0,0 +1,527 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * The EHCI driver for Sunplus SP7021
+ *
+ * Copyright (C) 2019 Sunplus Technology Inc., All rights reserved.

You have not touched this file since 2019?

Quoted 142 lines
drivers/usb/host/ehci-sunplus.c new file +142 -0
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/usb.h>
+#include <linux/usb/ehci_pdriver.h>
+#include <linux/usb/hcd.h>
+
+#include "ehci.h"
+
+#define RF_MASK_V(mask, val)                 (((mask) << 16) | (val))
+#define RF_MASK_V_CLR(mask)                  (((mask) << 16) | 0)
+
+#define USB_PORT0_ID                         0
+#define USB_PORT1_ID                         1
+#define USB_PORT_NUM                         2
+
+#define MASK_BITS                            0xffff
+
+#define OTP_DISC_LEVEL_DEFAULT                       0xd
+#define OTP_DISC_LEVEL_BITS                  0x5
+
+// GROUP 140/150 UPHY0/UPHY1
+#define CONFIG1                                      0x4
+#define J_HS_TX_PWRSAV                               BIT(5)
+#define CONFIG3                                      0xc
+#define J_FORCE_DISC_ON                              BIT(5)
+#define J_DEBUG_CTRL_ADDR_MACRO                      BIT(0)
+#define CONFIG7                                      0x1c
+#define J_DISC                                       0X1f
+#define CONFIG9                                      0x24
+#define J_ECO_PATH                           BIT(6)
+#define CONFIG16                             0x40
+#define J_TBCWAIT_MASK                               GENMASK(6, 5)
+#define J_TBCWAIT_1P1_MS                     FIELD_PREP(J_TBCWAIT_MASK, 0)
+#define J_TVDM_SRC_DIS_MASK                  GENMASK(4, 3)
+#define J_TVDM_SRC_DIS_8P2_MS                        FIELD_PREP(J_TVDM_SRC_DIS_MASK, 3)
+#define J_TVDM_SRC_EN_MASK                   GENMASK(2, 1)
+#define J_TVDM_SRC_EN_1P6_MS                 FIELD_PREP(J_TVDM_SRC_EN_MASK, 0)
+#define J_BC_EN                                      BIT(0)
+#define CONFIG17                             0x44
+#define IBG_TRIM0_MASK                               GENMASK(7, 5)
+#define IBG_TRIM0_SSLVHT                     FIELD_PREP(IBG_TRIM0_MASK, 4)
+#define J_VDATREE_TRIM_MASK                  GENMASK(4, 1)
+#define J_VDATREE_TRIM_DEFAULT                       FIELD_PREP(J_VDATREE_TRIM_MASK, 9)
+#define CONFIG23                             0x5c
+#define PROB_MASK                            GENMASK(5, 3)
+#define PROB                                 FIELD_PREP(PROB_MASK, 7)
+
+// GROUP 0 MOON 0
+#define HARDWARE_RESET_CONTROL2                      0x5c
+#define UPHY1_RESET                          BIT(14)
+#define UPHY0_RESET                          BIT(13)
+#define USBC1_RESET                          BIT(11)
+#define USBC0_RESET                          BIT(10)
+
+// GROUP 4 MOON 4
+#define USBC_CONTROL                         0x44
+#define MO1_USBC1_USB0_SEL                   BIT(13)
+#define MO1_USBC1_USB0_CTRL                  BIT(12)
+#define MO1_USBC0_USB0_SEL                   BIT(5)
+#define MO1_USBC0_USB0_CTRL                  BIT(4)
+#define UPHY0_CONTROL0                               0x48
+#define UPHY0_CONTROL1                               0x4c
+#define UPHY0_CONTROL2                               0x50
+#define MO1_UPHY0_RX_CLK_SEL                 BIT(6)
+#define UPHY0_CONTROL3                               0x54
+#define MO1_UPHY0_PLL_POWER_OFF_SEL          BIT(7)
+#define MO1_UPHY0_PLL_POWER_OFF                      BIT(3)
+#define UPHY1_CONTROL0                               0x58
+#define UPHY1_CONTROL1                               0x5c
+#define UPHY1_CONTROL2                               0x60
+#define MO1_UPHY1_RX_CLK_SEL                 BIT(6)
+#define UPHY1_CONTROL3                               0x64
+#define MO1_UPHY1_PLL_POWER_OFF_SEL          BIT(7)
+#define MO1_UPHY1_PLL_POWER_OFF                      BIT(3)
+
+#define hcd_to_sp_ehci_priv(h) \
+             ((struct sp_ehci_priv *)hcd_to_ehci(h)->priv)
+
+struct sp_ehci_priv {
+     struct resource *uphy_res_mem[USB_PORT_NUM];
+     struct resource *moon0_res_mem;
+     struct resource *moon4_res_mem;
+     struct clk *uphy_clk[USB_PORT_NUM];
+     struct clk *ehci_clk[USB_PORT_NUM];
+     void __iomem *uphy_regs[USB_PORT_NUM];
+     void __iomem *moon0_regs;
+     void __iomem *moon4_regs;
+};
+
+static int ehci_sunplus_reset(struct usb_hcd *hcd)
+{
+     struct platform_device *pdev = to_platform_device(hcd->self.controller);
+     struct usb_ehci_pdata *pdata = pdev->dev.platform_data;
+     struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+     int retval;
+
+     hcd->has_tt = pdata->has_tt;
+     ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug;
+     ehci->big_endian_desc = pdata->big_endian_desc;
+     ehci->big_endian_mmio = pdata->big_endian_mmio;
+
+     ehci->caps = hcd->regs + pdata->caps_offset;
+     retval = ehci_setup(hcd);
+
+     return retval;
+}
+
+static struct hc_driver __read_mostly ehci_sunplus_driver;
+
+static const struct ehci_driver_overrides ehci_sunplus_overrides __initconst = {
+     .reset =                ehci_sunplus_reset,
+     .extra_priv_size =      sizeof(struct sp_ehci_priv),
+};
+
+static int uphy0_init(struct platform_device *pdev, struct sp_ehci_priv *sp_priv)
+{
+     struct nvmem_cell *cell;
+     char *disc_name = "disc_vol";
+     ssize_t otp_l = 0;
+     char *otp_v;
+     int port = pdev->id - 1;
+     u32 val, set, pll_pwr_on, pll_pwr_off;
+
+     /* Default value modification */
+     writel(RF_MASK_V(MASK_BITS, 0x4002), sp_priv->moon4_regs + UPHY0_CONTROL0);
+     writel(RF_MASK_V(MASK_BITS, 0x8747), sp_priv->moon4_regs + UPHY0_CONTROL1);
+
+     /* PLL power off/on twice */
+     pll_pwr_off = (readl(sp_priv->moon4_regs + UPHY0_CONTROL3) & ~MASK_BITS)
+                     | MO1_UPHY0_PLL_POWER_OFF_SEL | MO1_UPHY0_PLL_POWER_OFF;
+     pll_pwr_on = (readl(sp_priv->moon4_regs + UPHY0_CONTROL3) & ~MASK_BITS)
+                     | MO1_UPHY0_PLL_POWER_OFF_SEL;
+

drivers/usb/host/ehci-sunplus.c line 158

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+     writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY0_CONTROL3);
+     mdelay(1);
+     writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY0_CONTROL3);
+     mdelay(1);
+     writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY0_CONTROL3);
+     mdelay(1);
+     writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY0_CONTROL3);
+     mdelay(1);
+     writel(RF_MASK_V(MASK_BITS, 0x0), sp_priv->moon4_regs + UPHY0_CONTROL3);
+

Why not just do a read after a write so that you know that is the correct delay? Doing a sleep like this seems slow and possibly wrong.

drivers/usb/host/ehci-sunplus.c line 162

Quoted 4 lines
drivers/usb/host/ehci-sunplus.c new file +4 -0
+     /* reset UPHY0 */
+     writel(RF_MASK_V(UPHY0_RESET, UPHY0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+     writel(RF_MASK_V_CLR(UPHY0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+     mdelay(1);

Again, read()?

Quoted 42 lines
drivers/usb/host/ehci-sunplus.c new file +42 -0
+
+     /* board uphy 0 internal register modification for tid certification */
+     cell = nvmem_cell_get(&pdev->dev, disc_name);
+     if (IS_ERR_OR_NULL(cell)) {
+             if (PTR_ERR(cell) == -EPROBE_DEFER)
+                     return -EPROBE_DEFER;
+     }
+
+     otp_v = nvmem_cell_read(cell, &otp_l);
+     nvmem_cell_put(cell);
+
+     if (otp_v)
+             set = ((*otp_v >> OTP_DISC_LEVEL_BITS) |
+                     (*(otp_v + 1) << (sizeof(char) * 8 - OTP_DISC_LEVEL_BITS))) & J_DISC;
+
+     if (!otp_v || set == 0)
+             set = OTP_DISC_LEVEL_DEFAULT;
+
+     val = readl(sp_priv->uphy_regs[port] + CONFIG7);
+     val = (val & ~J_DISC) | set;
+     writel(val, sp_priv->uphy_regs[port] + CONFIG7);
+
+     val = readl(sp_priv->uphy_regs[port] + CONFIG9);
+     val &= ~(J_ECO_PATH);
+     writel(val, sp_priv->uphy_regs[port] + CONFIG9);
+
+     val = readl(sp_priv->uphy_regs[port] + CONFIG1);
+     val &= ~(J_HS_TX_PWRSAV);
+     writel(val, sp_priv->uphy_regs[port] + CONFIG1);
+
+     val = readl(sp_priv->uphy_regs[port] + CONFIG23);
+     val = (val & ~PROB) | PROB;
+     writel(val, sp_priv->uphy_regs[port] + CONFIG23);
+
+     /* USBC 0 reset */
+     writel(RF_MASK_V(USBC0_RESET, USBC0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+     writel(RF_MASK_V_CLR(USBC0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+
+     /* port 0 uphy clk fix */
+     writel(RF_MASK_V(MO1_UPHY0_RX_CLK_SEL, MO1_UPHY0_RX_CLK_SEL),
+            sp_priv->moon4_regs + UPHY0_CONTROL2);
+

drivers/usb/host/ehci-sunplus.c line 214

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+     /* switch to host */
+     writel(RF_MASK_V(MO1_USBC0_USB0_SEL | MO1_USBC0_USB0_CTRL,
+                      MO1_USBC0_USB0_SEL | MO1_USBC0_USB0_CTRL),
+            sp_priv->moon4_regs + USBC_CONTROL);
+
+     /* battery charger */
+     writel(J_TBCWAIT_1P1_MS | J_TVDM_SRC_DIS_8P2_MS | J_TVDM_SRC_EN_1P6_MS | J_BC_EN,
+            sp_priv->uphy_regs[port] + CONFIG16);
+     writel(IBG_TRIM0_SSLVHT | J_VDATREE_TRIM_DEFAULT, sp_priv->uphy_regs[port] + CONFIG17);
+     writel(J_FORCE_DISC_ON | J_DEBUG_CTRL_ADDR_MACRO, sp_priv->uphy_regs[port] + CONFIG3);

What about all of these writes, are you sure they made it to the hardware ok? No need to read?

Same for elsewhere in the file.

thanks,

greg k-h

maintainer
#4 lore raw

Re: [PATCH v1 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021

Quoted 5 lines · 施錕鴻

Hi, Greg
About this issue, my colleague Hammer Hsieh has explained it to
you recently in the mail of "[PATCH v7 2/2] serial: sunplus-uart: Add Sunplus
SoC UART Driver". The ehci driver and uart one are in the same Sunplus Soc.

I do not know what you are referring to, sorry. Remember we get thousands of emails a week.

Please be explicit and make the code work properly for each patch you submit.

thanks,

greg k-h

author
#5 lore raw

Re: [PATCH v1 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021

Greg KH <[email protected]> 於 2022年2月14日 週一 下午5:14寫道：

Quoted 11 lines · 施錕鴻
Quoted 5 lines

Hi, Greg
About this issue, my colleague Hammer Hsieh has explained it to
you recently in the mail of "[PATCH v7 2/2] serial: sunplus-uart: Add Sunplus
SoC UART Driver". The ehci driver and uart one are in the same Sunplus Soc.

I do not know what you are referring to, sorry. Remember we get thousands of emails a week.

Quoted 6 lines

Please be explicit and make the code work properly for each patch you submit.

thanks,

greg k-h

Hi, Greg
About data incoherence issue between memory bus and peripheral bus.
In case of AXI bus, use non-posted write can avoid data incoherence issue.
What if in case of post write:
Send a specific command after last write command. SDCTRL identify specific command, means previous write command done. Then send the interrupt signal to interrupt controller. And then interrupt controller sends done signal to Master. Master receives done signal, means write command done. Then issue a interrupt or proceed next write command.
Thanks for your review.

maintainer
#6 lore raw

Re: [PATCH v1 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021

Quoted 19 lines · 施錕鴻

Greg KH <[email protected]> 於 2022年2月14日 週一 下午5:14寫道：

Quoted 11 lines · 施錕鴻
Quoted 5 lines

Hi, Greg
About this issue, my colleague Hammer Hsieh has explained it to
you recently in the mail of "[PATCH v7 2/2] serial: sunplus-uart: Add Sunplus
SoC UART Driver". The ehci driver and uart one are in the same Sunplus Soc.

I do not know what you are referring to, sorry. Remember we get thousands of emails a week.

Quoted 6 lines

Please be explicit and make the code work properly for each patch you submit.

thanks,

greg k-h

Quoted 9 lines

Hi, Greg
About data incoherence issue between memory bus and peripheral bus.
In case of AXI bus, use non-posted write can avoid data incoherence issue.
What if in case of post write:
Send a specific command after last write command. SDCTRL identify specific command, means previous write command done. Then send the interrupt signal to interrupt controller. And then interrupt controller sends done signal to Master. Master receives done signal, means write command done. Then issue a interrupt or proceed next write command.

I do not understand what you are referring to here as I have no context :(

author
#7 lore raw

Re: [PATCH v1 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021

In reply to Greg KH

I am sorry. I did not send the e-mail to others, I reply it again.

Greg KH <[email protected]> 於 2022年2月14日 週一 下午4:38寫道：

Quoted 62 lines · Vincent Shih
Quoted 60 lines

Add driver for ehci in Sunplus SP7021

Signed-off-by Vincent Shih <[email protected]>
---
 MAINTAINERS                     |   6 +
 drivers/usb/host/Kconfig        |  11 +
 drivers/usb/host/Makefile       |   1 +
 drivers/usb/host/ehci-sunplus.c | 527 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 545 insertions(+)
 create mode 100644 drivers/usb/host/ehci-sunplus.c
MAINTAINERS +6 -0
index 80eebc1..6c2faf3 100644
@@ -17947,6 +17947,12 @@ L:   [email protected]
 S:   Maintained
 F:   drivers/net/ethernet/dlink/sundance.c
 
+SUNPLUS USB EHCI DRIVER
+M:   Vincent Shih <[email protected]>
+L:   [email protected]
+S:   Maintained
+F:   drivers/usb/host/ehci-sunplus.c
+
 SUPERH
 M:   Yoshinori Sato <[email protected]>
 M:   Rich Felker <[email protected]>
drivers/usb/host/Kconfig +11 -0
index c4736d1..9728d3b 100644
@@ -339,6 +339,17 @@ config USB_OCTEON_EHCI
        USB 2.0 device support.  All CN6XXX based chips with USB are
        supported.
 
+config USB_EHCI_SUNPLUS
+     tristate "EHCI support for Sunplus USB controller"
+     depends on SOC_SP7021
+     select NVMEM_SUNPLUS_OCOTP
+     help
+       Enables support for the on-chip EHCI controller in Sunplus
+       SoCs. It supports 32-bit AHB/AXI config bus and 64-bit AXI
+       data bus.
+       This driver can also be built as a module. If so, the module
+       will be called ehci-sunplus.
+
 endif # USB_EHCI_HCD
 
 config USB_OXU210HP_HCD
drivers/usb/host/Makefile +1 -0
index 171de4d..7e9b5f1 100644
@@ -46,6 +46,7 @@ obj-$(CONFIG_USB_EHCI_HCD_STI)      += ehci-st.o
 obj-$(CONFIG_USB_EHCI_EXYNOS)        += ehci-exynos.o
 obj-$(CONFIG_USB_EHCI_HCD_AT91) += ehci-atmel.o
 obj-$(CONFIG_USB_EHCI_BRCMSTB)       += ehci-brcm.o
+obj-$(CONFIG_USB_EHCI_SUNPLUS)       += ehci-sunplus.o
Quoted 17 lines
Quoted 8 lines
drivers/usb/host/Makefile +0 -0
 obj-$(CONFIG_USB_OXU210HP_HCD)       += oxu210hp-hcd.o
 obj-$(CONFIG_USB_ISP116X_HCD)        += isp116x-hcd.o
drivers/usb/host/ehci-sunplus.c new file +0 -0
new file mode 100644
index 0000000..007923a

drivers/usb/host/ehci-sunplus.c line 6

Quoted 6 lines
drivers/usb/host/ehci-sunplus.c new file +6 -0
@@ -0,0 +1,527 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * The EHCI driver for Sunplus SP7021
+ *
+ * Copyright (C) 2019 Sunplus Technology Inc., All rights reserved.

You have not touched this file since 2019?

I did not notice it. I will modify it.

Quoted 148 lines
Quoted 148 lines
drivers/usb/host/ehci-sunplus.c new file +147 -0
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/usb.h>
+#include <linux/usb/ehci_pdriver.h>
+#include <linux/usb/hcd.h>
+
+#include "ehci.h"
+
+#define RF_MASK_V(mask, val)                 (((mask) << 16) | (val))
+#define RF_MASK_V_CLR(mask)                  (((mask) << 16) | 0)
+
+#define USB_PORT0_ID                         0
+#define USB_PORT1_ID                         1
+#define USB_PORT_NUM                         2
+
+#define MASK_BITS                            0xffff
+
+#define OTP_DISC_LEVEL_DEFAULT                       0xd
+#define OTP_DISC_LEVEL_BITS                  0x5
+
+// GROUP 140/150 UPHY0/UPHY1
+#define CONFIG1                                      0x4
+#define J_HS_TX_PWRSAV                               BIT(5)
+#define CONFIG3                                      0xc
+#define J_FORCE_DISC_ON                              BIT(5)
+#define J_DEBUG_CTRL_ADDR_MACRO                      BIT(0)
+#define CONFIG7                                      0x1c
+#define J_DISC                                       0X1f
+#define CONFIG9                                      0x24
+#define J_ECO_PATH                           BIT(6)
+#define CONFIG16                             0x40
+#define J_TBCWAIT_MASK                               GENMASK(6, 5)
+#define J_TBCWAIT_1P1_MS                     FIELD_PREP(J_TBCWAIT_MASK, 0)
+#define J_TVDM_SRC_DIS_MASK                  GENMASK(4, 3)
+#define J_TVDM_SRC_DIS_8P2_MS                        FIELD_PREP(J_TVDM_SRC_DIS_MASK, 3)
+#define J_TVDM_SRC_EN_MASK                   GENMASK(2, 1)
+#define J_TVDM_SRC_EN_1P6_MS                 FIELD_PREP(J_TVDM_SRC_EN_MASK, 0)
+#define J_BC_EN                                      BIT(0)
+#define CONFIG17                             0x44
+#define IBG_TRIM0_MASK                               GENMASK(7, 5)
+#define IBG_TRIM0_SSLVHT                     FIELD_PREP(IBG_TRIM0_MASK, 4)
+#define J_VDATREE_TRIM_MASK                  GENMASK(4, 1)
+#define J_VDATREE_TRIM_DEFAULT                       FIELD_PREP(J_VDATREE_TRIM_MASK, 9)
+#define CONFIG23                             0x5c
+#define PROB_MASK                            GENMASK(5, 3)
+#define PROB                                 FIELD_PREP(PROB_MASK, 7)
+
+// GROUP 0 MOON 0
+#define HARDWARE_RESET_CONTROL2                      0x5c
+#define UPHY1_RESET                          BIT(14)
+#define UPHY0_RESET                          BIT(13)
+#define USBC1_RESET                          BIT(11)
+#define USBC0_RESET                          BIT(10)
+
+// GROUP 4 MOON 4
+#define USBC_CONTROL                         0x44
+#define MO1_USBC1_USB0_SEL                   BIT(13)
+#define MO1_USBC1_USB0_CTRL                  BIT(12)
+#define MO1_USBC0_USB0_SEL                   BIT(5)
+#define MO1_USBC0_USB0_CTRL                  BIT(4)
+#define UPHY0_CONTROL0                               0x48
+#define UPHY0_CONTROL1                               0x4c
+#define UPHY0_CONTROL2                               0x50
+#define MO1_UPHY0_RX_CLK_SEL                 BIT(6)
+#define UPHY0_CONTROL3                               0x54
+#define MO1_UPHY0_PLL_POWER_OFF_SEL          BIT(7)
+#define MO1_UPHY0_PLL_POWER_OFF                      BIT(3)
+#define UPHY1_CONTROL0                               0x58
+#define UPHY1_CONTROL1                               0x5c
+#define UPHY1_CONTROL2                               0x60
+#define MO1_UPHY1_RX_CLK_SEL                 BIT(6)
+#define UPHY1_CONTROL3                               0x64
+#define MO1_UPHY1_PLL_POWER_OFF_SEL          BIT(7)
+#define MO1_UPHY1_PLL_POWER_OFF                      BIT(3)
+
+#define hcd_to_sp_ehci_priv(h) \
+             ((struct sp_ehci_priv *)hcd_to_ehci(h)->priv)
+
+struct sp_ehci_priv {
+     struct resource *uphy_res_mem[USB_PORT_NUM];
+     struct resource *moon0_res_mem;
+     struct resource *moon4_res_mem;
+     struct clk *uphy_clk[USB_PORT_NUM];
+     struct clk *ehci_clk[USB_PORT_NUM];
+     void __iomem *uphy_regs[USB_PORT_NUM];
+     void __iomem *moon0_regs;
+     void __iomem *moon4_regs;
+};
+
+static int ehci_sunplus_reset(struct usb_hcd *hcd)
+{
+     struct platform_device *pdev = to_platform_device(hcd->self.controller);
+     struct usb_ehci_pdata *pdata = pdev->dev.platform_data;
+     struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+     int retval;
+
+     hcd->has_tt = pdata->has_tt;
+     ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug;
+     ehci->big_endian_desc = pdata->big_endian_desc;
+     ehci->big_endian_mmio = pdata->big_endian_mmio;
+
+     ehci->caps = hcd->regs + pdata->caps_offset;
+     retval = ehci_setup(hcd);
+
+     return retval;
+}
+
+static struct hc_driver __read_mostly ehci_sunplus_driver;
+
+static const struct ehci_driver_overrides ehci_sunplus_overrides __initconst = {
+     .reset =                ehci_sunplus_reset,
+     .extra_priv_size =      sizeof(struct sp_ehci_priv),
+};
+
+static int uphy0_init(struct platform_device *pdev, struct sp_ehci_priv *sp_priv)
+{
+     struct nvmem_cell *cell;
+     char *disc_name = "disc_vol";
+     ssize_t otp_l = 0;
+     char *otp_v;
+     int port = pdev->id - 1;
+     u32 val, set, pll_pwr_on, pll_pwr_off;
+
+     /* Default value modification */
+     writel(RF_MASK_V(MASK_BITS, 0x4002), sp_priv->moon4_regs + UPHY0_CONTROL0);
+     writel(RF_MASK_V(MASK_BITS, 0x8747), sp_priv->moon4_regs + UPHY0_CONTROL1);
+
+     /* PLL power off/on twice */
+     pll_pwr_off = (readl(sp_priv->moon4_regs + UPHY0_CONTROL3) & ~MASK_BITS)
+                     | MO1_UPHY0_PLL_POWER_OFF_SEL | MO1_UPHY0_PLL_POWER_OFF;
+     pll_pwr_on = (readl(sp_priv->moon4_regs + UPHY0_CONTROL3) & ~MASK_BITS)
+                     | MO1_UPHY0_PLL_POWER_OFF_SEL;
+
+     writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY0_CONTROL3);
+     mdelay(1);
+     writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY0_CONTROL3);
+     mdelay(1);
+     writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY0_CONTROL3);
Quoted 9 lines

drivers/usb/host/ehci-sunplus.c line 158

Quoted 5 lines
drivers/usb/host/ehci-sunplus.c new file +5 -0
+     mdelay(1);
+     writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY0_CONTROL3);
+     mdelay(1);
+     writel(RF_MASK_V(MASK_BITS, 0x0), sp_priv->moon4_regs + UPHY0_CONTROL3);
+

Why not just do a read after a write so that you know that is the correct delay? Doing a sleep like this seems slow and possibly wrong.

The purposes of these delays are for discharging the voltages of the capacitors in the circuit.
They have nothing to do with the registers.

Quoted 7 lines

drivers/usb/host/ehci-sunplus.c line 162

Quoted 4 lines
drivers/usb/host/ehci-sunplus.c new file +4 -0
+     /* reset UPHY0 */
+     writel(RF_MASK_V(UPHY0_RESET, UPHY0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+     writel(RF_MASK_V_CLR(UPHY0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+     mdelay(1);

Again, read()?

The purpose of the delay is for discharging the voltages of the capacitors in the circuit.
It has nothing to do with the registers.

Quoted 47 lines
Quoted 47 lines
drivers/usb/host/ehci-sunplus.c new file +47 -0
+
+     /* board uphy 0 internal register modification for tid certification */
+     cell = nvmem_cell_get(&pdev->dev, disc_name);
+     if (IS_ERR_OR_NULL(cell)) {
+             if (PTR_ERR(cell) == -EPROBE_DEFER)
+                     return -EPROBE_DEFER;
+     }
+
+     otp_v = nvmem_cell_read(cell, &otp_l);
+     nvmem_cell_put(cell);
+
+     if (otp_v)
+             set = ((*otp_v >> OTP_DISC_LEVEL_BITS) |
+                     (*(otp_v + 1) << (sizeof(char) * 8 - OTP_DISC_LEVEL_BITS))) & J_DISC;
+
+     if (!otp_v || set == 0)
+             set = OTP_DISC_LEVEL_DEFAULT;
+
+     val = readl(sp_priv->uphy_regs[port] + CONFIG7);
+     val = (val & ~J_DISC) | set;
+     writel(val, sp_priv->uphy_regs[port] + CONFIG7);
+
+     val = readl(sp_priv->uphy_regs[port] + CONFIG9);
+     val &= ~(J_ECO_PATH);
+     writel(val, sp_priv->uphy_regs[port] + CONFIG9);
+
+     val = readl(sp_priv->uphy_regs[port] + CONFIG1);
+     val &= ~(J_HS_TX_PWRSAV);
+     writel(val, sp_priv->uphy_regs[port] + CONFIG1);
+
+     val = readl(sp_priv->uphy_regs[port] + CONFIG23);
+     val = (val & ~PROB) | PROB;
+     writel(val, sp_priv->uphy_regs[port] + CONFIG23);
+
+     /* USBC 0 reset */
+     writel(RF_MASK_V(USBC0_RESET, USBC0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+     writel(RF_MASK_V_CLR(USBC0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+
+     /* port 0 uphy clk fix */
+     writel(RF_MASK_V(MO1_UPHY0_RX_CLK_SEL, MO1_UPHY0_RX_CLK_SEL),
+            sp_priv->moon4_regs + UPHY0_CONTROL2);
+
+     /* switch to host */
+     writel(RF_MASK_V(MO1_USBC0_USB0_SEL | MO1_USBC0_USB0_CTRL,
+                      MO1_USBC0_USB0_SEL | MO1_USBC0_USB0_CTRL),
+            sp_priv->moon4_regs + USBC_CONTROL);
+
Quoted 9 lines

drivers/usb/host/ehci-sunplus.c line 214

Quoted 5 lines
drivers/usb/host/ehci-sunplus.c new file +5 -0
+     /* battery charger */
+     writel(J_TBCWAIT_1P1_MS | J_TVDM_SRC_DIS_8P2_MS | J_TVDM_SRC_EN_1P6_MS | J_BC_EN,
+            sp_priv->uphy_regs[port] + CONFIG16);
+     writel(IBG_TRIM0_SSLVHT | J_VDATREE_TRIM_DEFAULT, sp_priv->uphy_regs[port] + CONFIG17);
+     writel(J_FORCE_DISC_ON | J_DEBUG_CTRL_ADDR_MACRO, sp_priv->uphy_regs[port] + CONFIG3);

What about all of these writes, are you sure they made it to the hardware ok? No need to read?

Yes, they made it to the hardware and there is no need to read. The write command of Sunplus SP7021 is posted write. But it has a MSI (Message Signal Interrupt) mechanism to fix the data incoherence issue between CPU and other bus masters. The steps are shown below.
1. The master issues a MSI command after the last write command.
2. When SDCTRL receives the MSI command, the previous write command have been done and SDCTRL would issue an interrupt to interrupt controller.
3. The inerrupt controller issues a "done" signal to the master.
4. When the master receives the "done" signal, the write command has been done. Then the master can process the next write command or issue an interrupt.

Quoted 5 lines

Same for elsewhere in the file.

thanks,

greg k-h

Thanks for your review.

maintainer
#8 lore raw

Re: [PATCH v1 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021

In reply to Vincent Shih
Quoted 1 line · Vincent Shih

Add driver for ehci in Sunplus SP7021

The binding looked odd with 0 and 1 in the compatible strings. That's rarely to never correct, so I looked here to see how it is used.

Quoted 332 lines
Signed-off-by Vincent Shih <[email protected]>
---
 MAINTAINERS                     |   6 +
 drivers/usb/host/Kconfig        |  11 +
 drivers/usb/host/Makefile       |   1 +
 drivers/usb/host/ehci-sunplus.c | 527 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 545 insertions(+)
 create mode 100644 drivers/usb/host/ehci-sunplus.c
drivers/usb/host/ehci-sunplus.c new file +315 -0
new file mode 100644
index 0000000..007923a
@@ -0,0 +1,527 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * The EHCI driver for Sunplus SP7021
+ *
+ * Copyright (C) 2019 Sunplus Technology Inc., All rights reserved.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/usb.h>
+#include <linux/usb/ehci_pdriver.h>
+#include <linux/usb/hcd.h>
+
+#include "ehci.h"
+
+#define RF_MASK_V(mask, val)			(((mask) << 16) | (val))
+#define RF_MASK_V_CLR(mask)			(((mask) << 16) | 0)
+
+#define USB_PORT0_ID				0
+#define USB_PORT1_ID				1
+#define USB_PORT_NUM				2
+
+#define MASK_BITS				0xffff
+
+#define OTP_DISC_LEVEL_DEFAULT			0xd
+#define OTP_DISC_LEVEL_BITS			0x5
+
+// GROUP 140/150 UPHY0/UPHY1
+#define CONFIG1					0x4
+#define J_HS_TX_PWRSAV				BIT(5)
+#define CONFIG3					0xc
+#define J_FORCE_DISC_ON				BIT(5)
+#define J_DEBUG_CTRL_ADDR_MACRO			BIT(0)
+#define CONFIG7					0x1c
+#define J_DISC					0X1f
+#define CONFIG9					0x24
+#define J_ECO_PATH				BIT(6)
+#define CONFIG16				0x40
+#define J_TBCWAIT_MASK				GENMASK(6, 5)
+#define J_TBCWAIT_1P1_MS			FIELD_PREP(J_TBCWAIT_MASK, 0)
+#define J_TVDM_SRC_DIS_MASK			GENMASK(4, 3)
+#define J_TVDM_SRC_DIS_8P2_MS			FIELD_PREP(J_TVDM_SRC_DIS_MASK, 3)
+#define J_TVDM_SRC_EN_MASK			GENMASK(2, 1)
+#define J_TVDM_SRC_EN_1P6_MS			FIELD_PREP(J_TVDM_SRC_EN_MASK, 0)
+#define J_BC_EN					BIT(0)
+#define CONFIG17				0x44
+#define IBG_TRIM0_MASK				GENMASK(7, 5)
+#define IBG_TRIM0_SSLVHT			FIELD_PREP(IBG_TRIM0_MASK, 4)
+#define J_VDATREE_TRIM_MASK			GENMASK(4, 1)
+#define J_VDATREE_TRIM_DEFAULT			FIELD_PREP(J_VDATREE_TRIM_MASK, 9)
+#define CONFIG23				0x5c
+#define PROB_MASK				GENMASK(5, 3)
+#define PROB					FIELD_PREP(PROB_MASK, 7)
+
+// GROUP 0 MOON 0
+#define HARDWARE_RESET_CONTROL2			0x5c
+#define UPHY1_RESET				BIT(14)
+#define UPHY0_RESET				BIT(13)
+#define USBC1_RESET				BIT(11)
+#define USBC0_RESET				BIT(10)
+
+// GROUP 4 MOON 4
+#define USBC_CONTROL				0x44
+#define MO1_USBC1_USB0_SEL			BIT(13)
+#define MO1_USBC1_USB0_CTRL			BIT(12)
+#define MO1_USBC0_USB0_SEL			BIT(5)
+#define MO1_USBC0_USB0_CTRL			BIT(4)
+#define UPHY0_CONTROL0				0x48
+#define UPHY0_CONTROL1				0x4c
+#define UPHY0_CONTROL2				0x50
+#define MO1_UPHY0_RX_CLK_SEL			BIT(6)
+#define UPHY0_CONTROL3				0x54
+#define MO1_UPHY0_PLL_POWER_OFF_SEL		BIT(7)
+#define MO1_UPHY0_PLL_POWER_OFF			BIT(3)
+#define UPHY1_CONTROL0				0x58
+#define UPHY1_CONTROL1				0x5c
+#define UPHY1_CONTROL2				0x60
+#define MO1_UPHY1_RX_CLK_SEL			BIT(6)
+#define UPHY1_CONTROL3				0x64
+#define MO1_UPHY1_PLL_POWER_OFF_SEL		BIT(7)
+#define MO1_UPHY1_PLL_POWER_OFF			BIT(3)
+
+#define hcd_to_sp_ehci_priv(h) \
+		((struct sp_ehci_priv *)hcd_to_ehci(h)->priv)
+
+struct sp_ehci_priv {
+	struct resource *uphy_res_mem[USB_PORT_NUM];
+	struct resource *moon0_res_mem;
+	struct resource *moon4_res_mem;
+	struct clk *uphy_clk[USB_PORT_NUM];
+	struct clk *ehci_clk[USB_PORT_NUM];
+	void __iomem *uphy_regs[USB_PORT_NUM];
+	void __iomem *moon0_regs;
+	void __iomem *moon4_regs;
+};
+
+static int ehci_sunplus_reset(struct usb_hcd *hcd)
+{
+	struct platform_device *pdev = to_platform_device(hcd->self.controller);
+	struct usb_ehci_pdata *pdata = pdev->dev.platform_data;
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+	int retval;
+
+	hcd->has_tt = pdata->has_tt;
+	ehci->has_synopsys_hc_bug = pdata->has_synopsys_hc_bug;
+	ehci->big_endian_desc = pdata->big_endian_desc;
+	ehci->big_endian_mmio = pdata->big_endian_mmio;
+
+	ehci->caps = hcd->regs + pdata->caps_offset;
+	retval = ehci_setup(hcd);
+
+	return retval;
+}
+
+static struct hc_driver __read_mostly ehci_sunplus_driver;
+
+static const struct ehci_driver_overrides ehci_sunplus_overrides __initconst = {
+	.reset =		ehci_sunplus_reset,
+	.extra_priv_size =	sizeof(struct sp_ehci_priv),
+};
+
+static int uphy0_init(struct platform_device *pdev, struct sp_ehci_priv *sp_priv)
+{
+	struct nvmem_cell *cell;
+	char *disc_name = "disc_vol";
+	ssize_t otp_l = 0;
+	char *otp_v;
+	int port = pdev->id - 1;
+	u32 val, set, pll_pwr_on, pll_pwr_off;
+
+	/* Default value modification */
+	writel(RF_MASK_V(MASK_BITS, 0x4002), sp_priv->moon4_regs + UPHY0_CONTROL0);
+	writel(RF_MASK_V(MASK_BITS, 0x8747), sp_priv->moon4_regs + UPHY0_CONTROL1);
+
+	/* PLL power off/on twice */
+	pll_pwr_off = (readl(sp_priv->moon4_regs + UPHY0_CONTROL3) & ~MASK_BITS)
+			| MO1_UPHY0_PLL_POWER_OFF_SEL | MO1_UPHY0_PLL_POWER_OFF;
+	pll_pwr_on = (readl(sp_priv->moon4_regs + UPHY0_CONTROL3) & ~MASK_BITS)
+			| MO1_UPHY0_PLL_POWER_OFF_SEL;
+
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY0_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY0_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY0_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY0_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, 0x0), sp_priv->moon4_regs + UPHY0_CONTROL3);
+
+	/* reset UPHY0 */
+	writel(RF_MASK_V(UPHY0_RESET, UPHY0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+	writel(RF_MASK_V_CLR(UPHY0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+	mdelay(1);
+
+	/* board uphy 0 internal register modification for tid certification */
+	cell = nvmem_cell_get(&pdev->dev, disc_name);
+	if (IS_ERR_OR_NULL(cell)) {
+		if (PTR_ERR(cell) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+	}
+
+	otp_v = nvmem_cell_read(cell, &otp_l);
+	nvmem_cell_put(cell);
+
+	if (otp_v)
+		set = ((*otp_v >> OTP_DISC_LEVEL_BITS) |
+			(*(otp_v + 1) << (sizeof(char) * 8 - OTP_DISC_LEVEL_BITS))) & J_DISC;
+
+	if (!otp_v || set == 0)
+		set = OTP_DISC_LEVEL_DEFAULT;
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG7);
+	val = (val & ~J_DISC) | set;
+	writel(val, sp_priv->uphy_regs[port] + CONFIG7);
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG9);
+	val &= ~(J_ECO_PATH);
+	writel(val, sp_priv->uphy_regs[port] + CONFIG9);
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG1);
+	val &= ~(J_HS_TX_PWRSAV);
+	writel(val, sp_priv->uphy_regs[port] + CONFIG1);
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG23);
+	val = (val & ~PROB) | PROB;
+	writel(val, sp_priv->uphy_regs[port] + CONFIG23);
+
+	/* USBC 0 reset */
+	writel(RF_MASK_V(USBC0_RESET, USBC0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+	writel(RF_MASK_V_CLR(USBC0_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+
+	/* port 0 uphy clk fix */
+	writel(RF_MASK_V(MO1_UPHY0_RX_CLK_SEL, MO1_UPHY0_RX_CLK_SEL),
+	       sp_priv->moon4_regs + UPHY0_CONTROL2);
+
+	/* switch to host */
+	writel(RF_MASK_V(MO1_USBC0_USB0_SEL | MO1_USBC0_USB0_CTRL,
+			 MO1_USBC0_USB0_SEL | MO1_USBC0_USB0_CTRL),
+	       sp_priv->moon4_regs + USBC_CONTROL);
+
+	/* battery charger */
+	writel(J_TBCWAIT_1P1_MS | J_TVDM_SRC_DIS_8P2_MS | J_TVDM_SRC_EN_1P6_MS | J_BC_EN,
+	       sp_priv->uphy_regs[port] + CONFIG16);
+	writel(IBG_TRIM0_SSLVHT | J_VDATREE_TRIM_DEFAULT, sp_priv->uphy_regs[port] + CONFIG17);
+	writel(J_FORCE_DISC_ON | J_DEBUG_CTRL_ADDR_MACRO, sp_priv->uphy_regs[port] + CONFIG3);
+
+	return 0;
+}
+
+static int uphy1_init(struct platform_device *pdev, struct sp_ehci_priv *sp_priv)
+{
+	struct nvmem_cell *cell;
+	char *disc_name = "disc_vol";
+	ssize_t otp_l = 0;
+	char *otp_v;
+	int port = pdev->id - 1;
+	u32 val, set, pll_pwr_on, pll_pwr_off;
+
+	/* Default value modification */
+	writel(RF_MASK_V(MASK_BITS, 0x4002), sp_priv->moon4_regs + UPHY0_CONTROL0);
+	writel(RF_MASK_V(MASK_BITS, 0x8747), sp_priv->moon4_regs + UPHY0_CONTROL1);
+
+	/* PLL power off/on twice */
+	pll_pwr_off = (readl(sp_priv->moon4_regs + UPHY1_CONTROL3) & ~MASK_BITS)
+			| MO1_UPHY1_PLL_POWER_OFF_SEL | MO1_UPHY1_PLL_POWER_OFF;
+	pll_pwr_on = (readl(sp_priv->moon4_regs + UPHY1_CONTROL3) & ~MASK_BITS)
+			| MO1_UPHY1_PLL_POWER_OFF_SEL;
+
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY1_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY1_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_off), sp_priv->moon4_regs + UPHY1_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, pll_pwr_on), sp_priv->moon4_regs + UPHY1_CONTROL3);
+	mdelay(1);
+	writel(RF_MASK_V(MASK_BITS, 0x0), sp_priv->moon4_regs + UPHY1_CONTROL3);
+
+	/* reset UPHY1 */
+	writel(RF_MASK_V(UPHY1_RESET, UPHY1_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+	writel(RF_MASK_V_CLR(UPHY1_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+	mdelay(1);
+
+	/* board uphy 1 internal register modification for tid certification */
+	cell = nvmem_cell_get(&pdev->dev, disc_name);
+	if (IS_ERR_OR_NULL(cell)) {
+		if (PTR_ERR(cell) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+	}
+
+	otp_v = nvmem_cell_read(cell, &otp_l);
+	nvmem_cell_put(cell);
+
+	if (otp_v)
+		set = ((*otp_v >> OTP_DISC_LEVEL_BITS) |
+			(*(otp_v + 1) << (sizeof(char) * 8 - OTP_DISC_LEVEL_BITS))) & J_DISC;
+
+	if (!otp_v || set == 0)
+		set = OTP_DISC_LEVEL_DEFAULT;
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG7);
+	val = (val & ~J_DISC) | set;
+	writel(val, sp_priv->uphy_regs[port] + CONFIG7);
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG9);
+	val &= ~(J_ECO_PATH);
+	writel(val, sp_priv->uphy_regs[port] + CONFIG9);
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG1);
+	val &= ~(J_HS_TX_PWRSAV);
+	writel(val, sp_priv->uphy_regs[port] + CONFIG1);
+
+	val = readl(sp_priv->uphy_regs[port] + CONFIG23);
+	val = (val & ~PROB) | PROB;
+	writel(val, sp_priv->uphy_regs[port] + CONFIG23);
+
+	/* USBC 1 reset */
+	writel(RF_MASK_V(USBC1_RESET, USBC1_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+	writel(RF_MASK_V_CLR(USBC1_RESET), sp_priv->moon0_regs + HARDWARE_RESET_CONTROL2);
+
+	/* port 1 uphy clk fix */
+	writel(RF_MASK_V(MO1_UPHY1_RX_CLK_SEL, MO1_UPHY1_RX_CLK_SEL),
+	       sp_priv->moon4_regs + UPHY1_CONTROL2);
+
+	/* switch to host */
+	writel(RF_MASK_V(MO1_USBC1_USB0_SEL | MO1_USBC1_USB0_CTRL,
+			 MO1_USBC1_USB0_SEL | MO1_USBC1_USB0_CTRL),
+	       sp_priv->moon4_regs + USBC_CONTROL);
+
+	/* battery charger */
+	writel(J_TBCWAIT_1P1_MS | J_TVDM_SRC_DIS_8P2_MS | J_TVDM_SRC_EN_1P6_MS | J_BC_EN,
+	       sp_priv->uphy_regs[port] + CONFIG16);
+	writel(IBG_TRIM0_SSLVHT | J_VDATREE_TRIM_DEFAULT, sp_priv->uphy_regs[port] + CONFIG17);
+	writel(J_FORCE_DISC_ON | J_DEBUG_CTRL_ADDR_MACRO, sp_priv->uphy_regs[port] + CONFIG3);
+
+	return 0;
+}
+
+static struct usb_ehci_pdata usb_ehci_pdata = {
+};
+
+static int ehci_sunplus_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sp_ehci_priv *sp_priv;
+	struct resource *res_mem;

drivers/usb/host/ehci-sunplus.c line 325

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+	struct usb_hcd *hcd;
+	int port, irq, err;
+
+	if (usb_disabled())
+		return -ENODEV;
+
+	if (of_device_is_compatible(pdev->dev.of_node, "sunplus,sp7021-usb-ehci0"))
+		pdev->id = 1;
+	else if (of_device_is_compatible(pdev->dev.of_node, "sunplus,sp7021-usb-ehci1"))
+		pdev->id = 2;

I don't think drivers are free to set 'id'. It's already been set long before probe happens. Different compatibles is not how you distinguish different instances of the same block. It seems that perhaps you should be using the phy binding to handle the difference in uphy registers. That seems to be the only difference.

drivers/usb/host/ehci-sunplus.c line 328

Quoted 3 lines
drivers/usb/host/ehci-sunplus.c new file +3 -0
+
+	port = pdev->id - 1;
+	pdev->dev.platform_data = &usb_ehci_pdata;

Drivers don't set platform_data either. Plus there is no point here since every instance gets the same data.

drivers/usb/host/ehci-sunplus.c line 341

Quoted 13 lines
drivers/usb/host/ehci-sunplus.c new file +13 -0
+
+	/* initialize hcd */
+	hcd = usb_create_hcd(&ehci_sunplus_driver, dev, dev_name(dev));
+	if (!hcd)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, hcd);
+	sp_priv = hcd_to_sp_ehci_priv(hcd);
+
+	/* initialize uphy0/uphy1 */
+	sp_priv->uphy_res_mem[port] = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	sp_priv->uphy_regs[port] = ioremap(sp_priv->uphy_res_mem[port]->start,
+					   resource_size(sp_priv->uphy_res_mem[port]));

sp_priv is per instance, and there's only 1 port for each instance. So why do you have an array here?

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+
+	if (IS_ERR(sp_priv->uphy_regs[port]))
+		return PTR_ERR(sp_priv->uphy_regs[port]);
+
+	sp_priv->moon0_res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+	sp_priv->moon0_regs = devm_ioremap(dev, sp_priv->moon0_res_mem->start,
+					   resource_size(sp_priv->moon0_res_mem));
+	if (IS_ERR(sp_priv->moon0_regs))
+		return PTR_ERR(sp_priv->moon0_regs);
+

drivers/usb/host/ehci-sunplus.c line 361

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+	sp_priv->moon4_res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 3);
+	sp_priv->moon4_regs = devm_ioremap(dev, sp_priv->moon4_res_mem->start,
+					   resource_size(sp_priv->moon4_res_mem));
+	if (IS_ERR(sp_priv->moon4_regs))
+		return PTR_ERR(sp_priv->moon4_regs);
+
+	if (port == USB_PORT0_ID)
+		sp_priv->uphy_clk[USB_PORT0_ID] = devm_clk_get(dev, "USB2_UPHY0");
+	else if (port == USB_PORT1_ID)
+		sp_priv->uphy_clk[USB_PORT1_ID] = devm_clk_get(dev, "USB2_UPHY1");

This looks wrong too. There's only 1 clock per instance and the name should be the same.

The clock for the phy goes with the phy node.

Quoted 30 lines
drivers/usb/host/ehci-sunplus.c new file +30 -0
+
+	if (IS_ERR(sp_priv->uphy_clk[port])) {
+		err = PTR_ERR(sp_priv->uphy_clk[port]);
+		goto err_put_hcd;
+	}
+
+	err = clk_prepare_enable(sp_priv->uphy_clk[port]);
+	if (err)
+		goto err_put_hcd;
+
+	if (port == USB_PORT0_ID)
+		err = uphy0_init(pdev, sp_priv);
+	else if (port == USB_PORT1_ID)
+		err = uphy1_init(pdev, sp_priv);
+
+	if (err == -EPROBE_DEFER)
+		goto err_uphy_clk;
+
+	/* ehci */
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	hcd->regs = devm_ioremap_resource(dev, res_mem);
+	if (IS_ERR(hcd->regs)) {
+		err = PTR_ERR(hcd->regs);
+		goto err_uphy_clk;
+	}
+
+	hcd->rsrc_start = res_mem->start;
+	hcd->rsrc_len = resource_size(res_mem);
+
+	irq = platform_get_irq(pdev, 0);

drivers/usb/host/ehci-sunplus.c line 401

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+	if (irq < 0) {
+		err = irq;
+		goto err_uphy_clk;
+	}
+	dev_dbg(&pdev->dev, "ehci_id:%d,irq:%d\n", port, irq);
+
+	if (port == USB_PORT0_ID)
+		sp_priv->ehci_clk[port] = devm_clk_get(dev, "USB2_USBC0");
+	else if (port == USB_PORT1_ID)
+		sp_priv->ehci_clk[port] = devm_clk_get(dev, "USB2_USBC1");

This looks wrong too. There's only 1 clock per instance and the name should be the same.

Quoted 27 lines
drivers/usb/host/ehci-sunplus.c new file +27 -0
+
+	if (IS_ERR(sp_priv->ehci_clk[port])) {
+		err = PTR_ERR(sp_priv->ehci_clk[port]);
+		goto err_uphy_clk;
+	}
+
+	err = clk_prepare_enable(sp_priv->ehci_clk[port]);
+	if (err)
+		goto err_uphy_clk;
+
+	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
+	if (err)
+		goto err_ehci_clk;
+
+	dev_dbg(dev, "hcd_irq:%d,%d\n", hcd->irq, irq);
+
+	return err;
+
+err_ehci_clk:
+	clk_disable_unprepare(sp_priv->ehci_clk[port]);
+err_uphy_clk:
+	clk_disable_unprepare(sp_priv->uphy_clk[port]);
+err_put_hcd:
+	usb_put_hcd(hcd);
+
+	return err;
+}
author
#9 lore raw

PATCH 2/2 dt-bindings: usb: Add bindings doc for Sunplus EHCI driver

In reply to Vincent Shih

Add bindings doc for Sunplus EHCI driver

Signed-off-by Vincent Shih <[email protected]>
---
 .../bindings/usb/sunplus,sp7021-usb-ehci.yaml      | 69 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml
Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml new file +69 -0
new file mode 100644
index 0000000..299f7b7
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd. 2021
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/sunplus,sp7021-usb-ehci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sunplus SP7021 EHCI Device Tree bindings for ehci0/ehci1
+
+maintainers:
+  - Vincent Shih <[email protected]>
+
+allOf:
+  - $ref: usb-hcd.yaml#
+
+properties:
+  compatible:
+    enum:
+      - sunplus,sp7021-usb-ehci0
+      - sunplus,sp7021-usb-ehci1
+
+  clocks:
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: USB2_USBC0
+      - const: USB2_UPHY0
+
+  reg:
+    maxItems: 4
+
+  interrupts:
+    maxItems: 1
+
+  nvmem-cell-names:
+    description: names corresponding to the nvmem cells of disconnect voltage
+    const: disc_vol
+
+  nvmem-cells:
+    description: nvmem cell address of disconnect voltage
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - reg
+  - interrupts
+  - nvmem-cell-names
+  - nvmem-cells
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    sp_ehci0: usb@9c102100 {
+      compatible = "sunplus,sp7021-usb-ehci0";
+      clocks = <&clkc 0x3a>, <&clkc 0x3d>;
+      clock-names = "USB2_USBC0", "USB2_UPHY0";
+      reg = <0x9c102100 0x68>, <0x9c004a80 0x80>,
+               <0x9c000000 0x80>, <0x9c000200 0x80>;
+      interrupt-parent = <&intc>;
+      interrupts = <14 IRQ_TYPE_LEVEL_HIGH>;
+      nvmem-cell-names = "disc_vol";
+      nvmem-cells = <&disc_vol>;
+    };
+...
MAINTAINERS +1 -0
index 6c2faf3..49702c4 100644
@@ -17951,6 +17951,7 @@ SUNPLUS USB EHCI DRIVER
 M:	Vincent Shih <[email protected]>
 L:	[email protected]
 S:	Maintained
+F:	Documentation/devicetree/bindings/nvmem/sunplus,sp7021-ocotp.yaml
 F:	drivers/usb/host/ehci-sunplus.c
 
 SUPERH
Signature · 1 line
2.7.4
maintainer
#10 lore raw

Re: [PATCH v1 2/2] dt-bindings: usb: Add bindings doc for Sunplus EHCI driver

Quoted 33 lines · Vincent Shih

Add bindings doc for Sunplus EHCI driver

Signed-off-by Vincent Shih <[email protected]>
---
 .../bindings/usb/sunplus,sp7021-usb-ehci.yaml      | 69 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 70 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml
Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml new file +18 -0
new file mode 100644
index 0000000..299f7b7
@@ -0,0 +1,69 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright (C) Sunplus Co., Ltd. 2021
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/sunplus,sp7021-usb-ehci.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Sunplus SP7021 EHCI Device Tree bindings for ehci0/ehci1
+
+maintainers:
+  - Vincent Shih <[email protected]>
+
+allOf:
+  - $ref: usb-hcd.yaml#
+
+properties:
+  compatible:
+    enum:

Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml line 28

Quoted 10 lines
Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml new file +10 -0
+      - sunplus,sp7021-usb-ehci0
+      - sunplus,sp7021-usb-ehci1
+
+  clocks:
+    maxItems: 2
+
+  clock-names:
+    items:
+      - const: USB2_USBC0
+      - const: USB2_UPHY0

This doesn't match what the driver looks for, though the 0 or 1 on the end is wrong. You obviously have not tested this against your actual dts file(s).

The 'USB2_' part is redundant. You only need enough information to distinguish clocks within a h/w block.

Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml line 31

Quoted 3 lines
Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml new file +3 -0
+
+  reg:
+    maxItems: 4

You have to define each entry.

Quoted 54 lines
Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml new file +38 -0
+
+  interrupts:
+    maxItems: 1
+
+  nvmem-cell-names:
+    description: names corresponding to the nvmem cells of disconnect voltage
+    const: disc_vol
+
+  nvmem-cells:
+    description: nvmem cell address of disconnect voltage
+    maxItems: 1
+
+required:
+  - compatible
+  - clocks
+  - reg
+  - interrupts
+  - nvmem-cell-names
+  - nvmem-cells
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    sp_ehci0: usb@9c102100 {
+      compatible = "sunplus,sp7021-usb-ehci0";
+      clocks = <&clkc 0x3a>, <&clkc 0x3d>;
+      clock-names = "USB2_USBC0", "USB2_UPHY0";
+      reg = <0x9c102100 0x68>, <0x9c004a80 0x80>,
+               <0x9c000000 0x80>, <0x9c000200 0x80>;
+      interrupt-parent = <&intc>;
+      interrupts = <14 IRQ_TYPE_LEVEL_HIGH>;
+      nvmem-cell-names = "disc_vol";
+      nvmem-cells = <&disc_vol>;
+    };
+...
MAINTAINERS +1 -0
index 6c2faf3..49702c4 100644
@@ -17951,6 +17951,7 @@ SUNPLUS USB EHCI DRIVER
 M:	Vincent Shih <[email protected]>
 L:	[email protected]
 S:	Maintained
+F:	Documentation/devicetree/bindings/nvmem/sunplus,sp7021-ocotp.yaml
 F:	drivers/usb/host/ehci-sunplus.c
 
 SUPERH
Signature · 1 line
2.7.4