[PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Martin Schiller]

This adds the possibility to configure the RGMII RX/TX clock skew via
devicetree.

Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and add
the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the
devicetree.

Furthermore, a warning is now issued if the phy mode is configured to
"rgmii" and an internal delay is set in the phy (e.g. by pin-strapping),
as in the dp83867 driver.

Signed-off-by: Martin Schiller <ms@dev.tdt.de>
---

Changes to v5:
o remove #if IS_ENABLED(CONFIG_OF_MDIO) check
o rename new function to xway_gphy_rgmii_init()

Changes to v4:
o Fix Alignment to match open parenthesis

Changes to v3:
o Fix typo in commit message
o use FIELD_PREP() and FIELD_GET() macros
o further code cleanups
o always mask rxskew AND txskew value in the register value

Changes to v2:
o Fix missing whitespace in warning.

Changes to v1:
o code cleanup and use phy_modify().
o use default of 2.0ns if delay property is absent instead of returning
  an error.

---
 drivers/net/phy/intel-xway.c | 78 ++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c
index d453ec016168..fd7da2eeb963 100644
--- a/drivers/net/phy/intel-xway.c
+++ b/drivers/net/phy/intel-xway.c
@@ -8,11 +8,16 @@
 #include <linux/module.h>
 #include <linux/phy.h>
 #include <linux/of.h>
+#include <linux/bitfield.h>
 
+#define XWAY_MDIO_MIICTRL		0x17	/* mii control */
 #define XWAY_MDIO_IMASK			0x19	/* interrupt mask */
 #define XWAY_MDIO_ISTAT			0x1A	/* interrupt status */
 #define XWAY_MDIO_LED			0x1B	/* led control */
 
+#define XWAY_MDIO_MIICTRL_RXSKEW_MASK	GENMASK(14, 12)
+#define XWAY_MDIO_MIICTRL_TXSKEW_MASK	GENMASK(10, 8)
+
 /* bit 15:12 are reserved */
 #define XWAY_MDIO_LED_LED3_EN		BIT(11)	/* Enable the integrated function of LED3 */
 #define XWAY_MDIO_LED_LED2_EN		BIT(10)	/* Enable the integrated function of LED2 */
@@ -157,6 +162,75 @@
 #define PHY_ID_PHY11G_VR9_1_2		0xD565A409
 #define PHY_ID_PHY22F_VR9_1_2		0xD565A419
 
+static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000, 2500,
+					 3000, 3500};
+
+static int xway_gphy_rgmii_init(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	unsigned int delay_size = ARRAY_SIZE(xway_internal_delay);
+	s32 int_delay;
+	int val = 0;
+
+	if (!phy_interface_is_rgmii(phydev))
+		return 0;
+
+	/* Existing behavior was to use default pin strapping delay in rgmii
+	 * mode, but rgmii should have meant no delay.  Warn existing users,
+	 * but do not change anything at the moment.
+	 */
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
+		u16 txskew, rxskew;
+
+		val = phy_read(phydev, XWAY_MDIO_MIICTRL);
+		if (val < 0)
+			return val;
+
+		txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK, val);
+		rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, val);
+
+		if (txskew > 0 || rxskew > 0)
+			phydev_warn(phydev,
+				    "PHY has delays (e.g. via pin strapping), but phy-mode = 'rgmii'\n"
+				    "Should be 'rgmii-id' to use internal delays txskew:%d ps rxskew:%d ps\n",
+				    xway_internal_delay[txskew],
+				    xway_internal_delay[rxskew]);
+		return 0;
+	}
+
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+		int_delay = phy_get_internal_delay(phydev, dev,
+						   xway_internal_delay,
+						   delay_size, true);
+
+		if (int_delay < 0) {
+			phydev_warn(phydev, "rx-internal-delay-ps is missing, use default of 2.0 ns\n");
+			int_delay = 4; /* 2000 ps */
+		}
+
+		val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, int_delay);
+	}
+
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+		int_delay = phy_get_internal_delay(phydev, dev,
+						   xway_internal_delay,
+						   delay_size, false);
+
+		if (int_delay < 0) {
+			phydev_warn(phydev, "tx-internal-delay-ps is missing, use default of 2.0 ns\n");
+			int_delay = 4; /* 2000 ps */
+		}
+
+		val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK, int_delay);
+	}
+
+	return phy_modify(phydev, XWAY_MDIO_MIICTRL,
+			  XWAY_MDIO_MIICTRL_RXSKEW_MASK |
+			  XWAY_MDIO_MIICTRL_TXSKEW_MASK, val);
+}
+
 static int xway_gphy_config_init(struct phy_device *phydev)
 {
 	int err;
@@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct phy_device *phydev)
 	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh);
 	phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl);
 
+	err = xway_gphy_rgmii_init(phydev);
+	if (err)
+		return err;
+
 	return 0;
 }
 
-- 
2.20.1

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Andrew Lunn]

> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> +		int_delay = phy_get_internal_delay(phydev, dev,
> +						   xway_internal_delay,
> +						   delay_size, true);
> +
> +		if (int_delay < 0) {
> +			phydev_warn(phydev, "rx-internal-delay-ps is missing, use default of 2.0 ns\n");
> +			int_delay = 4; /* 2000 ps */

The binding say:

 rx-internal-delay-ps:
    description: |
      RGMII Receive PHY Clock Delay defined in pico seconds.  This is used for
      PHY's that have configurable RX internal delays.  If this property is
      present then the PHY applies the RX delay.

So the property is optional. It being missing should not generate a
warning. Please just use the default of 2ns. This makes the usage the
same as the other drivers using phy_get_internal_delay().

     Andrew

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Martin Schiller]

On 2021-07-19 22:56, Andrew Lunn wrote:
>> +	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> +	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
>> +		int_delay = phy_get_internal_delay(phydev, dev,
>> +						   xway_internal_delay,
>> +						   delay_size, true);
>> +
>> +		if (int_delay < 0) {
>> +			phydev_warn(phydev, "rx-internal-delay-ps is missing, use default 
>> of 2.0 ns\n");
>> +			int_delay = 4; /* 2000 ps */
> 
> The binding say:
> 
>  rx-internal-delay-ps:
>     description: |
>       RGMII Receive PHY Clock Delay defined in pico seconds.  This is 
> used for
>       PHY's that have configurable RX internal delays.  If this 
> property is
>       present then the PHY applies the RX delay.
> 
> So the property is optional. It being missing should not generate a
> warning. Please just use the default of 2ns. This makes the usage the
> same as the other drivers using phy_get_internal_delay().
> 
>      Andrew

OK, I'll remove the warnings. Thanks.

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Tim Harvey]

On Mon, Jul 19, 2021 at 2:07 AM Martin Schiller <ms@dev.tdt.de> wrote:
>
> This adds the possibility to configure the RGMII RX/TX clock skew via
> devicetree.
>
> Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and add
> the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the
> devicetree.
>
> Furthermore, a warning is now issued if the phy mode is configured to
> "rgmii" and an internal delay is set in the phy (e.g. by pin-strapping),
> as in the dp83867 driver.
>
> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
> ---
>
> Changes to v5:
> o remove #if IS_ENABLED(CONFIG_OF_MDIO) check
> o rename new function to xway_gphy_rgmii_init()
>
> Changes to v4:
> o Fix Alignment to match open parenthesis
>
> Changes to v3:
> o Fix typo in commit message
> o use FIELD_PREP() and FIELD_GET() macros
> o further code cleanups
> o always mask rxskew AND txskew value in the register value
>
> Changes to v2:
> o Fix missing whitespace in warning.
>
> Changes to v1:
> o code cleanup and use phy_modify().
> o use default of 2.0ns if delay property is absent instead of returning
>   an error.
>
> ---
>  drivers/net/phy/intel-xway.c | 78 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
>
> diff --git a/drivers/net/phy/intel-xway.c b/drivers/net/phy/intel-xway.c
> index d453ec016168..fd7da2eeb963 100644
> --- a/drivers/net/phy/intel-xway.c
> +++ b/drivers/net/phy/intel-xway.c
> @@ -8,11 +8,16 @@
>  #include <linux/module.h>
>  #include <linux/phy.h>
>  #include <linux/of.h>
> +#include <linux/bitfield.h>
>
> +#define XWAY_MDIO_MIICTRL              0x17    /* mii control */
>  #define XWAY_MDIO_IMASK                        0x19    /* interrupt mask */
>  #define XWAY_MDIO_ISTAT                        0x1A    /* interrupt status */
>  #define XWAY_MDIO_LED                  0x1B    /* led control */
>
> +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK  GENMASK(14, 12)
> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK  GENMASK(10, 8)
> +
>  /* bit 15:12 are reserved */
>  #define XWAY_MDIO_LED_LED3_EN          BIT(11) /* Enable the integrated function of LED3 */
>  #define XWAY_MDIO_LED_LED2_EN          BIT(10) /* Enable the integrated function of LED2 */
> @@ -157,6 +162,75 @@
>  #define PHY_ID_PHY11G_VR9_1_2          0xD565A409
>  #define PHY_ID_PHY22F_VR9_1_2          0xD565A419
>
> +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000, 2500,
> +                                        3000, 3500};
> +
> +static int xway_gphy_rgmii_init(struct phy_device *phydev)
> +{
> +       struct device *dev = &phydev->mdio.dev;
> +       unsigned int delay_size = ARRAY_SIZE(xway_internal_delay);
> +       s32 int_delay;
> +       int val = 0;
> +
> +       if (!phy_interface_is_rgmii(phydev))
> +               return 0;
> +
> +       /* Existing behavior was to use default pin strapping delay in rgmii
> +        * mode, but rgmii should have meant no delay.  Warn existing users,
> +        * but do not change anything at the moment.
> +        */
> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
> +               u16 txskew, rxskew;
> +
> +               val = phy_read(phydev, XWAY_MDIO_MIICTRL);
> +               if (val < 0)
> +                       return val;
> +
> +               txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK, val);
> +               rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, val);
> +
> +               if (txskew > 0 || rxskew > 0)
> +                       phydev_warn(phydev,
> +                                   "PHY has delays (e.g. via pin strapping), but phy-mode = 'rgmii'\n"
> +                                   "Should be 'rgmii-id' to use internal delays txskew:%d ps rxskew:%d ps\n",
> +                                   xway_internal_delay[txskew],
> +                                   xway_internal_delay[rxskew]);
> +               return 0;
> +       }
> +
> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> +               int_delay = phy_get_internal_delay(phydev, dev,
> +                                                  xway_internal_delay,
> +                                                  delay_size, true);
> +
> +               if (int_delay < 0) {
> +                       phydev_warn(phydev, "rx-internal-delay-ps is missing, use default of 2.0 ns\n");
> +                       int_delay = 4; /* 2000 ps */
> +               }
> +
> +               val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, int_delay);
> +       }
> +
> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> +               int_delay = phy_get_internal_delay(phydev, dev,
> +                                                  xway_internal_delay,
> +                                                  delay_size, false);
> +
> +               if (int_delay < 0) {
> +                       phydev_warn(phydev, "tx-internal-delay-ps is missing, use default of 2.0 ns\n");
> +                       int_delay = 4; /* 2000 ps */
> +               }
> +
> +               val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK, int_delay);
> +       }
> +
> +       return phy_modify(phydev, XWAY_MDIO_MIICTRL,
> +                         XWAY_MDIO_MIICTRL_RXSKEW_MASK |
> +                         XWAY_MDIO_MIICTRL_TXSKEW_MASK, val);
> +}
> +
>  static int xway_gphy_config_init(struct phy_device *phydev)
>  {
>         int err;
> @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct phy_device *phydev)
>         phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh);
>         phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl);
>
> +       err = xway_gphy_rgmii_init(phydev);
> +       if (err)
> +               return err;
> +
>         return 0;
>  }
>
> --
> 2.20.1
>

Martin,

I've got some boards with the GPY111 phy on them and I'm finding that
modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I
do a soft reset (BCMR_RESET) first. I don't see anything in the
datasheet which specifies this to be the case so I'm interested it
what you have found. Are you sure adjusting the skews like this
without a soft (or hard pin based) reset actually works?

Best regards,

Tim

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Martin Schiller]

