Linux-USB Archive on lore.kernel.org
 help / color / 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; 28+ 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] 28+ 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; 28+ 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	[flat|nested] 28+ 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; 28+ 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	[flat|nested] 28+ 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; 28+ 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	[flat|nested] 28+ 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; 28+ 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	[flat|nested] 28+ 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; 28+ 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	[flat|nested] 28+ 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; 28+ 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	[flat|nested] 28+ 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; 28+ 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	[flat|nested] 28+ 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; 28+ 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	[flat|nested] 28+ 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; 28+ 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	[flat|nested] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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
  0 siblings, 0 replies; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ 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; 28+ 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] 28+ messages in thread

end of thread, back to index

Thread overview: 28+ 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
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

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git