[RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Peter Geis]

This is my newly reworked phy driver for the rk3328 usb3 phy. It is
based loosely on my original version, but as of now almost nothing of
the original driver remains. The main fix here is the discovery of
BIT(6) in the interrupt enable grf register fixes the usb3 disconnection
detection (mostly). On occasion an unpopulated usb3 hub will take
several seconds to disconnect. However this means all of the hack around
work to reset the usb core manually is no longer required.

I did my best to document all registers I could find. A lot was taken
from emails between myself and Rockchip's engineers, much thanks to
William Wu <wulf@rock-chips.com> for their assistance here. The rest of
the config bits were taken from the rk3328 and rk3228h TRMs and the
downstream driver. Everything that I couldn't find a definition for is
prefixed UNK_ or UNKNOWN_. There's a lot of obviously used configuration
registers with the pipe interface that are also undocumented.

The only major bug I have so far is my AX88179 usb3 gigabit ethernet
adapter (Pluggable brand) crashes out when large amounts of data are
transmitted. I suspect this is related to the RX and TX tuning, as
leaving it at defaults makes things worse. As I am not a USB3 engineer
and I do not have the specialized knowledge and hardware to determine
what is going wrong, I am hoping the mailing list will have an answer
here.

Please test and review.

Very Respectfully,
Peter Geis



Peter Geis (6):
  clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328
  dt-bindings: phy: rockchip: add rk3328 usb3 phy
  phy: rockchip: add driver for rk3328 usb3 phy
  arm64: dts: rockchip: add rk3328 usb3 phy node
  arm64: dts: rockchip: enable the usb3 phy on rk3328-roc boards
  arm64: dts: rockchip: enable the usb3 phy on remaining rk3328 boards

 .../bindings/phy/rockchip,inno-usb3phy.yaml   | 166 ++++
 .../boot/dts/rockchip/rk3328-nanopi-r2.dtsi   |  12 +
 .../dts/rockchip/rk3328-orangepi-r1-plus.dtsi |  12 +
 arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi  |  12 +
 .../boot/dts/rockchip/rk3328-rock-pi-e.dts    |  12 +
 .../arm64/boot/dts/rockchip/rk3328-rock64.dts |  12 +
 arch/arm64/boot/dts/rockchip/rk3328.dtsi      |  39 +
 drivers/clk/rockchip/clk-rk3328.c             |   2 +-
 drivers/phy/rockchip/Kconfig                  |  10 +
 drivers/phy/rockchip/Makefile                 |   1 +
 drivers/phy/rockchip/phy-rockchip-inno-usb3.c | 869 ++++++++++++++++++
 11 files changed, 1146 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
 create mode 100644 drivers/phy/rockchip/phy-rockchip-inno-usb3.c

-- 
2.39.5

[RFC PATCH v1 2/6] dt-bindings: phy: rockchip: add rk3328 usb3 phy

[Peter Geis]

Add documentation for the usb3 phy as implemented on the rk3328 SoC.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---

 .../bindings/phy/rockchip,inno-usb3phy.yaml   | 166 ++++++++++++++++++
 1 file changed, 166 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml

diff --git a/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
new file mode 100644
index 000000000000..cde489ca87ab
--- /dev/null
+++ b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
@@ -0,0 +1,166 @@
+# SPDX-License-Identifier: GPL-2.0-only
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/phy/rockchip,inno-usb3phy.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Rockchip USB 3.0 phy with Innosilicon IP block
+
+maintainers:
+  - Heiko Stuebner <heiko@sntech.de>
+
+properties:
+  compatible:
+    enum:
+      - rockchip,rk3328-usb3phy
+
+  reg:
+    maxItems: 1
+
+  clocks:
+    minItems: 3
+    maxItems: 3
+
+  clock-names:
+    items:
+      - const: refclk-usb3otg
+      - const: usb3phy-otg
+      - const: usb3phy-pipe
+
+  interrupts:
+    minItems: 4
+
+  interrupt-names:
+    items:
+      - const: bvalid
+      - const: id
+      - const: linestate
+      - const: rxdet
+
+  resets:
+    minItems: 6
+
+  reset-names:
+    items:
+      - const: usb3phy-u2-por
+      - const: usb3phy-u3-por
+      - const: usb3phy-pipe-mac
+      - const: usb3phy-utmi-mac
+      - const: usb3phy-utmi-apb
+      - const: usb3phy-pipe-apb
+
+  "#address-cells":
+    const: 2
+
+  "#size-cells":
+    const: 2
+
+  ranges: true
+
+patternProperties:
+
+  utmi-port@[0-9a-f]+$:
+    type: object
+    additionalProperties: false
+
+    properties:
+      compatible:
+        enum:
+          - rockchip,rk3328-usb3phy-utmi
+
+      reg:
+        maxItems: 1
+
+      "#phy-cells":
+        const: 0
+
+      phy-supply:
+        description:
+          Phandle to a regulator that provides power to VBUS.
+          See ./phy-bindings.txt for details.
+
+    required:
+      - compatible
+      - reg
+      - "#phy-cells"
+
+  pipe-port@[0-9a-f]+$:
+    type: object
+    additionalProperties: false
+
+    properties:
+      compatible:
+        enum:
+          - rockchip,rk3328-usb3phy-pipe
+
+      reg:
+        maxItems: 1
+
+      "#phy-cells":
+        const: 0
+
+      phy-supply:
+        description:
+          Phandle to a regulator that provides power to VBUS.
+          See ./phy-bindings.txt for details.
+
+    required:
+      - compatible
+      - reg
+      - "#phy-cells"
+
+required:
+  - compatible
+  - reg
+  - clocks
+  - clock-names
+  - interrupts
+  - interrupt-names
+  - resets
+  - reset-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rk3328-cru.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/interrupt-controller/irq.h>
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      usb3phy: usb3-phy@ff460000 {
+        compatible = "rockchip,rk3328-usb3phy";
+        reg = <0x0 0xff460000 0x0 0x10000>;
+        clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
+        clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe";
+        interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
+                     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
+        interrupt-names = "bvalid", "id", "linestate", "rxdet";
+        resets = <&cru SRST_USB3PHY_U2>,
+                 <&cru SRST_USB3PHY_U3>,
+                 <&cru SRST_USB3PHY_PIPE>,
+                 <&cru SRST_USB3OTG_UTMI>,
+                 <&cru SRST_USB3PHY_OTG_P>,
+                 <&cru SRST_USB3PHY_PIPE_P>;
+        reset-names = "usb3phy-u2-por", "usb3phy-u3-por",
+                      "usb3phy-pipe-mac", "usb3phy-utmi-mac",
+                      "usb3phy-utmi-apb", "usb3phy-pipe-apb";
+        #address-cells = <2>;
+        #size-cells = <2>;
+        ranges;
+
+        usb3phy_utmi: utmi-port@ff470000 {
+          compatible = "rockchip,rk3328-usb3phy-utmi";
+          reg = <0x0 0xff470000 0x0 0x8000>;
+          #phy-cells = <0>;
+        };
+
+        usb3phy_pipe: pipe-port@ff478000 {
+          compatible = "rockchip,rk3328-usb3phy-pipe";
+          reg = <0x0 0xff478000 0x0 0x8000>;
+          #phy-cells = <0>;
+        };
+      };
+    };
-- 
2.39.5

[RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Peter Geis]

Add the node for the rk3328 usb3 phy. This node provides a combined usb2
and usb3 phy which are permenantly tied to the dwc3 usb3 controller.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---

 arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
index 7d992c3c01ce..181a900d41f9 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
@@ -903,6 +903,43 @@ u2phy_host: host-port {
 		};
 	};
 
