[PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Bartosz Folta]

There are hardware PCI implementations of Cadence GEM network
controller. This patch will allow to use such hardware with reuse of
existing Platform Driver.

Signed-off-by: Bartosz Folta <bfolta@cadence.com>
---
Changed in v3:
Fixed dependencies in Kconfig.
---
Changed in v2:
Respin to net-next. Changed patch formatting.
---
 drivers/net/ethernet/cadence/Kconfig    |   9 ++
 drivers/net/ethernet/cadence/Makefile   |   1 +
 drivers/net/ethernet/cadence/macb.c     |  31 +++++--
 drivers/net/ethernet/cadence/macb_pci.c | 153 ++++++++++++++++++++++++++++++++
 include/linux/platform_data/macb.h      |   6 ++
 5 files changed, 195 insertions(+), 5 deletions(-)
 create mode 100644 drivers/net/ethernet/cadence/macb_pci.c

diff --git a/drivers/net/ethernet/cadence/Kconfig b/drivers/net/ethernet/cadence/Kconfig
index f0bcb15..608bea1 100644
--- a/drivers/net/ethernet/cadence/Kconfig
+++ b/drivers/net/ethernet/cadence/Kconfig
@@ -31,4 +31,13 @@ config MACB
 	  To compile this driver as a module, choose M here: the module
 	  will be called macb.
 
+config MACB_PCI
+	tristate "Cadence PCI MACB/GEM support"
+	depends on MACB && PCI && COMMON_CLK
+	---help---
+	  This is PCI wrapper for MACB driver.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called macb_pci.
+
 endif # NET_CADENCE
diff --git a/drivers/net/ethernet/cadence/Makefile b/drivers/net/ethernet/cadence/Makefile
index 91f79b1..4ba7559 100644
--- a/drivers/net/ethernet/cadence/Makefile
+++ b/drivers/net/ethernet/cadence/Makefile
@@ -3,3 +3,4 @@
 #
 
 obj-$(CONFIG_MACB) += macb.o
+obj-$(CONFIG_MACB_PCI) += macb_pci.o
diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
index 538544a..c0fb80a 100644
--- a/drivers/net/ethernet/cadence/macb.c
+++ b/drivers/net/ethernet/cadence/macb.c
@@ -404,6 +404,8 @@ static int macb_mii_probe(struct net_device *dev)
 			phy_irq = gpio_to_irq(pdata->phy_irq_pin);
 			phydev->irq = (phy_irq < 0) ? PHY_POLL : phy_irq;
 		}
+	} else {
+		phydev->irq = PHY_POLL;
 	}
 
 	/* attach the mac to the phy */
@@ -482,6 +484,9 @@ static int macb_mii_init(struct macb *bp)
 				goto err_out_unregister_bus;
 		}
 	} else {
+		for (i = 0; i < PHY_MAX_ADDR; i++)
+			bp->mii_bus->irq[i] = PHY_POLL;
+
 		if (pdata)
 			bp->mii_bus->phy_mask = pdata->phy_mask;
 
@@ -2523,16 +2528,24 @@ static int macb_clk_init(struct platform_device *pdev, struct clk **pclk,
 			 struct clk **hclk, struct clk **tx_clk,
 			 struct clk **rx_clk)
 {
+	struct macb_platform_data *pdata;
 	int err;
 
-	*pclk = devm_clk_get(&pdev->dev, "pclk");
+	pdata = dev_get_platdata(&pdev->dev);
+	if (pdata) {
+		*pclk = pdata->pclk;
+		*hclk = pdata->hclk;
+	} else {
+		*pclk = devm_clk_get(&pdev->dev, "pclk");
+		*hclk = devm_clk_get(&pdev->dev, "hclk");
+	}
+
 	if (IS_ERR(*pclk)) {
 		err = PTR_ERR(*pclk);
 		dev_err(&pdev->dev, "failed to get macb_clk (%u)\n", err);
 		return err;
 	}
 
-	*hclk = devm_clk_get(&pdev->dev, "hclk");
 	if (IS_ERR(*hclk)) {
 		err = PTR_ERR(*hclk);
 		dev_err(&pdev->dev, "failed to get hclk (%u)\n", err);
@@ -3107,15 +3120,23 @@ static int at91ether_init(struct platform_device *pdev)
 MODULE_DEVICE_TABLE(of, macb_dt_ids);
 #endif /* CONFIG_OF */
 
+static const struct macb_config default_gem_config = {
+	.caps = MACB_CAPS_GIGABIT_MODE_AVAILABLE | MACB_CAPS_JUMBO,
+	.dma_burst_length = 16,
+	.clk_init = macb_clk_init,
+	.init = macb_init,
+	.jumbo_max_len = 10240,
+};
+
 static int macb_probe(struct platform_device *pdev)
 {
+	const struct macb_config *macb_config = &default_gem_config;
 	int (*clk_init)(struct platform_device *, struct clk **,
 			struct clk **, struct clk **,  struct clk **)
-					      = macb_clk_init;
-	int (*init)(struct platform_device *) = macb_init;
+					      = macb_config->clk_init;
+	int (*init)(struct platform_device *) = macb_config->init;
 	struct device_node *np = pdev->dev.of_node;
 	struct device_node *phy_node;
-	const struct macb_config *macb_config = NULL;
 	struct clk *pclk, *hclk = NULL, *tx_clk = NULL, *rx_clk = NULL;
 	unsigned int queue_mask, num_queues;
 	struct macb_platform_data *pdata;
diff --git a/drivers/net/ethernet/cadence/macb_pci.c b/drivers/net/ethernet/cadence/macb_pci.c
new file mode 100644
index 0000000..92be2cd
--- /dev/null
+++ b/drivers/net/ethernet/cadence/macb_pci.c
@@ -0,0 +1,153 @@
+/**
+ * macb_pci.c - Cadence GEM PCI wrapper.
+ *
+ * Copyright (C) 2016 Cadence Design Systems - http://www.cadence.com
+ *
+ * Authors: Rafal Ozieblo <rafalo@cadence.com>
+ *	    Bartosz Folta <bfolta@cadence.com>
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2  of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/clk.h>
+#include <linux/clk-provider.h>
+#include <linux/etherdevice.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_data/macb.h>
+#include <linux/platform_device.h>
+#include "macb.h"
+
+#define PCI_DRIVER_NAME "macb_pci"
+#define PLAT_DRIVER_NAME "macb"
+
+#define CDNS_VENDOR_ID 0x17cd
+#define CDNS_DEVICE_ID 0xe007
+
+#define GEM_PCLK_RATE 50000000
+#define GEM_HCLK_RATE 50000000
+
+static int macb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
+{
+	int err;
+	struct platform_device *plat_dev;
+	struct platform_device_info plat_info;
+	struct macb_platform_data plat_data;
+	struct resource res[2];
+
+	/* sanity check */
+	if (!id)
+		return -EINVAL;
+
+	/* enable pci device */
+	err = pci_enable_device(pdev);
+	if (err < 0) {
+		dev_err(&pdev->dev, "Enabling PCI device has failed: 0x%04X",
+			err);
+		return -EACCES;
+	}
+
+	pci_set_master(pdev);
+
+	/* set up resources */
+	memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));
+	res[0].start = pdev->resource[0].start;
+	res[0].end = pdev->resource[0].end;
+	res[0].name = PCI_DRIVER_NAME;
+	res[0].flags = IORESOURCE_MEM;
+	res[1].start = pdev->irq;
+	res[1].name = PCI_DRIVER_NAME;
+	res[1].flags = IORESOURCE_IRQ;
+
+	dev_info(&pdev->dev, "EMAC physical base addr = 0x%p\n",
+		 (void *)(uintptr_t)pci_resource_start(pdev, 0));
+
+	/* set up macb platform data */
+	memset(&plat_data, 0, sizeof(plat_data));
+
+	/* initialize clocks */
+	plat_data.pclk = clk_register_fixed_rate(&pdev->dev, "pclk", NULL, 0,
+						 GEM_PCLK_RATE);
+	if (IS_ERR(plat_data.pclk)) {
+		err = PTR_ERR(plat_data.pclk);
+		goto err_pclk_register;
+	}
+
+	plat_data.hclk = clk_register_fixed_rate(&pdev->dev, "hclk", NULL, 0,
+						 GEM_HCLK_RATE);
+	if (IS_ERR(plat_data.hclk)) {
+		err = PTR_ERR(plat_data.hclk);
+		goto err_hclk_register;
+	}
+
+	/* set up platform device info */
+	memset(&plat_info, 0, sizeof(plat_info));
+	plat_info.parent = &pdev->dev;
+	plat_info.fwnode = pdev->dev.fwnode;
+	plat_info.name = PLAT_DRIVER_NAME;
+	plat_info.id = pdev->devfn;
+	plat_info.res = res;
+	plat_info.num_res = ARRAY_SIZE(res);
+	plat_info.data = &plat_data;
+	plat_info.size_data = sizeof(plat_data);
+	plat_info.dma_mask = DMA_BIT_MASK(32);
+
+	/* register platform device */
+	plat_dev = platform_device_register_full(&plat_info);
+	if (IS_ERR(plat_dev)) {
+		err = PTR_ERR(plat_dev);
+		goto err_plat_dev_register;
+	}
+
+	pci_set_drvdata(pdev, plat_dev);
+
+	return 0;
+
+err_plat_dev_register:
+	clk_unregister(plat_data.hclk);
+
+err_hclk_register:
+	clk_unregister(plat_data.pclk);
+
+err_pclk_register:
+	pci_disable_device(pdev);
+	return err;
+}
+
+static void macb_remove(struct pci_dev *pdev)
+{
+	struct platform_device *plat_dev = pci_get_drvdata(pdev);
+	struct macb_platform_data *plat_data = dev_get_platdata(&plat_dev->dev);
+
+	platform_device_unregister(plat_dev);
+	pci_disable_device(pdev);
+	clk_unregister(plat_data->pclk);
+	clk_unregister(plat_data->hclk);
+}
+
+static struct pci_device_id dev_id_table[] = {
+	{ PCI_DEVICE(CDNS_VENDOR_ID, CDNS_DEVICE_ID), },
+	{ 0, }
+};
+
+static struct pci_driver macb_pci_driver = {
+	.name     = PCI_DRIVER_NAME,
+	.id_table = dev_id_table,
+	.probe    = macb_probe,
+	.remove	  = macb_remove,
+};
+
+module_pci_driver(macb_pci_driver);
+MODULE_DEVICE_TABLE(pci, dev_id_table);
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Cadence NIC PCI wrapper");
diff --git a/include/linux/platform_data/macb.h b/include/linux/platform_data/macb.h
index 21b15f6..7815d50 100644
--- a/include/linux/platform_data/macb.h
+++ b/include/linux/platform_data/macb.h
@@ -8,6 +8,8 @@
 #ifndef __MACB_PDATA_H__
 #define __MACB_PDATA_H__
 
+#include <linux/clk.h>
+
 /**
  * struct macb_platform_data - platform data for MACB Ethernet
  * @phy_mask:		phy mask passed when register the MDIO bus
@@ -15,12 +17,16 @@
  * @phy_irq_pin:	PHY IRQ
  * @is_rmii:		using RMII interface?
  * @rev_eth_addr:	reverse Ethernet address byte order
+ * @pclk:		platform clock
+ * @hclk:		AHB clock
  */
 struct macb_platform_data {
 	u32		phy_mask;
 	int		phy_irq_pin;
 	u8		is_rmii;
 	u8		rev_eth_addr;
+	struct clk	*pclk;
+	struct clk	*hclk;
 };
 
 #endif /* __MACB_PDATA_H__ */
-- 
1.9.1

Re: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[David Miller]

From: Bartosz Folta <bfolta@cadence.com>
Date: Wed, 14 Dec 2016 06:39:15 +0000

> There are hardware PCI implementations of Cadence GEM network
> controller. This patch will allow to use such hardware with reuse of
> existing Platform Driver.
> 
> Signed-off-by: Bartosz Folta <bfolta@cadence.com>
> ---
> Changed in v3:
> Fixed dependencies in Kconfig.
> ---
> Changed in v2:
> Respin to net-next. Changed patch formatting.

Applied.

Re: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Andy Shevchenko]

