linux-kernel - Re: [PATCH net-next v4 4/4] net: ftgmac100: Add RGMII delay support for AST2600

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] [day] [month] [year] [list]
Message-ID: <8c6e1286-782a-44c5-ac9b-21d1db177d6e@lunn.ch>
Date: Mon, 10 Nov 2025 16:45:58 +0100
From: Andrew Lunn <andrew@...n.ch>
To: Jacky Chou <jacky_chou@...eedtech.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Po-Yu Chuang <ratbert@...aday-tech.com>,
	Joel Stanley <joel@....id.au>,
	Andrew Jeffery <andrew@...econstruct.com.au>,
	netdev@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-aspeed@...ts.ozlabs.org, taoren@...a.com
Subject: Re: [PATCH net-next v4 4/4] net: ftgmac100: Add RGMII delay support
 for AST2600

On Mon, Nov 10, 2025 at 07:09:28PM +0800, Jacky Chou wrote:
> On the AST2600 platform, the RGMII delay is controlled via the
> SCU registers. The delay chain configuration differs between MAC0/1
> and MAC2/3, even though all four MACs use a 32-stage delay chain.
> +------+----------+-----------+-------------+-------------+
> |      |Delay Unit|Delay Stage|TX Edge Stage|RX Edge Stage|
> +------+----------+-----------+-------------+-------------+
> |MAC0/1|     45 ps|        32 |           0 |           0 |
> +------+----------+-----------+-------------+-------------+
> |MAC2/3|    250 ps|        32 |           0 |          26 |
> +------+----------+-----------+-------------+-------------+
> For MAC2/3, the "no delay" condition starts from stage 26.
> Setting the RX delay stage to 26 means that no additional RX
> delay is applied.
> Here lists the RX delay setting of MAC2/3 below.
> 26 -> 0   ns, 27 -> 0.25 ns, ... , 31 -> 1.25 ns,
> 0  -> 1.5 ns, 1  -> 1.75 ns, ... , 25 -> 7.75 ns
> 
> Therefore, we calculate the delay stage from the
> rx-internal-delay-ps of MAC2/3 to add 26. If the stage is equel
> to or bigger than 32, the delay stage will be mask 0x1f to get
> the correct setting.
> The delay chain is like a ring for configuration.
> Example for the rx-internal-delay-ps of MAC2/3 is 2000 ps,
> we will get the delay stage is 2.
> 
> Strating to this patch, driver will remind the legacy dts to update the
> "phy-mode" to "rgmii-id, and add the corresponding rgmii delay with
> "rx-internal-delay-id" and "tx-internal-delay-id".
> If lack these properties, driver will configure the default rgmii delay,
> that means driver will disable the TX and RX delay in MAC side.
> 
> Signed-off-by: Jacky Chou <jacky_chou@...eedtech.com>
> ---
>  drivers/net/ethernet/faraday/ftgmac100.c | 148 +++++++++++++++++++++++++++++++
>  drivers/net/ethernet/faraday/ftgmac100.h |  20 +++++
>  2 files changed, 168 insertions(+)
> 
> diff --git a/drivers/net/ethernet/faraday/ftgmac100.c b/drivers/net/ethernet/faraday/ftgmac100.c
> index a863f7841210..5cecdd4583f6 100644
> --- a/drivers/net/ethernet/faraday/ftgmac100.c
> +static int ftgmac100_set_ast2600_rgmii_delay(struct ftgmac100 *priv,
> +					     u32 rgmii_tx_delay,
> +					     u32 rgmii_rx_delay)
> +{
> +	struct device *dev = priv->dev;
> +	struct device_node *np;
> +	u32 rgmii_delay_unit;
> +	u32 rx_delay_index;
> +	u32 tx_delay_index;
> +	struct regmap *scu;
> +	int dly_mask;
> +	int dly_reg;
> +	int mac_id;
> +	int ret;
> +
> +	np = dev->of_node;
> +
> +	/* Add a warning to notify the existed dts based on AST2600. It is
> +	 * recommended to update the dts to add the rx/tx-internal-delay-ps to
> +	 * specify the RGMII delay and we recommend using the "rgmii-id" for
> +	 * phy-mode property to tell the PHY enables TX/RX internal delay and
> +	 * add the corresponding rx/tx-internal-delay-ps properties.
> +	 */
> +	if (priv->netdev->phydev->interface != PHY_INTERFACE_MODE_RGMII_ID)
> +		dev_warn(dev, "Update the phy-mode to 'rgmii-id'.\n");
> +
> +	scu = syscon_regmap_lookup_by_phandle(np, "aspeed,scu");
> +	if (IS_ERR(scu)) {
> +		dev_err(dev, "failed to get aspeed,scu");
> +		return PTR_ERR(scu);
> +	}

You are adding the scu to the dtsi.

> +
> +	ret = of_property_read_u32(np, "aspeed,rgmii-delay-ps",
> +				   &rgmii_delay_unit);
> +	if (ret) {
> +		dev_err(dev, "failed to get aspeed,rgmii-delay-ps value\n");
> +		return -EINVAL;
> +	}

This is probably going away, replaced by hard coded values.

> +	tx_delay_index = DIV_ROUND_CLOSEST(rgmii_tx_delay, rgmii_delay_unit);
> +	if (tx_delay_index >= 32) {
> +		dev_err(dev, "The %u ps of TX delay is out of range\n",
> +			rgmii_tx_delay);
> +		return -EINVAL;
> +	}
> +
> +	rx_delay_index = DIV_ROUND_CLOSEST(rgmii_rx_delay, rgmii_delay_unit);
> +	if (rx_delay_index >= 32) {
> +		dev_err(dev, "The %u ps of RX delay is out of range\n",
> +			rgmii_rx_delay);
> +		return -EINVAL;
> +	}

> +	regmap_update_bits(scu, dly_reg, dly_mask, tx_delay_index | rx_delay_index);

How does backwards compatibility work? rgmii_rx_delay and
rgmii_tx_delay default to zero? So by default, todays working DT blobs
will have 'rgmii', so end up turning off delays here. And then they
pass _RGMII to the PHY, and have no delays. And networking is broken.

I think you actually need to be reading the current SCU settings and
then deciding if you want to override it or not.

I suggest you change the order of the patches in this series. Move
this patch before you update your RDK .dts file. That way, you get to
test both an old and new blob.

	Andrew