[U-Boot] [PATCH v4 0/5] Add helper function for linking a DM Eth device to a PHY

[U-Boot] [PATCH v4 0/5] Add helper function for linking a DM Eth device to a PHY

[Alex Marginean]

The patch set introduces dm_eth_phy_connect which takes in an ethernet device
and uses DT information to find the associated PHY and connect the Ethernet
interface to it.  This should simplify similar code in ethernet drivers.
A new binding is introduced for scanning an MDIO bus, useful if the bus is
private to the Ethernet interface.  We're going to try to get this binding
accepted in Linux too.
Finally the patch set updates fsl_enetc driver to use the new helper function.


This patch set supersedes v2 series:
https://patchwork.ozlabs.org/project/uboot/list/?series=124537
and v3 series:
https://patchwork.ozlabs.org/project/uboot/list/?series=140114

Changes in v4:
 - rebased on current head

Changes in v3:
 - added cover letter
 - check for null PHY pointer before using it in dm_eth_connect_phy_handle
 - moved the code dealing with MDIO scanning into a separate patch
 - renames several arguments and variables for a bit more clarity and
   consistency

Changes in v2:
- Moved MDIO scan code into dm_mdio_phy_scan which is also exported
- Use interface instead of if_type for consistency
- don't use phy pointer if NULL in fsl_enetc code

Alex Marginean (5):
  net: mdio-uclass: rename arguments of dm_mdio_phy_connect for
    consistency
  net: mdio-uclass: add dm_eth_phy_connect helper function
  net: mdio-uclass: Add helper functions for scanning the MDIO bus
  doc: bindings: add mdio-handle property to ethernet nodes
  drivers: net: fsl_enetc: use the new MDIO DM helper functions

 doc/device-tree-bindings/net/ethernet.txt |   2 +
 drivers/net/fsl_enetc.c                   |  52 ++------
 drivers/net/fsl_enetc.h                   |   1 +
 include/miiphy.h                          |  32 ++++-
 net/mdio-uclass.c                         | 139 +++++++++++++++++++++-
 5 files changed, 175 insertions(+), 51 deletions(-)

-- 
2.17.1

[U-Boot] [PATCH v4 1/5] net: mdio-uclass: rename arguments of dm_mdio_phy_connect for clarity

[Alex Marginean]

Renamed dm_mdio_phy_connect arguments dev to mdiodev and addr to phyaddr
for a bit more clarity and consistency with the following patches.
Also use NULL instead of 0 on error return path.

Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
---
 include/miiphy.h  |  6 +++---
 net/mdio-uclass.c | 10 +++++-----
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/miiphy.h b/include/miiphy.h
index 9b97d09f18..94bf0da24a 100644
--- a/include/miiphy.h
+++ b/include/miiphy.h
@@ -154,14 +154,14 @@ void dm_mdio_probe_devices(void);
 /**
  * dm_mdio_phy_connect - Wrapper over phy_connect for DM MDIO
  *
- * @dev: mdio dev
- * @addr: PHY address on MDIO bus
+ * @mdiodev: mdio device the PHY is accesible on
+ * @phyaddr: PHY address on MDIO bus
  * @ethdev: ethernet device to connect to the PHY
  * @interface: MAC-PHY protocol
  *
  * @return pointer to phy_device, or 0 on error
  */
-struct phy_device *dm_mdio_phy_connect(struct udevice *dev, int addr,
+struct phy_device *dm_mdio_phy_connect(struct udevice *mdiodev, int phyaddr,
 				       struct udevice *ethdev,
 				       phy_interface_t interface);
 
diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
index 6f922e80b6..7a5f1d6dcc 100644
--- a/net/mdio-uclass.c
+++ b/net/mdio-uclass.c
@@ -104,16 +104,16 @@ static int dm_mdio_pre_remove(struct udevice *dev)
 	return 0;
 }
 
