netdev - Re: 回覆: [PATCH v2 05/10] ARM: dts: aspeed: system1: Add RGMII support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <8042c67c-04d3-41c0-9e88-8ce99839f70b@lunn.ch>
Date: Thu, 9 Jan 2025 14:21:23 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jacky Chou <jacky_chou@...eedtech.com>
Cc: Ninad Palsule <ninad@...ux.ibm.com>,
	"andrew+netdev@...n.ch" <andrew+netdev@...n.ch>,
	"andrew@...econstruct.com.au" <andrew@...econstruct.com.au>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>,
	"davem@...emloft.net" <davem@...emloft.net>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"eajames@...ux.ibm.com" <eajames@...ux.ibm.com>,
	"edumazet@...gle.com" <edumazet@...gle.com>,
	"joel@....id.au" <joel@....id.au>,
	"krzk+dt@...nel.org" <krzk+dt@...nel.org>,
	"kuba@...nel.org" <kuba@...nel.org>,
	"linux-arm-kernel@...ts.infradead.org" <linux-arm-kernel@...ts.infradead.org>,
	"linux-aspeed@...ts.ozlabs.org" <linux-aspeed@...ts.ozlabs.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"minyard@....org" <minyard@....org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"openipmi-developer@...ts.sourceforge.net" <openipmi-developer@...ts.sourceforge.net>,
	"pabeni@...hat.com" <pabeni@...hat.com>,
	"ratbert@...aday-tech.com" <ratbert@...aday-tech.com>,
	"robh@...nel.org" <robh@...nel.org>
Subject: Re: 回覆: [PATCH v2 05/10] ARM:
 dts: aspeed: system1: Add RGMII support

On Thu, Jan 09, 2025 at 10:33:20AM +0000, Jacky Chou wrote:
> Hi Andrew,
> 
> > > There are around 11 boards in Aspeed SOC with phy-mode set to "rgmii"
> > > (some of them are mac0&1 and others are mac2&3). "rgmii-rxid" is only
> > mine.
> > >
> > > No one in aspeed SOC using "rgmii-id".
> > 
> > O.K, so we have to be careful how we fix this. But the fact they are all equally
> > broken might help here.
> > 
> > > > Humm, interesting. Looking at ftgmac100.c, i don't see where you
> > > > configure the RGMII delays in the MAC?
> > 
> > This is going to be important. How are delays configured if they are not in the
> > MAC driver?
> 
> The RGMII delay is adjusted on clk-ast2600 driver. Please refer to the following link.
> https://github.com/AspeedTech-BMC/linux/blob/f52a0cf7c475dc576482db46759e2d854c1f36e4/drivers/clk/clk-ast2600.c#L1008

O.K. So in your vendor tree, you have additional DT properties
mac1-clk-delay, mac2-clk-delay, mac3-clk-delay. Which is fine, you can
do whatever you want in your vendor tree, it is all open source.

But for mainline, this will not be accepted. We have standard
properties defined for configuring MAC delays in picoseconds:

        rx-internal-delay-ps:
          description:
            RGMII Receive Clock Delay defined in pico seconds. This is used for
            controllers that have configurable RX internal delays. If this
            property is present then the MAC applies the RX delay.
        tx-internal-delay-ps:
          description:
            RGMII Transmit Clock Delay defined in pico seconds. This is used for
            controllers that have configurable TX internal delays. If this
            property is present then the MAC applies the TX delay.


You need to use these, and in the MAC driver, not a clock driver. That
is also part of the issue. Your MAC driver looks correct, it just
silently passes phy-mode to the PHY just like every other MAC
driver. But you have some code hidden away in the clock controller
which adds the delays. If this was in the MAC driver, where it should
be, this broken behaviour would of been found earlier.

So, looking at mainline, i see where you create a gated clock. But
what i do not see is where you set the delays.

How does this work in mainline? Is there more hidden code somewhere
setting the ASPEED_MAC12_CLK_DLY register?

	Andrew