[PATCH net-next 0/3] net: phy: Support enabling clocks prior to bus probe

[PATCH net-next 0/3] net: phy: Support enabling clocks prior to bus probe

[Florian Fainelli]

Hi all,

This patch series takes care of enabling the Ethernet PHY clocks in
DT-based systems (we have no way to do it for ACPI, and ACPI would
likely keep all of this hardware enabled anyway).

Please test on your respective platforms, mine still seems to have
a race condition that I am tracking down as it looks like we are not
waiting long enough post clock enable.

The check on the clock reference count is necessary to avoid an
artificial bump of the clock reference count and to support the unbind
-> bind of the PHY driver. We could solve it in different ways.

Comments and test results welcome!

Changes since RFC:

- resolved the timing hazard on ARCH_BRCMSTB platforms, the resource
  enabling for these platforms needs to happen *right before* the
  dummy BMSR read which is needed to work around a flaw in the internal
  Gigabit PHYs MDIO logic, doing this after would not work. This means
  that we need to enable resources during bus->reset for those specific
  platforms.

- export of_mdiobus_device_enable_resources() for other drivers to use
  (like mdio-bcm-unimac), would they need it

- added boolean to keep track of resources being already enabled

- disable resources just before calling drv->probe() so as to let PHY
  drivers manage resources with a normal reference count

Florian Fainelli (3):
  net: phy: Support enabling clocks prior to bus probe
  net: phy: mdio-bcm-unimac: Enable GPHY resources during bus reset
  net: phy: bcm7xxx: request and manage GPHY clock

 drivers/net/mdio/mdio-bcm-unimac.c | 10 ++++
 drivers/net/phy/bcm7xxx.c          | 18 +++++-
 drivers/net/phy/phy_device.c       | 15 ++++-
 drivers/of/of_mdio.c               | 95 ++++++++++++++++++++++++++++++
 include/linux/of_mdio.h            | 16 +++++
 include/linux/phy.h                | 13 ++++
 6 files changed, 165 insertions(+), 2 deletions(-)

-- 
2.25.1

[PATCH net-next 1/3] net: phy: Support enabling clocks prior to bus probe

[Florian Fainelli]

Some Ethernet PHYs may require that their clock, which typically drives
their logic to respond to reads on the MDIO bus be enabled before
issusing a MDIO bus scan.

We have a chicken and egg problem though which is that we cannot enable
a given Ethernet PHY's device clock until we have a phy_device instance
create and called the driver's probe function. This will not happen
unless we are successful in probing the PHY device, which requires its
clock(s) to be turned on.

For DT based systems we can solve this by using of_clk_get() which
operates on a device_node reference, and make sure that all clocks
associaed with the node are enabled prior to doing any reads towards the
device. In order to avoid drivers having to know the a priori reference
count of the resources, we drop them back to 0 right before calling
->probe() which is then supposed to manage the resources normally.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/phy_device.c | 15 +++++-
 drivers/of/of_mdio.c         | 95 ++++++++++++++++++++++++++++++++++++
 include/linux/of_mdio.h      | 16 ++++++
 include/linux/phy.h          | 13 +++++
 4 files changed, 138 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 57d44648c8dd..bf2824ba056e 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -23,6 +23,7 @@
 #include <linux/mm.h>
 #include <linux/module.h>
 #include <linux/netdevice.h>
+#include <linux/of_mdio.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
 #include <linux/property.h>
@@ -2845,6 +2846,15 @@ static int phy_probe(struct device *dev)
 
 	mutex_lock(&phydev->lock);
 
+	/* To allow PHY drivers to manage device resources such as
+	 * clocks, regulators or others, disable the resources that
+	 * were enabled during the bus->reset or the PHY registration
+	 * routine such that they work with a natural resource reference
+	 * count.
+	 */
+	of_mdiobus_device_disable_resources(phydev->mdio.bus,
+					    phydev->mdio.addr);
+
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
 
@@ -2914,8 +2924,11 @@ static int phy_probe(struct device *dev)
 
 out:
 	/* Assert the reset signal */
-	if (err)
+	if (err) {
 		phy_device_reset(phydev, 1);
+		of_mdiobus_device_disable_resources(phydev->mdio.bus,
+						    phydev->mdio.addr);
+	}
 
 	mutex_unlock(&phydev->lock);
 
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index cb32d7ef4938..bbce4a70312c 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -19,6 +19,7 @@
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/module.h>
+#include <linux/clk.h>
 
 #define DEFAULT_GPIO_RESET_DELAY	10	/* in microseconds */
 
@@ -60,6 +61,92 @@ static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
 	return register_mii_timestamper(arg.np, arg.args[0]);
 }
 
