[PATCH net-next v2 0/3] net: phy: broadcom: Cleanups and APD

[PATCH net-next v2 0/3] net: phy: broadcom: Cleanups and APD

[Florian Fainelli]

This patch series cleans up the brcmphy.h header and its numerous unused
phydev->dev_flags, fixes the RXC/TXC clock disabling bit and allows the
BCM54210E PHY to utilize APD.

Changes in v2:

- dropped the patch that attempted to fix a possible discrepancy between
  the datasheet and the actual hardware
- added a patch to remove a forward declaration
- do additional flags cleanup

Florian Fainelli (3):
  net: phy: broadcom: Avoid forward for bcm54xx_config_clock_delay()
  net: phy: broadcom: Remove unused flags
  net: phy: broadcom: Allow BCM54210E to configure APD

 drivers/net/phy/broadcom.c | 101 +++++++++++++++++++++----------------
 include/linux/brcmphy.h    |  23 ++++-----
 2 files changed, 66 insertions(+), 58 deletions(-)

-- 
2.25.1

[PATCH net-next v2 1/3] net: phy: broadcom: Avoid forward for bcm54xx_config_clock_delay()

[Florian Fainelli]

Avoid a forward declaration by moving the callers of
bcm54xx_config_clock_delay() below its body.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/broadcom.c | 74 +++++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 38 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 0472b3470c59..4142f69c1530 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -26,44 +26,6 @@ MODULE_DESCRIPTION("Broadcom PHY driver");
 MODULE_AUTHOR("Maciej W. Rozycki");
 MODULE_LICENSE("GPL");
 
-static int bcm54xx_config_clock_delay(struct phy_device *phydev);
-
-static int bcm54210e_config_init(struct phy_device *phydev)
-{
-	int val;
-
-	bcm54xx_config_clock_delay(phydev);
-
-	if (phydev->dev_flags & PHY_BRCM_EN_MASTER_MODE) {
-		val = phy_read(phydev, MII_CTRL1000);
-		val |= CTL1000_AS_MASTER | CTL1000_ENABLE_MASTER;
-		phy_write(phydev, MII_CTRL1000, val);
-	}
-
-	return 0;
-}
-
-static int bcm54612e_config_init(struct phy_device *phydev)
-{
-	int reg;
-
-	bcm54xx_config_clock_delay(phydev);
-
-	/* Enable CLK125 MUX on LED4 if ref clock is enabled. */
-	if (!(phydev->dev_flags & PHY_BRCM_RX_REFCLK_UNUSED)) {
-		int err;
-
-		reg = bcm_phy_read_exp(phydev, BCM54612E_EXP_SPARE0);
-		err = bcm_phy_write_exp(phydev, BCM54612E_EXP_SPARE0,
-					BCM54612E_LED4_CLK125OUT_EN | reg);
-
-		if (err < 0)
-			return err;
-	}
-
-	return 0;
-}
-
 static int bcm54xx_config_clock_delay(struct phy_device *phydev)
 {
 	int rc, val;
@@ -105,6 +67,42 @@ static int bcm54xx_config_clock_delay(struct phy_device *phydev)
 	return 0;
 }
 
+static int bcm54210e_config_init(struct phy_device *phydev)
+{
+	int val;
+
+	bcm54xx_config_clock_delay(phydev);
+
+	if (phydev->dev_flags & PHY_BRCM_EN_MASTER_MODE) {
+		val = phy_read(phydev, MII_CTRL1000);
+		val |= CTL1000_AS_MASTER | CTL1000_ENABLE_MASTER;
+		phy_write(phydev, MII_CTRL1000, val);
+	}
+
+	return 0;
+}
+
+static int bcm54612e_config_init(struct phy_device *phydev)
+{
+	int reg;
+
+	bcm54xx_config_clock_delay(phydev);
+
+	/* Enable CLK125 MUX on LED4 if ref clock is enabled. */
+	if (!(phydev->dev_flags & PHY_BRCM_RX_REFCLK_UNUSED)) {
+		int err;
+
+		reg = bcm_phy_read_exp(phydev, BCM54612E_EXP_SPARE0);
+		err = bcm_phy_write_exp(phydev, BCM54612E_EXP_SPARE0,
+					BCM54612E_LED4_CLK125OUT_EN | reg);
+
+		if (err < 0)
+			return err;
+	}
+
+	return 0;
+}
+
 /* Needs SMDSP clock enabled via bcm54xx_phydsp_config() */
 static int bcm50610_a0_workaround(struct phy_device *phydev)
 {
-- 
2.25.1

Re: [PATCH net-next v2 1/3] net: phy: broadcom: Avoid forward for bcm54xx_config_clock_delay()

[Vladimir Oltean]

On Fri, Feb 12, 2021 at 07:46:30PM -0800, Florian Fainelli wrote:
> Avoid a forward declaration by moving the callers of
> bcm54xx_config_clock_delay() below its body.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH net-next v2 2/3] net: phy: broadcom: Remove unused flags

[Florian Fainelli]

We have a number of unused flags defined today and since we are scarce
on space and may need to introduce new flags in the future remove and
shift every existing flag down into a contiguous assignment.
PHY_BCM_FLAGS_MODE_1000BX was only used internally for the BCM54616S
PHY, so we allocate a driver private structure instead to store that
flag instead of canibalizing one from phydev->dev_flags for that
purpose.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/broadcom.c | 19 ++++++++++++++++---
 include/linux/brcmphy.h    | 21 ++++++++-------------
 2 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 4142f69c1530..3ce266ab521b 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -381,10 +381,21 @@ static int bcm5481_config_aneg(struct phy_device *phydev)
 	return ret;
 }
 
+struct bcm54616s_phy_priv {
+	bool mode_1000bx_en;
+};
+
 static int bcm54616s_probe(struct phy_device *phydev)
 {
+	struct bcm54616s_phy_priv *priv;
 	int val, intf_sel;
 
+	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	phydev->priv = priv;
+
 	val = bcm_phy_read_shadow(phydev, BCM54XX_SHD_MODE);
 	if (val < 0)
 		return val;
@@ -407,7 +418,7 @@ static int bcm54616s_probe(struct phy_device *phydev)
 		 * 1000BASE-X configuration.
 		 */
 		if (!(val & BCM54616S_100FX_MODE))
-			phydev->dev_flags |= PHY_BCM_FLAGS_MODE_1000BX;
+			priv->mode_1000bx_en = true;
 
 		phydev->port = PORT_FIBRE;
 	}
