[U-Boot] [PATCH v2] arm: socfpga: set skew settings for ethernet phy

[U-Boot] [PATCH v2] arm: socfpga: set skew settings for ethernet phy

[dinguyen at opensource.altera.com]

From: Dinh Nguyen <dinguyen@opensource.altera.com>

Set the PHY skew settings for the ethernet phy on the SOCFPGA Cyclone5
hardware.

Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
Cc: Vince Bridgers <vbridger@opensource.altera.com>
Cc: Pavel Machek <pavel@denx.de>
Cc: Marek Vasut <marex@denx.de>
Cc: Tom Rini <trini@ti.com>
Cc: Albert Aribaud <albert.u.boot@aribaud.net>
Cc: Wolfgang Denk <wd@denx.de>
---
v2: Added comments on why it is necessary for setting the skew values.
---
 board/altera/socfpga/socfpga_cyclone5.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/board/altera/socfpga/socfpga_cyclone5.c b/board/altera/socfpga/socfpga_cyclone5.c
index 0f81d89..543a143 100644
--- a/board/altera/socfpga/socfpga_cyclone5.c
+++ b/board/altera/socfpga/socfpga_cyclone5.c
@@ -8,7 +8,9 @@
 #include <asm/arch/reset_manager.h>
 #include <asm/io.h>
 
+#include <micrel.h>
 #include <netdev.h>
+#include <phy.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -39,3 +41,17 @@ int board_init(void)
 
 	return 0;
 }
+
+int board_phy_config(struct phy_device *phydev)
+{
+	/*
+	 * These skew settings for the KSZ9021 ethernet phy is required for ethernet
+	 * to work reliably on most flavors of cyclone5 boards.
+	 */
+	ksz9021_phy_extended_write(phydev, MII_KSZ9021_EXT_RGMII_RX_DATA_SKEW,
+				   0x0);
+	ksz9021_phy_extended_write(phydev, MII_KSZ9021_EXT_RGMII_TX_DATA_SKEW,
+				   0x0);
+	ksz9021_phy_extended_write(phydev, MII_KSZ9021_EXT_RGMII_CLOCK_SKEW,
+				   0xf0f0);
+}
-- 
2.0.3

[U-Boot] [PATCH v2] arm: socfpga: set skew settings for ethernet phy

[Pavel Machek]

On Thu 2014-11-13 11:23:41, dinguyen at opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Set the PHY skew settings for the ethernet phy on the SOCFPGA Cyclone5
> hardware.

Is it to need similar settings for all ksz9021 phys? Should this go to
drivers/ as I suggested with a patch?

Thanks,
								Pavel

> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Vince Bridgers <vbridger@opensource.altera.com>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Tom Rini <trini@ti.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> Cc: Wolfgang Denk <wd@denx.de>
> ---
> v2: Added comments on why it is necessary for setting the skew values.
> ---
>  board/altera/socfpga/socfpga_cyclone5.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/board/altera/socfpga/socfpga_cyclone5.c b/board/altera/socfpga/socfpga_cyclone5.c
> index 0f81d89..543a143 100644
> --- a/board/altera/socfpga/socfpga_cyclone5.c
> +++ b/board/altera/socfpga/socfpga_cyclone5.c
> @@ -8,7 +8,9 @@
>  #include <asm/arch/reset_manager.h>
>  #include <asm/io.h>
>  
> +#include <micrel.h>
>  #include <netdev.h>
> +#include <phy.h>
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -39,3 +41,17 @@ int board_init(void)
>  
>  	return 0;
>  }
> +
> +int board_phy_config(struct phy_device *phydev)
> +{
> +	/*
> +	 * These skew settings for the KSZ9021 ethernet phy is required for ethernet
> +	 * to work reliably on most flavors of cyclone5 boards.
> +	 */
> +	ksz9021_phy_extended_write(phydev, MII_KSZ9021_EXT_RGMII_RX_DATA_SKEW,
> +				   0x0);
> +	ksz9021_phy_extended_write(phydev, MII_KSZ9021_EXT_RGMII_TX_DATA_SKEW,
> +				   0x0);
> +	ksz9021_phy_extended_write(phydev, MII_KSZ9021_EXT_RGMII_CLOCK_SKEW,
> +				   0xf0f0);
> +}

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[U-Boot] [PATCH v2] arm: socfpga: set skew settings for ethernet phy

[Marek Vasut]

On Friday, November 14, 2014 at 11:23:27 AM, Pavel Machek wrote:
> On Thu 2014-11-13 11:23:41, dinguyen at opensource.altera.com wrote:
> > From: Dinh Nguyen <dinguyen@opensource.altera.com>
> > 
> > Set the PHY skew settings for the ethernet phy on the SOCFPGA Cyclone5
> > hardware.
> 
> Is it to need similar settings for all ksz9021 phys? Should this go to
> drivers/ as I suggested with a patch?
> 

