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