[PATCH net-next 00/12] net: bcmgenet: add support for GENETv5

[PATCH net-next 00/12] net: bcmgenet: add support for GENETv5

[Doug Berger]

This collection of patches contains changes related to adding
support for the BCM7260, BCM7268, and BCM7271 devices that
contain a new version of the GENET MAC IP block (v5) and a new
fast ethernet (10/100BASE-T) internal PHY.

These patches were originally developed on top of the bug fixes
of the "[PATCH v2 net 0/8] net: bcmgenet: minor bug fixes" patch
set previously accepted into the net repository, but this
submission is designed to be applied to the current net-next
that does not yet include them. As a result there will be some
merge conflicts that I would be happy to help resolve if desired.

Specifically, conflicts should occur with these patches from the
minor bug fixes set:
[PATCH v2 net 3/8] net: bcmgenet: reserved phy revisions must be checked first
[PATCH v2 net 5/8] net: bcmgenet: synchronize irq0 status between the isr and task
[PATCH v2 net 8/8] net: bcmgenet: decouple flow control from bcmgenet_tx_reclaim

Doug Berger (12):
  net: phy: bcm-phylib: replace obsolete EEE macro references
  net: phy: bcm7xxx: add support for 28nm EPHY
  net: bcmgenet: simplify circular pointer arithmetic
  net: bcmgenet: remove meaningless lines
  net: bcmgenet: manage dma interrupts in napi code
  net: bcmgenet: remove handling of wol interrupts from isr0
  net: bcmgenet: clear status to reduce spurious interrupts
  net: bcmgenet: correct return value of __bcmgenet_tx_reclaim
  net: bcmgenet: return EOPNOTSUPP for unknown ioctl commands
  dt-bindings: net: document bcmgenet WoL interrupt
  dt-bindings: net: update bcmgenet binding for GENETv5
  net: bcmgenet: add support for the GENETv5 hardware

 .../devicetree/bindings/net/brcm,bcmgenet.txt      |  19 +-
 .../devicetree/bindings/net/brcm,unimac-mdio.txt   |   5 +-
 drivers/net/ethernet/broadcom/genet/bcmgenet.c     | 214 +++++++++++---------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h     |  10 +-
 drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c |  13 --
 drivers/net/ethernet/broadcom/genet/bcmmii.c       |  62 +++---
 drivers/net/phy/bcm-phy-lib.c                      |   6 +-
 drivers/net/phy/bcm7xxx.c                          | 215 ++++++++++++++++++++-
 include/linux/brcmphy.h                            |   3 +
 9 files changed, 403 insertions(+), 144 deletions(-)

-- 
2.11.1

[PATCH net-next 00/12] net: bcmgenet: add support for GENETv5

[Doug Berger]

This collection of patches contains changes related to adding
support for the BCM7260, BCM7268, and BCM7271 devices that
contain a new version of the GENET MAC IP block (v5) and a new
fast ethernet (10/100BASE-T) internal PHY.

These patches were originally developed on top of the bug fixes
of the "[PATCH v2 net 0/8] net: bcmgenet: minor bug fixes" patch
set previously accepted into the net repository, but this
submission is designed to be applied to the current net-next
that does not yet include them. As a result there will be some
merge conflicts that I would be happy to help resolve if desired.

Specifically, conflicts should occur with these patches from the
minor bug fixes set:
[PATCH v2 net 3/8] net: bcmgenet: reserved phy revisions must be checked first
[PATCH v2 net 5/8] net: bcmgenet: synchronize irq0 status between the isr and task
[PATCH v2 net 8/8] net: bcmgenet: decouple flow control from bcmgenet_tx_reclaim

Doug Berger (12):
  net: phy: bcm-phylib: replace obsolete EEE macro references
  net: phy: bcm7xxx: add support for 28nm EPHY
  net: bcmgenet: simplify circular pointer arithmetic
  net: bcmgenet: remove meaningless lines
  net: bcmgenet: manage dma interrupts in napi code
  net: bcmgenet: remove handling of wol interrupts from isr0
  net: bcmgenet: clear status to reduce spurious interrupts
  net: bcmgenet: correct return value of __bcmgenet_tx_reclaim
  net: bcmgenet: return EOPNOTSUPP for unknown ioctl commands
  dt-bindings: net: document bcmgenet WoL interrupt
  dt-bindings: net: update bcmgenet binding for GENETv5
  net: bcmgenet: add support for the GENETv5 hardware

 .../devicetree/bindings/net/brcm,bcmgenet.txt      |  19 +-
 .../devicetree/bindings/net/brcm,unimac-mdio.txt   |   5 +-
 drivers/net/ethernet/broadcom/genet/bcmgenet.c     | 214 +++++++++++---------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h     |  10 +-
 drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c |  13 --
 drivers/net/ethernet/broadcom/genet/bcmmii.c       |  62 +++---
 drivers/net/phy/bcm-phy-lib.c                      |   6 +-
 drivers/net/phy/bcm7xxx.c                          | 215 ++++++++++++++++++++-
 include/linux/brcmphy.h                            |   3 +
 9 files changed, 403 insertions(+), 144 deletions(-)

-- 
2.11.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 01/12] net: phy: bcm-phylib: replace obsolete EEE macro references

[Doug Berger]

The macros MDIO_AN_EEE_ADV_100TX and MDIO_AN_EEE_ADV_1000T are now
considered obsolete and are replaced in the kernel with the generic
macros MDIO_EEE_100TX and MDIO_EEE_1000T respectively.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/phy/bcm-phy-lib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
index ab9ad689617c..9656dbeb5de5 100644
--- a/drivers/net/phy/bcm-phy-lib.c
+++ b/drivers/net/phy/bcm-phy-lib.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Broadcom Corporation
+ * Copyright (C) 2015-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License as
@@ -221,9 +221,9 @@ int bcm_phy_set_eee(struct phy_device *phydev, bool enable)
 		return val;
 
 	if (enable)
-		val |= (MDIO_AN_EEE_ADV_100TX | MDIO_AN_EEE_ADV_1000T);
+		val |= (MDIO_EEE_100TX | MDIO_EEE_1000T);
 	else
-		val &= ~(MDIO_AN_EEE_ADV_100TX | MDIO_AN_EEE_ADV_1000T);
+		val &= ~(MDIO_EEE_100TX | MDIO_EEE_1000T);
 
 	phy_write_mmd_indirect(phydev, BCM_CL45VEN_EEE_ADV,
 			       MDIO_MMD_AN, (u32)val);
-- 
2.11.1

Re: [PATCH net-next 01/12] net: phy: bcm-phylib: replace obsolete EEE macro references

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> The macros MDIO_AN_EEE_ADV_100TX and MDIO_AN_EEE_ADV_1000T are now
> considered obsolete and are replaced in the kernel with the generic
> macros MDIO_EEE_100TX and MDIO_EEE_1000T respectively.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 01/12] net: phy: bcm-phylib: replace obsolete EEE macro references

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> The macros MDIO_AN_EEE_ADV_100TX and MDIO_AN_EEE_ADV_1000T are now
> considered obsolete and are replaced in the kernel with the generic
> macros MDIO_EEE_100TX and MDIO_EEE_1000T respectively.
> 
> Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Doug Berger]

This commit adds support for the internal fast ethernet 10/100 PHY
found in the BCM7260, BCM7268, and BCM7271 devices.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/phy/bcm7xxx.c | 215 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/brcmphy.h   |   3 +
 2 files changed, 216 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index d1c2614dad3a..caa9f6e17f34 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -1,7 +1,7 @@
 /*
  * Broadcom BCM7xxx internal transceivers support.
  *
- * Copyright (C) 2014, Broadcom Corporation
+ * Copyright (C) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or
  * modify it under the terms of the GNU General Public License
@@ -19,7 +19,7 @@
 
 /* Broadcom BCM7xxx internal PHY registers */
 
-/* 40nm only register definitions */
+/* EPHY only register definitions */
 #define MII_BCM7XXX_100TX_AUX_CTL	0x10
 #define MII_BCM7XXX_100TX_FALSE_CAR	0x13
 #define MII_BCM7XXX_100TX_DISC		0x14
@@ -27,6 +27,19 @@
 #define  MII_BCM7XXX_64CLK_MDIO		BIT(12)
 #define MII_BCM7XXX_TEST		0x1f
 #define  MII_BCM7XXX_SHD_MODE_2		BIT(2)
+#define MII_BCM7XXX_SHD_2_ADDR_CTRL	0xe
+#define MII_BCM7XXX_SHD_2_CTRL_STAT	0xf
+#define MII_BCM7XXX_SHD_2_BIAS_TRIM	0x1a
+#define MII_BCM7XXX_SHD_3_AN_EEE_ADV	0x3
+#define MII_BCM7XXX_SHD_3_PCS_CTRL_2	0x6
+#define  MII_BCM7XXX_PCS_CTRL_2_DEF	0x4400
+#define MII_BCM7XXX_SHD_3_AN_STAT	0xb
+#define  MII_BCM7XXX_AN_NULL_MSG_EN	BIT(0)
+#define  MII_BCM7XXX_AN_EEE_EN		BIT(1)
+#define MII_BCM7XXX_SHD_3_EEE_THRESH	0xe
+#define  MII_BCM7XXX_EEE_THRESH_DEF	0x50
+#define MII_BCM7XXX_SHD_3_TL4		0x23
+#define  MII_BCM7XXX_TL4_RST_MSK	(BIT(2) | BIT(1))
 
 /* 28nm only register definitions */
 #define MISC_ADDR(base, channel)	base, channel
@@ -286,6 +299,181 @@ static int phy_set_clr_bits(struct phy_device *dev, int location,
 	return v;
 }
 
+static int bcm7xxx_28nm_ephy_01_afe_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	/* set shadow mode 2 */
+	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
+			       MII_BCM7XXX_SHD_MODE_2, 0);
+	if (ret < 0)
+		return ret;
+
+	/* Set current trim values INT_trim = -1, Ext_trim =0 */
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0);
+	if (ret < 0)
+		goto reset_shadow_mode;
+
+	/* Cal reset */
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
+			MII_BCM7XXX_SHD_3_TL4);
+	if (ret < 0)
+		goto reset_shadow_mode;
+	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
+			       MII_BCM7XXX_TL4_RST_MSK, 0);
+	if (ret < 0)
+		goto reset_shadow_mode;
+
+	/* Cal reset disable */
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
+			MII_BCM7XXX_SHD_3_TL4);
+	if (ret < 0)
+		goto reset_shadow_mode;
+	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
+			       0, MII_BCM7XXX_TL4_RST_MSK);
+	if (ret < 0)
+		goto reset_shadow_mode;
+
+reset_shadow_mode:
+	/* reset shadow mode 2 */
+	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
+			       MII_BCM7XXX_SHD_MODE_2);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+/* The 28nm EPHY does not support Clause 45 (MMD) used by bcm-phy-lib */
+static int bcm7xxx_28nm_ephy_apd_enable(struct phy_device *phydev)
+{
+	int ret;
+
+	/* set shadow mode 1 */
+	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST,
+			       MII_BRCM_FET_BT_SRE, 0);
+	if (ret < 0)
+		return ret;
+
+	/* Enable auto-power down */
+	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_SHDW_AUXSTAT2,
+			       MII_BRCM_FET_SHDW_AS2_APDE, 0);
+	if (ret < 0)
+		return ret;
+
+	/* reset shadow mode 1 */
+	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST, 0,
+			       MII_BRCM_FET_BT_SRE);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int bcm7xxx_28nm_ephy_eee_enable(struct phy_device *phydev)
+{
+	int ret;
+
+	/* set shadow mode 2 */
+	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
+			       MII_BCM7XXX_SHD_MODE_2, 0);
+	if (ret < 0)
+		return ret;
+
+	/* Advertise supported modes */
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
+			MII_BCM7XXX_SHD_3_AN_EEE_ADV);
+	if (ret < 0)
+		goto reset_shadow_mode;
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
+			MDIO_EEE_100TX);
+	if (ret < 0)
+		goto reset_shadow_mode;
+
+	/* Restore Defaults */
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
+			MII_BCM7XXX_SHD_3_PCS_CTRL_2);
+	if (ret < 0)
+		goto reset_shadow_mode;
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
+			MII_BCM7XXX_PCS_CTRL_2_DEF);
+	if (ret < 0)
+		goto reset_shadow_mode;
+
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
+			MII_BCM7XXX_SHD_3_EEE_THRESH);
+	if (ret < 0)
+		goto reset_shadow_mode;
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
+			MII_BCM7XXX_EEE_THRESH_DEF);
+	if (ret < 0)
+		goto reset_shadow_mode;
+
+	/* Enable EEE autonegotiation */
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
+			MII_BCM7XXX_SHD_3_AN_STAT);
+	if (ret < 0)
+		goto reset_shadow_mode;
+	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
+			(MII_BCM7XXX_AN_NULL_MSG_EN | MII_BCM7XXX_AN_EEE_EN));
+	if (ret < 0)
+		goto reset_shadow_mode;
+
+reset_shadow_mode:
+	/* reset shadow mode 2 */
+	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
+			       MII_BCM7XXX_SHD_MODE_2);
+	if (ret < 0)
+		return ret;
+
+	/* Restart autoneg */
+	phy_write(phydev, MII_BMCR,
+		  (BMCR_SPEED100 | BMCR_ANENABLE | BMCR_ANRESTART));
+
+	return 0;
+}
+
+static int bcm7xxx_28nm_ephy_config_init(struct phy_device *phydev)
+{
+	u8 rev = phydev->phy_id & ~phydev->drv->phy_id_mask;
+	int ret = 0;
+
+	pr_info_once("%s: %s PHY revision: 0x%02x\n",
+		     phydev_name(phydev), phydev->drv->name, rev);
+
+	/* Dummy read to a register to workaround a possible issue upon reset
+	 * where the internal inverter may not allow the first MDIO transaction
+	 * to pass the MDIO management controller and make us return 0xffff for
+	 * such reads.
+	 */
+	phy_read(phydev, MII_BMSR);
+
+	/* Apply AFE software work-around if necessary */
+	if (rev == 0x01) {
+		ret = bcm7xxx_28nm_ephy_01_afe_config_init(phydev);
+		if (ret)
+			return ret;
+	}
+
+	ret = bcm7xxx_28nm_ephy_eee_enable(phydev);
+	if (ret)
+		return ret;
+
+	return bcm7xxx_28nm_ephy_apd_enable(phydev);
+}
+
+static int bcm7xxx_28nm_ephy_resume(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Re-apply workarounds coming out suspend/resume */
+	ret = bcm7xxx_28nm_ephy_config_init(phydev);
+	if (ret)
+		return ret;
+
+	return genphy_config_aneg(phydev);
+}
+
 static int bcm7xxx_config_init(struct phy_device *phydev)
 {
 	int ret;
@@ -434,6 +622,23 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	.probe		= bcm7xxx_28nm_probe,				\
 }
 
+#define BCM7XXX_28NM_EPHY(_oui, _name)					\
+{									\
+	.phy_id		= (_oui),					\
+	.phy_id_mask	= 0xfffffff0,					\
+	.name		= _name,					\
+	.features	= PHY_BASIC_FEATURES,				\
+	.flags		= PHY_IS_INTERNAL,				\
+	.config_init	= bcm7xxx_28nm_ephy_config_init,		\
+	.config_aneg	= genphy_config_aneg,				\
+	.read_status	= genphy_read_status,				\
+	.resume		= bcm7xxx_28nm_ephy_resume,			\
+	.get_sset_count	= bcm_phy_get_sset_count,			\
+	.get_strings	= bcm_phy_get_strings,				\
+	.get_stats	= bcm7xxx_28nm_get_phy_stats,			\
+	.probe		= bcm7xxx_28nm_probe,				\
+}
+
 #define BCM7XXX_40NM_EPHY(_oui, _name)					\
 {									\
 	.phy_id         = (_oui),					\
@@ -450,6 +655,9 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 
 static struct phy_driver bcm7xxx_driver[] = {
 	BCM7XXX_28NM_GPHY(PHY_ID_BCM7250, "Broadcom BCM7250"),
+	BCM7XXX_28NM_EPHY(PHY_ID_BCM7260, "Broadcom BCM7260"),
+	BCM7XXX_28NM_EPHY(PHY_ID_BCM7268, "Broadcom BCM7268"),
+	BCM7XXX_28NM_EPHY(PHY_ID_BCM7271, "Broadcom BCM7271"),
 	BCM7XXX_28NM_GPHY(PHY_ID_BCM7278, "Broadcom BCM7278"),
 	BCM7XXX_28NM_GPHY(PHY_ID_BCM7364, "Broadcom BCM7364"),
 	BCM7XXX_28NM_GPHY(PHY_ID_BCM7366, "Broadcom BCM7366"),
@@ -466,6 +674,9 @@ static struct phy_driver bcm7xxx_driver[] = {
 
 static struct mdio_device_id __maybe_unused bcm7xxx_tbl[] = {
 	{ PHY_ID_BCM7250, 0xfffffff0, },
+	{ PHY_ID_BCM7260, 0xfffffff0, },
+	{ PHY_ID_BCM7268, 0xfffffff0, },
+	{ PHY_ID_BCM7271, 0xfffffff0, },
 	{ PHY_ID_BCM7278, 0xfffffff0, },
 	{ PHY_ID_BCM7364, 0xfffffff0, },
 	{ PHY_ID_BCM7366, 0xfffffff0, },
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 55e517130311..abcda9b458ab 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -25,6 +25,9 @@
 #define PHY_ID_BCM57780			0x03625d90
 
 #define PHY_ID_BCM7250			0xae025280
+#define PHY_ID_BCM7260			0xae025190
+#define PHY_ID_BCM7268			0xae025090
+#define PHY_ID_BCM7271			0xae0253b0
 #define PHY_ID_BCM7278			0xae0251a0
 #define PHY_ID_BCM7364			0xae025260
 #define PHY_ID_BCM7366			0x600d8490
-- 
2.11.1

Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> This commit adds support for the internal fast ethernet 10/100 PHY
> found in the BCM7260, BCM7268, and BCM7271 devices.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> This commit adds support for the internal fast ethernet 10/100 PHY
> found in the BCM7260, BCM7268, and BCM7271 devices.
> 
> Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Andrew Lunn]

On Mon, Mar 13, 2017 at 05:41:32PM -0700, Doug Berger wrote:
> +static int bcm7xxx_28nm_ephy_01_afe_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* set shadow mode 2 */
> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
> +			       MII_BCM7XXX_SHD_MODE_2, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set current trim values INT_trim = -1, Ext_trim =0 */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	/* Cal reset */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_TL4);
> +	if (ret < 0)
> +		goto reset_shadow_mode;

Hi Doug

It would be nice to have a few blank lines here and there...

> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			       MII_BCM7XXX_TL4_RST_MSK, 0);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	/* Cal reset disable */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_TL4);
> +	if (ret < 0)
> +		goto reset_shadow_mode;

... just to break things up into readable chunks.

> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			       0, MII_BCM7XXX_TL4_RST_MSK);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +reset_shadow_mode:
> +	/* reset shadow mode 2 */
> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
> +			       MII_BCM7XXX_SHD_MODE_2);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

How about:

	return phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
	       		        MII_BCM7XXX_SHD_MODE_2);

> +/* The 28nm EPHY does not support Clause 45 (MMD) used by bcm-phy-lib */
> +static int bcm7xxx_28nm_ephy_apd_enable(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* set shadow mode 1 */
> +	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST,
> +			       MII_BRCM_FET_BT_SRE, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable auto-power down */
> +	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_SHDW_AUXSTAT2,
> +			       MII_BRCM_FET_SHDW_AS2_APDE, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* reset shadow mode 1 */
> +	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST, 0,
> +			       MII_BRCM_FET_BT_SRE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

How about just

    return phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST, 0,
    	   		       MII_BRCM_FET_BT_SRE);


> +}
> +
> +static int bcm7xxx_28nm_ephy_eee_enable(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* set shadow mode 2 */
> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
> +			       MII_BCM7XXX_SHD_MODE_2, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Advertise supported modes */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_AN_EEE_ADV);
> +	if (ret < 0)
> +		goto reset_shadow_mode;

blank...

> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			MDIO_EEE_100TX);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	/* Restore Defaults */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_PCS_CTRL_2);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			MII_BCM7XXX_PCS_CTRL_2_DEF);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_EEE_THRESH);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			MII_BCM7XXX_EEE_THRESH_DEF);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	/* Enable EEE autonegotiation */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_AN_STAT);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			(MII_BCM7XXX_AN_NULL_MSG_EN | MII_BCM7XXX_AN_EEE_EN));
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +reset_shadow_mode:
> +	/* reset shadow mode 2 */
> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
> +			       MII_BCM7XXX_SHD_MODE_2);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Restart autoneg */
> +	phy_write(phydev, MII_BMCR,
> +		  (BMCR_SPEED100 | BMCR_ANENABLE | BMCR_ANRESTART));
> +
> +	return 0;

  return phy_write(.....); ?

> +}

  Andrew

Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Andrew Lunn]

On Mon, Mar 13, 2017 at 05:41:32PM -0700, Doug Berger wrote:
> +static int bcm7xxx_28nm_ephy_01_afe_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* set shadow mode 2 */
> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
> +			       MII_BCM7XXX_SHD_MODE_2, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Set current trim values INT_trim = -1, Ext_trim =0 */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	/* Cal reset */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_TL4);
> +	if (ret < 0)
> +		goto reset_shadow_mode;

Hi Doug

It would be nice to have a few blank lines here and there...

> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			       MII_BCM7XXX_TL4_RST_MSK, 0);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	/* Cal reset disable */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_TL4);
> +	if (ret < 0)
> +		goto reset_shadow_mode;

... just to break things up into readable chunks.

> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			       0, MII_BCM7XXX_TL4_RST_MSK);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +reset_shadow_mode:
> +	/* reset shadow mode 2 */
> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
> +			       MII_BCM7XXX_SHD_MODE_2);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

How about:

	return phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
	       		        MII_BCM7XXX_SHD_MODE_2);

> +/* The 28nm EPHY does not support Clause 45 (MMD) used by bcm-phy-lib */
> +static int bcm7xxx_28nm_ephy_apd_enable(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* set shadow mode 1 */
> +	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST,
> +			       MII_BRCM_FET_BT_SRE, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable auto-power down */
> +	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_SHDW_AUXSTAT2,
> +			       MII_BRCM_FET_SHDW_AS2_APDE, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* reset shadow mode 1 */
> +	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST, 0,
> +			       MII_BRCM_FET_BT_SRE);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;

How about just

    return phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST, 0,
    	   		       MII_BRCM_FET_BT_SRE);


> +}
> +
> +static int bcm7xxx_28nm_ephy_eee_enable(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	/* set shadow mode 2 */
> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
> +			       MII_BCM7XXX_SHD_MODE_2, 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Advertise supported modes */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_AN_EEE_ADV);
> +	if (ret < 0)
> +		goto reset_shadow_mode;

blank...

> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			MDIO_EEE_100TX);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	/* Restore Defaults */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_PCS_CTRL_2);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			MII_BCM7XXX_PCS_CTRL_2_DEF);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_EEE_THRESH);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			MII_BCM7XXX_EEE_THRESH_DEF);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +	/* Enable EEE autonegotiation */
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> +			MII_BCM7XXX_SHD_3_AN_STAT);
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> +			(MII_BCM7XXX_AN_NULL_MSG_EN | MII_BCM7XXX_AN_EEE_EN));
> +	if (ret < 0)
> +		goto reset_shadow_mode;
> +
> +reset_shadow_mode:
> +	/* reset shadow mode 2 */
> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
> +			       MII_BCM7XXX_SHD_MODE_2);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Restart autoneg */
> +	phy_write(phydev, MII_BMCR,
> +		  (BMCR_SPEED100 | BMCR_ANENABLE | BMCR_ANRESTART));
> +
> +	return 0;

  return phy_write(.....); ?

> +}

  Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Doug Berger]

