Skip to contentSkip to conversation

Add driver for EHCI in Sunplus SP7021

PATCH In review Vincent Shih 4 participants 7 messages Apr 29, 2022 – Apr 30, 2022

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v4 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 Cortex 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      |  79 ++++++
 MAINTAINERS                                        |   7 +
 drivers/usb/host/Kconfig                           |  11 +
 drivers/usb/host/Makefile                          |   1 +
 drivers/usb/host/ehci-sunplus.c                    | 289 +++++++++++++++++++++
 5 files changed, 387 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
#1 lore raw

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

Add driver for ehci in Sunplus SP7021

---
Changes in v4:
- Implement power_on, power_off and power_suspend functions.

 MAINTAINERS                     |   6 +
 drivers/usb/host/Kconfig        |  11 ++
 drivers/usb/host/Makefile       |   1 +
 drivers/usb/host/ehci-sunplus.c | 289 ++++++++++++++++++++++++++++++++++++++++
 4 files changed, 307 insertions(+)
 create mode 100644 drivers/usb/host/ehci-sunplus.c
MAINTAINERS +6 -0
index dd61684..e103afa 100644
@@ -18914,6 +18914,12 @@ S:	Maintained
 F:	Documentation/devicetree/bindings/serial/sunplus,sp7021-uart.yaml
 F:	drivers/tty/serial/sunplus-uart.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 57ca5f9..3103831 100644
@@ -340,6 +340,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 SP7021"
+	depends on SOC_SP7021
+	select PHY_SUNPLUS_USB
+	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 2948983..ab49fbb 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 +289 -0

289 lines not on this page · raw message

Signature · 1 line
2.7.4
#2 lore raw

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

Hi Vincent,

On Fr, 2022-04-29 at 16:27 +0800, Vincent Shih wrote:
[...]

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

