Skip to contentSkip to conversation

net: bcmgenet: add support for GENETv5

PATCH In review Doug Berger 6 participants 33 messages Mar 14, 2017 – Mar 20, 2017

Subsystems (6)
lore
Share & tools
lore

Doug Berger

author maintainer
#0 lore raw

cover net: bcmgenet: add support for GENETv5

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(-)
Signature · 6 lines
2.11.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author maintainer
#1 lore raw

PATCH 1/12 net: phy: bcm-phylib: replace obsolete EEE macro references

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.

---
 drivers/net/phy/bcm-phy-lib.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
drivers/net/phy/bcm-phy-lib.c +3 -3
index ab9ad689617c..9656dbeb5de5 100644
@@ -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);
Signature · 1 line
2.11.1
Reviewed-by on v1 1/12
#2 lore raw

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

Quoted 5 lines · 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.

Signature · 5 lines
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author maintainer
#3 lore raw

PATCH 2/12 net: phy: bcm7xxx: add support for 28nm EPHY

In reply to Doug Berger

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

---
 drivers/net/phy/bcm7xxx.c | 215 +++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/brcmphy.h   |   3 +
 2 files changed, 216 insertions(+), 2 deletions(-)
drivers/net/phy/bcm7xxx.c +213 -2
index d1c2614dad3a..caa9f6e17f34 100644
@@ -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, },
include/linux/brcmphy.h +3 -0
index 55e517130311..abcda9b458ab 100644
@@ -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
Signature · 1 line
2.11.1
Reviewed-by on v1 2/12
#4 lore raw

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

Quoted 4 lines · Doug Berger

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

Signature · 5 lines
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#5 lore raw

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

In reply to Doug Berger
Quoted 10 lines · Doug Berger
drivers/net/phy/bcm7xxx.c +10 -0
+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;
+

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 321

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+	/* 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...

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 331

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+	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.

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 344

Quoted 13 lines
drivers/net/phy/bcm7xxx.c +13 -0
+	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);
Quoted 14 lines
drivers/net/phy/bcm7xxx.c +14 -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);

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 370

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -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);

Quoted 7 lines
drivers/net/phy/bcm7xxx.c +7 -0
+}
+
+static int bcm7xxx_28nm_ephy_eee_enable(struct phy_device *phydev)
+{
+	int ret;
+
+	/* set shadow mode 2 */

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 387

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+	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...

Quoted 36 lines
drivers/net/phy/bcm7xxx.c +36 -0
+	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 */

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 433

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+	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(.....); ?

Quoted 1 line

+}

Andrew
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#6 lore raw

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

Quoted 24 lines · Andrew Lunn
Quoted 10 lines · Doug Berger
drivers/net/phy/bcm7xxx.c +10 -0
+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;
+

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 321

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+	/* 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

Quoted 2 lines

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;

Quoted 4 lines
Quoted 4 lines
drivers/net/phy/bcm7xxx.c +4 -0
+	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;
Quoted 9 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 331

Quoted 6 lines
drivers/net/phy/bcm7xxx.c +6 -0
+
+	/* 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;

Quoted 16 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 344

Quoted 13 lines
drivers/net/phy/bcm7xxx.c +13 -0
+	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:

Quoted 3 lines
	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.

Quoted 27 lines
Quoted 14 lines
drivers/net/phy/bcm7xxx.c +14 -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);

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 370

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -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

Quoted 3 lines

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

Same remark as above.

Quoted 20 lines
Quoted 7 lines
drivers/net/phy/bcm7xxx.c +7 -0
+}
+
+static int bcm7xxx_28nm_ephy_eee_enable(struct phy_device *phydev)
+{
+	int ret;
+
+	/* set shadow mode 2 */

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 387

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+	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...

Quoted 4 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 391

Quoted 4 lines
drivers/net/phy/bcm7xxx.c +4 -0
+	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.

