[PATCH 0/3] Add support for Fujitsu USB host controller

[PATCH 0/3] Add support for Fujitsu USB host controller

[Sneeker Yeh]

These patches add support for EHCI and XHCI compliant Host controller found
on Fujitsu Socs, and are based on
http://www.spinics.net/lists/arm-kernel/msg385573.html

First patch is EHCI platform glue driver. Patch 2 introduces Fujitsu
Specific Glue layer in Synopsis DesignWare USB3 IP driver. Patch 3
introduces a quirk with device disconnection management necessary for IPs
using the Synopsys Designware dwc3 core. It solves a problem where without
the quirk, that host controller will die after a usb device is disconnected
from port of root hub, which happened in Synopsis core with Fujitsu
config-free usb phy integrated.

Successfully tested on Fujitsu mb86s7x board.

Sneeker Yeh (3):
  usb: host: f_usb20ho: add support for Fujitsu ehci/ohci USB 2.0 host
    controller
  usb: dwc3: add Fujitsu Specific Glue layer
  usb: dwc3: add a quirk for device disconnection issue in Synopsis
    dwc3 core

 .../devicetree/bindings/usb/fujitsu-dwc3.txt       |   25 ++
 .../devicetree/bindings/usb/fujitsu-ehci.txt       |   22 ++
 drivers/usb/dwc3/Kconfig                           |   11 +
 drivers/usb/dwc3/Makefile                          |    1 +
 drivers/usb/dwc3/dwc3-mb86s70.c                    |  194 +++++++++++++
 drivers/usb/host/Kconfig                           |   11 +
 drivers/usb/host/Makefile                          |    1 +
 drivers/usb/host/f_usb20ho_hcd.c                   |  306 ++++++++++++++++++++
 drivers/usb/host/f_usb20ho_hcd.h                   |   35 +++
 drivers/usb/host/xhci-hub.c                        |    4 +
 drivers/usb/host/xhci-plat.c                       |    5 +
 drivers/usb/host/xhci.c                            |   29 ++
 drivers/usb/host/xhci.h                            |    7 +
 13 files changed, 651 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
 create mode 100644 Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
 create mode 100644 drivers/usb/dwc3/dwc3-mb86s70.c
 create mode 100644 drivers/usb/host/f_usb20ho_hcd.c
 create mode 100644 drivers/usb/host/f_usb20ho_hcd.h

-- 
1.7.9.5

[PATCH 1/3] usb: host: f_usb20ho: add support for Fujitsu ehci/ohci USB 2.0 host controller

[Sneeker Yeh]

This patch adds support for EHCI compliant Host controller found
on Fujitsu Socs.

Signed-off-by: Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>
---
 .../devicetree/bindings/usb/fujitsu-ehci.txt       |   22 ++
 drivers/usb/host/Kconfig                           |   11 +
 drivers/usb/host/Makefile                          |    1 +
 drivers/usb/host/f_usb20ho_hcd.c                   |  306 ++++++++++++++++++++
 drivers/usb/host/f_usb20ho_hcd.h                   |   35 +++
 5 files changed, 375 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
 create mode 100644 drivers/usb/host/f_usb20ho_hcd.c
 create mode 100644 drivers/usb/host/f_usb20ho_hcd.h

diff --git a/Documentation/devicetree/bindings/usb/fujitsu-ehci.txt b/Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
new file mode 100644
index 0000000..e180860
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
@@ -0,0 +1,22 @@
+FUJITSU GLUE COMPONENTS
+
+MB86S7x EHCI GLUE
+ - compatible : Should be "fujitsu,f_usb20ho_hcd"
+ - reg : Address and length of the register set for the device.
+ - interrupts : The irq number of this device that is used to interrupt the
+   CPU
+ - clocks: from common clock binding, handle to usb clock.
+ - clock-names: from common clock binding.
+ - #stream-id-cells : handle to use "arm,mmu-400" ARM IOMMU driver
+ - fujitsu,power-domain : pd_usb2h node has to be builded, details can be
+   found in:
+   Documentation/devicetree/bindings/
+
+hcd21: f_usb20ho_hcd {
+	compatible = "fujitsu,f_usb20ho_hcd";
+	reg = <0 0x34200000 0x80000>;
+	interrupts = <0 419 0x4>;
+	clocks = <&clk_main_2_4>, <&clk_main_4_5>, <&clk_usb_0_0>;
+	clock-names = "h_clk", "p_clk", "p_cryclk";
+	#stream-id-cells = <0>;
+};
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index fafc628..9482140 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -786,6 +786,17 @@ config USB_HCD_SSB
 
 	  If unsure, say N.
 
+config USB_F_USB20HO_HCD
+	tristate "F_USB20HO USB Host Controller"
+	depends on USB && ARCH_MB86S7X
+	default y
+	help
+	  This driver enables support for USB20HO USB Host Controller,
+	  the driver supports High, Full and Low Speed USB.
+
+	  To compile this driver a module, choose M here: the module
+	  will be called "f_usb20ho-hcd".
+
 config USB_HCD_TEST_MODE
 	bool "HCD test mode support"
 	---help---
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index d6216a4..b89e536 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -78,3 +78,4 @@ obj-$(CONFIG_USB_HCD_SSB)	+= ssb-hcd.o
 obj-$(CONFIG_USB_FUSBH200_HCD)	+= fusbh200-hcd.o
 obj-$(CONFIG_USB_FOTG210_HCD)	+= fotg210-hcd.o
 obj-$(CONFIG_USB_MAX3421_HCD)	+= max3421-hcd.o