Quoted 9 lines
drivers/usb/host/ehci-sunplus.c new file +9 -0
+static int sp_ehci_platform_power_on(struct platform_device *pdev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct sp_ehci_priv *sp_priv = hcd_to_sp_ehci_priv(hcd);
+	int ret;
+
+	ret = clk_prepare_enable(sp_priv->ehci_clk);
+	if (ret)
+		goto err_ehci_clk;

This should be:

		return ret;

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

Quoted 4 lines
drivers/usb/host/ehci-sunplus.c new file +4 -0
+
+	ret = reset_control_deassert(sp_priv->ehci_rstc);
+	if (ret)
+		goto err_ehci_reset;

And this should be:

		goto err_ehci_clk;
Quoted 32 lines
drivers/usb/host/ehci-sunplus.c new file +32 -0
+
+	ret = phy_init(sp_priv->phy);
+	if (ret)
+		goto err_ehci_reset;
+
+	ret = phy_power_on(sp_priv->phy);
+	if (ret)
+		goto err_phy_exit;
+
+	return 0;
+
+err_phy_exit:
+	phy_exit(sp_priv->phy);
+err_ehci_reset:
+	reset_control_assert(sp_priv->ehci_rstc);
+err_ehci_clk:
+	clk_disable_unprepare(sp_priv->ehci_clk);
+
+	return ret;
+}
+
+static void sp_ehci_platform_power_off(struct platform_device *pdev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct sp_ehci_priv *sp_priv = hcd_to_sp_ehci_priv(hcd);
+
+	phy_power_off(sp_priv->phy);
+	phy_exit(sp_priv->phy);
+
+	reset_control_assert(sp_priv->ehci_rstc);
+	clk_disable_unprepare(sp_priv->ehci_clk);
+}

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

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+
+static struct usb_ehci_pdata usb_ehci_pdata = {
+	.has_tt = 1,
+	.has_synopsys_hc_bug = 1,
+	.big_endian_desc = 1,
+	.big_endian_mmio = 1,
+	.power_on = sp_ehci_platform_power_on,
+	.power_suspend = sp_ehci_platform_power_off,
+	.power_off = sp_ehci_platform_power_off,
+

Superfluous whitespace.

regards
Philipp

maintainer
#3 lore raw

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

In reply to Vincent Shih
Quoted 6 lines · Vincent Shih

Add driver for ehci in Sunplus SP7021

---
Changes in v4:
- Implement power_on, power_off and power_suspend functions.

Did you test these? Did you try to rmmod the ehci-sunplus module after adding these functions?

Quoted 6 lines
drivers/usb/host/ehci-sunplus.c new file +0 -0
new file mode 100644
index 0000000..4d8e20d
@@ -0,0 +1,289 @@

...

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

Quoted 7 lines
drivers/usb/host/ehci-sunplus.c new file +7 -0
+static void sp_ehci_platform_power_off(struct platform_device *pdev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct sp_ehci_priv *sp_priv = hcd_to_sp_ehci_priv(hcd);
+
+	phy_power_off(sp_priv->phy);
+	phy_exit(sp_priv->phy);

Notice that this dereferences a field contained in the sp_ehci_priv extension to the usb_hcd structure.

...

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

Quoted 7 lines
drivers/usb/host/ehci-sunplus.c new file +7 -0
+static int ehci_sunplus_remove(struct platform_device *pdev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(pdev);
+	struct usb_ehci_pdata *pdata = pdev->dev.platform_data;
+
+	usb_remove_hcd(hcd);
+	usb_put_hcd(hcd);

This call deallocates the usb_hcd structure.

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

Quoted 3 lines
drivers/usb/host/ehci-sunplus.c new file +3 -0
+
+	if (pdata->power_off)
+		pdata->power_off(pdev);

But here you dereference a field that it contains. This power-off operation must occur _before_ the usb_put_hcd() call.

Alan Stern

maintainer
#4 lore raw

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

In reply to Vincent Shih
Quoted 23 lines · Vincent Shih

Add driver for ehci in Sunplus SP7021

---

drivers/usb/host/ehci-sunplus.c new file +12 -0
new file mode 100644
index 0000000..4d8e20d
 
+static struct usb_ehci_pdata usb_ehci_pdata = {
+	.has_tt = 1,
+	.has_synopsys_hc_bug = 1,
+	.big_endian_desc = 1,
+	.big_endian_mmio = 1,
+	.power_on = sp_ehci_platform_power_on,
+	.power_suspend = sp_ehci_platform_power_off,
+	.power_off = sp_ehci_platform_power_off,
+
+};
+
+static int ehci_sunplus_reset(struct usb_hcd *hcd)

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

Quoted 10 lines
drivers/usb/host/ehci-sunplus.c new file +10 -0
+{
+	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;

By the way, you don't need to add all this pdata stuff. You can just set hcd->has_tt, ehci->has_synopsys_hc_bug, ehci_big_endian_desc, and ehci->big_endian_mmio directly, since you already know what their values should be.

It looks like you simply copied the code from the ehci-platform driver. But that driver has to handle many different kinds of platform devices, so it needs to be told about their individual differences, whereas your driver only has to handle one kind.

In fact, there's no obvious reason why you didn't just use the ehci-platform driver instead of writing your own driver. That's the sort of thing you need to explain the patch description.

Alan Stern

#5 lore raw

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

In reply to Vincent Shih

Add bindings doc for Sunplus EHCI driver

---
Changes in v2:
- Address the comments from Mr. Krzysztof Kozlowski and Mr. Rob Herring

 .../bindings/usb/sunplus,sp7021-usb-ehci.yaml      | 79 ++++++++++++++++++++++
 MAINTAINERS                                        |  1 +
 2 files changed, 80 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml
Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml new file +79 -0
new file mode 100644
index 0000000..307b018
@@ -0,0 +1,79 @@
+# 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 Controller Device Tree Bindings
+
+allOf:
+  - $ref: usb-hcd.yaml#
+
+maintainers:
+  - Vincent Shih <[email protected]>
+
+description:
+  Sunplus SP7021 USB HOST IP is a USB2.0 Host Controller. It supports both
+  Enhanced Host Controller Interface (EHCI) and Open Host Controller Interface
+  (OHCI).
+
+  It supports 32-bits address bus and 64bit data bus interface, compliant
+  to AMBA AXI interface for data transfer.
+
+  It supports 32-bits address and data bus interface, compliant to AMBA
+  AHB interface for register configurations.
+
+  It supports 32-bits address and data bus interface, compliant to AMBA
+  AXI interface for register alternative configurations.
+
+  The UTM Interface block generates PHY control signals, compliant to
+  USB2.0 Transceiver Macrocell Interface Specification Revision 1.0.
+
+properties:
+  compatible:
+    const: sunplus,sp7021-usb-ehci
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    maxItems: 1
+
+  resets:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  phys:
+    maxItems: 1
+
+  phy-names:
+    const: uphy
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - resets
+  - interrupts
+  - phys
+  - phy-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    sp_ehci0: usb@9c102100 {
+      compatible = "sunplus,sp7021-usb-ehci";
+      reg = <0x9c102100 0x68>;
+      clocks = <&clkc 0x3a>;
+      resets = <&rstc 0x2a>;
+      interrupt-parent = <&intc>;
+      interrupts = <14 IRQ_TYPE_LEVEL_HIGH>;
+      phys = <&sp_uphy0>;
+      phy-names = "uphy";
+    };
MAINTAINERS +1 -0
index e103afa..a057288 100644
@@ -18918,6 +18918,7 @@ SUNPLUS USB EHCI DRIVER
 M:	Vincent Shih <[email protected]>
 L:	[email protected]
 S:	Maintained
+F:	Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml
 F:	drivers/usb/host/ehci-sunplus.c
 
 SUPERH
Signature · 1 line
2.7.4
#6 lore raw

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

Quoted 6 lines · Vincent Shih

Add bindings doc for Sunplus EHCI driver

---
Changes in v2:
- Address the comments from Mr. Krzysztof Kozlowski and Mr. Rob Herring

Improved, but still I don't think you applied all of my comments. Please go through them one more time.

https://lore.kernel.org/all/[email protected]/

Best regards,
Krzysztof