[PATCH net-next v2] net: phy: bcm7xxx: request and manage GPHY clock

[PATCH net-next v2] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli]

The internal Gigabit PHY on Broadcom STB chips has a digital clock which
drives its MDIO interface among other things, the driver now requests
and manage that clock during .probe() and .remove() accordingly.

Because the PHY driver can be probed with the clocks turned off we need
to apply the dummy BMSR workaround during the driver probe function to
ensure subsequent MDIO read or write towards the PHY will succeed.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Changes in v2:

- localize the changes exclusively within the PHY driver and do not
  involve the MDIO driver at all. Using the ethernet-phyidAAAA.BBBB
  compatible string we can get straight to the desired driver without
  requiring clocks to be assumed on.

 drivers/net/phy/bcm7xxx.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index 692048d86ab1..744c24491d63 100644
--- a/drivers/net/phy/bcm7xxx.c
+++ b/drivers/net/phy/bcm7xxx.c
@@ -11,6 +11,7 @@
 #include "bcm-phy-lib.h"
 #include <linux/bitops.h>
 #include <linux/brcmphy.h>
+#include <linux/clk.h>
 #include <linux/mdio.h>
 
 /* Broadcom BCM7xxx internal PHY registers */
@@ -39,6 +40,7 @@
 
 struct bcm7xxx_phy_priv {
 	u64	*stats;
+	struct clk *clk;
 };
 
 static int bcm7xxx_28nm_d0_afe_config_init(struct phy_device *phydev)
@@ -521,6 +523,7 @@ static void bcm7xxx_28nm_get_phy_stats(struct phy_device *phydev,
 static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 {
 	struct bcm7xxx_phy_priv *priv;
+	int ret = 0;
 
 	priv = devm_kzalloc(&phydev->mdio.dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -534,7 +537,30 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	if (!priv->stats)
 		return -ENOMEM;
 
-	return 0;
+	priv->clk = devm_clk_get_optional(&phydev->mdio.dev, NULL);
+	if (IS_ERR(priv->clk))
+		return PTR_ERR(priv->clk);
+
+	ret = clk_prepare_enable(priv->clk);
+	if (ret)
+		return ret;
+
+	/* Dummy read to a register to workaround an 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. This is needed to ensure that any subsequent reads to the
+	 * PHY will succeed.
+	 */
+	phy_read(phydev, MII_BMSR);
+
+	return ret;
+}
+
+static void bcm7xxx_28nm_remove(struct phy_device *phydev)
+{
+	struct bcm7xxx_phy_priv *priv = phydev->priv;
+
+	clk_disable_unprepare(priv->clk);
 }
 
 #define BCM7XXX_28NM_GPHY(_oui, _name)					\
@@ -552,6 +578,7 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	.get_strings	= bcm_phy_get_strings,				\
 	.get_stats	= bcm7xxx_28nm_get_phy_stats,			\
 	.probe		= bcm7xxx_28nm_probe,				\
+	.remove		= bcm7xxx_28nm_remove,				\
 }
 
 #define BCM7XXX_28NM_EPHY(_oui, _name)					\
@@ -567,6 +594,7 @@ static int bcm7xxx_28nm_probe(struct phy_device *phydev)
 	.get_strings	= bcm_phy_get_strings,				\
 	.get_stats	= bcm7xxx_28nm_get_phy_stats,			\
 	.probe		= bcm7xxx_28nm_probe,				\
+	.remove		= bcm7xxx_28nm_remove,				\
 }
 
 #define BCM7XXX_40NM_EPHY(_oui, _name)					\
-- 
2.25.1

Re: [PATCH net-next v2] net: phy: bcm7xxx: request and manage GPHY clock

[Andrew Lunn]

On Wed, Sep 16, 2020 at 07:04:13PM -0700, Florian Fainelli wrote:
> The internal Gigabit PHY on Broadcom STB chips has a digital clock which
> drives its MDIO interface among other things, the driver now requests
> and manage that clock during .probe() and .remove() accordingly.
> 
> Because the PHY driver can be probed with the clocks turned off we need
> to apply the dummy BMSR workaround during the driver probe function to
> ensure subsequent MDIO read or write towards the PHY will succeed.

Hi Florian

Is it worth mentioning this in the DT binding? It is all pretty much
standard lego pieces, but it has taken you a while to assemble them in
the correct way. So giving hits to others who might want to uses these
STB chips could be nice.

	Andrew

Re: [PATCH net-next v2] net: phy: bcm7xxx: request and manage GPHY clock

[Florian Fainelli]

On 9/17/2020 6:11 AM, Andrew Lunn wrote:
> On Wed, Sep 16, 2020 at 07:04:13PM -0700, Florian Fainelli wrote:
>> The internal Gigabit PHY on Broadcom STB chips has a digital clock which
>> drives its MDIO interface among other things, the driver now requests
>> and manage that clock during .probe() and .remove() accordingly.
>>
>> Because the PHY driver can be probed with the clocks turned off we need
>> to apply the dummy BMSR workaround during the driver probe function to
>> ensure subsequent MDIO read or write towards the PHY will succeed.
> 
> Hi Florian
> 
> Is it worth mentioning this in the DT binding? It is all pretty much
> standard lego pieces, but it has taken you a while to assemble them in
> the correct way. So giving hits to others who might want to uses these
> STB chips could be nice.

In some respect this does not really belong in the DT binding because we 
are describing how Linux will be matching a given compatible string with 
its driver, but it is certainly worth mentioning somewhere, like in the 
PHY document.

Are you fine with the changes though?
-- 
Florian

Re: [PATCH net-next v2] net: phy: bcm7xxx: request and manage GPHY clock

[Andrew Lunn]

On Wed, Sep 16, 2020 at 07:04:13PM -0700, Florian Fainelli wrote:
> The internal Gigabit PHY on Broadcom STB chips has a digital clock which
> drives its MDIO interface among other things, the driver now requests
> and manage that clock during .probe() and .remove() accordingly.
> 
> Because the PHY driver can be probed with the clocks turned off we need
> to apply the dummy BMSR workaround during the driver probe function to
> ensure subsequent MDIO read or write towards the PHY will succeed.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next v2] net: phy: bcm7xxx: request and manage GPHY clock

[David Miller]

From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 16 Sep 2020 19:04:13 -0700

> The internal Gigabit PHY on Broadcom STB chips has a digital clock which
> drives its MDIO interface among other things, the driver now requests
> and manage that clock during .probe() and .remove() accordingly.
> 
> Because the PHY driver can be probed with the clocks turned off we need
> to apply the dummy BMSR workaround during the driver probe function to
> ensure subsequent MDIO read or write towards the PHY will succeed.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
> Changes in v2:
> 
> - localize the changes exclusively within the PHY driver and do not
>   involve the MDIO driver at all. Using the ethernet-phyidAAAA.BBBB
>   compatible string we can get straight to the desired driver without
>   requiring clocks to be assumed on.

Applied and queued up for -stable, thanks.