+obj-$(CONFIG_USB_F_USB20HO_HCD)+= f_usb20ho_hcd.o
diff --git a/drivers/usb/host/f_usb20ho_hcd.c b/drivers/usb/host/f_usb20ho_hcd.c
new file mode 100644
index 0000000..d665487
--- /dev/null
+++ b/drivers/usb/host/f_usb20ho_hcd.c
@@ -0,0 +1,306 @@
+/**
+ * f_usb20ho_hcd.c - Fujitsu EHCI platform driver
+ *
+ * Copyright (c) 2013 - 2014 FUJITSU SEMICONDUCTOR LIMITED
+ *		http://jp.fujitsu.com/group/fsl
+ *
+ * based on bcma-hcd.c
+ *
+ * Author: Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/errno.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/usb/ehci_pdriver.h>
+#include <linux/usb/ohci_pdriver.h>
+#include <linux/dma-mapping.h>
+#include <linux/of.h>
+#include <linux/pm_runtime.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+#include "f_usb20ho_hcd.h"
+
+static int f_usb20ho_clk_control(struct device *dev, bool on)
+{
+	int ret, i;
+	struct clk *clk;
+
+	if (!on)
+		goto clock_off;
+
+	for (i = 0;; i++) {
+		clk = of_clk_get(dev->of_node, i);
+		if (IS_ERR(clk))
+			break;
+
+		ret = clk_prepare_enable(clk);
+		if (ret) {
+			dev_err(dev, "failed to enable clock[%d]\n", i);
+			goto clock_off;
+		}
+	}
+
+	return 0;
+
+clock_off:
+	for (i = 0;; i++) {
+		clk = of_clk_get(dev->of_node, i);
+		if (IS_ERR(clk))
+			break;
+
+		clk_disable_unprepare(clk);
+	}
+
+	return on;
+}
+
+static struct platform_device *f_usb20ho_hcd_create_pdev(
+		struct platform_device *pdev, bool ohci)
+{
+	struct resource *resource;
+	struct platform_device *hci_dev;
+	struct resource hci_res[2];
+	int ret = -ENOMEM;
+	int irq;
+	resource_size_t resource_size;
+
+	memset(hci_res, 0, sizeof(hci_res));
+
+	resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!resource) {
+		dev_err(&pdev->dev, "%s() platform_get_resource() failed\n"
+			, __func__);
+		ret = -ENODEV;
+		goto err_res;
+	}
+	resource_size = resource->end - resource->start + 1;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev,
+			"%s() platform_get_irq() for F_USB20HO failed at %d\n",
+			__func__, irq);
+		ret = -ENODEV;
+		goto err_res;
+	}
+
+	hci_res[0].start = ohci ? resource->start + F_OHCI_OFFSET :
+		resource->start + F_EHCI_OFFSET;
+	hci_res[0].end = ohci ?
+			resource->start + F_OHCI_OFFSET + F_OHCI_SIZE - 1 :
+			resource->start + F_EHCI_OFFSET + F_EHCI_SIZE - 1;
+	hci_res[0].flags = IORESOURCE_MEM;
+
+	hci_res[1].start = irq;
+	hci_res[1].flags = IORESOURCE_IRQ;
+
+	hci_dev = platform_device_alloc(ohci ? "ohci-platform" :
+					"ehci-platform", 0);
+	if (!hci_dev) {
+		dev_err(&pdev->dev, "platform_device_alloc() failed\n");
+		ret = -ENODEV;
+		goto err_res;
+	}
+
+	dma_set_coherent_mask(&hci_dev->dev, pdev->dev.coherent_dma_mask);
+	hci_dev->dev.parent = &pdev->dev;
+	hci_dev->dev.dma_mask = pdev->dev.dma_mask;
+
+	ret = platform_device_add_resources(hci_dev, hci_res,
+					    ARRAY_SIZE(hci_res));
+	if (ret) {
+		dev_err(&pdev->dev
+			, "platform_device_add_resources() failed\n");
+		goto err_alloc;
+	}
+
+	ret = platform_device_add(hci_dev);
+	if (ret) {
+		dev_err(&pdev->dev, "platform_device_add() failed\n");
+		goto err_alloc;
+	}
+
+	return hci_dev;
+
+err_alloc:
+	platform_device_put(hci_dev);
+err_res:
+	return ERR_PTR(ret);
+}
+
+static u64 f_usb20ho_dma_mask =  DMA_BIT_MASK(32);
+
+static int f_usb20ho_hcd_probe(struct platform_device *pdev)
+{
+	int err;
+	struct f_usb20ho_hcd *usb_dev;
+	struct device *dev = &pdev->dev;
+
+	dev->dma_mask = &f_usb20ho_dma_mask;
+	if (!dev->coherent_dma_mask)
+		dev->coherent_dma_mask = DMA_BIT_MASK(32);
+
+	usb_dev = kzalloc(sizeof(*usb_dev), GFP_KERNEL);
+	if (!usb_dev)
+		return -ENOMEM;
+
+	usb_dev->dev = &pdev->dev;
+	platform_set_drvdata(pdev, usb_dev);
+
+	usb_dev->irq = platform_get_irq(pdev, 0);
+	if (usb_dev->irq < 0) {
+		dev_err(&pdev->dev,
+			"%s() platform_get_irq() for F_USB20HO failed at %d\n",
+			__func__, usb_dev->irq);
+		err = -ENODEV;
+		goto err_free_usb_dev;
+	}
+	disable_irq(usb_dev->irq);
+
+	/* resume driver for clock, power, irq */
+	pm_runtime_enable(&pdev->dev);
+	err = pm_runtime_get_sync(&pdev->dev);
+	if (err < 0) {
+		dev_err(&pdev->dev, "get_sync failed with err %d\n", err);
+		goto err_unregister_ohci_dev;
+	}
+
+	usb_dev->ehci_dev = f_usb20ho_hcd_create_pdev(pdev, false);
+	if (IS_ERR(usb_dev->ehci_dev)) {
+		dev_err(&pdev->dev, "failed to create EHCI driver\n");
+		err = -ENODEV;
+		goto err_free_usb_dev;
+	}
+
+	usb_dev->ohci_dev = f_usb20ho_hcd_create_pdev(pdev, true);
+	if (IS_ERR(usb_dev->ohci_dev)) {
+		dev_err(&pdev->dev, "failed to create OHCI driver\n");
+		err = -ENODEV;
+		goto err_unregister_ehci_dev;
+	}
+
+	return 0;
+
+err_unregister_ohci_dev:
+	platform_device_unregister(usb_dev->ohci_dev);
+err_unregister_ehci_dev:
+	platform_device_unregister(usb_dev->ehci_dev);
+	pm_runtime_put_sync(&pdev->dev);
+err_free_usb_dev:
+	kfree(usb_dev);
+
+	return err;
+}
+
+static int f_usb20ho_hcd_remove(struct platform_device *pdev)
+{
+	struct f_usb20ho_hcd *usb_dev = platform_get_drvdata(pdev);
+	struct platform_device *ohci_dev = usb_dev->ohci_dev;
+	struct platform_device *ehci_dev = usb_dev->ehci_dev;
+
+	if (ohci_dev)
+		platform_device_unregister(ohci_dev);
+	if (ehci_dev)
+		platform_device_unregister(ehci_dev);
+
+	/* disable power,clock,irq */
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+#ifdef CONFIG_PM
+#ifdef CONFIG_PM_RUNTIME
+static int  f_usb20ho_runtime_suspend(struct device *dev)
+{
+	struct f_usb20ho_hcd *usb_dev = dev_get_drvdata(dev);
+
+	disable_irq(usb_dev->irq);
+	f_usb20ho_clk_control(dev, false);
+
+	return 0;
+}
+
+static int  f_usb20ho_runtime_resume(struct device *dev)
+{
+	struct f_usb20ho_hcd *usb_dev = dev_get_drvdata(dev);
+
+	f_usb20ho_clk_control(dev, true);
+	enable_irq(usb_dev->irq);
+
+	return 0;
+}
+#endif /* CONFIG_PM_RUNTIME */
+
+static int f_usb20ho_hcd_suspend(struct device *dev)
+{
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
+	return f_usb20ho_runtime_suspend(dev);
+}
+
+static int f_usb20ho_hcd_resume(struct device *dev)
+{
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
+	return f_usb20ho_runtime_resume(dev);
+}
+
+static const struct dev_pm_ops f_usb20ho_hcd_ops = {
+	.suspend =  f_usb20ho_hcd_suspend,
+	.resume = f_usb20ho_hcd_resume,
+	SET_RUNTIME_PM_OPS(f_usb20ho_runtime_suspend
+		, f_usb20ho_runtime_resume, NULL)
+};
+
+#define DEV_PM (&f_usb20ho_hcd_ops)
+#else /* !CONFIG_PM */
+#define DEV_PM	NULL
+#endif /* CONFIG_PM */
+
+static void f_usb20ho_hcd_shutdown(struct platform_device *pdev)
+{
+#ifdef CONFIG_PM
+	 f_usb20ho_hcd_suspend(&pdev->dev);
+#endif
+}
+
+static const struct of_device_id f_usb20ho_hcd_dt_ids[] = {
+	{ .compatible = "fujitsu,f_usb20ho_hcd" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, f_usb20ho_hcd_dt_ids);
+
+static struct platform_driver f_usb20ho_hcd_driver = {
+	.probe		= f_usb20ho_hcd_probe,
+	.remove		= __exit_p(f_usb20ho_hcd_remove),
+	.shutdown	= f_usb20ho_hcd_shutdown,
+	.driver = {
+		.name = "f_usb20ho_hcd",
+		.owner = THIS_MODULE,
+		.pm = DEV_PM,
+		.of_match_table = f_usb20ho_hcd_dt_ids,
+	},
+};
+
+module_platform_driver(f_usb20ho_hcd_driver);
+
+MODULE_ALIAS("platform:f_usb20ho_hcd");
+MODULE_AUTHOR("Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("USB platform driver for f_usb20ho_lap IP ");
diff --git a/drivers/usb/host/f_usb20ho_hcd.h b/drivers/usb/host/f_usb20ho_hcd.h
new file mode 100644
index 0000000..046c636
--- /dev/null
+++ b/drivers/usb/host/f_usb20ho_hcd.h
@@ -0,0 +1,35 @@
+/*
+ * linux/drivers/usb/host/f_usb20ho_hcd.h - F_USB20HDC USB
+ * host controller driver
+ *
+ * Copyright (C) FUJITSU ELECTRONICS INC. 2011. All rights reserved.
+ * Copyright (C) 2013 - 2014 FUJITSU SEMICONDUCTOR LIMITED.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version 2
+ * of the License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _F_USB20HO_HCD_H
+#define _F_USB20HO_HCD_H
+
+#define F_EHCI_OFFSET	0x40000
+#define F_EHCI_SIZE		0x1000
+#define F_OHCI_OFFSET	0x41000
+#define F_OHCI_SIZE		0x1000
+#define F_OTHER_OFFSET	0x42000
+#define F_OTHER_SIZE		0x1000
+
+struct f_usb20ho_hcd {
+	struct device *dev;
+	struct platform_device *ehci_dev;
+	struct platform_device *ohci_dev;
+	int irq;
+};
+#endif
-- 
1.7.9.5

Re: [PATCH 1/3] usb: host: f_usb20ho: add support for Fujitsu ehci/ohci USB 2.0 host controller

[Arnd Bergmann]

On Tuesday 16 December 2014 10:10:26 Sneeker Yeh wrote:
> This patch adds support for EHCI compliant Host controller found
> on Fujitsu Socs.
> 
> Signed-off-by: Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>
> ---
>  .../devicetree/bindings/usb/fujitsu-ehci.txt       |   22 ++
>  drivers/usb/host/Kconfig                           |   11 +
>  drivers/usb/host/Makefile                          |    1 +
>  drivers/usb/host/f_usb20ho_hcd.c                   |  306 ++++++++++++++++++++
>  drivers/usb/host/f_usb20ho_hcd.h                   |   35 +++
>  5 files changed, 375 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
>  create mode 100644 drivers/usb/host/f_usb20ho_hcd.c
>  create mode 100644 drivers/usb/host/f_usb20ho_hcd.h
> 
> diff --git a/Documentation/devicetree/bindings/usb/fujitsu-ehci.txt b/Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
> new file mode 100644
> index 0000000..e180860
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
> @@ -0,0 +1,22 @@
> +FUJITSU GLUE COMPONENTS
> +
> +MB86S7x EHCI GLUE
> + - compatible : Should be "fujitsu,f_usb20ho_hcd"

Please try to use the binding from

Documentation/devicetree/bindings/usb/usb-ehci.txt and the respective
ohci binding first, and use two separate devic enodes.

> + - reg : Address and length of the register set for the device.
> + - interrupts : The irq number of this device that is used to interrupt the
> +   CPU
> + - clocks: from common clock binding, handle to usb clock.
> + - clock-names: from common clock binding.

You should always document the specific strings for a named property.

> + - #stream-id-cells : handle to use "arm,mmu-400" ARM IOMMU driver

Don't use that binding, we are trying to kill that off. Instead, use
an 'iommus' property.


> +	hci_res[0].start = ohci ? resource->start + F_OHCI_OFFSET :
> +		resource->start + F_EHCI_OFFSET;
> +	hci_res[0].end = ohci ?
> +			resource->start + F_OHCI_OFFSET + F_OHCI_SIZE - 1 :
> +			resource->start + F_EHCI_OFFSET + F_EHCI_SIZE - 1;
> +	hci_res[0].flags = IORESOURCE_MEM;
> +
> +	hci_res[1].start = irq;
> +	hci_res[1].flags = IORESOURCE_IRQ;
> +
> +	hci_dev = platform_device_alloc(ohci ? "ohci-platform" :
> +					"ehci-platform", 0);
> +	if (!hci_dev) {
> +		dev_err(&pdev->dev, "platform_device_alloc() failed\n");
> +		ret = -ENODEV;
> +		goto err_res;
> +	}

No need for playing games with child devices, just see how the other
drivers do it.

> +	ret = platform_device_add(hci_dev);
> +	if (ret) {
> +		dev_err(&pdev->dev, "platform_device_add() failed\n");
> +		goto err_alloc;
> +	}
> +
> +	return hci_dev;
> +
> +err_alloc:
> +	platform_device_put(hci_dev);
> +err_res:
> +	return ERR_PTR(ret);
> +}
> +
> +static u64 f_usb20ho_dma_mask =  DMA_BIT_MASK(32);

The dma mask should come from the dma-ranges property of the parent bus,
as of_platform_populate now does.

	Arnd

RE: [PATCH 1/3] usb: host: f_usb20ho: add support for Fujitsu ehci/ohci USB 2.0 host controller

[Sneeker.Yeh]

>-----Original Message-----
>From: Arnd Bergmann [mailto:arnd@arndb.de]
>Sent: Tuesday, December 16, 2014 5:06 PM
>To: Sneeker Yeh
>Cc: Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar Gala; Felipe
>Balbi; Greg Kroah-Hartman; Mathias Nyman; Grant Likely; Alan Stern; Paul
>Bolle; Hans de Goede; Thomas Pugliese; David Mosberger; Peter Griffin;
>Sylwester Nawrocki; Andrew Bresticker; Gregory CLEMENT; Yoshihiro Shimoda;
>devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; Andy Green; Jassi Brar;
>Sneeker Yeh
>Subject: Re: [PATCH 1/3] usb: host: f_usb20ho: add support for Fujitsu ehci/ohci
>USB 2.0 host controller
>
>On Tuesday 16 December 2014 10:10:26 Sneeker Yeh wrote:
>> This patch adds support for EHCI compliant Host controller found on
>> Fujitsu Socs.
>>
>> Signed-off-by: Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>
>> ---
>>  .../devicetree/bindings/usb/fujitsu-ehci.txt       |   22 ++
>>  drivers/usb/host/Kconfig                           |   11 +
>>  drivers/usb/host/Makefile                          |    1 +
>>  drivers/usb/host/f_usb20ho_hcd.c                   |  306
>++++++++++++++++++++
>>  drivers/usb/host/f_usb20ho_hcd.h                   |   35 +++
>>  5 files changed, 375 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
>>  create mode 100644 drivers/usb/host/f_usb20ho_hcd.c  create mode
>> 100644 drivers/usb/host/f_usb20ho_hcd.h
>>
>> diff --git a/Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
>> b/Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
>> new file mode 100644
>> index 0000000..e180860
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/fujitsu-ehci.txt
>> @@ -0,0 +1,22 @@
>> +FUJITSU GLUE COMPONENTS
>> +
>> +MB86S7x EHCI GLUE
>> + - compatible : Should be "fujitsu,f_usb20ho_hcd"
>
>Please try to use the binding from
>
>Documentation/devicetree/bindings/usb/usb-ehci.txt and the respective ohci
>binding first, and use two separate devic enodes.

Okay.

>
>> + - reg : Address and length of the register set for the device.
>> + - interrupts : The irq number of this device that is used to interrupt the
>> +   CPU
>> + - clocks: from common clock binding, handle to usb clock.
>> + - clock-names: from common clock binding.
>
>You should always document the specific strings for a named property.

Okay, I see.

>
>> + - #stream-id-cells : handle to use "arm,mmu-400" ARM IOMMU driver
>
>Don't use that binding, we are trying to kill that off. Instead, use an 'iommus'
>property.

Okay.

>
>
>> +     hci_res[0].start = ohci ? resource->start + F_OHCI_OFFSET :
>> +             resource->start + F_EHCI_OFFSET;
>> +     hci_res[0].end = ohci ?
>> +                     resource->start + F_OHCI_OFFSET + F_OHCI_SIZE -
>1 :
>> +                     resource->start + F_EHCI_OFFSET + F_EHCI_SIZE -
>1;
>> +     hci_res[0].flags = IORESOURCE_MEM;
>> +
>> +     hci_res[1].start = irq;
>> +     hci_res[1].flags = IORESOURCE_IRQ;
>> +
>> +     hci_dev = platform_device_alloc(ohci ? "ohci-platform" :
>> +                                     "ehci-platform", 0);
>> +     if (!hci_dev) {
>> +             dev_err(&pdev->dev, "platform_device_alloc() failed\n");
>> +             ret = -ENODEV;
>> +             goto err_res;
>> +     }
>
>No need for playing games with child devices, just see how the other drivers do
>it.

I am wondering if this is what you mentioned here? :
Hcd21: f_usb20ho {
        compatible = "fujitsu,f_usb20ho";
        ...
        Ehci {
                compatible = "generic-ehci";
                reg = <>;
        }
        ohci {
                compatible = "generic-ohci";
                reg = <>;
        }
}
By doing that, I can just use of_platform_populate() to create platform device more gracefully,
than using platform_device_xxx() those have been done in of_platform_populate().
And also, managing register offset is totally done via dts description, not in the code.

>
>> +     ret = platform_device_add(hci_dev);
>> +     if (ret) {
>> +             dev_err(&pdev->dev, "platform_device_add() failed\n");
>> +             goto err_alloc;
>> +     }
>> +
>> +     return hci_dev;
>> +
>> +err_alloc:
>> +     platform_device_put(hci_dev);
>> +err_res:
>> +     return ERR_PTR(ret);
>> +}
>> +
>> +static u64 f_usb20ho_dma_mask =  DMA_BIT_MASK(32);
>
>The dma mask should come from the dma-ranges property of the parent bus,
>as of_platform_populate now does.

Okay, I see.
Thanks for reviewing these patiently.

Much appreciate,
Sneeker

>
>        Arnd
>
>-----
>未在此訊息中找到病毒。
>已透過 AVG 檢查 - www.avg.com
>版本: 2012.0.2249 / 病毒庫: 4235/8239 - 發佈日期: 12/15/14

Re: [PATCH 1/3] usb: host: f_usb20ho: add support for Fujitsu ehci/ohci USB 2.0 host controller

[Arnd Bergmann]

On Wednesday 17 December 2014 23:33:02 Sneeker. Yeh wrote:
> >
> >No need for playing games with child devices, just see how the other drivers do
> >it.
> 
> I am wondering if this is what you mentioned here? :
> Hcd21: f_usb20ho {
>         compatible = "fujitsu,f_usb20ho";
>         ...
>         Ehci {
>                 compatible = "generic-ehci";
>                 reg = <>;
>         }
>         ohci {
>                 compatible = "generic-ohci";
>                 reg = <>;
>         }
> }
> By doing that, I can just use of_platform_populate() to create platform device more gracefully,
> than using platform_device_xxx() those have been done in of_platform_populate().
> And also, managing register offset is totally done via dts description, not in the code.

What I mean is leaving out the fujitsu,f_usb20ho node entirely, just put the
two USB controllers on the bus directly and remove this driver.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 1/3] usb: host: f_usb20ho: add support for Fujitsu ehci/ohci USB 2.0 host controller

[Sneeker.Yeh]

>-----Original Message-----
>From: Arnd Bergmann [mailto:arnd@arndb.de]
>Sent: Wednesday, December 17, 2014 11:44 PM
>To: Sneeker.Yeh
>Cc: Sneeker Yeh; Rob Herring; Pawel Moll; Mark Rutland; Ian Campbell; Kumar
>Gala; Felipe Balbi; Greg Kroah-Hartman; Mathias Nyman; Grant Likely; Alan
>Stern; Paul Bolle; Hans de Goede; Thomas Pugliese; David Mosberger; Peter
>Griffin; Sylwester Nawrocki; Andrew Bresticker; Gregory CLEMENT; Yoshihiro
>Shimoda; devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
>linux-usb@vger.kernel.org; linux-omap@vger.kernel.org; Andy Green; Jassi Brar
>Subject: Re: [PATCH 1/3] usb: host: f_usb20ho: add support for Fujitsu ehci/ohci
>USB 2.0 host controller
>
>On Wednesday 17 December 2014 23:33:02 Sneeker. Yeh wrote:
>> >
>> >No need for playing games with child devices, just see how the other
>> >drivers do it.
>>
>> I am wondering if this is what you mentioned here? :
>> Hcd21: f_usb20ho {
>>         compatible = "fujitsu,f_usb20ho";
>>         ...
>>         Ehci {
>>                 compatible = "generic-ehci";
>>                 reg = <>;
>>         }
>>         ohci {
>>                 compatible = "generic-ohci";
>>                 reg = <>;
>>         }
>> }
>> By doing that, I can just use of_platform_populate() to create
>> platform device more gracefully, than using platform_device_xxx() those
>have been done in of_platform_populate().
>> And also, managing register offset is totally done via dts description, not in
>the code.
>
>What I mean is leaving out the fujitsu,f_usb20ho node entirely, just put the two
>USB controllers on the bus directly and remove this driver.

Okay, I see it now.
Thanks.

Much appreciate,
Sneeker

>
>        Arnd
>
>-----
>未在此訊息中找到病毒。
>已透過 AVG 檢查 - www.avg.com
>版本: 2012.0.2249 / 病毒庫: 4235/8251 - 發佈日期: 12/17/14

[PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Sneeker Yeh]

This patch adds support for Synopsis DesignWare USB3 IP Core found
on Fujitsu Socs.

Signed-off-by: Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>
---
 .../devicetree/bindings/usb/fujitsu-dwc3.txt       |   25 +++
 drivers/usb/dwc3/Kconfig                           |   11 ++
 drivers/usb/dwc3/Makefile                          |    1 +
 drivers/usb/dwc3/dwc3-mb86s70.c                    |  194 ++++++++++++++++++++
 4 files changed, 231 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
 create mode 100644 drivers/usb/dwc3/dwc3-mb86s70.c

diff --git a/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
new file mode 100644
index 0000000..df77f91
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
@@ -0,0 +1,25 @@
+FUJITSU GLUE COMPONENTS
+
+MB86S7x DWC3 GLUE
+ - compatible : Should be "fujitsu,mb86s70-dwc3"
+ - clocks: from common clock binding, handle to usb clock.
+ - clock-names: from common clock binding.
+ - #address-cells, #size-cells : Must be present if the device has sub-nodes
+ - ranges: the child address space are mapped 1:1 onto the parent address space
+ - #stream-id-cells : handle to use "arm,mmu-400" ARM IOMMU driver
+
+Sub-nodes:
+The dwc3 core should be added as subnode to MB86S7x dwc3 glue.
+- dwc3 :
+   The binding details of dwc3 can be found in:
+   Documentation/devicetree/bindings/usb/dwc3.txt
+
+usb3host: mb86s70_usb3host {
+	compatible = "fujitsu,mb86s70-dwc3";
+	clocks = <&clk_alw_1_1>;
+	clock-names = "bus_clk";
+	#address-cells = <2>;
+	#size-cells = <1>;
+	ranges;
+	#stream-id-cells = <0>;
+};
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 58b5b2c..3390d42 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -61,6 +61,17 @@ config USB_DWC3_EXYNOS
 	  Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
 	  say 'Y' or 'M' if you have one such device.
 
+config USB_DWC3_MB86S70
+	tristate "MB86S70 Designware USB3 Platform code"
+	default USB_DWC3
+	help
+	  MB86S7X SOC ship with DesignWare Core USB3 IP inside,
+	  this implementation also integrated Fujitsu USB PHY inside
+	  this Core USB3 IP.
+
+	  say 'Y' or 'M' if you have one such device.
+
+
 config USB_DWC3_PCI
 	tristate "PCIe-based Platforms"
 	depends on PCI
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index bb34fbc..05d1de2 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -38,3 +38,4 @@ obj-$(CONFIG_USB_DWC3_PCI)		+= dwc3-pci.o
 obj-$(CONFIG_USB_DWC3_KEYSTONE)		+= dwc3-keystone.o
 obj-$(CONFIG_USB_DWC3_QCOM)		+= dwc3-qcom.o
 obj-$(CONFIG_USB_DWC3_ST)		+= dwc3-st.o
+obj-$(CONFIG_USB_DWC3_MB86S70)		+= dwc3-mb86s70.o
diff --git a/drivers/usb/dwc3/dwc3-mb86s70.c b/drivers/usb/dwc3/dwc3-mb86s70.c
new file mode 100644
index 0000000..241c5bd
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-mb86s70.c
@@ -0,0 +1,194 @@
+/**
+ * dwc3-mb86s70.c - Fujitsu mb86s70 DWC3 Specific Glue layer
+ *
+ * Copyright (c) 2013 - 2014 FUJITSU SEMICONDUCTOR LIMITED
+ *		http://jp.fujitsu.com/group/fsl
+ *
+ * Author: Alice Chan <Alice.Chan@tw.fujitsu.com>
+ * based on dwc3-exynos.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/platform_device.h>
+#include <linux/dma-mapping.h>
+#include <linux/interrupt.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/pm_runtime.h>
+#include <linux/clk.h>
+
+struct dwc3_mb86s70 {
+	struct device	*dev;
+	struct clk		**clk_table;
+};
+
+/* return 0 means successful */
+static int dwc3_mb86s70_clk_control(struct device *dev, bool on)
+{
+	int ret, i;
+	struct clk *clk;
+
+	if (!on)
+		goto clock_off;
+
+	for (i = 0;; i++) {
+		clk = of_clk_get(dev->of_node, i);
+		if (IS_ERR(clk))
+			break;
+
+		ret = clk_prepare_enable(clk);
+		if (ret) {
+			dev_err(dev, "failed to enable clock[%d]\n", i);
+			goto clock_off;
+		}
+	}
+
+	return 0;
+
+clock_off:
+	for (i = 0;; i++) {
+		clk = of_clk_get(dev->of_node, i);
+		if (IS_ERR(clk))
+			break;
+
+		clk_disable_unprepare(clk);
+	}
+
+	return on;
+}
+
+static int dwc3_mb86s70_remove_child(struct device *dev, void *unused)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	of_device_unregister(pdev);
+
+	return 0;
+}
+
+static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
+
+static int dwc3_mb86s70_probe(struct platform_device *pdev)
+{
+	struct dwc3_mb86s70	*mb86s70;
+	struct device		*dev = &pdev->dev;
+	struct device_node	*node = dev->of_node;
+	int			ret;
+
+	mb86s70 = devm_kzalloc(dev, sizeof(*mb86s70), GFP_KERNEL);
+	if (!mb86s70) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	dev->dma_mask = &dwc3_mb86s70_dma_mask;
+
+	platform_set_drvdata(pdev, mb86s70);
+
+	mb86s70->dev = dev;
+
+	/* resume driver for clock, power */
+	pm_runtime_enable(&pdev->dev);
+	ret = pm_runtime_get_sync(&pdev->dev);
+	if (ret < 0) {
+		dev_err(&pdev->dev, "get_sync failed with err %d\n", ret);
+		goto err;
+	}
+
+	if (node) {
+		ret = of_platform_populate(node, NULL, NULL, dev);
+		if (!ret)
+			return 0;
+		dev_err(dev, "failed to add dwc3 core\n");
+	}
+	dev_err(dev, "no device node, failed to add dwc3 core\n");
+	ret = -ENODEV;
+
+err:
+	return ret;
+}
+
+static int dwc3_mb86s70_remove(struct platform_device *pdev)
+{
+	device_for_each_child(&pdev->dev, NULL, dwc3_mb86s70_remove_child);
+
+	pm_runtime_put_sync(&pdev->dev);
+	pm_runtime_disable(&pdev->dev);
+
+	return 0;
+}
+
+static const struct of_device_id mb86s70_dwc3_match[] = {
+	{ .compatible = "fujitsu,mb86s70-dwc3" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, mb86s70_dwc3_match);
+
+#ifdef CONFIG_PM
+#ifdef CONFIG_PM_RUNTIME
+static int dwc3_mb86s70_runtime_suspend(struct device *dev)
+{
+	dwc3_mb86s70_clk_control(dev, false);
+
+	return 0;
+}
+
+static int  dwc3_mb86s70_runtime_resume(struct device *dev)
+{
+	dwc3_mb86s70_clk_control(dev, true);
+
+	return 0;
+}
+#endif /* CONFIG_PM_RUNTIME */
+static int dwc3_mb86s70_suspend(struct device *dev)
+{
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
+	return dwc3_mb86s70_runtime_suspend(dev);
+}
+
+static int dwc3_mb86s70_resume(struct device *dev)
+{
+	if (pm_runtime_status_suspended(dev))
+		return 0;
+
+	return dwc3_mb86s70_runtime_resume(dev);
+}
+
+static const struct dev_pm_ops dwc3_mb86s70_dev_pm_ops = {
+	.suspend = dwc3_mb86s70_suspend,
+	.resume = dwc3_mb86s70_resume,
+	SET_RUNTIME_PM_OPS(
+		dwc3_mb86s70_runtime_suspend,
+		dwc3_mb86s70_runtime_resume, NULL)
+};
+
+#define DEV_PM_OPS	(&dwc3_mb86s70_dev_pm_ops)
+#else
+#define DEV_PM_OPS	NULL
+#endif /* CONFIG_PM_SLEEP */
+
+static struct platform_driver dwc3_mb86s70_driver = {
+	.probe		= dwc3_mb86s70_probe,
+	.remove		= dwc3_mb86s70_remove,
+	.driver		= {
+		.name	= "mb86s70-dwc3",
+		.of_match_table = of_match_ptr(mb86s70_dwc3_match),
+		.pm	= DEV_PM_OPS,
+	},
+};
+
+module_platform_driver(dwc3_mb86s70_driver);
+
+MODULE_ALIAS("platform:mb86s70-dwc3");
+MODULE_AUTHOR("Alice Chan <Alice.Chan@tw.fujitsu.com>");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("DesignWare USB3 mb86s70 Glue Layer");
-- 
1.7.9.5

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 8203 bytes --]

Hi,

On Tue, Dec 16, 2014 at 10:10:27AM +0800, Sneeker Yeh wrote:
> This patch adds support for Synopsis DesignWare USB3 IP Core found
> on Fujitsu Socs.
> 
> Signed-off-by: Sneeker Yeh <Sneeker.Yeh-l16TxrwUIHTQFUHtdCDX3A@public.gmane.org>
> ---
>  .../devicetree/bindings/usb/fujitsu-dwc3.txt       |   25 +++
>  drivers/usb/dwc3/Kconfig                           |   11 ++
>  drivers/usb/dwc3/Makefile                          |    1 +
>  drivers/usb/dwc3/dwc3-mb86s70.c                    |  194 ++++++++++++++++++++
>  4 files changed, 231 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
>  create mode 100644 drivers/usb/dwc3/dwc3-mb86s70.c
> 
> diff --git a/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> new file mode 100644
> index 0000000..df77f91
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> @@ -0,0 +1,25 @@
> +FUJITSU GLUE COMPONENTS
> +
> +MB86S7x DWC3 GLUE
> + - compatible : Should be "fujitsu,mb86s70-dwc3"
> + - clocks: from common clock binding, handle to usb clock.
> + - clock-names: from common clock binding.
> + - #address-cells, #size-cells : Must be present if the device has sub-nodes
> + - ranges: the child address space are mapped 1:1 onto the parent address space
> + - #stream-id-cells : handle to use "arm,mmu-400" ARM IOMMU driver
> +
> +Sub-nodes:
> +The dwc3 core should be added as subnode to MB86S7x dwc3 glue.
> +- dwc3 :
> +   The binding details of dwc3 can be found in:
> +   Documentation/devicetree/bindings/usb/dwc3.txt
> +
> +usb3host: mb86s70_usb3host {
> +	compatible = "fujitsu,mb86s70-dwc3";
> +	clocks = <&clk_alw_1_1>;
> +	clock-names = "bus_clk";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	ranges;
> +	#stream-id-cells = <0>;
> +};
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 58b5b2c..3390d42 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -61,6 +61,17 @@ config USB_DWC3_EXYNOS
>  	  Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
>  	  say 'Y' or 'M' if you have one such device.
>  
> +config USB_DWC3_MB86S70
> +	tristate "MB86S70 Designware USB3 Platform code"
> +	default USB_DWC3
> +	help
> +	  MB86S7X SOC ship with DesignWare Core USB3 IP inside,
> +	  this implementation also integrated Fujitsu USB PHY inside
> +	  this Core USB3 IP.
> +
> +	  say 'Y' or 'M' if you have one such device.
> +
> +
>  config USB_DWC3_PCI
>  	tristate "PCIe-based Platforms"
>  	depends on PCI
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index bb34fbc..05d1de2 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -38,3 +38,4 @@ obj-$(CONFIG_USB_DWC3_PCI)		+= dwc3-pci.o
>  obj-$(CONFIG_USB_DWC3_KEYSTONE)		+= dwc3-keystone.o
>  obj-$(CONFIG_USB_DWC3_QCOM)		+= dwc3-qcom.o
>  obj-$(CONFIG_USB_DWC3_ST)		+= dwc3-st.o
> +obj-$(CONFIG_USB_DWC3_MB86S70)		+= dwc3-mb86s70.o
> diff --git a/drivers/usb/dwc3/dwc3-mb86s70.c b/drivers/usb/dwc3/dwc3-mb86s70.c
> new file mode 100644
> index 0000000..241c5bd
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-mb86s70.c
> @@ -0,0 +1,194 @@
> +/**
> + * dwc3-mb86s70.c - Fujitsu mb86s70 DWC3 Specific Glue layer
> + *
> + * Copyright (c) 2013 - 2014 FUJITSU SEMICONDUCTOR LIMITED
> + *		http://jp.fujitsu.com/group/fsl
> + *
> + * Author: Alice Chan <Alice.Chan-l16TxrwUIHTQFUHtdCDX3A@public.gmane.org>
> + * based on dwc3-exynos.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/platform_device.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/interrupt.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/clk.h>
> +
> +struct dwc3_mb86s70 {
> +	struct device	*dev;
> +	struct clk		**clk_table;

please align these.

> +};
> +
> +/* return 0 means successful */

this comment is unnecessary :-)

> +static int dwc3_mb86s70_clk_control(struct device *dev, bool on)
> +{
> +	int ret, i;
> +	struct clk *clk;
> +
> +	if (!on)
> +		goto clock_off;
> +
> +	for (i = 0;; i++) {
> +		clk = of_clk_get(dev->of_node, i);

you could count the number of properties first, then allocate clk_table
to the exact size.

> +		if (IS_ERR(clk))
> +			break;
> +
> +		ret = clk_prepare_enable(clk);
> +		if (ret) {
> +			dev_err(dev, "failed to enable clock[%d]\n", i);
> +			goto clock_off;
> +		}
> +	}
> +
> +	return 0;
> +
> +clock_off:
> +	for (i = 0;; i++) {
> +		clk = of_clk_get(dev->of_node, i);
> +		if (IS_ERR(clk))
> +			break;
> +
> +		clk_disable_unprepare(clk);
> +	}
> +
> +	return on;
> +}
> +
> +static int dwc3_mb86s70_remove_child(struct device *dev, void *unused)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +
> +	of_device_unregister(pdev);
> +
> +	return 0;
> +}
> +
> +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);