-struct phy_device *dm_mdio_phy_connect(struct udevice *dev, int addr,
+struct phy_device *dm_mdio_phy_connect(struct udevice *mdiodev, int phyaddr,
 				       struct udevice *ethdev,
 				       phy_interface_t interface)
 {
-	struct mdio_perdev_priv *pdata = dev_get_uclass_priv(dev);
+	struct mdio_perdev_priv *pdata = dev_get_uclass_priv(mdiodev);
 
-	if (device_probe(dev))
-		return 0;
+	if (device_probe(mdiodev))
+		return NULL;
 
-	return phy_connect(pdata->mii_bus, addr, ethdev, interface);
+	return phy_connect(pdata->mii_bus, phyaddr, ethdev, interface);
 }
 
 UCLASS_DRIVER(mdio) = {
-- 
2.17.1

[U-Boot] [PATCH v4 2/5] net: mdio-uclass: add dm_eth_phy_connect helper function

[Alex Marginean]

The function connects an ethernet device to a PHY using DT information.
This API is only available for eth devices with an associated device tree
node.

Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
---
 include/miiphy.h  | 12 ++++++++
 net/mdio-uclass.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+)

diff --git a/include/miiphy.h b/include/miiphy.h
index 94bf0da24a..e591d93416 100644
--- a/include/miiphy.h
+++ b/include/miiphy.h
@@ -165,6 +165,18 @@ struct phy_device *dm_mdio_phy_connect(struct udevice *mdiodev, int phyaddr,
 				       struct udevice *ethdev,
 				       phy_interface_t interface);
 
+/**
+ * dm_eth_phy_connect - Connect an Eth device to a PHY based on device tree
+ *
+ * Picks up the DT phy-handle/mdio-handle and phy-mode from ethernet device node
+ * and connects the ethernet device to the linked PHY.
+ *
+ * @ethdev: ethernet device
+ *
+ * @return pointer to phy_device, or 0 on error
+ */
+struct phy_device *dm_eth_phy_connect(struct udevice *ethdev);
+
 #endif
 
 #ifdef CONFIG_DM_MDIO_MUX
diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
index 7a5f1d6dcc..cf736a3ae5 100644
--- a/net/mdio-uclass.c
+++ b/net/mdio-uclass.c
@@ -116,6 +116,78 @@ struct phy_device *dm_mdio_phy_connect(struct udevice *mdiodev, int phyaddr,
 	return phy_connect(pdata->mii_bus, phyaddr, ethdev, interface);
 }
 
+static struct phy_device *dm_eth_connect_phy_handle(struct udevice *ethdev,
+						    phy_interface_t interface)
+{
+	u32 phy_phandle, phy_addr;
+	struct udevice *mdiodev;
+	struct phy_device *phy;
+	ofnode phy_node;
+
+	if (ofnode_read_u32(ethdev->node, "phy-handle", &phy_phandle)) {
+		dev_dbg(dev, "phy-handle missing in ethernet node\n");
+		return NULL;
+	}
+
+	phy_node = ofnode_get_by_phandle(phy_phandle);
+	if (!ofnode_valid(phy_node)) {
+		dev_dbg(dev, "invalid phy node\n");
+		return NULL;
+	}
+
+	if (ofnode_read_u32(phy_node, "reg", &phy_addr)) {
+		dev_dbg(ethdev, "missing reg property in phy node\n");
+		return NULL;
+	}
+
+	if (uclass_get_device_by_ofnode(UCLASS_MDIO,
+					ofnode_get_parent(phy_node),
+					&mdiodev)) {
+		dev_dbg(dev, "can't find MDIO bus for node %s\n",
+			ofnode_get_name(ofnode_get_parent(phy_node)));
+		return NULL;
+	}
+
+	phy = dm_mdio_phy_connect(mdiodev, phy_addr, ethdev, interface);
+
+	if (phy)
+		phy->node = phy_node;
+
+	return phy;
+}
+
+/* Connect to a PHY linked in eth DT node */
+struct phy_device *dm_eth_phy_connect(struct udevice *ethdev)
+{
+	const char *if_str;
+	phy_interface_t interface;
+	struct phy_device *phy;
+
+	if (!ofnode_valid(ethdev->node)) {
+		debug("%s: supplied eth dev has no DT node!\n", ethdev->name);
+		return NULL;
+	}
+
+	interface = PHY_INTERFACE_MODE_NONE;
+	if_str = ofnode_read_string(ethdev->node, "phy-mode");
+	if (if_str)
+		interface = phy_get_interface_by_name(if_str);
+	if (interface < 0)
+		interface = PHY_INTERFACE_MODE_NONE;
+
+	if (interface == PHY_INTERFACE_MODE_NONE)
+		dev_dbg(ethdev, "can't find interface mode, default to NONE\n");
+
+	phy = dm_eth_connect_phy_handle(ethdev, interface);
+
+	if (!phy)
+		return NULL;
+
+	phy->interface = interface;
+
+	return phy;
+}
+
 UCLASS_DRIVER(mdio) = {
 	.id = UCLASS_MDIO,
 	.name = "mdio",
-- 
2.17.1

[U-Boot] [PATCH v4 3/5] net: mdio-uclass: Add helper functions for scanning the MDIO bus

[Alex Marginean]

The patch introduces dm_mdio_phy_scan which is a DM wrapper over
phy_find_by_mask.  It also updates dm_eth_phy_connect to scan a MDIO bus
for PHYs if the ethernet node contains a mdio-handle property.

Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
---
 include/miiphy.h  | 14 ++++++++++++
 net/mdio-uclass.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+)

diff --git a/include/miiphy.h b/include/miiphy.h
index e591d93416..a5bdc29a47 100644
--- a/include/miiphy.h
+++ b/include/miiphy.h
@@ -165,6 +165,20 @@ struct phy_device *dm_mdio_phy_connect(struct udevice *mdiodev, int phyaddr,
 				       struct udevice *ethdev,
 				       phy_interface_t interface);
 
+/**
+ * dm_mdio_phy_scan - Scan for a PHY on the given MDIO bus
+ *
+ * @mdiodev: mdio device to scan
+ * @ethdev: ethernet device to connect to the PHY
+ * @interface: MAC-PHY protocol
+ * @addrmask: bitmask of PHY addresses to scan, use all Fs to scan all addresses
+ *
+ * @return pointer to phy_device, or 0 on error
+ */
+struct phy_device *dm_mdio_phy_scan(struct udevice *mdiodev,
+				    struct udevice *ethdev,
+				    phy_interface_t interface, uint addrmask);
+
 /**
  * dm_eth_phy_connect - Connect an Eth device to a PHY based on device tree
  *
diff --git a/net/mdio-uclass.c b/net/mdio-uclass.c
index cf736a3ae5..9ca462edb2 100644
--- a/net/mdio-uclass.c
+++ b/net/mdio-uclass.c
@@ -116,6 +116,24 @@ struct phy_device *dm_mdio_phy_connect(struct udevice *mdiodev, int phyaddr,
 	return phy_connect(pdata->mii_bus, phyaddr, ethdev, interface);
 }
 
+struct phy_device *dm_mdio_phy_scan(struct udevice *mdiodev,
+				    struct udevice *ethdev,
+				    phy_interface_t interface, uint addrmask)
+{
+	struct mdio_perdev_priv *pdata;
+	struct phy_device *phy;
+
+	pdata = dev_get_uclass_priv(mdiodev);
+
+	phy = phy_find_by_mask(pdata->mii_bus, addrmask, interface);
+	if (phy)
+		phy_connect_dev(phy, ethdev);
+	else
+		dev_dbg(mdiodev, "no PHY detected on bus\n");
+
+	return phy;
+}
+
 static struct phy_device *dm_eth_connect_phy_handle(struct udevice *ethdev,
 						    phy_interface_t interface)
 {
@@ -156,6 +174,34 @@ static struct phy_device *dm_eth_connect_phy_handle(struct udevice *ethdev,
 	return phy;
 }
 
+static struct phy_device *dm_eth_connect_mdio_handle(struct udevice *ethdev,
+						     phy_interface_t interface)
+{
+	u32 mdio_phandle;
+	ofnode mdio_node;
+	struct udevice *mdiodev;
+	uint mask = 0xffffffff;
+
+	if (ofnode_read_u32(ethdev->node, "mdio-handle", &mdio_phandle)) {
+		dev_dbg(ethdev, "mdio-handle missing in ethernet node\n");
+		return NULL;
+	}
+
+	mdio_node = ofnode_get_by_phandle(mdio_phandle);
+	if (!ofnode_valid(mdio_node)) {
+		dev_dbg(dev, "invalid mdio node\n");
+		return NULL;
+	}
+
+	if (uclass_get_device_by_ofnode(UCLASS_MDIO, mdio_node, &mdiodev)) {
+		dev_dbg(ethdev, "can't find MDIO bus for node %s\n",
+			ofnode_get_name(mdio_node));
+		return NULL;
+	}
+
+	return dm_mdio_phy_scan(mdiodev, ethdev, interface, mask);
+}
+
 /* Connect to a PHY linked in eth DT node */
 struct phy_device *dm_eth_phy_connect(struct udevice *ethdev)
 {
@@ -178,8 +224,19 @@ struct phy_device *dm_eth_phy_connect(struct udevice *ethdev)
 	if (interface == PHY_INTERFACE_MODE_NONE)
 		dev_dbg(ethdev, "can't find interface mode, default to NONE\n");
 
+	/*
+	 * The sequence is:
+	 * - if there is a phy-handle property, follow that,
+	 * - if there is a mdio-handle property, follow that and scan for the
+	 *   PHY,
+	 * - if the above came out empty, return NULL.
+	 */
+
 	phy = dm_eth_connect_phy_handle(ethdev, interface);
 
+	if (!phy)
+		phy = dm_eth_connect_mdio_handle(ethdev, interface);
+
 	if (!phy)
 		return NULL;
 
-- 
2.17.1

[U-Boot] [PATCH v4 4/5] doc: bindings: add mdio-handle property to ethernet nodes

[Alex Marginean]

Adds an optional mdio-handle property which identifies a MDIO bus which can
be scanned to find the relevant PHY.  The property is ignored if phy-handle
is also present.

Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
---
 doc/device-tree-bindings/net/ethernet.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/doc/device-tree-bindings/net/ethernet.txt b/doc/device-tree-bindings/net/ethernet.txt
index 3fc360523b..9f9629f8d6 100644
--- a/doc/device-tree-bindings/net/ethernet.txt
+++ b/doc/device-tree-bindings/net/ethernet.txt
@@ -9,6 +9,8 @@ The following properties are common to the Ethernet controllers:
 - max-speed: number, specifies maximum speed in Mbit/s supported by the device;
 - max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
   the maximum frame size (there's contradiction in ePAPR).
+- mdio-handle: phandle, specifies a reference to a MDIO bus to be scanned to
+  find the PHY device.  Ignored if phy-handle is also present.
 - phy-mode: string, operation mode of the PHY interface; supported values are
   "mii", "gmii", "sgmii", "qsgmii", "tbi", "rev-mii", "rmii", "rgmii", "rgmii-id",
   "rgmii-rxid", "rgmii-txid", "rtbi", "smii", "xgmii"; this is now a de-facto
-- 
2.17.1

[U-Boot] [PATCH v4 5/5] drivers: net: fsl_enetc: use the new MDIO DM helper functions

[Alex Marginean]

Uses the new dm_eth_phy_connect helper to connect to the PHY to simplify
the code.

Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
---
 drivers/net/fsl_enetc.c | 53 +++++++----------------------------------
 drivers/net/fsl_enetc.h |  1 +
 2 files changed, 10 insertions(+), 44 deletions(-)

diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
index 0ca7e838a8..e1713a3337 100644
--- a/drivers/net/fsl_enetc.c
+++ b/drivers/net/fsl_enetc.c
@@ -203,57 +203,20 @@ static void enetc_start_pcs(struct udevice *dev)
 }
 
 /* Configure the actual/external ethernet PHY, if one is found */
-static void enetc_start_phy(struct udevice *dev)
+static void enetc_config_phy(struct udevice *dev)
 {
 	struct enetc_priv *priv = dev_get_priv(dev);
-	struct udevice *miidev;
-	struct phy_device *phy;
-	u32 phandle, phy_id;
-	ofnode phy_node;
 	int supported;
 
-	if (!ofnode_valid(dev->node)) {
-		enetc_dbg(dev, "no enetc ofnode found, skipping PHY set-up\n");
-		return;
-	}
-
-	if (ofnode_read_u32(dev->node, "phy-handle", &phandle)) {
-		enetc_dbg(dev, "phy-handle not found, skipping PHY set-up\n");
-		return;
-	}
-
-	phy_node = ofnode_get_by_phandle(phandle);
-	if (!ofnode_valid(phy_node)) {
-		enetc_dbg(dev, "invalid phy node, skipping PHY set-up\n");
-		return;
-	}
-	enetc_dbg(dev, "phy node: %s\n", ofnode_get_name(phy_node));
+	priv->phy = dm_eth_phy_connect(dev);
 
-	if (ofnode_read_u32(phy_node, "reg", &phy_id)) {
-		enetc_dbg(dev,
-			  "missing reg in PHY node, skipping PHY set-up\n");
+	if (!priv->phy)
 		return;
-	}
-
-	if (uclass_get_device_by_ofnode(UCLASS_MDIO,
-					ofnode_get_parent(phy_node),
-					&miidev)) {
-		enetc_dbg(dev, "can't find MDIO bus for node %s\n",
-			  ofnode_get_name(ofnode_get_parent(phy_node)));
-		return;
-	}
-
-	phy = dm_mdio_phy_connect(miidev, phy_id, dev, priv->if_type);
-	if (!phy) {
-		enetc_dbg(dev, "dm_mdio_phy_connect returned null\n");
-		return;
-	}
 
 	supported = GENMASK(6, 0); /* speeds up to 1G & AN */
-	phy->advertising = phy->supported & supported;
-	phy->node = phy_node;
-	phy_config(phy);
-	phy_startup(phy);
+	priv->phy->advertising = priv->phy->supported & supported;
+
+	phy_config(priv->phy);
 }
 
 /*
@@ -468,7 +431,9 @@ static int enetc_start(struct udevice *dev)
 	enetc_setup_rx_bdr(dev);
 
 	enetc_start_pcs(dev);
-	enetc_start_phy(dev);
+	enetc_config_phy(dev);
+	if (priv->phy)
+		phy_startup(priv->phy);
 
 	return 0;
 }
diff --git a/drivers/net/fsl_enetc.h b/drivers/net/fsl_enetc.h
index 0bb4cdff47..9a36cdad80 100644
--- a/drivers/net/fsl_enetc.h
+++ b/drivers/net/fsl_enetc.h
@@ -154,6 +154,7 @@ struct enetc_priv {
 
 	int if_type;
 	struct mii_dev imdio;
+	struct phy_device *phy;
 };
 
 /* register accessors */
-- 
2.17.1

[U-Boot] [PATCH v4 0/5] Add helper function for linking a DM Eth device to a PHY

[Michael Walle]

Hi,

sorry for being so late..

> We're going to try to get this binding accepted in Linux too.
shouldn't we try to get it accepted to linux first? To avoid any 
incompatibilities?


-michael

[U-Boot] [PATCH v4 0/5] Add helper function for linking a DM Eth device to a PHY

[Alexandru Marginean]

Hi Michael,

On 11/14/2019 6:41 PM, Michael Walle wrote:
> 
> Hi,
> 
> sorry for being so late..
> 
>> We're going to try to get this binding accepted in Linux too.
> shouldn't we try to get it accepted to linux first? To avoid any 
> incompatibilities?

I can propagate any changes kernel crows asks us to do here, that
shouldn't be too difficult.  Of course any feedback from the
U-Boot community is welcome too, in the meantime :)
Alex

> 
> 
> -michael
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v4 0/5] Add helper function for linking a DM Eth device to a PHY

