[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