linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/9] Prereqs for HiKey960 USB support
@ 2019-10-28 21:59 John Stultz
  2019-10-28 21:59 ` [PATCH v4 1/9] dt-bindings: usb: rt1711h: Add connector bindings John Stultz
                   ` (8 more replies)
  0 siblings, 9 replies; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

Just another round here trying to push forward a patch series
submitted previously by Yu Chen to get HiKey960 dev-board's USB
functionality working.

This series focuses on just the dwc3 changes and bindings
needed.

The current full patchset can be found here:
  https://git.linaro.org/people/john.stultz/android-dev.git/log/?id=ce013895404bd4a5b6487df0ac30ed2dbea43f5d

NOTE: I unfortunately don't have any deep knowledge of the
hardware other then the previously submitted code  and what I
can intuit from testing, but I tried to document the previously
undocumented bindings as best I could, fixed up a few minor
checkpatch issues and tried to address previous feedback as best
I could.

I'd greatly appreciate any feedback or thoughts!

thanks
-john

New in v4:
* Just added a few Reviewed-by: lines from Rob Herring


Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org

John Stultz (6):
  dt-bindings: usb: rt1711h: Add connector bindings
  dt-bindings: usb: dwc3: Allow clock list & resets to be more flexible
  usb: dwc3: Rework clock initialization to be more flexible
  usb: dwc3: Rework resets initialization to be more flexible
  dt-bindings: usb: generic: Add role-switch-default-host binding
  usb: dwc3: Add host-mode as default support

Yu Chen (3):
  usb: dwc3: Execute GCTL Core Soft Reset while switch modes
  usb: dwc3: Increase timeout for CmdAct cleared by device controller
  usb: dwc3: Registering a role switch in the DRD code.

 .../devicetree/bindings/usb/dwc3.txt          |  5 +-
 .../devicetree/bindings/usb/generic.txt       |  5 ++
 .../bindings/usb/richtek,rt1711h.txt          | 29 +++++++
 drivers/usb/dwc3/Kconfig                      |  1 +
 drivers/usb/dwc3/core.c                       | 38 +++++----
 drivers/usb/dwc3/core.h                       |  6 ++
 drivers/usb/dwc3/drd.c                        | 78 ++++++++++++++++++-
 drivers/usb/dwc3/gadget.c                     |  2 +-
 8 files changed, 144 insertions(+), 20 deletions(-)

-- 
2.17.1


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

* [PATCH v4 1/9] dt-bindings: usb: rt1711h: Add connector bindings
  2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
@ 2019-10-28 21:59 ` John Stultz
  2019-10-28 21:59 ` [PATCH v4 2/9] usb: dwc3: Execute GCTL Core Soft Reset while switch modes John Stultz
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, linux-usb, devicetree

Add connector binding documentation for Richtek RT1711H Type-C
chip driver

It was noted by Rob Herring that the rt1711h binding docs
doesn't include the connector binding.

Thus this patch adds such documentation following the details
in Documentation/devicetree/bindings/usb/typec-tcpci.txt

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 .../bindings/usb/richtek,rt1711h.txt          | 29 +++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
index d4cf53c071d9..e3fc57e605ed 100644
--- a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
+++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
@@ -6,10 +6,39 @@ Required properties:
  - interrupts : <a b> where a is the interrupt number and b represents an
    encoding of the sense and level information for the interrupt.
 
+Required sub-node:
+- connector: The "usb-c-connector" attached to the tcpci chip, the bindings
+  of connector node are specified in
+  Documentation/devicetree/bindings/connector/usb-connector.txt
+
 Example :
 rt1711h@4e {
 	compatible = "richtek,rt1711h";
 	reg = <0x4e>;
 	interrupt-parent = <&gpio26>;
 	interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
+
+	usb_con: connector {
+		compatible = "usb-c-connector";
+		label = "USB-C";
+		data-role = "dual";
+		power-role = "dual";
+		try-power-role = "sink";
+		source-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)>;
+		sink-pdos = <PDO_FIXED(5000, 2000, PDO_FIXED_USB_COMM)
+			     PDO_VAR(5000, 12000, 2000)>;
+		op-sink-microwatt = <10000000>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@1 {
+				reg = <1>;
+				usb_con_ss: endpoint {
+					remote-endpoint = <&usb3_data_ss>;
+				};
+			};
+		};
+	};
 };
-- 
2.17.1


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

* [PATCH v4 2/9] usb: dwc3: Execute GCTL Core Soft Reset while switch modes
  2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
  2019-10-28 21:59 ` [PATCH v4 1/9] dt-bindings: usb: rt1711h: Add connector bindings John Stultz
@ 2019-10-28 21:59 ` John Stultz
  2019-10-29  9:09   ` Felipe Balbi
  2019-10-28 21:59 ` [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller John Stultz
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree,
	John Stultz

From: Yu Chen <chenyu56@huawei.com>

On the HiKey960, we need to do a GCTL soft reset when
switching modes.

Jack Pham also noted that in the Synopsys databook it
mentions performing a GCTL CoreSoftReset when changing the
PrtCapDir between device & host modes.

So this patch always does a GCTL Core Soft Reset when
changing the mode.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v3: Remove quirk conditional, as Jack Pham noted the
    Synopsis databook states this should be done generally.
    Also, at Jacks' suggestion, make the reset call before
    changing the prtcap direction.
---
 drivers/usb/dwc3/core.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 999ce5e84d3c..a039e35ec7ad 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
 	dwc->current_dr_role = mode;
 }
 
