[PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Madalin Bucur]

The FMan MAC configuration code needs the speed and duplex information
for fixed-link interfaces that is parsed now by the of function
of_phy_register_fixed_link(). This parses the fixed-link parameters but
does not expose to the caller neither the phy_device pointer nor the
status struct where it loads the fixed-link params. By extracting the
fixed-link parsing code from of_phy_register_fixed_link() into a
separate function the parsed values are made available without changing
the existing API. This change also removes a small redundancy in the
previous code calling fixed_phy_register().

The FMan patch relies on the latest FMan driver v4 submission by Igal Liberman:
https://patchwork.ozlabs.org/project/netdev/list/?submitter=Igal.Liberman&state=*&q=v4

Madalin Bucur (2):
  of: separate fixed link parsing from registration
  fsl_fman: use fixed_phy_status for MEMAC

 .../ethernet/freescale/fman/flib/fsl_fman_memac.h  |  6 ++-
 drivers/net/ethernet/freescale/fman/inc/mac.h      |  2 +-
 drivers/net/ethernet/freescale/fman/mac/fm_memac.c | 42 ++++++++++++-----
 drivers/net/ethernet/freescale/fman/mac/fm_memac.h |  3 +-
 drivers/net/ethernet/freescale/fman/mac/mac.c      | 18 ++++++--
 drivers/of/of_mdio.c                               | 52 ++++++++++++++--------
 include/linux/of_mdio.h                            |  9 ++++
 7 files changed, 94 insertions(+), 38 deletions(-)

-- 
1.7.11.7

[PATCH RFC 1/2] of: separate fixed link parsing from registration

[Madalin Bucur]

Some drivers may need to parse the fixed link values before registering
the fixed link phy or access the status values. Separate the parsing from
the actual registration and provide an export for the added parsing function.

Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
---
 drivers/of/of_mdio.c    | 52 +++++++++++++++++++++++++++++++------------------
 include/linux/of_mdio.h |  9 +++++++++
 2 files changed, 42 insertions(+), 19 deletions(-)

diff --git a/drivers/of/of_mdio.c b/drivers/of/of_mdio.c
index fdc60db..b7e8288 100644
--- a/drivers/of/of_mdio.c
+++ b/drivers/of/of_mdio.c
@@ -284,43 +284,57 @@ bool of_phy_is_fixed_link(struct device_node *np)
 }
 EXPORT_SYMBOL(of_phy_is_fixed_link);
 
-int of_phy_register_fixed_link(struct device_node *np)
+int of_phy_parse_fixed_link(struct device_node *np,
+			    struct fixed_phy_status *status)
 {
-	struct fixed_phy_status status = {};
 	struct device_node *fixed_link_node;
 	const __be32 *fixed_link_prop;
 	int len;
-	struct phy_device *phy;
 
 	/* New binding */
 	fixed_link_node = of_get_child_by_name(np, "fixed-link");
 	if (fixed_link_node) {
-		status.link = 1;
-		status.duplex = of_property_read_bool(fixed_link_node,
-						      "full-duplex");
-		if (of_property_read_u32(fixed_link_node, "speed", &status.speed))
+		status->link = 1;
+		status->duplex = of_property_read_bool(fixed_link_node,
+						       "full-duplex");
+		if (of_property_read_u32(fixed_link_node, "speed",
+					 &status->speed))
 			return -EINVAL;
-		status.pause = of_property_read_bool(fixed_link_node, "pause");
-		status.asym_pause = of_property_read_bool(fixed_link_node,
-							  "asym-pause");
+		status->pause = of_property_read_bool(fixed_link_node,
+						      "pause");
+		status->asym_pause = of_property_read_bool(fixed_link_node,
+							   "asym-pause");
 		of_node_put(fixed_link_node);
-		phy = fixed_phy_register(PHY_POLL, &status, np);
-		return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+
+		return 0;
 	}
 
 	/* Old binding */
 	fixed_link_prop = of_get_property(np, "fixed-link", &len);
 	if (fixed_link_prop && len == (5 * sizeof(__be32))) {
-		status.link = 1;
-		status.duplex = be32_to_cpu(fixed_link_prop[1]);
-		status.speed = be32_to_cpu(fixed_link_prop[2]);
-		status.pause = be32_to_cpu(fixed_link_prop[3]);
-		status.asym_pause = be32_to_cpu(fixed_link_prop[4]);
-		phy = fixed_phy_register(PHY_POLL, &status, np);
-		return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+		status->link = 1;
+		status->duplex = be32_to_cpu(fixed_link_prop[1]);
+		status->speed = be32_to_cpu(fixed_link_prop[2]);
+		status->pause = be32_to_cpu(fixed_link_prop[3]);
+		status->asym_pause = be32_to_cpu(fixed_link_prop[4]);
+
+		return 0;
 	}
 
 	return -ENODEV;
 }
+EXPORT_SYMBOL(of_phy_parse_fixed_link);
+
+int of_phy_register_fixed_link(struct device_node *np)
+{
+	struct phy_device *phy;
+	struct fixed_phy_status status = {};
+
+	if (of_phy_parse_fixed_link(np, &status))
+		return -ENODEV;
+
+	phy = fixed_phy_register(PHY_POLL, &status, np);
+	return IS_ERR(phy) ? PTR_ERR(phy) : 0;
+}
 EXPORT_SYMBOL(of_phy_register_fixed_link);
 #endif
diff --git a/include/linux/of_mdio.h b/include/linux/of_mdio.h
index 8f2237e..311b2cf 100644
--- a/include/linux/of_mdio.h
+++ b/include/linux/of_mdio.h
@@ -12,6 +12,8 @@
 #include <linux/phy.h>
 #include <linux/of.h>
 
+struct fixed_phy_status;
+
 #ifdef CONFIG_OF
 extern int of_mdiobus_register(struct mii_bus *mdio, struct device_node *np);
 extern struct phy_device *of_phy_find_device(struct device_node *phy_np);