On 03/13/2017 06:06 PM, Andrew Lunn wrote:
> On Mon, Mar 13, 2017 at 05:41:32PM -0700, Doug Berger wrote:
>> +static int bcm7xxx_28nm_ephy_01_afe_config_init(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	/* set shadow mode 2 */
>> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
>> +			       MII_BCM7XXX_SHD_MODE_2, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Set current trim values INT_trim = -1, Ext_trim =0 */
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
>> +
>> +	/* Cal reset */
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
>> +			MII_BCM7XXX_SHD_3_TL4);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
> 
> Hi Doug
> 
> It would be nice to have a few blank lines here and there...
> 
Thanks for taking the time to review this.

In general I try to keep lines of related functionality together and use
the blank lines to help identify boundaries.  In this particular case, I
believe it is clearer to keep the code that may return an error code
together with the code that tests for the error.

Perhaps this would be a better alternative:
+	/* Set current trim values INT_trim = -1, Ext_trim =0 */
+	if (phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0) < 0)
+		goto reset_shadow_mode;
+
+	/* Cal reset */
+	if (phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
+		      MII_BCM7XXX_SHD_3_TL4) < 0)
+		goto reset_shadow_mode;

>> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
>> +			       MII_BCM7XXX_TL4_RST_MSK, 0);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
>> +
>> +	/* Cal reset disable */
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
>> +			MII_BCM7XXX_SHD_3_TL4);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
> 
> ... just to break things up into readable chunks.
> 
Maybe:
+	if (phy_set_clr_bits(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
+			     MII_BCM7XXX_TL4_RST_MSK, 0) < 0)
+		goto reset_shadow_mode;
+
+	/* Cal reset disable */
+	if (phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
+		      MII_BCM7XXX_SHD_3_TL4) < 0)
+		goto reset_shadow_mode;

>> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
>> +			       0, MII_BCM7XXX_TL4_RST_MSK);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
>> +
>> +reset_shadow_mode:
>> +	/* reset shadow mode 2 */
>> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
>> +			       MII_BCM7XXX_SHD_MODE_2);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> How about:
> 
> 	return phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
> 	       		        MII_BCM7XXX_SHD_MODE_2);
> 
The trouble here is that currently the phy_set_clr_bits() function
returns the value written or a negative error and the function
bcm7xxx_28nm_ephy_01_afe_config_init() is supposed to return 0 on
success and non-zero on failure so this would not have the same
functionality.  I suppose we could change the phy_set_clr_bits() API to
return 0 on success to make this work, but I wasn't trying to change
preexisting functionality in this file.

>> +/* The 28nm EPHY does not support Clause 45 (MMD) used by bcm-phy-lib */
>> +static int bcm7xxx_28nm_ephy_apd_enable(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	/* set shadow mode 1 */
>> +	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST,
>> +			       MII_BRCM_FET_BT_SRE, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Enable auto-power down */
>> +	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_SHDW_AUXSTAT2,
>> +			       MII_BRCM_FET_SHDW_AS2_APDE, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* reset shadow mode 1 */
>> +	ret = phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST, 0,
>> +			       MII_BRCM_FET_BT_SRE);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	return 0;
> 
> How about just
> 
>     return phy_set_clr_bits(phydev, MII_BRCM_FET_BRCMTEST, 0,
>     	   		       MII_BRCM_FET_BT_SRE);
> 
Same remark as above.

>> +}
>> +
>> +static int bcm7xxx_28nm_ephy_eee_enable(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	/* set shadow mode 2 */
>> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
>> +			       MII_BCM7XXX_SHD_MODE_2, 0);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Advertise supported modes */
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
>> +			MII_BCM7XXX_SHD_3_AN_EEE_ADV);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
> 
> blank...
> 
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
>> +			MDIO_EEE_100TX);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
Here the two phy_write() calls are required to "/* Advertise supported
modes */" (one sets an address and the other specifies the data to write
to that address) so I kept them together to imply an association with
the preceding comment.  If new code in the future came between them it
would be "bad" so closing the space helps indicate that it is not a safe
coding boundary for inserting code.

>> +
>> +	/* Restore Defaults */
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
>> +			MII_BCM7XXX_SHD_3_PCS_CTRL_2);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
>> +			MII_BCM7XXX_PCS_CTRL_2_DEF);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
Same here.

>> +
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
>> +			MII_BCM7XXX_SHD_3_EEE_THRESH);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
>> +			MII_BCM7XXX_EEE_THRESH_DEF);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
Here...

>> +
>> +	/* Enable EEE autonegotiation */
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
>> +			MII_BCM7XXX_SHD_3_AN_STAT);
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
>> +			(MII_BCM7XXX_AN_NULL_MSG_EN | MII_BCM7XXX_AN_EEE_EN));
>> +	if (ret < 0)
>> +		goto reset_shadow_mode;
and here.

>> +
>> +reset_shadow_mode:
>> +	/* reset shadow mode 2 */
>> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
>> +			       MII_BCM7XXX_SHD_MODE_2);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	/* Restart autoneg */
>> +	phy_write(phydev, MII_BMCR,
>> +		  (BMCR_SPEED100 | BMCR_ANENABLE | BMCR_ANRESTART));
>> +
>> +	return 0;
> 
>   return phy_write(.....); ?
> 
I would feel more comfortable with this if the return value of the
struct mii_bus write member function was more clearly defined.  In our
case, we return 0 on success so I would consider this change, but I
would prefer a consensus that all mii_bus write functions return 0 on
success before doing so.

>> +}
> 
>   Andrew
> 
Thanks again,
    Doug

Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Andrew Lunn]

On Mon, Mar 13, 2017 at 07:06:25PM -0700, Doug Berger wrote:
> On 03/13/2017 06:06 PM, Andrew Lunn wrote:
> > On Mon, Mar 13, 2017 at 05:41:32PM -0700, Doug Berger wrote:
> >> +static int bcm7xxx_28nm_ephy_01_afe_config_init(struct phy_device *phydev)
> >> +{
> >> +	int ret;
> >> +
> >> +	/* set shadow mode 2 */
> >> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
> >> +			       MII_BCM7XXX_SHD_MODE_2, 0);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	/* Set current trim values INT_trim = -1, Ext_trim =0 */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> >> +
> >> +	/* Cal reset */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_TL4);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> > 
> > Hi Doug
> > 
> > It would be nice to have a few blank lines here and there...
> > 
> Thanks for taking the time to review this.
> 
> In general I try to keep lines of related functionality together and use
> the blank lines to help identify boundaries.  In this particular case, I
> believe it is clearer to keep the code that may return an error code
> together with the code that tests for the error.

Hi Doug

I agree with that. Which is why i placed the comment between the goto
and the next block of code. This is where i think there should be a
blank line, to separate it from setting the trim values.

> > 	return phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
> > 	       		        MII_BCM7XXX_SHD_MODE_2);
> > 
> The trouble here is that currently the phy_set_clr_bits() function
> returns the value written or a negative error and the function
> bcm7xxx_28nm_ephy_01_afe_config_init() is supposed to return 0 on
> success and non-zero on failure so this would not have the same
> functionality.

Ah, O.K. No problem.

> >> +	/* Advertise supported modes */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_AN_EEE_ADV);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> > 
> > blank...
> > 
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> >> +			MDIO_EEE_100TX);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> Here the two phy_write() calls are required to "/* Advertise supported
> modes */" (one sets an address and the other specifies the data to write
> to that address) so I kept them together to imply an association with
> the preceding comment.

O.K, i probably would if written a little helper function. And you
seem to have this repeated a few times, so the helper would be used a
few times.

> >> +
> >> +	/* Restore Defaults */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_PCS_CTRL_2);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> >> +			MII_BCM7XXX_PCS_CTRL_2_DEF);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> Same here.
> 
> >> +
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_EEE_THRESH);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> >> +			MII_BCM7XXX_EEE_THRESH_DEF);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> Here...
> 
> >> +
> >> +	/* Enable EEE autonegotiation */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_AN_STAT);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> >> +			(MII_BCM7XXX_AN_NULL_MSG_EN | MII_BCM7XXX_AN_EEE_EN));
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> and here.
> 
> >> +
> >> +reset_shadow_mode:
> >> +	/* reset shadow mode 2 */
> >> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
> >> +			       MII_BCM7XXX_SHD_MODE_2);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	/* Restart autoneg */
> >> +	phy_write(phydev, MII_BMCR,
> >> +		  (BMCR_SPEED100 | BMCR_ANENABLE | BMCR_ANRESTART));
> >> +
> >> +	return 0;
> > 
> >   return phy_write(.....); ?
> > 
> I would feel more comfortable with this if the return value of the
> struct mii_bus write member function was more clearly defined.  In our
> case, we return 0 on success so I would consider this change, but I
> would prefer a consensus that all mii_bus write functions return 0 on
> success before doing so.

You are right in that this is not clearly defined. But i just looked
through all the mdio drivers in drivers/net/phy and they all do return
0 for their write operation.

  Andrew

Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Andrew Lunn]

On Mon, Mar 13, 2017 at 07:06:25PM -0700, Doug Berger wrote:
> On 03/13/2017 06:06 PM, Andrew Lunn wrote:
> > On Mon, Mar 13, 2017 at 05:41:32PM -0700, Doug Berger wrote:
> >> +static int bcm7xxx_28nm_ephy_01_afe_config_init(struct phy_device *phydev)
> >> +{
> >> +	int ret;
> >> +
> >> +	/* set shadow mode 2 */
> >> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
> >> +			       MII_BCM7XXX_SHD_MODE_2, 0);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	/* Set current trim values INT_trim = -1, Ext_trim =0 */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> >> +
> >> +	/* Cal reset */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_TL4);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> > 
> > Hi Doug
> > 
> > It would be nice to have a few blank lines here and there...
> > 
> Thanks for taking the time to review this.
> 
> In general I try to keep lines of related functionality together and use
> the blank lines to help identify boundaries.  In this particular case, I
> believe it is clearer to keep the code that may return an error code
> together with the code that tests for the error.

Hi Doug

I agree with that. Which is why i placed the comment between the goto
and the next block of code. This is where i think there should be a
blank line, to separate it from setting the trim values.

> > 	return phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
> > 	       		        MII_BCM7XXX_SHD_MODE_2);
> > 
> The trouble here is that currently the phy_set_clr_bits() function
> returns the value written or a negative error and the function
> bcm7xxx_28nm_ephy_01_afe_config_init() is supposed to return 0 on
> success and non-zero on failure so this would not have the same
> functionality.

Ah, O.K. No problem.

> >> +	/* Advertise supported modes */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_AN_EEE_ADV);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> > 
> > blank...
> > 
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> >> +			MDIO_EEE_100TX);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> Here the two phy_write() calls are required to "/* Advertise supported
> modes */" (one sets an address and the other specifies the data to write
> to that address) so I kept them together to imply an association with
> the preceding comment.

O.K, i probably would if written a little helper function. And you
seem to have this repeated a few times, so the helper would be used a
few times.

> >> +
> >> +	/* Restore Defaults */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_PCS_CTRL_2);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> >> +			MII_BCM7XXX_PCS_CTRL_2_DEF);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> Same here.
> 
> >> +
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_EEE_THRESH);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> >> +			MII_BCM7XXX_EEE_THRESH_DEF);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> Here...
> 
> >> +
> >> +	/* Enable EEE autonegotiation */
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
> >> +			MII_BCM7XXX_SHD_3_AN_STAT);
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> >> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_CTRL_STAT,
> >> +			(MII_BCM7XXX_AN_NULL_MSG_EN | MII_BCM7XXX_AN_EEE_EN));
> >> +	if (ret < 0)
> >> +		goto reset_shadow_mode;
> and here.
> 
> >> +
> >> +reset_shadow_mode:
> >> +	/* reset shadow mode 2 */
> >> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST, 0,
> >> +			       MII_BCM7XXX_SHD_MODE_2);
> >> +	if (ret < 0)
> >> +		return ret;
> >> +
> >> +	/* Restart autoneg */
> >> +	phy_write(phydev, MII_BMCR,
> >> +		  (BMCR_SPEED100 | BMCR_ANENABLE | BMCR_ANRESTART));
> >> +
> >> +	return 0;
> > 
> >   return phy_write(.....); ?
> > 
> I would feel more comfortable with this if the return value of the
> struct mii_bus write member function was more clearly defined.  In our
> case, we return 0 on success so I would consider this change, but I
> would prefer a consensus that all mii_bus write functions return 0 on
> success before doing so.

