[PATCH 00/15] net: phy: correctly model the PHY voltage supply in DT

[PATCH 00/15] net: phy: correctly model the PHY voltage supply in DT

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

PHY devices on an MDIO bus can have their own regulators. This is currently
mostly modeled using the semi-standard phy-supply property on the MAC node.
This seems to be conceptually wrong though, as the MAC shouldn't need to
care about enabling the PHY regulator explicitly. Instead this should be
done by the core PHY/MDIO code.

This series introduces a new DT property at the PHY node level in mdio.yaml
and adds support for PHY regulator, then uses the new property on pumpkin
boards. It also addresses several issues I noticed when working on this
feature.

First four patches are just cosmetic improvements in source files we'll
modify later in this series.

Patches 5 and 6 modify the way PHY reset handling works. Currently the
probe() callback needs to be implemented to take the PHY out of reset but
PHY drivers without probe() can have resets defined as well.

Patches 7-11 address an issue where we probe the PHY for ID without
deasserting its reset signal. We delay the ID read until after the logical
device is created and resets have been configured.

Last four patches add the new DT property, implement support for PHY
regulator in phy and mdio code and set the new property in the DT file
for MediaTek's pumpkin boards.

Bartosz Golaszewski (15):
  net: phy: arrange headers in mdio_bus.c alphabetically
  net: phy: arrange headers in mdio_device.c alphabetically
  net: phy: arrange headers in phy_device.c alphabetically
  net: mdio: add a forward declaration for reset_control to mdio.h
  net: phy: reset the PHY even if probe() is not implemented
  net: phy: mdio: reset MDIO devices even if probe() is not implemented
  net: phy: split out the PHY driver request out of phy_device_create()
  net: phy: check the PHY presence in get_phy_id()
  net: phy: delay PHY driver probe until PHY registration
  net: phy: simplify phy_device_create()
  net: phy: drop get_phy_device()
  dt-bindings: mdio: add phy-supply property to ethernet phy node
  net: phy: mdio: add support for PHY supply regulator
  net: phy: add PHY regulator support
  ARM64: dts: mediatek: add a phy regulator to pumpkin-common.dtsi

 .../devicetree/bindings/net/mdio.yaml         |   4 +
 .../boot/dts/mediatek/pumpkin-common.dtsi     |   1 +
 drivers/net/dsa/ocelot/felix_vsc9959.c        |   3 +-
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c   |   5 +-
 .../net/ethernet/hisilicon/hns/hns_dsaf_mac.c |   2 +-
 drivers/net/ethernet/socionext/netsec.c       |   3 +-
 drivers/net/phy/fixed_phy.c                   |   2 +-
 drivers/net/phy/mdio-xgene.c                  |   2 +-
 drivers/net/phy/mdio_bus.c                    |  55 +++--
 drivers/net/phy/mdio_device.c                 |  51 ++++-
 drivers/net/phy/nxp-tja11xx.c                 |   2 +-
 drivers/net/phy/phy_device.c                  | 216 ++++++++++--------
 drivers/net/phy/sfp.c                         |   2 +-
 drivers/of/of_mdio.c                          |  11 +-
 include/linux/mdio.h                          |   4 +
 include/linux/phy.h                           |  21 +-
 16 files changed, 240 insertions(+), 144 deletions(-)

-- 
2.26.1

[PATCH 01/15] net: phy: arrange headers in mdio_bus.c alphabetically

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Keeping the headers in alphabetical order is better for readability and
allows to easily see if given header is already included.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/mdio_bus.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 6ceee82b2839..296cf9771483 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -8,32 +8,32 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/kernel.h>
-#include <linux/string.h>
-#include <linux/errno.h>
-#include <linux/unistd.h>
-#include <linux/slab.h>
-#include <linux/interrupt.h>
-#include <linux/init.h>
 #include <linux/delay.h>
 #include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/netdevice.h>
 #include <linux/of_device.h>
-#include <linux/of_mdio.h>
 #include <linux/of_gpio.h>
-#include <linux/netdevice.h>
-#include <linux/etherdevice.h>
+#include <linux/of_mdio.h>
+#include <linux/phy.h>
 #include <linux/reset.h>
 #include <linux/skbuff.h>
+#include <linux/slab.h>
 #include <linux/spinlock.h>
-#include <linux/mm.h>
-#include <linux/module.h>
-#include <linux/mii.h>
-#include <linux/ethtool.h>
-#include <linux/phy.h>
-#include <linux/io.h>
+#include <linux/string.h>
 #include <linux/uaccess.h>
+#include <linux/unistd.h>
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/mdio.h>
-- 
2.26.1

Re: [PATCH 01/15] net: phy: arrange headers in mdio_bus.c alphabetically

[Andrew Lunn]

On Mon, Jun 22, 2020 at 11:37:30AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

    Andrew

Re: [PATCH 01/15] net: phy: arrange headers in mdio_bus.c alphabetically

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 01/15] net: phy: arrange headers in mdio_bus.c alphabetically

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 01/15] net: phy: arrange headers in mdio_bus.c alphabetically

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 02/15] net: phy: arrange headers in mdio_device.c alphabetically

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Keeping the headers in alphabetical order is better for readability and
allows to easily see if given header is already included.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/mdio_device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index c1d345c3cab3..f60443e48622 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -6,6 +6,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/delay.h>
 #include <linux/errno.h>
 #include <linux/gpio.h>
 #include <linux/gpio/consumer.h>
@@ -20,7 +21,6 @@
 #include <linux/slab.h>
 #include <linux/string.h>
 #include <linux/unistd.h>
-#include <linux/delay.h>
 
 void mdio_device_free(struct mdio_device *mdiodev)
 {
-- 
2.26.1

Re: [PATCH 02/15] net: phy: arrange headers in mdio_device.c alphabetically

[Andrew Lunn]

On Mon, Jun 22, 2020 at 11:37:31AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

    Andrew

Re: [PATCH 02/15] net: phy: arrange headers in mdio_device.c alphabetically

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 02/15] net: phy: arrange headers in mdio_device.c alphabetically

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/15] net: phy: arrange headers in mdio_device.c alphabetically

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 03/15] net: phy: arrange headers in phy_device.c alphabetically

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Keeping the headers in alphabetical order is better for readability and
allows to easily see if given header is already included.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/phy_device.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 04946de74fa0..1b4df12c70ad 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,28 +9,28 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
-#include <linux/kernel.h>
-#include <linux/string.h>
-#include <linux/errno.h>
-#include <linux/unistd.h>
-#include <linux/slab.h>
-#include <linux/interrupt.h>
-#include <linux/init.h>
+#include <linux/bitmap.h>
 #include <linux/delay.h>
-#include <linux/netdevice.h>
+#include <linux/errno.h>
 #include <linux/etherdevice.h>
-#include <linux/skbuff.h>
+#include <linux/ethtool.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/mdio.h>
+#include <linux/mii.h>
 #include <linux/mm.h>
 #include <linux/module.h>
-#include <linux/mii.h>
-#include <linux/ethtool.h>
-#include <linux/bitmap.h>
+#include <linux/netdevice.h>
 #include <linux/phy.h>
 #include <linux/phy_led_triggers.h>
 #include <linux/sfp.h>
-#include <linux/mdio.h>
-#include <linux/io.h>
+#include <linux/skbuff.h>
+#include <linux/slab.h>
+#include <linux/string.h>
 #include <linux/uaccess.h>
+#include <linux/unistd.h>
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
-- 
2.26.1

Re: [PATCH 03/15] net: phy: arrange headers in phy_device.c alphabetically

[Andrew Lunn]

On Mon, Jun 22, 2020 at 11:37:32AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

    Andrew

Re: [PATCH 03/15] net: phy: arrange headers in phy_device.c alphabetically

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH 03/15] net: phy: arrange headers in phy_device.c alphabetically

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 03/15] net: phy: arrange headers in phy_device.c alphabetically

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Keeping the headers in alphabetical order is better for readability and
> allows to easily see if given header is already included.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 04/15] net: mdio: add a forward declaration for reset_control to mdio.h

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

This header refers to struct reset_control but doesn't include any reset
header. The structure definition is probably somehow indirectly pulled in
since no warnings are reported but for the sake of correctness add the
forward declaration for struct reset_control.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 include/linux/mdio.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 36d2e0673d03..9ac5e7ff6156 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -17,6 +17,7 @@
 #define MII_REGADDR_C45_MASK	GENMASK(15, 0)
 
 struct gpio_desc;
+struct reset_control;
 struct mii_bus;
 
 /* Multiple levels of nesting are possible. However typically this is
-- 
2.26.1

Re: [PATCH 04/15] net: mdio: add a forward declaration for reset_control to mdio.h

[Andrew Lunn]

On Mon, Jun 22, 2020 at 11:37:33AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> This header refers to struct reset_control but doesn't include any reset
> header. The structure definition is probably somehow indirectly pulled in
> since no warnings are reported but for the sake of correctness add the
> forward declaration for struct reset_control.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

    Andrew

Re: [PATCH 04/15] net: mdio: add a forward declaration for reset_control to mdio.h

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> This header refers to struct reset_control but doesn't include any reset
> header. The structure definition is probably somehow indirectly pulled in
> since no warnings are reported but for the sake of correctness add the
> forward declaration for struct reset_control.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/mdio.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> index 36d2e0673d03..9ac5e7ff6156 100644
> --- a/include/linux/mdio.h
> +++ b/include/linux/mdio.h
> @@ -17,6 +17,7 @@
>  #define MII_REGADDR_C45_MASK	GENMASK(15, 0)
>  
>  struct gpio_desc;
> +struct reset_control;
>  struct mii_bus;

You wrote 3 patches to sort the headers alphabetically, do you mind
doing the same thing for forward declarations as well?
-- 
Florian

Re: [PATCH 04/15] net: mdio: add a forward declaration for reset_control to mdio.h

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> This header refers to struct reset_control but doesn't include any reset
> header. The structure definition is probably somehow indirectly pulled in
> since no warnings are reported but for the sake of correctness add the
> forward declaration for struct reset_control.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/mdio.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> index 36d2e0673d03..9ac5e7ff6156 100644
> --- a/include/linux/mdio.h
> +++ b/include/linux/mdio.h
> @@ -17,6 +17,7 @@
>  #define MII_REGADDR_C45_MASK	GENMASK(15, 0)
>  
>  struct gpio_desc;
> +struct reset_control;
>  struct mii_bus;

You wrote 3 patches to sort the headers alphabetically, do you mind
doing the same thing for forward declarations as well?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 04/15] net: mdio: add a forward declaration for reset_control to mdio.h

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> This header refers to struct reset_control but doesn't include any reset
> header. The structure definition is probably somehow indirectly pulled in
> since no warnings are reported but for the sake of correctness add the
> forward declaration for struct reset_control.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  include/linux/mdio.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/mdio.h b/include/linux/mdio.h
> index 36d2e0673d03..9ac5e7ff6156 100644
> --- a/include/linux/mdio.h
> +++ b/include/linux/mdio.h
> @@ -17,6 +17,7 @@
>  #define MII_REGADDR_C45_MASK	GENMASK(15, 0)
>  
>  struct gpio_desc;
> +struct reset_control;
>  struct mii_bus;

You wrote 3 patches to sort the headers alphabetically, do you mind
doing the same thing for forward declarations as well?
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 05/15] net: phy: reset the PHY even if probe() is not implemented

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Currently we only call phy_device_reset() if the PHY driver implements
the probe() callback. This is not mandatory and many drivers (e.g.
realtek) don't need probe() for most devices but still can have reset
GPIOs defined. There's no reason to depend on the presence of probe()
here so pull the reset code out of the if clause.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/phy_device.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1b4df12c70ad..f6985db08340 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -2690,16 +2690,13 @@ static int phy_probe(struct device *dev)
 
 	mutex_lock(&phydev->lock);
 
-	if (phydev->drv->probe) {
-		/* Deassert the reset signal */
-		phy_device_reset(phydev, 0);
+	/* Deassert the reset signal */
+	phy_device_reset(phydev, 0);
 
+	if (phydev->drv->probe) {
 		err = phydev->drv->probe(phydev);
-		if (err) {
-			/* Assert the reset signal */
-			phy_device_reset(phydev, 1);
+		if (err)
 			goto out;
-		}
 	}
 
 	/* Start out supporting everything. Eventually,
@@ -2761,6 +2758,10 @@ static int phy_probe(struct device *dev)
 	phydev->state = PHY_READY;
 
 out:
+	/* Assert the reset signal */
+	if (err)
+		phy_device_reset(phydev, 1);
+
 	mutex_unlock(&phydev->lock);
 
 	return err;
@@ -2779,12 +2780,12 @@ static int phy_remove(struct device *dev)
 	sfp_bus_del_upstream(phydev->sfp_bus);
 	phydev->sfp_bus = NULL;
 
-	if (phydev->drv && phydev->drv->remove) {
+	if (phydev->drv && phydev->drv->remove)
 		phydev->drv->remove(phydev);
 
-		/* Assert the reset signal */
-		phy_device_reset(phydev, 1);
-	}
+	/* Assert the reset signal */
+	phy_device_reset(phydev, 1);
+
 	phydev->drv = NULL;
 
 	return 0;
-- 
2.26.1

Re: [PATCH 05/15] net: phy: reset the PHY even if probe() is not implemented

[Andrew Lunn]

On Mon, Jun 22, 2020 at 11:37:34AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Currently we only call phy_device_reset() if the PHY driver implements
> the probe() callback. This is not mandatory and many drivers (e.g.
> realtek) don't need probe() for most devices but still can have reset
> GPIOs defined. There's no reason to depend on the presence of probe()
> here so pull the reset code out of the if clause.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

    Andrew

Re: [PATCH 05/15] net: phy: reset the PHY even if probe() is not implemented

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Currently we only call phy_device_reset() if the PHY driver implements
> the probe() callback. This is not mandatory and many drivers (e.g.
> realtek) don't need probe() for most devices but still can have reset
> GPIOs defined. There's no reason to depend on the presence of probe()
> here so pull the reset code out of the if clause.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

OK, but now let's imagine that a PHY device has two or more reset lines,
one of them is going to be managed by the core PHY library and the rest
is going to be under the responsibility of the PHY driver, that does not
sound intuitive or convenient at all. This is a hypothetical case, but
it could conceivable happen, so how about adding a flag to the driver
that says "let me manage it a all"?
-- 
Florian

Re: [PATCH 05/15] net: phy: reset the PHY even if probe() is not implemented

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Currently we only call phy_device_reset() if the PHY driver implements
> the probe() callback. This is not mandatory and many drivers (e.g.
> realtek) don't need probe() for most devices but still can have reset
> GPIOs defined. There's no reason to depend on the presence of probe()
> here so pull the reset code out of the if clause.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

OK, but now let's imagine that a PHY device has two or more reset lines,
one of them is going to be managed by the core PHY library and the rest
is going to be under the responsibility of the PHY driver, that does not
sound intuitive or convenient at all. This is a hypothetical case, but
it could conceivable happen, so how about adding a flag to the driver
that says "let me manage it a all"?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 05/15] net: phy: reset the PHY even if probe() is not implemented

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Currently we only call phy_device_reset() if the PHY driver implements
> the probe() callback. This is not mandatory and many drivers (e.g.
> realtek) don't need probe() for most devices but still can have reset
> GPIOs defined. There's no reason to depend on the presence of probe()
> here so pull the reset code out of the if clause.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

OK, but now let's imagine that a PHY device has two or more reset lines,
one of them is going to be managed by the core PHY library and the rest
is going to be under the responsibility of the PHY driver, that does not
sound intuitive or convenient at all. This is a hypothetical case, but
it could conceivable happen, so how about adding a flag to the driver
that says "let me manage it a all"?
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 05/15] net: phy: reset the PHY even if probe() is not implemented

[Bartosz Golaszewski]

wt., 23 cze 2020 o 21:14 Florian Fainelli <f.fainelli@gmail.com> napisał(a):
>
> On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Currently we only call phy_device_reset() if the PHY driver implements
> > the probe() callback. This is not mandatory and many drivers (e.g.
> > realtek) don't need probe() for most devices but still can have reset
> > GPIOs defined. There's no reason to depend on the presence of probe()
> > here so pull the reset code out of the if clause.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> OK, but now let's imagine that a PHY device has two or more reset lines,
> one of them is going to be managed by the core PHY library and the rest
> is going to be under the responsibility of the PHY driver, that does not
> sound intuitive or convenient at all. This is a hypothetical case, but
> it could conceivable happen, so how about adding a flag to the driver
> that says "let me manage it a all"?

This sounds good as a new feature idea but doesn't seem to be related
to what this patch is trying to do. The only thing it does is improve
the current behavior. I'll note your point for the future work on the
pre-probe stage.

Bartosz

Re: [PATCH 05/15] net: phy: reset the PHY even if probe() is not implemented

[Bartosz Golaszewski]

wt., 23 cze 2020 o 21:14 Florian Fainelli <f.fainelli@gmail.com> napisał(a):
>
> On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Currently we only call phy_device_reset() if the PHY driver implements
> > the probe() callback. This is not mandatory and many drivers (e.g.
> > realtek) don't need probe() for most devices but still can have reset
> > GPIOs defined. There's no reason to depend on the presence of probe()
> > here so pull the reset code out of the if clause.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> OK, but now let's imagine that a PHY device has two or more reset lines,
> one of them is going to be managed by the core PHY library and the rest
> is going to be under the responsibility of the PHY driver, that does not
> sound intuitive or convenient at all. This is a hypothetical case, but
> it could conceivable happen, so how about adding a flag to the driver
> that says "let me manage it a all"?

This sounds good as a new feature idea but doesn't seem to be related
to what this patch is trying to do. The only thing it does is improve
the current behavior. I'll note your point for the future work on the
pre-probe stage.

Bartosz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 05/15] net: phy: reset the PHY even if probe() is not implemented

[Bartosz Golaszewski]

wt., 23 cze 2020 o 21:14 Florian Fainelli <f.fainelli@gmail.com> napisał(a):
>
> On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Currently we only call phy_device_reset() if the PHY driver implements
> > the probe() callback. This is not mandatory and many drivers (e.g.
> > realtek) don't need probe() for most devices but still can have reset
> > GPIOs defined. There's no reason to depend on the presence of probe()
> > here so pull the reset code out of the if clause.
> >
> > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
>
> OK, but now let's imagine that a PHY device has two or more reset lines,
> one of them is going to be managed by the core PHY library and the rest
> is going to be under the responsibility of the PHY driver, that does not
> sound intuitive or convenient at all. This is a hypothetical case, but
> it could conceivable happen, so how about adding a flag to the driver
> that says "let me manage it a all"?

This sounds good as a new feature idea but doesn't seem to be related
to what this patch is trying to do. The only thing it does is improve
the current behavior. I'll note your point for the future work on the
pre-probe stage.

Bartosz

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 06/15] net: phy: mdio: reset MDIO devices even if probe() is not implemented

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Similarily to PHY drivers - there's no reason to require probe() to be
implemented in order to call mdio_device_reset(). MDIO devices can have
resets defined without needing to do anything in probe().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/mdio_device.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index f60443e48622..be615504b829 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -150,10 +150,10 @@ static int mdio_probe(struct device *dev)
 	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
 	int err = 0;
 
-	if (mdiodrv->probe) {
-		/* Deassert the reset signal */
-		mdio_device_reset(mdiodev, 0);
+	/* Deassert the reset signal */
+	mdio_device_reset(mdiodev, 0);
 
+	if (mdiodrv->probe) {
 		err = mdiodrv->probe(mdiodev);
 		if (err) {
 			/* Assert the reset signal */
@@ -170,12 +170,11 @@ static int mdio_remove(struct device *dev)
 	struct device_driver *drv = mdiodev->dev.driver;
 	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
 
-	if (mdiodrv->remove) {
+	if (mdiodrv->remove)
 		mdiodrv->remove(mdiodev);
 
-		/* Assert the reset signal */
-		mdio_device_reset(mdiodev, 1);
-	}
+	/* Assert the reset signal */
+	mdio_device_reset(mdiodev, 1);
 
 	return 0;
 }
-- 
2.26.1

Re: [PATCH 06/15] net: phy: mdio: reset MDIO devices even if probe() is not implemented

[Andrew Lunn]

On Mon, Jun 22, 2020 at 11:37:35AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Similarily to PHY drivers - there's no reason to require probe() to be
> implemented in order to call mdio_device_reset(). MDIO devices can have
> resets defined without needing to do anything in probe().
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

I would be surprised if there is any MDIO driver which don't have a
probe callback.

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

    Andrew

Re: [PATCH 06/15] net: phy: mdio: reset MDIO devices even if probe() is not implemented

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Similarily to PHY drivers - there's no reason to require probe() to be
> implemented in order to call mdio_device_reset(). MDIO devices can have
> resets defined without needing to do anything in probe().
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Same comment as patch #5, I would prefer that we seek a solution that
allows MDIO drivers to manage multiple reset lines (would that exist) on
their own instead of this one size fits all approach. Thank you.
-- 
Florian

Re: [PATCH 06/15] net: phy: mdio: reset MDIO devices even if probe() is not implemented

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Similarily to PHY drivers - there's no reason to require probe() to be
> implemented in order to call mdio_device_reset(). MDIO devices can have
> resets defined without needing to do anything in probe().
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Same comment as patch #5, I would prefer that we seek a solution that
allows MDIO drivers to manage multiple reset lines (would that exist) on
their own instead of this one size fits all approach. Thank you.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 06/15] net: phy: mdio: reset MDIO devices even if probe() is not implemented

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Similarily to PHY drivers - there's no reason to require probe() to be
> implemented in order to call mdio_device_reset(). MDIO devices can have
> resets defined without needing to do anything in probe().
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Same comment as patch #5, I would prefer that we seek a solution that
allows MDIO drivers to manage multiple reset lines (would that exist) on
their own instead of this one size fits all approach. Thank you.
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 07/15] net: phy: split out the PHY driver request out of phy_device_create()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Move the code requesting the PHY driver module out of phy_device_create()
into a separate helper. This will be later reused when we delay the
module loading.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/phy_device.c | 71 ++++++++++++++++++++----------------
 1 file changed, 39 insertions(+), 32 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index f6985db08340..8037a9663a85 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -558,7 +558,7 @@ static const struct device_type mdio_bus_phy_type = {
 	.pm = MDIO_BUS_PHY_PM_OPS,
 };
 
-static int phy_request_driver_module(struct phy_device *dev, u32 phy_id)
+static int phy_do_request_driver_module(struct phy_device *dev, u32 phy_id)
 {
 	int ret;
 
@@ -578,6 +578,40 @@ static int phy_request_driver_module(struct phy_device *dev, u32 phy_id)
 	return 0;
 }
 
+static int phy_request_driver_module(struct phy_device *phydev)
+{
+	int ret;
+
+	/* Request the appropriate module unconditionally; don't
+	 * bother trying to do so only if it isn't already loaded,
+	 * because that gets complicated. A hotplug event would have
+	 * done an unconditional modprobe anyway.
+	 * We don't do normal hotplug because it won't work for MDIO
+	 * -- because it relies on the device staying around for long
+	 * enough for the driver to get loaded. With MDIO, the NIC
+	 * driver will get bored and give up as soon as it finds that
+	 * there's no driver _already_ loaded.
+	 */
+	if (phydev->is_c45) {
+		const int num_ids = ARRAY_SIZE(phydev->c45_ids.device_ids);
+		int i;
+
+		for (i = 1; i < num_ids; i++) {
+			if (phydev->c45_ids.device_ids[i] == 0xffffffff)
+				continue;
+
+			ret = phy_do_request_driver_module(phydev,
+						phydev->c45_ids.device_ids[i]);
+			if (ret)
+				break;
+		}
+	} else {
+		ret = phy_do_request_driver_module(phydev, phydev->phy_id);
+	}
+
+	return ret;
+}
+
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     bool is_c45,
 				     struct phy_c45_device_ids *c45_ids)
@@ -622,38 +656,11 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 
 	mutex_init(&dev->lock);
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
+	device_initialize(&mdiodev->dev);
 
-	/* Request the appropriate module unconditionally; don't
-	 * bother trying to do so only if it isn't already loaded,
-	 * because that gets complicated. A hotplug event would have
-	 * done an unconditional modprobe anyway.
-	 * We don't do normal hotplug because it won't work for MDIO
-	 * -- because it relies on the device staying around for long
-	 * enough for the driver to get loaded. With MDIO, the NIC
-	 * driver will get bored and give up as soon as it finds that
-	 * there's no driver _already_ loaded.
-	 */
-	if (is_c45 && c45_ids) {
-		const int num_ids = ARRAY_SIZE(c45_ids->device_ids);
-		int i;
-
-		for (i = 1; i < num_ids; i++) {
-			if (c45_ids->device_ids[i] == 0xffffffff)
-				continue;
-
-			ret = phy_request_driver_module(dev,
-						c45_ids->device_ids[i]);
-			if (ret)
-				break;
-		}
-	} else {
-		ret = phy_request_driver_module(dev, phy_id);
-	}
-
-	if (!ret) {
-		device_initialize(&mdiodev->dev);
-	} else {
-		kfree(dev);
+	ret = phy_request_driver_module(dev);
+	if (ret) {
+		phy_device_free(dev);
 		dev = ERR_PTR(ret);
 	}
 
-- 
2.26.1

[PATCH 08/15] net: phy: check the PHY presence in get_phy_id()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

get_phy_id() is only called from get_phy_device() so the check for the
0x1fffffff value can be pulled into the former. This way it'll be easier
to remove get_phy_device() later on.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/phy_device.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8037a9663a85..eccbf6aea63d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -806,6 +806,10 @@ static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
 
 	*phy_id |= phy_reg;
 
+	/* If the phy_id is mostly Fs, there is no device there */
+	if ((*phy_id & 0x1fffffff) == 0x1fffffff)
+		return -ENODEV;
+
 	return 0;
 }
 
@@ -832,10 +836,6 @@ struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 	if (r)
 		return ERR_PTR(r);
 
-	/* If the phy_id is mostly Fs, there is no device there */
-	if ((phy_id & 0x1fffffff) == 0x1fffffff)
-		return ERR_PTR(-ENODEV);
-
 	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
 }
 EXPORT_SYMBOL(get_phy_device);
-- 
2.26.1

[PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Currently the PHY ID is read without taking the PHY out of reset. This
can only work if no resets are defined. This change delays the ID read
until we're actually registering the PHY device - this is needed because
earlier (when creating the device) we don't have a struct device yet
with resets already configured.

While we could use the of_ helpers for GPIO and resets, we will be adding
PHY regulator support layer on and there are no regulator APIs that work
without struct device.

This means that phy_device_create() now only instantiates the device but
doesn't request the relevant driver. If no phy_id is passed to
phy_device_create() (for that we introduce a new define: PHY_ID_NONE)
then the ID will be read inside phy_device_register().

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/phy_device.c | 47 +++++++++++++++++++-----------------
 include/linux/phy.h          |  1 +
 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index eccbf6aea63d..94944fffa9bb 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -658,12 +658,6 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 	INIT_DELAYED_WORK(&dev->state_queue, phy_state_machine);
 	device_initialize(&mdiodev->dev);
 
-	ret = phy_request_driver_module(dev);
-	if (ret) {
-		phy_device_free(dev);
-		dev = ERR_PTR(ret);
-	}
-
 	return dev;
 }
 EXPORT_SYMBOL(phy_device_create);
@@ -813,30 +807,29 @@ static int get_phy_id(struct mii_bus *bus, int addr, u32 *phy_id,
 	return 0;
 }
 
+static int phy_device_read_id(struct phy_device *phydev)
+{
+	struct mdio_device *mdiodev = &phydev->mdio;
+
+	phydev->c45_ids.devices_in_package = 0;
+	memset(phydev->c45_ids.device_ids, 0xff,
+	       sizeof(phydev->c45_ids.device_ids));
+
+	return get_phy_id(mdiodev->bus, mdiodev->addr, &phydev->phy_id,
+			  phydev->is_c45, &phydev->c45_ids);
+}
+
 /**
- * get_phy_device - reads the specified PHY device and returns its @phy_device
- *		    struct
+ * get_phy_device - create a phy_device withoug PHY ID
  * @bus: the target MII bus
  * @addr: PHY address on the MII bus
  * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
  *
- * Description: Reads the ID registers of the PHY at @addr on the
- *   @bus, then allocates and returns the phy_device to represent it.
+ * Allocates a new phy_device for @addr on the @bus.
  */
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
-	struct phy_c45_device_ids c45_ids;
-	u32 phy_id = 0;
-	int r;
-
-	c45_ids.devices_in_package = 0;
-	memset(c45_ids.device_ids, 0xff, sizeof(c45_ids.device_ids));
-
-	r = get_phy_id(bus, addr, &phy_id, is_c45, &c45_ids);
-	if (r)
-		return ERR_PTR(r);
-
-	return phy_device_create(bus, addr, phy_id, is_c45, &c45_ids);
+	return phy_device_create(bus, addr, PHY_ID_NONE, is_c45, NULL);
 }
 EXPORT_SYMBOL(get_phy_device);
 
@@ -855,6 +848,16 @@ int phy_device_register(struct phy_device *phydev)
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
 
+	if (phydev->phy_id == PHY_ID_NONE) {
+		err = phy_device_read_id(phydev);
+		if (err)
+			goto err_unregister_mdio;
+	}
+
+	err = phy_request_driver_module(phydev);
+	if (err)
+		goto err_unregister_mdio;
+
 	/* Run all of the fixups for this PHY */
 	err = phy_scan_fixups(phydev);
 	if (err) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 8c05d0fb5c00..2a695cd90c7c 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -742,6 +742,7 @@ struct phy_driver {
 
 #define PHY_ANY_ID "MATCH ANY PHY"
 #define PHY_ANY_UID 0xffffffff
+#define PHY_ID_NONE 0
 
 #define PHY_ID_MATCH_EXACT(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 0)
 #define PHY_ID_MATCH_MODEL(id) .phy_id = (id), .phy_id_mask = GENMASK(31, 4)
-- 
2.26.1

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Andrew Lunn]

On Mon, Jun 22, 2020 at 11:37:38AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Currently the PHY ID is read without taking the PHY out of reset. This
> can only work if no resets are defined. This change delays the ID read
> until we're actually registering the PHY device - this is needed because
> earlier (when creating the device) we don't have a struct device yet
> with resets already configured.
> 
> While we could use the of_ helpers for GPIO and resets, we will be adding
> PHY regulator support layer on and there are no regulator APIs that work
> without struct device.

The PHY subsystem cannot be the first to run into this problem, that
you need a device structure to make use of the regulator API, but you
need the regulator API to probe the device. How do other subsystems
work around this?

Maybe it is time to add a lower level API to the regulator framework?

   Andrew

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]

On Mon, Jun 22, 2020 at 03:39:40PM +0200, Andrew Lunn wrote:

> The PHY subsystem cannot be the first to run into this problem, that
> you need a device structure to make use of the regulator API, but you
> need the regulator API to probe the device. How do other subsystems
> work around this?

If the bus includes power management for the devices on the bus the
controller is generally responsible for that rather than the devices,
the devices access this via facilities provided by the bus if needed.
If the device is enumerated by firmware prior to being physically
enumerable then the bus will generally instantiate the device model
device and then arrange to wait for the physical device to appear and
get joined up with the device model device, typically in such situations
the physical device might appear and disappear dynamically at runtime
based on what the driver is doing anyway.

> Maybe it is time to add a lower level API to the regulator framework?

I don't see any need for that here, this is far from the only thing
that's keyed off a struct device and having the device appear and
disappear at runtime can make things like runtime PM look really messy
to userspace.

We could use a pre-probe stage in the device model for hotpluggable
buses in embedded contexts where you might need to bring things out of
reset or power them up before they'll appear on the bus for enumeration
but buses have mostly handled that at their level.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Florian Fainelli]

On 6/22/20 6:51 AM, Mark Brown wrote:
> On Mon, Jun 22, 2020 at 03:39:40PM +0200, Andrew Lunn wrote:
> 
>> The PHY subsystem cannot be the first to run into this problem, that
>> you need a device structure to make use of the regulator API, but you
>> need the regulator API to probe the device. How do other subsystems
>> work around this?
> 
> If the bus includes power management for the devices on the bus the
> controller is generally responsible for that rather than the devices,
> the devices access this via facilities provided by the bus if needed.
> If the device is enumerated by firmware prior to being physically
> enumerable then the bus will generally instantiate the device model
> device and then arrange to wait for the physical device to appear and
> get joined up with the device model device, typically in such situations
> the physical device might appear and disappear dynamically at runtime
> based on what the driver is doing anyway.

In premise there is nothing that prevents the MDIO bus from taking care
of the regulators, resets, prior to probing the PHY driver, what is
complicated here is that we do need to issue a read of the actual PHY to
know its 32-bit unique identifier and match it with an appropriate
driver. The way that we have worked around this with if you do not wish
such a hardware access to be made, is to provide an Ethernet PHY node
compatible string that encodes that 32-bit OUI directly. In premise the
same challenges exist with PCI devices/endpoints as well as USB, would
they have reset or regulator typically attached to them.

> 
>> Maybe it is time to add a lower level API to the regulator framework?
> 
> I don't see any need for that here, this is far from the only thing
> that's keyed off a struct device and having the device appear and
> disappear at runtime can make things like runtime PM look really messy
> to userspace.
> 
> We could use a pre-probe stage in the device model for hotpluggable
> buses in embedded contexts where you might need to bring things out of
> reset or power them up before they'll appear on the bus for enumeration
> but buses have mostly handled that at their level.
> 

