linux-kernel - 回覆: [net-next 4/4] net: ftgmac100: add RGMII delay 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] [thread-next>] [day] [month] [year] [list]

Message-ID:
 <SEYPR06MB51346129A2D6A5BD1753C4D39DDF2@SEYPR06MB5134.apcprd06.prod.outlook.com>
Date: Mon, 17 Mar 2025 09:28:12 +0000
From: Jacky Chou <jacky_chou@...eedtech.com>
To: Maxime Chevallier <maxime.chevallier@...tlin.com>
CC: "andrew+netdev@...n.ch" <andrew+netdev@...n.ch>, "davem@...emloft.net"
	<davem@...emloft.net>, "edumazet@...gle.com" <edumazet@...gle.com>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"robh@...nel.org" <robh@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "conor+dt@...nel.org" <conor+dt@...nel.org>,
	"joel@....id.au" <joel@....id.au>, "andrew@...econstruct.com.au"
	<andrew@...econstruct.com.au>, "ratbert@...aday-tech.com"
	<ratbert@...aday-tech.com>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-aspeed@...ts.ozlabs.org"
	<linux-aspeed@...ts.ozlabs.org>, BMC-SW <BMC-SW@...eedtech.com>
Subject:
 回覆: [net-next 4/4] net: ftgmac100: add RGMII delay for AST2600

> > +static void ftgmac100_set_internal_delay(struct platform_device
> > +*pdev) {
> > +	struct device_node *np = pdev->dev.of_node;
> > +	struct net_device *netdev;
> > +	struct ftgmac100 *priv;
> > +	struct regmap *scu;
> > +	u32 rgmii_tx_delay, rgmii_rx_delay;
> > +	u32 dly_reg, tx_dly_mask, rx_dly_mask;
> > +	int tx, rx;
> 
> Please use the reverse christmas tree notation, sorting declarations by
> descending line length

Got it. I will modify these in next version.

> > +	netdev = platform_get_drvdata(pdev);
> > +	priv = netdev_priv(netdev);
> > +
> > +	tx = of_property_read_u32(np, "tx-internal-delay-ps", &rgmii_tx_delay);
> > +	rx = of_property_read_u32(np, "rx-internal-delay-ps",
> > +&rgmii_rx_delay);
> > +
> > +	if (of_device_is_compatible(np, "aspeed,ast2600-mac")) {
> > +		/* According to mac base address to get mac index */
> > +		switch (priv->res->start) {
> > +		case 0x1e660000:
> > +			dly_reg = AST2600_MAC12_CLK_DLY;
> > +			tx_dly_mask = AST2600_MAC1_TX_DLY;
> > +			rx_dly_mask = AST2600_MAC1_RX_DLY;
> > +			rgmii_tx_delay = FIELD_PREP(AST2600_MAC1_TX_DLY,
> rgmii_tx_delay);
> > +			rgmii_rx_delay = FIELD_PREP(AST2600_MAC1_RX_DLY,
> rgmii_rx_delay);
> > +			break;
> > +		case 0x1e680000:
> > +			dly_reg = AST2600_MAC12_CLK_DLY;
> > +			tx_dly_mask = AST2600_MAC2_TX_DLY;
> > +			rx_dly_mask = AST2600_MAC2_RX_DLY;
> > +			rgmii_tx_delay = FIELD_PREP(AST2600_MAC2_TX_DLY,
> rgmii_tx_delay);
> > +			rgmii_rx_delay = FIELD_PREP(AST2600_MAC2_RX_DLY,
> rgmii_rx_delay);
> > +			break;
> > +		case 0x1e670000:
> > +			dly_reg = AST2600_MAC34_CLK_DLY;
> > +			tx_dly_mask = AST2600_MAC3_TX_DLY;
> > +			rx_dly_mask = AST2600_MAC3_RX_DLY;
> > +			rgmii_tx_delay = FIELD_PREP(AST2600_MAC3_TX_DLY,
> rgmii_tx_delay);
> > +			rgmii_rx_delay = FIELD_PREP(AST2600_MAC3_RX_DLY,
> rgmii_rx_delay);
> > +			break;
> > +		case 0x1e690000:
> > +			dly_reg = AST2600_MAC34_CLK_DLY;
> > +			tx_dly_mask = AST2600_MAC4_TX_DLY;
> > +			rx_dly_mask = AST2600_MAC4_RX_DLY;
> > +			rgmii_tx_delay = FIELD_PREP(AST2600_MAC4_TX_DLY,
> rgmii_tx_delay);
> > +			rgmii_rx_delay = FIELD_PREP(AST2600_MAC4_RX_DLY,
> rgmii_rx_delay);
> > +			break;
> > +		default:
> > +			dev_warn(&pdev->dev, "Invalid mac base address");
> > +			return;
> 
> There has to be a better way that directly looking up the base address.
> Maybe you need an extra DT property ?

I use the base address to identify the MAC index because it is the only fixed value 
that is paired with the MAC.
If I create a property to identify the MAC index and someone use the wrong value, 
the driver will configure the wrong register field.
Therefore, I use the base address as MAC index, and it is very clear which MAC is 
configured.

> 
> > +		}
> > +	} else {
> > +		return;
> > +	}
> > +
> > +	scu = syscon_regmap_lookup_by_phandle(np, "scu");
> > +	if (IS_ERR(scu)) {
> > +		dev_warn(&pdev->dev, "failed to map scu base");
> > +		return;
> > +	}
> > +
> > +	if (!tx) {
> > +		/* Use tx-internal-delay-ps as index to configure tx delay
> > +		 * into scu register.
> > +		 */
> 
> So this goes completely against the naming of the property. It has the -ps suffix,
> so you would expect to have picoseconds values passed, and not an arbiraty
> index.
> 
> Take a look at other drivers, you should accept picseconds values from these
> properties, then compute the relevant index in the driver. That index should be
> something internal to your driver.
> 
> An example here :
> 
> https://elixir.bootlin.com/linux/v6.14-rc6/source/drivers/net/ethernet/microchi
> p/sparx5/lan969x/lan969x_rgmii.c#L51
> 

Agreed. Thank you for your information.
I will adjust this part to use ps unit to translate to the relevant index in next version.

Jacky