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
Avoid a forward declaration by moving the callers of
bcm54xx_config_clock_delay() below its body.
Signed-off-by: Florian Fainelli <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
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 <[email protected]>
> ---
So you want to remove the IBND dev_flags separately, okay.
Reviewed-by: Vladimir Oltean <[email protected]>
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 <[email protected]>
> ---
Reviewed-by: Vladimir Oltean <[email protected]>
> 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
>
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 <[email protected]>
>> ---
>
> Reviewed-by: Vladimir Oltean <[email protected]>
>
>> 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
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
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 <[email protected]>
>>> ---
>>
>> Reviewed-by: Vladimir Oltean <[email protected]>
>>
>>> 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
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?
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