That sounds like a better solution, are there any subsystems currently
implementing that, or would this be a generic Linux device driver model
addition that needs to be done?
-- 
Florian

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Florian Fainelli]

On 6/22/20 6:51 AM, Mark Brown wrote:
> On Mon, Jun 22, 2020 at 03:39:40PM +0200, Andrew Lunn wrote:
> 
>> The PHY subsystem cannot be the first to run into this problem, that
>> you need a device structure to make use of the regulator API, but you
>> need the regulator API to probe the device. How do other subsystems
>> work around this?
> 
> If the bus includes power management for the devices on the bus the
> controller is generally responsible for that rather than the devices,
> the devices access this via facilities provided by the bus if needed.
> If the device is enumerated by firmware prior to being physically
> enumerable then the bus will generally instantiate the device model
> device and then arrange to wait for the physical device to appear and
> get joined up with the device model device, typically in such situations
> the physical device might appear and disappear dynamically at runtime
> based on what the driver is doing anyway.

In premise there is nothing that prevents the MDIO bus from taking care
of the regulators, resets, prior to probing the PHY driver, what is
complicated here is that we do need to issue a read of the actual PHY to
know its 32-bit unique identifier and match it with an appropriate
driver. The way that we have worked around this with if you do not wish
such a hardware access to be made, is to provide an Ethernet PHY node
compatible string that encodes that 32-bit OUI directly. In premise the
same challenges exist with PCI devices/endpoints as well as USB, would
they have reset or regulator typically attached to them.

> 
>> Maybe it is time to add a lower level API to the regulator framework?
> 
> I don't see any need for that here, this is far from the only thing
> that's keyed off a struct device and having the device appear and
> disappear at runtime can make things like runtime PM look really messy
> to userspace.
> 
> We could use a pre-probe stage in the device model for hotpluggable
> buses in embedded contexts where you might need to bring things out of
> reset or power them up before they'll appear on the bus for enumeration
> but buses have mostly handled that at their level.
> 

That sounds like a better solution, are there any subsystems currently
implementing that, or would this be a generic Linux device driver model
addition that needs to be done?
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Florian Fainelli]

On 6/22/20 6:51 AM, Mark Brown wrote:
> On Mon, Jun 22, 2020 at 03:39:40PM +0200, Andrew Lunn wrote:
> 
>> The PHY subsystem cannot be the first to run into this problem, that
>> you need a device structure to make use of the regulator API, but you
>> need the regulator API to probe the device. How do other subsystems
>> work around this?
> 
> If the bus includes power management for the devices on the bus the
> controller is generally responsible for that rather than the devices,
> the devices access this via facilities provided by the bus if needed.
> If the device is enumerated by firmware prior to being physically
> enumerable then the bus will generally instantiate the device model
> device and then arrange to wait for the physical device to appear and
> get joined up with the device model device, typically in such situations
> the physical device might appear and disappear dynamically at runtime
> based on what the driver is doing anyway.

In premise there is nothing that prevents the MDIO bus from taking care
of the regulators, resets, prior to probing the PHY driver, what is
complicated here is that we do need to issue a read of the actual PHY to
know its 32-bit unique identifier and match it with an appropriate
driver. The way that we have worked around this with if you do not wish
such a hardware access to be made, is to provide an Ethernet PHY node
compatible string that encodes that 32-bit OUI directly. In premise the
same challenges exist with PCI devices/endpoints as well as USB, would
they have reset or regulator typically attached to them.

> 
>> Maybe it is time to add a lower level API to the regulator framework?
> 
> I don't see any need for that here, this is far from the only thing
> that's keyed off a struct device and having the device appear and
> disappear at runtime can make things like runtime PM look really messy
> to userspace.
> 
> We could use a pre-probe stage in the device model for hotpluggable
> buses in embedded contexts where you might need to bring things out of
> reset or power them up before they'll appear on the bus for enumeration
> but buses have mostly handled that at their level.
> 

That sounds like a better solution, are there any subsystems currently
implementing that, or would this be a generic Linux device driver model
addition that needs to be done?
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1982 bytes --]

On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote:
> On 6/22/20 6:51 AM, Mark Brown wrote:

> > If the bus includes power management for the devices on the bus the
> > controller is generally responsible for that rather than the devices,
> > the devices access this via facilities provided by the bus if needed.
> > If the device is enumerated by firmware prior to being physically
> > enumerable then the bus will generally instantiate the device model
> > device and then arrange to wait for the physical device to appear and
> > get joined up with the device model device, typically in such situations
> > the physical device might appear and disappear dynamically at runtime
> > based on what the driver is doing anyway.

> In premise there is nothing that prevents the MDIO bus from taking care
> of the regulators, resets, prior to probing the PHY driver, what is
> complicated here is that we do need to issue a read of the actual PHY to
> know its 32-bit unique identifier and match it with an appropriate
> driver. The way that we have worked around this with if you do not wish
> such a hardware access to be made, is to provide an Ethernet PHY node
> compatible string that encodes that 32-bit OUI directly. In premise the
> same challenges exist with PCI devices/endpoints as well as USB, would
> they have reset or regulator typically attached to them.

That all sounds very normal and is covered by both cases I describe?

> > We could use a pre-probe stage in the device model for hotpluggable
> > buses in embedded contexts where you might need to bring things out of
> > reset or power them up before they'll appear on the bus for enumeration
> > but buses have mostly handled that at their level.

> That sounds like a better solution, are there any subsystems currently
> implementing that, or would this be a generic Linux device driver model
> addition that needs to be done?

Like I say I'm suggesting doing something at the device model level.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1982 bytes --]

On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote:
> On 6/22/20 6:51 AM, Mark Brown wrote:

> > If the bus includes power management for the devices on the bus the
> > controller is generally responsible for that rather than the devices,
> > the devices access this via facilities provided by the bus if needed.
> > If the device is enumerated by firmware prior to being physically
> > enumerable then the bus will generally instantiate the device model
> > device and then arrange to wait for the physical device to appear and
> > get joined up with the device model device, typically in such situations
> > the physical device might appear and disappear dynamically at runtime
> > based on what the driver is doing anyway.

> In premise there is nothing that prevents the MDIO bus from taking care
> of the regulators, resets, prior to probing the PHY driver, what is
> complicated here is that we do need to issue a read of the actual PHY to
> know its 32-bit unique identifier and match it with an appropriate
> driver. The way that we have worked around this with if you do not wish
> such a hardware access to be made, is to provide an Ethernet PHY node
> compatible string that encodes that 32-bit OUI directly. In premise the
> same challenges exist with PCI devices/endpoints as well as USB, would
> they have reset or regulator typically attached to them.

That all sounds very normal and is covered by both cases I describe?

> > We could use a pre-probe stage in the device model for hotpluggable
> > buses in embedded contexts where you might need to bring things out of
> > reset or power them up before they'll appear on the bus for enumeration
> > but buses have mostly handled that at their level.

> That sounds like a better solution, are there any subsystems currently
> implementing that, or would this be a generic Linux device driver model
> addition that needs to be done?

Like I say I'm suggesting doing something at the device model level.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1982 bytes --]

On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote:
> On 6/22/20 6:51 AM, Mark Brown wrote:

> > If the bus includes power management for the devices on the bus the
> > controller is generally responsible for that rather than the devices,
> > the devices access this via facilities provided by the bus if needed.
> > If the device is enumerated by firmware prior to being physically
> > enumerable then the bus will generally instantiate the device model
> > device and then arrange to wait for the physical device to appear and
> > get joined up with the device model device, typically in such situations
> > the physical device might appear and disappear dynamically at runtime
> > based on what the driver is doing anyway.

> In premise there is nothing that prevents the MDIO bus from taking care
> of the regulators, resets, prior to probing the PHY driver, what is
> complicated here is that we do need to issue a read of the actual PHY to
> know its 32-bit unique identifier and match it with an appropriate
> driver. The way that we have worked around this with if you do not wish
> such a hardware access to be made, is to provide an Ethernet PHY node
> compatible string that encodes that 32-bit OUI directly. In premise the
> same challenges exist with PCI devices/endpoints as well as USB, would
> they have reset or regulator typically attached to them.

That all sounds very normal and is covered by both cases I describe?

> > We could use a pre-probe stage in the device model for hotpluggable
> > buses in embedded contexts where you might need to bring things out of
> > reset or power them up before they'll appear on the bus for enumeration
> > but buses have mostly handled that at their level.

> That sounds like a better solution, are there any subsystems currently
> implementing that, or would this be a generic Linux device driver model
> addition that needs to be done?

Like I say I'm suggesting doing something at the device model level.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Bartosz Golaszewski]

śr., 24 cze 2020 o 11:43 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote:
> > On 6/22/20 6:51 AM, Mark Brown wrote:
>
> > > If the bus includes power management for the devices on the bus the
> > > controller is generally responsible for that rather than the devices,
> > > the devices access this via facilities provided by the bus if needed.
> > > If the device is enumerated by firmware prior to being physically
> > > enumerable then the bus will generally instantiate the device model
> > > device and then arrange to wait for the physical device to appear and
> > > get joined up with the device model device, typically in such situations
> > > the physical device might appear and disappear dynamically at runtime
> > > based on what the driver is doing anyway.
>
> > In premise there is nothing that prevents the MDIO bus from taking care
> > of the regulators, resets, prior to probing the PHY driver, what is
> > complicated here is that we do need to issue a read of the actual PHY to
> > know its 32-bit unique identifier and match it with an appropriate
> > driver. The way that we have worked around this with if you do not wish
> > such a hardware access to be made, is to provide an Ethernet PHY node
> > compatible string that encodes that 32-bit OUI directly. In premise the
> > same challenges exist with PCI devices/endpoints as well as USB, would
> > they have reset or regulator typically attached to them.
>
> That all sounds very normal and is covered by both cases I describe?
>
> > > We could use a pre-probe stage in the device model for hotpluggable
> > > buses in embedded contexts where you might need to bring things out of
> > > reset or power them up before they'll appear on the bus for enumeration
> > > but buses have mostly handled that at their level.
>
> > That sounds like a better solution, are there any subsystems currently
> > implementing that, or would this be a generic Linux device driver model
> > addition that needs to be done?
>
> Like I say I'm suggesting doing something at the device model level.

I didn't expect to open such a can of worms...

This has evolved into several new concepts being proposed vs my
use-case which is relatively simple. The former will probably take
several months of development, reviews and discussions and it will
block supporting the phy supply on pumpkin boards upstream. I would
prefer not to redo what other MAC drivers do (phy-supply property on
the MAC node, controlling it from the MAC driver itself) if we've
already established it's wrong.

Is there any compromise we could reach to add support for a basic,
common use-case of a single regulator supplying a PHY that needs
enabling before reading its ID short-term (just like we currently
support a single reset or reset-gpios property for PHYs) and
introducing a whole new concept to the device model for more advanced
(but currently mostly hypothetical) cases long-term?

Bart

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Bartosz Golaszewski]

śr., 24 cze 2020 o 11:43 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote:
> > On 6/22/20 6:51 AM, Mark Brown wrote:
>
> > > If the bus includes power management for the devices on the bus the
> > > controller is generally responsible for that rather than the devices,
> > > the devices access this via facilities provided by the bus if needed.
> > > If the device is enumerated by firmware prior to being physically
> > > enumerable then the bus will generally instantiate the device model
> > > device and then arrange to wait for the physical device to appear and
> > > get joined up with the device model device, typically in such situations
> > > the physical device might appear and disappear dynamically at runtime
> > > based on what the driver is doing anyway.
>
> > In premise there is nothing that prevents the MDIO bus from taking care
> > of the regulators, resets, prior to probing the PHY driver, what is
> > complicated here is that we do need to issue a read of the actual PHY to
> > know its 32-bit unique identifier and match it with an appropriate
> > driver. The way that we have worked around this with if you do not wish
> > such a hardware access to be made, is to provide an Ethernet PHY node
> > compatible string that encodes that 32-bit OUI directly. In premise the
> > same challenges exist with PCI devices/endpoints as well as USB, would
> > they have reset or regulator typically attached to them.
>
> That all sounds very normal and is covered by both cases I describe?
>
> > > We could use a pre-probe stage in the device model for hotpluggable
> > > buses in embedded contexts where you might need to bring things out of
> > > reset or power them up before they'll appear on the bus for enumeration
> > > but buses have mostly handled that at their level.
>
> > That sounds like a better solution, are there any subsystems currently
> > implementing that, or would this be a generic Linux device driver model
> > addition that needs to be done?
>
> Like I say I'm suggesting doing something at the device model level.

I didn't expect to open such a can of worms...

This has evolved into several new concepts being proposed vs my
use-case which is relatively simple. The former will probably take
several months of development, reviews and discussions and it will
block supporting the phy supply on pumpkin boards upstream. I would
prefer not to redo what other MAC drivers do (phy-supply property on
the MAC node, controlling it from the MAC driver itself) if we've
already established it's wrong.

Is there any compromise we could reach to add support for a basic,
common use-case of a single regulator supplying a PHY that needs
enabling before reading its ID short-term (just like we currently
support a single reset or reset-gpios property for PHYs) and
introducing a whole new concept to the device model for more advanced
(but currently mostly hypothetical) cases long-term?

Bart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Bartosz Golaszewski]

śr., 24 cze 2020 o 11:43 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote:
> > On 6/22/20 6:51 AM, Mark Brown wrote:
>
> > > If the bus includes power management for the devices on the bus the
> > > controller is generally responsible for that rather than the devices,
> > > the devices access this via facilities provided by the bus if needed.
> > > If the device is enumerated by firmware prior to being physically
> > > enumerable then the bus will generally instantiate the device model
> > > device and then arrange to wait for the physical device to appear and
> > > get joined up with the device model device, typically in such situations
> > > the physical device might appear and disappear dynamically at runtime
> > > based on what the driver is doing anyway.
>
> > In premise there is nothing that prevents the MDIO bus from taking care
> > of the regulators, resets, prior to probing the PHY driver, what is
> > complicated here is that we do need to issue a read of the actual PHY to
> > know its 32-bit unique identifier and match it with an appropriate
> > driver. The way that we have worked around this with if you do not wish
> > such a hardware access to be made, is to provide an Ethernet PHY node
> > compatible string that encodes that 32-bit OUI directly. In premise the
> > same challenges exist with PCI devices/endpoints as well as USB, would
> > they have reset or regulator typically attached to them.
>
> That all sounds very normal and is covered by both cases I describe?
>
> > > We could use a pre-probe stage in the device model for hotpluggable
> > > buses in embedded contexts where you might need to bring things out of
> > > reset or power them up before they'll appear on the bus for enumeration
> > > but buses have mostly handled that at their level.
>
> > That sounds like a better solution, are there any subsystems currently
> > implementing that, or would this be a generic Linux device driver model
> > addition that needs to be done?
>
> Like I say I'm suggesting doing something at the device model level.

I didn't expect to open such a can of worms...

This has evolved into several new concepts being proposed vs my
use-case which is relatively simple. The former will probably take
several months of development, reviews and discussions and it will
block supporting the phy supply on pumpkin boards upstream. I would
prefer not to redo what other MAC drivers do (phy-supply property on
the MAC node, controlling it from the MAC driver itself) if we've
already established it's wrong.

Is there any compromise we could reach to add support for a basic,
common use-case of a single regulator supplying a PHY that needs
enabling before reading its ID short-term (just like we currently
support a single reset or reset-gpios property for PHYs) and
introducing a whole new concept to the device model for more advanced
(but currently mostly hypothetical) cases long-term?

Bart

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Florian Fainelli]

On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote:
> śr., 24 cze 2020 o 11:43 Mark Brown <broonie@kernel.org> napisał(a):
>>
>> On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote:
>>> On 6/22/20 6:51 AM, Mark Brown wrote:
>>
>>>> If the bus includes power management for the devices on the bus the
>>>> controller is generally responsible for that rather than the devices,
>>>> the devices access this via facilities provided by the bus if needed.
>>>> If the device is enumerated by firmware prior to being physically
>>>> enumerable then the bus will generally instantiate the device model
>>>> device and then arrange to wait for the physical device to appear and
>>>> get joined up with the device model device, typically in such situations
>>>> the physical device might appear and disappear dynamically at runtime
>>>> based on what the driver is doing anyway.
>>
>>> In premise there is nothing that prevents the MDIO bus from taking care
>>> of the regulators, resets, prior to probing the PHY driver, what is
>>> complicated here is that we do need to issue a read of the actual PHY to
>>> know its 32-bit unique identifier and match it with an appropriate
>>> driver. The way that we have worked around this with if you do not wish
>>> such a hardware access to be made, is to provide an Ethernet PHY node
>>> compatible string that encodes that 32-bit OUI directly. In premise the
>>> same challenges exist with PCI devices/endpoints as well as USB, would
>>> they have reset or regulator typically attached to them.
>>
>> That all sounds very normal and is covered by both cases I describe?
>>
>>>> We could use a pre-probe stage in the device model for hotpluggable
>>>> buses in embedded contexts where you might need to bring things out of
>>>> reset or power them up before they'll appear on the bus for enumeration
>>>> but buses have mostly handled that at their level.
>>
>>> That sounds like a better solution, are there any subsystems currently
>>> implementing that, or would this be a generic Linux device driver model
>>> addition that needs to be done?
>>
>> Like I say I'm suggesting doing something at the device model level.
> 
> I didn't expect to open such a can of worms...
> 
> This has evolved into several new concepts being proposed vs my
> use-case which is relatively simple. The former will probably take
> several months of development, reviews and discussions and it will
> block supporting the phy supply on pumpkin boards upstream. I would
> prefer not to redo what other MAC drivers do (phy-supply property on
> the MAC node, controlling it from the MAC driver itself) if we've
> already established it's wrong.