On Wed, Dec 14, 2016 at 8:39 AM, Bartosz Folta <bfolta@cadence.com> wrote:
> There are hardware PCI implementations of Cadence GEM network
> controller. This patch will allow to use such hardware with reuse of
> existing Platform Driver.

Since it's already applied, perhaps you would consider addressing my
below comments as follow up clean up.

> +++ b/drivers/net/ethernet/cadence/macb_pci.c
> @@ -0,0 +1,153 @@
> +/**

> + * macb_pci.c - Cadence GEM PCI wrapper.

If at some point file is renamed it will be a bit more work to change
this line. Perhaps drop file name?

> +static int macb_probe(struct pci_dev *pdev, const struct pci_device_id *id)
> +{
> +       int err;
> +       struct platform_device *plat_dev;
> +       struct platform_device_info plat_info;
> +       struct macb_platform_data plat_data;
> +       struct resource res[2];
> +

> +       /* sanity check */
> +       if (!id)
> +               return -EINVAL;

Is it even possible in modern kernels?

> +
> +       /* enable pci device */
> +       err = pci_enable_device(pdev);

pcim_enable_device();

> +       if (err < 0) {
> +               dev_err(&pdev->dev, "Enabling PCI device has failed: 0x%04X",

%x for error code?! Confusing.

> +                       err);

> +               return -EACCES;

return err;

> +       }
> +
> +       pci_set_master(pdev);
> +
> +       /* set up resources */
> +       memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));

> +       res[0].start = pdev->resource[0].start;
> +       res[0].end = pdev->resource[0].end;

pci_resource_start()
pci_resource_end()

> +       res[0].name = PCI_DRIVER_NAME;
> +       res[0].flags = IORESOURCE_MEM;

> +       res[1].start = pdev->irq;

Btw, does it support MSI? If so and are planning to use it,
pci_irq_...() API would help here.

> +       res[1].name = PCI_DRIVER_NAME;
> +       res[1].flags = IORESOURCE_IRQ;
> +
> +       dev_info(&pdev->dev, "EMAC physical base addr = 0x%p\n",

> +                (void *)(uintptr_t)pci_resource_start(pdev, 0));

Oh, %pa

> +
> +       /* set up macb platform data */
> +       memset(&plat_data, 0, sizeof(plat_data));
> +
> +       /* initialize clocks */
> +       plat_data.pclk = clk_register_fixed_rate(&pdev->dev, "pclk", NULL, 0,
> +                                                GEM_PCLK_RATE);
> +       if (IS_ERR(plat_data.pclk)) {
> +               err = PTR_ERR(plat_data.pclk);
> +               goto err_pclk_register;
> +       }
> +
> +       plat_data.hclk = clk_register_fixed_rate(&pdev->dev, "hclk", NULL, 0,
> +                                                GEM_HCLK_RATE);
> +       if (IS_ERR(plat_data.hclk)) {
> +               err = PTR_ERR(plat_data.hclk);
> +               goto err_hclk_register;
> +       }
> +
> +       /* set up platform device info */
> +       memset(&plat_info, 0, sizeof(plat_info));
> +       plat_info.parent = &pdev->dev;
> +       plat_info.fwnode = pdev->dev.fwnode;
> +       plat_info.name = PLAT_DRIVER_NAME;
> +       plat_info.id = pdev->devfn;
> +       plat_info.res = res;
> +       plat_info.num_res = ARRAY_SIZE(res);
> +       plat_info.data = &plat_data;
> +       plat_info.size_data = sizeof(plat_data);
> +       plat_info.dma_mask = DMA_BIT_MASK(32);
> +
> +       /* register platform device */
> +       plat_dev = platform_device_register_full(&plat_info);
> +       if (IS_ERR(plat_dev)) {
> +               err = PTR_ERR(plat_dev);
> +               goto err_plat_dev_register;
> +       }
> +
> +       pci_set_drvdata(pdev, plat_dev);
> +
> +       return 0;
> +
> +err_plat_dev_register:
> +       clk_unregister(plat_data.hclk);
> +
> +err_hclk_register:
> +       clk_unregister(plat_data.pclk);
> +

