[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Sasha Neftin]

This patch adds the beginning framework onto which I am going to add
the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
Ethernet Controller.

Sasha Neftin (v2):
update module author, copyright and licence header
cosmetic changes

Sasha Neftin (v3):
remove unused header files

Sasha Neftin (v4):
update brand name
fix syntax by input from community
replace e1000_ prefix with igc_ prefix

Sasha Neftin (v5):
no changes

Sasha Neftin (v6):
no changes

Signed-off-by: Sasha Neftin <sasha.neftin@intel.com>
---
 drivers/net/ethernet/intel/Kconfig        |  16 +++
 drivers/net/ethernet/intel/Makefile       |   1 +
 drivers/net/ethernet/intel/igc/Makefile   |  10 ++
 drivers/net/ethernet/intel/igc/igc.h      |  29 +++++
 drivers/net/ethernet/intel/igc/igc_hw.h   |  10 ++
 drivers/net/ethernet/intel/igc/igc_main.c | 146 ++++++++++++++++++++++
 6 files changed, 212 insertions(+)
 create mode 100644 drivers/net/ethernet/intel/igc/Makefile
 create mode 100644 drivers/net/ethernet/intel/igc/igc.h
 create mode 100644 drivers/net/ethernet/intel/igc/igc_hw.h
 create mode 100644 drivers/net/ethernet/intel/igc/igc_main.c

diff --git a/drivers/net/ethernet/intel/Kconfig b/drivers/net/ethernet/intel/Kconfig
index 1ab613eb5796..fcb676398022 100644
--- a/drivers/net/ethernet/intel/Kconfig
+++ b/drivers/net/ethernet/intel/Kconfig
@@ -280,4 +280,20 @@ config FM10K
 	  To compile this driver as a module, choose M here. The module
 	  will be called fm10k.  MSI-X interrupt support is required
 
+config IGC
+	tristate "Intel(R) Ethernet Controller I225-LM/I225-V support"
+	default n
+	depends on PCI
+	---help---
+	  This driver supports Intel(R) Ethernet Controller I225-LM/I225-V
+	  family of adapters.
+
+	  For more information on how to identify your adapter, go
+	  to the Adapter & Driver ID Guide that can be located at:
+
+	  <http://support.intel.com>
+
+	  To compile this driver as a module, choose M here. The module
+	  will be called igc.
+
 endif # NET_VENDOR_INTEL
diff --git a/drivers/net/ethernet/intel/Makefile b/drivers/net/ethernet/intel/Makefile
index 807a4f8c7e4e..18f4aac68bc0 100644
--- a/drivers/net/ethernet/intel/Makefile
+++ b/drivers/net/ethernet/intel/Makefile
@@ -7,6 +7,7 @@ obj-$(CONFIG_E100) += e100.o
 obj-$(CONFIG_E1000) += e1000/
 obj-$(CONFIG_E1000E) += e1000e/
 obj-$(CONFIG_IGB) += igb/
+obj-$(CONFIG_IGC) += igc/
 obj-$(CONFIG_IGBVF) += igbvf/
 obj-$(CONFIG_IXGBE) += ixgbe/
 obj-$(CONFIG_IXGBEVF) += ixgbevf/