On 2022-01-11 00:12, Tim Harvey wrote:
> On Mon, Jul 19, 2021 at 2:07 AM Martin Schiller <ms@dev.tdt.de> wrote:
>> 
>> This adds the possibility to configure the RGMII RX/TX clock skew via
>> devicetree.
>> 
>> Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and 
>> add
>> the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the
>> devicetree.
>> 
>> Furthermore, a warning is now issued if the phy mode is configured to
>> "rgmii" and an internal delay is set in the phy (e.g. by 
>> pin-strapping),
>> as in the dp83867 driver.
>> 
>> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
>> ---
>> 
>> Changes to v5:
>> o remove #if IS_ENABLED(CONFIG_OF_MDIO) check
>> o rename new function to xway_gphy_rgmii_init()
>> 
>> Changes to v4:
>> o Fix Alignment to match open parenthesis
>> 
>> Changes to v3:
>> o Fix typo in commit message
>> o use FIELD_PREP() and FIELD_GET() macros
>> o further code cleanups
>> o always mask rxskew AND txskew value in the register value
>> 
>> Changes to v2:
>> o Fix missing whitespace in warning.
>> 
>> Changes to v1:
>> o code cleanup and use phy_modify().
>> o use default of 2.0ns if delay property is absent instead of 
>> returning
>>   an error.
>> 
>> ---
>>  drivers/net/phy/intel-xway.c | 78 
>> ++++++++++++++++++++++++++++++++++++
>>  1 file changed, 78 insertions(+)
>> 
>> diff --git a/drivers/net/phy/intel-xway.c 
>> b/drivers/net/phy/intel-xway.c
>> index d453ec016168..fd7da2eeb963 100644
>> --- a/drivers/net/phy/intel-xway.c
>> +++ b/drivers/net/phy/intel-xway.c
>> @@ -8,11 +8,16 @@
>>  #include <linux/module.h>
>>  #include <linux/phy.h>
>>  #include <linux/of.h>
>> +#include <linux/bitfield.h>
>> 
>> +#define XWAY_MDIO_MIICTRL              0x17    /* mii control */
>>  #define XWAY_MDIO_IMASK                        0x19    /* interrupt 
>> mask */
>>  #define XWAY_MDIO_ISTAT                        0x1A    /* interrupt 
>> status */
>>  #define XWAY_MDIO_LED                  0x1B    /* led control */
>> 
>> +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK  GENMASK(14, 12)
>> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK  GENMASK(10, 8)
>> +
>>  /* bit 15:12 are reserved */
>>  #define XWAY_MDIO_LED_LED3_EN          BIT(11) /* Enable the 
>> integrated function of LED3 */
>>  #define XWAY_MDIO_LED_LED2_EN          BIT(10) /* Enable the 
>> integrated function of LED2 */
>> @@ -157,6 +162,75 @@
>>  #define PHY_ID_PHY11G_VR9_1_2          0xD565A409
>>  #define PHY_ID_PHY22F_VR9_1_2          0xD565A419
>> 
>> +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000, 
>> 2500,
>> +                                        3000, 3500};
>> +
>> +static int xway_gphy_rgmii_init(struct phy_device *phydev)
>> +{
>> +       struct device *dev = &phydev->mdio.dev;
>> +       unsigned int delay_size = ARRAY_SIZE(xway_internal_delay);
>> +       s32 int_delay;
>> +       int val = 0;
>> +
>> +       if (!phy_interface_is_rgmii(phydev))
>> +               return 0;
>> +
>> +       /* Existing behavior was to use default pin strapping delay in 
>> rgmii
>> +        * mode, but rgmii should have meant no delay.  Warn existing 
>> users,
>> +        * but do not change anything at the moment.
>> +        */
>> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
>> +               u16 txskew, rxskew;
>> +
>> +               val = phy_read(phydev, XWAY_MDIO_MIICTRL);
>> +               if (val < 0)
>> +                       return val;
>> +
>> +               txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK, 
>> val);
>> +               rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK, 
>> val);
>> +
>> +               if (txskew > 0 || rxskew > 0)
>> +                       phydev_warn(phydev,
>> +                                   "PHY has delays (e.g. via pin 
>> strapping), but phy-mode = 'rgmii'\n"
>> +                                   "Should be 'rgmii-id' to use 
>> internal delays txskew:%d ps rxskew:%d ps\n",
>> +                                   xway_internal_delay[txskew],
>> +                                   xway_internal_delay[rxskew]);
>> +               return 0;
>> +       }
>> +
>> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
>> +               int_delay = phy_get_internal_delay(phydev, dev,
>> +                                                  
>> xway_internal_delay,
>> +                                                  delay_size, true);
>> +
>> +               if (int_delay < 0) {
>> +                       phydev_warn(phydev, "rx-internal-delay-ps is 
>> missing, use default of 2.0 ns\n");
>> +                       int_delay = 4; /* 2000 ps */
>> +               }
>> +
>> +               val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK, 
>> int_delay);
>> +       }
>> +
>> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
>> +               int_delay = phy_get_internal_delay(phydev, dev,
>> +                                                  
>> xway_internal_delay,
>> +                                                  delay_size, false);
>> +
>> +               if (int_delay < 0) {
>> +                       phydev_warn(phydev, "tx-internal-delay-ps is 
>> missing, use default of 2.0 ns\n");
>> +                       int_delay = 4; /* 2000 ps */
>> +               }
>> +
>> +               val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK, 
>> int_delay);
>> +       }
>> +
>> +       return phy_modify(phydev, XWAY_MDIO_MIICTRL,
>> +                         XWAY_MDIO_MIICTRL_RXSKEW_MASK |
>> +                         XWAY_MDIO_MIICTRL_TXSKEW_MASK, val);
>> +}
>> +
>>  static int xway_gphy_config_init(struct phy_device *phydev)
>>  {
>>         int err;
>> @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct 
>> phy_device *phydev)
>>         phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh);
>>         phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl);
>> 
>> +       err = xway_gphy_rgmii_init(phydev);
>> +       if (err)
>> +               return err;
>> +
>>         return 0;
>>  }
>> 
>> --
>> 2.20.1
>> 
> 
> Martin,
> 
> I've got some boards with the GPY111 phy on them and I'm finding that
> modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I
> do a soft reset (BCMR_RESET) first. I don't see anything in the
> datasheet which specifies this to be the case so I'm interested it
> what you have found. Are you sure adjusting the skews like this
> without a soft (or hard pin based) reset actually works?
> 
> Best regards,
> 
> Tim

Hello Tim,

yes, you are right. It is not applied immediately. The link needs to be
toggled to get this settings active. But my experience shows that this
would be done in the further boot process anyway e.g. by restarting the
autonegotiation etc.

Regards,
Martin

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Andrew Lunn]

> > Martin,
> > 
> > I've got some boards with the GPY111 phy on them and I'm finding that
> > modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I
> > do a soft reset (BCMR_RESET) first. I don't see anything in the
> > datasheet which specifies this to be the case so I'm interested it
> > what you have found. Are you sure adjusting the skews like this
> > without a soft (or hard pin based) reset actually works?
> > 
> > Best regards,
> > 
> > Tim
> 
> Hello Tim,
> 
> yes, you are right. It is not applied immediately. The link needs to be
> toggled to get this settings active. But my experience shows that this
> would be done in the further boot process anyway e.g. by restarting the
> autonegotiation etc.

Hi Martin

Have you verified this? Maybe try NFS root, so the kernel is the one
up'ing the interface, before user space exists.

       Andrew

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Tim Harvey]

On Mon, Jan 10, 2022 at 11:44 PM Martin Schiller <ms@dev.tdt.de> wrote:
>
> On 2022-01-11 00:12, Tim Harvey wrote:
> > On Mon, Jul 19, 2021 at 2:07 AM Martin Schiller <ms@dev.tdt.de> wrote:
> >>
> >> This adds the possibility to configure the RGMII RX/TX clock skew via
> >> devicetree.
> >>
> >> Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and
> >> add
> >> the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the
> >> devicetree.
> >>
> >> Furthermore, a warning is now issued if the phy mode is configured to
> >> "rgmii" and an internal delay is set in the phy (e.g. by
> >> pin-strapping),
> >> as in the dp83867 driver.
> >>
> >> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
> >> ---
> >>
> >> Changes to v5:
> >> o remove #if IS_ENABLED(CONFIG_OF_MDIO) check
> >> o rename new function to xway_gphy_rgmii_init()
> >>
> >> Changes to v4:
> >> o Fix Alignment to match open parenthesis
> >>
> >> Changes to v3:
> >> o Fix typo in commit message
> >> o use FIELD_PREP() and FIELD_GET() macros
> >> o further code cleanups
> >> o always mask rxskew AND txskew value in the register value
> >>
> >> Changes to v2:
> >> o Fix missing whitespace in warning.
> >>
> >> Changes to v1:
> >> o code cleanup and use phy_modify().
> >> o use default of 2.0ns if delay property is absent instead of
> >> returning
> >>   an error.
> >>
> >> ---
> >>  drivers/net/phy/intel-xway.c | 78
> >> ++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 78 insertions(+)
> >>
> >> diff --git a/drivers/net/phy/intel-xway.c
> >> b/drivers/net/phy/intel-xway.c
> >> index d453ec016168..fd7da2eeb963 100644
> >> --- a/drivers/net/phy/intel-xway.c
> >> +++ b/drivers/net/phy/intel-xway.c
> >> @@ -8,11 +8,16 @@
> >>  #include <linux/module.h>
> >>  #include <linux/phy.h>
> >>  #include <linux/of.h>
> >> +#include <linux/bitfield.h>
> >>
> >> +#define XWAY_MDIO_MIICTRL              0x17    /* mii control */
> >>  #define XWAY_MDIO_IMASK                        0x19    /* interrupt
> >> mask */
> >>  #define XWAY_MDIO_ISTAT                        0x1A    /* interrupt
> >> status */
> >>  #define XWAY_MDIO_LED                  0x1B    /* led control */
> >>
> >> +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK  GENMASK(14, 12)
> >> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK  GENMASK(10, 8)
> >> +
> >>  /* bit 15:12 are reserved */
> >>  #define XWAY_MDIO_LED_LED3_EN          BIT(11) /* Enable the
> >> integrated function of LED3 */
> >>  #define XWAY_MDIO_LED_LED2_EN          BIT(10) /* Enable the
> >> integrated function of LED2 */
> >> @@ -157,6 +162,75 @@
> >>  #define PHY_ID_PHY11G_VR9_1_2          0xD565A409
> >>  #define PHY_ID_PHY22F_VR9_1_2          0xD565A419
> >>
> >> +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000,
> >> 2500,
> >> +                                        3000, 3500};
> >> +
> >> +static int xway_gphy_rgmii_init(struct phy_device *phydev)
> >> +{
> >> +       struct device *dev = &phydev->mdio.dev;
> >> +       unsigned int delay_size = ARRAY_SIZE(xway_internal_delay);
> >> +       s32 int_delay;
> >> +       int val = 0;
> >> +
> >> +       if (!phy_interface_is_rgmii(phydev))
> >> +               return 0;
> >> +
> >> +       /* Existing behavior was to use default pin strapping delay in
> >> rgmii
> >> +        * mode, but rgmii should have meant no delay.  Warn existing
> >> users,
> >> +        * but do not change anything at the moment.
> >> +        */
> >> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
> >> +               u16 txskew, rxskew;
> >> +
> >> +               val = phy_read(phydev, XWAY_MDIO_MIICTRL);
> >> +               if (val < 0)
> >> +                       return val;
> >> +
> >> +               txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK,
> >> val);
> >> +               rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK,
> >> val);
> >> +
> >> +               if (txskew > 0 || rxskew > 0)
> >> +                       phydev_warn(phydev,
> >> +                                   "PHY has delays (e.g. via pin
> >> strapping), but phy-mode = 'rgmii'\n"
> >> +                                   "Should be 'rgmii-id' to use
> >> internal delays txskew:%d ps rxskew:%d ps\n",
> >> +                                   xway_internal_delay[txskew],
> >> +                                   xway_internal_delay[rxskew]);
> >> +               return 0;
> >> +       }
> >> +
> >> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> >> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> >> +               int_delay = phy_get_internal_delay(phydev, dev,
> >> +
> >> xway_internal_delay,
> >> +                                                  delay_size, true);
> >> +
> >> +               if (int_delay < 0) {
> >> +                       phydev_warn(phydev, "rx-internal-delay-ps is
> >> missing, use default of 2.0 ns\n");
> >> +                       int_delay = 4; /* 2000 ps */
> >> +               }
> >> +
> >> +               val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK,
> >> int_delay);
> >> +       }
> >> +
> >> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> >> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> >> +               int_delay = phy_get_internal_delay(phydev, dev,
> >> +
> >> xway_internal_delay,
> >> +                                                  delay_size, false);
> >> +
> >> +               if (int_delay < 0) {
> >> +                       phydev_warn(phydev, "tx-internal-delay-ps is
> >> missing, use default of 2.0 ns\n");
> >> +                       int_delay = 4; /* 2000 ps */
> >> +               }
> >> +
> >> +               val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK,
> >> int_delay);
> >> +       }
> >> +
> >> +       return phy_modify(phydev, XWAY_MDIO_MIICTRL,
> >> +                         XWAY_MDIO_MIICTRL_RXSKEW_MASK |
> >> +                         XWAY_MDIO_MIICTRL_TXSKEW_MASK, val);
> >> +}
> >> +
> >>  static int xway_gphy_config_init(struct phy_device *phydev)
> >>  {
> >>         int err;
> >> @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct
> >> phy_device *phydev)
> >>         phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh);
> >>         phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl);
> >>
> >> +       err = xway_gphy_rgmii_init(phydev);
> >> +       if (err)
> >> +               return err;
> >> +
> >>         return 0;
> >>  }
> >>
> >> --
> >> 2.20.1
> >>
> >
> > Martin,
> >
> > I've got some boards with the GPY111 phy on them and I'm finding that
> > modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I
> > do a soft reset (BCMR_RESET) first. I don't see anything in the
> > datasheet which specifies this to be the case so I'm interested it
> > what you have found. Are you sure adjusting the skews like this
> > without a soft (or hard pin based) reset actually works?
> >
> > Best regards,
> >
> > Tim
>
> Hello Tim,
>
> yes, you are right. It is not applied immediately. The link needs to be
> toggled to get this settings active. But my experience shows that this
> would be done in the further boot process anyway e.g. by restarting the
> autonegotiation etc.
>

Martin,

I added a debug statement in xway_gphy_rgmii_init and here you can see
it gets called 'before' the link comes up from the NIC on a board that
has a cable plugged in at power-on. I can tell from testing that the
rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take
effect unless I then bring the link down and up again manually as you
indicate.

