[PATCH net-next 0/3] net: ethernet: bgmac: PM support and clean-ups

[PATCH net-next 0/3] net: ethernet: bgmac: PM support and clean-ups

[Jon Mason]

Add code to support Power Management (only tested on NS2), and add some
code clean-ups

Joey Zhong (1):
  net: ethernet: bgmac: driver power manangement

Jon Mason (2):
  net: ethernet: bgmac: use #defines for MAX size
  net: ethernet: bgmac: unify code of the same family

 drivers/net/ethernet/broadcom/bgmac-bcma.c     | 64 +++++++++++---------------
 drivers/net/ethernet/broadcom/bgmac-platform.c | 34 ++++++++++++++
 drivers/net/ethernet/broadcom/bgmac.c          | 53 +++++++++++++++++++++
 drivers/net/ethernet/broadcom/bgmac.h          |  4 +-
 4 files changed, 118 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH 1/3] net: ethernet: bgmac: use #defines for MAX size

[Jon Mason]

The maximum frame size is really just the standard ethernet frame size
and FCS.  So use those existing defines to make the code a little more
beautiful.

Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 6d0b5b3..5a518fe 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -402,7 +402,7 @@
 
 #define BGMAC_WEIGHT	64
 
-#define ETHER_MAX_LEN   1518
+#define ETHER_MAX_LEN	(ETH_FRAME_LEN + ETH_FCS_LEN)
 
 /* Feature Flags */
 #define BGMAC_FEAT_TX_MASK_SETUP	BIT(0)
-- 
2.7.4

[PATCH 2/3] net: ethernet: bgmac: unify code of the same family

[Jon Mason]

BCM471X and BCM535X are of the same family (from what I can derive from
internal documents).  Group them into the case statement together, which
results in more code reuse.

Also, use existing helper variables to make the code a little more
readable too.

Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac-bcma.c | 64 +++++++++++++-----------------
 1 file changed, 28 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bgmac-bcma.c b/drivers/net/ethernet/broadcom/bgmac-bcma.c
index 5ef60d4..f5c27f4 100644
--- a/drivers/net/ethernet/broadcom/bgmac-bcma.c
+++ b/drivers/net/ethernet/broadcom/bgmac-bcma.c
@@ -192,36 +192,50 @@ static int bgmac_probe(struct bcma_device *core)
 		goto err1;
 	}
 
-	bgmac->has_robosw = !!(core->bus->sprom.boardflags_lo &
-			       BGMAC_BFL_ENETROBO);
+	bgmac->has_robosw = !!(sprom->boardflags_lo & BGMAC_BFL_ENETROBO);
 	if (bgmac->has_robosw)
 		dev_warn(bgmac->dev, "Support for Roboswitch not implemented\n");
 
-	if (core->bus->sprom.boardflags_lo & BGMAC_BFL_ENETADM)
+	if (sprom->boardflags_lo & BGMAC_BFL_ENETADM)
 		dev_warn(bgmac->dev, "Support for ADMtek ethernet switch not implemented\n");
 
 	/* Feature Flags */