This is board specific, but with the driver model, this should be moved into the 
PHY driver and the values should be passed via platform data or DT.

I will apply this now, since it fixes real issue.

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] arm: socfpga: set skew settings for ethernet phy

[Pavel Machek]

On Fri 2014-11-14 21:02:48, Marek Vasut wrote:
> On Friday, November 14, 2014 at 11:23:27 AM, Pavel Machek wrote:
> > On Thu 2014-11-13 11:23:41, dinguyen at opensource.altera.com wrote:
> > > From: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > 
> > > Set the PHY skew settings for the ethernet phy on the SOCFPGA Cyclone5
> > > hardware.
> > 
> > Is it to need similar settings for all ksz9021 phys? Should this go to
> > drivers/ as I suggested with a patch?
> > 
> 
> This is board specific, but with the driver model, this should be moved into the 
> PHY driver and the values should be passed via platform data or DT.
> 
> I will apply this now, since it fixes real issue.

Hmm, could you apply the patch I posted, instead? It has configuration
in socfpga_config.h -- "platform data".

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[U-Boot] [PATCH v2] arm: socfpga: set skew settings for ethernet phy

[Marek Vasut]

On Saturday, November 15, 2014 at 08:54:06 PM, Pavel Machek wrote:
> On Fri 2014-11-14 21:02:48, Marek Vasut wrote:
> > On Friday, November 14, 2014 at 11:23:27 AM, Pavel Machek wrote:
> > > On Thu 2014-11-13 11:23:41, dinguyen at opensource.altera.com wrote:
> > > > From: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > > 
> > > > Set the PHY skew settings for the ethernet phy on the SOCFPGA
> > > > Cyclone5 hardware.
> > > 
> > > Is it to need similar settings for all ksz9021 phys? Should this go to
> > > drivers/ as I suggested with a patch?
> > 
> > This is board specific, but with the driver model, this should be moved
> > into the PHY driver and the values should be passed via platform data or
> > DT.
> > 
> > I will apply this now, since it fixes real issue.
> 
> Hmm, could you apply the patch I posted, instead? It has configuration
> in socfpga_config.h -- "platform data".

Hi!

The patch you provided (if we're referring to the same patch, Message-ID: 
<20141113120956.GB30779@amd>) breaks every board which uses the Micrel PHY, 
doesn't it ? The problem for all other boards would be the new CONFIG_* options
which are added into the Micrel driver by that patch. Furthermore, the patch 
doesn't even apply, it was decided that the patch was an RFC, wasn't it ?
Please correct me if I'm wrong.

The way forward here would be for you to prepare a patch for the Micrel PHY
driver, submit that one (make sure to CC Joe, the network custodian) and
in the next step clean up the writes into PHY configuration from both the
SoCFPGA CV and from Nitrogen 6x support (I recall I saw the same thing done
for that board).

Oh, and btw., the new CONFIG_* options should be documented somewhere, so
please keep that in mind. Otherwise, the amount of various CONFIG_* options
become an unmanagable mess.

Thanks!

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] arm: socfpga: set skew settings for ethernet phy

[Pavel Machek]

On Sat 2014-11-15 22:29:00, Marek Vasut wrote:
> On Saturday, November 15, 2014 at 08:54:06 PM, Pavel Machek wrote:
> > On Fri 2014-11-14 21:02:48, Marek Vasut wrote:
> > > On Friday, November 14, 2014 at 11:23:27 AM, Pavel Machek wrote:
> > > > On Thu 2014-11-13 11:23:41, dinguyen at opensource.altera.com wrote:
> > > > > From: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > > > 
> > > > > Set the PHY skew settings for the ethernet phy on the SOCFPGA
> > > > > Cyclone5 hardware.
> > > > 
> > > > Is it to need similar settings for all ksz9021 phys? Should this go to
> > > > drivers/ as I suggested with a patch?
> > > 
> > > This is board specific, but with the driver model, this should be moved
> > > into the PHY driver and the values should be passed via platform data or
> > > DT.
> > > 
> > > I will apply this now, since it fixes real issue.
> > 
> > Hmm, could you apply the patch I posted, instead? It has configuration
> > in socfpga_config.h -- "platform data".
> 
> Hi!
> 
> The patch you provided (if we're referring to the same patch, Message-ID: 
> <20141113120956.GB30779@amd>) breaks every board which uses the Micrel PHY, 
> doesn't it ? The problem for all other boards would be the new CONFIG_* options
> which are added into the Micrel driver by that patch. Furthermore, the patch 
> doesn't even apply, it was decided that the patch was an RFC, wasn't it ?
> Please correct me if I'm wrong.