# dmesg | egrep "xway|nicvf"
[    6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100
rx_delay=1500 tx_delay=500
[    6.999651] nicvf, ver 1.0
[    7.002478] nicvf 0000:05:00.1: Adding to iommu group 7
[    7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006)
[    7.053189] nicvf 0000:05:00.2: Adding to iommu group 8
[    7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006)
[   11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex

If I add a 'genphy_soft_reset(phydev);' at the top of
xway_gphy_rgmii_init before the write to XWAY_MDIO_MIICTRL the values
do take effect so perhaps that's the proper fix.

I'm not fond of even using this phy driver either as it blatantly
forces LED configuration which may not agree with what boot firmware
does. I've noticed phy drivers starting to configure LED behavior more
and more but it seems like there should be dt bindings for that or
maybe an option to preserve the configuration that is set from boot
firmware.

Best regards,

Tim

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Martin Schiller]

On 2022-01-11 20:12, Tim Harvey wrote:
> On Mon, Jan 10, 2022 at 11:44 PM Martin Schiller <ms@dev.tdt.de> wrote:
>> 
>> On 2022-01-11 00:12, Tim Harvey wrote:
>> > On Mon, Jul 19, 2021 at 2:07 AM Martin Schiller <ms@dev.tdt.de> wrote:
>> >>
>> >> This adds the possibility to configure the RGMII RX/TX clock skew via
>> >> devicetree.
>> >>
>> >> Simply set phy mode to "rgmii-id", "rgmii-rxid" or "rgmii-txid" and
>> >> add
>> >> the "rx-internal-delay-ps" or "tx-internal-delay-ps" property to the
>> >> devicetree.
>> >>
>> >> Furthermore, a warning is now issued if the phy mode is configured to
>> >> "rgmii" and an internal delay is set in the phy (e.g. by
>> >> pin-strapping),
>> >> as in the dp83867 driver.
>> >>
>> >> Signed-off-by: Martin Schiller <ms@dev.tdt.de>
>> >> ---
>> >>
>> >> Changes to v5:
>> >> o remove #if IS_ENABLED(CONFIG_OF_MDIO) check
>> >> o rename new function to xway_gphy_rgmii_init()
>> >>
>> >> Changes to v4:
>> >> o Fix Alignment to match open parenthesis
>> >>
>> >> Changes to v3:
>> >> o Fix typo in commit message
>> >> o use FIELD_PREP() and FIELD_GET() macros
>> >> o further code cleanups
>> >> o always mask rxskew AND txskew value in the register value
>> >>
>> >> Changes to v2:
>> >> o Fix missing whitespace in warning.
>> >>
>> >> Changes to v1:
>> >> o code cleanup and use phy_modify().
>> >> o use default of 2.0ns if delay property is absent instead of
>> >> returning
>> >>   an error.
>> >>
>> >> ---
>> >>  drivers/net/phy/intel-xway.c | 78
>> >> ++++++++++++++++++++++++++++++++++++
>> >>  1 file changed, 78 insertions(+)
>> >>
>> >> diff --git a/drivers/net/phy/intel-xway.c
>> >> b/drivers/net/phy/intel-xway.c
>> >> index d453ec016168..fd7da2eeb963 100644
>> >> --- a/drivers/net/phy/intel-xway.c
>> >> +++ b/drivers/net/phy/intel-xway.c
>> >> @@ -8,11 +8,16 @@
>> >>  #include <linux/module.h>
>> >>  #include <linux/phy.h>
>> >>  #include <linux/of.h>
>> >> +#include <linux/bitfield.h>
>> >>
>> >> +#define XWAY_MDIO_MIICTRL              0x17    /* mii control */
>> >>  #define XWAY_MDIO_IMASK                        0x19    /* interrupt
>> >> mask */
>> >>  #define XWAY_MDIO_ISTAT                        0x1A    /* interrupt
>> >> status */
>> >>  #define XWAY_MDIO_LED                  0x1B    /* led control */
>> >>
>> >> +#define XWAY_MDIO_MIICTRL_RXSKEW_MASK  GENMASK(14, 12)
>> >> +#define XWAY_MDIO_MIICTRL_TXSKEW_MASK  GENMASK(10, 8)
>> >> +
>> >>  /* bit 15:12 are reserved */
>> >>  #define XWAY_MDIO_LED_LED3_EN          BIT(11) /* Enable the
>> >> integrated function of LED3 */
>> >>  #define XWAY_MDIO_LED_LED2_EN          BIT(10) /* Enable the
>> >> integrated function of LED2 */
>> >> @@ -157,6 +162,75 @@
>> >>  #define PHY_ID_PHY11G_VR9_1_2          0xD565A409
>> >>  #define PHY_ID_PHY22F_VR9_1_2          0xD565A419
>> >>
>> >> +static const int xway_internal_delay[] = {0, 500, 1000, 1500, 2000,
>> >> 2500,
>> >> +                                        3000, 3500};
>> >> +
>> >> +static int xway_gphy_rgmii_init(struct phy_device *phydev)
>> >> +{
>> >> +       struct device *dev = &phydev->mdio.dev;
>> >> +       unsigned int delay_size = ARRAY_SIZE(xway_internal_delay);
>> >> +       s32 int_delay;
>> >> +       int val = 0;
>> >> +
>> >> +       if (!phy_interface_is_rgmii(phydev))
>> >> +               return 0;
>> >> +
>> >> +       /* Existing behavior was to use default pin strapping delay in
>> >> rgmii
>> >> +        * mode, but rgmii should have meant no delay.  Warn existing
>> >> users,
>> >> +        * but do not change anything at the moment.
>> >> +        */
>> >> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
>> >> +               u16 txskew, rxskew;
>> >> +
>> >> +               val = phy_read(phydev, XWAY_MDIO_MIICTRL);
>> >> +               if (val < 0)
>> >> +                       return val;
>> >> +
>> >> +               txskew = FIELD_GET(XWAY_MDIO_MIICTRL_TXSKEW_MASK,
>> >> val);
>> >> +               rxskew = FIELD_GET(XWAY_MDIO_MIICTRL_RXSKEW_MASK,
>> >> val);
>> >> +
>> >> +               if (txskew > 0 || rxskew > 0)
>> >> +                       phydev_warn(phydev,
>> >> +                                   "PHY has delays (e.g. via pin
>> >> strapping), but phy-mode = 'rgmii'\n"
>> >> +                                   "Should be 'rgmii-id' to use
>> >> internal delays txskew:%d ps rxskew:%d ps\n",
>> >> +                                   xway_internal_delay[txskew],
>> >> +                                   xway_internal_delay[rxskew]);
>> >> +               return 0;
>> >> +       }
>> >> +
>> >> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> >> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
>> >> +               int_delay = phy_get_internal_delay(phydev, dev,
>> >> +
>> >> xway_internal_delay,
>> >> +                                                  delay_size, true);
>> >> +
>> >> +               if (int_delay < 0) {
>> >> +                       phydev_warn(phydev, "rx-internal-delay-ps is
>> >> missing, use default of 2.0 ns\n");
>> >> +                       int_delay = 4; /* 2000 ps */
>> >> +               }
>> >> +
>> >> +               val |= FIELD_PREP(XWAY_MDIO_MIICTRL_RXSKEW_MASK,
>> >> int_delay);
>> >> +       }
>> >> +
>> >> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
>> >> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
>> >> +               int_delay = phy_get_internal_delay(phydev, dev,
>> >> +
>> >> xway_internal_delay,
>> >> +                                                  delay_size, false);
>> >> +
>> >> +               if (int_delay < 0) {
>> >> +                       phydev_warn(phydev, "tx-internal-delay-ps is
>> >> missing, use default of 2.0 ns\n");
>> >> +                       int_delay = 4; /* 2000 ps */
>> >> +               }
>> >> +
>> >> +               val |= FIELD_PREP(XWAY_MDIO_MIICTRL_TXSKEW_MASK,
>> >> int_delay);
>> >> +       }
>> >> +
>> >> +       return phy_modify(phydev, XWAY_MDIO_MIICTRL,
>> >> +                         XWAY_MDIO_MIICTRL_RXSKEW_MASK |
>> >> +                         XWAY_MDIO_MIICTRL_TXSKEW_MASK, val);
>> >> +}
>> >> +
>> >>  static int xway_gphy_config_init(struct phy_device *phydev)
>> >>  {
>> >>         int err;
>> >> @@ -204,6 +278,10 @@ static int xway_gphy_config_init(struct
>> >> phy_device *phydev)
>> >>         phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2H, ledxh);
>> >>         phy_write_mmd(phydev, MDIO_MMD_VEND2, XWAY_MMD_LED2L, ledxl);
>> >>
>> >> +       err = xway_gphy_rgmii_init(phydev);
>> >> +       if (err)
>> >> +               return err;
>> >> +
>> >>         return 0;
>> >>  }
>> >>
>> >> --
>> >> 2.20.1
>> >>
>> >
>> > Martin,
>> >
>> > I've got some boards with the GPY111 phy on them and I'm finding that
>> > modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I
>> > do a soft reset (BCMR_RESET) first. I don't see anything in the
>> > datasheet which specifies this to be the case so I'm interested it
>> > what you have found. Are you sure adjusting the skews like this
>> > without a soft (or hard pin based) reset actually works?
>> >
>> > Best regards,
>> >
>> > Tim
>> 
>> Hello Tim,
>> 
>> yes, you are right. It is not applied immediately. The link needs to 
>> be
>> toggled to get this settings active. But my experience shows that this
>> would be done in the further boot process anyway e.g. by restarting 
>> the
>> autonegotiation etc.
>> 
> 
> Martin,
> 
> I added a debug statement in xway_gphy_rgmii_init and here you can see
> it gets called 'before' the link comes up from the NIC on a board that
> has a cable plugged in at power-on. I can tell from testing that the
> rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take
> effect unless I then bring the link down and up again manually as you
> indicate.
> 
> # dmesg | egrep "xway|nicvf"
> [    6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100
> rx_delay=1500 tx_delay=500
> [    6.999651] nicvf, ver 1.0
> [    7.002478] nicvf 0000:05:00.1: Adding to iommu group 7
> [    7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006)
> [    7.053189] nicvf 0000:05:00.2: Adding to iommu group 8
> [    7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006)
> [   11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full 
> duplex
> 
> If I add a 'genphy_soft_reset(phydev);' at the top of
> xway_gphy_rgmii_init before the write to XWAY_MDIO_MIICTRL the values
> do take effect so perhaps that's the proper fix.

OK, I see that we have to change something here.
But I would like to avoid a complete reset (BMCR_RESET) if possible.
How about this:
Before configuring the skews set BMCR_PDOWN using genphy_suspend(phydev)
and when we are done let's call genphy_resume(phydev).

> 
> I'm not fond of even using this phy driver either as it blatantly
> forces LED configuration which may not agree with what boot firmware
> does. I've noticed phy drivers starting to configure LED behavior more
> and more but it seems like there should be dt bindings for that or
> maybe an option to preserve the configuration that is set from boot
> firmware.

There is already an approach which is used in openwrt [1] and which
Hauke tried to get into the kernel [2].

Interesting would be also a solution like this approach here [3].

But this is rather off-topic now.

[1] 
https://git.openwrt.org/?p=openwrt/openwrt.git;a=blob;f=target/linux/lantiq/patches-5.10/0023-NET-PHY-add-led-support-for-intel-xway.patch;h=fb8d97511066bd7e20f8cd298401e81f74e02ae9;hb=d337731f85c880acc96e8a6b99b62aeb57b8253f
[2] https://www.spinics.net/lists/devicetree/msg129574.html
[3] https://www.spinics.net/lists/linux-leds/msg17241.html

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Andrew Lunn]

> > If I add a 'genphy_soft_reset(phydev);' at the top of
> > xway_gphy_rgmii_init before the write to XWAY_MDIO_MIICTRL the values
> > do take effect so perhaps that's the proper fix.
> 
> OK, I see that we have to change something here.
> But I would like to avoid a complete reset (BMCR_RESET) if possible.

What does the datasheet say about BMCR_RESET? Some PHYs, like Marvell,
it only resets the internal state machines. Register values are not
changed back to defaults or anything like that. Also for many register
writes in Marvell PHYs the write does not take effect until the next
reset.

So a BMCR_RESET can be the correct thing to do.

   Andrew

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Russell King (Oracle)]

