[PATCH 0/2] of/phylib: Use device tree properties for PHY configuration.

[PATCH 0/2] of/phylib: Use device tree properties for PHY configuration.

[David Daney]

Here is my first take at using the device tree to control PHY
configuration.  This first set enables some Marvell PHYs.  If the
scheme is deemed acceptable, I would probably do something similar for
Broadcom PHYs as well.

The first patch is the meat of the change.  The second adds support
for the 88E1149R PHY that I have on one of my boards.

David Daney (2):
  of/phylib: Use device tree properties to initialize Marvell PHYs.
  phylib: Add support for Marvell 88E1149R devices.

 drivers/net/phy/marvell.c   |  135 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/marvell_phy.h |    1 +
 2 files changed, 136 insertions(+), 0 deletions(-)

-- 
1.7.2.3

[PATCH 0/2] of/phylib: Use device tree properties for PHY configuration.

[David Daney]

Here is my first take at using the device tree to control PHY
configuration.  This first set enables some Marvell PHYs.  If the
scheme is deemed acceptable, I would probably do something similar for
Broadcom PHYs as well.

The first patch is the meat of the change.  The second adds support
for the 88E1149R PHY that I have on one of my boards.

David Daney (2):
  of/phylib: Use device tree properties to initialize Marvell PHYs.
  phylib: Add support for Marvell 88E1149R devices.

 drivers/net/phy/marvell.c   |  135 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/marvell_phy.h |    1 +
 2 files changed, 136 insertions(+), 0 deletions(-)

-- 
1.7.2.3

[PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[David Daney]

Some aspects of PHY initialization are board dependent, things like
indicator LED connections and some clocking modes cannot be determined
by probing.  The dev_flags element of struct phy_device can be used to
control these things if an appropriate value can be passed from the
Ethernet driver.  We run into problems however if the PHY connections
are specified by the device tree.  There is no way for the Ethernet
driver to know what flags it should pass.

If we are using the device tree, the struct phy_device will be
populated with the device tree node corresponding to the PHY, and we
can extract extra configuration information from there.

The next question is what should the format of that information be?
It is highly device specific, and the device tree representation
should not be tied to any arbitrary kernel defined constants.  A
straight forward representation is just to specify the exact bits that
should be set using the "marvell,reg-init" property:

      phy5: ethernet-phy@5 {
	reg = <5>;
	device_type = "ethernet-phy";
	marvell,reg-init =
		<0x00030010 0x5777>, /* Reg 3,16 <- 0x5777 */
		<0x00030011 0x00aa>, /* Reg 3,17 <- 0x00aa */
		<0x00030012 0x4105>, /* Reg 3,18 <- 0x4105 */
		<0x00030013 0x0060>; /* Reg 3,19 <- 0x0060 */
		<0x00020015 0x00300000>; /* clear bits 4..5 of Reg 2,21 */
      };

The Marvell PHYs have a page select register at register 22 (0x16), we
can specify any register by its page and register number.  These are
encoded in the high and low parts of the first word.  The second word
contains a mask and value to be ORed in its high and low parts.  The
new marvell_of_reg_init function leaves the page select register
unchanged, so a call to it can be dropped into the .config_init
functions without unduly affecting the state of the PHY.

If CONFIG_OF is not set, there is no of_node, or no "marvell,reg-init"
property, the PHY initialization is unchanged.

Signed-off-by: David Daney <ddaney@caviumnetworks.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Cyril Chemparathy <cyril@ti.com>
Cc: David Daney <ddaney@caviumnetworks.com>
Cc: Arnaud Patard <arnaud.patard@rtp-net.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/phy/marvell.c |   91 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 91 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f0bd1a1..33ad654 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -30,6 +30,7 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 #include <linux/marvell_phy.h>
+#include <linux/of.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -186,6 +187,85 @@ static int marvell_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
+#ifndef CONFIG_OF
+static int marvell_of_reg_init(struct phy_device *phydev)
+{
+	return 0;
+}
+#else
+/*
+ * Set and/or override some configuration registers based on the
+ * marvell,reg-init property stored in the of_node for the phydev.
+ *
+ * marvell,reg-init = <reg-spec val-spec>,...;
+ *
+ * There may be one or more  pairs of <reg-spec val-spec>:
+ * reg-spec [16..31]: Page address.
+ * reg-spec [0..15]: Register address.
+ *
+ * val-spec [16..31]: Mask bits.
+ * val-spec [0..15]: Register bits.
+ */
+static int marvell_of_reg_init(struct phy_device *phydev)
+{
+	const __be32 *paddr;
+	int len, i, saved_page, current_page, page_changed, ret;
+
+	if (!phydev->dev.of_node)
+		return 0;
+
+	paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init", &len);
+	if (!paddr || len < (2 * sizeof(u32)))
+		return 0;
+
+	saved_page = phy_read(phydev, 22);
+	if (saved_page < 0)
+		return saved_page;
+	page_changed = 0;
+	current_page = saved_page;
+
+	ret = 0;
+	len /= sizeof(u32);
+	for (i = 0; i < len / 2; i += 2) {
+		u32 reg_spec =  be32_to_cpup(&paddr[i]);
+		u32 val_spec =  be32_to_cpup(&paddr[i + 1]);
+		u16 reg = reg_spec & 0xffff;
+		u16 reg_page = reg_spec >> 16;
+		u16 val_bits = val_spec & 0xffff;
+		u16 mask = val_spec >> 16;
+		int val;
+
+		if (reg_page != current_page) {
+			ret = phy_write(phydev, 22, reg_page);
+			if (ret < 0)
+				goto err;
+			current_page = reg_page;
+			page_changed = 1;
+		}
+
+		val = 0;
+		if (mask) {
+			val = phy_read(phydev, reg);
+			if (val < 0) {
+				ret = val;
+				goto err;
+			}
+			val &= mask;
+		}
+		val |= val_bits;
+
+		ret = phy_write(phydev, reg, (u16)val);
+		if (ret < 0)
+			goto err;
+
+	}
+err:
+	if (page_changed)
+		ret = phy_write(phydev, 22, saved_page);
+	return ret;
+}
+#endif /* CONFIG_OF */
+
 static int m88e1121_config_aneg(struct phy_device *phydev)
 {
 	int err, oldpage, mscr;
@@ -368,6 +448,9 @@ static int m88e1111_config_init(struct phy_device *phydev)
 			return err;
 	}
 
+	err = marvell_of_reg_init(phydev);
+	if (err < 0)
+		return err;
 
 	err = phy_write(phydev, MII_BMCR, BMCR_RESET);
 	if (err < 0)
@@ -420,6 +503,10 @@ static int m88e1118_config_init(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
+	err = marvell_of_reg_init(phydev);
+	if (err < 0)
+		return err;
+
 	/* Reset address */
 	err = phy_write(phydev, 0x16, 0x0);
 	if (err < 0)
@@ -491,6 +578,10 @@ static int m88e1145_config_init(struct phy_device *phydev)
 		}
 	}
 
+	err = marvell_of_reg_init(phydev);
+	if (err < 0)
+		return err;
+
 	return 0;
 }
 
-- 
1.7.2.3

[PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[David Daney]

Some aspects of PHY initialization are board dependent, things like
indicator LED connections and some clocking modes cannot be determined
by probing.  The dev_flags element of struct phy_device can be used to
control these things if an appropriate value can be passed from the
Ethernet driver.  We run into problems however if the PHY connections
are specified by the device tree.  There is no way for the Ethernet
driver to know what flags it should pass.

If we are using the device tree, the struct phy_device will be
populated with the device tree node corresponding to the PHY, and we
can extract extra configuration information from there.

The next question is what should the format of that information be?
It is highly device specific, and the device tree representation
should not be tied to any arbitrary kernel defined constants.  A
straight forward representation is just to specify the exact bits that
should be set using the "marvell,reg-init" property:

      phy5: ethernet-phy@5 {
	reg = <5>;
	device_type = "ethernet-phy";
	marvell,reg-init =
		<0x00030010 0x5777>, /* Reg 3,16 <- 0x5777 */
		<0x00030011 0x00aa>, /* Reg 3,17 <- 0x00aa */
		<0x00030012 0x4105>, /* Reg 3,18 <- 0x4105 */
		<0x00030013 0x0060>; /* Reg 3,19 <- 0x0060 */
		<0x00020015 0x00300000>; /* clear bits 4..5 of Reg 2,21 */
      };

The Marvell PHYs have a page select register at register 22 (0x16), we
can specify any register by its page and register number.  These are
encoded in the high and low parts of the first word.  The second word
contains a mask and value to be ORed in its high and low parts.  The
new marvell_of_reg_init function leaves the page select register
unchanged, so a call to it can be dropped into the .config_init
functions without unduly affecting the state of the PHY.

If CONFIG_OF is not set, there is no of_node, or no "marvell,reg-init"
property, the PHY initialization is unchanged.

Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Cyril Chemparathy <cyril-l0cyMroinI0@public.gmane.org>
Cc: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
Cc: Arnaud Patard <arnaud.patard-dQbF7i+pzddAfugRpC6u6w@public.gmane.org>
Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
---
 drivers/net/phy/marvell.c |   91 +++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 91 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index f0bd1a1..33ad654 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -30,6 +30,7 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 #include <linux/marvell_phy.h>
+#include <linux/of.h>
 
 #include <asm/io.h>
 #include <asm/irq.h>
@@ -186,6 +187,85 @@ static int marvell_config_aneg(struct phy_device *phydev)
 	return 0;
 }
 
+#ifndef CONFIG_OF
+static int marvell_of_reg_init(struct phy_device *phydev)
+{
+	return 0;
+}
+#else
+/*
+ * Set and/or override some configuration registers based on the
+ * marvell,reg-init property stored in the of_node for the phydev.
+ *
+ * marvell,reg-init = <reg-spec val-spec>,...;
+ *
+ * There may be one or more  pairs of <reg-spec val-spec>:
+ * reg-spec [16..31]: Page address.
+ * reg-spec [0..15]: Register address.
+ *
+ * val-spec [16..31]: Mask bits.
+ * val-spec [0..15]: Register bits.
+ */
+static int marvell_of_reg_init(struct phy_device *phydev)
+{
+	const __be32 *paddr;
+	int len, i, saved_page, current_page, page_changed, ret;
+
+	if (!phydev->dev.of_node)
+		return 0;
+
+	paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init", &len);
+	if (!paddr || len < (2 * sizeof(u32)))
+		return 0;
+
+	saved_page = phy_read(phydev, 22);
+	if (saved_page < 0)
+		return saved_page;
+	page_changed = 0;
+	current_page = saved_page;
+
+	ret = 0;
+	len /= sizeof(u32);
+	for (i = 0; i < len / 2; i += 2) {
+		u32 reg_spec =  be32_to_cpup(&paddr[i]);
+		u32 val_spec =  be32_to_cpup(&paddr[i + 1]);
+		u16 reg = reg_spec & 0xffff;
+		u16 reg_page = reg_spec >> 16;
+		u16 val_bits = val_spec & 0xffff;
+		u16 mask = val_spec >> 16;
+		int val;
+
+		if (reg_page != current_page) {
+			ret = phy_write(phydev, 22, reg_page);
+			if (ret < 0)
+				goto err;
+			current_page = reg_page;
+			page_changed = 1;
+		}
+
+		val = 0;
+		if (mask) {
+			val = phy_read(phydev, reg);
+			if (val < 0) {
+				ret = val;
+				goto err;
+			}
+			val &= mask;
+		}
+		val |= val_bits;
+
+		ret = phy_write(phydev, reg, (u16)val);
+		if (ret < 0)
+			goto err;
+
+	}
+err:
+	if (page_changed)
+		ret = phy_write(phydev, 22, saved_page);
+	return ret;
+}
+#endif /* CONFIG_OF */
+
 static int m88e1121_config_aneg(struct phy_device *phydev)
 {
 	int err, oldpage, mscr;
@@ -368,6 +448,9 @@ static int m88e1111_config_init(struct phy_device *phydev)
 			return err;
 	}
 
+	err = marvell_of_reg_init(phydev);
+	if (err < 0)
+		return err;
 
 	err = phy_write(phydev, MII_BMCR, BMCR_RESET);
 	if (err < 0)
@@ -420,6 +503,10 @@ static int m88e1118_config_init(struct phy_device *phydev)
 	if (err < 0)
 		return err;
 
+	err = marvell_of_reg_init(phydev);
+	if (err < 0)
+		return err;
+
 	/* Reset address */
 	err = phy_write(phydev, 0x16, 0x0);
 	if (err < 0)
@@ -491,6 +578,10 @@ static int m88e1145_config_init(struct phy_device *phydev)
 		}
 	}
 
+	err = marvell_of_reg_init(phydev);
+	if (err < 0)
+		return err;
+
 	return 0;
 }
 
-- 
1.7.2.3

