[U-Boot] [PATCH 0/2] Add getenv_long and Micrel ksz9031 skew settings

[U-Boot] [PATCH 0/2] Add getenv_long and Micrel ksz9031 skew settings

[Vince Bridgers]

This patch series first adds the ability to read a signed integer from the
UBoot environment, then adds a patch to the Micrel ksz9031 phy driver to 
program the skew values per settings in the UBoot environment. The strings
chosen to use in the UBoot environment are equivalent to those used by the
Micrel devicetree settings when that is eventually supported.


Vince Bridgers (2):
  cmd: Add getenv_long to support reading signed integers from the uboot
    env
  net: phy: Add ability to program the ksz9031 skew values from the
    uboot env

 common/cmd_nvedit.c      |  22 +++-
 drivers/net/phy/micrel.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++-
 include/common.h         |   1 +
 3 files changed, 320 insertions(+), 2 deletions(-)

-- 
1.9.1

[U-Boot] [PATCH 1/2] cmd: Add getenv_long to support reading signed integers from the uboot env

[Vince Bridgers]

This patch adds a function to read a signed integer from the uboot environment
for the Micrel ksz9031 Phy driver to read postive and negatice skew values
from the uboot environment.

Signed-off-by: Vince Bridgers <vbridger@opensource.altera.com>
---
 common/cmd_nvedit.c | 22 +++++++++++++++++++++-
 include/common.h    |  1 +
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c
index 855808c..ec23696 100644
--- a/common/cmd_nvedit.c
+++ b/common/cmd_nvedit.c
@@ -665,7 +665,27 @@ int getenv_f(const char *name, char *buf, unsigned len)
 }
 
 /**
- * Decode the integer value of an environment variable and return it.
+ * Decode the signed integer value of an environment variable and return it.
+ *
+ * @param name		Name of environemnt variable
+ * @param base		Number base to use (normally 10, or 16 for hex)
+ * @param default_val	Default value to return if the variable is not
+ *			found
+ * @return the decoded value, or default_val if not found
+ */
+long getenv_long(const char *name, int base, long default_val)
+{
+	/*
+	 * We can use getenv() here, even before relocation, since the
+	 * environment variable value is an integer and thus short.
+	 */
+	const char *str = getenv(name);
+
+	return str ? simple_strtol(str, NULL, base) : default_val;
+}
+
+/**
+ * Decode the unsigned integer value of an environment variable and return it.
  *
  * @param name		Name of environemnt variable
  * @param base		Number base to use (normally 10, or 16 for hex)
diff --git a/include/common.h b/include/common.h
index 97c8f79..55a8cb3 100644
--- a/include/common.h
+++ b/include/common.h
@@ -304,6 +304,7 @@ int	envmatch     (uchar *, int);
 char	*getenv	     (const char *);
 int	getenv_f     (const char *name, char *buf, unsigned len);
 ulong getenv_ulong(const char *name, int base, ulong default_val);
+long getenv_long(const char *name, int base, long default_val);
 
 /**
  * getenv_hex() - Return an environment variable as a hex value
-- 
1.9.1

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Vince Bridgers]

This patch adds the ability for the Micrel ksz9031 PHY driver to detect
skew settings from the UBoot environment and program those values at PHY
initialization time. This is a first step towards setting these values from
the UBoot environment or the Micrel PHY devicetree once supported. The skew
values may be negative or positive, therefore depends on UBoot environment
support to read positive or negative values in units of picoseconds.

Signed-off-by: Vince Bridgers <vbridger@opensource.altera.com>
---
 drivers/net/phy/micrel.c | 299 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 298 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 1815b29..d8819a8 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -211,6 +211,13 @@ static struct phy_driver ksz9021_driver = {
 /* PHY Registers */
 #define MII_KSZ9031_MMD_ACCES_CTRL	0x0d
 #define MII_KSZ9031_MMD_REG_DATA	0x0e
