From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751463AbcG3HeR (ORCPT ); Sat, 30 Jul 2016 03:34:17 -0400 Received: from down.free-electrons.com ([37.187.137.238]:57994 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750809AbcG3HeQ (ORCPT ); Sat, 30 Jul 2016 03:34:16 -0400 Date: Sat, 30 Jul 2016 09:34:12 +0200 From: Maxime Ripard To: Chen-Yu Tsai Cc: LABBE Corentin , Rob Herring , Mark Rutland , Russell King , David Miller , netdev , devicetree , linux-arm-kernel , linux-kernel , linux-sunxi Subject: Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver Message-ID: <20160730073412.GL6215@lukather> References: <1469001800-11615-1-git-send-email-clabbe.montjoie@gmail.com> <1469001800-11615-2-git-send-email-clabbe.montjoie@gmail.com> <20160725195455.GQ7419@lukather> <20160728145734.GD7582@Red> <20160729172526.GE6215@lukather> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="kaF1vgn83Aa7CiXN" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --kaF1vgn83Aa7CiXN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Jul 30, 2016 at 09:30:01AM +0800, Chen-Yu Tsai wrote: > >> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev) > >> > > +{ > >> > > + struct sun8i_emac_priv *priv =3D netdev_priv(ndev); > >> > > + u32 reg =3D 0; > >> > > + > >> > > + if (priv->variant =3D=3D H3_EMAC) > >> > > + reg =3D H3_EPHY_DEFAULT_VALUE; > >> > > >> > Why do you need that? > >> > > >> For resetting the syscon to the factory default. > > > > Yes, but does it matter? Does it have any side effect? Is that > > register shared with another device? > > > > Otherwise, either it won't be used anymore, and you don't care, or you > > will reload the driver later, and the driver should work whatever > > state is programmed in there. In both cases, you don't need to reset > > that value. >=20 > The "default" setting also disables and powers down the internal PHY. > I think that's a good thing? The naming could be better. Ah, it might, and that would obviously be the right thing to do. Using a define for those enable and power down bits would be better though. > >> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id) > >> > > +{ > >> > > + struct net_device *ndev =3D dev_id; > >> > > + struct sun8i_emac_priv *priv =3D netdev_priv(ndev); > >> > > + u32 v, u; > >> > > + > >> > > + v =3D readl(priv->base + SUN8I_EMAC_INT_STA); > >> > > + > >> > > + /* When this bit is asserted, a frame transmission is completed.= */ > >> > > + if (v & BIT(0)) { > >> > > + priv->estats.tx_int++; > >> > > + writel(0, priv->base + SUN8I_EMAC_INT_EN); > >> > > + napi_schedule(&priv->napi); > >> > > + } > >> > > + > >> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */ > >> > > + if (v & BIT(1)) > >> > > + priv->estats.tx_dma_stop++; > >> > > + > >> > > + /* When this asserted, the TX DMA can not acquire next TX descri= ptor > >> > > + * and TX DMA FSM is suspended. > >> > > + */ > >> > > + if (v & BIT(2)) > >> > > + priv->estats.tx_dma_ua++; > >> > > + > >> > > + if (v & BIT(3)) > >> > > + netif_dbg(priv, intr, ndev, "Unhandled interrupt TX TIME= OUT\n"); > >> > > >> > Why do you enable that interrupt if you can't handle it? > >> > >> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_U= A_INT) > > > > So the bits 9 and 2, respectively, in the interrupt enable register > > are useless? >=20 > Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it = just > the interrupt state showing an event? IIRC some other hardware blocks hav= e this > behavior, such as the timer. That's quite easy to implement, you can do a bitwise and on the status and enable registers. Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --kaF1vgn83Aa7CiXN Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXnFh0AAoJEBx+YmzsjxAgCmQP/RlT8yaQqNXzwJI9pGSCgtm2 6SnzN65hT3FbgTdy3WQgfZNSqJmrNM+7Skig8Qq/YIMCs+FJAqMlWoq97kvK8vSX iTmb03coDLpOlNoh4E4YsB2LmGxeBEaYz2MZKhX78OHTTqLvaiB/7S+evNULUrDM tKGKONPFS24fMFhcSCaquuj67JsHQnwh2cmcCcnn2wGtr3eLZ9ZQiks3EDmBk+Lb tMAT0qEWkJWHT0tMr2eBnPXhYMWKlvvoggL+wcZSR3KEQL5MgRfOvNSf6Ttda80J irio2MEFqSvvbMRrV7+vsAyha47+x6IAn8QXiwlmO+ZsHS4Ijoz793sTkXGZjJ62 k/CIEivjV35mFM7ik+FxKuURuoCS2Ymjliy1H2CI3zH3mVvCxdD37VwSLPxPI6Ef 8mLNcY/VV4qr4i1xhE8IzgMF9/FACmJbLUqqfEaMd26uOXXQ9CsK1aVdqBKVAp5/ BdwFG9gV+pr9Orqgt3nFdROhJ1teY5N8Pt76xwtlHpYNP2M83aSJTL9pg+fl4H3t I4gATf7srVhOqFxSC+b2CFZQITI7svk6MU9uUJYhwJCZ6BVLDaQuJ2xiGZCi4p+B /Bc9KiewZDZ0IIIOTnxt2WZR5WzOcjIPu/QHUJcKzfLq/4hJWAZF6FMONmMaFZDo lcgmjU8pEfoipWWV1qd3 =oqMF -----END PGP SIGNATURE----- --kaF1vgn83Aa7CiXN-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maxime Ripard Subject: Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver Date: Sat, 30 Jul 2016 09:34:12 +0200 Message-ID: <20160730073412.GL6215@lukather> References: <1469001800-11615-1-git-send-email-clabbe.montjoie@gmail.com> <1469001800-11615-2-git-send-email-clabbe.montjoie@gmail.com> <20160725195455.GQ7419@lukather> <20160728145734.GD7582@Red> <20160729172526.GE6215@lukather> Reply-To: maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="kaF1vgn83Aa7CiXN" Cc: LABBE Corentin , Rob Herring , Mark Rutland , Russell King , David Miller , netdev , devicetree , linux-arm-kernel , linux-kernel , linux-sunxi To: Chen-Yu Tsai Return-path: Sender: linux-sunxi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org Content-Disposition: inline In-Reply-To: List-Post: , List-Help: , List-Archive: , List-Unsubscribe: , List-Id: netdev.vger.kernel.org --kaF1vgn83Aa7CiXN Content-Type: text/plain; charset=UTF-8 Content-Disposition: inline On Sat, Jul 30, 2016 at 09:30:01AM +0800, Chen-Yu Tsai wrote: > >> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev) > >> > > +{ > >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > >> > > + u32 reg = 0; > >> > > + > >> > > + if (priv->variant == H3_EMAC) > >> > > + reg = H3_EPHY_DEFAULT_VALUE; > >> > > >> > Why do you need that? > >> > > >> For resetting the syscon to the factory default. > > > > Yes, but does it matter? Does it have any side effect? Is that > > register shared with another device? > > > > Otherwise, either it won't be used anymore, and you don't care, or you > > will reload the driver later, and the driver should work whatever > > state is programmed in there. In both cases, you don't need to reset > > that value. > > The "default" setting also disables and powers down the internal PHY. > I think that's a good thing? The naming could be better. Ah, it might, and that would obviously be the right thing to do. Using a define for those enable and power down bits would be better though. > >> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id) > >> > > +{ > >> > > + struct net_device *ndev = dev_id; > >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > >> > > + u32 v, u; > >> > > + > >> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA); > >> > > + > >> > > + /* When this bit is asserted, a frame transmission is completed. */ > >> > > + if (v & BIT(0)) { > >> > > + priv->estats.tx_int++; > >> > > + writel(0, priv->base + SUN8I_EMAC_INT_EN); > >> > > + napi_schedule(&priv->napi); > >> > > + } > >> > > + > >> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */ > >> > > + if (v & BIT(1)) > >> > > + priv->estats.tx_dma_stop++; > >> > > + > >> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor > >> > > + * and TX DMA FSM is suspended. > >> > > + */ > >> > > + if (v & BIT(2)) > >> > > + priv->estats.tx_dma_ua++; > >> > > + > >> > > + if (v & BIT(3)) > >> > > + netif_dbg(priv, intr, ndev, "Unhandled interrupt TX TIMEOUT\n"); > >> > > >> > Why do you enable that interrupt if you can't handle it? > >> > >> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT) > > > > So the bits 9 and 2, respectively, in the interrupt enable register > > are useless? > > Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it just > the interrupt state showing an event? IIRC some other hardware blocks have this > behavior, such as the timer. That's quite easy to implement, you can do a bitwise and on the status and enable registers. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --kaF1vgn83Aa7CiXN-- From mboxrd@z Thu Jan 1 00:00:00 1970 From: maxime.ripard@free-electrons.com (Maxime Ripard) Date: Sat, 30 Jul 2016 09:34:12 +0200 Subject: [PATCH v2 1/5] ethernet: add sun8i-emac driver In-Reply-To: References: <1469001800-11615-1-git-send-email-clabbe.montjoie@gmail.com> <1469001800-11615-2-git-send-email-clabbe.montjoie@gmail.com> <20160725195455.GQ7419@lukather> <20160728145734.GD7582@Red> <20160729172526.GE6215@lukather> Message-ID: <20160730073412.GL6215@lukather> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Sat, Jul 30, 2016 at 09:30:01AM +0800, Chen-Yu Tsai wrote: > >> > > +static void sun8i_emac_unset_syscon(struct net_device *ndev) > >> > > +{ > >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > >> > > + u32 reg = 0; > >> > > + > >> > > + if (priv->variant == H3_EMAC) > >> > > + reg = H3_EPHY_DEFAULT_VALUE; > >> > > >> > Why do you need that? > >> > > >> For resetting the syscon to the factory default. > > > > Yes, but does it matter? Does it have any side effect? Is that > > register shared with another device? > > > > Otherwise, either it won't be used anymore, and you don't care, or you > > will reload the driver later, and the driver should work whatever > > state is programmed in there. In both cases, you don't need to reset > > that value. > > The "default" setting also disables and powers down the internal PHY. > I think that's a good thing? The naming could be better. Ah, it might, and that would obviously be the right thing to do. Using a define for those enable and power down bits would be better though. > >> > > +static irqreturn_t sun8i_emac_dma_interrupt(int irq, void *dev_id) > >> > > +{ > >> > > + struct net_device *ndev = dev_id; > >> > > + struct sun8i_emac_priv *priv = netdev_priv(ndev); > >> > > + u32 v, u; > >> > > + > >> > > + v = readl(priv->base + SUN8I_EMAC_INT_STA); > >> > > + > >> > > + /* When this bit is asserted, a frame transmission is completed. */ > >> > > + if (v & BIT(0)) { > >> > > + priv->estats.tx_int++; > >> > > + writel(0, priv->base + SUN8I_EMAC_INT_EN); > >> > > + napi_schedule(&priv->napi); > >> > > + } > >> > > + > >> > > + /* When this bit is asserted, the TX DMA FSM is stopped. */ > >> > > + if (v & BIT(1)) > >> > > + priv->estats.tx_dma_stop++; > >> > > + > >> > > + /* When this asserted, the TX DMA can not acquire next TX descriptor > >> > > + * and TX DMA FSM is suspended. > >> > > + */ > >> > > + if (v & BIT(2)) > >> > > + priv->estats.tx_dma_ua++; > >> > > + > >> > > + if (v & BIT(3)) > >> > > + netif_dbg(priv, intr, ndev, "Unhandled interrupt TX TIMEOUT\n"); > >> > > >> > Why do you enable that interrupt if you can't handle it? > >> > >> Some interrupt fire even when not enabled (like RX_BUF_UA_INT/TX_BUF_UA_INT) > > > > So the bits 9 and 2, respectively, in the interrupt enable register > > are useless? > > Does it actually fire, i.e. pull the interrupt line on the GIC? Or is it just > the interrupt state showing an event? IIRC some other hardware blocks have this > behavior, such as the timer. That's quite easy to implement, you can do a bitwise and on the status and enable registers. Maxime -- Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: not available URL: