[net-next PATCH v3 0/5] Introduce new fwnode based APIs to support phylink and phy layers

[net-next PATCH v3 0/5] Introduce new fwnode based APIs to support phylink and phy layers

[Calvin Johnson]

Following functions are defined:
  phylink_fwnode_phy_connect()
  fwnode_phy_find_device()
  device_phy_find_device()
  fwnode_get_phy_node()
  fwnode_mdiobus_register_phy()
  fwnode_get_phy_id()

First one helps in connecting phy to phylink instance.
Next two help in finding a phy on a mdiobus.
Next one helps in getting phy_node from a fwnode.
Last two help in getting phy_id and registering phy to mdiobus

Changes in v3:
  move fwnode APIs to appropriate place
  stubs fwnode APIs for !CONFIG_PHYLIB
  improve comment on function return condition.
  remove NULL return check as it is invalid
  remove unused phylink_device_phy_connect()
  Introduce two functions to register phy to mdiobus using fwnode

Changes in v2:
  move phy code from base/property.c to net/phy/phy_device.c
  replace acpi & of code to get phy-handle with fwnode_find_reference
  replace of_ and acpi_ code with generic fwnode to get phy-handle.

Calvin Johnson (5):
  net: phy: Introduce phy related fwnode functions
  net: phy: alphabetically sort header includes
  phylink: Introduce phylink_fwnode_phy_connect()
  net: phy: Introduce fwnode_get_phy_id()
  net: mdiobus: Introduce fwnode_mdiobus_register_phy()

 drivers/net/phy/mdio_bus.c   |  41 ++++++++++++++
 drivers/net/phy/phy_device.c | 102 ++++++++++++++++++++++++++++++-----
 drivers/net/phy/phylink.c    |  48 +++++++++++++++++
 include/linux/mdio.h         |   2 +
 include/linux/phy.h          |  24 +++++++++
 include/linux/phylink.h      |   3 ++
 6 files changed, 206 insertions(+), 14 deletions(-)

-- 
2.17.1

[net-next PATCH v3 1/5] net: phy: Introduce phy related fwnode functions

[Calvin Johnson]

Define fwnode_phy_find_device() to iterate an mdiobus and find the
phy device of the provided phy fwnode. Additionally define
device_phy_find_device() to find phy device of provided device.

Define fwnode_get_phy_node() to get phy_node using named reference.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v3:
  move fwnode APIs to appropriate place
  stubs fwnode APIs for !CONFIG_PHYLIB
  improve comment on function return condition.

Changes in v2:
  move phy code from base/property.c to net/phy/phy_device.c
  replace acpi & of code to get phy-handle with fwnode_find_reference

 drivers/net/phy/phy_device.c | 53 ++++++++++++++++++++++++++++++++++++
 include/linux/phy.h          | 19 +++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 7e1ddd5745d2..3e8224132218 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -31,6 +31,7 @@
 #include <linux/mdio.h>
 #include <linux/io.h>
 #include <linux/uaccess.h>
+#include <linux/property.h>
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
@@ -2436,6 +2437,58 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
 	return phydrv->config_intr && phydrv->ack_interrupt;
 }
 
+/**
+ * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
+ * phy_fwnode.
+ * @phy_fwnode: Pointer to the phy's fwnode.
+ *
+ * If successful, returns a pointer to the phy_device with the embedded
+ * struct device refcount incremented by one, or NULL on failure.
+ */
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	struct device *d;
+	struct mdio_device *mdiodev;
+
+	if (!phy_fwnode)
+		return NULL;
+
+	d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
+	if (d) {
+		mdiodev = to_mdio_device(d);
+		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
+			return to_phy_device(d);
+		put_device(d);
+	}
+
+	return NULL;
+}
+EXPORT_SYMBOL(fwnode_phy_find_device);
+
+/**
+ * device_phy_find_device - For the given device, get the phy_device
+ * @dev: Pointer to the given device
+ *
+ * Refer return conditions of fwnode_phy_find_device().
+ */
+struct phy_device *device_phy_find_device(struct device *dev)
+{
+	return fwnode_phy_find_device(dev_fwnode(dev));
+}
+EXPORT_SYMBOL_GPL(device_phy_find_device);
+
+/**
+ * fwnode_get_phy_node - Get the phy_node using the named reference.
+ * @fwnode: Pointer to fwnode from which phy_node has to be obtained.
+ *
+ * Refer return conditions of fwnode_find_reference().
+ */
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+	return fwnode_find_reference(fwnode, "phy-handle", 0);
+}
+EXPORT_SYMBOL_GPL(fwnode_get_phy_node);
+
 /**
  * phy_probe - probe and init a PHY device
  * @dev: device to probe and init
diff --git a/include/linux/phy.h b/include/linux/phy.h
index e2bfb9240587..f2664730a331 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1141,10 +1141,29 @@ 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);
 #if IS_ENABLED(CONFIG_PHYLIB)
+struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
+struct phy_device *device_phy_find_device(struct device *dev);
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
 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 *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
+{
+	return NULL;
+}
+
+static inline struct phy_device *device_phy_find_device(struct device *dev)
+{
+	return NULL;
+}
+
+struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
+{
+	return NULL;
+}
+
 static inline
 struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
 {
-- 
2.17.1

Re: [net-next PATCH v3 1/5] net: phy: Introduce phy related fwnode functions

[Russell King - ARM Linux admin]

On Tue, May 05, 2020 at 06:59:01PM +0530, Calvin Johnson wrote:
> Define fwnode_phy_find_device() to iterate an mdiobus and find the
> phy device of the provided phy fwnode. Additionally define
> device_phy_find_device() to find phy device of provided device.
> 
> Define fwnode_get_phy_node() to get phy_node using named reference.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
> Changes in v3:
>   move fwnode APIs to appropriate place
>   stubs fwnode APIs for !CONFIG_PHYLIB
>   improve comment on function return condition.
> 
> Changes in v2:
>   move phy code from base/property.c to net/phy/phy_device.c
>   replace acpi & of code to get phy-handle with fwnode_find_reference
> 
>  drivers/net/phy/phy_device.c | 53 ++++++++++++++++++++++++++++++++++++
>  include/linux/phy.h          | 19 +++++++++++++
>  2 files changed, 72 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 7e1ddd5745d2..3e8224132218 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -31,6 +31,7 @@
>  #include <linux/mdio.h>
>  #include <linux/io.h>
>  #include <linux/uaccess.h>
> +#include <linux/property.h>
>  
>  MODULE_DESCRIPTION("PHY library");
>  MODULE_AUTHOR("Andy Fleming");
> @@ -2436,6 +2437,58 @@ static bool phy_drv_supports_irq(struct phy_driver *phydrv)
>  	return phydrv->config_intr && phydrv->ack_interrupt;
>  }
>  
> +/**
> + * fwnode_phy_find_device - Find phy_device on the mdiobus for the provided
> + * phy_fwnode.
> + * @phy_fwnode: Pointer to the phy's fwnode.
> + *
> + * If successful, returns a pointer to the phy_device with the embedded
> + * struct device refcount incremented by one, or NULL on failure.
> + */
> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> +	struct device *d;
> +	struct mdio_device *mdiodev;
> +
> +	if (!phy_fwnode)
> +		return NULL;
> +
> +	d = bus_find_device_by_fwnode(&mdio_bus_type, phy_fwnode);
> +	if (d) {
> +		mdiodev = to_mdio_device(d);
> +		if (mdiodev->flags & MDIO_DEVICE_FLAG_PHY)
> +			return to_phy_device(d);
> +		put_device(d);
> +	}
> +
> +	return NULL;
> +}
> +EXPORT_SYMBOL(fwnode_phy_find_device);

This is basically functionally equivalent to of_phy_find_device().  If
we replaced of_mdio_find_device() with a fwnode equivalent and used that
above, we could have both of_mdio_find_device() and of_phy_find_device()
be wrappers around their fwnode equivalents.

That also means less lines of code to maintain, and means that we're
unlikely to have two implementations that may drift apart functionally
over time because their separated in two different parts of the kernel.
That is an especially important point given that fwnodes can be DT
nodes, so one may call fwnode APIs on a DT platform.

> +
> +/**
> + * device_phy_find_device - For the given device, get the phy_device
> + * @dev: Pointer to the given device
> + *
> + * Refer return conditions of fwnode_phy_find_device().
> + */
> +struct phy_device *device_phy_find_device(struct device *dev)
> +{
> +	return fwnode_phy_find_device(dev_fwnode(dev));
> +}
> +EXPORT_SYMBOL_GPL(device_phy_find_device);
> +
> +/**
> + * fwnode_get_phy_node - Get the phy_node using the named reference.
> + * @fwnode: Pointer to fwnode from which phy_node has to be obtained.
> + *
> + * Refer return conditions of fwnode_find_reference().
> + */
> +struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> +{
> +	return fwnode_find_reference(fwnode, "phy-handle", 0);
> +}
> +EXPORT_SYMBOL_GPL(fwnode_get_phy_node);

What if the fwnode is a DT device handle?  Shouldn't this also check for
the legacy properties as well, so we can transition code over to this
new interface?

> +
>  /**
>   * phy_probe - probe and init a PHY device
>   * @dev: device to probe and init
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index e2bfb9240587..f2664730a331 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -1141,10 +1141,29 @@ 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);
>  #if IS_ENABLED(CONFIG_PHYLIB)
> +struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
> +struct phy_device *device_phy_find_device(struct device *dev);
> +struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
>  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 *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
> +{
> +	return NULL;
> +}
> +
> +static inline struct phy_device *device_phy_find_device(struct device *dev)
> +{
> +	return NULL;
> +}
> +
> +struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> +{
> +	return NULL;
> +}
> +
>  static inline
>  struct phy_device *get_phy_device(struct mii_bus *bus, int addr, bool is_c45)
>  {
> -- 
> 2.17.1
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v3 1/5] net: phy: Introduce phy related fwnode functions

[kbuild test robot]

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

Hi Calvin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on linus/master v5.7-rc4 next-20200505]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Calvin-Johnson/Introduce-new-fwnode-based-APIs-to-support-phylink-and-phy-layers/20200506-051400
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3e1853e4e1137ba0a4d314521d153852dbf4aff5
config: sh-allnoconfig (attached as .config)
compiler: sh4-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=sh 

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

All errors (new ones prefixed by >>):

   sh4-linux-ld: drivers/base/property.o: in function `fwnode_get_phy_node':
>> property.c:(.text+0xf4): multiple definition of `fwnode_get_phy_node'; arch/sh/kernel/cpu/sh2/setup-sh7619.o:setup-sh7619.c:(.text+0x0): first defined here

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

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

Re: [net-next PATCH v3 1/5] net: phy: Introduce phy related fwnode functions

[Russell King - ARM Linux admin]

On Tue, May 05, 2020 at 06:59:01PM +0530, Calvin Johnson wrote:
> +static inline struct phy_device *device_phy_find_device(struct device *dev)
> +{
> +	return NULL;
> +}
> +
> +struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode)
> +{
> +	return NULL;
> +}

This wants to be "static inline" to avoid the issue the 0-day robot
found.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v3 1/5] net: phy: Introduce phy related fwnode functions

[kbuild test robot]

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

Hi Calvin,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on net-next/master]
[also build test ERROR on linus/master v5.7-rc4 next-20200505]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Calvin-Johnson/Introduce-new-fwnode-based-APIs-to-support-phylink-and-phy-layers/20200506-051400
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 3e1853e4e1137ba0a4d314521d153852dbf4aff5
config: m68k-m5475evb_defconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day GCC_VERSION=9.3.0 make.cross ARCH=m68k 

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

All errors (new ones prefixed by >>):

   m68k-linux-ld: drivers/base/property.o: in function `fwnode_get_phy_node':
>> property.c:(.text+0xd4e): multiple definition of `fwnode_get_phy_node'; arch/m68k/coldfire/device.o:device.c:(.text+0x0): first defined here

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

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

[net-next PATCH v3 2/5] net: phy: alphabetically sort header includes

[Calvin Johnson]

Header includes are not sorted. Alphabetically sort them.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v3: None
Changes in v2: None

 drivers/net/phy/phy_device.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 3e8224132218..4204d49586cd 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -9,29 +9,29 @@
 
 #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/property.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/property.h>
+#include <linux/unistd.h>
 
 MODULE_DESCRIPTION("PHY library");
 MODULE_AUTHOR("Andy Fleming");
-- 
2.17.1

[net-next PATCH v3 3/5] phylink: Introduce phylink_fwnode_phy_connect()

[Calvin Johnson]

Define phylink_fwnode_phy_connect() to connect phy specified by
a fwnode to a phylink instance.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v3:
  remove NULL return check as it is invalid
  remove unused phylink_device_phy_connect()

Changes in v2:
  replace of_ and acpi_ code with generic fwnode to get phy-handle.

 drivers/net/phy/phylink.c | 48 +++++++++++++++++++++++++++++++++++++++
 include/linux/phylink.h   |  3 +++
 2 files changed, 51 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 0f23bec431c1..560d1069426c 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -961,6 +961,54 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
 }
 EXPORT_SYMBOL_GPL(phylink_connect_phy);
 
+/**
+ * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode.
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ * @fwnode: a pointer to a &struct fwnode_handle.
+ * @flags: PHY-specific flags to communicate to the PHY device driver
+ *
+ * Connect the phy specified @fwnode to the phylink instance specified
+ * by @pl. Actions specified in phylink_connect_phy() will be
+ * performed.
+ *
+ * Returns 0 on success or a negative errno.
+ */
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags)
+{
+	struct fwnode_handle *phy_fwnode;
+	struct phy_device *phy_dev;
+	int ret = 0;
+
+	/* Fixed links and 802.3z are handled without needing a PHY */
+	if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
+	    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
+	     phy_interface_mode_is_8023z(pl->link_interface)))
+		return 0;
+
+	phy_fwnode = fwnode_get_phy_node(fwnode);
+	if ((IS_ERR(phy_fwnode)) && pl->cfg_link_an_mode == MLO_AN_PHY)
+		return -ENODEV;
+
+	phy_dev = fwnode_phy_find_device(phy_fwnode);
+	fwnode_handle_put(phy_fwnode);
+	if (!phy_dev)
+		return -ENODEV;
+
+	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
+				pl->link_interface);
+	if (ret)
+		return ret;
+
+	ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
+	if (ret)
+		phy_detach(phy_dev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);
+
 /**
  * phylink_of_phy_connect() - connect the PHY specified in the DT mode.
  * @pl: a pointer to a &struct phylink returned from phylink_create()
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index cc5b452a184e..107fb0afc3bb 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -367,6 +367,9 @@ void phylink_add_pcs(struct phylink *, const struct phylink_pcs_ops *ops);
 void phylink_destroy(struct phylink *);
 
 int phylink_connect_phy(struct phylink *, struct phy_device *);
+int phylink_fwnode_phy_connect(struct phylink *pl,
+			       struct fwnode_handle *fwnode,
+			       u32 flags);
 int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
 void phylink_disconnect_phy(struct phylink *);
 
-- 
2.17.1

Re: [net-next PATCH v3 3/5] phylink: Introduce phylink_fwnode_phy_connect()

[Andy Shevchenko]

On Tue, May 5, 2020 at 4:29 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Define phylink_fwnode_phy_connect() to connect phy specified by
> a fwnode to a phylink instance.

...

> +       int ret = 0;

Redundant assignment.

> +       if ((IS_ERR(phy_fwnode)) && pl->cfg_link_an_mode == MLO_AN_PHY)

No Lisp, please.

> +               return -ENODEV;

...

> +       phy_dev = fwnode_phy_find_device(phy_fwnode);
> +       fwnode_handle_put(phy_fwnode);

Hmm... Isn't it racy? I mean if you put fwnode here the phy_dev may
already be gone before you call phy_attach_direct, right?

> +       if (!phy_dev)
> +               return -ENODEV;
> +
> +       ret = phy_attach_direct(pl->netdev, phy_dev, flags,
> +                               pl->link_interface);
> +       if (ret)
> +               return ret;
> +
> +       ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
> +       if (ret)
> +               phy_detach(phy_dev);
> +
> +       return ret;

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 3/5] phylink: Introduce phylink_fwnode_phy_connect()

[Russell King - ARM Linux admin]

On Tue, May 05, 2020 at 06:59:03PM +0530, Calvin Johnson wrote:
> Define phylink_fwnode_phy_connect() to connect phy specified by
> a fwnode to a phylink instance.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
> Changes in v3:
>   remove NULL return check as it is invalid
>   remove unused phylink_device_phy_connect()
> 
> Changes in v2:
>   replace of_ and acpi_ code with generic fwnode to get phy-handle.
> 
>  drivers/net/phy/phylink.c | 48 +++++++++++++++++++++++++++++++++++++++
>  include/linux/phylink.h   |  3 +++
>  2 files changed, 51 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 0f23bec431c1..560d1069426c 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -961,6 +961,54 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
>  }
>  EXPORT_SYMBOL_GPL(phylink_connect_phy);
>  
> +/**
> + * phylink_fwnode_phy_connect() - connect the PHY specified in the fwnode.
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + * @fwnode: a pointer to a &struct fwnode_handle.
> + * @flags: PHY-specific flags to communicate to the PHY device driver
> + *
> + * Connect the phy specified @fwnode to the phylink instance specified
> + * by @pl. Actions specified in phylink_connect_phy() will be
> + * performed.
> + *
> + * Returns 0 on success or a negative errno.
> + */
> +int phylink_fwnode_phy_connect(struct phylink *pl,
> +			       struct fwnode_handle *fwnode,
> +			       u32 flags)
> +{
> +	struct fwnode_handle *phy_fwnode;
> +	struct phy_device *phy_dev;
> +	int ret = 0;
> +
> +	/* Fixed links and 802.3z are handled without needing a PHY */
> +	if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
> +	    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
> +	     phy_interface_mode_is_8023z(pl->link_interface)))
> +		return 0;
> +
> +	phy_fwnode = fwnode_get_phy_node(fwnode);
> +	if ((IS_ERR(phy_fwnode)) && pl->cfg_link_an_mode == MLO_AN_PHY)
> +		return -ENODEV;

This doesn't reflect the behaviour of phylink_of_phy_connect() - it is
*not* a cleanup of what is there, which is:

                if (!phy_node) {
                        if (pl->cfg_link_an_mode == MLO_AN_PHY)
                                return -ENODEV;
                        return 0;
                }

which does:

- if there is a PHY node, find the PHY and connect it.
- if there is no PHY node, then:
   + if we are expecting a PHY to be present, return an error.
   + otherwise, it is not a problem, continue.

That is very important behaviour - it allows drivers to call
phylink_*_phy_connect() without knowing whether there should or should
not be a PHY - and keeps that knowledge within phylink.  It means
network drivers don't have to parse the firmware to find out if there's
a fixed link or SFP cage attached, and decide whether to call these
functions.

> +
> +	phy_dev = fwnode_phy_find_device(phy_fwnode);
> +	fwnode_handle_put(phy_fwnode);
> +	if (!phy_dev)
> +		return -ENODEV;
> +
> +	ret = phy_attach_direct(pl->netdev, phy_dev, flags,
> +				pl->link_interface);
> +	if (ret)
> +		return ret;
> +
> +	ret = phylink_bringup_phy(pl, phy_dev, pl->link_config.interface);
> +	if (ret)
> +		phy_detach(phy_dev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(phylink_fwnode_phy_connect);
> +

I think we need to go further with this, and we need to have
phylink_fwnode_phy_connect() functionally identical to
phylink_of_phy_connect() for DT-based fwnodes.  Doing so will avoid
introducing errors such as the one you've added above.

The only difference between these two is that DT has a number of
legacy properties - these can be omitted if the fwnode is not a DT
node.

Remember that fwnode is compatible with DT, so fwnode_phy_find_device()
can internally decide whether to look for the ACPI property or one of
the three DT properties.

It also means that phylink_of_phy_connect() can become:

int phylink_of_phy_connect(struct phylink *pl, struct device_node *dn,
                           u32 flags)
{
        return phylink_fwnode_phy_connect(pl, of_fwnode_handle(dn), flags);
}

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

[net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

Extract phy_id from compatible string. This will be used by
fwnode_mdiobus_register_phy() to create phy device using the
phy_id.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v3: None
Changes in v2: None

 drivers/net/phy/phy_device.c | 21 +++++++++++++++++++++
 include/linux/phy.h          |  5 +++++
 2 files changed, 26 insertions(+)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 4204d49586cd..f4ad47f73f8d 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -662,6 +662,27 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
 }
 EXPORT_SYMBOL(phy_device_create);
 
+/* Extract the phy ID from the compatible string of the form
+ * ethernet-phy-idAAAA.BBBB.
+ */
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+	const char *cp;
+	unsigned int upper, lower;
+	int ret;
+
+	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
+	if (!ret) {
+		if (sscanf(cp, "ethernet-phy-id%4x.%4x",
+			   &upper, &lower) == 2) {
+			*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
+			return 0;
+		}
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL(fwnode_get_phy_id);
+
 /* get_phy_c45_devs_in_pkg - reads a MMD's devices in package registers.
  * @bus: the target MII bus
  * @addr: PHY address on the MII bus
diff --git a/include/linux/phy.h b/include/linux/phy.h
index f2664730a331..d78ae56509e1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -1141,6 +1141,7 @@ 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);
 #if IS_ENABLED(CONFIG_PHYLIB)
+int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id);
 struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode);
 struct phy_device *device_phy_find_device(struct device *dev);
 struct fwnode_handle *fwnode_get_phy_node(struct fwnode_handle *fwnode);
@@ -1148,6 +1149,10 @@ 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 int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
+{
+	return 0;
+}
 static inline
 struct phy_device *fwnode_phy_find_device(struct fwnode_handle *phy_fwnode)
 {
-- 
2.17.1

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Andy Shevchenko]

On Tue, May 5, 2020 at 4:29 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Extract phy_id from compatible string. This will be used by
> fwnode_mdiobus_register_phy() to create phy device using the
> phy_id.

> +int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
> +{
> +       const char *cp;
> +       unsigned int upper, lower;
> +       int ret;
> +
> +       ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> +       if (!ret) {

if (ret)
 return ret;

will help a lot with readability of this.

> +               if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> +                          &upper, &lower) == 2) {

> +                       *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);

How upper can be bigger than 0xfff? Same for lower.

> +                       return 0;
> +               }
> +       }
> +       return -EINVAL;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Russell King - ARM Linux admin]

On Tue, May 05, 2020 at 05:15:16PM +0300, Andy Shevchenko wrote:
> On Tue, May 5, 2020 at 4:29 PM Calvin Johnson
> > +               if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> > +                          &upper, &lower) == 2) {
> 
> > +                       *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> 
> How upper can be bigger than 0xfff? Same for lower.

I think your comment is incorrect here.  Four hex digits can be larger
than 0xfff.  "1000" interpreted as hex is four hex digits and larger
than 0xfff, for example.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Jeremy Linton]

Hi,

On 5/5/20 8:29 AM, Calvin Johnson wrote:
> Extract phy_id from compatible string. This will be used by
> fwnode_mdiobus_register_phy() to create phy device using the
> phy_id.
> 
> Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
> ---
> 
> Changes in v3: None
> Changes in v2: None
> 
>   drivers/net/phy/phy_device.c | 21 +++++++++++++++++++++
>   include/linux/phy.h          |  5 +++++
>   2 files changed, 26 insertions(+)
> 
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 4204d49586cd..f4ad47f73f8d 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -662,6 +662,27 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, u32 phy_id,
>   }
>   EXPORT_SYMBOL(phy_device_create);
>   
> +/* Extract the phy ID from the compatible string of the form
> + * ethernet-phy-idAAAA.BBBB.
> + */
> +int fwnode_get_phy_id(struct fwnode_handle *fwnode, u32 *phy_id)
> +{
> +	const char *cp;
> +	unsigned int upper, lower;
> +	int ret;
> +
> +	ret = fwnode_property_read_string(fwnode, "compatible", &cp);
> +	if (!ret) {
> +		if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> +			   &upper, &lower) == 2) {
> +			*phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> +			return 0;
> +		}


Isn't the ACPI _CID() conceptually similar to the DT compatible 
property? It even appears to be getting used in a similar way to 
identify particular phy drivers in this case.


Thanks,

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Andy Shevchenko]

On Thu, May 7, 2020 at 4:26 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
> On 5/5/20 8:29 AM, Calvin Johnson wrote:

> > +             if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> > +                        &upper, &lower) == 2) {
> > +                     *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> > +                     return 0;
> > +             }

> Isn't the ACPI _CID() conceptually similar to the DT compatible
> property?

Where?

> It even appears to be getting used in a similar way to
> identify particular phy drivers in this case.

_CID() is a string. It can't be used as pure number.

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Jeremy Linton]

Hi,

On 5/7/20 12:27 PM, Andy Shevchenko wrote:
> On Thu, May 7, 2020 at 4:26 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
>> On 5/5/20 8:29 AM, Calvin Johnson wrote:
> 
>>> +             if (sscanf(cp, "ethernet-phy-id%4x.%4x",
>>> +                        &upper, &lower) == 2) {
>>> +                     *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
>>> +                     return 0;
>>> +             }
> 
>> Isn't the ACPI _CID() conceptually similar to the DT compatible
>> property?
> 
> Where?

Not, sure I understand exactly what your asking. AFAIK, in general the 
dt property is used to select a device driver/etc based on a more to 
less compatible set of substrings. The phy case is a bit different 
because it codes a numerical part number into the string rather than 
just using arbitrary strings to select a driver and device. But it uses 
that as a vendor selector for binding to the correct driver/device.

Rephrasing the ACPI spec, the _CID() is either a single compatible id, 
or a list of ids in order of preference. Each id is either a HID (string 
or EISA type id) or a bus specific string encoding vendor/device/etc. 
(https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/acpi/acpica/utids.c#L186). 
One of the examples is "PCI\VEN_vvvv&DEV_dddd"

So that latter case seems to be almost exactly what we have here.

> 
>> It even appears to be getting used in a similar way to
>> identify particular phy drivers in this case.
> 
> _CID() is a string. It can't be used as pure number.
> 

It does have a numeric version defined for EISA types. OTOH I suspect 
that your right. If there were a "PHY\VEN_IDvvvv&ID_DDDD" definition, it 
may not be ideal to parse it. Instead the normal ACPI model of exactly 
matching the complete string in the phy driver might be more appropriate.

Similarly to how I suspect the next patch's use of "compatible" isn't 
ideal either, because whether a device is c45 or not, should tend to be 
fixed to a particular vendor/device implementation and not a firmware 
provided property.

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

On Thu, May 07, 2020 at 02:54:09PM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/7/20 12:27 PM, Andy Shevchenko wrote:
> > On Thu, May 7, 2020 at 4:26 PM Jeremy Linton <jeremy.linton@arm.com> wrote:
> > > On 5/5/20 8:29 AM, Calvin Johnson wrote:
> > 
> > > > +             if (sscanf(cp, "ethernet-phy-id%4x.%4x",
> > > > +                        &upper, &lower) == 2) {
> > > > +                     *phy_id = ((upper & 0xFFFF) << 16) | (lower & 0xFFFF);
> > > > +                     return 0;
> > > > +             }
> > 
> > > Isn't the ACPI _CID() conceptually similar to the DT compatible
> > > property?
> > 
> > Where?
> 
> Not, sure I understand exactly what your asking. AFAIK, in general the dt
> property is used to select a device driver/etc based on a more to less
> compatible set of substrings. The phy case is a bit different because it
> codes a numerical part number into the string rather than just using
> arbitrary strings to select a driver and device. But it uses that as a
> vendor selector for binding to the correct driver/device.
> 
> Rephrasing the ACPI spec, the _CID() is either a single compatible id, or a
> list of ids in order of preference. Each id is either a HID (string or EISA
> type id) or a bus specific string encoding vendor/device/etc. (https://elixir.bootlin.com/linux/v5.7-rc4/source/drivers/acpi/acpica/utids.c#L186).
> One of the examples is "PCI\VEN_vvvv&DEV_dddd"
> 
> So that latter case seems to be almost exactly what we have here.

Got your point. Yes, the ACPI spec says the same.
If we are using _CID as a string, then it must be a string that uses a
bus-specific nomenclature. This AFAIU may take the format
"PHY\VEN_IDvvvv&ID_DDDD" as you mentioned below and not
"ethernet-phy-id004d.d072" as used in DT.
So, we need to define it some where in the Linux ACPI Documentation.
I don't see any best place to document this. Any suggestions?

> 
> > 
> > > It even appears to be getting used in a similar way to
> > > identify particular phy drivers in this case.
> > 
> > _CID() is a string. It can't be used as pure number.
> > 
> 
> It does have a numeric version defined for EISA types. OTOH I suspect that
> your right. If there were a "PHY\VEN_IDvvvv&ID_DDDD" definition, it may not
> be ideal to parse it. Instead the normal ACPI model of exactly matching the
> complete string in the phy driver might be more appropriate.

IMO, it should be fine to parse the string to extract the phy_id. Is there any
reason why we cannot do this?

> 
> Similarly to how I suspect the next patch's use of "compatible" isn't ideal
> either, because whether a device is c45 or not, should tend to be fixed to a
> particular vendor/device implementation and not a firmware provided
> property.

I tend to agree with you on this. Even for DT, ideal case, IMO should be:

1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from
registers 2 and 3
2) if not found scan for c45 devices <= looks like this is missing in Linux
3) look for phy_id from compatible string.

