On Tue, Aug 01, 2023 at 05:06:46PM +0000, Shenwei Wang wrote:
>
>
> > -----Original Message-----
> > From: Russell King <[email protected]>
> > Sent: Tuesday, August 1, 2023 7:57 AM
> > To: Johannes Zink <[email protected]>
> > Cc: Shenwei Wang <[email protected]>; David S. Miller
> > <[email protected]>; Eric Dumazet <[email protected]>; Jakub
> > Kicinski <[email protected]>; Paolo Abeni <[email protected]>; Maxime
> > Coquelin <[email protected]>; Shawn Guo <[email protected]>;
> > Sascha Hauer <[email protected]>; Neil Armstrong
> > <[email protected]>; Kevin Hilman <[email protected]>; Vinod
> > Koul <[email protected]>; Chen-Yu Tsai <[email protected]>; Jernej Skrabec
> > <[email protected]>; Samuel Holland <[email protected]>;
> > Giuseppe Cavallaro <[email protected]>; Alexandre Torgue
> > <[email protected]>; Jose Abreu <[email protected]>;
> > Pengutronix Kernel Team <[email protected]>; Fabio Estevam
> > <[email protected]>; dl-linux-imx <[email protected]>; Jerome Brunet
> > <[email protected]>; Martin Blumenstingl
> > <[email protected]>; Bhupesh Sharma
> > <[email protected]>; Nobuhiro Iwamatsu
> > <[email protected]>; Simon Horman
> > <[email protected]>; Andrew Halaney <[email protected]>;
> > Bartosz Golaszewski <[email protected]>; Wong Vee Khee
> > <[email protected]>; Revanth Kumar Uppala <[email protected]>; Jochen
> > Henneberg <[email protected]>; [email protected]; linux-
> > [email protected]; [email protected];
> > [email protected]; [email protected];
> > [email protected]; Frank Li <[email protected]>
> > 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:
> > > Hi Shenwei,
> > >
> > > thanks for your patch.
> > >
> > > On 7/31/23 18:19, Shenwei Wang wrote:
> > > > 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
> >
> > <[email protected].
> > 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/[email protected]/T/#m08da3797a056d4d8ea4c1d8956b445ae967e7cfa
> " Yes, that's the purpose because it won't hurt even the other side is not SJA1105."
>
> > 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/
I hope that helps,
Andrew