[PATCH v2 0/6] stmmac: pci: Refactor DMI probing

[PATCH v2 0/6] stmmac: pci: Refactor DMI probing

[Jan Kiszka]

Some cleanups of the way we probe DMI platforms in the driver. Reduces
a bit of open-coding and makes the logic easier reusable for any
potential DMI platform != Quark.

Tested on IOT2000 and Galileo Gen2.

Changes in v2:
 - fixed bug that broke x86-64 build (and likely more)
 - refactored series to do smaller steps

All this remains cosmetic from a certain distance, but the result looks
more appealing, at least to me.

Jan

Jan Kiszka (6):
  stmmac: pci: Make stmmac_pci_info structure constant
  stmmac: pci: Use stmmac_pci_info for all devices
  stmmac: pci: Make stmmac_pci_find_phy_addr truly generic
  stmmac: pci: Select quark_pci_dmi_data from quark_default_data
  stmmac: pci: Use dmi_system_id table for retrieving PHY addresses
  stmmac: pci: Remove setup handler indirection via stmmac_pci_info

 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 184 ++++++++++++-----------
 1 file changed, 98 insertions(+), 86 deletions(-)

-- 
2.12.0

[PATCH v2 1/6] stmmac: pci: Make stmmac_pci_info structure constant

[Jan Kiszka]

By removing the PCI device reference from the structure and passing it
as parameters to the interested functions, we can make quark_pci_info
const.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 22f910795be4..0efe42659a37 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -38,17 +38,17 @@ struct stmmac_pci_dmi_data {
 };
 
 struct stmmac_pci_info {
-	struct pci_dev *pdev;
-	int (*setup)(struct plat_stmmacenet_data *plat,
-		     struct stmmac_pci_info *info);
+	int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat,
+		     const struct stmmac_pci_info *info);
 	struct stmmac_pci_dmi_data *dmi;
 };
 
-static int stmmac_pci_find_phy_addr(struct stmmac_pci_info *info)
+static int stmmac_pci_find_phy_addr(struct pci_dev *pdev,
+				    const struct stmmac_pci_info *info)
 {
 	const char *name = dmi_get_system_info(DMI_BOARD_NAME);
 	const char *asset_tag = dmi_get_system_info(DMI_BOARD_ASSET_TAG);
-	unsigned int func = PCI_FUNC(info->pdev->devfn);
+	unsigned int func = PCI_FUNC(pdev->devfn);
 	struct stmmac_pci_dmi_data *dmi;
 
 	/*
@@ -114,10 +114,10 @@ static void stmmac_default_data(struct plat_stmmacenet_data *plat)
 	/* TODO: AXI */
 }
 
