[PATCH v2 0/4] net: stmmac: Enable Intel Quark SoC X1000 Ethernet support

[PATCH v2 0/4] net: stmmac: Enable Intel Quark SoC X1000 Ethernet support

[Kweh Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

Hi,

Intel Quark X1000 SoC has 2 Ethernet controllers integrated on chip and they are
PCI devices. We adopted the stmmac_pci driver and added on code to support Intel
Quark SoC X1000 by creating the patchset below. The patchset has been built and
tested on Galileo board and found to be working as expected.

We believe that the changes are transparent to other non Intel Quark platform. 
Please help to review the code change and feedback if there is any concern.

Thank you very much.

---
changelog v2:
[PATCH 1/4]
* fix indentation to align with the 1st column of openning parenthesis
* replace the static variable instance_id by using pci bus & devfn number
[PATCH 2/4]
* replace the static variable instance_id by using pci bus & devfn number
[PATCH 3/4]
* replace the static variable instance_id by using pci bus & devfn number

Kweh, Hock Leong (4):
  net: stmmac: enhance to support multiple device instances
  net: stmmac: better code manageability with platform data struct
  net: stmmac: add support for Intel Quark X1000
  net: stmmac: add MSI support for Intel Quark X1000

 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |  194 +++++++++++++++++++---
 1 file changed, 171 insertions(+), 23 deletions(-)

-- 
1.7.9.5

[PATCH v2 1/4] net: stmmac: enhance to support multiple device instances

[Kweh Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

The original stmmac_pci code only supports single ethernet controller
instance. This modification allows the driver to support multiple
stmicro ethernet controller instances by converting the static global
variables plat_dat, mdio_data & dma_cfg to dynamic allocation.

This piece of work is derived from Bryan O'Donoghue's initial work for
Quark X1000 enabling.

Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
Reviewed-by: Ong, Boon Leong <boon.leong.ong@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |   61 ++++++++++++++--------
 1 file changed, 39 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 655a23b..4d9a5c2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -26,27 +26,21 @@
 #include <linux/pci.h>
 #include "stmmac.h"
 
-static struct plat_stmmacenet_data plat_dat;
-static struct stmmac_mdio_bus_data mdio_data;
-static struct stmmac_dma_cfg dma_cfg;
-
-static void stmmac_default_data(void)
+static void stmmac_default_data(struct plat_stmmacenet_data *plat_dat,
+				struct pci_dev *pdev)
 {
-	memset(&plat_dat, 0, sizeof(struct plat_stmmacenet_data));
-	plat_dat.bus_id = 1;
-	plat_dat.phy_addr = 0;
-	plat_dat.interface = PHY_INTERFACE_MODE_GMII;
-	plat_dat.clk_csr = 2;	/* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
-	plat_dat.has_gmac = 1;
-	plat_dat.force_sf_dma_mode = 1;
-
-	mdio_data.phy_reset = NULL;
-	mdio_data.phy_mask = 0;
-	plat_dat.mdio_bus_data = &mdio_data;
-
-	dma_cfg.pbl = 32;
-	dma_cfg.burst_len = DMA_AXI_BLEN_256;
-	plat_dat.dma_cfg = &dma_cfg;
+	plat_dat->bus_id = PCI_DEVID(pdev->bus->number, pdev->devfn);
+	plat_dat->phy_addr = 0;
+	plat_dat->interface = PHY_INTERFACE_MODE_GMII;
+	plat_dat->clk_csr = 2;	/* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
+	plat_dat->has_gmac = 1;
+	plat_dat->force_sf_dma_mode = 1;
+
+	plat_dat->mdio_bus_data->phy_reset = NULL;
+	plat_dat->mdio_bus_data->phy_mask = 0;
+
+	plat_dat->dma_cfg->pbl = 32;
+	plat_dat->dma_cfg->burst_len = DMA_AXI_BLEN_256;
 }
 
 /**
@@ -67,6 +61,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 	int ret = 0;
 	void __iomem *addr = NULL;
 	struct stmmac_priv *priv = NULL;
+	struct plat_stmmacenet_data *plat_dat;
 	int i;
 
 	/* Enable pci device */
@@ -97,9 +92,31 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 	}
 	pci_set_master(pdev);
 
-	stmmac_default_data();
+	plat_dat = devm_kzalloc(&pdev->dev, sizeof(*plat_dat), GFP_KERNEL);
+	if (!plat_dat) {
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	plat_dat->mdio_bus_data = devm_kzalloc(&pdev->dev,
+					       sizeof(*plat_dat->mdio_bus_data),
+					       GFP_KERNEL);
+	if (!plat_dat->mdio_bus_data) {
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	plat_dat->dma_cfg = devm_kzalloc(&pdev->dev,
+					 sizeof(*plat_dat->dma_cfg),
+					 GFP_KERNEL);
+	if (!plat_dat->dma_cfg) {
+		ret = -ENOMEM;
+		goto err_out;
+	}
+
+	stmmac_default_data(plat_dat, pdev);
 
-	priv = stmmac_dvr_probe(&(pdev->dev), &plat_dat, addr);
+	priv = stmmac_dvr_probe(&pdev->dev, plat_dat, addr);
 	if (IS_ERR(priv)) {
 		pr_err("%s: main driver probe failed", __func__);
 		ret = PTR_ERR(priv);
-- 
1.7.9.5

[PATCH v2 2/4] net: stmmac: better code manageability with platform data struct

[Kweh Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

Introduce additional struct to hold platform info for pci device. It
is used to store features that are supported by specific chip vendor.
This code change helps to expand further to support more platform
vendors and this implementation promotes better code manageability
and keep code base clean. In addition, this patch adds mcast & ucast
filter configuration in pci driver.

Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
Reviewed-by: Ong, Boon Leong <boon.leong.ong@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |   77 +++++++++++++++++-----
 1 file changed, 60 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 4d9a5c2..35fc884 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -26,21 +26,63 @@
 #include <linux/pci.h>
 #include "stmmac.h"
 
-static void stmmac_default_data(struct plat_stmmacenet_data *plat_dat,
-				struct pci_dev *pdev)
+enum chip {
+	CHIP_STMICRO = 0,
+};
+
+/* A struct for platform specific information which will be
+ * used in stmmac_default_data function for initialization
+ */
+struct platform_data {
+	int phy_addr;
+	int interface;
+	int clk_csr;
+	int has_gmac;
+	int force_sf_dma_mode;
+	int multicast_filter_bins;
+	int unicast_filter_entries;
+	int (*phy_reset)(void *priv);
+	unsigned int phy_mask;
+	int pbl;
+	int burst_len;
+};
+
+static struct platform_data platform_info[] = {
+	[CHIP_STMICRO] = {
+		.phy_addr = 0,
+		.interface = PHY_INTERFACE_MODE_GMII,
+		.clk_csr = 2,
+		.has_gmac = 1,
+		.force_sf_dma_mode = 1,
+		.multicast_filter_bins = HASH_TABLE_SIZE,
+		.unicast_filter_entries = 1,
+		.phy_reset = NULL,
+		.phy_mask = 0,
+		.pbl = 32,
+		.burst_len = DMA_AXI_BLEN_256,
+	},
+};
+
+static void stmmac_default_data(struct plat_stmmacenet_data *plat,
+				int chip_id, struct pci_dev *pdev)
 {
-	plat_dat->bus_id = PCI_DEVID(pdev->bus->number, pdev->devfn);
-	plat_dat->phy_addr = 0;
-	plat_dat->interface = PHY_INTERFACE_MODE_GMII;
-	plat_dat->clk_csr = 2;	/* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
-	plat_dat->has_gmac = 1;
-	plat_dat->force_sf_dma_mode = 1;
-
-	plat_dat->mdio_bus_data->phy_reset = NULL;
-	plat_dat->mdio_bus_data->phy_mask = 0;
-
-	plat_dat->dma_cfg->pbl = 32;
-	plat_dat->dma_cfg->burst_len = DMA_AXI_BLEN_256;
+	struct platform_data *chip_plat_dat = &platform_info[chip_id];
+
+	plat->bus_id = PCI_DEVID(pdev->bus->number, pdev->devfn);
+	plat->phy_addr = chip_plat_dat->phy_addr;
+	plat->interface = chip_plat_dat->interface;
+	/* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
+	plat->clk_csr = chip_plat_dat->clk_csr;
+	plat->has_gmac = chip_plat_dat->has_gmac;
+	plat->force_sf_dma_mode = chip_plat_dat->force_sf_dma_mode;
+	plat->multicast_filter_bins = chip_plat_dat->multicast_filter_bins;
+	plat->unicast_filter_entries = chip_plat_dat->unicast_filter_entries;
+
+	plat->mdio_bus_data->phy_reset = chip_plat_dat->phy_reset;
+	plat->mdio_bus_data->phy_mask = chip_plat_dat->phy_mask;
+
+	plat->dma_cfg->pbl = chip_plat_dat->pbl;
+	plat->dma_cfg->burst_len = chip_plat_dat->burst_len;
 }
 
 /**
@@ -114,7 +156,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 		goto err_out;
 	}
 
-	stmmac_default_data(plat_dat, pdev);
+	stmmac_default_data(plat_dat, id->driver_data, pdev);
 
 	priv = stmmac_dvr_probe(&pdev->dev, plat_dat, addr);
 	if (IS_ERR(priv)) {
@@ -188,8 +230,9 @@ static int stmmac_pci_resume(struct pci_dev *pdev)
 #define STMMAC_DEVICE_ID 0x1108
 
 static const struct pci_device_id stmmac_id_table[] = {
-	{PCI_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID)},
-	{PCI_DEVICE(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_MAC)},
+	{PCI_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID), PCI_ANY_ID,
+			PCI_ANY_ID, CHIP_STMICRO},
+	{PCI_VDEVICE(STMICRO, PCI_DEVICE_ID_STMICRO_MAC), CHIP_STMICRO},
 	{}
 };
 
-- 
1.7.9.5

[PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

[Kweh Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

The Intel Quark SoC X1000 provides two 10/100 Mbps Ethernet MAC
controllers which may or may not be connected to PHY on board.
This MAC controller only supports RMII PHY.

Besides adding Quark PCI ID to this driver, this patch introduces
run-time board detection through DMI and MAC-PHY configuration
function used by stmmac_default_data() during initialization.
It fills up the phy_address to -1 for Galileo and Galileo Gen2
boards to indicate that the 2nd Ethernet MAC controller is not
connected to any PHY.

The implementation takes into consideration for future expansion in
Quark series boards that may have different PHY address that is
linked to its MAC controllers.

This piece of work is derived from Bryan O'Donoghue's initial work for
Quark X1000 enabling.

Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
Reviewed-by: Ong, Boon Leong <boon.leong.ong@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |   87 ++++++++++++++++++++--
 1 file changed, 82 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 35fc884..4fae23f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -24,14 +24,18 @@
 *******************************************************************************/
 
 #include <linux/pci.h>
+#include <linux/dmi.h>
 #include "stmmac.h"
 
+static void quark_run_time_config(int chip_id, struct pci_dev *pdev);
+
 enum chip {
 	CHIP_STMICRO = 0,
+	CHIP_QUARK_X1000,
 };
 
-/* A struct for platform specific information which will be
- * used in stmmac_default_data function for initialization
+/* A struct for platform specific information which is used
+ * in stmmac_default_data function for initialization
  */
 struct platform_data {
 	int phy_addr;
@@ -44,7 +48,9 @@ struct platform_data {
 	int (*phy_reset)(void *priv);
 	unsigned int phy_mask;
 	int pbl;
+	int fixed_burst;
 	int burst_len;
+	void (*rt_config)(int chip_id, struct pci_dev *pdev);
 };
 
 static struct platform_data platform_info[] = {
@@ -59,15 +65,76 @@ static struct platform_data platform_info[] = {
 		.phy_reset = NULL,
 		.phy_mask = 0,
 		.pbl = 32,
+		.fixed_burst = 0,
 		.burst_len = DMA_AXI_BLEN_256,
+		.rt_config = NULL,
+	},
+	[CHIP_QUARK_X1000] = {
+		.phy_addr = 1,
+		.interface = PHY_INTERFACE_MODE_RMII,
+		.clk_csr = 2,
+		.has_gmac = 1,
+		.force_sf_dma_mode = 1,
+		.multicast_filter_bins = HASH_TABLE_SIZE,
+		.unicast_filter_entries = 1,
+		.phy_reset = NULL,
+		.phy_mask = 0,
+		.pbl = 16,
+		.fixed_burst = 1,
+		.burst_len = DMA_AXI_BLEN_256,
+		.rt_config = &quark_run_time_config,
+	},
+};
+
+/* This struct is used to associate PCI Function ID of MAC controller
+ * on a board, discovered via DMI, with phy_address. It is also used
+ * to describe if that MAC controller is connected with PHY.
+ */
+struct intel_quark_platform {
+	int pci_func_num;
+	const char *board_name;
+	int phy_address;
+};
+
+static struct intel_quark_platform quark_x1000_phy_info[] = {
+	{
+		.pci_func_num = 7,
+		.board_name = "Galileo",
+		/* Galileo ethernet port 2 does not connect to any PHY */
+		.phy_address = -1,
+	},
+	{
+		.pci_func_num = 7,
+		.board_name = "GalileoGen2",
+		/* Galileo Gen2 ethernet port 2 does not connect to any PHY */
+		.phy_address = -1,
 	},
 };
 
-static void stmmac_default_data(struct plat_stmmacenet_data *plat,
-				int chip_id, struct pci_dev *pdev)
+static void quark_run_time_config(int chip_id, struct pci_dev *pdev)
+{
+	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	int i;
+	int func_num = PCI_FUNC(pdev->devfn);
+
+	if (!board_name)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(quark_x1000_phy_info); i++) {
+		if ((!strcmp(quark_x1000_phy_info[i].board_name, board_name)) &&
+		    quark_x1000_phy_info[i].pci_func_num == func_num)
+			platform_info[chip_id].phy_addr =
+				quark_x1000_phy_info[i].phy_address;
+	}
+}
+
+static int stmmac_default_data(struct plat_stmmacenet_data *plat,
+			       int chip_id, struct pci_dev *pdev)
 {
 	struct platform_data *chip_plat_dat = &platform_info[chip_id];
 
+	if (chip_plat_dat->rt_config)
+		chip_plat_dat->rt_config(chip_id, pdev);
 	plat->bus_id = PCI_DEVID(pdev->bus->number, pdev->devfn);
 	plat->phy_addr = chip_plat_dat->phy_addr;
 	plat->interface = chip_plat_dat->interface;
@@ -82,7 +149,13 @@ static void stmmac_default_data(struct plat_stmmacenet_data *plat,
 	plat->mdio_bus_data->phy_mask = chip_plat_dat->phy_mask;
 
 	plat->dma_cfg->pbl = chip_plat_dat->pbl;
+	plat->dma_cfg->fixed_burst = chip_plat_dat->fixed_burst;
 	plat->dma_cfg->burst_len = chip_plat_dat->burst_len;
+
+	/* Refuse to load the driver and register net device
+	 * if MAC controller does not connect to any PHY interface
+	 */
+	return (plat->phy_addr != -1) ? 0 : -ENODEV;
 }
 
 /**
@@ -156,7 +229,9 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 		goto err_out;
 	}
 
-	stmmac_default_data(plat_dat, id->driver_data, pdev);
+	ret = stmmac_default_data(plat_dat, id->driver_data, pdev);
+	if (ret)
+		goto err_out;
 
 	priv = stmmac_dvr_probe(&pdev->dev, plat_dat, addr);
 	if (IS_ERR(priv)) {
@@ -228,11 +303,13 @@ static int stmmac_pci_resume(struct pci_dev *pdev)
 
 #define STMMAC_VENDOR_ID 0x700
 #define STMMAC_DEVICE_ID 0x1108
+#define STMMAC_QUARK_X1000_ID 0x0937
 
 static const struct pci_device_id stmmac_id_table[] = {
 	{PCI_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID), PCI_ANY_ID,
 			PCI_ANY_ID, CHIP_STMICRO},
 	{PCI_VDEVICE(STMICRO, PCI_DEVICE_ID_STMICRO_MAC), CHIP_STMICRO},
+	{PCI_VDEVICE(INTEL, STMMAC_QUARK_X1000_ID), CHIP_QUARK_X1000},
 	{}
 };
 
-- 
1.7.9.5

[PATCH v2 4/4] net: stmmac: add MSI support for Intel Quark X1000

[Kweh Hock Leong]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>

In Intel Quark SoC X1000, both of the Ethernet controllers support
MSI interrupt handling. This patch enables them to use MSI interrupt
servicing in stmmac_pci for Intel Quark X1000.

Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
Reviewed-by: Ong, Boon Leong <boon.leong.ong@intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |   19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 4fae23f..02861c3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -51,6 +51,7 @@ struct platform_data {
 	int fixed_burst;
 	int burst_len;
 	void (*rt_config)(int chip_id, struct pci_dev *pdev);
+	int support_msi;
 };
 
 static struct platform_data platform_info[] = {
@@ -68,6 +69,7 @@ static struct platform_data platform_info[] = {
 		.fixed_burst = 0,
 		.burst_len = DMA_AXI_BLEN_256,
 		.rt_config = NULL,
+		.support_msi = 0,
 	},
 	[CHIP_QUARK_X1000] = {
 		.phy_addr = 1,
@@ -83,6 +85,7 @@ static struct platform_data platform_info[] = {
 		.fixed_burst = 1,
 		.burst_len = DMA_AXI_BLEN_256,
 		.rt_config = &quark_run_time_config,
+		.support_msi = 1,
 	},
 };
 
@@ -210,7 +213,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 	plat_dat = devm_kzalloc(&pdev->dev, sizeof(*plat_dat), GFP_KERNEL);
 	if (!plat_dat) {
 		ret = -ENOMEM;
-		goto err_out;
+		goto err_out_alloc_failed;
 	}
 
 	plat_dat->mdio_bus_data = devm_kzalloc(&pdev->dev,
@@ -218,7 +221,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 					       GFP_KERNEL);
 	if (!plat_dat->mdio_bus_data) {
 		ret = -ENOMEM;
-		goto err_out;
+		goto err_out_alloc_failed;
 	}
 
 	plat_dat->dma_cfg = devm_kzalloc(&pdev->dev,
@@ -226,12 +229,15 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 					 GFP_KERNEL);
 	if (!plat_dat->dma_cfg) {
 		ret = -ENOMEM;
-		goto err_out;
+		goto err_out_alloc_failed;
 	}
 
 	ret = stmmac_default_data(plat_dat, id->driver_data, pdev);
 	if (ret)
-		goto err_out;
+		goto err_out_alloc_failed;
+
+	if (platform_info[id->driver_data].support_msi)
+		pci_enable_msi(pdev);
 
 	priv = stmmac_dvr_probe(&pdev->dev, plat_dat, addr);
 	if (IS_ERR(priv)) {
@@ -249,6 +255,9 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 	return 0;
 
 err_out:
+	if (platform_info[id->driver_data].support_msi)
+		pci_disable_msi(pdev);
+err_out_alloc_failed:
 	pci_clear_master(pdev);
 err_out_map_failed:
 	pci_release_regions(pdev);
@@ -272,6 +281,8 @@ static void stmmac_pci_remove(struct pci_dev *pdev)
 
 	stmmac_dvr_remove(ndev);
 
+	if (pci_dev_msi_enabled(pdev))
+		pci_disable_msi(pdev);
 	pci_iounmap(pdev, priv->ioaddr);
 	pci_release_regions(pdev);
 	pci_disable_device(pdev);
-- 
1.7.9.5

Re: [PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

[David Miller]

From: Kweh Hock Leong <hock.leong.kweh@intel.com>
Date: Thu, 11 Sep 2014 16:38:39 +0800

> +		if ((!strcmp(quark_x1000_phy_info[i].board_name, board_name)) &&
> +		    quark_x1000_phy_info[i].pci_func_num == func_num)

It is entirely erroneous to identify a device by it's _PHYSICAL_ geographic
location on the PCI bus.

Please get rid of this PCI function number comparison and if necessary
find another means of identification.

RE: [PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

[Kweh, Hock Leong]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Saturday, September 13, 2014 6:14 AM
> From: Kweh Hock Leong <hock.leong.kweh@intel.com>
> Date: Thu, 11 Sep 2014 16:38:39 +0800
> 
> > +		if ((!strcmp(quark_x1000_phy_info[i].board_name,
> board_name)) &&
> > +		    quark_x1000_phy_info[i].pci_func_num == func_num)
> 
> It is entirely erroneous to identify a device by it's _PHYSICAL_ geographic
> location on the PCI bus.
> 
> Please get rid of this PCI function number comparison and if necessary find
> another means of identification.

Hi David,

Here is some background of this work. Intel Quark X1000 has 2 stmmac Ethernet IP
built in the SoC. They both are using the same PCI DEVICE ID number. The only things 
to differentiate them is PCI BUS DEVICE FUNCTION (Bus:Dev:Func) number which are
fix numbers 00:20:6 for port 1 and 00:20:7 for port 2 stated in Quark X1000 datasheet.
(https://communities.intel.com/docs/DOC-23092 page 44 & 97)

When I was looking into making the code to upstream, I do think about is there a better
identification way to do it? But, my mind still brought me back to this PCI FUNC number.

So, i would like to understand the concern of using PCI FUNC number and also would like
to see is there any advices, suggestion or pointer to deal with the scenario here.

Appreciate to the comments sharing. Thanks.


Regards,
Wilson

Re: [PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

[David Miller]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
Date: Mon, 15 Sep 2014 12:42:03 +0000

> The only things to differentiate them is PCI BUS DEVICE FUNCTION
> (Bus:Dev:Func) number which are fix numbers 00:20:6 for port 1 and
> 00:20:7 for port 2 stated in Quark X1000 datasheet.

Match on the PCI device class, which must be PCI_CLASS_NETWORK_ETHERNET
or similar.

The pci_device_id used for probing supports matching on this directly.
.

RE: [PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

[Kweh, Hock Leong]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, September 17, 2014 3:01 AM
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> Date: Mon, 15 Sep 2014 12:42:03 +0000
> 
> > The only things to differentiate them is PCI BUS DEVICE FUNCTION
> > (Bus:Dev:Func) number which are fix numbers 00:20:6 for port 1 and
> > 00:20:7 for port 2 stated in Quark X1000 datasheet.
> 
> Match on the PCI device class, which must be
> PCI_CLASS_NETWORK_ETHERNET or similar.
> 
> The pci_device_id used for probing supports matching on this directly.
> .

Hi David,

Thanks for the pointer. I did a quickly checking on the class number to see if
I could use it for differentiation the ports number. Whereas I found them 
both have the same class number as well. Below shows the "lspci" dump to 
all the PCI devices on Quark X1000 Galileo board (Ethernet controllers are
00:14.6 and 00:14.7). Very unfortunately we are unlikely to use the class 
number as well as pci_device_id for the differentiation.
Thanks.

root@quark:~# lspci -m
00:00.0 "Class 0600" "8086" "0958" "8086" "095e"
00:14.0 "Class 0805" "8086" "08a7" "8086" "08a7"
00:14.1 "Class 0700" "8086" "0936" "8086" "0936"
00:14.2 "Class 0c03" "8086" "0939" "8086" "0939"
00:14.3 "Class 0c03" "8086" "0939" "8086" "0939"
00:14.4 "Class 0c03" "8086" "093a" "8086" "093a"
00:14.5 "Class 0700" "8086" "0936" "8086" "0936"
00:14.6 "Class 0200" "8086" "0937" "8086" "0937"
00:14.7 "Class 0200" "8086" "0937" "8086" "0937"
00:15.0 "Class 0c80" "8086" "0935" "8086" "0935"
00:15.1 "Class 0c80" "8086" "0935" "8086" "0935"
00:15.2 "Class 0c80" "8086" "0934" "8086" "0934"
00:17.0 "Class 0604" "8086" "11c3" "8086" "11c3"
00:17.1 "Class 0604" "8086" "11c4" "8086" "11c4"
00:1f.0 "Class 0601" "8086" "095e" "8086" "095e"
root@quark:~# lspci -k
00:00.0 Class 0600: 8086:0958 iosf_mbi_pci
00:14.0 Class 0805: 8086:08a7 sdhci-pci
00:14.1 Class 0700: 8086:0936 serial
00:14.2 Class 0c03: 8086:0939
00:14.3 Class 0c03: 8086:0939 ehci-pci
00:14.4 Class 0c03: 8086:093a ohci-pci
00:14.5 Class 0700: 8086:0936 serial
00:14.6 Class 0200: 8086:0937 stmmaceth
00:14.7 Class 0200: 8086:0937
00:15.0 Class 0c80: 8086:0935
00:15.1 Class 0c80: 8086:0935
00:15.2 Class 0c80: 8086:0934
00:17.0 Class 0604: 8086:11c3 pcieport
00:17.1 Class 0604: 8086:11c4 pcieport
00:1f.0 Class 0601: 8086:095e
root@quark:~#


Regards,
Wilson

Re: [PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

[David Miller]

From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
Date: Wed, 17 Sep 2014 02:41:39 +0000

> Thanks for the pointer. I did a quickly checking on the class number to see if
> I could use it for differentiation the ports number. Whereas I found them 
> both have the same class number as well. Below shows the "lspci" dump to 
> all the PCI devices on Quark X1000 Galileo board (Ethernet controllers are
> 00:14.6 and 00:14.7). Very unfortunately we are unlikely to use the class 
> number as well as pci_device_id for the differentiation.
 ...
> 00:14.6 "Class 0200" "8086" "0937" "8086" "0937"
> 00:14.7 "Class 0200" "8086" "0937" "8086" "0937"

Are you kidding me?  It's a perfect way to identify this device, it
properly uses PCI_CLASS_NETWORK_ETHERNET (0x0200) in both cases and
this will not match any other function on this PCI device at all.

Please do as I suggested and use the PCI class for the differentiation
and matching during probing.

RE: [PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

[Kweh, Hock Leong]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, September 17, 2014 12:56 PM
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
> Date: Wed, 17 Sep 2014 02:41:39 +0000
> 
> > Thanks for the pointer. I did a quickly checking on the class number
> > to see if I could use it for differentiation the ports number. Whereas
> > I found them both have the same class number as well. Below shows the
> > "lspci" dump to all the PCI devices on Quark X1000 Galileo board
> > (Ethernet controllers are
> > 00:14.6 and 00:14.7). Very unfortunately we are unlikely to use the
> > class number as well as pci_device_id for the differentiation.
>  ...
> > 00:14.6 "Class 0200" "8086" "0937" "8086" "0937"
> > 00:14.7 "Class 0200" "8086" "0937" "8086" "0937"
> 
> Are you kidding me?  It's a perfect way to identify this device, it properly uses
> PCI_CLASS_NETWORK_ETHERNET (0x0200) in both cases and this will not
> match any other function on this PCI device at all.
> 
> Please do as I suggested and use the PCI class for the differentiation and
> matching during probing.

Hi David,

Thanks for your feedback so far. Appreciate it.

Off the list because I think that my poorly written description may have caused some confusion. My sincere apology here. 
Unfortunately, I don't really grasp your idea clearly based on your responses which I appreciate them a lot. 
Sorry for the long description below but I hope to clearly pen down my thinking process so that you can follow my thinking incrementally without being confused.

So, let's roll back a bit so that with my following description, you can help correct me if my understanding of using PCI function ID to differentiate PHY port that is associated with each Ethernet controller is wrong:

The high-level idea about the change that I made for STMMAC IP inside Quark is as follow:

(1) Based on Quark-specific PCI ID declared inside stmmac_id_table[],  the probe() function is
       called to continue setting-up STMMAC for Quark.

@@ -228,11 +303,13 @@ static int stmmac_pci_resume(struct pci_dev *pdev)
 
 #define STMMAC_VENDOR_ID 0x700
 #define STMMAC_DEVICE_ID 0x1108
+#define STMMAC_QUARK_X1000_ID 0x0937
 
 static const struct pci_device_id stmmac_id_table[] = {
 	{PCI_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID), PCI_ANY_ID,
 			PCI_ANY_ID, CHIP_STMICRO},
 	{PCI_VDEVICE(STMICRO, PCI_DEVICE_ID_STMICRO_MAC), CHIP_STMICRO},
+	{PCI_VDEVICE(INTEL, STMMAC_QUARK_X1000_ID), CHIP_QUARK_X1000},
 	{}
 };

(2) Back-ground on STMMAC hardware configuration on Intel Galileo Gen 1 & Gen 2 platforms:
Intel Quark SoC has 2 MAC controller as described by lspci output below:

00:14.6 Class 0200: 8086:0937    ====> 1st MAC controller
00:14.7 Class 0200: 8086:0937    ====> 2nd MAC controller

These Galileo boards use the same Intel Quark SoC and there is only one PHY connect to the 1st MAC [00:14.6 Class 0200: 8086:0937] The 2nd MAC [00:14.7 Class 0200: 8086:0937] is NOT connected to any PHY at all.

So, it appears to me that the only way that I can differentiate between 1st & 2nd MAC are based on PCI function ID, i.e. 14.6 & 14.7. Therefore, within the probe() function, for Intel Quark SoC only, the function performs next-level discovery of 1st or 2nd MAC controller through quark_run_time_config() function. 
For other PCI ID (currently STMICRO_MAC) there is NO next-level discovery involved as rt_config is NULL.
Changes shown below:

static struct platform_data platform_info[] = { @@ -59,15 +65,76 @@ static struct platform_data platform_info[] = {
 		.phy_reset = NULL,
 		.phy_mask = 0,
 		.pbl = 32,
+		.fixed_burst = 0,
 		.burst_len = DMA_AXI_BLEN_256,
+		.rt_config = NULL,  ===================> no 2nd-level discovery for other PCI ID 
+	},
+	[CHIP_QUARK_X1000] = {
+		.phy_addr = 1,
+		.interface = PHY_INTERFACE_MODE_RMII,
+		.clk_csr = 2,
+		.has_gmac = 1,
+		.force_sf_dma_mode = 1,
+		.multicast_filter_bins = HASH_TABLE_SIZE,
+		.unicast_filter_entries = 1,
+		.phy_reset = NULL,
+		.phy_mask = 0,
+		.pbl = 16,
+		.fixed_burst = 1,
+		.burst_len = DMA_AXI_BLEN_256,
+		.rt_config = &quark_run_time_config,       ==========> Quark specific 2nd-level discovery
+	},
+};

(3) Within quark_run_time_config(), due to the only way to differentiate 1st or 2nd MAC controller according to difference in function ID explained above, the following changes are made:

+static void quark_run_time_config(int chip_id, struct pci_dev *pdev) {
+	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
+	int i;
+	int func_num = PCI_FUNC(pdev->devfn);
+
+	if (!board_name)
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(quark_x1000_phy_info); i++) {
+		if ((!strcmp(quark_x1000_phy_info[i].board_name, board_name)) &&
+		    quark_x1000_phy_info[i].pci_func_num == func_num)
+			platform_info[chip_id].phy_addr =
+				quark_x1000_phy_info[i].phy_address;
+	}
+}

The reasons for the above proposed condition checks, i.e. "board name" & "pci function name" are below:
  a) As described above, the only difference in both instance of STMMAC IP inside Intel Quark SoC is the function ID,
       so I have proposed to use function ID to be the decision point here to differentiate 1st MAC from 2nd MAC. 
  b) Allow future expansion of any other Intel Quark platforms with specific need to fix PHY address
  c) A PHY address set as "-1"  is to mark that the PHY (associated with function ID) is not connected to MAC, which
      is being used here for the 2 Galileo boards -> 2nd MAC port not connected with PHY.
  

Finally, based on the above description, it appears to me that using PCI function ID to decode seems viable for Intel Quark specific hardware configuration. 

Appreciate your time and any feedback is very much appreciated.

Thanks. 


Regards,
Wilson

Re: [PATCH v2 3/4] net: stmmac: add support for Intel Quark X1000

[David Miller]

I'm not discussing things in private email, keep the discussion on-list so
that any developer, not just me, can answer you.