why ? Use dma_coerce_mask_and_coherent().

> +
> +static int dwc3_mb86s70_probe(struct platform_device *pdev)
> +{
> +	struct dwc3_mb86s70	*mb86s70;
> +	struct device		*dev = &pdev->dev;
> +	struct device_node	*node = dev->of_node;
> +	int			ret;
> +
> +	mb86s70 = devm_kzalloc(dev, sizeof(*mb86s70), GFP_KERNEL);
> +	if (!mb86s70) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +
> +	dev->dma_mask = &dwc3_mb86s70_dma_mask;
> +
> +	platform_set_drvdata(pdev, mb86s70);
> +
> +	mb86s70->dev = dev;
> +
> +	/* resume driver for clock, power */
> +	pm_runtime_enable(&pdev->dev);
> +	ret = pm_runtime_get_sync(&pdev->dev);

here's a slightly better way (add error checking where necessary):

	dwc3_mb86s70_clk_control(dev, true);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);
	pm_runtime_get(dev);


> +	if (ret < 0) {
> +		dev_err(&pdev->dev, "get_sync failed with err %d\n", ret);
> +		goto err;

if get_sync() fails, you still need to call pm_runtime_put_sync(); In
this case, you also need to call pm_runtime_disable();

> +	}
> +
> +	if (node) {
> +		ret = of_platform_populate(node, NULL, NULL, dev);
> +		if (!ret)
> +			return 0;
> +		dev_err(dev, "failed to add dwc3 core\n");
> +	}
> +	dev_err(dev, "no device node, failed to add dwc3 core\n");
> +	ret = -ENODEV;
> +
> +err:
> +	return ret;
> +}
> +
> +static int dwc3_mb86s70_remove(struct platform_device *pdev)
> +{
> +	device_for_each_child(&pdev->dev, NULL, dwc3_mb86s70_remove_child);
> +
> +	pm_runtime_put_sync(&pdev->dev);
> +	pm_runtime_disable(&pdev->dev);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id mb86s70_dwc3_match[] = {
> +	{ .compatible = "fujitsu,mb86s70-dwc3" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, mb86s70_dwc3_match);
> +
> +#ifdef CONFIG_PM
> +#ifdef CONFIG_PM_RUNTIME
> +static int dwc3_mb86s70_runtime_suspend(struct device *dev)
> +{
> +	dwc3_mb86s70_clk_control(dev, false);
> +
> +	return 0;
> +}
> +
> +static int  dwc3_mb86s70_runtime_resume(struct device *dev)
> +{
> +	dwc3_mb86s70_clk_control(dev, true);
> +
> +	return 0;
> +}
> +#endif /* CONFIG_PM_RUNTIME */
> +static int dwc3_mb86s70_suspend(struct device *dev)
> +{
> +	if (pm_runtime_status_suspended(dev))
> +		return 0;
> +
> +	return dwc3_mb86s70_runtime_suspend(dev);

ok, this is a bit confusing and, in fact, wrong. You're not dealing with
wakeups (which are mandatory on runtime_pm and option on system sleep).

It's okay to have a common function called by both runtime_pm and
system_sleep methods, but calling your runtime_pm handler is a bit odd.

last but not least, I need to see results of this running on top of
v3.19-rc1. Are you using both host and peripheral modes or only one mode
for now ?

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Sneeker Yeh]

[-- Attachment #1: Type: text/plain, Size: 12636 bytes --]

Hi,

2014-12-22 23:59 GMT+08:00 Felipe Balbi <balbi@ti.com>:

> Hi,
>
> On Tue, Dec 16, 2014 at 10:10:27AM +0800, Sneeker Yeh wrote:
> > This patch adds support for Synopsis DesignWare USB3 IP Core found
> > on Fujitsu Socs.
> >
> > Signed-off-by: Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>
> > ---
> >  .../devicetree/bindings/usb/fujitsu-dwc3.txt       |   25 +++
> >  drivers/usb/dwc3/Kconfig                           |   11 ++
> >  drivers/usb/dwc3/Makefile                          |    1 +
> >  drivers/usb/dwc3/dwc3-mb86s70.c                    |  194
> ++++++++++++++++++++
> >  4 files changed, 231 insertions(+)
> >  create mode 100644
> Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> >  create mode 100644 drivers/usb/dwc3/dwc3-mb86s70.c
> >
> > diff --git a/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > new file mode 100644
> > index 0000000..df77f91
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > @@ -0,0 +1,25 @@
> > +FUJITSU GLUE COMPONENTS
> > +
> > +MB86S7x DWC3 GLUE
> > + - compatible : Should be "fujitsu,mb86s70-dwc3"
> > + - clocks: from common clock binding, handle to usb clock.
> > + - clock-names: from common clock binding.
> > + - #address-cells, #size-cells : Must be present if the device has
> sub-nodes
> > + - ranges: the child address space are mapped 1:1 onto the parent
> address space
> > + - #stream-id-cells : handle to use "arm,mmu-400" ARM IOMMU driver
> > +
> > +Sub-nodes:
> > +The dwc3 core should be added as subnode to MB86S7x dwc3 glue.
> > +- dwc3 :
> > +   The binding details of dwc3 can be found in:
> > +   Documentation/devicetree/bindings/usb/dwc3.txt
> > +
> > +usb3host: mb86s70_usb3host {
> > +     compatible = "fujitsu,mb86s70-dwc3";
> > +     clocks = <&clk_alw_1_1>;
> > +     clock-names = "bus_clk";
> > +     #address-cells = <2>;
> > +     #size-cells = <1>;
> > +     ranges;
> > +     #stream-id-cells = <0>;
> > +};
> > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> > index 58b5b2c..3390d42 100644
> > --- a/drivers/usb/dwc3/Kconfig
> > +++ b/drivers/usb/dwc3/Kconfig
> > @@ -61,6 +61,17 @@ config USB_DWC3_EXYNOS
> >         Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
> >         say 'Y' or 'M' if you have one such device.
> >
> > +config USB_DWC3_MB86S70
> > +     tristate "MB86S70 Designware USB3 Platform code"
> > +     default USB_DWC3
> > +     help
> > +       MB86S7X SOC ship with DesignWare Core USB3 IP inside,
> > +       this implementation also integrated Fujitsu USB PHY inside
> > +       this Core USB3 IP.
> > +
> > +       say 'Y' or 'M' if you have one such device.
> > +
> > +
> >  config USB_DWC3_PCI
> >       tristate "PCIe-based Platforms"
> >       depends on PCI
> > diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> > index bb34fbc..05d1de2 100644
> > --- a/drivers/usb/dwc3/Makefile
> > +++ b/drivers/usb/dwc3/Makefile
> > @@ -38,3 +38,4 @@ obj-$(CONFIG_USB_DWC3_PCI)          += dwc3-pci.o
> >  obj-$(CONFIG_USB_DWC3_KEYSTONE)              += dwc3-keystone.o
> >  obj-$(CONFIG_USB_DWC3_QCOM)          += dwc3-qcom.o
> >  obj-$(CONFIG_USB_DWC3_ST)            += dwc3-st.o
> > +obj-$(CONFIG_USB_DWC3_MB86S70)               += dwc3-mb86s70.o
> > diff --git a/drivers/usb/dwc3/dwc3-mb86s70.c
> b/drivers/usb/dwc3/dwc3-mb86s70.c
> > new file mode 100644
> > index 0000000..241c5bd
> > --- /dev/null
> > +++ b/drivers/usb/dwc3/dwc3-mb86s70.c
> > @@ -0,0 +1,194 @@
> > +/**
> > + * dwc3-mb86s70.c - Fujitsu mb86s70 DWC3 Specific Glue layer
> > + *
> > + * Copyright (c) 2013 - 2014 FUJITSU SEMICONDUCTOR LIMITED
> > + *           http://jp.fujitsu.com/group/fsl
> > + *
> > + * Author: Alice Chan <Alice.Chan@tw.fujitsu.com>
> > + * based on dwc3-exynos.c
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/dma-mapping.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/of.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/clk.h>
> > +
> > +struct dwc3_mb86s70 {
> > +     struct device   *dev;
> > +     struct clk              **clk_table;
>
> please align these.


okay.


>


> > +};
> > +
> > +/* return 0 means successful */
>
> this comment is unnecessary :-)
>

okay


>
> > +static int dwc3_mb86s70_clk_control(struct device *dev, bool on)
> > +{
> > +     int ret, i;
> > +     struct clk *clk;
> > +
> > +     if (!on)
> > +             goto clock_off;
> > +
> > +     for (i = 0;; i++) {
> > +             clk = of_clk_get(dev->of_node, i);
>
> you could count the number of properties first, then allocate clk_table
> to the exact size.
>

okay, thanks.


>
> > +             if (IS_ERR(clk))
> > +                     break;
> > +
> > +             ret = clk_prepare_enable(clk);
> > +             if (ret) {
> > +                     dev_err(dev, "failed to enable clock[%d]\n", i);
> > +                     goto clock_off;
> > +             }
> > +     }
> > +
> > +     return 0;
> > +
> > +clock_off:
> > +     for (i = 0;; i++) {
> > +             clk = of_clk_get(dev->of_node, i);
> > +             if (IS_ERR(clk))
> > +                     break;
> > +
> > +             clk_disable_unprepare(clk);
> > +     }
> > +
> > +     return on;
> > +}
> > +
> > +static int dwc3_mb86s70_remove_child(struct device *dev, void *unused)
> > +{
> > +     struct platform_device *pdev = to_platform_device(dev);
> > +
> > +     of_device_unregister(pdev);
> > +
> > +     return 0;
> > +}
> > +
> > +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
>
> why ? Use dma_coerce_mask_and_coherent().
>

okay.


>
> > +
> > +static int dwc3_mb86s70_probe(struct platform_device *pdev)
> > +{
> > +     struct dwc3_mb86s70     *mb86s70;
> > +     struct device           *dev = &pdev->dev;
> > +     struct device_node      *node = dev->of_node;
> > +     int                     ret;
> > +
> > +     mb86s70 = devm_kzalloc(dev, sizeof(*mb86s70), GFP_KERNEL);
> > +     if (!mb86s70) {
> > +             ret = -ENOMEM;
> > +             goto err;
> > +     }
> > +
> > +     dev->dma_mask = &dwc3_mb86s70_dma_mask;
> > +
> > +     platform_set_drvdata(pdev, mb86s70);
> > +
> > +     mb86s70->dev = dev;
> > +
> > +     /* resume driver for clock, power */
> > +     pm_runtime_enable(&pdev->dev);
> > +     ret = pm_runtime_get_sync(&pdev->dev);
>
> here's a slightly better way (add error checking where necessary):
>
>         dwc3_mb86s70_clk_control(dev, true);
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>         pm_runtime_get(dev);
>
>
okay, thanks,
these seems much better and more correct.



>
> > +     if (ret < 0) {
> > +             dev_err(&pdev->dev, "get_sync failed with err %d\n", ret);
> > +             goto err;
>
> if get_sync() fails, you still need to call pm_runtime_put_sync(); In
> this case, you also need to call pm_runtime_disable();
>

okay, thanks.


>
> > +     }
> > +
> > +     if (node) {
> > +             ret = of_platform_populate(node, NULL, NULL, dev);
> > +             if (!ret)
> > +                     return 0;
> > +             dev_err(dev, "failed to add dwc3 core\n");
> > +     }
> > +     dev_err(dev, "no device node, failed to add dwc3 core\n");
> > +     ret = -ENODEV;
> > +
> > +err:
> > +     return ret;
> > +}
> > +
> > +static int dwc3_mb86s70_remove(struct platform_device *pdev)
> > +{
> > +     device_for_each_child(&pdev->dev, NULL, dwc3_mb86s70_remove_child);
> > +
> > +     pm_runtime_put_sync(&pdev->dev);
> > +     pm_runtime_disable(&pdev->dev);
> > +
> > +     return 0;
> > +}
> > +
> > +static const struct of_device_id mb86s70_dwc3_match[] = {
> > +     { .compatible = "fujitsu,mb86s70-dwc3" },
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(of, mb86s70_dwc3_match);
> > +
> > +#ifdef CONFIG_PM
> > +#ifdef CONFIG_PM_RUNTIME
> > +static int dwc3_mb86s70_runtime_suspend(struct device *dev)
> > +{
> > +     dwc3_mb86s70_clk_control(dev, false);
> > +
> > +     return 0;
> > +}
> > +
> > +static int  dwc3_mb86s70_runtime_resume(struct device *dev)
> > +{
> > +     dwc3_mb86s70_clk_control(dev, true);
> > +
> > +     return 0;
> > +}
> > +#endif /* CONFIG_PM_RUNTIME */
> > +static int dwc3_mb86s70_suspend(struct device *dev)
> > +{
> > +     if (pm_runtime_status_suspended(dev))
> > +             return 0;
> > +
> > +     return dwc3_mb86s70_runtime_suspend(dev);
>
> ok, this is a bit confusing and, in fact, wrong. You're not dealing with
> wakeups (which are mandatory on runtime_pm and option on system sleep).
>
> It's okay to have a common function called by both runtime_pm and
> system_sleep methods, but calling your runtime_pm handler is a bit odd.
>

hm, sorry I might confuse people here.
I'll change this to what you suggested.


>
> last but not least, I need to see results of this running on top of
> v3.19-rc1. Are you using both host and peripheral modes or only one mode
> for now ?
>

The dwc3 IP we adapted is host-only, and we run this platform glue driver
in v3.19-rc1 for now.
bellow log is about suspend and resume successfully, and dwc3 still works.

root@linaro-nano:~# echo mem > /sys/power/state
[   56.713281] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_suspend() is
started.
[   56.720721] mb86s70-dwc3 mb86s70_usb3host1.5:
dwc3_mb86s70_runtime_suspend() is started.
[   56.729419] mb86s70-dwc3 mb86s70_usb3host1.5:
dwc3_mb86s70_runtime_suspend() is ended.
[   56.737363] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_suspend() is
ended.
[   56.744604] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_suspend() is
started.
[   56.752022] mb86s70-dwc3 mb86s70_usb3host0.4:
dwc3_mb86s70_runtime_suspend() is started.
[   56.760700] mb86s70-dwc3 mb86s70_usb3host0.4:
dwc3_mb86s70_runtime_suspend() is ended.
[   56.768639] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_suspend() is
ended.
[   57.107139] SUSPEND

[   74.564963] mmc0: execute_tuning --
[   74.877776] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_resume() is
started.
[   74.885126] mb86s70-dwc3 mb86s70_usb3host0.4:
dwc3_mb86s70_runtime_resume() is started.
[   74.893749] mb86s70-dwc3 mb86s70_usb3host0.4:
dwc3_mb86s70_runtime_resume() is ended.
[   74.901606] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_resume() is
ended.
[   74.908766] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_resume() is
started.
[   74.916086] mb86s70-dwc3 mb86s70_usb3host1.5:
dwc3_mb86s70_runtime_resume() is started.
[   74.924660] mb86s70-dwc3 mb86s70_usb3host1.5:
dwc3_mb86s70_runtime_resume() is ended.
[   74.932513] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_resume() is
ended.
root@linaro-nano:~#

But I realize something might confuse people, and even is wrong, through
your comment.
So I guess I should align these to a more clear form like dwc3-exynos.c :

static int dwc3_exynos_suspend(struct device *dev)
{
struct dwc3_exynos *exynos = dev_get_drvdata(dev);

clk_disable(exynos->clk);

return 0;
}

static int dwc3_exynos_resume(struct device *dev)
{
struct dwc3_exynos *exynos = dev_get_drvdata(dev);

clk_enable(exynos->clk);

/* runtime set active to reflect active state. */
pm_runtime_disable(dev);
pm_runtime_set_active(dev);
pm_runtime_enable(dev);

return 0;
}

Would it been better than how I do suspend/resume here?

however recently I got some thought inspired by Arnd and the commit
5f872175c867a839e2feeecfa59e3db38c88b561

For imitating that in my experiment recently, i only need to add clock
on/off in dwc3 core.
Then dwc3 core won't be always created via sub node in platform glue node,
and vendors like us can just drop platform glue which don't have any
specific platform code to wrap dwc3 core, and just directly use dwc3 core
via device tree.
Of course here dwc3 core won't process clock if he got null one, so this
won't affect other vendor's platform which only pass clock phandle to their
dwc3 platform glue driver.

Just wondering if it's better to write a platform glue only did clock
on/off,
or to do the same improvement people ever did for ehci-platform driver and
just drop this platform glue driver?

Hope I didn't reply too late, or make any impolite move here,
Super thanks for every comment you made here.

Much appreciate,
Sneeker



> --
> balbi
>

[-- Attachment #2: Type: text/html, Size: 18977 bytes --]

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 15062 bytes --]

Hi,

On Mon, Dec 29, 2014 at 01:52:04AM +0800, Sneeker Yeh wrote:
> Hi,
> 
> 2014-12-22 23:59 GMT+08:00 Felipe Balbi <balbi@ti.com>:
> 
> > Hi,
> >
> > On Tue, Dec 16, 2014 at 10:10:27AM +0800, Sneeker Yeh wrote:
> > > This patch adds support for Synopsis DesignWare USB3 IP Core found
> > > on Fujitsu Socs.
> > >
> > > Signed-off-by: Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>
> > > ---
> > >  .../devicetree/bindings/usb/fujitsu-dwc3.txt       |   25 +++
> > >  drivers/usb/dwc3/Kconfig                           |   11 ++
> > >  drivers/usb/dwc3/Makefile                          |    1 +
> > >  drivers/usb/dwc3/dwc3-mb86s70.c                    |  194
> > ++++++++++++++++++++
> > >  4 files changed, 231 insertions(+)
> > >  create mode 100644
> > Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > >  create mode 100644 drivers/usb/dwc3/dwc3-mb86s70.c
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > > new file mode 100644
> > > index 0000000..df77f91
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > > @@ -0,0 +1,25 @@
> > > +FUJITSU GLUE COMPONENTS
> > > +
> > > +MB86S7x DWC3 GLUE
> > > + - compatible : Should be "fujitsu,mb86s70-dwc3"
> > > + - clocks: from common clock binding, handle to usb clock.
> > > + - clock-names: from common clock binding.
> > > + - #address-cells, #size-cells : Must be present if the device has
> > sub-nodes
> > > + - ranges: the child address space are mapped 1:1 onto the parent
> > address space
> > > + - #stream-id-cells : handle to use "arm,mmu-400" ARM IOMMU driver
> > > +
> > > +Sub-nodes:
> > > +The dwc3 core should be added as subnode to MB86S7x dwc3 glue.
> > > +- dwc3 :
> > > +   The binding details of dwc3 can be found in:
> > > +   Documentation/devicetree/bindings/usb/dwc3.txt
> > > +
> > > +usb3host: mb86s70_usb3host {
> > > +     compatible = "fujitsu,mb86s70-dwc3";
> > > +     clocks = <&clk_alw_1_1>;
> > > +     clock-names = "bus_clk";
> > > +     #address-cells = <2>;
> > > +     #size-cells = <1>;
> > > +     ranges;
> > > +     #stream-id-cells = <0>;
> > > +};
> > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> > > index 58b5b2c..3390d42 100644
> > > --- a/drivers/usb/dwc3/Kconfig
> > > +++ b/drivers/usb/dwc3/Kconfig
> > > @@ -61,6 +61,17 @@ config USB_DWC3_EXYNOS
> > >         Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP inside,
> > >         say 'Y' or 'M' if you have one such device.
> > >
> > > +config USB_DWC3_MB86S70
> > > +     tristate "MB86S70 Designware USB3 Platform code"
> > > +     default USB_DWC3
> > > +     help
> > > +       MB86S7X SOC ship with DesignWare Core USB3 IP inside,
> > > +       this implementation also integrated Fujitsu USB PHY inside
> > > +       this Core USB3 IP.
> > > +
> > > +       say 'Y' or 'M' if you have one such device.
> > > +
> > > +
> > >  config USB_DWC3_PCI
> > >       tristate "PCIe-based Platforms"
> > >       depends on PCI
> > > diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> > > index bb34fbc..05d1de2 100644
> > > --- a/drivers/usb/dwc3/Makefile
> > > +++ b/drivers/usb/dwc3/Makefile
> > > @@ -38,3 +38,4 @@ obj-$(CONFIG_USB_DWC3_PCI)          += dwc3-pci.o
> > >  obj-$(CONFIG_USB_DWC3_KEYSTONE)              += dwc3-keystone.o
> > >  obj-$(CONFIG_USB_DWC3_QCOM)          += dwc3-qcom.o
> > >  obj-$(CONFIG_USB_DWC3_ST)            += dwc3-st.o
> > > +obj-$(CONFIG_USB_DWC3_MB86S70)               += dwc3-mb86s70.o
> > > diff --git a/drivers/usb/dwc3/dwc3-mb86s70.c
> > b/drivers/usb/dwc3/dwc3-mb86s70.c
> > > new file mode 100644
> > > index 0000000..241c5bd
> > > --- /dev/null
> > > +++ b/drivers/usb/dwc3/dwc3-mb86s70.c
> > > @@ -0,0 +1,194 @@
> > > +/**
> > > + * dwc3-mb86s70.c - Fujitsu mb86s70 DWC3 Specific Glue layer
> > > + *
> > > + * Copyright (c) 2013 - 2014 FUJITSU SEMICONDUCTOR LIMITED
> > > + *           http://jp.fujitsu.com/group/fsl
> > > + *
> > > + * Author: Alice Chan <Alice.Chan@tw.fujitsu.com>
> > > + * based on dwc3-exynos.c
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License as published by
> > > + * the Free Software Foundation; either version 2 of the License, or
> > > + * (at your option) any later version.
> > > + */
> > > +
> > > +#include <linux/module.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/dma-mapping.h>
> > > +#include <linux/interrupt.h>
> > > +#include <linux/of.h>
> > > +#include <linux/of_platform.h>
> > > +#include <linux/pm_runtime.h>
> > > +#include <linux/clk.h>
> > > +
> > > +struct dwc3_mb86s70 {
> > > +     struct device   *dev;
> > > +     struct clk              **clk_table;
> >
> > please align these.
> 
> 
> okay.
> 
> 
> >
> 
> 
> > > +};
> > > +
> > > +/* return 0 means successful */
> >
> > this comment is unnecessary :-)
> >
> 
> okay
> 
> 
> >
> > > +static int dwc3_mb86s70_clk_control(struct device *dev, bool on)
> > > +{
> > > +     int ret, i;
> > > +     struct clk *clk;
> > > +
> > > +     if (!on)
> > > +             goto clock_off;
> > > +
> > > +     for (i = 0;; i++) {
> > > +             clk = of_clk_get(dev->of_node, i);
> >
> > you could count the number of properties first, then allocate clk_table
> > to the exact size.
> >
> 
> okay, thanks.
> 
> 
> >
> > > +             if (IS_ERR(clk))
> > > +                     break;
> > > +
> > > +             ret = clk_prepare_enable(clk);
> > > +             if (ret) {
> > > +                     dev_err(dev, "failed to enable clock[%d]\n", i);
> > > +                     goto clock_off;
> > > +             }
> > > +     }
> > > +
> > > +     return 0;
> > > +
> > > +clock_off:
> > > +     for (i = 0;; i++) {
> > > +             clk = of_clk_get(dev->of_node, i);
> > > +             if (IS_ERR(clk))
> > > +                     break;
> > > +
> > > +             clk_disable_unprepare(clk);
> > > +     }
> > > +
> > > +     return on;
> > > +}
> > > +
> > > +static int dwc3_mb86s70_remove_child(struct device *dev, void *unused)
> > > +{
> > > +     struct platform_device *pdev = to_platform_device(dev);
> > > +
> > > +     of_device_unregister(pdev);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
> >
> > why ? Use dma_coerce_mask_and_coherent().
> >
> 
> okay.
> 
> 
> >
> > > +
> > > +static int dwc3_mb86s70_probe(struct platform_device *pdev)
> > > +{
> > > +     struct dwc3_mb86s70     *mb86s70;
> > > +     struct device           *dev = &pdev->dev;
> > > +     struct device_node      *node = dev->of_node;
> > > +     int                     ret;
> > > +
> > > +     mb86s70 = devm_kzalloc(dev, sizeof(*mb86s70), GFP_KERNEL);
> > > +     if (!mb86s70) {
> > > +             ret = -ENOMEM;
> > > +             goto err;
> > > +     }
> > > +
> > > +     dev->dma_mask = &dwc3_mb86s70_dma_mask;
> > > +
> > > +     platform_set_drvdata(pdev, mb86s70);
> > > +
> > > +     mb86s70->dev = dev;
> > > +
> > > +     /* resume driver for clock, power */
> > > +     pm_runtime_enable(&pdev->dev);
> > > +     ret = pm_runtime_get_sync(&pdev->dev);
> >
> > here's a slightly better way (add error checking where necessary):
> >
> >         dwc3_mb86s70_clk_control(dev, true);
> >         pm_runtime_set_active(dev);
> >         pm_runtime_enable(dev);
> >         pm_runtime_get(dev);
> >
> >
> okay, thanks,
> these seems much better and more correct.
> 
> 
> 
> >
> > > +     if (ret < 0) {
> > > +             dev_err(&pdev->dev, "get_sync failed with err %d\n", ret);
> > > +             goto err;
> >
> > if get_sync() fails, you still need to call pm_runtime_put_sync(); In
> > this case, you also need to call pm_runtime_disable();
> >
> 
> okay, thanks.
> 
> 
> >
> > > +     }
> > > +
> > > +     if (node) {
> > > +             ret = of_platform_populate(node, NULL, NULL, dev);
> > > +             if (!ret)
> > > +                     return 0;
> > > +             dev_err(dev, "failed to add dwc3 core\n");
> > > +     }
> > > +     dev_err(dev, "no device node, failed to add dwc3 core\n");
> > > +     ret = -ENODEV;
> > > +
> > > +err:
> > > +     return ret;
> > > +}
> > > +
> > > +static int dwc3_mb86s70_remove(struct platform_device *pdev)
> > > +{
> > > +     device_for_each_child(&pdev->dev, NULL, dwc3_mb86s70_remove_child);
> > > +
> > > +     pm_runtime_put_sync(&pdev->dev);
> > > +     pm_runtime_disable(&pdev->dev);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static const struct of_device_id mb86s70_dwc3_match[] = {
> > > +     { .compatible = "fujitsu,mb86s70-dwc3" },
> > > +     {},
> > > +};
> > > +MODULE_DEVICE_TABLE(of, mb86s70_dwc3_match);
> > > +
> > > +#ifdef CONFIG_PM
> > > +#ifdef CONFIG_PM_RUNTIME
> > > +static int dwc3_mb86s70_runtime_suspend(struct device *dev)
> > > +{
> > > +     dwc3_mb86s70_clk_control(dev, false);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static int  dwc3_mb86s70_runtime_resume(struct device *dev)
> > > +{
> > > +     dwc3_mb86s70_clk_control(dev, true);
> > > +
> > > +     return 0;
> > > +}
> > > +#endif /* CONFIG_PM_RUNTIME */
> > > +static int dwc3_mb86s70_suspend(struct device *dev)
> > > +{
> > > +     if (pm_runtime_status_suspended(dev))
> > > +             return 0;
> > > +
> > > +     return dwc3_mb86s70_runtime_suspend(dev);
> >
> > ok, this is a bit confusing and, in fact, wrong. You're not dealing with
> > wakeups (which are mandatory on runtime_pm and option on system sleep).
> >
> > It's okay to have a common function called by both runtime_pm and
> > system_sleep methods, but calling your runtime_pm handler is a bit odd.
> >
> 
> hm, sorry I might confuse people here.
> I'll change this to what you suggested.
> 
> 
> >
> > last but not least, I need to see results of this running on top of
> > v3.19-rc1. Are you using both host and peripheral modes or only one mode
> > for now ?
> >
> 
> The dwc3 IP we adapted is host-only, and we run this platform glue driver
> in v3.19-rc1 for now.
> bellow log is about suspend and resume successfully, and dwc3 still works.
> 
> root@linaro-nano:~# echo mem > /sys/power/state
> [   56.713281] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_suspend() is
> started.
> [   56.720721] mb86s70-dwc3 mb86s70_usb3host1.5:
> dwc3_mb86s70_runtime_suspend() is started.
> [   56.729419] mb86s70-dwc3 mb86s70_usb3host1.5:
> dwc3_mb86s70_runtime_suspend() is ended.
> [   56.737363] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_suspend() is
> ended.
> [   56.744604] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_suspend() is
> started.
> [   56.752022] mb86s70-dwc3 mb86s70_usb3host0.4:
> dwc3_mb86s70_runtime_suspend() is started.
> [   56.760700] mb86s70-dwc3 mb86s70_usb3host0.4:
> dwc3_mb86s70_runtime_suspend() is ended.
> [   56.768639] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_suspend() is
> ended.
> [   57.107139] SUSPEND
> 
> [   74.564963] mmc0: execute_tuning --
> [   74.877776] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_resume() is
> started.
> [   74.885126] mb86s70-dwc3 mb86s70_usb3host0.4:
> dwc3_mb86s70_runtime_resume() is started.
> [   74.893749] mb86s70-dwc3 mb86s70_usb3host0.4:
> dwc3_mb86s70_runtime_resume() is ended.
> [   74.901606] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_resume() is
> ended.
> [   74.908766] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_resume() is
> started.
> [   74.916086] mb86s70-dwc3 mb86s70_usb3host1.5:
> dwc3_mb86s70_runtime_resume() is started.
> [   74.924660] mb86s70-dwc3 mb86s70_usb3host1.5:
> dwc3_mb86s70_runtime_resume() is ended.
> [   74.932513] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_resume() is
> ended.
> root@linaro-nano:~#
> 
> But I realize something might confuse people, and even is wrong, through
> your comment.
> So I guess I should align these to a more clear form like dwc3-exynos.c :
> 
> static int dwc3_exynos_suspend(struct device *dev)
> {
> struct dwc3_exynos *exynos = dev_get_drvdata(dev);
> 
> clk_disable(exynos->clk);
> 
> return 0;
> }
> 
> static int dwc3_exynos_resume(struct device *dev)
> {
> struct dwc3_exynos *exynos = dev_get_drvdata(dev);
> 
> clk_enable(exynos->clk);
> 
> /* runtime set active to reflect active state. */
> pm_runtime_disable(dev);
> pm_runtime_set_active(dev);
> pm_runtime_enable(dev);
> 
> return 0;
> }
> 
> Would it been better than how I do suspend/resume here?

sure, that would be better :-)

> however recently I got some thought inspired by Arnd and the commit
> 5f872175c867a839e2feeecfa59e3db38c88b561

where is that commit ?

$ git show 5f872175c867a839e2feeecfa59e3db38c88b561
fatal: bad object 5f872175c867a839e2feeecfa59e3db38c88b561

I have here linus and linux-next just fetched before trying to git show.

> For imitating that in my experiment recently, i only need to add clock
> on/off in dwc3 core.

Let's wait a little bit more before doing that. We have a ton of users
which don't have controllable clocks and, frankly speaking, if we hide
all clock control on glue layer (which is a parent device of core) it
will still work just fine because of the ordering of children vs parent
that device core gives you for free :-)

> Then dwc3 core won't be always created via sub node in platform glue node,
> and vendors like us can just drop platform glue which don't have any
> specific platform code to wrap dwc3 core, and just directly use dwc3 core
> via device tree.

If your core is really just xhci, why don't just use xhci-plat.c
directly ? You don't need dwc3 at all. In any case, I refrain from
allowing people to use dwc3.ko directly because that has bitten me in
the past with MUSB, so sorry but I'll always force people to use the
glue layer as that helps me keep dwc3 clean of platform-specific
details.

> Of course here dwc3 core won't process clock if he got null one, so this
> won't affect other vendor's platform which only pass clock phandle to their
> dwc3 platform glue driver.

right

> Just wondering if it's better to write a platform glue only did clock
> on/off, or to do the same improvement people ever did for
> ehci-platform driver and just drop this platform glue driver?

I'll always the sour taste in my mouth due to MUSB. The thing is a
complete mess of platform-specific hacks all over the place.

> Hope I didn't reply too late, or make any impolite move here,

merge window just closed, we've got time.

> Super thanks for every comment you made here.

hey, no problem.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Sneeker Yeh]

[-- Attachment #1: Type: text/plain, Size: 16001 bytes --]

HI Balbi,

Thanks for your suggestion,

2014-12-30 0:14 GMT+08:00 Felipe Balbi <balbi@ti.com>:

> Hi,
>
> On Mon, Dec 29, 2014 at 01:52:04AM +0800, Sneeker Yeh wrote:
> > Hi,
> >
> > 2014-12-22 23:59 GMT+08:00 Felipe Balbi <balbi@ti.com>:
> >
> > > Hi,
> > >
> > > On Tue, Dec 16, 2014 at 10:10:27AM +0800, Sneeker Yeh wrote:
> > > > This patch adds support for Synopsis DesignWare USB3 IP Core found
> > > > on Fujitsu Socs.
> > > >
> > > > Signed-off-by: Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>
> > > > ---
> > > >  .../devicetree/bindings/usb/fujitsu-dwc3.txt       |   25 +++
> > > >  drivers/usb/dwc3/Kconfig                           |   11 ++
> > > >  drivers/usb/dwc3/Makefile                          |    1 +
> > > >  drivers/usb/dwc3/dwc3-mb86s70.c                    |  194
> > > ++++++++++++++++++++
> > > >  4 files changed, 231 insertions(+)
> > > >  create mode 100644
> > > Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > > >  create mode 100644 drivers/usb/dwc3/dwc3-mb86s70.c
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > > b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > > > new file mode 100644
> > > > index 0000000..df77f91
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/usb/fujitsu-dwc3.txt
> > > > @@ -0,0 +1,25 @@
> > > > +FUJITSU GLUE COMPONENTS
> > > > +
> > > > +MB86S7x DWC3 GLUE
> > > > + - compatible : Should be "fujitsu,mb86s70-dwc3"
> > > > + - clocks: from common clock binding, handle to usb clock.
> > > > + - clock-names: from common clock binding.
> > > > + - #address-cells, #size-cells : Must be present if the device has
> > > sub-nodes
> > > > + - ranges: the child address space are mapped 1:1 onto the parent
> > > address space
> > > > + - #stream-id-cells : handle to use "arm,mmu-400" ARM IOMMU driver
> > > > +
> > > > +Sub-nodes:
> > > > +The dwc3 core should be added as subnode to MB86S7x dwc3 glue.
> > > > +- dwc3 :
> > > > +   The binding details of dwc3 can be found in:
> > > > +   Documentation/devicetree/bindings/usb/dwc3.txt
> > > > +
> > > > +usb3host: mb86s70_usb3host {
> > > > +     compatible = "fujitsu,mb86s70-dwc3";
> > > > +     clocks = <&clk_alw_1_1>;
> > > > +     clock-names = "bus_clk";
> > > > +     #address-cells = <2>;
> > > > +     #size-cells = <1>;
> > > > +     ranges;
> > > > +     #stream-id-cells = <0>;
> > > > +};
> > > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> > > > index 58b5b2c..3390d42 100644
> > > > --- a/drivers/usb/dwc3/Kconfig
> > > > +++ b/drivers/usb/dwc3/Kconfig
> > > > @@ -61,6 +61,17 @@ config USB_DWC3_EXYNOS
> > > >         Recent Exynos5 SoCs ship with one DesignWare Core USB3 IP
> inside,
> > > >         say 'Y' or 'M' if you have one such device.
> > > >
> > > > +config USB_DWC3_MB86S70
> > > > +     tristate "MB86S70 Designware USB3 Platform code"
> > > > +     default USB_DWC3
> > > > +     help
> > > > +       MB86S7X SOC ship with DesignWare Core USB3 IP inside,
> > > > +       this implementation also integrated Fujitsu USB PHY inside
> > > > +       this Core USB3 IP.
> > > > +
> > > > +       say 'Y' or 'M' if you have one such device.
> > > > +
> > > > +
> > > >  config USB_DWC3_PCI
> > > >       tristate "PCIe-based Platforms"
> > > >       depends on PCI
> > > > diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> > > > index bb34fbc..05d1de2 100644
> > > > --- a/drivers/usb/dwc3/Makefile
> > > > +++ b/drivers/usb/dwc3/Makefile
> > > > @@ -38,3 +38,4 @@ obj-$(CONFIG_USB_DWC3_PCI)          += dwc3-pci.o
> > > >  obj-$(CONFIG_USB_DWC3_KEYSTONE)              += dwc3-keystone.o
> > > >  obj-$(CONFIG_USB_DWC3_QCOM)          += dwc3-qcom.o
> > > >  obj-$(CONFIG_USB_DWC3_ST)            += dwc3-st.o
> > > > +obj-$(CONFIG_USB_DWC3_MB86S70)               += dwc3-mb86s70.o
> > > > diff --git a/drivers/usb/dwc3/dwc3-mb86s70.c
> > > b/drivers/usb/dwc3/dwc3-mb86s70.c
> > > > new file mode 100644
> > > > index 0000000..241c5bd
> > > > --- /dev/null
> > > > +++ b/drivers/usb/dwc3/dwc3-mb86s70.c
> > > > @@ -0,0 +1,194 @@
> > > > +/**
> > > > + * dwc3-mb86s70.c - Fujitsu mb86s70 DWC3 Specific Glue layer
> > > > + *
> > > > + * Copyright (c) 2013 - 2014 FUJITSU SEMICONDUCTOR LIMITED
> > > > + *           http://jp.fujitsu.com/group/fsl
> > > > + *
> > > > + * Author: Alice Chan <Alice.Chan@tw.fujitsu.com>
> > > > + * based on dwc3-exynos.c
> > > > + *
> > > > + * This program is free software; you can redistribute it and/or
> modify
> > > > + * it under the terms of the GNU General Public License as
> published by
> > > > + * the Free Software Foundation; either version 2 of the License, or
> > > > + * (at your option) any later version.
> > > > + */
> > > > +
> > > > +#include <linux/module.h>
> > > > +#include <linux/kernel.h>
> > > > +#include <linux/slab.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/dma-mapping.h>
> > > > +#include <linux/interrupt.h>
> > > > +#include <linux/of.h>
> > > > +#include <linux/of_platform.h>
> > > > +#include <linux/pm_runtime.h>
> > > > +#include <linux/clk.h>
> > > > +
> > > > +struct dwc3_mb86s70 {
> > > > +     struct device   *dev;
> > > > +     struct clk              **clk_table;
> > >
> > > please align these.
> >
> >
> > okay.
> >
> >
> > >
> >
> >
> > > > +};
> > > > +
> > > > +/* return 0 means successful */
> > >
> > > this comment is unnecessary :-)
> > >
> >
> > okay
> >
> >
> > >
> > > > +static int dwc3_mb86s70_clk_control(struct device *dev, bool on)
> > > > +{
> > > > +     int ret, i;
> > > > +     struct clk *clk;
> > > > +
> > > > +     if (!on)
> > > > +             goto clock_off;
> > > > +
> > > > +     for (i = 0;; i++) {
> > > > +             clk = of_clk_get(dev->of_node, i);
> > >
> > > you could count the number of properties first, then allocate clk_table
> > > to the exact size.
> > >
> >
> > okay, thanks.
> >
> >
> > >
> > > > +             if (IS_ERR(clk))
> > > > +                     break;
> > > > +
> > > > +             ret = clk_prepare_enable(clk);
> > > > +             if (ret) {
> > > > +                     dev_err(dev, "failed to enable clock[%d]\n",
> i);
> > > > +                     goto clock_off;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +
> > > > +clock_off:
> > > > +     for (i = 0;; i++) {
> > > > +             clk = of_clk_get(dev->of_node, i);
> > > > +             if (IS_ERR(clk))
> > > > +                     break;
> > > > +
> > > > +             clk_disable_unprepare(clk);
> > > > +     }
> > > > +
> > > > +     return on;
> > > > +}
> > > > +
> > > > +static int dwc3_mb86s70_remove_child(struct device *dev, void
> *unused)
> > > > +{
> > > > +     struct platform_device *pdev = to_platform_device(dev);
> > > > +
> > > > +     of_device_unregister(pdev);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
> > >
> > > why ? Use dma_coerce_mask_and_coherent().
> > >
> >
> > okay.
> >
> >
> > >
> > > > +
> > > > +static int dwc3_mb86s70_probe(struct platform_device *pdev)
> > > > +{
> > > > +     struct dwc3_mb86s70     *mb86s70;
> > > > +     struct device           *dev = &pdev->dev;
> > > > +     struct device_node      *node = dev->of_node;
> > > > +     int                     ret;
> > > > +
> > > > +     mb86s70 = devm_kzalloc(dev, sizeof(*mb86s70), GFP_KERNEL);
> > > > +     if (!mb86s70) {
> > > > +             ret = -ENOMEM;
> > > > +             goto err;
> > > > +     }
> > > > +
> > > > +     dev->dma_mask = &dwc3_mb86s70_dma_mask;
> > > > +
> > > > +     platform_set_drvdata(pdev, mb86s70);
> > > > +
> > > > +     mb86s70->dev = dev;
> > > > +
> > > > +     /* resume driver for clock, power */
> > > > +     pm_runtime_enable(&pdev->dev);
> > > > +     ret = pm_runtime_get_sync(&pdev->dev);
> > >
> > > here's a slightly better way (add error checking where necessary):
> > >
> > >         dwc3_mb86s70_clk_control(dev, true);
> > >         pm_runtime_set_active(dev);
> > >         pm_runtime_enable(dev);
> > >         pm_runtime_get(dev);
> > >
> > >
> > okay, thanks,
> > these seems much better and more correct.
> >
> >
> >
> > >
> > > > +     if (ret < 0) {
> > > > +             dev_err(&pdev->dev, "get_sync failed with err %d\n",
> ret);
> > > > +             goto err;
> > >
> > > if get_sync() fails, you still need to call pm_runtime_put_sync(); In
> > > this case, you also need to call pm_runtime_disable();
> > >
> >
> > okay, thanks.
> >
> >
> > >
> > > > +     }
> > > > +
> > > > +     if (node) {
> > > > +             ret = of_platform_populate(node, NULL, NULL, dev);
> > > > +             if (!ret)
> > > > +                     return 0;
> > > > +             dev_err(dev, "failed to add dwc3 core\n");
> > > > +     }
> > > > +     dev_err(dev, "no device node, failed to add dwc3 core\n");
> > > > +     ret = -ENODEV;
> > > > +
> > > > +err:
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +static int dwc3_mb86s70_remove(struct platform_device *pdev)
> > > > +{
> > > > +     device_for_each_child(&pdev->dev, NULL,
> dwc3_mb86s70_remove_child);
> > > > +
> > > > +     pm_runtime_put_sync(&pdev->dev);
> > > > +     pm_runtime_disable(&pdev->dev);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static const struct of_device_id mb86s70_dwc3_match[] = {
> > > > +     { .compatible = "fujitsu,mb86s70-dwc3" },
> > > > +     {},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, mb86s70_dwc3_match);
> > > > +
> > > > +#ifdef CONFIG_PM
> > > > +#ifdef CONFIG_PM_RUNTIME
> > > > +static int dwc3_mb86s70_runtime_suspend(struct device *dev)
> > > > +{
> > > > +     dwc3_mb86s70_clk_control(dev, false);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int  dwc3_mb86s70_runtime_resume(struct device *dev)
> > > > +{
> > > > +     dwc3_mb86s70_clk_control(dev, true);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +#endif /* CONFIG_PM_RUNTIME */
> > > > +static int dwc3_mb86s70_suspend(struct device *dev)
> > > > +{
> > > > +     if (pm_runtime_status_suspended(dev))
> > > > +             return 0;
> > > > +
> > > > +     return dwc3_mb86s70_runtime_suspend(dev);
> > >
> > > ok, this is a bit confusing and, in fact, wrong. You're not dealing
> with
> > > wakeups (which are mandatory on runtime_pm and option on system sleep).
> > >
> > > It's okay to have a common function called by both runtime_pm and
> > > system_sleep methods, but calling your runtime_pm handler is a bit odd.
> > >
> >
> > hm, sorry I might confuse people here.
> > I'll change this to what you suggested.
> >
> >
> > >
> > > last but not least, I need to see results of this running on top of
> > > v3.19-rc1. Are you using both host and peripheral modes or only one
> mode
> > > for now ?
> > >
> >
> > The dwc3 IP we adapted is host-only, and we run this platform glue driver
> > in v3.19-rc1 for now.
> > bellow log is about suspend and resume successfully, and dwc3 still
> works.
> >
> > root@linaro-nano:~# echo mem > /sys/power/state
> > [   56.713281] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_suspend()
> is
> > started.
> > [   56.720721] mb86s70-dwc3 mb86s70_usb3host1.5:
> > dwc3_mb86s70_runtime_suspend() is started.
> > [   56.729419] mb86s70-dwc3 mb86s70_usb3host1.5:
> > dwc3_mb86s70_runtime_suspend() is ended.
> > [   56.737363] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_suspend()
> is
> > ended.
> > [   56.744604] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_suspend()
> is
> > started.
> > [   56.752022] mb86s70-dwc3 mb86s70_usb3host0.4:
> > dwc3_mb86s70_runtime_suspend() is started.
> > [   56.760700] mb86s70-dwc3 mb86s70_usb3host0.4:
> > dwc3_mb86s70_runtime_suspend() is ended.
> > [   56.768639] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_suspend()
> is
> > ended.
> > [   57.107139] SUSPEND
> >
> > [   74.564963] mmc0: execute_tuning --
> > [   74.877776] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_resume() is
> > started.
> > [   74.885126] mb86s70-dwc3 mb86s70_usb3host0.4:
> > dwc3_mb86s70_runtime_resume() is started.
> > [   74.893749] mb86s70-dwc3 mb86s70_usb3host0.4:
> > dwc3_mb86s70_runtime_resume() is ended.
> > [   74.901606] mb86s70-dwc3 mb86s70_usb3host0.4: dwc3_mb86s70_resume() is
> > ended.
> > [   74.908766] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_resume() is
> > started.
> > [   74.916086] mb86s70-dwc3 mb86s70_usb3host1.5:
> > dwc3_mb86s70_runtime_resume() is started.
> > [   74.924660] mb86s70-dwc3 mb86s70_usb3host1.5:
> > dwc3_mb86s70_runtime_resume() is ended.
> > [   74.932513] mb86s70-dwc3 mb86s70_usb3host1.5: dwc3_mb86s70_resume() is
> > ended.
> > root@linaro-nano:~#
> >
> > But I realize something might confuse people, and even is wrong, through
> > your comment.
> > So I guess I should align these to a more clear form like dwc3-exynos.c :
> >
> > static int dwc3_exynos_suspend(struct device *dev)
> > {
> > struct dwc3_exynos *exynos = dev_get_drvdata(dev);
> >
> > clk_disable(exynos->clk);
> >
> > return 0;
> > }
> >
> > static int dwc3_exynos_resume(struct device *dev)
> > {
> > struct dwc3_exynos *exynos = dev_get_drvdata(dev);
> >
> > clk_enable(exynos->clk);
> >
> > /* runtime set active to reflect active state. */
> > pm_runtime_disable(dev);
> > pm_runtime_set_active(dev);
> > pm_runtime_enable(dev);
> >
> > return 0;
> > }
> >
> > Would it been better than how I do suspend/resume here?
>
> sure, that would be better :-)
>
> > however recently I got some thought inspired by Arnd and the commit
> > 5f872175c867a839e2feeecfa59e3db38c88b561
>
> where is that commit ?
>
> $ git show 5f872175c867a839e2feeecfa59e3db38c88b561
> fatal: bad object 5f872175c867a839e2feeecfa59e3db38c88b561
>
> I have here linus and linux-next just fetched before trying to git show.
>

sorry, it should be this one:
a4aeb2117571292f4e002c54b3f91e138722bf7a


>
> > For imitating that in my experiment recently, i only need to add clock
> > on/off in dwc3 core.
>
> Let's wait a little bit more before doing that. We have a ton of users
> which don't have controllable clocks and, frankly speaking, if we hide
> all clock control on glue layer (which is a parent device of core) it
> will still work just fine because of the ordering of children vs parent
> that device core gives you for free :-)
>

hm, I see.


>
> > Then dwc3 core won't be always created via sub node in platform glue
> node,
> > and vendors like us can just drop platform glue which don't have any
> > specific platform code to wrap dwc3 core, and just directly use dwc3 core
> > via device tree.
>
> If your core is really just xhci, why don't just use xhci-plat.c
>

I've tried using xhci-plat.c, but our controller won't work today in
3.19-rc1.
Seems I still need the dwc3 initialization code.


> directly ? You don't need dwc3 at all. In any case, I refrain from
> allowing people to use dwc3.ko directly because that has bitten me in
> the past with MUSB, so sorry but I'll always force people to use the
> glue layer as that helps me keep dwc3 clean of platform-specific
> details.
>
> > Of course here dwc3 core won't process clock if he got null one, so this
> > won't affect other vendor's platform which only pass clock phandle to
> their
> > dwc3 platform glue driver.
>
> right
>
> > Just wondering if it's better to write a platform glue only did clock
> > on/off, or to do the same improvement people ever did for
> > ehci-platform driver and just drop this platform glue driver?
>
> I'll always the sour taste in my mouth due to MUSB. The thing is a
> complete mess of platform-specific hacks all over the place.
>
>
Super thanks for sharing this, for me it's quite an honor to know that
story behond.
and I'll take these suggestion here to revise our platform glue layer.

Much appreciate,
Sneeker


> > Hope I didn't reply too late, or make any impolite move here,
>
> merge window just closed, we've got time.
>
> > Super thanks for every comment you made here.
>
> hey, no problem.
>
> --
> balbi
>

[-- Attachment #2: Type: text/html, Size: 22680 bytes --]

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1748 bytes --]

Hi,

On Sun, Jan 04, 2015 at 09:16:01PM +0800, Sneeker Yeh wrote:
> > > Then dwc3 core won't be always created via sub node in platform glue
> > node,
> > > and vendors like us can just drop platform glue which don't have any
> > > specific platform code to wrap dwc3 core, and just directly use dwc3 core
> > > via device tree.
> >
> > If your core is really just xhci, why don't just use xhci-plat.c
> >
> 
> I've tried using xhci-plat.c, but our controller won't work today in
> 3.19-rc1.
> Seems I still need the dwc3 initialization code.

probably missing PHY initialization, that should be easy to patch on
xhci-plat.c

> > directly ? You don't need dwc3 at all. In any case, I refrain from
> > allowing people to use dwc3.ko directly because that has bitten me in
> > the past with MUSB, so sorry but I'll always force people to use the
> > glue layer as that helps me keep dwc3 clean of platform-specific
> > details.
> >
> > > Of course here dwc3 core won't process clock if he got null one, so this
> > > won't affect other vendor's platform which only pass clock phandle to
> > their
> > > dwc3 platform glue driver.
> >
> > right
> >
> > > Just wondering if it's better to write a platform glue only did clock
> > > on/off, or to do the same improvement people ever did for
> > > ehci-platform driver and just drop this platform glue driver?
> >
> > I'll always the sour taste in my mouth due to MUSB. The thing is a
> > complete mess of platform-specific hacks all over the place.
> >
> >
> Super thanks for sharing this, for me it's quite an honor to know that
> story behond.
> and I'll take these suggestion here to revise our platform glue layer.

hey, no problem. Cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Arnd Bergmann]

On Monday 29 December 2014 01:52:04 Sneeker Yeh wrote:
> > > +static int dwc3_mb86s70_remove_child(struct device *dev, void *unused)
> > > +{
> > > +     struct platform_device *pdev = to_platform_device(dev);
> > > +
> > > +     of_device_unregister(pdev);
> > > +
> > > +     return 0;
> > > +}
> > > +
> > > +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
> >
> > why ? Use dma_coerce_mask_and_coherent().
> >
> 
> okay.

Actually that is still wrong: we use dma_coerce_mask_and_coherent() to
annotate drivers that have traditionally been forcing their own dma mask
by some other means and that need to be changed to something proper (after
finding out why they did it in the first place).

Since this is about a child device, the correct interface is to use
platform_device_register_full().

Note that this will still only work on machines that have no dma offset,
iommu, swiotlb, special coherency requirements, or ones that need a
platform specific dma_map_ops. To make any of those work, the driver
would need to be changed to pass the correct device pointer into any
dma-mapping interfaces, so the settings can get set by the platform
code. We will need to do that for arm64 I assume.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1140 bytes --]

Hi,

On Tue, Dec 30, 2014 at 11:12:51AM +0100, Arnd Bergmann wrote:
> On Monday 29 December 2014 01:52:04 Sneeker Yeh wrote:
> > > > +static int dwc3_mb86s70_remove_child(struct device *dev, void *unused)
> > > > +{
> > > > +     struct platform_device *pdev = to_platform_device(dev);
> > > > +
> > > > +     of_device_unregister(pdev);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
> > >
> > > why ? Use dma_coerce_mask_and_coherent().
> > >
> > 
> > okay.
> 
> Actually that is still wrong: we use dma_coerce_mask_and_coherent() to
> annotate drivers that have traditionally been forcing their own dma mask
> by some other means and that need to be changed to something proper (after
> finding out why they did it in the first place).
> 
> Since this is about a child device, the correct interface is to use
> platform_device_register_full().

no, that's wrong. He's essentially fixing the default set by OF core,
which is always 32-bits anyway, so this can actually be removed. Your
suggestion would just make it worse.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Arnd Bergmann]

On Friday 02 January 2015 19:21:28 Felipe Balbi wrote:
> 
> On Tue, Dec 30, 2014 at 11:12:51AM +0100, Arnd Bergmann wrote:
> > On Monday 29 December 2014 01:52:04 Sneeker Yeh wrote:
> > > > > +static int dwc3_mb86s70_remove_child(struct device *dev, void *unused)
> > > > > +{
> > > > > +     struct platform_device *pdev = to_platform_device(dev);
> > > > > +
> > > > > +     of_device_unregister(pdev);
> > > > > +
> > > > > +     return 0;
> > > > > +}
> > > > > +
> > > > > +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
> > > >
> > > > why ? Use dma_coerce_mask_and_coherent().
> > > >
> > > 
> > > okay.
> > 
> > Actually that is still wrong: we use dma_coerce_mask_and_coherent() to
> > annotate drivers that have traditionally been forcing their own dma mask
> > by some other means and that need to be changed to something proper (after
> > finding out why they did it in the first place).
> > 
> > Since this is about a child device, the correct interface is to use
> > platform_device_register_full().
> 
> no, that's wrong. He's essentially fixing the default set by OF core,
> which is always 32-bits anyway, so this can actually be removed. Your
> suggestion would just make it worse.

Ah, so this was for a device that gets probed from DT? Yes, then no
change is needed here at all.

Note that I'm working on a patch set to change the DT initialization to
be a little smarter about the mask. Specifically any device whose parent
has a smaller dma-ranges window than 4GB needs to start out with the
smaller mask, and the dma_set_mask function that is called to allow 64-bit
DMA on a device will check if the bus can actually support it.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2165 bytes --]

Hi,

On Mon, Jan 05, 2015 at 04:28:34PM +0100, Arnd Bergmann wrote:
> On Friday 02 January 2015 19:21:28 Felipe Balbi wrote:
> > 
> > On Tue, Dec 30, 2014 at 11:12:51AM +0100, Arnd Bergmann wrote:
> > > On Monday 29 December 2014 01:52:04 Sneeker Yeh wrote:
> > > > > > +static int dwc3_mb86s70_remove_child(struct device *dev, void *unused)
> > > > > > +{
> > > > > > +     struct platform_device *pdev = to_platform_device(dev);
> > > > > > +
> > > > > > +     of_device_unregister(pdev);
> > > > > > +
> > > > > > +     return 0;
> > > > > > +}
> > > > > > +
> > > > > > +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
> > > > >
> > > > > why ? Use dma_coerce_mask_and_coherent().
> > > > >
> > > > 
> > > > okay.
> > > 
> > > Actually that is still wrong: we use dma_coerce_mask_and_coherent() to
> > > annotate drivers that have traditionally been forcing their own dma mask
> > > by some other means and that need to be changed to something proper (after
> > > finding out why they did it in the first place).
> > > 
> > > Since this is about a child device, the correct interface is to use
> > > platform_device_register_full().
> > 
> > no, that's wrong. He's essentially fixing the default set by OF core,
> > which is always 32-bits anyway, so this can actually be removed. Your
> > suggestion would just make it worse.
> 
> Ah, so this was for a device that gets probed from DT? Yes, then no
> change is needed here at all.

right.

> Note that I'm working on a patch set to change the DT initialization to
> be a little smarter about the mask. Specifically any device whose parent
> has a smaller dma-ranges window than 4GB needs to start out with the
> smaller mask, and the dma_set_mask function that is called to allow 64-bit
> DMA on a device will check if the bus can actually support it.

please make sure have that patch soaking in linux-next for weeks before
attempting to merge it upstream. I would target v3.21 with such a patch.
At least keystone might be a little weird about what you describe
considering the thing boots from HIGHMEM. Also needs to consider LPAE,
etc.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/3] usb: dwc3: add Fujitsu Specific Glue layer

[Sneeker Yeh]

[-- Attachment #1: Type: text/plain, Size: 2368 bytes --]

HI Arnd,

Thanks for reviewing this,

2014-12-30 18:12 GMT+08:00 Arnd Bergmann <arnd@arndb.de>:

> On Monday 29 December 2014 01:52:04 Sneeker Yeh wrote:
> > > > +static int dwc3_mb86s70_remove_child(struct device *dev, void
> *unused)
> > > > +{
> > > > +     struct platform_device *pdev = to_platform_device(dev);
> > > > +
> > > > +     of_device_unregister(pdev);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static u64 dwc3_mb86s70_dma_mask = DMA_BIT_MASK(32);
> > >
> > > why ? Use dma_coerce_mask_and_coherent().
> > >
> >
> > okay.
>
> Actually that is still wrong: we use dma_coerce_mask_and_coherent() to
> annotate drivers that have traditionally been forcing their own dma mask
> by some other means and that need to be changed to something proper (after
> finding out why they did it in the first place).
>

thanks,I remove this and test can be passed,
of_platform_populate() makes dwc3 owns a default one and don't have to
reference parent device's mask i set here.

if later i worry dwc3 might still not get a proper mask from
of_platform_populate() in our platform,
then i shall set a proper one via platform bus notifier somewhere else,
IIUC.


> Since this is about a child device, the correct interface is to use
> platform_device_register_full().
>
>
dwc3 core usually can be describe as sub node in platform glue device node:

usb3host0: mb86s70_usb3host0 {
    compatible = "fujitsu,mb86s70-dwc3";
    clocks = <&clock 0 1 1>;
    clock-names = "h_clk";
    #address-cells = <2>;
    #size-cells = <1>;
    ranges;

    dwc3@32200000 {
        compatible = "synopsys,dwc3";
        reg = <0 0x32200000 0x100000>;
        interrupts = <0 292 0x4>,
        <0 294 0x4>;
    };
};

so that make me able to use of_platform_populate() to create dwc3 device in
our platform glue device.


> Note that this will still only work on machines that have no dma offset,
> iommu, swiotlb, special coherency requirements, or ones that need a
> platform specific dma_map_ops. To make any of those work, the driver
> would need to be changed to pass the correct device pointer into any
> dma-mapping interfaces, so the settings can get set by the platform
> code. We will need to do that for arm64 I assume.
>

thanks for the teaching, for me it's gold and i'm always enjoying absorbing
these.

Much appreciate,
Sneeker



>
>         Arnd
>

[-- Attachment #2: Type: text/html, Size: 3822 bytes --]

[PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Sneeker Yeh]

Synopsis DesignWare USB3 IP Core integrated with a config-free
phy needs special handling during device disconnection to avoid
the host controller dying.

This quirk makes sure PORT_CSC is cleared after the disable slot
command when usb device is disconnected from internal root hub,
otherwise, Synopsis core would fall into a state that cannot use
any endpoint command. Consequently, device disconnection procedure
might not be finished because sometimes endpoint need to be stop
by endpoint stop command issuing.

Symptom usually happens when disconnected device is keyboard,
mouse, and hub.

Signed-off-by: Sneeker Yeh <Sneeker.Yeh@tw.fujitsu.com>
---
 drivers/usb/host/xhci-hub.c  |    4 ++++
 drivers/usb/host/xhci-plat.c |    5 +++++
 drivers/usb/host/xhci.c      |   29 +++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h      |    7 +++++++
 4 files changed, 45 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index a7865c4..3b8f7fc 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -368,6 +368,10 @@ static void xhci_clear_port_change_bit(struct xhci_hcd *xhci, u16 wValue,
 		port_change_bit = "warm(BH) reset";
 		break;
 	case USB_PORT_FEAT_C_CONNECTION:
+		if ((xhci->quirks & XHCI_DISCONNECT_QUIRK) &&
+		    !(readl(addr) & PORT_CONNECT))
+			return;
+
 		status = PORT_CSC;
 		port_change_bit = "connect";
 		break;
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 08d402b..3ede6b4 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -33,6 +33,11 @@ static void xhci_plat_quirks(struct device *dev, struct xhci_hcd *xhci)
 	 * dev struct in order to setup MSI
 	 */
 	xhci->quirks |= XHCI_PLAT;
+
+	if (dev->parent && dev->parent->parent &&
+	    of_device_is_compatible(dev->parent->parent->of_node,
+				    "fujitsu,mb86s70-dwc3"))
+		xhci->quirks |= XHCI_DISCONNECT_QUIRK;
 }
 
 /* called during probe() after chip reset completes */
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 01fcbb5..50d757b 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -3584,6 +3584,33 @@ command_cleanup:
 	return ret;
 }
 
+static void xhci_try_to_clear_csc(struct usb_hcd *hcd, int dev_port_num)
+{
+	int max_ports;
+	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+	__le32 __iomem **port_array;
+	u32 status;
+
+	/* print debug info */
+	if (hcd->speed == HCD_USB3) {
+		max_ports = xhci->num_usb3_ports;
+		port_array = xhci->usb3_ports;
+	} else {
+		max_ports = xhci->num_usb2_ports;
+		port_array = xhci->usb2_ports;
+	}
+
+	if (dev_port_num > max_ports) {
+		xhci_err(xhci, "%s() port number invalid", __func__);
+		return;
+	}
+	status = readl(port_array[dev_port_num - 1]);
+
+	/* write 1 to clear */
+	if (!(status & PORT_CONNECT) && (status & PORT_CSC))
+		writel(status & PORT_CSC, port_array[0]);
+}
+
 /*
  * At this point, the struct usb_device is about to go away, the device has
  * disconnected, and all traffic has been stopped and the endpoints have been
@@ -3649,6 +3676,8 @@ void xhci_free_dev(struct usb_hcd *hcd, struct usb_device *udev)
 	xhci_ring_cmd_db(xhci);
 	spin_unlock_irqrestore(&xhci->lock, flags);
 
+	if (xhci->quirks & XHCI_DISCONNECT_QUIRK)
+		xhci_try_to_clear_csc(hcd, udev->portnum);
 	/*
 	 * Event command completion handler will free any data structures
 	 * associated with the slot.  XXX Can free sleep?
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index cc7c5bb..e6c820c 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1560,6 +1560,13 @@ struct xhci_hcd {
 #define XHCI_SPURIOUS_WAKEUP	(1 << 18)
 /* For controllers with a broken beyond repair streams implementation */
 #define XHCI_BROKEN_STREAMS	(1 << 19)
+/*
+ * Synopsis dwc3 core integrated with a config-free phy needs special
+ * handling during device disconnection to avoid the host controller
+ * dying. This quirk makes sure PORT_CSC is cleared after the disable
+ * slot command.
+ */
+#define XHCI_DISCONNECT_QUIRK	(1 << 20)
 	unsigned int		num_active_eps;
 	unsigned int		limit_active_eps;
 	/* There are two roothubs to keep track of bus suspend info for */
-- 
1.7.9.5

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 857 bytes --]

On Tue, Dec 16, 2014 at 10:10:28AM +0800, Sneeker Yeh wrote:
> Synopsis DesignWare USB3 IP Core integrated with a config-free
> phy needs special handling during device disconnection to avoid
> the host controller dying.
> 
> This quirk makes sure PORT_CSC is cleared after the disable slot
> command when usb device is disconnected from internal root hub,
> otherwise, Synopsis core would fall into a state that cannot use
> any endpoint command. Consequently, device disconnection procedure
> might not be finished because sometimes endpoint need to be stop
> by endpoint stop command issuing.
> 
> Symptom usually happens when disconnected device is keyboard,
> mouse, and hub.

you need to point us to the synopsys STARS ticket number. That's the
only way to reference this specific quirk. Add it to a comment
somewhere.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Sneeker Yeh]

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

Hi,

2014-12-22 23:37 GMT+08:00 Felipe Balbi <balbi@ti.com>:

> On Tue, Dec 16, 2014 at 10:10:28AM +0800, Sneeker Yeh wrote:
> > Synopsis DesignWare USB3 IP Core integrated with a config-free
> > phy needs special handling during device disconnection to avoid
> > the host controller dying.
> >
> > This quirk makes sure PORT_CSC is cleared after the disable slot
> > command when usb device is disconnected from internal root hub,
> > otherwise, Synopsis core would fall into a state that cannot use
> > any endpoint command. Consequently, device disconnection procedure
> > might not be finished because sometimes endpoint need to be stop
> > by endpoint stop command issuing.
> >
> > Symptom usually happens when disconnected device is keyboard,
> > mouse, and hub.
>
> you need to point us to the synopsys STARS ticket number. That's the
> only way to reference this specific quirk. Add it to a comment
> somewhere.
>

Thanks, we're still waiting for Synopsis STARS ticket number.
I'll add it to comment later.

Much Appreciate,
Sneeker


>
> --
> balbi
>

[-- Attachment #2: Type: text/html, Size: 1795 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Sneeker Yeh]

[-- Attachment #1: Type: text/plain, Size: 1628 bytes --]

Hi,

2014-12-29 14:41 GMT+08:00 Sneeker Yeh <sneeker.yeh@gmail.com>:

> Hi,
>
> 2014-12-22 23:37 GMT+08:00 Felipe Balbi <balbi@ti.com>:
>
>> On Tue, Dec 16, 2014 at 10:10:28AM +0800, Sneeker Yeh wrote:
>> > Synopsis DesignWare USB3 IP Core integrated with a config-free
>> > phy needs special handling during device disconnection to avoid
>> > the host controller dying.
>> >
>> > This quirk makes sure PORT_CSC is cleared after the disable slot
>> > command when usb device is disconnected from internal root hub,
>> > otherwise, Synopsis core would fall into a state that cannot use
>> > any endpoint command. Consequently, device disconnection procedure
>> > might not be finished because sometimes endpoint need to be stop
>> > by endpoint stop command issuing.
>> >
>> > Symptom usually happens when disconnected device is keyboard,
>> > mouse, and hub.
>>
>> you need to point us to the synopsys STARS ticket number. That's the
>> only way to reference this specific quirk. Add it to a comment
>> somewhere.
>>
>
> Thanks, we're still waiting for Synopsis STARS ticket number.
> I'll add it to comment later.
>
>
Sorry I didn't express myself clearly.

So far Fujitsu Semiconductor got Synopsys internal case id , that is "
Case: 8000679552".
However the contents belongs this id cannot be referred except Fujitsu
Semiconductor and Synopsys.
Synopsis decide the official errata (STAR information) will be disclosed
next February.

Would you suggest if this quirk can be accepted with this case ID, or can
only be accepted with STARS ticket number?

BR,
Sneeker



> Much Appreciate,
> Sneeker
>
>
>>
>> --
>> balbi
>>
>
>

[-- Attachment #2: Type: text/html, Size: 3096 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 2426 bytes --]

Hi,

On Mon, Dec 29, 2014 at 04:07:50PM +0800, Sneeker Yeh wrote:
> Hi,
> 
> 2014-12-29 14:41 GMT+08:00 Sneeker Yeh <sneeker.yeh-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>:
> 
> > Hi,
> >
> > 2014-12-22 23:37 GMT+08:00 Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>:
> >
> >> On Tue, Dec 16, 2014 at 10:10:28AM +0800, Sneeker Yeh wrote:
> >> > Synopsis DesignWare USB3 IP Core integrated with a config-free
> >> > phy needs special handling during device disconnection to avoid
> >> > the host controller dying.
> >> >
> >> > This quirk makes sure PORT_CSC is cleared after the disable slot
> >> > command when usb device is disconnected from internal root hub,
> >> > otherwise, Synopsis core would fall into a state that cannot use
> >> > any endpoint command. Consequently, device disconnection procedure
> >> > might not be finished because sometimes endpoint need to be stop
> >> > by endpoint stop command issuing.
> >> >
> >> > Symptom usually happens when disconnected device is keyboard,
> >> > mouse, and hub.
> >>
> >> you need to point us to the synopsys STARS ticket number. That's the
> >> only way to reference this specific quirk. Add it to a comment
> >> somewhere.
> >>
> >
> > Thanks, we're still waiting for Synopsis STARS ticket number.
> > I'll add it to comment later.
> >
> >
> Sorry I didn't express myself clearly.
> 
> So far Fujitsu Semiconductor got Synopsys internal case id , that is "
> Case: 8000679552".
> However the contents belongs this id cannot be referred except Fujitsu
> Semiconductor and Synopsys.
> Synopsis decide the official errata (STAR information) will be disclosed
> next February.
> 
> Would you suggest if this quirk can be accepted with this case ID, or can
> only be accepted with STARS ticket number?

The thing is that without the STARS number I can't really verify which
versions of the IP would be affected. Clearly, it's not only your setup
and I think it's pretty unfair to have "is_compatible("fujitsu")" to
enable the quirk only for you. Isn't there a better way of enabling the
quirk based off of revision detection couple with a look on GHWPARAMS*
registers ?

What's tricking me is this claim that only config-free PHYs would be
affected, why ?

I still have many questions about this patch which are not answered by
commit log nor can I poke around on Synopsys solvnet for answers :-s

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Sneeker Yeh]

[-- Attachment #1: Type: text/plain, Size: 3090 bytes --]

Hi Balbi,

thanks for your review,

2014-12-30 0:06 GMT+08:00 Felipe Balbi <balbi@ti.com>:

> Hi,
>
> On Mon, Dec 29, 2014 at 04:07:50PM +0800, Sneeker Yeh wrote:
> > Hi,
> >
> > 2014-12-29 14:41 GMT+08:00 Sneeker Yeh <sneeker.yeh@gmail.com>:
> >
> > > Hi,
> > >
> > > 2014-12-22 23:37 GMT+08:00 Felipe Balbi <balbi@ti.com>:
> > >
> > >> On Tue, Dec 16, 2014 at 10:10:28AM +0800, Sneeker Yeh wrote:
> > >> > Synopsis DesignWare USB3 IP Core integrated with a config-free
> > >> > phy needs special handling during device disconnection to avoid
> > >> > the host controller dying.
> > >> >
> > >> > This quirk makes sure PORT_CSC is cleared after the disable slot
> > >> > command when usb device is disconnected from internal root hub,
> > >> > otherwise, Synopsis core would fall into a state that cannot use
> > >> > any endpoint command. Consequently, device disconnection procedure
> > >> > might not be finished because sometimes endpoint need to be stop
> > >> > by endpoint stop command issuing.
> > >> >
> > >> > Symptom usually happens when disconnected device is keyboard,
> > >> > mouse, and hub.
> > >>
> > >> you need to point us to the synopsys STARS ticket number. That's the
> > >> only way to reference this specific quirk. Add it to a comment
> > >> somewhere.
> > >>
> > >
> > > Thanks, we're still waiting for Synopsis STARS ticket number.
> > > I'll add it to comment later.
> > >
> > >
> > Sorry I didn't express myself clearly.
> >
> > So far Fujitsu Semiconductor got Synopsys internal case id , that is "
> > Case: 8000679552".
> > However the contents belongs this id cannot be referred except Fujitsu
> > Semiconductor and Synopsys.
> > Synopsis decide the official errata (STAR information) will be disclosed
> > next February.
> >
> > Would you suggest if this quirk can be accepted with this case ID, or can
> > only be accepted with STARS ticket number?
>
> The thing is that without the STARS number I can't really verify which
> versions of the IP would be affected. Clearly, it's not only your setup
> and I think it's pretty unfair to have "is_compatible("fujitsu")" to
>

just found some way of quirk injection i miss,
does it looks more reasonable if i have to decide to use it via Kconfig? :
20f6fdd01c2c0de9cc1109083222edded24c5350
b2f463e1300016785d63475c56f5807e2be00934


> enable the quirk only for you. Isn't there a better way of enabling the
> quirk based off of revision detection couple with a look on GHWPARAMS*
> registers ?
>
> What's tricking me is this claim that only config-free PHYs would be
> affected, why ?
>

i'm still struggling now to try to get more information about this.
some security policy inside Fujitsu make me unable to access full
information of this errata today.

Someday after i get enough information,
i shall take your suggestion here that seems better to incur quirk
dynamically via GHWPARAMS,
and then send it here again.

Much appreciate,
Sneeker


> I still have many questions about this patch which are not answered by
> commit log nor can I poke around on Synopsys solvnet for answers :-s
>
> --
> balbi
>

[-- Attachment #2: Type: text/html, Size: 4605 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1853 bytes --]

Hi,

On Sun, Jan 04, 2015 at 08:55:01PM +0800, Sneeker Yeh wrote:
> > > So far Fujitsu Semiconductor got Synopsys internal case id , that is "
> > > Case: 8000679552".
> > > However the contents belongs this id cannot be referred except Fujitsu
> > > Semiconductor and Synopsys.
> > > Synopsis decide the official errata (STAR information) will be disclosed
> > > next February.
> > >
> > > Would you suggest if this quirk can be accepted with this case ID, or can
> > > only be accepted with STARS ticket number?
> >
> > The thing is that without the STARS number I can't really verify which
> > versions of the IP would be affected. Clearly, it's not only your setup
> > and I think it's pretty unfair to have "is_compatible("fujitsu")" to
> >
> 
> just found some way of quirk injection i miss,
> does it looks more reasonable if i have to decide to use it via Kconfig? :
> 20f6fdd01c2c0de9cc1109083222edded24c5350
> b2f463e1300016785d63475c56f5807e2be00934

We will only use Kconfig for those which can't be detected in runtime.
LPM is one such case as there's no way for SW to detect that core was
configured with LPM Errata, IIRC.

> > enable the quirk only for you. Isn't there a better way of enabling the
> > quirk based off of revision detection couple with a look on GHWPARAMS*
> > registers ?
> >
> > What's tricking me is this claim that only config-free PHYs would be
> > affected, why ?
> >
> 
> i'm still struggling now to try to get more information about this.
> some security policy inside Fujitsu make me unable to access full
> information of this errata today.
> 
> Someday after i get enough information,
> i shall take your suggestion here that seems better to incur quirk
> dynamically via GHWPARAMS,
> and then send it here again.

ok, hopefully you'll find a way ;-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Sneeker Yeh]

[-- Attachment #1: Type: text/plain, Size: 3903 bytes --]

Hi,

2015-01-06 1:09 GMT+08:00 Felipe Balbi <balbi@ti.com>:

> Hi,
>
> On Sun, Jan 04, 2015 at 08:55:01PM +0800, Sneeker Yeh wrote:
> > > > So far Fujitsu Semiconductor got Synopsys internal case id , that is
> "
> > > > Case: 8000679552".
> > > > However the contents belongs this id cannot be referred except
> Fujitsu
> > > > Semiconductor and Synopsys.
> > > > Synopsis decide the official errata (STAR information) will be
> disclosed
> > > > next February.
> > > >
> > > > Would you suggest if this quirk can be accepted with this case ID,
> or can
> > > > only be accepted with STARS ticket number?
> > >
> > > The thing is that without the STARS number I can't really verify which
> > > versions of the IP would be affected. Clearly, it's not only your setup
> > > and I think it's pretty unfair to have "is_compatible("fujitsu")" to
> > >
> >
> > just found some way of quirk injection i miss,
> > does it looks more reasonable if i have to decide to use it via Kconfig?
> :
> > 20f6fdd01c2c0de9cc1109083222edded24c5350
> > b2f463e1300016785d63475c56f5807e2be00934
>
> We will only use Kconfig for those which can't be detected in runtime.
> LPM is one such case as there's no way for SW to detect that core was
> configured with LPM Errata, IIRC.
>

Super thanks, I see.


>
> > > enable the quirk only for you. Isn't there a better way of enabling the
> > > quirk based off of revision detection couple with a look on GHWPARAMS*
> > > registers ?
> > >
> > > What's tricking me is this claim that only config-free PHYs would be
> > > affected, why ?
> > >
> >
> > i'm still struggling now to try to get more information about this.
> > some security policy inside Fujitsu make me unable to access full
> > information of this errata today.
> >
> > Someday after i get enough information,
> > i shall take your suggestion here that seems better to incur quirk
> > dynamically via GHWPARAMS,
> > and then send it here again.
>
> ok, hopefully you'll find a way ;-)
>
> I got some update information here finally~
in case i express unclearly i also put a pdf:
https://drive.google.com/file/d/0B18MmcvvKjNNbDF6eEdHSzZCazA/view

This issue is defined by a two-way race at disconnect, between
1) class driver interrupt endpoint resheduling attempts if the ISR gave an
ep error event due to device detach (it would try 3 times)
2) Disconnect interrupt on PORTSC_CSC, which is cleared by hub thread
asynchronously
3) The hardware IP was configured in silicon with
       - DWC_USB3_SUSPEND_ON_DISCONNECT_EN=1 (this is an IP configuration
port whose state cannot be checked from software)
       - Synopsys IP version is < 3.00a
       The IP will auto-suspend itself on device detach with some
phy-specific interval after CSC is cleared by 2)

If 2) and 3) complete before 1), the interrupts it expects will not be
generated by the autosuspended IP, leading to a deadlock. Even later
disconnection procedure would detect that corresponding urb is still
in-progress and issue a ep stop command, auto-suspended IP still won't
respond to that command.

this defect would result in this when device detached:
-------------------
[   99.603544] usb 4-1: USB disconnect, device number 2
[  104.615254] xhci-hcd xhci-hcd.0.auto: xHCI host not responding to stop
endpoint command.
[  104.623362] xhci-hcd xhci-hcd.0.auto: Assuming host is dying, halting
host.
[  104.653261] xhci-hcd xhci-hcd.0.auto: Host not halted after 16000
microseconds.
[  104.660584] xhci-hcd xhci-hcd.0.auto: Non-responsive xHCI host is not
halting.
[  104.667817] xhci-hcd xhci-hcd.0.auto: Completing active URBs anyway.
[  104.674198] xhci-hcd xhci-hcd.0.auto: HC died; cleaning up
--------------------
As a result, when device detached, we desired to postpone "PORTCSC clear"
behind "disable slot". it's found that all executed ep command related to
disconnetion, are executed before "disable slot".

BR,
Sneeker


> --
> balbi
>

[-- Attachment #2: Type: text/html, Size: 5646 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 3537 bytes --]

Hi,

On Sun, Jan 11, 2015 at 11:19:55PM +0800, Sneeker Yeh wrote:
> > > > enable the quirk only for you. Isn't there a better way of enabling the
> > > > quirk based off of revision detection couple with a look on GHWPARAMS*
> > > > registers ?
> > > >
> > > > What's tricking me is this claim that only config-free PHYs would be
> > > > affected, why ?
> > > >
> > >
> > > i'm still struggling now to try to get more information about this.
> > > some security policy inside Fujitsu make me unable to access full
> > > information of this errata today.
> > >
> > > Someday after i get enough information,
> > > i shall take your suggestion here that seems better to incur quirk
> > > dynamically via GHWPARAMS,
> > > and then send it here again.
> >
> > ok, hopefully you'll find a way ;-)
> >
> > I got some update information here finally~
> in case i express unclearly i also put a pdf:
> https://drive.google.com/file/d/0B18MmcvvKjNNbDF6eEdHSzZCazA/view
> 
> This issue is defined by a two-way race at disconnect, between
> 1) class driver interrupt endpoint resheduling attempts if the ISR gave an
> ep error event due to device detach (it would try 3 times)
> 2) Disconnect interrupt on PORTSC_CSC, which is cleared by hub thread
> asynchronously
> 3) The hardware IP was configured in silicon with
>        - DWC_USB3_SUSPEND_ON_DISCONNECT_EN=1 (this is an IP configuration

yeah, aparently this is another configuration which is not exposed on
HWPARAMS registers. Paul, can you confirm for us ? I couldn't find it on
Databook on any of the HWPARAMS registers.

> port whose state cannot be checked from software)
>        - Synopsys IP version is < 3.00a

heh, so pretty much everybody :-)

>        The IP will auto-suspend itself on device detach with some
> phy-specific interval after CSC is cleared by 2)
> 
> If 2) and 3) complete before 1), the interrupts it expects will not be
> generated by the autosuspended IP, leading to a deadlock. Even later
> disconnection procedure would detect that corresponding urb is still
> in-progress and issue a ep stop command, auto-suspended IP still won't
> respond to that command.
> 
> this defect would result in this when device detached:
> -------------------
> [   99.603544] usb 4-1: USB disconnect, device number 2
> [  104.615254] xhci-hcd xhci-hcd.0.auto: xHCI host not responding to stop
> endpoint command.
> [  104.623362] xhci-hcd xhci-hcd.0.auto: Assuming host is dying, halting
> host.
> [  104.653261] xhci-hcd xhci-hcd.0.auto: Host not halted after 16000
> microseconds.
> [  104.660584] xhci-hcd xhci-hcd.0.auto: Non-responsive xHCI host is not
> halting.
> [  104.667817] xhci-hcd xhci-hcd.0.auto: Completing active URBs anyway.
> [  104.674198] xhci-hcd xhci-hcd.0.auto: HC died; cleaning up
> --------------------
> As a result, when device detached, we desired to postpone "PORTCSC clear"
> behind "disable slot". it's found that all executed ep command related to
> disconnetion, are executed before "disable slot".

