Netdev Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
@ 2021-07-19  8:27 Martin Schiller
  2021-07-19 20:56 ` Andrew Lunn
  2022-01-10 23:12 ` Tim Harvey
  0 siblings, 2 replies; 15+ messages in thread
From: Martin Schiller @ 2021-07-19  8:27 UTC (permalink / raw)
  To: hauke, martin.blumenstingl, f.fainelli, andrew, hkallweit1,
	linux, davem, kuba
  Cc: netdev, linux-kernel, 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


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2021-07-19  8:27 [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration Martin Schiller
@ 2021-07-19 20:56 ` Andrew Lunn
  2021-07-20 11:50   ` Martin Schiller
  2022-01-10 23:12 ` Tim Harvey
  1 sibling, 1 reply; 15+ messages in thread
From: Andrew Lunn @ 2021-07-19 20:56 UTC (permalink / raw)
  To: Martin Schiller
  Cc: hauke, martin.blumenstingl, f.fainelli, hkallweit1, linux, davem,
	kuba, netdev, linux-kernel

> +	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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2021-07-19 20:56 ` Andrew Lunn
@ 2021-07-20 11:50   ` Martin Schiller
  0 siblings, 0 replies; 15+ messages in thread
From: Martin Schiller @ 2021-07-20 11:50 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: hauke, martin.blumenstingl, f.fainelli, hkallweit1, linux, davem,
	kuba, netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2021-07-19  8:27 [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration Martin Schiller
  2021-07-19 20:56 ` Andrew Lunn
@ 2022-01-10 23:12 ` Tim Harvey
  2022-01-11  7:44   ` Martin Schiller
  1 sibling, 1 reply; 15+ messages in thread
From: Tim Harvey @ 2022-01-10 23:12 UTC (permalink / raw)
  To: Martin Schiller
  Cc: Hauke Mehrtens, martin.blumenstingl, Florian Fainelli,
	Andrew Lunn, hkallweit1, Russell King - ARM Linux, David Miller,
	kuba, netdev, open list

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-10 23:12 ` Tim Harvey
@ 2022-01-11  7:44   ` Martin Schiller
  2022-01-11 13:34     ` Andrew Lunn
  2022-01-11 19:12     ` Tim Harvey
  0 siblings, 2 replies; 15+ messages in thread
From: Martin Schiller @ 2022-01-11  7:44 UTC (permalink / raw)
  To: Tim Harvey
  Cc: Hauke Mehrtens, martin.blumenstingl, Florian Fainelli,
	Andrew Lunn, hkallweit1, Russell King - ARM Linux, David Miller,
	kuba, netdev, open list

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-11  7:44   ` Martin Schiller
@ 2022-01-11 13:34     ` Andrew Lunn
  2022-01-11 19:12     ` Tim Harvey
  1 sibling, 0 replies; 15+ messages in thread
From: Andrew Lunn @ 2022-01-11 13:34 UTC (permalink / raw)
  To: Martin Schiller
  Cc: Tim Harvey, Hauke Mehrtens, martin.blumenstingl,
	Florian Fainelli, hkallweit1, Russell King - ARM Linux,
	David Miller, kuba, netdev, open list

> > 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-11  7:44   ` Martin Schiller
  2022-01-11 13:34     ` Andrew Lunn
@ 2022-01-11 19:12     ` Tim Harvey
  2022-01-12 11:07       ` Martin Schiller
  2022-01-12 13:46       ` Russell King (Oracle)
  1 sibling, 2 replies; 15+ messages in thread
From: Tim Harvey @ 2022-01-11 19:12 UTC (permalink / raw)
  To: Martin Schiller
  Cc: Hauke Mehrtens, martin.blumenstingl, Florian Fainelli,
	Andrew Lunn, hkallweit1, Russell King - ARM Linux, David Miller,
	kuba, netdev, open list

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-11 19:12     ` Tim Harvey
@ 2022-01-12 11:07       ` Martin Schiller
  2022-01-12 13:14         ` Andrew Lunn
  2022-01-12 13:46       ` Russell King (Oracle)
  1 sibling, 1 reply; 15+ messages in thread
From: Martin Schiller @ 2022-01-12 11:07 UTC (permalink / raw)
  To: Tim Harvey
  Cc: Hauke Mehrtens, martin.blumenstingl, Florian Fainelli,
	Andrew Lunn, hkallweit1, Russell King - ARM Linux, David Miller,
	kuba, netdev, open list

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-12 11:07       ` Martin Schiller
@ 2022-01-12 13:14         ` Andrew Lunn
  2022-01-12 18:24           ` Tim Harvey
  0 siblings, 1 reply; 15+ messages in thread
From: Andrew Lunn @ 2022-01-12 13:14 UTC (permalink / raw)
  To: Martin Schiller
  Cc: Tim Harvey, Hauke Mehrtens, martin.blumenstingl,
	Florian Fainelli, hkallweit1, Russell King - ARM Linux,
	David Miller, kuba, netdev, open list

> > 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-11 19:12     ` Tim Harvey
  2022-01-12 11:07       ` Martin Schiller
@ 2022-01-12 13:46       ` Russell King (Oracle)
  2022-01-12 18:25         ` Tim Harvey
  1 sibling, 1 reply; 15+ messages in thread
From: Russell King (Oracle) @ 2022-01-12 13:46 UTC (permalink / raw)
  To: Tim Harvey
  Cc: Martin Schiller, Hauke Mehrtens, martin.blumenstingl,
	Florian Fainelli, Andrew Lunn, hkallweit1, David Miller, kuba,
	netdev, open list

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!

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-12 13:14         ` Andrew Lunn
@ 2022-01-12 18:24           ` Tim Harvey
  0 siblings, 0 replies; 15+ messages in thread
From: Tim Harvey @ 2022-01-12 18:24 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Martin Schiller, Hauke Mehrtens, martin.blumenstingl,
	Florian Fainelli, hkallweit1, Russell King - ARM Linux,
	David Miller, kuba, netdev, open list

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-12 13:46       ` Russell King (Oracle)
@ 2022-01-12 18:25         ` Tim Harvey
  2022-01-13  6:32           ` Martin Schiller
  0 siblings, 1 reply; 15+ messages in thread
From: Tim Harvey @ 2022-01-12 18:25 UTC (permalink / raw)
  To: Russell King (Oracle)
  Cc: Martin Schiller, Hauke Mehrtens, martin.blumenstingl,
	Florian Fainelli, Andrew Lunn, hkallweit1, David Miller, kuba,
	netdev, open list

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-12 18:25         ` Tim Harvey
@ 2022-01-13  6:32           ` Martin Schiller
  2022-02-01 20:28             ` Tim Harvey
  0 siblings, 1 reply; 15+ messages in thread
From: Martin Schiller @ 2022-01-13  6:32 UTC (permalink / raw)
  To: Tim Harvey
  Cc: Russell King (Oracle),
	Hauke Mehrtens, martin.blumenstingl, Florian Fainelli,
	Andrew Lunn, hkallweit1, David Miller, kuba, netdev, open list

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-01-13  6:32           ` Martin Schiller
@ 2022-02-01 20:28             ` Tim Harvey
  2022-02-01 20:49               ` Andrew Lunn
  0 siblings, 1 reply; 15+ messages in thread
From: Tim Harvey @ 2022-02-01 20:28 UTC (permalink / raw)
  To: Martin Schiller
  Cc: Russell King (Oracle),
	Hauke Mehrtens, martin.blumenstingl, Florian Fainelli,
	Andrew Lunn, hkallweit1, David Miller, kuba, netdev, open list,
	Dan Murphy

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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration
  2022-02-01 20:28             ` Tim Harvey
@ 2022-02-01 20:49               ` Andrew Lunn
  0 siblings, 0 replies; 15+ messages in thread
From: Andrew Lunn @ 2022-02-01 20:49 UTC (permalink / raw)
  To: Tim Harvey
  Cc: Martin Schiller, Russell King (Oracle),
	Hauke Mehrtens, martin.blumenstingl, Florian Fainelli,
	hkallweit1, David Miller, kuba, netdev, open list, Dan Murphy

> 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-02-01 20:49 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-19  8:27 [PATCH net-next v6] net: phy: intel-xway: Add RGMII internal delay configuration Martin Schiller
2021-07-19 20:56 ` Andrew Lunn
2021-07-20 11:50   ` Martin Schiller
2022-01-10 23:12 ` Tim Harvey
2022-01-11  7:44   ` Martin Schiller
2022-01-11 13:34     ` Andrew Lunn
2022-01-11 19:12     ` Tim Harvey
2022-01-12 11:07       ` Martin Schiller
2022-01-12 13:14         ` Andrew Lunn
2022-01-12 18:24           ` Tim Harvey
2022-01-12 13:46       ` Russell King (Oracle)
2022-01-12 18:25         ` Tim Harvey
2022-01-13  6:32           ` Martin Schiller
2022-02-01 20:28             ` Tim Harvey
2022-02-01 20:49               ` Andrew Lunn

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).