@@ -70,9 +72,16 @@ static inline int of_mdio_parse_addr(struct device *dev,
 #endif /* CONFIG_OF */
 
 #if defined(CONFIG_OF) && defined(CONFIG_FIXED_PHY)
+extern int of_phy_parse_fixed_link(struct device_node *np,
+				   struct fixed_phy_status *status);
 extern int of_phy_register_fixed_link(struct device_node *np);
 extern bool of_phy_is_fixed_link(struct device_node *np);
 #else
+static inline int of_phy_parse_fixed_link(struct device_node *np,
+					  struct fixed_phy_status *status)
+{
+	return -EINVAL;
+}
 static inline int of_phy_register_fixed_link(struct device_node *np)
 {
 	return -ENOSYS;
-- 
1.7.11.7

[PATCH RFC 2/2] fsl_fman: use fixed_phy_status for MEMAC

[Madalin Bucur]

Use the speed and duplex information from the device tree fixed link
node accessing the status structure parsed by of_phy_parse_fixed_link().

Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
---
 .../ethernet/freescale/fman/flib/fsl_fman_memac.h  |  6 ++--
 drivers/net/ethernet/freescale/fman/inc/mac.h      |  2 +-
 drivers/net/ethernet/freescale/fman/mac/fm_memac.c | 42 ++++++++++++++++------
 drivers/net/ethernet/freescale/fman/mac/fm_memac.h |  3 +-
 drivers/net/ethernet/freescale/fman/mac/mac.c      | 18 +++++++---
 5 files changed, 52 insertions(+), 19 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/flib/fsl_fman_memac.h b/drivers/net/ethernet/freescale/fman/flib/fsl_fman_memac.h
index ebf7989..50bed14 100644
--- a/drivers/net/ethernet/freescale/fman/flib/fsl_fman_memac.h
+++ b/drivers/net/ethernet/freescale/fman/flib/fsl_fman_memac.h
@@ -33,8 +33,10 @@
 #define __FSL_FMAN_MEMAC_H
 
 #include <linux/io.h>
-
+#include <linux/netdevice.h>
+#include <linux/phy_fixed.h>
 #include "fsl_enet.h"
+
 /* Num of additional exact match MAC adr regs */
 #define MEMAC_NUM_OF_PADDRS 7
 
@@ -373,7 +375,7 @@ struct memac_cfg {
 	bool tx_pbl_fwd;
 	bool debug_mode;
 	bool wake_on_lan;
-	bool fixed_link;
+	struct fixed_phy_status *fixed_link;
 	u16 max_frame_length;
 	u16 pause_quanta;
 	u32 tx_ipg_length;
diff --git a/drivers/net/ethernet/freescale/fman/inc/mac.h b/drivers/net/ethernet/freescale/fman/inc/mac.h
index f86d0bc..fbeb957 100644
--- a/drivers/net/ethernet/freescale/fman/inc/mac.h
+++ b/drivers/net/ethernet/freescale/fman/inc/mac.h
@@ -62,7 +62,7 @@ struct mac_device {
 	phy_interface_t		 phy_if;
 	u32			 if_support;
 	bool			 link;
-	bool			 fixed_link;
+	struct fixed_phy_status	*fixed_link;
 	u16		 speed;
 	u16		 max_speed;
 	struct device_node	*phy_node;
diff --git a/drivers/net/ethernet/freescale/fman/mac/fm_memac.c b/drivers/net/ethernet/freescale/fman/mac/fm_memac.c
index becbb88..3d5ede3 100644
--- a/drivers/net/ethernet/freescale/fman/mac/fm_memac.c
+++ b/drivers/net/ethernet/freescale/fman/mac/fm_memac.c
@@ -71,9 +71,10 @@ static int memac_mii_write_phy_reg(struct memac_t *memac, u8 phy_addr,
 }
 
 static void setup_sgmii_internal_phy(struct memac_t *memac, u8 phy_addr,
-				     bool fixed_link)
+				     struct fixed_phy_status *fixed_link)
 {
 	u16 tmp_reg16;
+	enum ethernet_interface enet_if;
 	enum e_enet_mode enet_mode;
 
 	/* In case the higher MACs are used (i.e. the MACs that should
@@ -81,20 +82,37 @@ static void setup_sgmii_internal_phy(struct memac_t *memac, u8 phy_addr,
 	 * Temporary modify enet mode to 1G one, so MII functions can
 	 * work correctly.
 	 */
+	enet_if = ENET_INTERFACE_FROM_MODE(memac->enet_mode);
 	enet_mode = memac->enet_mode;
-	memac->enet_mode =
-	    MAKE_ENET_MODE(ENET_INTERFACE_FROM_MODE(memac->enet_mode),
-			   ENET_SPEED_1000);
+	memac->enet_mode = MAKE_ENET_MODE(enet_if, ENET_SPEED_1000);
 
 	/* SGMII mode */
 	tmp_reg16 = PHY_SGMII_IF_MODE_SGMII;
 	if (!fixed_link)
 		/* AN enable */
 		tmp_reg16 |= PHY_SGMII_IF_MODE_AN;
-	else
-		/* Fixed link 1Gb FD */
-		tmp_reg16 |= PHY_SGMII_IF_MODE_SPEED_GB |
-			     PHY_SGMII_IF_MODE_DUPLEX_FULL;
+	else {
+		switch (fixed_link->speed) {
+		case 10:
+			tmp_reg16 |= PHY_SGMII_IF_MODE_SPEED_10M;
+			memac->enet_mode = MAKE_ENET_MODE(enet_if,
+							  ENET_SPEED_10);
+		break;
+		case 100:
+			tmp_reg16 |= PHY_SGMII_IF_MODE_SPEED_100M;
+			memac->enet_mode = MAKE_ENET_MODE(enet_if,
+							  ENET_SPEED_100);
+		break;
+		case 1000: /* fallthrough */
+		default:
+			tmp_reg16 |= PHY_SGMII_IF_MODE_SPEED_GB;
+		break;
+		}
+		if (fixed_link->duplex)
+			tmp_reg16 |= PHY_SGMII_IF_MODE_DUPLEX_FULL;
+		else
+			tmp_reg16 |= PHY_SGMII_IF_MODE_DUPLEX_HALF;
+	}
 	memac_mii_write_phy_reg(memac, phy_addr, 0x14, tmp_reg16);
 
 	/* Device ability according to SGMII specification */
@@ -120,6 +138,7 @@ static void setup_sgmii_internal_phy(struct memac_t *memac, u8 phy_addr,
 		/* Restart AN */
 		tmp_reg16 = PHY_SGMII_CR_DEF_VAL | PHY_SGMII_CR_RESET_AN;
 	else
+		/* AN disabled */
 		tmp_reg16 = PHY_SGMII_CR_DEF_VAL & ~PHY_SGMII_CR_AN_ENABLE;
 	memac_mii_write_phy_reg(memac, phy_addr, 0x0, tmp_reg16);
 
@@ -370,14 +389,15 @@ int memac_cfg_reset_on_init(struct fm_mac_dev *fm_mac_dev, bool enable)
 	return 0;
 }
 
-int memac_cfg_fixed_link(struct fm_mac_dev *fm_mac_dev, bool enable)
+int memac_cfg_fixed_link(struct fm_mac_dev *fm_mac_dev,
+			 struct fixed_phy_status *fixed_link)
 {
 	struct memac_t *memac = (struct memac_t *)fm_mac_dev;
 
 	if (is_init_done(memac->memac_drv_param))
 		return -EINVAL;
 
-	memac->memac_drv_param->fixed_link = enable;
+	memac->memac_drv_param->fixed_link = fixed_link;
 
 	return 0;
 }
@@ -524,7 +544,7 @@ int memac_init(struct fm_mac_dev *fm_mac_dev)
 	enet_addr_t eth_addr;
 	enum fm_mac_type port_type;
 	bool slow_10g_if = false;
-	bool fixed_link;
+	struct fixed_phy_status *fixed_link;
 	int err;
 	u32 reg32 = 0;
 
diff --git a/drivers/net/ethernet/freescale/fman/mac/fm_memac.h b/drivers/net/ethernet/freescale/fman/mac/fm_memac.h
index 8fcbd64..7c13706 100644
--- a/drivers/net/ethernet/freescale/fman/mac/fm_memac.h
+++ b/drivers/net/ethernet/freescale/fman/mac/fm_memac.h
@@ -104,7 +104,8 @@ int memac_adjust_link(struct fm_mac_dev *fm_mac_dev,
 		      enum ethernet_speed speed);
 int memac_cfg_max_frame_len(struct fm_mac_dev *fm_mac_dev, u16 new_val);
 int memac_cfg_reset_on_init(struct fm_mac_dev *fm_mac_dev, bool enable);
-int memac_cfg_fixed_link(struct fm_mac_dev *fm_mac_dev, bool enable);
+int memac_cfg_fixed_link(struct fm_mac_dev *fm_mac_dev,
+			 struct fixed_phy_status *fixed_link);
 int memac_enable(struct fm_mac_dev *fm_mac_dev, enum comm_mode mode);
 int memac_disable(struct fm_mac_dev *fm_mac_dev, enum comm_mode mode);
 int memac_init(struct fm_mac_dev *fm_mac_dev);
diff --git a/drivers/net/ethernet/freescale/fman/mac/mac.c b/drivers/net/ethernet/freescale/fman/mac/mac.c
index c44544b..4a0ab7e 100644
--- a/drivers/net/ethernet/freescale/fman/mac/mac.c
+++ b/drivers/net/ethernet/freescale/fman/mac/mac.c
@@ -39,6 +39,8 @@
 #include <linux/of_mdio.h>
 #include <linux/device.h>
 #include <linux/phy.h>
+#include <linux/netdevice.h>
+#include <linux/phy_fixed.h>
 
 #include "mac.h"
 
@@ -363,7 +365,6 @@ static int mac_probe(struct platform_device *_of_dev)
 	}
 
 	mac_dev->link		= false;
-	mac_dev->fixed_link	= false;
 	mac_dev->speed		= phy2speed[mac_dev->phy_if];
 	mac_dev->max_speed	= mac_dev->speed;
 	mac_dev->if_support = DTSEC_SUPPORTED;
@@ -383,10 +384,18 @@ static int mac_probe(struct platform_device *_of_dev)
 	/* Get the rest of the PHY information */
 	mac_dev->phy_node = of_parse_phandle(mac_node, "phy-handle", 0);
 	if (!mac_dev->phy_node && of_phy_is_fixed_link(mac_node)) {
-		err = of_phy_register_fixed_link(mac_node);
-		if (err)
+		struct phy_device *phy;
+
+		mac_dev->fixed_link = kzalloc(sizeof(*mac_dev->fixed_link),
+					      GFP_KERNEL);
+		if (of_phy_parse_fixed_link(mac_node, mac_dev->fixed_link))
+			goto _return_dev_set_drvdata;
+
+		phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
+					 mac_node);
+		if (IS_ERR(phy))
 			goto _return_dev_set_drvdata;
-		mac_dev->fixed_link = true;
+
 		mac_dev->phy_node = of_node_get(mac_node);
 	}
 
@@ -429,6 +438,7 @@ static int mac_probe(struct platform_device *_of_dev)
 _return_of_node_put:
 	of_node_put(dev_node);
 _return_dev_set_drvdata:
+	kfree(mac_dev->fixed_link);
 	dev_set_drvdata(dev, NULL);
 _return:
 	return err;
-- 
1.7.11.7

Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Florian Fainelli]

CC'ing Stas,

Le 08/05/15 07:42, Madalin Bucur a écrit :
> The FMan MAC configuration code needs the speed and duplex information
> for fixed-link interfaces that is parsed now by the of function
> of_phy_register_fixed_link(). This parses the fixed-link parameters but
> does not expose to the caller neither the phy_device pointer nor the
> status struct where it loads the fixed-link params. By extracting the
> fixed-link parsing code from of_phy_register_fixed_link() into a
> separate function the parsed values are made available without changing
> the existing API. This change also removes a small redundancy in the
> previous code calling fixed_phy_register().

I will look into this shortly, sorry for the delay.

> 
> The FMan patch relies on the latest FMan driver v4 submission by Igal Liberman:
> https://patchwork.ozlabs.org/project/netdev/list/?submitter=Igal.Liberman&state=*&q=v4
> 
> Madalin Bucur (2):
>   of: separate fixed link parsing from registration
>   fsl_fman: use fixed_phy_status for MEMAC
> 
>  .../ethernet/freescale/fman/flib/fsl_fman_memac.h  |  6 ++-
>  drivers/net/ethernet/freescale/fman/inc/mac.h      |  2 +-
>  drivers/net/ethernet/freescale/fman/mac/fm_memac.c | 42 ++++++++++++-----
>  drivers/net/ethernet/freescale/fman/mac/fm_memac.h |  3 +-
>  drivers/net/ethernet/freescale/fman/mac/mac.c      | 18 ++++++--
>  drivers/of/of_mdio.c                               | 52 ++++++++++++++--------
>  include/linux/of_mdio.h                            |  9 ++++
>  7 files changed, 94 insertions(+), 38 deletions(-)
> 


-- 
Florian

Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Stas Sergeev]

