[U-Boot,5/8] net: phy: dp83867: rework delay rgmii delay handling
diff mbox series

Message ID 20191118210447.7433-6-grygorii.strashko@ti.com
State Accepted
Commit 20f7ea4c35
Delegated to: Joe Hershberger
Headers show
Series
  • net: phy: dp83867: ti: sync with linux kernel and
Related show

Commit Message

Grygorii Strashko Nov. 18, 2019, 9:04 p.m. UTC
Based on commit c11669a2757e ("net: phy: dp83867: Rework delay rgmii delay
handling") of mainline linux kernel.

The current code is assuming the reset default of the delay control
register was to have delay disabled.  This is what the datasheet shows as
the register's initial value.  However, that's not actually true: the
default is controlled by the PHY's pin strapping.

This patch:
- insures the other direction's delay is disabled If the interface mode is
selected as RX or TX delay only
- validates the delay values and fail if they are not in range
- checks if the board is strapped to have a delay and is configured to use
"rgmii" mode and warning is generated that "rgmii-id" should have been
used.

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
 drivers/net/phy/dp83867.c | 76 ++++++++++++++++++++++++++++++++-------
 1 file changed, 64 insertions(+), 12 deletions(-)

Comments

Joe Hershberger Nov. 19, 2019, 9:54 p.m. UTC | #1
On Mon, Nov 18, 2019 at 3:26 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
>
> Based on commit c11669a2757e ("net: phy: dp83867: Rework delay rgmii delay
> handling") of mainline linux kernel.
>
> The current code is assuming the reset default of the delay control
> register was to have delay disabled.  This is what the datasheet shows as
> the register's initial value.  However, that's not actually true: the
> default is controlled by the PHY's pin strapping.
>
> This patch:
> - insures the other direction's delay is disabled If the interface mode is
> selected as RX or TX delay only
> - validates the delay values and fail if they are not in range
> - checks if the board is strapped to have a delay and is configured to use
> "rgmii" mode and warning is generated that "rgmii-id" should have been
> used.
>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Nit below...

Acked-by: Joe Hershberger <joe.hershberger@ni.com>

> ---
>  drivers/net/phy/dp83867.c | 76 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 64 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
> index cd3c1c596a..1721f6892b 100644
> --- a/drivers/net/phy/dp83867.c
> +++ b/drivers/net/phy/dp83867.c
> @@ -25,6 +25,7 @@
>  #define DP83867_CFG4           0x0031
>  #define DP83867_RGMIICTL       0x0032
>  #define DP83867_STRAP_STS1     0x006E
> +#define DP83867_STRAP_STS2     0x006f
>  #define DP83867_RGMIIDCTL      0x0086
>  #define DP83867_IO_MUX_CFG     0x0170
>
> @@ -52,6 +53,13 @@
>  /* STRAP_STS1 bits */
>  #define DP83867_STRAP_STS1_RESERVED            BIT(11)
>
> +/* STRAP_STS2 bits */
> +#define DP83867_STRAP_STS2_CLK_SKEW_TX_MASK    GENMASK(6, 4)
> +#define DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT   4
> +#define DP83867_STRAP_STS2_CLK_SKEW_RX_MASK    GENMASK(2, 0)
> +#define DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT   0
> +#define DP83867_STRAP_STS2_CLK_SKEW_NONE       BIT(2)
> +
>  /* PHY CTRL bits */
>  #define DP83867_PHYCR_FIFO_DEPTH_SHIFT         14
>  #define DP83867_PHYCR_RESERVED_MASK    BIT(11)
> @@ -63,7 +71,9 @@
>  #define DP83867_PHYCTRL_TXFIFO_SHIFT   14
>
>  /* RGMIIDCTL bits */
> +#define DP83867_RGMII_TX_CLK_DELAY_MAX         0xf
>  #define DP83867_RGMII_TX_CLK_DELAY_SHIFT       4
> +#define DP83867_RGMII_RX_CLK_DELAY_MAX         0xf
>
>  /* CFG2 bits */
>  #define MII_DP83867_CFG2_SPEEDOPT_10EN         0x0040
> @@ -74,8 +84,6 @@
>  #define MII_DP83867_CFG2_MASK                  0x003F
>
>  /* User setting - can be taken from DTS */
> -#define DEFAULT_RX_ID_DELAY    DP83867_RGMIIDCTL_2_25_NS
> -#define DEFAULT_TX_ID_DELAY    DP83867_RGMIIDCTL_2_75_NS
>  #define DEFAULT_FIFO_DEPTH     DP83867_PHYCR_FIFO_DEPTH_4_B_NIB
>
>  /* IO_MUX_CFG bits */
> @@ -98,8 +106,8 @@ enum {
>  };
>
>  struct dp83867_private {
> -       int rx_id_delay;
> -       int tx_id_delay;
> +       u32 rx_id_delay;
> +       u32 tx_id_delay;
>         int fifo_depth;
>         int io_impedance;
>         bool rxctrl_strap_quirk;
> @@ -168,13 +176,55 @@ static int dp83867_of_init(struct phy_device *phydev)
>
>         if (ofnode_read_bool(node, "ti,dp83867-rxctrl-strap-quirk"))
>                 dp83867->rxctrl_strap_quirk = true;
> -       dp83867->rx_id_delay = ofnode_read_u32_default(node,
> -                                                      "ti,rx-internal-delay",
> -                                                      DEFAULT_RX_ID_DELAY);
>
> -       dp83867->tx_id_delay = ofnode_read_u32_default(node,
> -                                                      "ti,tx-internal-delay",
> -                                                      DEFAULT_TX_ID_DELAY);
> +       /* Existing behavior was to use default pin strapping delay in rgmii

Is this copied out of Linux verbatim? If not, please change the
comment block format.

> +        * mode, but rgmii should have meant no delay.  Warn existing users.
> +        */
> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
> +               u16 val = phy_read_mmd(phydev, DP83867_DEVADDR,
> +                                      DP83867_STRAP_STS2);
> +               u16 txskew = (val & DP83867_STRAP_STS2_CLK_SKEW_TX_MASK) >>
> +                            DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT;
> +               u16 rxskew = (val & DP83867_STRAP_STS2_CLK_SKEW_RX_MASK) >>
> +                            DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT;
> +
> +               if (txskew != DP83867_STRAP_STS2_CLK_SKEW_NONE ||
> +                   rxskew != DP83867_STRAP_STS2_CLK_SKEW_NONE)
> +                       pr_warn("PHY has delays via pin strapping, but phy-mode = 'rgmii'\n"
> +                               "Should be 'rgmii-id' to use internal delays\n");
> +       }
> +
> +       /* RX delay *must* be specified if internal delay of RX is used. */
> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
> +               ret = ofnode_read_u32(node, "ti,rx-internal-delay",
> +                                     &dp83867->rx_id_delay);
> +               if (ret) {
> +                       pr_debug("ti,rx-internal-delay must be specified\n");
> +                       return ret;
> +               }
> +               if (dp83867->rx_id_delay > DP83867_RGMII_RX_CLK_DELAY_MAX) {
> +                       pr_debug("ti,rx-internal-delay value of %u out of range\n",
> +                                dp83867->rx_id_delay);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       /* TX delay *must* be specified if internal delay of RX is used. */
> +       if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> +           phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
> +               ret = ofnode_read_u32(node, "ti,tx-internal-delay",
> +                                     &dp83867->tx_id_delay);
> +               if (ret) {
> +                       debug("ti,tx-internal-delay must be specified\n");
> +                       return ret;
> +               }
> +               if (dp83867->tx_id_delay > DP83867_RGMII_TX_CLK_DELAY_MAX) {
> +                       pr_debug("ti,tx-internal-delay value of %u out of range\n",
> +                                dp83867->tx_id_delay);
> +                       return -EINVAL;
> +               }
> +       }
>
>         dp83867->fifo_depth = ofnode_read_u32_default(node, "ti,fifo-depth",
>                                                       DEFAULT_FIFO_DEPTH);
> @@ -191,8 +241,8 @@ static int dp83867_of_init(struct phy_device *phydev)
>  {
>         struct dp83867_private *dp83867 = phydev->priv;
>
> -       dp83867->rx_id_delay = DEFAULT_RX_ID_DELAY;
> -       dp83867->tx_id_delay = DEFAULT_TX_ID_DELAY;
> +       dp83867->rx_id_delay = DP83867_RGMIIDCTL_2_25_NS;
> +       dp83867->tx_id_delay = DP83867_RGMIIDCTL_2_75_NS;
>         dp83867->fifo_depth = DEFAULT_FIFO_DEPTH;
>         dp83867->io_impedance = -EINVAL;
>
> @@ -281,6 +331,8 @@ static int dp83867_config(struct phy_device *phydev)
>                 val = phy_read_mmd(phydev, DP83867_DEVADDR,
>                                    DP83867_RGMIICTL);
>
> +               val &= ~(DP83867_RGMII_TX_CLK_DELAY_EN |
> +                        DP83867_RGMII_RX_CLK_DELAY_EN);
>                 if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
>                         val |= (DP83867_RGMII_TX_CLK_DELAY_EN |
>                                 DP83867_RGMII_RX_CLK_DELAY_EN);
> --
> 2.17.1
>
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Patch
diff mbox series

diff --git a/drivers/net/phy/dp83867.c b/drivers/net/phy/dp83867.c
index cd3c1c596a..1721f6892b 100644
--- a/drivers/net/phy/dp83867.c
+++ b/drivers/net/phy/dp83867.c
@@ -25,6 +25,7 @@ 
 #define DP83867_CFG4		0x0031
 #define DP83867_RGMIICTL	0x0032
 #define DP83867_STRAP_STS1	0x006E
+#define DP83867_STRAP_STS2	0x006f
 #define DP83867_RGMIIDCTL	0x0086
 #define DP83867_IO_MUX_CFG	0x0170
 
@@ -52,6 +53,13 @@ 
 /* STRAP_STS1 bits */
 #define DP83867_STRAP_STS1_RESERVED		BIT(11)
 
+/* STRAP_STS2 bits */
+#define DP83867_STRAP_STS2_CLK_SKEW_TX_MASK	GENMASK(6, 4)
+#define DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT	4
+#define DP83867_STRAP_STS2_CLK_SKEW_RX_MASK	GENMASK(2, 0)
+#define DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT	0
+#define DP83867_STRAP_STS2_CLK_SKEW_NONE	BIT(2)
+
 /* PHY CTRL bits */
 #define DP83867_PHYCR_FIFO_DEPTH_SHIFT		14
 #define DP83867_PHYCR_RESERVED_MASK	BIT(11)
@@ -63,7 +71,9 @@ 
 #define DP83867_PHYCTRL_TXFIFO_SHIFT	14
 
 /* RGMIIDCTL bits */
+#define DP83867_RGMII_TX_CLK_DELAY_MAX		0xf
 #define DP83867_RGMII_TX_CLK_DELAY_SHIFT	4
+#define DP83867_RGMII_RX_CLK_DELAY_MAX		0xf
 
 /* CFG2 bits */
 #define MII_DP83867_CFG2_SPEEDOPT_10EN		0x0040
@@ -74,8 +84,6 @@ 
 #define MII_DP83867_CFG2_MASK			0x003F
 
 /* User setting - can be taken from DTS */
-#define DEFAULT_RX_ID_DELAY	DP83867_RGMIIDCTL_2_25_NS
-#define DEFAULT_TX_ID_DELAY	DP83867_RGMIIDCTL_2_75_NS
 #define DEFAULT_FIFO_DEPTH	DP83867_PHYCR_FIFO_DEPTH_4_B_NIB
 
 /* IO_MUX_CFG bits */
@@ -98,8 +106,8 @@  enum {
 };
 
 struct dp83867_private {
-	int rx_id_delay;
-	int tx_id_delay;
+	u32 rx_id_delay;
+	u32 tx_id_delay;
 	int fifo_depth;
 	int io_impedance;
 	bool rxctrl_strap_quirk;
@@ -168,13 +176,55 @@  static int dp83867_of_init(struct phy_device *phydev)
 
 	if (ofnode_read_bool(node, "ti,dp83867-rxctrl-strap-quirk"))
 		dp83867->rxctrl_strap_quirk = true;
-	dp83867->rx_id_delay = ofnode_read_u32_default(node,
-						       "ti,rx-internal-delay",
-						       DEFAULT_RX_ID_DELAY);
 
-	dp83867->tx_id_delay = ofnode_read_u32_default(node,
-						       "ti,tx-internal-delay",
-						       DEFAULT_TX_ID_DELAY);
+	/* Existing behavior was to use default pin strapping delay in rgmii
+	 * mode, but rgmii should have meant no delay.  Warn existing users.
+	 */
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII) {
+		u16 val = phy_read_mmd(phydev, DP83867_DEVADDR,
+				       DP83867_STRAP_STS2);
+		u16 txskew = (val & DP83867_STRAP_STS2_CLK_SKEW_TX_MASK) >>
+			     DP83867_STRAP_STS2_CLK_SKEW_TX_SHIFT;
+		u16 rxskew = (val & DP83867_STRAP_STS2_CLK_SKEW_RX_MASK) >>
+			     DP83867_STRAP_STS2_CLK_SKEW_RX_SHIFT;
+
+		if (txskew != DP83867_STRAP_STS2_CLK_SKEW_NONE ||
+		    rxskew != DP83867_STRAP_STS2_CLK_SKEW_NONE)
+			pr_warn("PHY has delays via pin strapping, but phy-mode = 'rgmii'\n"
+				"Should be 'rgmii-id' to use internal delays\n");
+	}
+
+	/* RX delay *must* be specified if internal delay of RX is used. */
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_RXID) {
+		ret = ofnode_read_u32(node, "ti,rx-internal-delay",
+				      &dp83867->rx_id_delay);
+		if (ret) {
+			pr_debug("ti,rx-internal-delay must be specified\n");
+			return ret;
+		}
+		if (dp83867->rx_id_delay > DP83867_RGMII_RX_CLK_DELAY_MAX) {
+			pr_debug("ti,rx-internal-delay value of %u out of range\n",
+				 dp83867->rx_id_delay);
+			return -EINVAL;
+		}
+	}
+
+	/* TX delay *must* be specified if internal delay of RX is used. */
+	if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID ||
+	    phydev->interface == PHY_INTERFACE_MODE_RGMII_TXID) {
+		ret = ofnode_read_u32(node, "ti,tx-internal-delay",
+				      &dp83867->tx_id_delay);
+		if (ret) {
+			debug("ti,tx-internal-delay must be specified\n");
+			return ret;
+		}
+		if (dp83867->tx_id_delay > DP83867_RGMII_TX_CLK_DELAY_MAX) {
+			pr_debug("ti,tx-internal-delay value of %u out of range\n",
+				 dp83867->tx_id_delay);
+			return -EINVAL;
+		}
+	}
 
 	dp83867->fifo_depth = ofnode_read_u32_default(node, "ti,fifo-depth",
 						      DEFAULT_FIFO_DEPTH);
@@ -191,8 +241,8 @@  static int dp83867_of_init(struct phy_device *phydev)
 {
 	struct dp83867_private *dp83867 = phydev->priv;
 
-	dp83867->rx_id_delay = DEFAULT_RX_ID_DELAY;
-	dp83867->tx_id_delay = DEFAULT_TX_ID_DELAY;
+	dp83867->rx_id_delay = DP83867_RGMIIDCTL_2_25_NS;
+	dp83867->tx_id_delay = DP83867_RGMIIDCTL_2_75_NS;
 	dp83867->fifo_depth = DEFAULT_FIFO_DEPTH;
 	dp83867->io_impedance = -EINVAL;
 
@@ -281,6 +331,8 @@  static int dp83867_config(struct phy_device *phydev)
 		val = phy_read_mmd(phydev, DP83867_DEVADDR,
 				   DP83867_RGMIICTL);
 
+		val &= ~(DP83867_RGMII_TX_CLK_DELAY_EN |
+			 DP83867_RGMII_RX_CLK_DELAY_EN);
 		if (phydev->interface == PHY_INTERFACE_MODE_RGMII_ID)
 			val |= (DP83867_RGMII_TX_CLK_DELAY_EN |
 				DP83867_RGMII_RX_CLK_DELAY_EN);