On Tue, Jan 11, 2022 at 11:12:33AM -0800, Tim Harvey wrote:
> I added a debug statement in xway_gphy_rgmii_init and here you can see
> it gets called 'before' the link comes up from the NIC on a board that
> has a cable plugged in at power-on. I can tell from testing that the
> rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take
> effect unless I then bring the link down and up again manually as you
> indicate.
> 
> # dmesg | egrep "xway|nicvf"
> [    6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100
> rx_delay=1500 tx_delay=500
> [    6.999651] nicvf, ver 1.0
> [    7.002478] nicvf 0000:05:00.1: Adding to iommu group 7
> [    7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006)
> [    7.053189] nicvf 0000:05:00.2: Adding to iommu group 8
> [    7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006)
> [   11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex

Does the kernel message about the link coming up reflect what is going
on physically with the link though?

If a network interface is down, it's entirely possible that the link is
already established at the hardware level, buit the "Link is Up" message
gets reported when the network interface is later brought up. So,
debugging this by looking at the kernel messages is unreliable.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Tim Harvey]

On Wed, Jan 12, 2022 at 5:14 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > If I add a 'genphy_soft_reset(phydev);' at the top of
> > > xway_gphy_rgmii_init before the write to XWAY_MDIO_MIICTRL the values
> > > do take effect so perhaps that's the proper fix.
> >
> > OK, I see that we have to change something here.
> > But I would like to avoid a complete reset (BMCR_RESET) if possible.
>
> What does the datasheet say about BMCR_RESET? Some PHYs, like Marvell,
> it only resets the internal state machines. Register values are not
> changed back to defaults or anything like that. Also for many register
> writes in Marvell PHYs the write does not take effect until the next
> reset.
>
> So a BMCR_RESET can be the correct thing to do.
>

Andrew,

Datasheet [1] says "Resets the PHY to its default state. Active links
are terminated. Note that this is a self-clearing bit which is set to
zero by the hardware after reset has been done. See also IEEE
802.3-2008 22.2.4.1.1."

Experimentally I can change the delays and read them back as such,
then issue a BMCR_RESET and read them and they revert to strapped
values so I know BMCR_RESET resets at least some of the registers.

I suppose something to force auto-negotiation to occur again
(BMCR_ANEN?) would suffice. I'm not sure what the best course of
action is.

Tim
[1] https://assets.maxlinear.com/web/documents/618152_gpy111_pef7071vv16_hd_rev1.5.pdf

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Tim Harvey]