diff --git a/drivers/net/ethernet/intel/igc/Makefile b/drivers/net/ethernet/intel/igc/Makefile
new file mode 100644
index 000000000000..3d13b015d401
--- /dev/null
+++ b/drivers/net/ethernet/intel/igc/Makefile
@@ -0,0 +1,10 @@
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c)  2018 Intel Corporation
+
+#
+# Intel(R) I225-LM/I225-V 2.5G Ethernet Controller
+#
+
+obj-$(CONFIG_IGC) += igc.o
+
+igc-objs := igc_main.o
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
new file mode 100644
index 000000000000..afe595cfcf63
--- /dev/null
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c)  2018 Intel Corporation */
+
+#ifndef _IGC_H_
+#define _IGC_H_
+
+#include <linux/kobject.h>
+
+#include <linux/pci.h>
+#include <linux/netdevice.h>
+#include <linux/vmalloc.h>
+
+#include <linux/ethtool.h>
+
+#include <linux/sctp.h>
+
+#define IGC_ERR(args...) pr_err("igc: " args)
+
+#define PFX "igc: "
+
+#include <linux/timecounter.h>
+#include <linux/net_tstamp.h>
+#include <linux/ptp_clock_kernel.h>
+
+/* main */
+extern char igc_driver_name[];
+extern char igc_driver_version[];
+
+#endif /* _IGC_H_ */
diff --git a/drivers/net/ethernet/intel/igc/igc_hw.h b/drivers/net/ethernet/intel/igc/igc_hw.h
new file mode 100644
index 000000000000..aa68b4516700
--- /dev/null
+++ b/drivers/net/ethernet/intel/igc/igc_hw.h
@@ -0,0 +1,10 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c)  2018 Intel Corporation */
+
+#ifndef _IGC_HW_H_
+#define _IGC_HW_H_
+
+#define IGC_DEV_ID_I225_LM			0x15F2
+#define IGC_DEV_ID_I225_V			0x15F3
+
+#endif /* _IGC_HW_H_ */
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
new file mode 100644
index 000000000000..c926dab0057d
--- /dev/null
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -0,0 +1,146 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c)  2018 Intel Corporation */
+
+#include <linux/module.h>
+#include <linux/types.h>
+
+#include "igc.h"
+#include "igc_hw.h"
+
+#define DRV_VERSION	"0.0.1-k"
+#define DRV_SUMMARY	"Intel(R) 2.5G Ethernet Linux Driver"
+
+char igc_driver_name[] = "igc";
+char igc_driver_version[] = DRV_VERSION;
+static const char igc_driver_string[] = DRV_SUMMARY;
+static const char igc_copyright[] =
+	"Copyright(c) 2018 Intel Corporation.";
+
+static const struct pci_device_id igc_pci_tbl[] = {
+	{ PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_LM) },
+	{ PCI_VDEVICE(INTEL, IGC_DEV_ID_I225_V) },
+	/* required last entry */
+	{0, }
+};
+
+MODULE_DEVICE_TABLE(pci, igc_pci_tbl);
+
+/**
+ *  igc_probe - Device Initialization Routine
+ *  @pdev: PCI device information struct
+ *  @ent: entry in igc_pci_tbl
+ *
+ *  Returns 0 on success, negative on failure
+ *
+ *  igc_probe initializes an adapter identified by a pci_dev structure.
+ *  The OS initialization, configuring the adapter private structure,
+ *  and a hardware reset occur.
+ **/
+static int igc_probe(struct pci_dev *pdev,
+		     const struct pci_device_id *ent)
+{
+	int err, pci_using_dac;
+
+	err = pci_enable_device_mem(pdev);
+	if (err)
+		return err;
+
+	pci_using_dac = 0;
+	err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(64));
+	if (!err) {
+		err = dma_set_coherent_mask(&pdev->dev,
+					    DMA_BIT_MASK(64));
+		if (!err)
+			pci_using_dac = 1;
+	} else {
+		err = dma_set_mask(&pdev->dev, DMA_BIT_MASK(32));
+		if (err) {
+			err = dma_set_coherent_mask(&pdev->dev,
+						    DMA_BIT_MASK(32));
+			if (err) {
+				IGC_ERR("Wrong DMA configuration, aborting\n");
+				goto err_dma;
+			}
+		}
+	}
+
+	err = pci_request_selected_regions(pdev,
+					   pci_select_bars(pdev,
+							   IORESOURCE_MEM),
+					   igc_driver_name);
+	if (err)
+		goto err_pci_reg;
+
+	pci_set_master(pdev);
+	pci_save_state(pdev);
+	return 0;
+
+err_pci_reg:
+err_dma:
+	pci_disable_device(pdev);
+	return err;
+}
+
+/**
+ *  igc_remove - Device Removal Routine
+ *  @pdev: PCI device information struct
+ *
+ *  igc_remove is called by the PCI subsystem to alert the driver
+ *  that it should release a PCI device.  This could be caused by a
+ *  Hot-Plug event, or because the driver is going to be removed from
+ *  memory.
+ **/
+static void igc_remove(struct pci_dev *pdev)
+{
+	pci_release_selected_regions(pdev,
+				     pci_select_bars(pdev, IORESOURCE_MEM));
+
+	pci_disable_device(pdev);
+}
+
+static struct pci_driver igc_driver = {
+	.name     = igc_driver_name,
+	.id_table = igc_pci_tbl,
+	.probe    = igc_probe,
+	.remove   = igc_remove,
+};
+
+/**
+ *  igc_init_module - Driver Registration Routine
+ *
+ *  igc_init_module is the first routine called when the driver is
+ *  loaded. All it does is register with the PCI subsystem.
+ **/
+static int __init igc_init_module(void)
+{
+	int ret;
+
+	pr_info("%s - version %s\n",
+		igc_driver_string, igc_driver_version);
+
+	pr_info("%s\n", igc_copyright);
+
+	ret = pci_register_driver(&igc_driver);
+	return ret;
+}
+
+module_init(igc_init_module);
+
+/**
+ *  igc_exit_module - Driver Exit Cleanup Routine
+ *
+ *  igc_exit_module is called just before the driver is removed
+ *  from memory.
+ **/
+static void __exit igc_exit_module(void)
+{
+	pci_unregister_driver(&igc_driver);
+}
+
+module_exit(igc_exit_module);
+
+MODULE_AUTHOR("Intel Corporation, <linux.nics@intel.com>");
+MODULE_DESCRIPTION(DRV_SUMMARY);
+MODULE_LICENSE("GPL");
+MODULE_VERSION(DRV_VERSION);
+/* igc_main.c */
-- 
2.18.0

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Corinna Vinschen]

