Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1599954 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm EPHY Doug Berger <opendmb@gmail.com> - 2017-03-14 01:50 +0100
    Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm  EPHY Florian Fainelli <f.fainelli@gmail.com> - 2017-03-14 02:00 +0100
    Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm  EPHY Andrew Lunn <andrew@lunn.ch> - 2017-03-14 02:10 +0100
      Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm  EPHY Doug Berger <opendmb@gmail.com> - 2017-03-14 03:10 +0100
        Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm  EPHY Andrew Lunn <andrew@lunn.ch> - 2017-03-14 03:50 +0100
          Re: [PATCH net-next 02/12] net: phy: bcm7xxx: add support for 28nm  EPHY Doug Berger <opendmb@gmail.com> - 2017-03-14 18:30 +0100

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

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

[toc] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web