You are not new to Linux development, so none of this should come as a
surprise to you. Your proposed solution has clearly short comings and is
a hack, especially around the PHY_ID_NONE business to get a phy_device
only then to have the real PHY device ID. You should also now that "I
need it now because my product deliverable depends on it" has never been
received as a valid argument to coerce people into accepting a solution
for which there are at review time known deficiencies to the proposed
approach.

> 
> Is there any compromise we could reach to add support for a basic,
> common use-case of a single regulator supplying a PHY that needs
> enabling before reading its ID short-term (just like we currently
> support a single reset or reset-gpios property for PHYs) and
> introducing a whole new concept to the device model for more advanced
> (but currently mostly hypothetical) cases long-term?

The pre-probe use case is absolutely not hypothetical, and I would need
it for pcie-brcmstb.c at some point which is a PCIe root complex driver
with multiple regulators that need to be turned on *prior* to
enumerating the PCIe bus and creating pci_device instances. It is
literally the same thing as what you are trying to do, just in a
different subsystem, therefore I am happy to test and review your patches.
-- 
Florian

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Florian Fainelli]

On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote:
> śr., 24 cze 2020 o 11:43 Mark Brown <broonie@kernel.org> napisał(a):
>>
>> On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote:
>>> On 6/22/20 6:51 AM, Mark Brown wrote:
>>
>>>> If the bus includes power management for the devices on the bus the
>>>> controller is generally responsible for that rather than the devices,
>>>> the devices access this via facilities provided by the bus if needed.
>>>> If the device is enumerated by firmware prior to being physically
>>>> enumerable then the bus will generally instantiate the device model
>>>> device and then arrange to wait for the physical device to appear and
>>>> get joined up with the device model device, typically in such situations
>>>> the physical device might appear and disappear dynamically at runtime
>>>> based on what the driver is doing anyway.
>>
>>> In premise there is nothing that prevents the MDIO bus from taking care
>>> of the regulators, resets, prior to probing the PHY driver, what is
>>> complicated here is that we do need to issue a read of the actual PHY to
>>> know its 32-bit unique identifier and match it with an appropriate
>>> driver. The way that we have worked around this with if you do not wish
>>> such a hardware access to be made, is to provide an Ethernet PHY node
>>> compatible string that encodes that 32-bit OUI directly. In premise the
>>> same challenges exist with PCI devices/endpoints as well as USB, would
>>> they have reset or regulator typically attached to them.
>>
>> That all sounds very normal and is covered by both cases I describe?
>>
>>>> We could use a pre-probe stage in the device model for hotpluggable
>>>> buses in embedded contexts where you might need to bring things out of
>>>> reset or power them up before they'll appear on the bus for enumeration
>>>> but buses have mostly handled that at their level.
>>
>>> That sounds like a better solution, are there any subsystems currently
>>> implementing that, or would this be a generic Linux device driver model
>>> addition that needs to be done?
>>
>> Like I say I'm suggesting doing something at the device model level.
> 
> I didn't expect to open such a can of worms...
> 
> This has evolved into several new concepts being proposed vs my
> use-case which is relatively simple. The former will probably take
> several months of development, reviews and discussions and it will
> block supporting the phy supply on pumpkin boards upstream. I would
> prefer not to redo what other MAC drivers do (phy-supply property on
> the MAC node, controlling it from the MAC driver itself) if we've
> already established it's wrong.

You are not new to Linux development, so none of this should come as a
surprise to you. Your proposed solution has clearly short comings and is
a hack, especially around the PHY_ID_NONE business to get a phy_device
only then to have the real PHY device ID. You should also now that "I
need it now because my product deliverable depends on it" has never been
received as a valid argument to coerce people into accepting a solution
for which there are at review time known deficiencies to the proposed
approach.

> 
> Is there any compromise we could reach to add support for a basic,
> common use-case of a single regulator supplying a PHY that needs
> enabling before reading its ID short-term (just like we currently
> support a single reset or reset-gpios property for PHYs) and
> introducing a whole new concept to the device model for more advanced
> (but currently mostly hypothetical) cases long-term?

The pre-probe use case is absolutely not hypothetical, and I would need
it for pcie-brcmstb.c at some point which is a PCIe root complex driver
with multiple regulators that need to be turned on *prior* to
enumerating the PCIe bus and creating pci_device instances. It is
literally the same thing as what you are trying to do, just in a
different subsystem, therefore I am happy to test and review your patches.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Florian Fainelli]

On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote:
> śr., 24 cze 2020 o 11:43 Mark Brown <broonie@kernel.org> napisał(a):
>>
>> On Tue, Jun 23, 2020 at 12:49:15PM -0700, Florian Fainelli wrote:
>>> On 6/22/20 6:51 AM, Mark Brown wrote:
>>
>>>> If the bus includes power management for the devices on the bus the
>>>> controller is generally responsible for that rather than the devices,
>>>> the devices access this via facilities provided by the bus if needed.
>>>> If the device is enumerated by firmware prior to being physically
>>>> enumerable then the bus will generally instantiate the device model
>>>> device and then arrange to wait for the physical device to appear and
>>>> get joined up with the device model device, typically in such situations
>>>> the physical device might appear and disappear dynamically at runtime
>>>> based on what the driver is doing anyway.
>>
>>> In premise there is nothing that prevents the MDIO bus from taking care
>>> of the regulators, resets, prior to probing the PHY driver, what is
>>> complicated here is that we do need to issue a read of the actual PHY to
>>> know its 32-bit unique identifier and match it with an appropriate
>>> driver. The way that we have worked around this with if you do not wish
>>> such a hardware access to be made, is to provide an Ethernet PHY node
>>> compatible string that encodes that 32-bit OUI directly. In premise the
>>> same challenges exist with PCI devices/endpoints as well as USB, would
>>> they have reset or regulator typically attached to them.
>>
>> That all sounds very normal and is covered by both cases I describe?
>>
>>>> We could use a pre-probe stage in the device model for hotpluggable
>>>> buses in embedded contexts where you might need to bring things out of
>>>> reset or power them up before they'll appear on the bus for enumeration
>>>> but buses have mostly handled that at their level.
>>
>>> That sounds like a better solution, are there any subsystems currently
>>> implementing that, or would this be a generic Linux device driver model
>>> addition that needs to be done?
>>
>> Like I say I'm suggesting doing something at the device model level.
> 
> I didn't expect to open such a can of worms...
> 
> This has evolved into several new concepts being proposed vs my
> use-case which is relatively simple. The former will probably take
> several months of development, reviews and discussions and it will
> block supporting the phy supply on pumpkin boards upstream. I would
> prefer not to redo what other MAC drivers do (phy-supply property on
> the MAC node, controlling it from the MAC driver itself) if we've
> already established it's wrong.

You are not new to Linux development, so none of this should come as a
surprise to you. Your proposed solution has clearly short comings and is
a hack, especially around the PHY_ID_NONE business to get a phy_device
only then to have the real PHY device ID. You should also now that "I
need it now because my product deliverable depends on it" has never been
received as a valid argument to coerce people into accepting a solution
for which there are at review time known deficiencies to the proposed
approach.

> 
> Is there any compromise we could reach to add support for a basic,
> common use-case of a single regulator supplying a PHY that needs
> enabling before reading its ID short-term (just like we currently
> support a single reset or reset-gpios property for PHYs) and
> introducing a whole new concept to the device model for more advanced
> (but currently mostly hypothetical) cases long-term?

The pre-probe use case is absolutely not hypothetical, and I would need
it for pcie-brcmstb.c at some point which is a PCIe root complex driver
with multiple regulators that need to be turned on *prior* to
enumerating the PCIe bus and creating pci_device instances. It is
literally the same thing as what you are trying to do, just in a
different subsystem, therefore I am happy to test and review your patches.
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Bartosz Golaszewski]

On Wed, Jun 24, 2020 at 6:06 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>

[snip!]

> >
> > This has evolved into several new concepts being proposed vs my
> > use-case which is relatively simple. The former will probably take
> > several months of development, reviews and discussions and it will
> > block supporting the phy supply on pumpkin boards upstream. I would
> > prefer not to redo what other MAC drivers do (phy-supply property on
> > the MAC node, controlling it from the MAC driver itself) if we've
> > already established it's wrong.
>
> You are not new to Linux development, so none of this should come as a
> surprise to you. Your proposed solution has clearly short comings and is
> a hack, especially around the PHY_ID_NONE business to get a phy_device
> only then to have the real PHY device ID. You should also now that "I
> need it now because my product deliverable depends on it" has never been
> received as a valid argument to coerce people into accepting a solution
> for which there are at review time known deficiencies to the proposed
> approach.
>

Don't get me wrong, I understand that full well. On the other hand a
couple years ago I put a significant amount of work into the concept
of early platform device drivers for linux clocksource, clock and
interrupt drivers. Every reviewer had his own preferred approach and
after something like three completely different submissions and
several conversations at conferences I simply gave up due to all the
bikeshedding. It just wasn't moving forward and frankly: I expect any
changes to the core driver model to follow a similar path of most
resistance.

I will give it a shot but at some point getting the job done is better
than not getting it done just because the solution isn't perfect. IMO
this approach is still slightly more correct than controlling the
PHY's supply from the MAC driver.

Bartosz

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Bartosz Golaszewski]

On Wed, Jun 24, 2020 at 6:06 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>

[snip!]

> >
> > This has evolved into several new concepts being proposed vs my
> > use-case which is relatively simple. The former will probably take
> > several months of development, reviews and discussions and it will
> > block supporting the phy supply on pumpkin boards upstream. I would
> > prefer not to redo what other MAC drivers do (phy-supply property on
> > the MAC node, controlling it from the MAC driver itself) if we've
> > already established it's wrong.
>
> You are not new to Linux development, so none of this should come as a
> surprise to you. Your proposed solution has clearly short comings and is
> a hack, especially around the PHY_ID_NONE business to get a phy_device
> only then to have the real PHY device ID. You should also now that "I
> need it now because my product deliverable depends on it" has never been
> received as a valid argument to coerce people into accepting a solution
> for which there are at review time known deficiencies to the proposed
> approach.
>

Don't get me wrong, I understand that full well. On the other hand a
couple years ago I put a significant amount of work into the concept
of early platform device drivers for linux clocksource, clock and
interrupt drivers. Every reviewer had his own preferred approach and
after something like three completely different submissions and
several conversations at conferences I simply gave up due to all the
bikeshedding. It just wasn't moving forward and frankly: I expect any
changes to the core driver model to follow a similar path of most
resistance.

I will give it a shot but at some point getting the job done is better
than not getting it done just because the solution isn't perfect. IMO
this approach is still slightly more correct than controlling the
PHY's supply from the MAC driver.

Bartosz

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Bartosz Golaszewski]

On Wed, Jun 24, 2020 at 6:06 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
>

[snip!]

> >
> > This has evolved into several new concepts being proposed vs my
> > use-case which is relatively simple. The former will probably take
> > several months of development, reviews and discussions and it will
> > block supporting the phy supply on pumpkin boards upstream. I would
> > prefer not to redo what other MAC drivers do (phy-supply property on
> > the MAC node, controlling it from the MAC driver itself) if we've
> > already established it's wrong.
>
> You are not new to Linux development, so none of this should come as a
> surprise to you. Your proposed solution has clearly short comings and is
> a hack, especially around the PHY_ID_NONE business to get a phy_device
> only then to have the real PHY device ID. You should also now that "I
> need it now because my product deliverable depends on it" has never been
> received as a valid argument to coerce people into accepting a solution
> for which there are at review time known deficiencies to the proposed
> approach.
>

Don't get me wrong, I understand that full well. On the other hand a
couple years ago I put a significant amount of work into the concept
of early platform device drivers for linux clocksource, clock and
interrupt drivers. Every reviewer had his own preferred approach and
after something like three completely different submissions and
several conversations at conferences I simply gave up due to all the
bikeshedding. It just wasn't moving forward and frankly: I expect any
changes to the core driver model to follow a similar path of most
resistance.

I will give it a shot but at some point getting the job done is better
than not getting it done just because the solution isn't perfect. IMO
this approach is still slightly more correct than controlling the
PHY's supply from the MAC driver.

Bartosz

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Russell King - ARM Linux admin]

On Wed, Jun 24, 2020 at 09:06:28AM -0700, Florian Fainelli wrote:
> On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote:
> > I didn't expect to open such a can of worms...
> > 
> > This has evolved into several new concepts being proposed vs my
> > use-case which is relatively simple. The former will probably take
> > several months of development, reviews and discussions and it will
> > block supporting the phy supply on pumpkin boards upstream. I would
> > prefer not to redo what other MAC drivers do (phy-supply property on
> > the MAC node, controlling it from the MAC driver itself) if we've
> > already established it's wrong.
> 
> You are not new to Linux development, so none of this should come as a
> surprise to you. Your proposed solution has clearly short comings and is
> a hack, especially around the PHY_ID_NONE business to get a phy_device
> only then to have the real PHY device ID. You should also now that "I
> need it now because my product deliverable depends on it" has never been
> received as a valid argument to coerce people into accepting a solution
> for which there are at review time known deficiencies to the proposed
> approach.

It /is/ a generic issue.  The same problem exists for AMBA Primecell
devices, and that code has an internal deferred device list that it
manages.  See drivers/amba/bus.c, amba_deferred_retry_func(),
amba_device_try_add(), and amba_device_add().

As we see more devices gain this property, it needs to be addressed
in a generic way, rather than coming up with multiple bus specific
implementations.

Maybe struct bus_type needs a method to do the preparation to add
a device (such as reading IDs etc), which is called by device_add().
If that method returns -EPROBE_DEFER, the device gets added to a
deferred list, which gets retried when drivers are successfully
probed.  Possible maybe?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Russell King - ARM Linux admin]

On Wed, Jun 24, 2020 at 09:06:28AM -0700, Florian Fainelli wrote:
> On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote:
> > I didn't expect to open such a can of worms...
> > 
> > This has evolved into several new concepts being proposed vs my
> > use-case which is relatively simple. The former will probably take
> > several months of development, reviews and discussions and it will
> > block supporting the phy supply on pumpkin boards upstream. I would
> > prefer not to redo what other MAC drivers do (phy-supply property on
> > the MAC node, controlling it from the MAC driver itself) if we've
> > already established it's wrong.
> 
> You are not new to Linux development, so none of this should come as a
> surprise to you. Your proposed solution has clearly short comings and is
> a hack, especially around the PHY_ID_NONE business to get a phy_device
> only then to have the real PHY device ID. You should also now that "I
> need it now because my product deliverable depends on it" has never been
> received as a valid argument to coerce people into accepting a solution
> for which there are at review time known deficiencies to the proposed
> approach.

It /is/ a generic issue.  The same problem exists for AMBA Primecell
devices, and that code has an internal deferred device list that it
manages.  See drivers/amba/bus.c, amba_deferred_retry_func(),
amba_device_try_add(), and amba_device_add().

As we see more devices gain this property, it needs to be addressed
in a generic way, rather than coming up with multiple bus specific
implementations.

Maybe struct bus_type needs a method to do the preparation to add
a device (such as reading IDs etc), which is called by device_add().
If that method returns -EPROBE_DEFER, the device gets added to a
deferred list, which gets retried when drivers are successfully
probed.  Possible maybe?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Russell King - ARM Linux admin]

On Wed, Jun 24, 2020 at 09:06:28AM -0700, Florian Fainelli wrote:
> On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote:
> > I didn't expect to open such a can of worms...
> > 
> > This has evolved into several new concepts being proposed vs my
> > use-case which is relatively simple. The former will probably take
> > several months of development, reviews and discussions and it will
> > block supporting the phy supply on pumpkin boards upstream. I would
> > prefer not to redo what other MAC drivers do (phy-supply property on
> > the MAC node, controlling it from the MAC driver itself) if we've
> > already established it's wrong.
> 
> You are not new to Linux development, so none of this should come as a
> surprise to you. Your proposed solution has clearly short comings and is
> a hack, especially around the PHY_ID_NONE business to get a phy_device
> only then to have the real PHY device ID. You should also now that "I
> need it now because my product deliverable depends on it" has never been
> received as a valid argument to coerce people into accepting a solution
> for which there are at review time known deficiencies to the proposed
> approach.

