From mboxrd@z Thu Jan 1 00:00:00 1970 From: Claudiu Beznea Subject: Re: [PATCH] net: macb: do not disable MDIO bus when closing interface Date: Mon, 13 Aug 2018 12:08:10 +0300 Message-ID: <3e55fc51-503c-d174-841c-a1a5a0f5ae69@microchip.com> References: <20180808121901.10264-1-anssi.hannula@bitwise.fi> <425ed5af-eac2-a58b-b6ca-f022a80367e4@microchip.com> <3c3a606e-b3e1-41fc-fcd8-7b98968fd06b@bitwise.fi> <20180809151433.GB20006@lunn.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Cc: Nicolas Ferre , "David S. Miller" , To: Anssi Hannula , Andrew Lunn Return-path: Received: from esa1.microchip.iphmx.com ([68.232.147.91]:47366 "EHLO esa1.microchip.iphmx.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727799AbeHMLte (ORCPT ); Mon, 13 Aug 2018 07:49:34 -0400 In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 10.08.2018 09:22, Anssi Hannula wrote: > On 9.8.2018 18:14, Andrew Lunn wrote: >> Hi Anssi > > Hi! > >>> macb_reset_hw() is called in init path too, I only see it in macb_close() and macb_open() called from macb_init_hw(). though, so maybe clearing >>> all bits is intentional / wanted to get the controller to a known state, As far as I know the NCR is used in this driver only to control transmit enable, receive enable, MPE and tx start. Not clearing NCR in the init phase should not have any impact on IP. Same for not clearing Tx start on "if down" path (if tx is in progress). >>> even though the comment only mentions TX/RX? >> You need to be careful here. Once of_mdiobus_register() is called, the >> MDIO should be usable. If you happen to have an Ethernet switch on the >> bus, it could be probed then. The DSA driver will start using the bus. >> Or if you have a second PHY, connected to some other MAC, it could be >> used by the other MAC. This all happens in the macb_probe function. >> >> Sometime later, the interface will be up'ed. At this point macb_open() >> is called, which calls macb_init_hw(), which calls >> macb_reset_hw(). What you don't want happening is changes to the NCR >> at this point breaking an MDIO transaction which might be going on. >> >> Ideally, the MPE should be enabled before of_mdiobus_register(), and >> left alone until mdiobus_unregister() is called in macb_remove(). > > Yep, fixing the use case of having PHYs of other MACs is why I wrote the > patch :) > > Currently the reset code disables MPE while other MACs are using PHYs on > the bus. MPE is set in the early phase of initialization, in macb_mii_init(), called from probe() function. Due to the fact that NCR is cleared in macb_reset_hw() it is set again at the end of macb_init_hw(). So, as per Andrew comments in this thread, setting it in macb_mii_init() and letting it unchanged until remove would be the best. So, if you clear only TE and RE bits in macb_reset_hw() please also remove MACB_BIT(MPE) from below line at the end of macb_init_hw(): macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE)); Thank you, Claudiu Beznea >