Messages in this thread | | | Date | Tue, 1 Aug 2023 12:23:56 -0500 | From | Andrew Halaney <> | Subject | Re: [EXT] Re: [PATCH v3 net 2/2] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link |
| |
On Tue, Aug 01, 2023 at 05:06:46PM +0000, Shenwei Wang wrote: > > > > -----Original Message----- > > From: Russell King <linux@armlinux.org.uk> > > Sent: Tuesday, August 1, 2023 7:57 AM > > To: Johannes Zink <j.zink@pengutronix.de> > > Cc: Shenwei Wang <shenwei.wang@nxp.com>; David S. Miller > > <davem@davemloft.net>; Eric Dumazet <edumazet@google.com>; Jakub > > Kicinski <kuba@kernel.org>; Paolo Abeni <pabeni@redhat.com>; Maxime > > Coquelin <mcoquelin.stm32@gmail.com>; Shawn Guo <shawnguo@kernel.org>; > > Sascha Hauer <s.hauer@pengutronix.de>; Neil Armstrong > > <neil.armstrong@linaro.org>; Kevin Hilman <khilman@baylibre.com>; Vinod > > Koul <vkoul@kernel.org>; Chen-Yu Tsai <wens@csie.org>; Jernej Skrabec > > <jernej.skrabec@gmail.com>; Samuel Holland <samuel@sholland.org>; > > Giuseppe Cavallaro <peppe.cavallaro@st.com>; Alexandre Torgue > > <alexandre.torgue@foss.st.com>; Jose Abreu <joabreu@synopsys.com>; > > Pengutronix Kernel Team <kernel@pengutronix.de>; Fabio Estevam > > <festevam@gmail.com>; dl-linux-imx <linux-imx@nxp.com>; Jerome Brunet > > <jbrunet@baylibre.com>; Martin Blumenstingl > > <martin.blumenstingl@googlemail.com>; Bhupesh Sharma > > <bhupesh.sharma@linaro.org>; Nobuhiro Iwamatsu > > <nobuhiro1.iwamatsu@toshiba.co.jp>; Simon Horman > > <simon.horman@corigine.com>; Andrew Halaney <ahalaney@redhat.com>; > > Bartosz Golaszewski <bartosz.golaszewski@linaro.org>; Wong Vee Khee > > <veekhee@apple.com>; Revanth Kumar Uppala <ruppala@nvidia.com>; Jochen > > Henneberg <jh@henneberg-systemdesign.com>; 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 <frank.li@nxp.com> > > 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 > > > > <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 > " 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
| |