+	usb3phy: usb3-phy@ff460000 {
+		compatible = "rockchip,rk3328-usb3phy";
+		reg = <0x0 0xff460000 0x0 0x10000>;
+		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
+		clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe";
+		interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
+			     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
+		interrupt-names = "bvalid", "id", "linestate", "rxdet";
+		resets = <&cru SRST_USB3PHY_U2>,
+			 <&cru SRST_USB3PHY_U3>,
+			 <&cru SRST_USB3PHY_PIPE>,
+			 <&cru SRST_USB3OTG_UTMI>,
+			 <&cru SRST_USB3PHY_OTG_P>,
+			 <&cru SRST_USB3PHY_PIPE_P>;
+		reset-names = "usb3phy-u2-por", "usb3phy-u3-por",
+			      "usb3phy-pipe-mac", "usb3phy-utmi-mac",
+			      "usb3phy-utmi-apb", "usb3phy-pipe-apb";
+		#address-cells = <2>;
+		#size-cells = <2>;
+		ranges;
+		status = "disabled";
+
+		usb3phy_utmi: utmi-port@ff470000 {
+			compatible = "rockchip,rk3328-usb3phy-utmi";
+			reg = <0x0 0xff470000 0x0 0x8000>;
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+
+		usb3phy_pipe: pipe-port@ff478000 {
+			compatible = "rockchip,rk3328-usb3phy-pipe";
+			reg = <0x0 0xff478000 0x0 0x8000>;
+			#phy-cells = <0>;
+			status = "disabled";
+		};
+	};
+
 	sdmmc: mmc@ff500000 {
 		compatible = "rockchip,rk3328-dw-mshc", "rockchip,rk3288-dw-mshc";
 		reg = <0x0 0xff500000 0x0 0x4000>;
@@ -1067,6 +1104,8 @@ usbdrd3: usb@ff600000 {
 		clock-names = "ref_clk", "suspend_clk",
 			      "bus_clk";
 		dr_mode = "otg";
+		phys = <&usb3phy_utmi>, <&usb3phy_pipe>;
+		phy-names = "usb2-phy", "usb3-phy";
 		phy_type = "utmi_wide";
 		snps,dis-del-phy-power-chg-quirk;
 		snps,dis_enblslpm_quirk;
-- 
2.39.5

[RFC PATCH v1 5/6] arm64: dts: rockchip: enable the usb3 phy on rk3328-roc boards

[Peter Geis]

Enable the new usb3 phy nodes on the rk3328-roc boards.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---

 arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
index b5bd5e7d5748..22d93c1901ed 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi
@@ -360,6 +360,18 @@ &usbdrd3 {
 	status = "okay";
 };
 
+&usb3phy {
+	status = "okay";
+};
+
+&usb3phy_utmi {
+	status = "okay";
+};
+
+&usb3phy_pipe {
+	status = "okay";
+};
+
 &usb_host0_ehci {
 	status = "okay";
 };
-- 
2.39.5

[RFC PATCH v1 6/6] arm64: dts: rockchip: enable the usb3 phy on remaining rk3328 boards

[Peter Geis]

Enable the new usb3 phy nodes on the remaining rk3328 boards. This is
done separately from the rk3328-roc boards due to lack of testing, in
case of regressions.

Signed-off-by: Peter Geis <pgwipeout@gmail.com>
---

 arch/arm64/boot/dts/rockchip/rk3328-nanopi-r2.dtsi   | 12 ++++++++++++
 .../boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi   | 12 ++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts    | 12 ++++++++++++
 arch/arm64/boot/dts/rockchip/rk3328-rock64.dts       | 12 ++++++++++++
 4 files changed, 48 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3328-nanopi-r2.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-nanopi-r2.dtsi
index 1715d311e1f2..a4d880725a96 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-nanopi-r2.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328-nanopi-r2.dtsi
@@ -385,6 +385,18 @@ rtl8153: device@2 {
 	};
 };
 
+&usb3phy {
+	status = "okay";
+};
+
+&usb3phy_utmi {
+	status = "okay";
+};
+
+&usb3phy_pipe {
+	status = "okay";
+};
+
 &usb_host0_ehci {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
index 82021ffb0a49..f70d28e6da5d 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3328-orangepi-r1-plus.dtsi
@@ -349,6 +349,18 @@ rtl8153: device@2 {
 	};
 };
 
+&usb3phy {
+	status = "okay";
+};
+
+&usb3phy_utmi {
+	status = "okay";
+};
+
+&usb3phy_pipe {
+	status = "okay";
+};
+
 &usb_host0_ehci {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
index 425de197ddb8..4ce6b16cf291 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-rock-pi-e.dts
@@ -440,6 +440,18 @@ &usbdrd3 {
 	status = "okay";
 };
 
+&usb3phy {
+	status = "okay";
+};
+
+&usb3phy_utmi {
+	status = "okay";
+};
+
+&usb3phy_pipe {
+	status = "okay";
+};
+
 &usb_host0_ehci {
 	status = "okay";
 };
diff --git a/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts b/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
index 745d3e996418..2e366de96390 100644
--- a/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3328-rock64.dts
@@ -379,6 +379,18 @@ &usbdrd3 {
 	status = "okay";
 };
 
+&usb3phy {
+	status = "okay";
+};
+
+&usb3phy_utmi {
+	status = "okay";
+};
+
+&usb3phy_pipe {
+	status = "okay";
+};
+
 &usb_host0_ehci {
 	status = "okay";
 };
-- 
2.39.5

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Piotr Oniszczuk]

> Wiadomość napisana przez Peter Geis <pgwipeout@gmail.com> w dniu 15 sty 2025, o godz. 02:26:
> 
> 
> This is my newly reworked phy driver for the rk3328 usb3 phy. It is
> based loosely on my original version, but as of now almost nothing of
> the original driver remains. The main fix here is the discovery of
> BIT(6) in the interrupt enable grf register fixes the usb3 disconnection
> detection (mostly). On occasion an unpopulated usb3 hub will take
> several seconds to disconnect. However this means all of the hack around
> work to reset the usb core manually is no longer required.
> 
> I did my best to document all registers I could find. A lot was taken
> from emails between myself and Rockchip's engineers, much thanks to
> William Wu <wulf@rock-chips.com> for their assistance here. The rest of
> the config bits were taken from the rk3328 and rk3228h TRMs and the
> downstream driver. Everything that I couldn't find a definition for is
> prefixed UNK_ or UNKNOWN_. There's a lot of obviously used configuration
> registers with the pipe interface that are also undocumented.
> 
> The only major bug I have so far is my AX88179 usb3 gigabit ethernet
> adapter (Pluggable brand) crashes out when large amounts of data are
> transmitted. I suspect this is related to the RX and TX tuning, as
> leaving it at defaults makes things worse. As I am not a USB3 engineer
> and I do not have the specialized knowledge and hardware to determine
> what is going wrong, I am hoping the mailing list will have an answer
> here.
> 
> Please test and review.
> 
> Very Respectfully,
> Peter Geis
> 
> 
> 
> Peter Geis (6):
>  clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328
>  dt-bindings: phy: rockchip: add rk3328 usb3 phy
>  phy: rockchip: add driver for rk3328 usb3 phy
>  arm64: dts: rockchip: add rk3328 usb3 phy node
>  arm64: dts: rockchip: enable the usb3 phy on rk3328-roc boards
>  arm64: dts: rockchip: enable the usb3 phy on remaining rk3328 boards
> 
> .../bindings/phy/rockchip,inno-usb3phy.yaml   | 166 ++++
> .../boot/dts/rockchip/rk3328-nanopi-r2.dtsi   |  12 +
> .../dts/rockchip/rk3328-orangepi-r1-plus.dtsi |  12 +
> arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi  |  12 +
> .../boot/dts/rockchip/rk3328-rock-pi-e.dts    |  12 +
> .../arm64/boot/dts/rockchip/rk3328-rock64.dts |  12 +
> arch/arm64/boot/dts/rockchip/rk3328.dtsi      |  39 +
> drivers/clk/rockchip/clk-rk3328.c             |   2 +-
> drivers/phy/rockchip/Kconfig                  |  10 +
> drivers/phy/rockchip/Makefile                 |   1 +
> drivers/phy/rockchip/phy-rockchip-inno-usb3.c | 869 ++++++++++++++++++
> 11 files changed, 1146 insertions(+), 1 deletion(-)
> create mode 100644 Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> create mode 100644 drivers/phy/rockchip/phy-rockchip-inno-usb3.c
> 
> -- 
> 2.39.5
> 
> 

Peter,
I applied this as test run on 6.12.9 and dmesg says:

[   16.368514] rockchip-usb3-phy ff460000.usb3-phy: could not get usb3phy ref clock
[   16.368534] rockchip-usb3-phy ff460000.usb3-phy: parse dt failed -2
[   16.368543] rockchip-usb3-phy ff460000.usb3-phy: probe with driver rockchip-usb3-phy failed with error -2

This is on beelink a1 tvbox.

Do I miss something?

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Peter Geis]

On Wed, Jan 15, 2025 at 6:22 AM Piotr Oniszczuk
<piotr.oniszczuk@gmail.com> wrote:
>
>
>
> > Wiadomość napisana przez Peter Geis <pgwipeout@gmail.com> w dniu 15 sty 2025, o godz. 02:26:
> >
> >
> > This is my newly reworked phy driver for the rk3328 usb3 phy. It is
> > based loosely on my original version, but as of now almost nothing of
> > the original driver remains. The main fix here is the discovery of
> > BIT(6) in the interrupt enable grf register fixes the usb3 disconnection
> > detection (mostly). On occasion an unpopulated usb3 hub will take
> > several seconds to disconnect. However this means all of the hack around
> > work to reset the usb core manually is no longer required.
> >
> > I did my best to document all registers I could find. A lot was taken
> > from emails between myself and Rockchip's engineers, much thanks to
> > William Wu <wulf@rock-chips.com> for their assistance here. The rest of
> > the config bits were taken from the rk3328 and rk3228h TRMs and the
> > downstream driver. Everything that I couldn't find a definition for is
> > prefixed UNK_ or UNKNOWN_. There's a lot of obviously used configuration
> > registers with the pipe interface that are also undocumented.
> >
> > The only major bug I have so far is my AX88179 usb3 gigabit ethernet
> > adapter (Pluggable brand) crashes out when large amounts of data are
> > transmitted. I suspect this is related to the RX and TX tuning, as
> > leaving it at defaults makes things worse. As I am not a USB3 engineer
> > and I do not have the specialized knowledge and hardware to determine
> > what is going wrong, I am hoping the mailing list will have an answer
> > here.
> >
> > Please test and review.
> >
> > Very Respectfully,
> > Peter Geis
> >
> >
> >
> > Peter Geis (6):
> >  clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328
> >  dt-bindings: phy: rockchip: add rk3328 usb3 phy
> >  phy: rockchip: add driver for rk3328 usb3 phy
> >  arm64: dts: rockchip: add rk3328 usb3 phy node
> >  arm64: dts: rockchip: enable the usb3 phy on rk3328-roc boards
> >  arm64: dts: rockchip: enable the usb3 phy on remaining rk3328 boards
> >
> > .../bindings/phy/rockchip,inno-usb3phy.yaml   | 166 ++++
> > .../boot/dts/rockchip/rk3328-nanopi-r2.dtsi   |  12 +
> > .../dts/rockchip/rk3328-orangepi-r1-plus.dtsi |  12 +
> > arch/arm64/boot/dts/rockchip/rk3328-roc.dtsi  |  12 +
> > .../boot/dts/rockchip/rk3328-rock-pi-e.dts    |  12 +
> > .../arm64/boot/dts/rockchip/rk3328-rock64.dts |  12 +
> > arch/arm64/boot/dts/rockchip/rk3328.dtsi      |  39 +
> > drivers/clk/rockchip/clk-rk3328.c             |   2 +-
> > drivers/phy/rockchip/Kconfig                  |  10 +
> > drivers/phy/rockchip/Makefile                 |   1 +
> > drivers/phy/rockchip/phy-rockchip-inno-usb3.c | 869 ++++++++++++++++++
> > 11 files changed, 1146 insertions(+), 1 deletion(-)
> > create mode 100644 Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> > create mode 100644 drivers/phy/rockchip/phy-rockchip-inno-usb3.c
> >
> > --
> > 2.39.5
> >
> >
>
> Peter,
> I applied this as test run on 6.12.9 and dmesg says:
>
> [   16.368514] rockchip-usb3-phy ff460000.usb3-phy: could not get usb3phy ref clock
> [   16.368534] rockchip-usb3-phy ff460000.usb3-phy: parse dt failed -2
> [   16.368543] rockchip-usb3-phy ff460000.usb3-phy: probe with driver rockchip-usb3-phy failed with error -2
>
> This is on beelink a1 tvbox.
>
> Do I miss something?
>

Good Morning,

That is an interesting failure. The simplest answer is the
`clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe";` line
was corrupted. Please check that when applied it matches the patch
exactly. If you are still having problems, you can send me the
compiled DTB and I'll take a look.

Very Respectfully,
Peter Geis

>

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Piotr Oniszczuk]

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



> Wiadomość napisana przez Peter Geis <pgwipeout@gmail.com> w dniu 15 sty 2025, o godz. 13:25:
> 
>> 
>> 
>> Do I miss something?
>> 
> 
> Good Morning,
> 
> That is an interesting failure. The simplest answer is the
> `clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe";` line
> was corrupted. Please check that when applied it matches the patch
> exactly. If you are still having problems, you can send me the
> compiled DTB and I'll take a look.
> 

oh - this check i done as first thing to look on :-)

pls find:

dtsi: https://gist.github.com/warpme/6af9e2778fb06bfb47b64f98fe79d678
and dt: https://gist.github.com/warpme/79340c54c87b2b1e52f2a146461d8c9f


and compiled .dtb


[-- Attachment #2: rk3328-a1.dtb --]
[-- Type: application/octet-stream, Size: 40319 bytes --]

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Peter Geis]

On Wed, Jan 15, 2025 at 7:35 AM Piotr Oniszczuk
<piotr.oniszczuk@gmail.com> wrote:
>
>
>
> > Wiadomość napisana przez Peter Geis <pgwipeout@gmail.com> w dniu 15 sty 2025, o godz. 13:25:
> >
> >>
> >>
> >> Do I miss something?
> >>
> >
> > Good Morning,
> >
> > That is an interesting failure. The simplest answer is the
> > `clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe";` line
> > was corrupted. Please check that when applied it matches the patch
> > exactly. If you are still having problems, you can send me the
> > compiled DTB and I'll take a look.
> >
>
> oh - this check i done as first thing to look on :-)
>
> pls find:
>
> dtsi: https://gist.github.com/warpme/6af9e2778fb06bfb47b64f98fe79d678
> and dt: https://gist.github.com/warpme/79340c54c87b2b1e52f2a146461d8c9f
>
>
> and compiled .dtb

It all looks good.
Do you have any CRU errors in the boot log?
Can you check for the presence of clk_ref_usb3otg in
/sys/kernel/debug/clk/clk_summary?

>

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Piotr Oniszczuk]

> Wiadomość napisana przez Peter Geis <pgwipeout@gmail.com> w dniu 15 sty 2025, o godz. 14:15:
> 
>> 
>> 
>> pls find:
>> 
>> dtsi: https://gist.github.com/warpme/6af9e2778fb06bfb47b64f98fe79d678
>> and dt: https://gist.github.com/warpme/79340c54c87b2b1e52f2a146461d8c9f
>> 
>> 
>> and compiled .dtb
> 
> It all looks good.
> Do you have any CRU errors in the boot log?
> Can you check for the presence of clk_ref_usb3otg in
> /sys/kernel/debug/clk/clk_summary?
> 
>> 

cru errors in dmesg - seems no any.
here is my dmesg: https://gist.github.com/warpme/ecf38482cc88fb68471355d011ecf974

For clk_ref_usb3otg i’m getting following:

root@myth-frontend-e67a8de4c815:~ # cat /sys/kernel/debug/clk/clk_summary | grep usb3
    clk_usb3otg_suspend              0       0        0        30770       0          0     50000      N      deviceless                      no_connection_id
    clk_usb3otg_ref                  0       0        0        24000000    0          0     50000      N      deviceless                      no_connection_id
    clk_ref_usb3otg                  0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
                   pclk_usb3_grf     0       0        0        75000000    0          0     50000      Y                     deviceless                      no_connection_id
                   pclk_usb3phy_pipe 0       0        0        75000000    0          0     50000      N                     deviceless                      no_connection_id
                   pclk_usb3phy_otg  0       0        0        75000000    0          0     50000      N                     deviceless                      no_connection_id
          clk_ref_usb3otg_src        0       0        0        37500000    0          0     50000      N            deviceless                      no_connection_id
                   aclk_usb3otg      0       0        0        150000000   0          0     50000      N                     deviceless                      no_connection_id

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Krzysztof Kozlowski]

On 15/01/2025 02:26, Peter Geis wrote:
> Add the node for the rk3328 usb3 phy. This node provides a combined usb2
> and usb3 phy which are permenantly tied to the dwc3 usb3 controller.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
> 
>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> index 7d992c3c01ce..181a900d41f9 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> @@ -903,6 +903,43 @@ u2phy_host: host-port {
>  		};
>  	};
>  
> +	usb3phy: usb3-phy@ff460000 {
> +		compatible = "rockchip,rk3328-usb3phy";
> +		reg = <0x0 0xff460000 0x0 0x10000>;
> +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;

Please wrap code according to coding style (checkpatch is not a coding
style description, but only a tool), so at 80.


> +		clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe";
> +		interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
> +			     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
> +		interrupt-names = "bvalid", "id", "linestate", "rxdet";
> +		resets = <&cru SRST_USB3PHY_U2>,
> +			 <&cru SRST_USB3PHY_U3>,
> +			 <&cru SRST_USB3PHY_PIPE>,
> +			 <&cru SRST_USB3OTG_UTMI>,
> +			 <&cru SRST_USB3PHY_OTG_P>,
> +			 <&cru SRST_USB3PHY_PIPE_P>;
> +		reset-names = "usb3phy-u2-por", "usb3phy-u3-por",
> +			      "usb3phy-pipe-mac", "usb3phy-utmi-mac",
> +			      "usb3phy-utmi-apb", "usb3phy-pipe-apb";
> +		#address-cells = <2>;
> +		#size-cells = <2>;
> +		ranges;
> +		status = "disabled";
> +
> +		usb3phy_utmi: utmi-port@ff470000 {
> +			compatible = "rockchip,rk3328-usb3phy-utmi";
> +			reg = <0x0 0xff470000 0x0 0x8000>;
> +			#phy-cells = <0>;

Parent device is the phy provider, not child. This is odd...

> +			status = "disabled";
> +		};
> +
> +		usb3phy_pipe: pipe-port@ff478000 {
> +			compatible = "rockchip,rk3328-usb3phy-pipe";
> +			reg = <0x0 0xff478000 0x0 0x8000>;
> +			#phy-cells = <0>;
> +			status = "disabled";
> +		};
> +	};

Best regards,
Krzysztof

Re: [RFC PATCH v1 2/6] dt-bindings: phy: rockchip: add rk3328 usb3 phy

[Krzysztof Kozlowski]

On 15/01/2025 02:26, Peter Geis wrote:
> Add documentation for the usb3 phy as implemented on the rk3328 SoC.
> 
> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> ---
> 
>  .../bindings/phy/rockchip,inno-usb3phy.yaml   | 166 ++++++++++++++++++
>  1 file changed, 166 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> 
> diff --git a/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> new file mode 100644
> index 000000000000..cde489ca87ab
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> @@ -0,0 +1,166 @@
> +# SPDX-License-Identifier: GPL-2.0-only

Wrong license.

Please run scripts/checkpatch.pl and fix reported warnings. After that,
run also `scripts/checkpatch.pl --strict` and (probably) fix more
warnings. Some warnings can be ignored, especially from --strict run,
but the code here looks like it needs a fix. Feel free to get in touch
if the warning is not clear.

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/phy/rockchip,inno-usb3phy.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Rockchip USB 3.0 phy with Innosilicon IP block

> +
> +maintainers:
> +  - Heiko Stuebner <heiko@sntech.de>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - rockchip,rk3328-usb3phy

Why is this binding entirely different than existing usb2 phy? Nothing
in common? This looks like made for driver and both - driver and binding
- duplicating a lot.

> +
> +  reg:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 3

Drop

> +    maxItems: 3
> +
> +  clock-names:
> +    items:
> +      - const: refclk-usb3otg

ref

> +      - const: usb3phy-otg

otg

> +      - const: usb3phy-pipe

pipe

> +
> +  interrupts:
> +    minItems: 4

You code here randomly. reg has only maxItems, clocks have both and this
have only minItems. Does not make sense. If you get it wrong, I would
assume you repeat the same mistake but here it is like randomly chosen
pieces. And this is making me wonder whether this was not sent too fast.

Anyway: only maxItems.


> +
> +  interrupt-names:
> +    items:
> +      - const: bvalid
> +      - const: id
> +      - const: linestate
> +      - const: rxdet
> +
> +  resets:
> +    minItems: 6

maxItems instead

> +
> +  reset-names:
> +    items:
> +      - const: usb3phy-u2-por
> +      - const: usb3phy-u3-por
> +      - const: usb3phy-pipe-mac
> +      - const: usb3phy-utmi-mac
> +      - const: usb3phy-utmi-apb
> +      - const: usb3phy-pipe-apb
> +
> +  "#address-cells":
> +    const: 2
> +
> +  "#size-cells":
> +    const: 2
> +
> +  ranges: true
> +
> +patternProperties:
> +

Drop blank line

> +  utmi-port@[0-9a-f]+$:

This wasn't tested. Missing quotes, missing starting anchor.

> +    type: object

Explain what are the children here - description.


> +    additionalProperties: false
> +
> +    properties:
> +      compatible:
> +        enum:
> +          - rockchip,rk3328-usb3phy-utmi
> +
> +      reg:
> +        maxItems: 1
> +
> +      "#phy-cells":
> +        const: 0

Does not look correct. Your parent device is the phy, not child. Why do
you create children per each type of phy?

> +
> +      phy-supply:
> +        description:
> +          Phandle to a regulator that provides power to VBUS.
> +          See ./phy-bindings.txt for details.
> +
> +    required:
> +      - compatible
> +      - reg
> +      - "#phy-cells"
> +
> +  pipe-port@[0-9a-f]+$:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      compatible:
> +        enum:
> +          - rockchip,rk3328-usb3phy-pipe
> +
> +      reg:
> +        maxItems: 1
> +
> +      "#phy-cells":
> +        const: 0
> +
> +      phy-supply:
> +        description:
> +          Phandle to a regulator that provides power to VBUS.
> +          See ./phy-bindings.txt for details.

Drop "see ....".

> +
> +    required:
> +      - compatible
> +      - reg
> +      - "#phy-cells"
> +
> +required:
> +  - compatible
> +  - reg
> +  - clocks
> +  - clock-names
> +  - interrupts
> +  - interrupt-names
> +  - resets
> +  - reset-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/rk3328-cru.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      usb3phy: usb3-phy@ff460000 {
> +        compatible = "rockchip,rk3328-usb3phy";
> +        reg = <0x0 0xff460000 0x0 0x10000>;
> +        clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;

That's way over the limit. Wrapping is at 80.

> +        clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe";
> +        interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
> +                     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
> +        interrupt-names = "bvalid", "id", "linestate", "rxdet";




Best regards,
Krzysztof

Re: [RFC PATCH v1 2/6] dt-bindings: phy: rockchip: add rk3328 usb3 phy

[Peter Geis]

On Thu, Jan 16, 2025 at 8:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 15/01/2025 02:26, Peter Geis wrote:
> > Add documentation for the usb3 phy as implemented on the rk3328 SoC.
> >
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> >
> >  .../bindings/phy/rockchip,inno-usb3phy.yaml   | 166 ++++++++++++++++++
> >  1 file changed, 166 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> > new file mode 100644
> > index 000000000000..cde489ca87ab
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> > @@ -0,0 +1,166 @@
> > +# SPDX-License-Identifier: GPL-2.0-only
>
> Wrong license.
>
> Please run scripts/checkpatch.pl and fix reported warnings. After that,
> run also `scripts/checkpatch.pl --strict` and (probably) fix more
> warnings. Some warnings can be ignored, especially from --strict run,
> but the code here looks like it needs a fix. Feel free to get in touch
> if the warning is not clear.

Checkpatch literally told me to change this. Ran against my original
dev binding:
./scripts/checkpatch.pl --strict
Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
CHECK: DT binding documents should be licensed (GPL-2.0-only OR BSD-2-Clause)
#1: FILE: Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml:1:
+# SPDX-License-Identifier: GPL-2.0

>
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/phy/rockchip,inno-usb3phy.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Rockchip USB 3.0 phy with Innosilicon IP block
>
> > +
> > +maintainers:
> > +  - Heiko Stuebner <heiko@sntech.de>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - rockchip,rk3328-usb3phy
>
> Why is this binding entirely different than existing usb2 phy? Nothing
> in common? This looks like made for driver and both - driver and binding
> - duplicating a lot.

Hmm, I hadn't considered merging it into the usb2 binding as it is a
unique (and uniquely broken) device. I'd like Heiko's thoughts on
this.

>
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 3
>
> Drop
>
> > +    maxItems: 3
> > +
> > +  clock-names:
> > +    items:
> > +      - const: refclk-usb3otg
>
> ref
>
> > +      - const: usb3phy-otg
>
> otg
>
> > +      - const: usb3phy-pipe
>
> pipe
>
> > +
> > +  interrupts:
> > +    minItems: 4
>
> You code here randomly. reg has only maxItems, clocks have both and this
> have only minItems. Does not make sense. If you get it wrong, I would
> assume you repeat the same mistake but here it is like randomly chosen
> pieces. And this is making me wonder whether this was not sent too fast.

I admit, I dread writing bindings as even now I'm weak in YAML and it
seems to pick and choose what rules it wants to follow.

>
> Anyway: only maxItems.
>
>
> > +
> > +  interrupt-names:
> > +    items:
> > +      - const: bvalid
> > +      - const: id
> > +      - const: linestate
> > +      - const: rxdet
> > +
> > +  resets:
> > +    minItems: 6
>
> maxItems instead
>
> > +
> > +  reset-names:
> > +    items:
> > +      - const: usb3phy-u2-por
> > +      - const: usb3phy-u3-por
> > +      - const: usb3phy-pipe-mac
> > +      - const: usb3phy-utmi-mac
> > +      - const: usb3phy-utmi-apb
> > +      - const: usb3phy-pipe-apb
> > +
> > +  "#address-cells":
> > +    const: 2
> > +
> > +  "#size-cells":
> > +    const: 2
> > +
> > +  ranges: true
> > +
> > +patternProperties:
> > +
>
> Drop blank line
>
> > +  utmi-port@[0-9a-f]+$:
>
> This wasn't tested. Missing quotes, missing starting anchor.

make W=1 dt_binding_check didn't complain about it, using the
dt-schema from pip3 from about a week ago.

>
> > +    type: object
>
> Explain what are the children here - description.

Fair, will do.

>
>
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - rockchip,rk3328-usb3phy-utmi
> > +
> > +      reg:
> > +        maxItems: 1
> > +
> > +      "#phy-cells":
> > +        const: 0
>
> Does not look correct. Your parent device is the phy, not child. Why do
> you create children per each type of phy?

Because that's how it's done elsewhere in Rockchip's phys [1]. How
should it be done?

>
> > +
> > +      phy-supply:
> > +        description:
> > +          Phandle to a regulator that provides power to VBUS.
> > +          See ./phy-bindings.txt for details.
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - "#phy-cells"
> > +
> > +  pipe-port@[0-9a-f]+$:
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      compatible:
> > +        enum:
> > +          - rockchip,rk3328-usb3phy-pipe
> > +
> > +      reg:
> > +        maxItems: 1
> > +
> > +      "#phy-cells":
> > +        const: 0
> > +
> > +      phy-supply:
> > +        description:
> > +          Phandle to a regulator that provides power to VBUS.
> > +          See ./phy-bindings.txt for details.
>
> Drop "see ....".

I was tempted to convert phy-bindings.txt over but as above I dread
writing bindings. Will drop.

>
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> > +      - "#phy-cells"
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - resets
> > +  - reset-names
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/clock/rk3328-cru.h>
> > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    soc {
> > +      #address-cells = <2>;
> > +      #size-cells = <2>;
> > +
> > +      usb3phy: usb3-phy@ff460000 {
> > +        compatible = "rockchip,rk3328-usb3phy";
> > +        reg = <0x0 0xff460000 0x0 0x10000>;
> > +        clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
>
> That's way over the limit. Wrapping is at 80.

Will correct.

>
> > +        clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe";
> > +        interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
> > +                     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
> > +        interrupt-names = "bvalid", "id", "linestate", "rxdet";
>
>
>

I appreciate all the feedback, I'll incorporate the changes you've recommended.

Very Respectfully,
Peter Geis

>
> Best regards,
> Krzysztof

[1] https://elixir.bootlin.com/linux/v6.13-rc3/source/arch/arm64/boot/dts/rockchip/rk3328.dtsi#L887

Re: [RFC PATCH v1 2/6] dt-bindings: phy: rockchip: add rk3328 usb3 phy

[Peter Geis]

On Thu, Jan 16, 2025 at 8:32 AM Peter Geis <pgwipeout@gmail.com> wrote:
>
> On Thu, Jan 16, 2025 at 8:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
> >
> > On 15/01/2025 02:26, Peter Geis wrote:
> > > Add documentation for the usb3 phy as implemented on the rk3328 SoC.
> > >
> > > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > > ---
> > >
> > >  .../bindings/phy/rockchip,inno-usb3phy.yaml   | 166 ++++++++++++++++++
> > >  1 file changed, 166 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> > > new file mode 100644
> > > index 000000000000..cde489ca87ab
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> > > @@ -0,0 +1,166 @@
> > > +# SPDX-License-Identifier: GPL-2.0-only
> >
> > Wrong license.
> >
> > Please run scripts/checkpatch.pl and fix reported warnings. After that,
> > run also `scripts/checkpatch.pl --strict` and (probably) fix more
> > warnings. Some warnings can be ignored, especially from --strict run,
> > but the code here looks like it needs a fix. Feel free to get in touch
> > if the warning is not clear.
>
> Checkpatch literally told me to change this. Ran against my original
> dev binding:
> ./scripts/checkpatch.pl --strict
> Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml
> CHECK: DT binding documents should be licensed (GPL-2.0-only OR BSD-2-Clause)
> #1: FILE: Documentation/devicetree/bindings/phy/rockchip,inno-usb3phy.yaml:1:
> +# SPDX-License-Identifier: GPL-2.0

I understand now, thank you. Perhaps checkpatch could put that in
quotes, instead saying ("GPL-2.0-only OR BSD-2-Clause") to make it
clear it's one thing.

>
> >
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/phy/rockchip,inno-usb3phy.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Rockchip USB 3.0 phy with Innosilicon IP block
> >
> > > +
> > > +maintainers:
> > > +  - Heiko Stuebner <heiko@sntech.de>
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - rockchip,rk3328-usb3phy
> >
> > Why is this binding entirely different than existing usb2 phy? Nothing
> > in common? This looks like made for driver and both - driver and binding
> > - duplicating a lot.
>
> Hmm, I hadn't considered merging it into the usb2 binding as it is a
> unique (and uniquely broken) device. I'd like Heiko's thoughts on
> this.
>
> >
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  clocks:
> > > +    minItems: 3
> >
> > Drop
> >
> > > +    maxItems: 3
> > > +
> > > +  clock-names:
> > > +    items:
> > > +      - const: refclk-usb3otg
> >
> > ref
> >
> > > +      - const: usb3phy-otg
> >
> > otg
> >
> > > +      - const: usb3phy-pipe
> >
> > pipe
> >
> > > +
> > > +  interrupts:
> > > +    minItems: 4
> >
> > You code here randomly. reg has only maxItems, clocks have both and this
> > have only minItems. Does not make sense. If you get it wrong, I would
> > assume you repeat the same mistake but here it is like randomly chosen
> > pieces. And this is making me wonder whether this was not sent too fast.
>
> I admit, I dread writing bindings as even now I'm weak in YAML and it
> seems to pick and choose what rules it wants to follow.
>
> >
> > Anyway: only maxItems.
> >
> >
> > > +
> > > +  interrupt-names:
> > > +    items:
> > > +      - const: bvalid
> > > +      - const: id
> > > +      - const: linestate
> > > +      - const: rxdet
> > > +
> > > +  resets:
> > > +    minItems: 6
> >
> > maxItems instead
> >
> > > +
> > > +  reset-names:
> > > +    items:
> > > +      - const: usb3phy-u2-por
> > > +      - const: usb3phy-u3-por
> > > +      - const: usb3phy-pipe-mac
> > > +      - const: usb3phy-utmi-mac
> > > +      - const: usb3phy-utmi-apb
> > > +      - const: usb3phy-pipe-apb
> > > +
> > > +  "#address-cells":
> > > +    const: 2
> > > +
> > > +  "#size-cells":
> > > +    const: 2
> > > +
> > > +  ranges: true
> > > +
> > > +patternProperties:
> > > +
> >
> > Drop blank line
> >
> > > +  utmi-port@[0-9a-f]+$:
> >
> > This wasn't tested. Missing quotes, missing starting anchor.
>
> make W=1 dt_binding_check didn't complain about it, using the
> dt-schema from pip3 from about a week ago.
>
> >
> > > +    type: object
> >
> > Explain what are the children here - description.
>
> Fair, will do.
>
> >
> >
> > > +    additionalProperties: false
> > > +
> > > +    properties:
> > > +      compatible:
> > > +        enum:
> > > +          - rockchip,rk3328-usb3phy-utmi
> > > +
> > > +      reg:
> > > +        maxItems: 1
> > > +
> > > +      "#phy-cells":
> > > +        const: 0
> >
> > Does not look correct. Your parent device is the phy, not child. Why do
> > you create children per each type of phy?
>
> Because that's how it's done elsewhere in Rockchip's phys [1]. How
> should it be done?
>
> >
> > > +
> > > +      phy-supply:
> > > +        description:
> > > +          Phandle to a regulator that provides power to VBUS.
> > > +          See ./phy-bindings.txt for details.
> > > +
> > > +    required:
> > > +      - compatible
> > > +      - reg
> > > +      - "#phy-cells"
> > > +
> > > +  pipe-port@[0-9a-f]+$:
> > > +    type: object
> > > +    additionalProperties: false
> > > +
> > > +    properties:
> > > +      compatible:
> > > +        enum:
> > > +          - rockchip,rk3328-usb3phy-pipe
> > > +
> > > +      reg:
> > > +        maxItems: 1
> > > +
> > > +      "#phy-cells":
> > > +        const: 0
> > > +
> > > +      phy-supply:
> > > +        description:
> > > +          Phandle to a regulator that provides power to VBUS.
> > > +          See ./phy-bindings.txt for details.
> >
> > Drop "see ....".
>
> I was tempted to convert phy-bindings.txt over but as above I dread
> writing bindings. Will drop.
>
> >
> > > +
> > > +    required:
> > > +      - compatible
> > > +      - reg
> > > +      - "#phy-cells"
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - clocks
> > > +  - clock-names
> > > +  - interrupts
> > > +  - interrupt-names
> > > +  - resets
> > > +  - reset-names
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/clock/rk3328-cru.h>
> > > +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +    soc {
> > > +      #address-cells = <2>;
> > > +      #size-cells = <2>;
> > > +
> > > +      usb3phy: usb3-phy@ff460000 {
> > > +        compatible = "rockchip,rk3328-usb3phy";
> > > +        reg = <0x0 0xff460000 0x0 0x10000>;
> > > +        clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
> >
> > That's way over the limit. Wrapping is at 80.
>
> Will correct.
>
> >
> > > +        clock-names = "refclk-usb3otg", "usb3phy-otg", "usb3phy-pipe";
> > > +        interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>,
> > > +                     <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>, <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
> > > +        interrupt-names = "bvalid", "id", "linestate", "rxdet";
> >
> >
> >
>
> I appreciate all the feedback, I'll incorporate the changes you've recommended.
>
> Very Respectfully,
> Peter Geis
>
> >
> > Best regards,
> > Krzysztof
>
> [1] https://elixir.bootlin.com/linux/v6.13-rc3/source/arch/arm64/boot/dts/rockchip/rk3328.dtsi#L887

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Peter Geis]

On Wed, Jan 15, 2025 at 8:25 AM Piotr Oniszczuk
<piotr.oniszczuk@gmail.com> wrote:
>
>
>
> > Wiadomość napisana przez Peter Geis <pgwipeout@gmail.com> w dniu 15 sty 2025, o godz. 14:15:
> >
> >>
> >>
> >> pls find:
> >>
> >> dtsi: https://gist.github.com/warpme/6af9e2778fb06bfb47b64f98fe79d678
> >> and dt: https://gist.github.com/warpme/79340c54c87b2b1e52f2a146461d8c9f
> >>
> >>
> >> and compiled .dtb
> >
> > It all looks good.
> > Do you have any CRU errors in the boot log?
> > Can you check for the presence of clk_ref_usb3otg in
> > /sys/kernel/debug/clk/clk_summary?
> >
> >>
>
> cru errors in dmesg - seems no any.
> here is my dmesg: https://gist.github.com/warpme/ecf38482cc88fb68471355d011ecf974
>
> For clk_ref_usb3otg i’m getting following:
>
> root@myth-frontend-e67a8de4c815:~ # cat /sys/kernel/debug/clk/clk_summary | grep usb3
>     clk_usb3otg_suspend              0       0        0        30770       0          0     50000      N      deviceless                      no_connection_id
>     clk_usb3otg_ref                  0       0        0        24000000    0          0     50000      N      deviceless                      no_connection_id
>     clk_ref_usb3otg                  0       0        0        24000000    0          0     50000      Y      deviceless                      no_connection_id
>                    pclk_usb3_grf     0       0        0        75000000    0          0     50000      Y                     deviceless                      no_connection_id
>                    pclk_usb3phy_pipe 0       0        0        75000000    0          0     50000      N                     deviceless                      no_connection_id
>                    pclk_usb3phy_otg  0       0        0        75000000    0          0     50000      N                     deviceless                      no_connection_id
>           clk_ref_usb3otg_src        0       0        0        37500000    0          0     50000      N            deviceless                      no_connection_id
>                    aclk_usb3otg      0       0        0        150000000   0          0     50000      N                     deviceless                      no_connection_id
>

I'm at a loss here, I applied the patches to a clean 6.9 tree and even
built it as a module (thank you for the sentinel). I have no issues.
I'm wondering if it's your .config or something about your bootloader.

>

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Piotr Oniszczuk]

> Wiadomość napisana przez Peter Geis <pgwipeout@gmail.com> w dniu 16 sty 2025, o godz. 15:02:
> 
>> 
> 
> I'm at a loss here, I applied the patches to a clean 6.9 tree and even
>> 

oh maybe issue is in kernel age?
6.9 seems a bit quite old.
i’m on 6.12.9….

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Peter Geis]

On Thu, Jan 16, 2025 at 9:35 AM Piotr Oniszczuk
<piotr.oniszczuk@gmail.com> wrote:
>
>
>
> > Wiadomość napisana przez Peter Geis <pgwipeout@gmail.com> w dniu 16 sty 2025, o godz. 15:02:
> >
> >>
> >
> > I'm at a loss here, I applied the patches to a clean 6.9 tree and even
> >>
>
> oh maybe issue is in kernel age?
> 6.9 seems a bit quite old.
> i’m on 6.12.9….

The patches were prepared and tested against 6.13-rc1, but nothing has
changed significantly in the kernel in regards to rk3328 clock
handling in several years. I jumped back to 6.9 due to dyslexia.

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Diederik de Haas]

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

On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
> On 15/01/2025 02:26, Peter Geis wrote:
> > Add the node for the rk3328 usb3 phy. This node provides a combined usb2
> > and usb3 phy which are permenantly tied to the dwc3 usb3 controller.
> > 
> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
> > ---
> > 
> >  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > index 7d992c3c01ce..181a900d41f9 100644
> > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> > @@ -903,6 +903,43 @@ u2phy_host: host-port {
> >  		};
> >  	};
> >  
> > +	usb3phy: usb3-phy@ff460000 {
> > +		compatible = "rockchip,rk3328-usb3phy";
> > +		reg = <0x0 0xff460000 0x0 0x10000>;
> > +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
>
> Please wrap code according to coding style (checkpatch is not a coding
> style description, but only a tool), so at 80.

I'm confused: is it 80 or 100?

I always thought it was 80, but then I saw several patches/commits by
Dragan Simic which deliberately changed code to make use of 100.
Being fed up with my own confusion, I submitted a PR to 
https://github.com/gregkh/kernel-coding-style/ which got accepted:
https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a

So now both the vim plugins code and README say 100.
But as noted in my commit message:

  Note that the current upstream 'Linux kernel coding style' does NOT
  mention the 100 char limit, but only mentions the preferred max length
  of 80.

Or is it 100 for code, but 80 for DeviceTree files and bindings?

Cheers,
  Diederik

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

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Dragan Simic]