You are right in that this is not clearly defined. But i just looked
through all the mdio drivers in drivers/net/phy and they all do return
0 for their write operation.

  Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Doug Berger]

On 03/13/2017 07:43 PM, Andrew Lunn wrote:
> On Mon, Mar 13, 2017 at 07:06:25PM -0700, Doug Berger wrote:
>> On 03/13/2017 06:06 PM, Andrew Lunn wrote:
>>> On Mon, Mar 13, 2017 at 05:41:32PM -0700, Doug Berger wrote:
>>>> +static int bcm7xxx_28nm_ephy_01_afe_config_init(struct phy_device *phydev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	/* set shadow mode 2 */
>>>> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
>>>> +			       MII_BCM7XXX_SHD_MODE_2, 0);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	/* Set current trim values INT_trim = -1, Ext_trim =0 */
>>>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0);
>>>> +	if (ret < 0)
>>>> +		goto reset_shadow_mode;
>>>> +
>>>> +	/* Cal reset */
>>>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
>>>> +			MII_BCM7XXX_SHD_3_TL4);
>>>> +	if (ret < 0)
>>>> +		goto reset_shadow_mode;
>>>
>>> Hi Doug
>>>
>>> It would be nice to have a few blank lines here and there...
>>>
>> Thanks for taking the time to review this.
>>
>> In general I try to keep lines of related functionality together and use
>> the blank lines to help identify boundaries.  In this particular case, I
>> believe it is clearer to keep the code that may return an error code
>> together with the code that tests for the error.
> 
> Hi Doug
> 
> I agree with that. Which is why i placed the comment between the goto
> and the next block of code. This is where i think there should be a
> blank line, to separate it from setting the trim values.
> 
OK, I see.  I thought you were referring to the code blocks above the
comment.  In that case, as described earlier, the code below the comment
is tightly coupled with the code above the comment since the pair of
transactions are how we "/* Cal reset */".  The idea of introducing a
subroutine/helper function for these paired (addr/data) transactions
might help readability so I will consider it for a future patch.

Thanks again for the feedback,
    Doug

Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY

[Doug Berger]

On 03/13/2017 07:43 PM, Andrew Lunn wrote:
> On Mon, Mar 13, 2017 at 07:06:25PM -0700, Doug Berger wrote:
>> On 03/13/2017 06:06 PM, Andrew Lunn wrote:
>>> On Mon, Mar 13, 2017 at 05:41:32PM -0700, Doug Berger wrote:
>>>> +static int bcm7xxx_28nm_ephy_01_afe_config_init(struct phy_device *phydev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	/* set shadow mode 2 */
>>>> +	ret = phy_set_clr_bits(phydev, MII_BCM7XXX_TEST,
>>>> +			       MII_BCM7XXX_SHD_MODE_2, 0);
>>>> +	if (ret < 0)
>>>> +		return ret;
>>>> +
>>>> +	/* Set current trim values INT_trim = -1, Ext_trim =0 */
>>>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_BIAS_TRIM, 0x3BE0);
>>>> +	if (ret < 0)
>>>> +		goto reset_shadow_mode;
>>>> +
>>>> +	/* Cal reset */
>>>> +	ret = phy_write(phydev, MII_BCM7XXX_SHD_2_ADDR_CTRL,
>>>> +			MII_BCM7XXX_SHD_3_TL4);
>>>> +	if (ret < 0)
>>>> +		goto reset_shadow_mode;
>>>
>>> Hi Doug
>>>
>>> It would be nice to have a few blank lines here and there...
>>>
>> Thanks for taking the time to review this.
>>
>> In general I try to keep lines of related functionality together and use
>> the blank lines to help identify boundaries.  In this particular case, I
>> believe it is clearer to keep the code that may return an error code
>> together with the code that tests for the error.
> 
> Hi Doug
> 
> I agree with that. Which is why i placed the comment between the goto
> and the next block of code. This is where i think there should be a
> blank line, to separate it from setting the trim values.
> 
OK, I see.  I thought you were referring to the code blocks above the
comment.  In that case, as described earlier, the code below the comment
is tightly coupled with the code above the comment since the pair of
transactions are how we "/* Cal reset */".  The idea of introducing a
subroutine/helper function for these paired (addr/data) transactions
might help readability so I will consider it for a future patch.

Thanks again for the feedback,
    Doug
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 03/12] net: bcmgenet: simplify circular pointer arithmetic

[Doug Berger]

A 2's complement subtraction will always do a borrow, so masking
off the sign bits is the same as conditionally adding (mask+1).

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index f92896835d2a..2c008b09c4e3 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1,7 +1,7 @@
 /*
  * Broadcom GENET (Gigabit Ethernet) controller driver
  *
- * Copyright (c) 2014 Broadcom Corporation
+ * Copyright (c) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -1175,13 +1175,9 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
 	unsigned int txbds_processed = 0;
 
 	/* Compute how many buffers are transmitted since last xmit call */
-	c_index = bcmgenet_tdma_ring_readl(priv, ring->index, TDMA_CONS_INDEX);
-	c_index &= DMA_C_INDEX_MASK;
-
-	if (likely(c_index >= ring->c_index))
-		txbds_ready = c_index - ring->c_index;
-	else
-		txbds_ready = (DMA_C_INDEX_MASK + 1) - ring->c_index + c_index;
+	c_index = bcmgenet_tdma_ring_readl(priv, ring->index, TDMA_CONS_INDEX)
+		& DMA_C_INDEX_MASK;
+	txbds_ready = (c_index - ring->c_index) & DMA_C_INDEX_MASK;
 
 	netif_dbg(priv, tx_done, dev,
 		  "%s ring=%d old_c_index=%u c_index=%u txbds_ready=%u\n",
@@ -1611,12 +1607,7 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 	}
 
 	p_index &= DMA_P_INDEX_MASK;
-
-	if (likely(p_index >= ring->c_index))
-		rxpkttoprocess = p_index - ring->c_index;
-	else
-		rxpkttoprocess = (DMA_C_INDEX_MASK + 1) - ring->c_index +
-				 p_index;
+	rxpkttoprocess = (p_index - ring->c_index) & DMA_C_INDEX_MASK;
 
 	netif_dbg(priv, rx_status, dev,
 		  "RDMA: rxpkttoprocess=%d\n", rxpkttoprocess);
-- 
2.11.1

Re: [PATCH net-next 03/12] net: bcmgenet: simplify circular pointer arithmetic

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> A 2's complement subtraction will always do a borrow, so masking
> off the sign bits is the same as conditionally adding (mask+1).
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 03/12] net: bcmgenet: simplify circular pointer arithmetic

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> A 2's complement subtraction will always do a borrow, so masking
> off the sign bits is the same as conditionally adding (mask+1).
> 
> Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 04/12] net: bcmgenet: remove meaningless lines

[Doug Berger]

An assortment of non-functional lines are removed to reduce confusion
and some typos in comments are corrected.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 2c008b09c4e3..22c92f5a9829 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -605,7 +605,7 @@ static int bcmgenet_set_coalesce(struct net_device *dev,
 
 	/* GENET TDMA hardware does not support a configurable timeout, but will
 	 * always generate an interrupt either after MBDONE packets have been
-	 * transmitted, or when the ring is emtpy.
+	 * transmitted, or when the ring is empty.
 	 */
 	if (ec->tx_coalesce_usecs || ec->tx_coalesce_usecs_high ||
 	    ec->tx_coalesce_usecs_irq || ec->tx_coalesce_usecs_low)
@@ -1834,10 +1834,8 @@ static void bcmgenet_intr_disable(struct bcmgenet_priv *priv)
 	/* Mask all interrupts.*/
 	bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF, INTRL2_CPU_MASK_SET);
 	bcmgenet_intrl2_0_writel(priv, 0xFFFFFFFF, INTRL2_CPU_CLEAR);
-	bcmgenet_intrl2_0_writel(priv, 0, INTRL2_CPU_MASK_CLEAR);
 	bcmgenet_intrl2_1_writel(priv, 0xFFFFFFFF, INTRL2_CPU_MASK_SET);
 	bcmgenet_intrl2_1_writel(priv, 0xFFFFFFFF, INTRL2_CPU_CLEAR);
-	bcmgenet_intrl2_1_writel(priv, 0, INTRL2_CPU_MASK_CLEAR);
 }
 
 static void bcmgenet_link_intr_enable(struct bcmgenet_priv *priv)
@@ -1926,7 +1924,6 @@ static int init_umac(struct bcmgenet_priv *priv)
 	bcmgenet_intrl2_0_writel(priv, int0_enable, INTRL2_CPU_MASK_CLEAR);
 	bcmgenet_intrl2_1_writel(priv, int1_enable, INTRL2_CPU_MASK_CLEAR);
 
-	/* Enable rx/tx engine.*/
 	dev_dbg(kdev, "done init umac\n");
 
 	return 0;
@@ -2836,7 +2833,7 @@ static int bcmgenet_close(struct net_device *dev)
 	if (ret)
 		return ret;
 
-	/* Disable MAC transmit. TX DMA disabled have to done before this */
+	/* Disable MAC transmit. TX DMA disabled must be done before this */
 	umac_enable_set(priv, CMD_TX_EN, false);
 
 	/* tx reclaim */
@@ -3115,22 +3112,18 @@ static void bcmgenet_set_hw_params(struct bcmgenet_priv *priv)
 		bcmgenet_dma_regs = bcmgenet_dma_regs_v3plus;
 		genet_dma_ring_regs = genet_dma_ring_regs_v4;
 		priv->dma_rx_chk_bit = DMA_RX_CHK_V3PLUS;
-		priv->version = GENET_V4;
 	} else if (GENET_IS_V3(priv)) {
 		bcmgenet_dma_regs = bcmgenet_dma_regs_v3plus;
 		genet_dma_ring_regs = genet_dma_ring_regs_v123;
 		priv->dma_rx_chk_bit = DMA_RX_CHK_V3PLUS;
-		priv->version = GENET_V3;
 	} else if (GENET_IS_V2(priv)) {
 		bcmgenet_dma_regs = bcmgenet_dma_regs_v2;
 		genet_dma_ring_regs = genet_dma_ring_regs_v123;
 		priv->dma_rx_chk_bit = DMA_RX_CHK_V12;
-		priv->version = GENET_V2;
 	} else if (GENET_IS_V1(priv)) {
 		bcmgenet_dma_regs = bcmgenet_dma_regs_v1;
 		genet_dma_ring_regs = genet_dma_ring_regs_v123;
 		priv->dma_rx_chk_bit = DMA_RX_CHK_V12;
-		priv->version = GENET_V1;
 	}
 
 	/* enum genet_version starts at 1 */
@@ -3397,7 +3390,7 @@ static int bcmgenet_suspend(struct device *d)
 	if (ret)
 		return ret;
 
-	/* Disable MAC transmit. TX DMA disabled have to done before this */
+	/* Disable MAC transmit. TX DMA disabled must be done before this */
 	umac_enable_set(priv, CMD_TX_EN, false);
 
 	/* tx reclaim */
-- 
2.11.1

Re: [PATCH net-next 04/12] net: bcmgenet: remove meaningless lines

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> An assortment of non-functional lines are removed to reduce confusion
> and some typos in comments are corrected.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 04/12] net: bcmgenet: remove meaningless lines

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> An assortment of non-functional lines are removed to reduce confusion
> and some typos in comments are corrected.
> 
> Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 05/12] net: bcmgenet: manage dma interrupts in napi code

[Doug Berger]

This commit moves DMA interrupt enabling out of init_umac() and adds
the masking of these interrupts to the napi enable and disable code.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 39 +++++++++++++++-----------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 22c92f5a9829..9be884021679 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1862,8 +1862,6 @@ static int init_umac(struct bcmgenet_priv *priv)
 	int ret;
 	u32 reg;
 	u32 int0_enable = 0;
-	u32 int1_enable = 0;
-	int i;
 
 	dev_dbg(&priv->pdev->dev, "bcmgenet: init_umac\n");
 
@@ -1890,12 +1888,6 @@ static int init_umac(struct bcmgenet_priv *priv)
 
 	bcmgenet_intr_disable(priv);
 
