[PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Benjamin Herrenschmidt]

Without this change, the network LED doesn't work on the device. The
value itself comes from the vendor kernel.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/phy/marvell.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 64c7fbe..22b1efa 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -34,6 +34,10 @@
 #include <asm/irq.h>
 #include <asm/uaccess.h>
 
+#ifdef CONFIG_ARM
+#include <asm/mach-types.h>
+#endif
+
 #define MII_M1011_IEVENT		0x13
 #define MII_M1011_IEVENT_CLEAR		0x0000
 
@@ -350,7 +354,14 @@ static int m88e1118_config_init(struct phy_device *phydev)
 		return err;
 
 	/* Adjust LED Control */
+#ifdef CONFIG_MACH_DNS323
+	/* The DNS-323 needs a special value in here for the LED to work */
+	if (machine_is_dns323())
+		err = phy_write(phydev, 0x10, 0x1100);
+	else
+#else
 	err = phy_write(phydev, 0x10, 0x021e);
+#endif
 	if (err < 0)
 		return err;
 

[PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Benjamin Herrenschmidt]

Without this change, the network LED doesn't work on the device. The
value itself comes from the vendor kernel.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 drivers/net/phy/marvell.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
index 64c7fbe..22b1efa 100644
--- a/drivers/net/phy/marvell.c
+++ b/drivers/net/phy/marvell.c
@@ -34,6 +34,10 @@
 #include <asm/irq.h>
 #include <asm/uaccess.h>
 
+#ifdef CONFIG_ARM
+#include <asm/mach-types.h>
+#endif
+
 #define MII_M1011_IEVENT		0x13
 #define MII_M1011_IEVENT_CLEAR		0x0000
 
@@ -350,7 +354,14 @@ static int m88e1118_config_init(struct phy_device *phydev)
 		return err;
 
 	/* Adjust LED Control */
+#ifdef CONFIG_MACH_DNS323
+	/* The DNS-323 needs a special value in here for the LED to work */
+	if (machine_is_dns323())
+		err = phy_write(phydev, 0x10, 0x1100);
+	else
+#else
 	err = phy_write(phydev, 0x10, 0x021e);
+#endif
 	if (err < 0)
 		return err;
 

[PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Wolfram Sang]

On Mon, May 17, 2010 at 10:27:38AM +1000, Benjamin Herrenschmidt wrote:
> Without this change, the network LED doesn't work on the device. The
> value itself comes from the vendor kernel.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/net/phy/marvell.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 64c7fbe..22b1efa 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -34,6 +34,10 @@
>  #include <asm/irq.h>
>  #include <asm/uaccess.h>
>  
> +#ifdef CONFIG_ARM
> +#include <asm/mach-types.h>
> +#endif
> +
>  #define MII_M1011_IEVENT		0x13
>  #define MII_M1011_IEVENT_CLEAR		0x0000
>  
> @@ -350,7 +354,14 @@ static int m88e1118_config_init(struct phy_device *phydev)
>  		return err;
>  
>  	/* Adjust LED Control */
> +#ifdef CONFIG_MACH_DNS323
> +	/* The DNS-323 needs a special value in here for the LED to work */
> +	if (machine_is_dns323())
> +		err = phy_write(phydev, 0x10, 0x1100);
> +	else
> +#else
>  	err = phy_write(phydev, 0x10, 0x021e);
> +#endif

There is a fixup()-callback to prevent boardcode in the drivers. See
Documentation/networking/phy.txt, last chapter.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100517/d2b6bf79/attachment.sig>

Re: [PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Wolfram Sang]

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

On Mon, May 17, 2010 at 10:27:38AM +1000, Benjamin Herrenschmidt wrote:
> Without this change, the network LED doesn't work on the device. The
> value itself comes from the vendor kernel.
> 
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> ---
>  drivers/net/phy/marvell.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 64c7fbe..22b1efa 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -34,6 +34,10 @@
>  #include <asm/irq.h>
>  #include <asm/uaccess.h>
>  
> +#ifdef CONFIG_ARM
> +#include <asm/mach-types.h>
> +#endif
> +
>  #define MII_M1011_IEVENT		0x13
>  #define MII_M1011_IEVENT_CLEAR		0x0000
>  
> @@ -350,7 +354,14 @@ static int m88e1118_config_init(struct phy_device *phydev)
>  		return err;
>  
>  	/* Adjust LED Control */
> +#ifdef CONFIG_MACH_DNS323
> +	/* The DNS-323 needs a special value in here for the LED to work */
> +	if (machine_is_dns323())
> +		err = phy_write(phydev, 0x10, 0x1100);
> +	else
> +#else
>  	err = phy_write(phydev, 0x10, 0x021e);
> +#endif

There is a fixup()-callback to prevent boardcode in the drivers. See
Documentation/networking/phy.txt, last chapter.

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

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

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

[PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Benjamin Herrenschmidt]

On Mon, 2010-05-17 at 02:59 +0200, Wolfram Sang wrote:
> There is a fixup()-callback to prevent boardcode in the drivers. See
> Documentation/networking/phy.txt, last chapter.

Ah nice ! I missed that bit. I'll add a fixup and see if it works.

The problem is that writing to this register seems to be part of a
specific initialization sequence, which is done one way in the linux
driver and differently in the vendor kernel. I don't know whether
I can just 'override' the value and I have no docs for that part.

But I'll definitely give it a go tonight.

Cheers,
Ben.

Re: [PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Benjamin Herrenschmidt]

On Mon, 2010-05-17 at 02:59 +0200, Wolfram Sang wrote:
> There is a fixup()-callback to prevent boardcode in the drivers. See
> Documentation/networking/phy.txt, last chapter.

Ah nice ! I missed that bit. I'll add a fixup and see if it works.

The problem is that writing to this register seems to be part of a
specific initialization sequence, which is done one way in the linux
driver and differently in the vendor kernel. I don't know whether
I can just 'override' the value and I have no docs for that part.

But I'll definitely give it a go tonight.

Cheers,
Ben.

[PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Benjamin Herrenschmidt]

On Mon, 2010-05-17 at 11:07 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2010-05-17 at 02:59 +0200, Wolfram Sang wrote:
> > There is a fixup()-callback to prevent boardcode in the drivers. See
> > Documentation/networking/phy.txt, last chapter.
> 
> Ah nice ! I missed that bit. I'll add a fixup and see if it works.
> 
> The problem is that writing to this register seems to be part of a
> specific initialization sequence, which is done one way in the linux
> driver and differently in the vendor kernel. I don't know whether
> I can just 'override' the value and I have no docs for that part.
> 
> But I'll definitely give it a go tonight.

Ok, that doesn't work.

The problem is that the fixups are called -after- the config_init()
callback of the PHY driver. However, the marvell m88e1118 PHY driver
will unconditionally reset the LEDs setting.

So either we add a layer of PHY fixups to run after init, or I replace
the init completely in my fixup routine. A way to do that would be to do
a small change to allow the fixup code to request the core to skip
config_init().

Something along those lines:

net/phy: Allow platform fixups to completely override the PHY init routine

The fixups are called before the PHY init routine. In some case, that
means that whatever LEDs setup they attempt to do is undone by the said
initialization code.

This patch allows to work around it by enabling the fixups to return the
positive PHY_FIXUP_SKIP_INIT result code, which will cause the core to
skip the normal init routine.

Using this, a platform fixup can effectively replace the entire init
routine for a given PHY.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
--- 