+static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
+{
+	u32 reg;
+
+	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+	reg |= DWC3_GCTL_CORESOFTRESET;
+	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+
+	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
+	reg &= ~DWC3_GCTL_CORESOFTRESET;
+	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
+}
+
 static void __dwc3_set_mode(struct work_struct *work)
 {
 	struct dwc3 *dwc = work_to_dwc(work);
@@ -154,6 +167,9 @@ static void __dwc3_set_mode(struct work_struct *work)
 
 	spin_lock_irqsave(&dwc->lock, flags);
 
+	/* Execute a GCTL Core Soft Reset when switch mode */
+	dwc3_gctl_core_soft_reset(dwc);
+
 	dwc3_set_prtcap(dwc, dwc->desired_dr_role);
 
 	spin_unlock_irqrestore(&dwc->lock, flags);
-- 
2.17.1


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

* [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
  2019-10-28 21:59 ` [PATCH v4 1/9] dt-bindings: usb: rt1711h: Add connector bindings John Stultz
  2019-10-28 21:59 ` [PATCH v4 2/9] usb: dwc3: Execute GCTL Core Soft Reset while switch modes John Stultz
@ 2019-10-28 21:59 ` John Stultz
  2019-10-29  9:11   ` Felipe Balbi
  2019-10-28 21:59 ` [PATCH v4 4/9] dt-bindings: usb: dwc3: Allow clock list & resets to be more flexible John Stultz
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree,
	John Stultz

From: Yu Chen <chenyu56@huawei.com>

It needs more time for the device controller to clear the CmdAct of
DEPCMD on Hisilicon Kirin Soc.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/usb/dwc3/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 86dc1db788a9..168eb4a0a9b0 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -270,7 +270,7 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
 {
 	const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
 	struct dwc3		*dwc = dep->dwc;
-	u32			timeout = 1000;
+	u32			timeout = 5000;
 	u32			saved_config = 0;
 	u32			reg;
 
-- 
2.17.1


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

* [PATCH v4 4/9] dt-bindings: usb: dwc3: Allow clock list & resets to be more flexible
  2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
                   ` (2 preceding siblings ...)
  2019-10-28 21:59 ` [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller John Stultz
@ 2019-10-28 21:59 ` John Stultz
  2019-10-28 21:59 ` [PATCH v4 5/9] usb: dwc3: Rework clock initialization " John Stultz
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

Rather then adding another device specific binding to support
hikey960, Rob Herring suggested we expand the current dwc3
binding to allow for variable numbers of clocks and resets.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Suggested-by: Rob Herring <robh@kernel.org>
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 Documentation/devicetree/bindings/usb/dwc3.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 66780a47ad85..29768b0ca923 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -7,7 +7,8 @@ Required properties:
  - compatible: must be "snps,dwc3"
  - reg : Address and length of the register set for the device
  - interrupts: Interrupts used by the dwc3 controller.
- - clock-names: should contain "ref", "bus_early", "suspend"
+ - clock-names: list of clock names. Ideally should be "ref",
+                "bus_early", "suspend" but may be less or more.
  - clocks: list of phandle and clock specifier pairs corresponding to
            entries in the clock-names property.
 
@@ -36,7 +37,7 @@ Optional properties:
  - phys: from the *Generic PHY* bindings
  - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy"
 	or "usb3-phy".
- - resets: a single pair of phandle and reset specifier
+ - resets: set of phandle and reset specifier pairs
  - snps,usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
  - snps,usb3_lpm_capable: determines if platform is USB3 LPM capable
  - snps,dis-start-transfer-quirk: when set, disable isoc START TRANSFER command
-- 
2.17.1


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

* [PATCH v4 5/9] usb: dwc3: Rework clock initialization to be more flexible
  2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
                   ` (3 preceding siblings ...)
  2019-10-28 21:59 ` [PATCH v4 4/9] dt-bindings: usb: dwc3: Allow clock list & resets to be more flexible John Stultz
@ 2019-10-28 21:59 ` John Stultz
  2019-10-29  9:13   ` Felipe Balbi
  2019-10-28 21:59 ` [PATCH v4 6/9] usb: dwc3: Rework resets " John Stultz
                   ` (3 subsequent siblings)
  8 siblings, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

The dwc3 core binding specifies three clocks:
  ref, bus_early, and suspend

which are all controlled in the driver together.

However some variants of the hardware my not have all three clks

So this patch reworks the reading of the clks from the dts to
use devm_clk_bulk_get_all() will will fetch all the clocks
specified in the dts together.

This patch was reccomended by Rob Herring <robh@kernel.org>
as an alternative to creating multiple bindings for each variant
of hardware when the only unique bits were clocks and resets.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v3: Rework dwc3 core rather then adding another dwc-of-simple
    binding.
---
 drivers/usb/dwc3/core.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a039e35ec7ad..4d4f1836b62c 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -305,12 +305,6 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
 	return 0;
 }
 
-static const struct clk_bulk_data dwc3_core_clks[] = {
-	{ .id = "ref" },
-	{ .id = "bus_early" },
-	{ .id = "suspend" },
-};
-
 /*
  * dwc3_frame_length_adjustment - Adjusts frame length if required
  * @dwc3: Pointer to our controller context structure
@@ -1418,11 +1412,6 @@ static int dwc3_probe(struct platform_device *pdev)
 	if (!dwc)
 		return -ENOMEM;
 
-	dwc->clks = devm_kmemdup(dev, dwc3_core_clks, sizeof(dwc3_core_clks),
-				 GFP_KERNEL);
-	if (!dwc->clks)
-		return -ENOMEM;
-
 	dwc->dev = dev;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1458,17 +1447,18 @@ static int dwc3_probe(struct platform_device *pdev)
 		return PTR_ERR(dwc->reset);
 
 	if (dev->of_node) {
-		dwc->num_clks = ARRAY_SIZE(dwc3_core_clks);
-
-		ret = devm_clk_bulk_get(dev, dwc->num_clks, dwc->clks);
+		ret = devm_clk_bulk_get_all(dev, &dwc->clks);
 		if (ret == -EPROBE_DEFER)
 			return ret;
 		/*
 		 * Clocks are optional, but new DT platforms should support all
 		 * clocks as required by the DT-binding.
 		 */
-		if (ret)
+		if (ret < 0)
 			dwc->num_clks = 0;
+		else
+			dwc->num_clks = ret;
+
 	}
 
 	ret = reset_control_deassert(dwc->reset);
-- 
2.17.1


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

* [PATCH v4 6/9] usb: dwc3: Rework resets initialization to be more flexible
  2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
                   ` (4 preceding siblings ...)
  2019-10-28 21:59 ` [PATCH v4 5/9] usb: dwc3: Rework clock initialization " John Stultz
@ 2019-10-28 21:59 ` John Stultz
  2019-10-29  9:17   ` Felipe Balbi
  2019-10-28 21:59 ` [PATCH v4 7/9] usb: dwc3: Registering a role switch in the DRD code John Stultz
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

The dwc3 core binding specifies one reset.

However some variants of the hardware my not have more.

So this patch reworks the reading of the resets to fetch all the
resets specified in the dts together.

This patch was reccomended by Rob Herring <robh@kernel.org>
as an alternative to creating multiple bindings for each variant
of hardware when the only unique bits were clocks and resets.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Suggested-by: Rob Herring <robh@kernel.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v3: Rework dwc3 core rather then adding another dwc-of-simple
    binding.
---
 drivers/usb/dwc3/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 4d4f1836b62c..ef52ffa5d6cb 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1442,7 +1442,7 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	dwc3_get_properties(dwc);
 
-	dwc->reset = devm_reset_control_get_optional_shared(dev, NULL);
+	dwc->reset = devm_reset_control_array_get(dev, true, true);
 	if (IS_ERR(dwc->reset))
 		return PTR_ERR(dwc->reset);
 
-- 
2.17.1


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

* [PATCH v4 7/9] usb: dwc3: Registering a role switch in the DRD code.
  2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
                   ` (5 preceding siblings ...)
  2019-10-28 21:59 ` [PATCH v4 6/9] usb: dwc3: Rework resets " John Stultz
@ 2019-10-28 21:59 ` John Stultz
  2019-10-29  9:21   ` Felipe Balbi
  2019-10-28 21:59 ` [PATCH v4 8/9] dt-bindings: usb: generic: Add role-switch-default-host binding John Stultz
  2019-10-28 21:59 ` [PATCH v4 9/9] usb: dwc3: Add host-mode as default support John Stultz
  8 siblings, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree,
	John Stultz

From: Yu Chen <chenyu56@huawei.com>

The Type-C drivers use USB role switch API to inform the
system about the negotiated data role, so registering a role
switch in the DRD code in order to support platforms with
USB Type-C connectors.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Signed-off-by: Yu Chen <chenyu56@huawei.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v2: Fix role_sw and role_switch_default_mode descriptions as
    reported by kbuild test robot <lkp@intel.com>

v3: Split out the role-switch-default-host logic into its own
    patch
---
 drivers/usb/dwc3/Kconfig |  1 +
 drivers/usb/dwc3/core.h  |  3 ++
 drivers/usb/dwc3/drd.c   | 66 +++++++++++++++++++++++++++++++++++++++-
 3 files changed, 69 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 89abc6078703..1104745c41a9 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -44,6 +44,7 @@ config USB_DWC3_DUAL_ROLE
 	bool "Dual Role mode"
 	depends on ((USB=y || USB=USB_DWC3) && (USB_GADGET=y || USB_GADGET=USB_DWC3))
 	depends on (EXTCON=y || EXTCON=USB_DWC3)
+	select USB_ROLE_SWITCH
 	help
 	  This is the default mode of working of DWC3 controller where
 	  both host and gadget features are enabled.
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 1c8b349379af..6f19e9891767 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -25,6 +25,7 @@
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
 #include <linux/usb/otg.h>
+#include <linux/usb/role.h>
 #include <linux/ulpi/interface.h>
 
 #include <linux/phy/phy.h>
@@ -951,6 +952,7 @@ struct dwc3_scratchpad_array {
  * @hsphy_mode: UTMI phy mode, one of following:
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
+ * @role_sw: usb_role_switch handle
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY
@@ -1084,6 +1086,7 @@ struct dwc3 {
 	struct extcon_dev	*edev;
 	struct notifier_block	edev_nb;
 	enum usb_phy_interface	hsphy_mode;
+	struct usb_role_switch	*role_sw;
 
 	u32			fladj;
 	u32			irq_gadget;
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index c946d64142ad..61d4fd8aead4 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -476,6 +476,52 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
 	return edev;
 }
 
+static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role)
+{
+	struct dwc3 *dwc = dev_get_drvdata(dev);
+	u32 mode;
+
+	switch (role) {
+	case USB_ROLE_HOST:
+		mode = DWC3_GCTL_PRTCAP_HOST;
+		break;
+	case USB_ROLE_DEVICE:
+		mode = DWC3_GCTL_PRTCAP_DEVICE;
+		break;
+	default:
+		mode = DWC3_GCTL_PRTCAP_DEVICE;
+		break;
+	}
+
+	dwc3_set_mode(dwc, mode);
+	return 0;
+}
+
+static enum usb_role dwc3_usb_role_switch_get(struct device *dev)
+{
+	struct dwc3 *dwc = dev_get_drvdata(dev);
+	unsigned long flags;
+	enum usb_role role;
+
+	spin_lock_irqsave(&dwc->lock, flags);
+	switch (dwc->current_dr_role) {
+	case DWC3_GCTL_PRTCAP_HOST:
+		role = USB_ROLE_HOST;
+		break;
+	case DWC3_GCTL_PRTCAP_DEVICE:
+		role = USB_ROLE_DEVICE;
+		break;
+	case DWC3_GCTL_PRTCAP_OTG:
+		role = dwc->current_otg_role;
+		break;
+	default:
+		role = USB_ROLE_DEVICE;
+		break;
+	}
+	spin_unlock_irqrestore(&dwc->lock, flags);
+	return role;
+}
+
 int dwc3_drd_init(struct dwc3 *dwc)
 {
 	int ret, irq;
@@ -484,7 +530,22 @@ int dwc3_drd_init(struct dwc3 *dwc)
 	if (IS_ERR(dwc->edev))
 		return PTR_ERR(dwc->edev);
 
-	if (dwc->edev) {
+	if (device_property_read_bool(dwc->dev, "usb-role-switch")) {
+		struct usb_role_switch_desc dwc3_role_switch = {NULL};
+		u32 mode;
+
+		mode = DWC3_GCTL_PRTCAP_DEVICE;
+
+		dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
+		dwc3_role_switch.set = dwc3_usb_role_switch_set;
+		dwc3_role_switch.get = dwc3_usb_role_switch_get;
+		dwc->role_sw = usb_role_switch_register(dwc->dev,
+							&dwc3_role_switch);
+		if (IS_ERR(dwc->role_sw))
+			return PTR_ERR(dwc->role_sw);
+
+		dwc3_set_mode(dwc, mode);
+	} else if (dwc->edev) {
 		dwc->edev_nb.notifier_call = dwc3_drd_notifier;
 		ret = extcon_register_notifier(dwc->edev, EXTCON_USB_HOST,
 					       &dwc->edev_nb);
@@ -531,6 +592,9 @@ void dwc3_drd_exit(struct dwc3 *dwc)
 {
 	unsigned long flags;
 
+	if (dwc->role_sw)
+		usb_role_switch_unregister(dwc->role_sw);
+
 	if (dwc->edev)
 		extcon_unregister_notifier(dwc->edev, EXTCON_USB_HOST,
 					   &dwc->edev_nb);
-- 
2.17.1


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

* [PATCH v4 8/9] dt-bindings: usb: generic: Add role-switch-default-host binding
  2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
                   ` (6 preceding siblings ...)
  2019-10-28 21:59 ` [PATCH v4 7/9] usb: dwc3: Registering a role switch in the DRD code John Stultz
@ 2019-10-28 21:59 ` John Stultz
  2019-10-29  9:23   ` Felipe Balbi
  2019-10-28 21:59 ` [PATCH v4 9/9] usb: dwc3: Add host-mode as default support John Stultz
  8 siblings, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

Add binding to configure the default role the controller
assumes is host mode when the usb role is USB_ROLE_NONE.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 Documentation/devicetree/bindings/usb/generic.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/generic.txt b/Documentation/devicetree/bindings/usb/generic.txt
index cf5a1ad456e6..013782fde293 100644
--- a/Documentation/devicetree/bindings/usb/generic.txt
+++ b/Documentation/devicetree/bindings/usb/generic.txt
@@ -34,6 +34,11 @@ Optional properties:
 			the USB data role (USB host or USB device) for a given
 			USB connector, such as Type-C, Type-B(micro).
 			see connector/usb-connector.txt.
+ - role-switch-default-host: boolean, indicating if usb-role-switch is enabled
+			the device default operation mode of controller while
+			usb role is USB_ROLE_NONE is host mode. If this is not
+			set or false, it will be assumed the default is device
+			mode.
 
 This is an attribute to a USB controller such as:
 
-- 
2.17.1


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

* [PATCH v4 9/9] usb: dwc3: Add host-mode as default support
  2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
                   ` (7 preceding siblings ...)
  2019-10-28 21:59 ` [PATCH v4 8/9] dt-bindings: usb: generic: Add role-switch-default-host binding John Stultz
@ 2019-10-28 21:59 ` John Stultz
  2019-10-29  9:25   ` Felipe Balbi
  8 siblings, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-28 21:59 UTC (permalink / raw)
  To: lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Felipe Balbi, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

Support configuring the default role the controller assumes as
host mode when the usb role is USB_ROLE_NONE

This patch was split out from a larger patch originally by
Yu Chen <chenyu56@huawei.com>

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Mark Rutland <mark.rutland@arm.com>
CC: ShuFan Lee <shufan_lee@richtek.com>
Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
Cc: Yu Chen <chenyu56@huawei.com>
Cc: Felipe Balbi <balbi@kernel.org>
Cc: Hans de Goede <hdegoede@redhat.com>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Jun Li <lijun.kernel@gmail.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>
Cc: Jack Pham <jackp@codeaurora.org>
Cc: linux-usb@vger.kernel.org
Cc: devicetree@vger.kernel.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
v3: Split this patch out from addition of usb-role-switch
    handling
---
 drivers/usb/dwc3/core.h |  3 +++
 drivers/usb/dwc3/drd.c  | 20 ++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 6f19e9891767..3c879c9ab1aa 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -953,6 +953,8 @@ struct dwc3_scratchpad_array {
  *		- USBPHY_INTERFACE_MODE_UTMI
  *		- USBPHY_INTERFACE_MODE_UTMIW
  * @role_sw: usb_role_switch handle
+ * @role_switch_default_mode: default operation mode of controller while
+ *			usb role is USB_ROLE_NONE.
  * @usb2_phy: pointer to USB2 PHY
  * @usb3_phy: pointer to USB3 PHY
  * @usb2_generic_phy: pointer to USB2 PHY
@@ -1087,6 +1089,7 @@ struct dwc3 {
 	struct notifier_block	edev_nb;
 	enum usb_phy_interface	hsphy_mode;
 	struct usb_role_switch	*role_sw;
+	enum usb_dr_mode	role_switch_default_mode;
 
 	u32			fladj;
 	u32			irq_gadget;
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 61d4fd8aead4..0e3466fe5ac4 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -489,7 +489,10 @@ static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role)
 		mode = DWC3_GCTL_PRTCAP_DEVICE;
 		break;
 	default:
-		mode = DWC3_GCTL_PRTCAP_DEVICE;
+		if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
+			mode = DWC3_GCTL_PRTCAP_HOST;
+		else
+			mode = DWC3_GCTL_PRTCAP_DEVICE;
 		break;
 	}
 
@@ -515,7 +518,10 @@ static enum usb_role dwc3_usb_role_switch_get(struct device *dev)
 		role = dwc->current_otg_role;
 		break;
 	default:
-		role = USB_ROLE_DEVICE;
+		if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
+			role = USB_ROLE_HOST;
+		else
+			role = USB_ROLE_DEVICE;
 		break;
 	}
 	spin_unlock_irqrestore(&dwc->lock, flags);
@@ -534,8 +540,14 @@ int dwc3_drd_init(struct dwc3 *dwc)
 		struct usb_role_switch_desc dwc3_role_switch = {NULL};
 		u32 mode;
 
-		mode = DWC3_GCTL_PRTCAP_DEVICE;
-
+		if (device_property_read_bool(dwc->dev,
+					      "role-switch-default-host")) {
+			dwc->role_switch_default_mode = USB_DR_MODE_HOST;
+			mode = DWC3_GCTL_PRTCAP_HOST;
+		} else {
+			dwc->role_switch_default_mode = USB_DR_MODE_PERIPHERAL;
+			mode = DWC3_GCTL_PRTCAP_DEVICE;
+		}
 		dwc3_role_switch.fwnode = dev_fwnode(dwc->dev);
 		dwc3_role_switch.set = dwc3_usb_role_switch_set;
 		dwc3_role_switch.get = dwc3_usb_role_switch_get;
-- 
2.17.1


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

* Re: [PATCH v4 2/9] usb: dwc3: Execute GCTL Core Soft Reset while switch modes
  2019-10-28 21:59 ` [PATCH v4 2/9] usb: dwc3: Execute GCTL Core Soft Reset while switch modes John Stultz
@ 2019-10-29  9:09   ` Felipe Balbi
  2019-10-29 21:21     ` John Stultz
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2019-10-29  9:09 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, linux-usb, devicetree, John Stultz

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


Hi,

John Stultz <john.stultz@linaro.org> writes:
> From: Yu Chen <chenyu56@huawei.com>
>
> On the HiKey960, we need to do a GCTL soft reset when
> switching modes.
>
> Jack Pham also noted that in the Synopsys databook it
> mentions performing a GCTL CoreSoftReset when changing the
> PrtCapDir between device & host modes.
>
> So this patch always does a GCTL Core Soft Reset when
> changing the mode.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> CC: ShuFan Lee <shufan_lee@richtek.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Yu Chen <chenyu56@huawei.com>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Jun Li <lijun.kernel@gmail.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Jack Pham <jackp@codeaurora.org>
> Cc: linux-usb@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v3: Remove quirk conditional, as Jack Pham noted the
>     Synopsis databook states this should be done generally.
>     Also, at Jacks' suggestion, make the reset call before
>     changing the prtcap direction.
> ---
>  drivers/usb/dwc3/core.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 999ce5e84d3c..a039e35ec7ad 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>  	dwc->current_dr_role = mode;
>  }
>  
> +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
> +{
> +	u32 reg;
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg |= DWC3_GCTL_CORESOFTRESET;
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +
> +	reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> +	reg &= ~DWC3_GCTL_CORESOFTRESET;
> +	dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> +}
> +
>  static void __dwc3_set_mode(struct work_struct *work)
>  {
>  	struct dwc3 *dwc = work_to_dwc(work);
> @@ -154,6 +167,9 @@ static void __dwc3_set_mode(struct work_struct *work)
>  
>  	spin_lock_irqsave(&dwc->lock, flags);
>  
> +	/* Execute a GCTL Core Soft Reset when switch mode */
> +	dwc3_gctl_core_soft_reset(dwc);
> +

This is totally unnecessary. We have several platforms supporting dual
role *without* this trick. The only reason why the databook mentions a
reset is because some registers are shadowed, meaning that they share
the same physical space and just appear as different things for SW. The
reason being that Synopsys wanted to reduce the area of the IP and
decided to shadow registers which are mutually exclusive.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2019-10-28 21:59 ` [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller John Stultz
@ 2019-10-29  9:11   ` Felipe Balbi
  2019-10-29 21:17     ` John Stultz
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2019-10-29  9:11 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, linux-usb, devicetree, John Stultz

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


Hi,

John Stultz <john.stultz@linaro.org> writes:
> From: Yu Chen <chenyu56@huawei.com>
>
> It needs more time for the device controller to clear the CmdAct of
> DEPCMD on Hisilicon Kirin Soc.

Why does it need more time? Why is it so that no other platform needs
more time, only this one? And which command, specifically, causes
problem?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 5/9] usb: dwc3: Rework clock initialization to be more flexible
  2019-10-28 21:59 ` [PATCH v4 5/9] usb: dwc3: Rework clock initialization " John Stultz
@ 2019-10-29  9:13   ` Felipe Balbi
  2019-10-29 16:08     ` John Stultz
  2019-11-07 21:53     ` Rob Herring
  0 siblings, 2 replies; 53+ messages in thread
From: Felipe Balbi @ 2019-10-29  9:13 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

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


Hi,

John Stultz <john.stultz@linaro.org> writes:

> The dwc3 core binding specifies three clocks:
>   ref, bus_early, and suspend
>
> which are all controlled in the driver together.
>
> However some variants of the hardware my not have all three clks
                                        ^^
                                        may

In fact *all* platforms have all three clocks. It's just that in some
cases clock pins are shorted together (or take input from same clock).

> So this patch reworks the reading of the clks from the dts to
> use devm_clk_bulk_get_all() will will fetch all the clocks
                              ^^^^
                              which?

> specified in the dts together.
>
> This patch was reccomended by Rob Herring <robh@kernel.org>
> as an alternative to creating multiple bindings for each variant
> of hardware when the only unique bits were clocks and resets.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> CC: ShuFan Lee <shufan_lee@richtek.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Yu Chen <chenyu56@huawei.com>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Jun Li <lijun.kernel@gmail.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Jack Pham <jackp@codeaurora.org>
> Cc: linux-usb@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Suggested-by: Rob Herring <robh@kernel.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v3: Rework dwc3 core rather then adding another dwc-of-simple
>     binding.
> ---
>  drivers/usb/dwc3/core.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index a039e35ec7ad..4d4f1836b62c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -305,12 +305,6 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>  	return 0;
>  }
>  
> -static const struct clk_bulk_data dwc3_core_clks[] = {
> -	{ .id = "ref" },
> -	{ .id = "bus_early" },
> -	{ .id = "suspend" },
> -};

another option would be to pass three clocks with the same phandle. That
would even make sure that clock usage counts are correct, no?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 6/9] usb: dwc3: Rework resets initialization to be more flexible
  2019-10-28 21:59 ` [PATCH v4 6/9] usb: dwc3: Rework resets " John Stultz
@ 2019-10-29  9:17   ` Felipe Balbi
  2019-10-29 18:05     ` John Stultz
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2019-10-29  9:17 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

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


Hi,

John Stultz <john.stultz@linaro.org> writes:
> The dwc3 core binding specifies one reset.
>
> However some variants of the hardware my not have more.
                                        ^^
                                        may

According to synopsys databook, there's a single *input* reset signal on
this IP. What is this extra reset you have?

Is this, perhaps, specific to your glue layer around the synopsys ip?
Should, perhaps, your extra reset be managed by the glue layer?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 7/9] usb: dwc3: Registering a role switch in the DRD code.
  2019-10-28 21:59 ` [PATCH v4 7/9] usb: dwc3: Registering a role switch in the DRD code John Stultz
@ 2019-10-29  9:21   ` Felipe Balbi
  2019-11-07 23:20     ` John Stultz
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2019-10-29  9:21 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, linux-usb, devicetree, John Stultz

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


Hi,

John Stultz <john.stultz@linaro.org> writes:
> From: Yu Chen <chenyu56@huawei.com>
>
> The Type-C drivers use USB role switch API to inform the
> system about the negotiated data role, so registering a role
> switch in the DRD code in order to support platforms with
> USB Type-C connectors.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> CC: ShuFan Lee <shufan_lee@richtek.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Yu Chen <chenyu56@huawei.com>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Jun Li <lijun.kernel@gmail.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Jack Pham <jackp@codeaurora.org>
> Cc: linux-usb@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Signed-off-by: Yu Chen <chenyu56@huawei.com>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v2: Fix role_sw and role_switch_default_mode descriptions as
>     reported by kbuild test robot <lkp@intel.com>
>
> v3: Split out the role-switch-default-host logic into its own
>     patch
> ---
>  drivers/usb/dwc3/Kconfig |  1 +
>  drivers/usb/dwc3/core.h  |  3 ++
>  drivers/usb/dwc3/drd.c   | 66 +++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 69 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 89abc6078703..1104745c41a9 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -44,6 +44,7 @@ config USB_DWC3_DUAL_ROLE
>  	bool "Dual Role mode"
>  	depends on ((USB=y || USB=USB_DWC3) && (USB_GADGET=y || USB_GADGET=USB_DWC3))
>  	depends on (EXTCON=y || EXTCON=USB_DWC3)
> +	select USB_ROLE_SWITCH

so even those using DWC3 as a peripheral-only or host-only driver will
need role switch?

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 1c8b349379af..6f19e9891767 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -25,6 +25,7 @@
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
>  #include <linux/usb/otg.h>
> +#include <linux/usb/role.h>
>  #include <linux/ulpi/interface.h>
>  
>  #include <linux/phy/phy.h>
> @@ -951,6 +952,7 @@ struct dwc3_scratchpad_array {
>   * @hsphy_mode: UTMI phy mode, one of following:
>   *		- USBPHY_INTERFACE_MODE_UTMI
>   *		- USBPHY_INTERFACE_MODE_UTMIW
> + * @role_sw: usb_role_switch handle
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
>   * @usb2_generic_phy: pointer to USB2 PHY
> @@ -1084,6 +1086,7 @@ struct dwc3 {
>  	struct extcon_dev	*edev;
>  	struct notifier_block	edev_nb;
>  	enum usb_phy_interface	hsphy_mode;
> +	struct usb_role_switch	*role_sw;
>  
>  	u32			fladj;
>  	u32			irq_gadget;
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index c946d64142ad..61d4fd8aead4 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -476,6 +476,52 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc)
>  	return edev;
>  }
>  
> +static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role)
> +{
> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> +	u32 mode;
> +
> +	switch (role) {
> +	case USB_ROLE_HOST:
> +		mode = DWC3_GCTL_PRTCAP_HOST;
> +		break;
> +	case USB_ROLE_DEVICE:
> +		mode = DWC3_GCTL_PRTCAP_DEVICE;
> +		break;
> +	default:
> +		mode = DWC3_GCTL_PRTCAP_DEVICE;
> +		break;
> +	}
> +
> +	dwc3_set_mode(dwc, mode);
> +	return 0;
> +}

role switching is starting to get way too complicated in DWC3. We now
have a function that queues a work on the system_freezable_wq that will
configure PHY and change PRTCAP. Is there a way we can simplify some of
this a little?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 8/9] dt-bindings: usb: generic: Add role-switch-default-host binding
  2019-10-28 21:59 ` [PATCH v4 8/9] dt-bindings: usb: generic: Add role-switch-default-host binding John Stultz
@ 2019-10-29  9:23   ` Felipe Balbi
  2019-10-29 18:26     ` John Stultz
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2019-10-29  9:23 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

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


Hi,

John Stultz <john.stultz@linaro.org> writes:

> Add binding to configure the default role the controller
> assumes is host mode when the usb role is USB_ROLE_NONE.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> CC: ShuFan Lee <shufan_lee@richtek.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Yu Chen <chenyu56@huawei.com>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Jun Li <lijun.kernel@gmail.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Jack Pham <jackp@codeaurora.org>
> Cc: linux-usb@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Reviewed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  Documentation/devicetree/bindings/usb/generic.txt | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/generic.txt b/Documentation/devicetree/bindings/usb/generic.txt
> index cf5a1ad456e6..013782fde293 100644
> --- a/Documentation/devicetree/bindings/usb/generic.txt
> +++ b/Documentation/devicetree/bindings/usb/generic.txt
> @@ -34,6 +34,11 @@ Optional properties:
>  			the USB data role (USB host or USB device) for a given
>  			USB connector, such as Type-C, Type-B(micro).
>  			see connector/usb-connector.txt.
> + - role-switch-default-host: boolean, indicating if usb-role-switch is enabled
> +			the device default operation mode of controller while
> +			usb role is USB_ROLE_NONE is host mode. If this is not
> +			set or false, it will be assumed the default is device
> +			mode.

Do we also need a role-switch-default-peripheral? Would it be better to
have a single role-switch-default property which accepts "host" or
"peripheral" arguments?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 9/9] usb: dwc3: Add host-mode as default support
  2019-10-28 21:59 ` [PATCH v4 9/9] usb: dwc3: Add host-mode as default support John Stultz
@ 2019-10-29  9:25   ` Felipe Balbi
  2019-11-07 22:23     ` John Stultz
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2019-10-29  9:25 UTC (permalink / raw)
  To: John Stultz, lkml
  Cc: John Stultz, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Yu Chen, Hans de Goede, Andy Shevchenko, Jun Li,
	Valentin Schneider, Jack Pham, linux-usb, devicetree

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


Hi,

John Stultz <john.stultz@linaro.org> writes:
> Support configuring the default role the controller assumes as
> host mode when the usb role is USB_ROLE_NONE
>
> This patch was split out from a larger patch originally by
> Yu Chen <chenyu56@huawei.com>
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> CC: ShuFan Lee <shufan_lee@richtek.com>
> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> Cc: Yu Chen <chenyu56@huawei.com>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Jun Li <lijun.kernel@gmail.com>
> Cc: Valentin Schneider <valentin.schneider@arm.com>
> Cc: Jack Pham <jackp@codeaurora.org>
> Cc: linux-usb@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
> v3: Split this patch out from addition of usb-role-switch
>     handling
> ---
>  drivers/usb/dwc3/core.h |  3 +++
>  drivers/usb/dwc3/drd.c  | 20 ++++++++++++++++----
>  2 files changed, 19 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 6f19e9891767..3c879c9ab1aa 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -953,6 +953,8 @@ struct dwc3_scratchpad_array {
>   *		- USBPHY_INTERFACE_MODE_UTMI
>   *		- USBPHY_INTERFACE_MODE_UTMIW
>   * @role_sw: usb_role_switch handle
> + * @role_switch_default_mode: default operation mode of controller while
> + *			usb role is USB_ROLE_NONE.
>   * @usb2_phy: pointer to USB2 PHY
>   * @usb3_phy: pointer to USB3 PHY
>   * @usb2_generic_phy: pointer to USB2 PHY
> @@ -1087,6 +1089,7 @@ struct dwc3 {
>  	struct notifier_block	edev_nb;
>  	enum usb_phy_interface	hsphy_mode;
>  	struct usb_role_switch	*role_sw;
> +	enum usb_dr_mode	role_switch_default_mode;
>  
>  	u32			fladj;
>  	u32			irq_gadget;
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index 61d4fd8aead4..0e3466fe5ac4 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -489,7 +489,10 @@ static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role)
>  		mode = DWC3_GCTL_PRTCAP_DEVICE;
>  		break;
>  	default:
> -		mode = DWC3_GCTL_PRTCAP_DEVICE;
> +		if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
> +			mode = DWC3_GCTL_PRTCAP_HOST;
> +		else
> +			mode = DWC3_GCTL_PRTCAP_DEVICE;
>  		break;
>  	}
>  
> @@ -515,7 +518,10 @@ static enum usb_role dwc3_usb_role_switch_get(struct device *dev)
>  		role = dwc->current_otg_role;
>  		break;
>  	default:
> -		role = USB_ROLE_DEVICE;
> +		if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
> +			role = USB_ROLE_HOST;

look at this, we now have 3 different encodings for role which DWC3
needs to understand. One is its own PRTCAP_DIR, then there USB_DR_MODE_*
and now USB_ROLE_*, can we make it so that we only have one private
encoding and one generic encoding?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 5/9] usb: dwc3: Rework clock initialization to be more flexible
  2019-10-29  9:13   ` Felipe Balbi
@ 2019-10-29 16:08     ` John Stultz
  2019-10-30  9:02       ` Felipe Balbi
  2019-11-07 21:53     ` Rob Herring
  1 sibling, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-29 16:08 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: lkml, Greg Kroah-Hartman, Rob Herring, Mark Rutland, ShuFan Lee,
	Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun, Yu Chen,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Tue, Oct 29, 2019 at 2:14 AM Felipe Balbi <balbi@kernel.org> wrote:
> John Stultz <john.stultz@linaro.org> writes:
>
> > The dwc3 core binding specifies three clocks:
> >   ref, bus_early, and suspend
> >
> > which are all controlled in the driver together.
> >
> > However some variants of the hardware my not have all three clks
>                                         ^^
>                                         may
>
> In fact *all* platforms have all three clocks. It's just that in some
> cases clock pins are shorted together (or take input from same clock).
>
...
> another option would be to pass three clocks with the same phandle. That
> would even make sure that clock usage counts are correct, no?

Hey Felipe!

So I actually had done that initially (and it seemed to work), but Rob
suggested this way instead.
I'm fine with either, as long as having multiple references to the
same clk in the enable/disable paths doesn't cause trouble.

Thanks so much for the review here!
-john

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

* Re: [PATCH v4 6/9] usb: dwc3: Rework resets initialization to be more flexible
  2019-10-29  9:17   ` Felipe Balbi
@ 2019-10-29 18:05     ` John Stultz
  2019-10-30  9:01       ` Felipe Balbi
  0 siblings, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-29 18:05 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: lkml, Greg Kroah-Hartman, Rob Herring, Mark Rutland, ShuFan Lee,
	Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun, Yu Chen,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Tue, Oct 29, 2019 at 2:17 AM Felipe Balbi <balbi@kernel.org> wrote:
> John Stultz <john.stultz@linaro.org> writes:
> > The dwc3 core binding specifies one reset.
> >
> > However some variants of the hardware my not have more.
>                                         ^^
>                                         may
>
> According to synopsys databook, there's a single *input* reset signal on
> this IP. What is this extra reset you have?
>
> Is this, perhaps, specific to your glue layer around the synopsys ip?

Likely (again, I unfortunately don't have a ton of detail on the hardware).

> Should, perhaps, your extra reset be managed by the glue layer?

So yes the dwc3-of-simple does much of this already (it handles
multiple resets, and variable clocks), but unfortunately we seem to
need new bindings for each device added?  I think the suggestion from
Rob was due to the sprawl of bindings for the glue code, and the extra
complexity of the parent node.  So I believe Rob just thought it made
sense to collapse this down into the core?

I'm not really passionate about either approach, and am happy to
rework (as long as there is eventual progress :).
Just let me know what you'd prefer.

thanks
-john

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

* Re: [PATCH v4 8/9] dt-bindings: usb: generic: Add role-switch-default-host binding
  2019-10-29  9:23   ` Felipe Balbi
@ 2019-10-29 18:26     ` John Stultz
  0 siblings, 0 replies; 53+ messages in thread
From: John Stultz @ 2019-10-29 18:26 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: lkml, Greg Kroah-Hartman, Rob Herring, Mark Rutland, ShuFan Lee,
	Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun, Yu Chen,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Tue, Oct 29, 2019 at 2:23 AM Felipe Balbi <balbi@kernel.org> wrote:
> John Stultz <john.stultz@linaro.org> writes:
>
> > Add binding to configure the default role the controller
> > assumes is host mode when the usb role is USB_ROLE_NONE.
> >
...
> > + - role-switch-default-host: boolean, indicating if usb-role-switch is enabled
> > +                     the device default operation mode of controller while
> > +                     usb role is USB_ROLE_NONE is host mode. If this is not
> > +                     set or false, it will be assumed the default is device
> > +                     mode.
>
> Do we also need a role-switch-default-peripheral? Would it be better to
> have a single role-switch-default property which accepts "host" or
> "peripheral" arguments?

I guess the standard default is peripheral, so this differentiated
from that, but I agree it might be more forward thinking to let it
specify a type argument in case there is another option in the future.

I'll rework this.

Thanks again for the review and feedback!
-john

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2019-10-29  9:11   ` Felipe Balbi
@ 2019-10-29 21:17     ` John Stultz
  2020-05-06  9:00       ` Jun Li
  0 siblings, 1 reply; 53+ messages in thread
From: John Stultz @ 2019-10-29 21:17 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote:
> John Stultz <john.stultz@linaro.org> writes:
> > From: Yu Chen <chenyu56@huawei.com>
> >
> > It needs more time for the device controller to clear the CmdAct of
> > DEPCMD on Hisilicon Kirin Soc.
>
> Why does it need more time? Why is it so that no other platform needs
> more time, only this one? And which command, specifically, causes
> problem?

Hrm. Sadly I don't have that context (again I'm picking up a
semi-abandoned patchset here), which is unfortunate, as I'm sure
someone spent a number of hours debugging things to come up with this.
:)

But alas, I've dropped this for now in my stack, and things seem to be
working ok so far. I suspect there's some edge case I'll run into, but
hopefully I'll be able to debug and get more details when that
happens.

I do appreciate the review and pushback here!

thanks
-john

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

* Re: [PATCH v4 2/9] usb: dwc3: Execute GCTL Core Soft Reset while switch modes
  2019-10-29  9:09   ` Felipe Balbi
@ 2019-10-29 21:21     ` John Stultz
  0 siblings, 0 replies; 53+ messages in thread
From: John Stultz @ 2019-10-29 21:21 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Tue, Oct 29, 2019 at 2:09 AM Felipe Balbi <balbi@kernel.org> wrote:
> John Stultz <john.stultz@linaro.org> writes:
> > From: Yu Chen <chenyu56@huawei.com>
> >
> > On the HiKey960, we need to do a GCTL soft reset when
> > switching modes.
> >
> > Jack Pham also noted that in the Synopsys databook it
> > mentions performing a GCTL CoreSoftReset when changing the
> > PrtCapDir between device & host modes.
> >
> > So this patch always does a GCTL Core Soft Reset when
> > changing the mode.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > CC: ShuFan Lee <shufan_lee@richtek.com>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > Cc: Yu Chen <chenyu56@huawei.com>
> > Cc: Felipe Balbi <balbi@kernel.org>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Cc: Jun Li <lijun.kernel@gmail.com>
> > Cc: Valentin Schneider <valentin.schneider@arm.com>
> > Cc: Jack Pham <jackp@codeaurora.org>
> > Cc: linux-usb@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Signed-off-by: Yu Chen <chenyu56@huawei.com>
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> > v3: Remove quirk conditional, as Jack Pham noted the
> >     Synopsis databook states this should be done generally.
> >     Also, at Jacks' suggestion, make the reset call before
> >     changing the prtcap direction.
> > ---
> >  drivers/usb/dwc3/core.c | 16 ++++++++++++++++
> >  1 file changed, 16 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 999ce5e84d3c..a039e35ec7ad 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
> >       dwc->current_dr_role = mode;
> >  }
> >
> > +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc)
> > +{
> > +     u32 reg;
> > +
> > +     reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> > +     reg |= DWC3_GCTL_CORESOFTRESET;
> > +     dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> > +
> > +     reg = dwc3_readl(dwc->regs, DWC3_GCTL);
> > +     reg &= ~DWC3_GCTL_CORESOFTRESET;
> > +     dwc3_writel(dwc->regs, DWC3_GCTL, reg);
> > +}
> > +
> >  static void __dwc3_set_mode(struct work_struct *work)
> >  {
> >       struct dwc3 *dwc = work_to_dwc(work);
> > @@ -154,6 +167,9 @@ static void __dwc3_set_mode(struct work_struct *work)
> >
> >       spin_lock_irqsave(&dwc->lock, flags);
> >
> > +     /* Execute a GCTL Core Soft Reset when switch mode */
> > +     dwc3_gctl_core_soft_reset(dwc);
> > +
>
> This is totally unnecessary. We have several platforms supporting dual
> role *without* this trick. The only reason why the databook mentions a
> reset is because some registers are shadowed, meaning that they share
> the same physical space and just appear as different things for SW. The
> reason being that Synopsys wanted to reduce the area of the IP and
> decided to shadow registers which are mutually exclusive.

Ok. I've dropped this for now. Without this I do see an occasional
issues seemingly more frequently where he board seems to initialize
improperly on boot (usb-c is connected, but it doesn't seem to detect
until I unplug and replug), but it also trips (though seemingly less
frequently) without this, so this may be just affecting the timing of
a initialization race issue. I'll watch this for more info and follow
up on it later.

Thanks for the review!
-john

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

* Re: [PATCH v4 6/9] usb: dwc3: Rework resets initialization to be more flexible
  2019-10-29 18:05     ` John Stultz
@ 2019-10-30  9:01       ` Felipe Balbi
  2019-11-07 21:45         ` Rob Herring
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2019-10-30  9:01 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Greg Kroah-Hartman, Rob Herring, Mark Rutland, ShuFan Lee,
	Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun, Yu Chen,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS


Hi,

John Stultz <john.stultz@linaro.org> writes:

> On Tue, Oct 29, 2019 at 2:17 AM Felipe Balbi <balbi@kernel.org> wrote:
>> John Stultz <john.stultz@linaro.org> writes:
>> > The dwc3 core binding specifies one reset.
>> >
>> > However some variants of the hardware my not have more.
>>                                         ^^
>>                                         may
>>
>> According to synopsys databook, there's a single *input* reset signal on
>> this IP. What is this extra reset you have?
>>
>> Is this, perhaps, specific to your glue layer around the synopsys ip?
>
> Likely (again, I unfortunately don't have a ton of detail on the hardware).
>
>> Should, perhaps, your extra reset be managed by the glue layer?
>
> So yes the dwc3-of-simple does much of this already (it handles
> multiple resets, and variable clocks), but unfortunately we seem to
> need new bindings for each device added?  I think the suggestion from
> Rob was due to the sprawl of bindings for the glue code, and the extra
> complexity of the parent node.  So I believe Rob just thought it made
> sense to collapse this down into the core?
>
> I'm not really passionate about either approach, and am happy to
> rework (as long as there is eventual progress :).
> Just let me know what you'd prefer.

Well, I was under the impression we were supposed to describe the
HW. Synopsys IP has a single reset input :-p

-- 
balbi

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

* Re: [PATCH v4 5/9] usb: dwc3: Rework clock initialization to be more flexible
  2019-10-29 16:08     ` John Stultz
@ 2019-10-30  9:02       ` Felipe Balbi
  0 siblings, 0 replies; 53+ messages in thread
From: Felipe Balbi @ 2019-10-30  9:02 UTC (permalink / raw)
  To: John Stultz
  Cc: lkml, Greg Kroah-Hartman, Rob Herring, Mark Rutland, ShuFan Lee,
	Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun, Yu Chen,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS


Hi,

John Stultz <john.stultz@linaro.org> writes:
> On Tue, Oct 29, 2019 at 2:14 AM Felipe Balbi <balbi@kernel.org> wrote:
>> John Stultz <john.stultz@linaro.org> writes:
>>
>> > The dwc3 core binding specifies three clocks:
>> >   ref, bus_early, and suspend
>> >
>> > which are all controlled in the driver together.
>> >
>> > However some variants of the hardware my not have all three clks
>>                                         ^^
>>                                         may
>>
>> In fact *all* platforms have all three clocks. It's just that in some
>> cases clock pins are shorted together (or take input from same clock).
>>
> ...
>> another option would be to pass three clocks with the same phandle. That
>> would even make sure that clock usage counts are correct, no?
>
> Hey Felipe!
>
> So I actually had done that initially (and it seemed to work), but Rob
> suggested this way instead.
> I'm fine with either, as long as having multiple references to the
> same clk in the enable/disable paths doesn't cause trouble.
>
> Thanks so much for the review here!

same as the other patch, if we're supposed to describe the HW, then we
should describe what's actually happening.

-- 
balbi

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

* Re: [PATCH v4 6/9] usb: dwc3: Rework resets initialization to be more flexible
  2019-10-30  9:01       ` Felipe Balbi
@ 2019-11-07 21:45         ` Rob Herring
  0 siblings, 0 replies; 53+ messages in thread
From: Rob Herring @ 2019-11-07 21:45 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: John Stultz, lkml, Greg Kroah-Hartman, Mark Rutland, ShuFan Lee,
	Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun, Yu Chen,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, Oct 30, 2019 at 4:01 AM Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> John Stultz <john.stultz@linaro.org> writes:
>
> > On Tue, Oct 29, 2019 at 2:17 AM Felipe Balbi <balbi@kernel.org> wrote:
> >> John Stultz <john.stultz@linaro.org> writes:
> >> > The dwc3 core binding specifies one reset.
> >> >
> >> > However some variants of the hardware my not have more.
> >>                                         ^^
> >>                                         may
> >>
> >> According to synopsys databook, there's a single *input* reset signal on
> >> this IP. What is this extra reset you have?
> >>
> >> Is this, perhaps, specific to your glue layer around the synopsys ip?
> >
> > Likely (again, I unfortunately don't have a ton of detail on the hardware).
> >
> >> Should, perhaps, your extra reset be managed by the glue layer?

An extra clock or reset is a silly reason to have a whole other node
and driver. If there's additional blocks and registers, then yes a
glue node makes sense.

> > So yes the dwc3-of-simple does much of this already (it handles
> > multiple resets, and variable clocks), but unfortunately we seem to
> > need new bindings for each device added?  I think the suggestion from
> > Rob was due to the sprawl of bindings for the glue code, and the extra
> > complexity of the parent node.  So I believe Rob just thought it made
> > sense to collapse this down into the core?
> >
> > I'm not really passionate about either approach, and am happy to
> > rework (as long as there is eventual progress :).
> > Just let me know what you'd prefer.
>
> Well, I was under the impression we were supposed to describe the
> HW. Synopsys IP has a single reset input :-p

John is. His chip requires 2 resets to use the USB block and the
compatible provides that distinction. Maybe HiSilicon has a newer or
customized IP version that has 2 resets. The block could have external
RAMs (because every process has its own) which may have their own
reset. With NDA specifications and little knowledge of the full
revision history, we can really never know. Also, omitting clocks and
resets from the dwc3 node entirely is just as much not describing the
h/w (only the glue needs clocks?).

This block is the oddball. I think there's 1 or 2 other blocks where
this glue node was done, but please stop. If we did this every time
there's a variation in clocks or resets, we'd pretty much have glue
nodes everywhere.

Rob

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

* Re: [PATCH v4 5/9] usb: dwc3: Rework clock initialization to be more flexible
  2019-10-29  9:13   ` Felipe Balbi
  2019-10-29 16:08     ` John Stultz
@ 2019-11-07 21:53     ` Rob Herring
  1 sibling, 0 replies; 53+ messages in thread
From: Rob Herring @ 2019-11-07 21:53 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: John Stultz, lkml, Greg Kroah-Hartman, Mark Rutland, ShuFan Lee,
	Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun, Yu Chen,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List, devicetree

On Tue, Oct 29, 2019 at 4:14 AM Felipe Balbi <balbi@kernel.org> wrote:
>
>
> Hi,
>
> John Stultz <john.stultz@linaro.org> writes:
>
> > The dwc3 core binding specifies three clocks:
> >   ref, bus_early, and suspend
> >
> > which are all controlled in the driver together.
> >
> > However some variants of the hardware my not have all three clks
>                                         ^^
>                                         may
>
> In fact *all* platforms have all three clocks. It's just that in some
> cases clock pins are shorted together (or take input from same clock).
>
> > So this patch reworks the reading of the clks from the dts to
> > use devm_clk_bulk_get_all() will will fetch all the clocks
>                               ^^^^
>                               which?
>
> > specified in the dts together.
> >
> > This patch was reccomended by Rob Herring <robh@kernel.org>
> > as an alternative to creating multiple bindings for each variant
> > of hardware when the only unique bits were clocks and resets.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > CC: ShuFan Lee <shufan_lee@richtek.com>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > Cc: Yu Chen <chenyu56@huawei.com>
> > Cc: Felipe Balbi <balbi@kernel.org>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Cc: Jun Li <lijun.kernel@gmail.com>
> > Cc: Valentin Schneider <valentin.schneider@arm.com>
> > Cc: Jack Pham <jackp@codeaurora.org>
> > Cc: linux-usb@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Suggested-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> > v3: Rework dwc3 core rather then adding another dwc-of-simple
> >     binding.
> > ---
> >  drivers/usb/dwc3/core.c | 20 +++++---------------
> >  1 file changed, 5 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index a039e35ec7ad..4d4f1836b62c 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -305,12 +305,6 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
> >       return 0;
> >  }
> >
> > -static const struct clk_bulk_data dwc3_core_clks[] = {
> > -     { .id = "ref" },
> > -     { .id = "bus_early" },
> > -     { .id = "suspend" },
> > -};
>
> another option would be to pass three clocks with the same phandle. That
> would even make sure that clock usage counts are correct, no?

If you have the datasheet for the block, then perhaps some suggestion
of which clocks code be the same. My guess would be ref and suspend.

Maybe suspend is a fixed clock which is unmanaged on HiSilicon
platforms. If we allow for no clocks on some platforms, then why does
it have to be all or none?

Rob

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

* Re: [PATCH v4 9/9] usb: dwc3: Add host-mode as default support
  2019-10-29  9:25   ` Felipe Balbi
@ 2019-11-07 22:23     ` John Stultz
  0 siblings, 0 replies; 53+ messages in thread
From: John Stultz @ 2019-11-07 22:23 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: lkml, Greg Kroah-Hartman, Rob Herring, Mark Rutland, ShuFan Lee,
	Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun, Yu Chen,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Tue, Oct 29, 2019 at 2:25 AM Felipe Balbi <balbi@kernel.org> wrote:
> John Stultz <john.stultz@linaro.org> writes:
> > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> > index 61d4fd8aead4..0e3466fe5ac4 100644
> > --- a/drivers/usb/dwc3/drd.c
> > +++ b/drivers/usb/dwc3/drd.c
> > @@ -489,7 +489,10 @@ static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role)
> >               mode = DWC3_GCTL_PRTCAP_DEVICE;
> >               break;
> >       default:
> > -             mode = DWC3_GCTL_PRTCAP_DEVICE;
> > +             if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
> > +                     mode = DWC3_GCTL_PRTCAP_HOST;
> > +             else
> > +                     mode = DWC3_GCTL_PRTCAP_DEVICE;
> >               break;
> >       }
> >
> > @@ -515,7 +518,10 @@ static enum usb_role dwc3_usb_role_switch_get(struct device *dev)
> >               role = dwc->current_otg_role;
> >               break;
> >       default:
> > -             role = USB_ROLE_DEVICE;
> > +             if (dwc->role_switch_default_mode == USB_DR_MODE_HOST)
> > +                     role = USB_ROLE_HOST;
>
> look at this, we now have 3 different encodings for role which DWC3
> needs to understand. One is its own PRTCAP_DIR, then there USB_DR_MODE_*
> and now USB_ROLE_*, can we make it so that we only have one private
> encoding and one generic encoding?

And you left out the DWC3_OTG_ROLE_* set too!

So I agree it can be easy to muddle up.  The enums are *almost* equivalent:

include/linux/usb/role.h:
enum usb_role {
        USB_ROLE_NONE,
        USB_ROLE_HOST,
        USB_ROLE_DEVICE,
};

include/linux/usb/otg.h:
enum usb_dr_mode {
        USB_DR_MODE_UNKNOWN,
        USB_DR_MODE_HOST,
        USB_DR_MODE_PERIPHERAL,
        USB_DR_MODE_OTG,
};

But both are widely used:
$ git grep USB_ROLE_ | wc -l
123
$ git grep USB_DR_MODE_ | wc -l
190

So I'm not sure how easy it will be to condense down, since the usage
is coming from different usb subsystems (otg and role switching)  and
I worry assuming them equivalent in just one driver may run into
trouble eventually if the values diverge (ie someone adds
USB_ROLE_BRICK or something).

Heikki/Greg: Any thoughts on this? Does it make sense to try to drop
the usb_role enum and users and replace it with usb_dr_mode?

thanks
-john

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

* Re: [PATCH v4 7/9] usb: dwc3: Registering a role switch in the DRD code.
  2019-10-29  9:21   ` Felipe Balbi
@ 2019-11-07 23:20     ` John Stultz
  0 siblings, 0 replies; 53+ messages in thread
From: John Stultz @ 2019-11-07 23:20 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Hans de Goede, Andy Shevchenko, Jun Li, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Tue, Oct 29, 2019 at 2:21 AM Felipe Balbi <balbi@kernel.org> wrote:
> John Stultz <john.stultz@linaro.org> writes:
> > From: Yu Chen <chenyu56@huawei.com>
> >
> > The Type-C drivers use USB role switch API to inform the
> > system about the negotiated data role, so registering a role
> > switch in the DRD code in order to support platforms with
> > USB Type-C connectors.
> >
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > CC: ShuFan Lee <shufan_lee@richtek.com>
> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com>
> > Cc: Yu Chen <chenyu56@huawei.com>
> > Cc: Felipe Balbi <balbi@kernel.org>
> > Cc: Hans de Goede <hdegoede@redhat.com>
> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> > Cc: Jun Li <lijun.kernel@gmail.com>
> > Cc: Valentin Schneider <valentin.schneider@arm.com>
> > Cc: Jack Pham <jackp@codeaurora.org>
> > Cc: linux-usb@vger.kernel.org
> > Cc: devicetree@vger.kernel.org
> > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
> > Signed-off-by: Yu Chen <chenyu56@huawei.com>
> > Signed-off-by: John Stultz <john.stultz@linaro.org>
> > ---
> > v2: Fix role_sw and role_switch_default_mode descriptions as
> >     reported by kbuild test robot <lkp@intel.com>
> >
> > v3: Split out the role-switch-default-host logic into its own
> >     patch
> > ---
> >  drivers/usb/dwc3/Kconfig |  1 +
> >  drivers/usb/dwc3/core.h  |  3 ++
> >  drivers/usb/dwc3/drd.c   | 66 +++++++++++++++++++++++++++++++++++++++-
> >  3 files changed, 69 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> > index 89abc6078703..1104745c41a9 100644
> > --- a/drivers/usb/dwc3/Kconfig
> > +++ b/drivers/usb/dwc3/Kconfig
> > @@ -44,6 +44,7 @@ config USB_DWC3_DUAL_ROLE
> >       bool "Dual Role mode"
> >       depends on ((USB=y || USB=USB_DWC3) && (USB_GADGET=y || USB_GADGET=USB_DWC3))
> >       depends on (EXTCON=y || EXTCON=USB_DWC3)
> > +     select USB_ROLE_SWITCH
>
> so even those using DWC3 as a peripheral-only or host-only driver will
> need role switch?

So, just to clarify, the select is added to the
CONFIG_USB_DWC3_DUAL_ROLE, wouldn't peripheral-only or host-only
drivers select USB_DWC3_GADGET or USB_DWC3_HOST instead?

Even so, if you'd prefer I can avoid the select, and add more #ifdef
CONFIG_USB_ROLE_SWITCH around the logic added in this patch. I just
worry it makes getting a valid config for some devices more complex
and clutters the logic a touch.

> > +static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role)
> > +{
> > +     struct dwc3 *dwc = dev_get_drvdata(dev);
> > +     u32 mode;
> > +
> > +     switch (role) {
> > +     case USB_ROLE_HOST:
> > +             mode = DWC3_GCTL_PRTCAP_HOST;
> > +             break;
> > +     case USB_ROLE_DEVICE:
> > +             mode = DWC3_GCTL_PRTCAP_DEVICE;
> > +             break;
> > +     default:
> > +             mode = DWC3_GCTL_PRTCAP_DEVICE;
> > +             break;
> > +     }
> > +
> > +     dwc3_set_mode(dwc, mode);
> > +     return 0;
> > +}
>
> role switching is starting to get way too complicated in DWC3. We now
> have a function that queues a work on the system_freezable_wq that will
> configure PHY and change PRTCAP. Is there a way we can simplify some of
> this a little?

I'm sorry, could you expand a bit on this point? I'm not sure I quite
see what you are envisioning as a simpler role_switch set handler? Is
the objection that I'm calling dwc3_set_mode() and instead should be
calling some non-static variant of __dwc3_set_mode() directly?

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2019-10-29 21:17     ` John Stultz
@ 2020-05-06  9:00       ` Jun Li
  2020-05-06 22:27         ` John Stultz
  2020-05-08 12:33         ` Felipe Balbi
  0 siblings, 2 replies; 53+ messages in thread
From: Jun Li @ 2020-05-06  9:00 UTC (permalink / raw)
  To: John Stultz
  Cc: Felipe Balbi, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道:
>
> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote:
> > John Stultz <john.stultz@linaro.org> writes:
> > > From: Yu Chen <chenyu56@huawei.com>
> > >
> > > It needs more time for the device controller to clear the CmdAct of
> > > DEPCMD on Hisilicon Kirin Soc.
> >
> > Why does it need more time? Why is it so that no other platform needs
> > more time, only this one? And which command, specifically, causes
> > problem?

Sorry for my back to this so late.

This change is required on my dwc3 based HW too, I gave a check
and the reason is suspend_clk is used in case the PIPE phy is at P3,
this slow clock makes my EP command below timeout.

dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401]
params 00001000 00000500 00000000 --> status: Timed Out

Success case takes about 400us to complete, see below trace(44.286278
- 44.285897 = 0.000381):

configfs_acm.sh-822   [000] d..1    44.285896: dwc3_writel: addr
000000006d59aae1 value 00000401
configfs_acm.sh-822   [000] d..1    44.285897: dwc3_readl: addr
000000006d59aae1 value 00000401
... ...
configfs_acm.sh-822   [000] d..1    44.286278: dwc3_readl: addr
000000006d59aae1 value 00000001
configfs_acm.sh-822   [000] d..1    44.286279: dwc3_gadget_ep_cmd:
ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000
00000500 00000000 --> status: Successful

Hi John,

Do you still have this problem? if yes, What's the value of
USBLNKST[21:18] when the timeout happens?

thanks
Li Jun
>
> Hrm. Sadly I don't have that context (again I'm picking up a
> semi-abandoned patchset here), which is unfortunate, as I'm sure
> someone spent a number of hours debugging things to come up with this.
> :)
>
> But alas, I've dropped this for now in my stack, and things seem to be
> working ok so far. I suspect there's some edge case I'll run into, but
> hopefully I'll be able to debug and get more details when that
> happens.
>
> I do appreciate the review and pushback here!
>
> thanks
> -john

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-06  9:00       ` Jun Li
@ 2020-05-06 22:27         ` John Stultz
  2020-05-07  3:08           ` Jun Li
  2020-05-08 12:33         ` Felipe Balbi
  1 sibling, 1 reply; 53+ messages in thread
From: John Stultz @ 2020-05-06 22:27 UTC (permalink / raw)
  To: Jun Li
  Cc: Felipe Balbi, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote:
> John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道:
> > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote:
> > > John Stultz <john.stultz@linaro.org> writes:
> > > > From: Yu Chen <chenyu56@huawei.com>
> > > >
> > > > It needs more time for the device controller to clear the CmdAct of
> > > > DEPCMD on Hisilicon Kirin Soc.
> > >
> > > Why does it need more time? Why is it so that no other platform needs
> > > more time, only this one? And which command, specifically, causes
> > > problem?
>
> Sorry for my back to this so late.
>
> This change is required on my dwc3 based HW too, I gave a check
> and the reason is suspend_clk is used in case the PIPE phy is at P3,
> this slow clock makes my EP command below timeout.
>
> dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401]
> params 00001000 00000500 00000000 --> status: Timed Out
>
> Success case takes about 400us to complete, see below trace(44.286278
> - 44.285897 = 0.000381):
>
> configfs_acm.sh-822   [000] d..1    44.285896: dwc3_writel: addr
> 000000006d59aae1 value 00000401
> configfs_acm.sh-822   [000] d..1    44.285897: dwc3_readl: addr
> 000000006d59aae1 value 00000401
> ... ...
> configfs_acm.sh-822   [000] d..1    44.286278: dwc3_readl: addr
> 000000006d59aae1 value 00000001
> configfs_acm.sh-822   [000] d..1    44.286279: dwc3_gadget_ep_cmd:
> ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000
> 00000500 00000000 --> status: Successful
>
> Hi John,
>
> Do you still have this problem? if yes, What's the value of
> USBLNKST[21:18] when the timeout happens?

Sorry. As I mentioned, I was working to upstream a patchset that I
hadn't created, so the context I had was limited. As I couldn't
reproduce an issue without the change on the device I had, I figured
it would be best to drop it.

However, as you have some analysis and rational for why such a change
would be needed, I don't have an objection to it. Do you want to
resubmit the patch with your explanation and detailed log above in the
commit message?

thanks
-john

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-06 22:27         ` John Stultz
@ 2020-05-07  3:08           ` Jun Li
  2020-05-08 12:22             ` Jun Li
  0 siblings, 1 reply; 53+ messages in thread
From: Jun Li @ 2020-05-07  3:08 UTC (permalink / raw)
  To: John Stultz
  Cc: Felipe Balbi, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道:
>
> On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote:
> > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道:
> > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote:
> > > > John Stultz <john.stultz@linaro.org> writes:
> > > > > From: Yu Chen <chenyu56@huawei.com>
> > > > >
> > > > > It needs more time for the device controller to clear the CmdAct of
> > > > > DEPCMD on Hisilicon Kirin Soc.
> > > >
> > > > Why does it need more time? Why is it so that no other platform needs
> > > > more time, only this one? And which command, specifically, causes
> > > > problem?
> >
> > Sorry for my back to this so late.
> >
> > This change is required on my dwc3 based HW too, I gave a check
> > and the reason is suspend_clk is used in case the PIPE phy is at P3,
> > this slow clock makes my EP command below timeout.
> >
> > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401]
> > params 00001000 00000500 00000000 --> status: Timed Out
> >
> > Success case takes about 400us to complete, see below trace(44.286278
> > - 44.285897 = 0.000381):
> >
> > configfs_acm.sh-822   [000] d..1    44.285896: dwc3_writel: addr
> > 000000006d59aae1 value 00000401
> > configfs_acm.sh-822   [000] d..1    44.285897: dwc3_readl: addr
> > 000000006d59aae1 value 00000401
> > ... ...
> > configfs_acm.sh-822   [000] d..1    44.286278: dwc3_readl: addr
> > 000000006d59aae1 value 00000001
> > configfs_acm.sh-822   [000] d..1    44.286279: dwc3_gadget_ep_cmd:
> > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000
> > 00000500 00000000 --> status: Successful
> >
> > Hi John,
> >
> > Do you still have this problem? if yes, What's the value of
> > USBLNKST[21:18] when the timeout happens?
>
> Sorry. As I mentioned, I was working to upstream a patchset that I
> hadn't created, so the context I had was limited. As I couldn't
> reproduce an issue without the change on the device I had, I figured
> it would be best to drop it.

That was fine.
>
> However, as you have some analysis and rational for why such a change
> would be needed, I don't have an objection to it. Do you want to
> resubmit the patch with your explanation and detailed log above in the
> commit message?

Sure, I will resubmit the patch with my explanation added in commit message.

thanks
Li Jun
>
> thanks
> -john

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-07  3:08           ` Jun Li
@ 2020-05-08 12:22             ` Jun Li
  2020-05-08 12:35               ` Felipe Balbi
  0 siblings, 1 reply; 53+ messages in thread
From: Jun Li @ 2020-05-08 12:22 UTC (permalink / raw)
  To: John Stultz
  Cc: Felipe Balbi, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Jun Li <lijun.kernel@gmail.com> 于2020年5月7日周四 上午11:08写道:
>
> John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道:
> >
> > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote:
> > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道:
> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote:
> > > > > John Stultz <john.stultz@linaro.org> writes:
> > > > > > From: Yu Chen <chenyu56@huawei.com>
> > > > > >
> > > > > > It needs more time for the device controller to clear the CmdAct of
> > > > > > DEPCMD on Hisilicon Kirin Soc.
> > > > >
> > > > > Why does it need more time? Why is it so that no other platform needs
> > > > > more time, only this one? And which command, specifically, causes
> > > > > problem?
> > >
> > > Sorry for my back to this so late.
> > >
> > > This change is required on my dwc3 based HW too, I gave a check
> > > and the reason is suspend_clk is used in case the PIPE phy is at P3,
> > > this slow clock makes my EP command below timeout.
> > >
> > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401]
> > > params 00001000 00000500 00000000 --> status: Timed Out
> > >
> > > Success case takes about 400us to complete, see below trace(44.286278
> > > - 44.285897 = 0.000381):
> > >
> > > configfs_acm.sh-822   [000] d..1    44.285896: dwc3_writel: addr
> > > 000000006d59aae1 value 00000401
> > > configfs_acm.sh-822   [000] d..1    44.285897: dwc3_readl: addr
> > > 000000006d59aae1 value 00000401
> > > ... ...
> > > configfs_acm.sh-822   [000] d..1    44.286278: dwc3_readl: addr
> > > 000000006d59aae1 value 00000001
> > > configfs_acm.sh-822   [000] d..1    44.286279: dwc3_gadget_ep_cmd:
> > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000
> > > 00000500 00000000 --> status: Successful
> > >
> > > Hi John,
> > >
> > > Do you still have this problem? if yes, What's the value of
> > > USBLNKST[21:18] when the timeout happens?
> >
> > Sorry. As I mentioned, I was working to upstream a patchset that I
> > hadn't created, so the context I had was limited. As I couldn't
> > reproduce an issue without the change on the device I had, I figured
> > it would be best to drop it.
>
> That was fine.
> >
> > However, as you have some analysis and rational for why such a change
> > would be needed, I don't have an objection to it. Do you want to
> > resubmit the patch with your explanation and detailed log above in the
> > commit message?
>
> Sure, I will resubmit the patch with my explanation added in commit message.

Hi John

A second think of this, I feel use readl_poll_timeout_atomic() to wait by time
is more proper here, so I create a new patch to address this also other
registers polling, see below patch with you CCed:

https://patchwork.kernel.org/patch/11536081/

thanks
Li Jun
>
> thanks
> Li Jun
> >
> > thanks
> > -john

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-06  9:00       ` Jun Li
  2020-05-06 22:27         ` John Stultz
@ 2020-05-08 12:33         ` Felipe Balbi
  2020-05-09  8:10           ` Jun Li
  1 sibling, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2020-05-08 12:33 UTC (permalink / raw)
  To: Jun Li, John Stultz
  Cc: lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Hans de Goede, Andy Shevchenko, Valentin Schneider, Jack Pham,
	Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

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


Hi,

Jun Li <lijun.kernel@gmail.com> writes:
> John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道:
>>
>> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote:
>> > John Stultz <john.stultz@linaro.org> writes:
>> > > From: Yu Chen <chenyu56@huawei.com>
>> > >
>> > > It needs more time for the device controller to clear the CmdAct of
>> > > DEPCMD on Hisilicon Kirin Soc.
>> >
>> > Why does it need more time? Why is it so that no other platform needs
>> > more time, only this one? And which command, specifically, causes
>> > problem?
>
> Sorry for my back to this so late.
>
> This change is required on my dwc3 based HW too, I gave a check
> and the reason is suspend_clk is used in case the PIPE phy is at P3,
> this slow clock makes my EP command below timeout.

The phy needs to woken up before the command is triggered. Currently we
only wake up the HS PHY. Does it help you if we wake up the SS phy as
well?

Something like below ought to do it:

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index a0555252dee0..ee46c2dacaeb 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -274,7 +274,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
 	const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
 	struct dwc3		*dwc = dep->dwc;
 	u32			timeout = 1000;
-	u32			saved_config = 0;
+	u32			saved_hs_config = 0;
+	u32			saved_ss_config = 0;
 	u32			reg;
 
 	int			cmd_status = 0;
@@ -293,19 +294,28 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
 	if (dwc->gadget.speed <= USB_SPEED_HIGH) {
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
 		if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
-			saved_config |= DWC3_GUSB2PHYCFG_SUSPHY;
+			saved_hs_config |= DWC3_GUSB2PHYCFG_SUSPHY;
 			reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
 		}
 
 		if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) {
-			saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM;
+			saved_hs_config |= DWC3_GUSB2PHYCFG_ENBLSLPM;
 			reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
 		}
 
-		if (saved_config)
+		if (saved_hs_config)
 			dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
 	}
 
+	reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
+	if (unlikely(reg & DWC3_GUSB3PIPECTL_SUSPHY)) {
+		saved_ss_config |= DWC3_GUSB3PIPECTL_SUSPHY;
+		reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
+	}
+
+	if (saved_ss_config)
+		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
+
 	if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) {
 		int		needs_wakeup;
 
@@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
 			dwc3_gadget_ep_get_transfer_index(dep);
 	}
 
-	if (saved_config) {
+	if (saved_hs_config) {
 		reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
-		reg |= saved_config;
+		reg |= saved_hs_config;
 		dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
 	}
 
+	if (saved_ss_config) {
+		reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
+		reg |= saved_ss_config;
+		dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
+	}
+
 	return ret;
 }
 
-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-08 12:22             ` Jun Li
@ 2020-05-08 12:35               ` Felipe Balbi
  2020-05-09  8:28                 ` Jun Li
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2020-05-08 12:35 UTC (permalink / raw)
  To: Jun Li, John Stultz
  Cc: lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring, Mark Rutland,
	ShuFan Lee, Heikki Krogerus, Suzuki K Poulose, Chunfeng Yun,
	Hans de Goede, Andy Shevchenko, Valentin Schneider, Jack Pham,
	Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

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


Hi,

Jun Li <lijun.kernel@gmail.com> writes:
> Jun Li <lijun.kernel@gmail.com> 于2020年5月7日周四 上午11:08写道:
>>
>> John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道:
>> >
>> > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote:
>> > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道:
>> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote:
>> > > > > John Stultz <john.stultz@linaro.org> writes:
>> > > > > > From: Yu Chen <chenyu56@huawei.com>
>> > > > > >
>> > > > > > It needs more time for the device controller to clear the CmdAct of
>> > > > > > DEPCMD on Hisilicon Kirin Soc.
>> > > > >
>> > > > > Why does it need more time? Why is it so that no other platform needs
>> > > > > more time, only this one? And which command, specifically, causes
>> > > > > problem?
>> > >
>> > > Sorry for my back to this so late.
>> > >
>> > > This change is required on my dwc3 based HW too, I gave a check
>> > > and the reason is suspend_clk is used in case the PIPE phy is at P3,
>> > > this slow clock makes my EP command below timeout.
>> > >
>> > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401]
>> > > params 00001000 00000500 00000000 --> status: Timed Out
>> > >
>> > > Success case takes about 400us to complete, see below trace(44.286278
>> > > - 44.285897 = 0.000381):
>> > >
>> > > configfs_acm.sh-822   [000] d..1    44.285896: dwc3_writel: addr
>> > > 000000006d59aae1 value 00000401
>> > > configfs_acm.sh-822   [000] d..1    44.285897: dwc3_readl: addr
>> > > 000000006d59aae1 value 00000401
>> > > ... ...
>> > > configfs_acm.sh-822   [000] d..1    44.286278: dwc3_readl: addr
>> > > 000000006d59aae1 value 00000001
>> > > configfs_acm.sh-822   [000] d..1    44.286279: dwc3_gadget_ep_cmd:
>> > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000
>> > > 00000500 00000000 --> status: Successful
>> > >
>> > > Hi John,
>> > >
>> > > Do you still have this problem? if yes, What's the value of
>> > > USBLNKST[21:18] when the timeout happens?
>> >
>> > Sorry. As I mentioned, I was working to upstream a patchset that I
>> > hadn't created, so the context I had was limited. As I couldn't
>> > reproduce an issue without the change on the device I had, I figured
>> > it would be best to drop it.
>>
>> That was fine.
>> >
>> > However, as you have some analysis and rational for why such a change
>> > would be needed, I don't have an objection to it. Do you want to
>> > resubmit the patch with your explanation and detailed log above in the
>> > commit message?
>>
>> Sure, I will resubmit the patch with my explanation added in commit message.
>
> Hi John
>
> A second think of this, I feel use readl_poll_timeout_atomic() to wait by time
> is more proper here, so I create a new patch to address this also other
> registers polling, see below patch with you CCed:
>
> https://patchwork.kernel.org/patch/11536081/

Fixing a bug has nothing to do with using
readl_poll_timeout_atomic(). Please don't mix things as it just makes
review time consuming.

Let's find out what the bug is all about, only then should we consider
moving over to readl_poll_timeout_atomic().

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-08 12:33         ` Felipe Balbi
@ 2020-05-09  8:10           ` Jun Li
  2020-05-15  9:31             ` Felipe Balbi
  0 siblings, 1 reply; 53+ messages in thread
From: Jun Li @ 2020-05-09  8:10 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	peter.chen, Li Jun, Thinh Nguyen

Hi,

Felipe Balbi <balbi@kernel.org> 于2020年5月8日周五 下午8:33写道:
>
>
> Hi,
>
> Jun Li <lijun.kernel@gmail.com> writes:
> > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道:
> >>
> >> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote:
> >> > John Stultz <john.stultz@linaro.org> writes:
> >> > > From: Yu Chen <chenyu56@huawei.com>
> >> > >
> >> > > It needs more time for the device controller to clear the CmdAct of
> >> > > DEPCMD on Hisilicon Kirin Soc.
> >> >
> >> > Why does it need more time? Why is it so that no other platform needs
> >> > more time, only this one? And which command, specifically, causes
> >> > problem?
> >
> > Sorry for my back to this so late.
> >
> > This change is required on my dwc3 based HW too, I gave a check
> > and the reason is suspend_clk is used in case the PIPE phy is at P3,
> > this slow clock makes my EP command below timeout.
>
> The phy needs to woken up before the command is triggered. Currently we
> only wake up the HS PHY. Does it help you if we wake up the SS phy as
> well?
>
> Something like below ought to do it:
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index a0555252dee0..ee46c2dacaeb 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -274,7 +274,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
>         const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
>         struct dwc3             *dwc = dep->dwc;
>         u32                     timeout = 1000;
> -       u32                     saved_config = 0;
> +       u32                     saved_hs_config = 0;
> +       u32                     saved_ss_config = 0;
>         u32                     reg;
>
>         int                     cmd_status = 0;
> @@ -293,19 +294,28 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
>         if (dwc->gadget.speed <= USB_SPEED_HIGH) {
>                 reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>                 if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) {
> -                       saved_config |= DWC3_GUSB2PHYCFG_SUSPHY;
> +                       saved_hs_config |= DWC3_GUSB2PHYCFG_SUSPHY;
>                         reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
>                 }
>
>                 if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) {
> -                       saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM;
> +                       saved_hs_config |= DWC3_GUSB2PHYCFG_ENBLSLPM;
>                         reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
>                 }
>
> -               if (saved_config)
> +               if (saved_hs_config)
>                         dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>         }
>
> +       reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> +       if (unlikely(reg & DWC3_GUSB3PIPECTL_SUSPHY)) {
> +               saved_ss_config |= DWC3_GUSB3PIPECTL_SUSPHY;
> +               reg &= ~DWC3_GUSB3PIPECTL_SUSPHY;
> +       }
> +
> +       if (saved_ss_config)
> +               dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> +
>         if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) {
>                 int             needs_wakeup;
>
> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
>                         dwc3_gadget_ep_get_transfer_index(dep);
>         }
>
> -       if (saved_config) {
> +       if (saved_hs_config) {
>                 reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> -               reg |= saved_config;
> +               reg |= saved_hs_config;
>                 dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>         }
>
> +       if (saved_ss_config) {
> +               reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> +               reg |= saved_ss_config;
> +               dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> +       }
> +
>         return ret;
>  }

Unfortunately this way can't work, once the SS PHY enters P3, disable
suspend_en can't force SS PHY exit P3, unless do this at the very beginning
to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for test).

thanks
Li Jun
>
> --
> balbi

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-08 12:35               ` Felipe Balbi
@ 2020-05-09  8:28                 ` Jun Li
  0 siblings, 0 replies; 53+ messages in thread
From: Jun Li @ 2020-05-09  8:28 UTC (permalink / raw)
  To: Felipe Balbi
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS

Felipe Balbi <balbi@kernel.org> 于2020年5月8日周五 下午8:35写道:
>
>
> Hi,
>
> Jun Li <lijun.kernel@gmail.com> writes:
> > Jun Li <lijun.kernel@gmail.com> 于2020年5月7日周四 上午11:08写道:
> >>
> >> John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道:
> >> >
> >> > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote:
> >> > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道:
> >> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote:
> >> > > > > John Stultz <john.stultz@linaro.org> writes:
> >> > > > > > From: Yu Chen <chenyu56@huawei.com>
> >> > > > > >
> >> > > > > > It needs more time for the device controller to clear the CmdAct of
> >> > > > > > DEPCMD on Hisilicon Kirin Soc.
> >> > > > >
> >> > > > > Why does it need more time? Why is it so that no other platform needs
> >> > > > > more time, only this one? And which command, specifically, causes
> >> > > > > problem?
> >> > >
> >> > > Sorry for my back to this so late.
> >> > >
> >> > > This change is required on my dwc3 based HW too, I gave a check
> >> > > and the reason is suspend_clk is used in case the PIPE phy is at P3,
> >> > > this slow clock makes my EP command below timeout.
> >> > >
> >> > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401]
> >> > > params 00001000 00000500 00000000 --> status: Timed Out
> >> > >
> >> > > Success case takes about 400us to complete, see below trace(44.286278
> >> > > - 44.285897 = 0.000381):
> >> > >
> >> > > configfs_acm.sh-822   [000] d..1    44.285896: dwc3_writel: addr
> >> > > 000000006d59aae1 value 00000401
> >> > > configfs_acm.sh-822   [000] d..1    44.285897: dwc3_readl: addr
> >> > > 000000006d59aae1 value 00000401
> >> > > ... ...
> >> > > configfs_acm.sh-822   [000] d..1    44.286278: dwc3_readl: addr
> >> > > 000000006d59aae1 value 00000001
> >> > > configfs_acm.sh-822   [000] d..1    44.286279: dwc3_gadget_ep_cmd:
> >> > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000
> >> > > 00000500 00000000 --> status: Successful
> >> > >
> >> > > Hi John,
> >> > >
> >> > > Do you still have this problem? if yes, What's the value of
> >> > > USBLNKST[21:18] when the timeout happens?
> >> >
> >> > Sorry. As I mentioned, I was working to upstream a patchset that I
> >> > hadn't created, so the context I had was limited. As I couldn't
> >> > reproduce an issue without the change on the device I had, I figured
> >> > it would be best to drop it.
> >>
> >> That was fine.
> >> >
> >> > However, as you have some analysis and rational for why such a change
> >> > would be needed, I don't have an objection to it. Do you want to
> >> > resubmit the patch with your explanation and detailed log above in the
> >> > commit message?
> >>
> >> Sure, I will resubmit the patch with my explanation added in commit message.
> >
> > Hi John
> >
> > A second think of this, I feel use readl_poll_timeout_atomic() to wait by time
> > is more proper here, so I create a new patch to address this also other
> > registers polling, see below patch with you CCed:
> >
> > https://patchwork.kernel.org/patch/11536081/
>
> Fixing a bug has nothing to do with using
> readl_poll_timeout_atomic(). Please don't mix things as it just makes
> review time consuming.
>
> Let's find out what the bug is all about, only then should we consider
> moving over to readl_poll_timeout_atomic().

Agreed, sorry about that, I will hold on my readl_poll_timeout_atomic() changes
until we have a conclusion on this issue fix.

thanks
Li Jun
>
> --
> balbi

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-09  8:10           ` Jun Li
@ 2020-05-15  9:31             ` Felipe Balbi
  2020-05-15 10:07               ` Jun Li
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2020-05-15  9:31 UTC (permalink / raw)
  To: Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	peter.chen, Li Jun, Thinh Nguyen

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


Hi,

Jun Li <lijun.kernel@gmail.com> writes:
>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd,
>>                         dwc3_gadget_ep_get_transfer_index(dep);
>>         }
>>
>> -       if (saved_config) {
>> +       if (saved_hs_config) {
>>                 reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>> -               reg |= saved_config;
>> +               reg |= saved_hs_config;
>>                 dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>>         }
>>
>> +       if (saved_ss_config) {
>> +               reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>> +               reg |= saved_ss_config;
>> +               dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
>> +       }
>> +
>>         return ret;
>>  }
>
> Unfortunately this way can't work, once the SS PHY enters P3, disable
> suspend_en can't force SS PHY exit P3, unless do this at the very beginning
> to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for test).

It sounds like you have a quirky PHY. If that's the case, then you
probably need to use the flag you mentioned above. Please verify with
that.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-15  9:31             ` Felipe Balbi
@ 2020-05-15 10:07               ` Jun Li
  2020-05-15 10:41                 ` Felipe Balbi
  2020-05-16  0:25                 ` Thinh Nguyen
  0 siblings, 2 replies; 53+ messages in thread
From: Jun Li @ 2020-05-15 10:07 UTC (permalink / raw)
  To: Felipe Balbi, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen, Thinh Nguyen



> -----Original Message-----
> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
> Sent: 2020年5月15日 17:31
> To: Jun Li <lijun.kernel@gmail.com>
> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh Nguyen
> <Thinh.Nguyen@synopsys.com>
> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
> 
> 
> Hi,
> 
> Jun Li <lijun.kernel@gmail.com> writes:
> >> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned
> cmd,
> >>                         dwc3_gadget_ep_get_transfer_index(dep);
> >>         }
> >>
> >> -       if (saved_config) {
> >> +       if (saved_hs_config) {
> >>                 reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> >> -               reg |= saved_config;
> >> +               reg |= saved_hs_config;
> >>                 dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> >>         }
> >>
> >> +       if (saved_ss_config) {
> >> +               reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> >> +               reg |= saved_ss_config;
> >> +               dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> >> +       }
> >> +
> >>         return ret;
> >>  }
> >
> > Unfortunately this way can't work, once the SS PHY enters P3, disable
> > suspend_en can't force SS PHY exit P3, unless do this at the very
> > beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for
> test).
> 
> It sounds like you have a quirky PHY. 

From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY
bit should be as what I said, not a quirky.

Hi Thinh, could you comment this?

> If that's the case, then you probably need
> to use the flag you mentioned above. Please verify with that.

With quirk of "snps,dis_u3_susphy_quirk", I had verified it can
resolve the problem, but this will make USB3 Super Speed PHY
never enter P3, this is a huge impact on USB power consumption.

The timeout increase has no impact on those platforms which have
no this problem, but can give chance for platform with very low
supspend clk(like my case 32k) to work.

Thanks
Li Jun
> 
> --
> balbi

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

* RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-15 10:07               ` Jun Li
@ 2020-05-15 10:41                 ` Felipe Balbi
  2020-05-16  0:25                 ` Thinh Nguyen
  1 sibling, 0 replies; 53+ messages in thread
From: Felipe Balbi @ 2020-05-15 10:41 UTC (permalink / raw)
  To: Jun Li, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen, Thinh Nguyen

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


Hi,

Jun Li <jun.li@nxp.com> writes:
>> Jun Li <lijun.kernel@gmail.com> writes:
>> >> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned
>> cmd,
>> >>                         dwc3_gadget_ep_get_transfer_index(dep);
>> >>         }
>> >>
>> >> -       if (saved_config) {
>> >> +       if (saved_hs_config) {
>> >>                 reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>> >> -               reg |= saved_config;
>> >> +               reg |= saved_hs_config;
>> >>                 dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>> >>         }
>> >>
>> >> +       if (saved_ss_config) {
>> >> +               reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>> >> +               reg |= saved_ss_config;
>> >> +               dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
>> >> +       }
>> >> +
>> >>         return ret;
>> >>  }
>> >
>> > Unfortunately this way can't work, once the SS PHY enters P3, disable
>> > suspend_en can't force SS PHY exit P3, unless do this at the very
>> > beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for
>> test).
>> 
>> It sounds like you have a quirky PHY. 
>
> From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY
> bit should be as what I said, not a quirky.
>
> Hi Thinh, could you comment this?
>
>> If that's the case, then you probably need
>> to use the flag you mentioned above. Please verify with that.
>
> With quirk of "snps,dis_u3_susphy_quirk", I had verified it can
> resolve the problem, but this will make USB3 Super Speed PHY
> never enter P3, this is a huge impact on USB power consumption.
>
> The timeout increase has no impact on those platforms which have
> no this problem, but can give chance for platform with very low
> supspend clk(like my case 32k) to work.

I was under the impression that issuing a command would wake the PHY
up. I don't have access to DWC3 documentation to verify, but that's as I
remember. Is that not the case?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-15 10:07               ` Jun Li
  2020-05-15 10:41                 ` Felipe Balbi
@ 2020-05-16  0:25                 ` Thinh Nguyen
  2020-05-16  7:12                   ` Felipe Balbi
  1 sibling, 1 reply; 53+ messages in thread
From: Thinh Nguyen @ 2020-05-16  0:25 UTC (permalink / raw)
  To: Jun Li, Felipe Balbi, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen, Thinh Nguyen

Hi,

Jun Li wrote:
>> -----Original Message-----
>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
>> Sent: 2020年5月15日 17:31
>> To: Jun Li <lijun.kernel@gmail.com>
>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh Nguyen
>> <Thinh.Nguyen@synopsys.com>
>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>> controller
>>
>>
>> Hi,
>>
>> Jun Li <lijun.kernel@gmail.com> writes:
>>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned
>> cmd,
>>>>                          dwc3_gadget_ep_get_transfer_index(dep);
>>>>          }
>>>>
>>>> -       if (saved_config) {
>>>> +       if (saved_hs_config) {
>>>>                  reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>>>> -               reg |= saved_config;
>>>> +               reg |= saved_hs_config;
>>>>                  dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>>>>          }
>>>>
>>>> +       if (saved_ss_config) {
>>>> +               reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>>>> +               reg |= saved_ss_config;
>>>> +               dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
>>>> +       }
>>>> +
>>>>          return ret;
>>>>   }
>>> Unfortunately this way can't work, once the SS PHY enters P3, disable
>>> suspend_en can't force SS PHY exit P3, unless do this at the very
>>> beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for
>> test).
>>
>> It sounds like you have a quirky PHY.
>  From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY
> bit should be as what I said, not a quirky.
>
> Hi Thinh, could you comment this?

You only need to wake up the usb2 phy when issuing the command while 
running in highspeed or below. If you're running in SS or higher, 
internally the controller does it for you for usb3 phy. In Jun's case, 
it seems like it takes longer for his phy to wake up.

IMO, in this case, I think it's fine to increase the command timeout.

BR,
Thinh


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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-16  0:25                 ` Thinh Nguyen
@ 2020-05-16  7:12                   ` Felipe Balbi
  2020-05-16  9:20                     ` Jun Li
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2020-05-16  7:12 UTC (permalink / raw)
  To: Thinh Nguyen, Jun Li, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen, Thinh Nguyen

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> Jun Li wrote:
>>> -----Original Message-----
>>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
>>> Sent: 2020年5月15日 17:31
>>> To: Jun Li <lijun.kernel@gmail.com>
>>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
>>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
>>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
>>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
>>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
>>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
>>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>>> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh Nguyen
>>> <Thinh.Nguyen@synopsys.com>
>>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>>> controller
>>>
>>>
>>> Hi,
>>>
>>> Jun Li <lijun.kernel@gmail.com> writes:
>>>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned
>>> cmd,
>>>>>                          dwc3_gadget_ep_get_transfer_index(dep);
>>>>>          }
>>>>>
>>>>> -       if (saved_config) {
>>>>> +       if (saved_hs_config) {
>>>>>                  reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
>>>>> -               reg |= saved_config;
>>>>> +               reg |= saved_hs_config;
>>>>>                  dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
>>>>>          }
>>>>>
>>>>> +       if (saved_ss_config) {
>>>>> +               reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
>>>>> +               reg |= saved_ss_config;
>>>>> +               dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
>>>>> +       }
>>>>> +
>>>>>          return ret;
>>>>>   }
>>>> Unfortunately this way can't work, once the SS PHY enters P3, disable
>>>> suspend_en can't force SS PHY exit P3, unless do this at the very
>>>> beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for
>>> test).
>>>
>>> It sounds like you have a quirky PHY.
>>  From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY
>> bit should be as what I said, not a quirky.
>>
>> Hi Thinh, could you comment this?
>
> You only need to wake up the usb2 phy when issuing the command while 
> running in highspeed or below. If you're running in SS or higher, 
> internally the controller does it for you for usb3 phy. In Jun's case, 
> it seems like it takes longer for his phy to wake up.
>
> IMO, in this case, I think it's fine to increase the command timeout.