-	/* Enable Rx default queue 16 interrupts */
-	int0_enable |= UMAC_IRQ_RXDMA_DONE;
-
-	/* Enable Tx default queue 16 interrupts */
-	int0_enable |= UMAC_IRQ_TXDMA_DONE;
-
 	/* Configure backpressure vectors for MoCA */
 	if (priv->phy_interface == PHY_INTERFACE_MODE_MOCA) {
 		reg = bcmgenet_bp_mc_get(priv);
@@ -1913,16 +1905,7 @@ static int init_umac(struct bcmgenet_priv *priv)
 	if (priv->hw_params->flags & GENET_HAS_MDIO_INTR)
 		int0_enable |= (UMAC_IRQ_MDIO_DONE | UMAC_IRQ_MDIO_ERROR);
 
-	/* Enable Rx priority queue interrupts */
-	for (i = 0; i < priv->hw_params->rx_queues; ++i)
-		int1_enable |= (1 << (UMAC_IRQ1_RX_INTR_SHIFT + i));
-
-	/* Enable Tx priority queue interrupts */
-	for (i = 0; i < priv->hw_params->tx_queues; ++i)
-		int1_enable |= (1 << i);
-
 	bcmgenet_intrl2_0_writel(priv, int0_enable, INTRL2_CPU_MASK_CLEAR);
-	bcmgenet_intrl2_1_writel(priv, int1_enable, INTRL2_CPU_MASK_CLEAR);
 
 	dev_dbg(kdev, "done init umac\n");
 
@@ -2055,22 +2038,33 @@ static void bcmgenet_init_tx_napi(struct bcmgenet_priv *priv)
 static void bcmgenet_enable_tx_napi(struct bcmgenet_priv *priv)
 {
 	unsigned int i;
+	u32 int0_enable = UMAC_IRQ_TXDMA_DONE;
+	u32 int1_enable = 0;
 	struct bcmgenet_tx_ring *ring;
 
 	for (i = 0; i < priv->hw_params->tx_queues; ++i) {
 		ring = &priv->tx_rings[i];
 		napi_enable(&ring->napi);
+		int1_enable |= (1 << i);
 	}
 
 	ring = &priv->tx_rings[DESC_INDEX];
 	napi_enable(&ring->napi);
+
+	bcmgenet_intrl2_0_writel(priv, int0_enable, INTRL2_CPU_MASK_CLEAR);
+	bcmgenet_intrl2_1_writel(priv, int1_enable, INTRL2_CPU_MASK_CLEAR);
 }
 
 static void bcmgenet_disable_tx_napi(struct bcmgenet_priv *priv)
 {
 	unsigned int i;
+	u32 int0_disable = UMAC_IRQ_TXDMA_DONE;
+	u32 int1_disable = 0xffff;
 	struct bcmgenet_tx_ring *ring;
 
+	bcmgenet_intrl2_0_writel(priv, int0_disable, INTRL2_CPU_MASK_SET);
+	bcmgenet_intrl2_1_writel(priv, int1_disable, INTRL2_CPU_MASK_SET);
+
 	for (i = 0; i < priv->hw_params->tx_queues; ++i) {
 		ring = &priv->tx_rings[i];
 		napi_disable(&ring->napi);
@@ -2183,22 +2177,33 @@ static void bcmgenet_init_rx_napi(struct bcmgenet_priv *priv)
 static void bcmgenet_enable_rx_napi(struct bcmgenet_priv *priv)
 {
 	unsigned int i;
+	u32 int0_enable = UMAC_IRQ_RXDMA_DONE;
+	u32 int1_enable = 0;
 	struct bcmgenet_rx_ring *ring;
 
 	for (i = 0; i < priv->hw_params->rx_queues; ++i) {
 		ring = &priv->rx_rings[i];
 		napi_enable(&ring->napi);
+		int1_enable |= (1 << (UMAC_IRQ1_RX_INTR_SHIFT + i));
 	}
 
 	ring = &priv->rx_rings[DESC_INDEX];
 	napi_enable(&ring->napi);
+
+	bcmgenet_intrl2_0_writel(priv, int0_enable, INTRL2_CPU_MASK_CLEAR);
+	bcmgenet_intrl2_1_writel(priv, int1_enable, INTRL2_CPU_MASK_CLEAR);
 }
 
 static void bcmgenet_disable_rx_napi(struct bcmgenet_priv *priv)
 {
 	unsigned int i;
+	u32 int0_disable = UMAC_IRQ_RXDMA_DONE;
+	u32 int1_disable = 0xffff << UMAC_IRQ1_RX_INTR_SHIFT;
 	struct bcmgenet_rx_ring *ring;
 
+	bcmgenet_intrl2_0_writel(priv, int0_disable, INTRL2_CPU_MASK_SET);
+	bcmgenet_intrl2_1_writel(priv, int1_disable, INTRL2_CPU_MASK_SET);
+
 	for (i = 0; i < priv->hw_params->rx_queues; ++i) {
 		ring = &priv->rx_rings[i];
 		napi_disable(&ring->napi);
-- 
2.11.1

Re: [PATCH net-next 05/12] net: bcmgenet: manage dma interrupts in napi code

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> This commit moves DMA interrupt enabling out of init_umac() and adds
> the masking of these interrupts to the napi enable and disable code.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 06/12] net: bcmgenet: remove handling of wol interrupts from isr0

[Doug Berger]

The bcmgenet_wol_isr() handler performs the necessary processing for
waking from a GENET event.  There is no necessary functionality behind
servicing the UMAC_IRQ_MPD_R event in the handling of isr0.  Therefore
the code that unmasks and masks this interrupt and that gets invoked
in response to it is removed by this commit.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c     | 10 +---------
 drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 15 +--------------
 2 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 9be884021679..661ca1b39c89 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2455,13 +2455,6 @@ static void bcmgenet_irq_task(struct work_struct *work)
 
 	netif_dbg(priv, intr, priv->dev, "%s\n", __func__);
 
-	if (priv->irq0_stat & UMAC_IRQ_MPD_R) {
-		priv->irq0_stat &= ~UMAC_IRQ_MPD_R;
-		netif_dbg(priv, wol, priv->dev,
-			  "magic packet detected, waking up\n");
-		bcmgenet_power_up(priv, GENET_POWER_WOL_MAGIC);
-	}
-
 	/* Link UP/DOWN event */
 	if (priv->irq0_stat & UMAC_IRQ_LINK_EVENT) {
 		phy_mac_interrupt(priv->phydev,
@@ -2558,8 +2551,7 @@ static irqreturn_t bcmgenet_isr0(int irq, void *dev_id)
 				UMAC_IRQ_PHY_DET_F |
 				UMAC_IRQ_LINK_EVENT |
 				UMAC_IRQ_HFB_SM |
-				UMAC_IRQ_HFB_MM |
-				UMAC_IRQ_MPD_R)) {
+				UMAC_IRQ_HFB_MM)) {
 		/* all other interested interrupts handled in bottom half */
 		schedule_work(&priv->bcmgenet_irq_work);
 	}
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
index b97122926d3a..2fbd027f0148 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
@@ -1,7 +1,7 @@
 /*
  * Broadcom GENET (Gigabit Ethernet) Wake-on-LAN support
  *
- * Copyright (c) 2014 Broadcom Corporation
+ * Copyright (c) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -127,7 +127,6 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
 				enum bcmgenet_power_mode mode)
 {
 	struct net_device *dev = priv->dev;
-	u32 cpu_mask_clear;
 	int retries = 0;
 	u32 reg;
 
@@ -173,18 +172,12 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
 		bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
 	}
 
-	/* Enable the MPD interrupt */
-	cpu_mask_clear = UMAC_IRQ_MPD_R;
-
-	bcmgenet_intrl2_0_writel(priv, cpu_mask_clear, INTRL2_CPU_MASK_CLEAR);
-
 	return 0;
 }
 
 void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
 			       enum bcmgenet_power_mode mode)
 {
-	u32 cpu_mask_set;
 	u32 reg;
 
 	if (mode != GENET_POWER_WOL_MAGIC) {
@@ -201,10 +194,4 @@ void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
 	reg &= ~CMD_CRC_FWD;
 	bcmgenet_umac_writel(priv, reg, UMAC_CMD);
 	priv->crc_fwd_en = 0;
-
-	/* Stop monitoring magic packet IRQ */
-	cpu_mask_set = UMAC_IRQ_MPD_R;
-
-	/* Stop monitoring magic packet IRQ */
-	bcmgenet_intrl2_0_writel(priv, cpu_mask_set, INTRL2_CPU_MASK_SET);
 }
-- 
2.11.1

[PATCH net-next 06/12] net: bcmgenet: remove handling of wol interrupts from isr0

[Doug Berger]

The bcmgenet_wol_isr() handler performs the necessary processing for
waking from a GENET event.  There is no necessary functionality behind
servicing the UMAC_IRQ_MPD_R event in the handling of isr0.  Therefore
the code that unmasks and masks this interrupt and that gets invoked
in response to it is removed by this commit.

Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c     | 10 +---------
 drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 15 +--------------
 2 files changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 9be884021679..661ca1b39c89 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -2455,13 +2455,6 @@ static void bcmgenet_irq_task(struct work_struct *work)
 
 	netif_dbg(priv, intr, priv->dev, "%s\n", __func__);
 
-	if (priv->irq0_stat & UMAC_IRQ_MPD_R) {
-		priv->irq0_stat &= ~UMAC_IRQ_MPD_R;
-		netif_dbg(priv, wol, priv->dev,
-			  "magic packet detected, waking up\n");
-		bcmgenet_power_up(priv, GENET_POWER_WOL_MAGIC);
-	}
-
 	/* Link UP/DOWN event */
 	if (priv->irq0_stat & UMAC_IRQ_LINK_EVENT) {
 		phy_mac_interrupt(priv->phydev,
@@ -2558,8 +2551,7 @@ static irqreturn_t bcmgenet_isr0(int irq, void *dev_id)
 				UMAC_IRQ_PHY_DET_F |
 				UMAC_IRQ_LINK_EVENT |
 				UMAC_IRQ_HFB_SM |
-				UMAC_IRQ_HFB_MM |
-				UMAC_IRQ_MPD_R)) {
+				UMAC_IRQ_HFB_MM)) {
 		/* all other interested interrupts handled in bottom half */
 		schedule_work(&priv->bcmgenet_irq_work);
 	}
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
index b97122926d3a..2fbd027f0148 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c
@@ -1,7 +1,7 @@
 /*
  * Broadcom GENET (Gigabit Ethernet) Wake-on-LAN support
  *
- * Copyright (c) 2014 Broadcom Corporation
+ * Copyright (c) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -127,7 +127,6 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
 				enum bcmgenet_power_mode mode)
 {
 	struct net_device *dev = priv->dev;
-	u32 cpu_mask_clear;
 	int retries = 0;
 	u32 reg;
 
@@ -173,18 +172,12 @@ int bcmgenet_wol_power_down_cfg(struct bcmgenet_priv *priv,
 		bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
 	}
 
-	/* Enable the MPD interrupt */
-	cpu_mask_clear = UMAC_IRQ_MPD_R;
-
-	bcmgenet_intrl2_0_writel(priv, cpu_mask_clear, INTRL2_CPU_MASK_CLEAR);
-
 	return 0;
 }
 
 void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
 			       enum bcmgenet_power_mode mode)
 {
-	u32 cpu_mask_set;
 	u32 reg;
 
 	if (mode != GENET_POWER_WOL_MAGIC) {
@@ -201,10 +194,4 @@ void bcmgenet_wol_power_up_cfg(struct bcmgenet_priv *priv,
 	reg &= ~CMD_CRC_FWD;
 	bcmgenet_umac_writel(priv, reg, UMAC_CMD);
 	priv->crc_fwd_en = 0;
-
-	/* Stop monitoring magic packet IRQ */
-	cpu_mask_set = UMAC_IRQ_MPD_R;
-
-	/* Stop monitoring magic packet IRQ */
-	bcmgenet_intrl2_0_writel(priv, cpu_mask_set, INTRL2_CPU_MASK_SET);
 }
-- 
2.11.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 06/12] net: bcmgenet: remove handling of wol interrupts from isr0

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> The bcmgenet_wol_isr() handler performs the necessary processing for
> waking from a GENET event.  There is no necessary functionality behind
> servicing the UMAC_IRQ_MPD_R event in the handling of isr0.  Therefore
> the code that unmasks and masks this interrupt and that gets invoked
> in response to it is removed by this commit.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 06/12] net: bcmgenet: remove handling of wol interrupts from isr0

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> The bcmgenet_wol_isr() handler performs the necessary processing for
> waking from a GENET event.  There is no necessary functionality behind
> servicing the UMAC_IRQ_MPD_R event in the handling of isr0.  Therefore
> the code that unmasks and masks this interrupt and that gets invoked
> in response to it is removed by this commit.
> 
> Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 07/12] net: bcmgenet: clear status to reduce spurious interrupts