On Wed, Jan 12, 2022 at 5:46 AM Russell King (Oracle)
<linux@armlinux.org.uk> wrote:
>
> On Tue, Jan 11, 2022 at 11:12:33AM -0800, Tim Harvey wrote:
> > I added a debug statement in xway_gphy_rgmii_init and here you can see
> > it gets called 'before' the link comes up from the NIC on a board that
> > has a cable plugged in at power-on. I can tell from testing that the
> > rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take
> > effect unless I then bring the link down and up again manually as you
> > indicate.
> >
> > # dmesg | egrep "xway|nicvf"
> > [    6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100
> > rx_delay=1500 tx_delay=500
> > [    6.999651] nicvf, ver 1.0
> > [    7.002478] nicvf 0000:05:00.1: Adding to iommu group 7
> > [    7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006)
> > [    7.053189] nicvf 0000:05:00.2: Adding to iommu group 8
> > [    7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006)
> > [   11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex
>
> Does the kernel message about the link coming up reflect what is going
> on physically with the link though?
>
> If a network interface is down, it's entirely possible that the link is
> already established at the hardware level, buit the "Link is Up" message
> gets reported when the network interface is later brought up. So,
> debugging this by looking at the kernel messages is unreliable.
>

Russell,

You are correct... the link doesn't come up at that point its already
linked. So we need to force a reset or an auto negotiation reset after
modifying the delays.

Tim

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Martin Schiller]

On 2022-01-12 19:25, Tim Harvey wrote:
> On Wed, Jan 12, 2022 at 5:46 AM Russell King (Oracle)
> <linux@armlinux.org.uk> wrote:
>> 
>> On Tue, Jan 11, 2022 at 11:12:33AM -0800, Tim Harvey wrote:
>> > I added a debug statement in xway_gphy_rgmii_init and here you can see
>> > it gets called 'before' the link comes up from the NIC on a board that
>> > has a cable plugged in at power-on. I can tell from testing that the
>> > rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take
>> > effect unless I then bring the link down and up again manually as you
>> > indicate.
>> >
>> > # dmesg | egrep "xway|nicvf"
>> > [    6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100
>> > rx_delay=1500 tx_delay=500
>> > [    6.999651] nicvf, ver 1.0
>> > [    7.002478] nicvf 0000:05:00.1: Adding to iommu group 7
>> > [    7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006)
>> > [    7.053189] nicvf 0000:05:00.2: Adding to iommu group 8
>> > [    7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006)
>> > [   11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex
>> 
>> Does the kernel message about the link coming up reflect what is going
>> on physically with the link though?
>> 
>> If a network interface is down, it's entirely possible that the link 
>> is
>> already established at the hardware level, buit the "Link is Up" 
>> message
>> gets reported when the network interface is later brought up. So,
>> debugging this by looking at the kernel messages is unreliable.
>> 
> 
> Russell,
> 
> You are correct... the link doesn't come up at that point its already
> linked. So we need to force a reset or an auto negotiation reset after
> modifying the delays.
> 
> Tim

Setting BMCR_ANRESTART would work, but only if BMCR_ANENABLE is also or
already set. Otherwise BMCR_ANRESTART has no effect (see the note in the
datasheet).

This is the reason why I came up with the idea of BMCR_PDOWN.

Personally I would have no problem with setting BMCR_ANRESTART and
BMCR_ANENABLE, but it would possibly change the existing configuration
if (e.g. by the bootloader) aneg should be disabled.

Martin

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Tim Harvey]

On Wed, Jan 12, 2022 at 10:32 PM Martin Schiller <ms@dev.tdt.de> wrote:
>
> On 2022-01-12 19:25, Tim Harvey wrote:
> > On Wed, Jan 12, 2022 at 5:46 AM Russell King (Oracle)
> > <linux@armlinux.org.uk> wrote:
> >>
> >> On Tue, Jan 11, 2022 at 11:12:33AM -0800, Tim Harvey wrote:
> >> > I added a debug statement in xway_gphy_rgmii_init and here you can see
> >> > it gets called 'before' the link comes up from the NIC on a board that
> >> > has a cable plugged in at power-on. I can tell from testing that the
> >> > rx_delay/tx_delay set in xway_gphy_rgmii_init does not actually take
> >> > effect unless I then bring the link down and up again manually as you
> >> > indicate.
> >> >
> >> > # dmesg | egrep "xway|nicvf"
> >> > [    6.855971] xway_gphy_rgmii_init mdio_thunder MDI_MIICTRL:0xb100
> >> > rx_delay=1500 tx_delay=500
> >> > [    6.999651] nicvf, ver 1.0
> >> > [    7.002478] nicvf 0000:05:00.1: Adding to iommu group 7
> >> > [    7.007785] nicvf 0000:05:00.1: enabling device (0004 -> 0006)
> >> > [    7.053189] nicvf 0000:05:00.2: Adding to iommu group 8
> >> > [    7.058511] nicvf 0000:05:00.2: enabling device (0004 -> 0006)
> >> > [   11.044616] nicvf 0000:05:00.2 eth1: Link is Up 1000 Mbps Full duplex
> >>
> >> Does the kernel message about the link coming up reflect what is going
> >> on physically with the link though?
> >>
> >> If a network interface is down, it's entirely possible that the link
> >> is
> >> already established at the hardware level, buit the "Link is Up"
> >> message
> >> gets reported when the network interface is later brought up. So,
> >> debugging this by looking at the kernel messages is unreliable.
> >>
> >
> > Russell,
> >
> > You are correct... the link doesn't come up at that point its already
> > linked. So we need to force a reset or an auto negotiation reset after
> > modifying the delays.
> >
> > Tim
>
> Setting BMCR_ANRESTART would work, but only if BMCR_ANENABLE is also or
> already set. Otherwise BMCR_ANRESTART has no effect (see the note in the
> datasheet).
>
> This is the reason why I came up with the idea of BMCR_PDOWN.
>
> Personally I would have no problem with setting BMCR_ANRESTART and
> BMCR_ANENABLE, but it would possibly change the existing configuration
> if (e.g. by the bootloader) aneg should be disabled.
>

Martin,

Sorry for the silence - I've been trying to figure out if and how I
can deal with some very nasty errata on this particular PHY that can
cause the link to not be stable and/or excessive errors in packets
sent to the MAC.

I do like the idea of BMCR_PDOWN. With my board its pretty obvious if
the pin-strapped rx/tx delays are being used rather than the ones
configured in the phy driver, so I'll have to do some testing when I
find some time. However, I don't at all like the fact that this
particular patch defaults the delays to 2ns if 'rx-internal-delay-ps'
and 'tx-internal-delay-ps' is missing from the dt.

These properties were added via Dan Murphy's series 'RGMII Internal
delay common property' which was merged into v5.9:
8095295292b5 ("net: phy: DP83822: Add setting the fixed internal delay")
736b25afe284 ("net: dp83869: Add RGMII internal delay configuration")
2fb305c37d5b ("dt-bindings: net: Add RGMII internal delay for DP83869")
92252eec913b ("net: phy: Add a helper to return the index for of the
internal delay")
9150069bf5fc dt-bindings: net: Add tx and rx internal delays

The issue I have here is that if dt's have not been updated to add the
common delay properties this code will override what the boot firmware
may have configured them to. I feel that if these common delay
properties are not found in the device tree, then no changes to the
delays should be made at all.

Best Regards,

Tim

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Andrew Lunn]

> However, I don't at all like the fact that this
> particular patch defaults the delays to 2ns if 'rx-internal-delay-ps'
> and 'tx-internal-delay-ps' is missing from the dt.

How does this work in combination with phy-mode 'rgmii', 'rgmii-id'
etc? Using 2ns as a default for rgmii-id is sensible, however i would
say it is wrong for rgmii.

> The issue I have here is that if dt's have not been updated to add the
> common delay properties this code will override what the boot firmware
> may have configured them to. I feel that if these common delay
> properties are not found in the device tree, then no changes to the
> delays should be made at all.

If you don't want the PHY driver to touch the delays at all because
you know something else has set it up, you can use phy-mode="", which
should be interpreted as PHY_INTERFACE_MODE_NA.

       Andrew

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Michael Walle]

Hi Tim, Hi Martin,

> I've got some boards with the GPY111 phy on them and I'm finding that
> modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I
> do a soft reset (BCMR_RESET) first. I don't see anything in the
> datasheet which specifies this to be the case so I'm interested it
> what you have found. Are you sure adjusting the skews like this
> without a soft (or hard pin based) reset actually works?

I do have the same PHY and I'm puzzled with the delay settings. Do
you have an EEPROM attached to the PHY? According to my datasheet,
that seems to make a difference. Apparently, only if there is an
EEPROM, you can change the value (the value is then also written to
the EEPROM according the datasheet).
If you don't have one, the values will get overwritten by the
external strappings on a soft reset. Therefore, it seems they cannot
be set. (FWIW there is also a sticky bit, but that doesn't seem to
help in this case).

-michael

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Martin Schiller]