-static int quark_default_data(struct plat_stmmacenet_data *plat,
-			      struct stmmac_pci_info *info)
+static int quark_default_data(struct pci_dev *pdev,
+			      struct plat_stmmacenet_data *plat,
+			      const struct stmmac_pci_info *info)
 {
-	struct pci_dev *pdev = info->pdev;
 	int ret;
 
 	/* Set common default data first */
@@ -127,7 +127,7 @@ static int quark_default_data(struct plat_stmmacenet_data *plat,
 	 * Refuse to load the driver and register net device if MAC controller
 	 * does not connect to any PHY interface.
 	 */
-	ret = stmmac_pci_find_phy_addr(info);
+	ret = stmmac_pci_find_phy_addr(pdev, info);
 	if (ret < 0)
 		return ret;
 
@@ -175,7 +175,7 @@ static struct stmmac_pci_dmi_data quark_pci_dmi_data[] = {
 	{}
 };
 
-static struct stmmac_pci_info quark_pci_info = {
+static const struct stmmac_pci_info quark_pci_info = {
 	.setup = quark_default_data,
 	.dmi = quark_pci_dmi_data,
 };
@@ -237,9 +237,8 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 	pci_set_master(pdev);
 
 	if (info) {
-		info->pdev = pdev;
 		if (info->setup) {
-			ret = info->setup(plat, info);
+			ret = info->setup(pdev, plat, info);
 			if (ret)
 				return ret;
 		}
-- 
2.12.0

[PATCH v2 2/6] stmmac: pci: Use stmmac_pci_info for all devices

[Jan Kiszka]

Make stmmac_default_data compatible with stmmac_pci_info.setup and use
an info structure for all devices. This allows to make the probing more
regular.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 33 +++++++++++++++---------
 1 file changed, 21 insertions(+), 12 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 0efe42659a37..9aca14f8b55e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -100,7 +100,9 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
 	plat->rx_queues_cfg[0].pkt_route = 0x0;
 }
 
-static void stmmac_default_data(struct plat_stmmacenet_data *plat)
+static int stmmac_default_data(struct pci_dev *pdev,
+			       struct plat_stmmacenet_data *plat,
+			       const struct stmmac_pci_info *info)
 {
 	/* Set common default data first */
 	common_default_data(plat);
@@ -112,8 +114,14 @@ static void stmmac_default_data(struct plat_stmmacenet_data *plat)
 	plat->dma_cfg->pbl = 32;
 	plat->dma_cfg->pblx8 = true;
 	/* TODO: AXI */
+
+	return 0;
 }
 
+static const struct stmmac_pci_info stmmac_pci_info = {
+	.setup = stmmac_default_data,
+};
+
 static int quark_default_data(struct pci_dev *pdev,
 			      struct plat_stmmacenet_data *plat,
 			      const struct stmmac_pci_info *info)
@@ -236,14 +244,9 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 
 	pci_set_master(pdev);
 
-	if (info) {
-		if (info->setup) {
-			ret = info->setup(pdev, plat, info);
-			if (ret)
-				return ret;
-		}
-	} else
-		stmmac_default_data(plat);
+	ret = info->setup(pdev, plat, info);
+	if (ret)
+		return ret;
 
 	pci_enable_msi(pdev);
 
@@ -273,10 +276,16 @@ static SIMPLE_DEV_PM_OPS(stmmac_pm_ops, stmmac_suspend, stmmac_resume);
 #define STMMAC_QUARK_ID  0x0937
 #define STMMAC_DEVICE_ID 0x1108
 
+#define STMMAC_DEVICE(vendor_id, dev_id, info)	{	\
+	PCI_DEVICE(vendor_id, dev_id),			\
+	.driver_data = (kernel_ulong_t)&info		\
+	}
+
 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_VDEVICE(INTEL, STMMAC_QUARK_ID), (kernel_ulong_t)&quark_pci_info},
+	STMMAC_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID, stmmac_pci_info),
+	STMMAC_DEVICE(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_MAC,
+		      stmmac_pci_info),
+	STMMAC_DEVICE(PCI_VENDOR_ID_INTEL, STMMAC_QUARK_ID, quark_pci_info),
 	{}
 };
 
-- 
2.12.0

[PATCH v2 3/6] stmmac: pci: Make stmmac_pci_find_phy_addr truly generic

[Jan Kiszka]

