From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753612Ab0IOMlf (ORCPT ); Wed, 15 Sep 2010 08:41:35 -0400 Received: from sm-d311v.smileserver.ne.jp ([203.211.202.206]:25206 "EHLO sm-d311v.smileserver.ne.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752724Ab0IOMkx (ORCPT ); Wed, 15 Sep 2010 08:40:53 -0400 Message-ID: <001701cb54d3$3b75a730$66f8800a@maildom.okisemi.com> From: "Masayuki Ohtake" To: "Joe Perches" Cc: "David S. Miller" , "Randy Dunlap" , "John Linn" , "Ralf Baechle" , "Kristoffer Glembo" , "Maxime Bizon" , "Greg Rose" , "ML netdev" , "LKML" , "MeeGo" , "Stephen Hemminger" , "Jili Slaby" , "FUJITA Tomonori" , "Wang, Qi" , "Wang, Yong Y" , "Andrew" , "Intel OTC" , "Foster, Margie" , "Arjan" , "Toshiharu Okada" , "Takahiro Shimizu" , "Tomoya Morinaga" References: <4C89C23A.6090707@dsn.okisemi.com> <1284100327.24986.324.camel@Joe-Laptop> Subject: Re: [PATCH v3] Gigabit Ethernet driver of Topcliff PCH Date: Wed, 15 Sep 2010 21:19:46 +0900 X-Priority: 3 X-MSMail-Priority: Normal X-Mailer: Microsoft Outlook Express 6.00.2800.1983 X-MimeOLE: Produced By Microsoft MimeOLE V6.00.2800.1983 X-Hosting-Pf: 0 X-NAI-Spam-Score: 1 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Joe Thank you for your comments. My reply comments are included in the following. I will resubmit after modified. Thanks Ohtake ----- Date: Thu, 09 Sep 2010 23:32:07 -0700 From: "Joe Perches" > > diff --git a/drivers/net/pch_gbe/pch_gbe_ethtool.c b/drivers/net/pch_gbe/pch_gbe_ethtool.c > [] > > +static void pch_gbe_get_regs(struct net_device *netdev, > > + struct ethtool_regs *regs, void *p) > > +{ > > + struct pch_gbe_adapter *adapter = netdev_priv(netdev); > > + struct pch_gbe_hw *hw = &adapter->hw; > > + struct pci_dev *pdev = adapter->pdev; > > + u32 *regs_buff = p; > > + u16 i, reg, tmp; > > + > > + regs->version = pdev->revision; > > + regs->version = 0x1000000 | (regs->version << 16) | pdev->device; > > Might be simpler as: > regs->version = 0x1000000 | (__u32)pdev->revision << 16 | pdev->device; > > The block below is a bit confusing to me. [masa] This will be modified so. > > + memset(p, 0, PCH_GBE_REGS_LEN * (int)sizeof(u32)); > > It seems the memset is unnecessary as it's completely > overwritten below. [masa] This will be deleted. > > > + for (i = 0; i < PCH_GBE_MAC_REGS_LEN; i++) > > + regs_buff[i] = ioread32(&hw->reg->INT_ST + i); > > + /* PHY register */ > > + for (i = PCH_GBE_MAC_REGS_LEN, reg = 0; reg < PCH_GBE_PHY_REGS_LEN; > > + i++, reg++) { > > + pch_gbe_hal_read_phy_reg(&adapter->hw, reg, &tmp); > > + regs_buff[i] = tmp; > > + } > > I think i'd be simpler to do something like: > > for (i = 0; i < PCH_GBE_MAC_REGS_LEN; i++) > *regs_buff++ = ioread32(&hw->reg->INT_ST + i); > > /* PHY register */ > for (i = 0; i < PCH_GBE_PHY_REGS_LEN; i++) { > pch_gbe_hal_read_phy_reg(&adapter->hw, i, &tmp); > *regs_buff++ = tmp; > } [masa] This will be modified so. > +void pch_gbe_mac_mar_set(struct pch_gbe_hw *hw, u8 * addr, u32 index) > > +{ > > + u32 mar_low, mar_high, adrmask; > > + > > + pr_debug("index : 0x%x\n", index); > > + > > + /* > > + * HW expects these in little endian so we reverse the byte order > > + * from network order (big endian) to little endian > > + */ > > + mar_high = ((u32) addr[0] | ((u32) addr[1] << 8) | > > + ((u32) addr[2] << 16) | ((u32) addr[3] << 24)); > > + mar_low = ((u32) addr[4] | ((u32) addr[5] << 8)); > > + /* Stop the MAC Address of index. */ > > + adrmask = ioread32(&hw->reg->ADDR_MASK); > > + iowrite32((adrmask | (0x0001 << index)), &hw->reg->ADDR_MASK); > > + /* wait busy */ > > + while ((ioread32(&hw->reg->ADDR_MASK) & PCH_GBE_BUSY)) { > > + int tmp = 0; > > + udelay(20); > > + tmp++; > > + if (tmp == 1000) { > > + pr_err("Address mask bit is not cleared\n"); > > + break; > > + } > > + } > > You need to move the declaration of tmp out of the while. > > Do these really need to be busy-waits? [masa] The declaration of tmp will be moved. It is necessary to wait for BUSY by the specification of hardware. > > > + /* Set the MAC address to the MAC address 1A/1B register */ > > + iowrite32(mar_high, &hw->reg->mac_adr[index].high); > > + iowrite32(mar_low, &hw->reg->mac_adr[index].low); > > + /* Start the MAC address of index */ > > + iowrite32((adrmask & ~(0x0001 << index)), &hw->reg->ADDR_MASK); > > + /* wait busy */ > > + while ((ioread32(&hw->reg->ADDR_MASK) & PCH_GBE_BUSY)) { > > + int tmp = 0; > > + udelay(20); > > + tmp++; > > + if (tmp == 1000) { > > + pr_err("Address mask bit is not cleared\n"); > > + break; > > + } > > + } > > +} > > Here too. There are more of these too I'm not listing. > Perhaps you could review the logic a bit more. > [masa] This will be modified.