[Alexandru Marginean]

On 11/14/2019 7:19 PM, Alexandru Marginean wrote:
> Hi Michael,
> 
> On 11/14/2019 6:41 PM, Michael Walle wrote:
>>
>> Hi,
>>
>> sorry for being so late..
>>
>>> We're going to try to get this binding accepted in Linux too.
>> shouldn't we try to get it accepted to linux first? To avoid any 
>> incompatibilities?
> 
> I can propagate any changes kernel crows asks us to do here, that

that was a weird typo.  I meant 'kernel crowd' :)

> shouldn't be too difficult.  Of course any feedback from the
> U-Boot community is welcome too, in the meantime :)
> Alex
> 
>>
>>
>> -michael
>> _______________________________________________
>> U-Boot mailing list
>> U-Boot at lists.denx.de
>> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH v4 4/5] doc: bindings: add mdio-handle property to ethernet nodes

[Grygorii Strashko]

On 14/11/2019 17:04, Alex Marginean wrote:
> Adds an optional mdio-handle property which identifies a MDIO bus which can
> be scanned to find the relevant PHY.  The property is ignored if phy-handle
> is also present.
> 
> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
> ---
>   doc/device-tree-bindings/net/ethernet.txt | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/doc/device-tree-bindings/net/ethernet.txt b/doc/device-tree-bindings/net/ethernet.txt
> index 3fc360523b..9f9629f8d6 100644
> --- a/doc/device-tree-bindings/net/ethernet.txt
> +++ b/doc/device-tree-bindings/net/ethernet.txt
> @@ -9,6 +9,8 @@ The following properties are common to the Ethernet controllers:
>   - max-speed: number, specifies maximum speed in Mbit/s supported by the device;
>   - max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
>     the maximum frame size (there's contradiction in ePAPR).
> +- mdio-handle: phandle, specifies a reference to a MDIO bus to be scanned to
> +  find the PHY device.  Ignored if phy-handle is also present.

Sry, but it looks redundant. The Ethernet-controller bindings
expects to use phy-handle which, in turn, allows to get MDIO node.

So, if your platform is DT based and can use DT then it's reasonable to follow standard binding,
which, in addition, allows to specify Ethernet PHY properties.
More over, your series does not provide user for this new property.

Personally I do not see even reasons to have doc/device-tree-bindings/net/ethernet.txt in u-boot
and think we should follow [1]

>   - phy-mode: string, operation mode of the PHY interface; supported values are
>     "mii", "gmii", "sgmii", "qsgmii", "tbi", "rev-mii", "rmii", "rgmii", "rgmii-id",
>     "rgmii-rxid", "rgmii-txid", "rtbi", "smii", "xgmii"; this is now a de-facto
> 

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-controller.yaml

-- 
Best regards,
grygorii

[U-Boot] [PATCH v4 4/5] doc: bindings: add mdio-handle property to ethernet nodes

[Alexandru Marginean]

On 11/18/2019 8:11 PM, Grygorii Strashko wrote:
> 
> 
> On 14/11/2019 17:04, Alex Marginean wrote:
>> Adds an optional mdio-handle property which identifies a MDIO bus 
>> which can
>> be scanned to find the relevant PHY.  The property is ignored if 
>> phy-handle
>> is also present.
>>
>> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
>> ---
>>   doc/device-tree-bindings/net/ethernet.txt | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/doc/device-tree-bindings/net/ethernet.txt 
>> b/doc/device-tree-bindings/net/ethernet.txt
>> index 3fc360523b..9f9629f8d6 100644
>> --- a/doc/device-tree-bindings/net/ethernet.txt
>> +++ b/doc/device-tree-bindings/net/ethernet.txt
>> @@ -9,6 +9,8 @@ The following properties are common to the Ethernet 
>> controllers:
>>   - max-speed: number, specifies maximum speed in Mbit/s supported by 
>> the device;
>>   - max-frame-size: number, maximum transfer unit (IEEE defined MTU), 
>> rather than
>>     the maximum frame size (there's contradiction in ePAPR).
>> +- mdio-handle: phandle, specifies a reference to a MDIO bus to be 
>> scanned to
>> +  find the PHY device.  Ignored if phy-handle is also present.
> 
> Sry, but it looks redundant. The Ethernet-controller bindings
> expects to use phy-handle which, in turn, allows to get MDIO node.

The problem I'm trying to solve is not that I don't want to get the
parent MDIO bus of a PHY referenced though phy-handle, but rather that I
want to avoid having a specific U-Boot DT for each PHY plug-in card that
could be used in our systems.  I'll explain that in more detail.

We have these QDS systems which support plug-in cards with PHYs on them.
The way it works is that a given ethernet interface is associated with
one of the QDS slots that should contain a plug-in card.  Each of the
slots has a MDIO bus associated with it, this bus is accessed through a
MDIO MUX on the QDS board.
The slot may be populated with one of several types of cards, which
are different designs, have different types of PHYs and more importantly
use different PHY addresses on the MDIO bus.

So now the summary is that an Ethernet interface is associated with a
MDIO bus that could be scanned to find the relevant PHY, while using the
existing phy-handle binding requires that U-Boot DT is edited whenever
a different card is plugged in, which is not practical and avoidable.

> 
> So, if your platform is DT based and can use DT then it's reasonable to 
> follow standard binding,
> which, in addition, allows to specify Ethernet PHY properties.

Sure, and we do that on boards and for PHYs which are at fixed addresses
on MDIO bus, but that is impractical with the swappable cards.

> More over, your series does not provide user for this new property.

That's true, I was planning to send the QDS DT after this was merged,
but if it helps I can add it to this patch set.

Alex

> Personally I do not see even reasons to have 
> doc/device-tree-bindings/net/ethernet.txt in u-boot
> and think we should follow [1]
> 
>>   - phy-mode: string, operation mode of the PHY interface; supported 
>> values are
>>     "mii", "gmii", "sgmii", "qsgmii", "tbi", "rev-mii", "rmii", 
>> "rgmii", "rgmii-id",
>>     "rgmii-rxid", "rgmii-txid", "rtbi", "smii", "xgmii"; this is now a 
>> de-facto
>>
> 
> [1] 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-controller.yaml 
> 
> 

[U-Boot] [PATCH v4 4/5] doc: bindings: add mdio-handle property to ethernet nodes

[Grygorii Strashko]

On 19/11/2019 01:31, Alexandru Marginean wrote:
> 
> On 11/18/2019 8:11 PM, Grygorii Strashko wrote:
>>
>>
>> On 14/11/2019 17:04, Alex Marginean wrote:
>>> Adds an optional mdio-handle property which identifies a MDIO bus which can
>>> be scanned to find the relevant PHY.  The property is ignored if phy-handle
>>> is also present.
>>>
>>> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
>>> ---
>>>   doc/device-tree-bindings/net/ethernet.txt | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/doc/device-tree-bindings/net/ethernet.txt b/doc/device-tree-bindings/net/ethernet.txt
>>> index 3fc360523b..9f9629f8d6 100644
>>> --- a/doc/device-tree-bindings/net/ethernet.txt
>>> +++ b/doc/device-tree-bindings/net/ethernet.txt
>>> @@ -9,6 +9,8 @@ The following properties are common to the Ethernet controllers:
>>>   - max-speed: number, specifies maximum speed in Mbit/s supported by the device;
>>>   - max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
>>>     the maximum frame size (there's contradiction in ePAPR).
>>> +- mdio-handle: phandle, specifies a reference to a MDIO bus to be scanned to
>>> +  find the PHY device.  Ignored if phy-handle is also present.
>>
>> Sry, but it looks redundant. The Ethernet-controller bindings
>> expects to use phy-handle which, in turn, allows to get MDIO node.
> 
> The problem I'm trying to solve is not that I don't want to get the
> parent MDIO bus of a PHY referenced though phy-handle, but rather that I
> want to avoid having a specific U-Boot DT for each PHY plug-in card that
> could be used in our systems.  I'll explain that in more detail.
> 
> We have these QDS systems which support plug-in cards with PHYs on them.
> The way it works is that a given ethernet interface is associated with
> one of the QDS slots that should contain a plug-in card.  Each of the
> slots has a MDIO bus associated with it, this bus is accessed through a
> MDIO MUX on the QDS board.
> The slot may be populated with one of several types of cards, which
> are different designs, have different types of PHYs and more importantly
> use different PHY addresses on the MDIO bus.
> 
> So now the summary is that an Ethernet interface is associated with a
> MDIO bus that could be scanned to find the relevant PHY, while using the
> existing phy-handle binding requires that U-Boot DT is edited whenever
> a different card is plugged in, which is not practical and avoidable.

Thank you for explanation. It's clear now.
I think above description is good to have as part of commit message.

> 
>>
>> So, if your platform is DT based and can use DT then it's reasonable to follow standard binding,
>> which, in addition, allows to specify Ethernet PHY properties.
> 
> Sure, and we do that on boards and for PHYs which are at fixed addresses
> on MDIO bus, but that is impractical with the swappable cards.
> 
>> More over, your series does not provide user for this new property.
> 
> That's true, I was planning to send the QDS DT after this was merged,
> but if it helps I can add it to this patch set.
> 
> Alex
> 
>> Personally I do not see even reasons to have doc/device-tree-bindings/net/ethernet.txt in u-boot
>> and think we should follow [1]
>>
>>>   - phy-mode: string, operation mode of the PHY interface; supported values are
>>>     "mii", "gmii", "sgmii", "qsgmii", "tbi", "rev-mii", "rmii", "rgmii", "rgmii-id",
>>>     "rgmii-rxid", "rgmii-txid", "rtbi", "smii", "xgmii"; this is now a de-facto
>>>
>>
>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/net/ethernet-controller.yaml
>>

-- 
Best regards,
grygorii

[U-Boot] [PATCH v4 4/5] doc: bindings: add mdio-handle property to ethernet nodes

[Grygorii Strashko]

On 19/11/2019 20:58, Grygorii Strashko wrote:
> 
> 
> On 19/11/2019 01:31, Alexandru Marginean wrote:
>>
>> On 11/18/2019 8:11 PM, Grygorii Strashko wrote:
>>>
>>>
>>> On 14/11/2019 17:04, Alex Marginean wrote:
>>>> Adds an optional mdio-handle property which identifies a MDIO bus which can
>>>> be scanned to find the relevant PHY.  The property is ignored if phy-handle
>>>> is also present.
>>>>
>>>> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
>>>> ---
>>>>   doc/device-tree-bindings/net/ethernet.txt | 2 ++
>>>>   1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/doc/device-tree-bindings/net/ethernet.txt b/doc/device-tree-bindings/net/ethernet.txt
>>>> index 3fc360523b..9f9629f8d6 100644
>>>> --- a/doc/device-tree-bindings/net/ethernet.txt
>>>> +++ b/doc/device-tree-bindings/net/ethernet.txt
>>>> @@ -9,6 +9,8 @@ The following properties are common to the Ethernet controllers:
>>>>   - max-speed: number, specifies maximum speed in Mbit/s supported by the device;
>>>>   - max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
>>>>     the maximum frame size (there's contradiction in ePAPR).
>>>> +- mdio-handle: phandle, specifies a reference to a MDIO bus to be scanned to
>>>> +  find the PHY device.  Ignored if phy-handle is also present.
>>>
>>> Sry, but it looks redundant. The Ethernet-controller bindings
>>> expects to use phy-handle which, in turn, allows to get MDIO node.
>>
>> The problem I'm trying to solve is not that I don't want to get the
>> parent MDIO bus of a PHY referenced though phy-handle, but rather that I
>> want to avoid having a specific U-Boot DT for each PHY plug-in card that
>> could be used in our systems.  I'll explain that in more detail.
>>
>> We have these QDS systems which support plug-in cards with PHYs on them.
>> The way it works is that a given ethernet interface is associated with
>> one of the QDS slots that should contain a plug-in card.  Each of the
>> slots has a MDIO bus associated with it, this bus is accessed through a
>> MDIO MUX on the QDS board.
>> The slot may be populated with one of several types of cards, which
>> are different designs, have different types of PHYs and more importantly
>> use different PHY addresses on the MDIO bus.
>>
>> So now the summary is that an Ethernet interface is associated with a
>> MDIO bus that could be scanned to find the relevant PHY, while using the
>> existing phy-handle binding requires that U-Boot DT is edited whenever
>> a different card is plugged in, which is not practical and avoidable.
> 
> Thank you for explanation. It's clear now.
> I think above description is good to have as part of commit message.
> 
>>
>>>
>>> So, if your platform is DT based and can use DT then it's reasonable to follow standard binding,
>>> which, in addition, allows to specify Ethernet PHY properties.
>>
>> Sure, and we do that on boards and for PHYs which are at fixed addresses
>> on MDIO bus, but that is impractical with the swappable cards.
>>

Some thought, which i think might help.
- u-boot allows phy node not to have "reg" property.
- phy_connect() will return first PHY discovered on the MDIO bus if addr<=0

So, if MDIO assignment per ethernet interface/slot is fixed DT can look like

mdioX {
	mdiox_phy0: ethernet-phy@0 {
	};
}

ethernetX {
	phy-handle = <&mdiox_phy0>;
}

and so on. driver's parsing code can ignore PHY "reg" prop in phy node and pass
addr<=0 to phy_connect().

It seems, current Linux approach is to have PHY "reg" property as required, but
your case might be the reason to change this.

-- 
Best regards,
grygorii

[U-Boot] [PATCH v4 4/5] doc: bindings: add mdio-handle property to ethernet nodes

[Alexandru Marginean]

On 11/20/2019 10:51 AM, Grygorii Strashko wrote:
> 
> 
> On 19/11/2019 20:58, Grygorii Strashko wrote:
>>
>>
>> On 19/11/2019 01:31, Alexandru Marginean wrote:
>>>
>>> On 11/18/2019 8:11 PM, Grygorii Strashko wrote:
>>>>
>>>>
>>>> On 14/11/2019 17:04, Alex Marginean wrote:
>>>>> Adds an optional mdio-handle property which identifies a MDIO bus 
>>>>> which can
>>>>> be scanned to find the relevant PHY.  The property is ignored if 
>>>>> phy-handle
>>>>> is also present.
>>>>>
>>>>> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
>>>>> ---
>>>>>   doc/device-tree-bindings/net/ethernet.txt | 2 ++
>>>>>   1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/doc/device-tree-bindings/net/ethernet.txt 
>>>>> b/doc/device-tree-bindings/net/ethernet.txt
>>>>> index 3fc360523b..9f9629f8d6 100644
>>>>> --- a/doc/device-tree-bindings/net/ethernet.txt
>>>>> +++ b/doc/device-tree-bindings/net/ethernet.txt
>>>>> @@ -9,6 +9,8 @@ The following properties are common to the Ethernet 
>>>>> controllers:
>>>>>   - max-speed: number, specifies maximum speed in Mbit/s supported 
>>>>> by the device;
>>>>>   - max-frame-size: number, maximum transfer unit (IEEE defined 
>>>>> MTU), rather than
>>>>>     the maximum frame size (there's contradiction in ePAPR).
>>>>> +- mdio-handle: phandle, specifies a reference to a MDIO bus to be 
>>>>> scanned to
>>>>> +  find the PHY device.  Ignored if phy-handle is also present.
>>>>
>>>> Sry, but it looks redundant. The Ethernet-controller bindings
>>>> expects to use phy-handle which, in turn, allows to get MDIO node.
>>>
>>> The problem I'm trying to solve is not that I don't want to get the
>>> parent MDIO bus of a PHY referenced though phy-handle, but rather that I
>>> want to avoid having a specific U-Boot DT for each PHY plug-in card that
>>> could be used in our systems.  I'll explain that in more detail.
>>>
>>> We have these QDS systems which support plug-in cards with PHYs on them.
>>> The way it works is that a given ethernet interface is associated with
>>> one of the QDS slots that should contain a plug-in card.  Each of the
>>> slots has a MDIO bus associated with it, this bus is accessed through a
>>> MDIO MUX on the QDS board.
>>> The slot may be populated with one of several types of cards, which
>>> are different designs, have different types of PHYs and more importantly
>>> use different PHY addresses on the MDIO bus.
>>>
>>> So now the summary is that an Ethernet interface is associated with a
>>> MDIO bus that could be scanned to find the relevant PHY, while using the
>>> existing phy-handle binding requires that U-Boot DT is edited whenever
>>> a different card is plugged in, which is not practical and avoidable.
>>
>> Thank you for explanation. It's clear now.
>> I think above description is good to have as part of commit message.
>>
>>>
>>>>
>>>> So, if your platform is DT based and can use DT then it's reasonable 
>>>> to follow standard binding,
>>>> which, in addition, allows to specify Ethernet PHY properties.
>>>
>>> Sure, and we do that on boards and for PHYs which are at fixed addresses
>>> on MDIO bus, but that is impractical with the swappable cards.
>>>
> 
> Some thought, which i think might help.
> - u-boot allows phy node not to have "reg" property.
> - phy_connect() will return first PHY discovered on the MDIO bus if addr<=0
> 
> So, if MDIO assignment per ethernet interface/slot is fixed DT can look 
> like
> 
> mdioX {
>      mdiox_phy0: ethernet-phy at 0 {
>      };
> }
> 
> ethernetX {
>      phy-handle = <&mdiox_phy0>;
> }
> 
> and so on. driver's parsing code can ignore PHY "reg" prop in phy
> node and pass addr<=0 to phy_connect().
Functionally that's what I'm looking for, yes.  There is the problem of
not strictly following the kernel binding and@the end of the day I
will have the same problem with the kernel too, so I should take this
topic to the netdev list anyway.

Having 'reg' made optional is actually an interesting idea.  I think
'mdio-handle' is a bit more clear what it is, but having an actual PHY
node allows passing DT properties on to the PHY driver which is
certainly useful for some PHYs.  It's like saying I don't know what PHY
I'm going to find, but if it's a PHY that has these configurable
properties here is your configuration.

> 
> It seems, current Linux approach is to have PHY "reg" property as 
> required, but your case might be the reason to change this. >
I'll have to do a better write-up for the kernel list and we'll see
how this will go.
In the meantime any feedback on the other patches, except the one
introducing mdio-handle?  For instance I should deal with fixed links
in the dm_eth_phy_connect helper too (calling phy_connect_fixed),
my intent is to take some code that is otherwise pretty generic out of
the drivers.

Thanks!
Alex

[U-Boot] [PATCH v4 4/5] doc: bindings: add mdio-handle property to ethernet nodes

[Grygorii Strashko]

On 21/11/2019 12:36, Alexandru Marginean wrote:
> On 11/20/2019 10:51 AM, Grygorii Strashko wrote:
>>
>>
>> On 19/11/2019 20:58, Grygorii Strashko wrote:
>>>
>>>
>>> On 19/11/2019 01:31, Alexandru Marginean wrote:
>>>>
>>>> On 11/18/2019 8:11 PM, Grygorii Strashko wrote:
>>>>>
>>>>>
>>>>> On 14/11/2019 17:04, Alex Marginean wrote:
>>>>>> Adds an optional mdio-handle property which identifies a MDIO bus which can
>>>>>> be scanned to find the relevant PHY.  The property is ignored if phy-handle
>>>>>> is also present.
>>>>>>
>>>>>> Signed-off-by: Alex Marginean <alexandru.marginean@nxp.com>
>>>>>> ---
>>>>>>   doc/device-tree-bindings/net/ethernet.txt | 2 ++
>>>>>>   1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/doc/device-tree-bindings/net/ethernet.txt b/doc/device-tree-bindings/net/ethernet.txt
>>>>>> index 3fc360523b..9f9629f8d6 100644
>>>>>> --- a/doc/device-tree-bindings/net/ethernet.txt
>>>>>> +++ b/doc/device-tree-bindings/net/ethernet.txt
>>>>>> @@ -9,6 +9,8 @@ The following properties are common to the Ethernet controllers:
>>>>>>   - max-speed: number, specifies maximum speed in Mbit/s supported by the device;
>>>>>>   - max-frame-size: number, maximum transfer unit (IEEE defined MTU), rather than
>>>>>>     the maximum frame size (there's contradiction in ePAPR).
>>>>>> +- mdio-handle: phandle, specifies a reference to a MDIO bus to be scanned to
>>>>>> +  find the PHY device.  Ignored if phy-handle is also present.
>>>>>
>>>>> Sry, but it looks redundant. The Ethernet-controller bindings
>>>>> expects to use phy-handle which, in turn, allows to get MDIO node.
>>>>
>>>> The problem I'm trying to solve is not that I don't want to get the
>>>> parent MDIO bus of a PHY referenced though phy-handle, but rather that I
>>>> want to avoid having a specific U-Boot DT for each PHY plug-in card that
>>>> could be used in our systems.  I'll explain that in more detail.
>>>>
>>>> We have these QDS systems which support plug-in cards with PHYs on them.
>>>> The way it works is that a given ethernet interface is associated with
>>>> one of the QDS slots that should contain a plug-in card.  Each of the
>>>> slots has a MDIO bus associated with it, this bus is accessed through a
>>>> MDIO MUX on the QDS board.
>>>> The slot may be populated with one of several types of cards, which
>>>> are different designs, have different types of PHYs and more importantly
>>>> use different PHY addresses on the MDIO bus.
>>>>
>>>> So now the summary is that an Ethernet interface is associated with a
>>>> MDIO bus that could be scanned to find the relevant PHY, while using the
>>>> existing phy-handle binding requires that U-Boot DT is edited whenever
>>>> a different card is plugged in, which is not practical and avoidable.
>>>
>>> Thank you for explanation. It's clear now.
>>> I think above description is good to have as part of commit message.
>>>
>>>>
>>>>>
>>>>> So, if your platform is DT based and can use DT then it's reasonable to follow standard binding,
>>>>> which, in addition, allows to specify Ethernet PHY properties.
>>>>
>>>> Sure, and we do that on boards and for PHYs which are at fixed addresses
>>>> on MDIO bus, but that is impractical with the swappable cards.
>>>>
>>
>> Some thought, which i think might help.
>> - u-boot allows phy node not to have "reg" property.
>> - phy_connect() will return first PHY discovered on the MDIO bus if addr<=0
>>
>> So, if MDIO assignment per ethernet interface/slot is fixed DT can look like
>>
>> mdioX {
>>      mdiox_phy0: ethernet-phy at 0 {
>>      };
>> }
>>
>> ethernetX {
>>      phy-handle = <&mdiox_phy0>;
>> }
>>
>> and so on. driver's parsing code can ignore PHY "reg" prop in phy
>> node and pass addr<=0 to phy_connect().
> Functionally that's what I'm looking for, yes.  There is the problem of
> not strictly following the kernel binding and at the end of the day I
> will have the same problem with the kernel too, so I should take this
> topic to the netdev list anyway.
> 
> Having 'reg' made optional is actually an interesting idea.  I think
> 'mdio-handle' is a bit more clear what it is, but having an actual PHY
> node allows passing DT properties on to the PHY driver which is
> certainly useful for some PHYs.  It's like saying I don't know what PHY
> I'm going to find, but if it's a PHY that has these configurable
> properties here is your configuration.

Another good question is "phy-connection-type"/"phy-mode".
Are all your cards works in the same mode? And how is this solved?

Is there anything common with SFP? Linux: bindings/net/sff,sfp.txt

> 
>>
>> It seems, current Linux approach is to have PHY "reg" property as required, but your case might be the reason to change this. >
> I'll have to do a better write-up for the kernel list and we'll see
> how this will go.
> In the meantime any feedback on the other patches, except the one
> introducing mdio-handle?  For instance I should deal with fixed links
> in the dm_eth_phy_connect helper too (calling phy_connect_fixed),
> my intent is to take some code that is otherwise pretty generic out of
> the drivers.

 From one side, it look nice. From another, in my case MDIO is not a device,
so can't try it.

But, any way, I'd like to share some notes I have. It may help you or may
save your time by reducing number of possible regressions.

To be honest, there is a little mess in PHY creation area :(
Common way is to do in drivers:
phy_connect(mii_bus, phy_addr, dev, interface)
  |-phy_find_by_mask()
      |-get_phy_device_by_mask()
	|-create_phy_by_mask()
	  |-create_phy_by_mask()
             |-phy_device_create()
		|-phy_probe()
  |-phy_connect_dev(phydev, dev);
..
#ifdef CONFIG_DM_ETH
	if (ofnode_valid(slave->data->phy_of_handle))
		phydev->node = slave->data->phy_of_handle;
#endif

As you can see from above - the PHY probe will be called when
both phy_device->dev and phydev->node have not been initialized yet,
so no way to perform DT parsing in PHY .probe().

Another, not obvious, thing is that DT node for fixed phy passed
through the int addr parameter in create_phy_by_mask().

And finally, as phy as fixed-phy may be connected to DT node which is
*not" ethernet device node and describes Port. The ethernet device is parent DT node
in this case.

In my opinion, it could really simplify things if phy_device->dev and phydev->node will
be configured before calling PHY .probe(), but the challenge is to make it work
for !DT case.

-- 
Best regards,
grygorii

[U-Boot] [PATCH v4 4/5] doc: bindings: add mdio-handle property to ethernet nodes

[Alexandru Marginean]

On 11/21/2019 1:30 PM, Grygorii Strashko wrote:
> 
>>>
>>> Some thought, which i think might help.
>>> - u-boot allows phy node not to have "reg" property.
>>> - phy_connect() will return first PHY discovered on the MDIO bus if 
>>> addr<=0
>>>
>>> So, if MDIO assignment per ethernet interface/slot is fixed DT can 
>>> look like
>>>
>>> mdioX {
>>>      mdiox_phy0: ethernet-phy at 0 {
>>>      };
>>> }
>>>
>>> ethernetX {
>>>      phy-handle = <&mdiox_phy0>;
>>> }
>>>
>>> and so on. driver's parsing code can ignore PHY "reg" prop in phy
>>> node and pass addr<=0 to phy_connect().
>> Functionally that's what I'm looking for, yes.  There is the problem of
>> not strictly following the kernel binding and at the end of the day I
>> will have the same problem with the kernel too, so I should take this
>> topic to the netdev list anyway.
>>
>> Having 'reg' made optional is actually an interesting idea.  I think
>> 'mdio-handle' is a bit more clear what it is, but having an actual PHY
>> node allows passing DT properties on to the PHY driver which is
>> certainly useful for some PHYs.  It's like saying I don't know what PHY
>> I'm going to find, but if it's a PHY that has these configurable
>> properties here is your configuration.
> 
> Another good question is "phy-connection-type"/"phy-mode".
> Are all your cards works in the same mode? And how is this solved?

These SoCs generally support multiple protocols on the serdes lanes.
The protocol is selected at power on so just using a single static
DT isn't sufficient.

For Linux DT I tried the DT overlay which seems to work fine.  I'm
using it to fix up phy-connection-type based on serdes configuration
that is known to u-boot.  Of course it's possible to apply an
overlay for each specific plug-in card in a given slot too, but that's
not practical especially for mixes of board with many slots combined
with many types of cards.  I'm not sure this can be done automatically
in U-Boot such that the user doesn't have to do any manual configuration
when swapping cards either.

U-Boot doesn't have the luxury of having overlays applied before it
starts, we don't use SPL.  I'm considering ways to have platform init
code apply some sort of fix-up or overlay for U-Boot DT based on serdes
configuration, but I don't have something running yet.  Live tree is
something I want to look into to see if it would work here.

> Is there anything common with SFP? Linux: bindings/net/sff,sfp.txt

They are not SFPs, they are normal MDIO driven PHYs just that they are
mounted on plug-in cards.


>>> It seems, current Linux approach is to have PHY "reg" property as 
>>> required, but your case might be the reason to change this. >
>> I'll have to do a better write-up for the kernel list and we'll see
>> how this will go.
>> In the meantime any feedback on the other patches, except the one
>> introducing mdio-handle?  For instance I should deal with fixed links
>> in the dm_eth_phy_connect helper too (calling phy_connect_fixed),
>> my intent is to take some code that is otherwise pretty generic out of
>> the drivers.
> 
> From one side, it look nice. From another, in my case MDIO is not a 
> device, so can't try it.
> 
> But, any way, I'd like to share some notes I have. It may help you or may
> save your time by reducing number of possible regressions.
> 
> To be honest, there is a little mess in PHY creation area :(
> Common way is to do in drivers:
> phy_connect(mii_bus, phy_addr, dev, interface)
>   |-phy_find_by_mask()
>       |-get_phy_device_by_mask()
>      |-create_phy_by_mask()
>        |-create_phy_by_mask()
>              |-phy_device_create()
>          |-phy_probe()
>   |-phy_connect_dev(phydev, dev);
> ..
> #ifdef CONFIG_DM_ETH
>      if (ofnode_valid(slave->data->phy_of_handle))
>          phydev->node = slave->data->phy_of_handle;
> #endif
> 
> As you can see from above - the PHY probe will be called when
> both phy_device->dev and phydev->node have not been initialized yet,
> so no way to perform DT parsing in PHY .probe().

That is certainly inconsistent with the way other kinds of devices work.
What I did for DT based configuration on aquantia PHYs was to read the
DT properties in _config.  Just from a practical stand-point that's
fine, phydev->node is presumably set up by now and ethernet drivers have
to call _config anyway and that gives the PHY driver a chance to check
the DT properties.  But conceptually it is messy, it makes phy drivers
special.  I suppose the solution is to make phy drivers behave like
the other udevices, either actually making them udevices or at least
introducing phy_bind ahead of probe.

On the current code, do you see any issues with dealing with the
PHY DT properties in _config, other than this inconsistency with the
other devices?

> Another, not obvious, thing is that DT node for fixed phy passed
> through the int addr parameter in create_phy_by_mask().

This is a work-around for the previous issue I assume.

> And finally, as phy as fixed-phy may be connected to DT node which is
> *not" ethernet device node and describes Port. The ethernet device is 
> parent DT node in this case.

Are these switch ports or on a multi-port Ethernet card?  If that's the
case I would say the better solution is to actually drive ports as
ethernet devices in that case, rather than driving the switch as a whole
as an Ethernet interface.  I realize this approach may not work in all
cases, for various reasons, but then it's a matter of drivers that are
different not getting the benefit of using high level helper functions
that don't work for them.  At least we should make sure such drivers can
set things up properly if they are using custom binding or no DT at all.
On the switch topic I just sent some patches to support DSA switches
and the code drives ports as ethernet devices.  Perhaps the code is
not applicable to your device but I'd like to hear whether it makes
sense at the concept level.

> In my opinion, it could really simplify things if phy_device->dev and
>  phydev->node will be configured before calling PHY .probe(), but the
> challenge is to make it work for !DT case.
I think the nice way to fix that would be to introduce a bind function
for phy drivers, I could try to look into that after I free up my todo
list a little bit.

Thanks!
Alex

[U-Boot] [PATCH v4 0/5] Add helper function for linking a DM Eth device to a PHY

[Michael Walle]

Am 2019-11-14 16:04, schrieb Alex Marginean:
> The patch set introduces dm_eth_phy_connect which takes in an ethernet 
> device
> and uses DT information to find the associated PHY and connect the 
> Ethernet
> interface to it.  This should simplify similar code in ethernet 
> drivers.
> A new binding is introduced for scanning an MDIO bus, useful if the bus 
> is
> private to the Ethernet interface.  We're going to try to get this 
> binding
> accepted in Linux too.
> Finally the patch set updates fsl_enetc driver to use the new helper 
> function.
> 
> 
> This patch set supersedes v2 series:
> https://patchwork.ozlabs.org/project/uboot/list/?series=124537
> and v3 series:
> https://patchwork.ozlabs.org/project/uboot/list/?series=140114
> 
> Changes in v4:
>  - rebased on current head
> 
> Changes in v3:
>  - added cover letter
>  - check for null PHY pointer before using it in 
> dm_eth_connect_phy_handle
>  - moved the code dealing with MDIO scanning into a separate patch
>  - renames several arguments and variables for a bit more clarity and
>    consistency
> 
> Changes in v2:
> - Moved MDIO scan code into dm_mdio_phy_scan which is also exported
> - Use interface instead of if_type for consistency
> - don't use phy pointer if NULL in fsl_enetc code
> 
> Alex Marginean (5):
>   net: mdio-uclass: rename arguments of dm_mdio_phy_connect for
>     consistency
>   net: mdio-uclass: add dm_eth_phy_connect helper function
>   net: mdio-uclass: Add helper functions for scanning the MDIO bus
>   doc: bindings: add mdio-handle property to ethernet nodes
>   drivers: net: fsl_enetc: use the new MDIO DM helper functions
> 
>  doc/device-tree-bindings/net/ethernet.txt |   2 +
>  drivers/net/fsl_enetc.c                   |  52 ++------
>  drivers/net/fsl_enetc.h                   |   1 +
>  include/miiphy.h                          |  32 ++++-
>  net/mdio-uclass.c                         | 139 +++++++++++++++++++++-
>  5 files changed, 175 insertions(+), 51 deletions(-)

For this series:
Tested-by: Michael Walle <michael@walle.cc>