[PATCH v1 0/8] enable usb support on rk356x

[PATCH v1 0/8] enable usb support on rk356x

[Peter Geis]

Good Morning,

This is my patch series that I have maintained out of tree until the
combophy driver landed. Note, it is dependent on this series to function
(aside from the rk3566 dwc3-otg port, which will work currently).

This series is still dependent on Patch 4 of the combophy series, which
has yet to be accepted.

Patches 1 and 2 add the dt bindings for the grf changes necessary for
this series.
Patch 3 adds support to the grf driver to set the rk3566 otg clock
source.
Patch 4 is a downstream patch ported forward to shut down the usb3 clock
when the controller is operating in usb2 mode.
Patches 5 and 6 clean up the dwc3-of-simple driver and add the
compatible for the rk3568.
Patch 7 adds the dwc3 nodes to the rk356x device tree includes.
Patch 8 enables the dwc3 nodes on the Quartz64 Model A.

Please review and apply.

Very Respectfully,
Peter Geis

Bin Yang (1):
  usb: dwc3: core: do not use 3.0 clock when operating in 2.0 mode

Peter Geis (7):
  dt-bindings: soc: grf: add rk3566-pipe-grf compatible
  dt-bindings: usb: dwc3: add description for rk3568
  soc: rockchip: set dwc3 clock for rk3566
  usb: dwc3: reorder dwc-of-simple compatibles
  usb: dwc3: add rk3568 dwc3 support
  arm64: dts: rockchip: add the dwc3 usb3 nodes to rk356x
  arm64: dts: rockchip: enable the dwc3 nodes on quartz64-a

 .../devicetree/bindings/soc/rockchip/grf.yaml |  1 +
 .../bindings/usb/rockchip,dwc3.yaml           |  7 ++-
 .../boot/dts/rockchip/rk3566-quartz64-a.dts   | 37 +++++++++++++++
 arch/arm64/boot/dts/rockchip/rk3566.dtsi      | 12 +++++
 arch/arm64/boot/dts/rockchip/rk3568.dtsi      |  9 ++++
 arch/arm64/boot/dts/rockchip/rk356x.dtsi      | 45 ++++++++++++++++++-
 drivers/soc/rockchip/grf.c                    | 17 +++++++
 drivers/usb/dwc3/core.c                       |  4 ++
 drivers/usb/dwc3/core.h                       |  1 +
 drivers/usb/dwc3/dwc3-of-simple.c             | 10 +++--
 10 files changed, 137 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH v1 1/8] dt-bindings: soc: grf: add rk3566-pipe-grf compatible

[Peter Geis]

The rk3566 requires special handling for the dwc3-otg clock in order for
the port to function correctly.
Add a binding for the rk3566-pipe-grf so we can handle setup with the
grf driver.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 Documentation/devicetree/bindings/soc/rockchip/grf.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/soc/rockchip/grf.yaml b/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
index 5079e9d24af6..75a2b8bb25fb 100644
--- a/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
+++ b/Documentation/devicetree/bindings/soc/rockchip/grf.yaml
@@ -15,6 +15,7 @@ properties:
       - items:
           - enum:
               - rockchip,rk3288-sgrf
+              - rockchip,rk3566-pipe-grf
               - rockchip,rk3568-pipe-grf
               - rockchip,rk3568-pipe-phy-grf
               - rockchip,rk3568-usb2phy-grf
-- 
2.25.1

[PATCH v1 2/8] dt-bindings: usb: dwc3: add description for rk3568

[Peter Geis]

The rk3568 dwc3 controllers are backwards compatible with the rk3399.
Add the device tree description for it.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
index 04077f2d7faf..e3044e81cc72 100644
--- a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
@@ -30,6 +30,7 @@ select:
         enum:
           - rockchip,rk3328-dwc3
           - rockchip,rk3399-dwc3
+          - rockchip,rk3568-dwc3
   required:
     - compatible
 
@@ -39,6 +40,7 @@ properties:
       - enum:
           - rockchip,rk3328-dwc3
           - rockchip,rk3399-dwc3
+          - rockchip,rk3568-dwc3
       - const: snps,dwc3
 
   reg:
@@ -75,7 +77,10 @@ properties:
     maxItems: 1
 
   reset-names:
-    const: usb3-otg
+    items:
+      - enum:
+          - usb3-otg
+          - usb3-host
 
 unevaluatedProperties: false
 
-- 
2.25.1

[PATCH v1 3/8] soc: rockchip: set dwc3 clock for rk3566

[Peter Geis]

The rk3566 dwc3 otg port clock is unavailable at boot, as it defaults to
the combophy as the clock source. As combophy0 doesn't exist on rk3566,
we need to set the clock source to the usb2 phy instead.

Add handling to the grf driver to handle this on boot.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 drivers/soc/rockchip/grf.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/soc/rockchip/grf.c b/drivers/soc/rockchip/grf.c
index 494cf2b5bf7b..384461b70684 100644
--- a/drivers/soc/rockchip/grf.c
+++ b/drivers/soc/rockchip/grf.c
@@ -108,6 +108,20 @@ static const struct rockchip_grf_info rk3399_grf __initconst = {
 	.num_values = ARRAY_SIZE(rk3399_defaults),
 };
 
+#define RK3566_GRF_USB3OTG0_CON1	0x0104
+
+static const struct rockchip_grf_value rk3566_defaults[] __initconst = {
+	{ "usb3otg port switch", RK3566_GRF_USB3OTG0_CON1, HIWORD_UPDATE(0, 1, 12) },
+	{ "usb3otg clock switch", RK3566_GRF_USB3OTG0_CON1, HIWORD_UPDATE(1, 1, 7) },
+	{ "usb3otg disable usb3", RK3566_GRF_USB3OTG0_CON1, HIWORD_UPDATE(1, 1, 0) },
+};
+
+static const struct rockchip_grf_info rk3566_pipegrf __initconst = {
+	.values = rk3566_defaults,
+	.num_values = ARRAY_SIZE(rk3566_defaults),
+};
+
+
 static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
 	{
 		.compatible = "rockchip,rk3036-grf",
@@ -130,6 +144,9 @@ static const struct of_device_id rockchip_grf_dt_match[] __initconst = {
 	}, {
 		.compatible = "rockchip,rk3399-grf",
 		.data = (void *)&rk3399_grf,
+	}, {
+		.compatible = "rockchip,rk3566-pipe-grf",
+		.data = (void *)&rk3566_pipegrf,
 	},
 	{ /* sentinel */ },
 };