On Aug 23 10:05, Sasha Neftin wrote:
> This patch adds the beginning framework onto which I am going to add
> the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
> Ethernet Controller.

I'm curious.  The code looks very much like the igb code.  Wouldn't it
be simpler and less error prone to add these new NICs to igb, rather
than creating a new driver by (mostly) copying the code from igb?

What's the idea behind this?

> Sasha Neftin (v2):
> update module author, copyright and licence header
> cosmetic changes
> 
> Sasha Neftin (v3):
> remove unused header files
> 
> Sasha Neftin (v4):
> update brand name
> fix syntax by input from community
> replace e1000_ prefix with igc_ prefix

In theory this is neither necessary, nor does it actually help.
By keeping the e1000 prefixes it's much easier to compare to the
other, already existing drivers.

> Sasha Neftin (v5):
> no changes
> 
> Sasha Neftin (v6):
> no changes

Is that an oversight?  If there are no changes in v5 and v6, why
do we have them?!?


Corinna
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180823/a312b52b/attachment.asc>

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Jeff Kirsher]

On Thu, 2018-08-23 at 16:03 +0200, Corinna Vinschen wrote:
> On Aug 23 10:05, Sasha Neftin wrote:
> > This patch adds the beginning framework onto which I am going to
> add
> > the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
> > Ethernet Controller.
> 
> I'm curious.  The code looks very much like the igb code.  Wouldn't
> it
> be simpler and less error prone to add these new NICs to igb, rather
> than creating a new driver by (mostly) copying the code from igb?
> 
> What's the idea behind this?

This new driver is for a new upcoming client part, not server (which is
the igb driver).  Yes, the way that this new client part does resemble
how the igb driver operates, but this new client part will not have
many of the advance features (like SRIOV) that igb uses.  We also want
to keep the igb driver strictly for the 1GbE server devices.