Hello Diederik,

On 2025-01-16 17:53, Diederik de Haas wrote:
> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
>> On 15/01/2025 02:26, Peter Geis wrote:
>> > Add the node for the rk3328 usb3 phy. This node provides a combined usb2
>> > and usb3 phy which are permenantly tied to the dwc3 usb3 controller.
>> >
>> > Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>> > ---
>> >
>> >  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++
>> >  1 file changed, 39 insertions(+)
>> >
>> > diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> > index 7d992c3c01ce..181a900d41f9 100644
>> > --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> > +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>> > @@ -903,6 +903,43 @@ u2phy_host: host-port {
>> >  		};
>> >  	};
>> >
>> > +	usb3phy: usb3-phy@ff460000 {
>> > +		compatible = "rockchip,rk3328-usb3phy";
>> > +		reg = <0x0 0xff460000 0x0 0x10000>;
>> > +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
>> 
>> Please wrap code according to coding style (checkpatch is not a coding
>> style description, but only a tool), so at 80.
> 
> I'm confused: is it 80 or 100?
> 
> I always thought it was 80, but then I saw several patches/commits by
> Dragan Simic which deliberately changed code to make use of 100.
> Being fed up with my own confusion, I submitted a PR to
> https://github.com/gregkh/kernel-coding-style/ which got accepted:
> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a
> 
> So now both the vim plugins code and README say 100.
> But as noted in my commit message:
> 
>   Note that the current upstream 'Linux kernel coding style' does NOT
>   mention the 100 char limit, but only mentions the preferred max 
> length
>   of 80.
> 
> Or is it 100 for code, but 80 for DeviceTree files and bindings?

I don't know about the DT files and bindings, but the 100-column limit
for the kernel code has been in effect for years.  In this day and age,
80 columns is really not much (for the record, I've been around when
using 80x25 _physical_ CRT screens was the norm).

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Krzysztof Kozlowski]

On 16/01/2025 17:53, Diederik de Haas wrote:
> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
>> On 15/01/2025 02:26, Peter Geis wrote:
>>> Add the node for the rk3328 usb3 phy. This node provides a combined usb2
>>> and usb3 phy which are permenantly tied to the dwc3 usb3 controller.
>>>
>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>> ---
>>>
>>>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++
>>>  1 file changed, 39 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>> index 7d992c3c01ce..181a900d41f9 100644
>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>> @@ -903,6 +903,43 @@ u2phy_host: host-port {
>>>  		};
>>>  	};
>>>  
>>> +	usb3phy: usb3-phy@ff460000 {
>>> +		compatible = "rockchip,rk3328-usb3phy";
>>> +		reg = <0x0 0xff460000 0x0 0x10000>;
>>> +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
>>
>> Please wrap code according to coding style (checkpatch is not a coding
>> style description, but only a tool), so at 80.
> 
> I'm confused: is it 80 or 100?
> 
> I always thought it was 80, but then I saw several patches/commits by

Coding style is clear: it is 80. It also has caveat about code
readability and several maintainers have their own preference.

> Dragan Simic which deliberately changed code to make use of 100.
> Being fed up with my own confusion, I submitted a PR to 
> https://github.com/gregkh/kernel-coding-style/ which got accepted:
> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a

That's not kernel. That's Greg...

> 
> So now both the vim plugins code and README say 100.
> But as noted in my commit message:
> 
>   Note that the current upstream 'Linux kernel coding style' does NOT
>   mention the 100 char limit, but only mentions the preferred max length
>   of 80.
> 
> Or is it 100 for code, but 80 for DeviceTree files and bindings?

From where did you get 100? Checkpatch, right? Kernel coding style is
clear, there is no discussion, no mentioning 100:

"The preferred limit on the length of a single line is 80 columns. "

So to be clear: all DTS, all DT bindings, all code maintained by me and
some maintainers follows above (and further - there is caveat)
instruction from coding style. Some maintainers follow other rules and
that's fine.

Best regards,
Krzysztof

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Krzysztof Kozlowski]