Move the special case for the early Galileo firmware into
quark_default_setup. This allows to use stmmac_pci_find_phy_addr for
non-quark cases.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 9aca14f8b55e..1a89fa9ee39d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -51,12 +51,8 @@ static int stmmac_pci_find_phy_addr(struct pci_dev *pdev,
 	unsigned int func = PCI_FUNC(pdev->devfn);
 	struct stmmac_pci_dmi_data *dmi;
 
-	/*
-	 * Galileo boards with old firmware don't support DMI. We always return
-	 * 1 here, so at least first found MAC controller would be probed.
-	 */
 	if (!name)
-		return 1;
+		return -ENODEV;
 
 	for (dmi = info->dmi; dmi->name && *dmi->name; dmi++) {
 		if (!strcmp(dmi->name, name) && dmi->func == func) {
@@ -136,8 +132,17 @@ static int quark_default_data(struct pci_dev *pdev,
 	 * does not connect to any PHY interface.
 	 */
 	ret = stmmac_pci_find_phy_addr(pdev, info);
-	if (ret < 0)
-		return ret;
+	if (ret < 0) {
+		/*
+		 * Galileo boards with old firmware don't support DMI. We always
+		 * use 1 here as PHY address, so at least the first found MAC
+		 * controller would be probed.
+		 */
+		if (!dmi_get_system_info(DMI_BOARD_NAME))
+			ret = 1;
+		else
+			return ret;
+	}
 
 	plat->bus_id = PCI_DEVID(pdev->bus->number, pdev->devfn);
 	plat->phy_addr = ret;
-- 
2.12.0

[PATCH v2 4/6] stmmac: pci: Select quark_pci_dmi_data from quark_default_data

[Jan Kiszka]

No need to carry this reference in stmmac_pci_info - the Quark-specific
setup handler knows that it needs to use the Quark-specific DMI table.
This also allows to drop the stmmac_pci_info reference from the setup
handler parameter list.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 83 +++++++++++-------------
 1 file changed, 39 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 1a89fa9ee39d..07af42531fd4 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -38,13 +38,11 @@ struct stmmac_pci_dmi_data {
 };
 
 struct stmmac_pci_info {
-	int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat,
-		     const struct stmmac_pci_info *info);
-	struct stmmac_pci_dmi_data *dmi;
+	int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat);
 };
 
 static int stmmac_pci_find_phy_addr(struct pci_dev *pdev,
-				    const struct stmmac_pci_info *info)
+				    struct stmmac_pci_dmi_data *dmi_data)
 {
 	const char *name = dmi_get_system_info(DMI_BOARD_NAME);
 	const char *asset_tag = dmi_get_system_info(DMI_BOARD_ASSET_TAG);
@@ -54,7 +52,7 @@ static int stmmac_pci_find_phy_addr(struct pci_dev *pdev,
 	if (!name)
 		return -ENODEV;
 
-	for (dmi = info->dmi; dmi->name && *dmi->name; dmi++) {
+	for (dmi = dmi_data; dmi->name && *dmi->name; dmi++) {
 		if (!strcmp(dmi->name, name) && dmi->func == func) {
 			/* If asset tag is provided, match on it as well. */
 			if (dmi->asset_tag && strcmp(dmi->asset_tag, asset_tag))
@@ -97,8 +95,7 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
 }
 
 static int stmmac_default_data(struct pci_dev *pdev,
-			       struct plat_stmmacenet_data *plat,
-			       const struct stmmac_pci_info *info)
+			       struct plat_stmmacenet_data *plat)
 {
 	/* Set common default data first */
 	common_default_data(plat);
@@ -118,9 +115,40 @@ static const struct stmmac_pci_info stmmac_pci_info = {
 	.setup = stmmac_default_data,
 };
 
+static struct stmmac_pci_dmi_data quark_pci_dmi_data[] = {
+	{
+		.name = "Galileo",
+		.func = 6,
+		.phy_addr = 1,
+	},
+	{
+		.name = "GalileoGen2",
+		.func = 6,
+		.phy_addr = 1,
+	},
+	{
+		.name = "SIMATIC IOT2000",
+		.asset_tag = "6ES7647-0AA00-0YA2",
+		.func = 6,
+		.phy_addr = 1,
+	},
+	{
+		.name = "SIMATIC IOT2000",
+		.asset_tag = "6ES7647-0AA00-1YA2",
+		.func = 6,
+		.phy_addr = 1,
+	},
+	{
+		.name = "SIMATIC IOT2000",
+		.asset_tag = "6ES7647-0AA00-1YA2",
+		.func = 7,
+		.phy_addr = 1,
+	},
+	{}
+};
+
 static int quark_default_data(struct pci_dev *pdev,
-			      struct plat_stmmacenet_data *plat,
-			      const struct stmmac_pci_info *info)
+			      struct plat_stmmacenet_data *plat)
 {
 	int ret;
 
@@ -131,7 +159,7 @@ static int quark_default_data(struct pci_dev *pdev,
 	 * Refuse to load the driver and register net device if MAC controller
 	 * does not connect to any PHY interface.
 	 */
-	ret = stmmac_pci_find_phy_addr(pdev, info);
+	ret = stmmac_pci_find_phy_addr(pdev, quark_pci_dmi_data);
 	if (ret < 0) {
 		/*
 		 * Galileo boards with old firmware don't support DMI. We always
@@ -156,41 +184,8 @@ static int quark_default_data(struct pci_dev *pdev,
 	return 0;
 }
 
-static struct stmmac_pci_dmi_data quark_pci_dmi_data[] = {
-	{
-		.name = "Galileo",
-		.func = 6,
-		.phy_addr = 1,
-	},
-	{
-		.name = "GalileoGen2",
-		.func = 6,
-		.phy_addr = 1,
-	},
-	{
-		.name = "SIMATIC IOT2000",
-		.asset_tag = "6ES7647-0AA00-0YA2",
-		.func = 6,
-		.phy_addr = 1,
-	},
-	{
-		.name = "SIMATIC IOT2000",
-		.asset_tag = "6ES7647-0AA00-1YA2",
-		.func = 6,
-		.phy_addr = 1,
-	},
-	{
-		.name = "SIMATIC IOT2000",
-		.asset_tag = "6ES7647-0AA00-1YA2",
-		.func = 7,
-		.phy_addr = 1,
-	},
-	{}
-};
-
 static const struct stmmac_pci_info quark_pci_info = {
 	.setup = quark_default_data,
-	.dmi = quark_pci_dmi_data,
 };
 
 /**
@@ -249,7 +244,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 
 	pci_set_master(pdev);
 
-	ret = info->setup(pdev, plat, info);
+	ret = info->setup(pdev, plat);
 	if (ret)
 		return ret;
 
-- 
2.12.0

[PATCH v2 5/6] stmmac: pci: Use dmi_system_id table for retrieving PHY addresses

[Jan Kiszka]

Avoids reimplementation of DMI matching in stmmac_pci_find_phy_addr.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 77 ++++++++++++++----------
 1 file changed, 45 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 07af42531fd4..061cb28f642d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -31,9 +31,7 @@
  * with PHY.
  */
 struct stmmac_pci_dmi_data {
-	const char *name;
-	const char *asset_tag;
-	unsigned int func;
+	int func;
 	int phy_addr;
 };
 
@@ -42,24 +40,19 @@ struct stmmac_pci_info {
 };
 
 static int stmmac_pci_find_phy_addr(struct pci_dev *pdev,
-				    struct stmmac_pci_dmi_data *dmi_data)
+				    const struct dmi_system_id *dmi_list)
 {
-	const char *name = dmi_get_system_info(DMI_BOARD_NAME);
-	const char *asset_tag = dmi_get_system_info(DMI_BOARD_ASSET_TAG);
-	unsigned int func = PCI_FUNC(pdev->devfn);
-	struct stmmac_pci_dmi_data *dmi;
+	const struct stmmac_pci_dmi_data *dmi_data;
+	const struct dmi_system_id *dmi_id;
+	int func = PCI_FUNC(pdev->devfn);
 
-	if (!name)
+	dmi_id = dmi_first_match(dmi_list);
+	if (!dmi_id)
 		return -ENODEV;
 
-	for (dmi = dmi_data; dmi->name && *dmi->name; dmi++) {
-		if (!strcmp(dmi->name, name) && dmi->func == func) {
-			/* If asset tag is provided, match on it as well. */
-			if (dmi->asset_tag && strcmp(dmi->asset_tag, asset_tag))
-				continue;
-			return dmi->phy_addr;
-		}
-	}
+	for (dmi_data = dmi_id->driver_data; dmi_data->func >= 0; dmi_data++)
+		if (dmi_data->func == func)
+			return dmi_data->phy_addr;
 
 	return -ENODEV;
 }
@@ -115,34 +108,54 @@ static const struct stmmac_pci_info stmmac_pci_info = {
 	.setup = stmmac_default_data,
 };
 
-static struct stmmac_pci_dmi_data quark_pci_dmi_data[] = {
+static const struct stmmac_pci_dmi_data galileo_stmmac_dmi_data[] = {
 	{
-		.name = "Galileo",
 		.func = 6,
 		.phy_addr = 1,
 	},
+	{-1, -1},
+};
+
+static const struct stmmac_pci_dmi_data iot2040_stmmac_dmi_data[] = {
 	{
-		.name = "GalileoGen2",
 		.func = 6,
 		.phy_addr = 1,
 	},
 	{
-		.name = "SIMATIC IOT2000",
-		.asset_tag = "6ES7647-0AA00-0YA2",
-		.func = 6,
+		.func = 7,
 		.phy_addr = 1,
 	},
+	{-1, -1},
+};
+
+static const struct dmi_system_id quark_pci_dmi[] = {
 	{
-		.name = "SIMATIC IOT2000",
-		.asset_tag = "6ES7647-0AA00-1YA2",
-		.func = 6,
-		.phy_addr = 1,
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "Galileo"),
+		},
+		.driver_data = (void *)galileo_stmmac_dmi_data,
 	},
 	{
-		.name = "SIMATIC IOT2000",
-		.asset_tag = "6ES7647-0AA00-1YA2",
-		.func = 7,
-		.phy_addr = 1,
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"),
+		},
+		.driver_data = (void *)galileo_stmmac_dmi_data,
+	},
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "SIMATIC IOT2000"),
+			DMI_EXACT_MATCH(DMI_BOARD_ASSET_TAG,
+					"6ES7647-0AA00-0YA2"),
+		},
+		.driver_data = (void *)galileo_stmmac_dmi_data,
+	},
+	{
+		.matches = {
+			DMI_EXACT_MATCH(DMI_BOARD_NAME, "SIMATIC IOT2000"),
+			DMI_EXACT_MATCH(DMI_BOARD_ASSET_TAG,
+					"6ES7647-0AA00-1YA2"),
+		},
+		.driver_data = (void *)iot2040_stmmac_dmi_data,
 	},
 	{}
 };
@@ -159,7 +172,7 @@ static int quark_default_data(struct pci_dev *pdev,
 	 * Refuse to load the driver and register net device if MAC controller
 	 * does not connect to any PHY interface.
 	 */
-	ret = stmmac_pci_find_phy_addr(pdev, quark_pci_dmi_data);
+	ret = stmmac_pci_find_phy_addr(pdev, quark_pci_dmi);
 	if (ret < 0) {
 		/*
 		 * Galileo boards with old firmware don't support DMI. We always
-- 
2.12.0

[PATCH v2 6/6] stmmac: pci: Remove setup handler indirection via stmmac_pci_info

[Jan Kiszka]

By now, stmmac_pci_info only contains a single entry. Register this
directly with the PCI device table, removing one indirection.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 35 +++++++++---------------
 1 file changed, 13 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 061cb28f642d..485216369705 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -35,9 +35,7 @@ struct stmmac_pci_dmi_data {
 	int phy_addr;
 };
 
-struct stmmac_pci_info {
-	int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat);
-};
+typedef int (*stmmac_setup)(struct pci_dev *, struct plat_stmmacenet_data *);
 
 static int stmmac_pci_find_phy_addr(struct pci_dev *pdev,
 				    const struct dmi_system_id *dmi_list)
@@ -87,8 +85,8 @@ static void common_default_data(struct plat_stmmacenet_data *plat)
 	plat->rx_queues_cfg[0].pkt_route = 0x0;
 }
 
-static int stmmac_default_data(struct pci_dev *pdev,
-			       struct plat_stmmacenet_data *plat)
+static int stmmac_default_setup(struct pci_dev *pdev,
+				struct plat_stmmacenet_data *plat)
 {
 	/* Set common default data first */
 	common_default_data(plat);
@@ -104,10 +102,6 @@ static int stmmac_default_data(struct pci_dev *pdev,
 	return 0;
 }
 
-static const struct stmmac_pci_info stmmac_pci_info = {
-	.setup = stmmac_default_data,
-};
-
 static const struct stmmac_pci_dmi_data galileo_stmmac_dmi_data[] = {
 	{
 		.func = 6,
@@ -160,8 +154,8 @@ static const struct dmi_system_id quark_pci_dmi[] = {
 	{}
 };
 
-static int quark_default_data(struct pci_dev *pdev,
-			      struct plat_stmmacenet_data *plat)
+static int quark_default_setup(struct pci_dev *pdev,
+			       struct plat_stmmacenet_data *plat)
 {
 	int ret;
 
@@ -197,10 +191,6 @@ static int quark_default_data(struct pci_dev *pdev,
 	return 0;
 }
 
-static const struct stmmac_pci_info quark_pci_info = {
-	.setup = quark_default_data,
-};
-
 /**
  * stmmac_pci_probe
  *
@@ -216,7 +206,7 @@ static const struct stmmac_pci_info quark_pci_info = {
 static int stmmac_pci_probe(struct pci_dev *pdev,
 			    const struct pci_device_id *id)
 {
-	struct stmmac_pci_info *info = (struct stmmac_pci_info *)id->driver_data;
+	stmmac_setup setup = (stmmac_setup)id->driver_data;
 	struct plat_stmmacenet_data *plat;
 	struct stmmac_resources res;
 	int i;
@@ -257,7 +247,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
 
 	pci_set_master(pdev);
 
-	ret = info->setup(pdev, plat);
+	ret = setup(pdev, plat);
 	if (ret)
 		return ret;
 
@@ -289,16 +279,17 @@ static SIMPLE_DEV_PM_OPS(stmmac_pm_ops, stmmac_suspend, stmmac_resume);
 #define STMMAC_QUARK_ID  0x0937
 #define STMMAC_DEVICE_ID 0x1108
 
-#define STMMAC_DEVICE(vendor_id, dev_id, info)	{	\
+#define STMMAC_DEVICE(vendor_id, dev_id, setup)	{	\
 	PCI_DEVICE(vendor_id, dev_id),			\
-	.driver_data = (kernel_ulong_t)&info		\
+	.driver_data = (kernel_ulong_t)&setup		\
 	}
 
 static const struct pci_device_id stmmac_id_table[] = {
-	STMMAC_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID, stmmac_pci_info),
+	STMMAC_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID, stmmac_default_setup),
 	STMMAC_DEVICE(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_MAC,
-		      stmmac_pci_info),
-	STMMAC_DEVICE(PCI_VENDOR_ID_INTEL, STMMAC_QUARK_ID, quark_pci_info),
+		      stmmac_default_setup),
+	STMMAC_DEVICE(PCI_VENDOR_ID_INTEL, STMMAC_QUARK_ID,
+		      quark_default_setup),
 	{}
 };
 
-- 
2.12.0

Re: [PATCH v2 2/6] stmmac: pci: Use stmmac_pci_info for all devices

[Andy Shevchenko]

On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Make stmmac_default_data compatible with stmmac_pci_info.setup and use
> an info structure for all devices. This allows to make the probing more
> regular.


> +#define STMMAC_DEVICE(vendor_id, dev_id, info) {       \
> +       PCI_DEVICE(vendor_id, dev_id),                  \

Perhaps

#define STMMAC_DEVICE(_vid, _did, info) {       \
       PCI_DEVICE(PCI_VENDOR_ID_##_vid, _did),                  \

Or converting defines first to PCI_DEVICE_ID_*
and

#define STMMAC_DEVICE(_vid, _did, info) {       \
       PCI_DEVICE(PCI_VENDOR_ID_##_vid, PCI_DEVICE_ID_##_did),
         \

which I like even better.

> +       .driver_data = (kernel_ulong_t)&info            \
> +       }
> +
>  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_VDEVICE(INTEL, STMMAC_QUARK_ID), (kernel_ulong_t)&quark_pci_info},
> +       STMMAC_DEVICE(STMMAC_VENDOR_ID, STMMAC_DEVICE_ID, stmmac_pci_info),
> +       STMMAC_DEVICE(PCI_VENDOR_ID_STMICRO, PCI_DEVICE_ID_STMICRO_MAC,
> +                     stmmac_pci_info),
> +       STMMAC_DEVICE(PCI_VENDOR_ID_INTEL, STMMAC_QUARK_ID, quark_pci_info),

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 2/6] stmmac: pci: Use stmmac_pci_info for all devices

[Andy Shevchenko]

On Sat, May 27, 2017 at 4:13 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Make stmmac_default_data compatible with stmmac_pci_info.setup and use
>> an info structure for all devices. This allows to make the probing more
>> regular.

> Or converting defines first to PCI_DEVICE_ID_*

It looks even for previously mentioned approach we need to rename
constants first.

> and
>
> #define STMMAC_DEVICE(_vid, _did, info) {       \
>        PCI_DEVICE(PCI_VENDOR_ID_##_vid, PCI_DEVICE_ID_##_did),
>          \
>
> which I like even better.

Or even
 #define STMMAC_DEVICE(_vid, _did, info) {       \
        PCI_VDEVICE(_vid, PCI_DEVICE_ID_##_did),        \


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 3/6] stmmac: pci: Make stmmac_pci_find_phy_addr truly generic

[Andy Shevchenko]

On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Move the special case for the early Galileo firmware into
> quark_default_setup. This allows to use stmmac_pci_find_phy_addr for
> non-quark cases.

>         ret = stmmac_pci_find_phy_addr(pdev, info);
> -       if (ret < 0)
> -               return ret;
> +       if (ret < 0) {
> +               /*
> +                * Galileo boards with old firmware don't support DMI. We always
> +                * use 1 here as PHY address, so at least the first found MAC
> +                * controller would be probed.
> +                */
> +               if (!dmi_get_system_info(DMI_BOARD_NAME))
> +                       ret = 1;
> +               else
> +                       return ret;

Perhaps

/* Return error to the caller on DMI enabled boards */
if (dmi_...)
 return ret;
/*
 * Comment goes here, I suppose.
 */
ret = 1;

> +       }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 5/6] stmmac: pci: Use dmi_system_id table for retrieving PHY addresses

[Andy Shevchenko]

On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> Avoids reimplementation of DMI matching in stmmac_pci_find_phy_addr.

>  struct stmmac_pci_dmi_data {
> -       const char *name;
> -       const char *asset_tag;
> -       unsigned int func;
> +       int func;
>         int phy_addr;
>  };

Can we leave unsigned type here...

> -static struct stmmac_pci_dmi_data quark_pci_dmi_data[] = {
> +static const struct stmmac_pci_dmi_data galileo_stmmac_dmi_data[] = {

> +       {-1, -1},
> +};

> +static const struct stmmac_pci_dmi_data iot2040_stmmac_dmi_data[] = {

> +       {-1, -1},
> +};

...and avoid this not so standard terminators?

> +               .matches = {
> +                       DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"),
> +               },

> +               .driver_data = (void *)galileo_stmmac_dmi_data,

Can't be slightly better

             .driver_data = &galileo_stmmac_dmi_data,

?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] stmmac: pci: Remove setup handler indirection via stmmac_pci_info

[Andy Shevchenko]

On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> By now, stmmac_pci_info only contains a single entry.

_For now_.

> Register this
> directly with the PCI device table, removing one indirection.

I am not sure this patch is needed.

Next time something comes up we would need to extend this and
effectively revert this change.
So, my vote is to leave it as is for now.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 6/6] stmmac: pci: Remove setup handler indirection via stmmac_pci_info

[Jan Kiszka]

On 2017-05-27 15:38, Andy Shevchenko wrote:
> On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> By now, stmmac_pci_info only contains a single entry.
> 
> _For now_.
> 
>> Register this
>> directly with the PCI device table, removing one indirection.
> 
> I am not sure this patch is needed.
> 
> Next time something comes up we would need to extend this and
> effectively revert this change.
> So, my vote is to leave it as is for now.

Therefore moved this to the end: may the maintainer pick it or not.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 5/6] stmmac: pci: Use dmi_system_id table for retrieving PHY addresses

[Jan Kiszka]

On 2017-05-27 15:28, Andy Shevchenko wrote:
> On Fri, May 26, 2017 at 7:07 PM, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>> Avoids reimplementation of DMI matching in stmmac_pci_find_phy_addr.
> 
>>  struct stmmac_pci_dmi_data {
>> -       const char *name;
>> -       const char *asset_tag;
>> -       unsigned int func;
>> +       int func;
>>         int phy_addr;
>>  };
> 
> Can we leave unsigned type here...
> 
>> -static struct stmmac_pci_dmi_data quark_pci_dmi_data[] = {
>> +static const struct stmmac_pci_dmi_data galileo_stmmac_dmi_data[] = {
> 
>> +       {-1, -1},
>> +};
> 
>> +static const struct stmmac_pci_dmi_data iot2040_stmmac_dmi_data[] = {
> 
>> +       {-1, -1},
>> +};
> 
> ...and avoid this not so standard terminators?

0 is a valid PCI function, thus can't be use as terminator. Therefore I
chose -1 as an obviously invalid value.

> 
>> +               .matches = {
>> +                       DMI_EXACT_MATCH(DMI_BOARD_NAME, "GalileoGen2"),
>> +               },
> 
>> +               .driver_data = (void *)galileo_stmmac_dmi_data,
> 
> Can't be slightly better
> 
>              .driver_data = &galileo_stmmac_dmi_data,
> 
> ?
> 

Interesting, that removes the "const" as well. OK.

Jan

-- 
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux