On 12/04/2022 12:45, Puranjay Mohan wrote:
+ Roger, Grygorii On 07/04/22 00:07, Andrew Lunn wrote:+static int emac_phy_connect(struct prueth_emac *emac) +{ + struct prueth *prueth = emac->prueth; + + /* connect PHY */ + emac->phydev = of_phy_connect(emac->ndev, emac->phy_node, + &emac_adjust_link, 0, emac->phy_if);+static int prueth_config_rgmiidelay(struct prueth *prueth, + struct device_node *eth_np, + phy_interface_t phy_if) +{ + struct device *dev = prueth->dev; + struct regmap *ctrl_mmr; + u32 rgmii_tx_id = 0; + u32 icssgctrl_reg; + + if (!phy_interface_mode_is_rgmii(phy_if)) + return 0; + + ctrl_mmr = syscon_regmap_lookup_by_phandle(eth_np, "ti,syscon-rgmii-delay"); + if (IS_ERR(ctrl_mmr)) { + dev_err(dev, "couldn't get ti,syscon-rgmii-delay\n"); + return -ENODEV; + } + + if (of_property_read_u32_index(eth_np, "ti,syscon-rgmii-delay", 1, + &icssgctrl_reg)) { + dev_err(dev, "couldn't get ti,rgmii-delay reg. offset\n"); + return -ENODEV; + } + + if (phy_if == PHY_INTERFACE_MODE_RGMII_ID || + phy_if == PHY_INTERFACE_MODE_RGMII_TXID) + rgmii_tx_id |= ICSSG_CTRL_RGMII_ID_MODE; + + regmap_update_bits(ctrl_mmr, icssgctrl_reg, ICSSG_CTRL_RGMII_ID_MODE, rgmii_tx_id); + + return 0; +}O.K, so this does not do what i initially thought it was doing. I was thinking it was to fine tune the delay, ti,syscon-rgmii-delay would be a small pico second value to allow the 2ns delay to be tuned to the board. But now i think this is actually inserting the full 2ns delay? The problem is, you also pass phy_if to of_phy_connect() so the PHY will also insert the delay if requested. So you end up with double delays for rgmii_id and rgmii_txid.
It's misunderstanding here. The bit field name in TRM is RGMII0_ID_MODE and meaning: 0h - Internal transmit delay is enabled 1h - Internal transmit delay is not enabled. So here internal delay will be disabled for RGMII_ID/RGMII_TXID.
The general recommendation is that the PHY inserts the delay, based on phy-mode. The MAC does not add a delay, so i suggest you always write 0 here, just to ensure the system is in a deterministic state, and the bootloader and not being messing around with things. Andrew
-- Best regards, Grygorii, Ukraine
