From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753044AbdBUWWb (ORCPT ); Tue, 21 Feb 2017 17:22:31 -0500 Received: from mail.free-electrons.com ([62.4.15.54]:38606 "EHLO mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751208AbdBUWWW (ORCPT ); Tue, 21 Feb 2017 17:22:22 -0500 Date: Tue, 21 Feb 2017 14:22:17 -0800 From: Maxime Ripard To: Corentin Labbe Cc: peppe.cavallaro@st.com, robh+dt@kernel.org, mark.rutland@arm.com, wens@csie.org, linux@armlinux.org.uk, catalin.marinas@arm.com, will.deacon@arm.com, alexandre.torgue@st.com, 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 05/21] net-next: stmmac: Add dwmac-sun8i Message-ID: <20170221222217.62barcu445stewvr@lukather> References: <20170216124859.14346-1-clabbe.montjoie@gmail.com> <20170216124859.14346-6-clabbe.montjoie@gmail.com> <20170216190524.l2ddzhwabzdpdphx@lukather> <20170217131802.GB24993@Red> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="zzcjb24ogs6xglso" Content-Disposition: inline In-Reply-To: <20170217131802.GB24993@Red> User-Agent: Mutt/1.6.2-neo (2016-08-21) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --zzcjb24ogs6xglso Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Fri, Feb 17, 2017 at 02:18:02PM +0100, Corentin Labbe wrote: > On Thu, Feb 16, 2017 at 08:05:24PM +0100, Maxime Ripard wrote: > > Hi, > >=20 >=20 > [...] > > > + > > > +struct emac_variant { > > > + u32 default_syscon_value; > >=20 > > Why do you need a default value? Can't you read it from the syscon > > directly? > >=20 >=20 > Why not, but you can see the default value as "value for disabled > state". i'm not sure what you mean here, sorry. > My fear is that something (uboot) modify it (keep it activated) > before driver load. You could have the same argument there then for the board that require reading it. What if U-boot modified it to some non-functional state? Either you trust the value there, and you read it, or you don't, and then you never read it. But being stuck in between doesn't seem that great. > > > +static void sun8i_dwmac_dma_start_tx(void __iomem *ioaddr) > > > +{ > > > + u32 v; > > > + > > > + v =3D readl(ioaddr + EMAC_TX_CTL0); > > > + v |=3D EMAC_TX_TRANSMITTER_EN; > > > + writel(v, ioaddr + EMAC_TX_CTL0); > > > + > > > + v =3D readl(ioaddr + EMAC_TX_CTL1); > > > + v |=3D EMAC_TX_DMA_START; > > > + v |=3D EMAC_TX_DMA_EN; > > > + writel(v, ioaddr + EMAC_TX_CTL1); > >=20 > > This is a bit worrying. There's not a single lock in your driver, > > while you have a significant number of read / modify / write. > >=20 > > Where is the locking handled? >=20 > All thoses function are handled by the "stmmac_ops framework", all > other glue drivers does not lock anything. Most of them seem to use regmap though, that has an internal lock. > The few functions that need locking already got it on the calling > stmmac side. Ok. > > > + > > > + if (of_property_read_bool(priv->plat->phy_node, > > > + "allwinner,leds-active-low")) > > > + reg |=3D H3_EPHY_LED_POL; > > > + else > > > + reg &=3D ~H3_EPHY_LED_POL; > > > + > > > + ret =3D of_mdio_parse_addr(priv->device, > > > + priv->plat->phy_node); > > > + if (ret < 0) { > > > + dev_err(priv->device, "Could not parse MDIO addr\n"); > > > + return ret; > > > + } > > > + /* of_mdio_parse_addr returns a valid (0 ~ 31) PHY > > > + * address. No need to mask it again. > > > + */ > > > + reg |=3D ret << H3_EPHY_ADDR_SHIFT; > > > + } > > > + } > > > + > > > + if (!of_property_read_u32(node, "allwinner,tx-delay", &val)) { > >=20 > > How do you compute it? Can't this be done through auto-training? >=20 > The value is the same as used in vendor BSP kernel. This is not really usable though. I've had already three boards that never got any BSP kernel. You need to be able at least to document some way to compute it (even if it's based on manual, trial and error process). > I do not understand what you mean by auto-training. Being able to automatically detect the optimal settings at boot time. Thanks! Maxime --=20 Maxime Ripard, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com --zzcjb24ogs6xglso Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIcBAEBCAAGBQJYrL2VAAoJEBx+YmzsjxAgnF0P/25lXhE5u4mnCF/C+KZi7N8S BrK/mG+ZUycpDddBwaSXa9XE8o4RS4BF2drSsHFG6PNKxYr3XmhCWlz+BQ976hCh 1j8H+X590MbYOTtdNzwMuLO4kFvZHKqe6fz9qJX8ErJQb4KxWpsd+8Ht+2DVculV OBZyJ/V1yGTxyoD6P/v6pm93LpKuH8VveM29dNc8Od1EoFVLPLF7ijy5oL15l9KE i7faKjtTaOoDBDRWc/xMu0sD/pVIpfPRXKcUB0RWTzr0+/qZVpPtonvOaUJr4CNC ZBPnrcd4vnEjOdz3HYWqViDf/YxD1UkZ20vLS/5dhM1ngh/enolWX1jZFU0Ci7hk QWZYU3z7d66nZJ+6eFkS3HHYHzp0GOXiYMqQ3y+JhQ0KYz+hJq7/Gd2quq2Y7yhG RTizJCfMbL4ch5X1q250YnaouGhRev1bA8SyhJw9KtmqTszRg13mexwwpg+9UKaF 43dWl3AGTEWzIBIBYLil8Iw7E7PtZHSUPqZ0HkZ12tog8/HDNIZv6hSVs3UiveJZ 2WNmJIEYpMxEsfbVQprN4J3y835VeCzZSAM7knySUlXc0skIaBGAmKEL9BQRAf9z 4ULKpeHiP4fU5mCKdXmaW/tCEKma1lMOIp2y7EF9jLf2XxYpkpe7zsLdoRKXSXue XD/DV77KDsfg3VT3/Re6 =O4NF -----END PGP SIGNATURE----- --zzcjb24ogs6xglso--