+int of_mdiobus_device_enable_resources(struct mii_bus *bus,
+				       struct device_node *child,
+				       u32 addr)
+{
+	struct mdio_device_resource *res = &bus->mdio_resources[addr];
+	unsigned int i;
+	int rc;
+
+	if (res->enabled_resources) {
+		dev_dbg(&bus->dev,
+			"MDIO resources for %d already enabled\n", addr);
+		return 0;
+	}
+
+	rc = of_count_phandle_with_args(child, "clocks", "#clock-cells");
+	if (rc < 0) {
+		if (rc == -ENOENT)
+			rc = 0;
+		else
+			return rc;
+	}
+
+	res->num_clks = rc;
+	if (rc == 0)
+		return rc;
+
+	dev_dbg(&bus->dev, "Found %d clocks for child at %d\n", rc, addr);
+
+	res->clks = devm_kcalloc(&bus->dev, res->num_clks,
+				 sizeof(struct clk *), GFP_KERNEL);
+	if (!res->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < res->num_clks; i++) {
+		res->clks[i] = of_clk_get(child, i);
+		if (IS_ERR(res->clks[i])) {
+			if (PTR_ERR(res->clks[i]) == -ENOENT)
+				continue;
+
+			return PTR_ERR(res->clks[i]);
+		}
+
+		rc = clk_prepare_enable(res->clks[i]);
+		if (rc) {
+			dev_err(&bus->dev,
+				"Failed to enabled clock for %d (rc: %d)\n",
+				addr, rc);
+			goto out_clk_disable;
+		}
+
+		dev_dbg(&bus->dev,
+			"Enable clock %d for child at %d\n",
+			i, addr);
+	}
+
+	res->enabled_resources = true;
+
+	return 0;
+
+out_clk_disable:
+	while (i-- > 0) {
+		clk_disable_unprepare(res->clks[i]);
+		clk_put(res->clks[i]);
+	}
+	res->enabled_resources = false;
+	return rc;
+}
+EXPORT_SYMBOL(of_mdiobus_device_enable_resources);
+
+void of_mdiobus_device_disable_resources(struct mii_bus *bus, u32 addr)
+{
+	struct mdio_device_resource *res = &bus->mdio_resources[addr];
+	unsigned int i;
+
+	if (!res->enabled_resources || res->num_clks == 0)
+		return;
+
+	for (i = 0; i < res->num_clks; i++) {
+		clk_disable_unprepare(res->clks[i]);
+		clk_put(res->clks[i]);
+		dev_dbg(&bus->dev, "Disabled clk %d for %d\n", i, addr);
+	}
+	res->enabled_resources = false;
+}
+EXPORT_SYMBOL(of_mdiobus_device_disable_resources);
+
 int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
 			      struct device_node *child, u32 addr)
 {
@@ -117,6 +204,12 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
 	if (IS_ERR(mii_ts))
 		return PTR_ERR(mii_ts);
 
+	rc = of_mdiobus_device_enable_resources(mdio, child, addr);
+	if (rc) {
+		dev_err(&mdio->dev, "enable resources: %d\n", rc);
+		return rc;
+	}
+
 	is_c45 = of_device_is_compatible(child,
 					 "ethernet-phy-ieee802.3-c45");
 
@@ -125,6 +218,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
 	else
 		phy = get_phy_device(mdio, addr, is_c45);
 	if (IS_ERR(phy)) {
+		of_mdiobus_device_disable_resources(mdio, addr);
 		if (mii_ts)
 			unregister_mii_timestamper(mii_ts);
 		return PTR_ERR(phy);
@@ -132,6 +226,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
 
 	rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);
 	if (rc) {
+		of_mdiobus_device_disable_resources(mdio, addr);
 		if (mii_ts)
 			unregister_mii_timestamper(mii_ts);
 		phy_device_free(phy);
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 1efb88d9f892..f43f4bcb3f22 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -58,6 +58,10 @@ static inline int of_mdio_parse_addr(struct device *dev,
 	return addr;
 }
 
+int of_mdiobus_device_enable_resources(struct mii_bus *mdio,
+				       struct device_node *child, u32 addr);
+void of_mdiobus_device_disable_resources(struct mii_bus *mdio, u32 addr);
+
 #else /* CONFIG_OF_MDIO */
 static inline bool of_mdiobus_child_is_phy(struct device_node *child)
 {
@@ -129,6 +133,18 @@ static inline int of_mdiobus_phy_device_register(struct mii_bus *mdio,
 {
 	return -ENOSYS;
 }
+
+static inline int of_mdiobus_device_enable_resources(struct mii_bus *mdio,
+						     struct device_node *child,
+						     u32 addr)
+{
+	return 0;
+}
+
+static inline void of_mdiobus_device_disable_resources(struct mii_bus *mdio,
+						       u32 addr)
+{
+}
 #endif
 
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 3a09d2bf69ea..a01953daea45 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -247,6 +247,14 @@ struct phy_package_shared {
 #define PHY_SHARED_F_INIT_DONE  0
 #define PHY_SHARED_F_PROBE_DONE 1
 
+struct clk;
+
+struct mdio_device_resource {
+	bool enabled_resources;
+	unsigned int num_clks;
+	struct clk **clks;
+};
+
 /*
  * The Bus class for PHYs.  Devices which provide access to
  * PHYs should register using this structure
@@ -291,6 +299,11 @@ struct mii_bus {
 	 */
 	int irq[PHY_MAX_ADDR];
 
+	/* An array of MDIO device resources that must be enabled
+	 * during probe for identification to succeed.
+	 */
+	struct mdio_device_resource mdio_resources[PHY_MAX_ADDR];
+
 	/* GPIO reset pulse width in microseconds */
 	int reset_delay_us;
 	/* GPIO reset deassert delay in microseconds */
-- 
2.25.1

[PATCH net-next 2/3] net: phy: mdio-bcm-unimac: Enable GPHY resources during bus reset

[Florian Fainelli]

The UniMAC MDIO bus controller allows the interfacing with various
internal Broadcom STB Gigabit PHYs which do require two things:

- they require that a digital clock be enabled for their MDIO interface
  to work at all

- they require that at least one MDIO transaction goes through their
  interface to respond correctly to subsequent MDIO reads

Because of these constraints, we need to have the bus driver's reset
callback to call of_mdiobus_device_enable_resources() in order for
clocks to be enabled prior to doing the dummy BMSR read.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/mdio/mdio-bcm-unimac.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/mdio/mdio-bcm-unimac.c b/drivers/net/mdio/mdio-bcm-unimac.c
index fbd36891ee64..c8fed16c1f27 100644
--- a/drivers/net/mdio/mdio-bcm-unimac.c
+++ b/drivers/net/mdio/mdio-bcm-unimac.c
@@ -10,6 +10,7 @@
 #include <linux/platform_device.h>
 #include <linux/sched.h>
 #include <linux/module.h>
+#include <linux/of_mdio.h>
 #include <linux/io.h>
 #include <linux/delay.h>
 #include <linux/clk.h>
@@ -162,6 +163,7 @@ static int unimac_mdio_reset(struct mii_bus *bus)
 	struct device_node *child;
 	u32 read_mask = 0;
 	int addr;
+	int rc;
 
 	if (!np) {
 		read_mask = ~bus->phy_mask;
@@ -172,6 +174,14 @@ static int unimac_mdio_reset(struct mii_bus *bus)
 				continue;
 
 			read_mask |= 1 << addr;
+
+			/* Enable resources such as clocks *right now* for the
+			 * workaround on the next line to be effective.
+			 */
+			rc = of_mdiobus_device_enable_resources(bus, child,
+								addr);
+			if (rc)
+				return rc;
 		}
 	}
 
-- 
2.25.1

[PATCH net-next 3/3] 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.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
index 692048d86ab1..f0ffcdcaef03 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)
@@ -534,7 +536,19 @@ 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);
+
+	return clk_prepare_enable(priv->clk);
+}
+
+static void bcm7xxx_28nm_remove(struct phy_device *phydev)
+{
+	struct bcm7xxx_phy_priv *priv = phydev->priv;
+
+	clk_disable_unprepare(priv->clk);
+	devm_clk_put(&phydev->mdio.dev, priv->clk);
 }
 
 #define BCM7XXX_28NM_GPHY(_oui, _name)					\
@@ -552,6 +566,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 +582,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 1/3] net: phy: Support enabling clocks prior to bus probe