[Doug Berger]

Since the DMA interrupt status is latched and the DMA servicing can be
polled, it is a good idea to clear the latched status of a DMA interrupt
before performing the service that would be invoked by the interrupt.

This prevents old status from causing spurious interrupts when the
interrupt is unmasked at a later time.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 661ca1b39c89..1f94ba1773dd 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1174,6 +1174,14 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
 	unsigned int txbds_ready;
 	unsigned int txbds_processed = 0;
 
+	/* Clear status before servicing to reduce spurious interrupts */
+	if (ring->index == DESC_INDEX)
+		bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_TXDMA_DONE,
+					 INTRL2_CPU_CLEAR);
+	else
+		bcmgenet_intrl2_1_writel(priv, (1 << ring->index),
+					 INTRL2_CPU_CLEAR);
+
 	/* Compute how many buffers are transmitted since last xmit call */
 	c_index = bcmgenet_tdma_ring_readl(priv, ring->index, TDMA_CONS_INDEX)
 		& DMA_C_INDEX_MASK;
@@ -1584,10 +1592,21 @@ static unsigned int bcmgenet_desc_rx(struct bcmgenet_rx_ring *ring,
 	unsigned long dma_flag;
 	int len;
 	unsigned int rxpktprocessed = 0, rxpkttoprocess;
-	unsigned int p_index;
+	unsigned int p_index, mask;
 	unsigned int discards;
 	unsigned int chksum_ok = 0;
 
+	/* Clear status before servicing to reduce spurious interrupts */
+	if (ring->index == DESC_INDEX) {
+		bcmgenet_intrl2_0_writel(priv, UMAC_IRQ_RXDMA_DONE,
+					 INTRL2_CPU_CLEAR);
+	} else {
+		mask = 1 << (UMAC_IRQ1_RX_INTR_SHIFT + ring->index);
+		bcmgenet_intrl2_1_writel(priv,
+					 mask,
+					 INTRL2_CPU_CLEAR);
+	}
+
 	p_index = bcmgenet_rdma_ring_readl(priv, ring->index, RDMA_PROD_INDEX);
 
 	discards = (p_index >> DMA_P_INDEX_DISCARD_CNT_SHIFT) &
-- 
2.11.1

Re: [PATCH net-next 07/12] net: bcmgenet: clear status to reduce spurious interrupts

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> Since the DMA interrupt status is latched and the DMA servicing can be
> polled, it is a good idea to clear the latched status of a DMA interrupt
> before performing the service that would be invoked by the interrupt.
> 
> This prevents old status from causing spurious interrupts when the
> interrupt is unmasked at a later time.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 07/12] net: bcmgenet: clear status to reduce spurious interrupts

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> Since the DMA interrupt status is latched and the DMA servicing can be
> polled, it is a good idea to clear the latched status of a DMA interrupt
> before performing the service that would be invoked by the interrupt.
> 
> This prevents old status from causing spurious interrupts when the
> interrupt is unmasked at a later time.
> 
> Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 08/12] net: bcmgenet: correct return value of __bcmgenet_tx_reclaim

[Doug Berger]

The reclaim function should return the number of buffer descriptors
reclaimed, not just the number corresponding to skb packets.

Also, remove the unnecessary computation when updating the consumer
index.

While this is not a functional problem it could degrade performance
of napi in a fragmented transmit stream.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 1f94ba1773dd..d90d366b286f 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1218,7 +1218,7 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
 	}
 
 	ring->free_bds += txbds_processed;
-	ring->c_index = (ring->c_index + txbds_processed) & DMA_C_INDEX_MASK;
+	ring->c_index = c_index;
 
 	dev->stats.tx_packets += pkts_compl;
 	dev->stats.tx_bytes += bytes_compl;
@@ -1231,7 +1231,7 @@ static unsigned int __bcmgenet_tx_reclaim(struct net_device *dev,
 			netif_tx_wake_queue(txq);
 	}
 
-	return pkts_compl;
+	return txbds_processed;
 }
 
 static unsigned int bcmgenet_tx_reclaim(struct net_device *dev,
-- 
2.11.1

Re: [PATCH net-next 08/12] net: bcmgenet: correct return value of __bcmgenet_tx_reclaim

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> The reclaim function should return the number of buffer descriptors
> reclaimed, not just the number corresponding to skb packets.
> 
> Also, remove the unnecessary computation when updating the consumer
> index.
> 
> While this is not a functional problem it could degrade performance
> of napi in a fragmented transmit stream.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 09/12] net: bcmgenet: return EOPNOTSUPP for unknown ioctl commands

[Doug Berger]

This commit changes the ioctl handling behavior to return the
EOPNOTSUPP error code instead of the EINVAL error code when an
unknown ioctl command value is detected.

It also removes some redundant parsing of the ioctl command value
and allows the SIOCSHWTSTAMP value to be handled.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index d90d366b286f..3b49c14128e2 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1062,27 +1062,14 @@ static void bcmgenet_power_up(struct bcmgenet_priv *priv,
 static int bcmgenet_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
 {
 	struct bcmgenet_priv *priv = netdev_priv(dev);
-	int val = 0;
 
 	if (!netif_running(dev))
 		return -EINVAL;
 
-	switch (cmd) {
-	case SIOCGMIIPHY:
-	case SIOCGMIIREG:
-	case SIOCSMIIREG:
-		if (!priv->phydev)
-			val = -ENODEV;
-		else
-			val = phy_mii_ioctl(priv->phydev, rq, cmd);
-		break;
-
-	default:
-		val = -EINVAL;
-		break;
-	}
+	if (!priv->phydev)
+		return -ENODEV;
 
-	return val;
+	return phy_mii_ioctl(priv->phydev, rq, cmd);
 }
 
 static struct enet_cb *bcmgenet_get_txcb(struct bcmgenet_priv *priv,
-- 
2.11.1

Re: [PATCH net-next 09/12] net: bcmgenet: return EOPNOTSUPP for unknown ioctl commands

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> This commit changes the ioctl handling behavior to return the
> EOPNOTSUPP error code instead of the EINVAL error code when an
> unknown ioctl command value is detected.
> 
> It also removes some redundant parsing of the ioctl command value
> and allows the SIOCSHWTSTAMP value to be handled.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

RE: [PATCH net-next 09/12] net: bcmgenet: return EOPNOTSUPP for unknown ioctl commands

[David Laight]

From: Doug Berger
> Sent: 14 March 2017 00:42
> This commit changes the ioctl handling behavior to return the
> EOPNOTSUPP error code instead of the EINVAL error code when an
> unknown ioctl command value is detected.
> 
> It also removes some redundant parsing of the ioctl command value
> and allows the SIOCSHWTSTAMP value to be handled.

A better description would seem to be:
Remove checks on ioctl command and just forward all ioctl requests
to phy_mii_ioctl().

I also thought the 'generic' response to an unknown ioctl command
was ENOTTY.

	David

Re: [PATCH net-next 09/12] net: bcmgenet: return EOPNOTSUPP for unknown ioctl commands

[Doug Berger]

On 03/14/2017 04:04 AM, David Laight wrote:
> From: Doug Berger
>> Sent: 14 March 2017 00:42
>> This commit changes the ioctl handling behavior to return the
>> EOPNOTSUPP error code instead of the EINVAL error code when an
>> unknown ioctl command value is detected.
>>
>> It also removes some redundant parsing of the ioctl command value
>> and allows the SIOCSHWTSTAMP value to be handled.
> 
> A better description would seem to be:
> Remove checks on ioctl command and just forward all ioctl requests
> to phy_mii_ioctl().
That is a good description of the code change, but I felt that was
clearly conveyed by the patch content.  I thought it would be a better
use of the comment to describe the more subtle functional change that
might be less clear.

> 
> I also thought the 'generic' response to an unknown ioctl command
> was ENOTTY.
and I think it probably helped solicit this feedback :).  I would have
thought that error makes more sense if there is no ioctl handler, but I
will definitely look into it.

Thanks for the feedback,
    Doug

[PATCH net-next 10/12] dt-bindings: net: document bcmgenet WoL interrupt

[Doug Berger]

A third interrupt cell can be provided to optionally specify
the interrupt used for handling Wake on LAN events.

Typically the wake up handling uses a separate interrupt
controller, so the interrupts-extended property is used to
accommodate this.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 Documentation/devicetree/bindings/net/brcm,bcmgenet.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
index 10587bdadbbe..01a70463cbc5 100644
--- a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
+++ b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
@@ -4,9 +4,12 @@ Required properties:
 - compatible: should contain one of "brcm,genet-v1", "brcm,genet-v2",
   "brcm,genet-v3", "brcm,genet-v4".
 - reg: address and length of the register set for the device
-- interrupts: must be two cells, the first cell is the general purpose
-  interrupt line, while the second cell is the interrupt for the ring
-  RX and TX queues operating in ring mode
+- interrupts and/or interrupts-extended: must be two cells, the first cell
+  is the general purpose interrupt line, while the second cell is the
+  interrupt for the ring RX and TX queues operating in ring mode.  An
+  optional third interrupt cell for Wake-on-LAN can be specified.
+  See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
+  for information on the property specifics.
 - phy-mode: see ethernet.txt file in the same directory
 - #address-cells: should be 1
 - #size-cells: should be 1
-- 
2.11.1

Re: [PATCH net-next 10/12] dt-bindings: net: document bcmgenet WoL interrupt

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> A third interrupt cell can be provided to optionally specify
> the interrupt used for handling Wake on LAN events.
> 
> Typically the wake up handling uses a separate interrupt
> controller, so the interrupts-extended property is used to
> accommodate this.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 10/12] dt-bindings: net: document bcmgenet WoL interrupt

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> A third interrupt cell can be provided to optionally specify
> the interrupt used for handling Wake on LAN events.
> 
> Typically the wake up handling uses a separate interrupt
> controller, so the interrupts-extended property is used to
> accommodate this.
> 
> Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 10/12] dt-bindings: net: document bcmgenet WoL interrupt

[Rob Herring]

On Mon, Mar 13, 2017 at 05:41:40PM -0700, Doug Berger wrote:
> A third interrupt cell can be provided to optionally specify
> the interrupt used for handling Wake on LAN events.
> 
> Typically the wake up handling uses a separate interrupt
> controller, so the interrupts-extended property is used to
> accommodate this.

Using interrupts vs. interrupts-extended is beyond the scope of the 
binding doc. IOW, documenting interrupts is enough and using
interrupts-extended is allowed.

> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>
> ---
>  Documentation/devicetree/bindings/net/brcm,bcmgenet.txt | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
> index 10587bdadbbe..01a70463cbc5 100644
> --- a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
> +++ b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
> @@ -4,9 +4,12 @@ Required properties:
>  - compatible: should contain one of "brcm,genet-v1", "brcm,genet-v2",
>    "brcm,genet-v3", "brcm,genet-v4".
>  - reg: address and length of the register set for the device
> -- interrupts: must be two cells, the first cell is the general purpose
> -  interrupt line, while the second cell is the interrupt for the ring
> -  RX and TX queues operating in ring mode
> +- interrupts and/or interrupts-extended: must be two cells, the first cell
> +  is the general purpose interrupt line, while the second cell is the
> +  interrupt for the ring RX and TX queues operating in ring mode.  An
> +  optional third interrupt cell for Wake-on-LAN can be specified.

Does the generic wakeup source work for this?

> +  See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +  for information on the property specifics.
>  - phy-mode: see ethernet.txt file in the same directory
>  - #address-cells: should be 1
>  - #size-cells: should be 1
> -- 
> 2.11.1
> 