@@ -417,10 +428,11 @@ static int bcm54616s_probe(struct phy_device *phydev)
 
 static int bcm54616s_config_aneg(struct phy_device *phydev)
 {
+	struct bcm54616s_phy_priv *priv = phydev->priv;
 	int ret;
 
 	/* Aneg firstly. */
-	if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
+	if (priv->mode_1000bx_en)
 		ret = genphy_c37_config_aneg(phydev);
 	else
 		ret = genphy_config_aneg(phydev);
@@ -433,9 +445,10 @@ static int bcm54616s_config_aneg(struct phy_device *phydev)
 
 static int bcm54616s_read_status(struct phy_device *phydev)
 {
+	struct bcm54616s_phy_priv *priv = phydev->priv;
 	int err;
 
-	if (phydev->dev_flags & PHY_BCM_FLAGS_MODE_1000BX)
+	if (priv->mode_1000bx_en)
 		err = genphy_c37_read_status(phydev);
 	else
 		err = genphy_read_status(phydev);
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index de9430d55c90..844dcfe789a2 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -61,19 +61,14 @@
 #define PHY_BCM_OUI_5			0x03625e00
 #define PHY_BCM_OUI_6			0xae025000
 
-#define PHY_BCM_FLAGS_MODE_COPPER	0x00000001
-#define PHY_BCM_FLAGS_MODE_1000BX	0x00000002
-#define PHY_BCM_FLAGS_INTF_SGMII	0x00000010
-#define PHY_BCM_FLAGS_INTF_XAUI		0x00000020
-#define PHY_BRCM_WIRESPEED_ENABLE	0x00000100
-#define PHY_BRCM_AUTO_PWRDWN_ENABLE	0x00000200
-#define PHY_BRCM_RX_REFCLK_UNUSED	0x00000400
-#define PHY_BRCM_STD_IBND_DISABLE	0x00000800
-#define PHY_BRCM_EXT_IBND_RX_ENABLE	0x00001000
-#define PHY_BRCM_EXT_IBND_TX_ENABLE	0x00002000
-#define PHY_BRCM_CLEAR_RGMII_MODE	0x00004000
-#define PHY_BRCM_DIS_TXCRXC_NOENRGY	0x00008000
-#define PHY_BRCM_EN_MASTER_MODE		0x00010000
+#define PHY_BRCM_AUTO_PWRDWN_ENABLE	0x00000001
+#define PHY_BRCM_RX_REFCLK_UNUSED	0x00000002
+#define PHY_BRCM_STD_IBND_DISABLE	0x00000004
+#define PHY_BRCM_EXT_IBND_RX_ENABLE	0x00000008
+#define PHY_BRCM_EXT_IBND_TX_ENABLE	0x00000010
+#define PHY_BRCM_CLEAR_RGMII_MODE	0x00000020
+#define PHY_BRCM_DIS_TXCRXC_NOENRGY	0x00000040
+#define PHY_BRCM_EN_MASTER_MODE		0x00000080
 
 /* Broadcom BCM7xxx specific workarounds */
 #define PHY_BRCM_7XXX_REV(x)		(((x) >> 8) & 0xff)
-- 
2.25.1

Re: [PATCH net-next v2 2/3] net: phy: broadcom: Remove unused flags

[Vladimir Oltean]

On Fri, Feb 12, 2021 at 07:46:31PM -0800, Florian Fainelli wrote:
> We have a number of unused flags defined today and since we are scarce
> on space and may need to introduce new flags in the future remove and
> shift every existing flag down into a contiguous assignment.
> PHY_BCM_FLAGS_MODE_1000BX was only used internally for the BCM54616S
> PHY, so we allocate a driver private structure instead to store that
> flag instead of canibalizing one from phydev->dev_flags for that
> purpose.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

So you want to remove the IBND dev_flags separately, okay.

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

[PATCH net-next v2 3/3] net: phy: broadcom: Allow BCM54210E to configure APD

[Florian Fainelli]

BCM54210E/BCM50212E has been verified to work correctly with the
auto-power down configuration done by bcm54xx_adjust_rxrefclk(), add it
to the list of PHYs working.

While we are at it, provide an appropriate name for the bit we are
changing which disables the RXC and TXC during auto-power down when
there is no energy on the cable.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/broadcom.c | 8 +++++---
 include/linux/brcmphy.h    | 2 +-
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
index 3ce266ab521b..91fbd26c809e 100644
--- a/drivers/net/phy/broadcom.c
+++ b/drivers/net/phy/broadcom.c
@@ -193,6 +193,7 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
 	if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM57780 &&
 	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610 &&
 	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610M &&
+	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54210E &&
 	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54810 &&
 	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54811)
 		return;
@@ -227,9 +228,10 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
 		val |= BCM54XX_SHD_SCR3_DLLAPD_DIS;
 
 	if (phydev->dev_flags & PHY_BRCM_DIS_TXCRXC_NOENRGY) {
-		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
-		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54811)
-			val |= BCM54810_SHD_SCR3_TRDDAPD;
+		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E ||
+		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
+		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E)
+			val |= BCM54XX_SHD_SCR3_RXCTXC_DIS;
 		else
 			val |= BCM54XX_SHD_SCR3_TRDDAPD;
 	}
diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
index 844dcfe789a2..16597d3fa011 100644
--- a/include/linux/brcmphy.h
+++ b/include/linux/brcmphy.h
@@ -193,6 +193,7 @@
 #define  BCM54XX_SHD_SCR3_DEF_CLK125	0x0001
 #define  BCM54XX_SHD_SCR3_DLLAPD_DIS	0x0002
 #define  BCM54XX_SHD_SCR3_TRDDAPD	0x0004
+#define  BCM54XX_SHD_SCR3_RXCTXC_DIS	0x0100
 
 /* 01010: Auto Power-Down */
 #define BCM54XX_SHD_APD			0x0a
@@ -253,7 +254,6 @@
 #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN	(1 << 0)
 #define BCM54810_SHD_CLK_CTL			0x3
 #define BCM54810_SHD_CLK_CTL_GTXCLK_EN		(1 << 9)