[Andrew Lunn]

On Wed, Sep 02, 2020 at 09:39:45PM -0700, Florian Fainelli wrote:
> Some Ethernet PHYs may require that their clock, which typically drives
> their logic to respond to reads on the MDIO bus be enabled before
> issusing a MDIO bus scan.

issuing

> 
> We have a chicken and egg problem though which is that we cannot enable
> a given Ethernet PHY's device clock until we have a phy_device instance
> create and called the driver's probe function. This will not happen
> unless we are successful in probing the PHY device, which requires its
> clock(s) to be turned on.
> 
> For DT based systems we can solve this by using of_clk_get() which
> operates on a device_node reference, and make sure that all clocks
> associaed with the node are enabled prior to doing any reads towards the

associated.

> device. In order to avoid drivers having to know the a priori reference
> count of the resources, we drop them back to 0 right before calling
> ->probe() which is then supposed to manage the resources normally.

  Andrew

Re: [PATCH net-next 1/3] net: phy: Support enabling clocks prior to bus probe

[Andrew Lunn]

On Wed, Sep 02, 2020 at 09:39:45PM -0700, Florian Fainelli wrote:
> Some Ethernet PHYs may require that their clock, which typically drives
> their logic to respond to reads on the MDIO bus be enabled before
> issusing a MDIO bus scan.
> 
> We have a chicken and egg problem though which is that we cannot enable
> a given Ethernet PHY's device clock until we have a phy_device instance
> create and called the driver's probe function. This will not happen
> unless we are successful in probing the PHY device, which requires its
> clock(s) to be turned on.
> 
> For DT based systems we can solve this by using of_clk_get() which
> operates on a device_node reference, and make sure that all clocks
> associaed with the node are enabled prior to doing any reads towards the
> device. In order to avoid drivers having to know the a priori reference
> count of the resources, we drop them back to 0 right before calling
> ->probe() which is then supposed to manage the resources normally.

Hi Florian

This seems asymmetric. The resources are enabled in
of_mdiobus_phy_device_register() but disabled in phy_probe().

What we are really interested in is having the clock ticking while
trying to read the ID registers. Could the enable/disable be placed
around the call to get_phy_id()?

       Andrew

Re: [PATCH net-next 1/3] net: phy: Support enabling clocks prior to bus probe

[Rob Herring]

On Wed, Sep 2, 2020 at 10:39 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> Some Ethernet PHYs may require that their clock, which typically drives
> their logic to respond to reads on the MDIO bus be enabled before
> issusing a MDIO bus scan.
>
> We have a chicken and egg problem though which is that we cannot enable
> a given Ethernet PHY's device clock until we have a phy_device instance
> create and called the driver's probe function. This will not happen
> unless we are successful in probing the PHY device, which requires its
> clock(s) to be turned on.
>
> For DT based systems we can solve this by using of_clk_get() which
> operates on a device_node reference, and make sure that all clocks
> associaed with the node are enabled prior to doing any reads towards the
> device. In order to avoid drivers having to know the a priori reference
> count of the resources, we drop them back to 0 right before calling
> ->probe() which is then supposed to manage the resources normally.

What if a device requires clocks enabled in a certain order or timing?
It's not just clocks, you could have some GPIOs or a regulator that
need enabling first. It's device specific, so really needs a per
device solution. This is not just an issue with MDIO. I think we
really need some sort of pre-probe hook in the driver model in order
to do any non-discoverable init for discoverable buses. Or perhaps
forcing probe when there are devices defined in DT if they're not
discovered by normal means.

However, you're not trying to do this in DT (see mmc-pwrseq :( ) and
this can evolve within the kernel to a better solution.

>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/phy/phy_device.c | 15 +++++-
>  drivers/of/of_mdio.c         | 95 ++++++++++++++++++++++++++++++++++++

BTW, I'd really like to move this to drivers/net/ as we've done with
all the other bus/subsystem DT code.

