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

From: Andrew Lunn <andrew@lunn.ch>
To: "Uwe Kleine-König" <ukleinek@debian.org>
Cc: Rob Herring <robh+dt@kernel.org>,
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
	Conor Dooley <conor+dt@kernel.org>,
	Heiko Stuebner <heiko@sntech.de>,
	devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	linux-rockchip@lists.infradead.org
Subject: Re: [PATCH 2/2] arm64: dts: rockchip: Add basic support for QNAP TS-433
Date: Wed, 28 Feb 2024 14:53:26 +0100	[thread overview]
Message-ID: <3295af58-4015-4962-91a0-87b70f18754e@lunn.ch> (raw)
In-Reply-To: <lcvokzcmifxl7skfz2h2shewuou7xpazuhtpnpkgtcyejcfgcy@vvdn5ypyklsx>

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