Meanwhile, please note some usage of compatible property in edk2-platforms:
https://github.com/tianocore/edk2-platforms/blob/master/Platform/96Boards/Secure96Dxe/Secure96.asl#L20
https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl#L280
https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Socionext/SynQuacer/Drivers/PlatformDxe/Optee.asl#L17

Regards
Calvin

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Andrew Lunn]

> > It does have a numeric version defined for EISA types. OTOH I suspect that
> > your right. If there were a "PHY\VEN_IDvvvv&ID_DDDD" definition, it may not
> > be ideal to parse it. Instead the normal ACPI model of exactly matching the
> > complete string in the phy driver might be more appropriate.
> 
> IMO, it should be fine to parse the string to extract the phy_id. Is there any
> reason why we cannot do this?

Some background here, about what the PHY core does.

PHYs have two ID registers. This contains vendor, device, and often
revision of the PHY. Only the vendor part is standardised, vendors can
decide how to use the device part, but it is common for the lowest
nibble to be revision. The core will read these ID registers, and then
go through all the PHY drivers registered and ask them if they support
this ID. The drivers provide a table of IDs and masks. The mask is
applied, and then if the ID matches, the driver is used. The mask
allows the revision to be ignored, etc.

There is a very small number of devices where the vendor messed up,
and did not put valid contents in the ID registers. In such cases, we
can read the IDs from device tree. These are then used in exactly the
same way as if they were read from the device.

If you want the ACPI model to be used, an exact match on the string,
you are going to have to modify the core and the drivers. They
currently don't have any string, and have no idea about different
revisions which are out in the wild.

> > Similarly to how I suspect the next patch's use of "compatible" isn't ideal
> > either, because whether a device is c45 or not, should tend to be fixed to a
> > particular vendor/device implementation and not a firmware provided
> > property.

Not exactly true. It is the combination of can the bus master do C45
and can the device do C45. Unfortunately, we have no knowledge of the
bus masters capabilities, if it can do C45. And many MDIO drivers will
do a C22 transaction when asked to perform a C45 transaction. All new
submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if
C45 is not supported. But we cannot rely on that. Too much history.

> 
> I tend to agree with you on this. Even for DT, ideal case, IMO should be:
> 
> 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from
> registers 2 and 3
> 2) if not found scan for c45 devices <= looks like this is missing in Linux
> 3) look for phy_id from compatible string.

It is somewhat more complex, in that there are a small number of
devices which will respond to both C22 and C45. Generally, you want to
use C45 if supported. So you would want to do the C45 scan first. But
then the earlier problem comes to play, you have no idea if the bus
master actually correctly supports C45.

Given the issues, we assume all bus masters and PHY devices are C22
unless DT says the bus master and PHY combination is compatible with
C45.

	   Andrew

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Jeremy Linton]

Hi,

On 5/8/20 1:13 PM, Andrew Lunn wrote:
>>> It does have a numeric version defined for EISA types. OTOH I suspect that
>>> your right. If there were a "PHY\VEN_IDvvvv&ID_DDDD" definition, it may not
>>> be ideal to parse it. Instead the normal ACPI model of exactly matching the
>>> complete string in the phy driver might be more appropriate.
>>
>> IMO, it should be fine to parse the string to extract the phy_id. Is there any
>> reason why we cannot do this?
> 
> Some background here, about what the PHY core does.
> 
> PHYs have two ID registers. This contains vendor, device, and often
> revision of the PHY. Only the vendor part is standardised, vendors can
> decide how to use the device part, but it is common for the lowest
> nibble to be revision. The core will read these ID registers, and then
> go through all the PHY drivers registered and ask them if they support
> this ID. The drivers provide a table of IDs and masks. The mask is
> applied, and then if the ID matches, the driver is used. The mask
> allows the revision to be ignored, etc.
> 
> There is a very small number of devices where the vendor messed up,
> and did not put valid contents in the ID registers. In such cases, we
> can read the IDs from device tree. These are then used in exactly the
> same way as if they were read from the device.
>

Is that the case here?

Also, how much of this was caused by uboot being deficient, and failing 
to do vendor specific setup? AKA like what has been happening with the 
mac addresses, where it turns out the difference between a chip being 
used on x86 vs arm has frequently been that no one bothered to port all 
the option rom functionality to uboot (and in some cases edk2).

> If you want the ACPI model to be used, an exact match on the string,
> you are going to have to modify the core and the drivers. They
> currently don't have any string, and have no idea about different
> revisions which are out in the wild.

Right, not pretty. But _DSD should never be used to provide functionally 
provided elsewhere in the spec, and with ACPI the attempt is to make the 
firmware less linux focused, and more generic.

> 
>>> Similarly to how I suspect the next patch's use of "compatible" isn't ideal
>>> either, because whether a device is c45 or not, should tend to be fixed to a
>>> particular vendor/device implementation and not a firmware provided
>>> property.
> 
> Not exactly true. It is the combination of can the bus master do C45
> and can the device do C45. Unfortunately, we have no knowledge of the
> bus masters capabilities, if it can do C45. And many MDIO drivers will
> do a C22 transaction when asked to perform a C45 transaction. All new
> submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if
> C45 is not supported. But we cannot rely on that. Too much history >
>>
>> I tend to agree with you on this. Even for DT, ideal case, IMO should be:
>>
>> 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from
>> registers 2 and 3
>> 2) if not found scan for c45 devices <= looks like this is missing in Linux
>> 3) look for phy_id from compatible string.
> 
> It is somewhat more complex, in that there are a small number of
> devices which will respond to both C22 and C45. Generally, you want to
> use C45 if supported. So you would want to do the C45 scan first. But
> then the earlier problem comes to play, you have no idea if the bus
> master actually correctly supports C45.

But this shouldn't this be implied by the mdio vendor/model? AKA, you 
wouldn't have a part with a given _HID() where the capabilities would 
change.You would only have a situation where the phy's capabilities 
change, but they must in the end support whatever is provided by the master.

> 
> Given the issues, we assume all bus masters and PHY devices are C22
> unless DT says the bus master and PHY combination is compatible with
> C45.

How much of this can be simplified for ACPI buy ignoring the legacy and 
putting some guides around the ACPI/platform requirements?


Thanks,

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Andrew Lunn]

> > There is a very small number of devices where the vendor messed up,
> > and did not put valid contents in the ID registers. In such cases, we
> > can read the IDs from device tree. These are then used in exactly the
> > same way as if they were read from the device.
> > 
> 
> Is that the case here?

Sorry, I don't understand the question?

> Also, how much of this was caused by uboot being deficient

None. It is a silicon issue. The PHY chip simply has the wrong or no
ID value in the registers.

> > Not exactly true. It is the combination of can the bus master do C45
> > and can the device do C45. Unfortunately, we have no knowledge of the
> > bus masters capabilities, if it can do C45. And many MDIO drivers will
> > do a C22 transaction when asked to perform a C45 transaction. All new
> > submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if
> > C45 is not supported. But we cannot rely on that. Too much history >
> > > 
> > > I tend to agree with you on this. Even for DT, ideal case, IMO should be:
> > > 
> > > 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from
> > > registers 2 and 3
> > > 2) if not found scan for c45 devices <= looks like this is missing in Linux
> > > 3) look for phy_id from compatible string.
> > 
> > It is somewhat more complex, in that there are a small number of
> > devices which will respond to both C22 and C45. Generally, you want to
> > use C45 if supported. So you would want to do the C45 scan first. But
> > then the earlier problem comes to play, you have no idea if the bus
> > master actually correctly supports C45.
> 
> But this shouldn't this be implied by the mdio vendor/model?

Nope. Many MDIO bus masters don't even appear in DT, because they are
embedded into the MAC driver. The MAC driver just instantiates an MDIO
device, maybe passing a pointer where to find the PHY properties in
DT. If the MDIO bus master is in its own address range, then it
probably does exist in device tree, and has a compatible string. But
that just gets the driver loaded, it says nothing about what it is
capable of, C22 and or C45. And there are cases where the MDIO bus is
embedded inside an Ethernet switch, which is hanging off another MDIO
bus, etc.

> How much of this can be simplified for ACPI buy ignoring the legacy and
> putting some guides around the ACPI/platform requirements?

You can probably ignore the phy-idXXXX.YYYY compatible, since that is
working around silicon issues, and put in place some guidelines that
the PHY silicon needs to conform to the basics of C22 and C45 in terms
of ID registers.

C45 you are going to need. ACPI tends to be more high end devices,
which in general have higher speed network interfaces. Multi-Gige PHYs
tend to be C45. But there is also interest in using ACPI on 1G PHYs
where the majority is C22.

      Andrew

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Jeremy Linton]

Hi,

On 5/8/20 3:27 PM, Andrew Lunn wrote:
>>> There is a very small number of devices where the vendor messed up,
>>> and did not put valid contents in the ID registers. In such cases, we
>>> can read the IDs from device tree. These are then used in exactly the
>>> same way as if they were read from the device.
>>>
>>
>> Is that the case here?
> 
> Sorry, I don't understand the question?

I was asking in general, does this machine report the ID's correctly. 
More directed at Calvin, but part of it is the board vendor too. So I 
suspect no one can really answer "yes", despite that seeming to be the case.