> +err_pclk_register:
> +       pci_disable_device(pdev);

Redundant when use pcim_enable_device().

> +       return err;
> +}
> +
> +static void macb_remove(struct pci_dev *pdev)
> +{
> +       struct platform_device *plat_dev = pci_get_drvdata(pdev);
> +       struct macb_platform_data *plat_data = dev_get_platdata(&plat_dev->dev);
> +
> +       platform_device_unregister(plat_dev);

> +       pci_disable_device(pdev);

Ditto.

> +       clk_unregister(plat_data->pclk);
> +       clk_unregister(plat_data->hclk);
> +}


-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Bartosz Folta]

Please find link to patch holding changes that you suggested:
https://lkml.kernel.org/r/SN1PR0701MB1951B994F86160A24C436F30CC930@SN1PR0701MB1951.namprd07.prod.outlook.com

Here is alternative link to mentioned patch.
https://patchwork.ozlabs.org/patch/707720/

I think that there is a delay on some mailing lists (marc.info and
mail-archive.com). Latest messages I found there are from December
16th 2016.

Regards,
Bartosz Folta

Re: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Andy Shevchenko]

On Wed, Dec 21, 2016 at 1:53 PM, Bartosz Folta <bfolta@cadence.com> wrote:
> Please find link to patch holding changes that you suggested:
> https://lkml.kernel.org/r/SN1PR0701MB1951B994F86160A24C436F30CC930@SN1PR0701MB1951.namprd07.prod.outlook.com
>
> Here is alternative link to mentioned patch.
> https://patchwork.ozlabs.org/patch/707720/

Thanks!

>
> I think that there is a delay on some mailing lists (marc.info and
> mail-archive.com). Latest messages I found there are from December
> 16th 2016.
>

You may refer to
https://www.spinics.net/lists/netdev/msg410864.html

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Andy Shevchenko]

On Wed, Dec 21, 2016 at 2:16 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Wed, Dec 21, 2016 at 1:53 PM, Bartosz Folta <bfolta@cadence.com> wrote:

>> I think that there is a delay on some mailing lists (marc.info and
>> mail-archive.com). Latest messages I found there are from December
>> 16th 2016.
>
> You may refer to
> https://www.spinics.net/lists/netdev/msg410864.html

And one more

+       plat_info.dma_mask = DMA_BIT_MASK(32);

Perhaps
 plat_info.dma_mask = pdev->dma_mask;
?

-- 
With Best Regards,
Andy Shevchenko

RE: [PATCH v3] net: macb: Added PCI wrapper for Platform Driver.

[Bartosz Folta]

> And one more
>
> +       plat_info.dma_mask = DMA_BIT_MASK(32);
>
> Perhaps
>  plat_info.dma_mask = pdev->dma_mask;
> ?

Thank you.

I tested your suggestion. I have sent second version of patch.
https://patchwork.ozlabs.org/patch/707800/

Regards,
Bartosz Folta