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

[PATCH 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.

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 |  195 +++++++++++++++++++---
 1 file changed, 172 insertions(+), 23 deletions(-)

-- 
1.7.9.5

[PATCH 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 |   60 ++++++++++++++--------
 1 file changed, 39 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 655a23b..9d98935 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -26,27 +26,22 @@
 #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 int instance_id = 1;
 
-static void stmmac_default_data(void)
+static void stmmac_default_data(struct plat_stmmacenet_data *plat_dat)
 {
-	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 = instance_id++;
+	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 +62,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 +93,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);
 
-	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 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 |   76 +++++++++++++++++-----
 1 file changed, 60 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 9d98935..40290da 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -28,20 +28,63 @@
 
 static int instance_id = 1;
 
-static void stmmac_default_data(struct plat_stmmacenet_data *plat_dat)
+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)
 {
-	plat_dat->bus_id = instance_id++;
-	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 = instance_id++;
+	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;
 }
 
 /**
@@ -115,7 +158,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 		goto err_out;
 	}
 
-	stmmac_default_data(plat_dat);
+	stmmac_default_data(plat_dat, id->driver_data);
 
 	priv = stmmac_dvr_probe(&pdev->dev, plat_dat, addr);
 	if (IS_ERR(priv)) {
@@ -189,8 +232,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 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 |   86 ++++++++++++++++++++--
 1 file changed, 81 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 40290da..81e48f4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -24,16 +24,19 @@
 *******************************************************************************/
 
 #include <linux/pci.h>
+#include <linux/dmi.h>
 #include "stmmac.h"
 
 static int instance_id = 1;
+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;
@@ -46,7 +49,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[] = {
@@ -61,15 +66,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 void stmmac_default_data(struct plat_stmmacenet_data *plat,
-				int chip_id)
+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 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 = instance_id++;
 	plat->phy_addr = chip_plat_dat->phy_addr;
 	plat->interface = chip_plat_dat->interface;
@@ -84,7 +150,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;
 }
 
 /**
@@ -158,7 +230,9 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 		goto err_out;
 	}
 
-	stmmac_default_data(plat_dat, id->driver_data);
+	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)) {
@@ -230,11 +304,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 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 81e48f4..36e36d5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -52,6 +52,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[] = {
@@ -69,6 +70,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,
@@ -84,6 +86,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,
 	},
 };
 
@@ -211,7 +214,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,
@@ -219,7 +222,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,
@@ -227,12 +230,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)) {
@@ -250,6 +256,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);
@@ -273,6 +282,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 0/4] net: stmmac: Enable Intel Quark SoC X1000 Ethernet support

[Giuseppe CAVALLARO]

On 8/27/2014 12:32 PM, Kweh Hock Leong wrote:
> 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.

hello and thx for these patches that at first glance look ok to me.
Just some minor remark, in the stmmac I try to align the function 
parameters with the open parenthesis (devm_kzalloc in your case in not 
aligned).
Added on copy also Rayagond he tested PCI. I cannot do any test because 
I have no PCI cards.

peppe

>
> Thank you very much.
>
> 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 |  195 +++++++++++++++++++---
>   1 file changed, 172 insertions(+), 23 deletions(-)
>

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Giuseppe CAVALLARO [mailto:peppe.cavallaro@st.com]
> Sent: Wednesday, August 27, 2014 8:53 PM
> To: Kweh, Hock Leong; David S. Miller
> Cc: netdev@vger.kernel.org; LKML; Ong, Boon Leong; Rayagond K
> Subject: Re: [PATCH 0/4] net: stmmac: Enable Intel Quark SoC X1000 Ethernet
> support
> 
> On 8/27/2014 12:32 PM, Kweh Hock Leong wrote:
> 
> hello and thx for these patches that at first glance look ok to me.
> Just some minor remark, in the stmmac I try to align the function parameters
> with the open parenthesis (devm_kzalloc in your case in not aligned).
> Added on copy also Rayagond he tested PCI. I cannot do any test because I
> have no PCI cards.
> 
> peppe
> 

Hi, noted and will update that in the v2 patch. Looking forward to get more feedback
before sending out the second version. Thanks.


Regards,
Wilson

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

[David Miller]

From: Kweh Hock Leong <hock.leong.kweh@intel.com>
Date: Wed, 27 Aug 2014 18:32:26 +0800

> @@ -26,27 +26,22 @@
>  #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 int instance_id = 1;

Don't do this instance stuff.  Instead pull in some identifier that
can come from elsewhere.

> +	plat_dat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> +				    sizeof(*plat_dat->mdio_bus_data),
> +				    GFP_KERNEL);

This is not indented properly.

On the second and subsequent lines of a multi-line function call,
the lines should start exactly at the first column after the openning
parenthesis of the first line.

You must use the correct number of TAB and SPACE characters necessary
to do so.  Generally speaking, if you are indenting using only TAB
characters, odds are you are doing it wrong.

Please audit for, and fix this, in your entire patch series.

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Saturday, August 30, 2014 11:06 AM
> To: Kweh, Hock Leong
> Cc: peppe.cavallaro@st.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Ong, Boon Leong
> Subject: Re: [PATCH 1/4] net: stmmac: enhance to support multiple device
> instances
> 
> From: Kweh Hock Leong <hock.leong.kweh@intel.com>
> Date: Wed, 27 Aug 2014 18:32:26 +0800
> 
> > +static int instance_id = 1;
> 
> Don't do this instance stuff.  Instead pull in some identifier that
> can come from elsewhere.

Regarding this, I would like to open up a discussion here. This "instance_id" actually
is used for registering the mdio bus as a bus id. The original code use "1" for the bus
id. If the system plug in more than one stmmac pci cards, I believe there is conflict on
the mdio bus registration. So introduce this static global variable is to increase the
bus id starting from "1" base on how many stmmac pci cards being plugged in.

So, to change the "instance_id" by using some identifier, the only thing come to my
mind is pci_dev->devfn number. Is anyone have concern about using devfn number
as an mdio bus id ?


> > +	plat_dat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> > +				    sizeof(*plat_dat->mdio_bus_data),
> > +				    GFP_KERNEL);
> 
> This is not indented properly.
> 
> On the second and subsequent lines of a multi-line function call,
> the lines should start exactly at the first column after the openning
> parenthesis of the first line.
> 
> You must use the correct number of TAB and SPACE characters necessary
> to do so.  Generally speaking, if you are indenting using only TAB
> characters, odds are you are doing it wrong.
> 
> Please audit for, and fix this, in your entire patch series.

Noted. Will fix the indentation on version 2 patch. Thanks.

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Kweh, Hock Leong
> Sent: Saturday, August 30, 2014 11:48 AM
> To: David Miller
> Cc: peppe.cavallaro@st.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org; Ong, Boon Leong; Rayagond K
> Subject: RE: [PATCH 1/4] net: stmmac: enhance to support multiple device
> instances
> 
> > From: David Miller [mailto:davem@davemloft.net]
> > Sent: Saturday, August 30, 2014 11:06 AM
> > > From: Kweh Hock Leong <hock.leong.kweh@intel.com>
> > >Date: Wed, 27 Aug 2014 18:32:26 +0800
> > >
> > > +static int instance_id = 1;
> >
> > Don't do this instance stuff.  Instead pull in some identifier that
> > can come from elsewhere.
> 
> Regarding this, I would like to open up a discussion here. This "instance_id"
> actually is used for registering the mdio bus as a bus id. The original code use
> "1" for the bus id. If the system plug in more than one stmmac pci cards, I
> believe there is conflict on the mdio bus registration. So introduce this static
> global variable is to increase the bus id starting from "1" base on how many
> stmmac pci cards being plugged in.
> 
> So, to change the "instance_id" by using some identifier, the only thing come
> to my mind is pci_dev->devfn number. Is anyone have concern about using
> devfn number as an mdio bus id ?

Hi David and everyone,

Just gently ping to see if any concern about the above discussion that
removing the instance_id and use the PCI_DEVID number for the mdio bus
number registration as showed below:

-static void stmmac_default_data(void)
+static void stmmac_default_data(struct plat_stmmacenet_data *plat_dat,
+                                                                   struct pci_dev *pdev)
 {
-       plat_dat.bus_id = 1;
+       plat_dat->bus_id = PCI_DEVID(pdev->bus->number, pdev->devfn);


Looking forward to get inputs from you guys. I will send the 2nd version
by this Wed if no one have concern to this. Thanks.


Regards,
Wilson