On 17/01/2025 05:10, Dragan Simic wrote:
> Hello Diederik,
> 
> On 2025-01-16 17:53, Diederik de Haas wrote:
>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
>>> On 15/01/2025 02:26, Peter Geis wrote:
>>>> Add the node for the rk3328 usb3 phy. This node provides a combined usb2
>>>> and usb3 phy which are permenantly tied to the dwc3 usb3 controller.
>>>>
>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>>> ---
>>>>
>>>>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++
>>>>  1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>> index 7d992c3c01ce..181a900d41f9 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port {
>>>>  		};
>>>>  	};
>>>>
>>>> +	usb3phy: usb3-phy@ff460000 {
>>>> +		compatible = "rockchip,rk3328-usb3phy";
>>>> +		reg = <0x0 0xff460000 0x0 0x10000>;
>>>> +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
>>>
>>> Please wrap code according to coding style (checkpatch is not a coding
>>> style description, but only a tool), so at 80.
>>
>> I'm confused: is it 80 or 100?
>>
>> I always thought it was 80, but then I saw several patches/commits by
>> Dragan Simic which deliberately changed code to make use of 100.
>> Being fed up with my own confusion, I submitted a PR to
>> https://github.com/gregkh/kernel-coding-style/ which got accepted:
>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a
>>
>> So now both the vim plugins code and README say 100.
>> But as noted in my commit message:
>>
>>   Note that the current upstream 'Linux kernel coding style' does NOT
>>   mention the 100 char limit, but only mentions the preferred max 
>> length
>>   of 80.
>>
>> Or is it 100 for code, but 80 for DeviceTree files and bindings?
> 
> I don't know about the DT files and bindings, but the 100-column limit
> for the kernel code has been in effect for years.  In this day and age,

That's just false. It was never in effect for years. Read kernel coding
style document.

> 80 columns is really not much (for the record, I've been around when
> using 80x25 _physical_ CRT screens was the norm).

You mistake agreement on dropping strong restriction in 2020 in
checkpatch, which is "not for years" and even read that commit: "Yes,
staying withing 80 columns is certainly still _preferred_."

Checkpatch is not coding style. Since when it would be? It's just a tool.

And there were more talks and the 80-preference got relaxed yet still
"not for years" (last talk was 2022?) and sill kernel coding style is
here specific.

Best regards,
Krzysztof

Re: [RFC PATCH v1 2/6] dt-bindings: phy: rockchip: add rk3328 usb3 phy

[Krzysztof Kozlowski]

On 16/01/2025 14:32, Peter Geis wrote:
>>
>>
>>> +    additionalProperties: false
>>> +
>>> +    properties:
>>> +      compatible:
>>> +        enum:
>>> +          - rockchip,rk3328-usb3phy-utmi
>>> +
>>> +      reg:
>>> +        maxItems: 1
>>> +
>>> +      "#phy-cells":
>>> +        const: 0
>>
>> Does not look correct. Your parent device is the phy, not child. Why do
>> you create children per each type of phy?
> 
> Because that's how it's done elsewhere in Rockchip's phys [1]. How
> should it be done?


The phys have separate supplies and IO addresses? Then it is reasonable
to keep them separate and as children. But then more questions appear:
why resets - which are also per utmi or port - are in top-level node?
This should be represented in coherent way: either you define the
properties/nodes per PHY or just everything in one/entire PHY
controller. Not mixed.

Same concerns about clocks in top-level.

It also might be that everything is a bit mixed, so you have entire phy
controller handling common resources and still separate phy for USB2 and
USB3 as children, but that should be conscious choice coming from actual
hardware. You have entire "description:" in binding to explain the
hardware and any questions I asked now.


Best regards,
Krzysztof

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Krzysztof Kozlowski]

On 15/01/2025 02:26, Peter Geis wrote:
> 
> This is my newly reworked phy driver for the rk3328 usb3 phy. It is
> based loosely on my original version, but as of now almost nothing of
> the original driver remains. The main fix here is the discovery of
> BIT(6) in the interrupt enable grf register fixes the usb3 disconnection
> detection (mostly). On occasion an unpopulated usb3 hub will take
> several seconds to disconnect. However this means all of the hack around
> work to reset the usb core manually is no longer required.
> 
BTW, RFC for some maintainers means "do not review, work-in-progress".
For some means "review, but low priority" or "review, but for sure I
have bugs here". I usually review and then someone responds: "it is not
for review, it is just RFC", so to avoid my wasted time please always
mention in cover letter why this is RFC. What do you expect here or why
this is not ready for review as normal patch.

Best regards,
Krzysztof

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Krzysztof Kozlowski]

On 18/01/2025 09:41, Krzysztof Kozlowski wrote:
> On 16/01/2025 17:53, Diederik de Haas wrote:
>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
>>> On 15/01/2025 02:26, Peter Geis wrote:
>>>> Add the node for the rk3328 usb3 phy. This node provides a combined usb2
>>>> and usb3 phy which are permenantly tied to the dwc3 usb3 controller.
>>>>
>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>>> ---
>>>>
>>>>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 ++++++++++++++++++++++++
>>>>  1 file changed, 39 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>> index 7d992c3c01ce..181a900d41f9 100644
>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port {
>>>>  		};
>>>>  	};
>>>>  
>>>> +	usb3phy: usb3-phy@ff460000 {
>>>> +		compatible = "rockchip,rk3328-usb3phy";
>>>> +		reg = <0x0 0xff460000 0x0 0x10000>;
>>>> +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
>>>
>>> Please wrap code according to coding style (checkpatch is not a coding
>>> style description, but only a tool), so at 80.
>>
>> I'm confused: is it 80 or 100?
>>
>> I always thought it was 80, but then I saw several patches/commits by
> 
> Coding style is clear: it is 80. It also has caveat about code
> readability and several maintainers have their own preference.
> 
>> Dragan Simic which deliberately changed code to make use of 100.
>> Being fed up with my own confusion, I submitted a PR to 
>> https://github.com/gregkh/kernel-coding-style/ which got accepted:
>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a
> 
> That's not kernel. That's Greg...
> 
>>
>> So now both the vim plugins code and README say 100.
>> But as noted in my commit message:
>>
>>   Note that the current upstream 'Linux kernel coding style' does NOT
>>   mention the 100 char limit, but only mentions the preferred max length
>>   of 80.
>>
>> Or is it 100 for code, but 80 for DeviceTree files and bindings?
> 
> From where did you get 100? Checkpatch, right? Kernel coding style is
> clear, there is no discussion, no mentioning 100:
> 
> "The preferred limit on the length of a single line is 80 columns. "
> 
> So to be clear: all DTS, all DT bindings, all code maintained by me and
> some maintainers follows above (and further - there is caveat)
> instruction from coding style. Some maintainers follow other rules and
> that's fine.


Although let me add here caveat, after looking at some other code: DTS
due to its nature of a lot of parent-child relationships combined with
long constants ("GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>") has the strongest
exception or the strongest second part of the coding style:
"...unless exceeding 80 columns significantly increases readability..."

And again: that's from official coding style document (so something
which have been for years), no matter what other people tell you they
think exists since years as coding style.

Splitting line, I commented here in this patch, did not improve readability.

Quite opposite: the line there was less readable in current format thus
it is not even about coding style anymore, but just readability style.
Any list with more than two short entries (by number of characters in
list item) or any list with more than one long entry should be split for
readability. However actual ITEMS in list should not be split - but
again coding style is here very precise since years. 80 unless
significantly increases readability.

Best regards,
Krzysztof

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Dragan Simic]

Hello Krzysztof,

On 2025-01-18 09:46, Krzysztof Kozlowski wrote:
> On 17/01/2025 05:10, Dragan Simic wrote:
>> On 2025-01-16 17:53, Diederik de Haas wrote:
>>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
>>>> On 15/01/2025 02:26, Peter Geis wrote:
>>>>> Add the node for the rk3328 usb3 phy. This node provides a combined 
>>>>> usb2
>>>>> and usb3 phy which are permenantly tied to the dwc3 usb3 
>>>>> controller.
>>>>> 
>>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>>>> ---
>>>>> 
>>>>>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 
>>>>> ++++++++++++++++++++++++
>>>>>  1 file changed, 39 insertions(+)
>>>>> 
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi 
>>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> index 7d992c3c01ce..181a900d41f9 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port {
>>>>>  		};
>>>>>  	};
>>>>> 
>>>>> +	usb3phy: usb3-phy@ff460000 {
>>>>> +		compatible = "rockchip,rk3328-usb3phy";
>>>>> +		reg = <0x0 0xff460000 0x0 0x10000>;
>>>>> +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru 
>>>>> PCLK_USB3PHY_PIPE>;
>>>> 
>>>> Please wrap code according to coding style (checkpatch is not a 
>>>> coding
>>>> style description, but only a tool), so at 80.
>>> 
>>> I'm confused: is it 80 or 100?
>>> 
>>> I always thought it was 80, but then I saw several patches/commits by
>>> Dragan Simic which deliberately changed code to make use of 100.
>>> Being fed up with my own confusion, I submitted a PR to
>>> https://github.com/gregkh/kernel-coding-style/ which got accepted:
>>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a
>>> 
>>> So now both the vim plugins code and README say 100.
>>> But as noted in my commit message:
>>> 
>>>   Note that the current upstream 'Linux kernel coding style' does NOT
>>>   mention the 100 char limit, but only mentions the preferred max
>>> length
>>>   of 80.
>>> 
>>> Or is it 100 for code, but 80 for DeviceTree files and bindings?
>> 
>> I don't know about the DT files and bindings, but the 100-column limit
>> for the kernel code has been in effect for years.  In this day and 
>> age,
> 
> That's just false. It was never in effect for years. Read kernel coding
> style document.

Perhaps it's about the semantics.

Please see the commit bdc48fa11e46 (checkpatch/coding-style: deprecate
80-column warning, 2020-05-29), which clearly shows that the 80-column
rule is still _preferred_, but no longer _mandatory_.

>> 80 columns is really not much (for the record, I've been around when
>> using 80x25 _physical_ CRT screens was the norm).
> 
> You mistake agreement on dropping strong restriction in 2020 in
> checkpatch, which is "not for years" and even read that commit: "Yes,
> staying withing 80 columns is certainly still _preferred_."
> 
> Checkpatch is not coding style. Since when it would be? It's just a 
> tool.
> 
> And there were more talks and the 80-preference got relaxed yet still
> "not for years" (last talk was 2022?) and sill kernel coding style is
> here specific.

It's perhaps again about the semantics, this time about the meaning
of "for years".  I don't think there's some strict definition of that
term, so perhaps different people see it differently.

To get back to the above-mentioned commit bdc48fa11e46, the 80-column
limit has obviously been lifted, putting the new 100-column limit as
an option for those who prefer having fewer "artificial" line breaks
over adhering strictly to the rules.

Thus, as a maintainer, you're obviously free to enforce the 80-column
limit of you want so.

If my opinion counts, I'd agree with the 80-column limit when it comes
to the device trees and bindings.  Keeping those files at the lower
width usually makes them more readable to me.  However, enforcing the
80-column limit in C and header files very often leads to having line
breaks that do nothing but make the code look a bit silly.

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Krzysztof Kozlowski]

On 18/01/2025 10:25, Dragan Simic wrote:
> Hello Krzysztof,
> 
> On 2025-01-18 09:46, Krzysztof Kozlowski wrote:
>> On 17/01/2025 05:10, Dragan Simic wrote:
>>> On 2025-01-16 17:53, Diederik de Haas wrote:
>>>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
>>>>> On 15/01/2025 02:26, Peter Geis wrote:
>>>>>> Add the node for the rk3328 usb3 phy. This node provides a combined 
>>>>>> usb2
>>>>>> and usb3 phy which are permenantly tied to the dwc3 usb3 
>>>>>> controller.
>>>>>>
>>>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>>>>> ---
>>>>>>
>>>>>>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 
>>>>>> ++++++++++++++++++++++++
>>>>>>  1 file changed, 39 insertions(+)
>>>>>>
>>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi 
>>>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>>> index 7d992c3c01ce..181a900d41f9 100644
>>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port {
>>>>>>  		};
>>>>>>  	};
>>>>>>
>>>>>> +	usb3phy: usb3-phy@ff460000 {
>>>>>> +		compatible = "rockchip,rk3328-usb3phy";
>>>>>> +		reg = <0x0 0xff460000 0x0 0x10000>;
>>>>>> +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru 
>>>>>> PCLK_USB3PHY_PIPE>;
>>>>>
>>>>> Please wrap code according to coding style (checkpatch is not a 
>>>>> coding
>>>>> style description, but only a tool), so at 80.
>>>>
>>>> I'm confused: is it 80 or 100?
>>>>
>>>> I always thought it was 80, but then I saw several patches/commits by
>>>> Dragan Simic which deliberately changed code to make use of 100.
>>>> Being fed up with my own confusion, I submitted a PR to
>>>> https://github.com/gregkh/kernel-coding-style/ which got accepted:
>>>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a
>>>>
>>>> So now both the vim plugins code and README say 100.
>>>> But as noted in my commit message:
>>>>
>>>>   Note that the current upstream 'Linux kernel coding style' does NOT
>>>>   mention the 100 char limit, but only mentions the preferred max
>>>> length
>>>>   of 80.
>>>>
>>>> Or is it 100 for code, but 80 for DeviceTree files and bindings?
>>>
>>> I don't know about the DT files and bindings, but the 100-column limit
>>> for the kernel code has been in effect for years.  In this day and 
>>> age,
>>
>> That's just false. It was never in effect for years. Read kernel coding
>> style document.
> 
> Perhaps it's about the semantics.
> 
> Please see the commit bdc48fa11e46 (checkpatch/coding-style: deprecate
> 80-column warning, 2020-05-29), which clearly shows that the 80-column
> rule is still _preferred_, but no longer _mandatory_.

I brought that commit, but nice that you also found it.

Still: read the coding style, not checkpatch tool.

> 
>>> 80 columns is really not much (for the record, I've been around when
>>> using 80x25 _physical_ CRT screens was the norm).
>>
>> You mistake agreement on dropping strong restriction in 2020 in
>> checkpatch, which is "not for years" and even read that commit: "Yes,
>> staying withing 80 columns is certainly still _preferred_."
>>
>> Checkpatch is not coding style. Since when it would be? It's just a 
>> tool.
>>
>> And there were more talks and the 80-preference got relaxed yet still
>> "not for years" (last talk was 2022?) and sill kernel coding style is
>> here specific.
> 
> It's perhaps again about the semantics, this time about the meaning
> of "for years".  I don't think there's some strict definition of that
> term, so perhaps different people see it differently.
> 
> To get back to the above-mentioned commit bdc48fa11e46, the 80-column
> limit has obviously been lifted, putting the new 100-column limit as


"Lifted" on *CHECKPATCH*, not on coding style. Do you see the
difference? One is a helper tool which people were using blindly and
wrapping lines without thinking, claiming that checkpatch told them to
do so. Other is the actual coding style.

You claim that coding style was changed. This never happened.

And my first  - really the first - comment here was also precise
mentioning that difference:

"Please wrap code according to coding style (*checkpatch is not* a
coding style description, but only a tool), so at 80."