+#define KSZ9031_PS_TO_REG		60
+
+/* Extended registers */
+#define MII_KSZ9031RN_CONTROL_PAD_SKEW	4
+#define MII_KSZ9031RN_RX_DATA_PAD_SKEW	5
+#define MII_KSZ9031RN_TX_DATA_PAD_SKEW	6
+#define MII_KSZ9031RN_CLK_PAD_SKEW	8
 
 /* Accessors to extended registers*/
 int ksz9031_phy_extended_write(struct phy_device *phydev,
@@ -256,13 +263,303 @@ static int ksz9031_phy_extwrite(struct phy_device *phydev, int addr,
 					 MII_KSZ9031_MOD_DATA_POST_INC_RW, val);
 };
 
+u8
+ksz9031_get_rxc_skew(struct phy_device *phydev)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 8,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+	reg &= 0x1f;
+	return (u8)reg;
+}
+
+u8
+ksz9031_get_txc_skew(struct phy_device *phydev)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 8,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+	reg = reg >> 5;
+	reg &= 0x1f;
+	return (u8)reg;
+}
+
+u8
+ksz9031_get_txen_skew(struct phy_device *phydev)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 4,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+	reg &= 0xf;
+	return (u8)reg;
+}
+
+u8
+ksz9031_get_rxdv_skew(struct phy_device *phydev)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 4,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+	reg = reg >> 4;
+	reg &= 0xf;
+	return (u8)reg;
+}
+
+u8
+ksz9031_get_rxdx_skew(struct phy_device *phydev, int index)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 5,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+	reg = reg >> (index*4);
+	reg &= 0xf;
+	return (u8)reg;
+}
+
+u8
+ksz9031_get_txdx_skew(struct phy_device *phydev, int index)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 6,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+	reg = reg >> (index*4);
+	reg &= 0xf;
+	return (u8)reg;
+}
+
+void
+ksz9031_set_rxc_skew(struct phy_device *phydev, u8 val)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 8,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+
+	reg = reg & 0xffe0;
+	reg |= (u16)val;
+
+	ksz9031_phy_extended_write(phydev, 2, 8,
+				   MII_KSZ9031_MOD_DATA_NO_POST_INC, reg);
+}
+
+void
+ksz9031_set_txc_skew(struct phy_device *phydev, u8 val)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 8,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+	reg = reg & 0xfc1f;
+	reg |= ((u16)val) << 5;
+
+	ksz9031_phy_extended_write(phydev, 2, 8,
+				   MII_KSZ9031_MOD_DATA_NO_POST_INC, reg);
+}
+
+void
+ksz9031_set_txen_skew(struct phy_device *phydev, u8 val)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 4,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+
+	reg = reg & 0xfff0;
+	reg |= ((u16)val);
+	ksz9031_phy_extended_write(phydev, 2, 4,
+				   MII_KSZ9031_MOD_DATA_NO_POST_INC, reg);
+}
+
+void
+ksz9031_set_rxdv_skew(struct phy_device *phydev, u8 val)
+{
+	u16 reg;
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 4,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+
+	reg = reg & 0xff0f;
+	reg |= ((u16)val) << 4;
+	ksz9031_phy_extended_write(phydev, 2, 4,
+				   MII_KSZ9031_MOD_DATA_NO_POST_INC, reg);
+}
+
+void
+ksz9031_set_rxdx_skew(struct phy_device *phydev, int index, u8 val)
+{
+	u16 reg;
+	u16 mask;
+
+	mask = ~(0xf << (index * 4));
+
+	reg = ksz9031_phy_extended_read(phydev, 2, 5,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+	reg = reg & mask;
+	reg |= ((u16)val) << (4 * index);
+
+	ksz9031_phy_extended_write(phydev, 2, 5,
+				   MII_KSZ9031_MOD_DATA_NO_POST_INC, reg);
+}
+
+void
+ksz9031_set_txdx_skew(struct phy_device *phydev, int index, u8 val)
+{
+	u16 reg;
+	u16 mask;
+
+	mask = ~(0xf << (index * 4));
+	reg = ksz9031_phy_extended_read(phydev, 2, 6,
+					MII_KSZ9031_MOD_DATA_NO_POST_INC);
+
+	reg = reg & mask;
+	reg |= ((u16)val) << (4 * index);
+
+	ksz9031_phy_extended_write(phydev, 2, 6,
+				   MII_KSZ9031_MOD_DATA_NO_POST_INC, reg);
+}
+
+long ksz9031_range_check(const char *name, long val, long lower, long upper)
+{
+	long returnval = val;
+
+	if (val < lower) {
+		printf("Warning: %s value %ld lt %ld, clipping to %ld\n",
+			name, val, lower, lower);
+		returnval = lower;
+	}
+	if (val > upper) {
+		printf("Warning: %s value %ld gt %ld, clipping to %ld\n",
+			name, val, upper, upper);
+		returnval = upper;
+	}
+	return returnval;
+}
+
+#define KSZ9031_DEFAULT_RXC_SKEW_PS	0
+#define KSZ9031_DEFAULT_TXC_SKEW_PS	0
+#define KSZ9031_DEFAULT_TXEN_SKEW_PS	0
+#define KSZ9031_DEFAULT_RXDV_SKEW_PS	0
+
+#define KSZ9031_DEFAULT_RXD_SKEW_PS	0
+#define KSZ9031_DEFAULT_TXD_SKEW_PS	0
+int ksz9031_config_init(struct phy_device *phydev)
+{
+	long rxc_skew_ps;       /* -900ps through 960ps */
+	long txc_skew_ps;       /* -900ps through 960ps */
+	long txen_skew_ps;      /* -420ps through 480ps */
+	long rxdv_skew_ps;      /* -420ps through 480ps */
+	long rxdx_skew_ps[4];   /* -420ps through 480ps */
+	long txdx_skew_ps[4];   /* -420ps through 480ps */
+
+	u8 rxc_skew_regval;     /* 5 bit range */
+	u8 txc_skew_regval;     /* 5 bit range */
+	u8 txen_skew_regval;    /* 4 bit range */
+	u8 rxdv_skew_regval;    /* 4 bit range */
+	u8 rxdx_skew_regval[4]; /* 4 bit range */
+	u8 txdx_skew_regval[4]; /* 4 bit range */
+
+	long upper, lower;
+
+	int i;
+
+	char *rx_data_skews[4] = {
+		"rxd0-skew-ps", "rxd1-skew-ps",
+		"rxd2-skew-ps", "rxd3-skew-ps"
+	};
+	char *tx_data_skews[4] = {
+		"txd0-skew-ps", "txd1-skew-ps",
+		"txd2-skew-ps", "txd3-skew-ps"
+	};
+
+	/* Get values from the environment */
+
+	rxc_skew_ps = getenv_long("rxc-skew-ps", 10,
+				  KSZ9031_DEFAULT_RXC_SKEW_PS);
+	txc_skew_ps = getenv_long("txc-skew-ps", 10,
+				  KSZ9031_DEFAULT_TXC_SKEW_PS);
+	txen_skew_ps = getenv_long("txen-skew-ps", 10,
+				   KSZ9031_DEFAULT_TXEN_SKEW_PS);
+	rxdv_skew_ps = getenv_long("rxdv-skew-ps", 10,
+				   KSZ9031_DEFAULT_RXDV_SKEW_PS);
+
+	for (i = 0; i < 4; i++) {
+		rxdx_skew_ps[i] = getenv_long(rx_data_skews[i], 10,
+					      KSZ9031_DEFAULT_RXD_SKEW_PS);
+		txdx_skew_ps[i] = getenv_long(tx_data_skews[i], 10,
+					      KSZ9031_DEFAULT_TXD_SKEW_PS);
+	}
+
+	/* Range check, normalize values */
+	lower = -900;
+	upper = 960;
+	rxc_skew_ps = ksz9031_range_check("rxc-skew-ps", rxc_skew_ps,
+					  lower, upper);
+	txc_skew_ps = ksz9031_range_check("txc-skew-ps", txc_skew_ps,
+					  lower, upper);
+
+	lower = -420;
+	upper = 480;
+	txen_skew_ps = ksz9031_range_check("txen-skew-ps", txen_skew_ps,
+					   lower, upper);
+	rxdv_skew_ps = ksz9031_range_check("rxdv-skew-ps", rxdv_skew_ps,
+					   lower, upper);
+
+	for (i = 0; i < 4; i++) {
+		rxdx_skew_ps[i] = ksz9031_range_check("rxd[x]-skew-ps",
+						      rxdx_skew_ps[i],
+						      lower, upper);
+		txdx_skew_ps[i] = ksz9031_range_check("txd[x]-skew-ps",
+						      txdx_skew_ps[i],
+						      lower, upper);
+	}
+
+	/* Assign values to register variables */
+
+	rxc_skew_regval = (rxc_skew_ps + 900)/60;
+	txc_skew_regval = (txc_skew_ps + 900)/60;
+
+	txen_skew_regval = (txen_skew_ps + 420)/60;
+	rxdv_skew_regval = (rxdv_skew_ps + 420)/60;
+
+	for (i = 0; i < 4; i++) {
+		rxdx_skew_regval[i] = (rxdx_skew_ps[i] + 420)/60;
+		txdx_skew_regval[i] = (txdx_skew_ps[i] + 420)/60;
+	}
+
+	/* Write the values to the registers */
+	ksz9031_set_rxc_skew(phydev, rxc_skew_regval);
+	ksz9031_set_txc_skew(phydev, txc_skew_regval);
+	ksz9031_set_txen_skew(phydev, txen_skew_regval);
+	ksz9031_set_rxdv_skew(phydev, rxdv_skew_regval);
+
+	for (i = 0; i < 4; i++) {
+		ksz9031_set_rxdx_skew(phydev, i, rxdx_skew_regval[i]);
+		ksz9031_set_txdx_skew(phydev, i, txdx_skew_regval[i]);
+	}
+
+	/* Be sure to disable asym pause since if enabled, the ksz9031 is not
+	 * able to establish a link in some cases. There is an errata for
+	 * asym pause on the ksz9031.
+	 */
+
+	genphy_config_aneg(phydev);
+	genphy_restart_aneg(phydev);
+
+	return 0;
+}
 
 static struct phy_driver ksz9031_driver = {
 	.name = "Micrel ksz9031",
 	.uid  = 0x221620,
 	.mask = 0xfffff0,
 	.features = PHY_GBIT_FEATURES,
-	.config   = &genphy_config,
+	.config   = &ksz9031_config_init,
 	.startup  = &ksz90xx_startup,
 	.shutdown = &genphy_shutdown,
 	.writeext = &ksz9031_phy_extwrite,
-- 
1.9.1

[U-Boot] [PATCH 1/2] cmd: Add getenv_long to support reading signed integers from the uboot env

[Marek Vasut]

On Monday, February 09, 2015 at 03:44:32 PM, Vince Bridgers wrote:
> This patch adds a function to read a signed integer from the uboot
> environment for the Micrel ksz9031 Phy driver to read postive and negatice
> skew values from the uboot environment.
> 
> Signed-off-by: Vince Bridgers <vbridger@opensource.altera.com>

Reviewed-by: Marek Vasut <marex@denx.de>

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Marek Vasut]

On Monday, February 09, 2015 at 03:44:33 PM, Vince Bridgers wrote:
> This patch adds the ability for the Micrel ksz9031 PHY driver to detect
> skew settings from the UBoot environment and program those values at PHY
> initialization time. This is a first step towards setting these values from
> the UBoot environment or the Micrel PHY devicetree once supported. The skew
> values may be negative or positive, therefore depends on UBoot environment
> support to read positive or negative values in units of picoseconds.
> 
> Signed-off-by: Vince Bridgers <vbridger@opensource.altera.com>

Hi!

We already do this kind of a programming in board/altera/socfpga/socfpga.c
in board_phy_config(), don't we ?

Also, see [1], once I apply this, the DT support (not DM) for SoCFPGA will
become mandatory. Won't it make more sense to pull these values from the
DT instead of poluting the board environment with those please ?

Thanks!

[1] http://git.denx.de/?p=u-boot/u-boot-
socfpga.git;a=shortlog;h=refs/heads/topic/vip-arriav

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Vince Bridgers]

Hi Marek!

> We already do this kind of a programming in board/altera/socfpga/socfpga.c
> in board_phy_config(), don't we ?

Yes, good point. This patch series is a first in some upcoming patches to make this better. The Linux implementation uses devicetree settings to set the skews, so if we were to follow that same model the code in socfpga.c would become deprecated in favor of setting the skews through the phy driver and subsequently removed. That way other users could take advantage of this through devicetree. The other problem with the current implementation is the skew values are part specific - we set the actual register values in the environment when it would be better to use a skewed time value (in +/- picoseconds). 

> Also, see [1], once I apply this, the DT support (not DM) for SoCFPGA will
> become mandatory. Won't it make more sense to pull these values from the
> DT instead of poluting the board environment with those please ?

I agree it would make more sense to pull these from devicetree - I'm planning on adding that in a future patch. I thought it would be a good idea to pull these values from the environment first, overriding the devicetree (if present in the environment). This approach is helpful during bringup & debug since it doesn't require one to change the devicetree to try something quickly. I'm ok with any approach you think would work for the community. 

I'm ok with whatever the community decides considering my rationale above. 

All the best!

Vince
vbridger at opensource.altera.com

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Marek Vasut]

On Monday, February 09, 2015 at 10:31:42 PM, Vince Bridgers wrote:
> Hi Marek!

Hi Vince!

> > We already do this kind of a programming in
> > board/altera/socfpga/socfpga.c in board_phy_config(), don't we ?
> 
> Yes, good point. This patch series is a first in some upcoming patches to
> make this better. The Linux implementation uses devicetree settings to set
> the skews, so if we were to follow that same model the code in socfpga.c
> would become deprecated in favor of setting the skews through the phy
> driver and subsequently removed. That way other users could take advantage
> of this through devicetree.

Setting the skews in DT would indeed be preferable in my opinion.

> The other problem with the current
> implementation is the skew values are part specific - we set the actual
> register values in the environment when it would be better to use a skewed
> time value (in +/- picoseconds).

You mean they are specific for particular PHY model ? Or board model ? Or
even particular board itself ?

> > Also, see [1], once I apply this, the DT support (not DM) for SoCFPGA
> > will become mandatory. Won't it make more sense to pull these values
> > from the DT instead of poluting the board environment with those please
> > ?
> 
> I agree it would make more sense to pull these from devicetree - I'm
> planning on adding that in a future patch. I thought it would be a good
> idea to pull these values from the environment first, overriding the
> devicetree (if present in the environment). This approach is helpful
> during bringup & debug since it doesn't require one to change the
> devicetree to try something quickly. I'm ok with any approach you think
> would work for the community.

You can do that with the 'mii' command as well I think, but I might be wrong.

> I'm ok with whatever the community decides considering my rationale above.

+CC Stefan and Pavel, please comment.

Best regards,
Marek Vasut

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Stefan Roese]

(Added Joe Hershberger to Cc, because this discussion is "network" 
related and not really SoCFPGA related)

On 10.02.2015 19:51, Marek Vasut wrote:
>>> We already do this kind of a programming in
>>> board/altera/socfpga/socfpga.c in board_phy_config(), don't we ?
>>
>> Yes, good point. This patch series is a first in some upcoming patches to
>> make this better. The Linux implementation uses devicetree settings to set
>> the skews, so if we were to follow that same model the code in socfpga.c
>> would become deprecated in favor of setting the skews through the phy
>> driver and subsequently removed. That way other users could take advantage
>> of this through devicetree.
>
> Setting the skews in DT would indeed be preferable in my opinion.

+1 from me.

>> The other problem with the current
>> implementation is the skew values are part specific - we set the actual
>> register values in the environment when it would be better to use a skewed
>> time value (in +/- picoseconds).
>
> You mean they are specific for particular PHY model ? Or board model ? Or
> even particular board itself ?
>
>>> Also, see [1], once I apply this, the DT support (not DM) for SoCFPGA
>>> will become mandatory. Won't it make more sense to pull these values
>>> from the DT instead of poluting the board environment with those please
>>> ?
>>
>> I agree it would make more sense to pull these from devicetree - I'm
>> planning on adding that in a future patch. I thought it would be a good
>> idea to pull these values from the environment first, overriding the
>> devicetree (if present in the environment). This approach is helpful
>> during bringup & debug since it doesn't require one to change the
>> devicetree to try something quickly. I'm ok with any approach you think
>> would work for the community.
>
> You can do that with the 'mii' command as well I think, but I might be wrong.

