回覆: [net-next 4/4] net: ftgmac100: add RGMII delay for AST2600

[Jacky Chou]

> > +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