linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/2] Add driver for EHCI in Sunplus SP7021
@ 2022-04-29  8:27 Vincent Shih
  2022-04-29  8:27 ` [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci " Vincent Shih
  2022-04-29  8:27 ` [PATCH v4 2/2] dt-bindings: usb: Add bindings doc for Sunplus EHCI driver Vincent Shih
  0 siblings, 2 replies; 7+ messages in thread
From: Vincent Shih @ 2022-04-29  8:27 UTC (permalink / raw)
  To: gregkh, stern, p.zabel, davem, vladimir.oltean, linux-kernel,
	linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree, wells.lu
  Cc: Vincent Shih

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

-- 
2.7.4


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021
  2022-04-29  8:27 [PATCH v4 0/2] Add driver for EHCI in Sunplus SP7021 Vincent Shih
@ 2022-04-29  8:27 ` Vincent Shih
  2022-04-29 12:07   ` Philipp Zabel
                     ` (2 more replies)
  2022-04-29  8:27 ` [PATCH v4 2/2] dt-bindings: usb: Add bindings doc for Sunplus EHCI driver Vincent Shih
  1 sibling, 3 replies; 7+ messages in thread
From: Vincent Shih @ 2022-04-29  8:27 UTC (permalink / raw)
  To: gregkh, stern, p.zabel, davem, vladimir.oltean, linux-kernel,
	linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree, wells.lu
  Cc: Vincent Shih

Add driver for ehci in Sunplus SP7021

Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
---
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