-- 
2.25.1

[PATCH v1 4/8] usb: dwc3: core: do not use 3.0 clock when operating in 2.0 mode

[Peter Geis]

From: Bin Yang <yangbin@rock-chips.com>

In the 3.0 device core, if the core is programmed to operate in
2.0 only, then setting the GUCTL1.DEV_FORCE_20_CLK_FOR_30_CLK makes
the internal 2.0(utmi/ulpi) clock to be routed as the 3.0 (pipe)
clock. Enabling this feature allows the pipe3 clock to be not-running
when forcibly operating in 2.0 device mode.

Signed-off-by: Bin Yang <yangbin@rock-chips.com>
Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 drivers/usb/dwc3/core.c | 4 ++++
 drivers/usb/dwc3/core.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 18adddfba3da..032d40794fae 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1167,6 +1167,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
 		if (dwc->parkmode_disable_ss_quirk)
 			reg |= DWC3_GUCTL1_PARKMODE_DISABLE_SS;
 
+		if (dwc->maximum_speed == USB_SPEED_HIGH ||
+		    dwc->maximum_speed == USB_SPEED_FULL)
+			reg |= DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK;
+
 		dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
 	}
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index eb9c1efced05..ea3ca04406bb 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -259,6 +259,7 @@
 /* Global User Control 1 Register */
 #define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT	BIT(31)
 #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
+#define DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK	BIT(26)
 #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW		BIT(24)
 #define DWC3_GUCTL1_PARKMODE_DISABLE_SS		BIT(17)
 
-- 
2.25.1

[PATCH v1 5/8] usb: dwc3: reorder dwc-of-simple compatibles

[Peter Geis]