Now this is all great information and they should all be part of your
commit log and probably a big comment should be added to code as well.

Thanks for going after all these details, now let's figure out a way to
pass dwc3 revision to xhci, or maybe we pass just a flag for the quirk,
something like:

	if (dwc->revision < 3.00a && dwc->has_suspend_on_disconnect)
		xhci_pdata.delay_portcsc_clear = true;

or something similar to that.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Paul Bolle]

On Mon, 2015-01-12 at 11:20 -0600, Felipe Balbi wrote:
> On Sun, Jan 11, 2015 at 11:19:55PM +0800, Sneeker Yeh wrote:
> > in case i express unclearly i also put a pdf:
> > https://drive.google.com/file/d/0B18MmcvvKjNNbDF6eEdHSzZCazA/view
> > 
> > This issue is defined by a two-way race at disconnect, between
> > 1) class driver interrupt endpoint resheduling attempts if the ISR gave an
> > ep error event due to device detach (it would try 3 times)
> > 2) Disconnect interrupt on PORTSC_CSC, which is cleared by hub thread
> > asynchronously
> > 3) The hardware IP was configured in silicon with
> >        - DWC_USB3_SUSPEND_ON_DISCONNECT_EN=1 (this is an IP configuration
> 
> yeah, aparently this is another configuration which is not exposed on
> HWPARAMS registers. Paul, can you confirm for us ? I couldn't find it on
> Databook on any of the HWPARAMS registers.

I guess you meant to ask Pawel, because I know nothing at all about all
that! 

Thanks,


Paul Bolle

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1117 bytes --]