> 
>> Also, how much of this was caused by uboot being deficient
> 
> None. It is a silicon issue. The PHY chip simply has the wrong or no
> ID value in the registers.
> 
>>> Not exactly true. It is the combination of can the bus master do C45
>>> and can the device do C45. Unfortunately, we have no knowledge of the
>>> bus masters capabilities, if it can do C45. And many MDIO drivers will
>>> do a C22 transaction when asked to perform a C45 transaction. All new
>>> submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if
>>> C45 is not supported. But we cannot rely on that. Too much history >
>>>>
>>>> I tend to agree with you on this. Even for DT, ideal case, IMO should be:
>>>>
>>>> 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from
>>>> registers 2 and 3
>>>> 2) if not found scan for c45 devices <= looks like this is missing in Linux
>>>> 3) look for phy_id from compatible string.
>>>
>>> It is somewhat more complex, in that there are a small number of
>>> devices which will respond to both C22 and C45. Generally, you want to
>>> use C45 if supported. So you would want to do the C45 scan first. But
>>> then the earlier problem comes to play, you have no idea if the bus
>>> master actually correctly supports C45.
>>
>> But this shouldn't this be implied by the mdio vendor/model?
> 
> Nope. Many MDIO bus masters don't even appear in DT, because they are
> embedded into the MAC driver. The MAC driver just instantiates an MDIO
> device, maybe passing a pointer where to find the PHY properties in
> DT. If the MDIO bus master is in its own address range, then it
> probably does exist in device tree, and has a compatible string. But
> that just gets the driver loaded, it says nothing about what it is
> capable of, C22 and or C45. And there are cases where the MDIO bus is
> embedded inside an Ethernet switch, which is hanging off another MDIO
> bus, etc.

The embedded single mac:mdio per nic case seems like the normal case, 
and most of the existing ACPI described devices are setup that way. But 
at the same time, that shifts the c22/45 question to the nic driver, 
where use of a DSD property before instantiating/probing MDIO isn't 
really a problem if needed.

In fact this embedded nic/mac/mdio/phy 1:1:1 case, is likely a 
requirement for passthrough into a generic VM, otherwise someone has to 
create a virtual mdio, and pass the phy in for the nic/mac.

AFAIK, NXP's part avoids this despite having a shared MDIO, because the 
phy state never leaves the mgmt side of the picture. It monitors the 
state and then feeds that back into their nic mgmt complex rather than 
using it directly.
> 
>> How much of this can be simplified for ACPI buy ignoring the legacy and
>> putting some guides around the ACPI/platform requirements?
> 
> You can probably ignore the phy-idXXXX.YYYY compatible, since that is
> working around silicon issues, and put in place some guidelines that
> the PHY silicon needs to conform to the basics of C22 and C45 in terms
> of ID registers.
> 
> C45 you are going to need. ACPI tends to be more high end devices,
> which in general have higher speed network interfaces. Multi-Gige PHYs
> tend to be C45. But there is also interest in using ACPI on 1G PHYs
> where the majority is C22.

Oh, I was just trying to see if we can get away with saying things like 
"your phy's must respond as specified by the spec" and leave it at that 
for the time being to simplify the probing sequence. I'm not really sure 
we can represent the more complex switch/etc situations in ACPI either. 
There is a certain amount of "use DT if you machine doesn't conform to 
standards".

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Andrew Lunn]

On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/8/20 3:27 PM, Andrew Lunn wrote:
> > > > There is a very small number of devices where the vendor messed up,
> > > > and did not put valid contents in the ID registers. In such cases, we
> > > > can read the IDs from device tree. These are then used in exactly the
> > > > same way as if they were read from the device.
> > > > 
> > > 
> > > Is that the case here?
> > 
> > Sorry, I don't understand the question?
> 
> I was asking in general, does this machine report the ID's correctly.

Very likely, it does.

> The embedded single mac:mdio per nic case seems like the normal case, and
> most of the existing ACPI described devices are setup that way.

Somebody in this thread pointed to ACPI patches for the
MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet
interfaces, and two MDIO bus masters. One of the bus masters can only
do C22 and the other can only do C45. It is expected that the busses
are shared, not a nice one to one mapping.

> But at the same time, that shifts the c22/45 question to the nic
> driver, where use of a DSD property before instantiating/probing
> MDIO isn't really a problem if needed.

This in fact does not help you. The MAC driver has no idea what PHY is
connected to it. The MAC does not know if it is C22 or C45. It uses
the phylib abstraction which hides all this. Even if you assume 1:1,
use phy_find_first(), it will not find a C45 PHY because without
knowing there is a C45 PHY, we don't scan for it. And we should expect
C45 PHYs to become more popular in the next few years.

> In fact this embedded nic/mac/mdio/phy 1:1:1 case, is likely a requirement
> for passthrough into a generic VM, otherwise someone has to create a virtual
> mdio, and pass the phy in for the nic/mac.
> 
> AFAIK, NXP's part avoids this despite having a shared MDIO, because the phy
> state never leaves the mgmt side of the picture. It monitors the state and
> then feeds that back into their nic mgmt complex rather than using it
> directly.

That is the other model. Don't use Linux to drive the PHY, use
firmware in the MAC. A number of MACs do that, but it has the usual
problems of firmware. It limits you on your choice of PHYs, bugs in
the firmware cannot be fixed by the community, no sharing of drivers
because firmware is generally proprietary, no 'for free features'
because somebody else added features to the linux PHY driver etc.  But
it will make ACPI support simple, this whole discussion goes away, no
ACPI needed at all.

   Andrew

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Jeremy Linton]

On 5/8/20 6:42 PM, Andrew Lunn wrote:
> On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote:
>> Hi,
>>
>> On 5/8/20 3:27 PM, Andrew Lunn wrote:
>>>>> There is a very small number of devices where the vendor messed up,
>>>>> and did not put valid contents in the ID registers. In such cases, we
>>>>> can read the IDs from device tree. These are then used in exactly the
>>>>> same way as if they were read from the device.
>>>>>
>>>>
>>>> Is that the case here?
>>>
>>> Sorry, I don't understand the question?
>>
>> I was asking in general, does this machine report the ID's correctly.
> 
> Very likely, it does.
> 
>> The embedded single mac:mdio per nic case seems like the normal case, and
>> most of the existing ACPI described devices are setup that way.
> 
> Somebody in this thread pointed to ACPI patches for the
> MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet
> interfaces, and two MDIO bus masters. One of the bus masters can only
> do C22 and the other can only do C45. It is expected that the busses
> are shared, not a nice one to one mapping.

Thats whats going on with the NXP too AFAIK. But the mcbin is one of the 
ones with the "compatible" embedded in the DSD properties.. AKA, they 
buried the entire DT node there.

> 
>> But at the same time, that shifts the c22/45 question to the nic
>> driver, where use of a DSD property before instantiating/probing
>> MDIO isn't really a problem if needed.
> 
> This in fact does not help you. The MAC driver has no idea what PHY is
> connected to it. The MAC does not know if it is C22 or C45. It uses
Thats all I was trying to say (the mac side capability is implied by the 
HID/CID that instantiates it).

> the phylib abstraction which hides all this. Even if you assume 1:1,
> use phy_find_first(), it will not find a C45 PHY because without
> knowing there is a C45 PHY, we don't scan for it. And we should expect
> C45 PHYs to become more popular in the next few years.
> 
>> In fact this embedded nic/mac/mdio/phy 1:1:1 case, is likely a requirement
>> for passthrough into a generic VM, otherwise someone has to create a virtual
>> mdio, and pass the phy in for the nic/mac.
>>
>> AFAIK, NXP's part avoids this despite having a shared MDIO, because the phy
>> state never leaves the mgmt side of the picture. It monitors the state and
>> then feeds that back into their nic mgmt complex rather than using it
>> directly.
> 
> That is the other model. Don't use Linux to drive the PHY, use
> firmware in the MAC. A number of MACs do that, but it has the usual
> problems of firmware. It limits you on your choice of PHYs, bugs in
> the firmware cannot be fixed by the community, no sharing of drivers
> because firmware is generally proprietary, no 'for free features'
> because somebody else added features to the linux PHY driver etc.  But
> it will make ACPI support simple, this whole discussion goes away, no
> ACPI needed at all.

Open source firmware! :)

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote:
> On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote:
> > Hi,
> > 
> > On 5/8/20 3:27 PM, Andrew Lunn wrote:
> > > > > There is a very small number of devices where the vendor messed up,
> > > > > and did not put valid contents in the ID registers. In such cases, we
> > > > > can read the IDs from device tree. These are then used in exactly the
> > > > > same way as if they were read from the device.
> > > > > 
> > > > 
> > > > Is that the case here?
> > > 
> > > Sorry, I don't understand the question?
> > 
> > I was asking in general, does this machine report the ID's correctly.
> 
> Very likely, it does.
> 
> > The embedded single mac:mdio per nic case seems like the normal case, and
> > most of the existing ACPI described devices are setup that way.
> 
> Somebody in this thread pointed to ACPI patches for the
> MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet
> interfaces, and two MDIO bus masters. One of the bus masters can only
> do C22 and the other can only do C45. It is expected that the busses
> are shared, not a nice one to one mapping.
> 
> > But at the same time, that shifts the c22/45 question to the nic
> > driver, where use of a DSD property before instantiating/probing
> > MDIO isn't really a problem if needed.
> 
> This in fact does not help you. The MAC driver has no idea what PHY is
> connected to it. The MAC does not know if it is C22 or C45. It uses
> the phylib abstraction which hides all this. Even if you assume 1:1,
> use phy_find_first(), it will not find a C45 PHY because without
> knowing there is a C45 PHY, we don't scan for it. And we should expect
> C45 PHYs to become more popular in the next few years.

Agree.

NXP's LX2160ARDB platform currently has the following MDIO-PHY connection.

MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
MDIO-2 ==> one 25G PHY

Both the MDIOs are capable of C45.

There can be other complex use cases as well.
So, it is important to configure MDIO talk both C22 and C45 for different PHYs
connected on the same bus.

Regards
Calvin

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Russell King - ARM Linux admin]

On Mon, May 11, 2020 at 01:30:40PM +0530, Calvin Johnson wrote:
> On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote:
> > On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote:
> > > Hi,
> > > 
> > > On 5/8/20 3:27 PM, Andrew Lunn wrote:
> > > > > > There is a very small number of devices where the vendor messed up,
> > > > > > and did not put valid contents in the ID registers. In such cases, we
> > > > > > can read the IDs from device tree. These are then used in exactly the
> > > > > > same way as if they were read from the device.
> > > > > > 
> > > > > 
> > > > > Is that the case here?
> > > > 
> > > > Sorry, I don't understand the question?
> > > 
> > > I was asking in general, does this machine report the ID's correctly.
> > 
> > Very likely, it does.
> > 
> > > The embedded single mac:mdio per nic case seems like the normal case, and
> > > most of the existing ACPI described devices are setup that way.
> > 
> > Somebody in this thread pointed to ACPI patches for the
> > MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet
> > interfaces, and two MDIO bus masters. One of the bus masters can only
> > do C22 and the other can only do C45. It is expected that the busses
> > are shared, not a nice one to one mapping.
> > 
> > > But at the same time, that shifts the c22/45 question to the nic
> > > driver, where use of a DSD property before instantiating/probing
> > > MDIO isn't really a problem if needed.
> > 
> > This in fact does not help you. The MAC driver has no idea what PHY is
> > connected to it. The MAC does not know if it is C22 or C45. It uses
> > the phylib abstraction which hides all this. Even if you assume 1:1,
> > use phy_find_first(), it will not find a C45 PHY because without
> > knowing there is a C45 PHY, we don't scan for it. And we should expect
> > C45 PHYs to become more popular in the next few years.
> 
> Agree.
> 
> NXP's LX2160ARDB platform currently has the following MDIO-PHY connection.
> 
> MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)

I'm not entirely sure you have that correct.  The Clause 45 register set
as defined by IEEE 802.3 does not define registers for 1G negotiation,
unless the PHY either supports Clause 22 accesses, or implements some
kind of vendor extension.  For a 1G PHY, this would be wasteful, and
likely incompatible with a lot of hardware/software.

Conversely, Clause 22 does not define registers for 10G speeds, except
accessing Clause 45 registers indirectly through clause 22 registers,
which would also be wasteful.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

On Mon, May 11, 2020 at 10:38:49AM +0100, Russell King - ARM Linux admin wrote:
> On Mon, May 11, 2020 at 01:30:40PM +0530, Calvin Johnson wrote:
> > On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote:
> > > On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote:
> > > > Hi,
> > > > 
> > > > On 5/8/20 3:27 PM, Andrew Lunn wrote:
> > > > > > > There is a very small number of devices where the vendor messed up,
> > > > > > > and did not put valid contents in the ID registers. In such cases, we
> > > > > > > can read the IDs from device tree. These are then used in exactly the
> > > > > > > same way as if they were read from the device.
> > > > > > > 
> > > > > > 
> > > > > > Is that the case here?
> > > > > 
> > > > > Sorry, I don't understand the question?
> > > > 
> > > > I was asking in general, does this machine report the ID's correctly.
> > > 
> > > Very likely, it does.
> > > 
> > > > The embedded single mac:mdio per nic case seems like the normal case, and
> > > > most of the existing ACPI described devices are setup that way.
> > > 
> > > Somebody in this thread pointed to ACPI patches for the
> > > MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet
> > > interfaces, and two MDIO bus masters. One of the bus masters can only
> > > do C22 and the other can only do C45. It is expected that the busses
> > > are shared, not a nice one to one mapping.
> > > 
> > > > But at the same time, that shifts the c22/45 question to the nic
> > > > driver, where use of a DSD property before instantiating/probing
> > > > MDIO isn't really a problem if needed.
> > > 
> > > This in fact does not help you. The MAC driver has no idea what PHY is
> > > connected to it. The MAC does not know if it is C22 or C45. It uses
> > > the phylib abstraction which hides all this. Even if you assume 1:1,
> > > use phy_find_first(), it will not find a C45 PHY because without
> > > knowing there is a C45 PHY, we don't scan for it. And we should expect
> > > C45 PHYs to become more popular in the next few years.
> > 
> > Agree.
> > 
> > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection.
> > 
> > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
> 
> I'm not entirely sure you have that correct.  The Clause 45 register set
> as defined by IEEE 802.3 does not define registers for 1G negotiation,
> unless the PHY either supports Clause 22 accesses, or implements some
> kind of vendor extension.  For a 1G PHY, this would be wasteful, and
> likely incompatible with a lot of hardware/software.
> 
> Conversely, Clause 22 does not define registers for 10G speeds, except
> accessing Clause 45 registers indirectly through clause 22 registers,
> which would also be wasteful.
> 
Got your point.
Let me try to clarify.

MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
MDIO-2 ==> one 25G PHY
This is the physical connection of MDIO & PHYs on the platform.

For the c45 PHYs(two 10G), we use compatible "ethernet-phy-ieee802.3-c45"(not
yet upstreamed).
For c22 PHYs(two 1G), we don't mention the c45 compatible string and hence the
access also will be using c22, if I'm not wrong.

Regards
Calvin
k

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Russell King - ARM Linux admin]

On Mon, May 11, 2020 at 03:59:30PM +0530, Calvin Johnson wrote:
> On Mon, May 11, 2020 at 10:38:49AM +0100, Russell King - ARM Linux admin wrote:
> > On Mon, May 11, 2020 at 01:30:40PM +0530, Calvin Johnson wrote:
> > > On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote:
> > > > On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote:
> > > > > Hi,
> > > > > 
> > > > > On 5/8/20 3:27 PM, Andrew Lunn wrote:
> > > > > > > > There is a very small number of devices where the vendor messed up,
> > > > > > > > and did not put valid contents in the ID registers. In such cases, we
> > > > > > > > can read the IDs from device tree. These are then used in exactly the
> > > > > > > > same way as if they were read from the device.
> > > > > > > > 
> > > > > > > 
> > > > > > > Is that the case here?
> > > > > > 
> > > > > > Sorry, I don't understand the question?
> > > > > 
> > > > > I was asking in general, does this machine report the ID's correctly.
> > > > 
> > > > Very likely, it does.
> > > > 
> > > > > The embedded single mac:mdio per nic case seems like the normal case, and
> > > > > most of the existing ACPI described devices are setup that way.
> > > > 
> > > > Somebody in this thread pointed to ACPI patches for the
> > > > MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet
> > > > interfaces, and two MDIO bus masters. One of the bus masters can only
> > > > do C22 and the other can only do C45. It is expected that the busses
> > > > are shared, not a nice one to one mapping.
> > > > 
> > > > > But at the same time, that shifts the c22/45 question to the nic
> > > > > driver, where use of a DSD property before instantiating/probing
> > > > > MDIO isn't really a problem if needed.
> > > > 
> > > > This in fact does not help you. The MAC driver has no idea what PHY is
> > > > connected to it. The MAC does not know if it is C22 or C45. It uses
> > > > the phylib abstraction which hides all this. Even if you assume 1:1,
> > > > use phy_find_first(), it will not find a C45 PHY because without
> > > > knowing there is a C45 PHY, we don't scan for it. And we should expect
> > > > C45 PHYs to become more popular in the next few years.
> > > 
> > > Agree.
> > > 
> > > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection.
> > > 
> > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
> > 
> > I'm not entirely sure you have that correct.  The Clause 45 register set
> > as defined by IEEE 802.3 does not define registers for 1G negotiation,
> > unless the PHY either supports Clause 22 accesses, or implements some
> > kind of vendor extension.  For a 1G PHY, this would be wasteful, and
> > likely incompatible with a lot of hardware/software.
> > 
> > Conversely, Clause 22 does not define registers for 10G speeds, except
> > accessing Clause 45 registers indirectly through clause 22 registers,
> > which would also be wasteful.
> > 
> Got your point.
> Let me try to clarify.
> 
> MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
> MDIO-2 ==> one 25G PHY
> This is the physical connection of MDIO & PHYs on the platform.
> 
> For the c45 PHYs(two 10G), we use compatible "ethernet-phy-ieee802.3-c45"(not
> yet upstreamed).
> For c22 PHYs(two 1G), we don't mention the c45 compatible string and hence the
> access also will be using c22, if I'm not wrong.

You seem to have just repeated the same mistake (it seems to be a direct
copy-n-paste of what you sent in the email I replied to) - and then gone
on to say something different.  Either you're confused or you're not
writing in your email what you intend to.

You first say "MDIO-1 ==> two 1G PHYs(C45)".  You then say lower down
"For C22 PHYs (two 1G)".  Both these statements can't be true.

Similarly, you first say "MDIO-1 ==> two 10G PHYs(C22)".  You then say
lower down "For the c45 PHYs(two 10G)".  Again, both these statements
can't be true.

Given that this discussion in this thread has been about C22 vs C45, I
would have thought accuracy in regard to this point would have been of
the up-most importance.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