Best regards,
Krzysztof

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Dragan Simic]

On 2025-01-18 10:19, Krzysztof Kozlowski wrote:
> On 18/01/2025 09:41, Krzysztof Kozlowski wrote:
>> On 16/01/2025 17:53, Diederik de Haas wrote:
>>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
>>>> On 15/01/2025 02:26, Peter Geis wrote:
>>>>> Add the node for the rk3328 usb3 phy. This node provides a combined 
>>>>> usb2
>>>>> and usb3 phy which are permenantly tied to the dwc3 usb3 
>>>>> controller.
>>>>> 
>>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>>>> ---
>>>>> 
>>>>>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39 
>>>>> ++++++++++++++++++++++++
>>>>>  1 file changed, 39 insertions(+)
>>>>> 
>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi 
>>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> index 7d992c3c01ce..181a900d41f9 100644
>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port {
>>>>>  		};
>>>>>  	};
>>>>> 
>>>>> +	usb3phy: usb3-phy@ff460000 {
>>>>> +		compatible = "rockchip,rk3328-usb3phy";
>>>>> +		reg = <0x0 0xff460000 0x0 0x10000>;
>>>>> +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru 
>>>>> PCLK_USB3PHY_PIPE>;
>>>> 
>>>> Please wrap code according to coding style (checkpatch is not a 
>>>> coding
>>>> style description, but only a tool), so at 80.
>>> 
>>> I'm confused: is it 80 or 100?
>>> 
>>> I always thought it was 80, but then I saw several patches/commits by
>> 
>> Coding style is clear: it is 80. It also has caveat about code
>> readability and several maintainers have their own preference.
>> 
>>> Dragan Simic which deliberately changed code to make use of 100.
>>> Being fed up with my own confusion, I submitted a PR to
>>> https://github.com/gregkh/kernel-coding-style/ which got accepted:
>>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a
>> 
>> That's not kernel. That's Greg...
>> 
>>> 
>>> So now both the vim plugins code and README say 100.
>>> But as noted in my commit message:
>>> 
>>>   Note that the current upstream 'Linux kernel coding style' does NOT
>>>   mention the 100 char limit, but only mentions the preferred max 
>>> length
>>>   of 80.
>>> 
>>> Or is it 100 for code, but 80 for DeviceTree files and bindings?
>> 
>> From where did you get 100? Checkpatch, right? Kernel coding style is
>> clear, there is no discussion, no mentioning 100:
>> 
>> "The preferred limit on the length of a single line is 80 columns. "
>> 
>> So to be clear: all DTS, all DT bindings, all code maintained by me 
>> and
>> some maintainers follows above (and further - there is caveat)
>> instruction from coding style. Some maintainers follow other rules and
>> that's fine.
> 
> 
> Although let me add here caveat, after looking at some other code: DTS
> due to its nature of a lot of parent-child relationships combined with
> long constants ("GIC_SPI 147 IRQ_TYPE_LEVEL_HIGH>") has the strongest
> exception or the strongest second part of the coding style:
> "...unless exceeding 80 columns significantly increases readability..."
> 
> And again: that's from official coding style document (so something
> which have been for years), no matter what other people tell you they
> think exists since years as coding style.
> 
> Splitting line, I commented here in this patch, did not improve 
> readability.
> 
> Quite opposite: the line there was less readable in current format thus
> it is not even about coding style anymore, but just readability style.
> Any list with more than two short entries (by number of characters in
> list item) or any list with more than one long entry should be split 
> for
> readability. However actual ITEMS in list should not be split - but
> again coding style is here very precise since years. 80 unless
> significantly increases readability.

I fully agree with the readability being the most important factor when
it comes to deciding on the column width.  That's very well illustrated
by the example above, i.e. the list items in device trees, which are 
much
more readable when the items are placed in separate lines.

Though, as I wrote in my earlier response, enforcing the 80-column limit
in C and headers files rather often leads to line breaks that are 
obviously
"artificial" and do nothing but make the code less readable.  That's 
where
the 100-column with limit often improves the readability.

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Dragan Simic]

On 2025-01-18 10:31, Krzysztof Kozlowski wrote:
> On 18/01/2025 10:25, Dragan Simic wrote:
>> On 2025-01-18 09:46, Krzysztof Kozlowski wrote:
>>> On 17/01/2025 05:10, Dragan Simic wrote:
>>>> On 2025-01-16 17:53, Diederik de Haas wrote:
>>>>> On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
>>>>>> On 15/01/2025 02:26, Peter Geis wrote:
>>>>>>> Add the node for the rk3328 usb3 phy. This node provides a 
>>>>>>> combined
>>>>>>> usb2
>>>>>>> and usb3 phy which are permenantly tied to the dwc3 usb3
>>>>>>> controller.
>>>>>>> 
>>>>>>> Signed-off-by: Peter Geis <pgwipeout@gmail.com>
>>>>>>> ---
>>>>>>> 
>>>>>>>  arch/arm64/boot/dts/rockchip/rk3328.dtsi | 39
>>>>>>> ++++++++++++++++++++++++
>>>>>>>  1 file changed, 39 insertions(+)
>>>>>>> 
>>>>>>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>>>> b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>>>> index 7d992c3c01ce..181a900d41f9 100644
>>>>>>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>>>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
>>>>>>> @@ -903,6 +903,43 @@ u2phy_host: host-port {
>>>>>>>  		};
>>>>>>>  	};
>>>>>>> 
>>>>>>> +	usb3phy: usb3-phy@ff460000 {
>>>>>>> +		compatible = "rockchip,rk3328-usb3phy";
>>>>>>> +		reg = <0x0 0xff460000 0x0 0x10000>;
>>>>>>> +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, 
>>>>>>> <&cru
>>>>>>> PCLK_USB3PHY_PIPE>;
>>>>>> 
>>>>>> Please wrap code according to coding style (checkpatch is not a
>>>>>> coding
>>>>>> style description, but only a tool), so at 80.
>>>>> 
>>>>> I'm confused: is it 80 or 100?
>>>>> 
>>>>> I always thought it was 80, but then I saw several patches/commits 
>>>>> by
>>>>> Dragan Simic which deliberately changed code to make use of 100.
>>>>> Being fed up with my own confusion, I submitted a PR to
>>>>> https://github.com/gregkh/kernel-coding-style/ which got accepted:
>>>>> https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a
>>>>> 
>>>>> So now both the vim plugins code and README say 100.
>>>>> But as noted in my commit message:
>>>>> 
>>>>>   Note that the current upstream 'Linux kernel coding style' does 
>>>>> NOT
>>>>>   mention the 100 char limit, but only mentions the preferred max
>>>>> length
>>>>>   of 80.
>>>>> 
>>>>> Or is it 100 for code, but 80 for DeviceTree files and bindings?
>>>> 
>>>> I don't know about the DT files and bindings, but the 100-column 
>>>> limit
>>>> for the kernel code has been in effect for years.  In this day and
>>>> age,
>>> 
>>> That's just false. It was never in effect for years. Read kernel 
>>> coding
>>> style document.
>> 
>> Perhaps it's about the semantics.
>> 
>> Please see the commit bdc48fa11e46 (checkpatch/coding-style: deprecate
>> 80-column warning, 2020-05-29), which clearly shows that the 80-column
>> rule is still _preferred_, but no longer _mandatory_.
> 
> I brought that commit, but nice that you also found it.
> 
> Still: read the coding style, not checkpatch tool.
> 
>>>> 80 columns is really not much (for the record, I've been around when
>>>> using 80x25 _physical_ CRT screens was the norm).
>>> 
>>> You mistake agreement on dropping strong restriction in 2020 in
>>> checkpatch, which is "not for years" and even read that commit: "Yes,
>>> staying withing 80 columns is certainly still _preferred_."
>>> 
>>> Checkpatch is not coding style. Since when it would be? It's just a
>>> tool.
>>> 
>>> And there were more talks and the 80-preference got relaxed yet still
>>> "not for years" (last talk was 2022?) and sill kernel coding style is
>>> here specific.
>> 
>> It's perhaps again about the semantics, this time about the meaning
>> of "for years".  I don't think there's some strict definition of that
>> term, so perhaps different people see it differently.
>> 
>> To get back to the above-mentioned commit bdc48fa11e46, the 80-column
>> limit has obviously been lifted, putting the new 100-column limit as
> 
> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the
> difference? One is a helper tool which people were using blindly and
> wrapping lines without thinking, claiming that checkpatch told them to
> do so. Other is the actual coding style.
> 
> You claim that coding style was changed. This never happened.

It was obviously changed in the commit bdc48fa11e46, by making the
80-column width preferred, instead of if being mandatory.  The way
I read the changes to the coding style introduced in that commit,
it's now possible to go over 80 columns, up to 100 columns, _if_
that actually improves the readability of the source code.

Just like enforcing the 80-column blindly can make the code much
less readable, it's also that going liberally to 100 columns can
make the code even less readable.  To me, those rules aren't to be
followed blindly, but the resulting readability of the code should
be the deciding factor for pretty much each line of the code.

> And my first  - really the first - comment here was also precise
> mentioning that difference:
> 
> "Please wrap code according to coding style (*checkpatch is not* a
> coding style description, but only a tool), so at 80."

Again, I think that the readability should be the deciding factor.

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Krzysztof Kozlowski]

On 18/01/2025 10:43, Dragan Simic wrote:
>>>
>>> Please see the commit bdc48fa11e46 (checkpatch/coding-style: deprecate
>>> 80-column warning, 2020-05-29), which clearly shows that the 80-column
>>> rule is still _preferred_, but no longer _mandatory_.
>>
>> I brought that commit, but nice that you also found it.
>>
>> Still: read the coding style, not checkpatch tool.
>>
>>>>> 80 columns is really not much (for the record, I've been around when
>>>>> using 80x25 _physical_ CRT screens was the norm).
>>>>
>>>> You mistake agreement on dropping strong restriction in 2020 in
>>>> checkpatch, which is "not for years" and even read that commit: "Yes,
>>>> staying withing 80 columns is certainly still _preferred_."
>>>>
>>>> Checkpatch is not coding style. Since when it would be? It's just a
>>>> tool.
>>>>
>>>> And there were more talks and the 80-preference got relaxed yet still
>>>> "not for years" (last talk was 2022?) and sill kernel coding style is
>>>> here specific.
>>>
>>> It's perhaps again about the semantics, this time about the meaning
>>> of "for years".  I don't think there's some strict definition of that
>>> term, so perhaps different people see it differently.
>>>
>>> To get back to the above-mentioned commit bdc48fa11e46, the 80-column
>>> limit has obviously been lifted, putting the new 100-column limit as
>>
>> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the

Repeating myself about because you are not addressing the actual difference.

>> difference? One is a helper tool which people were using blindly and
>> wrapping lines without thinking, claiming that checkpatch told them to
>> do so. Other is the actual coding style.
>>
>> You claim that coding style was changed. This never happened.
> 
> It was obviously changed in the commit bdc48fa11e46, by making the
> 80-column width preferred, instead of if being mandatory.  The way
> I read the changes to the coding style introduced in that commit,
> it's now possible to go over 80 columns, up to 100 columns, _if_
> that actually improves the readability of the source code.

The commit is for checkpatch. Point to the change in coding style. You
are bringing argument for checkpatch, so only a tool, as argument for
coding style. Again, coding style did not change since years.


Best regards,
Krzysztof

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Dragan Simic]

On 2025-01-18 10:52, Krzysztof Kozlowski wrote:
> On 18/01/2025 10:43, Dragan Simic wrote:
>>>> 
>>>> Please see the commit bdc48fa11e46 (checkpatch/coding-style: 
>>>> deprecate
>>>> 80-column warning, 2020-05-29), which clearly shows that the 
>>>> 80-column
>>>> rule is still _preferred_, but no longer _mandatory_.
>>> 
>>> I brought that commit, but nice that you also found it.
>>> 
>>> Still: read the coding style, not checkpatch tool.
>>> 
>>>>>> 80 columns is really not much (for the record, I've been around 
>>>>>> when
>>>>>> using 80x25 _physical_ CRT screens was the norm).
>>>>> 
>>>>> You mistake agreement on dropping strong restriction in 2020 in
>>>>> checkpatch, which is "not for years" and even read that commit: 
>>>>> "Yes,
>>>>> staying withing 80 columns is certainly still _preferred_."
>>>>> 
>>>>> Checkpatch is not coding style. Since when it would be? It's just a
>>>>> tool.
>>>>> 
>>>>> And there were more talks and the 80-preference got relaxed yet 
>>>>> still
>>>>> "not for years" (last talk was 2022?) and sill kernel coding style 
>>>>> is
>>>>> here specific.
>>>> 
>>>> It's perhaps again about the semantics, this time about the meaning
>>>> of "for years".  I don't think there's some strict definition of 
>>>> that
>>>> term, so perhaps different people see it differently.
>>>> 
>>>> To get back to the above-mentioned commit bdc48fa11e46, the 
>>>> 80-column
>>>> limit has obviously been lifted, putting the new 100-column limit as
>>> 
>>> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the
> 
> Repeating myself about because you are not addressing the actual 
> difference.