On Mon, Jan 12, 2015 at 06:29:43PM +0100, Paul Bolle wrote:
> On Mon, 2015-01-12 at 11:20 -0600, Felipe Balbi wrote:
> > On Sun, Jan 11, 2015 at 11:19:55PM +0800, Sneeker Yeh wrote:
> > > in case i express unclearly i also put a pdf:
> > > https://drive.google.com/file/d/0B18MmcvvKjNNbDF6eEdHSzZCazA/view
> > > 
> > > This issue is defined by a two-way race at disconnect, between
> > > 1) class driver interrupt endpoint resheduling attempts if the ISR gave an
> > > ep error event due to device detach (it would try 3 times)
> > > 2) Disconnect interrupt on PORTSC_CSC, which is cleared by hub thread
> > > asynchronously
> > > 3) The hardware IP was configured in silicon with
> > >        - DWC_USB3_SUSPEND_ON_DISCONNECT_EN=1 (this is an IP configuration
> > 
> > yeah, aparently this is another configuration which is not exposed on
> > HWPARAMS registers. Paul, can you confirm for us ? I couldn't find it on
> > Databook on any of the HWPARAMS registers.
> 
> I guess you meant to ask Pawel, because I know nothing at all about all
> that! 

No, it was Paul Zimmerman :-)

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Sneeker Yeh]