Is there an upper limit to this? Is 32k clock the slowest that can be
fed to the PHY as a suspend clock?

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-16  7:12                   ` Felipe Balbi
@ 2020-05-16  9:20                     ` Jun Li
  2020-05-16 11:57                       ` Felipe Balbi
  0 siblings, 1 reply; 53+ messages in thread
From: Jun Li @ 2020-05-16  9:20 UTC (permalink / raw)
  To: Felipe Balbi, Thinh Nguyen, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen, Thinh Nguyen

Hi,
> -----Original Message-----
> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
> Sent: 2020年5月16日 15:13
> To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li <jun.li@nxp.com>; Jun Li
> <lijun.kernel@gmail.com>
> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
> 
> 
> Hi,
> 
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
> > Jun Li wrote:
> >>> -----Original Message-----
> >>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
> >>> Sent: 2020年5月15日 17:31
> >>> To: Jun Li <lijun.kernel@gmail.com>
> >>> Cc: John Stultz <john.stultz@linaro.org>; lkml
> >>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg
> >>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
> >>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan
> >>> Lee <shufan_lee@richtek.com>; Heikki Krogerus
> >>> <heikki.krogerus@linux.intel.com>;
> >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>;
> >>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider
> >>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>;
> >>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE
> >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> >>> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh
> >>> Nguyen <Thinh.Nguyen@synopsys.com>
> >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
> >>> cleared by device controller
> >>>
> >>>
> >>> Hi,
> >>>
> >>> Jun Li <lijun.kernel@gmail.com> writes:
> >>>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep
> >>>>> *dep, unsigned
> >>> cmd,
> >>>>>                          dwc3_gadget_ep_get_transfer_index(dep);
> >>>>>          }
> >>>>>
> >>>>> -       if (saved_config) {
> >>>>> +       if (saved_hs_config) {
> >>>>>                  reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
> >>>>> -               reg |= saved_config;
> >>>>> +               reg |= saved_hs_config;
> >>>>>                  dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
> >>>>>          }
> >>>>>
> >>>>> +       if (saved_ss_config) {
> >>>>> +               reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
> >>>>> +               reg |= saved_ss_config;
> >>>>> +               dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
> >>>>> +       }
> >>>>> +
> >>>>>          return ret;
> >>>>>   }
> >>>> Unfortunately this way can't work, once the SS PHY enters P3,
> >>>> disable suspend_en can't force SS PHY exit P3, unless do this at
> >>>> the very beginning to prevent SS PHY entering P3(e.g. add
> >>>> "snps,dis_u3_susphy_quirk" for
> >>> test).
> >>>
> >>> It sounds like you have a quirky PHY.
> >>  From what I got from the IC design, the behavior of
> >> DWC3_GUSB3PIPECTL_SUSPHY bit should be as what I said, not a quirky.
> >>
> >> Hi Thinh, could you comment this?
> >
> > You only need to wake up the usb2 phy when issuing the command while
> > running in highspeed or below. If you're running in SS or higher,
> > internally the controller does it for you for usb3 phy. In Jun's case,
> > it seems like it takes longer for his phy to wake up.
> >
> > IMO, in this case, I think it's fine to increase the command timeout.
> 
> Is there an upper limit to this? Is 32k clock the slowest that can be fed to the
> PHY as a suspend clock?

Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale
(bits 31:19 of GCTL):

"Power Down Scale (PwrDnScale)
The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source
to a small part of the USB3 controller that operates when the SS PHY
is in its lowest power (P3) state, and therefore does not provide a clock.
The Power Down Scale field specifies how many suspend_clk periods
fit into a 16 kHz clock period. When performing the division, round up
the remainder.
For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend clock,
Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up)
Note:
- Minimum Suspend clock frequency is 32 kHz
- Maximum Suspend clock frequency is 125 MHz"

Li Jun
> 
> --
> balbi

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

* RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-16  9:20                     ` Jun Li
@ 2020-05-16 11:57                       ` Felipe Balbi
  2020-05-19  2:24                         ` Jun Li
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2020-05-16 11:57 UTC (permalink / raw)
  To: Jun Li, Thinh Nguyen, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen, Thinh Nguyen

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