>  include/linux/of_mdio.h      | 16 ++++++
>  include/linux/phy.h          | 13 +++++
>  4 files changed, 138 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 57d44648c8dd..bf2824ba056e 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -23,6 +23,7 @@
>  #include <linux/mm.h>
>  #include <linux/module.h>
>  #include <linux/netdevice.h>
> +#include <linux/of_mdio.h>
>  #include <linux/phy.h>
>  #include <linux/phy_led_triggers.h>
>  #include <linux/property.h>
> @@ -2845,6 +2846,15 @@ static int phy_probe(struct device *dev)
>
>         mutex_lock(&phydev->lock);
>
> +       /* To allow PHY drivers to manage device resources such as
> +        * clocks, regulators or others, disable the resources that
> +        * were enabled during the bus->reset or the PHY registration
> +        * routine such that they work with a natural resource reference
> +        * count.
> +        */
> +       of_mdiobus_device_disable_resources(phydev->mdio.bus,
> +                                           phydev->mdio.addr);
> +
>         /* Deassert the reset signal */
>         phy_device_reset(phydev, 0);
>
> @@ -2914,8 +2924,11 @@ static int phy_probe(struct device *dev)
>
>  out:
>         /* Assert the reset signal */
> -       if (err)
> +       if (err) {
>                 phy_device_reset(phydev, 1);
> +               of_mdiobus_device_disable_resources(phydev->mdio.bus,
> +                                                   phydev->mdio.addr);
> +       }
>
>         mutex_unlock(&phydev->lock);
>
> diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
> index cb32d7ef4938..bbce4a70312c 100644
> --- a/drivers/of/of_mdio.c
> +++ b/drivers/of/of_mdio.c
> @@ -19,6 +19,7 @@
>  #include <linux/of_mdio.h>
>  #include <linux/of_net.h>
>  #include <linux/module.h>
> +#include <linux/clk.h>
>
>  #define DEFAULT_GPIO_RESET_DELAY       10      /* in microseconds */
>
> @@ -60,6 +61,92 @@ static struct mii_timestamper *of_find_mii_timestamper(struct device_node *node)
>         return register_mii_timestamper(arg.np, arg.args[0]);
>  }
>
> +int of_mdiobus_device_enable_resources(struct mii_bus *bus,
> +                                      struct device_node *child,
> +                                      u32 addr)
> +{
> +       struct mdio_device_resource *res = &bus->mdio_resources[addr];
> +       unsigned int i;
> +       int rc;
> +
> +       if (res->enabled_resources) {
> +               dev_dbg(&bus->dev,
> +                       "MDIO resources for %d already enabled\n", addr);
> +               return 0;
> +       }
> +
> +       rc = of_count_phandle_with_args(child, "clocks", "#clock-cells");
> +       if (rc < 0) {
> +               if (rc == -ENOENT)
> +                       rc = 0;
> +               else
> +                       return rc;
> +       }
> +
> +       res->num_clks = rc;
> +       if (rc == 0)
> +               return rc;
> +
> +       dev_dbg(&bus->dev, "Found %d clocks for child at %d\n", rc, addr);
> +
> +       res->clks = devm_kcalloc(&bus->dev, res->num_clks,
> +                                sizeof(struct clk *), GFP_KERNEL);
> +       if (!res->clks)
> +               return -ENOMEM;
> +
> +       for (i = 0; i < res->num_clks; i++) {
> +               res->clks[i] = of_clk_get(child, i);
> +               if (IS_ERR(res->clks[i])) {
> +                       if (PTR_ERR(res->clks[i]) == -ENOENT)
> +                               continue;
> +
> +                       return PTR_ERR(res->clks[i]);

If you return here, you are potentially leaving clocks enabled and
with a reference.

> +               }
> +
> +               rc = clk_prepare_enable(res->clks[i]);

Can't you use clk_bulk_prepare_enable?

Looks like there's no bulk version of of_clk_get though...

> +               if (rc) {
> +                       dev_err(&bus->dev,
> +                               "Failed to enabled clock for %d (rc: %d)\n",
> +                               addr, rc);
> +                       goto out_clk_disable;
> +               }
> +
> +               dev_dbg(&bus->dev,
> +                       "Enable clock %d for child at %d\n",
> +                       i, addr);
> +       }
> +
> +       res->enabled_resources = true;
> +
> +       return 0;
> +
> +out_clk_disable:
> +       while (i-- > 0) {
> +               clk_disable_unprepare(res->clks[i]);
> +               clk_put(res->clks[i]);
> +       }
> +       res->enabled_resources = false;
> +       return rc;
> +}
> +EXPORT_SYMBOL(of_mdiobus_device_enable_resources);
> +
> +void of_mdiobus_device_disable_resources(struct mii_bus *bus, u32 addr)
> +{
> +       struct mdio_device_resource *res = &bus->mdio_resources[addr];
> +       unsigned int i;
> +
> +       if (!res->enabled_resources || res->num_clks == 0)
> +               return;
> +
> +       for (i = 0; i < res->num_clks; i++) {
> +               clk_disable_unprepare(res->clks[i]);
> +               clk_put(res->clks[i]);
> +               dev_dbg(&bus->dev, "Disabled clk %d for %d\n", i, addr);
> +       }
> +       res->enabled_resources = false;
> +}
> +EXPORT_SYMBOL(of_mdiobus_device_disable_resources);
> +
>  int of_mdiobus_phy_device_register(struct mii_bus *mdio, struct phy_device *phy,
>                               struct device_node *child, u32 addr)
>  {
> @@ -117,6 +204,12 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
>         if (IS_ERR(mii_ts))
>                 return PTR_ERR(mii_ts);
>
> +       rc = of_mdiobus_device_enable_resources(mdio, child, addr);
> +       if (rc) {
> +               dev_err(&mdio->dev, "enable resources: %d\n", rc);
> +               return rc;
> +       }
> +
>         is_c45 = of_device_is_compatible(child,
>                                          "ethernet-phy-ieee802.3-c45");
>
> @@ -125,6 +218,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
>         else
>                 phy = get_phy_device(mdio, addr, is_c45);
>         if (IS_ERR(phy)) {
> +               of_mdiobus_device_disable_resources(mdio, addr);
>                 if (mii_ts)
>                         unregister_mii_timestamper(mii_ts);
>                 return PTR_ERR(phy);
> @@ -132,6 +226,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
>
>         rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);
>         if (rc) {
> +               of_mdiobus_device_disable_resources(mdio, addr);
>                 if (mii_ts)
>                         unregister_mii_timestamper(mii_ts);
>                 phy_device_free(phy);
> diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
> index 1efb88d9f892..f43f4bcb3f22 100644
> --- a/include/linux/of_mdio.h
> +++ b/include/linux/of_mdio.h
> @@ -58,6 +58,10 @@ static inline int of_mdio_parse_addr(struct device *dev,
>         return addr;
>  }
>
> +int of_mdiobus_device_enable_resources(struct mii_bus *mdio,
> +                                      struct device_node *child, u32 addr);
> +void of_mdiobus_device_disable_resources(struct mii_bus *mdio, u32 addr);
> +
>  #else /* CONFIG_OF_MDIO */
>  static inline bool of_mdiobus_child_is_phy(struct device_node *child)
>  {
> @@ -129,6 +133,18 @@ static inline int of_mdiobus_phy_device_register(struct mii_bus *mdio,
>  {
>         return -ENOSYS;
>  }
> +
> +static inline int of_mdiobus_device_enable_resources(struct mii_bus *mdio,
> +                                                    struct device_node *child,
> +                                                    u32 addr)
> +{
> +       return 0;
> +}
> +
> +static inline void of_mdiobus_device_disable_resources(struct mii_bus *mdio,
> +                                                      u32 addr)
> +{
> +}
>  #endif
>
>
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 3a09d2bf69ea..a01953daea45 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -247,6 +247,14 @@ struct phy_package_shared {
>  #define PHY_SHARED_F_INIT_DONE  0
>  #define PHY_SHARED_F_PROBE_DONE 1
>
> +struct clk;
> +
> +struct mdio_device_resource {
> +       bool enabled_resources;
> +       unsigned int num_clks;
> +       struct clk **clks;
> +};
> +
>  /*
>   * The Bus class for PHYs.  Devices which provide access to
>   * PHYs should register using this structure
> @@ -291,6 +299,11 @@ struct mii_bus {
>          */
>         int irq[PHY_MAX_ADDR];
>
> +       /* An array of MDIO device resources that must be enabled
> +        * during probe for identification to succeed.
> +        */
> +       struct mdio_device_resource mdio_resources[PHY_MAX_ADDR];
> +
>         /* GPIO reset pulse width in microseconds */
>         int reset_delay_us;
>         /* GPIO reset deassert delay in microseconds */
> --
> 2.25.1
>

Re: [PATCH net-next 1/3] net: phy: Support enabling clocks prior to bus probe

[Andrew Lunn]

On Thu, Sep 03, 2020 at 03:28:22PM -0600, Rob Herring wrote:
> What if a device requires clocks enabled in a certain order or timing?
> It's not just clocks, you could have some GPIOs or a regulator that
> need enabling first. It's device specific, so really needs a per
> device solution. This is not just an issue with MDIO. I think we
> really need some sort of pre-probe hook in the driver model in order
> to do any non-discoverable init for discoverable buses.

Hi Rob

How do you solve the chicken/egg of knowing what device specific init
is needed before you can discover what device you have on the bus?

> Or perhaps forcing probe when there are devices defined in DT if
> they're not discovered by normal means.

The PHY subsystem has this. You came specify in DT the ID of the
device which we would normally read during bus discovery. The correct
driver is then loaded and probed. But it is good practice to avoid
this. OEMs are known to change the PHY in order to perform cost
optimisation. So we prefer to do discover and do the right thing if
the PHY has changed.

As for GPIOS and regulators, i expect this code will expand pretty
soon after being merged to handle those. There are users wanting
it. We already have some standard properties defined, in terms of
gpios, delay while off, delay after turning it on. As for ordering, i
guess it would make sense to enable the clocks and then hit it with a
reset? If there is a device which cannot be handled like this, it can
always hard code its ID in device tree, and fully control its
resources in the driver.

	  Andrew

Re: [PATCH net-next 1/3] net: phy: Support enabling clocks prior to bus probe

[Florian Fainelli]

On 9/3/2020 2:28 PM, Rob Herring wrote:
> On Wed, Sep 2, 2020 at 10:39 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> Some Ethernet PHYs may require that their clock, which typically drives
>> their logic to respond to reads on the MDIO bus be enabled before
>> issusing a MDIO bus scan.
>>
>> We have a chicken and egg problem though which is that we cannot enable
>> a given Ethernet PHY's device clock until we have a phy_device instance
>> create and called the driver's probe function. This will not happen
>> unless we are successful in probing the PHY device, which requires its
>> clock(s) to be turned on.
>>
>> For DT based systems we can solve this by using of_clk_get() which
>> operates on a device_node reference, and make sure that all clocks
>> associaed with the node are enabled prior to doing any reads towards the
>> device. In order to avoid drivers having to know the a priori reference
>> count of the resources, we drop them back to 0 right before calling
>> ->probe() which is then supposed to manage the resources normally.
> 
> What if a device requires clocks enabled in a certain order or timing?
> It's not just clocks, you could have some GPIOs or a regulator that
> need enabling first. It's device specific, so really needs a per
> device solution. This is not just an issue with MDIO. I think we
> really need some sort of pre-probe hook in the driver model in order
> to do any non-discoverable init for discoverable buses. Or perhaps
> forcing probe when there are devices defined in DT if they're not
> discovered by normal means.

I like the pre-probe hook idea, and there are other devices that I have 
access to that would benefit from that, like PCIe end-points that 
require regulators to be turned on in order for them to be discoverable.

For MDIO we might actually be able to create the backing device 
reference without having read the device ID yet, provided that we know 
its address on the bus, which DT can tell us.

Bartosz attempted to do that not so long ago and we sort of stalled 
there, too:

https://lkml.org/lkml/2020/6/22/253

Let me see if I can just add a pre-probe hook, make use of it in the 
MDIO layer, and we see how we can apply it to other subsystems?
-- 
Florian

Re: [PATCH net-next 1/3] net: phy: Support enabling clocks prior to bus probe

[Florian Fainelli]

On 9/3/2020 2:42 PM, Andrew Lunn wrote:
> On Thu, Sep 03, 2020 at 03:28:22PM -0600, Rob Herring wrote:
>> What if a device requires clocks enabled in a certain order or timing?
>> It's not just clocks, you could have some GPIOs or a regulator that
>> need enabling first. It's device specific, so really needs a per
>> device solution. This is not just an issue with MDIO. I think we
>> really need some sort of pre-probe hook in the driver model in order
>> to do any non-discoverable init for discoverable buses.
> 
> Hi Rob
> 
> How do you solve the chicken/egg of knowing what device specific init
> is needed before you can discover what device you have on the bus?

For MDIO since we have a fixed number of devices on the bus, we could 
pre-populate the MDIO map for all addresses, and free up the devices 
that we did not probe.

When using DT we can first parse the address, create a mdio_device 
there, and then turn on clocks/regulators/GPIOs whatever since we now 
have a device reference. Only then do we bind the device with its driver.

If we are using the DT scanning loop because the node did not provide a 
"reg" property, then the PHY must be in a functional state to be probed, 
we cannot guess what we do not know.

All of this uses MDIO implementation knowledge though.

> 
>> Or perhaps forcing probe when there are devices defined in DT if
>> they're not discovered by normal means.
> 
> The PHY subsystem has this. You came specify in DT the ID of the
> device which we would normally read during bus discovery. The correct
> driver is then loaded and probed. But it is good practice to avoid
> this. OEMs are known to change the PHY in order to perform cost
> optimisation. So we prefer to do discover and do the right thing if
> the PHY has changed.
> 
> As for GPIOS and regulators, i expect this code will expand pretty
> soon after being merged to handle those. There are users wanting
> it. We already have some standard properties defined, in terms of
> gpios, delay while off, delay after turning it on. As for ordering, i
> guess it would make sense to enable the clocks and then hit it with a
> reset? If there is a device which cannot be handled like this, it can
> always hard code its ID in device tree, and fully control its
> resources in the driver.
> 
> 	  Andrew
> 

-- 
Florian

Re: [PATCH net-next 0/3] net: phy: Support enabling clocks prior to bus probe

[Florian Fainelli]

On 9/2/2020 9:39 PM, Florian Fainelli wrote:
> Hi all,
> 
> This patch series takes care of enabling the Ethernet PHY clocks in
> DT-based systems (we have no way to do it for ACPI, and ACPI would
> likely keep all of this hardware enabled anyway).
> 
> Please test on your respective platforms, mine still seems to have
> a race condition that I am tracking down as it looks like we are not
> waiting long enough post clock enable.
> 
> The check on the clock reference count is necessary to avoid an
> artificial bump of the clock reference count and to support the unbind
> -> bind of the PHY driver. We could solve it in different ways.
> 
> Comments and test results welcome!

Andrew, while we figure out a proper way to support this with the Linux 
device driver model, would you be opposed in a single patch to 
drivers/net/mdio/mdio-bcm-unimac.c which takes care of enabling the 
PHY's clock during bus->reset just for the sake of getting those systems 
to work, and later on we move over to the pre-probe mechanism?

That would allow me to continue working with upstream kernels on these 
systems without carrying a big pile of patches.
-- 
Florian

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

[Marco Felsch]

Hi Florian,

On 20-09-02 21:39, 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.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
> index 692048d86ab1..f0ffcdcaef03 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)
> @@ -534,7 +536,19 @@ 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);

Since the clock is binded to the mdio-dev here..

> +	if (IS_ERR(priv->clk))
> +		return PTR_ERR(priv->clk);
> +
> +	return clk_prepare_enable(priv->clk);

clould we use devm_add_action_or_reset() here so we don't have to
register the .remove() hook?

> +}
> +
> +static void bcm7xxx_28nm_remove(struct phy_device *phydev)
> +{
> +	struct bcm7xxx_phy_priv *priv = phydev->priv;
> +
> +	clk_disable_unprepare(priv->clk);
> +	devm_clk_put(&phydev->mdio.dev, priv->clk);

Is this really necessary? The devm_clk_get_optional() function already
registers the devm_clk_release() hook.

Regards,
  Marco

>  }
>  
>  #define BCM7XXX_28NM_GPHY(_oui, _name)					\
> @@ -552,6 +566,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 +582,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
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Marco Felsch]

On 20-09-02 21:39, 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.

Hi Florian,

Seems like you added the same support here like I did for the smsc
driver. So should I go with my proposed patch which can be adapted later
after you guys figured out who to enable the required resources?

Regards,
  Marco

Re: [PATCH net-next 0/3] net: phy: Support enabling clocks prior to bus probe

[Adam Rudziński]

W dniu 2020-09-04 o 06:04, Florian Fainelli pisze:
>
>
> On 9/2/2020 9:39 PM, Florian Fainelli wrote:
>> Hi all,
>>
>> This patch series takes care of enabling the Ethernet PHY clocks in
>> DT-based systems (we have no way to do it for ACPI, and ACPI would
>> likely keep all of this hardware enabled anyway).
>>
>> Please test on your respective platforms, mine still seems to have
>> a race condition that I am tracking down as it looks like we are not
>> waiting long enough post clock enable.
>>
>> The check on the clock reference count is necessary to avoid an
>> artificial bump of the clock reference count and to support the unbind
>> -> bind of the PHY driver. We could solve it in different ways.
>>
>> Comments and test results welcome!
>
> Andrew, while we figure out a proper way to support this with the 
> Linux device driver model, would you be opposed in a single patch to 
> drivers/net/mdio/mdio-bcm-unimac.c which takes care of enabling the 
> PHY's clock during bus->reset just for the sake of getting those 
> systems to work, and later on we move over to the pre-probe mechanism?
>
> That would allow me to continue working with upstream kernels on these 
> systems without carrying a big pile of patches.

Just a bunch of questions.

Actually, why is it necessary to have a full MDIO bus scan already 
during probing peripherals?

If during probing the peripherals enable their resources (like clocks), 
what's wrong in having the full MDIO bus scan after probing of all 
peripherals is complete (and all peripherals are up)?

Also, what's wrong in letting the MDIO bus scan find only some PHYs in 
the first go, and then letting each driver instance (of particular 
peripheral) initiate scan only for its specific PHY, if it was not found 
yet?
(Is it thatof_mdio.h provides public function of_mdiobus_register, but 
not something similar to add only specific devices/phys without 
destroying the existing state?)
I'd say that it is not necessary to have a PHY getting found before it 
is needed to setup the complete interface.

Best regards,
Adam

Re: [PATCH net-next 0/3] net: phy: Support enabling clocks prior to bus probe

[Andrew Lunn]

> Just a bunch of questions.
> 
> Actually, why is it necessary to have a full MDIO bus scan already during
> probing peripherals?

That is the Linux bus model. It does not matter what sort of bus it
is, PCI, USB, MDIO, etc. When the bus driver is loaded, the bus is
enumerated and drivers probe for each device found on the bus.

> I'd say that it is not necessary to have a PHY getting found before it is
> needed to setup the complete interface.

It is like saying, we don't need to probe the keyboard until the first
time the "Press Enter" prompt is given?

     Andrew

Re: [PATCH net-next 0/3] net: phy: Support enabling clocks prior to bus probe

[Andrew Lunn]

On Thu, Sep 03, 2020 at 09:04:11PM -0700, Florian Fainelli wrote:
> 
> 
> On 9/2/2020 9:39 PM, Florian Fainelli wrote:
> > Hi all,
> > 
> > This patch series takes care of enabling the Ethernet PHY clocks in
> > DT-based systems (we have no way to do it for ACPI, and ACPI would
> > likely keep all of this hardware enabled anyway).
> > 
> > Please test on your respective platforms, mine still seems to have
> > a race condition that I am tracking down as it looks like we are not
> > waiting long enough post clock enable.
> > 
> > The check on the clock reference count is necessary to avoid an
> > artificial bump of the clock reference count and to support the unbind
> > -> bind of the PHY driver. We could solve it in different ways.
> > 
> > Comments and test results welcome!
> 
> Andrew, while we figure out a proper way to support this with the Linux
> device driver model, would you be opposed in a single patch to
> drivers/net/mdio/mdio-bcm-unimac.c which takes care of enabling the PHY's
> clock during bus->reset just for the sake of getting those systems to work,
> and later on we move over to the pre-probe mechanism?
> 
> That would allow me to continue working with upstream kernels on these
> systems without carrying a big pile of patches.

We do have quite a need for the proper solution. I wouldn't want you
dropping the proper solution because you have a hack in place.

Please add a comment: "HORRIBLE TEMPORARY HACK", to give you
motivation to remove it again :-)

   Andrew