(untested btw)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 64be466..48436e6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -353,10 +353,9 @@ int phy_mii_ioctl(struct phy_device *phydev,
 		phy_write(phydev, mii_data->reg_num, val);
 		
 		if (mii_data->reg_num == MII_BMCR &&
-		    val & BMCR_RESET &&
-		    phydev->drv->config_init) {
-			phy_scan_fixups(phydev);
-			phydev->drv->config_init(phydev);
+		    val & BMCR_RESET && phydev->drv->config_init) {
+			if (phy_scan_fixups(phydev) != PHY_FIXUP_SKIP_INIT)
+				phydev->drv->config_init(phydev);
 		}
 		break;
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index db17945..44ab890 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -126,6 +126,7 @@ static int phy_needs_fixup(struct phy_device *phydev, struct phy_fixup *fixup)
 int phy_scan_fixups(struct phy_device *phydev)
 {
 	struct phy_fixup *fixup;
+	int rc = 0;
 
 	mutex_lock(&phy_fixup_lock);
 	list_for_each_entry(fixup, &phy_fixup_list, list) {
@@ -138,11 +139,13 @@ int phy_scan_fixups(struct phy_device *phydev)
 				mutex_unlock(&phy_fixup_lock);
 				return err;
 			}
+			if (err == PHY_FIXUP_SKIP_INIT)
+				rc = err;
 		}
 	}
 	mutex_unlock(&phy_fixup_lock);
 
-	return 0;
+	return rc;
 }
 EXPORT_SYMBOL(phy_scan_fixups);
 
@@ -405,6 +408,8 @@ int phy_init_hw(struct phy_device *phydev)
 	ret = phy_scan_fixups(phydev);
 	if (ret < 0)
 		return ret;
+	if (ret == PHY_FIXUP_SKIP_INIT)
+		return 0;
 
 	return phydev->drv->config_init(phydev);
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 14d7fdf..5e2b026 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -413,6 +413,12 @@ struct phy_fixup {
 	int (*run)(struct phy_device *phydev);
 };
 