Yes. For testing or board bringup this might really serve. Even though 
this setting via environment as proposed from Vince is more elegant and 
less hackish. And easier to adjust/tune for "normal users".

>> I'm ok with whatever the community decides considering my rationale above.
>
> +CC Stefan and Pavel, please comment.

The default values should come from the DT, once this is all in place. 
But I think that for initial board bringup / testing such a method, to 
override those values via environment variables can be quite helpful.

Joe, whats your opinion on this?

Thanks,
Stefan

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Joe Hershberger]

On Wed, Feb 11, 2015 at 1:08 AM, Stefan Roese <sr@denx.de> wrote:
>
> (Added Joe Hershberger to Cc, because this discussion is "network"
related and not really SoCFPGA related)
>
> On 10.02.2015 19:51, Marek Vasut wrote:
>>>>
>>>> We already do this kind of a programming in
>>>> board/altera/socfpga/socfpga.c in board_phy_config(), don't we ?
>>>
>>>
>>> Yes, good point. This patch series is a first in some upcoming patches
to
>>> make this better. The Linux implementation uses devicetree settings to
set
>>> the skews, so if we were to follow that same model the code in socfpga.c
>>> would become deprecated in favor of setting the skews through the phy
>>> driver and subsequently removed. That way other users could take
advantage
>>> of this through devicetree.
>>
>>
>> Setting the skews in DT would indeed be preferable in my opinion.
>
>
> +1 from me.

+1 from me as well.

>>> The other problem with the current
>>> implementation is the skew values are part specific - we set the actual
>>> register values in the environment when it would be better to use a
skewed
>>> time value (in +/- picoseconds).
>>
>>
>> You mean they are specific for particular PHY model ? Or board model ? Or
>> even particular board itself ?
>>
>>>> Also, see [1], once I apply this, the DT support (not DM) for SoCFPGA
>>>> will become mandatory. Won't it make more sense to pull these values
>>>> from the DT instead of poluting the board environment with those please
>>>> ?
>>>
>>>
>>> I agree it would make more sense to pull these from devicetree - I'm
>>> planning on adding that in a future patch. I thought it would be a good
>>> idea to pull these values from the environment first, overriding the
>>> devicetree (if present in the environment). This approach is helpful
>>> during bringup & debug since it doesn't require one to change the
>>> devicetree to try something quickly. I'm ok with any approach you think
>>> would work for the community.
>>
>>
>> You can do that with the 'mii' command as well I think, but I might be
wrong.
>
>
> Yes. For testing or board bringup this might really serve. Even though
this setting via environment as proposed from Vince is more elegant and
less hackish. And easier to adjust/tune for "normal users".
>
> The default values should come from the DT, once this is all in place.
But I think that for initial board bringup / testing such a method, to
override those values via environment variables can be quite helpful.
>
> Joe, whats your opinion on this?

Do we really think this is a strong use-case?  This seem like the type of
thing I would expect to tweak for testing through mii / mdio commands and
then configure the device tree based on that.  This is pretty much a
one-time thing for a given board it seems to me.

If we really want a polished interface to it, we should define a
sub-command / new command that phy drivers can implement.  I'm not sure an
undiscoverable, un-"help"-able list of env vars will be apparent to users.
Do you have a feeling for how close to universal any of these parameters
are across phys?

-Joe

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Stefan Roese]