Re: [PATCH net-next 0/3] net: phy: Support enabling clocks prior to bus probe

[Adam Rudziński]

W dniu 2020-09-04 o 15:45, Andrew Lunn pisze:
>> Just a bunch of questions.
>>
>> Actually, why is it necessary to have a full MDIO bus scan already during
>> probing peripherals?
> That is the Linux bus model. It does not matter what sort of bus it
> is, PCI, USB, MDIO, etc. When the bus driver is loaded, the bus is
> enumerated and drivers probe for each device found on the bus.

OK. But is it always expected to find all the devices on the bus in the 
first run? Does the bus model ever allow to just add any more devices? 
Kind of, "hotplug". :)

>> I'd say that it is not necessary to have a PHY getting found before it is
>> needed to setup the complete interface.
> It is like saying, we don't need to probe the keyboard until the first
> time the "Press Enter" prompt is given?

I'm not sure what you mean. It's like saying that we don't need to care 
if we even have the keyboard until we are interested in any interaction 
with it. (This might be reading a key, an autotest, ..., or not using, 
but avoiding a conflict - depends on application.)

Best regards,
Adam

Re: [PATCH net-next 0/3] net: phy: Support enabling clocks prior to bus probe

[Andrew Lunn]

On Fri, Sep 04, 2020 at 04:00:55PM +0200, Adam Rudziński wrote:
> W dniu 2020-09-04 o 15:45, Andrew Lunn pisze:
> > > Just a bunch of questions.
> > > 
> > > Actually, why is it necessary to have a full MDIO bus scan already during
> > > probing peripherals?
> > That is the Linux bus model. It does not matter what sort of bus it
> > is, PCI, USB, MDIO, etc. When the bus driver is loaded, the bus is
> > enumerated and drivers probe for each device found on the bus.
> 
> OK. But is it always expected to find all the devices on the bus in the
> first run?