Please see below.

>>> difference? One is a helper tool which people were using blindly and
>>> wrapping lines without thinking, claiming that checkpatch told them 
>>> to
>>> do so. Other is the actual coding style.
>>> 
>>> You claim that coding style was changed. This never happened.
>> 
>> It was obviously changed in the commit bdc48fa11e46, by making the
>> 80-column width preferred, instead of if being mandatory.  The way
>> I read the changes to the coding style introduced in that commit,
>> it's now possible to go over 80 columns, up to 100 columns, _if_
>> that actually improves the readability of the source code.
> 
> The commit is for checkpatch. Point to the change in coding style. You
> are bringing argument for checkpatch, so only a tool, as argument for
> coding style. Again, coding style did not change since years.

Commit bdc48fa11e46 obviously addresses 
Documentation/process/coding-style.rst
as well, as visible in the quotation from the commit below:

   -The limit on the length of lines is 80 columns and this is a strongly
   -preferred limit.
   -
   -Statements longer than 80 columns will be broken into sensible 
chunks, unless
   -exceeding 80 columns significantly increases readability and does not 
hide
   -information. Descendants are always substantially shorter than the 
parent and
   -are placed substantially to the right. The same applies to function 
headers
   -with a long argument list. However, never break user-visible strings 
such as
   -printk messages, because that breaks the ability to grep for them.
   +The preferred limit on the length of a single line is 80 columns.
   +
   +Statements longer than 80 columns should be broken into sensible 
chunks,
   +unless exceeding 80 columns significantly increases readability and 
does
   +not hide information.
   +
   +Descendants are always substantially shorter than the parent and are
   +are placed substantially to the right.  A very commonly used style
   +is to align descendants to a function open parenthesis.
   +
   +These same rules are applied to function headers with a long argument 
list.
   +
   +However, never break user-visible strings such as printk messages 
because
   +that breaks the ability to grep for them.

I think it's obvious that the 80-column width is no longer _strongly_
preferred, but has been demoted to some kind of a bit weaker preference.

Also, please note that the coding style explicitly says that the 80-
column rule is to be followed "unless exceeding 80 columns significantly
increases readability and does not hide information".

This just reinforces my opinion that the readability is what matters
most when deciding on the column width, instead of following the rules
blindly, both when deciding whether to wrap some lines at column 50,
for example, or to wrap them at column 98.

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Krzysztof Kozlowski]

On 18/01/2025 11:10, Dragan Simic wrote:
> On 2025-01-18 10:52, Krzysztof Kozlowski wrote:
>> On 18/01/2025 10:43, Dragan Simic wrote:
>>>>>
>>>>> Please see the commit bdc48fa11e46 (checkpatch/coding-style: 
>>>>> deprecate
>>>>> 80-column warning, 2020-05-29), which clearly shows that the 
>>>>> 80-column
>>>>> rule is still _preferred_, but no longer _mandatory_.
>>>>
>>>> I brought that commit, but nice that you also found it.
>>>>
>>>> Still: read the coding style, not checkpatch tool.
>>>>
>>>>>>> 80 columns is really not much (for the record, I've been around 
>>>>>>> when
>>>>>>> using 80x25 _physical_ CRT screens was the norm).
>>>>>>
>>>>>> You mistake agreement on dropping strong restriction in 2020 in
>>>>>> checkpatch, which is "not for years" and even read that commit: 
>>>>>> "Yes,
>>>>>> staying withing 80 columns is certainly still _preferred_."
>>>>>>
>>>>>> Checkpatch is not coding style. Since when it would be? It's just a
>>>>>> tool.
>>>>>>
>>>>>> And there were more talks and the 80-preference got relaxed yet 
>>>>>> still
>>>>>> "not for years" (last talk was 2022?) and sill kernel coding style 
>>>>>> is
>>>>>> here specific.
>>>>>
>>>>> It's perhaps again about the semantics, this time about the meaning
>>>>> of "for years".  I don't think there's some strict definition of 
>>>>> that
>>>>> term, so perhaps different people see it differently.
>>>>>
>>>>> To get back to the above-mentioned commit bdc48fa11e46, the 
>>>>> 80-column
>>>>> limit has obviously been lifted, putting the new 100-column limit as
>>>>
>>>> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the
>>
>> Repeating myself about because you are not addressing the actual 
>> difference.
> 
> Please see below.
> 
>>>> difference? One is a helper tool which people were using blindly and
>>>> wrapping lines without thinking, claiming that checkpatch told them 
>>>> to
>>>> do so. Other is the actual coding style.
>>>>
>>>> You claim that coding style was changed. This never happened.
>>>
>>> It was obviously changed in the commit bdc48fa11e46, by making the
>>> 80-column width preferred, instead of if being mandatory.  The way
>>> I read the changes to the coding style introduced in that commit,
>>> it's now possible to go over 80 columns, up to 100 columns, _if_
>>> that actually improves the readability of the source code.
>>
>> The commit is for checkpatch. Point to the change in coding style. You
>> are bringing argument for checkpatch, so only a tool, as argument for
>> coding style. Again, coding style did not change since years.
> 
> Commit bdc48fa11e46 obviously addresses 
> Documentation/process/coding-style.rst
> as well, as visible in the quotation from the commit below:

Yes.

> 
>    -The limit on the length of lines is 80 columns and this is a strongly

80 is here...

>    -preferred limit.
>    -
>    -Statements longer than 80 columns will be broken into sensible 
> chunks, unless
>    -exceeding 80 columns significantly increases readability and does not 
> hide
>    -information. Descendants are always substantially shorter than the 
> parent and
>    -are placed substantially to the right. The same applies to function 
> headers
>    -with a long argument list. However, never break user-visible strings 
> such as
>    -printk messages, because that breaks the ability to grep for them.
>    +The preferred limit on the length of a single line is 80 columns.
>    +
>    +Statements longer than 80 columns should be broken into sensible 

80 is here as well.

So now to your original statement:
" but the 100-column limit
for the kernel code has been in effect for years."

Where is 100? Only in checkpatch. There is no 100 limit in kernel coding
style.

The change in coding style and checkpatch was partially done because of
your understanding: reading checkpatch output as a rule. But this was
never a correct approach and still is not. So whatever checkpatch is
telling you, e.g. "100 column limit", is not coding style. It's only
checkpatch, a tool trying to help you.

> chunks,
>    +unless exceeding 80 columns significantly increases readability and 
> does
>    +not hide information.
>    +
>    +Descendants are always substantially shorter than the parent and are
>    +are placed substantially to the right.  A very commonly used style
>    +is to align descendants to a function open parenthesis.
>    +
>    +These same rules are applied to function headers with a long argument 
> list.
>    +
>    +However, never break user-visible strings such as printk messages 
> because
>    +that breaks the ability to grep for them.
> 
> I think it's obvious that the 80-column width is no longer _strongly_
> preferred, but has been demoted to some kind of a bit weaker preference.

Yes, but this is not what you said before and this is not what I questioned.

> 
> Also, please note that the coding style explicitly says that the 80-
> column rule is to be followed "unless exceeding 80 columns significantly
> increases readability and does not hide information".

I already said it earlier... so yeah, we keep repeating ourselves while
discussing original point claiming now something else than we actually
discuss.

Best regards,
Krzysztof

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Dragan Simic]

On 2025-01-18 11:29, Krzysztof Kozlowski wrote:
> On 18/01/2025 11:10, Dragan Simic wrote:
>> On 2025-01-18 10:52, Krzysztof Kozlowski wrote:
>>> On 18/01/2025 10:43, Dragan Simic wrote:
>>>>>> 
>>>>>> Please see the commit bdc48fa11e46 (checkpatch/coding-style:
>>>>>> deprecate
>>>>>> 80-column warning, 2020-05-29), which clearly shows that the
>>>>>> 80-column
>>>>>> rule is still _preferred_, but no longer _mandatory_.
>>>>> 
>>>>> I brought that commit, but nice that you also found it.
>>>>> 
>>>>> Still: read the coding style, not checkpatch tool.
>>>>> 
>>>>>>>> 80 columns is really not much (for the record, I've been around
>>>>>>>> when
>>>>>>>> using 80x25 _physical_ CRT screens was the norm).
>>>>>>> 
>>>>>>> You mistake agreement on dropping strong restriction in 2020 in
>>>>>>> checkpatch, which is "not for years" and even read that commit:
>>>>>>> "Yes,
>>>>>>> staying withing 80 columns is certainly still _preferred_."
>>>>>>> 
>>>>>>> Checkpatch is not coding style. Since when it would be? It's just 
>>>>>>> a
>>>>>>> tool.
>>>>>>> 
>>>>>>> And there were more talks and the 80-preference got relaxed yet
>>>>>>> still
>>>>>>> "not for years" (last talk was 2022?) and sill kernel coding 
>>>>>>> style
>>>>>>> is
>>>>>>> here specific.
>>>>>> 
>>>>>> It's perhaps again about the semantics, this time about the 
>>>>>> meaning
>>>>>> of "for years".  I don't think there's some strict definition of
>>>>>> that
>>>>>> term, so perhaps different people see it differently.
>>>>>> 
>>>>>> To get back to the above-mentioned commit bdc48fa11e46, the
>>>>>> 80-column
>>>>>> limit has obviously been lifted, putting the new 100-column limit 
>>>>>> as
>>>>> 
>>>>> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the
>>> 
>>> Repeating myself about because you are not addressing the actual
>>> difference.
>> 
>> Please see below.
>> 
>>>>> difference? One is a helper tool which people were using blindly 
>>>>> and
>>>>> wrapping lines without thinking, claiming that checkpatch told them
>>>>> to
>>>>> do so. Other is the actual coding style.
>>>>> 
>>>>> You claim that coding style was changed. This never happened.
>>>> 
>>>> It was obviously changed in the commit bdc48fa11e46, by making the
>>>> 80-column width preferred, instead of if being mandatory.  The way
>>>> I read the changes to the coding style introduced in that commit,
>>>> it's now possible to go over 80 columns, up to 100 columns, _if_
>>>> that actually improves the readability of the source code.
>>> 
>>> The commit is for checkpatch. Point to the change in coding style. 
>>> You
>>> are bringing argument for checkpatch, so only a tool, as argument for
>>> coding style. Again, coding style did not change since years.
>> 
>> Commit bdc48fa11e46 obviously addresses
>> Documentation/process/coding-style.rst
>> as well, as visible in the quotation from the commit below:
> 
> Yes.
> 
>> 
>>    -The limit on the length of lines is 80 columns and this is a 
>> strongly
> 
> 80 is here...
> 
>>    -preferred limit.
>>    -
>>    -Statements longer than 80 columns will be broken into sensible
>> chunks, unless
>>    -exceeding 80 columns significantly increases readability and does 
>> not
>> hide
>>    -information. Descendants are always substantially shorter than the
>> parent and
>>    -are placed substantially to the right. The same applies to 
>> function
>> headers
>>    -with a long argument list. However, never break user-visible 
>> strings
>> such as
>>    -printk messages, because that breaks the ability to grep for them.
>>    +The preferred limit on the length of a single line is 80 columns.
>>    +
>>    +Statements longer than 80 columns should be broken into sensible
> 
> 80 is here as well.
> 
> So now to your original statement:
> " but the 100-column limit
> for the kernel code has been in effect for years."
> 
> Where is 100? Only in checkpatch. There is no 100 limit in kernel 
> coding
> style.

Yes, "100" is in checkpatch only, but the coding style explicitly
says that going over the 80-column limit it fine if it improves
the readability.  Thus, going over the 80 columns has been allowed
"for years", whatever one finds that term to mean, or more precisely
since mid-2020, and having "100" present in checkpatch establishes
"100" as the effective "hard" limit.

> The change in coding style and checkpatch was partially done because of
> your understanding: reading checkpatch output as a rule. But this was
> never a correct approach and still is not. So whatever checkpatch is
> telling you, e.g. "100 column limit", is not coding style. It's only
> checkpatch, a tool trying to help you.

No, that isn't my understanding.  I don't take checkpatch's output
as some kind of mandatory rules; however, what checkpatch does and
suggests should be based on the coding style, and if checkpatch
advises wrongly, it should be fixed instead of being accused to be
invalid and pointless.

Though, in this particular case, checkpatch does it right.  The
coding style explicitly says that going over the 80-column limit
is fine if that improves the readability, and checkpatch follows
that by allowing up to 100 columns.

>> chunks,
>>    +unless exceeding 80 columns significantly increases readability 
>> and
>> does
>>    +not hide information.
>>    +
>>    +Descendants are always substantially shorter than the parent and 
>> are
>>    +are placed substantially to the right.  A very commonly used style
>>    +is to align descendants to a function open parenthesis.
>>    +
>>    +These same rules are applied to function headers with a long 
>> argument
>> list.
>>    +
>>    +However, never break user-visible strings such as printk messages
>> because
>>    +that breaks the ability to grep for them.
>> 
>> I think it's obvious that the 80-column width is no longer _strongly_
>> preferred, but has been demoted to some kind of a bit weaker 
>> preference.
> 
> Yes, but this is not what you said before and this is not what I 
> questioned.