It /is/ a generic issue.  The same problem exists for AMBA Primecell
devices, and that code has an internal deferred device list that it
manages.  See drivers/amba/bus.c, amba_deferred_retry_func(),
amba_device_try_add(), and amba_device_add().

As we see more devices gain this property, it needs to be addressed
in a generic way, rather than coming up with multiple bus specific
implementations.

Maybe struct bus_type needs a method to do the preparation to add
a device (such as reading IDs etc), which is called by device_add().
If that method returns -EPROBE_DEFER, the device gets added to a
deferred list, which gets retried when drivers are successfully
probed.  Possible maybe?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Robin Murphy]

On 2020-06-24 17:50, Russell King - ARM Linux admin wrote:
> On Wed, Jun 24, 2020 at 09:06:28AM -0700, Florian Fainelli wrote:
>> On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote:
>>> I didn't expect to open such a can of worms...
>>>
>>> This has evolved into several new concepts being proposed vs my
>>> use-case which is relatively simple. The former will probably take
>>> several months of development, reviews and discussions and it will
>>> block supporting the phy supply on pumpkin boards upstream. I would
>>> prefer not to redo what other MAC drivers do (phy-supply property on
>>> the MAC node, controlling it from the MAC driver itself) if we've
>>> already established it's wrong.
>>
>> You are not new to Linux development, so none of this should come as a
>> surprise to you. Your proposed solution has clearly short comings and is
>> a hack, especially around the PHY_ID_NONE business to get a phy_device
>> only then to have the real PHY device ID. You should also now that "I
>> need it now because my product deliverable depends on it" has never been
>> received as a valid argument to coerce people into accepting a solution
>> for which there are at review time known deficiencies to the proposed
>> approach.
> 
> It /is/ a generic issue.  The same problem exists for AMBA Primecell
> devices, and that code has an internal deferred device list that it
> manages.  See drivers/amba/bus.c, amba_deferred_retry_func(),
> amba_device_try_add(), and amba_device_add().
> 
> As we see more devices gain this property, it needs to be addressed
> in a generic way, rather than coming up with multiple bus specific
> implementations.
> 
> Maybe struct bus_type needs a method to do the preparation to add
> a device (such as reading IDs etc), which is called by device_add().
> If that method returns -EPROBE_DEFER, the device gets added to a
> deferred list, which gets retried when drivers are successfully
> probed.  Possible maybe?

FWIW that would be ideal for solving an ordering a problem we have in 
the IOMMU subsystem too (which we currently sort-of-handle by deferring 
driver probe from dma_configure(), but it really needs to be done 
earlier and not depend on drivers being present at all).

Robin.

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Robin Murphy]

On 2020-06-24 17:50, Russell King - ARM Linux admin wrote:
> On Wed, Jun 24, 2020 at 09:06:28AM -0700, Florian Fainelli wrote:
>> On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote:
>>> I didn't expect to open such a can of worms...
>>>
>>> This has evolved into several new concepts being proposed vs my
>>> use-case which is relatively simple. The former will probably take
>>> several months of development, reviews and discussions and it will
>>> block supporting the phy supply on pumpkin boards upstream. I would
>>> prefer not to redo what other MAC drivers do (phy-supply property on
>>> the MAC node, controlling it from the MAC driver itself) if we've
>>> already established it's wrong.
>>
>> You are not new to Linux development, so none of this should come as a
>> surprise to you. Your proposed solution has clearly short comings and is
>> a hack, especially around the PHY_ID_NONE business to get a phy_device
>> only then to have the real PHY device ID. You should also now that "I
>> need it now because my product deliverable depends on it" has never been
>> received as a valid argument to coerce people into accepting a solution
>> for which there are at review time known deficiencies to the proposed
>> approach.
> 
> It /is/ a generic issue.  The same problem exists for AMBA Primecell
> devices, and that code has an internal deferred device list that it
> manages.  See drivers/amba/bus.c, amba_deferred_retry_func(),
> amba_device_try_add(), and amba_device_add().
> 
> As we see more devices gain this property, it needs to be addressed
> in a generic way, rather than coming up with multiple bus specific
> implementations.
> 
> Maybe struct bus_type needs a method to do the preparation to add
> a device (such as reading IDs etc), which is called by device_add().
> If that method returns -EPROBE_DEFER, the device gets added to a
> deferred list, which gets retried when drivers are successfully
> probed.  Possible maybe?

FWIW that would be ideal for solving an ordering a problem we have in 
the IOMMU subsystem too (which we currently sort-of-handle by deferring 
driver probe from dma_configure(), but it really needs to be done 
earlier and not depend on drivers being present at all).

Robin.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 09/15] net: phy: delay PHY driver probe until PHY registration

[Robin Murphy]

On 2020-06-24 17:50, Russell King - ARM Linux admin wrote:
> On Wed, Jun 24, 2020 at 09:06:28AM -0700, Florian Fainelli wrote:
>> On 6/24/2020 6:48 AM, Bartosz Golaszewski wrote:
>>> I didn't expect to open such a can of worms...
>>>
>>> This has evolved into several new concepts being proposed vs my
>>> use-case which is relatively simple. The former will probably take
>>> several months of development, reviews and discussions and it will
>>> block supporting the phy supply on pumpkin boards upstream. I would
>>> prefer not to redo what other MAC drivers do (phy-supply property on
>>> the MAC node, controlling it from the MAC driver itself) if we've
>>> already established it's wrong.
>>
>> You are not new to Linux development, so none of this should come as a
>> surprise to you. Your proposed solution has clearly short comings and is
>> a hack, especially around the PHY_ID_NONE business to get a phy_device
>> only then to have the real PHY device ID. You should also now that "I
>> need it now because my product deliverable depends on it" has never been
>> received as a valid argument to coerce people into accepting a solution
>> for which there are at review time known deficiencies to the proposed
>> approach.
> 
> It /is/ a generic issue.  The same problem exists for AMBA Primecell
> devices, and that code has an internal deferred device list that it
> manages.  See drivers/amba/bus.c, amba_deferred_retry_func(),
> amba_device_try_add(), and amba_device_add().
> 
> As we see more devices gain this property, it needs to be addressed
> in a generic way, rather than coming up with multiple bus specific
> implementations.
> 
> Maybe struct bus_type needs a method to do the preparation to add
> a device (such as reading IDs etc), which is called by device_add().
> If that method returns -EPROBE_DEFER, the device gets added to a
> deferred list, which gets retried when drivers are successfully
> probed.  Possible maybe?

FWIW that would be ideal for solving an ordering a problem we have in 
the IOMMU subsystem too (which we currently sort-of-handle by deferring 
driver probe from dma_configure(), but it really needs to be done 
earlier and not depend on drivers being present at all).

Robin.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 10/15] net: phy: simplify phy_device_create()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The last argument passed to phy_device_create() (c45_ids) is never used
in current mainline outside of the core PHY code - it can only be
configured when reading the PHY ID from phy_device_read_id().

Let's drop this argument treewide.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/nxp-tja11xx.c | 2 +-
 drivers/net/phy/phy_device.c  | 8 +++-----
 drivers/of/of_mdio.c          | 2 +-
 include/linux/phy.h           | 3 +--
 4 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
index a72fa0d2e7c7..b98b620ef88c 100644
--- a/drivers/net/phy/nxp-tja11xx.c
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -507,7 +507,7 @@ static void tja1102_p1_register(struct work_struct *work)
 		}
 
 		/* Real PHY ID of Port 1 is 0 */
-		phy = phy_device_create(bus, addr, PHY_ID_TJA1102, false, NULL);
+		phy = phy_device_create(bus, addr, PHY_ID_TJA1102, false);
 		if (IS_ERR(phy)) {
 			dev_err(dev, "Can't create PHY device for Port 1: %i\n",
 				addr);
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 94944fffa9bb..ad7c4cd9d357 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -613,8 +613,7 @@ static int phy_request_driver_module(struct phy_device *phydev)
 }
 
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
-				     bool is_c45,
-				     struct phy_c45_device_ids *c45_ids)
+				     bool is_c45)
 {
 	struct phy_device *dev;
 	struct mdio_device *mdiodev;
@@ -647,8 +646,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 
 	dev->is_c45 = is_c45;
 	dev->phy_id = phy_id;
-	if (c45_ids)
-		dev->c45_ids = *c45_ids;
+
 	dev->irq = bus->irq[addr];
 	dev_set_name(&mdiodev->dev, PHY_ID_FMT, bus->id, addr);
 
@@ -829,7 +827,7 @@ static int phy_device_read_id(struct phy_device *phydev)
  */
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
-	return phy_device_create(bus, addr, PHY_ID_NONE, is_c45, NULL);
+	return phy_device_create(bus, addr, PHY_ID_NONE, is_c45);
 }
 EXPORT_SYMBOL(get_phy_device);
 
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index a04afe79529c..63843037673c 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -121,7 +121,7 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
 					 "ethernet-phy-ieee802.3-c45");
 
 	if (!is_c45 && !of_get_phy_id(child, &phy_id))
-		phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
+		phy = phy_device_create(mdio, addr, phy_id, 0);
 	else
 		phy = get_phy_device(mdio, addr, is_c45);
 	if (IS_ERR(phy)) {
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 2a695cd90c7c..662919c1dd27 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1213,8 +1213,7 @@ int phy_modify_paged(struct phy_device *phydev, int page, u32 regnum,
 		     u16 mask, u16 set);
 
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
-				     bool is_c45,
-				     struct phy_c45_device_ids *c45_ids);
+				     bool is_c45);
 #if IS_ENABLED(CONFIG_PHYLIB)
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
-- 
2.26.1

[PATCH 11/15] net: phy: drop get_phy_device()

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

get_phy_device() has now become just a wrapper for phy_device_create()
with the phy_id argument set to PHY_ID_NONE. Let's remove this function
treewide and replace it with opencoded phy_device_create(). This has the
advantage of being more explicit about the PHY not having the ID set
when being created.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/dsa/ocelot/felix_vsc9959.c            |  3 ++-
 drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c       |  5 +++--
 drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c |  2 +-
 drivers/net/ethernet/socionext/netsec.c           |  3 ++-
 drivers/net/phy/fixed_phy.c                       |  2 +-
 drivers/net/phy/mdio-xgene.c                      |  2 +-
 drivers/net/phy/mdio_bus.c                        |  2 +-
 drivers/net/phy/phy_device.c                      | 14 --------------
 drivers/net/phy/sfp.c                             |  2 +-
 drivers/of/of_mdio.c                              | 11 +++++++----
 include/linux/phy.h                               |  7 -------
 11 files changed, 19 insertions(+), 34 deletions(-)

diff --git a/drivers/net/dsa/ocelot/felix_vsc9959.c b/drivers/net/dsa/ocelot/felix_vsc9959.c
index 1dd9e348152d..40c868382e03 100644
--- a/drivers/net/dsa/ocelot/felix_vsc9959.c
+++ b/drivers/net/dsa/ocelot/felix_vsc9959.c
@@ -1183,7 +1183,8 @@ static int vsc9959_mdio_bus_alloc(struct ocelot *ocelot)
 		if (ocelot_port->phy_mode == PHY_INTERFACE_MODE_USXGMII)
 			is_c45 = true;
 
-		pcs = get_phy_device(felix->imdio, port, is_c45);
+		pcs = phy_device_create(felix->imdio, port,
+					PHY_ID_NONE, is_c45);
 		if (IS_ERR(pcs))
 			continue;
 
diff --git a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
index 46c3c1ca38d6..1117ed468abf 100644
--- a/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
+++ b/drivers/net/ethernet/amd/xgbe/xgbe-phy-v2.c
@@ -1017,8 +1017,9 @@ static int xgbe_phy_find_phy_device(struct xgbe_prv_data *pdata)
 	}
 
 	/* Create and connect to the PHY device */
-	phydev = get_phy_device(phy_data->mii, phy_data->mdio_addr,
-				(phy_data->phydev_mode == XGBE_MDIO_MODE_CL45));
+	phydev = phy_device_create(phy_data->mii, phy_data->mdio_addr,
+			PHY_ID_NONE,
+			phy_data->phydev_mode == XGBE_MDIO_MODE_CL45);
 	if (IS_ERR(phydev)) {
 		netdev_err(pdata->netdev, "get_phy_device failed\n");
 		return -ENODEV;
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
index 9a907947ba19..75fa6a855727 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
@@ -703,7 +703,7 @@ hns_mac_register_phydev(struct mii_bus *mdio, struct hns_mac_cb *mac_cb,
 	else
 		return -ENODATA;
 
-	phy = get_phy_device(mdio, addr, is_c45);
+	phy = phy_device_create(mdio, addr, PHY_ID_NONE, is_c45);
 	if (!phy || IS_ERR(phy))
 		return -EIO;
 
diff --git a/drivers/net/ethernet/socionext/netsec.c b/drivers/net/ethernet/socionext/netsec.c
index 328bc38848bb..48c405d81000 100644
--- a/drivers/net/ethernet/socionext/netsec.c
+++ b/drivers/net/ethernet/socionext/netsec.c
@@ -1935,7 +1935,8 @@ static int netsec_register_mdio(struct netsec_priv *priv, u32 phy_addr)
 			return ret;
 		}
 
-		priv->phydev = get_phy_device(bus, phy_addr, false);
+		priv->phydev = phy_device_create(bus, phy_addr,
+						 PHY_ID_NONE, false);
 		if (IS_ERR(priv->phydev)) {
 			ret = PTR_ERR(priv->phydev);
 			dev_err(priv->dev, "get_phy_device err(%d)\n", ret);
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index c4641b1704d6..9019f0035ef0 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -254,7 +254,7 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
 		return ERR_PTR(ret);
 	}
 
-	phy = get_phy_device(fmb->mii_bus, phy_addr, false);
+	phy = phy_device_create(fmb->mii_bus, phy_addr, PHY_ID_NONE, false);
 	if (IS_ERR(phy)) {
 		fixed_phy_del(phy_addr);
 		return ERR_PTR(-EINVAL);
diff --git a/drivers/net/phy/mdio-xgene.c b/drivers/net/phy/mdio-xgene.c
index 34990eaa3298..6698e7caaf78 100644
--- a/drivers/net/phy/mdio-xgene.c
+++ b/drivers/net/phy/mdio-xgene.c
@@ -264,7 +264,7 @@ struct phy_device *xgene_enet_phy_register(struct mii_bus *bus, int phy_addr)
 {
 	struct phy_device *phy_dev;
 
-	phy_dev = get_phy_device(bus, phy_addr, false);
+	phy_dev = phy_device_create(bus, phy_addr, PHY_ID_NONE, false);
 	if (!phy_dev || IS_ERR(phy_dev))
 		return NULL;
 
diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 296cf9771483..53e2fb0be7b9 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -742,7 +742,7 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr)
 	struct phy_device *phydev;
 	int err;
 
-	phydev = get_phy_device(bus, addr, false);
+	phydev = phy_device_create(bus, addr, PHY_ID_NONE, false);
 	if (IS_ERR(phydev))
 		return phydev;
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index ad7c4cd9d357..58923826838b 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -817,20 +817,6 @@ static int phy_device_read_id(struct phy_device *phydev)
 			  phydev->is_c45, &phydev->c45_ids);
 }
 