-#define BCM54810_SHD_SCR3_TRDDAPD		0x0100
 
 /* BCM54612E Registers */
 #define BCM54612E_EXP_SPARE0		(MII_BCM54XX_EXP_SEL_ETC + 0x34)
-- 
2.25.1

Re: [PATCH net-next v2 3/3] net: phy: broadcom: Allow BCM54210E to configure APD

[Vladimir Oltean]

On Fri, Feb 12, 2021 at 07:46:32PM -0800, Florian Fainelli wrote:
> BCM54210E/BCM50212E has been verified to work correctly with the
> auto-power down configuration done by bcm54xx_adjust_rxrefclk(), add it
> to the list of PHYs working.
> 
> While we are at it, provide an appropriate name for the bit we are
> changing which disables the RXC and TXC during auto-power down when
> there is no energy on the cable.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

>  drivers/net/phy/broadcom.c | 8 +++++---
>  include/linux/brcmphy.h    | 2 +-
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 3ce266ab521b..91fbd26c809e 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -193,6 +193,7 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
>  	if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM57780 &&
>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610 &&
>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610M &&
> +	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54210E &&
>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54810 &&
>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54811)
>  		return;
> @@ -227,9 +228,10 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
>  		val |= BCM54XX_SHD_SCR3_DLLAPD_DIS;
>  
>  	if (phydev->dev_flags & PHY_BRCM_DIS_TXCRXC_NOENRGY) {
> -		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
> -		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54811)
> -			val |= BCM54810_SHD_SCR3_TRDDAPD;
> +		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E ||
> +		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
> +		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E)
> +			val |= BCM54XX_SHD_SCR3_RXCTXC_DIS;
>  		else
>  			val |= BCM54XX_SHD_SCR3_TRDDAPD;
>  	}
> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> index 844dcfe789a2..16597d3fa011 100644
> --- a/include/linux/brcmphy.h
> +++ b/include/linux/brcmphy.h
> @@ -193,6 +193,7 @@
>  #define  BCM54XX_SHD_SCR3_DEF_CLK125	0x0001
>  #define  BCM54XX_SHD_SCR3_DLLAPD_DIS	0x0002
>  #define  BCM54XX_SHD_SCR3_TRDDAPD	0x0004
> +#define  BCM54XX_SHD_SCR3_RXCTXC_DIS	0x0100

Curiously enough, my BCM5464R datasheet does say:

The TXC and RXC outputs can be disabled during auto-power down by setting the “1000BASE-T/100BASE-TX/10BASE-T
Spare Control 3 Register (Address 1Ch, Shadow Value 00101),” bit 8 =1.

but when I go to the definition of the register, bit 8 is hidden. Odd.

How can I ensure that the auto power down feature is doing something?

>  
>  /* 01010: Auto Power-Down */
>  #define BCM54XX_SHD_APD			0x0a
> @@ -253,7 +254,6 @@
>  #define BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN	(1 << 0)
>  #define BCM54810_SHD_CLK_CTL			0x3
>  #define BCM54810_SHD_CLK_CTL_GTXCLK_EN		(1 << 9)
> -#define BCM54810_SHD_SCR3_TRDDAPD		0x0100
>  
>  /* BCM54612E Registers */
>  #define BCM54612E_EXP_SPARE0		(MII_BCM54XX_EXP_SEL_ETC + 0x34)
> -- 
> 2.25.1
> 

Re: [PATCH net-next v2 3/3] net: phy: broadcom: Allow BCM54210E to configure APD

[Florian Fainelli]

