[<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
Powered by blists - more mailing lists