The purpose of that mail was to show that there's better way, so that
someone (you) would not apply bandaid when something close to real fix
is available.

I overlooked the "breaks compilation" part, but I'm sure it could have
been solved with strategicaly placed #ifdef. Notice original patch
submission in

[U-Boot] [PATCH v2] arm: socfpga: set skew settings for ethernet phy

[Marek Vasut]

On Saturday, November 15, 2014 at 11:20:54 PM, Pavel Machek wrote:
> On Sat 2014-11-15 22:29:00, Marek Vasut wrote:
> > On Saturday, November 15, 2014 at 08:54:06 PM, Pavel Machek wrote:
> > > On Fri 2014-11-14 21:02:48, Marek Vasut wrote:
> > > > On Friday, November 14, 2014 at 11:23:27 AM, Pavel Machek wrote:
> > > > > On Thu 2014-11-13 11:23:41, dinguyen at opensource.altera.com wrote:
> > > > > > From: Dinh Nguyen <dinguyen@opensource.altera.com>
> > > > > > 
> > > > > > Set the PHY skew settings for the ethernet phy on the SOCFPGA
> > > > > > Cyclone5 hardware.
> > > > > 
> > > > > Is it to need similar settings for all ksz9021 phys? Should this go
> > > > > to drivers/ as I suggested with a patch?
> > > > 
> > > > This is board specific, but with the driver model, this should be
> > > > moved into the PHY driver and the values should be passed via
> > > > platform data or DT.
> > > > 
> > > > I will apply this now, since it fixes real issue.
> > > 
> > > Hmm, could you apply the patch I posted, instead? It has configuration
> > > in socfpga_config.h -- "platform data".
> > 
> > Hi!
> > 
> > The patch you provided (if we're referring to the same patch, Message-ID:
> > <20141113120956.GB30779@amd>) breaks every board which uses the Micrel
> > PHY, doesn't it ? The problem for all other boards would be the new
> > CONFIG_* options which are added into the Micrel driver by that patch.
> > Furthermore, the patch doesn't even apply, it was decided that the patch
> > was an RFC, wasn't it ? Please correct me if I'm wrong.
> 
> The purpose of that mail was to show that there's better way, so that
> someone (you) would not apply bandaid when something close to real fix
> is available.

I had to pick from two options -- apply a patch which solves a real problem now 
and which follows approach already implemented for a different board in mainline 
-- or wait until an RFC patch is done discussing and makes it into mainline.

I decided to solve a real problem now and in case a proper fix/cleanup arrives
afterward, I will only be happy about that.

> I overlooked the "breaks compilation" part, but I'm sure it could have
> been solved with strategicaly placed #ifdef. Notice original patch
> submission in
> 
> From pavel at denx.de Sat Oct 25 13:27:52 2014
> Date: Sat, 25 Oct 2014 13:27:52 +0200
> From: Pavel Machek <pavel@denx.de>
> Subject: [patch] net: micrel: configure skew values for
> Message-ID: <20141025112752.GA19599@amd>
> 
> it is cleaner then <20141113120956.GB30779@amd> version. I don't think
> I received any reply there.

Sorry, I missed the new version. I'll comment on it separatelly.

> But the patch you have just merged would interfere with my approach,
> and makes it unneccessary, so I'll let you solve it.

In fact, the Micrel PHY patch changes only the Micrel driver. This patch
changes only the SoCFPGA CV board. They are two completely orthogonal
patches.

The Micrel PHY patch can just be followed by a cleanup patch which removes
the explicitly PHY setup function from both the MX6 Nitrogen 6x and SoCFPGA
boards. I don't see any problem with such cleanup, do you ?

Best regards,
Marek Vasut

[U-Boot] [PATCH v2] arm: socfpga: set skew settings for ethernet phy

[Marek Vasut]

On Thursday, November 13, 2014 at 06:23:41 PM, dinguyen at opensource.altera.com 
wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Set the PHY skew settings for the ethernet phy on the SOCFPGA Cyclone5
> hardware.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> Cc: Vince Bridgers <vbridger@opensource.altera.com>
> Cc: Pavel Machek <pavel@denx.de>
> Cc: Marek Vasut <marex@denx.de>
> Cc: Tom Rini <trini@ti.com>
> Cc: Albert Aribaud <albert.u.boot@aribaud.net>
> Cc: Wolfgang Denk <wd@denx.de>
> ---
> v2: Added comments on why it is necessary for setting the skew values.

Applied, thanks!

Best regards,
Marek Vasut