On 2/13/2021 2:42 AM, Vladimir Oltean wrote:
> On Fri, Feb 12, 2021 at 07:46:32PM -0800, Florian Fainelli wrote:
>> BCM54210E/BCM50212E has been verified to work correctly with the
>> auto-power down configuration done by bcm54xx_adjust_rxrefclk(), add it
>> to the list of PHYs working.
>>
>> While we are at it, provide an appropriate name for the bit we are
>> changing which disables the RXC and TXC during auto-power down when
>> there is no energy on the cable.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
> 
> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
> 
>>  drivers/net/phy/broadcom.c | 8 +++++---
>>  include/linux/brcmphy.h    | 2 +-
>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>> index 3ce266ab521b..91fbd26c809e 100644
>> --- a/drivers/net/phy/broadcom.c
>> +++ b/drivers/net/phy/broadcom.c
>> @@ -193,6 +193,7 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
>>  	if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM57780 &&
>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610 &&
>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610M &&
>> +	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54210E &&
>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54810 &&
>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54811)
>>  		return;
>> @@ -227,9 +228,10 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
>>  		val |= BCM54XX_SHD_SCR3_DLLAPD_DIS;
>>  
>>  	if (phydev->dev_flags & PHY_BRCM_DIS_TXCRXC_NOENRGY) {
>> -		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
>> -		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54811)
>> -			val |= BCM54810_SHD_SCR3_TRDDAPD;
>> +		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E ||
>> +		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
>> +		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E)
>> +			val |= BCM54XX_SHD_SCR3_RXCTXC_DIS;
>>  		else
>>  			val |= BCM54XX_SHD_SCR3_TRDDAPD;
>>  	}
>> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
>> index 844dcfe789a2..16597d3fa011 100644
>> --- a/include/linux/brcmphy.h
>> +++ b/include/linux/brcmphy.h
>> @@ -193,6 +193,7 @@
>>  #define  BCM54XX_SHD_SCR3_DEF_CLK125	0x0001
>>  #define  BCM54XX_SHD_SCR3_DLLAPD_DIS	0x0002
>>  #define  BCM54XX_SHD_SCR3_TRDDAPD	0x0004
>> +#define  BCM54XX_SHD_SCR3_RXCTXC_DIS	0x0100
> 
> Curiously enough, my BCM5464R datasheet does say:
> 
> The TXC and RXC outputs can be disabled during auto-power down by setting the “1000BASE-T/100BASE-TX/10BASE-T
> Spare Control 3 Register (Address 1Ch, Shadow Value 00101),” bit 8 =1.
> 
> but when I go to the definition of the register, bit 8 is hidden. Odd.
> 
> How can I ensure that the auto power down feature is doing something?

I am trying to confirm what the expected power levels should be from the
54210E product engineer so I can give you an estimate of what you should
see with and without while measure the PHY's regulators.
-- 
Florian

Re: [PATCH net-next v2 3/3] net: phy: broadcom: Allow BCM54210E to configure APD

[Florian Fainelli]

On 2/14/2021 8:29 PM, Florian Fainelli wrote:
> 
> 
> On 2/13/2021 2:42 AM, Vladimir Oltean wrote:
>> On Fri, Feb 12, 2021 at 07:46:32PM -0800, Florian Fainelli wrote:
>>> BCM54210E/BCM50212E has been verified to work correctly with the
>>> auto-power down configuration done by bcm54xx_adjust_rxrefclk(), add it
>>> to the list of PHYs working.
>>>
>>> While we are at it, provide an appropriate name for the bit we are
>>> changing which disables the RXC and TXC during auto-power down when
>>> there is no energy on the cable.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>
>> Reviewed-by: Vladimir Oltean <olteanv@gmail.com>
>>
>>>  drivers/net/phy/broadcom.c | 8 +++++---
>>>  include/linux/brcmphy.h    | 2 +-
>>>  2 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
>>> index 3ce266ab521b..91fbd26c809e 100644
>>> --- a/drivers/net/phy/broadcom.c
>>> +++ b/drivers/net/phy/broadcom.c
>>> @@ -193,6 +193,7 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
>>>  	if (BRCM_PHY_MODEL(phydev) != PHY_ID_BCM57780 &&
>>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610 &&
>>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM50610M &&
>>> +	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54210E &&
>>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54810 &&
>>>  	    BRCM_PHY_MODEL(phydev) != PHY_ID_BCM54811)
>>>  		return;
>>> @@ -227,9 +228,10 @@ static void bcm54xx_adjust_rxrefclk(struct phy_device *phydev)
>>>  		val |= BCM54XX_SHD_SCR3_DLLAPD_DIS;
>>>  
>>>  	if (phydev->dev_flags & PHY_BRCM_DIS_TXCRXC_NOENRGY) {
>>> -		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
>>> -		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54811)
>>> -			val |= BCM54810_SHD_SCR3_TRDDAPD;
>>> +		if (BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E ||
>>> +		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54810 ||
>>> +		    BRCM_PHY_MODEL(phydev) == PHY_ID_BCM54210E)
>>> +			val |= BCM54XX_SHD_SCR3_RXCTXC_DIS;
>>>  		else
>>>  			val |= BCM54XX_SHD_SCR3_TRDDAPD;
>>>  	}
>>> diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
>>> index 844dcfe789a2..16597d3fa011 100644
>>> --- a/include/linux/brcmphy.h
>>> +++ b/include/linux/brcmphy.h
>>> @@ -193,6 +193,7 @@
>>>  #define  BCM54XX_SHD_SCR3_DEF_CLK125	0x0001
>>>  #define  BCM54XX_SHD_SCR3_DLLAPD_DIS	0x0002
>>>  #define  BCM54XX_SHD_SCR3_TRDDAPD	0x0004
>>> +#define  BCM54XX_SHD_SCR3_RXCTXC_DIS	0x0100
>>
>> Curiously enough, my BCM5464R datasheet does say:
>>
>> The TXC and RXC outputs can be disabled during auto-power down by setting the “1000BASE-T/100BASE-TX/10BASE-T
>> Spare Control 3 Register (Address 1Ch, Shadow Value 00101),” bit 8 =1.
>>
>> but when I go to the definition of the register, bit 8 is hidden. Odd.
>>
>> How can I ensure that the auto power down feature is doing something?
> 
> I am trying to confirm what the expected power levels should be from the
> 54210E product engineer so I can give you an estimate of what you should
> see with and without while measure the PHY's regulators.