Re: [PATCH net-next 10/12] dt-bindings: net: document bcmgenet WoL interrupt

[Rob Herring]

On Mon, Mar 13, 2017 at 05:41:40PM -0700, Doug Berger wrote:
> A third interrupt cell can be provided to optionally specify
> the interrupt used for handling Wake on LAN events.
> 
> Typically the wake up handling uses a separate interrupt
> controller, so the interrupts-extended property is used to
> accommodate this.

Using interrupts vs. interrupts-extended is beyond the scope of the 
binding doc. IOW, documenting interrupts is enough and using
interrupts-extended is allowed.

> 
> Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/net/brcm,bcmgenet.txt | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
> index 10587bdadbbe..01a70463cbc5 100644
> --- a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
> +++ b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
> @@ -4,9 +4,12 @@ Required properties:
>  - compatible: should contain one of "brcm,genet-v1", "brcm,genet-v2",
>    "brcm,genet-v3", "brcm,genet-v4".
>  - reg: address and length of the register set for the device
> -- interrupts: must be two cells, the first cell is the general purpose
> -  interrupt line, while the second cell is the interrupt for the ring
> -  RX and TX queues operating in ring mode
> +- interrupts and/or interrupts-extended: must be two cells, the first cell
> +  is the general purpose interrupt line, while the second cell is the
> +  interrupt for the ring RX and TX queues operating in ring mode.  An
> +  optional third interrupt cell for Wake-on-LAN can be specified.

Does the generic wakeup source work for this?

> +  See Documentation/devicetree/bindings/interrupt-controller/interrupts.txt
> +  for information on the property specifics.
>  - phy-mode: see ethernet.txt file in the same directory
>  - #address-cells: should be 1
>  - #size-cells: should be 1
> -- 
> 2.11.1
> 
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH net-next 11/12] dt-bindings: net: update bcmgenet binding for GENETv5

[Doug Berger]

The device tree documentation must be updated to reflect the new compatible
strings "brcm,genet-v5" and "brcm,genet-mdio-v5" used by the GENETv5 driver.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 Documentation/devicetree/bindings/net/brcm,bcmgenet.txt    | 10 +++++-----
 Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt |  5 +++--
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
index 01a70463cbc5..26c77d985faf 100644
--- a/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
+++ b/Documentation/devicetree/bindings/net/brcm,bcmgenet.txt
@@ -2,7 +2,7 @@
 
 Required properties:
 - compatible: should contain one of "brcm,genet-v1", "brcm,genet-v2",
-  "brcm,genet-v3", "brcm,genet-v4".
+  "brcm,genet-v3", "brcm,genet-v4", "brcm,genet-v5".
 - reg: address and length of the register set for the device
 - interrupts and/or interrupts-extended: must be two cells, the first cell
   is the general purpose interrupt line, while the second cell is the
@@ -32,15 +32,15 @@ Optional properties:
 
 Required child nodes:
 
-- mdio bus node: this node should always be present regarless of the PHY
+- mdio bus node: this node should always be present regardless of the PHY
   configuration of the GENET instance
 
 MDIO bus node required properties:
 
 - compatible: should contain one of "brcm,genet-mdio-v1", "brcm,genet-mdio-v2"
-  "brcm,genet-mdio-v3", "brcm,genet-mdio-v4", the version has to match the
-  parent node compatible property (e.g: brcm,genet-v4 pairs with
-  brcm,genet-mdio-v4)
+  "brcm,genet-mdio-v3", "brcm,genet-mdio-v4", "brcm,genet-mdio-v5", the version
+  has to match the parent node compatible property (e.g: brcm,genet-v4 pairs
+  with brcm,genet-mdio-v4)
 - reg: address and length relative to the parent node base register address
 - #address-cells: address cell for MDIO bus addressing, should be 1
 - #size-cells: size of the cells for MDIO bus addressing, should be 0
diff --git a/Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt b/Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt
index ab0bb4247d14..4648948f7c3b 100644
--- a/Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt
+++ b/Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt
@@ -2,8 +2,9 @@
 
 Required properties:
 - compatible: should one from "brcm,genet-mdio-v1", "brcm,genet-mdio-v2",
-  "brcm,genet-mdio-v3", "brcm,genet-mdio-v4" or "brcm,unimac-mdio"
-- reg: address and length of the regsiter set for the device, first one is the
+  "brcm,genet-mdio-v3", "brcm,genet-mdio-v4", "brcm,genet-mdio-v5" or
+  "brcm,unimac-mdio"
+- reg: address and length of the register set for the device, first one is the
   base register, and the second one is optional and for indirect accesses to
   larger than 16-bits MDIO transactions
 - reg-names: name(s) of the register must be "mdio" and optional "mdio_indir_rw"
-- 
2.11.1

Re: [PATCH net-next 11/12] dt-bindings: net: update bcmgenet binding for GENETv5

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> The device tree documentation must be updated to reflect the new compatible
> strings "brcm,genet-v5" and "brcm,genet-mdio-v5" used by the GENETv5 driver.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

[PATCH net-next 12/12] net: bcmgenet: add support for the GENETv5 hardware

[Doug Berger]

This commit adds support for the GENETv5 implementation.

The GENETv5 reports a major version of 6 instead of 5 so compensate
for this when verifying the configuration of the driver.  Also the
EPHY revision is now contained in the MDIO registers of the PHY so
the EPHY revision of 0 in GENET_VER_FMT is expected for GENETv5.

Signed-off-by: Doug Berger <opendmb@gmail.com>
---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 91 ++++++++++++++++++++------
 drivers/net/ethernet/broadcom/genet/bcmgenet.h | 12 +++-
 drivers/net/ethernet/broadcom/genet/bcmmii.c   | 62 ++++++++++--------
 drivers/net/phy/mdio-bcm-unimac.c              |  3 +-
 4 files changed, 118 insertions(+), 50 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.c b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