-/**
- * get_phy_device - create a phy_device withoug PHY ID
- * @bus: the target MII bus
- * @addr: PHY address on the MII bus
- * @is_c45: If true the PHY uses the 802.3 clause 45 protocol
- *
- * Allocates a new phy_device for @addr on the @bus.
- */
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
-{
-	return phy_device_create(bus, addr, PHY_ID_NONE, is_c45);
-}
-EXPORT_SYMBOL(get_phy_device);
-
 /**
  * phy_device_register - Register the phy device on the MDIO bus
  * @phydev: phy_device structure to be added to the MDIO bus
diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
index 73c2969f11a4..0b165d928762 100644
--- a/drivers/net/phy/sfp.c
+++ b/drivers/net/phy/sfp.c
@@ -1431,7 +1431,7 @@ static int sfp_sm_probe_phy(struct sfp *sfp, bool is_c45)
 	struct phy_device *phy;
 	int err;
 
-	phy = get_phy_device(sfp->i2c_mii, SFP_PHY_ADDR, is_c45);
+	phy = phy_device_create(sfp->i2c_mii, SFP_PHY_ADDR, PHY_ID_NONE, is_c45);
 	if (phy == ERR_PTR(-ENODEV))
 		return PTR_ERR(phy);
 	if (IS_ERR(phy)) {
diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index 63843037673c..af576d056e45 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -120,10 +120,13 @@ static int of_mdiobus_register_phy(struct mii_bus *mdio,
 	is_c45 = of_device_is_compatible(child,
 					 "ethernet-phy-ieee802.3-c45");
 
-	if (!is_c45 && !of_get_phy_id(child, &phy_id))
-		phy = phy_device_create(mdio, addr, phy_id, 0);
-	else
-		phy = get_phy_device(mdio, addr, is_c45);
+	if (!is_c45) {
+		rc = of_get_phy_id(child, &phy_id);
+		if (rc)
+			phy_id = PHY_ID_NONE;
+	}
+
+	phy = phy_device_create(mdio, addr, phy_id, is_c45);
 	if (IS_ERR(phy)) {
 		if (mii_ts)
 			unregister_mii_timestamper(mii_ts);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 662919c1dd27..01d24a934ad1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1215,16 +1215,9 @@ int phy_modify_paged(struct phy_device *phydev, int page, u32 regnum,
 struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 				     bool is_c45);
 #if IS_ENABLED(CONFIG_PHYLIB)
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45);
 int phy_device_register(struct phy_device *phy);
 void phy_device_free(struct phy_device *phydev);
 #else
-static inline
-struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
-{
-	return NULL;
-}
-
 static inline int phy_device_register(struct phy_device *phy)
 {
 	return 0;
-- 
2.26.1

Re: [PATCH 11/15] net: phy: drop get_phy_device()

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 3451 bytes --]

Hi Bartosz,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.8-rc2]
[cannot apply to arm/for-next next-20200622]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use  as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Bartosz-Golaszewski/net-phy-correctly-model-the-PHY-voltage-supply-in-DT/20200622-174726
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: x86_64-allyesconfig (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 1d4c87335d5236ea1f35937e1014980ba961ae34)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install x86_64 cross compiling tool for clang build
        # apt-get install binutils-x86-64-linux-gnu
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>, old ones prefixed by <<):

>> drivers/of/of_mdio.c:123:6: warning: variable 'phy_id' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]
if (!is_c45) {
^~~~~~~
drivers/of/of_mdio.c:129:38: note: uninitialized use occurs here
phy = phy_device_create(mdio, addr, phy_id, is_c45);
^~~~~~
drivers/of/of_mdio.c:123:2: note: remove the 'if' if its condition is always true
if (!is_c45) {
^~~~~~~~~~~~~
drivers/of/of_mdio.c:114:12: note: initialize the variable 'phy_id' to silence this warning
u32 phy_id;
^
= 0
1 warning generated.

vim +123 drivers/of/of_mdio.c

   106	
   107	static int of_mdiobus_register_phy(struct mii_bus *mdio,
   108					    struct device_node *child, u32 addr)
   109	{
   110		struct mii_timestamper *mii_ts;
   111		struct phy_device *phy;
   112		bool is_c45;
   113		int rc;
   114		u32 phy_id;
   115	
   116		mii_ts = of_find_mii_timestamper(child);
   117		if (IS_ERR(mii_ts))
   118			return PTR_ERR(mii_ts);
   119	
   120		is_c45 = of_device_is_compatible(child,
   121						 "ethernet-phy-ieee802.3-c45");
   122	
 > 123		if (!is_c45) {
   124			rc = of_get_phy_id(child, &phy_id);
   125			if (rc)
   126				phy_id = PHY_ID_NONE;
   127		}
   128	
   129		phy = phy_device_create(mdio, addr, phy_id, is_c45);
   130		if (IS_ERR(phy)) {
   131			if (mii_ts)
   132				unregister_mii_timestamper(mii_ts);
   133			return PTR_ERR(phy);
   134		}
   135	
   136		rc = of_mdiobus_phy_device_register(mdio, phy, child, addr);
   137		if (rc) {
   138			if (mii_ts)
   139				unregister_mii_timestamper(mii_ts);
   140			phy_device_free(phy);
   141			return rc;
   142		}
   143	
   144		/* phy->mii_ts may already be defined by the PHY driver. A
   145		 * mii_timestamper probed via the device tree will still have
   146		 * precedence.
   147		 */
   148		if (mii_ts)
   149			phy->mii_ts = mii_ts;
   150	
   151		return 0;
   152	}
   153	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 75292 bytes --]

[PATCH 12/15] dt-bindings: mdio: add phy-supply property to ethernet phy node

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The phy-supply property is often added to MAC nodes but this is wrong
conceptually. These supplies should be part of the PHY node on the
MDIO bus. Add phy-supply property at PHY level to mdio.yaml.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 Documentation/devicetree/bindings/net/mdio.yaml | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
index d6a3bf8550eb..9c10012c2093 100644
--- a/Documentation/devicetree/bindings/net/mdio.yaml
+++ b/Documentation/devicetree/bindings/net/mdio.yaml
@@ -90,6 +90,10 @@ patternProperties:
           Delay after the reset was deasserted in microseconds. If
           this property is missing the delay will be skipped.
 
+      phy-supply:
+        description:
+          Phandle to the regulator that provides the supply voltage to the PHY.
+
     required:
       - reg
 
-- 
2.26.1

Re: [PATCH 12/15] dt-bindings: mdio: add phy-supply property to ethernet phy node

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The phy-supply property is often added to MAC nodes but this is wrong
> conceptually. These supplies should be part of the PHY node on the
> MDIO bus. Add phy-supply property at PHY level to mdio.yaml.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  Documentation/devicetree/bindings/net/mdio.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
> index d6a3bf8550eb..9c10012c2093 100644
> --- a/Documentation/devicetree/bindings/net/mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/mdio.yaml
> @@ -90,6 +90,10 @@ patternProperties:
>            Delay after the reset was deasserted in microseconds. If
>            this property is missing the delay will be skipped.
>  
> +      phy-supply:
> +        description:
> +          Phandle to the regulator that provides the supply voltage to the PHY.

I do not see how you can come up with a generic name here, there could
be PHYs supporting different voltages (3.3V, 1.8V, 1.5V) depending on
their operation mode/strapping, there can also be different parts of the
PHY being powered by different regulators, the analog part could be on
an always-on island such that Wake-on-LAN from the PHY could be done,
and the digital part could be on a complete different island.
-- 
Florian

Re: [PATCH 12/15] dt-bindings: mdio: add phy-supply property to ethernet phy node

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The phy-supply property is often added to MAC nodes but this is wrong
> conceptually. These supplies should be part of the PHY node on the
> MDIO bus. Add phy-supply property at PHY level to mdio.yaml.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  Documentation/devicetree/bindings/net/mdio.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
> index d6a3bf8550eb..9c10012c2093 100644
> --- a/Documentation/devicetree/bindings/net/mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/mdio.yaml
> @@ -90,6 +90,10 @@ patternProperties:
>            Delay after the reset was deasserted in microseconds. If
>            this property is missing the delay will be skipped.
>  
> +      phy-supply:
> +        description:
> +          Phandle to the regulator that provides the supply voltage to the PHY.

I do not see how you can come up with a generic name here, there could
be PHYs supporting different voltages (3.3V, 1.8V, 1.5V) depending on
their operation mode/strapping, there can also be different parts of the
PHY being powered by different regulators, the analog part could be on
an always-on island such that Wake-on-LAN from the PHY could be done,
and the digital part could be on a complete different island.
-- 
Florian

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 12/15] dt-bindings: mdio: add phy-supply property to ethernet phy node

[Florian Fainelli]

On 6/22/20 2:37 AM, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The phy-supply property is often added to MAC nodes but this is wrong
> conceptually. These supplies should be part of the PHY node on the
> MDIO bus. Add phy-supply property at PHY level to mdio.yaml.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  Documentation/devicetree/bindings/net/mdio.yaml | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/net/mdio.yaml b/Documentation/devicetree/bindings/net/mdio.yaml
> index d6a3bf8550eb..9c10012c2093 100644
> --- a/Documentation/devicetree/bindings/net/mdio.yaml
> +++ b/Documentation/devicetree/bindings/net/mdio.yaml
> @@ -90,6 +90,10 @@ patternProperties:
>            Delay after the reset was deasserted in microseconds. If
>            this property is missing the delay will be skipped.
>  
> +      phy-supply:
> +        description:
> +          Phandle to the regulator that provides the supply voltage to the PHY.

I do not see how you can come up with a generic name here, there could
be PHYs supporting different voltages (3.3V, 1.8V, 1.5V) depending on
their operation mode/strapping, there can also be different parts of the
PHY being powered by different regulators, the analog part could be on
an always-on island such that Wake-on-LAN from the PHY could be done,
and the digital part could be on a complete different island.
-- 
Florian

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 13/15] net: phy: mdio: add support for PHY supply regulator

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Currently many MAC drivers control the regulator supplying the PHY but
this is conceptually wrong. The regulator should be defined as a property
of the PHY node on the MDIO bus and controlled by the MDIO sub-system.

Add support for an optional PHY regulator which will be enabled before
optional deasserting of the reset signal.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/mdio_bus.c    | 21 ++++++++++++++++++++
 drivers/net/phy/mdio_device.c | 36 +++++++++++++++++++++++++++++++++++
 include/linux/mdio.h          |  3 +++
 3 files changed, 60 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 53e2fb0be7b9..19f0b9664fe3 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -27,6 +27,7 @@
 #include <linux/of_gpio.h>
 #include <linux/of_mdio.h>
 #include <linux/phy.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/skbuff.h>
 #include <linux/slab.h>
@@ -67,6 +68,22 @@ static int mdiobus_register_reset(struct mdio_device *mdiodev)
 	return 0;
 }
 
+static int mdiobus_register_regulator(struct mdio_device *mdiodev)
+{
+	struct regulator *phy_supply;
+
+	phy_supply = regulator_get_optional(&mdiodev->dev, "phy");
+	if (IS_ERR(phy_supply)) {
+		if (PTR_ERR(phy_supply) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		phy_supply = NULL;
+	}
+
+	mdiodev->phy_supply = phy_supply;
+
+	return 0;
+}
+
 int mdiobus_register_device(struct mdio_device *mdiodev)
 {
 	int err;
@@ -83,6 +100,10 @@ int mdiobus_register_device(struct mdio_device *mdiodev)
 		if (err)
 			return err;
 
+		err = mdiobus_register_regulator(mdiodev);
+		if (err)
+			return err;
+
 		/* Assert the reset signal */
 		mdio_device_reset(mdiodev, 1);
 	}
diff --git a/drivers/net/phy/mdio_device.c b/drivers/net/phy/mdio_device.c
index be615504b829..0f698d7a770b 100644
--- a/drivers/net/phy/mdio_device.c
+++ b/drivers/net/phy/mdio_device.c
@@ -17,6 +17,7 @@
 #include <linux/mii.h>
 #include <linux/module.h>
 #include <linux/phy.h>
+#include <linux/regulator/consumer.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
 #include <linux/string.h>
@@ -136,6 +137,32 @@ void mdio_device_reset(struct mdio_device *mdiodev, int value)
 }
 EXPORT_SYMBOL(mdio_device_reset);
 
+int mdio_device_power_on(struct mdio_device *mdiodev)
+{
+	int ret = 0;
+
+	if (mdiodev->phy_supply)
+		/* TODO We may need a delay here just like reset_assert_delay
+		 * but since no user currently needs it, I'm not adding it just
+		 * yet.
+		 */
+		ret = regulator_enable(mdiodev->phy_supply);
+
+	return ret;
+}
+EXPORT_SYMBOL(mdio_device_power_on);
+
+int mdio_device_power_off(struct mdio_device *mdiodev)
+{
+	int ret = 0;
+
+	if (mdiodev->phy_supply)
+		ret = regulator_disable(mdiodev->phy_supply);
+
+	return ret;
+}
+EXPORT_SYMBOL(mdio_device_power_off);
+
 /**
  * mdio_probe - probe an MDIO device
  * @dev: device to probe
@@ -150,6 +177,11 @@ static int mdio_probe(struct device *dev)
 	struct mdio_driver *mdiodrv = to_mdio_driver(drv);
 	int err = 0;
 
+	/* Enable the power supply */
+	err = mdio_device_power_on(mdiodev);
+	if (err)
+		return err;
+
 	/* Deassert the reset signal */
 	mdio_device_reset(mdiodev, 0);
 
@@ -158,6 +190,8 @@ static int mdio_probe(struct device *dev)
 		if (err) {
 			/* Assert the reset signal */
 			mdio_device_reset(mdiodev, 1);
+			/* Disable the power supply */
+			mdio_device_power_off(mdiodev);
 		}
 	}
 
@@ -175,6 +209,8 @@ static int mdio_remove(struct device *dev)
 
 	/* Assert the reset signal */
 	mdio_device_reset(mdiodev, 1);
+	/* Disable the power supply */
+	mdio_device_power_off(mdiodev);
 
 	return 0;
 }
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 9ac5e7ff6156..0ae07365a6ca 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -46,6 +46,7 @@ struct mdio_device {
 	int flags;
 	struct gpio_desc *reset_gpio;
 	struct reset_control *reset_ctrl;
+	struct regulator *phy_supply;
 	unsigned int reset_assert_delay;
 	unsigned int reset_deassert_delay;
 };
@@ -92,6 +93,8 @@ struct mdio_device *mdio_device_create(struct mii_bus *bus, int addr);
 int mdio_device_register(struct mdio_device *mdiodev);
 void mdio_device_remove(struct mdio_device *mdiodev);
 void mdio_device_reset(struct mdio_device *mdiodev, int value);
+int mdio_device_power_on(struct mdio_device *mdiodev);
+int mdio_device_power_off(struct mdio_device *mdiodev);
 int mdio_driver_register(struct mdio_driver *drv);
 void mdio_driver_unregister(struct mdio_driver *drv);
 int mdio_device_bus_match(struct device *dev, struct device_driver *drv);
-- 
2.26.1

Re: [PATCH 13/15] net: phy: mdio: add support for PHY supply regulator

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 11:37:42AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> Currently many MAC drivers control the regulator supplying the PHY but
> this is conceptually wrong. The regulator should be defined as a property
> of the PHY node on the MDIO bus and controlled by the MDIO sub-system.
> 
> Add support for an optional PHY regulator which will be enabled before
> optional deasserting of the reset signal.

I wonder if this is the right place for this - MDIO devices do not have
to be PHYs - they can be switches, and using "phy-supply" for a switch
doesn't seem logical.

However, I can see the utility of having having a supply provided for
all mdio devices, so it seems to me to be a naming issue.  Andrew?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 13/15] net: phy: mdio: add support for PHY supply regulator

[Bartosz Golaszewski]

pon., 22 cze 2020 o 15:25 Russell King - ARM Linux admin
<linux@armlinux.org.uk> napisał(a):
>
> On Mon, Jun 22, 2020 at 11:37:42AM +0200, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> >
> > Currently many MAC drivers control the regulator supplying the PHY but
> > this is conceptually wrong. The regulator should be defined as a property
> > of the PHY node on the MDIO bus and controlled by the MDIO sub-system.
> >
> > Add support for an optional PHY regulator which will be enabled before
> > optional deasserting of the reset signal.
>
> I wonder if this is the right place for this - MDIO devices do not have
> to be PHYs - they can be switches, and using "phy-supply" for a switch
> doesn't seem logical.
>
> However, I can see the utility of having having a supply provided for
> all mdio devices, so it seems to me to be a naming issue.  Andrew?
>

I followed the example of the phy reset retrieved in
mdiobus_register_reset(). I'm happy to change it to some other name.

Bart

[PATCH 14/15] net: phy: add PHY regulator support

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

The MDIO sub-system now supports PHY regulators. Let's reuse the code
to extend this support over to the PHY device.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 drivers/net/phy/phy_device.c | 49 ++++++++++++++++++++++++++++--------
 include/linux/phy.h          | 10 ++++++++
 2 files changed, 48 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 58923826838b..d755adb748a5 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -827,7 +827,12 @@ int phy_device_register(struct phy_device *phydev)
 
 	err = mdiobus_register_device(&phydev->mdio);
 	if (err)
-		return err;
+		goto err_out;
+
+	/* Enable the PHY regulator */
+	err = phy_device_power_on(phydev);
+	if (err)
+		goto err_unregister_mdio;
 
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
@@ -846,22 +851,25 @@ int phy_device_register(struct phy_device *phydev)
 	err = phy_scan_fixups(phydev);
 	if (err) {
 		phydev_err(phydev, "failed to initialize\n");
-		goto out;
+		goto err_reset;
 	}
 
 	err = device_add(&phydev->mdio.dev);
 	if (err) {
 		phydev_err(phydev, "failed to add\n");
-		goto out;
+		goto err_reset;
 	}
 
 	return 0;
 
- out:
+err_reset:
 	/* Assert the reset signal */
 	phy_device_reset(phydev, 1);
-
+	/* Disable the PHY regulator */
+	phy_device_power_off(phydev);
+err_unregister_mdio:
 	mdiobus_unregister_device(&phydev->mdio);
+err_out:
 	return err;
 }
 EXPORT_SYMBOL(phy_device_register);