08.08.2015 20:32, Florian Fainelli пишет:
> CC'ing Stas,
Hi.

> Le 08/05/15 07:42, Madalin Bucur a écrit :
>> The FMan MAC configuration code needs the speed and duplex information
>> for fixed-link interfaces that is parsed now by the of function
>> of_phy_register_fixed_link(). This parses the fixed-link parameters but
>> does not expose to the caller neither the phy_device pointer nor the
>> status struct where it loads the fixed-link params.
I have only barely touched that code, but IMO both things
are by design. There are some API deficiencies, and so, many
drivers still use of_phy_find_device() to circumvent the encapsulation
and get the phy_device pointer, but this is unlikely a good thing
to do. I even proposed some API extensions, but there was no
interest.

>>   By extracting the
>> fixed-link parsing code from of_phy_register_fixed_link() into a
>> separate function the parsed values are made available without changing
>> the existing API. This change also removes a small redundancy in the
>> previous code calling fixed_phy_register().
Today, the fixed_link is not always fixed.
See for example this patch (already mainlined):
https://lkml.org/lkml/2015/7/20/711
of_phy_is_fixed_link() returns 'true' if you have
managed="in-band-status", and so the SGMII in-band status
can update fixed-link params.

So my question is: why do you even need to know whether
the link is fixed or not? IIRC you can check the phy_device
pointer in the adjust_link callback of of_phy_connect() to get
the current link status values. Why is this not enough for your
task? Maybe the patch description should be updated to include
why the current technique is bad, what is actually fixed by the
change.
I think using the fixed-link DT values directly is not something
to be done. The encapsulation is there for a reason, so maybe
instead we can see what API additions do we need to avoid the
current limitations that force people to use of_phy_find_device()
and other work-arounds.

RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Madalin-Cristian Bucur]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 2994 bytes --]

+ Joakim, Shaohui

> -----Original Message-----
> From: Stas Sergeev [mailto:stsp@list.ru]
> 
> 08.08.2015 20:32, Florian Fainelli пишет:
> > CC'ing Stas,
> Hi.
> 
> > Le 08/05/15 07:42, Madalin Bucur a écrit :
> >> The FMan MAC configuration code needs the speed and duplex
> information
> >> for fixed-link interfaces that is parsed now by the of function
> >> of_phy_register_fixed_link(). This parses the fixed-link parameters but
> >> does not expose to the caller neither the phy_device pointer nor the
> >> status struct where it loads the fixed-link params.
> I have only barely touched that code, but IMO both things
> are by design. There are some API deficiencies, and so, many
> drivers still use of_phy_find_device() to circumvent the encapsulation
> and get the phy_device pointer, but this is unlikely a good thing
> to do. I even proposed some API extensions, but there was no
> interest.
> 
> >>   By extracting the
> >> fixed-link parsing code from of_phy_register_fixed_link() into a
> >> separate function the parsed values are made available without changing
> >> the existing API. This change also removes a small redundancy in the
> >> previous code calling fixed_phy_register().
> Today, the fixed_link is not always fixed.
> See for example this patch (already mainlined):
> https://lkml.org/lkml/2015/7/20/711
> of_phy_is_fixed_link() returns 'true' if you have
> managed="in-band-status", and so the SGMII in-band status
> can update fixed-link params.
> 
> So my question is: why do you even need to know whether
> the link is fixed or not? IIRC you can check the phy_device
> pointer in the adjust_link callback of of_phy_connect() to get
> the current link status values. Why is this not enough for your
> task? Maybe the patch description should be updated to include
> why the current technique is bad, what is actually fixed by the
> change.
> I think using the fixed-link DT values directly is not something
> to be done. The encapsulation is there for a reason, so maybe
> instead we can see what API additions do we need to avoid the
> current limitations that force people to use of_phy_find_device()
> and other work-arounds.

I need to be able to determine the imposed speed and duplex for fixed link
external PHYs because I need to configure the internal PHY with matching
values. If I do not set the same speed, given the fact that AN needs to be off,
there will be no link and no adjust link to fix things later (and the internal PHY is
not updated by adjust link anyway). I do not have access at the phy pointer at
the time I need the speed and duplex, to retrieve the defaults from there and
I've tried to make the smallest changes that allow me to retrieve those without
modifying existing API.
Why is it important to hide the default values from the MAC driver?

Thank you,
Madalin
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Stas Sergeev]

11.08.2015 19:33, Madalin-Cristian Bucur пишет:
> + Joakim, Shaohui
>
>> -----Original Message-----
>> From: Stas Sergeev [mailto:stsp@list.ru]
>>
>> 08.08.2015 20:32, Florian Fainelli пишет:
>>> CC'ing Stas,
>> Hi.
>>
>>> Le 08/05/15 07:42, Madalin Bucur a écrit :
>>>> The FMan MAC configuration code needs the speed and duplex
>> information
>>>> for fixed-link interfaces that is parsed now by the of function
>>>> of_phy_register_fixed_link(). This parses the fixed-link parameters but
>>>> does not expose to the caller neither the phy_device pointer nor the
>>>> status struct where it loads the fixed-link params.
>> I have only barely touched that code, but IMO both things
>> are by design. There are some API deficiencies, and so, many
>> drivers still use of_phy_find_device() to circumvent the encapsulation
>> and get the phy_device pointer, but this is unlikely a good thing
>> to do. I even proposed some API extensions, but there was no
>> interest.
>>
>>>>    By extracting the
>>>> fixed-link parsing code from of_phy_register_fixed_link() into a
>>>> separate function the parsed values are made available without changing
>>>> the existing API. This change also removes a small redundancy in the
>>>> previous code calling fixed_phy_register().
>> Today, the fixed_link is not always fixed.
>> See for example this patch (already mainlined):
>> https://lkml.org/lkml/2015/7/20/711
>> of_phy_is_fixed_link() returns 'true' if you have
>> managed="in-band-status", and so the SGMII in-band status
>> can update fixed-link params.
>>
>> So my question is: why do you even need to know whether
>> the link is fixed or not? IIRC you can check the phy_device
>> pointer in the adjust_link callback of of_phy_connect() to get
>> the current link status values. Why is this not enough for your
>> task? Maybe the patch description should be updated to include
>> why the current technique is bad, what is actually fixed by the
>> change.
>> I think using the fixed-link DT values directly is not something
>> to be done. The encapsulation is there for a reason, so maybe
>> instead we can see what API additions do we need to avoid the
>> current limitations that force people to use of_phy_find_device()
>> and other work-arounds.
> I need to be able to determine the imposed speed and duplex for fixed link
> external PHYs because I need to configure the internal PHY with matching
> values. If I do not set the same speed, given the fact that AN needs to be off,
> there will be no link and no adjust link to fix things later (and the internal PHY is
> not updated by adjust link anyway). I do not have access at the phy pointer at
> the time I need the speed and duplex, to retrieve the defaults from there and
> I've tried to make the smallest changes that allow me to retrieve those without
> modifying existing API.
> Why is it important to hide the default values from the MAC driver?
My worry is that the fixed values are not really fixed, and
therefore are not always useful to access directly. It is likely
not a problem for your use-case, as, as you say, the AN is
disabled, but this is probably not the best to do in general.
And also you do:
---