> 
> > Sasha Neftin (v2):
> > update module author, copyright and licence header
> > cosmetic changes
> > 
> > Sasha Neftin (v3):
> > remove unused header files
> > 
> > Sasha Neftin (v4):
> > update brand name
> > fix syntax by input from community
> > replace e1000_ prefix with igc_ prefix
> 
> In theory this is neither necessary, nor does it actually help.
> By keeping the e1000 prefixes it's much easier to compare to the
> other, already existing drivers.
> 
> > Sasha Neftin (v5):
> > no changes
> > 
> > Sasha Neftin (v6):
> > no changes
> 
> Is that an oversight?  If there are no changes in v5 and v6, why
> do we have them?!?

Sasha is just noting that this particular patch did not change in v6,
but other patches did have changes in this new version of the series.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180823/b52fee5c/attachment.asc>

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Corinna Vinschen]

On Aug 23 08:12, Jeff Kirsher wrote:
> On Thu, 2018-08-23 at 16:03 +0200, Corinna Vinschen wrote:
> > On Aug 23 10:05, Sasha Neftin wrote:
> > > This patch adds the beginning framework onto which I am going to
> > add
> > > the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
> > > Ethernet Controller.
> > 
> > I'm curious.  The code looks very much like the igb code.  Wouldn't
> > it
> > be simpler and less error prone to add these new NICs to igb, rather
> > than creating a new driver by (mostly) copying the code from igb?
> > 
> > What's the idea behind this?
> 
> This new driver is for a new upcoming client part, not server (which is
> the igb driver).  Yes, the way that this new client part does resemble
> how the igb driver operates, but this new client part will not have
> many of the advance features (like SRIOV) that igb uses.  We also want
> to keep the igb driver strictly for the 1GbE server devices.

Ok, thanks, but I'm still a bit puzzled.

What about E1000_DEV_ID_I354_BACKPLANE_2_5GBPS?  The later supposedly
already provides 2.5G within igb.  Is that the server part compared
to igc?  Otherwise, what's the equivalent server part providing 2.5G?

> > > Sasha Neftin (v2):
> > > update module author, copyright and licence header
> > > cosmetic changes
> > > 
> > > Sasha Neftin (v3):
> > > remove unused header files
> > > 
> > > Sasha Neftin (v4):
> > > update brand name
> > > fix syntax by input from community
> > > replace e1000_ prefix with igc_ prefix
> > 
> > In theory this is neither necessary, nor does it actually help.
> > By keeping the e1000 prefixes it's much easier to compare to the
> > other, already existing drivers.
> > 
> > > Sasha Neftin (v5):
> > > no changes
> > > 
> > > Sasha Neftin (v6):
> > > no changes
> > 
> > Is that an oversight?  If there are no changes in v5 and v6, why
> > do we have them?!?
> 
> Sasha is just noting that this particular patch did not change in v6,
> but other patches did have changes in this new version of the series.

Oh, right, thanks for the clarification.


Corinna
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180823/8490f85d/attachment.asc>

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Jeff Kirsher]

On Thu, 2018-08-23 at 17:21 +0200, Corinna Vinschen wrote:
> On Aug 23 08:12, Jeff Kirsher wrote:
> > On Thu, 2018-08-23 at 16:03 +0200, Corinna Vinschen wrote:
> > > On Aug 23 10:05, Sasha Neftin wrote:
> > > > This patch adds the beginning framework onto which I am going
> to
> > > add
> > > > the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
> > > > Ethernet Controller.
> > > 
> > > I'm curious.  The code looks very much like the igb code. 
> Wouldn't
> > > it
> > > be simpler and less error prone to add these new NICs to igb,
> rather
> > > than creating a new driver by (mostly) copying the code from igb?
> > > 
> > > What's the idea behind this?
> > 
> > This new driver is for a new upcoming client part, not server
> (which is
> > the igb driver).  Yes, the way that this new client part does
> resemble
> > how the igb driver operates, but this new client part will not have
> > many of the advance features (like SRIOV) that igb uses.  We also
> want
> > to keep the igb driver strictly for the 1GbE server devices.
> 
> Ok, thanks, but I'm still a bit puzzled.
> 
> What about E1000_DEV_ID_I354_BACKPLANE_2_5GBPS?  The later supposedly
> already provides 2.5G within igb.  Is that the server part compared
> to igc?  Otherwise, what's the equivalent server part providing 2.5G?