+/* The fixup can return this to skip the PHY driver init routine
+ * (ie. the fixup effectively replaces the init routine)
+ */
+#define PHY_FIXUP_SKIP_INIT	1
+
+
 /**
  * phy_read - Convenience function for reading a given PHY register
  * @phydev: the phy_device struct

Re: [PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Benjamin Herrenschmidt]

On Mon, 2010-05-17 at 11:07 +1000, Benjamin Herrenschmidt wrote:
> On Mon, 2010-05-17 at 02:59 +0200, Wolfram Sang wrote:
> > There is a fixup()-callback to prevent boardcode in the drivers. See
> > Documentation/networking/phy.txt, last chapter.
> 
> Ah nice ! I missed that bit. I'll add a fixup and see if it works.
> 
> The problem is that writing to this register seems to be part of a
> specific initialization sequence, which is done one way in the linux
> driver and differently in the vendor kernel. I don't know whether
> I can just 'override' the value and I have no docs for that part.
> 
> But I'll definitely give it a go tonight.

Ok, that doesn't work.

The problem is that the fixups are called -after- the config_init()
callback of the PHY driver. However, the marvell m88e1118 PHY driver
will unconditionally reset the LEDs setting.

So either we add a layer of PHY fixups to run after init, or I replace
the init completely in my fixup routine. A way to do that would be to do
a small change to allow the fixup code to request the core to skip
config_init().

Something along those lines:

net/phy: Allow platform fixups to completely override the PHY init routine

The fixups are called before the PHY init routine. In some case, that
means that whatever LEDs setup they attempt to do is undone by the said
initialization code.

This patch allows to work around it by enabling the fixups to return the
positive PHY_FIXUP_SKIP_INIT result code, which will cause the core to
skip the normal init routine.

Using this, a platform fixup can effectively replace the entire init
routine for a given PHY.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
--- 

(untested btw)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 64be466..48436e6 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -353,10 +353,9 @@ int phy_mii_ioctl(struct phy_device *phydev,
 		phy_write(phydev, mii_data->reg_num, val);
 		
 		if (mii_data->reg_num == MII_BMCR &&
-		    val & BMCR_RESET &&
-		    phydev->drv->config_init) {
-			phy_scan_fixups(phydev);
-			phydev->drv->config_init(phydev);
+		    val & BMCR_RESET && phydev->drv->config_init) {
+			if (phy_scan_fixups(phydev) != PHY_FIXUP_SKIP_INIT)
+				phydev->drv->config_init(phydev);
 		}
 		break;
 
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index db17945..44ab890 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -126,6 +126,7 @@ static int phy_needs_fixup(struct phy_device *phydev, struct phy_fixup *fixup)
 int phy_scan_fixups(struct phy_device *phydev)
 {
 	struct phy_fixup *fixup;
+	int rc = 0;
 
 	mutex_lock(&phy_fixup_lock);
 	list_for_each_entry(fixup, &phy_fixup_list, list) {
@@ -138,11 +139,13 @@ int phy_scan_fixups(struct phy_device *phydev)
 				mutex_unlock(&phy_fixup_lock);
 				return err;
 			}
+			if (err == PHY_FIXUP_SKIP_INIT)
+				rc = err;
 		}
 	}
 	mutex_unlock(&phy_fixup_lock);
 
-	return 0;
+	return rc;
 }
 EXPORT_SYMBOL(phy_scan_fixups);
 
@@ -405,6 +408,8 @@ int phy_init_hw(struct phy_device *phydev)
 	ret = phy_scan_fixups(phydev);
 	if (ret < 0)
 		return ret;
+	if (ret == PHY_FIXUP_SKIP_INIT)
+		return 0;
 
 	return phydev->drv->config_init(phydev);
 }
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 14d7fdf..5e2b026 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -413,6 +413,12 @@ struct phy_fixup {
 	int (*run)(struct phy_device *phydev);
 };
 
+/* The fixup can return this to skip the PHY driver init routine
+ * (ie. the fixup effectively replaces the init routine)
+ */
+#define PHY_FIXUP_SKIP_INIT	1
+
+
 /**
  * phy_read - Convenience function for reading a given PHY register
  * @phydev: the phy_device struct

[PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Wolfram Sang]

> The problem is that the fixups are called -after- the config_init()
> callback of the PHY driver. However, the marvell m88e1118 PHY driver
> will unconditionally reset the LEDs setting.

Haven't checked, just an idea: Is it possible to change the init of the driver
to use Read-Modify-Write and thus keep your settings?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20100517/98bf2188/attachment.sig>

Re: [PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Wolfram Sang]

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


> The problem is that the fixups are called -after- the config_init()
> callback of the PHY driver. However, the marvell m88e1118 PHY driver
> will unconditionally reset the LEDs setting.

Haven't checked, just an idea: Is it possible to change the init of the driver
to use Read-Modify-Write and thus keep your settings?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

[PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Benjamin Herrenschmidt]

On Mon, 2010-05-17 at 14:00 +0200, Wolfram Sang wrote:
> > The problem is that the fixups are called -after- the config_init()
> > callback of the PHY driver. However, the marvell m88e1118 PHY driver
> > will unconditionally reset the LEDs setting.
> 
> Haven't checked, just an idea: Is it possible to change the init of
> the driver to use Read-Modify-Write and thus keep your settings?

Well, the driver just unconditionally blasts the whole register and
the value I want to put there is also a "raw" value of the whole
register based on what the vendor code puts in there (in their old
2.6.12 based port).

So with only that at hand and no documentation for the actual PHY chip,
I don't see a simple solution here.

Cheers,
Ben.

Re: [PATCH] phy/marvell: Add special settings for D-Link DNS-323 rev C1

[Benjamin Herrenschmidt]

On Mon, 2010-05-17 at 14:00 +0200, Wolfram Sang wrote:
> > The problem is that the fixups are called -after- the config_init()
> > callback of the PHY driver. However, the marvell m88e1118 PHY driver
> > will unconditionally reset the LEDs setting.
> 
> Haven't checked, just an idea: Is it possible to change the init of
> the driver to use Read-Modify-Write and thus keep your settings?

Well, the driver just unconditionally blasts the whole register and
the value I want to put there is also a "raw" value of the whole
register based on what the vendor code puts in there (in their old
2.6.12 based port).

So with only that at hand and no documentation for the actual PHY chip,
I don't see a simple solution here.

Cheers,
Ben.