[-- Attachment #1: Type: text/plain, Size: 6365 bytes --]

Hi Felipe:

thanks for suggestion,

2015-01-13 1:20 GMT+08:00 Felipe Balbi <balbi@ti.com>:

> Hi,
>
> On Sun, Jan 11, 2015 at 11:19:55PM +0800, Sneeker Yeh wrote:
> > > > > enable the quirk only for you. Isn't there a better way of
> enabling the
> > > > > quirk based off of revision detection couple with a look on
> GHWPARAMS*
> > > > > registers ?
> > > > >
> > > > > What's tricking me is this claim that only config-free PHYs would
> be
> > > > > affected, why ?
> > > > >
> > > >
> > > > i'm still struggling now to try to get more information about this.
> > > > some security policy inside Fujitsu make me unable to access full
> > > > information of this errata today.
> > > >
> > > > Someday after i get enough information,
> > > > i shall take your suggestion here that seems better to incur quirk
> > > > dynamically via GHWPARAMS,
> > > > and then send it here again.
> > >
> > > ok, hopefully you'll find a way ;-)
> > >
> > > I got some update information here finally~
> > in case i express unclearly i also put a pdf:
> > https://drive.google.com/file/d/0B18MmcvvKjNNbDF6eEdHSzZCazA/view
> >
> > This issue is defined by a two-way race at disconnect, between
> > 1) class driver interrupt endpoint resheduling attempts if the ISR gave
> an
> > ep error event due to device detach (it would try 3 times)
> > 2) Disconnect interrupt on PORTSC_CSC, which is cleared by hub thread
> > asynchronously
> > 3) The hardware IP was configured in silicon with
> >        - DWC_USB3_SUSPEND_ON_DISCONNECT_EN=1 (this is an IP configuration
>
> yeah, aparently this is another configuration which is not exposed on
> HWPARAMS registers. Paul, can you confirm for us ? I couldn't find it on
> Databook on any of the HWPARAMS registers.
>
> > port whose state cannot be checked from software)
> >        - Synopsys IP version is < 3.00a
>
> heh, so pretty much everybody :-)
>