The dwc3-of-simple driver is getting rather disorganized with the new
inclusions.
Reorder the dwc3-of-simple compatibles to be alphabetical.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index 71fd620c5161..9dc6295df6b1 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -171,12 +171,12 @@ static const struct dev_pm_ops dwc3_of_simple_dev_pm_ops = {
 };
 
 static const struct of_device_id of_dwc3_simple_match[] = {
-	{ .compatible = "rockchip,rk3399-dwc3" },
-	{ .compatible = "cavium,octeon-7130-usb-uctl" },
-	{ .compatible = "sprd,sc9860-dwc3" },
 	{ .compatible = "allwinner,sun50i-h6-dwc3" },
+	{ .compatible = "cavium,octeon-7130-usb-uctl" },
 	{ .compatible = "hisilicon,hi3670-dwc3" },
 	{ .compatible = "intel,keembay-dwc3" },
+	{ .compatible = "rockchip,rk3399-dwc3" },
+	{ .compatible = "sprd,sc9860-dwc3" },
 	{ /* Sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, of_dwc3_simple_match);
-- 
2.25.1

[PATCH v1 6/8] usb: dwc3: add rk3568 dwc3 support

[Peter Geis]

The rk3568 dwc3 controller is backwards compatible with the rk3399 dwc3
controller.
Add support for it to the dwc3-of-simple driver.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index 9dc6295df6b1..1d52a261af55 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -49,7 +49,8 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 	 * Some controllers need to toggle the usb3-otg reset before trying to
 	 * initialize the PHY, otherwise the PHY times out.
 	 */
-	if (of_device_is_compatible(np, "rockchip,rk3399-dwc3"))
+	if (of_device_is_compatible(np, "rockchip,rk3399-dwc3") |
+	    of_device_is_compatible(np, "rockchip,rk3568-dwc3"))
 		simple->need_reset = true;
 
 	simple->resets = of_reset_control_array_get(np, false, true,
@@ -176,6 +177,7 @@ static const struct of_device_id of_dwc3_simple_match[] = {
 	{ .compatible = "hisilicon,hi3670-dwc3" },
 	{ .compatible = "intel,keembay-dwc3" },
 	{ .compatible = "rockchip,rk3399-dwc3" },
+	{ .compatible = "rockchip,rk3568-dwc3" },
 	{ .compatible = "sprd,sc9860-dwc3" },
 	{ /* Sentinel */ }
 };
-- 
2.25.1

[PATCH v1 7/8] arm64: dts: rockchip: add rk356x dwc3 usb3 nodes

[Peter Geis]

Add the dwc3 device nodes to the rk356x device trees.
The rk3566 has one usb2 capable dwc3 otg controller and one usb3 capable
dwc3 host controller.
The rk3568 has one usb3 capable dwc3 otg controller and one usb3 capable
dwc3 host controller.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 arch/arm64/boot/dts/rockchip/rk3566.dtsi | 12 +++++++
 arch/arm64/boot/dts/rockchip/rk3568.dtsi |  9 +++++
 arch/arm64/boot/dts/rockchip/rk356x.dtsi | 45 +++++++++++++++++++++++-
 3 files changed, 65 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/rockchip/rk3566.dtsi b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
index 3839eef5e4f7..8e8b52f58f44 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
@@ -6,6 +6,10 @@ / {
 	compatible = "rockchip,rk3566";
 };
 
+&pipegrf {
+	compatible = "rockchip,rk3566-pipe-grf", "syscon";
+};
+
 &power {
 	power-domain@RK3568_PD_PIPE {
 		reg = <RK3568_PD_PIPE>;
@@ -18,3 +22,11 @@ power-domain@RK3568_PD_PIPE {
 		#power-domain-cells = <0>;
 	};
 };
+
+&usbdrd30 {
+	phys = <&usb2phy0_otg>;
+	phy-names = "usb2-phy";
+	extcon = <&usb2phy0>;
+	maximum-speed = "high-speed";
+	snps,dis_u2_susphy_quirk;
+};
diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
index 5b0f528d6818..77c044cbaaad 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
@@ -99,6 +99,10 @@ opp-1992000000 {
 	};
 };
 
+&pipegrf {
+	compatible = "rockchip,rk3568-pipe-grf", "syscon";
+};
+
 &power {
 	power-domain@RK3568_PD_PIPE {
 		reg = <RK3568_PD_PIPE>;
@@ -114,3 +118,8 @@ power-domain@RK3568_PD_PIPE {
 		#power-domain-cells = <0>;
 	};
 };
+
+&usbdrd30 {
+	phys = <&usb2phy0_otg>, <&combphy0 PHY_TYPE_USB3>;
+	phy-names = "usb2-phy", "usb3-phy";
+};
diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
index 84d5d607e693..4fae5b3b326e 100644
--- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
@@ -230,6 +230,50 @@ scmi_shmem: sram@0 {
 		};
 	};
 
+	usbdrd30: usbdrd@fcc00000 {
+		compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
+		reg = <0x0 0xfcc00000 0x0 0x400000>;
+		interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru CLK_USB3OTG0_REF>, <&cru CLK_USB3OTG0_SUSPEND>,
+			 <&cru ACLK_USB3OTG0>, <&cru PCLK_PIPE>;
+		clock-names = "ref_clk", "suspend_clk",
+			      "bus_clk", "grf_clk";
+		dr_mode = "host";
+		phy_type = "utmi_wide";
+		power-domains = <&power RK3568_PD_PIPE>;
+		resets = <&cru SRST_USB3OTG0>;
+		reset-names = "usb3-otg";
+		snps,dis_enblslpm_quirk;
+		snps,dis-u2-freeclk-exists-quirk;
+		snps,dis-del-phy-power-chg-quirk;
+		snps,dis-tx-ipgap-linecheck-quirk;
+		snps,xhci-trb-ent-quirk;
+		status = "disabled";
+	};
+
+	usbhost30: usbhost@fd000000 {
+		compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
+		reg = <0x0 0xfd000000 0x0 0x400000>;
+		interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>,
+			 <&cru ACLK_USB3OTG1>, <&cru PCLK_PIPE>;
+		clock-names = "ref_clk", "suspend_clk",
+			      "bus_clk", "grf_clk";
+		dr_mode = "host";
+		phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>;
+		phy-names = "usb2-phy", "usb3-phy";
+		phy_type = "utmi_wide";
+		power-domains = <&power RK3568_PD_PIPE>;
+		resets = <&cru SRST_USB3OTG1>;
+		reset-names = "usb3-host";
+		snps,dis_enblslpm_quirk;
+		snps,dis-u2-freeclk-exists-quirk;
+		snps,dis_u2_susphy_quirk;
+		snps,dis-del-phy-power-chg-quirk;
+		snps,dis-tx-ipgap-linecheck-quirk;
+		status = "disabled";
+	};
+
 	gic: interrupt-controller@fd400000 {
 		compatible = "arm,gic-v3";
 		reg = <0x0 0xfd400000 0 0x10000>, /* GICD */
@@ -297,7 +341,6 @@ pmu_io_domains: io-domains {
 	};
 
 	pipegrf: syscon@fdc50000 {
-		compatible = "rockchip,rk3568-pipe-grf", "syscon";
 		reg = <0x0 0xfdc50000 0x0 0x1000>;
 	};
 
-- 
2.25.1

[PATCH v1 8/8] arm64: dts: rockchip: enable dwc3 on quartz64-a

[Peter Geis]

The quartz64 model a has support for both the dwc3 otg port and the dwc3
host port. Add the otg power supply and dwc3 nodes to the device tree to
enable support for these.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---
 .../boot/dts/rockchip/rk3566-quartz64-a.dts   | 37 +++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
index dd7f4b9b686b..a911fa1ef955 100644
--- a/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3566-quartz64-a.dts
@@ -151,6 +151,16 @@ vcc5v0_usb20_host: vcc5v0_usb20_host {
 		vin-supply = <&vcc5v0_usb>;
 	};
 
+	vcc5v0_usb20_otg: vcc5v0_usb20_otg {
+		compatible = "regulator-fixed";
+		enable-active-high;
+		gpio = <&gpio4 RK_PB5 GPIO_ACTIVE_HIGH>;
+		regulator-name = "vcc5v0_usb20_otg";
+		regulator-min-microvolt = <5000000>;
+		regulator-max-microvolt = <5000000>;
+		vin-supply = <&dcdc_boost>;
+	};
+
 	vcc3v3_sd: vcc3v3_sd {
 		compatible = "regulator-fixed";
 		enable-active-low;
@@ -187,6 +197,10 @@ vcc_wl: vcc_wl {
 	};
 };
 
+&combphy1 {
+	status = "okay";
+};
+
 &cpu0 {
 	cpu-supply = <&vdd_cpu>;
 };
@@ -672,6 +686,20 @@ &usb_host1_ohci {
 	status = "okay";
 };
 
+&usb2phy0 {
+	status = "okay";
+};
+
+&usb2phy0_host {
+	phy-supply = <&vcc5v0_usb20_host>;
+	status = "okay";
+};
+
+&usb2phy0_otg {
+	phy-supply = <&vcc5v0_usb20_otg>;
+	status = "okay";
+};
+
 &usb2phy1 {
 	status = "okay";
 };
@@ -685,3 +713,12 @@ &usb2phy1_otg {
 	phy-supply = <&vcc5v0_usb20_host>;
 	status = "okay";
 };
+
+&usbdrd30 {
+	status = "okay";
+};
+
+/* usb3 controller is muxed with sata1 */
+&usbhost30 {
+	status = "okay";
+};
-- 
2.25.1

Re: [PATCH v1 2/8] dt-bindings: usb: dwc3: add description for rk3568

[Johan Jonker]

Hi Peter,

Lots of USB series all of a sudden.
Combine possible?

On 2/25/22 15:54, Peter Geis wrote:
> The rk3568 dwc3 controllers are backwards compatible with the rk3399.
> Add the device tree description for it.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
> index 04077f2d7faf..e3044e81cc72 100644
> --- a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
> @@ -30,6 +30,7 @@ select:
>          enum:
>            - rockchip,rk3328-dwc3
>            - rockchip,rk3399-dwc3
> +          - rockchip,rk3568-dwc3
>    required:
>      - compatible
>  
> @@ -39,6 +40,7 @@ properties:
>        - enum:
>            - rockchip,rk3328-dwc3
>            - rockchip,rk3399-dwc3
> +          - rockchip,rk3568-dwc3
>        - const: snps,dwc3
>  
>    reg:
> @@ -75,7 +77,10 @@ properties:
>      maxItems: 1
>  
he
>    reset-names:
> -    const: usb3-otg
> +    items:
> +      - enum:
> +          - usb3-otg
> +          - usb3-host

The use of reset-names is "sort of" only related to the rk3399 legacy
node. Still using this sub node DT to not to break older existing boot
loaders.

https://github.com/torvalds/linux/search?q=usb3-otg

It's only mentioned as comment in dwc3-of-simple.c but not used:

 	simple->resets = of_reset_control_array_get(np, false, true,
						    true);
core.c uses something similar.

	dwc->reset = devm_reset_control_array_get_optional_shared(dev);
	if (IS_ERR(dwc->reset))
		return PTR_ERR(dwc->reset);


Up to the maintainers, but I wouldn't add another variant/name for the
same thing as it also optional(= not required) and no longer needed.

Johan

===

Maybe drop PCLK_PIPE as well to reduce notifications.

See example:
https://lore.kernel.org/linux-rockchip/20220225131602.2283499-4-michael.riesch@wolfvision.net/T/#u

>  
>  unevaluatedProperties: false
>  

Re: [PATCH v1 7/8] arm64: dts: rockchip: add rk356x dwc3 usb3 nodes

[Michael Riesch]

Hi Peter,

(It should be noted that there was a slight mishap in communications
between the two of us resulting in two series with the same goal. Now
let's clean up the mess :-)

Thanks for your series. Seeing that it contains more patches than mine
it probably makes sense to use your series as basis. Please Cc: me in
future iterations of this patch series and consider my comments below.

On 2/25/22 15:54, Peter Geis wrote:
> Add the dwc3 device nodes to the rk356x device trees.
> The rk3566 has one usb2 capable dwc3 otg controller and one usb3 capable
> dwc3 host controller.
> The rk3568 has one usb3 capable dwc3 otg controller and one usb3 capable
> dwc3 host controller.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>  arch/arm64/boot/dts/rockchip/rk3566.dtsi | 12 +++++++
>  arch/arm64/boot/dts/rockchip/rk3568.dtsi |  9 +++++
>  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 45 +++++++++++++++++++++++-
>  3 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3566.dtsi b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> index 3839eef5e4f7..8e8b52f58f44 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> @@ -6,6 +6,10 @@ / {
>  	compatible = "rockchip,rk3566";
>  };
>  
> +&pipegrf {
> +	compatible = "rockchip,rk3566-pipe-grf", "syscon";
> +};
> +
>  &power {
>  	power-domain@RK3568_PD_PIPE {
>  		reg = <RK3568_PD_PIPE>;
> @@ -18,3 +22,11 @@ power-domain@RK3568_PD_PIPE {
>  		#power-domain-cells = <0>;
>  	};
>  };
> +
> +&usbdrd30 {

I would really love to have some alignment with the other USB controllers

usb_host{0,1}_{e,o}hci

here. I am aware that older SoCs and the SDK are using these names and
it might be painful to have different versions to maintain at the
moment, but can we please agree on

usb_host0_xhci
usb_host1_xhci

or something like that?

> +	phys = <&usb2phy0_otg>;
> +	phy-names = "usb2-phy";
> +	extcon = <&usb2phy0>;
> +	maximum-speed = "high-speed";
> +	snps,dis_u2_susphy_quirk;
> +};
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> index 5b0f528d6818..77c044cbaaad 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> @@ -99,6 +99,10 @@ opp-1992000000 {
>  	};
>  };
>  
> +&pipegrf {
> +	compatible = "rockchip,rk3568-pipe-grf", "syscon";
> +};
> +
>  &power {
>  	power-domain@RK3568_PD_PIPE {
>  		reg = <RK3568_PD_PIPE>;
> @@ -114,3 +118,8 @@ power-domain@RK3568_PD_PIPE {
>  		#power-domain-cells = <0>;
>  	};
>  };
> +
> +&usbdrd30 {
> +	phys = <&usb2phy0_otg>, <&combphy0 PHY_TYPE_USB3>;
> +	phy-names = "usb2-phy", "usb3-phy";
> +};
> diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> index 84d5d607e693..4fae5b3b326e 100644
> --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> @@ -230,6 +230,50 @@ scmi_shmem: sram@0 {
>  		};
>  	};
>  
> +	usbdrd30: usbdrd@fcc00000 {
> +		compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
> +		reg = <0x0 0xfcc00000 0x0 0x400000>;
> +		interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru CLK_USB3OTG0_REF>, <&cru CLK_USB3OTG0_SUSPEND>,
> +			 <&cru ACLK_USB3OTG0>, <&cru PCLK_PIPE>;
> +		clock-names = "ref_clk", "suspend_clk",
> +			      "bus_clk", "grf_clk";

Please consider Johan's comments on my first series. In my tests
removing the PCLK_PIPE clock did not make any difference.

> +		dr_mode = "host";

Based on the description in the commit message above it should be "otg",
right? Boards are free to overrule this, of course.

> +		phy_type = "utmi_wide";
> +		power-domains = <&power RK3568_PD_PIPE>;
> +		resets = <&cru SRST_USB3OTG0>;
> +		reset-names = "usb3-otg";
> +		snps,dis_enblslpm_quirk;
> +		snps,dis-u2-freeclk-exists-quirk;
> +		snps,dis-del-phy-power-chg-quirk;
> +		snps,dis-tx-ipgap-linecheck-quirk;
> +		snps,xhci-trb-ent-quirk;

In my first version I had all those quirks as well, but are they
actually necessary? I decided to remove them all to have a fresh start
(also activating them did not seem to affect my test setup).

> +		status = "disabled";
> +	};
> +
> +	usbhost30: usbhost@fd000000 {

Please reconsider the this name as well.

> +		compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
> +		reg = <0x0 0xfd000000 0x0 0x400000>;
> +		interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>,
> +			 <&cru ACLK_USB3OTG1>, <&cru PCLK_PIPE>;
> +		clock-names = "ref_clk", "suspend_clk",
> +			      "bus_clk", "grf_clk";
> +		dr_mode = "host";

Here "host" clearly makes sense, as this controller is not capable of otg.

> +		phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>;
> +		phy-names = "usb2-phy", "usb3-phy";
> +		phy_type = "utmi_wide";
> +		power-domains = <&power RK3568_PD_PIPE>;
> +		resets = <&cru SRST_USB3OTG1>;
> +		reset-names = "usb3-host";
> +		snps,dis_enblslpm_quirk;
> +		snps,dis-u2-freeclk-exists-quirk;
> +		snps,dis_u2_susphy_quirk;
> +		snps,dis-del-phy-power-chg-quirk;
> +		snps,dis-tx-ipgap-linecheck-quirk;

What was said about quirks above holds here as well (although one quirk
not documented in the bindings is missing here).

Best regards,
Michael

> +		status = "disabled";
> +	};
> +
>  	gic: interrupt-controller@fd400000 {
>  		compatible = "arm,gic-v3";
>  		reg = <0x0 0xfd400000 0 0x10000>, /* GICD */
> @@ -297,7 +341,6 @@ pmu_io_domains: io-domains {
>  	};
>  
>  	pipegrf: syscon@fdc50000 {
> -		compatible = "rockchip,rk3568-pipe-grf", "syscon";
>  		reg = <0x0 0xfdc50000 0x0 0x1000>;
>  	};
>  

Re: [PATCH v1 6/8] usb: dwc3: add rk3568 dwc3 support

[Michael Riesch]

Hi Peter,

On 2/25/22 15:54, Peter Geis wrote:
> The rk3568 dwc3 controller is backwards compatible with the rk3399 dwc3
> controller.
> Add support for it to the dwc3-of-simple driver.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>  drivers/usb/dwc3/dwc3-of-simple.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> index 9dc6295df6b1..1d52a261af55 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -49,7 +49,8 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  	 * Some controllers need to toggle the usb3-otg reset before trying to
>  	 * initialize the PHY, otherwise the PHY times out.
>  	 */
> -	if (of_device_is_compatible(np, "rockchip,rk3399-dwc3"))
> +	if (of_device_is_compatible(np, "rockchip,rk3399-dwc3") |
> +	    of_device_is_compatible(np, "rockchip,rk3568-dwc3"))
>  		simple->need_reset = true;

Maybe read this value out the match data...

>  	simple->resets = of_reset_control_array_get(np, false, true,
> @@ -176,6 +177,7 @@ static const struct of_device_id of_dwc3_simple_match[] = {
>  	{ .compatible = "hisilicon,hi3670-dwc3" },
>  	{ .compatible = "intel,keembay-dwc3" },
>  	{ .compatible = "rockchip,rk3399-dwc3" },
> +	{ .compatible = "rockchip,rk3568-dwc3" },

... so all future variants that are compatible to the rk3399 can share it?

Best regards,
Michael

>  	{ .compatible = "sprd,sc9860-dwc3" },
>  	{ /* Sentinel */ }
>  };

Re: [PATCH v1 4/8] usb: dwc3: core: do not use 3.0 clock when operating in 2.0 mode

[Jack Pham]

+Thinh

Hi Peter,

On Fri, Feb 25, 2022 at 09:54:27AM -0500, Peter Geis wrote:
> From: Bin Yang <yangbin@rock-chips.com>
> 
> In the 3.0 device core, if the core is programmed to operate in
> 2.0 only, then setting the GUCTL1.DEV_FORCE_20_CLK_FOR_30_CLK makes
> the internal 2.0(utmi/ulpi) clock to be routed as the 3.0 (pipe)
> clock. Enabling this feature allows the pipe3 clock to be not-running
> when forcibly operating in 2.0 device mode.
> 
> Signed-off-by: Bin Yang <yangbin@rock-chips.com>
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
>  drivers/usb/dwc3/core.c | 4 ++++
>  drivers/usb/dwc3/core.h | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 18adddfba3da..032d40794fae 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1167,6 +1167,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  		if (dwc->parkmode_disable_ss_quirk)
>  			reg |= DWC3_GUCTL1_PARKMODE_DISABLE_SS;
>  
> +		if (dwc->maximum_speed == USB_SPEED_HIGH ||
> +		    dwc->maximum_speed == USB_SPEED_FULL)
> +			reg |= DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK;
> +

I doubt this is applicable to all revisions of the DWC_usb3x IP cores?
For instance in the programming guide for DWC_usb31 1.90a bit 26 of
GUCTL1 is 'Reserved'.  While I do see it in the DWC_usb3 databook,
table 4-8 entry "Remove pipe_clk mux for 2.0 mode?" mentions this
feature was only added in v2.90a.

So this setting at least needs a revision check to make sure we're not
causing unexpected behavior.  Something like

	DWC3_VER_IS_WITHIN(DWC3, 290A, ANY)

Jack

>  		dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
>  	}
>  
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index eb9c1efced05..ea3ca04406bb 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -259,6 +259,7 @@
>  /* Global User Control 1 Register */
>  #define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT	BIT(31)
>  #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS	BIT(28)
> +#define DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK	BIT(26)
>  #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW		BIT(24)
>  #define DWC3_GUCTL1_PARKMODE_DISABLE_SS		BIT(17)
>  
> -- 
> 2.25.1
> 

Re: [PATCH v1 4/8] usb: dwc3: core: do not use 3.0 clock when operating in 2.0 mode

[Peter Geis]

On Fri, Feb 25, 2022 at 12:16 PM Jack Pham <quic_jackp@quicinc.com> wrote:
>
> +Thinh
>
> Hi Peter,
>
> On Fri, Feb 25, 2022 at 09:54:27AM -0500, Peter Geis wrote:
> > From: Bin Yang <yangbin@rock-chips.com>
> >
> > In the 3.0 device core, if the core is programmed to operate in
> > 2.0 only, then setting the GUCTL1.DEV_FORCE_20_CLK_FOR_30_CLK makes
> > the internal 2.0(utmi/ulpi) clock to be routed as the 3.0 (pipe)
> > clock. Enabling this feature allows the pipe3 clock to be not-running
> > when forcibly operating in 2.0 device mode.
> >
> > Signed-off-by: Bin Yang <yangbin@rock-chips.com>
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >  drivers/usb/dwc3/core.c | 4 ++++
> >  drivers/usb/dwc3/core.h | 1 +
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 18adddfba3da..032d40794fae 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -1167,6 +1167,10 @@ static int dwc3_core_init(struct dwc3 *dwc)
> >               if (dwc->parkmode_disable_ss_quirk)
> >                       reg |= DWC3_GUCTL1_PARKMODE_DISABLE_SS;
> >
> > +             if (dwc->maximum_speed == USB_SPEED_HIGH ||
> > +                 dwc->maximum_speed == USB_SPEED_FULL)
> > +                     reg |= DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK;
> > +
>
> I doubt this is applicable to all revisions of the DWC_usb3x IP cores?
> For instance in the programming guide for DWC_usb31 1.90a bit 26 of
> GUCTL1 is 'Reserved'.  While I do see it in the DWC_usb3 databook,
> table 4-8 entry "Remove pipe_clk mux for 2.0 mode?" mentions this
> feature was only added in v2.90a.

I was wondering about that, thanks for confirming for me.
Is it possible to get a copy of this programming guide or is it closed?

>
> So this setting at least needs a revision check to make sure we're not
> causing unexpected behavior.  Something like
>
>         DWC3_VER_IS_WITHIN(DWC3, 290A, ANY)

Will do, I appreciate it.

>
> Jack
>
> >               dwc3_writel(dwc->regs, DWC3_GUCTL1, reg);
> >       }
> >
> > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> > index eb9c1efced05..ea3ca04406bb 100644
> > --- a/drivers/usb/dwc3/core.h
> > +++ b/drivers/usb/dwc3/core.h
> > @@ -259,6 +259,7 @@
> >  /* Global User Control 1 Register */
> >  #define DWC3_GUCTL1_DEV_DECOUPLE_L1L2_EVT    BIT(31)
> >  #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS   BIT(28)
> > +#define DWC3_GUCTL1_DEV_FORCE_20_CLK_FOR_30_CLK      BIT(26)
> >  #define DWC3_GUCTL1_DEV_L1_EXIT_BY_HW                BIT(24)
> >  #define DWC3_GUCTL1_PARKMODE_DISABLE_SS              BIT(17)
> >
> > --
> > 2.25.1
> >

Re: [PATCH v1 6/8] usb: dwc3: add rk3568 dwc3 support

[Peter Geis]

On Fri, Feb 25, 2022 at 12:08 PM Michael Riesch
<michael.riesch@wolfvision.net> wrote:
>
> Hi Peter,
>
> On 2/25/22 15:54, Peter Geis wrote:
> > The rk3568 dwc3 controller is backwards compatible with the rk3399 dwc3
> > controller.
> > Add support for it to the dwc3-of-simple driver.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >  drivers/usb/dwc3/dwc3-of-simple.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> > index 9dc6295df6b1..1d52a261af55 100644
> > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > @@ -49,7 +49,8 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
> >        * Some controllers need to toggle the usb3-otg reset before trying to
> >        * initialize the PHY, otherwise the PHY times out.
> >        */
> > -     if (of_device_is_compatible(np, "rockchip,rk3399-dwc3"))
> > +     if (of_device_is_compatible(np, "rockchip,rk3399-dwc3") |
> > +         of_device_is_compatible(np, "rockchip,rk3568-dwc3"))
> >               simple->need_reset = true;
>
> Maybe read this value out the match data...

This is a good idea, thanks!

>
> >       simple->resets = of_reset_control_array_get(np, false, true,
> > @@ -176,6 +177,7 @@ static const struct of_device_id of_dwc3_simple_match[] = {
> >       { .compatible = "hisilicon,hi3670-dwc3" },
> >       { .compatible = "intel,keembay-dwc3" },
> >       { .compatible = "rockchip,rk3399-dwc3" },
> > +     { .compatible = "rockchip,rk3568-dwc3" },
>
> ... so all future variants that are compatible to the rk3399 can share it?
>
> Best regards,
> Michael
>
> >       { .compatible = "sprd,sc9860-dwc3" },
> >       { /* Sentinel */ }
> >  };

Re: [PATCH v1 7/8] arm64: dts: rockchip: add rk356x dwc3 usb3 nodes

[Peter Geis]

On Fri, Feb 25, 2022 at 12:01 PM Michael Riesch
<michael.riesch@wolfvision.net> wrote:
>
> Hi Peter,
>
> (It should be noted that there was a slight mishap in communications
> between the two of us resulting in two series with the same goal. Now
> let's clean up the mess :-)
>
> Thanks for your series. Seeing that it contains more patches than mine
> it probably makes sense to use your series as basis. Please Cc: me in
> future iterations of this patch series and consider my comments below.

Will do.
If you'd like I can also pull your enablement patch.

>
> On 2/25/22 15:54, Peter Geis wrote:
> > Add the dwc3 device nodes to the rk356x device trees.
> > The rk3566 has one usb2 capable dwc3 otg controller and one usb3 capable
> > dwc3 host controller.
> > The rk3568 has one usb3 capable dwc3 otg controller and one usb3 capable
> > dwc3 host controller.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >  arch/arm64/boot/dts/rockchip/rk3566.dtsi | 12 +++++++
> >  arch/arm64/boot/dts/rockchip/rk3568.dtsi |  9 +++++
> >  arch/arm64/boot/dts/rockchip/rk356x.dtsi | 45 +++++++++++++++++++++++-
> >  3 files changed, 65 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3566.dtsi b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> > index 3839eef5e4f7..8e8b52f58f44 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3566.dtsi
> > @@ -6,6 +6,10 @@ / {
> >       compatible = "rockchip,rk3566";
> >  };
> >
> > +&pipegrf {
> > +     compatible = "rockchip,rk3566-pipe-grf", "syscon";
> > +};
> > +
> >  &power {
> >       power-domain@RK3568_PD_PIPE {
> >               reg = <RK3568_PD_PIPE>;
> > @@ -18,3 +22,11 @@ power-domain@RK3568_PD_PIPE {
> >               #power-domain-cells = <0>;
> >       };
> >  };
> > +
> > +&usbdrd30 {
>
> I would really love to have some alignment with the other USB controllers
>
> usb_host{0,1}_{e,o}hci
>
> here. I am aware that older SoCs and the SDK are using these names and
> it might be painful to have different versions to maintain at the
> moment, but can we please agree on
>
> usb_host0_xhci
> usb_host1_xhci
>
> or something like that?

I agree, I like your naming better.

>
> > +     phys = <&usb2phy0_otg>;
> > +     phy-names = "usb2-phy";
> > +     extcon = <&usb2phy0>;
> > +     maximum-speed = "high-speed";
> > +     snps,dis_u2_susphy_quirk;
> > +};
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3568.dtsi b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> > index 5b0f528d6818..77c044cbaaad 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3568.dtsi
> > @@ -99,6 +99,10 @@ opp-1992000000 {
> >       };
> >  };
> >
> > +&pipegrf {
> > +     compatible = "rockchip,rk3568-pipe-grf", "syscon";
> > +};
> > +
> >  &power {
> >       power-domain@RK3568_PD_PIPE {
> >               reg = <RK3568_PD_PIPE>;
> > @@ -114,3 +118,8 @@ power-domain@RK3568_PD_PIPE {
> >               #power-domain-cells = <0>;
> >       };
> >  };
> > +
> > +&usbdrd30 {
> > +     phys = <&usb2phy0_otg>, <&combphy0 PHY_TYPE_USB3>;
> > +     phy-names = "usb2-phy", "usb3-phy";
> > +};
> > diff --git a/arch/arm64/boot/dts/rockchip/rk356x.dtsi b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > index 84d5d607e693..4fae5b3b326e 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk356x.dtsi
> > @@ -230,6 +230,50 @@ scmi_shmem: sram@0 {
> >               };
> >       };
> >
> > +     usbdrd30: usbdrd@fcc00000 {
> > +             compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
> > +             reg = <0x0 0xfcc00000 0x0 0x400000>;
> > +             interrupts = <GIC_SPI 169 IRQ_TYPE_LEVEL_HIGH>;
> > +             clocks = <&cru CLK_USB3OTG0_REF>, <&cru CLK_USB3OTG0_SUSPEND>,
> > +                      <&cru ACLK_USB3OTG0>, <&cru PCLK_PIPE>;
> > +             clock-names = "ref_clk", "suspend_clk",
> > +                           "bus_clk", "grf_clk";
>
> Please consider Johan's comments on my first series. In my tests
> removing the PCLK_PIPE clock did not make any difference.

I'd like to test that this isn't being used, or isn't just working
because it's enabled elsewhere.
If both of those are false, then I'll be happy to drop this.

>
> > +             dr_mode = "host";
>
> Based on the description in the commit message above it should be "otg",
> right? Boards are free to overrule this, of course.

Currently the usb2phy does not support OTG mode correctly.
There are patches in the works for this, but at the moment it's safer
to default to host.

>
> > +             phy_type = "utmi_wide";
> > +             power-domains = <&power RK3568_PD_PIPE>;
> > +             resets = <&cru SRST_USB3OTG0>;
> > +             reset-names = "usb3-otg";
> > +             snps,dis_enblslpm_quirk;
> > +             snps,dis-u2-freeclk-exists-quirk;
> > +             snps,dis-del-phy-power-chg-quirk;
> > +             snps,dis-tx-ipgap-linecheck-quirk;
> > +             snps,xhci-trb-ent-quirk;
>
> In my first version I had all those quirks as well, but are they
> actually necessary? I decided to remove them all to have a fresh start
> (also activating them did not seem to affect my test setup).

I'm now curious about this, can someone weigh in on valid ways of
testing each one of these in a way that is definite?

>
> > +             status = "disabled";
> > +     };
> > +
> > +     usbhost30: usbhost@fd000000 {
>
> Please reconsider the this name as well.
>
> > +             compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
> > +             reg = <0x0 0xfd000000 0x0 0x400000>;
> > +             interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>;
> > +             clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>,
> > +                      <&cru ACLK_USB3OTG1>, <&cru PCLK_PIPE>;
> > +             clock-names = "ref_clk", "suspend_clk",
> > +                           "bus_clk", "grf_clk";
> > +             dr_mode = "host";
>
> Here "host" clearly makes sense, as this controller is not capable of otg.
>
> > +             phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>;
> > +             phy-names = "usb2-phy", "usb3-phy";
> > +             phy_type = "utmi_wide";
> > +             power-domains = <&power RK3568_PD_PIPE>;
> > +             resets = <&cru SRST_USB3OTG1>;
> > +             reset-names = "usb3-host";
> > +             snps,dis_enblslpm_quirk;
> > +             snps,dis-u2-freeclk-exists-quirk;
> > +             snps,dis_u2_susphy_quirk;
> > +             snps,dis-del-phy-power-chg-quirk;
> > +             snps,dis-tx-ipgap-linecheck-quirk;
>
> What was said about quirks above holds here as well (although one quirk
> not documented in the bindings is missing here).

Same thing here, I'd like absolute testing to determine that these are
not necessary, since downstream (the oem) felt they were.

>
> Best regards,
> Michael
>
> > +             status = "disabled";
> > +     };
> > +
> >       gic: interrupt-controller@fd400000 {
> >               compatible = "arm,gic-v3";
> >               reg = <0x0 0xfd400000 0 0x10000>, /* GICD */
> > @@ -297,7 +341,6 @@ pmu_io_domains: io-domains {
> >       };
> >
> >       pipegrf: syscon@fdc50000 {
> > -             compatible = "rockchip,rk3568-pipe-grf", "syscon";
> >               reg = <0x0 0xfdc50000 0x0 0x1000>;
> >       };
> >

Re: [PATCH v1 2/8] dt-bindings: usb: dwc3: add description for rk3568

[Peter Geis]

On Fri, Feb 25, 2022 at 11:07 AM Johan Jonker <jbx6244@gmail.com> wrote:
>
> Hi Peter,
>
> Lots of USB series all of a sudden.
> Combine possible?
>
> On 2/25/22 15:54, Peter Geis wrote:
> > The rk3568 dwc3 controllers are backwards compatible with the rk3399.
> > Add the device tree description for it.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >  Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
> > index 04077f2d7faf..e3044e81cc72 100644
> > --- a/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
> > +++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.yaml
> > @@ -30,6 +30,7 @@ select:
> >          enum:
> >            - rockchip,rk3328-dwc3
> >            - rockchip,rk3399-dwc3
> > +          - rockchip,rk3568-dwc3
> >    required:
> >      - compatible
> >
> > @@ -39,6 +40,7 @@ properties:
> >        - enum:
> >            - rockchip,rk3328-dwc3
> >            - rockchip,rk3399-dwc3
> > +          - rockchip,rk3568-dwc3
> >        - const: snps,dwc3
> >
> >    reg:
> > @@ -75,7 +77,10 @@ properties:
> >      maxItems: 1
> >
> he
> >    reset-names:
> > -    const: usb3-otg
> > +    items:
> > +      - enum:
> > +          - usb3-otg
> > +          - usb3-host
>
> The use of reset-names is "sort of" only related to the rk3399 legacy
> node. Still using this sub node DT to not to break older existing boot
> loaders.
>
> https://github.com/torvalds/linux/search?q=usb3-otg
>
> It's only mentioned as comment in dwc3-of-simple.c but not used:
>
>         simple->resets = of_reset_control_array_get(np, false, true,
>                                                     true);
> core.c uses something similar.
>
>         dwc->reset = devm_reset_control_array_get_optional_shared(dev);
>         if (IS_ERR(dwc->reset))
>                 return PTR_ERR(dwc->reset);
>
>
> Up to the maintainers, but I wouldn't add another variant/name for the
> same thing as it also optional(= not required) and no longer needed.

I left these named separately since they are different reset signals,
but if it isn't an issue I don't mind having them both be usb3-otg.

>
> Johan
>
> ===
>
> Maybe drop PCLK_PIPE as well to reduce notifications.

I'll be conducting testing to determine if we need PCLK_PIPE here, and
as long as it isn't working simply because it's enabled by someone
else I'll drop it.
Ideally, it would be nice to have a proper clock map for these chips,
but currently that's not in the TRM.

>
> See example:
> https://lore.kernel.org/linux-rockchip/20220225131602.2283499-4-michael.riesch@wolfvision.net/T/#u
>
> >
> >  unevaluatedProperties: false
> >

Thanks for the review!

Re: [PATCH v1 7/8] arm64: dts: rockchip: add rk356x dwc3 usb3 nodes

[Michael Riesch]

Hi Peter,

On 2/25/22 18:59, Peter Geis wrote:
> On Fri, Feb 25, 2022 at 12:01 PM Michael Riesch
> <michael.riesch@wolfvision.net> wrote:
>>
>> Hi Peter,
>>
>> (It should be noted that there was a slight mishap in communications
>> between the two of us resulting in two series with the same goal. Now
>> let's clean up the mess :-)
>>
>> Thanks for your series. Seeing that it contains more patches than mine
>> it probably makes sense to use your series as basis. Please Cc: me in
>> future iterations of this patch series and consider my comments below.
> 
> Will do.
> If you'd like I can also pull your enablement patch.

That'd be great, thanks! I'll help testing of course :-)

>> On 2/25/22 15:54, Peter Geis wrote:
> [...]
>>> +             dr_mode = "host";
>>
>> Based on the description in the commit message above it should be "otg",
>> right? Boards are free to overrule this, of course.
> 
> Currently the usb2phy does not support OTG mode correctly.
> There are patches in the works for this, but at the moment it's safer
> to default to host.

Ah OK, makes sense!

Best regards,
Michael

> 
>>
>>> +             phy_type = "utmi_wide";
>>> +             power-domains = <&power RK3568_PD_PIPE>;
>>> +             resets = <&cru SRST_USB3OTG0>;
>>> +             reset-names = "usb3-otg";
>>> +             snps,dis_enblslpm_quirk;
>>> +             snps,dis-u2-freeclk-exists-quirk;
>>> +             snps,dis-del-phy-power-chg-quirk;
>>> +             snps,dis-tx-ipgap-linecheck-quirk;
>>> +             snps,xhci-trb-ent-quirk;
>>
>> In my first version I had all those quirks as well, but are they
>> actually necessary? I decided to remove them all to have a fresh start
>> (also activating them did not seem to affect my test setup).
> 
> I'm now curious about this, can someone weigh in on valid ways of
> testing each one of these in a way that is definite?
> 
>>
>>> +             status = "disabled";
>>> +     };
>>> +
>>> +     usbhost30: usbhost@fd000000 {
>>
>> Please reconsider the this name as well.
>>
>>> +             compatible = "rockchip,rk3568-dwc3", "snps,dwc3";
>>> +             reg = <0x0 0xfd000000 0x0 0x400000>;
>>> +             interrupts = <GIC_SPI 170 IRQ_TYPE_LEVEL_HIGH>;
>>> +             clocks = <&cru CLK_USB3OTG1_REF>, <&cru CLK_USB3OTG1_SUSPEND>,
>>> +                      <&cru ACLK_USB3OTG1>, <&cru PCLK_PIPE>;
>>> +             clock-names = "ref_clk", "suspend_clk",
>>> +                           "bus_clk", "grf_clk";
>>> +             dr_mode = "host";
>>
>> Here "host" clearly makes sense, as this controller is not capable of otg.
>>
>>> +             phys = <&usb2phy0_host>, <&combphy1 PHY_TYPE_USB3>;
>>> +             phy-names = "usb2-phy", "usb3-phy";
>>> +             phy_type = "utmi_wide";
>>> +             power-domains = <&power RK3568_PD_PIPE>;
>>> +             resets = <&cru SRST_USB3OTG1>;
>>> +             reset-names = "usb3-host";
>>> +             snps,dis_enblslpm_quirk;
>>> +             snps,dis-u2-freeclk-exists-quirk;
>>> +             snps,dis_u2_susphy_quirk;
>>> +             snps,dis-del-phy-power-chg-quirk;
>>> +             snps,dis-tx-ipgap-linecheck-quirk;
>>
>> What was said about quirks above holds here as well (although one quirk
>> not documented in the bindings is missing here).
> 
> Same thing here, I'd like absolute testing to determine that these are
> not necessary, since downstream (the oem) felt they were.
> 
>>
>> Best regards,
>> Michael
>>
>>> +             status = "disabled";
>>> +     };
>>> +
>>>       gic: interrupt-controller@fd400000 {
>>>               compatible = "arm,gic-v3";
>>>               reg = <0x0 0xfd400000 0 0x10000>, /* GICD */
>>> @@ -297,7 +341,6 @@ pmu_io_domains: io-domains {
>>>       };
>>>
>>>       pipegrf: syscon@fdc50000 {
>>> -             compatible = "rockchip,rk3568-pipe-grf", "syscon";
>>>               reg = <0x0 0xfdc50000 0x0 0x1000>;
>>>       };
>>>