Yes, one of the last server devices we developed back in 2013 was I354
and that device did support 2.5 GbE.  Again, this is a server part,
which supports more advanced features that client parts do not.

There is not a 1:1 on server parts and client parts, sometimes there
can be a server part that is similar to a client part or vice versa.

In this case, this new client part will be the next generation of
client parts.  So these will be essentially the new "e1000e" (our other
client driver), but since this next generation of client parts will
have MAC/PHY communications that are more similar to how some of our
server parts did, that is why portions of the driver will resemble igb.
But feature wise the igc driver will be more like e1000e than igb.

Hope that helps.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180823/9830e4dc/attachment-0001.asc>

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Corinna Vinschen]

On Aug 23 08:57, Jeff Kirsher wrote:
> On Thu, 2018-08-23 at 17:21 +0200, Corinna Vinschen wrote:
> > On Aug 23 08:12, Jeff Kirsher wrote:
> > > On Thu, 2018-08-23 at 16:03 +0200, Corinna Vinschen wrote:
> > > > On Aug 23 10:05, Sasha Neftin wrote:
> > > > > This patch adds the beginning framework onto which I am going
> > to
> > > > add
> > > > > the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
> > > > > Ethernet Controller.
> > > > 
> > > > I'm curious.  The code looks very much like the igb code. 
> > Wouldn't
> > > > it
> > > > be simpler and less error prone to add these new NICs to igb,
> > rather
> > > > than creating a new driver by (mostly) copying the code from igb?
> > > > 
> > > > What's the idea behind this?
> > > 
> > > This new driver is for a new upcoming client part, not server
> > (which is
> > > the igb driver).  Yes, the way that this new client part does
> > resemble
> > > how the igb driver operates, but this new client part will not have
> > > many of the advance features (like SRIOV) that igb uses.  We also
> > want
> > > to keep the igb driver strictly for the 1GbE server devices.
> > 
> > Ok, thanks, but I'm still a bit puzzled.
> > 
> > What about E1000_DEV_ID_I354_BACKPLANE_2_5GBPS?  The later supposedly
> > already provides 2.5G within igb.  Is that the server part compared
> > to igc?  Otherwise, what's the equivalent server part providing 2.5G?
> 
> Yes, one of the last server devices we developed back in 2013 was I354
> and that device did support 2.5 GbE.  Again, this is a server part,
> which supports more advanced features that client parts do not.
> 
> There is not a 1:1 on server parts and client parts, sometimes there
> can be a server part that is similar to a client part or vice versa.
> 
> In this case, this new client part will be the next generation of
> client parts.  So these will be essentially the new "e1000e" (our other
> client driver), but since this next generation of client parts will
> have MAC/PHY communications that are more similar to how some of our
> server parts did, that is why portions of the driver will resemble igb.
> But feature wise the igc driver will be more like e1000e than igb.
> 
> Hope that helps.

Yes, Jeff, this really helps.

But this just reinforces my doubts that changing the E1000 prefix
to IGC is a good thing.  The comparison of binary values only gets
more complicated.  In the end we have, for instance, this:

  e1000:  #define E1000_ERR_NVM        1
  e1000e: #define E1000_ERR_NVM        1
  igb:    #define E1000_ERR_NVM        1

but

  igc:    #define IGC_ERR_NVM          1

This doesn't make sense.


Thanks,
Corinna
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180823/f2087ca4/attachment.asc>

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Shannon Nelson]