On 11.02.2015 09:07, Joe Hershberger wrote:
>>>> I agree it would make more sense to pull these from devicetree - I'm
>>>> planning on adding that in a future patch. I thought it would be a good
>>>> idea to pull these values from the environment first, overriding the
>>>> devicetree (if present in the environment). This approach is helpful
>>>> during bringup & debug since it doesn't require one to change the
>>>> devicetree to try something quickly. I'm ok with any approach you think
>>>> would work for the community.
>>>
>>>
>>> You can do that with the 'mii' command as well I think, but I might be
> wrong.
>>
>>
>> Yes. For testing or board bringup this might really serve. Even though
> this setting via environment as proposed from Vince is more elegant and
> less hackish. And easier to adjust/tune for "normal users".
>>
>> The default values should come from the DT, once this is all in place.
> But I think that for initial board bringup / testing such a method, to
> override those values via environment variables can be quite helpful.
>>
>> Joe, whats your opinion on this?
>
> Do we really think this is a strong use-case?  This seem like the type of
> thing I would expect to tweak for testing through mii / mdio commands and
> then configure the device tree based on that.  This is pretty much a
> one-time thing for a given board it seems to me.
>
> If we really want a polished interface to it, we should define a
> sub-command / new command that phy drivers can implement.  I'm not sure an
> undiscoverable, un-"help"-able list of env vars will be apparent to users.
> Do you have a feeling for how close to universal any of these parameters
> are across phys?

No. But with your comments above (which make total sense), I tend to NAK 
this ability to configure the PHY skew timings via environment variables.

Thanks,
Stefan

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Vince Bridgers]

Hi Marek! Comments and answers follow ....

> Hi Vince!
> > > We already do this kind of a programming in
> > > board/altera/socfpga/socfpga.c in board_phy_config(), don't we ?
> >
> > Yes, good point. This patch series is a first in some upcoming patches to
> > make this better. The Linux implementation uses devicetree settings to set
> > the skews, so if we were to follow that same model the code in socfpga.c
> > would become deprecated in favor of setting the skews through the phy
> > driver and subsequently removed. That way other users could take advantage
> > of this through devicetree.

> Setting the skews in DT would indeed be preferable in my opinion.

Understood. The ability to set skew values through environment variables only helps the debug/bringup phase, so will focus on devicetree settings compatible with the current methods defined in Linux for the Micrel ksz9021 and ksz9031 phy. Per your comment below, we can always edit phy registers from the uboot environment through the mii command. 

> > The other problem with the current
> > implementation is the skew values are part specific - we set the actual
> > register values in the environment when it would be better to use a skewed
> > time value (in +/- picoseconds).

> You mean they are specific for particular PHY model ? Or board model ? Or
> even particular board itself ?

The current settings are a "value" where each count of the value represents a particular number of picoseconds. The ksz9021 and ksz9031 use different picoseconds per count, so the values currently set are specific to each part. This came out when we brought up the ksz9031 on a new board compared to the ksz9021 on the Cyclone 5 board. A better way to implement this would be to follow this Linux model where skew settings are specified in picoseconds and the phy driver converts picoseconds to a value to be written to a register - and comprehends the specific part's capabilities. 

> > > Also, see [1], once I apply this, the DT support (not DM) for SoCFPGA
> > > will become mandatory. Won't it make more sense to pull these values
> > > from the DT instead of poluting the board environment with those please
> > > ?
> >
> > I agree it would make more sense to pull these from devicetree - I'm
> > planning on adding that in a future patch. I thought it would be a good
> > idea to pull these values from the environment first, overriding the
> > devicetree (if present in the environment). This approach is helpful
> > during bringup & debug since it doesn't require one to change the
> > devicetree to try something quickly. I'm ok with any approach you think
> > would work for the community.

> You can do that with the 'mii' command as well I think, but I might be wrong.

Yes, this is true. I'm ok with dropping the idea of environment variable overrides and focusing on a devicetree based implementation for setting the PHY skew values as needed for particular components or boards. 

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Vince Bridgers]

Hi Stefan

> >
> > Setting the skews in DT would indeed be preferable in my opinion.

> +1 from me.