On Mon, May 11, 2020 at 11:48:17AM +0100, Russell King - ARM Linux admin wrote:
> On Mon, May 11, 2020 at 03:59:30PM +0530, Calvin Johnson wrote:
> > On Mon, May 11, 2020 at 10:38:49AM +0100, Russell King - ARM Linux admin wrote:
> > > On Mon, May 11, 2020 at 01:30:40PM +0530, Calvin Johnson wrote:
> > > > On Sat, May 09, 2020 at 01:42:57AM +0200, Andrew Lunn wrote:
> > > > > On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote:
> > > > > > Hi,
> > > > > > 
> > > > > > On 5/8/20 3:27 PM, Andrew Lunn wrote:
> > > > > > > > > There is a very small number of devices where the vendor messed up,
> > > > > > > > > and did not put valid contents in the ID registers. In such cases, we
> > > > > > > > > can read the IDs from device tree. These are then used in exactly the
> > > > > > > > > same way as if they were read from the device.
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > Is that the case here?
> > > > > > > 
> > > > > > > Sorry, I don't understand the question?
> > > > > > 
> > > > > > I was asking in general, does this machine report the ID's correctly.
> > > > > 
> > > > > Very likely, it does.
> > > > > 
> > > > > > The embedded single mac:mdio per nic case seems like the normal case, and
> > > > > > most of the existing ACPI described devices are setup that way.
> > > > > 
> > > > > Somebody in this thread pointed to ACPI patches for the
> > > > > MACCHIATOBin. If i remember the hardware correctly, it has 4 Ethernet
> > > > > interfaces, and two MDIO bus masters. One of the bus masters can only
> > > > > do C22 and the other can only do C45. It is expected that the busses
> > > > > are shared, not a nice one to one mapping.
> > > > > 
> > > > > > But at the same time, that shifts the c22/45 question to the nic
> > > > > > driver, where use of a DSD property before instantiating/probing
> > > > > > MDIO isn't really a problem if needed.
> > > > > 
> > > > > This in fact does not help you. The MAC driver has no idea what PHY is
> > > > > connected to it. The MAC does not know if it is C22 or C45. It uses
> > > > > the phylib abstraction which hides all this. Even if you assume 1:1,
> > > > > use phy_find_first(), it will not find a C45 PHY because without
> > > > > knowing there is a C45 PHY, we don't scan for it. And we should expect
> > > > > C45 PHYs to become more popular in the next few years.
> > > > 
> > > > Agree.
> > > > 
> > > > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection.
> > > > 
> > > > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
> > > 
> > > I'm not entirely sure you have that correct.  The Clause 45 register set
> > > as defined by IEEE 802.3 does not define registers for 1G negotiation,
> > > unless the PHY either supports Clause 22 accesses, or implements some
> > > kind of vendor extension.  For a 1G PHY, this would be wasteful, and
> > > likely incompatible with a lot of hardware/software.
> > > 
> > > Conversely, Clause 22 does not define registers for 10G speeds, except
> > > accessing Clause 45 registers indirectly through clause 22 registers,
> > > which would also be wasteful.
> > > 
> > Got your point.
> > Let me try to clarify.
> > 
> > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
> > MDIO-2 ==> one 25G PHY
> > This is the physical connection of MDIO & PHYs on the platform.
> > 
> > For the c45 PHYs(two 10G), we use compatible "ethernet-phy-ieee802.3-c45"(not
> > yet upstreamed).
> > For c22 PHYs(two 1G), we don't mention the c45 compatible string and hence the
> > access also will be using c22, if I'm not wrong.
> 
> You seem to have just repeated the same mistake (it seems to be a direct
> copy-n-paste of what you sent in the email I replied to) - and then gone
> on to say something different.  Either you're confused or you're not
> writing in your email what you intend to.
> 
> You first say "MDIO-1 ==> two 1G PHYs(C45)".  You then say lower down
> "For C22 PHYs (two 1G)".  Both these statements can't be true.
> 
> Similarly, you first say "MDIO-1 ==> two 10G PHYs(C22)".  You then say
> lower down "For the c45 PHYs(two 10G)".  Again, both these statements
> can't be true.
> 
> Given that this discussion in this thread has been about C22 vs C45, I
> would have thought accuracy in regard to this point would have been of
> the up-most importance.
> 
It was my mistake. Sorry about that. 1G PHYs are C22 and 10G PHYs are C45.
I didn't notice the mistake, hence copied.
Correcting:
MDIO-1 ==> one 40G PHY, two 1G PHYs(C22), two 10G PHYs(C45)

Thanks
Calvin

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Andrew Lunn]

> NXP's LX2160ARDB platform currently has the following MDIO-PHY connection.
> 
> MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
> MDIO-2 ==> one 25G PHY

It has been suggested that ACPI only support a one to one
mapping. Each MAC has one MDIO bus, with one PHY on it. KISS.

This clearly does not work for your hardware. So not only do we need
to solve how PHY properties are described, we also need an equivalent
of phy-handle, so a MAC can indicate which PHY it is connected to.

So in effect, you seem to be heading towards a pretty full
reproduction of the DT binding. Before you go too much further and
waste too much of your time, you might want confirmation from the ACPI
people this is not too advanced for what ACPI can do and they tell you
to forget ACPI for this hardware and stick with DT.

   Andrew

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Russell King - ARM Linux admin]

On Mon, May 11, 2020 at 03:04:57PM +0200, Andrew Lunn wrote:
> > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection.
> > 
> > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
> > MDIO-2 ==> one 25G PHY
> 
> It has been suggested that ACPI only support a one to one
> mapping. Each MAC has one MDIO bus, with one PHY on it. KISS.
> 
> This clearly does not work for your hardware. So not only do we need
> to solve how PHY properties are described, we also need an equivalent
> of phy-handle, so a MAC can indicate which PHY it is connected to.

I'd suggest that doesn't work for a lot of hardware. It won't work for
the Macchiatobin for example, where there are two Clause 45 NBASE-T
PHYs on one MDIO bus.

The same is likely true on the LX2160A - there can be multiple ethernet
interfaces, but IIRC only two external MDIO buses that one can hang
PHYs off of.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

On Mon, May 11, 2020 at 03:04:57PM +0200, Andrew Lunn wrote:
> > NXP's LX2160ARDB platform currently has the following MDIO-PHY connection.
> > 
> > MDIO-1 ==> one 40G PHY, two 1G PHYs(C45), two 10G PHYs(C22)
> > MDIO-2 ==> one 25G PHY
> 
> It has been suggested that ACPI only support a one to one
> mapping. Each MAC has one MDIO bus, with one PHY on it. KISS.
> 
> This clearly does not work for your hardware. So not only do we need
> to solve how PHY properties are described, we also need an equivalent
> of phy-handle, so a MAC can indicate which PHY it is connected to.
> 

Right. I had introduced fwnode_get_phy_node() to take care of phy-handle.

> So in effect, you seem to be heading towards a pretty full
> reproduction of the DT binding. Before you go too much further and
> waste too much of your time, you might want confirmation from the ACPI
> people this is not too advanced for what ACPI can do and they tell you
> to forget ACPI for this hardware and stick with DT.

I've copied the patchset to Rafael and acpi ML.
Waiting for their response.

Thanks
Calvin

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

On Fri, May 08, 2020 at 05:48:33PM -0500, Jeremy Linton wrote:
> Hi,
> 
> On 5/8/20 3:27 PM, Andrew Lunn wrote:
> > > > There is a very small number of devices where the vendor messed up,
> > > > and did not put valid contents in the ID registers. In such cases, we
> > > > can read the IDs from device tree. These are then used in exactly the
> > > > same way as if they were read from the device.
> > > > 
> > > 
> > > Is that the case here?
> > 
> > Sorry, I don't understand the question?
> 
> I was asking in general, does this machine report the ID's correctly. More
> directed at Calvin, but part of it is the board vendor too. So I suspect no
> one can really answer "yes", despite that seeming to be the case.

I had tested RGMII PHYs(AR8035) with c22 using mdiobus_register. Vendor ID was
read out correctly while autoprobing.

AQR107(c45) PHYs that were connected gave back 0x00. This is expected
because the MDIO bus is configured to talk c22.

Regards
Calvin

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Calvin Johnson]

Thanks Andrew and Jeremy for the detailed discussion!

On Fri, May 08, 2020 at 08:13:01PM +0200, Andrew Lunn wrote:
> > > It does have a numeric version defined for EISA types. OTOH I suspect that
> > > your right. If there were a "PHY\VEN_IDvvvv&ID_DDDD" definition, it may not
> > > be ideal to parse it. Instead the normal ACPI model of exactly matching the
> > > complete string in the phy driver might be more appropriate.
> > 
> > IMO, it should be fine to parse the string to extract the phy_id. Is there any
> > reason why we cannot do this?
> 
> Some background here, about what the PHY core does.
> 
> PHYs have two ID registers. This contains vendor, device, and often
> revision of the PHY. Only the vendor part is standardised, vendors can
> decide how to use the device part, but it is common for the lowest
> nibble to be revision. The core will read these ID registers, and then
> go through all the PHY drivers registered and ask them if they support
> this ID. The drivers provide a table of IDs and masks. The mask is
> applied, and then if the ID matches, the driver is used. The mask
> allows the revision to be ignored, etc.
> 
> There is a very small number of devices where the vendor messed up,
> and did not put valid contents in the ID registers. In such cases, we
> can read the IDs from device tree. These are then used in exactly the
> same way as if they were read from the device.
> 
> If you want the ACPI model to be used, an exact match on the string,
> you are going to have to modify the core and the drivers. They
> currently don't have any string, and have no idea about different
> revisions which are out in the wild.

I don't think ACPI mandates that OS driver use exact string match and not parse
the string.

First of all, I would suggest that we use "compatible" property instead of _CID.
Not sure of a reason why we cannot. This will simplify implementation of fwnode
APIs.

Already I've pointed out couple of ASL files in tianocore where they are already
used.
one eg:https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl#L280

Even if we use _CID, I'm not sure we are prohibited from extracting characters
(phy_id) from it.
If we decide to use _CID, then we need to define somewhere and standardize
exactly how we are going to use it. I'm not sure where we can do this.

> 
> > > Similarly to how I suspect the next patch's use of "compatible" isn't ideal
> > > either, because whether a device is c45 or not, should tend to be fixed to a
> > > particular vendor/device implementation and not a firmware provided
> > > property.
> 
> Not exactly true. It is the combination of can the bus master do C45
> and can the device do C45. Unfortunately, we have no knowledge of the
> bus masters capabilities, if it can do C45. And many MDIO drivers will
> do a C22 transaction when asked to perform a C45 transaction. All new
> submissions for MDIO drivers i ask for EOPNOTSUPP to be returned if
> C45 is not supported. But we cannot rely on that. Too much history.

Makes sense to me.

> > 
> > I tend to agree with you on this. Even for DT, ideal case, IMO should be:
> > 
> > 1) mdiobus_scan scans the mdiobus for c22 devices by reading phy id from
> > registers 2 and 3
> > 2) if not found scan for c45 devices <= looks like this is missing in Linux
> > 3) look for phy_id from compatible string.
> 
> It is somewhat more complex, in that there are a small number of
> devices which will respond to both C22 and C45. Generally, you want to
> use C45 if supported. So you would want to do the C45 scan first. But
> then the earlier problem comes to play, you have no idea if the bus
> master actually correctly supports C45.
> 
> Given the issues, we assume all bus masters and PHY devices are C22
> unless DT says the bus master and PHY combination is compatible with
> C45.

