[PATCH 0/2] arm64: Add basic support for QNAP TS-433

[PATCH 0/2] arm64: Add basic support for QNAP TS-433

[Uwe Kleine-König]

Hello,

this is an initial dts to support QNAP's TS-433 NAS. It's enough to
start the debian installer with a custom built kernel (original Debian
kernel fails to open a console, didn't debug that yet) and access HD,
eMMC, RTC and network.

There are still some missing bits and pieces, but to make people aware
there are efforts to support this machine and so prevent duplicate work,
I think it makes sense to add the dts in its current form already.

Best regards
Uwe

Uwe Kleine-König (2):
  dt-bindings: arm: rockchip: Add QNAP TS-433
  arm64: dts: rockchip: Add basic support for QNAP TS-433

 .../devicetree/bindings/arm/rockchip.yaml     |  5 ++
 arch/arm64/boot/dts/rockchip/Makefile         |  1 +
 .../boot/dts/rockchip/rk3568-qnap-ts433.dts   | 86 +++++++++++++++++++
 3 files changed, 92 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts

base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
-- 
2.43.0

[PATCH 1/2] dt-bindings: arm: rockchip: Add QNAP TS-433

[Uwe Kleine-König]

This is a SOHO NAS with 4 hd bays and 4 GB of RAM.

Signed-off-by: Uwe Kleine-König <ukleinek@debian.org>
---
 Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/rockchip.yaml b/Documentation/devicetree/bindings/arm/rockchip.yaml
index 5cf5cbef2cf5..389bf0c7d702 100644
--- a/Documentation/devicetree/bindings/arm/rockchip.yaml
+++ b/Documentation/devicetree/bindings/arm/rockchip.yaml
@@ -697,6 +697,11 @@ properties:
               - powkiddy,x55
           - const: rockchip,rk3566
 
+      - description: QNAP TS-433-4G 4-Bay NAS
+        items:
+          - const: qnap,ts433
+          - const: rockchip,rk3568
+
       - description: Radxa Compute Module 3(CM3)
         items:
           - enum:
-- 
2.43.0

[PATCH 2/2] arm64: dts: rockchip: Add basic support for QNAP TS-433

[Uwe Kleine-König]

This is enough to make eMMC, networking, UART (console), RTC and a hard
disk accessible. Still missing are (at least): USB, LEDs, regulators,
fan.

Signed-off-by: Uwe Kleine-König <ukleinek@debian.org>
---
 arch/arm64/boot/dts/rockchip/Makefile         |  1 +
 .../boot/dts/rockchip/rk3568-qnap-ts433.dts   | 86 +++++++++++++++++++
 2 files changed, 87 insertions(+)
 create mode 100644 arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts

diff --git a/arch/arm64/boot/dts/rockchip/Makefile b/arch/arm64/boot/dts/rockchip/Makefile
index a7b30e11beaf..a28a6d445929 100644
--- a/arch/arm64/boot/dts/rockchip/Makefile
+++ b/arch/arm64/boot/dts/rockchip/Makefile
@@ -98,6 +98,7 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-lubancat-2.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-nanopi-r5c.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-nanopi-r5s.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-odroid-m1.dtb
+dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-qnap-ts433.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-radxa-e25.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-roc-pc.dtb
 dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3568-rock-3a.dtb
diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
new file mode 100644
index 000000000000..ba7137f80075
--- /dev/null
+++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
@@ -0,0 +1,86 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Copyright (c) 2021 Rockchip Electronics Co., Ltd.
+ * Copyright (c) 2024 Uwe Kleine-König
+ */
+
+/dts-v1/;
+
+#include <dt-bindings/gpio/gpio.h>
+#include "rk3568.dtsi"
+
+/ {
+	model = "Qnap TS-433-4G NAS System 4-Bay";
+	compatible = "qnap,ts433", "rockchip,rk3568";
+};
+
+&i2c0 {
+	pmic@20 {
+		compatible = "rockchip,rk809";
+		reg = <0x20>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
+	};
+};
+
+&i2c1 {
+	status = "okay";
+
+	rtc@51 {
+		compatible = "microcrystal,rv8263";
+		reg = <0x51>;
+		wakeup-source;
+	};
+};
+
+&gmac0 {
+	assigned-clocks = <&cru SCLK_GMAC0_RX_TX>, <&cru SCLK_GMAC0>;
+	assigned-clock-parents = <&cru SCLK_GMAC0_RGMII_SPEED>, <&cru CLK_MAC0_2TOP>;
+	assigned-clock-rates = <0>, <125000000>;
+	clock_in_out = "output";
+	phy-handle = <&rgmii_phy0>;
+	phy-mode = "rgmii";
+	pinctrl-names = "default";
+	pinctrl-0 = <&gmac0_miim
+		     &gmac0_tx_bus2
+		     &gmac0_rx_bus2
+		     &gmac0_rgmii_clk
+		     &gmac0_rgmii_bus>;
+	rx_delay = <0x2f>;
+	tx_delay = <0x3c>;
+	status = "okay";
+};
+
+&mdio0 {
+	rgmii_phy0: ethernet-phy@0 {
+		compatible = "ethernet-phy-ieee802.3-c22";
+		reg = <0x0>;
+	};
+};
+
+&pcie30phy {
+	status = "okay";
+};
+
+&pcie3x1 {
+	/* The downstream dts has: rockchip,bifurcation, XXX: find out what this is about */
+	reset-gpios = <&gpio0 RK_PC7 GPIO_ACTIVE_HIGH>;
+	status = "okay";
+};
+
+&sdhci {
+	bus-width = <8>;
+	max-frequency = <200000000>;
+	non-removable;
+	status = "okay";
+};
+
+/*
+ * Pins available on CN2 connector at TTL voltage level (3V3).
+ * ,_  _.
+ * |1234|  1=TX 2=VCC
+ * `----'  3=RX 4=GND
+ */
+&uart2 {
+	status = "okay";
+};
-- 
2.43.0

Re: [PATCH 0/2] arm64: Add basic support for QNAP TS-433

[Heiko Stübner]

Am Dienstag, 27. Februar 2024, 12:52:35 CET schrieb Uwe Kleine-König:
> Hello,
> 
> this is an initial dts to support QNAP's TS-433 NAS. It's enough to
> start the debian installer with a custom built kernel (original Debian
> kernel fails to open a console, didn't debug that yet) and access HD,
> eMMC, RTC and network.
> 
> There are still some missing bits and pieces, but to make people aware
> there are efforts to support this machine and so prevent duplicate work,
> I think it makes sense to add the dts in its current form already.

woohoo and yes we're adding support for not 100% supported boards
all the time, so adding basic support and extending it later is fully ok.


For people thinking about "following along at home", how much voodoo
is involved to make the device boot said Debian installer? ;-)


Heiko

Re: [PATCH 0/2] arm64: Add basic support for QNAP TS-433

[Rob Herring]

On Tue, 27 Feb 2024 12:52:35 +0100, Uwe Kleine-König wrote:
> Hello,
> 
> this is an initial dts to support QNAP's TS-433 NAS. It's enough to
> start the debian installer with a custom built kernel (original Debian
> kernel fails to open a console, didn't debug that yet) and access HD,
> eMMC, RTC and network.
> 
> There are still some missing bits and pieces, but to make people aware
> there are efforts to support this machine and so prevent duplicate work,
> I think it makes sense to add the dts in its current form already.
> 
> Best regards
> Uwe
> 
> Uwe Kleine-König (2):
>   dt-bindings: arm: rockchip: Add QNAP TS-433
>   arm64: dts: rockchip: Add basic support for QNAP TS-433
> 
>  .../devicetree/bindings/arm/rockchip.yaml     |  5 ++
>  arch/arm64/boot/dts/rockchip/Makefile         |  1 +
>  .../boot/dts/rockchip/rk3568-qnap-ts433.dts   | 86 +++++++++++++++++++
>  3 files changed, 92 insertions(+)
>  create mode 100644 arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
> 
> base-commit: 6613476e225e090cc9aad49be7fa504e290dd33d
> --
> 2.43.0
> 
> 


My bot found new DT warnings on the .dts files added or changed in this
series.

Some warnings may be from an existing SoC .dtsi. Or perhaps the warnings
are fixed by another series. Ultimately, it is up to the platform
maintainer whether these warnings are acceptable or not.

If you already ran DT checks and didn't see these error(s), then
make sure dt-schema is up to date:

  pip3 install dtschema --upgrade


New warnings running 'make CHECK_DTBS=y rockchip/rk3568-qnap-ts433.dtb' for cover.1709034476.git.ukleinek@debian.org:

arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dtb: video-codec@fdea0400: 'interrupt-names' is a required property
	from schema $id: http://devicetree.org/schemas/media/rockchip-vpu.yaml#
arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dtb: i2s@fe420000: reset-names:0: 'm' is not one of ['tx-m', 'rx-m']
	from schema $id: http://devicetree.org/schemas/sound/rockchip,i2s-tdm.yaml#

Re: [PATCH 0/2] arm64: Add basic support for QNAP TS-433

[Uwe Kleine-König]

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

Hello Heiko,

On Tue, Feb 27, 2024 at 01:26:19PM +0100, Heiko Stübner wrote:
> Am Dienstag, 27. Februar 2024, 12:52:35 CET schrieb Uwe Kleine-König:
> > this is an initial dts to support QNAP's TS-433 NAS. It's enough to
> > start the debian installer with a custom built kernel (original Debian
> > kernel fails to open a console, didn't debug that yet) and access HD,
> > eMMC, RTC and network.
> > 
> > There are still some missing bits and pieces, but to make people aware
> > there are efforts to support this machine and so prevent duplicate work,
> > I think it makes sense to add the dts in its current form already.
> 
> woohoo and yes we're adding support for not 100% supported boards
> all the time, so adding basic support and extending it later is fully ok.
> 
> For people thinking about "following along at home", how much voodoo
> is involved to make the device boot said Debian installer? ;-)

I'd ask these people for some patience as I intend to document the
details in the Debian wiki.

For those who are not that patient, here comes the short version:
I compiled v6.8-rc1 with this patch, put the resulting kernel image
(Image) and device tree into /srv/tftp, together with the initrd[1] from
Debian and then typed in U-boot (using the vendor version):

	setenv laptop 192.168.144.173
	setenv bootargs ${bootargs} clk_ignore_unused ignore_loglevel
	i2c dev 0; i2c mw 0x20 0xf4 0x10; i2c mw 0x40 0x00 0xa3; run phy_cmd
	dhcp ${fdt_addr_r} ${laptop}:rk3568-qnap-ts433.dtb
	dhcp ${kernel_addr_r} ${laptop}:Image
	dhcp 0x0b100000 ${laptop}:debian/20240222-02:09/initrd.gz
	booti ${kernel_addr_r} 0x0b100000:$filesize ${fdt_addr_r}

Feel free to ping me on irc if you have further questions.

Best regards
Uwe

[1] I used
https://d-i.debian.org/daily-images/arm64/20240222-02:09/netboot/debian-installer/arm64/initrd.gz

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 0/2] arm64: Add basic support for QNAP TS-433

[Uwe Kleine-König]

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

Hello Rob,

On Tue, Feb 27, 2024 at 07:45:06AM -0600, Rob Herring wrote:
> New warnings running 'make CHECK_DTBS=y rockchip/rk3568-qnap-ts433.dtb' for cover.1709034476.git.ukleinek@debian.org:
> 
> arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dtb: video-codec@fdea0400: 'interrupt-names' is a required property
> 	from schema $id: http://devicetree.org/schemas/media/rockchip-vpu.yaml#
> arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dtb: i2s@fe420000: reset-names:0: 'm' is not one of ['tx-m', 'rx-m']
> 	from schema $id: http://devicetree.org/schemas/sound/rockchip,i2s-tdm.yaml#

Yes, I saw these, too. But given that the added dts doesn't touch these
two nodes, I thought this to be a problem of rk3568.dtsi that I can
ignore for this series.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 0/2] arm64: Add basic support for QNAP TS-433

