Discussion:
[PATCH 0/5] stmmac: pci: various cleanups and fixes
Andy Shevchenko
2014-10-21 16:35:28 UTC
Permalink
There are few cleanups and fixes regarding to stmmac PCI driver.
This has been tested on Intel Galileo board with 3.18-rc1 kernel.

Andy Shevchenko (5):
stmmac: pci: convert to use dev_pm_ops
stmmac: pci: use managed resources
stmmac: pci: convert to use dev_* macros
stmmac: pci: set default filter bins
stmmac: pci: remove FSF address

drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 92 ++++++++----------------
1 file changed, 30 insertions(+), 62 deletions(-)
--
2.1.1
Andy Shevchenko
2014-10-21 16:35:32 UTC
Permalink
The commit 3b57de958e2a bring a support for different amount of filter bins,
but didn't update PCI driver accordingly. This patch append default values when
device is enumerated via PCI bus.

Fixes: 3b57de958e2a (net: stmmac: Support devicetree configs for mcast and ucast filter entries)
Signed-off-by: Andy Shevchenko <***@linux.intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index c16f74b..22cb5ff 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -67,6 +67,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
int ret = 0;
struct stmmac_priv *priv = NULL;
int pci_bar = 0;
+ struct plat_stmmacenet_data *plat = &plat_dat;

