Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751488AbdILJYF (ORCPT ); Tue, 12 Sep 2017 05:24:05 -0400 Received: from mx.socionext.com ([202.248.49.38]:35374 "EHLO mx.socionext.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751214AbdILJYD (ORCPT ); Tue, 12 Sep 2017 05:24:03 -0400 Date: Tue, 12 Sep 2017 18:24:01 +0900 From: Kunihiko Hayashi To: Andrew Lunn Subject: Re: [PATCH net-next 2/3] net: ethernet: socionext: add AVE ethernet driver Cc: , "David S. Miller" , Florian Fainelli , Rob Herring , Mark Rutland , , , , Masahiro Yamada , Masami Hiramatsu , Jassi Brar In-Reply-To: <20170911120009.GA24174@lunn.ch> References: <20170911155047.6717.4A936039@socionext.com> <20170911120009.GA24174@lunn.ch> Message-Id: <20170912182400.180E.4A936039@socionext.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Mailer: Becky! ver. 2.70 [ja] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2250 Lines: 64 Hi Andrew, On Mon, 11 Sep 2017 14:00:09 +0200 Andrew Lunn wrote: > > > > +static irqreturn_t ave_interrupt(int irq, void *netdev) > > > > +{ > > > > + struct net_device *ndev = (struct net_device *)netdev; > > > > + struct ave_private *priv = netdev_priv(ndev); > > > > + u32 gimr_val, gisr_val; > > > > + > > > > + gimr_val = ave_irq_disable_all(ndev); > > > > + > > > > + /* get interrupt status */ > > > > + gisr_val = ave_r32(ndev, AVE_GISR); > > > > + > > > > + /* PHY */ > > > > + if (gisr_val & AVE_GI_PHY) { > > > > + ave_w32(ndev, AVE_GISR, AVE_GI_PHY); > > > > + if (priv->internal_phy_interrupt) > > > > + phy_mac_interrupt(ndev->phydev, ndev->phydev->link); > > > > > > Humm. I don't think this is correct. You are supposed to give it the > > > new link state, not the old. > > > > > > What does a PHY interrupt mean here? > > > > In the general case, I think PHY events like changing link state are transmitted > > to CPU as interrupt via interrupt controller, then PHY driver itself can handle > > the interrupt. > > > > And in this case, PHY events are transmitted to MAC as one of its interrupt factor, > > then I thought that MAC driver had to tell the events to PHY. > > Could this be in-band SGMI signalling from the PHY to the MAC? Does > the documentation give examples of when this interrupt will happen? > > Andrew Unfortunately this MAC doesn't support SGMII. And there aren't any examples of when this interrupt will happen. This interrupt happens after ave_open() is called and link is established. However, I found that auto negotiation didn't start when this interrupt wasn't handled. Although ave_init() calls phy_start_aneg(), it doesn't make sense because phy driver doesn't start yet. And according to Florian's comment in ave_init(), > + phy_start_interrupts(phydev); > + > + phy_start_aneg(phydev); > > No, no, phy_start() would take care of all of that correctly for you, > you don't have have to do it just there because ave_open() eventually > calls phy_start() for you, so just drop these two calls. When moving phy_start_aneg() to ave_open(), the handler doesn't need to call phy_mac_interrupt() and I can remove it from the handler. --- Best Regards, Kunihiko Hayashi