Re: [PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[David Daney]

On 11/17/2010 03:54 PM, David Daney wrote:
> Some aspects of PHY initialization are board dependent, things like
> indicator LED connections and some clocking modes cannot be determined
> by probing.  The dev_flags element of struct phy_device can be used to
> control these things if an appropriate value can be passed from the
> Ethernet driver.  We run into problems however if the PHY connections
> are specified by the device tree.  There is no way for the Ethernet
> driver to know what flags it should pass.
>
> If we are using the device tree, the struct phy_device will be
> populated with the device tree node corresponding to the PHY, and we
> can extract extra configuration information from there.
>
> The next question is what should the format of that information be?
> It is highly device specific, and the device tree representation
> should not be tied to any arbitrary kernel defined constants.  A
> straight forward representation is just to specify the exact bits that
> should be set using the "marvell,reg-init" property:
>
>        phy5: ethernet-phy@5 {
> 	reg =<5>;
> 	device_type = "ethernet-phy";
> 	marvell,reg-init =
> 		<0x00030010 0x5777>, /* Reg 3,16<- 0x5777 */
> 		<0x00030011 0x00aa>, /* Reg 3,17<- 0x00aa */
> 		<0x00030012 0x4105>, /* Reg 3,18<- 0x4105 */
> 		<0x00030013 0x0060>; /* Reg 3,19<- 0x0060 */
> 		<0x00020015 0x00300000>; /* clear bits 4..5 of Reg 2,21 */

Well, of course these mask bits are reversed.  That last line should 
really be:

  	<0x00020015 0xffcf0000>; /* clear bits 4..5 of Reg 2,21 */


>        };
>
> The Marvell PHYs have a page select register at register 22 (0x16), we
> can specify any register by its page and register number.  These are
> encoded in the high and low parts of the first word.  The second word
> contains a mask and value to be ORed in its high and low parts.
> property, the PHY initialization is unchanged.
[...]

David Daney

Re: [PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[David Daney]

On 11/17/2010 03:54 PM, David Daney wrote:
> Some aspects of PHY initialization are board dependent, things like
> indicator LED connections and some clocking modes cannot be determined
> by probing.  The dev_flags element of struct phy_device can be used to
> control these things if an appropriate value can be passed from the
> Ethernet driver.  We run into problems however if the PHY connections
> are specified by the device tree.  There is no way for the Ethernet
> driver to know what flags it should pass.
>
> If we are using the device tree, the struct phy_device will be
> populated with the device tree node corresponding to the PHY, and we
> can extract extra configuration information from there.
>
> The next question is what should the format of that information be?
> It is highly device specific, and the device tree representation
> should not be tied to any arbitrary kernel defined constants.  A
> straight forward representation is just to specify the exact bits that
> should be set using the "marvell,reg-init" property:
>
>        phy5: ethernet-phy@5 {
> 	reg =<5>;
> 	device_type = "ethernet-phy";
> 	marvell,reg-init =
> 		<0x00030010 0x5777>, /* Reg 3,16<- 0x5777 */
> 		<0x00030011 0x00aa>, /* Reg 3,17<- 0x00aa */
> 		<0x00030012 0x4105>, /* Reg 3,18<- 0x4105 */
> 		<0x00030013 0x0060>; /* Reg 3,19<- 0x0060 */
> 		<0x00020015 0x00300000>; /* clear bits 4..5 of Reg 2,21 */

Well, of course these mask bits are reversed.  That last line should 
really be:

  	<0x00020015 0xffcf0000>; /* clear bits 4..5 of Reg 2,21 */


>        };
>
> The Marvell PHYs have a page select register at register 22 (0x16), we
> can specify any register by its page and register number.  These are
> encoded in the high and low parts of the first word.  The second word
> contains a mask and value to be ORed in its high and low parts.
> property, the PHY initialization is unchanged.
[...]

David Daney

Re: [1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[Milton Miller]

On Wed, 17 Nov 2010 15:54:30 -0800, David Daney wrote:
> Some aspects of PHY initialization are board dependent, things like
> indicator LED connections and some clocking modes cannot be determined
> by probing.  The dev_flags element of struct phy_device can be used to
> control these things if an appropriate value can be passed from the
> Ethernet driver.  We run into problems however if the PHY connections
> are specified by the device tree.  There is no way for the Ethernet
> driver to know what flags it should pass.
> 
> If we are using the device tree, the struct phy_device will be
> populated with the device tree node corresponding to the PHY, and we
> can extract extra configuration information from there.
..
> 
> ---
> drivers/net/phy/marvell.c |   91 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 91 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f0bd1a1..33ad654 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -30,6 +30,7 @@
>  #include <linux/ethtool.h>
>  #include <linux/phy.h>
>  #include <linux/marvell_phy.h>
> +#include <linux/of.h>
>  
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -186,6 +187,85 @@ static int marvell_config_aneg(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +#ifndef CONFIG_OF
> +static int marvell_of_reg_init(struct phy_device *phydev)
> +{
> +	return 0;
> +}
> +#else
> +/*
> + * Set and/or override some configuration registers based on the
> + * marvell,reg-init property stored in the of_node for the phydev.
> + *
> + * marvell,reg-init = <reg-spec val-spec>,...;
> + *
> + * There may be one or more  pairs of <reg-spec val-spec>:
> + * reg-spec [16..31]: Page address.
> + * reg-spec [0..15]: Register address.
> + *
> + * val-spec [16..31]: Mask bits.
> + * val-spec [0..15]: Register bits.
> + */
> +static int marvell_of_reg_init(struct phy_device *phydev)
> +{
> +	const __be32 *paddr;
> +	int len, i, saved_page, current_page, page_changed, ret;
> +
> +	if (!phydev->dev.of_node)
> +		return 0;
> +
> +	paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init", &len);
> +	if (!paddr || len < (2 * sizeof(u32)))
> +		return 0;
> +
> +	saved_page = phy_read(phydev, 22);
> +	if (saved_page < 0)
> +		return saved_page;
> +	page_changed = 0;
> +	current_page = saved_page;
> +
> +	ret = 0;
> +	len /= sizeof(u32);
> +	for (i = 0; i < len / 2; i += 2) {

i < len - 1 would execute all the register inits specified in the property.

> +		u32 reg_spec =  be32_to_cpup(&paddr[i]);
> +		u32 val_spec =  be32_to_cpup(&paddr[i + 1]);
> +		u16 reg = reg_spec & 0xffff;
> +		u16 reg_page = reg_spec >> 16;
> +		u16 val_bits = val_spec & 0xffff;
> +		u16 mask = val_spec >> 16;
> +		int val;
> +

While the outcome is the same, this code also mixes sizeof(u32) 
with __be32 pointer math.

milton

Re: [1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[David Daney]

On 11/17/2010 09:38 PM, Milton Miller wrote:
[...]
>> +static int marvell_of_reg_init(struct phy_device *phydev)
>> +{
>> +	const __be32 *paddr;
>> +	int len, i, saved_page, current_page, page_changed, ret;
>> +
>> +	if (!phydev->dev.of_node)
>> +		return 0;
>> +
>> +	paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init",&len);
>> +	if (!paddr || len<  (2 * sizeof(u32)))
>> +		return 0;
>> +
>> +	saved_page = phy_read(phydev, 22);
>> +	if (saved_page<  0)
>> +		return saved_page;
>> +	page_changed = 0;
>> +	current_page = saved_page;
>> +
>> +	ret = 0;
>> +	len /= sizeof(u32);
>> +	for (i = 0; i<  len / 2; i += 2) {
>
> i<  len - 1 would execute all the register inits specified in the property.
>

Right, a bit of a snafu I think.  I will fix it.


>> +		u32 reg_spec =  be32_to_cpup(&paddr[i]);
>> +		u32 val_spec =  be32_to_cpup(&paddr[i + 1]);
>> +		u16 reg = reg_spec&  0xffff;
>> +		u16 reg_page = reg_spec>>  16;
>> +		u16 val_bits = val_spec&  0xffff;
>> +		u16 mask = val_spec>>  16;
>> +		int val;
>> +
>
> While the outcome is the same, this code also mixes sizeof(u32)
> with __be32 pointer math.

I will think about how to make it more consistent.

Thank you for looking at it,
David Daney

Re: [1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[David Daney]

On 11/17/2010 09:38 PM, Milton Miller wrote:
[...]
>> +static int marvell_of_reg_init(struct phy_device *phydev)
>> +{
>> +	const __be32 *paddr;
>> +	int len, i, saved_page, current_page, page_changed, ret;
>> +
>> +	if (!phydev->dev.of_node)
>> +		return 0;
>> +
>> +	paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init",&len);
>> +	if (!paddr || len<  (2 * sizeof(u32)))
>> +		return 0;
>> +
>> +	saved_page = phy_read(phydev, 22);
>> +	if (saved_page<  0)
>> +		return saved_page;
>> +	page_changed = 0;
>> +	current_page = saved_page;
>> +
>> +	ret = 0;
>> +	len /= sizeof(u32);
>> +	for (i = 0; i<  len / 2; i += 2) {
>
> i<  len - 1 would execute all the register inits specified in the property.
>

Right, a bit of a snafu I think.  I will fix it.


>> +		u32 reg_spec =  be32_to_cpup(&paddr[i]);
>> +		u32 val_spec =  be32_to_cpup(&paddr[i + 1]);
>> +		u16 reg = reg_spec&  0xffff;
>> +		u16 reg_page = reg_spec>>  16;
>> +		u16 val_bits = val_spec&  0xffff;
>> +		u16 mask = val_spec>>  16;
>> +		int val;
>> +
>
> While the outcome is the same, this code also mixes sizeof(u32)
> with __be32 pointer math.

I will think about how to make it more consistent.

Thank you for looking at it,
David Daney

Re: [PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[Cyril Chemparathy]

On 11/17/2010 06:54 PM, David Daney wrote:
> Some aspects of PHY initialization are board dependent, things like
> indicator LED connections and some clocking modes cannot be determined
> by probing.  The dev_flags element of struct phy_device can be used to
> control these things if an appropriate value can be passed from the
> Ethernet driver.  We run into problems however if the PHY connections
> are specified by the device tree.  There is no way for the Ethernet
> driver to know what flags it should pass.
> 
> If we are using the device tree, the struct phy_device will be
> populated with the device tree node corresponding to the PHY, and we
> can extract extra configuration information from there.
> 
> The next question is what should the format of that information be?
> It is highly device specific, and the device tree representation
> should not be tied to any arbitrary kernel defined constants.  A
> straight forward representation is just to specify the exact bits that
> should be set using the "marvell,reg-init" property:
> 
>       phy5: ethernet-phy@5 {
> 	reg = <5>;
> 	device_type = "ethernet-phy";
> 	marvell,reg-init =
> 		<0x00030010 0x5777>, /* Reg 3,16 <- 0x5777 */
> 		<0x00030011 0x00aa>, /* Reg 3,17 <- 0x00aa */
> 		<0x00030012 0x4105>, /* Reg 3,18 <- 0x4105 */
> 		<0x00030013 0x0060>; /* Reg 3,19 <- 0x0060 */
> 		<0x00020015 0x00300000>; /* clear bits 4..5 of Reg 2,21 */
>       };
> 
> The Marvell PHYs have a page select register at register 22 (0x16), we
> can specify any register by its page and register number.  These are
> encoded in the high and low parts of the first word.  The second word
> contains a mask and value to be ORed in its high and low parts.  The
> new marvell_of_reg_init function leaves the page select register
> unchanged, so a call to it can be dropped into the .config_init
> functions without unduly affecting the state of the PHY.
> 
> If CONFIG_OF is not set, there is no of_node, or no "marvell,reg-init"
> property, the PHY initialization is unchanged.
> 
> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Cyril Chemparathy <cyril@ti.com>
> Cc: David Daney <ddaney@caviumnetworks.com>
> Cc: Arnaud Patard <arnaud.patard@rtp-net.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/net/phy/marvell.c |   91 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 91 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f0bd1a1..33ad654 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -30,6 +30,7 @@
>  #include <linux/ethtool.h>
>  #include <linux/phy.h>
>  #include <linux/marvell_phy.h>
> +#include <linux/of.h>
>  
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -186,6 +187,85 @@ static int marvell_config_aneg(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +#ifndef CONFIG_OF
> +static int marvell_of_reg_init(struct phy_device *phydev)
> +{
> +	return 0;
> +}
> +#else
> +/*
> + * Set and/or override some configuration registers based on the
> + * marvell,reg-init property stored in the of_node for the phydev.
> + *
> + * marvell,reg-init = <reg-spec val-spec>,...;
> + *
> + * There may be one or more  pairs of <reg-spec val-spec>:
> + * reg-spec [16..31]: Page address.
> + * reg-spec [0..15]: Register address.
> + *
> + * val-spec [16..31]: Mask bits.
> + * val-spec [0..15]: Register bits.
> + */
> +static int marvell_of_reg_init(struct phy_device *phydev)
> +{
> +	const __be32 *paddr;
> +	int len, i, saved_page, current_page, page_changed, ret;
> +
> +	if (!phydev->dev.of_node)
> +		return 0;
> +
> +	paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init", &len);
> +	if (!paddr || len < (2 * sizeof(u32)))
> +		return 0;
> +
> +	saved_page = phy_read(phydev, 22);

Please use MII_88E1121_PHY_PAGE (renamed if necessary) instead of 22 here.

> +	if (saved_page < 0)
> +		return saved_page;
> +	page_changed = 0;
> +	current_page = saved_page;
> +
> +	ret = 0;
> +	len /= sizeof(u32);
> +	for (i = 0; i < len / 2; i += 2) {
> +		u32 reg_spec =  be32_to_cpup(&paddr[i]);
> +		u32 val_spec =  be32_to_cpup(&paddr[i + 1]);
> +		u16 reg = reg_spec & 0xffff;
> +		u16 reg_page = reg_spec >> 16;
> +		u16 val_bits = val_spec & 0xffff;
> +		u16 mask = val_spec >> 16;
> +		int val;
> +
> +		if (reg_page != current_page) {
> +			ret = phy_write(phydev, 22, reg_page);
> +			if (ret < 0)
> +				goto err;
> +			current_page = reg_page;
> +			page_changed = 1;
> +		}
> +
> +		val = 0;
> +		if (mask) {
> +			val = phy_read(phydev, reg);

If this phy_read() were to fail...

> +			if (val < 0) {
> +				ret = val;
> +				goto err;
> +			}
> +			val &= mask;
> +		}
> +		val |= val_bits;
> +
> +		ret = phy_write(phydev, reg, (u16)val);
> +		if (ret < 0)
> +			goto err;
> +
> +	}
> +err:
> +	if (page_changed)
> +		ret = phy_write(phydev, 22, saved_page);
> +	return ret;

... this function may incorrectly return success if the phy_write()
succeeds.

Some mdio controllers verify turn around during read operations and
return failure if the phy fails to drive the data line low.  On write
transactions turn around is driven by the controller, and failures may
go undetected.

[...]

Regards
- Cyril.

Re: [PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[Grant Likely]

On Wed, Nov 17, 2010 at 03:54:30PM -0800, David Daney wrote:
> Some aspects of PHY initialization are board dependent, things like
> indicator LED connections and some clocking modes cannot be determined
> by probing.  The dev_flags element of struct phy_device can be used to
> control these things if an appropriate value can be passed from the
> Ethernet driver.  We run into problems however if the PHY connections
> are specified by the device tree.  There is no way for the Ethernet
> driver to know what flags it should pass.
> 
> If we are using the device tree, the struct phy_device will be
> populated with the device tree node corresponding to the PHY, and we
> can extract extra configuration information from there.
> 
> The next question is what should the format of that information be?
> It is highly device specific, and the device tree representation
> should not be tied to any arbitrary kernel defined constants.  A
> straight forward representation is just to specify the exact bits that
> should be set using the "marvell,reg-init" property:
> 
>       phy5: ethernet-phy@5 {
> 	reg = <5>;
> 	device_type = "ethernet-phy";

Some notes:
- device_type is only relevant for real openfirmware platforms.  It
  should not appear in dts files.
- This example phy node needs a compatible property
- This new binding needs to be documented.  You can use devicetree.org.

> 	marvell,reg-init =
> 		<0x00030010 0x5777>, /* Reg 3,16 <- 0x5777 */
> 		<0x00030011 0x00aa>, /* Reg 3,17 <- 0x00aa */
> 		<0x00030012 0x4105>, /* Reg 3,18 <- 0x4105 */
> 		<0x00030013 0x0060>; /* Reg 3,19 <- 0x0060 */
> 		<0x00020015 0x00300000>; /* clear bits 4..5 of Reg 2,21 */
>       };
> 
> The Marvell PHYs have a page select register at register 22 (0x16), we
> can specify any register by its page and register number.  These are
> encoded in the high and low parts of the first word.  The second word
> contains a mask and value to be ORed in its high and low parts.  The
> new marvell_of_reg_init function leaves the page select register
> unchanged, so a call to it can be dropped into the .config_init
> functions without unduly affecting the state of the PHY.

Don't bother trying to pack the values tightly.  Just use one cell per
value. ie: marvell,reg-init = < [page] [register] [mask] [value] >;

> 
> If CONFIG_OF is not set, there is no of_node, or no "marvell,reg-init"
> property, the PHY initialization is unchanged.
> 
> Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> Cc: Grant Likely <grant.likely@secretlab.ca>
> Cc: Cyril Chemparathy <cyril@ti.com>
> Cc: David Daney <ddaney@caviumnetworks.com>
> Cc: Arnaud Patard <arnaud.patard@rtp-net.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/net/phy/marvell.c |   91 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 91 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f0bd1a1..33ad654 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -30,6 +30,7 @@
>  #include <linux/ethtool.h>
>  #include <linux/phy.h>
>  #include <linux/marvell_phy.h>
> +#include <linux/of.h>
>  
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -186,6 +187,85 @@ static int marvell_config_aneg(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +#ifndef CONFIG_OF

ifndef CONFIG_OF_MDIO

Nit: I usually see these blocks constructed the other way around.  Put
the true case first, and then follow it up with the empty false case.

> +static int marvell_of_reg_init(struct phy_device *phydev)
> +{
> +	return 0;
> +}
> +#else
> +/*
> + * Set and/or override some configuration registers based on the
> + * marvell,reg-init property stored in the of_node for the phydev.
> + *
> + * marvell,reg-init = <reg-spec val-spec>,...;
> + *
> + * There may be one or more  pairs of <reg-spec val-spec>:
> + * reg-spec [16..31]: Page address.
> + * reg-spec [0..15]: Register address.
> + *
> + * val-spec [16..31]: Mask bits.
> + * val-spec [0..15]: Register bits.
> + */
> +static int marvell_of_reg_init(struct phy_device *phydev)
> +{
> +	const __be32 *paddr;
> +	int len, i, saved_page, current_page, page_changed, ret;
> +
> +	if (!phydev->dev.of_node)
> +		return 0;
> +
> +	paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init", &len);
> +	if (!paddr || len < (2 * sizeof(u32)))
> +		return 0;
> +
> +	saved_page = phy_read(phydev, 22);
> +	if (saved_page < 0)
> +		return saved_page;
> +	page_changed = 0;
> +	current_page = saved_page;
> +
> +	ret = 0;
> +	len /= sizeof(u32);
> +	for (i = 0; i < len / 2; i += 2) {
> +		u32 reg_spec =  be32_to_cpup(&paddr[i]);
> +		u32 val_spec =  be32_to_cpup(&paddr[i + 1]);
> +		u16 reg = reg_spec & 0xffff;
> +		u16 reg_page = reg_spec >> 16;
> +		u16 val_bits = val_spec & 0xffff;
> +		u16 mask = val_spec >> 16;
> +		int val;
> +
> +		if (reg_page != current_page) {
> +			ret = phy_write(phydev, 22, reg_page);
> +			if (ret < 0)
> +				goto err;
> +			current_page = reg_page;
> +			page_changed = 1;
> +		}
> +
> +		val = 0;
> +		if (mask) {
> +			val = phy_read(phydev, reg);
> +			if (val < 0) {
> +				ret = val;
> +				goto err;
> +			}
> +			val &= mask;
> +		}
> +		val |= val_bits;
> +
> +		ret = phy_write(phydev, reg, (u16)val);
> +		if (ret < 0)
> +			goto err;
> +
> +	}
> +err:
> +	if (page_changed)
> +		ret = phy_write(phydev, 22, saved_page);

This throws away the previous error code if a single write fails.  It
may return a false success.

> +	return ret;
> +}
> +#endif /* CONFIG_OF */
> +
>  static int m88e1121_config_aneg(struct phy_device *phydev)
>  {
>  	int err, oldpage, mscr;
> @@ -368,6 +448,9 @@ static int m88e1111_config_init(struct phy_device *phydev)
>  			return err;
>  	}
>  
> +	err = marvell_of_reg_init(phydev);
> +	if (err < 0)
> +		return err;
>  
>  	err = phy_write(phydev, MII_BMCR, BMCR_RESET);
>  	if (err < 0)
> @@ -420,6 +503,10 @@ static int m88e1118_config_init(struct phy_device *phydev)
>  	if (err < 0)
>  		return err;
>  
> +	err = marvell_of_reg_init(phydev);
> +	if (err < 0)
> +		return err;
> +
>  	/* Reset address */
>  	err = phy_write(phydev, 0x16, 0x0);
>  	if (err < 0)
> @@ -491,6 +578,10 @@ static int m88e1145_config_init(struct phy_device *phydev)
>  		}
>  	}
>  
> +	err = marvell_of_reg_init(phydev);
> +	if (err < 0)
> +		return err;
> +
>  	return 0;
>  }
>  
> -- 
> 1.7.2.3
> 

Re: [PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[Grant Likely]

On Wed, Nov 17, 2010 at 03:54:30PM -0800, David Daney wrote:
> Some aspects of PHY initialization are board dependent, things like
> indicator LED connections and some clocking modes cannot be determined
> by probing.  The dev_flags element of struct phy_device can be used to
> control these things if an appropriate value can be passed from the
> Ethernet driver.  We run into problems however if the PHY connections
> are specified by the device tree.  There is no way for the Ethernet
> driver to know what flags it should pass.
> 
> If we are using the device tree, the struct phy_device will be
> populated with the device tree node corresponding to the PHY, and we
> can extract extra configuration information from there.
> 
> The next question is what should the format of that information be?
> It is highly device specific, and the device tree representation
> should not be tied to any arbitrary kernel defined constants.  A
> straight forward representation is just to specify the exact bits that
> should be set using the "marvell,reg-init" property:
> 
>       phy5: ethernet-phy@5 {
> 	reg = <5>;
> 	device_type = "ethernet-phy";

Some notes:
- device_type is only relevant for real openfirmware platforms.  It
  should not appear in dts files.
- This example phy node needs a compatible property
- This new binding needs to be documented.  You can use devicetree.org.

> 	marvell,reg-init =
> 		<0x00030010 0x5777>, /* Reg 3,16 <- 0x5777 */
> 		<0x00030011 0x00aa>, /* Reg 3,17 <- 0x00aa */
> 		<0x00030012 0x4105>, /* Reg 3,18 <- 0x4105 */
> 		<0x00030013 0x0060>; /* Reg 3,19 <- 0x0060 */
> 		<0x00020015 0x00300000>; /* clear bits 4..5 of Reg 2,21 */
>       };
> 
> The Marvell PHYs have a page select register at register 22 (0x16), we
> can specify any register by its page and register number.  These are
> encoded in the high and low parts of the first word.  The second word
> contains a mask and value to be ORed in its high and low parts.  The
> new marvell_of_reg_init function leaves the page select register
> unchanged, so a call to it can be dropped into the .config_init
> functions without unduly affecting the state of the PHY.

Don't bother trying to pack the values tightly.  Just use one cell per
value. ie: marvell,reg-init = < [page] [register] [mask] [value] >;

> 
> If CONFIG_OF is not set, there is no of_node, or no "marvell,reg-init"
> property, the PHY initialization is unchanged.
> 
> Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
> Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
> Cc: Cyril Chemparathy <cyril-l0cyMroinI0@public.gmane.org>
> Cc: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
> Cc: Arnaud Patard <arnaud.patard-dQbF7i+pzddAfugRpC6u6w@public.gmane.org>
> Cc: Benjamin Herrenschmidt <benh-XVmvHMARGAS8U2dJNN8I7kB+6BGkLq7r@public.gmane.org>
> ---
>  drivers/net/phy/marvell.c |   91 +++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 91 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index f0bd1a1..33ad654 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -30,6 +30,7 @@
>  #include <linux/ethtool.h>
>  #include <linux/phy.h>
>  #include <linux/marvell_phy.h>
> +#include <linux/of.h>
>  
>  #include <asm/io.h>
>  #include <asm/irq.h>
> @@ -186,6 +187,85 @@ static int marvell_config_aneg(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +#ifndef CONFIG_OF

ifndef CONFIG_OF_MDIO

Nit: I usually see these blocks constructed the other way around.  Put
the true case first, and then follow it up with the empty false case.

> +static int marvell_of_reg_init(struct phy_device *phydev)
> +{
> +	return 0;
> +}
> +#else
> +/*
> + * Set and/or override some configuration registers based on the
> + * marvell,reg-init property stored in the of_node for the phydev.
> + *
> + * marvell,reg-init = <reg-spec val-spec>,...;
> + *
> + * There may be one or more  pairs of <reg-spec val-spec>:
> + * reg-spec [16..31]: Page address.
> + * reg-spec [0..15]: Register address.
> + *
> + * val-spec [16..31]: Mask bits.
> + * val-spec [0..15]: Register bits.
> + */
> +static int marvell_of_reg_init(struct phy_device *phydev)
> +{
> +	const __be32 *paddr;
> +	int len, i, saved_page, current_page, page_changed, ret;
> +
> +	if (!phydev->dev.of_node)
> +		return 0;
> +
> +	paddr = of_get_property(phydev->dev.of_node, "marvell,reg-init", &len);
> +	if (!paddr || len < (2 * sizeof(u32)))
> +		return 0;
> +
> +	saved_page = phy_read(phydev, 22);
> +	if (saved_page < 0)
> +		return saved_page;
> +	page_changed = 0;
> +	current_page = saved_page;
> +
> +	ret = 0;
> +	len /= sizeof(u32);
> +	for (i = 0; i < len / 2; i += 2) {
> +		u32 reg_spec =  be32_to_cpup(&paddr[i]);
> +		u32 val_spec =  be32_to_cpup(&paddr[i + 1]);
> +		u16 reg = reg_spec & 0xffff;
> +		u16 reg_page = reg_spec >> 16;
> +		u16 val_bits = val_spec & 0xffff;
> +		u16 mask = val_spec >> 16;
> +		int val;
> +
> +		if (reg_page != current_page) {
> +			ret = phy_write(phydev, 22, reg_page);
> +			if (ret < 0)
> +				goto err;
> +			current_page = reg_page;
> +			page_changed = 1;
> +		}
> +
> +		val = 0;
> +		if (mask) {
> +			val = phy_read(phydev, reg);
> +			if (val < 0) {
> +				ret = val;
> +				goto err;
> +			}
> +			val &= mask;
> +		}
> +		val |= val_bits;
> +
> +		ret = phy_write(phydev, reg, (u16)val);
> +		if (ret < 0)
> +			goto err;
> +
> +	}
> +err:
> +	if (page_changed)
> +		ret = phy_write(phydev, 22, saved_page);

This throws away the previous error code if a single write fails.  It
may return a false success.

> +	return ret;
> +}
> +#endif /* CONFIG_OF */
> +
>  static int m88e1121_config_aneg(struct phy_device *phydev)
>  {
>  	int err, oldpage, mscr;
> @@ -368,6 +448,9 @@ static int m88e1111_config_init(struct phy_device *phydev)
>  			return err;
>  	}
>  
> +	err = marvell_of_reg_init(phydev);
> +	if (err < 0)
> +		return err;
>  
>  	err = phy_write(phydev, MII_BMCR, BMCR_RESET);
>  	if (err < 0)
> @@ -420,6 +503,10 @@ static int m88e1118_config_init(struct phy_device *phydev)
>  	if (err < 0)
>  		return err;
>  
> +	err = marvell_of_reg_init(phydev);
> +	if (err < 0)
> +		return err;
> +
>  	/* Reset address */
>  	err = phy_write(phydev, 0x16, 0x0);
>  	if (err < 0)
> @@ -491,6 +578,10 @@ static int m88e1145_config_init(struct phy_device *phydev)
>  		}
>  	}
>  
> +	err = marvell_of_reg_init(phydev);
> +	if (err < 0)
> +		return err;
> +
>  	return 0;
>  }
>  
> -- 
> 1.7.2.3
> 

Re: [PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[David Daney]

On 11/18/2010 12:40 PM, Grant Likely wrote:
> On Wed, Nov 17, 2010 at 03:54:30PM -0800, David Daney wrote:
>> Some aspects of PHY initialization are board dependent, things like
>> indicator LED connections and some clocking modes cannot be determined
>> by probing.  The dev_flags element of struct phy_device can be used to
>> control these things if an appropriate value can be passed from the
>> Ethernet driver.  We run into problems however if the PHY connections
>> are specified by the device tree.  There is no way for the Ethernet
>> driver to know what flags it should pass.
>>
>> If we are using the device tree, the struct phy_device will be
>> populated with the device tree node corresponding to the PHY, and we
>> can extract extra configuration information from there.
>>
>> The next question is what should the format of that information be?
>> It is highly device specific, and the device tree representation
>> should not be tied to any arbitrary kernel defined constants.  A
>> straight forward representation is just to specify the exact bits that
>> should be set using the "marvell,reg-init" property:
>>
>>        phy5: ethernet-phy@5 {
>> 	reg =<5>;
>> 	device_type = "ethernet-phy";
>
> Some notes:
> - device_type is only relevant for real openfirmware platforms.  It
>    should not appear in dts files.

Documentation/powerpc/dts-bindings/phy.txt says device_type should be 
here.  I can remove it from my patch comment, but should it also be 
removed from the phy.txt file?

> - This example phy node needs a compatible property

Ok, what would you suggest?  Something like:

      compatible = "marvell,88e1145";

I can certainly do that, but I would note that the kernel probes these 
things and would completely ignore the compatible property.

> - This new binding needs to be documented.  You can use devicetree.org.
>

Agreed, I was planning to do that once the patch was approved.

Where would that go?  Vendor:Marvell, or Type:PHY/compatible=marvell,* 
... or somewhere else?

>> 	marvell,reg-init =
>> 		<0x00030010 0x5777>, /* Reg 3,16<- 0x5777 */
>> 		<0x00030011 0x00aa>, /* Reg 3,17<- 0x00aa */
>> 		<0x00030012 0x4105>, /* Reg 3,18<- 0x4105 */
>> 		<0x00030013 0x0060>; /* Reg 3,19<- 0x0060 */
>> 		<0x00020015 0x00300000>; /* clear bits 4..5 of Reg 2,21 */
>>        };
>>


Thanks,
David Daney

Re: [PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[David Daney]

On 11/18/2010 12:40 PM, Grant Likely wrote:
> On Wed, Nov 17, 2010 at 03:54:30PM -0800, David Daney wrote:
>> Some aspects of PHY initialization are board dependent, things like
>> indicator LED connections and some clocking modes cannot be determined
>> by probing.  The dev_flags element of struct phy_device can be used to
>> control these things if an appropriate value can be passed from the
>> Ethernet driver.  We run into problems however if the PHY connections
>> are specified by the device tree.  There is no way for the Ethernet
>> driver to know what flags it should pass.
>>
>> If we are using the device tree, the struct phy_device will be
>> populated with the device tree node corresponding to the PHY, and we
>> can extract extra configuration information from there.
>>
>> The next question is what should the format of that information be?
>> It is highly device specific, and the device tree representation
>> should not be tied to any arbitrary kernel defined constants.  A
>> straight forward representation is just to specify the exact bits that
>> should be set using the "marvell,reg-init" property:
>>
>>        phy5: ethernet-phy@5 {
>> 	reg =<5>;
>> 	device_type = "ethernet-phy";
>
> Some notes:
> - device_type is only relevant for real openfirmware platforms.  It
>    should not appear in dts files.

Documentation/powerpc/dts-bindings/phy.txt says device_type should be 
here.  I can remove it from my patch comment, but should it also be 
removed from the phy.txt file?

> - This example phy node needs a compatible property

Ok, what would you suggest?  Something like:

      compatible = "marvell,88e1145";

I can certainly do that, but I would note that the kernel probes these 
things and would completely ignore the compatible property.

> - This new binding needs to be documented.  You can use devicetree.org.
>

Agreed, I was planning to do that once the patch was approved.

Where would that go?  Vendor:Marvell, or Type:PHY/compatible=marvell,* 
... or somewhere else?

>> 	marvell,reg-init =
>> 		<0x00030010 0x5777>, /* Reg 3,16<- 0x5777 */
>> 		<0x00030011 0x00aa>, /* Reg 3,17<- 0x00aa */
>> 		<0x00030012 0x4105>, /* Reg 3,18<- 0x4105 */
>> 		<0x00030013 0x0060>; /* Reg 3,19<- 0x0060 */
>> 		<0x00020015 0x00300000>; /* clear bits 4..5 of Reg 2,21 */
>>        };
>>


Thanks,
David Daney

Re: [PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[Grant Likely]

On Thu, Nov 18, 2010 at 03:48:38PM -0800, David Daney wrote:
> On 11/18/2010 12:40 PM, Grant Likely wrote:
> >On Wed, Nov 17, 2010 at 03:54:30PM -0800, David Daney wrote:
> >>Some aspects of PHY initialization are board dependent, things like
> >>indicator LED connections and some clocking modes cannot be determined
> >>by probing.  The dev_flags element of struct phy_device can be used to
> >>control these things if an appropriate value can be passed from the
> >>Ethernet driver.  We run into problems however if the PHY connections
> >>are specified by the device tree.  There is no way for the Ethernet
> >>driver to know what flags it should pass.
> >>
> >>If we are using the device tree, the struct phy_device will be
> >>populated with the device tree node corresponding to the PHY, and we
> >>can extract extra configuration information from there.
> >>
> >>The next question is what should the format of that information be?
> >>It is highly device specific, and the device tree representation
> >>should not be tied to any arbitrary kernel defined constants.  A
> >>straight forward representation is just to specify the exact bits that
> >>should be set using the "marvell,reg-init" property:
> >>
> >>       phy5: ethernet-phy@5 {
> >>	reg =<5>;
> >>	device_type = "ethernet-phy";
> >
> >Some notes:
> >- device_type is only relevant for real openfirmware platforms.  It
> >   should not appear in dts files.
> 
> Documentation/powerpc/dts-bindings/phy.txt says device_type should
> be here.  I can remove it from my patch comment, but should it also
> be removed from the phy.txt file?

Heh, I didn't realize it was there.  Yes, it should be removed from
the phy.txt file.  It is clearly wrong.

> 
> >- This example phy node needs a compatible property
> 
> Ok, what would you suggest?  Something like:
> 
>      compatible = "marvell,88e1145";

Yes.

> 
> I can certainly do that, but I would note that the kernel probes
> these things and would completely ignore the compatible property.

That's okay.

> 
> >- This new binding needs to be documented.  You can use devicetree.org.
> >
> 
> Agreed, I was planning to do that once the patch was approved.
> 
> Where would that go?  Vendor:Marvell, or
> Type:PHY/compatible=marvell,* ... or somewhere else?

The page name should be compatible=marvell,88e1145, and it should be
have the vendor and type tags added so it appears in the category
pages.

g.

Re: [PATCH 1/2] of/phylib: Use device tree properties to initialize Marvell PHYs.

[Grant Likely]

On Thu, Nov 18, 2010 at 03:48:38PM -0800, David Daney wrote:
> On 11/18/2010 12:40 PM, Grant Likely wrote:
> >On Wed, Nov 17, 2010 at 03:54:30PM -0800, David Daney wrote:
> >>Some aspects of PHY initialization are board dependent, things like
> >>indicator LED connections and some clocking modes cannot be determined
> >>by probing.  The dev_flags element of struct phy_device can be used to
> >>control these things if an appropriate value can be passed from the
> >>Ethernet driver.  We run into problems however if the PHY connections
> >>are specified by the device tree.  There is no way for the Ethernet
> >>driver to know what flags it should pass.
> >>
> >>If we are using the device tree, the struct phy_device will be
> >>populated with the device tree node corresponding to the PHY, and we
> >>can extract extra configuration information from there.
> >>
> >>The next question is what should the format of that information be?
> >>It is highly device specific, and the device tree representation
> >>should not be tied to any arbitrary kernel defined constants.  A
> >>straight forward representation is just to specify the exact bits that
> >>should be set using the "marvell,reg-init" property:
> >>
> >>       phy5: ethernet-phy@5 {
> >>	reg =<5>;
> >>	device_type = "ethernet-phy";
> >
> >Some notes:
> >- device_type is only relevant for real openfirmware platforms.  It
> >   should not appear in dts files.
> 
> Documentation/powerpc/dts-bindings/phy.txt says device_type should
> be here.  I can remove it from my patch comment, but should it also
> be removed from the phy.txt file?

Heh, I didn't realize it was there.  Yes, it should be removed from
the phy.txt file.  It is clearly wrong.

> 
> >- This example phy node needs a compatible property
> 
> Ok, what would you suggest?  Something like:
> 
>      compatible = "marvell,88e1145";

Yes.

> 
> I can certainly do that, but I would note that the kernel probes
> these things and would completely ignore the compatible property.

That's okay.

> 
> >- This new binding needs to be documented.  You can use devicetree.org.
> >
> 
> Agreed, I was planning to do that once the patch was approved.
> 
> Where would that go?  Vendor:Marvell, or
> Type:PHY/compatible=marvell,* ... or somewhere else?

The page name should be compatible=marvell,88e1145, and it should be
have the vendor and type tags added so it appears in the category
pages.

g.

[PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[David Daney]

The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
.config_aneg function can be shared with 88E1118, but it needs its own
.config_init.

Signed-off-by: David Daney <ddaney@caviumnetworks.com>
---
 drivers/net/phy/marvell.c   |   44 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/marvell_phy.h |    1 +
 2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 33ad654..a0851c7 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -519,6 +519,36 @@ static int m88e1118_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int m88e1149_config_init(struct phy_device *phydev)
+{
+	int err;
+
+	/* Change address */
+	err = phy_write(phydev, 0x16, 0x0002);
+	if (err < 0)
+		return err;
+
+	/* Enable 1000 Mbit */
+	err = phy_write(phydev, 0x15, 0x1048);
+	if (err < 0)
+		return err;
+
+	err = marvell_of_reg_init(phydev);
+	if (err < 0)
+		return err;
+
+	/* Reset address */
+	err = phy_write(phydev, 0x16, 0x0);
+	if (err < 0)
+		return err;
+
+	err = phy_write(phydev, MII_BMCR, BMCR_RESET);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 static int m88e1145_config_init(struct phy_device *phydev)
 {
 	int err;
@@ -776,6 +806,19 @@ static struct phy_driver marvell_drivers[] = {
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
+		.phy_id = MARVELL_PHY_ID_88E1149R,
+		.phy_id_mask = MARVELL_PHY_ID_MASK,
+		.name = "Marvell 88E1149R",
+		.features = PHY_GBIT_FEATURES,
+		.flags = PHY_HAS_INTERRUPT,
+		.config_init = &m88e1149_config_init,
+		.config_aneg = &m88e1118_config_aneg,
+		.read_status = &genphy_read_status,
+		.ack_interrupt = &marvell_ack_interrupt,
+		.config_intr = &marvell_config_intr,
+		.driver = { .owner = THIS_MODULE },
+	},
+	{
 		.phy_id = MARVELL_PHY_ID_88E1240,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1240",
@@ -826,6 +869,7 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ 0x01410e10, 0xfffffff0 },
 	{ 0x01410cb0, 0xfffffff0 },
 	{ 0x01410cd0, 0xfffffff0 },
+	{ 0x01410e50, 0xfffffff0 },
 	{ 0x01410e30, 0xfffffff0 },
 	{ 0x01410e90, 0xfffffff0 },
 	{ }
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 1ff81b5..dd3c34e 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -11,6 +11,7 @@
 #define MARVELL_PHY_ID_88E1118		0x01410e10
 #define MARVELL_PHY_ID_88E1121R		0x01410cb0
 #define MARVELL_PHY_ID_88E1145		0x01410cd0
+#define MARVELL_PHY_ID_88E1149R		0x01410e50
 #define MARVELL_PHY_ID_88E1240		0x01410e30
 #define MARVELL_PHY_ID_88E1318S		0x01410e90
 
-- 
1.7.2.3

[PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[David Daney]

The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
.config_aneg function can be shared with 88E1118, but it needs its own
.config_init.

Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
---
 drivers/net/phy/marvell.c   |   44 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/marvell_phy.h |    1 +
 2 files changed, 45 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 33ad654..a0851c7 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -519,6 +519,36 @@ static int m88e1118_config_init(struct phy_device *phydev)
 	return 0;
 }
 
+static int m88e1149_config_init(struct phy_device *phydev)
+{
+	int err;
+
+	/* Change address */
+	err = phy_write(phydev, 0x16, 0x0002);
+	if (err < 0)
+		return err;
+
+	/* Enable 1000 Mbit */
+	err = phy_write(phydev, 0x15, 0x1048);
+	if (err < 0)
+		return err;
+
+	err = marvell_of_reg_init(phydev);
+	if (err < 0)
+		return err;
+
+	/* Reset address */
+	err = phy_write(phydev, 0x16, 0x0);
+	if (err < 0)
+		return err;
+
+	err = phy_write(phydev, MII_BMCR, BMCR_RESET);
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+
 static int m88e1145_config_init(struct phy_device *phydev)
 {
 	int err;
@@ -776,6 +806,19 @@ static struct phy_driver marvell_drivers[] = {
 		.driver = { .owner = THIS_MODULE },
 	},
 	{
+		.phy_id = MARVELL_PHY_ID_88E1149R,
+		.phy_id_mask = MARVELL_PHY_ID_MASK,
+		.name = "Marvell 88E1149R",
+		.features = PHY_GBIT_FEATURES,
+		.flags = PHY_HAS_INTERRUPT,
+		.config_init = &m88e1149_config_init,
+		.config_aneg = &m88e1118_config_aneg,
+		.read_status = &genphy_read_status,
+		.ack_interrupt = &marvell_ack_interrupt,
+		.config_intr = &marvell_config_intr,
+		.driver = { .owner = THIS_MODULE },
+	},
+	{
 		.phy_id = MARVELL_PHY_ID_88E1240,
 		.phy_id_mask = MARVELL_PHY_ID_MASK,
 		.name = "Marvell 88E1240",
@@ -826,6 +869,7 @@ static struct mdio_device_id __maybe_unused marvell_tbl[] = {
 	{ 0x01410e10, 0xfffffff0 },
 	{ 0x01410cb0, 0xfffffff0 },
 	{ 0x01410cd0, 0xfffffff0 },
+	{ 0x01410e50, 0xfffffff0 },
 	{ 0x01410e30, 0xfffffff0 },
 	{ 0x01410e90, 0xfffffff0 },
 	{ }
diff --git a/include/linux/marvell_phy.h b/include/linux/marvell_phy.h
index 1ff81b5..dd3c34e 100644
--- a/include/linux/marvell_phy.h
+++ b/include/linux/marvell_phy.h
@@ -11,6 +11,7 @@
 #define MARVELL_PHY_ID_88E1118		0x01410e10
 #define MARVELL_PHY_ID_88E1121R		0x01410cb0
 #define MARVELL_PHY_ID_88E1145		0x01410cd0
+#define MARVELL_PHY_ID_88E1149R		0x01410e50
 #define MARVELL_PHY_ID_88E1240		0x01410e30
 #define MARVELL_PHY_ID_88E1318S		0x01410e90
 
-- 
1.7.2.3

Re: [PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[David Miller]

From: David Daney <ddaney@caviumnetworks.com>
Date: Wed, 17 Nov 2010 15:54:31 -0800

> The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
> .config_aneg function can be shared with 88E1118, but it needs its own
> .config_init.
> 
> Signed-off-by: David Daney <ddaney@caviumnetworks.com>

Please resend this when you've respun patch #1 based upon the feedback
you've been given.

Thanks.

Re: [PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[David Miller]

From: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
Date: Wed, 17 Nov 2010 15:54:31 -0800

> The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
> .config_aneg function can be shared with 88E1118, but it needs its own
> .config_init.
> 
> Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>

Please resend this when you've respun patch #1 based upon the feedback
you've been given.

Thanks.

Re: [PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[Grant Likely]

On Thu, Nov 18, 2010 at 11:46:16AM -0800, David Miller wrote:
> From: David Daney <ddaney@caviumnetworks.com>
> Date: Wed, 17 Nov 2010 15:54:31 -0800
> 
> > The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
> > .config_aneg function can be shared with 88E1118, but it needs its own
> > .config_init.
> > 
> > Signed-off-by: David Daney <ddaney@caviumnetworks.com>
> 
> Please resend this when you've respun patch #1 based upon the feedback
> you've been given.

It looks to me that this patch has no dependencies on the first patch.
ddaney; what say you?

g.

Re: [PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[Grant Likely]

On Thu, Nov 18, 2010 at 11:46:16AM -0800, David Miller wrote:
> From: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
> Date: Wed, 17 Nov 2010 15:54:31 -0800
> 
> > The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
> > .config_aneg function can be shared with 88E1118, but it needs its own
> > .config_init.
> > 
> > Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
> 
> Please resend this when you've respun patch #1 based upon the feedback
> you've been given.

It looks to me that this patch has no dependencies on the first patch.
ddaney; what say you?

g.

Re: [PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[David Miller]

From: Grant Likely <grant.likely@secretlab.ca>
Date: Thu, 18 Nov 2010 13:44:10 -0700

> On Thu, Nov 18, 2010 at 11:46:16AM -0800, David Miller wrote:
>> From: David Daney <ddaney@caviumnetworks.com>
>> Date: Wed, 17 Nov 2010 15:54:31 -0800
>> 
>> > The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
>> > .config_aneg function can be shared with 88E1118, but it needs its own
>> > .config_init.
>> > 
>> > Signed-off-by: David Daney <ddaney@caviumnetworks.com>
>> 
>> Please resend this when you've respun patch #1 based upon the feedback
>> you've been given.
> 
> It looks to me that this patch has no dependencies on the first patch.
> ddaney; what say you?

It absolutely does, it references a function create by patch #1

In fact it's the whole _entire_ point of patch #1, to allow
patch #2 to be possible.  Did you even check?

Otherwise I would waste his time asking for a respin.

Re: [PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[David Miller]

From: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Date: Thu, 18 Nov 2010 13:44:10 -0700

> On Thu, Nov 18, 2010 at 11:46:16AM -0800, David Miller wrote:
>> From: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
>> Date: Wed, 17 Nov 2010 15:54:31 -0800
>> 
>> > The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
>> > .config_aneg function can be shared with 88E1118, but it needs its own
>> > .config_init.
>> > 
>> > Signed-off-by: David Daney <ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
>> 
>> Please resend this when you've respun patch #1 based upon the feedback
>> you've been given.
> 
> It looks to me that this patch has no dependencies on the first patch.
> ddaney; what say you?

It absolutely does, it references a function create by patch #1

In fact it's the whole _entire_ point of patch #1, to allow
patch #2 to be possible.  Did you even check?

Otherwise I would waste his time asking for a respin.

Re: [PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[David Daney]

On 11/18/2010 12:44 PM, Grant Likely wrote:
> On Thu, Nov 18, 2010 at 11:46:16AM -0800, David Miller wrote:
>> From: David Daney<ddaney@caviumnetworks.com>
>> Date: Wed, 17 Nov 2010 15:54:31 -0800
>>
>>> The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
>>> .config_aneg function can be shared with 88E1118, but it needs its own
>>> .config_init.
>>>
>>> Signed-off-by: David Daney<ddaney@caviumnetworks.com>
>>
>> Please resend this when you've respun patch #1 based upon the feedback
>> you've been given.
>
> It looks to me that this patch has no dependencies on the first patch.
> ddaney; what say you?
>

It calls the marvell_of_reg_init() function introduced in the first 
patch.  Reordering the patches would be possible, but since nobody else 
has cared enough to add 88E1149R support, it shouldn't affect anyone be 
me.

I arbitrarily ordered it this way.  If davem wishes, I could re-order 
the patches and 88E1149R could be merged before the device tree part.

David Daney

Re: [PATCH 2/2] phylib: Add support for Marvell 88E1149R devices.

[David Daney]

On 11/18/2010 12:44 PM, Grant Likely wrote:
> On Thu, Nov 18, 2010 at 11:46:16AM -0800, David Miller wrote:
>> From: David Daney<ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
>> Date: Wed, 17 Nov 2010 15:54:31 -0800
>>
>>> The 88E1149R is 10/100/1000 quad-gigabit Ethernet PHY.  The
>>> .config_aneg function can be shared with 88E1118, but it needs its own
>>> .config_init.
>>>
>>> Signed-off-by: David Daney<ddaney-M3mlKVOIwJVv6pq1l3V1OdBPR1lH4CV8@public.gmane.org>
>>
>> Please resend this when you've respun patch #1 based upon the feedback
>> you've been given.
>
> It looks to me that this patch has no dependencies on the first patch.
> ddaney; what say you?
>

It calls the marvell_of_reg_init() function introduced in the first 
patch.  Reordering the patches would be possible, but since nobody else 
has cared enough to add 88E1149R support, it shouldn't affect anyone be 
me.

I arbitrarily ordered it this way.  If davem wishes, I could re-order 
the patches and 88E1149R could be merged before the device tree part.

David Daney