[RESEND PATCH V5, 0/5] Add MediaTek USB3 DRD Driver

[RESEND PATCH V5, 0/5] Add MediaTek USB3 DRD Driver

[Chunfeng Yun]

>From e60d29d748a4e9f412c9bb08458083e97d3f523d Mon Sep 17 00:00:00 2001
From: Chunfeng Yun <chunfeng.yun@mediatek.com>
Date: Tue, 9 Aug 2016 16:12:31 +0800
Subject: [PATCH V5, 0/5] Add MediaTek USB3 DRD Driver

These patches introduce the MediaTek USB3 dual-role controller
driver.

The driver can be configured as Dual-Role Device (DRD),
Peripheral Only and Host Only (xHCI) modes. It works well
with Mass Storage, RNDIS and g_zero on FS/HS and SS. And it is
tested on MT8173 platform which only contains USB2.0 device IP,
and on MT6290 platform which contains USB3.0 device IP.

Change in v5:
1. modify some comments
2. rename some unsuitable variables
3. add reg-names property for host node
4. add USB_MTU3_DEBUG to control debug messages

Change in v4:
1. fix build errors on non-mediatek platforms
2. provide manual dual-role switch via debugfs instead of sysfs

Change in v3:
1. fix some typo error
2. rename mtu3.txt to mt8173-mtu3.txt

Change in v2:
1. modify binding docs according to suggestions
2. modify some comments and remove some dummy blank lines
3. fix memory leakage

Change in v2:
1. modify binding docs according to suggestions
2. modify some comments and remove some dummy blank lines
3. fix memory leakage


Chunfeng Yun (5):
  dt-bindings: mt8173-xhci: support host side of dual-role mode
  dt-bindings: mt8173-mtu3: add devicetree bindings
  usb: xhci-mtk: make IPPC register optional
  usb: Add MediaTek USB3 DRD Driver
  arm64: dts: mediatek: add USB3 DRD driver

 .../devicetree/bindings/usb/mt8173-mtu3.txt        |   87 ++
 .../devicetree/bindings/usb/mt8173-xhci.txt        |   54 +-
 arch/arm64/boot/dts/mediatek/mt8173-evb.dts        |   46 +-
 arch/arm64/boot/dts/mediatek/mt8173.dtsi           |   29 +-
 drivers/usb/Kconfig                                |    2 +
 drivers/usb/Makefile                               |    1 +
 drivers/usb/host/xhci-mtk.c                        |   36 +-
 drivers/usb/mtu3/Kconfig                           |   54 ++
 drivers/usb/mtu3/Makefile                          |   19 +
 drivers/usb/mtu3/mtu3.h                            |  422 ++++++++++
 drivers/usb/mtu3/mtu3_core.c                       |  874 +++++++++++++++++++
 drivers/usb/mtu3/mtu3_dr.c                         |  375 +++++++++
 drivers/usb/mtu3/mtu3_dr.h                         |  108 +++
 drivers/usb/mtu3/mtu3_gadget.c                     |  731 ++++++++++++++++
 drivers/usb/mtu3/mtu3_gadget_ep0.c                 |  879 ++++++++++++++++++++
 drivers/usb/mtu3/mtu3_host.c                       |  294 +++++++
 drivers/usb/mtu3/mtu3_hw_regs.h                    |  473 +++++++++++
 drivers/usb/mtu3/mtu3_plat.c                       |  490 +++++++++++
 drivers/usb/mtu3/mtu3_qmu.c                        |  599 +++++++++++++
 drivers/usb/mtu3/mtu3_qmu.h                        |   43 +
 20 files changed, 5598 insertions(+), 18 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/mt8173-mtu3.txt
 create mode 100644 drivers/usb/mtu3/Kconfig
 create mode 100644 drivers/usb/mtu3/Makefile
 create mode 100644 drivers/usb/mtu3/mtu3.h
 create mode 100644 drivers/usb/mtu3/mtu3_core.c
 create mode 100644 drivers/usb/mtu3/mtu3_dr.c
 create mode 100644 drivers/usb/mtu3/mtu3_dr.h
 create mode 100644 drivers/usb/mtu3/mtu3_gadget.c
 create mode 100644 drivers/usb/mtu3/mtu3_gadget_ep0.c
 create mode 100644 drivers/usb/mtu3/mtu3_host.c
 create mode 100644 drivers/usb/mtu3/mtu3_hw_regs.h
 create mode 100644 drivers/usb/mtu3/mtu3_plat.c
 create mode 100644 drivers/usb/mtu3/mtu3_qmu.c
 create mode 100644 drivers/usb/mtu3/mtu3_qmu.h

[RESEND PATCH, v5 1/5] dt-bindings: mt8173-xhci: support host side of dual-role mode

[Chunfeng Yun]

Some resources, such as IPPC register etc, shared with device
driver are moved into common glue layer when xHCI driver is the
host side of dual-role mode and they should be changed as optional
properties if they are required ones before. For clarity, add
a new part of binding to support host side of dual-role mode.

Additionally add optional properties of pinctrl for host only mode

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/usb/mt8173-xhci.txt        |   54 +++++++++++++++++++-
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/mt8173-xhci.txt b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt
index b3a7ffa..2a930bd 100644
--- a/Documentation/devicetree/bindings/usb/mt8173-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt
@@ -2,10 +2,18 @@ MT8173 xHCI
 
 The device node for Mediatek SOC USB3.0 host controller
 
+There are two scenarios: the first one only supports xHCI driver;
+the second one supports dual-role mode, and the host is based on xHCI
+driver. Take account of backward compatibility, we divide bindings
+into two parts.
+
+1st: only supports xHCI driver
+------------------------------------------------------------------------
+
 Required properties:
  - compatible : should contain "mediatek,mt8173-xhci"
- - reg : specifies physical base address and size of the registers,
-	the first one for MAC, the second for IPPC
+ - reg : specifies physical base address and size of the registers
+ - reg-names: should be "mac" for xHCI MAC and "ippc" for IP port control
  - interrupts : interrupt used by the controller
  - power-domains : a phandle to USB power domain node to control USB's
 	mtcmos
@@ -27,12 +35,16 @@ Optional properties:
 	control register, it depends on "mediatek,wakeup-src".
  - vbus-supply : reference to the VBUS regulator;
  - usb3-lpm-capable : supports USB3.0 LPM
+ - pinctrl-names : a pinctrl state named "default" must be defined
+ - pinctrl-0 : pin control group
+	See: Documentation/devicetree/bindings/pinctrl/pinctrl-binding.txt
 
 Example:
 usb30: usb at 11270000 {
 	compatible = "mediatek,mt8173-xhci";
 	reg = <0 0x11270000 0 0x1000>,
 	      <0 0x11280700 0 0x0100>;
+	reg-names = "mac", "ippc";
 	interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_LOW>;
 	power-domains = <&scpsys MT8173_POWER_DOMAIN_USB>;
 	clocks = <&topckgen CLK_TOP_USB30_SEL>,
@@ -49,3 +61,41 @@ usb30: usb at 11270000 {
 	mediatek,syscon-wakeup = <&pericfg>;
 	mediatek,wakeup-src = <1>;
 };
+
+2nd: dual-role mode with xHCI driver
+------------------------------------------------------------------------
+
+In the case, xhci is added as subnode to mtu3. An example and the DT binding
+details of mtu3 can be found in:
+Documentation/devicetree/bindings/usb/mtu3.txt
+
+Required properties:
+ - compatible : should contain "mediatek,mt8173-xhci"
+ - reg : specifies physical base address and size of the registers
+ - reg-names: should be "mac" for xHCI MAC
+ - interrupts : interrupt used by the host controller
+ - power-domains : a phandle to USB power domain node to control USB's
+	mtcmos
+ - vusb33-supply : regulator of USB avdd3.3v
+
+ - clocks : a list of phandle + clock-specifier pairs, one for each
+	entry in clock-names
+ - clock-names : must be
+	"sys_ck": for clock of xHCI MAC
+
+Optional properties:
+ - vbus-supply : reference to the VBUS regulator;
+ - usb3-lpm-capable : supports USB3.0 LPM
+
+Example:
+usb30: usb at 11270000 {
+	compatible = "mediatek,mt8173-xhci";
+	reg = <0 0x11270000 0 0x1000>;
+	reg-names = "mac";
+	interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_LOW>;
+	power-domains = <&scpsys MT8173_POWER_DOMAIN_USB>;
+	clocks = <&topckgen CLK_TOP_USB30_SEL>;
+	clock-names = "sys_ck";
+	vusb33-supply = <&mt6397_vusb_reg>;
+	usb3-lpm-capable;
+};
-- 
1.7.9.5

[RESEND PATCH, v5 2/5] dt-bindings: mt8173-mtu3: add devicetree bindings

[Chunfeng Yun]

add a DT binding doc for MediaTek USB3 DRD driver

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/usb/mt8173-mtu3.txt        |   87 ++++++++++++++++++++
 1 file changed, 87 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/mt8173-mtu3.txt

diff --git a/Documentation/devicetree/bindings/usb/mt8173-mtu3.txt b/Documentation/devicetree/bindings/usb/mt8173-mtu3.txt
new file mode 100644
index 0000000..e049d19
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/mt8173-mtu3.txt
@@ -0,0 +1,87 @@
+The device node for Mediatek USB3.0 DRD controller
+
+Required properties:
+ - compatible : should be "mediatek,mt8173-mtu3"
+ - reg : specifies physical base address and size of the registers
+ - reg-names: should be "mac" for device IP and "ippc" for IP port control
+ - interrupts : interrupt used by the device IP
+ - power-domains : a phandle to USB power domain node to control USB's
+	mtcmos
+ - vusb33-supply : regulator of USB avdd3.3v
+ - clocks : a list of phandle + clock-specifier pairs, one for each
+	entry in clock-names
+ - clock-names : must contain "sys_ck" for clock of controller;
+	"wakeup_deb_p0" and "wakeup_deb_p1" are optional, they are
+	depends on "mediatek,enable-wakeup"
+ - phys : a list of phandle + phy specifier pairs
+ - dr_mode : should be one of "host", "peripheral" or "otg",
+	refer to usb/generic.txt
+
+Optional properties:
+ - #address-cells, #size-cells : should be '2' if the device has sub-nodes
+	with 'reg' property
+ - ranges : allows valid 1:1 translation between child's address space and
+	parent's address space
+ - extcon : external connector for vbus and idpin changes detection, needed
+	when supports dual-role mode.
+ - vbus-supply : reference to the VBUS regulator, needed when supports
+	dual-role mode.
+ - pinctl-names : a pinctrl state named "default" must be defined,
+	"id_float" and "id_ground" are optinal which depends on
+	"mediatek,enable-manual-drd"
+ - pinctrl-0 : pin control group
+	See: Documentation/devicetree/bindings/pinctrl/pinctrl-binding.txt
+
+ - maximum-speed : valid arguments are "super-speed", "high-speed" and
+	"full-speed"; refer to usb/generic.txt
+ - enable-manual-drd : supports manual dual-role switch via debugfs; usually
+	used when receptacle is TYPE-A and also wants to support dual-role
+	mode.
+ - mediatek,enable-wakeup : supports ip sleep wakeup used by host mode
+ - mediatek,syscon-wakeup : phandle to syscon used to access USB wakeup
+	control register, it depends on "mediatek,enable-wakeup".
+
+Sub-nodes:
+The xhci should be added as subnode to mtu3 as shown in the following example
+if host mode is enabled. The DT binding details of xhci can be found in:
+Documentation/devicetree/bindings/usb/mt8173-xhci.txt
+
+Example:
+ssusb: usb at 11271000 {
+	compatible = "mediatek,mt8173-mtu3";
+	reg = <0 0x11271000 0 0x3000>,
+	      <0 0x11280700 0 0x0100>;
+	reg-names = "mac", "ippc";
+	interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_LOW>;
+	phys = <&phy_port0 PHY_TYPE_USB3>,
+	       <&phy_port1 PHY_TYPE_USB2>;
+	power-domains = <&scpsys MT8173_POWER_DOMAIN_USB>;
+	clocks = <&topckgen CLK_TOP_USB30_SEL>,
+		 <&pericfg CLK_PERI_USB0>,
+		 <&pericfg CLK_PERI_USB1>;
+	clock-names = "sys_ck",
+		      "wakeup_deb_p0",
+		      "wakeup_deb_p1";
+	vusb33-supply = <&mt6397_vusb_reg>;
+	vbus-supply = <&usb_p0_vbus>;
+	extcon = <&extcon_usb>;
+	dr_mode = "otg";
+	mediatek,enable-wakeup;
+	mediatek,syscon-wakeup = <&pericfg>;
+	#address-cells = <2>;
+	#size-cells = <2>;
+	ranges;
+	status = "disabled";
+
+	usb_host: xhci at 11270000 {
+		compatible = "mediatek,mt8173-xhci";
+		reg = <0 0x11270000 0 0x1000>;
+		reg-names = "mac";
+		interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_LOW>;
+		power-domains = <&scpsys MT8173_POWER_DOMAIN_USB>;
+		clocks = <&topckgen CLK_TOP_USB30_SEL>;
+		clock-names = "sys_ck";
+		vusb33-supply = <&mt6397_vusb_reg>;
+		status = "disabled";
+	};
+};
-- 
1.7.9.5

[RESEND PATCH, v5 3/5] usb: xhci-mtk: make IPPC register optional

[Chunfeng Yun]

Make IPPC register optional to support host side of dual-role mode,
due to it is moved into common glue layer for simplification.

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 drivers/usb/host/xhci-mtk.c |   36 +++++++++++++++++++++++++++++-------
 1 file changed, 29 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index 79959f1..4bf99b9 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -94,6 +94,9 @@ static int xhci_mtk_host_enable(struct xhci_hcd_mtk *mtk)
 	int ret;
 	int i;
 
+	if (ippc == NULL)
+		return 0;
+
 	/* power on host ip */
 	value = readl(&ippc->ip_pw_ctr1);
 	value &= ~CTRL1_IP_HOST_PDN;
@@ -139,6 +142,9 @@ static int xhci_mtk_host_disable(struct xhci_hcd_mtk *mtk)
 	int ret;
 	int i;
 
+	if (ippc == NULL)
+		return 0;
+
 	/* power down all u3 ports */
 	for (i = 0; i < mtk->num_u3_ports; i++) {
 		value = readl(&ippc->u3_ctrl_p[i]);
@@ -173,6 +179,9 @@ static int xhci_mtk_ssusb_config(struct xhci_hcd_mtk *mtk)
 	struct mu3c_ippc_regs __iomem *ippc = mtk->ippc_regs;
 	u32 value;
 
+	if (ippc == NULL)
+		return 0;
+
 	/* reset whole ip */
 	value = readl(&ippc->ip_pw_ctr0);
 	value |= CTRL0_IP_SW_RST;
@@ -475,6 +484,7 @@ static void xhci_mtk_quirks(struct device *dev, struct xhci_hcd *xhci)
 /* called during probe() after chip reset completes */
 static int xhci_mtk_setup(struct usb_hcd *hcd)
 {
+	struct xhci_hcd *xhci = hcd_to_xhci(hcd);
 	struct xhci_hcd_mtk *mtk = hcd_to_mtk(hcd);
 	int ret;
 
@@ -482,12 +492,21 @@ static int xhci_mtk_setup(struct usb_hcd *hcd)
 		ret = xhci_mtk_ssusb_config(mtk);
 		if (ret)
 			return ret;
+	}
+
+	ret = xhci_gen_setup(hcd, xhci_mtk_quirks);
+	if (ret)
+		return ret;
+
+	if (usb_hcd_is_primary_hcd(hcd)) {
+		mtk->num_u3_ports = xhci->num_usb3_ports;
+		mtk->num_u2_ports = xhci->num_usb2_ports;
 		ret = xhci_mtk_sch_init(mtk);
 		if (ret)
 			return ret;
 	}
 
-	return xhci_gen_setup(hcd, xhci_mtk_quirks);
+	return ret;
 }
 
 static int xhci_mtk_probe(struct platform_device *pdev)
@@ -586,7 +605,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	mtk->hcd = platform_get_drvdata(pdev);
 	platform_set_drvdata(pdev, mtk);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "mac");
 	hcd->regs = devm_ioremap_resource(dev, res);
 	if (IS_ERR(hcd->regs)) {
 		ret = PTR_ERR(hcd->regs);
@@ -595,11 +614,14 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	hcd->rsrc_start = res->start;
 	hcd->rsrc_len = resource_size(res);
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
-	mtk->ippc_regs = devm_ioremap_resource(dev, res);
-	if (IS_ERR(mtk->ippc_regs)) {
-		ret = PTR_ERR(mtk->ippc_regs);
-		goto put_usb2_hcd;
+	mtk->ippc_regs = NULL;
+	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ippc");
+	if (res) {	/* ippc register is optional */
+		mtk->ippc_regs = devm_ioremap_resource(dev, res);
+		if (IS_ERR(mtk->ippc_regs)) {
+			ret = PTR_ERR(mtk->ippc_regs);
+			goto put_usb2_hcd;
+		}
 	}
 
 	for (phy_num = 0; phy_num < mtk->num_phys; phy_num++) {
-- 
1.7.9.5

[RESEND PATCH, v5 5/5] arm64: dts: mediatek: add USB3 DRD driver

[Chunfeng Yun]

USB3 DRD driver is added for MT8173-EVB, and xHCI driver
becomes its subnode

Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>
---
 arch/arm64/boot/dts/mediatek/mt8173-evb.dts |   46 +++++++++++++++++++++++++--
 arch/arm64/boot/dts/mediatek/mt8173.dtsi    |   29 +++++++++++++----
 2 files changed, 66 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
index 7453a47..682dfd7 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8173-evb.dts
@@ -34,6 +34,11 @@
 
 	chosen { };
 
+	extcon_usb: extcon_iddig {
+		compatible = "linux,extcon-usb-gpio";
+		id-gpio = <&pio 16 GPIO_ACTIVE_HIGH>;
+	};
+
 	usb_p1_vbus: regulator at 0 {
 		compatible = "regulator-fixed";
 		regulator-name = "usb_vbus";
@@ -42,6 +47,16 @@
 		gpio = <&pio 130 GPIO_ACTIVE_HIGH>;
 		enable-active-high;
 	};
+
+	usb_p0_vbus: regulator at 1 {
+		compatible = "regulator-fixed";
+		regulator-name = "vbus";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		gpio = <&pio 9 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+	};
+
 };
 
 &i2c1 {
@@ -205,6 +220,20 @@
 			bias-pull-down = <MTK_PUPD_SET_R1R0_10>;
 		};
 	};
+
+	usb_id_pins_float: usb_iddig_pull_up {
+		pins_iddig {
+			pinmux = <MT8173_PIN_16_IDDIG__FUNC_IDDIG>;
+			bias-pull-up;
+		};
+	};
+
+	usb_id_pins_ground: usb_iddig_pull_down {
+		pins_iddig {
+			pinmux = <MT8173_PIN_16_IDDIG__FUNC_IDDIG>;
+			bias-pull-down;
+		};
+	};
 };
 
 &pwm0 {
@@ -431,12 +460,25 @@
 	status = "okay";
 };
 
+&ssusb {
+	vusb33-supply = <&mt6397_vusb_reg>;
+	vbus-supply = <&usb_p0_vbus>;
+	extcon = <&extcon_usb>;
+	dr_mode = "otg";
+	mediatek,enable-wakeup;
+	pinctrl-names = "default", "id_float", "id_ground";
+	pinctrl-0 = <&usb_id_pins_float>;
+	pinctrl-1 = <&usb_id_pins_float>;
+	pinctrl-2 = <&usb_id_pins_ground>;
+	status = "okay";
+};
+
 &uart0 {
 	status = "okay";
 };
 