Quoted 10 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 401

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+
+	/* 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.

Quoted 9 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 410

Quoted 9 lines
drivers/net/phy/bcm7xxx.c +9 -0
+
+	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...

Quoted 10 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 420

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+
+	/* 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.

Quoted 7 lines
Quoted 7 lines
drivers/net/phy/bcm7xxx.c +7 -0
+
+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;
Quoted 9 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 433

Quoted 6 lines
drivers/net/phy/bcm7xxx.c +6 -0
+
+	/* 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.

Quoted 4 lines
Quoted 1 line

+}

Andrew

Thanks again,
Doug

maintainer
#7 lore raw

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

Quoted 25 lines · Doug Berger
Quoted 24 lines · Andrew Lunn
Quoted 10 lines · Doug Berger
drivers/net/phy/bcm7xxx.c +10 -0
+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;
+

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 321

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+	/* 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

Quoted 8 lines
Quoted 2 lines

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.

Quoted 8 lines
Quoted 3 lines
	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.

Quoted 8 lines
Quoted 8 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 387

Quoted 5 lines
drivers/net/phy/bcm7xxx.c +5 -0
+	/* 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...

Quoted 8 lines
Quoted 4 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 391

Quoted 4 lines
drivers/net/phy/bcm7xxx.c +4 -0
+	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.

Quoted 51 lines
Quoted 10 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 401

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+
+	/* 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.

Quoted 9 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 410

Quoted 9 lines
drivers/net/phy/bcm7xxx.c +9 -0
+
+	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...

Quoted 10 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 420

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+
+	/* 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.

Quoted 7 lines
Quoted 7 lines
drivers/net/phy/bcm7xxx.c +7 -0
+
+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;
Quoted 9 lines

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 433

Quoted 6 lines
drivers/net/phy/bcm7xxx.c +6 -0
+
+	/* Restart autoneg */
+	phy_write(phydev, MII_BMCR,
+		  (BMCR_SPEED100 | BMCR_ANENABLE | BMCR_ANRESTART));
+
+	return 0;

return phy_write(.....); ?

Quoted 5 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#8 lore raw

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

Quoted 35 lines · Andrew Lunn
Quoted 25 lines · Doug Berger
Quoted 24 lines · Andrew Lunn
Quoted 10 lines · Doug Berger
drivers/net/phy/bcm7xxx.c +10 -0
+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;
+

drivers/net/phy/bcm7xxx.c static int phy_set_clr_bits(struct phy_device *dev, int location, · line 321

Quoted 10 lines
drivers/net/phy/bcm7xxx.c +10 -0
+	/* 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

Quoted 8 lines
Quoted 2 lines

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.

Quoted 6 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#9 lore raw

PATCH 3/12 net: bcmgenet: simplify circular pointer arithmetic

In reply to 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).

---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 19 +++++--------------
 1 file changed, 5 insertions(+), 14 deletions(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +5 -14
index f92896835d2a..2c008b09c4e3 100644
@@ -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);
Signature · 1 line
2.11.1
Reviewed-by on v1 3/12
#10 lore raw

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

Quoted 4 lines · 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).

Signature · 5 lines
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author maintainer
#11 lore raw

PATCH 4/12 net: bcmgenet: remove meaningless lines

In reply to Doug Berger

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

---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +3 -10
index 2c008b09c4e3..22c92f5a9829 100644
@@ -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 */
Signature · 1 line
2.11.1
Reviewed-by on v1 4/12
#12 lore raw

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

Quoted 4 lines · Doug Berger

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

Signature · 5 lines
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author maintainer
#13 lore raw

PATCH 5/12 net: bcmgenet: manage dma interrupts in napi code

In reply to 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.

---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 39 +++++++++++++++-----------
 1 file changed, 22 insertions(+), 17 deletions(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +22 -17
index 22c92f5a9829..9be884021679 100644
@@ -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);
Signature · 1 line
2.11.1
author maintainer
#15 lore raw

PATCH 6/12 net: bcmgenet: remove handling of wol interrupts from isr0

In reply to 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.

---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c     | 10 +---------
 drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c | 15 +--------------
 2 files changed, 2 insertions(+), 23 deletions(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +1 -9
index 9be884021679..661ca1b39c89 100644
@@ -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);
 	}
drivers/net/ethernet/broadcom/genet/bcmgenet_wol.c +1 -14
index b97122926d3a..2fbd027f0148 100644
@@ -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);
 }
Signature · 6 lines
2.11.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reviewed-by on v1 6/12
#16 lore raw

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

Quoted 7 lines · 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.

Signature · 5 lines
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author maintainer
#17 lore raw

PATCH 7/12 net: bcmgenet: clear status to reduce spurious interrupts

In reply to 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.