Hi,

Jun Li <jun.li@nxp.com> writes:
>> >> Hi Thinh, could you comment this?
>> >
>> > You only need to wake up the usb2 phy when issuing the command while
>> > running in highspeed or below. If you're running in SS or higher,
>> > internally the controller does it for you for usb3 phy. In Jun's case,
>> > it seems like it takes longer for his phy to wake up.
>> >
>> > IMO, in this case, I think it's fine to increase the command timeout.
>> 
>> Is there an upper limit to this? Is 32k clock the slowest that can be fed to the
>> PHY as a suspend clock?
>
> Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale
> (bits 31:19 of GCTL):
>
> "Power Down Scale (PwrDnScale)
> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source
> to a small part of the USB3 controller that operates when the SS PHY
> is in its lowest power (P3) state, and therefore does not provide a clock.
> The Power Down Scale field specifies how many suspend_clk periods
> fit into a 16 kHz clock period. When performing the division, round up
> the remainder.
> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend clock,
> Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up)
> Note:
> - Minimum Suspend clock frequency is 32 kHz
> - Maximum Suspend clock frequency is 125 MHz"

Cool, now do we have an upper bound for how many clock cycles it takes
to wake up the PHY? Then we can just set the time to that upper bound.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-16 11:57                       ` Felipe Balbi
@ 2020-05-19  2:24                         ` Jun Li
  2020-05-19  6:28                           ` Thinh Nguyen
  0 siblings, 1 reply; 53+ messages in thread
From: Jun Li @ 2020-05-19  2:24 UTC (permalink / raw)
  To: Felipe Balbi, Thinh Nguyen, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen, Thinh Nguyen



> -----Original Message-----
> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
> Sent: 2020年5月16日 19:57
> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li
> <lijun.kernel@gmail.com>
> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
> 
> 
> Hi,
> 
> Jun Li <jun.li@nxp.com> writes:
> >> >> Hi Thinh, could you comment this?
> >> >
> >> > You only need to wake up the usb2 phy when issuing the command
> >> > while running in highspeed or below. If you're running in SS or
> >> > higher, internally the controller does it for you for usb3 phy. In
> >> > Jun's case, it seems like it takes longer for his phy to wake up.
> >> >
> >> > IMO, in this case, I think it's fine to increase the command timeout.
> >>
> >> Is there an upper limit to this? Is 32k clock the slowest that can be
> >> fed to the PHY as a suspend clock?
> >
> > Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale
> > (bits 31:19 of GCTL):
> >
> > "Power Down Scale (PwrDnScale)
> > The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source to
> > a small part of the USB3 controller that operates when the SS PHY is
> > in its lowest power (P3) state, and therefore does not provide a clock.
> > The Power Down Scale field specifies how many suspend_clk periods fit
> > into a 16 kHz clock period. When performing the division, round up the
> > remainder.
> > For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend
> > clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up)
> > Note:
> > - Minimum Suspend clock frequency is 32 kHz
> > - Maximum Suspend clock frequency is 125 MHz"
> 
> Cool, now do we have an upper bound for how many clock cycles it takes to wake up
> the PHY? 
My understanding is this ep command does not wake up the SS PHY,
the SS PHY still stays at P3 when execute this ep command. The time
required here is to wait controller complete something for this ep
command with 32K clock.

> Then we can just set the time to that upper bound.
Per my test with trace, the time is about 400us(~13 cycles).

Thanks
Li Jun
> 
> --
> balbi

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-19  2:24                         ` Jun Li
@ 2020-05-19  6:28                           ` Thinh Nguyen
  2020-05-19  6:46                             ` Thinh Nguyen
  0 siblings, 1 reply; 53+ messages in thread