-		err = of_phy_register_fixed_link(mac_node);
-		if (err)
+		struct phy_device *phy;
+
+		mac_dev->fixed_link = kzalloc(sizeof(*mac_dev->fixed_link),
+					      GFP_KERNEL);
+		if (of_phy_parse_fixed_link(mac_node, mac_dev->fixed_link))
+			goto _return_dev_set_drvdata;
+
+		phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
+					 mac_node);

---

which means you really want to circumvent the current OF
api quite a lot, without saying why in the patch description.
As such, it may be difficult to review. Could you please write
a more complete description to the patch?

As to your problem: would it be possible to set speed & duplex
after you do of_phy_connect()? It returns the phy_device
pointer, and perhaps you can look into phydev->speed and
phydev->duplex at that point?

RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Madalin-Cristian Bucur]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 3898 bytes --]

> -----Original Message-----
> From: Stas Sergeev [mailto:stsp@list.ru]
> 
> 11.08.2015 19:33, Madalin-Cristian Bucur пишет:
> > + Joakim, Shaohui
> >
> >> -----Original Message-----
> >> From: Stas Sergeev [mailto:stsp@list.ru]
> >>
> >> 08.08.2015 20:32, Florian Fainelli пишет:
> >>> CC'ing Stas,
> >> Hi.
> >>
> >>> Le 08/05/15 07:42, Madalin Bucur a écrit :
> >>>> The FMan MAC configuration code needs the speed and duplex
> >> information
> >>>> for fixed-link interfaces that is parsed now by the of function
> >>>> of_phy_register_fixed_link(). This parses the fixed-link parameters but
> >>>> does not expose to the caller neither the phy_device pointer nor the
> >>>> status struct where it loads the fixed-link params.
<snip>

> > I've tried to make the smallest changes that allow me to retrieve those
> > without modifying existing API.
> > Why is it important to hide the default values from the MAC driver?
> My worry is that the fixed values are not really fixed, and
> therefore are not always useful to access directly. It is likely
> not a problem for your use-case, as, as you say, the AN is
> disabled, but this is probably not the best to do in general.

Yes, not a problem in my case.

> And also you do:
> ---
> 
> -		err = of_phy_register_fixed_link(mac_node);
> -		if (err)
> +		struct phy_device *phy;
> +
> +		mac_dev->fixed_link = kzalloc(sizeof(*mac_dev-
> >fixed_link),
> +					      GFP_KERNEL);
> +		if (of_phy_parse_fixed_link(mac_node, mac_dev-
> >fixed_link))
> +			goto _return_dev_set_drvdata;
> +
> +		phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
> +					 mac_node);
> 
> ---
> 
> which means you really want to circumvent the current OF
> api quite a lot, without saying why in the patch description.

I circumvent the API because I din not want to change existing API.
If I could get a reference to the status struct without changing any code 
or without being required to call by myself fixed_phy_register(), I
would of done that. Given the existing code in of_phy_register_fixed_link(),
this was my only option. I could have broken of_phy_register_fixed_link()
in two functions:

of_phy_parse_fixed_link() and of_phy_register_fixed_link(), the latter doing only
the call to fixed_phy_register()

that would allow to keep of_phy_register_fixed_link() as it is, broken in two stages:

- parsing
- registering

than can be used by other drivers in order to get the status but I think it's overkill.

> As such, it may be difficult to review. Could you please write
> a more complete description to the patch?

To better understand this patch, think of it as just a refactoring of the
of_phy_register_fixed_link() that does two things inside:

- parsing of fixed link node (2 bindings supported)
- register phy by calling fixed_phy_register() in the same way, in the same codebase

I've extracted the parsing in a separate function ( following the "one function should
do one thing" rule).

Then I've exported this function to make status available to callers.

> As to your problem: would it be possible to set speed & duplex
> after you do of_phy_connect()? It returns the phy_device
> pointer, and perhaps you can look into phydev->speed and
> phydev->duplex at that point?

It would be possible but un-natural as I'd have probing information only available at
runtime. That would just complicate matters for my particular case ans I suspect there
will be other drivers that get into this situation. You are concerned about people
abusing this API to read fixed link status when the link is not really fixed, I'm concerned
about declaring the link as fixed-link when it's not. Maybe the naming/binding needs to be
revised to cover the case when all is fixed but the link.

ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Stas Sergeev]

12.08.2015 16:26, Madalin-Cristian Bucur пишет:
>>> I've tried to make the smallest changes that allow me to retrieve those
>>> without modifying existing API.
>>> Why is it important to hide the default values from the MAC driver?
>> My worry is that the fixed values are not really fixed, and
>> therefore are not always useful to access directly. It is likely
>> not a problem for your use-case, as, as you say, the AN is
>> disabled, but this is probably not the best to do in general.
> Yes, not a problem in my case.
>
>> And also you do:
>> ---
>>
>> -		err = of_phy_register_fixed_link(mac_node);
>> -		if (err)
>> +		struct phy_device *phy;
>> +
>> +		mac_dev->fixed_link = kzalloc(sizeof(*mac_dev-
>>> fixed_link),
>> +					      GFP_KERNEL);
>> +		if (of_phy_parse_fixed_link(mac_node, mac_dev-
>>> fixed_link))
>> +			goto _return_dev_set_drvdata;
>> +
>> +		phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
>> +					 mac_node);
>>
>> ---
>>
>> which means you really want to circumvent the current OF
>> api quite a lot, without saying why in the patch description.
> I circumvent the API because I din not want to change existing API.
> If I could get a reference to the status struct without changing any code
> or without being required to call by myself fixed_phy_register(), I
> would of done that. Given the existing code in of_phy_register_fixed_link(),
> this was my only option. I could have broken of_phy_register_fixed_link()
> in two functions:
>
> of_phy_parse_fixed_link() and of_phy_register_fixed_link(), the latter doing only
> the call to fixed_phy_register()
>
> that would allow to keep of_phy_register_fixed_link() as it is, broken in two stages:
>
> - parsing
> - registering
>
> than can be used by other drivers in order to get the status but I think it's overkill.
What I referred to as "circumventing an API" is that you do
phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link, + mac_node);
by hands, instead of letting the of_phy_register_fixed_link() doing so.