Yes. Cold plug expects to find all the device while scanning the bus.

> Does the bus model ever allow to just add any more devices? Kind of,
> "hotplug". :)

Hotplug is triggered by hardware saying a new device has been
added/removed after cold plug.

This is not a hotplug case. The hardware has not suddenly appeared, it
has always been there.

   Andrew

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

[Florian Fainelli]

On 9/3/2020 11:15 PM, Marco Felsch wrote:
> Hi Florian,
> 
> On 20-09-02 21:39, 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.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>>   drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++-
>>   1 file changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
>> index 692048d86ab1..f0ffcdcaef03 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)
>> @@ -534,7 +536,19 @@ 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);
> 
> Since the clock is binded to the mdio-dev here..
> 
>> +	if (IS_ERR(priv->clk))
>> +		return PTR_ERR(priv->clk);
>> +
>> +	return clk_prepare_enable(priv->clk);
> 
> clould we use devm_add_action_or_reset() here so we don't have to
> register the .remove() hook?

Maybe, more on that below.

> 
>> +}
>> +
>> +static void bcm7xxx_28nm_remove(struct phy_device *phydev)
>> +{
>> +	struct bcm7xxx_phy_priv *priv = phydev->priv;
>> +
>> +	clk_disable_unprepare(priv->clk);
>> +	devm_clk_put(&phydev->mdio.dev, priv->clk);
> 
> Is this really necessary? The devm_clk_get_optional() function already
> registers the devm_clk_release() hook.

Yes, because you can unbind the PHY driver from sysfs, and if you want 
to bind that driver again, which will call .probe() again, you must undo 
strictly everything that .probe() did. The embedded mdio_device does not 
go away, so there will be no automatic freeing of resources. Using 
devm_* may be confusing, so using just the plain clk_get() and clk_put() 
may be clearer here.
-- 
Florian

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

[Florian Fainelli]

On 9/3/2020 11:18 PM, Marco Felsch wrote:
> On 20-09-02 21:39, 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.
> 
> Hi Florian,
> 
> Seems like you added the same support here like I did for the smsc
> driver. So should I go with my proposed patch which can be adapted later
> after you guys figured out who to enable the required resources?

That seems fine to me, on your platform there appears to be an 
assumption that we will be able to probe the SMSC PHY because everything 
we need is already enabled, right? If so, this patch series does not 
change that state.
-- 
Florian

Re: [PATCH net-next 0/3] net: phy: Support enabling clocks prior to bus probe

[Adam Rudziński]

W dniu 2020-09-04 o 16:23, Andrew Lunn pisze:
> On Fri, Sep 04, 2020 at 04:00:55PM +0200, Adam Rudziński wrote:
>> W dniu 2020-09-04 o 15:45, Andrew Lunn pisze:
>>>> Just a bunch of questions.
>>>>
>>>> Actually, why is it necessary to have a full MDIO bus scan already during
>>>> probing peripherals?
>>> That is the Linux bus model. It does not matter what sort of bus it
>>> is, PCI, USB, MDIO, etc. When the bus driver is loaded, the bus is
>>> enumerated and drivers probe for each device found on the bus.
>> OK. But is it always expected to find all the devices on the bus in the
>> first run?
> Yes. Cold plug expects to find all the device while scanning the bus.
>
>> Does the bus model ever allow to just add any more devices? Kind of,
>> "hotplug". :)
> Hotplug is triggered by hardware saying a new device has been
> added/removed after cold plug.
>
> This is not a hotplug case. The hardware has not suddenly appeared, it
> has always been there.
>
>     Andrew