On 8/23/2018 12:05 AM, Sasha Neftin wrote:
> This patch adds the beginning framework onto which I am going to add
> the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
> Ethernet Controller.
> 
> Sasha Neftin (v2):
> update module author, copyright and licence header
> cosmetic changes
> 
> Sasha Neftin (v3):
> remove unused header files
> 
> Sasha Neftin (v4):
> update brand name
> fix syntax by input from community
> replace e1000_ prefix with igc_ prefix
> 
> Sasha Neftin (v5):
> no changes
> 
> Sasha Neftin (v6):
> no changes

In all these patches, this version stuff will be removed from the commit 
message before pushing upstream, right?

Normally they appear after the "---" so as to not be a part of the 
formal commit message saved in git.

[...]

> diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
> new file mode 100644
> index 000000000000..afe595cfcf63
> --- /dev/null
> +++ b/drivers/net/ethernet/intel/igc/igc.h
> @@ -0,0 +1,29 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

[...]

> +MODULE_LICENSE("GPL");

Since you're using the SPDX "GPL-2.0", you might want to use the 
MODULE_LICENSE string "GPL v2" to match. since "GPL" means v2 or later. 
See include/linux/module.h for details.

By the way, this all looks much nicer with the "igc_" prefix - thanks.

sln

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Jeff Kirsher]

On Thu, 2018-08-23 at 18:21 +0200, Corinna Vinschen wrote:
> On Aug 23 08:57, Jeff Kirsher wrote:
> > On Thu, 2018-08-23 at 17:21 +0200, Corinna Vinschen wrote:
> > > On Aug 23 08:12, Jeff Kirsher wrote:
> > > > On Thu, 2018-08-23 at 16:03 +0200, Corinna Vinschen wrote:
> > > > > On Aug 23 10:05, Sasha Neftin wrote:
> > > > > > This patch adds the beginning framework onto which I am
> going
> > > to
> > > > > add
> > > > > > the igc driver which supports the Intel(R) I225-LM/I225-V
> 2.5G
> > > > > > Ethernet Controller.
> > > > > 
> > > > > I'm curious.  The code looks very much like the igb code. 
> > > Wouldn't
> > > > > it
> > > > > be simpler and less error prone to add these new NICs to igb,
> > > rather
> > > > > than creating a new driver by (mostly) copying the code from
> igb?
> > > > > 
> > > > > What's the idea behind this?
> > > > 
> > > > This new driver is for a new upcoming client part, not server
> > > (which is
> > > > the igb driver).  Yes, the way that this new client part does
> > > resemble
> > > > how the igb driver operates, but this new client part will not
> have
> > > > many of the advance features (like SRIOV) that igb uses.  We
> also
> > > want
> > > > to keep the igb driver strictly for the 1GbE server devices.
> > > 
> > > Ok, thanks, but I'm still a bit puzzled.
> > > 
> > > What about E1000_DEV_ID_I354_BACKPLANE_2_5GBPS?  The later
> supposedly
> > > already provides 2.5G within igb.  Is that the server part
> compared
> > > to igc?  Otherwise, what's the equivalent server part providing
> 2.5G?
> > 
> > Yes, one of the last server devices we developed back in 2013 was
> I354
> > and that device did support 2.5 GbE.  Again, this is a server part,
> > which supports more advanced features that client parts do not.
> > 
> > There is not a 1:1 on server parts and client parts, sometimes
> there
> > can be a server part that is similar to a client part or vice
> versa.
> > 
> > In this case, this new client part will be the next generation of
> > client parts.  So these will be essentially the new "e1000e" (our
> other
> > client driver), but since this next generation of client parts will
> > have MAC/PHY communications that are more similar to how some of
> our
> > server parts did, that is why portions of the driver will resemble
> igb.
> > But feature wise the igc driver will be more like e1000e than igb.
> > 
> > Hope that helps.
> 
> Yes, Jeff, this really helps.
> 
> But this just reinforces my doubts that changing the E1000 prefix
> to IGC is a good thing.  The comparison of binary values only gets
> more complicated.  In the end we have, for instance, this:
> 
>   e1000:  #define E1000_ERR_NVM        1
>   e1000e: #define E1000_ERR_NVM        1
>   igb:    #define E1000_ERR_NVM        1
> 
> but
> 
>   igc:    #define IGC_ERR_NVM          1
> 
> This doesn't make sense.