How about a smaller circumvention, like this for instance:
---
err = of_phy_register_fixed_link(mac_node);
phy = of_phy_find_device(dn);
status = fixed_phy_get_link_status(phy);    // no such func, to be coded up
---

Or even like this:
---
err = of_phy_register_fixed_link(mac_node);
phy = of_phy_find_device(dn);
set_speed_and_duplex(phy->speed, phy->duplex);    // not sure if these 
values are available that early
---

Also I meant the description should have been in the patch,
not in the e-mail. :) You only wrote _what_ the patch does
(which is of course obvious from the code itself), but not
_why_ and _what was fixed_ (what didn't work).

>> As to your problem: would it be possible to set speed & duplex
>> after you do of_phy_connect()? It returns the phy_device
>> pointer, and perhaps you can look into phydev->speed and
>> phydev->duplex at that point?
> It would be possible but un-natural as I'd have probing information only available at
> runtime.
This is un-natural only if you deal just with a fixed case.
If your driver can deal also with the non-fixed cases
(either AN or MDIO), then this looks more natural as the
non-fixed management should be done at any point of time,
and certainly _after_ of_phy_connect(). So if your driver is
universal, this look like the natural choise to me, but if it is
limited to the fixed case, then, as a simplification, you move
that to the init time.
But I am not argueing what is more natural. Maybe the
above approaches with of_phy_find_device() can be used
in init time?

>   That would just complicate matters for my particular case ans I suspect there
> will be other drivers that get into this situation.
I suspect only those that are limited to the fixed-link case.
Only then they may decide to move phy management to
init time, but IMHO this is just an optimization.

>   You are concerned about people
> abusing this API to read fixed link status when the link is not really fixed, I'm concerned
> about declaring the link as fixed-link when it's not. Maybe the naming/binding needs to be
> revised to cover the case when all is fixed but the link.
Yes, naming is the problem. fixed-link is just a bad name.
See how it is defined:
---
Some Ethernet MACs have a "fixed link", and are not connected to a
normal MDIO-managed PHY device.
---
To me this means any non-MDIO PHY connection, but
unfortunately the name "fixed-link" suggests a bit more
than advertised. :(

RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Madalin-Cristian Bucur]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 5594 bytes --]

> -----Original Message-----
> From: Stas Sergeev [mailto:stsp@list.ru]
>
> 12.08.2015 16:26, Madalin-Cristian Bucur пишет:
> >>> I've tried to make the smallest changes that allow me to retrieve those
> >>> without modifying existing API.
> >>> Why is it important to hide the default values from the MAC driver?
> >> My worry is that the fixed values are not really fixed, and
> >> therefore are not always useful to access directly. It is likely
> >> not a problem for your use-case, as, as you say, the AN is
> >> disabled, but this is probably not the best to do in general.
> > Yes, not a problem in my case.
> >
> >> And also you do:
> >> ---
> >>
> >> -		err = of_phy_register_fixed_link(mac_node);
> >> -		if (err)
> >> +		struct phy_device *phy;
> >> +
> >> +		mac_dev->fixed_link = kzalloc(sizeof(*mac_dev-
> >>> fixed_link),
> >> +					      GFP_KERNEL);
> >> +		if (of_phy_parse_fixed_link(mac_node, mac_dev-
> >>> fixed_link))
> >> +			goto _return_dev_set_drvdata;
> >> +
> >> +		phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
> >> +					 mac_node);
> >>
> >> ---
> >>
> >> which means you really want to circumvent the current OF
> >> api quite a lot, without saying why in the patch description.
> > I circumvent the API because I din not want to change existing API.
> > If I could get a reference to the status struct without changing any code
> > or without being required to call by myself fixed_phy_register(), I
> > would of done that. Given the existing code in
> of_phy_register_fixed_link(),
> > this was my only option. I could have broken of_phy_register_fixed_link()
> > in two functions:
> >
> > of_phy_parse_fixed_link() and of_phy_register_fixed_link(), the latter
> doing only
> > the call to fixed_phy_register()
> >
> > that would allow to keep of_phy_register_fixed_link() as it is, broken in
> two stages:
> >
> > - parsing
> > - registering
> >
> > than can be used by other drivers in order to get the status but I think it's
> overkill.
> What I referred to as "circumventing an API" is that you do
> phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link, + mac_node);
> by hands, instead of letting the of_phy_register_fixed_link() doing so.
> 
> How about a smaller circumvention, like this for instance:
> ---
> err = of_phy_register_fixed_link(mac_node);
> phy = of_phy_find_device(dn);
> status = fixed_phy_get_link_status(phy);    // no such func, to be coded up
> ---
> 
> Or even like this:
> ---
> err = of_phy_register_fixed_link(mac_node);
> phy = of_phy_find_device(dn);
> set_speed_and_duplex(phy->speed, phy->duplex);    // not sure if these
> values are available that early
> ---

After my patch, all that of_phy_register_fixed_link() does is to call
the new parsing function I introduced then register the fixed PHY.
I could have done this (pseudocode):

- add of_phy_parse_fixed_link() as seen in the patch
- add of_phy_register_fixed_phy() that just calls fixed_phy_register():

int of_phy_register_fixed_phy(node)
{
	phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
				     mac_node);
	return (!phy);
}

- change of_phy_register_fixed_link() to contain only this:

int of_phy_register_fixed_link(node)
{
	of_phy_parse_fixed_link(node, &status);

	return of_phy_register_fixed_phy(node);
}

Then I could call only of_* functions but the end result would be the same and
of_phy_register_fixed_phy() would not justify its existence that much...

The getter for status you suggest would be fine, but not sure how one would retrieve
it from the mac_node unless we change of_phy_register_fixed_link() to return the
pointer to phy (and all the drivers that use it...).

> 
> Also I meant the description should have been in the patch,
> not in the e-mail. :) You only wrote _what_ the patch does
> (which is of course obvious from the code itself), but not
> _why_ and _what was fixed_ (what didn't work).
> 


If you refer to the first, separation patch, I thought the description was enough:

    of: separate fixed link parsing from registration
    
    Some drivers may need to parse the fixed link values before registering
    the fixed link phy. Separate the parsing from the actual registration
    and provide an export for the added parsing function.
    
    Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>

For this one it was a bit brief, I admit - the longer version would be that before it
we were not using from fixed link anything else but the fact the link was fixed
(ignored actual speed, duplex values there) and this patch tries to fix that.
In the end this patch will be squashed in a new FMan patch set, let me use that as
an excuse for the brief commit log :)

<snip>