There still is something that I would like to have clarified. Let me ask 
a bit more.

Even if the hardware is fixed, in general it doesn't mean that there is 
only one possible way of bringing it up in software (software in 
general, not necessarily Linux). Does the Linux driver/bus model define 
the allowed options more precisely?

Or:

There is "coldplug", there is "hotplug", and let's call the intermediate 
possibility "warmplug" - the hardware is fixed, but it is discovered not 
in the same scan. Some devices are found in scans triggered by something 
later. For example, by a driver which doesn't have its attached devices 
discovered yet. Let's assume for now that it makes sense, and this can 
result in a perfectly running system.
Does the Linux driver/bus model explicitly and strictly require 
"coldplug" approach in the case of fixed hardware? Is "warmplug" 
explicitly and strictly not allowed, or it just "doesn't feel right"?

Best regards,
Adam

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

[Marco Felsch]

On 20-09-04 08:37, Florian Fainelli wrote:
> 
> 
> On 9/3/2020 11:15 PM, Marco Felsch wrote:
> > Hi Florian,
> > 
> > On 20-09-02 21:39, 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.
> > > 
> > > Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> > > ---
> > >   drivers/net/phy/bcm7xxx.c | 18 +++++++++++++++++-
> > >   1 file changed, 17 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
> > > index 692048d86ab1..f0ffcdcaef03 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)
> > > @@ -534,7 +536,19 @@ 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);
> > 
> > Since the clock is binded to the mdio-dev here..
> > 
> > > +	if (IS_ERR(priv->clk))
> > > +		return PTR_ERR(priv->clk);
> > > +
> > > +	return clk_prepare_enable(priv->clk);
> > 
> > clould we use devm_add_action_or_reset() here so we don't have to
> > register the .remove() hook?
> 
> Maybe, more on that below.
> 
> > 
> > > +}
> > > +
> > > +static void bcm7xxx_28nm_remove(struct phy_device *phydev)
> > > +{
> > > +	struct bcm7xxx_phy_priv *priv = phydev->priv;
> > > +
> > > +	clk_disable_unprepare(priv->clk);
> > > +	devm_clk_put(&phydev->mdio.dev, priv->clk);
> > 
> > Is this really necessary? The devm_clk_get_optional() function already
> > registers the devm_clk_release() hook.
> 
> Yes, because you can unbind the PHY driver from sysfs, and if you want to
> bind that driver again, which will call .probe() again, you must undo
> strictly everything that .probe() did. The embedded mdio_device does not go
> away, so there will be no automatic freeing of resources.

Okay I got this. Sry. I'm not that deep into the net-stack and the
device live time. Thanks for the clarification.

> Using devm_* may
> be confusing, so using just the plain clk_get() and clk_put() may be clearer
> here.

That would be better for others including me because I detected a
failure on my patchset.

Regards,
  Marco

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

[Marco Felsch]

On 20-09-04 08:38, Florian Fainelli wrote:
> 
> 
> On 9/3/2020 11:18 PM, Marco Felsch wrote:
> > On 20-09-02 21:39, 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.
> > 
> > Hi Florian,
> > 
> > Seems like you added the same support here like I did for the smsc
> > driver. So should I go with my proposed patch which can be adapted later
> > after you guys figured out who to enable the required resources?
> 
> That seems fine to me, on your platform there appears to be an assumption
> that we will be able to probe the SMSC PHY because everything we need is
> already enabled, right? If so, this patch series does not change that state.

Unfortunately yes.. The imx-fec driver enables all DT-specified clock
and then the mdio-bus probe is initiated. The good point is that my
patchset do not require a clock-id so the generic way can replace my
work later.

Regards,
  Marco

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

[Marco Felsch]

On 20-09-04 08:37, Florian Fainelli wrote:

...

> > Is this really necessary? The devm_clk_get_optional() function already
> > registers the devm_clk_release() hook.
> 
> Yes, because you can unbind the PHY driver from sysfs, and if you want to
> bind that driver again, which will call .probe() again, you must undo
> strictly everything that .probe() did. The embedded mdio_device does not go
> away, so there will be no automatic freeing of resources. Using devm_* may
> be confusing, so using just the plain clk_get() and clk_put() may be clearer
> here.

Hi Florian,

sorry for asking again... I'm getting a bit confused during applying
your comments to my smsc-phy patchset.
A few drivers are using the devm_kzalloc() (including your bcm7xxx.c and
my smsc.c). Does this mean that those drivers have a memory leak since
the mdio_device does not disappear and so the memory allocated during
probe() isn't freed?

Regards,
  Marco