yeah, and I'm really lucky to encounter this @@


>
> >        The IP will auto-suspend itself on device detach with some
> > phy-specific interval after CSC is cleared by 2)
> >
> > If 2) and 3) complete before 1), the interrupts it expects will not be
> > generated by the autosuspended IP, leading to a deadlock. Even later
> > disconnection procedure would detect that corresponding urb is still
> > in-progress and issue a ep stop command, auto-suspended IP still won't
> > respond to that command.
> >
> > this defect would result in this when device detached:
> > -------------------
> > [   99.603544] usb 4-1: USB disconnect, device number 2
> > [  104.615254] xhci-hcd xhci-hcd.0.auto: xHCI host not responding to stop
> > endpoint command.
> > [  104.623362] xhci-hcd xhci-hcd.0.auto: Assuming host is dying, halting
> > host.
> > [  104.653261] xhci-hcd xhci-hcd.0.auto: Host not halted after 16000
> > microseconds.
> > [  104.660584] xhci-hcd xhci-hcd.0.auto: Non-responsive xHCI host is not
> > halting.
> > [  104.667817] xhci-hcd xhci-hcd.0.auto: Completing active URBs anyway.
> > [  104.674198] xhci-hcd xhci-hcd.0.auto: HC died; cleaning up
> > --------------------
> > As a result, when device detached, we desired to postpone "PORTCSC clear"
> > behind "disable slot". it's found that all executed ep command related to
> > disconnetion, are executed before "disable slot".
>
> Now this is all great information and they should all be part of your
> commit log and probably a big comment should be added to code as well.
>