> >   You are concerned about people
> > abusing this API to read fixed link status when the link is not really fixed, I'm
> concerned
> > about declaring the link as fixed-link when it's not. Maybe the
> naming/binding needs to be
> > revised to cover the case when all is fixed but the link.
> Yes, naming is the problem. fixed-link is just a bad name.
> See how it is defined:
> ---
> Some Ethernet MACs have a "fixed link", and are not connected to a
> normal MDIO-managed PHY device.
> ---
> To me this means any non-MDIO PHY connection, but
> unfortunately the name "fixed-link" suggests a bit more
> than advertised. :(

Yes, maybe the new binding could be updated to load semantically the presence
or absence of certain parameters such as link.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Stas Sergeev]

12.08.2015 17:43, Madalin-Cristian Bucur пишет:
>> -----Original Message-----
>> From: Stas Sergeev [mailto:stsp@list.ru]
>>
>> 12.08.2015 16:26, Madalin-Cristian Bucur пишет:
>>>>> I've tried to make the smallest changes that allow me to retrieve those
>>>>> without modifying existing API.
>>>>> Why is it important to hide the default values from the MAC driver?
>>>> My worry is that the fixed values are not really fixed, and
>>>> therefore are not always useful to access directly. It is likely
>>>> not a problem for your use-case, as, as you say, the AN is
>>>> disabled, but this is probably not the best to do in general.
>>> Yes, not a problem in my case.
>>>
>>>> And also you do:
>>>> ---
>>>>
>>>> -		err = of_phy_register_fixed_link(mac_node);
>>>> -		if (err)
>>>> +		struct phy_device *phy;
>>>> +
>>>> +		mac_dev->fixed_link = kzalloc(sizeof(*mac_dev-
>>>>> fixed_link),
>>>> +					      GFP_KERNEL);
>>>> +		if (of_phy_parse_fixed_link(mac_node, mac_dev-
>>>>> fixed_link))
>>>> +			goto _return_dev_set_drvdata;
>>>> +
>>>> +		phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
>>>> +					 mac_node);
>>>>
>>>> ---
>>>>
>>>> which means you really want to circumvent the current OF
>>>> api quite a lot, without saying why in the patch description.
>>> I circumvent the API because I din not want to change existing API.
>>> If I could get a reference to the status struct without changing any code
>>> or without being required to call by myself fixed_phy_register(), I
>>> would of done that. Given the existing code in
>> of_phy_register_fixed_link(),
>>> this was my only option. I could have broken of_phy_register_fixed_link()
>>> in two functions:
>>>
>>> of_phy_parse_fixed_link() and of_phy_register_fixed_link(), the latter
>> doing only
>>> the call to fixed_phy_register()
>>>
>>> that would allow to keep of_phy_register_fixed_link() as it is, broken in
>> two stages:
>>> - parsing
>>> - registering
>>>
>>> than can be used by other drivers in order to get the status but I think it's
>> overkill.
>> What I referred to as "circumventing an API" is that you do
>> phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link, + mac_node);
>> by hands, instead of letting the of_phy_register_fixed_link() doing so.
>>
>> How about a smaller circumvention, like this for instance:
>> ---
>> err = of_phy_register_fixed_link(mac_node);
>> phy = of_phy_find_device(dn);
>> status = fixed_phy_get_link_status(phy);    // no such func, to be coded up
>> ---
>>
>> Or even like this:
>> ---
>> err = of_phy_register_fixed_link(mac_node);
>> phy = of_phy_find_device(dn);
>> set_speed_and_duplex(phy->speed, phy->duplex);    // not sure if these
>> values are available that early
>> ---
> After my patch, all that of_phy_register_fixed_link() does is to call
> the new parsing function I introduced then register the fixed PHY.
> I could have done this (pseudocode):
>
> - add of_phy_parse_fixed_link() as seen in the patch
> - add of_phy_register_fixed_phy() that just calls fixed_phy_register():
>
> int of_phy_register_fixed_phy(node)
> {
> 	phy = fixed_phy_register(PHY_POLL, mac_dev->fixed_link,
> 				     mac_node);
> 	return (!phy);
> }
>
> - change of_phy_register_fixed_link() to contain only this:
>
> int of_phy_register_fixed_link(node)
> {
> 	of_phy_parse_fixed_link(node, &status);
>
> 	return of_phy_register_fixed_phy(node);
> }
But have you looked into the patch I pointed previously?
https://lkml.org/lkml/2015/7/20/711
You code will likely clash with it because my patch extends
of_phy_register_fixed_link().

> Then I could call only of_* functions but the end result would be the same and
> of_phy_register_fixed_phy() would not justify its existence that much...
You didn't say you wanted to obsolete the of_phy_register_fixed_phy().
Since it is there (and even changed by me in a way your
patch will likely clash), IMHO it would be better if it is used,
rather than copy/pasted into the driver.

> The getter for status you suggest would be fine, but not sure how one would retrieve
> it from the mac_node unless we change of_phy_register_fixed_link() to return the
> pointer to phy (and all the drivers that use it...).
If you look for instance to mvneta.c, you'll find the following:
---
err = of_phy_register_fixed_link(dn);
/* In the case of a fixed PHY, the DT node associated
  * to the PHY is the Ethernet MAC DT node.
  */
  phy_node = of_node_get(dn);
...
phy = of_phy_find_device(dn);
---

So the answer is: just use the same mac_node for both.

>> Also I meant the description should have been in the patch,
>> not in the e-mail. :) You only wrote _what_ the patch does
>> (which is of course obvious from the code itself), but not
>> _why_ and _what was fixed_ (what didn't work).
>>
> If you refer to the first, separation patch, I thought the description was enough:
>
>      of: separate fixed link parsing from registration
>      
>      Some drivers may need
"may need"? I don't understand.
If it is a fix, then they _do need_, and in this case it should
be specified what was broken and what is fixed.
If it is just a clean-up, then "may need" may suffice, but it
was not mentioned it is a clean-up. So I still don't know what
this patch is all about.
"Some drivers" - which ones? The ones that are limited to
the purely fixed links, and never support AN or MDIO?
Or some other drivers too?
So really, the description sounds very cryptic to me.

>   to parse the fixed link values before registering
>      the fixed link phy. Separate the parsing from the actual registration
>      and provide an export for the added parsing function.
>      
>      Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
>
> For this one it was a bit brief, I admit - the longer version would be that before it
> we were not using from fixed link anything else but the fact the link was fixed
> (ignored actual speed, duplex values there)
And what didn't work as the result?

>   and this patch tries to fix that.
What started to work after that patch that didn't without it?

RE: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Madalin-Cristian Bucur]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4281 bytes --]