[Heiko Stübner]

Am Dienstag, 27. Februar 2024, 14:55:57 CET schrieb Uwe Kleine-König:
> Hello Rob,
> 
> On Tue, Feb 27, 2024 at 07:45:06AM -0600, Rob Herring wrote:
> > New warnings running 'make CHECK_DTBS=y rockchip/rk3568-qnap-ts433.dtb' for cover.1709034476.git.ukleinek@debian.org:
> > 
> > arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dtb: video-codec@fdea0400: 'interrupt-names' is a required property
> > 	from schema $id: http://devicetree.org/schemas/media/rockchip-vpu.yaml#
> > arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dtb: i2s@fe420000: reset-names:0: 'm' is not one of ['tx-m', 'rx-m']
> > 	from schema $id: http://devicetree.org/schemas/sound/rockchip,i2s-tdm.yaml#
> 
> Yes, I saw these, too. But given that the added dts doesn't touch these
> two nodes, I thought this to be a problem of rk3568.dtsi that I can
> ignore for this series.

I think I found suitable fixes for those in [0].

The question would be, if the bot finding errors causes the patch to get
removed from the review queue, because the binding addition still would
profit from an Ack :-)


Heiko


[0] https://lore.kernel.org/linux-rockchip/20240227173526.710056-1-heiko@sntech.de/

