Re: [RFC 13/13] net: ti: icssg-prueth: Add ICSSG ethernet driver

From: Andrew Lunn <andrew@lunn.ch>
To: Grygorii Strashko <grygorii.strashko@ti.com>
Cc: Puranjay Mohan <p-mohan@ti.com>,
	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>
Subject: Re: [RFC 13/13] net: ti: icssg-prueth: Add ICSSG ethernet driver
Date: Tue, 12 Apr 2022 15:10:36 +0200	[thread overview]
Message-ID: <YlV6TKTl5uQGZrQX@lunn.ch> (raw)
In-Reply-To: <f3ae48a8-7177-3a92-bdfd-3b243a5527c1@ti.com>

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

And enabled for the others?

Why don't you always disable the delays and let the PHY do it? That is
what pretty much every other MAC/PHY combination does.

    Andrew