From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Date: Mon, 11 Jan 2016 11:11:25 +0000 Subject: Re: [PATCH 1/2] ravb: stop reading ECMR in ravb_emac_init() Message-Id: <56938DDD.6000506@cogentembedded.com> List-Id: References: <5859243.yu8qorLmay@wasted.cogentembedded.com> <1554972.rhPQTyRgNp@wasted.cogentembedded.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Geert Uytterhoeven Cc: "netdev@vger.kernel.org" , Linux-sh list Hello. On 1/11/2016 12:45 PM, Geert Uytterhoeven wrote: > On Sun, Jan 10, 2016 at 10:27 PM, Sergei Shtylyov > wrote: >> The code in ravb_emac_init() twiddling the ECMR bits always looked a bit >> strange to me: if one intends to respect 'priv->duplex', why save old value >> of the ECMR.DM bit? As all the other bits are zeroed anyway, we don't >> really need to read ECMR before writing to it. > > Just an observation, I don't know about correctness: priv->duplex wasn't > respected if ECMR_DM was already set before. > > Same comment for the second patch. That's what the patches try to fix. Perhaps the description was too terse? >> Signed-off-by: Sergei Shtylyov >> >> --- >> drivers/net/ethernet/renesas/ravb_main.c | 7 ++----- >> 1 file changed, 2 insertions(+), 5 deletions(-) >> >> Index: net/drivers/net/ethernet/renesas/ravb_main.c >> =================================>> --- net.orig/drivers/net/ethernet/renesas/ravb_main.c >> +++ net/drivers/net/ethernet/renesas/ravb_main.c >> @@ -338,16 +338,13 @@ error: >> static void ravb_emac_init(struct net_device *ndev) >> { >> struct ravb_private *priv = netdev_priv(ndev); >> - u32 ecmr; >> >> /* Receive frame limit set register */ >> ravb_write(ndev, ndev->mtu + ETH_HLEN + VLAN_HLEN + ETH_FCS_LEN, RFLR); >> >> /* PAUSE prohibition */ >> - ecmr = ravb_read(ndev, ECMR); >> - ecmr &= ECMR_DM; > > Perhaps you misread as "ecmr &= ~ECMR_DM"? If it was so, there would be no point in patching... [...] > Gr{oetje,eeting}s, > > Geert MBR, Sergei