From: Thinh Nguyen @ 2020-05-19  6:28 UTC (permalink / raw)
  To: Jun Li, Felipe Balbi, Thinh Nguyen, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen

Jun Li wrote:
>> -----Original Message-----
>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
>> Sent: 2020年5月16日 19:57
>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li
>> <lijun.kernel@gmail.com>
>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>> controller
>>
>>
>> Hi,
>>
>> Jun Li <jun.li@nxp.com> writes:
>>>>>> Hi Thinh, could you comment this?
>>>>> You only need to wake up the usb2 phy when issuing the command
>>>>> while running in highspeed or below. If you're running in SS or
>>>>> higher, internally the controller does it for you for usb3 phy. In
>>>>> Jun's case, it seems like it takes longer for his phy to wake up.
>>>>>
>>>>> IMO, in this case, I think it's fine to increase the command timeout.
>>>> Is there an upper limit to this? Is 32k clock the slowest that can be
>>>> fed to the PHY as a suspend clock?
>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale
>>> (bits 31:19 of GCTL):
>>>
>>> "Power Down Scale (PwrDnScale)
>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source to
>>> a small part of the USB3 controller that operates when the SS PHY is
>>> in its lowest power (P3) state, and therefore does not provide a clock.
>>> The Power Down Scale field specifies how many suspend_clk periods fit
>>> into a 16 kHz clock period. When performing the division, round up the
>>> remainder.
>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend
>>> clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up)
>>> Note:
>>> - Minimum Suspend clock frequency is 32 kHz
>>> - Maximum Suspend clock frequency is 125 MHz"
>> Cool, now do we have an upper bound for how many clock cycles it takes to wake up
>> the PHY?
> My understanding is this ep command does not wake up the SS PHY,
> the SS PHY still stays at P3 when execute this ep command. The time
> required here is to wait controller complete something for this ep
> command with 32K clock.