/* Enable pci device */
ret = pcim_enable_device(pdev);
@@ -83,7 +84,13 @@ static int stmmac_pci_probe(struct pci_dev *pdev,

stmmac_default_data();

- priv = stmmac_dvr_probe(&pdev->dev, &plat_dat,
+ /* Set default value for multicast hash bins */
+ plat->multicast_filter_bins = HASH_TABLE_SIZE;
+
+ /* Set default value for unicast filter entries */
+ plat->unicast_filter_entries = 1;
+
+ priv = stmmac_dvr_probe(&pdev->dev, plat,
pcim_iomap_table(pdev)[pci_bar]);
if (IS_ERR(priv)) {
dev_err(&pdev->dev, "%s: main driver probe failed\n", __func__);
--
2.1.1
Andy Shevchenko
2014-10-21 16:35:29 UTC
Permalink
Convert system PM callbacks to use dev_pm_ops. In addition remove the PCI calls
related to a power state since the bus code cares about this already.

Signed-off-by: Andy Shevchenko <***@linux.intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 27 ++++++++++--------------
1 file changed, 11 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 655a23b..5459a4e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -143,30 +143,26 @@ static void stmmac_pci_remove(struct pci_dev *pdev)
pci_disable_device(pdev);
}

-#ifdef CONFIG_PM
-static int stmmac_pci_suspend(struct pci_dev *pdev, pm_message_t state)
+#ifdef CONFIG_PM_SLEEP
+static int stmmac_pci_suspend(struct device *dev)
{
+ struct pci_dev *pdev = to_pci_dev(dev);
struct net_device *ndev = pci_get_drvdata(pdev);
- int ret;

- ret = stmmac_suspend(ndev);
- pci_save_state(pdev);
- pci_set_power_state(pdev, pci_choose_state(pdev, state));
-
- return ret;
+ return stmmac_suspend(ndev);
}

-static int stmmac_pci_resume(struct pci_dev *pdev)
+static int stmmac_pci_resume(struct device *dev)
{
+ struct pci_dev *pdev = to_pci_dev(dev);
struct net_device *ndev = pci_get_drvdata(pdev);

- pci_set_power_state(pdev, PCI_D0);
- pci_restore_state(pdev);
-
return stmmac_resume(ndev);
}
#endif

+static SIMPLE_DEV_PM_OPS(stmmac_pm_ops, stmmac_pci_suspend, stmmac_pci_resume);
+
#define STMMAC_VENDOR_ID 0x700
#define STMMAC_DEVICE_ID 0x1108

@@ -183,10 +179,9 @@ struct pci_driver stmmac_pci_driver = {
.id_table = stmmac_id_table,
.probe = stmmac_pci_probe,
.remove = stmmac_pci_remove,
-#ifdef CONFIG_PM
- .suspend = stmmac_pci_suspend,
- .resume = stmmac_pci_resume,
-#endif
+ .driver = {
+ .pm = &stmmac_pm_ops,
+ },
};

MODULE_DESCRIPTION("STMMAC 10/100/1000 Ethernet PCI driver");
--
2.1.1
Andy Shevchenko
2014-10-21 16:35:31 UTC
Permalink
Instead of pr_* macros let's use dev_* macros which provide device name.

Signed-off-by: Andy Shevchenko <***@linux.intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index f8d4ce2..c16f74b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -71,8 +71,8 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
/* Enable pci device */
ret = pcim_enable_device(pdev);
if (ret) {
- pr_err("%s : ERROR: failed to enable %s device\n", __func__,
- pci_name(pdev));
+ dev_err(&pdev->dev, "%s: ERROR: failed to enable device\n",
+ __func__);
return ret;
}
ret = pcim_iomap_regions(pdev, BIT(pci_bar), pci_name(pdev));
@@ -86,7 +86,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
priv = stmmac_dvr_probe(&pdev->dev, &plat_dat,
pcim_iomap_table(pdev)[pci_bar]);
if (IS_ERR(priv)) {
- pr_err("%s: main driver probe failed", __func__);
+ dev_err(&pdev->dev, "%s: main driver probe failed\n", __func__);
return PTR_ERR(priv);
}
priv->dev->irq = pdev->irq;
@@ -94,7 +94,7 @@ static int stmmac_pci_probe(struct pci_dev *pdev,

pci_set_drvdata(pdev, priv->dev);

- pr_debug("STMMAC platform driver registration completed");
+ dev_dbg(&pdev->dev, "STMMAC PCI driver registration completed\n");

return 0;
}
--
2.1.1
Andy Shevchenko
2014-10-21 16:35:33 UTC
Permalink
The FSF address is subject to change, thus remove it from the file.

Signed-off-by: Andy Shevchenko <***@linux.intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 4 ----
1 file changed, 4 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 22cb5ff..95eb195 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -12,10 +12,6 @@
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, write to the Free Software Foundation, Inc.,
- 51 Franklin St - Fifth Floor, Boston, MA 02110-1301 USA.
-
The full GNU General Public License is included in this distribution in
the file called "COPYING".
--
2.1.1
Andy Shevchenko
2014-10-21 16:35:30 UTC
Permalink
Migrate pci driver to managed resources to reduce boilerplate error handling
code.

Signed-off-by: Andy Shevchenko <***@linux.intel.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 46 +++++-------------------
1 file changed, 8 insertions(+), 38 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 5459a4e..f8d4ce2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -65,45 +65,29 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
int ret = 0;
- void __iomem *addr = NULL;
struct stmmac_priv *priv = NULL;
- int i;
+ int pci_bar = 0;

/* Enable pci device */
- ret = pci_enable_device(pdev);
+ ret = pcim_enable_device(pdev);
if (ret) {
pr_err("%s : ERROR: failed to enable %s device\n", __func__,
pci_name(pdev));
return ret;
}
- if (pci_request_regions(pdev, STMMAC_RESOURCE_NAME)) {
- pr_err("%s: ERROR: failed to get PCI region\n", __func__);
- ret = -ENODEV;
- goto err_out_req_reg_failed;
- }
+ ret = pcim_iomap_regions(pdev, BIT(pci_bar), pci_name(pdev));
+ if (ret)
+ return ret;

- /* Get the base address of device */
- for (i = 0; i <= 5; i++) {
- if (pci_resource_len(pdev, i) == 0)
- continue;
- addr = pci_iomap(pdev, i, 0);
- if (addr == NULL) {
- pr_err("%s: ERROR: cannot map register memory aborting",
- __func__);
- ret = -EIO;
- goto err_out_map_failed;
- }
- break;
- }
pci_set_master(pdev);

stmmac_default_data();

- priv = stmmac_dvr_probe(&(pdev->dev), &plat_dat, addr);
+ priv = stmmac_dvr_probe(&pdev->dev, &plat_dat,
+ pcim_iomap_table(pdev)[pci_bar]);
if (IS_ERR(priv)) {
pr_err("%s: main driver probe failed", __func__);
- ret = PTR_ERR(priv);
- goto err_out;
+ return PTR_ERR(priv);
}
priv->dev->irq = pdev->irq;
priv->wol_irq = pdev->irq;
@@ -113,15 +97,6 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
pr_debug("STMMAC platform driver registration completed");

return 0;
-
-err_out:
- pci_clear_master(pdev);
-err_out_map_failed:
- pci_release_regions(pdev);
-err_out_req_reg_failed:
- pci_disable_device(pdev);
-
- return ret;
}

/**
@@ -134,13 +109,8 @@ err_out_req_reg_failed:
static void stmmac_pci_remove(struct pci_dev *pdev)
{
struct net_device *ndev = pci_get_drvdata(pdev);
- struct stmmac_priv *priv = netdev_priv(ndev);

stmmac_dvr_remove(ndev);
-
- pci_iounmap(pdev, priv->ioaddr);
- pci_release_regions(pdev);
- pci_disable_device(pdev);
}

#ifdef CONFIG_PM_SLEEP
--
2.1.1
Sergei Shtylyov
2014-10-21 20:55:03 UTC
Permalink
Hello.
Post by Andy Shevchenko
Migrate pci driver to managed resources to reduce boilerplate error handling
code.
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 46 +++++-------------------
1 file changed, 8 insertions(+), 38 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 5459a4e..f8d4ce2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -65,45 +65,29 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
int ret = 0;
- void __iomem *addr = NULL;
struct stmmac_priv *priv = NULL;
- int i;
+ int pci_bar = 0;
I don't see this variable changing anywhere...
Post by Andy Shevchenko
/* Enable pci device */
- ret = pci_enable_device(pdev);
+ ret = pcim_enable_device(pdev);
if (ret) {
pr_err("%s : ERROR: failed to enable %s device\n", __func__,
pci_name(pdev));
return ret;
}
- if (pci_request_regions(pdev, STMMAC_RESOURCE_NAME)) {
- pr_err("%s: ERROR: failed to get PCI region\n", __func__);
- ret = -ENODEV;
- goto err_out_req_reg_failed;
- }
+ ret = pcim_iomap_regions(pdev, BIT(pci_bar), pci_name(pdev));
+ if (ret)
+ return ret;
- /* Get the base address of device */
- for (i = 0; i <= 5; i++) {
- if (pci_resource_len(pdev, i) == 0)
- continue;
- addr = pci_iomap(pdev, i, 0);
- if (addr == NULL) {
- pr_err("%s: ERROR: cannot map register memory aborting",
- __func__);
- ret = -EIO;
- goto err_out_map_failed;
- }
- break;
- }
It's not an equivalent change: the old code mapped a first existing BAR,
you always map BAR0. Are you sure that's what you meant? If so, wouldn't hurt
to describe this in the changelog...

WBR, Sergei
Andy Shevchenko
2014-10-22 08:36:33 UTC
Permalink
Post by Sergei Shtylyov
Hello.
Post by Andy Shevchenko
Migrate pci driver to managed resources to reduce boilerplate error handling
code.
---
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c | 46 +++++-------------------
1 file changed, 8 insertions(+), 38 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index 5459a4e..f8d4ce2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -65,45 +65,29 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
const struct pci_device_id *id)
{
int ret = 0;
- void __iomem *addr = NULL;
struct stmmac_priv *priv = NULL;
- int i;
+ int pci_bar = 0;
I don't see this variable changing anywhere...
See my comment below.
Post by Sergei Shtylyov
Post by Andy Shevchenko
/* Enable pci device */
- ret = pci_enable_device(pdev);
+ ret = pcim_enable_device(pdev);
if (ret) {
pr_err("%s : ERROR: failed to enable %s device\n", __func__,
pci_name(pdev));
return ret;
}
- if (pci_request_regions(pdev, STMMAC_RESOURCE_NAME)) {
- pr_err("%s: ERROR: failed to get PCI region\n", __func__);
- ret = -ENODEV;
- goto err_out_req_reg_failed;
- }
+ ret = pcim_iomap_regions(pdev, BIT(pci_bar), pci_name(pdev));
+ if (ret)
+ return ret;
- /* Get the base address of device */
- for (i = 0; i <= 5; i++) {
- if (pci_resource_len(pdev, i) == 0)
- continue;
- addr = pci_iomap(pdev, i, 0);
- if (addr == NULL) {
- pr_err("%s: ERROR: cannot map register memory aborting",
- __func__);
- ret = -EIO;
- goto err_out_map_failed;
- }
- break;
- }
It's not an equivalent change: the old code mapped a first existing BAR,
you always map BAR0. Are you sure that's what you meant? If so, wouldn't hurt
to describe this in the changelog...
So, I was trying to find any specification on public regarding to boards
that have this IP, no luck so far. I guess that that code was created
due to XILINX FPGA usage which probably can provide any BAR user wants
to. Thus, I imply that in real applications the BAR most probably will
be 0. However, I left variable which can be overridden in future
(regarding to PCI ID).

It would be nice to hear someone from ST about this. Giuseppe?
--
Andy Shevchenko <***@intel.com>
Intel Finland Oy
Loading...