When naming functions/macros and defines, I can see pros and cons to
both approaches.  That particular code you referring to is "common
code" shared amongst the various drivers e1000, e1000e and igb.  The
igc driver did start with this "common code" as a starting point, but
this common code will be morphing to handle the future client parts. 
So we were looking to use the igc driver as the parent of this new
common code, much like how e1000 was the "original" or parent driver to
e1000e and igb.

It seemed right to break ties with the legacy naming of
functions/macros/defines, especially since this new generation of parts
and devices will have very little in common with the old e1000 devices.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180823/6887dbd2/attachment.asc>

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Jeff Kirsher]

On Thu, 2018-08-23 at 09:37 -0700, Shannon Nelson wrote:
> On 8/23/2018 12:05 AM, Sasha Neftin wrote:
> > This patch adds the beginning framework onto which I am going to
> > add
> > the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
> > Ethernet Controller.
> > 
> > Sasha Neftin (v2):
> > update module author, copyright and licence header
> > cosmetic changes
> > 
> > Sasha Neftin (v3):
> > remove unused header files
> > 
> > Sasha Neftin (v4):
> > update brand name
> > fix syntax by input from community
> > replace e1000_ prefix with igc_ prefix
> > 
> > Sasha Neftin (v5):
> > no changes
> > 
> > Sasha Neftin (v6):
> > no changes
> 
> In all these patches, this version stuff will be removed from the
> commit 
> message before pushing upstream, right?
> 
> Normally they appear after the "---" so as to not be a part of the 
> formal commit message saved in git.

Yes, when this eventually gets pushed upstream, the changelog will be
stripped.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20180823/2dd489bb/attachment.asc>

[Intel-wired-lan] [PATCH v6 01/11] igc: Add skeletal frame for Intel(R) 2.5G Ethernet Controller support.

[Neftin, Sasha]

On 8/23/2018 19:37, Shannon Nelson wrote:
> On 8/23/2018 12:05 AM, Sasha Neftin wrote:
>> This patch adds the beginning framework onto which I am going to add
>> the igc driver which supports the Intel(R) I225-LM/I225-V 2.5G
>> Ethernet Controller.
>>
>> Sasha Neftin (v2):
>> update module author, copyright and licence header
>> cosmetic changes
>>
>> Sasha Neftin (v3):
>> remove unused header files
>>
>> Sasha Neftin (v4):
>> update brand name
>> fix syntax by input from community
>> replace e1000_ prefix with igc_ prefix
>>
>> Sasha Neftin (v5):
>> no changes
>>
>> Sasha Neftin (v6):
>> no changes
> 
> In all these patches, this version stuff will be removed from the commit 
> message before pushing upstream, right?
> 
> Normally they appear after the "---" so as to not be a part of the 
> formal commit message saved in git.
> 
> [...]
> 
>> diff --git a/drivers/net/ethernet/intel/igc/igc.h 
>> b/drivers/net/ethernet/intel/igc/igc.h
>> new file mode 100644
>> index 000000000000..afe595cfcf63
>> --- /dev/null
>> +++ b/drivers/net/ethernet/intel/igc/igc.h
>> @@ -0,0 +1,29 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
> 
> [...]
> 
>> +MODULE_LICENSE("GPL");
> 
> Since you're using the SPDX "GPL-2.0", you might want to use the 
> MODULE_LICENSE string "GPL v2" to match. since "GPL" means v2 or later. 
> See include/linux/module.h for details.
> 
> By the way, this all looks much nicer with the "igc_" prefix - thanks.
> 
> sln
> Our new drivers, ice, i40e, fm10k used same convention for prefix 
naming. We decided keep igc_ prefix.
Thanks,
Sasha