@@ -883,6 +891,8 @@ void phy_device_remove(struct phy_device *phydev)
 
 	/* Assert the reset signal */
 	phy_device_reset(phydev, 1);
+	/* Disable the PHY regulator */
+	phy_device_power_off(phydev);
 
 	mdiobus_unregister_device(&phydev->mdio);
 }
@@ -1064,6 +1074,11 @@ int phy_init_hw(struct phy_device *phydev)
 {
 	int ret = 0;
 
+	/* Enable the PHY regulator */
+	ret = phy_device_power_on(phydev);
+	if (ret)
+		return ret;
+
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
 
@@ -1644,6 +1659,8 @@ void phy_detach(struct phy_device *phydev)
 
 	/* Assert the reset signal */
 	phy_device_reset(phydev, 1);
+	/* Disable the PHY regulator */
+	phy_device_power_off(phydev);
 }
 EXPORT_SYMBOL(phy_detach);
 
@@ -2684,13 +2701,18 @@ static int phy_probe(struct device *dev)
 
 	mutex_lock(&phydev->lock);
 
+	/* Enable the PHY regulator */
+	err = phy_device_power_on(phydev);
+	if (err)
+		goto out;
+
 	/* Deassert the reset signal */
 	phy_device_reset(phydev, 0);
 
 	if (phydev->drv->probe) {
 		err = phydev->drv->probe(phydev);
 		if (err)
-			goto out;
+			goto out_reset;
 	}
 
 	/* Start out supporting everything. Eventually,
@@ -2708,7 +2730,7 @@ static int phy_probe(struct device *dev)
 	}
 
 	if (err)
-		goto out;
+		goto out_reset;
 
 	if (!linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
 			       phydev->supported))
@@ -2751,11 +2773,16 @@ static int phy_probe(struct device *dev)
 	/* Set the state to READY by default */
 	phydev->state = PHY_READY;
 
-out:
-	/* Assert the reset signal */
-	if (err)
-		phy_device_reset(phydev, 1);
+	mutex_unlock(&phydev->lock);
+
+	return 0;
 
+out_reset:
+	/* Assert the reset signal */
+	phy_device_reset(phydev, 1);
+	/* Disable the PHY regulator */
+	phy_device_power_off(phydev);
+out:
 	mutex_unlock(&phydev->lock);
 
 	return err;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 01d24a934ad1..585ce8db32cf 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1291,6 +1291,16 @@ static inline void phy_device_reset(struct phy_device *phydev, int value)
 	mdio_device_reset(&phydev->mdio, value);
 }
 
+static inline int phy_device_power_on(struct phy_device *phydev)
+{
+	return mdio_device_power_on(&phydev->mdio);
+}
+
+static inline int phy_device_power_off(struct phy_device *phydev)
+{
+	return mdio_device_power_off(&phydev->mdio);
+}
+
 #define phydev_err(_phydev, format, args...)	\
 	dev_err(&_phydev->mdio.dev, format, ##args)
 
-- 
2.26.1

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Russell King - ARM Linux admin]

On Mon, Jun 22, 2020 at 11:37:43AM +0200, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> 
> The MDIO sub-system now supports PHY regulators. Let's reuse the code
> to extend this support over to the PHY device.
> 
> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> ---
>  drivers/net/phy/phy_device.c | 49 ++++++++++++++++++++++++++++--------
>  include/linux/phy.h          | 10 ++++++++
>  2 files changed, 48 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 58923826838b..d755adb748a5 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -827,7 +827,12 @@ int phy_device_register(struct phy_device *phydev)
>  
>  	err = mdiobus_register_device(&phydev->mdio);
>  	if (err)
> -		return err;
> +		goto err_out;
> +
> +	/* Enable the PHY regulator */
> +	err = phy_device_power_on(phydev);
> +	if (err)
> +		goto err_unregister_mdio;
>  
>  	/* Deassert the reset signal */
>  	phy_device_reset(phydev, 0);
> @@ -846,22 +851,25 @@ int phy_device_register(struct phy_device *phydev)
>  	err = phy_scan_fixups(phydev);
>  	if (err) {
>  		phydev_err(phydev, "failed to initialize\n");
> -		goto out;
> +		goto err_reset;
>  	}
>  
>  	err = device_add(&phydev->mdio.dev);
>  	if (err) {
>  		phydev_err(phydev, "failed to add\n");
> -		goto out;
> +		goto err_reset;
>  	}
>  
>  	return 0;
>  
> - out:
> +err_reset:
>  	/* Assert the reset signal */
>  	phy_device_reset(phydev, 1);
> -
> +	/* Disable the PHY regulator */
> +	phy_device_power_off(phydev);
> +err_unregister_mdio:
>  	mdiobus_unregister_device(&phydev->mdio);
> +err_out:
>  	return err;
>  }
>  EXPORT_SYMBOL(phy_device_register);
> @@ -883,6 +891,8 @@ void phy_device_remove(struct phy_device *phydev)
>  
>  	/* Assert the reset signal */
>  	phy_device_reset(phydev, 1);
> +	/* Disable the PHY regulator */
> +	phy_device_power_off(phydev);
>  
>  	mdiobus_unregister_device(&phydev->mdio);
>  }
> @@ -1064,6 +1074,11 @@ int phy_init_hw(struct phy_device *phydev)
>  {
>  	int ret = 0;
>  
> +	/* Enable the PHY regulator */
> +	ret = phy_device_power_on(phydev);
> +	if (ret)
> +		return ret;
> +
>  	/* Deassert the reset signal */
>  	phy_device_reset(phydev, 0);
>  
> @@ -1644,6 +1659,8 @@ void phy_detach(struct phy_device *phydev)
>  
>  	/* Assert the reset signal */
>  	phy_device_reset(phydev, 1);
> +	/* Disable the PHY regulator */
> +	phy_device_power_off(phydev);

This is likely to cause issues for some PHY drivers.  Note that we have
some PHY drivers which register a temperature sensor in the probe
function, which means they can be accessed independently of the lifetime
of the PHY bound to the network driver (which may only be while the
network device is "up".)  We certainly do not want hwmon failing just
because the network device is down.

That's kind of worked around for the reset stuff, because there are two
layers to that: the mdio device layer reset support which knows nothing
of the PHY binding state to the network driver, and the phylib reset
support, but it is not nice.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Bartosz Golaszewski]

pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
<linux@armlinux.org.uk> napisał(a):
>

[snip!]

>
> This is likely to cause issues for some PHY drivers.  Note that we have
> some PHY drivers which register a temperature sensor in the probe
> function, which means they can be accessed independently of the lifetime
> of the PHY bound to the network driver (which may only be while the
> network device is "up".)  We certainly do not want hwmon failing just
> because the network device is down.
>
> That's kind of worked around for the reset stuff, because there are two
> layers to that: the mdio device layer reset support which knows nothing
> of the PHY binding state to the network driver, and the phylib reset
> support, but it is not nice.
>

Regulators are reference counted so if the hwmon driver enables it
using mdio_device_power_on() it will stay on even after the PHY driver
calls phy_device_power_off(), right? Am I missing something?

Bart

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Russell King - ARM Linux admin]

On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> <linux@armlinux.org.uk> napisał(a):
> >
> 
> [snip!]
> 
> >
> > This is likely to cause issues for some PHY drivers.  Note that we have
> > some PHY drivers which register a temperature sensor in the probe
> > function, which means they can be accessed independently of the lifetime
> > of the PHY bound to the network driver (which may only be while the
> > network device is "up".)  We certainly do not want hwmon failing just
> > because the network device is down.
> >
> > That's kind of worked around for the reset stuff, because there are two
> > layers to that: the mdio device layer reset support which knows nothing
> > of the PHY binding state to the network driver, and the phylib reset
> > support, but it is not nice.
> >
> 
> Regulators are reference counted so if the hwmon driver enables it
> using mdio_device_power_on() it will stay on even after the PHY driver
> calls phy_device_power_off(), right? Am I missing something?

If that is true, you will need to audit the PHY drivers to add that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Bartosz Golaszewski]

wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
<linux@armlinux.org.uk> napisał(a):
>
> On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> napisał(a):
> > >
> >
> > [snip!]
> >
> > >
> > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > some PHY drivers which register a temperature sensor in the probe
> > > function, which means they can be accessed independently of the lifetime
> > > of the PHY bound to the network driver (which may only be while the
> > > network device is "up".)  We certainly do not want hwmon failing just
> > > because the network device is down.
> > >
> > > That's kind of worked around for the reset stuff, because there are two
> > > layers to that: the mdio device layer reset support which knows nothing
> > > of the PHY binding state to the network driver, and the phylib reset
> > > support, but it is not nice.
> > >
> >
> > Regulators are reference counted so if the hwmon driver enables it
> > using mdio_device_power_on() it will stay on even after the PHY driver
> > calls phy_device_power_off(), right? Am I missing something?
>
> If that is true, you will need to audit the PHY drivers to add that.
>

This change doesn't have any effect on devices which don't have a
regulator assigned in DT though. The one I'm adding in the last patch
is the first to use this.

Bart

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Russell King - ARM Linux admin]

On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> <linux@armlinux.org.uk> napisał(a):
> >
> > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> napisał(a):
> > > >
> > >
> > > [snip!]
> > >
> > > >
> > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > some PHY drivers which register a temperature sensor in the probe
> > > > function, which means they can be accessed independently of the lifetime
> > > > of the PHY bound to the network driver (which may only be while the
> > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > because the network device is down.
> > > >
> > > > That's kind of worked around for the reset stuff, because there are two
> > > > layers to that: the mdio device layer reset support which knows nothing
> > > > of the PHY binding state to the network driver, and the phylib reset
> > > > support, but it is not nice.
> > > >
> > >
> > > Regulators are reference counted so if the hwmon driver enables it
> > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > calls phy_device_power_off(), right? Am I missing something?
> >
> > If that is true, you will need to audit the PHY drivers to add that.
> >
> 
> This change doesn't have any effect on devices which don't have a
> regulator assigned in DT though. The one I'm adding in the last patch
> is the first to use this.

It's quality of implementation.

Should we wait for someone else to make use of the new regulator
support that has been added with a PHY that uses hwmon, and they
don't realise that it breaks hwmon on it, and several kernel versions
go by without it being noticed.  It will only be a noticable issue
when the associated network device is down, and that network device
driver detaches from the PHY, so _is_ likely not to be noticed.

Or should we do a small amount of work now to properly implement
regulator support, which includes a trivial grep for "hwmon" amongst
the PHY drivers, and add the necessary call to avoid the regulator
being shut off.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Bartosz Golaszewski]

wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
<linux@armlinux.org.uk> napisał(a):
>
> On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> napisał(a):
> > >
> > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> napisał(a):
> > > > >
> > > >
> > > > [snip!]
> > > >
> > > > >
> > > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > > some PHY drivers which register a temperature sensor in the probe
> > > > > function, which means they can be accessed independently of the lifetime
> > > > > of the PHY bound to the network driver (which may only be while the
> > > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > > because the network device is down.
> > > > >
> > > > > That's kind of worked around for the reset stuff, because there are two
> > > > > layers to that: the mdio device layer reset support which knows nothing
> > > > > of the PHY binding state to the network driver, and the phylib reset
> > > > > support, but it is not nice.
> > > > >
> > > >
> > > > Regulators are reference counted so if the hwmon driver enables it
> > > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > > calls phy_device_power_off(), right? Am I missing something?
> > >
> > > If that is true, you will need to audit the PHY drivers to add that.
> > >
> >
> > This change doesn't have any effect on devices which don't have a
> > regulator assigned in DT though. The one I'm adding in the last patch
> > is the first to use this.
>
> It's quality of implementation.
>
> Should we wait for someone else to make use of the new regulator
> support that has been added with a PHY that uses hwmon, and they
> don't realise that it breaks hwmon on it, and several kernel versions
> go by without it being noticed.  It will only be a noticable issue
> when the associated network device is down, and that network device
> driver detaches from the PHY, so _is_ likely not to be noticed.
>
> Or should we do a small amount of work now to properly implement
> regulator support, which includes a trivial grep for "hwmon" amongst
> the PHY drivers, and add the necessary call to avoid the regulator
> being shut off.
>

I'm not sure what the correct approach is here. Provide some helper
that, when called, would increase the regulator's reference count even
more to keep it enabled from the moment hwmon is registered to when
the driver is detached?

Bart

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Bartosz Golaszewski]

wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
<linux@armlinux.org.uk> napisał(a):
>
> On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> napisał(a):
> > >
> > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> napisał(a):
> > > > >
> > > >
> > > > [snip!]
> > > >
> > > > >
> > > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > > some PHY drivers which register a temperature sensor in the probe
> > > > > function, which means they can be accessed independently of the lifetime
> > > > > of the PHY bound to the network driver (which may only be while the
> > > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > > because the network device is down.
> > > > >
> > > > > That's kind of worked around for the reset stuff, because there are two
> > > > > layers to that: the mdio device layer reset support which knows nothing
> > > > > of the PHY binding state to the network driver, and the phylib reset
> > > > > support, but it is not nice.
> > > > >
> > > >
> > > > Regulators are reference counted so if the hwmon driver enables it
> > > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > > calls phy_device_power_off(), right? Am I missing something?
> > >
> > > If that is true, you will need to audit the PHY drivers to add that.
> > >
> >
> > This change doesn't have any effect on devices which don't have a
> > regulator assigned in DT though. The one I'm adding in the last patch
> > is the first to use this.
>
> It's quality of implementation.
>
> Should we wait for someone else to make use of the new regulator
> support that has been added with a PHY that uses hwmon, and they
> don't realise that it breaks hwmon on it, and several kernel versions
> go by without it being noticed.  It will only be a noticable issue
> when the associated network device is down, and that network device
> driver detaches from the PHY, so _is_ likely not to be noticed.
>
> Or should we do a small amount of work now to properly implement
> regulator support, which includes a trivial grep for "hwmon" amongst
> the PHY drivers, and add the necessary call to avoid the regulator
> being shut off.
>

I'm not sure what the correct approach is here. Provide some helper
that, when called, would increase the regulator's reference count even
more to keep it enabled from the moment hwmon is registered to when
the driver is detached?

Bart

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Bartosz Golaszewski]

wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
<linux@armlinux.org.uk> napisał(a):
>
> On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> napisał(a):
> > >
> > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> napisał(a):
> > > > >
> > > >
> > > > [snip!]
> > > >
> > > > >
> > > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > > some PHY drivers which register a temperature sensor in the probe
> > > > > function, which means they can be accessed independently of the lifetime
> > > > > of the PHY bound to the network driver (which may only be while the
> > > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > > because the network device is down.
> > > > >
> > > > > That's kind of worked around for the reset stuff, because there are two
> > > > > layers to that: the mdio device layer reset support which knows nothing
> > > > > of the PHY binding state to the network driver, and the phylib reset
> > > > > support, but it is not nice.
> > > > >
> > > >
> > > > Regulators are reference counted so if the hwmon driver enables it
> > > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > > calls phy_device_power_off(), right? Am I missing something?
> > >
> > > If that is true, you will need to audit the PHY drivers to add that.
> > >
> >
> > This change doesn't have any effect on devices which don't have a
> > regulator assigned in DT though. The one I'm adding in the last patch
> > is the first to use this.
>
> It's quality of implementation.
>
> Should we wait for someone else to make use of the new regulator
> support that has been added with a PHY that uses hwmon, and they
> don't realise that it breaks hwmon on it, and several kernel versions
> go by without it being noticed.  It will only be a noticable issue
> when the associated network device is down, and that network device
> driver detaches from the PHY, so _is_ likely not to be noticed.
>
> Or should we do a small amount of work now to properly implement
> regulator support, which includes a trivial grep for "hwmon" amongst
> the PHY drivers, and add the necessary call to avoid the regulator
> being shut off.
>

I'm not sure what the correct approach is here. Provide some helper
that, when called, would increase the regulator's reference count even
more to keep it enabled from the moment hwmon is registered to when
the driver is detached?

Bart

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Russell King - ARM Linux admin]