Sorry I made a mistake. You're right. Just checked with one of the RTL 
engineers, and it doesn't need to wake up the phy. However, if it is eSS 
speed, it may take longer time as the command may be completing with the 
suspend clock.

BR,
Thinh


>
>> Then we can just set the time to that upper bound.
> Per my test with trace, the time is about 400us(~13 cycles).
>
> Thanks
> Li Jun
>> --
>> balbi


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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-19  6:28                           ` Thinh Nguyen
@ 2020-05-19  6:46                             ` Thinh Nguyen
  2020-05-19  7:39                               ` Jun Li
  0 siblings, 1 reply; 53+ messages in thread
From: Thinh Nguyen @ 2020-05-19  6:46 UTC (permalink / raw)
  To: Jun Li, Felipe Balbi, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen

Thinh Nguyen wrote:
> Jun Li wrote:
>>> -----Original Message-----
>>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
>>> Sent: 2020年5月16日 19:57
>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li
>>> <lijun.kernel@gmail.com>
>>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
>>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
>>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
>>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
>>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
>>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
>>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>>> controller
>>>
>>>
>>> Hi,
>>>
>>> Jun Li <jun.li@nxp.com> writes:
>>>>>>> Hi Thinh, could you comment this?
>>>>>> You only need to wake up the usb2 phy when issuing the command
>>>>>> while running in highspeed or below. If you're running in SS or
>>>>>> higher, internally the controller does it for you for usb3 phy. In
>>>>>> Jun's case, it seems like it takes longer for his phy to wake up.
>>>>>>
>>>>>> IMO, in this case, I think it's fine to increase the command timeout.
>>>>> Is there an upper limit to this? Is 32k clock the slowest that can be
>>>>> fed to the PHY as a suspend clock?
>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale
>>>> (bits 31:19 of GCTL):
>>>>
>>>> "Power Down Scale (PwrDnScale)
>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source to
>>>> a small part of the USB3 controller that operates when the SS PHY is
>>>> in its lowest power (P3) state, and therefore does not provide a clock.
>>>> The Power Down Scale field specifies how many suspend_clk periods fit
>>>> into a 16 kHz clock period. When performing the division, round up the
>>>> remainder.
>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend
>>>> clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up)
>>>> Note:
>>>> - Minimum Suspend clock frequency is 32 kHz
>>>> - Maximum Suspend clock frequency is 125 MHz"
>>> Cool, now do we have an upper bound for how many clock cycles it takes to wake up
>>> the PHY?
>> My understanding is this ep command does not wake up the SS PHY,
>> the SS PHY still stays at P3 when execute this ep command. The time
>> required here is to wait controller complete something for this ep
>> command with 32K clock.
> Sorry I made a mistake. You're right. Just checked with one of the RTL
> engineers, and it doesn't need to wake up the phy. However, if it is eSS
> speed, it may take longer time as the command may be completing with the
> suspend clock.
>

What's the value for GCTL[7:6]?

BR,
Thinh

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

* RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-19  6:46                             ` Thinh Nguyen
@ 2020-05-19  7:39                               ` Jun Li
  2020-05-21  1:07                                 ` Thinh Nguyen
  0 siblings, 1 reply; 53+ messages in thread
