[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

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

[Bryan O'Donoghue]

On 01/10/14 12:55, Kweh, Hock Leong wrote:
>> -----Original Message-----
>> From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie]
>> Sent: Wednesday, October 01, 2014 7:29 PM
>> Hi Wilson.
>>
>> Seeing you post now on the PCI emumeration suggestion from Dave Miller I
>> see
>>
>> I wasn't copied on this https://lkml.org/lkml/2014/8/27/190 thread so can
>> only respond now....
>>
>> What's missing from your MSI enabling code is the PVM mask/unmask
>> required on the Quark X1000 bridge - for *all* downstream devices using MSI.
>>
>> I realise it's not an upstreaming friendly piece of code - however - without
>> the PVM mask operation all MSIs on Quark should be considered unreliable.
>>
>> Maybe you guys have submitted patches to the PCI layer on this already ?
>> If so feel free to ignore.
>>
>> If not then please re-evaluate all MSI enabling code.
>>
>>   From the original
>>
>> http://downloadmirror.intel.com/23171/eng/Board_Support_Package_Sour
>> ces_for_Intel_Quark_v1.0.0.7z
>>
>> +#if defined(CONFIG_INTEL_QUARK_X1000_SOC)
>> +	#define mask_pvm(x) qrk_pci_pvm_mask(x)
>> +	#define unmask_pvm(x) qrk_pci_pvm_unmask(x) #else
>> +	#define mask_pvm(x)
>> +	#define unmask_pvm(x)
>> +#endif
>> +
>>    static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
>>    {
>>    	struct net_device *dev = (struct net_device *)dev_id; @@ -1601,10
>> +1686,12 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
>>    		return IRQ_NONE;
>>    	}
>>
>> +	mask_pvm(priv->pdev);
>> +
>>    	/* To handle GMAC own interrupts */
>>    	if (priv->plat->has_gmac) {
>> -		int status = priv->hw->mac->host_irq_status((void __iomem
>> *)
>> -							    dev->base_addr);
>> +		int status = priv->hw->mac->host_irq_status(priv);
>> +
>>    		if (unlikely(status)) {
>>    			if (status & core_mmc_tx_irq)
>>    				priv->xstats.mmc_tx_irq_n++;
>> @@ -1634,6 +1721,8 @@ static irqreturn_t stmmac_interrupt(int irq, void
>> *dev_id)
>>    	/* To handle DMA interrupts */
>>    	stmmac_dma_interrupt(priv);
>>
>> +	unmask_pvm(priv->pdev);

> Hi Bryan,
>
> The MSI masking is already implemented in the MSI framework: http://lxr.free-electrons.com/source/drivers/pci/msi.c#L181.
> I don't see a reason to upstream a local set implementation to Ethernet subsystem.
> Thanks.

Hi Wilson.

Understand where you are getting your MSI enabling code from.

What I'm saying to you is that on Quark SoC X1000 there's an 
*additional* requirement with respect to MSIs

That's why the reference code for the Quark BSP does PVM masking for 
*all* MSI enabled code - not just ethernet.....

I'll have a review of the patches for the SoC thus far with a view to 
ensuring the MSI pvm issue is adequately addressed - but just to be 
clear it's emphatically *not* ethernet specific.

In essence the following additional requirement is place on the Quark 
SoC when using MSIs

pvm_mask();

/* handle your interrupt */

pvm_unmask();

It's the same behaviour in the USB gadget driver...

@@ -2779,55 +2788,70 @@ static irqreturn_t pch_udc_isr(int irq, void *pdev)
  {
  	struct pch_udc_dev *dev = (struct pch_udc_dev *) pdev;
  	u32 dev_intr, ep_intr;
-	int i;
-
-	dev_intr = pch_udc_read_device_interrupts(dev);
-	ep_intr = pch_udc_read_ep_interrupts(dev);
-
-	/* For a hot plug, this find that the controller is hung up. */
-	if (dev_intr == ep_intr)
-		if (dev_intr == pch_udc_readl(dev, UDC_DEVCFG_ADDR)) {
-			dev_dbg(&dev->pdev->dev, "UDC: Hung up\n");
-			/* The controller is reset */
-			pch_udc_writel(dev, UDC_SRST, UDC_SRST_ADDR);
-			return IRQ_HANDLED;
+	int i, events = 0;
+
+	mask_pvm(dev->pdev);
	// do stuff

+	unmask_pvm(dev->pdev);
+
  	return IRQ_HANDLED;
  }

And again in the GIP block

+static irqreturn_t intel_qrk_gip_handler(int irq, void *dev_id)
+{
+	irqreturn_t ret_i2c = IRQ_NONE;
+	irqreturn_t ret_gpio = IRQ_NONE;
+	struct intel_qrk_gip_data *data = (struct intel_qrk_gip_data *)dev_id;
+
+	mask_pvm(data->pci_device);
+
+	if (likely(i2c)) {
+		/* Only I2C gets platform data */
+		ret_i2c = i2c_dw_isr(irq, data->i2c_drvdata);
+	}
+
+	if (likely(gpio)) {
+		ret_gpio = intel_qrk_gpio_isr(irq, NULL);
+	}
+
+	unmask_pvm(data->pci_device);
+
+	if (likely(IRQ_HANDLED == ret_i2c || IRQ_HANDLED == ret_gpio))
+		return IRQ_HANDLED;
+
+	/* Each sub-ISR routine returns either IRQ_HANDLED or IRQ_NONE. */
+	return IRQ_NONE;
+}

Best,
	BOD

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

[Kweh, Hock Leong]

> -----Original Message-----
> From: Bryan O'Donoghue [mailto:pure.logic@nexus-software.ie]
> Sent: Wednesday, October 01, 2014 7:29 PM
> Hi Wilson.
> 
> Seeing you post now on the PCI emumeration suggestion from Dave Miller I
> see
> 
> I wasn't copied on this https://lkml.org/lkml/2014/8/27/190 thread so can
> only respond now....
> 
> What's missing from your MSI enabling code is the PVM mask/unmask
> required on the Quark X1000 bridge - for *all* downstream devices using MSI.
> 
> I realise it's not an upstreaming friendly piece of code - however - without
> the PVM mask operation all MSIs on Quark should be considered unreliable.
> 
> Maybe you guys have submitted patches to the PCI layer on this already ?
> If so feel free to ignore.
> 
> If not then please re-evaluate all MSI enabling code.
> 
>  From the original
> 
> http://downloadmirror.intel.com/23171/eng/Board_Support_Package_Sour
> ces_for_Intel_Quark_v1.0.0.7z
> 
> +#if defined(CONFIG_INTEL_QUARK_X1000_SOC)
> +	#define mask_pvm(x) qrk_pci_pvm_mask(x)
> +	#define unmask_pvm(x) qrk_pci_pvm_unmask(x) #else
> +	#define mask_pvm(x)
> +	#define unmask_pvm(x)
> +#endif
> +
>   static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
>   {
>   	struct net_device *dev = (struct net_device *)dev_id; @@ -1601,10
> +1686,12 @@ static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
>   		return IRQ_NONE;
>   	}
> 
> +	mask_pvm(priv->pdev);
> +
>   	/* To handle GMAC own interrupts */
>   	if (priv->plat->has_gmac) {
> -		int status = priv->hw->mac->host_irq_status((void __iomem
> *)
> -							    dev->base_addr);
> +		int status = priv->hw->mac->host_irq_status(priv);
> +
>   		if (unlikely(status)) {
>   			if (status & core_mmc_tx_irq)
>   				priv->xstats.mmc_tx_irq_n++;
> @@ -1634,6 +1721,8 @@ static irqreturn_t stmmac_interrupt(int irq, void
> *dev_id)
>   	/* To handle DMA interrupts */
>   	stmmac_dma_interrupt(priv);
> 
> +	unmask_pvm(priv->pdev);
> +
>   	return IRQ_HANDLED;
>   }

Hi Bryan,

The MSI masking is already implemented in the MSI framework: http://lxr.free-electrons.com/source/drivers/pci/msi.c#L181.
I don't see a reason to upstream a local set implementation to Ethernet subsystem.
Thanks.


Regards,
Wilson

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

[Bryan O'Donoghue]

> Hi Guys,
>
> Just gently ping for the discussion to carry on before forgetting the context.
> Anyone have any better idea or comments or concern to this topic?
> Hope the above explanation clear out your doubt.
>
>
> Regards,
> Wilson

Hi Wilson.

Seeing you post now on the PCI emumeration suggestion from Dave Miller I see

I wasn't copied on this https://lkml.org/lkml/2014/8/27/190 thread so 
can only respond now....

What's missing from your MSI enabling code is the PVM mask/unmask 
required on the Quark X1000 bridge - for *all* downstream devices using MSI.

I realise it's not an upstreaming friendly piece of code - however - 
without the PVM mask operation all MSIs on Quark should be considered 
unreliable.

Maybe you guys have submitted patches to the PCI layer on this already ? 
If so feel free to ignore.

If not then please re-evaluate all MSI enabling code.

 From the original

http://downloadmirror.intel.com/23171/eng/Board_Support_Package_Sources_for_Intel_Quark_v1.0.0.7z

+#if defined(CONFIG_INTEL_QUARK_X1000_SOC)
+	#define mask_pvm(x) qrk_pci_pvm_mask(x)
+	#define unmask_pvm(x) qrk_pci_pvm_unmask(x)
+#else
+	#define mask_pvm(x)
+	#define unmask_pvm(x)
+#endif
+
  static irqreturn_t stmmac_interrupt(int irq, void *dev_id)
  {
  	struct net_device *dev = (struct net_device *)dev_id;
@@ -1601,10 +1686,12 @@ static irqreturn_t stmmac_interrupt(int irq, 
void *dev_id)
  		return IRQ_NONE;
  	}

+	mask_pvm(priv->pdev);
+
  	/* To handle GMAC own interrupts */
  	if (priv->plat->has_gmac) {
-		int status = priv->hw->mac->host_irq_status((void __iomem *)
-							    dev->base_addr);
+		int status = priv->hw->mac->host_irq_status(priv);
+
  		if (unlikely(status)) {
  			if (status & core_mmc_tx_irq)
  				priv->xstats.mmc_tx_irq_n++;
@@ -1634,6 +1721,8 @@ static irqreturn_t stmmac_interrupt(int irq, void 
*dev_id)
  	/* To handle DMA interrupts */
  	stmmac_dma_interrupt(priv);

+	unmask_pvm(priv->pdev);
+
  	return IRQ_HANDLED;
  }