On Tue, Aug 01, 2023 at 05:06:46PM +0000, Shenwei Wang wrote:

quoted

-----Original Message-----
From: Russell King <linux@armlinux.org.uk>
Sent: Tuesday, August 1, 2023 7:57 AM
To: Johannes Zink <redacted>
Cc: Shenwei Wang <shenwei.wang@nxp.com>; David S. Miller
[off-list ref]; Eric Dumazet [off-list ref]; Jakub
Kicinski [off-list ref]; Paolo Abeni [off-list ref]; Maxime
Coquelin [off-list ref]; Shawn Guo [off-list ref];
Sascha Hauer [off-list ref]; Neil Armstrong
[off-list ref]; Kevin Hilman [off-list ref]; Vinod
Koul [off-list ref]; Chen-Yu Tsai [off-list ref]; Jernej Skrabec
[off-list ref]; Samuel Holland [off-list ref];
Giuseppe Cavallaro [off-list ref]; Alexandre Torgue
[off-list ref]; Jose Abreu [off-list ref];
Pengutronix Kernel Team [off-list ref]; Fabio Estevam
[off-list ref]; dl-linux-imx [off-list ref]; Jerome Brunet
[off-list ref]; Martin Blumenstingl
[off-list ref]; Bhupesh Sharma
[off-list ref]; Nobuhiro Iwamatsu
[off-list ref]; Simon Horman
[off-list ref]; Andrew Halaney [off-list ref];
Bartosz Golaszewski [off-list ref]; Wong Vee Khee
[off-list ref]; Revanth Kumar Uppala [off-list ref]; Jochen
Henneberg [off-list ref]; netdev@vger.kernel.org; linux-
stm32@st-md-mailman.stormreply.com; linux-arm-kernel@lists.infradead.org;
linux-kernel@vger.kernel.org; linux-amlogic@lists.infradead.org;
imx@lists.linux.dev; Frank Li [off-list ref]
Subject: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC
clock in fixed-link

Caution: This is an external email. Please take care when clicking links or
opening attachments. When in doubt, report the message using the 'Report this
email' button


On Tue, Aug 01, 2023 at 02:47:46PM +0200, Johannes Zink wrote:

quoted

Hi Shenwei,

thanks for your patch.

On 7/31/23 18:19, Shenwei Wang wrote:

quoted

When using a fixed-link setup, certain devices like the SJA1105
require a small pause in the TXC clock line to enable their internal
tunable delay line (TDL).

If this is only required for some devices, is it safe to enforce this
behaviour unconditionally for any kind of fixed link devices connected
to the MX93 EQOS or could this possibly break for other devices?

This same point has been raised by Andrew Halaney in message-id
<4govb566nypifbtqp5lcbsjhvoyble5luww3onaa2liinboguf@4kgihys6vhrg>
and Fabio Estevam in message-id

<CAOMZO5ANQmVbk_jy7qdVtzs3716FisT2c72W+3WZyu7FoAochw@mail.gmail.
com>
but we don't seem to have any answer for it.

Hi Russell,

I hope you have thoroughly read all of my earlier responses, as I believe I already addressed this question.
I'm happy to clarify further, but kindly avoid unsubstantiated comments.

https://lore.kernel.org/imx/20230727152503.2199550-1-shenwei.wang@nxp.com/T/#m08da3797a056d4d8ea4c1d8956b445ae967e7cfa (local)
" Yes, that's the purpose because it won't hurt even the other side is not SJA1105."

quoted

Also, the patch still uses wmb() between the write and the delay, and as Will
Deacon pointed out in his message, message-id
<20230728153611.GH21718@willie-the-truck>
this is not safe, yet still a new version was sent.

Can we conclude that even without the wmb() here, the desired delay time between
operations can still be ensured?

Will's talk[0] he linked has the sequence you've done here (writel's
followed by wmb() followed by a udelay), and he states it is wrong if
the goal is for the device to see the writes prior to the udelay. That's
discussed at around 28:00 and followed up by (thankfully, cuz I too
didn't understand it) a question at 34:10 to discuss why mb() isn't
sufficient (it completes the write, but the device *may not* see it
yet, the read forces that).

He mentioned that over at [1] in the review here, and suggested reading
from the device again prior to the udelay() instead to force the writes
to take affect on the device prior to the udelay.

I found a quick example in the ufs-qcom.c driver that I'll copy paste
here too from upstream that follows this advice:

		writel_relaxed(temp, host->dev_ref_clk_ctrl_mmio);

		/*
		 * Make sure the write to ref_clk reaches the destination and
		 * not stored in a Write Buffer (WB).
		 */
		readl(host->dev_ref_clk_ctrl_mmio);

		/*
		 * If we call hibern8 exit after this, we need to make sure that
		 * device ref_clk is stable for at least 1us before the hibern8
		 * exit command.
		 */
		if (enable)
			udelay(1);


[0] https://www.youtube.com/watch?v=i6DayghhA8Q
[1] https://lore.kernel.org/netdev/20230728153611.GH21718@willie-the-truck/ (local)

I hope that helps,
Andrew