I see, thanks.


>
> Thanks for going after all these details, now let's figure out a way to
> pass dwc3 revision to xhci, or maybe we pass just a flag for the quirk,
> something like:
>
>         if (dwc->revision < 3.00a && dwc->has_suspend_on_disconnect)
>                 xhci_pdata.delay_portcsc_clear = true;
>
> or something similar to that.
>

Sure, how about i write these like this, that i learn from the way dwc3
inject XHCI_LPM_SUPPORT to xhci via platform data:

--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -53,6 +53,10 @@ int dwc3_host_init(struct dwc3 *dwc)
        pdata.usb3_lpm_capable = 1;
 #endif

+       if ((dwc->revision < DWC3_REVISION_300A) &&
+           dwc->suspend_on_disconnect_quirk)
+               pdata.delay_portcsc_clear = 1;
+
        ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
        if (ret) {
                dev_err(dwc->dev, "couldn't add platform data to xHCI
device\n");
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -147,6 +147,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
        if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
                        (pdata && pdata->usb3_lpm_capable))
                xhci->quirks |= XHCI_LPM_SUPPORT;
+
+       if (pdata && pdata->delay_portcsc_clear)
+               xhci->quirks |= XHCI_DISCONNECT_QUIRK;
        /*
         * Set the xHCI pointer before xhci_plat_setup() (aka
hcd_driver.reset)
         * is called by usb_add_hcd().

but I'm a little confused about how we decide
dwc->suspend_on_disconnect_quirk.
just wondering if pdata & dts would be better than Kconfig to decide this?
because using Kconfig means forcing dwc3.ko has this quirk, but platform
might have another dwc3 core doesn't need that quirk.
So I write the following code, is it in a good shape? :

--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -838,6 +838,9 @@ static int dwc3_probe(struct platform_device *pdev)
                                "snps,tx_de_emphasis_quirk");
                of_property_read_u8(node, "snps,tx_de_emphasis",
                                &tx_de_emphasis);
+
+               dwc->suspend_on_disconnect_quirk =
of_property_read_bool(node,
+                               "snps,has_suspend_on_disconnect");
        } else if (pdata) {
                dwc->maximum_speed = pdata->maximum_speed;
                dwc->has_lpm_erratum = pdata->has_lpm_erratum;
@@ -864,6 +867,8 @@ static int dwc3_probe(struct platform_device *pdev)
                dwc->tx_de_emphasis_quirk = pdata->tx_de_emphasis_quirk;
                if (pdata->tx_de_emphasis)
                        tx_de_emphasis = pdata->tx_de_emphasis;
+
+               dwc->suspend_on_disconnect_quirk =
pdata->has_suspend_on_disconnect;
        }

        /* default to superspeed if no maximum_speed passed */


Much appreciate,
Sneeker



>
> cheers
>
> --
> balbi
>

[-- Attachment #2: Type: text/html, Size: 8955 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 7375 bytes --]

Hi,

On Wed, Jan 14, 2015 at 03:08:43PM +0800, Sneeker Yeh wrote:
> Hi Felipe:
> 
> thanks for suggestion,
> 
> 2015-01-13 1:20 GMT+08:00 Felipe Balbi <balbi@ti.com>:
> 
> > Hi,
> >
> > On Sun, Jan 11, 2015 at 11:19:55PM +0800, Sneeker Yeh wrote:
> > > > > > enable the quirk only for you. Isn't there a better way of
> > enabling the
> > > > > > quirk based off of revision detection couple with a look on
> > GHWPARAMS*
> > > > > > registers ?
> > > > > >
> > > > > > What's tricking me is this claim that only config-free PHYs would
> > be
> > > > > > affected, why ?
> > > > > >
> > > > >
> > > > > i'm still struggling now to try to get more information about this.
> > > > > some security policy inside Fujitsu make me unable to access full
> > > > > information of this errata today.
> > > > >
> > > > > Someday after i get enough information,
> > > > > i shall take your suggestion here that seems better to incur quirk
> > > > > dynamically via GHWPARAMS,
> > > > > and then send it here again.
> > > >
> > > > ok, hopefully you'll find a way ;-)
> > > >
> > > > I got some update information here finally~
> > > in case i express unclearly i also put a pdf:
> > > https://drive.google.com/file/d/0B18MmcvvKjNNbDF6eEdHSzZCazA/view
> > >
> > > This issue is defined by a two-way race at disconnect, between
> > > 1) class driver interrupt endpoint resheduling attempts if the ISR gave
> > an
> > > ep error event due to device detach (it would try 3 times)
> > > 2) Disconnect interrupt on PORTSC_CSC, which is cleared by hub thread
> > > asynchronously
> > > 3) The hardware IP was configured in silicon with
> > >        - DWC_USB3_SUSPEND_ON_DISCONNECT_EN=1 (this is an IP configuration
> >
> > yeah, aparently this is another configuration which is not exposed on
> > HWPARAMS registers. Paul, can you confirm for us ? I couldn't find it on
> > Databook on any of the HWPARAMS registers.
> >
> > > port whose state cannot be checked from software)
> > >        - Synopsys IP version is < 3.00a
> >
> > heh, so pretty much everybody :-)
> >
> 
> yeah, and I'm really lucky to encounter this @@
> 
> 
> >
> > >        The IP will auto-suspend itself on device detach with some
> > > phy-specific interval after CSC is cleared by 2)
> > >
> > > If 2) and 3) complete before 1), the interrupts it expects will not be
> > > generated by the autosuspended IP, leading to a deadlock. Even later
> > > disconnection procedure would detect that corresponding urb is still
> > > in-progress and issue a ep stop command, auto-suspended IP still won't
> > > respond to that command.
> > >
> > > this defect would result in this when device detached:
> > > -------------------
> > > [   99.603544] usb 4-1: USB disconnect, device number 2
> > > [  104.615254] xhci-hcd xhci-hcd.0.auto: xHCI host not responding to stop
> > > endpoint command.
> > > [  104.623362] xhci-hcd xhci-hcd.0.auto: Assuming host is dying, halting
> > > host.
> > > [  104.653261] xhci-hcd xhci-hcd.0.auto: Host not halted after 16000
> > > microseconds.
> > > [  104.660584] xhci-hcd xhci-hcd.0.auto: Non-responsive xHCI host is not
> > > halting.
> > > [  104.667817] xhci-hcd xhci-hcd.0.auto: Completing active URBs anyway.
> > > [  104.674198] xhci-hcd xhci-hcd.0.auto: HC died; cleaning up
> > > --------------------
> > > As a result, when device detached, we desired to postpone "PORTCSC clear"
> > > behind "disable slot". it's found that all executed ep command related to
> > > disconnetion, are executed before "disable slot".
> >
> > Now this is all great information and they should all be part of your
> > commit log and probably a big comment should be added to code as well.
> >
> 
> I see, thanks.
> 
> 
> >
> > Thanks for going after all these details, now let's figure out a way to
> > pass dwc3 revision to xhci, or maybe we pass just a flag for the quirk,
> > something like:
> >
> >         if (dwc->revision < 3.00a && dwc->has_suspend_on_disconnect)
> >                 xhci_pdata.delay_portcsc_clear = true;
> >
> > or something similar to that.
> >
> 
> Sure, how about i write these like this, that i learn from the way dwc3
> inject XHCI_LPM_SUPPORT to xhci via platform data:
> 
> --- a/drivers/usb/dwc3/host.c
> +++ b/drivers/usb/dwc3/host.c
> @@ -53,6 +53,10 @@ int dwc3_host_init(struct dwc3 *dwc)
>         pdata.usb3_lpm_capable = 1;
>  #endif
> 
> +       if ((dwc->revision < DWC3_REVISION_300A) &&
> +           dwc->suspend_on_disconnect_quirk)
> +               pdata.delay_portcsc_clear = 1;
> +
>         ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
>         if (ret) {
>                 dev_err(dwc->dev, "couldn't add platform data to xHCI
> device\n");
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -147,6 +147,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>         if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
>                         (pdata && pdata->usb3_lpm_capable))
>                 xhci->quirks |= XHCI_LPM_SUPPORT;
> +
> +       if (pdata && pdata->delay_portcsc_clear)
> +               xhci->quirks |= XHCI_DISCONNECT_QUIRK;
>         /*
>          * Set the xHCI pointer before xhci_plat_setup() (aka
> hcd_driver.reset)
>          * is called by usb_add_hcd().
> 
> but I'm a little confused about how we decide
> dwc->suspend_on_disconnect_quirk.
> just wondering if pdata & dts would be better than Kconfig to decide this?
> because using Kconfig means forcing dwc3.ko has this quirk, but platform
> might have another dwc3 core doesn't need that quirk.
> So I write the following code, is it in a good shape? :
> 
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -838,6 +838,9 @@ static int dwc3_probe(struct platform_device *pdev)
>                                 "snps,tx_de_emphasis_quirk");
>                 of_property_read_u8(node, "snps,tx_de_emphasis",
>                                 &tx_de_emphasis);
> +
> +               dwc->suspend_on_disconnect_quirk =
> of_property_read_bool(node,
> +                               "snps,has_suspend_on_disconnect");
>         } else if (pdata) {
>                 dwc->maximum_speed = pdata->maximum_speed;
>                 dwc->has_lpm_erratum = pdata->has_lpm_erratum;
> @@ -864,6 +867,8 @@ static int dwc3_probe(struct platform_device *pdev)
>                 dwc->tx_de_emphasis_quirk = pdata->tx_de_emphasis_quirk;
>                 if (pdata->tx_de_emphasis)
>                         tx_de_emphasis = pdata->tx_de_emphasis;
> +
> +               dwc->suspend_on_disconnect_quirk =
> pdata->has_suspend_on_disconnect;
>         }
> 
>         /* default to superspeed if no maximum_speed passed */

this all looks good, but let's split it in tiny little patches:

1) add macros for new revisions
2) add suspend_on_disconnect_quirk (and the DT counterpart, please don't
	forget to document the new binding)
3) pass the quirk to XHCI through pdata
4) Use the quirk on XHCI

That'll make patches real easy to review. As per the commit log, the
more verbose the better, specially when it comes to the quirk. It needs
to be 100% clear that we cannot detect this in runtime.

cheers

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 3/3] usb: dwc3: add a quirk for device disconnection issue in Synopsis dwc3 core

[Sneeker Yeh]

[-- Attachment #1: Type: text/plain, Size: 7786 bytes --]

Hi,

2015-01-15 1:02 GMT+08:00 Felipe Balbi <balbi@ti.com>:

> Hi,
>
> On Wed, Jan 14, 2015 at 03:08:43PM +0800, Sneeker Yeh wrote:
> > Hi Felipe:
> >
> > thanks for suggestion,
> >
> > 2015-01-13 1:20 GMT+08:00 Felipe Balbi <balbi@ti.com>:
> >
> > > Hi,
> > >
> > > On Sun, Jan 11, 2015 at 11:19:55PM +0800, Sneeker Yeh wrote:
> > > > > > > enable the quirk only for you. Isn't there a better way of
> > > enabling the
> > > > > > > quirk based off of revision detection couple with a look on
> > > GHWPARAMS*
> > > > > > > registers ?
> > > > > > >
> > > > > > > What's tricking me is this claim that only config-free PHYs
> would
> > > be
> > > > > > > affected, why ?
> > > > > > >
> > > > > >
> > > > > > i'm still struggling now to try to get more information about
> this.
> > > > > > some security policy inside Fujitsu make me unable to access full
> > > > > > information of this errata today.
> > > > > >
> > > > > > Someday after i get enough information,
> > > > > > i shall take your suggestion here that seems better to incur
> quirk
> > > > > > dynamically via GHWPARAMS,
> > > > > > and then send it here again.
> > > > >
> > > > > ok, hopefully you'll find a way ;-)
> > > > >
> > > > > I got some update information here finally~
> > > > in case i express unclearly i also put a pdf:
> > > > https://drive.google.com/file/d/0B18MmcvvKjNNbDF6eEdHSzZCazA/view
> > > >
> > > > This issue is defined by a two-way race at disconnect, between
> > > > 1) class driver interrupt endpoint resheduling attempts if the ISR
> gave
> > > an
> > > > ep error event due to device detach (it would try 3 times)
> > > > 2) Disconnect interrupt on PORTSC_CSC, which is cleared by hub thread
> > > > asynchronously
> > > > 3) The hardware IP was configured in silicon with
> > > >        - DWC_USB3_SUSPEND_ON_DISCONNECT_EN=1 (this is an IP
> configuration
> > >
> > > yeah, aparently this is another configuration which is not exposed on
> > > HWPARAMS registers. Paul, can you confirm for us ? I couldn't find it
> on
> > > Databook on any of the HWPARAMS registers.
> > >
> > > > port whose state cannot be checked from software)
> > > >        - Synopsys IP version is < 3.00a
> > >
> > > heh, so pretty much everybody :-)
> > >
> >
> > yeah, and I'm really lucky to encounter this @@
> >
> >
> > >
> > > >        The IP will auto-suspend itself on device detach with some
> > > > phy-specific interval after CSC is cleared by 2)
> > > >
> > > > If 2) and 3) complete before 1), the interrupts it expects will not
> be
> > > > generated by the autosuspended IP, leading to a deadlock. Even later
> > > > disconnection procedure would detect that corresponding urb is still
> > > > in-progress and issue a ep stop command, auto-suspended IP still
> won't
> > > > respond to that command.
> > > >
> > > > this defect would result in this when device detached:
> > > > -------------------
> > > > [   99.603544] usb 4-1: USB disconnect, device number 2
> > > > [  104.615254] xhci-hcd xhci-hcd.0.auto: xHCI host not responding to
> stop
> > > > endpoint command.
> > > > [  104.623362] xhci-hcd xhci-hcd.0.auto: Assuming host is dying,
> halting
> > > > host.
> > > > [  104.653261] xhci-hcd xhci-hcd.0.auto: Host not halted after 16000
> > > > microseconds.
> > > > [  104.660584] xhci-hcd xhci-hcd.0.auto: Non-responsive xHCI host is
> not
> > > > halting.
> > > > [  104.667817] xhci-hcd xhci-hcd.0.auto: Completing active URBs
> anyway.
> > > > [  104.674198] xhci-hcd xhci-hcd.0.auto: HC died; cleaning up
> > > > --------------------
> > > > As a result, when device detached, we desired to postpone "PORTCSC
> clear"
> > > > behind "disable slot". it's found that all executed ep command
> related to
> > > > disconnetion, are executed before "disable slot".
> > >
> > > Now this is all great information and they should all be part of your
> > > commit log and probably a big comment should be added to code as well.
> > >
> >
> > I see, thanks.
> >
> >
> > >
> > > Thanks for going after all these details, now let's figure out a way to
> > > pass dwc3 revision to xhci, or maybe we pass just a flag for the quirk,
> > > something like:
> > >
> > >         if (dwc->revision < 3.00a && dwc->has_suspend_on_disconnect)
> > >                 xhci_pdata.delay_portcsc_clear = true;
> > >
> > > or something similar to that.
> > >
> >
> > Sure, how about i write these like this, that i learn from the way dwc3
> > inject XHCI_LPM_SUPPORT to xhci via platform data:
> >
> > --- a/drivers/usb/dwc3/host.c
> > +++ b/drivers/usb/dwc3/host.c
> > @@ -53,6 +53,10 @@ int dwc3_host_init(struct dwc3 *dwc)
> >         pdata.usb3_lpm_capable = 1;
> >  #endif
> >
> > +       if ((dwc->revision < DWC3_REVISION_300A) &&
> > +           dwc->suspend_on_disconnect_quirk)
> > +               pdata.delay_portcsc_clear = 1;
> > +
> >         ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
> >         if (ret) {
> >                 dev_err(dwc->dev, "couldn't add platform data to xHCI
> > device\n");
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -147,6 +147,9 @@ static int xhci_plat_probe(struct platform_device
> *pdev)
> >         if ((node && of_property_read_bool(node, "usb3-lpm-capable")) ||
> >                         (pdata && pdata->usb3_lpm_capable))
> >                 xhci->quirks |= XHCI_LPM_SUPPORT;
> > +
> > +       if (pdata && pdata->delay_portcsc_clear)
> > +               xhci->quirks |= XHCI_DISCONNECT_QUIRK;
> >         /*
> >          * Set the xHCI pointer before xhci_plat_setup() (aka
> > hcd_driver.reset)
> >          * is called by usb_add_hcd().
> >
> > but I'm a little confused about how we decide
> > dwc->suspend_on_disconnect_quirk.
> > just wondering if pdata & dts would be better than Kconfig to decide
> this?
> > because using Kconfig means forcing dwc3.ko has this quirk, but platform
> > might have another dwc3 core doesn't need that quirk.
> > So I write the following code, is it in a good shape? :
> >
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -838,6 +838,9 @@ static int dwc3_probe(struct platform_device *pdev)
> >                                 "snps,tx_de_emphasis_quirk");
> >                 of_property_read_u8(node, "snps,tx_de_emphasis",
> >                                 &tx_de_emphasis);
> > +
> > +               dwc->suspend_on_disconnect_quirk =
> > of_property_read_bool(node,
> > +                               "snps,has_suspend_on_disconnect");
> >         } else if (pdata) {
> >                 dwc->maximum_speed = pdata->maximum_speed;
> >                 dwc->has_lpm_erratum = pdata->has_lpm_erratum;
> > @@ -864,6 +867,8 @@ static int dwc3_probe(struct platform_device *pdev)
> >                 dwc->tx_de_emphasis_quirk = pdata->tx_de_emphasis_quirk;
> >                 if (pdata->tx_de_emphasis)
> >                         tx_de_emphasis = pdata->tx_de_emphasis;
> > +
> > +               dwc->suspend_on_disconnect_quirk =
> > pdata->has_suspend_on_disconnect;
> >         }
> >
> >         /* default to superspeed if no maximum_speed passed */
>
> this all looks good, but let's split it in tiny little patches:
>
> 1) add macros for new revisions
> 2) add suspend_on_disconnect_quirk (and the DT counterpart, please don't
>         forget to document the new binding)
> 3) pass the quirk to XHCI through pdata
> 4) Use the quirk on XHCI
>
> That'll make patches real easy to review. As per the commit log, the
> more verbose the better, specially when it comes to the quirk. It needs
> to be 100% clear that we cannot detect this in runtime.
>

Super thanks,
I'm separating it and testing it, and would send v2 patch series later
based on all suggestions here.

Much appreciate,
Sneeker


>
> cheers
>
> --
> balbi
>

[-- Attachment #2: Type: text/html, Size: 10647 bytes --]