Took a while but for the 54210E reference board here are the numbers,
your mileage will vary depending on the supplies, regulator efficiency
and PCB design around the PHY obviously:

BMCR.PDOWN:			86.12 mW
auto-power down:		77.84 mW
auto-power-down, DLL disabled:  30.83 mW
IDDQ-low power:			 9.85 mW (requires a RESETn toggle)
IDDQ with soft recovery:	10.75 mW

Interestingly, the 50212E that I am using requires writing the PDOWN bit
and only that bit (not a RMW) in order to get in a correct state, both
LEDs keep flashing when that happens, fixes coming.

When net-next opens back up I will submit patches to support IDDQ with
soft recovery since that is clearly much better than the standard power
down and it does not require a RESETn toggle.
-- 
Florian

Re: [PATCH net-next v2 3/3] net: phy: broadcom: Allow BCM54210E to configure APD

[Vladimir Oltean]

On Tue, Mar 02, 2021 at 07:37:34PM -0800, Florian Fainelli wrote:
> Took a while but for the 54210E reference board here are the numbers,
> your mileage will vary depending on the supplies, regulator efficiency
> and PCB design around the PHY obviously:
> 
> BMCR.PDOWN:			86.12 mW
> auto-power down:		77.84 mW

Quite curious that the APD power is lower than the normal BMCR.PDOWN
value. As far as my understanding goes, when in APD mode, the PHY even
wakes up from time to time to send pulses to the link partner?

> auto-power-down, DLL disabled:  30.83 mW

