From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753333AbcG2RZm (ORCPT ); Fri, 29 Jul 2016 13:25:42 -0400 Received: from down.free-electrons.com ([37.187.137.238]:50185 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752283AbcG2RZj (ORCPT ); Fri, 29 Jul 2016 13:25:39 -0400 Date: Fri, 29 Jul 2016 19:25:26 +0200 From: Maxime Ripard To: LABBE Corentin Cc: robh+dt@kernel.org, mark.rutland@arm.com, wens@csie.org, linux@armlinux.org.uk, davem@davemloft.net, netdev@vger.kernel.org, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-sunxi@googlegroups.com Subject: Re: [PATCH v2 1/5] ethernet: add sun8i-emac driver Message-ID: <20160729172526.GE6215@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> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="ey/N+yb7u/X9mFhi" Content-Disposition: inline In-Reply-To: <20160728145734.GD7582@Red> 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 --ey/N+yb7u/X9mFhi Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jul 28, 2016 at 04:57:34PM +0200, LABBE Corentin wrote: > > > +static int sun8i_mdio_write(struct mii_bus *bus, int phy_addr, int p= hy_reg, > > > + u16 data) > > > +{ > > > + struct net_device *ndev =3D bus->priv; > > > + struct sun8i_emac_priv *priv =3D netdev_priv(ndev); > > > + u32 reg; > > > + int err; > > > + > > > + err =3D readl_poll_timeout(priv->base + SUN8I_EMAC_MDIO_CMD, reg, > > > + !(reg & MDIO_CMD_MII_BUSY), 100, 10000); > > > + if (err) { > > > + dev_err(priv->dev, "%s timeout %x\n", __func__, reg); > > > + return err; > > > + } > >=20 > > Why the poll_timeout variant? > >=20 > Because, in case of bad clock/reset/regulator setting, the value > expected to come could never be set. Ah, I missed that it was for a busy bit, my bad. However, you seem to be using that on several occasions, maybe you could turn that into a function? > > > +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; > >=20 > > Why do you need that? > >=20 > 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. > > > +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 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"); > >=20 > > 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_I= NT) So the bits 9 and 2, respectively, in the interrupt enable register are useless? > > And printing in the interrupt handler is a very bad idea. >=20 > There are printed only when DEBUG is set, so not a problem ? It's always a problem, this adds a very significant latency and will fill the kernel log buffer at an insane rate, flushing out actual important messages, for no particular reason. > > > + > > > + return IRQ_HANDLED; > >=20 > > The lack of spinlocks in there is quite worrying. > >=20 >=20 > The interrupt handler just do nothing harmfull if it race with itself. > Just stats, enabling NAPI etc.. > Anyway, It miss a comment for that non-locking strategy The interrupt handler cannot race with itself. The interrupts will be masked on the local CPU and the interrupt can only be delivered to a single CPU (so, the one that the handler is currently running from). > > > +} > > > + > > > +static int sun8i_emac_probe(struct platform_device *pdev) > > > +{ > > > + struct device_node *node =3D pdev->dev.of_node; > > > + struct sun8i_emac_priv *priv; > > > + struct net_device *ndev; > > > + struct resource *res; > > > + int ret; > > > + > > > + ret =3D dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32)); > > > + if (ret) { > > > + dev_err(&pdev->dev, "No suitable DMA available\n"); > > > + return ret; > > > + } > >=20 > > Isn't that the default? > >=20 > No, it is necessary on arm64 as apritzel requested. http://lxr.free-electrons.com/source/drivers/of/device.c#L93 It seems to be shared between the two. Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --ey/N+yb7u/X9mFhi Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 iQIcBAEBAgAGBQJXm5GGAAoJEBx+YmzsjxAgMGgQAIKJNzVlWjzydeKQuf8NCcmK WuRk7mFUfYfKmcj0L7Qjz4hN4XllU00Y57Gq7DomZ/K0QSWrS4i+P8G7NsbcEKVk ea/Jpeaw8UIrTgCwtjDIgcXN965x5og8IRbHdvoqlnL6DVXmFATsIUAVury+BGXW DzyBitHmQJqNqgPaeKFSkqx1K9eu7unKV6Y3p5sqXt2nPyz0QSfIwQ7CxqjHRh3y BWQwFOViqDOs17Vs2Y3z5Up2ADbAb6qXFHykEvLOWHGKbssVCcbLkqgVA6EO00wA 8tsNYvHsHSSX+bV67rbxlEcH4A3VGT1NYD9petvmHfI1jxPO9yDMhxk6t+g6gKzl cxERXh7y49YHj7bw8Hra4aa6dUGCx4YnSAC0uyKsP4G3LXSRf+CiGkD51IAVKctD TfFBHOiEpStfqfIbf0n3km8XCGNQ4rrZx0dz5T1QXsYH2Yoj1PXzTiUdJsNptMjD 3a0DRGp3KLgciInm8dO621isKIquKoMU238QGmrGuguwg7DZbZ2hZz/qLZszXF9X 6R3IivuokWq/XWyg5MQrMjEhzWViGYIVv9ouIo7RBGmhnhnYSSLsjwXv0khl4uu9 PXFR9WXO4sdoNZgOshOzyEYyePv/EKYPIbF3v0ADZUjxnUsEiJcthipeH6xeLtip eFcNyR7v4LYstaDbTHan =H3re -----END PGP SIGNATURE----- --ey/N+yb7u/X9mFhi--