From: Jun Li @ 2020-05-19  7:39 UTC (permalink / raw)
  To: Thinh Nguyen, Felipe Balbi, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen

Hi

> -----Original Message-----
> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Sent: 2020年5月19日 14:46
> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li
> <lijun.kernel@gmail.com>
> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> Peter Chen <peter.chen@nxp.com>
> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
> 
> Thinh Nguyen wrote:
> > Jun Li wrote:
> >>> -----Original Message-----
> >>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
> >>> Sent: 2020年5月16日 19:57
> >>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen
> >>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com>
> >>> Cc: John Stultz <john.stultz@linaro.org>; lkml
> >>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg
> >>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
> >>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan
> >>> Lee <shufan_lee@richtek.com>; Heikki Krogerus
> >>> <heikki.krogerus@linux.intel.com>;
> >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>;
> >>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider
> >>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>;
> >>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE
> >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> >>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen
> >>> <Thinh.Nguyen@synopsys.com>
> >>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
> >>> cleared by device controller
> >>>
> >>>
> >>> Hi,
> >>>
> >>> Jun Li <jun.li@nxp.com> writes:
> >>>>>>> Hi Thinh, could you comment this?
> >>>>>> You only need to wake up the usb2 phy when issuing the command
> >>>>>> while running in highspeed or below. If you're running in SS or
> >>>>>> higher, internally the controller does it for you for usb3 phy.
> >>>>>> In Jun's case, it seems like it takes longer for his phy to wake up.
> >>>>>>
> >>>>>> IMO, in this case, I think it's fine to increase the command timeout.
> >>>>> Is there an upper limit to this? Is 32k clock the slowest that can
> >>>>> be fed to the PHY as a suspend clock?
> >>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down
> >>>> Scale (bits 31:19 of GCTL):
> >>>>
> >>>> "Power Down Scale (PwrDnScale)
> >>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source
> >>>> to a small part of the USB3 controller that operates when the SS
> >>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock.
> >>>> The Power Down Scale field specifies how many suspend_clk periods
> >>>> fit into a 16 kHz clock period. When performing the division, round
> >>>> up the remainder.
> >>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz
> >>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563
> >>>> (rounder up)
> >>>> Note:
> >>>> - Minimum Suspend clock frequency is 32 kHz
> >>>> - Maximum Suspend clock frequency is 125 MHz"
> >>> Cool, now do we have an upper bound for how many clock cycles it
> >>> takes to wake up the PHY?
> >> My understanding is this ep command does not wake up the SS PHY, the
> >> SS PHY still stays at P3 when execute this ep command. The time
> >> required here is to wait controller complete something for this ep
> >> command with 32K clock.
> > Sorry I made a mistake. You're right. Just checked with one of the RTL
> > engineers, and it doesn't need to wake up the phy. However, if it is
> > eSS speed, it may take longer time as the command may be completing
> > with the suspend clock.
> >
> 
> What's the value for GCTL[7:6]?

2'b00

Thanks
Li Jun
> 
> BR,
> Thinh

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-19  7:39                               ` Jun Li
@ 2020-05-21  1:07                                 ` Thinh Nguyen
  2020-05-21  1:55                                   ` Thinh Nguyen
  0 siblings, 1 reply; 53+ messages in thread
From: Thinh Nguyen @ 2020-05-21  1:07 UTC (permalink / raw)
  To: Jun Li, Thinh Nguyen, Felipe Balbi, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen

Jun Li wrote:
> Hi
>
>> -----Original Message-----
>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>> Sent: 2020年5月19日 14:46
>> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li
>> <lijun.kernel@gmail.com>
>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>> Peter Chen <peter.chen@nxp.com>
>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>> controller
>>
>> Thinh Nguyen wrote:
>>> Jun Li wrote:
>>>>> -----Original Message-----
>>>>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
>>>>> Sent: 2020年5月16日 19:57
>>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen
>>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com>
>>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml
>>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg
>>>>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
>>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan
>>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus
>>>>> <heikki.krogerus@linux.intel.com>;
>>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>;
>>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider
>>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>;
>>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE
>>>>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>>>>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen
>>>>> <Thinh.Nguyen@synopsys.com>
>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
>>>>> cleared by device controller
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Jun Li <jun.li@nxp.com> writes:
>>>>>>>>> Hi Thinh, could you comment this?
>>>>>>>> You only need to wake up the usb2 phy when issuing the command
>>>>>>>> while running in highspeed or below. If you're running in SS or
>>>>>>>> higher, internally the controller does it for you for usb3 phy.
>>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up.
>>>>>>>>
>>>>>>>> IMO, in this case, I think it's fine to increase the command timeout.
>>>>>>> Is there an upper limit to this? Is 32k clock the slowest that can
>>>>>>> be fed to the PHY as a suspend clock?
>>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down
>>>>>> Scale (bits 31:19 of GCTL):
>>>>>>
>>>>>> "Power Down Scale (PwrDnScale)
>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source
>>>>>> to a small part of the USB3 controller that operates when the SS
>>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock.
>>>>>> The Power Down Scale field specifies how many suspend_clk periods
>>>>>> fit into a 16 kHz clock period. When performing the division, round
>>>>>> up the remainder.
>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz
>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563
>>>>>> (rounder up)
>>>>>> Note:
>>>>>> - Minimum Suspend clock frequency is 32 kHz
>>>>>> - Maximum Suspend clock frequency is 125 MHz"
>>>>> Cool, now do we have an upper bound for how many clock cycles it
>>>>> takes to wake up the PHY?
>>>> My understanding is this ep command does not wake up the SS PHY, the
>>>> SS PHY still stays at P3 when execute this ep command. The time
>>>> required here is to wait controller complete something for this ep
>>>> command with 32K clock.
>>> Sorry I made a mistake. You're right. Just checked with one of the RTL
>>> engineers, and it doesn't need to wake up the phy. However, if it is
>>> eSS speed, it may take longer time as the command may be completing
>>> with the suspend clock.
>>>
>> What's the value for GCTL[7:6]?
> 2'b00
>
> Thanks
> Li Jun

(Sorry for the delay reply)

If it's 0, then the ram clock should be the same as the bus_clk, which 
is odd since you mentioned that the suspend_clk is used instead while in P3.

Anyway, I was looking for a way maybe to improve the speed during 
issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should 
wakeup the phy anytime. I think Felipe suggested it. It's odd that it 
doesn't work for you. I don't have other ideas beside increasing the 
command timeout.

Thanks,
Thinh


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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-21  1:07                                 ` Thinh Nguyen
@ 2020-05-21  1:55                                   ` Thinh Nguyen
  2020-05-21  6:20                                     ` Felipe Balbi
  2020-05-21  7:33                                     ` Jun Li
  0 siblings, 2 replies; 53+ messages in thread
From: Thinh Nguyen @ 2020-05-21  1:55 UTC (permalink / raw)
  To: Jun Li, Felipe Balbi, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen

Thinh Nguyen wrote:
> Jun Li wrote:
>> Hi
>>
>>> -----Original Message-----
>>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
>>> Sent: 2020年5月19日 14:46
>>> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li
>>> <lijun.kernel@gmail.com>
>>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
>>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
>>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
>>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
>>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
>>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
>>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>>> Peter Chen <peter.chen@nxp.com>
>>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>>> controller
>>>
>>> Thinh Nguyen wrote:
>>>> Jun Li wrote:
>>>>>> -----Original Message-----
>>>>>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
>>>>>> Sent: 2020年5月16日 19:57
>>>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen
>>>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com>
>>>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml
>>>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg
>>>>>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
>>>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan
>>>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus
>>>>>> <heikki.krogerus@linux.intel.com>;
>>>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
>>>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>;
>>>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider
>>>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>;
>>>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE
>>>>>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
>>>>>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen
>>>>>> <Thinh.Nguyen@synopsys.com>
>>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
>>>>>> cleared by device controller
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Jun Li <jun.li@nxp.com> writes:
>>>>>>>>>> Hi Thinh, could you comment this?
>>>>>>>>> You only need to wake up the usb2 phy when issuing the command
>>>>>>>>> while running in highspeed or below. If you're running in SS or
>>>>>>>>> higher, internally the controller does it for you for usb3 phy.
>>>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up.
>>>>>>>>>
>>>>>>>>> IMO, in this case, I think it's fine to increase the command timeout.
>>>>>>>> Is there an upper limit to this? Is 32k clock the slowest that can
>>>>>>>> be fed to the PHY as a suspend clock?
>>>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down
>>>>>>> Scale (bits 31:19 of GCTL):
>>>>>>>
>>>>>>> "Power Down Scale (PwrDnScale)
>>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source
>>>>>>> to a small part of the USB3 controller that operates when the SS
>>>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock.
>>>>>>> The Power Down Scale field specifies how many suspend_clk periods
>>>>>>> fit into a 16 kHz clock period. When performing the division, round
>>>>>>> up the remainder.
>>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz
>>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563
>>>>>>> (rounder up)
>>>>>>> Note:
>>>>>>> - Minimum Suspend clock frequency is 32 kHz
>>>>>>> - Maximum Suspend clock frequency is 125 MHz"
>>>>>> Cool, now do we have an upper bound for how many clock cycles it
>>>>>> takes to wake up the PHY?
>>>>> My understanding is this ep command does not wake up the SS PHY, the
>>>>> SS PHY still stays at P3 when execute this ep command. The time
>>>>> required here is to wait controller complete something for this ep
>>>>> command with 32K clock.
>>>> Sorry I made a mistake. You're right. Just checked with one of the RTL
>>>> engineers, and it doesn't need to wake up the phy. However, if it is
>>>> eSS speed, it may take longer time as the command may be completing
>>>> with the suspend clock.
>>>>
>>> What's the value for GCTL[7:6]?
>> 2'b00
>>
>> Thanks
>> Li Jun
> (Sorry for the delay reply)
>
> If it's 0, then the ram clock should be the same as the bus_clk, which
> is odd since you mentioned that the suspend_clk is used instead while in P3.

Just checked with the RTL engineer, even if GCTL[7:6] is set to 0, 
internally it can still run with suspend clock during P3.

> Anyway, I was looking for a way maybe to improve the speed during
> issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should
> wakeup the phy anytime. I think Felipe suggested it. It's odd that it
> doesn't work for you. I don't have other ideas beside increasing the
> command timeout.
>

In any case, increasing the timeout should be fine with me. It maybe 
difficult to determine the max timeout base on the slowest clock rate 
and number of cycles. Different controller and controller versions 
behave differently and may have different number of clock cycles to 
complete a command.

The RTL engineer recommended timeout to be at least 1ms (which maybe 
more than the polling rate of this patch). I'm fine with either the rate 
provided by this tested patch or higher.

BR,
Thinh

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-21  1:55                                   ` Thinh Nguyen
@ 2020-05-21  6:20                                     ` Felipe Balbi
  2020-05-21  6:22                                       ` Felipe Balbi
  2020-05-21  7:33                                     ` Jun Li
  1 sibling, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2020-05-21  6:20 UTC (permalink / raw)
  To: Thinh Nguyen, Jun Li, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen

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


Hi,

Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>>>>>>>> "Power Down Scale (PwrDnScale)
>>>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source
>>>>>>>> to a small part of the USB3 controller that operates when the SS
>>>>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock.
>>>>>>>> The Power Down Scale field specifies how many suspend_clk periods
>>>>>>>> fit into a 16 kHz clock period. When performing the division, round
>>>>>>>> up the remainder.
>>>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz
>>>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563
>>>>>>>> (rounder up)
>>>>>>>> Note:
>>>>>>>> - Minimum Suspend clock frequency is 32 kHz
>>>>>>>> - Maximum Suspend clock frequency is 125 MHz"
>>>>>>> Cool, now do we have an upper bound for how many clock cycles it
>>>>>>> takes to wake up the PHY?
>>>>>> My understanding is this ep command does not wake up the SS PHY, the
>>>>>> SS PHY still stays at P3 when execute this ep command. The time
>>>>>> required here is to wait controller complete something for this ep
>>>>>> command with 32K clock.
>>>>> Sorry I made a mistake. You're right. Just checked with one of the RTL
>>>>> engineers, and it doesn't need to wake up the phy. However, if it is
>>>>> eSS speed, it may take longer time as the command may be completing
>>>>> with the suspend clock.
>>>>>
>>>> What's the value for GCTL[7:6]?
>>> 2'b00
>>>
>>> Thanks
>>> Li Jun
>> (Sorry for the delay reply)
>>
>> If it's 0, then the ram clock should be the same as the bus_clk, which
>> is odd since you mentioned that the suspend_clk is used instead while in P3.
>
> Just checked with the RTL engineer, even if GCTL[7:6] is set to 0, 
> internally it can still run with suspend clock during P3.
>
>> Anyway, I was looking for a way maybe to improve the speed during
>> issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should
>> wakeup the phy anytime. I think Felipe suggested it. It's odd that it
>> doesn't work for you. I don't have other ideas beside increasing the
>> command timeout.
>>
>
> In any case, increasing the timeout should be fine with me. It maybe 
> difficult to determine the max timeout base on the slowest clock rate 
> and number of cycles. Different controller and controller versions 
> behave differently and may have different number of clock cycles to 
> complete a command.
>
> The RTL engineer recommended timeout to be at least 1ms (which maybe 
> more than the polling rate of this patch). I'm fine with either the rate 
> provided by this tested patch or higher.

A whole ms waiting for a command to complete? Wow, that's a lot of time
blocking the CPU. It looks like, perhaps, we should move to command
completion interrupts. The difficulty here is that we issue commands
from within the interrupt handler and, as such, can't
wait_for_completion().

Meanwhile, we will take the timeout increase I guess, otherwise NXP
won't have a working setup.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-21  6:20                                     ` Felipe Balbi
@ 2020-05-21  6:22                                       ` Felipe Balbi
  2020-05-21  7:47                                         ` Jun Li
  0 siblings, 1 reply; 53+ messages in thread