-	switch (core->bus->chipinfo.id) {
+	switch (ci->id) {
+	/* BCM 471X/535X family */
+	case BCMA_CHIP_ID_BCM4716:
+		bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
+		/* fallthrough */
+	case BCMA_CHIP_ID_BCM47162:
+		bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL2;
+		bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+		break;
 	case BCMA_CHIP_ID_BCM5357:
+	case BCMA_CHIP_ID_BCM53572:
 		bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
 		bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
 		bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
 		bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-		if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM47186) {
-			bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+		if (ci->pkg == BCMA_PKG_ID_BCM47188 ||
+		    ci->pkg == BCMA_PKG_ID_BCM47186) {
 			bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
+			bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
 		}
-		if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM5358)
+		if (ci->pkg == BCMA_PKG_ID_BCM5358)
 			bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_EPHYRMII;
 		break;
-	case BCMA_CHIP_ID_BCM53572:
-		bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+	case BCMA_CHIP_ID_BCM53573:
 		bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
-		bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
-		bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-		if (core->bus->chipinfo.pkg == BCMA_PKG_ID_BCM47188) {
-			bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
+		bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
+		if (ci->pkg == BCMA_PKG_ID_BCM47189)
 			bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
+		if (core->core_unit == 0) {
+			bgmac->feature_flags |= BGMAC_FEAT_CC4_IF_SW_TYPE;
+			if (ci->pkg == BCMA_PKG_ID_BCM47189)
+				bgmac->feature_flags |=
+					BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII;
+		} else if (core->core_unit == 1) {
+			bgmac->feature_flags |= BGMAC_FEAT_IRQ_ID_OOB_6;
+			bgmac->feature_flags |= BGMAC_FEAT_CC7_IF_TYPE_RGMII;
 		}
 		break;
 	case BCMA_CHIP_ID_BCM4749:
@@ -229,18 +243,11 @@ static int bgmac_probe(struct bcma_device *core)
 		bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
 		bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL1;
 		bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_PHY;
-		if (core->bus->chipinfo.pkg == 10) {
+		if (ci->pkg == 10) {
 			bgmac->feature_flags |= BGMAC_FEAT_SW_TYPE_RGMII;
 			bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
 		}
 		break;
-	case BCMA_CHIP_ID_BCM4716:
-		bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
-		/* fallthrough */
-	case BCMA_CHIP_ID_BCM47162:
-		bgmac->feature_flags |= BGMAC_FEAT_FLW_CTRL2;
-		bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
-		break;
 	/* bcm4707_family */
 	case BCMA_CHIP_ID_BCM4707:
 	case BCMA_CHIP_ID_BCM47094:
@@ -249,21 +256,6 @@ static int bgmac_probe(struct bcma_device *core)
 		bgmac->feature_flags |= BGMAC_FEAT_NO_RESET;
 		bgmac->feature_flags |= BGMAC_FEAT_FORCE_SPEED_2500;
 		break;
-	case BCMA_CHIP_ID_BCM53573:
-		bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
-		bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
-		if (ci->pkg == BCMA_PKG_ID_BCM47189)
-			bgmac->feature_flags |= BGMAC_FEAT_IOST_ATTACHED;
-		if (core->core_unit == 0) {
-			bgmac->feature_flags |= BGMAC_FEAT_CC4_IF_SW_TYPE;
-			if (ci->pkg == BCMA_PKG_ID_BCM47189)
-				bgmac->feature_flags |=
-					BGMAC_FEAT_CC4_IF_SW_TYPE_RGMII;
-		} else if (core->core_unit == 1) {
-			bgmac->feature_flags |= BGMAC_FEAT_IRQ_ID_OOB_6;
-			bgmac->feature_flags |= BGMAC_FEAT_CC7_IF_TYPE_RGMII;
-		}
-		break;
 	default:
 		bgmac->feature_flags |= BGMAC_FEAT_CLKCTLST;
 		bgmac->feature_flags |= BGMAC_FEAT_SET_RXQ_CLK;
-- 
2.7.4

Re: [PATCH 2/3] net: ethernet: bgmac: unify code of the same family

[Rafał Miłecki]

On 2017-02-03 22:39, Jon Mason wrote:
> BCM471X and BCM535X are of the same family (from what I can derive from
> internal documents).  Group them into the case statement together, 
> which
> results in more code reuse.
> 
> Also, use existing helper variables to make the code a little more
> readable too.
> 
> Signed-off-by: Jon Mason <jon.mason@broadcom.com>

I'd like to review it / test it on few devices. Please give me weekend 
for
that.

Re: [PATCH 2/3] net: ethernet: bgmac: unify code of the same family

[Jon Mason]

On Fri, Feb 3, 2017 at 4:48 PM, Rafał Miłecki <rafal@milecki.pl> wrote:
> On 2017-02-03 22:39, Jon Mason wrote:
>>
>> BCM471X and BCM535X are of the same family (from what I can derive from
>> internal documents).  Group them into the case statement together, which
>> results in more code reuse.
>>
>> Also, use existing helper variables to make the code a little more
>> readable too.
>>
>> Signed-off-by: Jon Mason <jon.mason@broadcom.com>
>
>
> I'd like to review it / test it on few devices. Please give me weekend for
> that.

Yes, please test this as much as you can.  The code move was pretty
innocuous, and those are always the times when it comes back to bite
me.

Thanks,
Jon

[PATCH 3/3] net: ethernet: bgmac: driver power manangement

[Jon Mason]

From: Joey Zhong <zhongx@broadcom.com>

Implements suspend/resume, external phy 54810 is assumed
to remain powered up during deep-sleep for wake-on-lane.

Signed-off-by: Joey Zhong <zhongx@broadcom.com>
Signed-off-by: Jon Mason <jon.mason@broadcom.com>
---
 drivers/net/ethernet/broadcom/bgmac-platform.c | 34 +++++++++++++++++
 drivers/net/ethernet/broadcom/bgmac.c          | 53 ++++++++++++++++++++++++++
 drivers/net/ethernet/broadcom/bgmac.h          |  2 +
 3 files changed, 89 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bgmac-platform.c b/drivers/net/ethernet/broadcom/bgmac-platform.c
index 2d153f7..3df91e7 100644
--- a/drivers/net/ethernet/broadcom/bgmac-platform.c
+++ b/drivers/net/ethernet/broadcom/bgmac-platform.c
@@ -21,8 +21,12 @@
 #include <linux/of_net.h>
 #include "bgmac.h"
 
+#define NICPM_PADRING_CFG		0x00000004
 #define NICPM_IOMUX_CTRL		0x00000008
 
+#define NICPM_PADRING_CFG_INIT_VAL	0x74000000
+#define NICPM_IOMUX_CTRL_INIT_VAL_AX	0x21880000
+
 #define NICPM_IOMUX_CTRL_INIT_VAL	0x3196e000
 #define NICPM_IOMUX_CTRL_SPD_SHIFT	10
 #define NICPM_IOMUX_CTRL_SPD_10M	0
@@ -108,6 +112,10 @@ static void bgmac_nicpm_speed_set(struct net_device *net_dev)
 	if (!bgmac->plat.nicpm_base)
 		return;
 
+	/* SET RGMII IO CONFIG */
+	writel(NICPM_PADRING_CFG_INIT_VAL,
+	       bgmac->plat.nicpm_base + NICPM_PADRING_CFG);
+
 	val = NICPM_IOMUX_CTRL_INIT_VAL;
 	switch (bgmac->net_dev->phydev->speed) {
 	default:
@@ -239,6 +247,31 @@ static int bgmac_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int bgmac_suspend(struct device *dev)
+{
+	struct bgmac *bgmac = dev_get_drvdata(dev);
+
+	return bgmac_enet_suspend(bgmac);
+}
+
+static int bgmac_resume(struct device *dev)
+{
+	struct bgmac *bgmac = dev_get_drvdata(dev);
+
+	return bgmac_enet_resume(bgmac);
+}
+
+static const struct dev_pm_ops bgmac_pm_ops = {
+	.suspend = bgmac_suspend,
+	.resume = bgmac_resume
+};
+
+#define BGMAC_PM_OPS (&bgmac_pm_ops)
+#else
+#define BGMAC_PM_OPS NULL
+#endif /* CONFIG_PM */
+
 static const struct of_device_id bgmac_of_enet_match[] = {
 	{.compatible = "brcm,amac",},
 	{.compatible = "brcm,nsp-amac",},
@@ -252,6 +285,7 @@ static struct platform_driver bgmac_enet_driver = {
 	.driver = {
 		.name  = "bgmac-enet",
 		.of_match_table = bgmac_of_enet_match,
+		.pm = BGMAC_PM_OPS
 	},
 	.probe = bgmac_probe,
 	.remove = bgmac_remove,
diff --git a/drivers/net/ethernet/broadcom/bgmac.c b/drivers/net/ethernet/broadcom/bgmac.c
index bd549f8..8d3aada 100644
--- a/drivers/net/ethernet/broadcom/bgmac.c
+++ b/drivers/net/ethernet/broadcom/bgmac.c
@@ -1478,6 +1478,7 @@ int bgmac_enet_probe(struct bgmac *bgmac)
 
 	net_dev->irq = bgmac->irq;
 	SET_NETDEV_DEV(net_dev, bgmac->dev);
+	dev_set_drvdata(bgmac->dev, bgmac);
 
 	if (!is_valid_ether_addr(bgmac->mac_addr)) {
 		dev_err(bgmac->dev, "Invalid MAC addr: %pM\n",
@@ -1551,5 +1552,57 @@ void bgmac_enet_remove(struct bgmac *bgmac)
 }
 EXPORT_SYMBOL_GPL(bgmac_enet_remove);
 
+int bgmac_enet_suspend(struct bgmac *bgmac)
+{
+	netdev_info(bgmac->net_dev, "Suspending\n");
+
+	if (netif_running(bgmac->net_dev)) {
+		netif_stop_queue(bgmac->net_dev);
+
+		napi_disable(&bgmac->napi);
+
+		netif_tx_lock(bgmac->net_dev);
+		netif_device_detach(bgmac->net_dev);
+		netif_tx_unlock(bgmac->net_dev);
+
+		bgmac_chip_intrs_off(bgmac);
+		bgmac_chip_reset(bgmac);
+		bgmac_dma_cleanup(bgmac);
+	}
+
+	phy_stop(bgmac->net_dev->phydev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bgmac_enet_suspend);
+
+int bgmac_enet_resume(struct bgmac *bgmac)
+{
+	int rc;
+
+	netdev_info(bgmac->net_dev, "Resuming\n");
+
+	phy_start(bgmac->net_dev->phydev);
+
+	if (netif_running(bgmac->net_dev)) {
+		rc = bgmac_dma_init(bgmac);
+		if (rc)
+			return rc;
+
+		bgmac_chip_init(bgmac);
+
+		napi_enable(&bgmac->napi);
+
+		netif_tx_lock(bgmac->net_dev);
+		netif_device_attach(bgmac->net_dev);
+		netif_tx_unlock(bgmac->net_dev);
+
+		netif_start_queue(bgmac->net_dev);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(bgmac_enet_resume);
+
 MODULE_AUTHOR("Rafał Miłecki");
 MODULE_LICENSE("GPL");
diff --git a/drivers/net/ethernet/broadcom/bgmac.h b/drivers/net/ethernet/broadcom/bgmac.h
index 5a518fe..741ca27 100644
--- a/drivers/net/ethernet/broadcom/bgmac.h
+++ b/drivers/net/ethernet/broadcom/bgmac.h
@@ -538,6 +538,8 @@ int bgmac_enet_probe(struct bgmac *bgmac);
 void bgmac_enet_remove(struct bgmac *bgmac);
 void bgmac_adjust_link(struct net_device *net_dev);
 int bgmac_phy_connect_direct(struct bgmac *bgmac);
+int bgmac_enet_suspend(struct bgmac *bgmac);
+int bgmac_enet_resume(struct bgmac *bgmac);
 
 struct mii_bus *bcma_mdio_mii_register(struct bgmac *bgmac);
 void bcma_mdio_mii_unregister(struct mii_bus *mii_bus);
-- 
2.7.4

Re: [PATCH 3/3] net: ethernet: bgmac: driver power manangement

[Florian Fainelli]

On 02/03/2017 01:39 PM, Jon Mason wrote:
> From: Joey Zhong <zhongx@broadcom.com>
> 
> Implements suspend/resume, external phy 54810 is assumed
> to remain powered up during deep-sleep for wake-on-lane.

s/wake-on-lane/Wake-on-LAN, are you positive phy_stop() is not
suspending the PHY and issuing BMCR_PWRDOWN write?

This also seems incomplete in that, if the device is really configured
for Wake-on-LAN (through ethtool) you should call
device_set_wakeup_capable() and then check for device_may_wakeup()
during suspend or resume to know which part of the suspend/resume
portion should be done. You could refer to bcmgenet for an example.

>  
> +int bgmac_enet_suspend(struct bgmac *bgmac)
> +{
> +	netdev_info(bgmac->net_dev, "Suspending\n");

remove that message

> +
> +	if (netif_running(bgmac->net_dev)) {
> +		netif_stop_queue(bgmac->net_dev);
> +
> +		napi_disable(&bgmac->napi);
> +
> +		netif_tx_lock(bgmac->net_dev);
> +		netif_device_detach(bgmac->net_dev);
> +		netif_tx_unlock(bgmac->net_dev);
> +
> +		bgmac_chip_intrs_off(bgmac);
> +		bgmac_chip_reset(bgmac);
> +		bgmac_dma_cleanup(bgmac);
> +	}

Can you change the indentation to test for netiff_running() first and
return 0 in that case?

> +
> +	phy_stop(bgmac->net_dev->phydev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(bgmac_enet_suspend);
> +
> +int bgmac_enet_resume(struct bgmac *bgmac)
> +{
> +	int rc;
> +
> +	netdev_info(bgmac->net_dev, "Resuming\n");

Same here, this needs to be removed.

> +
> +	phy_start(bgmac->net_dev->phydev);
> +
> +	if (netif_running(bgmac->net_dev)) {
> +		rc = bgmac_dma_init(bgmac);
> +		if (rc)
> +			return rc;
> +
> +		bgmac_chip_init(bgmac);
> +
> +		napi_enable(&bgmac->napi);
> +
> +		netif_tx_lock(bgmac->net_dev);
> +		netif_device_attach(bgmac->net_dev);
> +		netif_tx_unlock(bgmac->net_dev);
> +
> +		netif_start_queue(bgmac->net_dev);
> +	}
-- 
Florian

Re: [PATCH 3/3] net: ethernet: bgmac: driver power manangement

[Jon Mason]

On Fri, Feb 3, 2017 at 9:16 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 02/03/2017 01:39 PM, Jon Mason wrote:
>> From: Joey Zhong <zhongx@broadcom.com>
>>
>> Implements suspend/resume, external phy 54810 is assumed
>> to remain powered up during deep-sleep for wake-on-lane.
>
> s/wake-on-lane/Wake-on-LAN, are you positive phy_stop() is not
> suspending the PHY and issuing BMCR_PWRDOWN write?
>
> This also seems incomplete in that, if the device is really configured
> for Wake-on-LAN (through ethtool) you should call
> device_set_wakeup_capable() and then check for device_may_wakeup()
> during suspend or resume to know which part of the suspend/resume
> portion should be done. You could refer to bcmgenet for an example.

After some internal discussion, WOL is not supported by our SVK.  So,
we have no way of testing it.  Given this limitation, I'm removing the
WOL comment until such time as we can actually test the logic.

>>
>> +int bgmac_enet_suspend(struct bgmac *bgmac)
>> +{
>> +     netdev_info(bgmac->net_dev, "Suspending\n");
>
> remove that message
>
>> +
>> +     if (netif_running(bgmac->net_dev)) {
>> +             netif_stop_queue(bgmac->net_dev);
>> +
>> +             napi_disable(&bgmac->napi);
>> +
>> +             netif_tx_lock(bgmac->net_dev);
>> +             netif_device_detach(bgmac->net_dev);
>> +             netif_tx_unlock(bgmac->net_dev);
>> +
>> +             bgmac_chip_intrs_off(bgmac);
>> +             bgmac_chip_reset(bgmac);
>> +             bgmac_dma_cleanup(bgmac);
>> +     }
>
> Can you change the indentation to test for netiff_running() first and
> return 0 in that case?
>
>> +
>> +     phy_stop(bgmac->net_dev->phydev);
>> +
>> +     return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(bgmac_enet_suspend);
>> +
>> +int bgmac_enet_resume(struct bgmac *bgmac)
>> +{
>> +     int rc;
>> +
>> +     netdev_info(bgmac->net_dev, "Resuming\n");
>
> Same here, this needs to be removed.

Will do this and above.

Thanks,
Jon

>
>> +
>> +     phy_start(bgmac->net_dev->phydev);
>> +
>> +     if (netif_running(bgmac->net_dev)) {
>> +             rc = bgmac_dma_init(bgmac);
>> +             if (rc)
>> +                     return rc;
>> +
>> +             bgmac_chip_init(bgmac);
>> +
>> +             napi_enable(&bgmac->napi);
>> +
>> +             netif_tx_lock(bgmac->net_dev);
>> +             netif_device_attach(bgmac->net_dev);
>> +             netif_tx_unlock(bgmac->net_dev);
>> +
>> +             netif_start_queue(bgmac->net_dev);
>> +     }
> --
> Florian