On Tue, Jun 23, 2020 at 06:27:06PM +0200, Bartosz Golaszewski wrote:
> wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
> <linux@armlinux.org.uk> napisał(a):
> >
> > On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> > > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> napisał(a):
> > > >
> > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > > > <linux@armlinux.org.uk> napisał(a):
> > > > > >
> > > > >
> > > > > [snip!]
> > > > >
> > > > > >
> > > > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > > > some PHY drivers which register a temperature sensor in the probe
> > > > > > function, which means they can be accessed independently of the lifetime
> > > > > > of the PHY bound to the network driver (which may only be while the
> > > > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > > > because the network device is down.
> > > > > >
> > > > > > That's kind of worked around for the reset stuff, because there are two
> > > > > > layers to that: the mdio device layer reset support which knows nothing
> > > > > > of the PHY binding state to the network driver, and the phylib reset
> > > > > > support, but it is not nice.
> > > > > >
> > > > >
> > > > > Regulators are reference counted so if the hwmon driver enables it
> > > > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > > > calls phy_device_power_off(), right? Am I missing something?
> > > >
> > > > If that is true, you will need to audit the PHY drivers to add that.
> > > >
> > >
> > > This change doesn't have any effect on devices which don't have a
> > > regulator assigned in DT though. The one I'm adding in the last patch
> > > is the first to use this.
> >
> > It's quality of implementation.
> >
> > Should we wait for someone else to make use of the new regulator
> > support that has been added with a PHY that uses hwmon, and they
> > don't realise that it breaks hwmon on it, and several kernel versions
> > go by without it being noticed.  It will only be a noticable issue
> > when the associated network device is down, and that network device
> > driver detaches from the PHY, so _is_ likely not to be noticed.
> >
> > Or should we do a small amount of work now to properly implement
> > regulator support, which includes a trivial grep for "hwmon" amongst
> > the PHY drivers, and add the necessary call to avoid the regulator
> > being shut off.
> >
> 
> I'm not sure what the correct approach is here. Provide some helper
> that, when called, would increase the regulator's reference count even
> more to keep it enabled from the moment hwmon is registered to when
> the driver is detached?

I think a PHY driver needs the utility to control this.  We need to be
careful here with naming, because phylib is not the only code in the
kernel that uses the phy_ prefix.

If we had runtime PM support for PHYs, with regulator support hooked
into runtime PM, then we already have standard interfaces that drivers
can use to control whether the device gets powered down.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Russell King - ARM Linux admin]

On Tue, Jun 23, 2020 at 06:27:06PM +0200, Bartosz Golaszewski wrote:
> wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
> <linux@armlinux.org.uk> napisał(a):
> >
> > On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> > > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> napisał(a):
> > > >
> > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > > > <linux@armlinux.org.uk> napisał(a):
> > > > > >
> > > > >
> > > > > [snip!]
> > > > >
> > > > > >
> > > > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > > > some PHY drivers which register a temperature sensor in the probe
> > > > > > function, which means they can be accessed independently of the lifetime
> > > > > > of the PHY bound to the network driver (which may only be while the
> > > > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > > > because the network device is down.
> > > > > >
> > > > > > That's kind of worked around for the reset stuff, because there are two
> > > > > > layers to that: the mdio device layer reset support which knows nothing
> > > > > > of the PHY binding state to the network driver, and the phylib reset
> > > > > > support, but it is not nice.
> > > > > >
> > > > >
> > > > > Regulators are reference counted so if the hwmon driver enables it
> > > > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > > > calls phy_device_power_off(), right? Am I missing something?
> > > >
> > > > If that is true, you will need to audit the PHY drivers to add that.
> > > >
> > >
> > > This change doesn't have any effect on devices which don't have a
> > > regulator assigned in DT though. The one I'm adding in the last patch
> > > is the first to use this.
> >
> > It's quality of implementation.
> >
> > Should we wait for someone else to make use of the new regulator
> > support that has been added with a PHY that uses hwmon, and they
> > don't realise that it breaks hwmon on it, and several kernel versions
> > go by without it being noticed.  It will only be a noticable issue
> > when the associated network device is down, and that network device
> > driver detaches from the PHY, so _is_ likely not to be noticed.
> >
> > Or should we do a small amount of work now to properly implement
> > regulator support, which includes a trivial grep for "hwmon" amongst
> > the PHY drivers, and add the necessary call to avoid the regulator
> > being shut off.
> >
> 
> I'm not sure what the correct approach is here. Provide some helper
> that, when called, would increase the regulator's reference count even
> more to keep it enabled from the moment hwmon is registered to when
> the driver is detached?

I think a PHY driver needs the utility to control this.  We need to be
careful here with naming, because phylib is not the only code in the
kernel that uses the phy_ prefix.

If we had runtime PM support for PHYs, with regulator support hooked
into runtime PM, then we already have standard interfaces that drivers
can use to control whether the device gets powered down.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Russell King - ARM Linux admin]

On Tue, Jun 23, 2020 at 06:27:06PM +0200, Bartosz Golaszewski wrote:
> wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
> <linux@armlinux.org.uk> napisał(a):
> >
> > On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> > > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> > > <linux@armlinux.org.uk> napisał(a):
> > > >
> > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > > > <linux@armlinux.org.uk> napisał(a):
> > > > > >
> > > > >
> > > > > [snip!]
> > > > >
> > > > > >
> > > > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > > > some PHY drivers which register a temperature sensor in the probe
> > > > > > function, which means they can be accessed independently of the lifetime
> > > > > > of the PHY bound to the network driver (which may only be while the
> > > > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > > > because the network device is down.
> > > > > >
> > > > > > That's kind of worked around for the reset stuff, because there are two
> > > > > > layers to that: the mdio device layer reset support which knows nothing
> > > > > > of the PHY binding state to the network driver, and the phylib reset
> > > > > > support, but it is not nice.
> > > > > >
> > > > >
> > > > > Regulators are reference counted so if the hwmon driver enables it
> > > > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > > > calls phy_device_power_off(), right? Am I missing something?
> > > >
> > > > If that is true, you will need to audit the PHY drivers to add that.
> > > >
> > >
> > > This change doesn't have any effect on devices which don't have a
> > > regulator assigned in DT though. The one I'm adding in the last patch
> > > is the first to use this.
> >
> > It's quality of implementation.
> >
> > Should we wait for someone else to make use of the new regulator
> > support that has been added with a PHY that uses hwmon, and they
> > don't realise that it breaks hwmon on it, and several kernel versions
> > go by without it being noticed.  It will only be a noticable issue
> > when the associated network device is down, and that network device
> > driver detaches from the PHY, so _is_ likely not to be noticed.
> >
> > Or should we do a small amount of work now to properly implement
> > regulator support, which includes a trivial grep for "hwmon" amongst
> > the PHY drivers, and add the necessary call to avoid the regulator
> > being shut off.
> >
> 
> I'm not sure what the correct approach is here. Provide some helper
> that, when called, would increase the regulator's reference count even
> more to keep it enabled from the moment hwmon is registered to when
> the driver is detached?

I think a PHY driver needs the utility to control this.  We need to be
careful here with naming, because phylib is not the only code in the
kernel that uses the phy_ prefix.

If we had runtime PM support for PHYs, with regulator support hooked
into runtime PM, then we already have standard interfaces that drivers
can use to control whether the device gets powered down.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Russell King - ARM Linux admin]

On Wed, Jun 24, 2020 at 05:57:19PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Jun 23, 2020 at 06:27:06PM +0200, Bartosz Golaszewski wrote:
> > wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> napisał(a):
> > >
> > > On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> > > > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> napisał(a):
> > > > >
> > > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > > > > <linux@armlinux.org.uk> napisał(a):
> > > > > > >
> > > > > >
> > > > > > [snip!]
> > > > > >
> > > > > > >
> > > > > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > > > > some PHY drivers which register a temperature sensor in the probe
> > > > > > > function, which means they can be accessed independently of the lifetime
> > > > > > > of the PHY bound to the network driver (which may only be while the
> > > > > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > > > > because the network device is down.
> > > > > > >
> > > > > > > That's kind of worked around for the reset stuff, because there are two
> > > > > > > layers to that: the mdio device layer reset support which knows nothing
> > > > > > > of the PHY binding state to the network driver, and the phylib reset
> > > > > > > support, but it is not nice.
> > > > > > >
> > > > > >
> > > > > > Regulators are reference counted so if the hwmon driver enables it
> > > > > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > > > > calls phy_device_power_off(), right? Am I missing something?
> > > > >
> > > > > If that is true, you will need to audit the PHY drivers to add that.
> > > > >
> > > >
> > > > This change doesn't have any effect on devices which don't have a
> > > > regulator assigned in DT though. The one I'm adding in the last patch
> > > > is the first to use this.
> > >
> > > It's quality of implementation.
> > >
> > > Should we wait for someone else to make use of the new regulator
> > > support that has been added with a PHY that uses hwmon, and they
> > > don't realise that it breaks hwmon on it, and several kernel versions
> > > go by without it being noticed.  It will only be a noticable issue
> > > when the associated network device is down, and that network device
> > > driver detaches from the PHY, so _is_ likely not to be noticed.
> > >
> > > Or should we do a small amount of work now to properly implement
> > > regulator support, which includes a trivial grep for "hwmon" amongst
> > > the PHY drivers, and add the necessary call to avoid the regulator
> > > being shut off.
> > >
> > 
> > I'm not sure what the correct approach is here. Provide some helper
> > that, when called, would increase the regulator's reference count even
> > more to keep it enabled from the moment hwmon is registered to when
> > the driver is detached?
> 
> I think a PHY driver needs the utility to control this.  We need to be
> careful here with naming, because phylib is not the only code in the
> kernel that uses the phy_ prefix.
> 
> If we had runtime PM support for PHYs, with regulator support hooked
> into runtime PM, then we already have standard interfaces that drivers
> can use to control whether the device gets powered down.

Other ideas:

- using genpd outside of the SoC to provide power domain management.
  This is already hooked into runtime PM, but would need their
  agreement, a genpd provider written, and runtime PM added to phylib.

- if we're going for some core driver model approach, then the driver
  model only knows when devices are bound and unbound to their driver,
  it knows nothing of phylib's attach/detach from the network
  interface.  If we want to shut off power when a PHY is not attached,
  we would likely need some kind of interface to do that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Russell King - ARM Linux admin]

On Wed, Jun 24, 2020 at 05:57:19PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Jun 23, 2020 at 06:27:06PM +0200, Bartosz Golaszewski wrote:
> > wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> napisał(a):
> > >
> > > On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> > > > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> napisał(a):
> > > > >
> > > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > > > > <linux@armlinux.org.uk> napisał(a):
> > > > > > >
> > > > > >
> > > > > > [snip!]
> > > > > >
> > > > > > >
> > > > > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > > > > some PHY drivers which register a temperature sensor in the probe
> > > > > > > function, which means they can be accessed independently of the lifetime
> > > > > > > of the PHY bound to the network driver (which may only be while the
> > > > > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > > > > because the network device is down.
> > > > > > >
> > > > > > > That's kind of worked around for the reset stuff, because there are two
> > > > > > > layers to that: the mdio device layer reset support which knows nothing
> > > > > > > of the PHY binding state to the network driver, and the phylib reset
> > > > > > > support, but it is not nice.
> > > > > > >
> > > > > >
> > > > > > Regulators are reference counted so if the hwmon driver enables it
> > > > > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > > > > calls phy_device_power_off(), right? Am I missing something?
> > > > >
> > > > > If that is true, you will need to audit the PHY drivers to add that.
> > > > >
> > > >
> > > > This change doesn't have any effect on devices which don't have a
> > > > regulator assigned in DT though. The one I'm adding in the last patch
> > > > is the first to use this.
> > >
> > > It's quality of implementation.
> > >
> > > Should we wait for someone else to make use of the new regulator
> > > support that has been added with a PHY that uses hwmon, and they
> > > don't realise that it breaks hwmon on it, and several kernel versions
> > > go by without it being noticed.  It will only be a noticable issue
> > > when the associated network device is down, and that network device
> > > driver detaches from the PHY, so _is_ likely not to be noticed.
> > >
> > > Or should we do a small amount of work now to properly implement
> > > regulator support, which includes a trivial grep for "hwmon" amongst
> > > the PHY drivers, and add the necessary call to avoid the regulator
> > > being shut off.
> > >
> > 
> > I'm not sure what the correct approach is here. Provide some helper
> > that, when called, would increase the regulator's reference count even
> > more to keep it enabled from the moment hwmon is registered to when
> > the driver is detached?
> 
> I think a PHY driver needs the utility to control this.  We need to be
> careful here with naming, because phylib is not the only code in the
> kernel that uses the phy_ prefix.
> 
> If we had runtime PM support for PHYs, with regulator support hooked
> into runtime PM, then we already have standard interfaces that drivers
> can use to control whether the device gets powered down.

Other ideas:

- using genpd outside of the SoC to provide power domain management.
  This is already hooked into runtime PM, but would need their
  agreement, a genpd provider written, and runtime PM added to phylib.

- if we're going for some core driver model approach, then the driver
  model only knows when devices are bound and unbound to their driver,
  it knows nothing of phylib's attach/detach from the network
  interface.  If we want to shut off power when a PHY is not attached,
  we would likely need some kind of interface to do that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 14/15] net: phy: add PHY regulator support

[Russell King - ARM Linux admin]

On Wed, Jun 24, 2020 at 05:57:19PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Jun 23, 2020 at 06:27:06PM +0200, Bartosz Golaszewski wrote:
> > wt., 23 cze 2020 o 11:56 Russell King - ARM Linux admin
> > <linux@armlinux.org.uk> napisał(a):
> > >
> > > On Tue, Jun 23, 2020 at 11:46:15AM +0200, Bartosz Golaszewski wrote:
> > > > wt., 23 cze 2020 o 11:43 Russell King - ARM Linux admin
> > > > <linux@armlinux.org.uk> napisał(a):
> > > > >
> > > > > On Tue, Jun 23, 2020 at 11:41:11AM +0200, Bartosz Golaszewski wrote:
> > > > > > pon., 22 cze 2020 o 15:29 Russell King - ARM Linux admin
> > > > > > <linux@armlinux.org.uk> napisał(a):
> > > > > > >
> > > > > >
> > > > > > [snip!]
> > > > > >
> > > > > > >
> > > > > > > This is likely to cause issues for some PHY drivers.  Note that we have
> > > > > > > some PHY drivers which register a temperature sensor in the probe
> > > > > > > function, which means they can be accessed independently of the lifetime
> > > > > > > of the PHY bound to the network driver (which may only be while the
> > > > > > > network device is "up".)  We certainly do not want hwmon failing just
> > > > > > > because the network device is down.
> > > > > > >
> > > > > > > That's kind of worked around for the reset stuff, because there are two
> > > > > > > layers to that: the mdio device layer reset support which knows nothing
> > > > > > > of the PHY binding state to the network driver, and the phylib reset
> > > > > > > support, but it is not nice.
> > > > > > >
> > > > > >
> > > > > > Regulators are reference counted so if the hwmon driver enables it
> > > > > > using mdio_device_power_on() it will stay on even after the PHY driver
> > > > > > calls phy_device_power_off(), right? Am I missing something?
> > > > >
> > > > > If that is true, you will need to audit the PHY drivers to add that.
> > > > >
> > > >
> > > > This change doesn't have any effect on devices which don't have a
> > > > regulator assigned in DT though. The one I'm adding in the last patch
> > > > is the first to use this.
> > >
> > > It's quality of implementation.
> > >
> > > Should we wait for someone else to make use of the new regulator
> > > support that has been added with a PHY that uses hwmon, and they
> > > don't realise that it breaks hwmon on it, and several kernel versions
> > > go by without it being noticed.  It will only be a noticable issue
> > > when the associated network device is down, and that network device
> > > driver detaches from the PHY, so _is_ likely not to be noticed.
> > >
> > > Or should we do a small amount of work now to properly implement
> > > regulator support, which includes a trivial grep for "hwmon" amongst
> > > the PHY drivers, and add the necessary call to avoid the regulator
> > > being shut off.
> > >
> > 
> > I'm not sure what the correct approach is here. Provide some helper
> > that, when called, would increase the regulator's reference count even
> > more to keep it enabled from the moment hwmon is registered to when
> > the driver is detached?
> 
> I think a PHY driver needs the utility to control this.  We need to be
> careful here with naming, because phylib is not the only code in the
> kernel that uses the phy_ prefix.
> 
> If we had runtime PM support for PHYs, with regulator support hooked
> into runtime PM, then we already have standard interfaces that drivers
> can use to control whether the device gets powered down.

Other ideas:

- using genpd outside of the SoC to provide power domain management.
  This is already hooked into runtime PM, but would need their
  agreement, a genpd provider written, and runtime PM added to phylib.

- if we're going for some core driver model approach, then the driver
  model only knows when devices are bound and unbound to their driver,
  it knows nothing of phylib's attach/detach from the network
  interface.  If we want to shut off power when a PHY is not attached,
  we would likely need some kind of interface to do that.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

[PATCH 15/15] ARM64: dts: mediatek: add a phy regulator to pumpkin-common.dtsi

[Bartosz Golaszewski]

From: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Add the appropriate supply to the PHY child-node on the MDIO bus.

Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
---
 arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi b/arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi
index 1a6998570db2..0f5fdc5d390b 100644
--- a/arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi
+++ b/arch/arm64/boot/dts/mediatek/pumpkin-common.dtsi
@@ -206,6 +206,7 @@ mdio {
 
 		eth_phy: ethernet-phy@0 {
 			reg = <0>;
+			phy-supply = <&mt6392_vmch_reg>;
 		};
 	};
 };
-- 
2.26.1