From: Felipe Balbi @ 2020-05-21  6:22 UTC (permalink / raw)
  To: Thinh Nguyen, Jun Li, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen

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


Hi Jun,

Felipe Balbi <balbi@kernel.org> writes:
>> In any case, increasing the timeout should be fine with me. It maybe 
>> difficult to determine the max timeout base on the slowest clock rate 
>> and number of cycles. Different controller and controller versions 
>> behave differently and may have different number of clock cycles to 
>> complete a command.
>>
>> The RTL engineer recommended timeout to be at least 1ms (which maybe 
>> more than the polling rate of this patch). I'm fine with either the rate 
>> provided by this tested patch or higher.
>
> A whole ms waiting for a command to complete? Wow, that's a lot of time
> blocking the CPU. It looks like, perhaps, we should move to command
> completion interrupts. The difficulty here is that we issue commands
> from within the interrupt handler and, as such, can't
> wait_for_completion().
>
> Meanwhile, we will take the timeout increase I guess, otherwise NXP
> won't have a working setup.

patch 1 in this series doesn't apply to testing/next. Care to rebase and
resend?

Thank you

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

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

* RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-21  1:55                                   ` Thinh Nguyen
  2020-05-21  6:20                                     ` Felipe Balbi
@ 2020-05-21  7:33                                     ` Jun Li
  1 sibling, 0 replies; 53+ messages in thread
From: Jun Li @ 2020-05-21  7:33 UTC (permalink / raw)
  To: Thinh Nguyen, Felipe Balbi, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen

Hi Thinh,
> -----Original Message-----
> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Sent: 2020年5月21日 9:56
> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li
> <lijun.kernel@gmail.com>
> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> Peter Chen <peter.chen@nxp.com>
> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
> 
> Thinh Nguyen wrote:
> > Jun Li wrote:
> >> Hi
> >>
> >>> -----Original Message-----
> >>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> >>> Sent: 2020年5月19日 14:46
> >>> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li
> >>> <lijun.kernel@gmail.com>
> >>> Cc: John Stultz <john.stultz@linaro.org>; lkml
> >>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg
> >>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
> >>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan
> >>> Lee <shufan_lee@richtek.com>; Heikki Krogerus
> >>> <heikki.krogerus@linux.intel.com>;
> >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>;
> >>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider
> >>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>;
> >>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE
> >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> >>> Peter Chen <peter.chen@nxp.com>
> >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
> >>> cleared by device controller
> >>>
> >>> Thinh Nguyen wrote:
> >>>> Jun Li wrote:
> >>>>>> -----Original Message-----
> >>>>>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
> >>>>>> Sent: 2020年5月16日 19:57
> >>>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen
> >>>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com>
> >>>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml
> >>>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>;
> >>>>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring
> >>>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan
> >>>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus
> >>>>>> <heikki.krogerus@linux.intel.com>;
> >>>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> >>>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>;
> >>>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider
> >>>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>;
> >>>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN
> >>>>>> FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> >>>>>> <devicetree@vger.kernel.org>; Peter Chen <peter.chen@nxp.com>;
> >>>>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> >>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for
> >>>>>> CmdAct cleared by device controller
> >>>>>>
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Jun Li <jun.li@nxp.com> writes:
> >>>>>>>>>> Hi Thinh, could you comment this?
> >>>>>>>>> You only need to wake up the usb2 phy when issuing the command
> >>>>>>>>> while running in highspeed or below. If you're running in SS
> >>>>>>>>> or higher, internally the controller does it for you for usb3 phy.
> >>>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up.
> >>>>>>>>>
> >>>>>>>>> IMO, in this case, I think it's fine to increase the command timeout.
> >>>>>>>> Is there an upper limit to this? Is 32k clock the slowest that
> >>>>>>>> can be fed to the PHY as a suspend clock?
> >>>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down
> >>>>>>> Scale (bits 31:19 of GCTL):
> >>>>>>>
> >>>>>>> "Power Down Scale (PwrDnScale)
> >>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock
> >>>>>>> source to a small part of the USB3 controller that operates when
> >>>>>>> the SS PHY is in its lowest power (P3) state, and therefore does not provide
> a clock.
> >>>>>>> The Power Down Scale field specifies how many suspend_clk
> >>>>>>> periods fit into a 16 kHz clock period. When performing the
> >>>>>>> division, round up the remainder.
> >>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz
> >>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563
> >>>>>>> (rounder up)
> >>>>>>> Note:
> >>>>>>> - Minimum Suspend clock frequency is 32 kHz
> >>>>>>> - Maximum Suspend clock frequency is 125 MHz"
> >>>>>> Cool, now do we have an upper bound for how many clock cycles it
> >>>>>> takes to wake up the PHY?
> >>>>> My understanding is this ep command does not wake up the SS PHY,
> >>>>> the SS PHY still stays at P3 when execute this ep command. The
> >>>>> time required here is to wait controller complete something for
> >>>>> this ep command with 32K clock.
> >>>> Sorry I made a mistake. You're right. Just checked with one of the
> >>>> RTL engineers, and it doesn't need to wake up the phy. However, if
> >>>> it is eSS speed, it may take longer time as the command may be
> >>>> completing with the suspend clock.
> >>>>
> >>> What's the value for GCTL[7:6]?
> >> 2'b00
> >>
> >> Thanks
> >> Li Jun
> > (Sorry for the delay reply)
> >
> > If it's 0, then the ram clock should be the same as the bus_clk, which
> > is odd since you mentioned that the suspend_clk is used instead while in P3.
> 
> Just checked with the RTL engineer, even if GCTL[7:6] is set to 0, internally it
> can still run with suspend clock during P3.

Thanks for your check.
> 
> > Anyway, I was looking for a way maybe to improve the speed during
> > issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should
> > wakeup the phy anytime. I think Felipe suggested it. It's odd that it
> > doesn't work for you. I don't have other ideas beside increasing the
> > command timeout.
> >
> 
> In any case, increasing the timeout should be fine with me. It maybe difficult to
> determine the max timeout base on the slowest clock rate and number of cycles.
> Different controller and controller versions behave differently and may have
> different number of clock cycles to complete a command.
> 
> The RTL engineer recommended timeout to be at least 1ms (which maybe more than the
> polling rate of this patch). I'm fine with either the rate provided by this tested
> patch or higher.

OK, I will change the timeout to be 1ms if no object from Felipe.

thanks
Li Jun
> 
> BR,
> Thinh

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

* RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller
  2020-05-21  6:22                                       ` Felipe Balbi
@ 2020-05-21  7:47                                         ` Jun Li
  0 siblings, 0 replies; 53+ messages in thread
From: Jun Li @ 2020-05-21  7:47 UTC (permalink / raw)
  To: Felipe Balbi, Thinh Nguyen, Jun Li
  Cc: John Stultz, lkml, Yu Chen, Greg Kroah-Hartman, Rob Herring,
	Mark Rutland, ShuFan Lee, Heikki Krogerus, Suzuki K Poulose,
	Chunfeng Yun, Hans de Goede, Andy Shevchenko, Valentin Schneider,
	Jack Pham, Linux USB List,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Peter Chen

Hi Felipe,

> -----Original Message-----
> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi
> Sent: 2020年5月21日 14:23
> To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li <jun.li@nxp.com>; Jun Li
> <lijun.kernel@gmail.com>
> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu
> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob
> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee
> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>;
> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun
> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>;
> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>;
> Peter Chen <peter.chen@nxp.com>
> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
> 
> 
> Hi Jun,
> 
> Felipe Balbi <balbi@kernel.org> writes:
> >> In any case, increasing the timeout should be fine with me. It maybe
> >> difficult to determine the max timeout base on the slowest clock rate
> >> and number of cycles. Different controller and controller versions
> >> behave differently and may have different number of clock cycles to
> >> complete a command.
> >>
> >> The RTL engineer recommended timeout to be at least 1ms (which maybe
> >> more than the polling rate of this patch). I'm fine with either the
> >> rate provided by this tested patch or higher.
> >
> > A whole ms waiting for a command to complete? Wow, that's a lot of
> > time blocking the CPU. It looks like, perhaps, we should move to
> > command completion interrupts. The difficulty here is that we issue
> > commands from within the interrupt handler and, as such, can't
> > wait_for_completion().
> >
> > Meanwhile, we will take the timeout increase I guess, otherwise NXP
> > won't have a working setup.
> 
> patch 1 in this series doesn't apply to testing/next. Care to rebase and resend?

Sure, I will rebase and resend this patch with timeout loop 5000.

Thanks
Li Jun
> 
> Thank you
> 
> --
> balbi

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

end of thread, other threads:[~2020-05-21  7:48 UTC | newest]

Thread overview: 53+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-28 21:59 [PATCH v4 0/9] Prereqs for HiKey960 USB support John Stultz
2019-10-28 21:59 ` [PATCH v4 1/9] dt-bindings: usb: rt1711h: Add connector bindings John Stultz
2019-10-28 21:59 ` [PATCH v4 2/9] usb: dwc3: Execute GCTL Core Soft Reset while switch modes John Stultz
2019-10-29  9:09   ` Felipe Balbi
2019-10-29 21:21     ` John Stultz
2019-10-28 21:59 ` [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device controller John Stultz
2019-10-29  9:11   ` Felipe Balbi
2019-10-29 21:17     ` John Stultz
2020-05-06  9:00       ` Jun Li
2020-05-06 22:27         ` John Stultz
2020-05-07  3:08           ` Jun Li
2020-05-08 12:22             ` Jun Li
2020-05-08 12:35               ` Felipe Balbi
2020-05-09  8:28                 ` Jun Li
2020-05-08 12:33         ` Felipe Balbi
2020-05-09  8:10           ` Jun Li
2020-05-15  9:31             ` Felipe Balbi
2020-05-15 10:07               ` Jun Li
2020-05-15 10:41                 ` Felipe Balbi
2020-05-16  0:25                 ` Thinh Nguyen
2020-05-16  7:12                   ` Felipe Balbi
2020-05-16  9:20                     ` Jun Li
2020-05-16 11:57                       ` Felipe Balbi
2020-05-19  2:24                         ` Jun Li
2020-05-19  6:28                           ` Thinh Nguyen
2020-05-19  6:46                             ` Thinh Nguyen
2020-05-19  7:39                               ` Jun Li
2020-05-21  1:07                                 ` Thinh Nguyen
2020-05-21  1:55                                   ` Thinh Nguyen
2020-05-21  6:20                                     ` Felipe Balbi
2020-05-21  6:22                                       ` Felipe Balbi
2020-05-21  7:47                                         ` Jun Li
2020-05-21  7:33                                     ` Jun Li
2019-10-28 21:59 ` [PATCH v4 4/9] dt-bindings: usb: dwc3: Allow clock list & resets to be more flexible John Stultz
2019-10-28 21:59 ` [PATCH v4 5/9] usb: dwc3: Rework clock initialization " John Stultz
2019-10-29  9:13   ` Felipe Balbi
2019-10-29 16:08     ` John Stultz
2019-10-30  9:02       ` Felipe Balbi
2019-11-07 21:53     ` Rob Herring
2019-10-28 21:59 ` [PATCH v4 6/9] usb: dwc3: Rework resets " John Stultz
2019-10-29  9:17   ` Felipe Balbi
2019-10-29 18:05     ` John Stultz
2019-10-30  9:01       ` Felipe Balbi
2019-11-07 21:45         ` Rob Herring
2019-10-28 21:59 ` [PATCH v4 7/9] usb: dwc3: Registering a role switch in the DRD code John Stultz
2019-10-29  9:21   ` Felipe Balbi
2019-11-07 23:20     ` John Stultz
2019-10-28 21:59 ` [PATCH v4 8/9] dt-bindings: usb: generic: Add role-switch-default-host binding John Stultz
2019-10-29  9:23   ` Felipe Balbi
2019-10-29 18:26     ` John Stultz
2019-10-28 21:59 ` [PATCH v4 9/9] usb: dwc3: Add host-mode as default support John Stultz
2019-10-29  9:25   ` Felipe Balbi
2019-11-07 22:23     ` John Stultz

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