Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:18 as permitted sender) client-ip=2620:137:e000::1:18;
Message-ID: <f3ae48a8-7177-3a92-bdfd-3b243a5527c1@ti.com>
Date:   Tue, 12 Apr 2022 12:56:18 +0300
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [RFC 13/13] net: ti: icssg-prueth: Add ICSSG ethernet driver
Content-Language: en-US
To:     Puranjay Mohan <p-mohan@ti.com>, Andrew Lunn <andrew@lunn.ch>
CC:     <linux-kernel@vger.kernel.org>, <bjorn.andersson@linaro.org>,
        <mathieu.poirier@linaro.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <linux-remoteproc@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <nm@ti.com>, <ssantosh@kernel.org>, <s-anna@ti.com>,
        <linux-arm-kernel@lists.infradead.org>, <davem@davemloft.net>,
        <kuba@kernel.org>, <netdev@vger.kernel.org>, <vigneshr@ti.com>,
        <kishon@ti.com>, Roger Quadros <rogerq@kernel.org>
References: <20220406094358.7895-1-p-mohan@ti.com>
 <20220406094358.7895-14-p-mohan@ti.com> <Yk3d/cC36fhNmfY2@lunn.ch>
 <468d4d9b-44b4-2894-2a75-4caab1e72147@ti.com>
From:   Grygorii Strashko <grygorii.strashko@ti.com>
In-Reply-To: <468d4d9b-44b4-2894-2a75-4caab1e72147@ti.com>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


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