From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Lunn Subject: Re: [PATCH v3 2/4] net: nb8800: Simplify nb8800_pause_config() Date: Wed, 15 Nov 2017 15:17:07 +0100 Message-ID: <20171115141707.GB2130@lunn.ch> References: <230165aa-eaf1-6e2b-7ff3-45b3ee4ffc62@sigmadesigns.com> <569542b1-8e56-d7da-f7a0-affd89bfed62@sigmadesigns.com> <6f534b2b-f69a-bda5-dc5b-0281bf0df129@sigmadesigns.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: Mans Rullgard , Florian Fainelli , Mason , netdev , Thibaud Cornic , David Miller , Linux ARM To: Marc Gonzalez Return-path: Received: from vps0.lunn.ch ([185.16.172.187]:55624 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932528AbdKOORP (ORCPT ); Wed, 15 Nov 2017 09:17:15 -0500 Content-Disposition: inline In-Reply-To: <6f534b2b-f69a-bda5-dc5b-0281bf0df129@sigmadesigns.com> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, Nov 15, 2017 at 11:53:23AM +0100, Marc Gonzalez wrote: > On 14/11/2017 14:22, Måns Rullgård wrote: > > > Marc Gonzalez wrote: > > > >> On 14/11/2017 13:38, Måns Rullgård wrote: > >> > >>> Marc Gonzalez writes: > >>> > >>>> The "flow control enable" bit can be tweaked, even if DMA is enabled. > >>> > >>> No, it can't. Maybe on some of your newer chips it can, but not on the > >>> older ones. > >> > >> Again, I suppose you are referring to your SMP8642-based board. > >> > >> Are you saying you tested this patch, and it doesn't work? > >> What are the symptoms? > > > > I didn't test that patch per se. I did initially try simply changing > > that bit, and this didn't work. Either it had no effect, or it locked > > up the controller, I forget which. The manual explicitly states that > > writes are forbidden while the DMA enabled bit is set. > > > > If shutting down the DMA really isn't possible, I would rather just > > allow changing the flow control setting only when the interface is > > stopped. The normal case of getting the initial setting from the > > auto-negotiation will still work properly. It just won't be possible to > > change it while the link is active. > > Hello Mans, > > With the default setup, > > priv->pause_aneg = true; > priv->pause_rx = true; > priv->pause_tx = true; Hi Marc So you are assuming the peer supports pause? Is that a safe assumption to make? Should you not have it disabled until auto-neg has completed and you then know what the peer can do? > > even the very first call to nb8800_pause_config() occurs with netif_running=1 > as well as the RX DMA bit enabled. > > buildroot login: root > # ip addr add 172.27.64.23/18 brd 172.27.127.255 dev eth0 > # ip link set eth0 up > [ 25.771000] ENTER nb8800_pause_config: netif_running=1 > [ 25.776195] CPU: 0 PID: 193 Comm: kworker/0:1 Not tainted 4.9.54-1-rc6 #18 > [ 25.783102] Hardware name: Sigma Tango DT > [ 25.787138] Workqueue: events_power_efficient phy_state_machine > [ 25.793107] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) > [ 25.800896] [] (show_stack) from [] (dump_stack+0x88/0x9c) > [ 25.808160] [] (dump_stack) from [] (nb8800_pause_config+0x28/0xf0) > [ 25.816208] [] (nb8800_pause_config) from [] (nb8800_link_reconfigure+0x134/0x148) > [ 25.825565] [] (nb8800_link_reconfigure) from [] (phy_state_machine+0x348/0x468) > [ 25.834750] [] (phy_state_machine) from [] (process_one_work+0x1d8/0x3f0) > [ 25.843323] [] (process_one_work) from [] (worker_thread+0x4c/0x560) > [ 25.851459] [] (worker_thread) from [] (kthread+0xec/0xf4) > [ 25.858721] [] (kthread) from [] (ret_from_fork+0x14/0x3c) > [ 25.874597] nb8800 26000.ethernet eth0: Link is Up - 100Mbps/Full - flow control rx/tx > > > This makes sense, since nb8800_open() calls nb8800_start_rx() > before phy_start(). > > Moving nb8800_start_rx() to after nb8800_pause_config() does > appear to work, but I'm not sure it is the correct action? nb8800_link_reconfigure() can be called whenever the link to the peer changes. auto-neg may happen later because the cable was not plugged in until later, etc. Andrew