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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: [email protected]
Cc: [email protected]
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
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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: [email protected]
Cc: [email protected]
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: [email protected]
Cc: [email protected]
Suggested-by: Rob Herring <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: [email protected]
Cc: [email protected]
Suggested-by: Rob Herring <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: [email protected]
Cc: [email protected]
Suggested-by: Rob Herring <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: [email protected]
Cc: [email protected]
Reviewed-by: Rob Herring <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
.../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
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 <[email protected]>
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: John Stultz <[email protected]>
---
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
From: Yu Chen <[email protected]>
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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: [email protected]
Cc: [email protected]
Suggested-by: Heikki Krogerus <[email protected]>
Signed-off-by: Yu Chen <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
v2: Fix role_sw and role_switch_default_mode descriptions as
reported by kbuild test robot <[email protected]>
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
From: Yu Chen <[email protected]>
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 <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Yu Chen <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
From: Yu Chen <[email protected]>
It needs more time for the device controller to clear the CmdAct of
DEPCMD on Hisilicon Kirin Soc.
Cc: Greg Kroah-Hartman <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Mark Rutland <[email protected]>
CC: ShuFan Lee <[email protected]>
Cc: Heikki Krogerus <[email protected]>
Cc: Suzuki K Poulose <[email protected]>
Cc: Chunfeng Yun <[email protected]>
Cc: Yu Chen <[email protected]>
Cc: Felipe Balbi <[email protected]>
Cc: Hans de Goede <[email protected]>
Cc: Andy Shevchenko <[email protected]>
Cc: Jun Li <[email protected]>
Cc: Valentin Schneider <[email protected]>
Cc: Jack Pham <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Yu Chen <[email protected]>
Signed-off-by: John Stultz <[email protected]>
---
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
Hi,
John Stultz <[email protected]> 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 <[email protected]>
> 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 <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> CC: ShuFan Lee <[email protected]>
> Cc: Heikki Krogerus <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Chunfeng Yun <[email protected]>
> Cc: Yu Chen <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Hans de Goede <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Jun Li <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Jack Pham <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Suggested-by: Rob Herring <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> 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
Hi,
John Stultz <[email protected]> writes:
> From: Yu Chen <[email protected]>
>
> 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
Hi,
John Stultz <[email protected]> writes:
> From: Yu Chen <[email protected]>
>
> 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 <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> CC: ShuFan Lee <[email protected]>
> Cc: Heikki Krogerus <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Chunfeng Yun <[email protected]>
> Cc: Yu Chen <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Hans de Goede <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Jun Li <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Jack Pham <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Suggested-by: Heikki Krogerus <[email protected]>
> Signed-off-by: Yu Chen <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> v2: Fix role_sw and role_switch_default_mode descriptions as
> reported by kbuild test robot <[email protected]>
>
> 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
Hi,
John Stultz <[email protected]> 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 <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> CC: ShuFan Lee <[email protected]>
> Cc: Heikki Krogerus <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Chunfeng Yun <[email protected]>
> Cc: Yu Chen <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Hans de Goede <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Jun Li <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Jack Pham <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Reviewed-by: Rob Herring <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> 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
Hi,
John Stultz <[email protected]> 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 <[email protected]>
>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> CC: ShuFan Lee <[email protected]>
> Cc: Heikki Krogerus <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Chunfeng Yun <[email protected]>
> Cc: Yu Chen <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Hans de Goede <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Jun Li <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Jack Pham <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: John Stultz <[email protected]>
> ---
> 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
On Tue, Oct 29, 2019 at 2:14 AM Felipe Balbi <[email protected]> wrote:
> John Stultz <[email protected]> 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
Hi,
John Stultz <[email protected]> writes:
> From: Yu Chen <[email protected]>
>
> 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 <[email protected]>
> Cc: Rob Herring <[email protected]>
> Cc: Mark Rutland <[email protected]>
> CC: ShuFan Lee <[email protected]>
> Cc: Heikki Krogerus <[email protected]>
> Cc: Suzuki K Poulose <[email protected]>
> Cc: Chunfeng Yun <[email protected]>
> Cc: Yu Chen <[email protected]>
> Cc: Felipe Balbi <[email protected]>
> Cc: Hans de Goede <[email protected]>
> Cc: Andy Shevchenko <[email protected]>
> Cc: Jun Li <[email protected]>
> Cc: Valentin Schneider <[email protected]>
> Cc: Jack Pham <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Yu Chen <[email protected]>
> Signed-off-by: John Stultz <[email protected]>
> ---
> 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
Hi,
John Stultz <[email protected]> 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
On Tue, Oct 29, 2019 at 2:17 AM Felipe Balbi <[email protected]> wrote:
> John Stultz <[email protected]> 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
On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <[email protected]> wrote:
> John Stultz <[email protected]> writes:
> > From: Yu Chen <[email protected]>
> >
> > 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
On Tue, Oct 29, 2019 at 2:09 AM Felipe Balbi <[email protected]> wrote:
> John Stultz <[email protected]> writes:
> > From: Yu Chen <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > CC: ShuFan Lee <[email protected]>
> > Cc: Heikki Krogerus <[email protected]>
> > Cc: Suzuki K Poulose <[email protected]>
> > Cc: Chunfeng Yun <[email protected]>
> > Cc: Yu Chen <[email protected]>
> > Cc: Felipe Balbi <[email protected]>
> > Cc: Hans de Goede <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Jun Li <[email protected]>
> > Cc: Valentin Schneider <[email protected]>
> > Cc: Jack Pham <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Signed-off-by: Yu Chen <[email protected]>
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > 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
On Tue, Oct 29, 2019 at 2:23 AM Felipe Balbi <[email protected]> wrote:
> John Stultz <[email protected]> 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
Hi,
John Stultz <[email protected]> writes:
> On Tue, Oct 29, 2019 at 2:17 AM Felipe Balbi <[email protected]> wrote:
>> John Stultz <[email protected]> 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
Hi,
John Stultz <[email protected]> writes:
> On Tue, Oct 29, 2019 at 2:14 AM Felipe Balbi <[email protected]> wrote:
>> John Stultz <[email protected]> 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
On Wed, Oct 30, 2019 at 4:01 AM Felipe Balbi <[email protected]> wrote:
>
>
> Hi,
>
> John Stultz <[email protected]> writes:
>
> > On Tue, Oct 29, 2019 at 2:17 AM Felipe Balbi <[email protected]> wrote:
> >> John Stultz <[email protected]> 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
On Tue, Oct 29, 2019 at 4:14 AM Felipe Balbi <[email protected]> wrote:
>
>
> Hi,
>
> John Stultz <[email protected]> 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 <[email protected]>
> > 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 <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > CC: ShuFan Lee <[email protected]>
> > Cc: Heikki Krogerus <[email protected]>
> > Cc: Suzuki K Poulose <[email protected]>
> > Cc: Chunfeng Yun <[email protected]>
> > Cc: Yu Chen <[email protected]>
> > Cc: Felipe Balbi <[email protected]>
> > Cc: Hans de Goede <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Jun Li <[email protected]>
> > Cc: Valentin Schneider <[email protected]>
> > Cc: Jack Pham <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Suggested-by: Rob Herring <[email protected]>
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > 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
On Tue, Oct 29, 2019 at 2:25 AM Felipe Balbi <[email protected]> wrote:
> John Stultz <[email protected]> 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
On Tue, Oct 29, 2019 at 2:21 AM Felipe Balbi <[email protected]> wrote:
> John Stultz <[email protected]> writes:
> > From: Yu Chen <[email protected]>
> >
> > 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 <[email protected]>
> > Cc: Rob Herring <[email protected]>
> > Cc: Mark Rutland <[email protected]>
> > CC: ShuFan Lee <[email protected]>
> > Cc: Heikki Krogerus <[email protected]>
> > Cc: Suzuki K Poulose <[email protected]>
> > Cc: Chunfeng Yun <[email protected]>
> > Cc: Yu Chen <[email protected]>
> > Cc: Felipe Balbi <[email protected]>
> > Cc: Hans de Goede <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Jun Li <[email protected]>
> > Cc: Valentin Schneider <[email protected]>
> > Cc: Jack Pham <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Suggested-by: Heikki Krogerus <[email protected]>
> > Signed-off-by: Yu Chen <[email protected]>
> > Signed-off-by: John Stultz <[email protected]>
> > ---
> > v2: Fix role_sw and role_switch_default_mode descriptions as
> > reported by kbuild test robot <[email protected]>
> >
> > 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?
John Stultz <[email protected]> 于2019年10月30日周三 上午5:18写道：
>
> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <[email protected]> wrote:
> > John Stultz <[email protected]> writes:
> > > From: Yu Chen <[email protected]>
> > >
> > > 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
On Wed, May 6, 2020 at 2:00 AM Jun Li <[email protected]> wrote:
> John Stultz <[email protected]> 于2019年10月30日周三 上午5:18写道：
> > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <[email protected]> wrote:
> > > John Stultz <[email protected]> writes:
> > > > From: Yu Chen <[email protected]>
> > > >
> > > > 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
John Stultz <[email protected]> 于2020年5月7日周四 上午6:27写道：
>
> On Wed, May 6, 2020 at 2:00 AM Jun Li <[email protected]> wrote:
> > John Stultz <[email protected]> 于2019年10月30日周三 上午5:18写道：
> > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <[email protected]> wrote:
> > > > John Stultz <[email protected]> writes:
> > > > > From: Yu Chen <[email protected]>
> > > > >
> > > > > 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
Jun Li <[email protected]> 于2020年5月7日周四 上午11:08写道：
>
> John Stultz <[email protected]> 于2020年5月7日周四 上午6:27写道：
> >
> > On Wed, May 6, 2020 at 2:00 AM Jun Li <[email protected]> wrote:
> > > John Stultz <[email protected]> 于2019年10月30日周三 上午5:18写道：
> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <[email protected]> wrote:
> > > > > John Stultz <[email protected]> writes:
> > > > > > From: Yu Chen <[email protected]>
> > > > > >
> > > > > > 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
Hi,
Jun Li <[email protected]> writes:
> John Stultz <[email protected]> 于2019年10月30日周三 上午5:18写道：
>>
>> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <[email protected]> wrote:
>> > John Stultz <[email protected]> writes:
>> > > From: Yu Chen <[email protected]>
>> > >
>> > > 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
Hi,
Jun Li <[email protected]> writes:
> Jun Li <[email protected]> 于2020年5月7日周四 上午11:08写道：
>>
>> John Stultz <[email protected]> 于2020年5月7日周四 上午6:27写道：
>> >
>> > On Wed, May 6, 2020 at 2:00 AM Jun Li <[email protected]> wrote:
>> > > John Stultz <[email protected]> 于2019年10月30日周三 上午5:18写道：
>> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <[email protected]> wrote:
>> > > > > John Stultz <[email protected]> writes:
>> > > > > > From: Yu Chen <[email protected]>
>> > > > > >
>> > > > > > 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
Hi,
Felipe Balbi <[email protected]> 于2020年5月8日周五 下午8:33写道：
>
>
> Hi,
>
> Jun Li <[email protected]> writes:
> > John Stultz <[email protected]> 于2019年10月30日周三 上午5:18写道：
> >>
> >> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <[email protected]> wrote:
> >> > John Stultz <[email protected]> writes:
> >> > > From: Yu Chen <[email protected]>
> >> > >
> >> > > 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
Felipe Balbi <[email protected]> 于2020年5月8日周五 下午8:35写道：
>
>
> Hi,
>
> Jun Li <[email protected]> writes:
> > Jun Li <[email protected]> 于2020年5月7日周四 上午11:08写道：
> >>
> >> John Stultz <[email protected]> 于2020年5月7日周四 上午6:27写道：
> >> >
> >> > On Wed, May 6, 2020 at 2:00 AM Jun Li <[email protected]> wrote:
> >> > > John Stultz <[email protected]> 于2019年10月30日周三 上午5:18写道：
> >> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <[email protected]> wrote:
> >> > > > > John Stultz <[email protected]> writes:
> >> > > > > > From: Yu Chen <[email protected]>
> >> > > > > >
> >> > > > > > 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
Hi,
Jun Li <[email protected]> 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
> -----Original Message-----
> From: Felipe Balbi <[email protected]> On Behalf Of Felipe Balbi
> Sent: 2020??5??15?? 17:31
> To: Jun Li <[email protected]>
> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
> <[email protected]>; Heikki Krogerus <[email protected]>;
> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
> <[email protected]>; Valentin Schneider <[email protected]>;
> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
> Peter Chen <[email protected]>; Jun Li <[email protected]>; Thinh Nguyen
> <[email protected]>
> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
>
>
> Hi,
>
> Jun Li <[email protected]> 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
Hi,
Jun Li <[email protected]> writes:
>> Jun Li <[email protected]> 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
Hi,
Jun Li wrote:
>> -----Original Message-----
>> From: Felipe Balbi <[email protected]> On Behalf Of Felipe Balbi
>> Sent: 2020??5??15?? 17:31
>> To: Jun Li <[email protected]>
>> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
>> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
>> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
>> <[email protected]>; Heikki Krogerus <[email protected]>;
>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
>> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
>> <[email protected]>; Valentin Schneider <[email protected]>;
>> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
>> Peter Chen <[email protected]>; Jun Li <[email protected]>; Thinh Nguyen
>> <[email protected]>
>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>> controller
>>
>>
>> Hi,
>>
>> Jun Li <[email protected]> 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
Hi,
Thinh Nguyen <[email protected]> writes:
> Jun Li wrote:
>>> -----Original Message-----
>>> From: Felipe Balbi <[email protected]> On Behalf Of Felipe Balbi
>>> Sent: 2020年5月15日 17:31
>>> To: Jun Li <[email protected]>
>>> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
>>> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
>>> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
>>> <[email protected]>; Heikki Krogerus <[email protected]>;
>>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
>>> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
>>> <[email protected]>; Valentin Schneider <[email protected]>;
>>> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
>>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
>>> Peter Chen <[email protected]>; Jun Li <[email protected]>; Thinh Nguyen
>>> <[email protected]>
>>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>>> controller
>>>
>>>
>>> Hi,
>>>
>>> Jun Li <[email protected]> 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
Hi,
> -----Original Message-----
> From: Felipe Balbi <[email protected]> On Behalf Of Felipe Balbi
> Sent: 2020年5月16日 15:13
> To: Thinh Nguyen <[email protected]>; Jun Li <[email protected]>; Jun Li
> <[email protected]>
> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
> <[email protected]>; Heikki Krogerus <[email protected]>;
> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
> <[email protected]>; Valentin Schneider <[email protected]>;
> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
> Peter Chen <[email protected]>; Thinh Nguyen <[email protected]>
> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
>
>
> Hi,
>
> Thinh Nguyen <[email protected]> writes:
> > Jun Li wrote:
> >>> -----Original Message-----
> >>> From: Felipe Balbi <[email protected]> On Behalf Of Felipe Balbi
> >>> Sent: 2020年5月15日 17:31
> >>> To: Jun Li <[email protected]>
> >>> Cc: John Stultz <[email protected]>; lkml
> >>> <[email protected]>; Yu Chen <[email protected]>; Greg
> >>> Kroah-Hartman <[email protected]>; Rob Herring
> >>> <[email protected]>; Mark Rutland <[email protected]>; ShuFan
> >>> Lee <[email protected]>; Heikki Krogerus
> >>> <[email protected]>;
> >>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> >>> <[email protected]>; Hans de Goede <[email protected]>;
> >>> Andy Shevchenko <[email protected]>; Valentin Schneider
> >>> <[email protected]>; Jack Pham <[email protected]>;
> >>> Linux USB List <[email protected]>; open list:OPEN FIRMWARE
> >>> AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
> >>> Peter Chen <[email protected]>; Jun Li <[email protected]>; Thinh
> >>> Nguyen <[email protected]>
> >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
> >>> cleared by device controller
> >>>
> >>>
> >>> Hi,
> >>>
> >>> Jun Li <[email protected]> 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
Hi,
Jun Li <[email protected]> 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
> -----Original Message-----
> From: Felipe Balbi <[email protected]> On Behalf Of Felipe Balbi
> Sent: 2020??5??16?? 19:57
> To: Jun Li <[email protected]>; Thinh Nguyen <[email protected]>; Jun Li
> <[email protected]>
> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
> <[email protected]>; Heikki Krogerus <[email protected]>;
> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
> <[email protected]>; Valentin Schneider <[email protected]>;
> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
> Peter Chen <[email protected]>; Thinh Nguyen <[email protected]>
> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
>
>
> Hi,
>
> Jun Li <[email protected]> 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
Jun Li wrote:
>> -----Original Message-----
>> From: Felipe Balbi <[email protected]> On Behalf Of Felipe Balbi
>> Sent: 2020??5??16?? 19:57
>> To: Jun Li <[email protected]>; Thinh Nguyen <[email protected]>; Jun Li
>> <[email protected]>
>> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
>> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
>> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
>> <[email protected]>; Heikki Krogerus <[email protected]>;
>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
>> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
>> <[email protected]>; Valentin Schneider <[email protected]>;
>> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
>> Peter Chen <[email protected]>; Thinh Nguyen <[email protected]>
>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>> controller
>>
>>
>> Hi,
>>
>> Jun Li <[email protected]> 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
Thinh Nguyen wrote:
> Jun Li wrote:
>>> -----Original Message-----
>>> From: Felipe Balbi <[email protected]> On Behalf Of Felipe Balbi
>>> Sent: 2020??5??16?? 19:57
>>> To: Jun Li <[email protected]>; Thinh Nguyen <[email protected]>; Jun Li
>>> <[email protected]>
>>> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
>>> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
>>> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
>>> <[email protected]>; Heikki Krogerus <[email protected]>;
>>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
>>> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
>>> <[email protected]>; Valentin Schneider <[email protected]>;
>>> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
>>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
>>> Peter Chen <[email protected]>; Thinh Nguyen <[email protected]>
>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
>>> controller
>>>
>>>
>>> Hi,
>>>
>>> Jun Li <[email protected]> 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
Hi
> -----Original Message-----
> From: Thinh Nguyen <[email protected]>
> Sent: 2020??5??19?? 14:46
> To: Jun Li <[email protected]>; Felipe Balbi <[email protected]>; Jun Li
> <[email protected]>
> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
> <[email protected]>; Heikki Krogerus <[email protected]>;
> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
> <[email protected]>; Valentin Schneider <[email protected]>;
> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
> Peter Chen <[email protected]>
> 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 <[email protected]> On Behalf Of Felipe Balbi
> >>> Sent: 2020??5??16?? 19:57
> >>> To: Jun Li <[email protected]>; Thinh Nguyen
> >>> <[email protected]>; Jun Li <[email protected]>
> >>> Cc: John Stultz <[email protected]>; lkml
> >>> <[email protected]>; Yu Chen <[email protected]>; Greg
> >>> Kroah-Hartman <[email protected]>; Rob Herring
> >>> <[email protected]>; Mark Rutland <[email protected]>; ShuFan
> >>> Lee <[email protected]>; Heikki Krogerus
> >>> <[email protected]>;
> >>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> >>> <[email protected]>; Hans de Goede <[email protected]>;
> >>> Andy Shevchenko <[email protected]>; Valentin Schneider
> >>> <[email protected]>; Jack Pham <[email protected]>;
> >>> Linux USB List <[email protected]>; open list:OPEN FIRMWARE
> >>> AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
> >>> Peter Chen <[email protected]>; Thinh Nguyen
> >>> <[email protected]>
> >>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
> >>> cleared by device controller
> >>>
> >>>
> >>> Hi,
> >>>
> >>> Jun Li <[email protected]> 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
Jun Li wrote:
> Hi
>
>> -----Original Message-----
>> From: Thinh Nguyen <[email protected]>
>> Sent: 2020年5月19日 14:46
>> To: Jun Li <[email protected]>; Felipe Balbi <[email protected]>; Jun Li
>> <[email protected]>
>> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
>> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
>> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
>> <[email protected]>; Heikki Krogerus <[email protected]>;
>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
>> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
>> <[email protected]>; Valentin Schneider <[email protected]>;
>> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
>> Peter Chen <[email protected]>
>> 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 <[email protected]> On Behalf Of Felipe Balbi
>>>>> Sent: 2020年5月16日 19:57
>>>>> To: Jun Li <[email protected]>; Thinh Nguyen
>>>>> <[email protected]>; Jun Li <[email protected]>
>>>>> Cc: John Stultz <[email protected]>; lkml
>>>>> <[email protected]>; Yu Chen <[email protected]>; Greg
>>>>> Kroah-Hartman <[email protected]>; Rob Herring
>>>>> <[email protected]>; Mark Rutland <[email protected]>; ShuFan
>>>>> Lee <[email protected]>; Heikki Krogerus
>>>>> <[email protected]>;
>>>>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
>>>>> <[email protected]>; Hans de Goede <[email protected]>;
>>>>> Andy Shevchenko <[email protected]>; Valentin Schneider
>>>>> <[email protected]>; Jack Pham <[email protected]>;
>>>>> Linux USB List <[email protected]>; open list:OPEN FIRMWARE
>>>>> AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
>>>>> Peter Chen <[email protected]>; Thinh Nguyen
>>>>> <[email protected]>
>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
>>>>> cleared by device controller
>>>>>
>>>>>
>>>>> Hi,
>>>>>
>>>>> Jun Li <[email protected]> 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
Thinh Nguyen wrote:
> Jun Li wrote:
>> Hi
>>
>>> -----Original Message-----
>>> From: Thinh Nguyen <[email protected]>
>>> Sent: 2020年5月19日 14:46
>>> To: Jun Li <[email protected]>; Felipe Balbi <[email protected]>; Jun Li
>>> <[email protected]>
>>> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
>>> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
>>> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
>>> <[email protected]>; Heikki Krogerus <[email protected]>;
>>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
>>> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
>>> <[email protected]>; Valentin Schneider <[email protected]>;
>>> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
>>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
>>> Peter Chen <[email protected]>
>>> 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 <[email protected]> On Behalf Of Felipe Balbi
>>>>>> Sent: 2020年5月16日 19:57
>>>>>> To: Jun Li <[email protected]>; Thinh Nguyen
>>>>>> <[email protected]>; Jun Li <[email protected]>
>>>>>> Cc: John Stultz <[email protected]>; lkml
>>>>>> <[email protected]>; Yu Chen <[email protected]>; Greg
>>>>>> Kroah-Hartman <[email protected]>; Rob Herring
>>>>>> <[email protected]>; Mark Rutland <[email protected]>; ShuFan
>>>>>> Lee <[email protected]>; Heikki Krogerus
>>>>>> <[email protected]>;
>>>>>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
>>>>>> <[email protected]>; Hans de Goede <[email protected]>;
>>>>>> Andy Shevchenko <[email protected]>; Valentin Schneider
>>>>>> <[email protected]>; Jack Pham <[email protected]>;
>>>>>> Linux USB List <[email protected]>; open list:OPEN FIRMWARE
>>>>>> AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
>>>>>> Peter Chen <[email protected]>; Thinh Nguyen
>>>>>> <[email protected]>
>>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct
>>>>>> cleared by device controller
>>>>>>
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> Jun Li <[email protected]> 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
Hi,
Thinh Nguyen <[email protected]> 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
Hi Jun,
Felipe Balbi <[email protected]> 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
Hi Thinh,
> -----Original Message-----
> From: Thinh Nguyen <[email protected]>
> Sent: 2020年5月21日 9:56
> To: Jun Li <[email protected]>; Felipe Balbi <[email protected]>; Jun Li
> <[email protected]>
> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
> <[email protected]>; Heikki Krogerus <[email protected]>;
> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
> <[email protected]>; Valentin Schneider <[email protected]>;
> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
> Peter Chen <[email protected]>
> 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 <[email protected]>
> >>> Sent: 2020年5月19日 14:46
> >>> To: Jun Li <[email protected]>; Felipe Balbi <[email protected]>; Jun Li
> >>> <[email protected]>
> >>> Cc: John Stultz <[email protected]>; lkml
> >>> <[email protected]>; Yu Chen <[email protected]>; Greg
> >>> Kroah-Hartman <[email protected]>; Rob Herring
> >>> <[email protected]>; Mark Rutland <[email protected]>; ShuFan
> >>> Lee <[email protected]>; Heikki Krogerus
> >>> <[email protected]>;
> >>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> >>> <[email protected]>; Hans de Goede <[email protected]>;
> >>> Andy Shevchenko <[email protected]>; Valentin Schneider
> >>> <[email protected]>; Jack Pham <[email protected]>;
> >>> Linux USB List <[email protected]>; open list:OPEN FIRMWARE
> >>> AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
> >>> Peter Chen <[email protected]>
> >>> 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 <[email protected]> On Behalf Of Felipe Balbi
> >>>>>> Sent: 2020年5月16日 19:57
> >>>>>> To: Jun Li <[email protected]>; Thinh Nguyen
> >>>>>> <[email protected]>; Jun Li <[email protected]>
> >>>>>> Cc: John Stultz <[email protected]>; lkml
> >>>>>> <[email protected]>; Yu Chen <[email protected]>;
> >>>>>> Greg Kroah-Hartman <[email protected]>; Rob Herring
> >>>>>> <[email protected]>; Mark Rutland <[email protected]>; ShuFan
> >>>>>> Lee <[email protected]>; Heikki Krogerus
> >>>>>> <[email protected]>;
> >>>>>> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> >>>>>> <[email protected]>; Hans de Goede <[email protected]>;
> >>>>>> Andy Shevchenko <[email protected]>; Valentin Schneider
> >>>>>> <[email protected]>; Jack Pham <[email protected]>;
> >>>>>> Linux USB List <[email protected]>; open list:OPEN
> >>>>>> FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> >>>>>> <[email protected]>; Peter Chen <[email protected]>;
> >>>>>> Thinh Nguyen <[email protected]>
> >>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for
> >>>>>> CmdAct cleared by device controller
> >>>>>>
> >>>>>>
> >>>>>> Hi,
> >>>>>>
> >>>>>> Jun Li <[email protected]> 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
Hi Felipe,
> -----Original Message-----
> From: Felipe Balbi <[email protected]> On Behalf Of Felipe Balbi
> Sent: 2020??5??21?? 14:23
> To: Thinh Nguyen <[email protected]>; Jun Li <[email protected]>; Jun Li
> <[email protected]>
> Cc: John Stultz <[email protected]>; lkml <[email protected]>; Yu
> Chen <[email protected]>; Greg Kroah-Hartman <[email protected]>; Rob
> Herring <[email protected]>; Mark Rutland <[email protected]>; ShuFan Lee
> <[email protected]>; Heikki Krogerus <[email protected]>;
> Suzuki K Poulose <[email protected]>; Chunfeng Yun
> <[email protected]>; Hans de Goede <[email protected]>; Andy Shevchenko
> <[email protected]>; Valentin Schneider <[email protected]>;
> Jack Pham <[email protected]>; Linux USB List <[email protected]>; open
> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <[email protected]>;
> Peter Chen <[email protected]>
> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device
> controller
>
>
> Hi Jun,
>
> Felipe Balbi <[email protected]> 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