index 3b49c14128e2..d848ac58189c 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.c
@@ -1011,8 +1011,17 @@ static int bcmgenet_power_down(struct bcmgenet_priv *priv,
 		/* Power down LED */
 		if (priv->hw_params->flags & GENET_HAS_EXT) {
 			reg = bcmgenet_ext_readl(priv, EXT_EXT_PWR_MGMT);
-			reg |= (EXT_PWR_DOWN_PHY |
-				EXT_PWR_DOWN_DLL | EXT_PWR_DOWN_BIAS);
+			if (GENET_IS_V5(priv))
+				reg |= EXT_PWR_DOWN_PHY_EN |
+				       EXT_PWR_DOWN_PHY_RD |
+				       EXT_PWR_DOWN_PHY_SD |
+				       EXT_PWR_DOWN_PHY_RX |
+				       EXT_PWR_DOWN_PHY_TX |
+				       EXT_IDDQ_GLBL_PWR;
+			else
+				reg |= EXT_PWR_DOWN_PHY;
+
+			reg |= (EXT_PWR_DOWN_DLL | EXT_PWR_DOWN_BIAS);
 			bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
 
 			bcmgenet_phy_power_set(priv->dev, false);
@@ -1037,12 +1046,34 @@ static void bcmgenet_power_up(struct bcmgenet_priv *priv,
 
 	switch (mode) {
 	case GENET_POWER_PASSIVE:
-		reg &= ~(EXT_PWR_DOWN_DLL | EXT_PWR_DOWN_PHY |
-				EXT_PWR_DOWN_BIAS);
-		/* fallthrough */
+		reg &= ~(EXT_PWR_DOWN_DLL | EXT_PWR_DOWN_BIAS);
+		if (GENET_IS_V5(priv)) {
+			reg &= ~(EXT_PWR_DOWN_PHY_EN |
+				 EXT_PWR_DOWN_PHY_RD |
+				 EXT_PWR_DOWN_PHY_SD |
+				 EXT_PWR_DOWN_PHY_RX |
+				 EXT_PWR_DOWN_PHY_TX |
+				 EXT_IDDQ_GLBL_PWR);
+			reg |=   EXT_PHY_RESET;
+			bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
+			mdelay(1);
+
+			reg &=  ~EXT_PHY_RESET;
+		} else {
+			reg &= ~EXT_PWR_DOWN_PHY;
+			reg |= EXT_PWR_DN_EN_LD;
+		}
+		bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
+		bcmgenet_phy_power_set(priv->dev, true);
+		bcmgenet_mii_reset(priv->dev);
+		break;
+
 	case GENET_POWER_CABLE_SENSE:
 		/* enable APD */
-		reg |= EXT_PWR_DN_EN_LD;
+		if (!GENET_IS_V5(priv)) {
+			reg |= EXT_PWR_DN_EN_LD;
+			bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
+		}
 		break;
 	case GENET_POWER_WOL_MAGIC:
 		bcmgenet_wol_power_up_cfg(priv, mode);
@@ -1050,12 +1081,6 @@ static void bcmgenet_power_up(struct bcmgenet_priv *priv,
 	default:
 		break;
 	}
-
-	bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
-	if (mode == GENET_POWER_PASSIVE) {
-		bcmgenet_phy_power_set(priv->dev, true);
-		bcmgenet_mii_reset(priv->dev);
-	}
 }
 
 /* ioctl handle special commands that are not present in ethtool. */
@@ -3101,6 +3126,25 @@ static struct bcmgenet_hw_params bcmgenet_hw_params[] = {
 		.flags = GENET_HAS_40BITS | GENET_HAS_EXT |
 			 GENET_HAS_MDIO_INTR | GENET_HAS_MOCA_LINK_DET,
 	},
+	[GENET_V5] = {
+		.tx_queues = 4,
+		.tx_bds_per_q = 32,
+		.rx_queues = 0,
+		.rx_bds_per_q = 0,
+		.bp_in_en_shift = 17,
+		.bp_in_mask = 0x1ffff,
+		.hfb_filter_cnt = 48,
+		.hfb_filter_size = 128,
+		.qtag_mask = 0x3F,
+		.tbuf_offset = 0x0600,
+		.hfb_offset = 0x8000,
+		.hfb_reg_offset = 0xfc00,
+		.rdma_offset = 0x2000,
+		.tdma_offset = 0x4000,
+		.words_per_bd = 3,
+		.flags = GENET_HAS_40BITS | GENET_HAS_EXT |
+			 GENET_HAS_MDIO_INTR | GENET_HAS_MOCA_LINK_DET,
+	},
 };
 
 /* Infer hardware parameters from the detected GENET version */
@@ -3111,7 +3155,7 @@ static void bcmgenet_set_hw_params(struct bcmgenet_priv *priv)
 	u8 major;
 	u16 gphy_rev;
 
-	if (GENET_IS_V4(priv)) {
+	if (GENET_IS_V5(priv) || GENET_IS_V4(priv)) {
 		bcmgenet_dma_regs = bcmgenet_dma_regs_v3plus;
 		genet_dma_ring_regs = genet_dma_ring_regs_v4;
 		priv->dma_rx_chk_bit = DMA_RX_CHK_V3PLUS;
@@ -3136,7 +3180,9 @@ static void bcmgenet_set_hw_params(struct bcmgenet_priv *priv)
 	/* Read GENET HW version */
 	reg = bcmgenet_sys_readl(priv, SYS_REV_CTRL);
 	major = (reg >> 24 & 0x0f);
-	if (major == 5)
+	if (major == 6)
+		major = 5;
+	else if (major == 5)
 		major = 4;
 	else if (major == 0)
 		major = 1;
@@ -3164,16 +3210,22 @@ static void bcmgenet_set_hw_params(struct bcmgenet_priv *priv)
 	 */
 	gphy_rev = reg & 0xffff;
 
+	if (GENET_IS_V5(priv)) {
+		/* The EPHY revision should come from the MDIO registers of
+		 * the PHY not from GENET.
+		 */
+		if (gphy_rev != 0) {
+			pr_warn("GENET is reporting EPHY revision: 0x%04x\n",
+				gphy_rev);
+		}
 	/* This is the good old scheme, just GPHY major, no minor nor patch */
-	if ((gphy_rev & 0xf0) != 0)
+	} else if ((gphy_rev & 0xf0) != 0) {
 		priv->gphy_rev = gphy_rev << 8;
-
 	/* This is the new scheme, GPHY major rolls over with 0x10 = rev G0 */
-	else if ((gphy_rev & 0xff00) != 0)
+	} else if ((gphy_rev & 0xff00) != 0) {
 		priv->gphy_rev = gphy_rev;
-
 	/* This is reserved so should require special treatment */
-	else if (gphy_rev == 0 || gphy_rev == 0x01ff) {
+	} else if (gphy_rev == 0 || gphy_rev == 0x01ff) {
 		pr_warn("Invalid GPHY revision detected: 0x%04x\n", gphy_rev);
 		return;
 	}
@@ -3206,6 +3258,7 @@ static const struct of_device_id bcmgenet_match[] = {
 	{ .compatible = "brcm,genet-v2", .data = (void *)GENET_V2 },
 	{ .compatible = "brcm,genet-v3", .data = (void *)GENET_V3 },
 	{ .compatible = "brcm,genet-v4", .data = (void *)GENET_V4 },
+	{ .compatible = "brcm,genet-v5", .data = (void *)GENET_V5 },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, bcmgenet_match);
diff --git a/drivers/net/ethernet/broadcom/genet/bcmgenet.h b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
index 1e2dc34d331a..1001d9131ba8 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmgenet.h
+++ b/drivers/net/ethernet/broadcom/genet/bcmgenet.h
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014 Broadcom Corporation
+ * Copyright (c) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -351,8 +351,14 @@ struct bcmgenet_mib_counters {
 #define  EXT_PWR_DN_EN_LD		(1 << 3)
 #define  EXT_ENERGY_DET			(1 << 4)
 #define  EXT_IDDQ_FROM_PHY		(1 << 5)
+#define  EXT_IDDQ_GLBL_PWR		(1 << 7)
 #define  EXT_PHY_RESET			(1 << 8)
 #define  EXT_ENERGY_DET_MASK		(1 << 12)
+#define  EXT_PWR_DOWN_PHY_TX		(1 << 16)
+#define  EXT_PWR_DOWN_PHY_RX		(1 << 17)
+#define  EXT_PWR_DOWN_PHY_SD		(1 << 18)
+#define  EXT_PWR_DOWN_PHY_RD		(1 << 19)
+#define  EXT_PWR_DOWN_PHY_EN		(1 << 20)
 
 #define EXT_RGMII_OOB_CTRL		0x0C
 #define  RGMII_LINK			(1 << 4)
@@ -495,13 +501,15 @@ enum bcmgenet_version {
 	GENET_V1 = 1,
 	GENET_V2,
 	GENET_V3,
-	GENET_V4
+	GENET_V4,
+	GENET_V5
 };
 
 #define GENET_IS_V1(p)	((p)->version == GENET_V1)
 #define GENET_IS_V2(p)	((p)->version == GENET_V2)
 #define GENET_IS_V3(p)	((p)->version == GENET_V3)
 #define GENET_IS_V4(p)	((p)->version == GENET_V4)
+#define GENET_IS_V5(p)	((p)->version == GENET_V5)
 
 /* Hardware flags */
 #define GENET_HAS_40BITS	(1 << 0)
diff --git a/drivers/net/ethernet/broadcom/genet/bcmmii.c b/drivers/net/ethernet/broadcom/genet/bcmmii.c
index e87607621e62..8df47c90cfc5 100644
--- a/drivers/net/ethernet/broadcom/genet/bcmmii.c
+++ b/drivers/net/ethernet/broadcom/genet/bcmmii.c
@@ -1,7 +1,7 @@
 /*
  * Broadcom GENET MDIO routines
  *
- * Copyright (c) 2014 Broadcom Corporation
+ * Copyright (c) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 2 as
@@ -195,29 +195,31 @@ void bcmgenet_phy_power_set(struct net_device *dev, bool enable)
 	u32 reg = 0;
 
 	/* EXT_GPHY_CTRL is only valid for GENETv4 and onward */
-	if (!GENET_IS_V4(priv))
-		return;
-
-	reg = bcmgenet_ext_readl(priv, EXT_GPHY_CTRL);
-	if (enable) {
-		reg &= ~EXT_CK25_DIS;
-		bcmgenet_ext_writel(priv, reg, EXT_GPHY_CTRL);
-		mdelay(1);
-
-		reg &= ~(EXT_CFG_IDDQ_BIAS | EXT_CFG_PWR_DOWN);
-		reg |= EXT_GPHY_RESET;
+	if (GENET_IS_V4(priv)) {
+		reg = bcmgenet_ext_readl(priv, EXT_GPHY_CTRL);
+		if (enable) {
+			reg &= ~EXT_CK25_DIS;
+			bcmgenet_ext_writel(priv, reg, EXT_GPHY_CTRL);
+			mdelay(1);
+
+			reg &= ~(EXT_CFG_IDDQ_BIAS | EXT_CFG_PWR_DOWN);
+			reg |= EXT_GPHY_RESET;
+			bcmgenet_ext_writel(priv, reg, EXT_GPHY_CTRL);
+			mdelay(1);
+
+			reg &= ~EXT_GPHY_RESET;
+		} else {
+			reg |= EXT_CFG_IDDQ_BIAS | EXT_CFG_PWR_DOWN |
+			       EXT_GPHY_RESET;
+			bcmgenet_ext_writel(priv, reg, EXT_GPHY_CTRL);
+			mdelay(1);
+			reg |= EXT_CK25_DIS;
+		}
 		bcmgenet_ext_writel(priv, reg, EXT_GPHY_CTRL);
-		mdelay(1);
-
-		reg &= ~EXT_GPHY_RESET;
+		udelay(60);
 	} else {
-		reg |= EXT_CFG_IDDQ_BIAS | EXT_CFG_PWR_DOWN | EXT_GPHY_RESET;
-		bcmgenet_ext_writel(priv, reg, EXT_GPHY_CTRL);
 		mdelay(1);
-		reg |= EXT_CK25_DIS;
 	}
-	bcmgenet_ext_writel(priv, reg, EXT_GPHY_CTRL);
-	udelay(60);
 }
 
 static void bcmgenet_internal_phy_setup(struct net_device *dev)
@@ -227,10 +229,12 @@ static void bcmgenet_internal_phy_setup(struct net_device *dev)
 
 	/* Power up PHY */
 	bcmgenet_phy_power_set(dev, true);
-	/* enable APD */
-	reg = bcmgenet_ext_readl(priv, EXT_EXT_PWR_MGMT);
-	reg |= EXT_PWR_DN_EN_LD;
-	bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
+	if (!GENET_IS_V5(priv)) {
+		/* enable APD */
+		reg = bcmgenet_ext_readl(priv, EXT_EXT_PWR_MGMT);
+		reg |= EXT_PWR_DN_EN_LD;
+		bcmgenet_ext_writel(priv, reg, EXT_EXT_PWR_MGMT);
+	}
 	bcmgenet_mii_reset(dev);
 }
 
@@ -238,10 +242,12 @@ static void bcmgenet_moca_phy_setup(struct bcmgenet_priv *priv)
 {
 	u32 reg;
 
-	/* Speed settings are set in bcmgenet_mii_setup() */
-	reg = bcmgenet_sys_readl(priv, SYS_PORT_CTRL);
-	reg |= LED_ACT_SOURCE_MAC;
-	bcmgenet_sys_writel(priv, reg, SYS_PORT_CTRL);
+	if (!GENET_IS_V5(priv)) {
+		/* Speed settings are set in bcmgenet_mii_setup() */
+		reg = bcmgenet_sys_readl(priv, SYS_PORT_CTRL);
+		reg |= LED_ACT_SOURCE_MAC;
+		bcmgenet_sys_writel(priv, reg, SYS_PORT_CTRL);
+	}
 
 	if (priv->hw_params->flags & GENET_HAS_MOCA_LINK_DET)
 		fixed_phy_set_link_update(priv->phydev,
diff --git a/drivers/net/phy/mdio-bcm-unimac.c b/drivers/net/phy/mdio-bcm-unimac.c
index 8c73b2e771dd..34395230ce70 100644
--- a/drivers/net/phy/mdio-bcm-unimac.c
+++ b/drivers/net/phy/mdio-bcm-unimac.c
@@ -1,7 +1,7 @@
 /*
  * Broadcom UniMAC MDIO bus controller driver
  *
- * Copyright (C) 2014, Broadcom Corporation
+ * Copyright (C) 2014-2017 Broadcom
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -228,6 +228,7 @@ static int unimac_mdio_remove(struct platform_device *pdev)
 }
 
 static const struct of_device_id unimac_mdio_ids[] = {
+	{ .compatible = "brcm,genet-mdio-v5", },
 	{ .compatible = "brcm,genet-mdio-v4", },
 	{ .compatible = "brcm,genet-mdio-v3", },
 	{ .compatible = "brcm,genet-mdio-v2", },
-- 
2.11.1

Re: [PATCH net-next 12/12] net: bcmgenet: add support for the GENETv5 hardware

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> This commit adds support for the GENETv5 implementation.
> 
> The GENETv5 reports a major version of 6 instead of 5 so compensate
> for this when verifying the configuration of the driver.  Also the
> EPHY revision is now contained in the MDIO registers of the PHY so
> the EPHY revision of 0 in GENET_VER_FMT is expected for GENETv5.
> 
> Signed-off-by: Doug Berger <opendmb@gmail.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH net-next 12/12] net: bcmgenet: add support for the GENETv5 hardware

[Florian Fainelli]

On 03/13/2017 05:41 PM, Doug Berger wrote:
> This commit adds support for the GENETv5 implementation.
> 
> The GENETv5 reports a major version of 6 instead of 5 so compensate
> for this when verifying the configuration of the driver.  Also the
> EPHY revision is now contained in the MDIO registers of the PHY so
> the EPHY revision of 0 in GENET_VER_FMT is expected for GENETv5.
> 
> Signed-off-by: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

Reviewed-by: Florian Fainelli <f.fainelli-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
-- 
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH net-next 00/12] net: bcmgenet: add support for GENETv5

[David Miller]

From: Doug Berger <opendmb@gmail.com>
Date: Mon, 13 Mar 2017 17:41:30 -0700

> This collection of patches contains changes related to adding
> support for the BCM7260, BCM7268, and BCM7271 devices that
> contain a new version of the GENET MAC IP block (v5) and a new
> fast ethernet (10/100BASE-T) internal PHY.
> 
> These patches were originally developed on top of the bug fixes
> of the "[PATCH v2 net 0/8] net: bcmgenet: minor bug fixes" patch
> set previously accepted into the net repository, but this
> submission is designed to be applied to the current net-next
> that does not yet include them. As a result there will be some
> merge conflicts that I would be happy to help resolve if desired.
> 
> Specifically, conflicts should occur with these patches from the
> minor bug fixes set:
> [PATCH v2 net 3/8] net: bcmgenet: reserved phy revisions must be checked first
> [PATCH v2 net 5/8] net: bcmgenet: synchronize irq0 status between the isr and task
> [PATCH v2 net 8/8] net: bcmgenet: decouple flow control from bcmgenet_tx_reclaim

Series applied, thanks Doug.

Re: [PATCH net-next 00/12] net: bcmgenet: add support for GENETv5

[David Miller]

From: Doug Berger <opendmb-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Date: Mon, 13 Mar 2017 17:41:30 -0700

> This collection of patches contains changes related to adding
> support for the BCM7260, BCM7268, and BCM7271 devices that
> contain a new version of the GENET MAC IP block (v5) and a new
> fast ethernet (10/100BASE-T) internal PHY.
> 
> These patches were originally developed on top of the bug fixes
> of the "[PATCH v2 net 0/8] net: bcmgenet: minor bug fixes" patch
> set previously accepted into the net repository, but this
> submission is designed to be applied to the current net-next
> that does not yet include them. As a result there will be some
> merge conflicts that I would be happy to help resolve if desired.
> 
> Specifically, conflicts should occur with these patches from the
> minor bug fixes set:
> [PATCH v2 net 3/8] net: bcmgenet: reserved phy revisions must be checked first
> [PATCH v2 net 5/8] net: bcmgenet: synchronize irq0 status between the isr and task
> [PATCH v2 net 8/8] net: bcmgenet: decouple flow control from bcmgenet_tx_reclaim

Series applied, thanks Doug.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html