Agreed. I'll focus on a devicetree based implementation, and editing phy values during board debug and bringup can be addressed with debug/development notes. I'd be willing to publish these somewhere with some suggestions about the best place for something like this to reside based on our experience. 

> >
> > You can do that with the 'mii' command as well I think, but I might be wrong.

> Yes. For testing or board bringup this might really serve. Even though
> this setting via environment as proposed from Vince is more elegant and
> less hackish. And easier to adjust/tune for "normal users".

This was the intent of having environment variables - to make trying different skew values easy for our customers. Not all developers/users are Uboot experts and need help from time to time. I think this can also be addressed by writing use case notes on how to edit phy registers using the mii command for debug/bringup purposes.


> The default values should come from the DT, once this is all in place.
> But I think that for initial board bringup / testing such a method, to
> override those values via environment variables can be quite helpful.

After reading through most of the comments, I agree with Marek it's possible (and probably best) to support overriding phy registers through use of the mii command. Non expert users (such as hardware developers bringing up a new board) could make use of notes to help them know how to try different settings. 

All the best!

Vince

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Vince Bridgers]

Hi Joe!

...
> > +1 from me as well.
...
> > Joe, whats your opinion on this?
...
> Do we really think this is a strong use-case?  This seem like the type of thing I
> would expect to tweak for testing through mii / mdio commands and then
> configure the device tree based on that.  This is pretty much a one-time thing for a
> given board it seems to me.
>
> If we really want a polished interface to it, we should define a sub-command /
> new command that phy drivers can implement.  I'm not sure an
> undiscoverable,  un-"help"-able list of env vars will be apparent to users.  Do you have a
> feeling for how close to universal any of these parameters are across phys?

I don't believe this is a strong use case for debugged and working boards. The intent was to support our downstream customers, that do develop their own boards and could use a method to help them with initial bringup. Many of these engineers in bringup are hardware/board developers and are not as comfortable as we are using uboot. It helps to support skewing these signals, especially if it can save a board spin  (having said this - this capability is not a substitute for a well designed board). The debug/bringup use case can be supported by describing to non-expert users how to make use of the mii command as Marek had originally suggested, so I'm fine with that solution.

On the question of how universal these parameters are across phys? Not so much. I've only encountered these on the two Micrel phys - the ksz9021 and ksz9031. And these have different signals that can be skewed.

Thanks for the comments and feedback! I'll focus on working out a solution through devicetree, and a README type of description of how to skew the signals for debug/bringup purposes using the mii command.

All the best!

Vince

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Vince Bridgers]

....
> No. But with your comments above (which make total sense), I tend to NAK
> this ability to configure the PHY skew timings via environment variables.

Understood, I'll focus on the devicetree implementation and a README to help non-expert users use the MII command for board bringup & debug purposes. 

Thanks for the comments and discussion!

Vince

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Stefan Roese]

Hi Vince,

On 13.02.2015 17:19, Vince Bridgers wrote:
> ....
>> No. But with your comments above (which make total sense), I tend to NAK
>> this ability to configure the PHY skew timings via environment variables.
>
> Understood, I'll focus on the devicetree implementation and a README to
 > help non-expert users use the MII command for board bringup & debug
 > purposes.

This README is a good idea and will be helpful. Thanks for doing this.

Thanks,
Stefan

[U-Boot] [PATCH 2/2] net: phy: Add ability to program the ksz9031 skew values from the uboot env

[Marek Vasut]

On Friday, February 13, 2015 at 05:44:11 PM, Stefan Roese wrote:
> Hi Vince,
> 
> On 13.02.2015 17:19, Vince Bridgers wrote:
> > ....
> > 
> >> No. But with your comments above (which make total sense), I tend to NAK
> >> this ability to configure the PHY skew timings via environment
> >> variables.
> > 
> > Understood, I'll focus on the devicetree implementation and a README to
> > 
>  > help non-expert users use the MII command for board bringup & debug
>  > purposes.
> 
> This README is a good idea and will be helpful. Thanks for doing this.

Hi!

Thank you for doing this indeed :)

Best regards,
Marek Vasut