---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +20 -1
index 661ca1b39c89..1f94ba1773dd 100644
@@ -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) &
Signature · 1 line
2.11.1
Reviewed-by on v1 7/12
#18 lore raw

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

Quoted 8 lines · 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.

Signature · 5 lines
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author maintainer
#19 lore raw

PATCH 8/12 net: bcmgenet: correct return value of __bcmgenet_tx_reclaim

In reply to 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.

---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +2 -2
index 1f94ba1773dd..d90d366b286f 100644
@@ -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,
Signature · 1 line
2.11.1
Reviewed-by on v1 8/12
#20 lore raw

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

Quoted 10 lines · 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.

Signature · 1 line
Florian
author maintainer
#21 lore raw

PATCH 9/12 net: bcmgenet: return EOPNOTSUPP for unknown ioctl commands

In reply to 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.

---
 drivers/net/ethernet/broadcom/genet/bcmgenet.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +3 -16
index d90d366b286f..3b49c14128e2 100644
@@ -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,
Signature · 1 line
2.11.1
Reviewed-by on v1 9/12
#22 lore raw

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

Quoted 8 lines · 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.

Signature · 1 line
Florian
#23 lore raw

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

In reply to Doug Berger

From: Doug Berger

Quoted 7 lines

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

author maintainer
#24 lore raw

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

Quoted 6 lines · David Laight

From: Doug Berger

Quoted 5 lines

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.

Quoted 6 lines
Quoted 2 lines

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.

Quoted 3 lines

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

author maintainer
#25 lore raw

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

In reply to 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.

---
 Documentation/devicetree/bindings/net/brcm,bcmgenet.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/net/brcm,bcmgenet.txt +6 -3
index 10587bdadbbe..01a70463cbc5 100644
@@ -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
Signature · 1 line
2.11.1
Reviewed-by on v1 10/12
#26 lore raw

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

Quoted 8 lines · 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.

Signature · 5 lines
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#27 lore raw

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

In reply to Doug Berger
Quoted 6 lines · 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.

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

Quoted 11 lines
---
 Documentation/devicetree/bindings/net/brcm,bcmgenet.txt | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Documentation/devicetree/bindings/net/brcm,bcmgenet.txt +0 -0
index 10587bdadbbe..01a70463cbc5 100644

Documentation/devicetree/bindings/net/brcm,bcmgenet.txt Required properties: · line 10

Quoted 10 lines
Documentation/devicetree/bindings/net/brcm,bcmgenet.txt +4 -3
@@ -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?

Quoted 8 lines
Documentation/devicetree/bindings/net/brcm,bcmgenet.txt +2 -0
+  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
Signature · 1 line
2.11.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

author maintainer
#28 lore raw

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

In reply to 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.

---
 Documentation/devicetree/bindings/net/brcm,bcmgenet.txt    | 10 +++++-----
 Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt |  5 +++--
 2 files changed, 8 insertions(+), 7 deletions(-)
Documentation/devicetree/bindings/net/brcm,bcmgenet.txt +5 -5
index 01a70463cbc5..26c77d985faf 100644
@@ -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
Documentation/devicetree/bindings/net/brcm,unimac-mdio.txt +3 -2
index ab0bb4247d14..4648948f7c3b 100644
@@ -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"
Signature · 1 line
2.11.1
author maintainer
#30 lore raw

PATCH 12/12 net: bcmgenet: add support for the GENETv5 hardware

In reply to 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.

---
 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(-)
drivers/net/ethernet/broadcom/genet/bcmgenet.c +72 -19
index 3b49c14128e2..d848ac58189c 100644
@@ -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);
drivers/net/ethernet/broadcom/genet/bcmgenet.h +10 -2
index 1e2dc34d331a..1001d9131ba8 100644
@@ -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)
drivers/net/ethernet/broadcom/genet/bcmmii.c +34 -28

90 lines not on this page · raw message

drivers/net/phy/mdio-bcm-unimac.c +2 -1
index 8c73b2e771dd..34395230ce70 100644
@@ -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", },
Signature · 1 line
2.11.1
Reviewed-by on v1 12/12
#31 lore raw

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

Quoted 8 lines · 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.

Signature · 5 lines
Florian
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#32 lore raw
In reply to Doug Berger

From: Doug Berger <[email protected]>
Date: Mon, 13 Mar 2017 17:41:30 -0700

Quoted 12 lines

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.

Quoted 5 lines

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 [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html