Re: [PATCH 2/2] arm64: dts: rockchip: Add basic support for QNAP TS-433

[Andrew Lunn]

> +&gmac0 {
> +	assigned-clocks = <&cru SCLK_GMAC0_RX_TX>, <&cru SCLK_GMAC0>;
> +	assigned-clock-parents = <&cru SCLK_GMAC0_RGMII_SPEED>, <&cru CLK_MAC0_2TOP>;
> +	assigned-clock-rates = <0>, <125000000>;
> +	clock_in_out = "output";
> +	phy-handle = <&rgmii_phy0>;
> +	phy-mode = "rgmii";
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&gmac0_miim
> +		     &gmac0_tx_bus2
> +		     &gmac0_rx_bus2
> +		     &gmac0_rgmii_clk
> +		     &gmac0_rgmii_bus>;
> +	rx_delay = <0x2f>;
> +	tx_delay = <0x3c>;

Have you tried phy-mode = "rgmii-id"; and not have these two _delay
settings?

In general, we try to have the PHY do the delay, not the MAC, so that
all devices work the same.

	Andrew

Re: [PATCH 2/2] arm64: dts: rockchip: Add basic support for QNAP TS-433

[Uwe Kleine-König]

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

On Tue, Feb 27, 2024 at 10:00:48PM +0100, Andrew Lunn wrote:
> > +&gmac0 {
> > +	assigned-clocks = <&cru SCLK_GMAC0_RX_TX>, <&cru SCLK_GMAC0>;
> > +	assigned-clock-parents = <&cru SCLK_GMAC0_RGMII_SPEED>, <&cru CLK_MAC0_2TOP>;
> > +	assigned-clock-rates = <0>, <125000000>;
> > +	clock_in_out = "output";
> > +	phy-handle = <&rgmii_phy0>;
> > +	phy-mode = "rgmii";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&gmac0_miim
> > +		     &gmac0_tx_bus2
> > +		     &gmac0_rx_bus2
> > +		     &gmac0_rgmii_clk
> > +		     &gmac0_rgmii_bus>;
> > +	rx_delay = <0x2f>;
> > +	tx_delay = <0x3c>;
> 
> Have you tried phy-mode = "rgmii-id"; and not have these two _delay
> settings?

I didnt' up to now. I applied the following on top of my dts:

diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
index ba7137f80075..a8747d9f36da 100644
--- a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
+++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
@@ -39,15 +39,13 @@ &gmac0 {
 	assigned-clock-rates = <0>, <125000000>;
 	clock_in_out = "output";
 	phy-handle = <&rgmii_phy0>;
-	phy-mode = "rgmii";
+	phy-mode = "rgmii-id";
 	pinctrl-names = "default";
 	pinctrl-0 = <&gmac0_miim
 		     &gmac0_tx_bus2
 		     &gmac0_rx_bus2
 		     &gmac0_rgmii_clk
 		     &gmac0_rgmii_bus>;
-	rx_delay = <0x2f>;
-	tx_delay = <0x3c>;
 	status = "okay";
 };
 
and this makes the machine unable to complete dhcp. I see the requests
and replies on the dhcp server side, but the machine tells me not to
receive a dhcp reply. So the above patch breaks the receive path.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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

Re: [PATCH 1/2] dt-bindings: arm: rockchip: Add QNAP TS-433

[Krzysztof Kozlowski]

On 27/02/2024 12:52, Uwe Kleine-König wrote:
> This is a SOHO NAS with 4 hd bays and 4 GB of RAM.
> 
> Signed-off-by: Uwe Kleine-König <ukleinek@debian.org>
> ---
>  Documentation/devicetree/bindings/arm/rockchip.yaml | 5 +++++
>  1 file changed, 5 insertions(+)

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH 0/2] arm64: Add basic support for QNAP TS-433

[Heiko Stuebner]

On Tue, 27 Feb 2024 12:52:35 +0100, Uwe Kleine-König wrote:
> this is an initial dts to support QNAP's TS-433 NAS. It's enough to
> start the debian installer with a custom built kernel (original Debian
> kernel fails to open a console, didn't debug that yet) and access HD,
> eMMC, RTC and network.
> 
> There are still some missing bits and pieces, but to make people aware
> there are efforts to support this machine and so prevent duplicate work,
> I think it makes sense to add the dts in its current form already.
> 
> [...]

Applied, thanks!

[1/2] dt-bindings: arm: rockchip: Add QNAP TS-433
      commit: 0660dd951e1a09e155e31732fbdf735112b5d6c4
[2/2] arm64: dts: rockchip: Add basic support for QNAP TS-433
      commit: 9da1c0327d58e0cfc2d86799192585ddeb1ae4a0

Moved &gmac above &i2c* - alphabetical sorting and all


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

Re: [PATCH 2/2] arm64: dts: rockchip: Add basic support for QNAP TS-433

[Andrew Lunn]

On Wed, Feb 28, 2024 at 08:23:33AM +0100, Uwe Kleine-König wrote:
> On Tue, Feb 27, 2024 at 10:00:48PM +0100, Andrew Lunn wrote:
> > > +&gmac0 {
> > > +	assigned-clocks = <&cru SCLK_GMAC0_RX_TX>, <&cru SCLK_GMAC0>;
> > > +	assigned-clock-parents = <&cru SCLK_GMAC0_RGMII_SPEED>, <&cru CLK_MAC0_2TOP>;
> > > +	assigned-clock-rates = <0>, <125000000>;
> > > +	clock_in_out = "output";
> > > +	phy-handle = <&rgmii_phy0>;
> > > +	phy-mode = "rgmii";
> > > +	pinctrl-names = "default";
> > > +	pinctrl-0 = <&gmac0_miim
> > > +		     &gmac0_tx_bus2
> > > +		     &gmac0_rx_bus2
> > > +		     &gmac0_rgmii_clk
> > > +		     &gmac0_rgmii_bus>;
> > > +	rx_delay = <0x2f>;
> > > +	tx_delay = <0x3c>;
> > 
> > Have you tried phy-mode = "rgmii-id"; and not have these two _delay
> > settings?
> 
> I didnt' up to now. I applied the following on top of my dts:
> 
> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
> index ba7137f80075..a8747d9f36da 100644
> --- a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
> +++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
> @@ -39,15 +39,13 @@ &gmac0 {
>  	assigned-clock-rates = <0>, <125000000>;
>  	clock_in_out = "output";
>  	phy-handle = <&rgmii_phy0>;
> -	phy-mode = "rgmii";
> +	phy-mode = "rgmii-id";
>  	pinctrl-names = "default";
>  	pinctrl-0 = <&gmac0_miim
>  		     &gmac0_tx_bus2
>  		     &gmac0_rx_bus2
>  		     &gmac0_rgmii_clk
>  		     &gmac0_rgmii_bus>;
> -	rx_delay = <0x2f>;
> -	tx_delay = <0x3c>;
>  	status = "okay";
>  };
>  
> and this makes the machine unable to complete dhcp. I see the requests
> and replies on the dhcp server side, but the machine tells me not to
> receive a dhcp reply. So the above patch breaks the receive path.

O.K.

This binding is particularly bad. What does 0x3c mean? Generally, we
use rx-internal-delay-ps making it clear what the value means.

RGMII requires a 2ns delay between the clock and the data. Generally,
we have the MAC insert 0 delay, and request the PHY add the 2ns delay
by specifying "rgmii-id". Sometimes you need to fine tune this,
because of the length of the tracks etc. You can then do that fine
tuning either in the PHY, or the MAC.

Looking at the binding:

  tx_delay:
    description: Delay value for TXD timing.
    $ref: /schemas/types.yaml#/definitions/uint32
    minimum: 0
    maximum: 0x7F
    default: 0x30

  rx_delay:
    description: Delay value for RXD timing.
    $ref: /schemas/types.yaml#/definitions/uint32
    minimum: 0
    maximum: 0x7F
    default: 0x10

For your board, you have increased both values from there default
values. My guess is 0x30 tx_delay is 2ns, and 0x10 rx_delay is also
2ns. 

So maybe try:

rx_delay = <0x1f>;
tx_delay = <0x0c>;

combined with rmgii-id.

	 Andrew

Re: [PATCH 2/2] arm64: dts: rockchip: Add basic support for QNAP TS-433

[Uwe Kleine-König]

On 28.02.24 14:53, Andrew Lunn wrote:
> On Wed, Feb 28, 2024 at 08:23:33AM +0100, Uwe Kleine-König wrote:
>> On Tue, Feb 27, 2024 at 10:00:48PM +0100, Andrew Lunn wrote:
>>>> +&gmac0 {
>>>> +	assigned-clocks = <&cru SCLK_GMAC0_RX_TX>, <&cru SCLK_GMAC0>;
>>>> +	assigned-clock-parents = <&cru SCLK_GMAC0_RGMII_SPEED>, <&cru CLK_MAC0_2TOP>;
>>>> +	assigned-clock-rates = <0>, <125000000>;
>>>> +	clock_in_out = "output";
>>>> +	phy-handle = <&rgmii_phy0>;
>>>> +	phy-mode = "rgmii";
>>>> +	pinctrl-names = "default";
>>>> +	pinctrl-0 = <&gmac0_miim
>>>> +		     &gmac0_tx_bus2
>>>> +		     &gmac0_rx_bus2
>>>> +		     &gmac0_rgmii_clk
>>>> +		     &gmac0_rgmii_bus>;
>>>> +	rx_delay = <0x2f>;
>>>> +	tx_delay = <0x3c>;
>>>
>>> Have you tried phy-mode = "rgmii-id"; and not have these two _delay
>>> settings?
>>
>> I didnt' up to now. I applied the following on top of my dts:
>>
>> diff --git a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
>> index ba7137f80075..a8747d9f36da 100644
>> --- a/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
>> +++ b/arch/arm64/boot/dts/rockchip/rk3568-qnap-ts433.dts
>> @@ -39,15 +39,13 @@ &gmac0 {
>>   	assigned-clock-rates = <0>, <125000000>;
>>   	clock_in_out = "output";
>>   	phy-handle = <&rgmii_phy0>;
>> -	phy-mode = "rgmii";
>> +	phy-mode = "rgmii-id";
>>   	pinctrl-names = "default";
>>   	pinctrl-0 = <&gmac0_miim
>>   		     &gmac0_tx_bus2
>>   		     &gmac0_rx_bus2
>>   		     &gmac0_rgmii_clk
>>   		     &gmac0_rgmii_bus>;
>> -	rx_delay = <0x2f>;
>> -	tx_delay = <0x3c>;
>>   	status = "okay";
>>   };
>>   
>> and this makes the machine unable to complete dhcp. I see the requests
>> and replies on the dhcp server side, but the machine tells me not to
>> receive a dhcp reply. So the above patch breaks the receive path.
> 
> O.K.
> 
> This binding is particularly bad. What does 0x3c mean? Generally, we
> use rx-internal-delay-ps making it clear what the value means.
> 
> RGMII requires a 2ns delay between the clock and the data. Generally,
> we have the MAC insert 0 delay, and request the PHY add the 2ns delay
> by specifying "rgmii-id". Sometimes you need to fine tune this,
> because of the length of the tracks etc. You can then do that fine
> tuning either in the PHY, or the MAC.
> 
> Looking at the binding:
> 
>    tx_delay:
>      description: Delay value for TXD timing.
>      $ref: /schemas/types.yaml#/definitions/uint32
>      minimum: 0
>      maximum: 0x7F
>      default: 0x30
> 
>    rx_delay:
>      description: Delay value for RXD timing.
>      $ref: /schemas/types.yaml#/definitions/uint32
>      minimum: 0
>      maximum: 0x7F
>      default: 0x10
> 
> For your board, you have increased both values from there default
> values. My guess is 0x30 tx_delay is 2ns, and 0x10 rx_delay is also
> 2ns.
> 
> So maybe try:
> 
> rx_delay = <0x1f>;
> tx_delay = <0x0c>;
> 
> combined with rmgii-id.

With the right phy driver (MOTORCOMM_PHY) enabled, it works also without 
specifying {rx,tx}_delay. Thanks to Oleksij for the relevant hint. I'll 
switch to that then.

Best regards
Uwe