diff --git a/MAINTAINERS b/MAINTAINERS
index dd61684..e103afa 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -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 <vincent.sunplus@gmail.com>
+L:	linux-usb@vger.kernel.org
+S:	Maintained
+F:	drivers/usb/host/ehci-sunplus.c
+
 SUPERH
 M:	Yoshinori Sato <ysato@users.sourceforge.jp>
 M:	Rich Felker <dalias@libc.org>
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 57ca5f9..3103831 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -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
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 2948983..ab49fbb 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -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
diff --git a/drivers/usb/host/ehci-sunplus.c b/drivers/usb/host/ehci-sunplus.c
new file mode 100644
index 0000000..4d8e20d
--- /dev/null
+++ b/drivers/usb/host/ehci-sunplus.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0
+
+/*
+ * The EHCI driver for Sunplus SP7021
+ *
+ * Copyright (C) 2022 Sunplus Technology Inc., All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/module.h>
+#include <linux/phy/phy.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/usb.h>
+#include <linux/usb/ehci_pdriver.h>
+#include <linux/usb/hcd.h>
+
+#include "ehci.h"
+
+#define hcd_to_sp_ehci_priv(h) \
+		((struct sp_ehci_priv *)hcd_to_ehci(h)->priv)
+
+struct sp_ehci_priv {
+	struct clk *ehci_clk;
+	struct reset_control *ehci_rstc;
+	struct phy *phy;
+	u32 disc_vol_addr_off;
+};
+
+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;
+
+	ret = reset_control_deassert(sp_priv->ehci_rstc);
+	if (ret)
+		goto err_ehci_reset;
+
+	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);
+}
+
+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)
+{
+	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;
+
+	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 ehci_sunplus_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct sp_ehci_priv *sp_priv;
+	struct resource *res_mem;
+	struct usb_ehci_pdata *pdata = &usb_ehci_pdata;
+	struct usb_hcd *hcd;
+	int irq, ret;
+
+	if (usb_disabled())
+		return -ENODEV;
+
+	pdev->dev.platform_data = pdata;
+
+	/* 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);
+
+	/* phy */
+	sp_priv->phy = devm_phy_get(dev, "uphy");
+	if (IS_ERR(sp_priv->phy)) {
+		dev_err(dev, "no USB phy configured\n");
+		ret = PTR_ERR(sp_priv->phy);
+		goto err_put_hcd;
+	}
+
+	/* ehci */
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	hcd->regs = devm_ioremap_resource(dev, res_mem);
+	if (IS_ERR(hcd->regs)) {
+		ret = PTR_ERR(hcd->regs);
+		goto err_put_hcd;
+	}
+
+	hcd->rsrc_start = res_mem->start;
+	hcd->rsrc_len = resource_size(res_mem);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		ret = irq;
+		goto err_put_hcd;
+	}
+	dev_dbg(&pdev->dev, "ehci_id:%d,irq:%d\n", pdev->id, irq);
+
+	sp_priv->ehci_clk = devm_clk_get(dev, NULL);
+
+	if (IS_ERR(sp_priv->ehci_clk)) {
+		ret = PTR_ERR(sp_priv->ehci_clk);
+		goto err_put_hcd;
+	}
+
+	sp_priv->ehci_rstc = devm_reset_control_get_exclusive(dev, NULL);
+	if (IS_ERR(sp_priv->ehci_rstc)) {
+		ret = PTR_ERR(sp_priv->ehci_rstc);
+		goto err_put_hcd;
+	}
+
+	if (pdata->power_on) {
+		ret = pdata->power_on(pdev);
+		if (ret < 0)
+			goto err_put_hcd;
+	}
+
+	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
+	if (ret)
+		goto err_put_hcd;
+
+	dev_dbg(dev, "hcd_irq:%d,%d\n", hcd->irq, irq);
+
+	return ret;
+
+err_put_hcd:
+	usb_put_hcd(hcd);
+
+	return ret;
+}
+
+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);
+
+	if (pdata->power_off)
+		pdata->power_off(pdev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+static int ehci_sunplus_drv_suspend(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct platform_device *pdev = to_platform_device(dev);
+	struct usb_ehci_pdata *pdata = pdev->dev.platform_data;
+	bool do_wakeup = device_may_wakeup(dev);
+	int rc;
+
+	rc = ehci_suspend(hcd, do_wakeup);
+	if (rc)
+		return rc;
+
+	if (pdata->power_suspend)
+		pdata->power_suspend(pdev);
+
+	return 0;
+}
+
+static int ehci_sunplus_drv_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct platform_device *pdev = to_platform_device(dev);
+	struct usb_ehci_pdata *pdata = pdev->dev.platform_data;
+	int ret;
+
+	if (pdata->power_on) {
+		ret = pdata->power_on(pdev);
+		if (ret < 0)
+			return ret;
+	}
+
+	ehci_resume(hcd, false);
+
+	return 0;
+}
+
+static const struct dev_pm_ops ehci_sunplus_pm_ops = {
+	.suspend = ehci_sunplus_drv_suspend,
+	.resume = ehci_sunplus_drv_resume,
+};
+#endif
+
+static const struct of_device_id ehci_sunplus_dt_ids[] = {
+	{ .compatible = "sunplus,sp7021-usb-ehci" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, ehci_sunplus_dt_ids);
+
+static struct platform_driver ehci_hcd_sunplus_driver = {
+	.probe			= ehci_sunplus_probe,
+	.remove			= ehci_sunplus_remove,
+	.shutdown		= usb_hcd_platform_shutdown,
+	.driver = {
+		.name		= "ehci-sunplus",
+		.of_match_table = ehci_sunplus_dt_ids,
+#ifdef CONFIG_PM
+		.pm = &ehci_sunplus_pm_ops,
+#endif
+	}
+};
+
+static int __init ehci_sunplus_init(void)
+{
+	if (usb_disabled())
+		return -ENODEV;
+
+	ehci_init_driver(&ehci_sunplus_driver, &ehci_sunplus_overrides);
+	return platform_driver_register(&ehci_hcd_sunplus_driver);
+}
+module_init(ehci_sunplus_init);
+
+static void __exit ehci_sunplus_cleanup(void)
+{
+	platform_driver_unregister(&ehci_hcd_sunplus_driver);
+}
+module_exit(ehci_sunplus_cleanup);
+
+MODULE_AUTHOR("Vincent Shih <vincent.sunplus@gmail.com>");
+MODULE_DESCRIPTION("Sunplus USB EHCI driver");
+MODULE_LICENSE("GPL");
+
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v4 2/2] dt-bindings: usb: Add bindings doc for Sunplus EHCI driver
  2022-04-29  8:27 [PATCH v4 0/2] Add driver for EHCI in Sunplus SP7021 Vincent Shih
  2022-04-29  8:27 ` [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci " Vincent Shih
@ 2022-04-29  8:27 ` Vincent Shih
  2022-04-29 20:53   ` Krzysztof Kozlowski
  1 sibling, 1 reply; 7+ messages in thread
From: Vincent Shih @ 2022-04-29  8:27 UTC (permalink / raw)
  To: gregkh, stern, p.zabel, davem, vladimir.oltean, linux-kernel,
	linux-usb, robh+dt, krzysztof.kozlowski+dt, devicetree, wells.lu
  Cc: Vincent Shih

Add bindings doc for Sunplus EHCI driver

Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
---
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

diff --git a/Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml b/Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml
new file mode 100644
index 0000000..307b018
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml
@@ -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 <vincent.sunplus@gmail.com>
+
+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";
+    };
diff --git a/MAINTAINERS b/MAINTAINERS
index e103afa..a057288 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -18918,6 +18918,7 @@ SUNPLUS USB EHCI DRIVER
 M:	Vincent Shih <vincent.sunplus@gmail.com>
 L:	linux-usb@vger.kernel.org
 S:	Maintained
+F:	Documentation/devicetree/bindings/usb/sunplus,sp7021-usb-ehci.yaml
 F:	drivers/usb/host/ehci-sunplus.c
 
 SUPERH
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021
  2022-04-29  8:27 ` [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci " Vincent Shih
@ 2022-04-29 12:07   ` Philipp Zabel
  2022-04-29 15:37   ` Alan Stern
  2022-04-30  1:26   ` Alan Stern
  2 siblings, 0 replies; 7+ messages in thread
From: Philipp Zabel @ 2022-04-29 12:07 UTC (permalink / raw)
  To: Vincent Shih, gregkh, stern, davem, vladimir.oltean,
	linux-kernel, linux-usb, robh+dt, krzysztof.kozlowski+dt,
	devicetree, wells.lu

Hi Vincent,

On Fr, 2022-04-29 at 16:27 +0800, Vincent Shih wrote:
[...]
> +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;

> +
> +	ret = reset_control_deassert(sp_priv->ehci_rstc);
> +	if (ret)
> +		goto err_ehci_reset;

And this should be:

		goto err_ehci_clk;

> +
> +	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);
> +}
> +
> +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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021
  2022-04-29  8:27 ` [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci " Vincent Shih
  2022-04-29 12:07   ` Philipp Zabel
@ 2022-04-29 15:37   ` Alan Stern
  2022-04-30  1:26   ` Alan Stern
  2 siblings, 0 replies; 7+ messages in thread
From: Alan Stern @ 2022-04-29 15:37 UTC (permalink / raw)
  To: Vincent Shih
  Cc: gregkh, p.zabel, davem, vladimir.oltean, linux-kernel, linux-usb,
	robh+dt, krzysztof.kozlowski+dt, devicetree, wells.lu

On Fri, Apr 29, 2022 at 04:27:55PM +0800, Vincent Shih wrote:
> Add driver for ehci in Sunplus SP7021
> 
> Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
> ---
> 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?

> diff --git a/drivers/usb/host/ehci-sunplus.c b/drivers/usb/host/ehci-sunplus.c
> new file mode 100644
> index 0000000..4d8e20d
> --- /dev/null
> +++ b/drivers/usb/host/ehci-sunplus.c
> @@ -0,0 +1,289 @@

...

> +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.

...

> +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.

> +
> +	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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 2/2] dt-bindings: usb: Add bindings doc for Sunplus EHCI driver
  2022-04-29  8:27 ` [PATCH v4 2/2] dt-bindings: usb: Add bindings doc for Sunplus EHCI driver Vincent Shih
@ 2022-04-29 20:53   ` Krzysztof Kozlowski
  0 siblings, 0 replies; 7+ messages in thread
From: Krzysztof Kozlowski @ 2022-04-29 20:53 UTC (permalink / raw)
  To: Vincent Shih, gregkh, stern, p.zabel, davem, vladimir.oltean,
	linux-kernel, linux-usb, robh+dt, krzysztof.kozlowski+dt,
	devicetree, wells.lu

On 29/04/2022 10:27, Vincent Shih wrote:
> Add bindings doc for Sunplus EHCI driver
> 
> Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
> ---
> 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/67ec6f3a-1579-f77a-a54d-381472252d6b@canonical.com/


Best regards,
Krzysztof

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci in Sunplus SP7021
  2022-04-29  8:27 ` [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci " Vincent Shih
  2022-04-29 12:07   ` Philipp Zabel
  2022-04-29 15:37   ` Alan Stern
@ 2022-04-30  1:26   ` Alan Stern
  2 siblings, 0 replies; 7+ messages in thread
From: Alan Stern @ 2022-04-30  1:26 UTC (permalink / raw)
  To: Vincent Shih
  Cc: gregkh, p.zabel, davem, vladimir.oltean, linux-kernel, linux-usb,
	robh+dt, krzysztof.kozlowski+dt, devicetree, wells.lu

On Fri, Apr 29, 2022 at 04:27:55PM +0800, Vincent Shih wrote:
> Add driver for ehci in Sunplus SP7021
> 
> Signed-off-by: Vincent Shih <vincent.sunplus@gmail.com>
> ---

> diff --git a/drivers/usb/host/ehci-sunplus.c b/drivers/usb/host/ehci-sunplus.c
> new file mode 100644
> index 0000000..4d8e20d
> --- /dev/null
> +++ b/drivers/usb/host/ehci-sunplus.c

> +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)
> +{
> +	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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-04-30  1:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-29  8:27 [PATCH v4 0/2] Add driver for EHCI in Sunplus SP7021 Vincent Shih
2022-04-29  8:27 ` [PATCH v4 1/2] usb: host: ehci-sunplus: Add driver for ehci " Vincent Shih
2022-04-29 12:07   ` Philipp Zabel
2022-04-29 15:37   ` Alan Stern
2022-04-30  1:26   ` Alan Stern
2022-04-29  8:27 ` [PATCH v4 2/2] dt-bindings: usb: Add bindings doc for Sunplus EHCI driver Vincent Shih
2022-04-29 20:53   ` Krzysztof Kozlowski

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).