-&usb30 {
+&usb_host {
 	vusb33-supply = <&mt6397_vusb_reg>;
 	vbus-supply = <&usb_p1_vbus>;
-	mediatek,wakeup-src = <1>;
+	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/mediatek/mt8173.dtsi b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
index 10f638f..925948a 100644
--- a/arch/arm64/boot/dts/mediatek/mt8173.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8173.dtsi
@@ -668,11 +668,14 @@
 			status = "disabled";
 		};
 
-		usb30: usb at 11270000 {
-			compatible = "mediatek,mt8173-xhci";
-			reg = <0 0x11270000 0 0x1000>,
+		ssusb: usb at 11271000 {
+			compatible = "mediatek,mt8173-mtu3";
+			reg = <0 0x11271000 0 0x3000>,
 			      <0 0x11280700 0 0x0100>;
-			interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_LOW>;
+			reg-names = "mac", "ippc";
+			interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_LOW>;
+			phys = <&phy_port0 PHY_TYPE_USB3>,
+			       <&phy_port1 PHY_TYPE_USB2>;
 			power-domains = <&scpsys MT8173_POWER_DOMAIN_USB>;
 			clocks = <&topckgen CLK_TOP_USB30_SEL>,
 				 <&pericfg CLK_PERI_USB0>,
@@ -680,10 +683,22 @@
 			clock-names = "sys_ck",
 				      "wakeup_deb_p0",
 				      "wakeup_deb_p1";
-			phys = <&phy_port0 PHY_TYPE_USB3>,
-			       <&phy_port1 PHY_TYPE_USB2>;
 			mediatek,syscon-wakeup = <&pericfg>;
-			status = "okay";
+			#address-cells = <2>;
+			#size-cells = <2>;
+			ranges;
+			status = "disabled";
+
+			usb_host: xhci at 11270000 {
+				compatible = "mediatek,mt8173-xhci";
+				reg = <0 0x11270000 0 0x1000>;
+				reg-names = "mac";
+				interrupts = <GIC_SPI 115 IRQ_TYPE_LEVEL_LOW>;
+				power-domains = <&scpsys MT8173_POWER_DOMAIN_USB>;
+				clocks = <&topckgen CLK_TOP_USB30_SEL>;
+				clock-names = "sys_ck";
+				status = "disabled";
+			};
 		};
 
 		u3phy: usb-phy at 11290000 {
-- 
1.7.9.5

[RESEND PATCH, v5 4/5] usb: Add MediaTek USB3 DRD Driver

[Oliver Neukum]

On Thu, 2016-08-25 at 11:05 +0800, Chunfeng Yun wrote:
> This patch adds support for the MediaTek USB3 controller
> integrated into MT8173. It can be configured as Dual-Role
> Device (DRD), Peripheral Only and Host Only (xHCI) modes.
> 

> +/**
> + * General Purpose Descriptor (GPD):
> + *	The format of TX GPD is a little different from RX one.
> + *	And the size of GPD is 16 bytes.
> + *
> + * @flag:
> + *	bit0: Hardware Own (HWO)
> + *	bit1: Buffer Descriptor Present (BDP), always 0, BD is not supported
> + *	bit2: Bypass (BPS), 1: HW skips this GPD if HWO = 1
> + *	bit7: Interrupt On Completion (IOC)
> + * @chksum: This is used to validate the contents of this GPD;
> + *	If TXQ_CS_EN / RXQ_CS_EN bit is set, an interrupt is issued
> + *	when checksum validation fails;
> + *	Checksum value is calculated over the 16 bytes of the GPD by default;
> + * @data_buf_len (RX ONLY): This value indicates the length of
> + *	the assigned data buffer
> + * @next_gpd: Physical address of the next GPD
> + * @buffer: Physical address of the data buffer
> + * @buf_len:
> + *	(TX): This value indicates the length of the assigned data buffer
> + *	(RX): The total length of data received
> + * @ext_len: reserved
> + * @ext_flag:
> + *	bit5 (TX ONLY): Zero Length Packet (ZLP),
> + */
> +struct qmu_gpd {
> +	u8 flag;
> +	u8 chksum;
> +	u16 data_buf_len;
> +	u32 next_gpd;
> +	u32 buffer;
> +	u16 buf_len;
> +	u8 ext_len;
> +	u8 ext_flag;
> +} __packed;

It looks like this is shared with hardware in memory.
But you leave the endianness of the bigger fields undeclared.

> +/**
> +* dma: physical base address of GPD segment
> +* start: virtual base address of GPD segment
> +* end: the last GPD element
> +* enqueue: the first empty GPD to use
> +* dequeue: the first completed GPD serviced by ISR
> +* NOTE: the size of GPD ring should be >= 2
> +*/
> +struct mtu3_gpd_ring {
> +	dma_addr_t dma;
> +	struct qmu_gpd *start;
> +	struct qmu_gpd *end;
> +	struct qmu_gpd *enqueue;
> +	struct qmu_gpd *dequeue;
> +};
> +
> +/**
> +* @vbus: vbus 5V used by host mode
> +* @edev: external connector used to detect vbus and iddig changes
> +* @vbus_nb: notifier for vbus detection
> +* @vbus_nb: notifier for iddig(idpin) detection
> +* @extcon_reg_dwork: delay work for extcon notifier register, waiting for
> +*		xHCI driver initialization, it's necessary for system bootup
> +*		as device.
> +* @is_u3_drd: whether port0 supports usb3.0 dual-role device or not
> +* @id_*: used to maually switch between host and device modes by idpin
> +* @manual_drd_enabled: it's true when supports dual-role device by debugfs
> +*		to switch host/device modes depending on user input.

Please use a common interface for this. The Intel people are introducing
one.


> +#endif
> diff --git a/drivers/usb/mtu3/mtu3_core.c b/drivers/usb/mtu3/mtu3_core.c
> new file mode 100644
> index 0000000..fdc11b6
> --- /dev/null
> +++ b/drivers/usb/mtu3/mtu3_core.c
> @@ -0,0 +1,874 @@
> +/*
> + * mtu3_core.c - hardware access layer and gadget init/exit of
> + *                     MediaTek usb3 Dual-Role Controller Driver
> + *
> + * Copyright (C) 2016 MediaTek Inc.
> + *
> + * Author: Chunfeng Yun <chunfeng.yun@mediatek.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_irq.h>
> +#include <linux/platform_device.h>
> +
> +#include "mtu3.h"
> +
> +static int ep_fifo_alloc(struct mtu3_ep *mep, u32 seg_size)
> +{
> +	struct mtu3_fifo_info *fifo = mep->fifo;
> +	u32 num_bits = DIV_ROUND_UP(seg_size, MTU3_EP_FIFO_UNIT);
> +	u32 start_bit;
> +
> +	/* ensure that @mep->fifo_seg_size is power of two */
> +	num_bits = roundup_pow_of_two(num_bits);
> +	if (num_bits > fifo->limit)
> +		return -EINVAL;
> +
> +	mep->fifo_seg_size = num_bits * MTU3_EP_FIFO_UNIT;
> +	num_bits = num_bits * (mep->slot + 1);
> +	start_bit = bitmap_find_next_zero_area(fifo->bitmap,
> +			fifo->limit, 0, num_bits, 0);
> +	if (start_bit >= fifo->limit)
> +		return -EOVERFLOW;
> +
> +	bitmap_set(fifo->bitmap, start_bit, num_bits);
> +	mep->fifo_size = num_bits * MTU3_EP_FIFO_UNIT;
> +	mep->fifo_addr = fifo->base + MTU3_EP_FIFO_UNIT * start_bit;
> +
> +	dev_dbg(mep->mtu->dev, "%s fifo:%#x/%#x, start_bit: %d\n",
> +		__func__, mep->fifo_seg_size, mep->fifo_size, start_bit);

If you use the "f" option to dynamic debugging it will give you the
function name anyway. So why include __func__ ?


> +/* enable/disable U3D SS function */
> +static void mtu3_ss_func_set(struct mtu3 *mtu, bool enable)

Inline maybe ?

> +{
> +	/* If usb3_en==0, LTSSM will go to SS.Disable state */
> +	if (enable)
> +		mtu3_setbits(mtu->mac_base, U3D_USB3_CONFIG, USB3_EN);
> +	else
> +		mtu3_clrbits(mtu->mac_base, U3D_USB3_CONFIG, USB3_EN);
> +
> +	dev_dbg(mtu->dev, "USB3_EN = %d\n", !!enable);
> +}
> +

[..]


> +int mtu3_config_ep(struct mtu3 *mtu, struct mtu3_ep *mep,
> +			int interval, int burst, int mult)
> +{
> +	void __iomem *mbase = mtu->mac_base;
> +	int epnum = mep->epnum;
> +	u32 csr0, csr1, csr2;
> +	int fifo_sgsz, fifo_addr;
> +	int num_pkts;
> +
> +	fifo_addr = ep_fifo_alloc(mep, mep->maxp);
> +	if (fifo_addr < 0) {
> +		dev_err(mtu->dev, "alloc ep fifo failed(%d)\n", mep->maxp);
> +		return -ENOMEM;
> +	}
> +	fifo_sgsz = ilog2(mep->fifo_seg_size);
> +	dev_dbg(mtu->dev, "%s fifosz: %x(%x/%x)\n", __func__, fifo_sgsz,
> +		mep->fifo_seg_size, mep->fifo_size);
> +
> +	if (mep->is_in) {
> +		csr0 = TX_TXMAXPKTSZ(mep->maxp);
> +		csr0 |= TX_DMAREQEN;
> +
> +		num_pkts = (burst + 1) * (mult + 1) - 1;
> +		csr1 = TX_SS_BURST(burst) | TX_SLOT(mep->slot);
> +		csr1 |= TX_MAX_PKT(num_pkts) | TX_MULT(mult);
> +
> +		csr2 = TX_FIFOADDR(fifo_addr >> 4);
> +		csr2 |= TX_FIFOSEGSIZE(fifo_sgsz);
> +
> +		switch (mep->type) {
> +		case USB_ENDPOINT_XFER_BULK:
> +			csr1 |= TX_TYPE(TYPE_BULK);
> +			break;
> +		case USB_ENDPOINT_XFER_ISOC:
> +			csr1 |= TX_TYPE(TYPE_ISO);
> +			csr2 |= TX_BINTERVAL(interval);
> +			break;
> +		case USB_ENDPOINT_XFER_INT:
> +			csr1 |= TX_TYPE(TYPE_INT);
> +			csr2 |= TX_BINTERVAL(interval);
> +			break;

And if it is control?

> +		}
> +
> +		/* Enable QMU Done interrupt */
> +		mtu3_setbits(mbase, U3D_QIESR0, QMU_TX_DONE_INT(epnum));
> +
> +		mtu3_writel(mbase, MU3D_EP_TXCR0(epnum), csr0);
> +		mtu3_writel(mbase, MU3D_EP_TXCR1(epnum), csr1);
> +		mtu3_writel(mbase, MU3D_EP_TXCR2(epnum), csr2);
> +
> +		dev_dbg(mtu->dev, "U3D_TX%d CSR0:%#x, CSR1:%#x, CSR2:%#x\n",
> +			epnum, mtu3_readl(mbase, MU3D_EP_TXCR0(epnum)),
> +			mtu3_readl(mbase, MU3D_EP_TXCR1(epnum)),
> +			mtu3_readl(mbase, MU3D_EP_TXCR2(epnum)));
> +	} else {
> +		csr0 = RX_RXMAXPKTSZ(mep->maxp);
> +		csr0 |= RX_DMAREQEN;
> +
> +		num_pkts = (burst + 1) * (mult + 1) - 1;
> +		csr1 = RX_SS_BURST(burst) | RX_SLOT(mep->slot);
> +		csr1 |= RX_MAX_PKT(num_pkts) | RX_MULT(mult);
> +
> +		csr2 = RX_FIFOADDR(fifo_addr >> 4);
> +		csr2 |= RX_FIFOSEGSIZE(fifo_sgsz);
> +
> +		switch (mep->type) {
> +		case USB_ENDPOINT_XFER_BULK:
> +			csr1 |= RX_TYPE(TYPE_BULK);
> +			break;
> +		case USB_ENDPOINT_XFER_ISOC:
> +			csr1 |= RX_TYPE(TYPE_ISO);
> +			csr2 |= RX_BINTERVAL(interval);
> +			break;
> +		case USB_ENDPOINT_XFER_INT:
> +			csr1 |= RX_TYPE(TYPE_INT);
> +			csr2 |= RX_BINTERVAL(interval);
> +			break;

Again: control endpoints?

> +		}
> +
> +		/*Enable QMU Done interrupt */
> +		mtu3_setbits(mbase, U3D_QIESR0, QMU_RX_DONE_INT(epnum));
> +
> +		mtu3_writel(mbase, MU3D_EP_RXCR0(epnum), csr0);
> +		mtu3_writel(mbase, MU3D_EP_RXCR1(epnum), csr1);
> +		mtu3_writel(mbase, MU3D_EP_RXCR2(epnum), csr2);
> +
> +		dev_dbg(mtu->dev, "U3D_RX%d CSR0:%#x, CSR1:%#x, CSR2:%#x\n",
> +			epnum, mtu3_readl(mbase, MU3D_EP_RXCR0(epnum)),
> +			mtu3_readl(mbase, MU3D_EP_RXCR1(epnum)),
> +			mtu3_readl(mbase, MU3D_EP_RXCR2(epnum)));
> +	}
> +
> +	dev_dbg(mtu->dev, "csr0:%#x, csr1:%#x, csr2:%#x\n", csr0, csr1, csr2);
> +	dev_dbg(mtu->dev, "%s: %s, fifo-addr:%#x, fifo-size:%#x(%#x/%#x)\n",
> +		__func__, mep->name, mep->fifo_addr, mep->fifo_size,
> +		fifo_sgsz, mep->fifo_seg_size);
> +
> +	return 0;
> +}
> +

[..]
> +static void mtu3_set_speed(struct mtu3 *mtu)
> +{
> +	void __iomem *mbase = mtu->mac_base;
> +
> +	if (!mtu->is_u3_ip && (mtu->max_speed > USB_SPEED_HIGH))
> +		mtu->max_speed = USB_SPEED_HIGH;

You are missing the checks for USB_SPEED_WIRELESS in general.
Can you just ignore it?

> +
> +	if (mtu->max_speed == USB_SPEED_FULL) {
> +		/* disable U3 SS function */
> +		mtu3_clrbits(mbase, U3D_USB3_CONFIG, USB3_EN);
> +		/* disable HS function */
> +		mtu3_clrbits(mbase, U3D_POWER_MANAGEMENT, HS_ENABLE);
> +	} else if (mtu->max_speed == USB_SPEED_HIGH) {
> +		mtu3_clrbits(mbase, U3D_USB3_CONFIG, USB3_EN);
> +		/* HS/FS detected by HW */
> +		mtu3_setbits(mbase, U3D_POWER_MANAGEMENT, HS_ENABLE);
> +	}
> +	dev_info(mtu->dev, "max_speed: %s\n",
> +		usb_speed_string(mtu->max_speed));
> +}

[..]
> +static irqreturn_t mtu3_link_isr(struct mtu3 *mtu)
> +{
> +	void __iomem *mbase = mtu->mac_base;
> +	enum usb_device_speed udev_speed;
> +	u32 maxpkt = 64;
> +	u32 link;
> +	u32 speed;
> +
> +	link = mtu3_readl(mbase, U3D_DEV_LINK_INTR);
> +	link &= mtu3_readl(mbase, U3D_DEV_LINK_INTR_ENABLE);
> +	mtu3_writel(mbase, U3D_DEV_LINK_INTR, link); /* W1C */
> +	dev_dbg(mtu->dev, "=== LINK[%x] ===\n", link);
> +
> +	if (!(link & SSUSB_DEV_SPEED_CHG_INTR))
> +		return IRQ_NONE;

Shouldn't you do this check before you clear interrupts?

> +	speed = SSUSB_DEV_SPEED(mtu3_readl(mbase, U3D_DEVICE_CONF));

Do you really want to read this out of the hardware on every interrupt?

> +
> +	switch (speed) {
> +	case MTU3_SPEED_FULL:
> +		udev_speed = USB_SPEED_FULL;
> +		/*BESLCK = 4 < BESLCK_U3 = 10 < BESLDCK = 15 */
> +		mtu3_writel(mbase, U3D_USB20_LPM_PARAMETER, LPM_BESLDCK(0xf)
> +				| LPM_BESLCK(4) | LPM_BESLCK_U3(0xa));
> +		mtu3_setbits(mbase, U3D_POWER_MANAGEMENT,
> +				LPM_BESL_STALL | LPM_BESLD_STALL);
> +		break;
> +	case MTU3_SPEED_HIGH:
> +		udev_speed = USB_SPEED_HIGH;
> +		/*BESLCK = 4 < BESLCK_U3 = 10 < BESLDCK = 15 */
> +		mtu3_writel(mbase, U3D_USB20_LPM_PARAMETER, LPM_BESLDCK(0xf)
> +				| LPM_BESLCK(4) | LPM_BESLCK_U3(0xa));
> +		mtu3_setbits(mbase, U3D_POWER_MANAGEMENT,
> +				LPM_BESL_STALL | LPM_BESLD_STALL);
> +		break;
> +	case MTU3_SPEED_SUPER:
> +		udev_speed = USB_SPEED_SUPER;
> +		maxpkt = 512;
> +		break;
> +	default:
> +		udev_speed = USB_SPEED_UNKNOWN;
> +		break;
> +	}
> +	dev_dbg(mtu->dev, "%s: %s\n", __func__, usb_speed_string(udev_speed));
> +
> +	mtu->g.speed = udev_speed;
> +	mtu->g.ep0->maxpacket = maxpkt;
> +	mtu->ep0_state = MU3D_EP0_STATE_SETUP;
> +
> +	if (udev_speed == USB_SPEED_UNKNOWN)
> +		mtu3_gadget_disconnect(mtu);
> +	else
> +		mtu3_ep0_setup(mtu);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t mtu3_u3_ltssm_isr(struct mtu3 *mtu)
> +{
> +	void __iomem *mbase = mtu->mac_base;
> +	u32 ltssm;
> +
> +	ltssm = mtu3_readl(mbase, U3D_LTSSM_INTR);
> +	ltssm &= mtu3_readl(mbase, U3D_LTSSM_INTR_ENABLE);
> +	mtu3_writel(mbase, U3D_LTSSM_INTR, ltssm); /* W1C */
> +	dev_dbg(mtu->dev, "=== LTSSM[%x] ===\n", ltssm);
> +
> +	if (ltssm & HOT_RST_INTR)
> +		mtu3_gadget_reset(mtu);
> +
> +	if (ltssm & WARM_RST_INTR)
> +		mtu3_gadget_reset(mtu);

You really would reset the gadget twice if that happens?
And do the rest of the tests make sense in that case?

> +
> +	if (ltssm & VBUS_FALL_INTR)
> +		mtu3_ss_func_set(mtu, false);
> +
> +	if (ltssm & VBUS_RISE_INTR)
> +		mtu3_ss_func_set(mtu, true);
> +
> +	if (ltssm & EXIT_U3_INTR)
> +		mtu3_gadget_resume(mtu);
> +
> +	if (ltssm & ENTER_U3_INTR)
> +		mtu3_gadget_suspend(mtu);
> +
> +	return IRQ_HANDLED;
> +}

[..]

> +static int mtu3_hw_init(struct mtu3 *mtu)
> +{
> +	int ret;
> +
> +	mtu3_device_reset(mtu);
> +
> +	ret = mtu3_device_enable(mtu);
> +	if (ret) {
> +		dev_err(mtu->dev, "device enable failed %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mtu3_mem_alloc(mtu);
> +	if (ret) {
> +		dev_err(mtu->dev, "mem alloc failed, aborting\n");

1. You can't leave the device enabled in that case.
2. No need for a message. The kernel will already do that if kmalloc()
fails under such circumstances.

> +		return -ENOMEM;
> +	}
> +
> +	mtu3_regs_init(mtu);
> +
> +	return 0;
> +}

> diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
> new file mode 100644
> index 0000000..f560f93
> --- /dev/null
> +++ b/drivers/usb/mtu3/mtu3_dr.c
> @@ -0,0 +1,375 @@
> +/*
> + * mtu3_dr.c - dual role switch and host glue layer
> + *
> + * Copyright (C) 2016 MediaTek Inc.
> + *
> + * Author: Chunfeng Yun <chunfeng.yun@mediatek.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * 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.
> + *
> + */
> +
> +#include <linux/debugfs.h>
> +#include <linux/irq.h>
> +#include <linux/kernel.h>
> +#include <linux/of_device.h>
> +#include <linux/pinctrl/consumer.h>
> +#include <linux/seq_file.h>
> +#include <linux/uaccess.h>
> +
> +#include "mtu3.h"
> +#include "mtu3_dr.h"
> +
> +#define USB2_PORT 2
> +#define USB3_PORT 3
> +
> +enum mtu3_vbus_id_state {
> +	MTU3_ID_FLOAT = 1,
> +	MTU3_ID_GROUND,
> +	MTU3_VBUS_OFF,
> +	MTU3_VBUS_VALID,
> +};
> +
> +static void toggle_opstate(struct ssusb_mtk *ssusb)
> +{
> +	if (!ssusb->otg_switch.is_u3_drd) {
> +		mtu3_setbits(ssusb->mac_base, U3D_DEVICE_CONTROL, DC_SESSION);
> +		mtu3_setbits(ssusb->mac_base, U3D_POWER_MANAGEMENT, SOFT_CONN);
> +	}
> +}
> +
> +/* only port0 supports dual-role mode */
> +static int ssusb_port0_switch(struct ssusb_mtk *ssusb,
> +	int version, bool tohost)
> +{
> +	void __iomem *ibase = ssusb->ippc_base;
> +	u32 value;
> +
> +	dev_dbg(ssusb->dev, "%s (switch u%d port0 to %s)\n", __func__,
> +		version, tohost ? "host" : "device");
> +
> +	if (version == USB2_PORT) {
> +		/* 1. power off and disable u2 port0 */
> +		value = mtu3_readl(ibase, SSUSB_U2_CTRL(0));
> +		value |= SSUSB_U2_PORT_PDN | SSUSB_U2_PORT_DIS;
> +		mtu3_writel(ibase, SSUSB_U2_CTRL(0), value);
> +
> +		/* 2. power on, enable u2 port0 and select its mode */
> +		value = mtu3_readl(ibase, SSUSB_U2_CTRL(0));
> +		value &= ~(SSUSB_U2_PORT_PDN | SSUSB_U2_PORT_DIS);
> +		value = tohost ? (value | SSUSB_U2_PORT_HOST_SEL) :
> +			(value & (~SSUSB_U2_PORT_HOST_SEL));
> +		mtu3_writel(ibase, SSUSB_U2_CTRL(0), value);
> +	} else {
> +		/* 1. power off and disable u3 port0 */
> +		value = mtu3_readl(ibase, SSUSB_U3_CTRL(0));
> +		value |= SSUSB_U3_PORT_PDN | SSUSB_U3_PORT_DIS;
> +		mtu3_writel(ibase, SSUSB_U3_CTRL(0), value);
> +
> +		/* 2. power on, enable u3 port0 and select its mode */
> +		value = mtu3_readl(ibase, SSUSB_U3_CTRL(0));
> +		value &= ~(SSUSB_U3_PORT_PDN | SSUSB_U3_PORT_DIS);
> +		value = tohost ? (value | SSUSB_U3_PORT_HOST_SEL) :
> +			(value & (~SSUSB_U3_PORT_HOST_SEL));
> +		mtu3_writel(ibase, SSUSB_U3_CTRL(0), value);
> +	}
> +
> +	return 0;
> +}
> +
> +static void switch_port_to_host(struct ssusb_mtk *ssusb)
> +{
> +	u32 check_clk = 0;
> +
> +	dev_dbg(ssusb->dev, "%s\n", __func__);
> +
> +	ssusb_port0_switch(ssusb, USB2_PORT, true);
> +
> +	if (ssusb->otg_switch.is_u3_drd) {
> +		ssusb_port0_switch(ssusb, USB3_PORT, true);
> +		check_clk = SSUSB_U3_MAC_RST_B_STS;
> +	}
> +
> +	ssusb_check_clocks(ssusb, check_clk);
> +
> +	/* after all clocks are stable */
> +	toggle_opstate(ssusb);
> +}
> +
> +static void switch_port_to_device(struct ssusb_mtk *ssusb)
> +{
> +	u32 check_clk = 0;
> +
> +	dev_dbg(ssusb->dev, "%s\n", __func__);
> +
> +	ssusb_port0_switch(ssusb, USB2_PORT, false);
> +
> +	if (ssusb->otg_switch.is_u3_drd) {
> +		ssusb_port0_switch(ssusb, USB3_PORT, false);
> +		check_clk = SSUSB_U3_MAC_RST_B_STS;
> +	}
> +
> +	ssusb_check_clocks(ssusb, check_clk);
> +}
> +
> +int ssusb_set_vbus(struct otg_switch_mtk *otg_sx, int is_on)
> +{
> +	struct ssusb_mtk *ssusb =
> +		container_of(otg_sx, struct ssusb_mtk, otg_switch);
> +	struct regulator *vbus = otg_sx->vbus;
> +	int ret;
> +
> +	/* vbus is optional */
> +	if (!vbus)
> +		return 0;
> +
> +	dev_dbg(ssusb->dev, "%s: turn %s\n", __func__, is_on ? "on" : "off");
> +
> +	if (is_on) {
> +		ret = regulator_enable(vbus);
> +		if (ret) {
> +			dev_err(ssusb->dev, "vbus regulator enable failed\n");
> +			return ret;
> +		}
> +	} else {
> +		regulator_disable(vbus);
> +	}
> +
> +	return 0;
> +}
> +
> +/*
> + * switch to host: -> MTU3_VBUS_OFF --> MTU3_ID_GROUND
> + * switch to device: -> MTU3_ID_FLOAT --> MTU3_VBUS_VALID
> + */
> +static void ssusb_set_mailbox(struct otg_switch_mtk *otg_sx,
> +	enum mtu3_vbus_id_state status)
> +{
> +	struct ssusb_mtk *ssusb =
> +		container_of(otg_sx, struct ssusb_mtk, otg_switch);
> +	struct mtu3 *mtu = ssusb->u3d;
> +
> +	dev_dbg(ssusb->dev, "mailbox state(%d)\n", status);
> +
> +	switch (status) {
> +	case MTU3_ID_GROUND:
> +		switch_port_to_host(ssusb);
> +		ssusb_set_vbus(otg_sx, 1);
> +		ssusb->is_host = true;
> +		break;
> +	case MTU3_ID_FLOAT:
> +		ssusb->is_host = false;
> +		ssusb_set_vbus(otg_sx, 0);
> +		switch_port_to_device(ssusb);
> +		break;
> +	case MTU3_VBUS_OFF:
> +		mtu3_stop(mtu);
> +		pm_relax(ssusb->dev);
> +		break;
> +	case MTU3_VBUS_VALID:
> +		/* avoid suspend when works as device */
> +		pm_stay_awake(ssusb->dev);
> +		mtu3_start(mtu);
> +		break;
> +	default:
> +		dev_err(ssusb->dev, "invalid state\n");
> +	}
> +}
> +
> +static int ssusb_id_notifier(struct notifier_block *nb,
> +	unsigned long event, void *ptr)
> +{
> +	struct otg_switch_mtk *otg_sx =
> +		container_of(nb, struct otg_switch_mtk, id_nb);
> +
> +	if (event)
> +		ssusb_set_mailbox(otg_sx, MTU3_ID_GROUND);
> +	else
> +		ssusb_set_mailbox(otg_sx, MTU3_ID_FLOAT);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int ssusb_vbus_notifier(struct notifier_block *nb,
> +	unsigned long event, void *ptr)
> +{
> +	struct otg_switch_mtk *otg_sx =
> +		container_of(nb, struct otg_switch_mtk, vbus_nb);
> +
> +	if (event)
> +		ssusb_set_mailbox(otg_sx, MTU3_VBUS_VALID);
> +	else
> +		ssusb_set_mailbox(otg_sx, MTU3_VBUS_OFF);
> +
> +	return NOTIFY_DONE;
> +}
> +
> +static int ssusb_extcon_register(struct otg_switch_mtk *otg_sx)
> +{
> +	struct ssusb_mtk *ssusb =
> +		container_of(otg_sx, struct ssusb_mtk, otg_switch);
> +	struct extcon_dev *edev = otg_sx->edev;
> +	int ret;
> +
> +	/* extcon is optional */
> +	if (!edev)
> +		return 0;
> +
> +	otg_sx->vbus_nb.notifier_call = ssusb_vbus_notifier;
> +	ret = extcon_register_notifier(edev, EXTCON_USB,
> +					&otg_sx->vbus_nb);
> +	if (ret < 0)
> +		dev_err(ssusb->dev, "failed to register notifier for USB\n");
> +
> +	otg_sx->id_nb.notifier_call = ssusb_id_notifier;
> +	ret = extcon_register_notifier(edev, EXTCON_USB_HOST,
> +					&otg_sx->id_nb);
> +	if (ret < 0)
> +		dev_err(ssusb->dev, "failed to register notifier for USB-HOST\n");
> +
> +	dev_dbg(ssusb->dev, "EXTCON_USB: %d, EXTCON_USB_HOST: %d\n",
> +		extcon_get_cable_state_(edev, EXTCON_USB),
> +		extcon_get_cable_state_(edev, EXTCON_USB_HOST));
> +
> +	/* default as host, switch to device mode if needed */
> +	if (extcon_get_cable_state_(edev, EXTCON_USB_HOST) == false)
> +		ssusb_set_mailbox(otg_sx, MTU3_ID_FLOAT);
> +	if (extcon_get_cable_state_(edev, EXTCON_USB) == true)
> +		ssusb_set_mailbox(otg_sx, MTU3_VBUS_VALID);
> +
> +	return 0;
> +}
> +
> +static void extcon_register_dwork(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	struct otg_switch_mtk *otg_sx =
> +	    container_of(dwork, struct otg_switch_mtk, extcon_reg_dwork);
> +
> +	ssusb_extcon_register(otg_sx);
> +}
> +
> +/*
> + * We provide an interface via debugfs to switch between host and device modes
> + * depending on user input.
> + * This is useful in special cases, such as uses TYPE-A receptacle but also
> + * wants to support dual-role mode.
> + * It generates cable state changes by pulling up/down IDPIN and
> + * notifies driver to switch mode by "extcon-usb-gpio".
> + * NOTE: when use MICRO receptacle, should not enable this interface.
> + */
> +static void ssusb_mode_manual_switch(struct ssusb_mtk *ssusb, int to_host)
> +{
> +	struct otg_switch_mtk *otg_sx = &ssusb->otg_switch;
> +
> +	if (to_host)
> +		pinctrl_select_state(otg_sx->id_pinctrl, otg_sx->id_ground);
> +	else
> +		pinctrl_select_state(otg_sx->id_pinctrl, otg_sx->id_float);
> +}
> +
> +
> +static int ssusb_mode_show(struct seq_file *sf, void *unused)
> +{
> +	struct ssusb_mtk *ssusb = sf->private;
> +
> +	seq_printf(sf, "current mode: %s(%s drd)\n(echo device/host)\n",
> +		ssusb->is_host ? "host" : "device",
> +		ssusb->otg_switch.manual_drd_enabled ? "manual" : "auto");
> +
> +	return 0;
> +}
> +
> +static int ssusb_mode_open(struct inode *inode, struct file *file)
> +{
> +	return single_open(file, ssusb_mode_show, inode->i_private);
> +}
> +
> +static ssize_t ssusb_mode_write(struct file *file,
> +	const char __user *ubuf, size_t count, loff_t *ppos)
> +{
> +	struct seq_file *sf = file->private_data;
> +	struct ssusb_mtk *ssusb = sf->private;
> +	char buf[16];
> +
> +	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> +		return -EFAULT;
> +
> +	if (!strncmp(buf, "host", 4) && !ssusb->is_host)
> +		ssusb_mode_manual_switch(ssusb, 1);
> +	else if (!strncmp(buf, "device", 6) && ssusb->is_host)
> +		ssusb_mode_manual_switch(ssusb, 0);
> +	else
> +		dev_err(ssusb->dev, "wrong or duplicated setting\n");

No proper error returns

> +
> +	return count;
> +}
> +
> +static const struct file_operations ssusb_mode_fops = {
> +	.open = ssusb_mode_open,
> +	.write = ssusb_mode_write,
> +	.read = seq_read,
> +	.llseek = seq_lseek,
> +	.release = single_release,
> +};
> +
> +static void ssusb_debugfs_init(struct ssusb_mtk *ssusb)
> +{
> +	struct dentry *root;
> +	struct dentry *file;
> +
> +	root = debugfs_create_dir(dev_name(ssusb->dev), usb_debug_root);
> +	if (IS_ERR_OR_NULL(root)) {
> +		if (!root)
> +			dev_err(ssusb->dev, "create debugfs root failed\n");
> +		return;
> +	}
> +	ssusb->dbgfs_root = root;
> +
> +	file = debugfs_create_file("mode", S_IRUGO | S_IWUSR, root,
> +			ssusb, &ssusb_mode_fops);
> +	if (!file)
> +		dev_dbg(ssusb->dev, "create debugfs mode failed\n");
> +}
> +
> +static void ssusb_debugfs_exit(struct ssusb_mtk *ssusb)
> +{
> +	debugfs_remove_recursive(ssusb->dbgfs_root);
> +}
> +
> +int ssusb_otg_switch_init(struct ssusb_mtk *ssusb)
> +{
> +	struct otg_switch_mtk *otg_sx = &ssusb->otg_switch;
> +
> +	INIT_DELAYED_WORK(&otg_sx->extcon_reg_dwork, extcon_register_dwork);
> +
> +	if (otg_sx->manual_drd_enabled)
> +		ssusb_debugfs_init(ssusb);
> +
> +	/* It is enough to delay 1s for waiting for host initialization */
> +	schedule_delayed_work(&otg_sx->extcon_reg_dwork, HZ);

You need to handle this still pending when you are deregistered.

> +
> +	return 0;
> +}
> +
> +void ssusb_otg_switch_exit(struct ssusb_mtk *ssusb)
> +{
> +	struct otg_switch_mtk *otg_sx = &ssusb->otg_switch;
> +
> +	if (otg_sx->edev) {
> +		extcon_unregister_notifier(otg_sx->edev,
> +			EXTCON_USB, &otg_sx->vbus_nb);
> +		extcon_unregister_notifier(otg_sx->edev,
> +			EXTCON_USB_HOST, &otg_sx->id_nb);
> +	}
> +
> +	if (otg_sx->manual_drd_enabled)
> +		ssusb_debugfs_exit(ssusb);
> +}

Could you break this up a bit? It is extensively long a patch?

	Regards
		Oliver

[RESEND PATCH, v5 4/5] usb: Add MediaTek USB3 DRD Driver

[chunfeng yun]

Hi,

On Thu, 2016-08-25 at 10:32 +0200, Oliver Neukum wrote:
> On Thu, 2016-08-25 at 11:05 +0800, Chunfeng Yun wrote:
> > This patch adds support for the MediaTek USB3 controller
> > integrated into MT8173. It can be configured as Dual-Role
> > Device (DRD), Peripheral Only and Host Only (xHCI) modes.
> > 
> 
> > +/**
> > + * General Purpose Descriptor (GPD):
> > + *	The format of TX GPD is a little different from RX one.
> > + *	And the size of GPD is 16 bytes.
> > + *
> > + * @flag:
> > + *	bit0: Hardware Own (HWO)
> > + *	bit1: Buffer Descriptor Present (BDP), always 0, BD is not supported
> > + *	bit2: Bypass (BPS), 1: HW skips this GPD if HWO = 1
> > + *	bit7: Interrupt On Completion (IOC)
> > + * @chksum: This is used to validate the contents of this GPD;
> > + *	If TXQ_CS_EN / RXQ_CS_EN bit is set, an interrupt is issued
> > + *	when checksum validation fails;
> > + *	Checksum value is calculated over the 16 bytes of the GPD by default;
> > + * @data_buf_len (RX ONLY): This value indicates the length of
> > + *	the assigned data buffer
> > + * @next_gpd: Physical address of the next GPD
> > + * @buffer: Physical address of the data buffer
> > + * @buf_len:
> > + *	(TX): This value indicates the length of the assigned data buffer
> > + *	(RX): The total length of data received
> > + * @ext_len: reserved
> > + * @ext_flag:
> > + *	bit5 (TX ONLY): Zero Length Packet (ZLP),
> > + */
> > +struct qmu_gpd {
> > +	u8 flag;
> > +	u8 chksum;
> > +	u16 data_buf_len;
> > +	u32 next_gpd;
> > +	u32 buffer;
> > +	u16 buf_len;
> > +	u8 ext_len;
> > +	u8 ext_flag;
> > +} __packed;
> 
> It looks like this is shared with hardware in memory.
> But you leave the endianness of the bigger fields undeclared.
> 
The driver only supports Little-Endian SoCs currently, because I have no
Big-Endian platform to test it.
 
> > +/**
> > +* dma: physical base address of GPD segment
> > +* start: virtual base address of GPD segment
> > +* end: the last GPD element
> > +* enqueue: the first empty GPD to use
> > +* dequeue: the first completed GPD serviced by ISR
> > +* NOTE: the size of GPD ring should be >= 2
> > +*/
> > +struct mtu3_gpd_ring {
> > +	dma_addr_t dma;
> > +	struct qmu_gpd *start;
> > +	struct qmu_gpd *end;
> > +	struct qmu_gpd *enqueue;
> > +	struct qmu_gpd *dequeue;
> > +};
> > +
> > +/**
> > +* @vbus: vbus 5V used by host mode
> > +* @edev: external connector used to detect vbus and iddig changes
> > +* @vbus_nb: notifier for vbus detection
> > +* @vbus_nb: notifier for iddig(idpin) detection
> > +* @extcon_reg_dwork: delay work for extcon notifier register, waiting for
> > +*		xHCI driver initialization, it's necessary for system bootup
> > +*		as device.
> > +* @is_u3_drd: whether port0 supports usb3.0 dual-role device or not
> > +* @id_*: used to maually switch between host and device modes by idpin
> > +* @manual_drd_enabled: it's true when supports dual-role device by debugfs
> > +*		to switch host/device modes depending on user input.
> 
> Please use a common interface for this. The Intel people are introducing
> one.
Can you give me the link address of the patch, I didn't find it.

> 
> 
> > +#endif
> > diff --git a/drivers/usb/mtu3/mtu3_core.c b/drivers/usb/mtu3/mtu3_core.c
> > new file mode 100644
> > index 0000000..fdc11b6
> > --- /dev/null
> > +++ b/drivers/usb/mtu3/mtu3_core.c
> > @@ -0,0 +1,874 @@
> > +/*
> > + * mtu3_core.c - hardware access layer and gadget init/exit of
> > + *                     MediaTek usb3 Dual-Role Controller Driver
> > + *
> > + * Copyright (C) 2016 MediaTek Inc.
> > + *
> > + * Author: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/platform_device.h>
> > +
> > +#include "mtu3.h"
> > +
> > +static int ep_fifo_alloc(struct mtu3_ep *mep, u32 seg_size)
> > +{
> > +	struct mtu3_fifo_info *fifo = mep->fifo;
> > +	u32 num_bits = DIV_ROUND_UP(seg_size, MTU3_EP_FIFO_UNIT);
> > +	u32 start_bit;
> > +
> > +	/* ensure that @mep->fifo_seg_size is power of two */
> > +	num_bits = roundup_pow_of_two(num_bits);
> > +	if (num_bits > fifo->limit)
> > +		return -EINVAL;
> > +
> > +	mep->fifo_seg_size = num_bits * MTU3_EP_FIFO_UNIT;
> > +	num_bits = num_bits * (mep->slot + 1);
> > +	start_bit = bitmap_find_next_zero_area(fifo->bitmap,
> > +			fifo->limit, 0, num_bits, 0);
> > +	if (start_bit >= fifo->limit)
> > +		return -EOVERFLOW;
> > +
> > +	bitmap_set(fifo->bitmap, start_bit, num_bits);
> > +	mep->fifo_size = num_bits * MTU3_EP_FIFO_UNIT;
> > +	mep->fifo_addr = fifo->base + MTU3_EP_FIFO_UNIT * start_bit;
> > +
> > +	dev_dbg(mep->mtu->dev, "%s fifo:%#x/%#x, start_bit: %d\n",
> > +		__func__, mep->fifo_seg_size, mep->fifo_size, start_bit);
> 
> If you use the "f" option to dynamic debugging it will give you the
> function name anyway. So why include __func__ ?
> 
It's used when dynamic debugging is disable but defines DEBUG macro.

> 
> > +/* enable/disable U3D SS function */
> > +static void mtu3_ss_func_set(struct mtu3 *mtu, bool enable)
> 
> Inline maybe ?
> 
> > +{
> > +	/* If usb3_en==0, LTSSM will go to SS.Disable state */
> > +	if (enable)
> > +		mtu3_setbits(mtu->mac_base, U3D_USB3_CONFIG, USB3_EN);
> > +	else
> > +		mtu3_clrbits(mtu->mac_base, U3D_USB3_CONFIG, USB3_EN);
> > +
> > +	dev_dbg(mtu->dev, "USB3_EN = %d\n", !!enable);
> > +}
> > +
> 
> [..]
> 
> 
> > +int mtu3_config_ep(struct mtu3 *mtu, struct mtu3_ep *mep,
> > +			int interval, int burst, int mult)
> > +{
> > +	void __iomem *mbase = mtu->mac_base;
> > +	int epnum = mep->epnum;
> > +	u32 csr0, csr1, csr2;
> > +	int fifo_sgsz, fifo_addr;
> > +	int num_pkts;
> > +
> > +	fifo_addr = ep_fifo_alloc(mep, mep->maxp);
> > +	if (fifo_addr < 0) {
> > +		dev_err(mtu->dev, "alloc ep fifo failed(%d)\n", mep->maxp);
> > +		return -ENOMEM;
> > +	}
> > +	fifo_sgsz = ilog2(mep->fifo_seg_size);
> > +	dev_dbg(mtu->dev, "%s fifosz: %x(%x/%x)\n", __func__, fifo_sgsz,
> > +		mep->fifo_seg_size, mep->fifo_size);
> > +
> > +	if (mep->is_in) {
> > +		csr0 = TX_TXMAXPKTSZ(mep->maxp);
> > +		csr0 |= TX_DMAREQEN;
> > +
> > +		num_pkts = (burst + 1) * (mult + 1) - 1;
> > +		csr1 = TX_SS_BURST(burst) | TX_SLOT(mep->slot);
> > +		csr1 |= TX_MAX_PKT(num_pkts) | TX_MULT(mult);
> > +
> > +		csr2 = TX_FIFOADDR(fifo_addr >> 4);
> > +		csr2 |= TX_FIFOSEGSIZE(fifo_sgsz);
> > +
> > +		switch (mep->type) {
> > +		case USB_ENDPOINT_XFER_BULK:
> > +			csr1 |= TX_TYPE(TYPE_BULK);
> > +			break;
> > +		case USB_ENDPOINT_XFER_ISOC:
> > +			csr1 |= TX_TYPE(TYPE_ISO);
> > +			csr2 |= TX_BINTERVAL(interval);
> > +			break;
> > +		case USB_ENDPOINT_XFER_INT:
> > +			csr1 |= TX_TYPE(TYPE_INT);
> > +			csr2 |= TX_BINTERVAL(interval);
> > +			break;
> 
> And if it is control?
The function is only called for non control EP.
I will add a comment for it.
> 
> > +		}
> > +
> > +		/* Enable QMU Done interrupt */
> > +		mtu3_setbits(mbase, U3D_QIESR0, QMU_TX_DONE_INT(epnum));
> > +
> > +		mtu3_writel(mbase, MU3D_EP_TXCR0(epnum), csr0);
> > +		mtu3_writel(mbase, MU3D_EP_TXCR1(epnum), csr1);
> > +		mtu3_writel(mbase, MU3D_EP_TXCR2(epnum), csr2);
> > +
> > +		dev_dbg(mtu->dev, "U3D_TX%d CSR0:%#x, CSR1:%#x, CSR2:%#x\n",
> > +			epnum, mtu3_readl(mbase, MU3D_EP_TXCR0(epnum)),
> > +			mtu3_readl(mbase, MU3D_EP_TXCR1(epnum)),
> > +			mtu3_readl(mbase, MU3D_EP_TXCR2(epnum)));
> > +	} else {
> > +		csr0 = RX_RXMAXPKTSZ(mep->maxp);
> > +		csr0 |= RX_DMAREQEN;
> > +
> > +		num_pkts = (burst + 1) * (mult + 1) - 1;
> > +		csr1 = RX_SS_BURST(burst) | RX_SLOT(mep->slot);
> > +		csr1 |= RX_MAX_PKT(num_pkts) | RX_MULT(mult);
> > +
> > +		csr2 = RX_FIFOADDR(fifo_addr >> 4);
> > +		csr2 |= RX_FIFOSEGSIZE(fifo_sgsz);
> > +
> > +		switch (mep->type) {
> > +		case USB_ENDPOINT_XFER_BULK:
> > +			csr1 |= RX_TYPE(TYPE_BULK);
> > +			break;
> > +		case USB_ENDPOINT_XFER_ISOC:
> > +			csr1 |= RX_TYPE(TYPE_ISO);
> > +			csr2 |= RX_BINTERVAL(interval);
> > +			break;
> > +		case USB_ENDPOINT_XFER_INT:
> > +			csr1 |= RX_TYPE(TYPE_INT);
> > +			csr2 |= RX_BINTERVAL(interval);
> > +			break;
> 
> Again: control endpoints?
Only for non-EP0.

> 
> > +		}
> > +
> > +		/*Enable QMU Done interrupt */
> > +		mtu3_setbits(mbase, U3D_QIESR0, QMU_RX_DONE_INT(epnum));
> > +
> > +		mtu3_writel(mbase, MU3D_EP_RXCR0(epnum), csr0);
> > +		mtu3_writel(mbase, MU3D_EP_RXCR1(epnum), csr1);
> > +		mtu3_writel(mbase, MU3D_EP_RXCR2(epnum), csr2);
> > +
> > +		dev_dbg(mtu->dev, "U3D_RX%d CSR0:%#x, CSR1:%#x, CSR2:%#x\n",
> > +			epnum, mtu3_readl(mbase, MU3D_EP_RXCR0(epnum)),
> > +			mtu3_readl(mbase, MU3D_EP_RXCR1(epnum)),
> > +			mtu3_readl(mbase, MU3D_EP_RXCR2(epnum)));
> > +	}
> > +
> > +	dev_dbg(mtu->dev, "csr0:%#x, csr1:%#x, csr2:%#x\n", csr0, csr1, csr2);
> > +	dev_dbg(mtu->dev, "%s: %s, fifo-addr:%#x, fifo-size:%#x(%#x/%#x)\n",
> > +		__func__, mep->name, mep->fifo_addr, mep->fifo_size,
> > +		fifo_sgsz, mep->fifo_seg_size);
> > +
> > +	return 0;
> > +}
> > +
> 
> [..]
> > +static void mtu3_set_speed(struct mtu3 *mtu)
> > +{
> > +	void __iomem *mbase = mtu->mac_base;
> > +
> > +	if (!mtu->is_u3_ip && (mtu->max_speed > USB_SPEED_HIGH))
> > +		mtu->max_speed = USB_SPEED_HIGH;
> 
> You are missing the checks for USB_SPEED_WIRELESS in general.
> Can you just ignore it?
Doesn't support USB_SPEED_WIRELESS, so ignore it.
> 
> > +
> > +	if (mtu->max_speed == USB_SPEED_FULL) {
> > +		/* disable U3 SS function */
> > +		mtu3_clrbits(mbase, U3D_USB3_CONFIG, USB3_EN);
> > +		/* disable HS function */
> > +		mtu3_clrbits(mbase, U3D_POWER_MANAGEMENT, HS_ENABLE);
> > +	} else if (mtu->max_speed == USB_SPEED_HIGH) {
> > +		mtu3_clrbits(mbase, U3D_USB3_CONFIG, USB3_EN);
> > +		/* HS/FS detected by HW */
> > +		mtu3_setbits(mbase, U3D_POWER_MANAGEMENT, HS_ENABLE);
> > +	}
> > +	dev_info(mtu->dev, "max_speed: %s\n",
> > +		usb_speed_string(mtu->max_speed));
> > +}
> 
> [..]
> > +static irqreturn_t mtu3_link_isr(struct mtu3 *mtu)
> > +{
> > +	void __iomem *mbase = mtu->mac_base;
> > +	enum usb_device_speed udev_speed;
> > +	u32 maxpkt = 64;
> > +	u32 link;
> > +	u32 speed;
> > +
> > +	link = mtu3_readl(mbase, U3D_DEV_LINK_INTR);
> > +	link &= mtu3_readl(mbase, U3D_DEV_LINK_INTR_ENABLE);
> > +	mtu3_writel(mbase, U3D_DEV_LINK_INTR, link); /* W1C */
> > +	dev_dbg(mtu->dev, "=== LINK[%x] ===\n", link);
> > +
> > +	if (!(link & SSUSB_DEV_SPEED_CHG_INTR))
> > +		return IRQ_NONE;
> 
> Shouldn't you do this check before you clear interrupts?
In fact, U3D_DEV_LINK_INTR has only one interrupt,
SSUSB_DEV_SPEED_CHG_INTR, others can be treated as spurious ones.

> 
> > +	speed = SSUSB_DEV_SPEED(mtu3_readl(mbase, U3D_DEVICE_CONF));
> 
> Do you really want to read this out of the hardware on every interrupt?
Yes, the interrupt is triggered only when speed is changed,
and get the new speed here.

> 
> > +
> > +	switch (speed) {
> > +	case MTU3_SPEED_FULL:
> > +		udev_speed = USB_SPEED_FULL;
> > +		/*BESLCK = 4 < BESLCK_U3 = 10 < BESLDCK = 15 */
> > +		mtu3_writel(mbase, U3D_USB20_LPM_PARAMETER, LPM_BESLDCK(0xf)
> > +				| LPM_BESLCK(4) | LPM_BESLCK_U3(0xa));
> > +		mtu3_setbits(mbase, U3D_POWER_MANAGEMENT,
> > +				LPM_BESL_STALL | LPM_BESLD_STALL);
> > +		break;
> > +	case MTU3_SPEED_HIGH:
> > +		udev_speed = USB_SPEED_HIGH;
> > +		/*BESLCK = 4 < BESLCK_U3 = 10 < BESLDCK = 15 */
> > +		mtu3_writel(mbase, U3D_USB20_LPM_PARAMETER, LPM_BESLDCK(0xf)
> > +				| LPM_BESLCK(4) | LPM_BESLCK_U3(0xa));
> > +		mtu3_setbits(mbase, U3D_POWER_MANAGEMENT,
> > +				LPM_BESL_STALL | LPM_BESLD_STALL);
> > +		break;
> > +	case MTU3_SPEED_SUPER:
> > +		udev_speed = USB_SPEED_SUPER;
> > +		maxpkt = 512;
> > +		break;
> > +	default:
> > +		udev_speed = USB_SPEED_UNKNOWN;
> > +		break;
> > +	}
> > +	dev_dbg(mtu->dev, "%s: %s\n", __func__, usb_speed_string(udev_speed));
> > +
> > +	mtu->g.speed = udev_speed;
> > +	mtu->g.ep0->maxpacket = maxpkt;
> > +	mtu->ep0_state = MU3D_EP0_STATE_SETUP;
> > +
> > +	if (udev_speed == USB_SPEED_UNKNOWN)
> > +		mtu3_gadget_disconnect(mtu);
> > +	else
> > +		mtu3_ep0_setup(mtu);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t mtu3_u3_ltssm_isr(struct mtu3 *mtu)
> > +{
> > +	void __iomem *mbase = mtu->mac_base;
> > +	u32 ltssm;
> > +
> > +	ltssm = mtu3_readl(mbase, U3D_LTSSM_INTR);
> > +	ltssm &= mtu3_readl(mbase, U3D_LTSSM_INTR_ENABLE);
> > +	mtu3_writel(mbase, U3D_LTSSM_INTR, ltssm); /* W1C */
> > +	dev_dbg(mtu->dev, "=== LTSSM[%x] ===\n", ltssm);
> > +
> > +	if (ltssm & HOT_RST_INTR)
> > +		mtu3_gadget_reset(mtu);
> > +
> > +	if (ltssm & WARM_RST_INTR)
> > +		mtu3_gadget_reset(mtu);
> 
> You really would reset the gadget twice if that happens?
> And do the rest of the tests make sense in that case?
I will merge them into one as following:

   if (ltssm & (HOT_RST_INTR | WARM_RST_INTR))
        ...

> 
> > +
> > +	if (ltssm & VBUS_FALL_INTR)
> > +		mtu3_ss_func_set(mtu, false);
> > +
> > +	if (ltssm & VBUS_RISE_INTR)
> > +		mtu3_ss_func_set(mtu, true);
> > +
> > +	if (ltssm & EXIT_U3_INTR)
> > +		mtu3_gadget_resume(mtu);
> > +
> > +	if (ltssm & ENTER_U3_INTR)
> > +		mtu3_gadget_suspend(mtu);
> > +
> > +	return IRQ_HANDLED;
> > +}
> 
> [..]
> 
> > +static int mtu3_hw_init(struct mtu3 *mtu)
> > +{
> > +	int ret;
> > +
> > +	mtu3_device_reset(mtu);
> > +
> > +	ret = mtu3_device_enable(mtu);
> > +	if (ret) {
> > +		dev_err(mtu->dev, "device enable failed %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = mtu3_mem_alloc(mtu);
> > +	if (ret) {
> > +		dev_err(mtu->dev, "mem alloc failed, aborting\n");
> 
> 1. You can't leave the device enabled in that case.
It enable device mac and port, otherwise I can't initialize other
register.
and the function is not enable until udc_start() is called.

> 2. No need for a message. The kernel will already do that if kmalloc()
> fails under such circumstances.
remove it latter.
> 
> > +		return -ENOMEM;
> > +	}
> > +
> > +	mtu3_regs_init(mtu);
> > +
> > +	return 0;
> > +}
> 
> > diff --git a/drivers/usb/mtu3/mtu3_dr.c b/drivers/usb/mtu3/mtu3_dr.c
> > new file mode 100644
> > index 0000000..f560f93
> > --- /dev/null
> > +++ b/drivers/usb/mtu3/mtu3_dr.c
> > @@ -0,0 +1,375 @@
> > +/*
> > + * mtu3_dr.c - dual role switch and host glue layer
> > + *
> > + * Copyright (C) 2016 MediaTek Inc.
> > + *
> > + * Author: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * 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.
> > + *
> > + */
> > +
> > +#include <linux/debugfs.h>
> > +#include <linux/irq.h>
> > +#include <linux/kernel.h>
> > +#include <linux/of_device.h>
> > +#include <linux/pinctrl/consumer.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/uaccess.h>
> > +
> > +#include "mtu3.h"
> > +#include "mtu3_dr.h"
> > +
> > +#define USB2_PORT 2
> > +#define USB3_PORT 3
> > +
[...]
> > +
> > +static ssize_t ssusb_mode_write(struct file *file,
> > +	const char __user *ubuf, size_t count, loff_t *ppos)
> > +{
> > +	struct seq_file *sf = file->private_data;
> > +	struct ssusb_mtk *ssusb = sf->private;
> > +	char buf[16];
> > +
> > +	if (copy_from_user(&buf, ubuf, min_t(size_t, sizeof(buf) - 1, count)))
> > +		return -EFAULT;
> > +
> > +	if (!strncmp(buf, "host", 4) && !ssusb->is_host)
> > +		ssusb_mode_manual_switch(ssusb, 1);
> > +	else if (!strncmp(buf, "device", 6) && ssusb->is_host)
> > +		ssusb_mode_manual_switch(ssusb, 0);
> > +	else
> > +		dev_err(ssusb->dev, "wrong or duplicated setting\n");
> 
> No proper error returns

add it later.

> > +
> > +	return count;
> > +}
> > +
> > +static const struct file_operations ssusb_mode_fops = {
> > +	.open = ssusb_mode_open,
> > +	.write = ssusb_mode_write,
> > +	.read = seq_read,
> > +	.llseek = seq_lseek,
> > +	.release = single_release,
> > +};
> > +
[..]
> > +static void ssusb_debugfs_exit(struct ssusb_mtk *ssusb)
> > +{
> > +	debugfs_remove_recursive(ssusb->dbgfs_root);
> > +}
> > +
> > +int ssusb_otg_switch_init(struct ssusb_mtk *ssusb)
> > +{
> > +	struct otg_switch_mtk *otg_sx = &ssusb->otg_switch;
> > +
> > +	INIT_DELAYED_WORK(&otg_sx->extcon_reg_dwork, extcon_register_dwork);
> > +
> > +	if (otg_sx->manual_drd_enabled)
> > +		ssusb_debugfs_init(ssusb);
> > +
> > +	/* It is enough to delay 1s for waiting for host initialization */
> > +	schedule_delayed_work(&otg_sx->extcon_reg_dwork, HZ);
> 
> You need to handle this still pending when you are deregistered.
OK.
> 
> > +
> > +	return 0;
> > +}
> > +
> > +void ssusb_otg_switch_exit(struct ssusb_mtk *ssusb)
> > +{
> > +	struct otg_switch_mtk *otg_sx = &ssusb->otg_switch;
> > +
> > +	if (otg_sx->edev) {
> > +		extcon_unregister_notifier(otg_sx->edev,
> > +			EXTCON_USB, &otg_sx->vbus_nb);
> > +		extcon_unregister_notifier(otg_sx->edev,
> > +			EXTCON_USB_HOST, &otg_sx->id_nb);
> > +	}
> > +
> > +	if (otg_sx->manual_drd_enabled)
> > +		ssusb_debugfs_exit(ssusb);
> > +}
> 
> Could you break this up a bit? It is extensively long a patch?
I afraid I can't break the patch into small ones but also provide
complete functionality at the same time.
Sorry for inconvenience.
> 
Thank you very much :-)

> 	Regards
> 		Oliver
> 

[RESEND PATCH, v5 3/5] usb: xhci-mtk: make IPPC register optional

[Felipe Balbi]

Chunfeng Yun <chunfeng.yun@mediatek.com> writes:
> Make IPPC register optional to support host side of dual-role mode,
> due to it is moved into common glue layer for simplification.
>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@mediatek.com>

Mathias? Are you taking these patches?? I don't wanna take peripheral
side driver if you won't take xhci side.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160829/18a8eb6b/attachment.sig>

[RESEND PATCH, v5 4/5] usb: Add MediaTek USB3 DRD Driver

[Greg Kroah-Hartman]

On Fri, Aug 26, 2016 at 05:38:27PM +0800, chunfeng yun wrote:
> Hi,
> 
> On Thu, 2016-08-25 at 10:32 +0200, Oliver Neukum wrote:
> > On Thu, 2016-08-25 at 11:05 +0800, Chunfeng Yun wrote:
> > > This patch adds support for the MediaTek USB3 controller
> > > integrated into MT8173. It can be configured as Dual-Role
> > > Device (DRD), Peripheral Only and Host Only (xHCI) modes.
> > > 
> > 
> > > +/**
> > > + * General Purpose Descriptor (GPD):
> > > + *	The format of TX GPD is a little different from RX one.
> > > + *	And the size of GPD is 16 bytes.
> > > + *
> > > + * @flag:
> > > + *	bit0: Hardware Own (HWO)
> > > + *	bit1: Buffer Descriptor Present (BDP), always 0, BD is not supported
> > > + *	bit2: Bypass (BPS), 1: HW skips this GPD if HWO = 1
> > > + *	bit7: Interrupt On Completion (IOC)
> > > + * @chksum: This is used to validate the contents of this GPD;
> > > + *	If TXQ_CS_EN / RXQ_CS_EN bit is set, an interrupt is issued
> > > + *	when checksum validation fails;
> > > + *	Checksum value is calculated over the 16 bytes of the GPD by default;
> > > + * @data_buf_len (RX ONLY): This value indicates the length of
> > > + *	the assigned data buffer
> > > + * @next_gpd: Physical address of the next GPD
> > > + * @buffer: Physical address of the data buffer
> > > + * @buf_len:
> > > + *	(TX): This value indicates the length of the assigned data buffer
> > > + *	(RX): The total length of data received
> > > + * @ext_len: reserved
> > > + * @ext_flag:
> > > + *	bit5 (TX ONLY): Zero Length Packet (ZLP),
> > > + */
> > > +struct qmu_gpd {
> > > +	u8 flag;
> > > +	u8 chksum;
> > > +	u16 data_buf_len;
> > > +	u32 next_gpd;
> > > +	u32 buffer;
> > > +	u16 buf_len;
> > > +	u8 ext_len;
> > > +	u8 ext_flag;
> > > +} __packed;
> > 
> > It looks like this is shared with hardware in memory.
> > But you leave the endianness of the bigger fields undeclared.
> > 
> The driver only supports Little-Endian SoCs currently, because I have no
> Big-Endian platform to test it.

that's ok, you still have to mark the endian of the data and use it in
that manner, you can't just not worry about it.

Please fix up.

thanks,

greg k-h

[RESEND PATCH, v5 4/5] usb: Add MediaTek USB3 DRD Driver

[Chunfeng Yun]

On Tue, 2016-08-30 at 19:20 +0200, Greg Kroah-Hartman wrote:
> On Fri, Aug 26, 2016 at 05:38:27PM +0800, chunfeng yun wrote:
> > Hi,
> > 
> > On Thu, 2016-08-25 at 10:32 +0200, Oliver Neukum wrote:
> > > On Thu, 2016-08-25 at 11:05 +0800, Chunfeng Yun wrote:
> > > > This patch adds support for the MediaTek USB3 controller
> > > > integrated into MT8173. It can be configured as Dual-Role
> > > > Device (DRD), Peripheral Only and Host Only (xHCI) modes.
> > > > 
> > > 
> > > > +/**
> > > > + * General Purpose Descriptor (GPD):
> > > > + *	The format of TX GPD is a little different from RX one.
> > > > + *	And the size of GPD is 16 bytes.
> > > > + *
> > > > + * @flag:
> > > > + *	bit0: Hardware Own (HWO)
> > > > + *	bit1: Buffer Descriptor Present (BDP), always 0, BD is not supported
> > > > + *	bit2: Bypass (BPS), 1: HW skips this GPD if HWO = 1
> > > > + *	bit7: Interrupt On Completion (IOC)
> > > > + * @chksum: This is used to validate the contents of this GPD;
> > > > + *	If TXQ_CS_EN / RXQ_CS_EN bit is set, an interrupt is issued
> > > > + *	when checksum validation fails;
> > > > + *	Checksum value is calculated over the 16 bytes of the GPD by default;
> > > > + * @data_buf_len (RX ONLY): This value indicates the length of
> > > > + *	the assigned data buffer
> > > > + * @next_gpd: Physical address of the next GPD
> > > > + * @buffer: Physical address of the data buffer
> > > > + * @buf_len:
> > > > + *	(TX): This value indicates the length of the assigned data buffer
> > > > + *	(RX): The total length of data received
> > > > + * @ext_len: reserved
> > > > + * @ext_flag:
> > > > + *	bit5 (TX ONLY): Zero Length Packet (ZLP),
> > > > + */
> > > > +struct qmu_gpd {
> > > > +	u8 flag;
> > > > +	u8 chksum;
> > > > +	u16 data_buf_len;
> > > > +	u32 next_gpd;
> > > > +	u32 buffer;
> > > > +	u16 buf_len;
> > > > +	u8 ext_len;
> > > > +	u8 ext_flag;
> > > > +} __packed;
> > > 
> > > It looks like this is shared with hardware in memory.
> > > But you leave the endianness of the bigger fields undeclared.
> > > 
> > The driver only supports Little-Endian SoCs currently, because I have no
> > Big-Endian platform to test it.
> 
> that's ok, you still have to mark the endian of the data and use it in
> that manner, you can't just not worry about it.
> 
> Please fix up.
Ok, I will do it

thank you
> 
> thanks,
> 
> greg k-h