> -----Original Message-----
> From: Stas Sergeev [mailto:stsp@list.ru]

<snip>
> But have you looked into the patch I pointed previously?
> https://lkml.org/lkml/2015/7/20/711
> You code will likely clash with it because my patch extends
> of_phy_register_fixed_link().
> 

I admin I failed to grasp the details of your change - the lack of ample context
Lines makes it a bit difficult. I'm sure your change could be merged then the
of parsing could be separated from the actual fixed_phy_register() call if anyone
cares about that.

> > Then I could call only of_* functions but the end result would be the same
> > and  of_phy_register_fixed_phy() would not justify its existence that much...
> You didn't say you wanted to obsolete the of_phy_register_fixed_phy().
> Since it is there (and even changed by me in a way your
> patch will likely clash), IMHO it would be better if it is used,
> rather than copy/pasted into the driver.

Please note I was referring to a fictional new function that would embed the call to 
fixed_phy_register(). I was not talking about some existing API, just about a new 
of_call  named of_phy_register_fixed_phy()  that would in the end be called by
of_phy_register_fixed_link() and by some drivers that want to get in the middle
of things and get a hold on status.

Maybe the fact we're reviewing two patches in one thread is what makes the
discussion less than optimal.

> > The getter for status you suggest would be fine, but not sure how one
> > would retrieve
> > it from the mac_node unless we change of_phy_register_fixed_link() to
> > return the
> > pointer to phy (and all the drivers that use it...).
> If you look for instance to mvneta.c, you'll find the following:
> ---
> err = of_phy_register_fixed_link(dn);
> /* In the case of a fixed PHY, the DT node associated
>   * to the PHY is the Ethernet MAC DT node.
>   */
>   phy_node = of_node_get(dn);
> ...
> phy = of_phy_find_device(dn);
> ---
> 
> So the answer is: just use the same mac_node for both.

I understand, I'll use this approach although is suboptimal imho to
scan the device tree again to get a phy pointer that you need just
to get some of info that was parsed in a call you just made.

> >> Also I meant the description should have been in the patch,
> >> not in the e-mail. :) You only wrote _what_ the patch does
> >> (which is of course obvious from the code itself), but not
> >> _why_ and _what was fixed_ (what didn't work).
> >>
> > If you refer to the first, separation patch, I thought the description was
> enough:
> >
> >      of: separate fixed link parsing from registration
> >
> >      Some drivers may need
> "may need"? I don't understand.
> If it is a fix, then they _do need_, and in this case it should
> be specified what was broken and what is fixed.
> If it is just a clean-up, then "may need" may suffice, but it
> was not mentioned it is a clean-up. So I still don't know what
> this patch is all about.
> "Some drivers" - which ones? The ones that are limited to
> the purely fixed links, and never support AN or MDIO?
> Or some other drivers too?
> So really, the description sounds very cryptic to me.

Mine, when there is a fixed link node, maybe others. When there isn't any
fixed link node, the internal PHY config defaults to 1G full duplex AN enabled
and adjust link takes care of things.

> 
> >   to parse the fixed link values before registering
> >      the fixed link phy. Separate the parsing from the actual registration
> >      and provide an export for the added parsing function.
> >
> >      Signed-off-by: Madalin Bucur <madalin.bucur@freescale.com>
> >
> > For this one it was a bit brief, I admit - the longer version would be that
> before it
> > we were not using from fixed link anything else but the fact the link was
> fixed
> > (ignored actual speed, duplex values there)
> And what didn't work as the result?
> 
> >   and this patch tries to fix that.
> What started to work after that patch that didn't without it?

10M half duplex for instance

I'd close this thread for now and use in my driver of_phy_find_device(mac_node).

Thank you,
Madalin
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH 0/2] of: fsl/fman: reuse the fixed node parsing code

[Stas Sergeev]

12.08.2015 18:27, Madalin-Cristian Bucur пишет:
>>> Then I could call only of_* functions but the end result would be the same
>>> and  of_phy_register_fixed_phy() would not justify its existence that much...
>> You didn't say you wanted to obsolete the of_phy_register_fixed_phy().
>> Since it is there (and even changed by me in a way your
>> patch will likely clash), IMHO it would be better if it is used,
>> rather than copy/pasted into the driver.
> Please note I was referring to a fictional new function that would embed the call to
> fixed_phy_register(). I was not talking about some existing API, just about a new
> of_call  named of_phy_register_fixed_phy()  that would in the end be called by
> of_phy_register_fixed_link() and by some drivers that want to get in the middle
> of things and get a hold on status.
Hmm, and for exactly unknown reason in your pseudocode
of_phy_register_fixed_phy() doesn't take status as an argument. :)
So I didn't see its point.
If you fix your pseudo-code, you'll add the status argument,
because it is needed for fixed_phy_register() anyway.
After that, the drivers that want to provide the status, will
just use it rather than to call fixed_phy_register() directly.
And with my changes it really have even more merits to exist.

> Maybe the fact we're reviewing two patches in one thread is what makes the
> discussion less than optimal.
I guess the bugs in the pseudo-code made me to miss its point.

>>> The getter for status you suggest would be fine, but not sure how one
>>> would retrieve
>>> it from the mac_node unless we change of_phy_register_fixed_link() to
>>> return the
>>> pointer to phy (and all the drivers that use it...).
>> If you look for instance to mvneta.c, you'll find the following:
>> ---
>> err = of_phy_register_fixed_link(dn);
>> /* In the case of a fixed PHY, the DT node associated
>>    * to the PHY is the Ethernet MAC DT node.
>>    */
>>    phy_node = of_node_get(dn);
>> ...
>> phy = of_phy_find_device(dn);
>> ---
>>
>> So the answer is: just use the same mac_node for both.
> I understand, I'll use this approach although is suboptimal imho to
Exactly!
But at least this way is used in many currently existing
drivers, while getting the fixed-link parameters directly from
DT - is a new way of circumventing the existing API.
So I'd vote for the currently existing hacks, and in fact
I already tried to start a discussion about getting rid of the
need for of_phy_find_device(), but it didn't go.

> scan the device tree again to get a phy pointer that you need just
> to get some of info that was parsed in a call you just made.
Maybe (just maybe) of_phy_register_fixed_link() could
return the phy_device pointer. At least it will solve your
problem very cheaply. But I am sure such API additions
require a separate discussion, can't be done in a context
of discussing a small fix. If you have some free time, feel
free to raise such a discussion with API extension proposals.