Date:   Fri, 17 Feb 2023 14:30:13 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
Cc:     Lee Jones <lee@kernel.org>, Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Emil Renner Berthing <kernel@esmil.dk>,
        Conor Dooley <conor@kernel.org>,
        Palmer Dabbelt <palmer@dabbelt.com>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Albert Ou <aou@eecs.berkeley.edu>,
        Giuseppe Cavallaro <peppe.cavallaro@st.com>,
        Alexandre Torgue <alexandre.torgue@foss.st.com>,
        Jose Abreu <joabreu@synopsys.com>,
        Maxime Coquelin <mcoquelin.stm32@gmail.com>,
        Richard Cochran <richardcochran@gmail.com>,
        Sagar Kadam <sagar.kadam@sifive.com>,
        Yanhong Wang <yanhong.wang@starfivetech.com>,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, linux-riscv@lists.infradead.org,
        linux-stm32@st-md-mailman.stormreply.com,
        linux-arm-kernel@lists.infradead.org, kernel@collabora.com
Subject: Re: [PATCH 07/12] dt-bindings: net: Add StarFive JH7100 SoC
Message-ID: <Y++BZWhJm1LpdrA9@lunn.ch>
References: <20230211031821.976408-1-cristian.ciocaltea@collabora.com>
 <20230211031821.976408-8-cristian.ciocaltea@collabora.com>
 <Y+e74UIV/Td91lKB@lunn.ch>
 <586971af-2d78-456d-a605-6c7b2aefda91@collabora.com>
 <Y+zXv90rGfQupjPP@lunn.ch>
 <cfa0f980-4bb6-4419-909c-3fce697cf8f9@collabora.com>
 <Y+5t4Jlb0ytw40pu@lunn.ch>
 <a824a7f6-0a62-7cab-180b-f20297311a2b@collabora.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <a824a7f6-0a62-7cab-180b-f20297311a2b@collabora.com>
Precedence: bulk

> > I would actually say it shows we don't understand what is going on
> > with delays. "rgmii" is not every often the correct value. The fact it
> > works suggests the MAC is adding delays.
> > 
> > What value are you using for starfive,gtxclk-dlychain ?
> 
> This is set to '4' in patch 12/12.
> 
> > Try 0 and then "rgmii-id"
> 
> I made some more tests and it seems the only stable configuration is "rgmii"
> with "starfive,gtxclk-dlychain" set to 4:
> 
> phy-mode | dlychain | status
> ---------+----------+--------------------------------------------
> rgmii    |        4 | OK (no issues observed)
> rgmii-id |        4 | BROKEN (errors reported [1])
> rgmii    |        0 | UNRELIABLE (no errors, but frequent stalls)
> rgmii-id |        0 | BROKEN (errors reported)
> 
> [1] Reported errors in case of BROKEN status:
> $ grep '' /sys/class/net/eth0/statistics/* | grep -v ':0$'

Thanks for the testing.

So it seems like something is adding delays when it probably should
not. Ideally we want to know what.

There is a danger here, something which has happened in the past. A
PHY which ignored "rgmii" and actually did power on defaults which was
"rgmii-id". As a result, lots of boards put "rmgii" in there DT blob,
which 'worked'. Until a board came along which really did need
"rgmii". The developer bringing that board up debugged the PHY, found
the problem and made it respect "rgmii" so their board worked. And the
fix broke a number of 'working' boards which had the wrong "rgmii"
instead of "rgmii-id".

So you have a choice. Go with 4 and "rgmii", but put in a big fat
warning, "Works somehow but is technically wrong and will probably
break sometime in the future". Or try to understand what is really
going on here, were are the delays coming from, and fix the issue.

      Andrew