Makes sense to me.

Re: [net-next PATCH v3 4/5] net: phy: Introduce fwnode_get_phy_id()

[Andrew Lunn]

On Mon, May 11, 2020 at 11:22:31AM +0530, Calvin Johnson wrote:
> Thanks Andrew and Jeremy for the detailed discussion!
> 
> On Fri, May 08, 2020 at 08:13:01PM +0200, Andrew Lunn wrote:
> > > > It does have a numeric version defined for EISA types. OTOH I suspect that
> > > > your right. If there were a "PHY\VEN_IDvvvv&ID_DDDD" definition, it may not
> > > > be ideal to parse it. Instead the normal ACPI model of exactly matching the
> > > > complete string in the phy driver might be more appropriate.
> > > 
> > > IMO, it should be fine to parse the string to extract the phy_id. Is there any
> > > reason why we cannot do this?
> > 
> > Some background here, about what the PHY core does.
> > 
> > PHYs have two ID registers. This contains vendor, device, and often
> > revision of the PHY. Only the vendor part is standardised, vendors can
> > decide how to use the device part, but it is common for the lowest
> > nibble to be revision. The core will read these ID registers, and then
> > go through all the PHY drivers registered and ask them if they support
> > this ID. The drivers provide a table of IDs and masks. The mask is
> > applied, and then if the ID matches, the driver is used. The mask
> > allows the revision to be ignored, etc.
> > 
> > There is a very small number of devices where the vendor messed up,
> > and did not put valid contents in the ID registers. In such cases, we
> > can read the IDs from device tree. These are then used in exactly the
> > same way as if they were read from the device.
> > 
> > If you want the ACPI model to be used, an exact match on the string,
> > you are going to have to modify the core and the drivers. They
> > currently don't have any string, and have no idea about different
> > revisions which are out in the wild.
> 
> I don't think ACPI mandates that OS driver use exact string match and not parse
> the string.
> 
> First of all, I would suggest that we use "compatible" property instead of _CID.
> Not sure of a reason why we cannot. This will simplify implementation of fwnode
> APIs.
> 
> Already I've pointed out couple of ASL files in tianocore where they are already
> used.
> one eg:https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Marvell/Armada7k8k/AcpiTables/Armada80x0McBin/Dsdt.asl#L280
> 
> Even if we use _CID, I'm not sure we are prohibited from extracting characters
> (phy_id) from it.
> If we decide to use _CID, then we need to define somewhere and standardize
> exactly how we are going to use it. I'm not sure where we can do this.

Hi Calvin

Whatever is decided needs to be documented as it becomes a defacto
standard. Once this is in the Linux PHY core, that is how it is done
for all boards using ACPI.

Maybe sometime in the future when the ACPI standards committee
definitively defines how this should be done, we can add a second
implementation which is standards conformant.

       Andrew

[net-next PATCH v3 5/5] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Calvin Johnson]

Introduce fwnode_mdiobus_register_phy() to register PHYs on the
mdiobus. From the compatible string, identify whether the PHY is
c45 and based on this create a PHY device instance which is
registered on the mdiobus.

Signed-off-by: Calvin Johnson <calvin.johnson@oss.nxp.com>
---

Changes in v3:
  Introduce two functions to register phy to mdiobus using fwnode

Changes in v2: None

 drivers/net/phy/mdio_bus.c | 41 ++++++++++++++++++++++++++++++++++++++
 include/linux/mdio.h       |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c
index 3e79b96fa344..b51e597c0479 100644
--- a/drivers/net/phy/mdio_bus.c
+++ b/drivers/net/phy/mdio_bus.c
@@ -106,6 +106,47 @@ int mdiobus_unregister_device(struct mdio_device *mdiodev)
 }
 EXPORT_SYMBOL(mdiobus_unregister_device);
 
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+				struct fwnode_handle *child, u32 addr)
+{
+	struct phy_device *phy;
+	bool is_c45 = false;
+	const char *cp;
+	u32 phy_id;
+	int rc;
+
+	fwnode_property_read_string(child, "compatible", &cp);
+	if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
+		is_c45 = true;
+
+	if (!is_c45 && !fwnode_get_phy_id(child, &phy_id))
+		phy = phy_device_create(bus, addr, phy_id, 0, NULL);
+	else
+		phy = get_phy_device(bus, addr, is_c45);
+	if (IS_ERR(phy))
+		return PTR_ERR(phy);
+
+	phy->irq = bus->irq[addr];
+
+	/* Associate the fwnode with the device structure so it
+	 * can be looked up later.
+	 */
+	phy->mdio.dev.fwnode = child;
+
+	/* All data is now stored in the phy struct, so register it */
+	rc = phy_device_register(phy);
+	if (rc) {
+		phy_device_free(phy);
+		fwnode_handle_put(child);
+		return rc;
+	}
+
+	dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
+
+	return 0;
+}
+EXPORT_SYMBOL(fwnode_mdiobus_register_phy);
+
 struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr)
 {
 	struct mdio_device *mdiodev = bus->mdio_map[addr];
diff --git a/include/linux/mdio.h b/include/linux/mdio.h
index 917e4bb2ed71..e55609697b42 100644
--- a/include/linux/mdio.h
+++ b/include/linux/mdio.h
@@ -330,6 +330,8 @@ int mdiobus_register_device(struct mdio_device *mdiodev);
 int mdiobus_unregister_device(struct mdio_device *mdiodev);
 bool mdiobus_is_registered_device(struct mii_bus *bus, int addr);
 struct phy_device *mdiobus_get_phy(struct mii_bus *bus, int addr);
+int fwnode_mdiobus_register_phy(struct mii_bus *bus,
+				      struct fwnode_handle *child, u32 addr);
 
 /**
  * mdio_module_driver() - Helper macro for registering mdio drivers
-- 
2.17.1

Re: [net-next PATCH v3 5/5] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Andy Shevchenko]

On Tue, May 5, 2020 at 4:30 PM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> Introduce fwnode_mdiobus_register_phy() to register PHYs on the
> mdiobus. From the compatible string, identify whether the PHY is
> c45 and based on this create a PHY device instance which is
> registered on the mdiobus.

...

> +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> +                               struct fwnode_handle *child, u32 addr)
> +{
> +       struct phy_device *phy;

> +       bool is_c45 = false;

Redundant assignment, see below.

> +       const char *cp;
> +       u32 phy_id;
> +       int rc;
> +

> +       fwnode_property_read_string(child, "compatible", &cp);

Consider rc = ...; otherwise you will have UB below.

> +       if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))

UB!

> +               is_c45 = true;

is_c45 = !(rc || strcmp(...));

> +       if (!is_c45 && !fwnode_get_phy_id(child, &phy_id))

Perhaps positive conditional

if (is_c45 || fwnode_...(...))
  get_phy_device();
else
  ...

> +               phy = phy_device_create(bus, addr, phy_id, 0, NULL);
> +       else
> +               phy = get_phy_device(bus, addr, is_c45);
> +       if (IS_ERR(phy))
> +               return PTR_ERR(phy);
> +
> +       phy->irq = bus->irq[addr];
> +
> +       /* Associate the fwnode with the device structure so it
> +        * can be looked up later.
> +        */
> +       phy->mdio.dev.fwnode = child;
> +
> +       /* All data is now stored in the phy struct, so register it */
> +       rc = phy_device_register(phy);
> +       if (rc) {
> +               phy_device_free(phy);

> +               fwnode_handle_put(child);

Shouldn't mdio.dev.fwnode be put rather than child (yes, I see the assignment)

> +               return rc;
> +       }
> +
> +       dev_dbg(&bus->dev, "registered phy at address %i\n", addr);
> +
> +       return 0;
> +}

-- 
With Best Regards,
Andy Shevchenko

Re: [net-next PATCH v3 5/5] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Calvin Johnson]

On Tue, May 05, 2020 at 05:22:00PM +0300, Andy Shevchenko wrote:
> On Tue, May 5, 2020 at 4:30 PM Calvin Johnson
> <calvin.johnson@oss.nxp.com> wrote:
> >
> > Introduce fwnode_mdiobus_register_phy() to register PHYs on the
> > mdiobus. From the compatible string, identify whether the PHY is
> > c45 and based on this create a PHY device instance which is
> > registered on the mdiobus.
> 
> ...
> 
> > +int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> > +                               struct fwnode_handle *child, u32 addr)
> > +{
> > +       struct phy_device *phy;
> 
> > +       bool is_c45 = false;
> 
> Redundant assignment, see below.
> 
> > +       const char *cp;
> > +       u32 phy_id;
> > +       int rc;
> > +
> 
> > +       fwnode_property_read_string(child, "compatible", &cp);
> 
> Consider rc = ...; otherwise you will have UB below.
> 
> > +       if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
> 
> UB!

Thanks for the comments! I've considered all of them.
What is UB, by the way? :)-

Regards
Calvin

Re: [net-next PATCH v3 5/5] net: mdiobus: Introduce fwnode_mdiobus_register_phy()

[Andy Shevchenko]

On Thu, May 7, 2020 at 10:44 AM Calvin Johnson
<calvin.johnson@oss.nxp.com> wrote:
>
> On Tue, May 05, 2020 at 05:22:00PM +0300, Andy Shevchenko wrote:
> > On Tue, May 5, 2020 at 4:30 PM Calvin Johnson
> > <calvin.johnson@oss.nxp.com> wrote:

...

> > > +       bool is_c45 = false;
> >
> > Redundant assignment, see below.
> >
> > > +       const char *cp;
> > > +       u32 phy_id;
> > > +       int rc;

> > > +       fwnode_property_read_string(child, "compatible", &cp);
> >
> > Consider rc = ...; otherwise you will have UB below.
> >
> > > +       if (!strcmp(cp, "ethernet-phy-ieee802.3-c45"))
> >
> > UB!
>
> Thanks for the comments! I've considered all of them.
> What is UB, by the way? :)-

Undefined Behaviour.

-- 
With Best Regards,
Andy Shevchenko