From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from vps0.lunn.ch ([185.16.172.187]:43254 "EHLO vps0.lunn.ch" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1424222AbeCBNks (ORCPT ); Fri, 2 Mar 2018 08:40:48 -0500 Date: Fri, 2 Mar 2018 14:40:42 +0100 From: Andrew Lunn To: Bryan.Whitehead@microchip.com Cc: davem@davemloft.net, UNGLinuxDriver@microchip.com, netdev@vger.kernel.org Subject: Re: [PATCH v3 net-next 1/2] lan743x: Add main source files for new lan743x driver Message-ID: <20180302134042.GA19566@lunn.ch> References: <1519938009-10322-1-git-send-email-Bryan.Whitehead@microchip.com> <1519938009-10322-2-git-send-email-Bryan.Whitehead@microchip.com> <20180301221228.GD343@lunn.ch> <90A7E81AE28BAE4CBDDB3B35F187D2644071EFDF@CHN-SV-EXMX02.mchp-main.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <90A7E81AE28BAE4CBDDB3B35F187D2644071EFDF@CHN-SV-EXMX02.mchp-main.com> Sender: netdev-owner@vger.kernel.org List-ID: On Fri, Mar 02, 2018 at 05:57:54AM +0000, Bryan.Whitehead@microchip.com wrote: > > > +static int lan743x_phy_reset(struct lan743x_adapter *adapter) { > > > + u32 data; > > > + > > > + data = lan743x_csr_read(adapter, PMT_CTL); > > > + data |= PMT_CTL_ETH_PHY_RST_; > > > + lan743x_csr_write(adapter, PMT_CTL, data); > > > + > > > + return readx_poll_timeout(LAN743X_CSR_READ_OP, PMT_CTL, > > data, > > > + (!(data & PMT_CTL_ETH_PHY_RST_) && > > > + (data & PMT_CTL_READY_)), > > > + 50000, 1000000); > > > +} > > > > Hi Bryan > > > > Could you explain this a bit more. What exactly is it resetting? Do we need to > > tell the phylib that the PHY has been reset and that it needs to re-program it? > > Or by phy do you mean a SERDES interface? > > Hi Andrew, > > This function resets the Ethernet phy. But it is called only in > probe and before mdiobus_register. So I don't believe it is > necessary to tell phylib. Hi Bryan So the PHY is built in? It should be safe. Normally we have the PHY driver, or the generic layers handle any such reset via a GPIO. But built in is different. > [snip] > > > + > > > + /* PHY interrupt enabled here */ > > > + phy_start(phydev); > > > + phy_start_aneg(phydev); > > > + return 0; > > > > Are phy interrupts really enabled here? I could of missed it, but i don't see > > anywhere PHY interrupts are configured. This is either done via device tree, > > you set phydev->irq, or mdiobus->irq[X]. > > Sorry that is an obsolete comment, I will remove it. It is not using > phy interrupts. It's using polling. Assuming it is built in, does the MAC get the interrupt? phy_mac_interrupt() can be called if so. Andrew