The jump from simple APD to APD with DLL disabled is pretty big.
Correct me if I'm wrong, but there's an intermediary step which was not
measured, where the CLK125 is disabled but the internal DLL (Delay
Locked Loop?) is still enabled. I think powering off the internal DLL
also implies powering off the CLK125 pin, at least that's how the PHY
driver treats things at the moment. But we don't know if the huge
reduction in power is due just to CLK125 or the DLL (it's more likely
it's due to both, in equal amounts).

Anyway, it's great to have some results which tell us exactly what is
worthwhile and what isn't. In other news, I've added the BCM5464 to the
list of PHYs with APD and I didn't see any issues thus far.

> IDDQ-low power:			 9.85 mW (requires a RESETn toggle)
> IDDQ with soft recovery:	10.75 mW
> 
> Interestingly, the 50212E that I am using requires writing the PDOWN bit
> and only that bit (not a RMW) in order to get in a correct state, both
> LEDs keep flashing when that happens, fixes coming.
> 
> When net-next opens back up I will submit patches to support IDDQ with
> soft recovery since that is clearly much better than the standard power
> down and it does not require a RESETn toggle.

Iddq must be the quiescent supply current, isn't it (but in that case,
I'm a bit confused to not see a value in mA)? Is it an actual operating
mode (I don't see anything about that mentioned in the BCM5464 sheet)
and if it is, what is there exactly to support?

Re: [PATCH net-next v2 3/3] net: phy: broadcom: Allow BCM54210E to configure APD

[Florian Fainelli]

On 3/4/2021 5:08 PM, Vladimir Oltean wrote:
> On Tue, Mar 02, 2021 at 07:37:34PM -0800, Florian Fainelli wrote:
>> Took a while but for the 54210E reference board here are the numbers,
>> your mileage will vary depending on the supplies, regulator efficiency
>> and PCB design around the PHY obviously:
>>
>> BMCR.PDOWN:			86.12 mW
>> auto-power down:		77.84 mW
> 
> Quite curious that the APD power is lower than the normal BMCR.PDOWN
> value. As far as my understanding goes, when in APD mode, the PHY even
> wakes up from time to time to send pulses to the link partner?

Auto-power down kicks in when the cable is disconnected. There is
another IDDQ mode that supports energy detection though I am unsure of
when it would be useful for most Linux enabled systems.

> 
>> auto-power-down, DLL disabled:  30.83 mW
> 
> The jump from simple APD to APD with DLL disabled is pretty big.
> Correct me if I'm wrong, but there's an intermediary step which was not
> measured, where the CLK125 is disabled but the internal DLL (Delay
> Locked Loop?) is still enabled. I think powering off the internal DLL
> also implies powering off the CLK125 pin, at least that's how the PHY
> driver treats things at the moment. But we don't know if the huge
> reduction in power is due just to CLK125 or the DLL (it's more likely
> it's due to both, in equal amounts).

Agree, I do not have the break down though.

> 
> Anyway, it's great to have some results which tell us exactly what is
> worthwhile and what isn't. In other news, I've added the BCM5464 to the
> list of PHYs with APD and I didn't see any issues thus far.
> 
>> IDDQ-low power:			 9.85 mW (requires a RESETn toggle)
>> IDDQ with soft recovery:	10.75 mW
>>
>> Interestingly, the 50212E that I am using requires writing the PDOWN bit
>> and only that bit (not a RMW) in order to get in a correct state, both
>> LEDs keep flashing when that happens, fixes coming.
>>
>> When net-next opens back up I will submit patches to support IDDQ with
>> soft recovery since that is clearly much better than the standard power
>> down and it does not require a RESETn toggle.
> 
> Iddq must be the quiescent supply current, isn't it (but in that case,
> I'm a bit confused to not see a value in mA)? Is it an actual operating
> mode (I don't see anything about that mentioned in the BCM5464 sheet)
> and if it is, what is there exactly to support?

You would put the PHY in IDDQ with soft recovery (or ultra low power)
when you are administratively bringing down the network interface (and
its PHY), or when suspending to a low power state where Wake-on-LAN is
not enabled.
-- 
Florian

Re: [PATCH net-next v2 0/3] net: phy: broadcom: Cleanups and APD

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Fri, 12 Feb 2021 19:46:29 -0800 you wrote:
> This patch series cleans up the brcmphy.h header and its numerous unused
> phydev->dev_flags, fixes the RXC/TXC clock disabling bit and allows the
> BCM54210E PHY to utilize APD.
> 
> Changes in v2:
> 
> - dropped the patch that attempted to fix a possible discrepancy between
>   the datasheet and the actual hardware
> - added a patch to remove a forward declaration
> - do additional flags cleanup
> 
> [...]

Here is the summary with links:
  - [net-next,v2,1/3] net: phy: broadcom: Avoid forward for bcm54xx_config_clock_delay()
    https://git.kernel.org/netdev/net-next/c/133bf7b4fbbe
  - [net-next,v2,2/3] net: phy: broadcom: Remove unused flags
    https://git.kernel.org/netdev/net-next/c/17d3a83afbbf
  - [net-next,v2,3/3] net: phy: broadcom: Allow BCM54210E to configure APD
    https://git.kernel.org/netdev/net-next/c/5d4358ede8eb

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html