On 2023-02-22 17:04, Michael Walle wrote:
> Hi Tim, Hi Martin,
> 
>> I've got some boards with the GPY111 phy on them and I'm finding that
>> modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I
>> do a soft reset (BCMR_RESET) first. I don't see anything in the
>> datasheet which specifies this to be the case so I'm interested it
>> what you have found. Are you sure adjusting the skews like this
>> without a soft (or hard pin based) reset actually works?
> 
> I do have the same PHY and I'm puzzled with the delay settings. Do
> you have an EEPROM attached to the PHY? According to my datasheet,
> that seems to make a difference. Apparently, only if there is an
> EEPROM, you can change the value (the value is then also written to
> the EEPROM according the datasheet).
> If you don't have one, the values will get overwritten by the
> external strappings on a soft reset. Therefore, it seems they cannot
> be set. (FWIW there is also a sticky bit, but that doesn't seem to
> help in this case).
> 
> -michael

Yes, you are right. The datasheet says: "In no-EEPROM mode, writing to
this register has no impact on operation of the device".

But changing this settings without an EEPROM indeed has an impact.

We don't use an EEPROM and without tuning this values some boards are
unable to communicate on the ethernet port(s).

I varied these values during operation in the uboot and was able to test
the limits very nicely.

I wouldn't have introduced this feature if it hasn't got any impact.

Regards,
Martin

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Michael Walle]

Hi Martin,

Am 2023-02-24 07:25, schrieb Martin Schiller:
> On 2023-02-22 17:04, Michael Walle wrote:
>> Hi Tim, Hi Martin,
>> 
>>> I've got some boards with the GPY111 phy on them and I'm finding that
>>> modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless I
>>> do a soft reset (BCMR_RESET) first. I don't see anything in the
>>> datasheet which specifies this to be the case so I'm interested it
>>> what you have found. Are you sure adjusting the skews like this
>>> without a soft (or hard pin based) reset actually works?
>> 
>> I do have the same PHY and I'm puzzled with the delay settings. Do
>> you have an EEPROM attached to the PHY? According to my datasheet,
>> that seems to make a difference. Apparently, only if there is an
>> EEPROM, you can change the value (the value is then also written to
>> the EEPROM according the datasheet).
>> If you don't have one, the values will get overwritten by the
>> external strappings on a soft reset. Therefore, it seems they cannot
>> be set. (FWIW there is also a sticky bit, but that doesn't seem to
>> help in this case).
>> 
>> -michael
> 
> Yes, you are right. The datasheet says: "In no-EEPROM mode, writing to
> this register has no impact on operation of the device".
> 
> But changing this settings without an EEPROM indeed has an impact.
> 
> We don't use an EEPROM and without tuning this values some boards are
> unable to communicate on the ethernet port(s).

Thanks for confirming! Could you share your PHYID1/PHYID2 register and
firmware version (FWV, 0x1E) contents?

In our case, any changes in MIICTRL are lost after a soft reset.

> I varied these values during operation in the uboot and was able to 
> test
> the limits very nicely.

So I guess, the value you write into MIICTRL are retained on a soft 
reset.
I.e.

mii write <phyad> 0x17 0xffff
mii write <phyad> 0x00 0x8000
mii read <phyad> 0x17

will still return 0xffff?

> 
> I wouldn't have introduced this feature if it hasn't got any impact.

Sure, I'm just trying to figure out the differences ;)

Thanks,
Michael

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Martin Schiller]

On 2023-02-24 09:04, Michael Walle wrote:
> Hi Martin,
> 
> Am 2023-02-24 07:25, schrieb Martin Schiller:
>> On 2023-02-22 17:04, Michael Walle wrote:
>>> Hi Tim, Hi Martin,
>>> 
>>>> I've got some boards with the GPY111 phy on them and I'm finding 
>>>> that
>>>> modifying XWAY_MDIO_MIICTRL to change the skew has no effect unless 
>>>> I
>>>> do a soft reset (BCMR_RESET) first. I don't see anything in the
>>>> datasheet which specifies this to be the case so I'm interested it
>>>> what you have found. Are you sure adjusting the skews like this
>>>> without a soft (or hard pin based) reset actually works?
>>> 
>>> I do have the same PHY and I'm puzzled with the delay settings. Do
>>> you have an EEPROM attached to the PHY? According to my datasheet,
>>> that seems to make a difference. Apparently, only if there is an
>>> EEPROM, you can change the value (the value is then also written to
>>> the EEPROM according the datasheet).
>>> If you don't have one, the values will get overwritten by the
>>> external strappings on a soft reset. Therefore, it seems they cannot
>>> be set. (FWIW there is also a sticky bit, but that doesn't seem to
>>> help in this case).
>>> 
>>> -michael
>> 
>> Yes, you are right. The datasheet says: "In no-EEPROM mode, writing to
>> this register has no impact on operation of the device".
>> 
>> But changing this settings without an EEPROM indeed has an impact.
>> 
>> We don't use an EEPROM and without tuning this values some boards are
>> unable to communicate on the ethernet port(s).
> 
> Thanks for confirming! Could you share your PHYID1/PHYID2 register and
> firmware version (FWV, 0x1E) contents?

I've 2 PHYs integrated into the VR268 SoC which shows this values:

STD_PHYID1(reg 0x02): 0xd565
STD_PHYID2(reg 0x03): 0xa409
PHY_FWV   (reg 0x1E): 0x8435

And then there are 2 external GPY111 with this values:

STD_PHYID1(reg 0x02): 0xd565
STD_PHYID2(reg 0x03): 0xa401
PHY_FWV   (reg 0x1E): 0x8435

And one external GPY112 with this values:

STD_PHYID1(reg 0x02): 0xd565
STD_PHYID2(reg 0x03): 0xa401
PHY_FWV   (reg 0x1E): 0x8435

> 
> In our case, any changes in MIICTRL are lost after a soft reset.
> 
>> I varied these values during operation in the uboot and was able to 
>> test
>> the limits very nicely.
> 
> So I guess, the value you write into MIICTRL are retained on a soft 
> reset.

No, the value I write into MIICTRL are not retained on a soft reset.

> I.e.
> 
> mii write <phyad> 0x17 0xffff
> mii write <phyad> 0x00 0x8000
> mii read <phyad> 0x17
> 
> will still return 0xffff?

In my tests I always set the skew values in register 0x17 first and
then triggered a restart of the ANEG via register 0x0. This then led to
the new values being adopted.

> 
>> 
>> I wouldn't have introduced this feature if it hasn't got any impact.
> 
> Sure, I'm just trying to figure out the differences ;)
> 
> Thanks,
> Michael

- Martin

Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration

[Michael Walle]

Hi Martin,

> In my tests I always set the skew values in register 0x17 first and
> then triggered a restart of the ANEG via register 0x0. This then led to
> the new values being adopted.

Right. I missed that. And I can confirm this is working perfectly 
although
mentioned otherwise in the datasheet. The answer FAE anser is still
outstanding.

Thank you!
-michael