Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752656AbbKOKnZ (ORCPT ); Sun, 15 Nov 2015 05:43:25 -0500 Received: from smtp-1b.atlantis.sk ([80.94.52.26]:37774 "EHLO smtp-1b.atlantis.sk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751540AbbKOKnW convert rfc822-to-8bit (ORCPT ); Sun, 15 Nov 2015 05:43:22 -0500 X-Greylist: delayed 587 seconds by postgrey-1.27 at vger.kernel.org; Sun, 15 Nov 2015 05:43:21 EST From: Ondrej Zary To: Andy Shevchenko Subject: Re: [PATCH 1/2] dl2k: Add support for IP1000A-based cards Date: Sun, 15 Nov 2015 11:33:23 +0100 User-Agent: KMail/1.9.10 (enterprise35 0.20100827.1168748) Cc: netdev , Francois Romieu , David Miller , Kernel development list References: <1447538738-28605-1-git-send-email-linux@rainbow-software.org> In-Reply-To: X-KMail-QuotePrefix: > MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 8BIT Content-Disposition: inline Message-Id: <201511151133.24471.linux@rainbow-software.org> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8163 Lines: 221 On Sunday 15 November 2015 03:30:25 Andy Shevchenko wrote: > On Sun, Nov 15, 2015 at 12:05 AM, Ondrej Zary > > wrote: > > Add support for IP1000A chips to dl2k driver. > > IP1000A chip looks like a TC9020 with integrated PHY. > > > > This allows IP1000A chips to work reliably because the ipg driver is > > buggy - it loses packets under load and then completely stops > > transmitting data. > > > > Tested with Asus NX1101 v2.0 at 10, 100 and 1000Mbps. > > Also verified that it does not break D-Link DGE-550T. > > > > Signed-off-by: Ondrej Zary > > Nitpicks below. > > > --- a/drivers/net/ethernet/dlink/dl2k.c > > +++ b/drivers/net/ethernet/dlink/dl2k.c > > @@ -253,6 +253,18 @@ rio_probe1 (struct pci_dev *pdev, const struct > > pci_device_id *ent) if (err) > > goto err_out_unmap_rx; > > > > + if (np->chip_id == CHIP_IP1000A && > > + (np->pdev->revision == 0x40 || np->pdev->revision == 0x41)) { > > + /* PHY magic taken from ipg driver */ > > + mii_write(dev, np->phy_addr, 31, 0x0001); > > + mii_write(dev, np->phy_addr, 27, 0x01e0); > > + mii_write(dev, np->phy_addr, 31, 0x0002); > > + mii_write(dev, np->phy_addr, 27, 0xeb8e); > > + mii_write(dev, np->phy_addr, 31, 0x0000); > > + mii_write(dev, np->phy_addr, 30, 0x005e); > > + mii_write(dev, np->phy_addr, 9, 0x0700); > > Isn't too many magic numbers? It's taken from the ipg driver where it was even more cryptic. I'll try to decode the magic numbers to something readable. > > + } > > + > > /* Fiber device? */ > > np->phy_media = (dr16(ASICCtrl) & PhyMedia) ? 1 : 0; > > np->link_status = 0; > > @@ -361,6 +373,11 @@ parse_eeprom (struct net_device *dev) > > for (i = 0; i < 6; i++) > > dev->dev_addr[i] = psrom->mac_addr[i]; > > > > + if (np->chip_id == CHIP_IP1000A) { > > + np->led_mode = psrom->led_mode; > > + return 0; > > + } > > + > > if (np->pdev->vendor != PCI_VENDOR_ID_DLINK) { > > return 0; > > } > > @@ -406,6 +423,28 @@ parse_eeprom (struct net_device *dev) > > return 0; > > } > > > > +static void rio_set_led_mode(struct net_device *dev) > > +{ > > + struct netdev_private *np = netdev_priv(dev); > > + void __iomem *ioaddr = np->ioaddr; > > + u32 mode; > > + > > + if (np->chip_id != CHIP_IP1000A) > > + return; > > + > > + mode = dr32(ASICCtrl); > > + mode &= ~(IPG_AC_LED_MODE_BIT_1 | IPG_AC_LED_MODE | > > IPG_AC_LED_SPEED); + > > + if ((np->led_mode & 0x03) > 1) > > Isn't "& 0x03) > 1" the same as plain "& 0x02"? Yes, you're right. It's copied from the ipg driver. I'll fix it. > > + mode |= IPG_AC_LED_MODE_BIT_1; > > + if ((np->led_mode & 0x01) == 1) > > Hmm… Seems redundant == 1. > > > + mode |= IPG_AC_LED_MODE; > > + if ((np->led_mode & 0x08) == 8) > > Similar here. > > > + mode |= IPG_AC_LED_SPEED; > > + > > + dw32(ASICCtrl, mode); > > +} > > + > > static int > > rio_open (struct net_device *dev) > > { > > @@ -424,6 +463,8 @@ rio_open (struct net_device *dev) > > GlobalReset | DMAReset | FIFOReset | NetworkReset | > > HostReset); mdelay(10); > > > > + rio_set_led_mode(dev); > > + > > /* DebugCtrl bit 4, 5, 9 must set */ > > dw32(DebugCtrl, dr32(DebugCtrl) | 0x0230); > > > > @@ -433,9 +474,10 @@ rio_open (struct net_device *dev) > > > > alloc_list (dev); > > > > - /* Get station address */ > > - for (i = 0; i < 6; i++) > > - dw8(StationAddr0 + i, dev->dev_addr[i]); > > + /* Set station address */ > > + for (i = 0; i < 3; i++) > > + dw16(StationAddr0 + 2 * i, > > + cpu_to_le16(((u16 *)dev->dev_addr)[i])); > > Is it specific requirement for new HW? If not, may be another patch > with micro optimizations. Yes, the old method does not work on IP1000A. In fact, even the TC9020 datasheet says: "Access Rule: Word, Double Word". So it was always wrong but worked anyway until now. > > set_multicast (dev); > > if (np->coalesce) { > > @@ -780,6 +822,7 @@ tx_error (struct net_device *dev, int tx_status) > > break; > > mdelay (1); > > } > > + rio_set_led_mode(dev); > > rio_free_tx (dev, 1); > > /* Reset TFDListPtr */ > > dw32(TFDListPtr0, np->tx_ring_dma + > > @@ -799,6 +842,7 @@ tx_error (struct net_device *dev, int tx_status) > > break; > > mdelay (1); > > } > > + rio_set_led_mode(dev); > > /* Let TxStartThresh stay default value */ > > } > > /* Maximum Collisions */ > > @@ -965,6 +1009,7 @@ rio_error (struct net_device *dev, int int_status) > > dev->name, int_status); > > dw16(ASICCtrl + 2, GlobalReset | HostReset); > > mdelay (500); > > + rio_set_led_mode(dev); > > } > > } > > > > diff --git a/drivers/net/ethernet/dlink/dl2k.h > > b/drivers/net/ethernet/dlink/dl2k.h index 23c07b0..8f4f612 100644 > > --- a/drivers/net/ethernet/dlink/dl2k.h > > +++ b/drivers/net/ethernet/dlink/dl2k.h > > @@ -211,6 +211,10 @@ enum ASICCtrl_HiWord_bits { > > ResetBusy = 0x0400, > > }; > > > > +#define IPG_AC_LED_MODE BIT(14) > > +#define IPG_AC_LED_SPEED BIT(27) > > +#define IPG_AC_LED_MODE_BIT_1 BIT(29) > > + > > /* Transmit Frame Control bits */ > > enum TFC_bits { > > DwordAlign = 0x00000000, > > @@ -332,7 +336,10 @@ typedef struct t_SROM { > > u16 asic_ctrl; /* 0x02 */ > > u16 sub_vendor_id; /* 0x04 */ > > u16 sub_system_id; /* 0x06 */ > > - u16 reserved1[12]; /* 0x08-0x1f */ > > + u16 pci_base_1; /* 0x08 (IP1000A only) */ > > + u16 pci_base_2; /* 0x0a (IP1000A only) */ > > + u16 led_mode; /* 0x0c (IP1000A only) */ > > + u16 reserved1[9]; /* 0x0e-0x1f */ > > u8 mac_addr[6]; /* 0x20-0x25 */ > > u8 reserved2[10]; /* 0x26-0x2f */ > > u8 sib[204]; /* 0x30-0xfb */ > > @@ -397,6 +404,7 @@ struct netdev_private { > > u16 advertising; /* NWay media advertisement */ > > u16 negotiate; /* Negotiated media */ > > int phy_addr; /* PHY addresses. */ > > + u16 led_mode; /* LED mode read from EEPROM (IP1000A > > only) */ }; > > > > /* The station address location in the EEPROM. */ > > @@ -407,10 +415,15 @@ struct netdev_private { > > class_mask of the class are honored during the > > comparison. driver_data Data private to the driver. > > */ > > +#define CHIP_IP1000A 1 > > > > static const struct pci_device_id rio_pci_tbl[] = { > > {0x1186, 0x4000, PCI_ANY_ID, PCI_ANY_ID, }, > > {0x13f0, 0x1021, PCI_ANY_ID, PCI_ANY_ID, }, > > + { PCI_VDEVICE(SUNDANCE, 0x1023), CHIP_IP1000A }, > > + { PCI_VDEVICE(SUNDANCE, 0x2021), CHIP_IP1000A }, > > + { PCI_VDEVICE(DLINK, 0x9021), CHIP_IP1000A }, > > + { PCI_VDEVICE(DLINK, 0x4020), CHIP_IP1000A }, > > { } > > }; > > MODULE_DEVICE_TABLE (pci, rio_pci_tbl); > > -- > > Ondrej Zary > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" > > in the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > Please read the FAQ at http://www.tux.org/lkml/ -- Ondrej Zary -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/