It is, if you read what I wrote above carefully.  The 100-column
width limit has been in effect "for years", and has been defined
by the combination of the coding style and checkpatch.  The former
says that going over 80 columns is fine, and the latter limits that
to 100 columns, to prevent some very long lines.

>> Also, please note that the coding style explicitly says that the 80-
>> column rule is to be followed "unless exceeding 80 columns 
>> significantly
>> increases readability and does not hide information".
> 
> I already said it earlier... so yeah, we keep repeating ourselves while
> discussing original point claiming now something else than we actually
> discuss.

I think it's again about the semantics. :)  Please see above.

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Peter Geis]

On Sat, Jan 18, 2025 at 5:45 AM Dragan Simic <dsimic@manjaro.org> wrote:
>
> On 2025-01-18 11:29, Krzysztof Kozlowski wrote:
> > On 18/01/2025 11:10, Dragan Simic wrote:
> >> On 2025-01-18 10:52, Krzysztof Kozlowski wrote:
> >>> On 18/01/2025 10:43, Dragan Simic wrote:
> >>>>>>
> >>>>>> Please see the commit bdc48fa11e46 (checkpatch/coding-style:
> >>>>>> deprecate
> >>>>>> 80-column warning, 2020-05-29), which clearly shows that the
> >>>>>> 80-column
> >>>>>> rule is still _preferred_, but no longer _mandatory_.
> >>>>>
> >>>>> I brought that commit, but nice that you also found it.
> >>>>>
> >>>>> Still: read the coding style, not checkpatch tool.
> >>>>>
> >>>>>>>> 80 columns is really not much (for the record, I've been around
> >>>>>>>> when
> >>>>>>>> using 80x25 _physical_ CRT screens was the norm).
> >>>>>>>
> >>>>>>> You mistake agreement on dropping strong restriction in 2020 in
> >>>>>>> checkpatch, which is "not for years" and even read that commit:
> >>>>>>> "Yes,
> >>>>>>> staying withing 80 columns is certainly still _preferred_."
> >>>>>>>
> >>>>>>> Checkpatch is not coding style. Since when it would be? It's just
> >>>>>>> a
> >>>>>>> tool.
> >>>>>>>
> >>>>>>> And there were more talks and the 80-preference got relaxed yet
> >>>>>>> still
> >>>>>>> "not for years" (last talk was 2022?) and sill kernel coding
> >>>>>>> style
> >>>>>>> is
> >>>>>>> here specific.
> >>>>>>
> >>>>>> It's perhaps again about the semantics, this time about the
> >>>>>> meaning
> >>>>>> of "for years".  I don't think there's some strict definition of
> >>>>>> that
> >>>>>> term, so perhaps different people see it differently.
> >>>>>>
> >>>>>> To get back to the above-mentioned commit bdc48fa11e46, the
> >>>>>> 80-column
> >>>>>> limit has obviously been lifted, putting the new 100-column limit
> >>>>>> as
> >>>>>
> >>>>> "Lifted" on *CHECKPATCH*, not on coding style. Do you see the
> >>>
> >>> Repeating myself about because you are not addressing the actual
> >>> difference.
> >>
> >> Please see below.
> >>
> >>>>> difference? One is a helper tool which people were using blindly
> >>>>> and
> >>>>> wrapping lines without thinking, claiming that checkpatch told them
> >>>>> to
> >>>>> do so. Other is the actual coding style.
> >>>>>
> >>>>> You claim that coding style was changed. This never happened.
> >>>>
> >>>> It was obviously changed in the commit bdc48fa11e46, by making the
> >>>> 80-column width preferred, instead of if being mandatory.  The way
> >>>> I read the changes to the coding style introduced in that commit,
> >>>> it's now possible to go over 80 columns, up to 100 columns, _if_
> >>>> that actually improves the readability of the source code.
> >>>
> >>> The commit is for checkpatch. Point to the change in coding style.
> >>> You
> >>> are bringing argument for checkpatch, so only a tool, as argument for
> >>> coding style. Again, coding style did not change since years.
> >>
> >> Commit bdc48fa11e46 obviously addresses
> >> Documentation/process/coding-style.rst
> >> as well, as visible in the quotation from the commit below:
> >
> > Yes.
> >
> >>
> >>    -The limit on the length of lines is 80 columns and this is a
> >> strongly
> >
> > 80 is here...
> >
> >>    -preferred limit.
> >>    -
> >>    -Statements longer than 80 columns will be broken into sensible
> >> chunks, unless
> >>    -exceeding 80 columns significantly increases readability and does
> >> not
> >> hide
> >>    -information. Descendants are always substantially shorter than the
> >> parent and
> >>    -are placed substantially to the right. The same applies to
> >> function
> >> headers
> >>    -with a long argument list. However, never break user-visible
> >> strings
> >> such as
> >>    -printk messages, because that breaks the ability to grep for them.
> >>    +The preferred limit on the length of a single line is 80 columns.
> >>    +
> >>    +Statements longer than 80 columns should be broken into sensible
> >
> > 80 is here as well.
> >
> > So now to your original statement:
> > " but the 100-column limit
> > for the kernel code has been in effect for years."
> >
> > Where is 100? Only in checkpatch. There is no 100 limit in kernel
> > coding
> > style.
>
> Yes, "100" is in checkpatch only, but the coding style explicitly
> says that going over the 80-column limit it fine if it improves
> the readability.  Thus, going over the 80 columns has been allowed
> "for years", whatever one finds that term to mean, or more precisely
> since mid-2020, and having "100" present in checkpatch establishes
> "100" as the effective "hard" limit.
>
> > The change in coding style and checkpatch was partially done because of
> > your understanding: reading checkpatch output as a rule. But this was
> > never a correct approach and still is not. So whatever checkpatch is
> > telling you, e.g. "100 column limit", is not coding style. It's only
> > checkpatch, a tool trying to help you.
>
> No, that isn't my understanding.  I don't take checkpatch's output
> as some kind of mandatory rules; however, what checkpatch does and
> suggests should be based on the coding style, and if checkpatch
> advises wrongly, it should be fixed instead of being accused to be
> invalid and pointless.
>
> Though, in this particular case, checkpatch does it right.  The
> coding style explicitly says that going over the 80-column limit
> is fine if that improves the readability, and checkpatch follows
> that by allowing up to 100 columns.
>
> >> chunks,
> >>    +unless exceeding 80 columns significantly increases readability
> >> and
> >> does
> >>    +not hide information.
> >>    +
> >>    +Descendants are always substantially shorter than the parent and
> >> are
> >>    +are placed substantially to the right.  A very commonly used style
> >>    +is to align descendants to a function open parenthesis.
> >>    +
> >>    +These same rules are applied to function headers with a long
> >> argument
> >> list.
> >>    +
> >>    +However, never break user-visible strings such as printk messages
> >> because
> >>    +that breaks the ability to grep for them.
> >>
> >> I think it's obvious that the 80-column width is no longer _strongly_
> >> preferred, but has been demoted to some kind of a bit weaker
> >> preference.
> >
> > Yes, but this is not what you said before and this is not what I
> > questioned.
>
> It is, if you read what I wrote above carefully.  The 100-column
> width limit has been in effect "for years", and has been defined
> by the combination of the coding style and checkpatch.  The former
> says that going over 80 columns is fine, and the latter limits that
> to 100 columns, to prevent some very long lines.

I'd like to say I appreciate the 80 / 100 limit on code, as it pushed
me to separate out my driver code write operations into a separate
function and significantly clean up the code.

I apologize if my submissions aren't perfect the first time around. I
admit I'm still a baby developer, this is not my day job (although I'd
probably love it if it was). This is only the second driver I've
written from scratch (first, if you consider this is the second
iteration of a driver I wrote before my motorcomm driver). I depend
heavily on checkpatch and the feedback from maintainers, as well as
the coding style from similar drivers. The style and coding policies
that employed kernel maintainers and developers have committed to
heart I need to look up every time I submit something.

So, thank you for your feedback.

>
> >> Also, please note that the coding style explicitly says that the 80-
> >> column rule is to be followed "unless exceeding 80 columns
> >> significantly
> >> increases readability and does not hide information".
> >
> > I already said it earlier... so yeah, we keep repeating ourselves while
> > discussing original point claiming now something else than we actually
> > discuss.
>
> I think it's again about the semantics. :)  Please see above.

Re: [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Peter Geis]

On Sat, Jan 18, 2025 at 4:08 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 15/01/2025 02:26, Peter Geis wrote:
> >
> > This is my newly reworked phy driver for the rk3328 usb3 phy. It is
> > based loosely on my original version, but as of now almost nothing of
> > the original driver remains. The main fix here is the discovery of
> > BIT(6) in the interrupt enable grf register fixes the usb3 disconnection
> > detection (mostly). On occasion an unpopulated usb3 hub will take
> > several seconds to disconnect. However this means all of the hack around
> > work to reset the usb core manually is no longer required.
> >
> BTW, RFC for some maintainers means "do not review, work-in-progress".
> For some means "review, but low priority" or "review, but for sure I
> have bugs here". I usually review and then someone responds: "it is not
> for review, it is just RFC", so to avoid my wasted time please always
> mention in cover letter why this is RFC. What do you expect here or why
> this is not ready for review as normal patch.

Thank you, that makes sense. I marked it as RFC as I'm aware it isn't
a perfect solution and there's a lot of undefined values. What I was
looking for here was:
- Review for code quality, so if I'm completely off track I can get to
work fixing it. (Thank you so far)
- Sanity testing from others struggling with the issues it fixes.
- Feedback from USB engineers about the issues remaining.
- Hopefully someone with access to the IP can provide insight into the
magic registers.

>
> Best regards,
> Krzysztof

Re: [RFC PATCH v1 4/6] arm64: dts: rockchip: add rk3328 usb3 phy node

[Diederik de Haas]

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

On Sat Jan 18, 2025 at 9:41 AM CET, Krzysztof Kozlowski wrote:
> On 16/01/2025 17:53, Diederik de Haas wrote:
> > On Thu Jan 16, 2025 at 2:01 PM CET, Krzysztof Kozlowski wrote:
> >> On 15/01/2025 02:26, Peter Geis wrote:
> >>> diff --git a/arch/arm64/boot/dts/rockchip/rk3328.dtsi b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> >>> index 7d992c3c01ce..181a900d41f9 100644
> >>> --- a/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> >>> +++ b/arch/arm64/boot/dts/rockchip/rk3328.dtsi
> >>> @@ -903,6 +903,43 @@ u2phy_host: host-port {
> >>>  		};
> >>>  	};
> >>>  
> >>> +	usb3phy: usb3-phy@ff460000 {
> >>> +		compatible = "rockchip,rk3328-usb3phy";
> >>> +		reg = <0x0 0xff460000 0x0 0x10000>;
> >>> +		clocks = <&cru SCLK_REF_USB3OTG>, <&cru PCLK_USB3PHY_OTG>, <&cru PCLK_USB3PHY_PIPE>;
> >>
> >> Please wrap code according to coding style (checkpatch is not a coding
> >> style description, but only a tool), so at 80.
> > 
> > I'm confused: is it 80 or 100?
> > 
> > I always thought it was 80, but then I saw several patches/commits by
>
> Coding style is clear: it is 80. It also has caveat about code
> readability and several maintainers have their own preference.
>
> > Dragan Simic which deliberately changed code to make use of 100.
> > Being fed up with my own confusion, I submitted a PR to 
> > https://github.com/gregkh/kernel-coding-style/ which got accepted:
> > https://github.com/gregkh/kernel-coding-style/commit/5c21f99dc79883bd0efeba368193180275c9c77a
>
> That's not kernel. That's Greg...

FWIW: what Greg and Linus think/say is relevant to me.

> > So now both the vim plugins code and README say 100.
> > But as noted in my commit message:
> > 
> >   Note that the current upstream 'Linux kernel coding style' does NOT
> >   mention the 100 char limit, but only mentions the preferred max length
> >   of 80.
> > 
> > Or is it 100 for code, but 80 for DeviceTree files and bindings?
>
> From where did you get 100? Checkpatch, right? Kernel coding style is
> clear, there is no discussion, no mentioning 100:
>
> "The preferred limit on the length of a single line is 80 columns. "
>
> So to be clear: all DTS, all DT bindings, all code maintained by me and
> some maintainers follows above (and further - there is caveat)
> instruction from coding style. Some maintainers follow other rules and
> that's fine.

But indeed, before Greg or Linus (likely) see it, a patch submitter
needs to convince the (subsystem) maintainer that it is an improvement.

Thanks for the clarification :-)

Cheers,
  Diederik

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

Re: (subset) [RFC PATCH v1 0/6] rockchip: add a functional usb3 phy driver for rk3328

[Heiko Stuebner]

On Wed, 15 Jan 2025 01:26:21 +0000, Peter Geis wrote:
> This is my newly reworked phy driver for the rk3328 usb3 phy. It is
> based loosely on my original version, but as of now almost nothing of
> the original driver remains. The main fix here is the discovery of
> BIT(6) in the interrupt enable grf register fixes the usb3 disconnection
> detection (mostly). On occasion an unpopulated usb3 hub will take
> several seconds to disconnect. However this means all of the hack around
> work to reset the usb core manually is no longer required.
> 
> [...]

Applied, thanks!

[1/6] clk: rockchip: fix wrong clk_ref_usb3otg parent for rk3328
      commit: a9e60f1ffe1ca57d6af6a2573e2f950e76efbf5b

Best regards,
-- 
Heiko Stuebner <heiko@sntech.de>