diff mbox series

[1/3] net: ethernet: stmmac: dwmac-rk: Disable delayline if it is invalid

Message ID 20220623162850.245608-2-sebastian.reichel@collabora.com (mailing list archive)
State New
Headers show
Series RK3588 Ethernet Support | expand

Commit Message

Sebastian Reichel June 23, 2022, 4:28 p.m. UTC
From: David Wu <david.wu@rock-chips.com>

Explicitly disable delayline if it is no value is configured in DT.

Signed-off-by: David Wu <david.wu@rock-chips.com>
[rebase]
Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 52 +++++++++----------
 1 file changed, 25 insertions(+), 27 deletions(-)

Comments

Andrew Lunn June 24, 2022, 11:18 a.m. UTC | #1
> @@ -1422,7 +1420,7 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
>  
>  	ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
>  	if (ret) {
> -		bsp_priv->tx_delay = 0x30;
> +		bsp_priv->tx_delay = -1;
>  		dev_err(dev, "Can not read property: tx_delay.");
>  		dev_err(dev, "set tx_delay to 0x%x\n",
>  			bsp_priv->tx_delay);
> @@ -1433,7 +1431,7 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
>  
>  	ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
>  	if (ret) {
> -		bsp_priv->rx_delay = 0x10;
> +		bsp_priv->rx_delay = -1;
>  		dev_err(dev, "Can not read property: rx_delay.");
>  		dev_err(dev, "set rx_delay to 0x%x\n",
>  			bsp_priv->rx_delay);

rockchip-dwmac.yaml says:


  tx_delay:
    description: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as default.
    $ref: /schemas/types.yaml#/definitions/uint32

  rx_delay:
    description: Delay value for RXD timing. Range value is 0~0x7F, 0x10 as default.
    $ref: /schemas/types.yaml#/definitions/uint32

So it seems to me you are changing the documented default. You cannot
do that, this is ABI.

   Andrew
Sebastian Reichel June 24, 2022, 4:29 p.m. UTC | #2
Hi,

On Fri, Jun 24, 2022 at 01:18:53PM +0200, Andrew Lunn wrote:
> > @@ -1422,7 +1420,7 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
> >  
> >  	ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
> >  	if (ret) {
> > -		bsp_priv->tx_delay = 0x30;
> > +		bsp_priv->tx_delay = -1;
> >  		dev_err(dev, "Can not read property: tx_delay.");
> >  		dev_err(dev, "set tx_delay to 0x%x\n",
> >  			bsp_priv->tx_delay);
> > @@ -1433,7 +1431,7 @@ static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
> >  
> >  	ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
> >  	if (ret) {
> > -		bsp_priv->rx_delay = 0x10;
> > +		bsp_priv->rx_delay = -1;
> >  		dev_err(dev, "Can not read property: rx_delay.");
> >  		dev_err(dev, "set rx_delay to 0x%x\n",
> >  			bsp_priv->rx_delay);
> 
> rockchip-dwmac.yaml says:
> 
>   tx_delay:
>     description: Delay value for TXD timing. Range value is 0~0x7F, 0x30 as default.
>     $ref: /schemas/types.yaml#/definitions/uint32
> 
>   rx_delay:
>     description: Delay value for RXD timing. Range value is 0~0x7F, 0x10 as default.
>     $ref: /schemas/types.yaml#/definitions/uint32
> 
> So it seems to me you are changing the documented default. You cannot
> do that, this is ABI.

Right. I suppose we either need a disable value or an extra property. I
can add support for supplying (-1) from DT. Does that sounds ok to
everyone?

-- Sebastian
Andrew Lunn June 24, 2022, 4:52 p.m. UTC | #3
> > So it seems to me you are changing the documented default. You cannot
> > do that, this is ABI.
> 
> Right. I suppose we either need a disable value or an extra property. I
> can add support for supplying (-1) from DT. Does that sounds ok to
> everyone?

I'm missing the big picture.

Does the hardware you are adding not support delays? If so, rather
than using the defaults, don't do anything. And if a value is
supplied, -EINVAL?

	  Andrew
Sebastian Reichel June 24, 2022, 5:35 p.m. UTC | #4
Hi,

On Fri, Jun 24, 2022 at 06:52:26PM +0200, Andrew Lunn wrote:
> > > So it seems to me you are changing the documented default. You cannot
> > > do that, this is ABI.
> > 
> > Right. I suppose we either need a disable value or an extra property. I
> > can add support for supplying (-1) from DT. Does that sounds ok to
> > everyone?
> 
> I'm missing the big picture.
> 
> Does the hardware you are adding not support delays? If so, rather
> than using the defaults, don't do anything. And if a value is
> supplied, -EINVAL?

The Rockchip hardware supports enabling/disabling delays and
configuring a delay value of 0x00-0x7f. For the RK3588 evaluation
board the RX delay should be disabled.

-- Sebastian
Andrew Lunn June 24, 2022, 5:37 p.m. UTC | #5
> The Rockchip hardware supports enabling/disabling delays and
> configuring a delay value of 0x00-0x7f. For the RK3588 evaluation
> board the RX delay should be disabled.

So you can just put 0 in DT then.

   Andrew
Sebastian Reichel June 24, 2022, 8:15 p.m. UTC | #6
Hi,

On Fri, Jun 24, 2022 at 07:37:08PM +0200, Andrew Lunn wrote:
> > The Rockchip hardware supports enabling/disabling delays and
> > configuring a delay value of 0x00-0x7f. For the RK3588 evaluation
> > board the RX delay should be disabled.
> 
> So you can just put 0 in DT then.

My understanding is, that there is a difference between
enabled delay with a delay value of 0 and delay fully
disabled. But I could not find any details aboout this
in the datasheet unfortunately.

-- Sebastian
Peter Geis June 24, 2022, 11:43 p.m. UTC | #7
On Fri, Jun 24, 2022 at 4:16 PM Sebastian Reichel
<sebastian.reichel@collabora.com> wrote:
>
> Hi,
>
> On Fri, Jun 24, 2022 at 07:37:08PM +0200, Andrew Lunn wrote:
> > > The Rockchip hardware supports enabling/disabling delays and
> > > configuring a delay value of 0x00-0x7f. For the RK3588 evaluation
> > > board the RX delay should be disabled.
> >
> > So you can just put 0 in DT then.
>
> My understanding is, that there is a difference between
> enabled delay with a delay value of 0 and delay fully
> disabled. But I could not find any details aboout this
> in the datasheet unfortunately.

The driver already sets the delays to 0 in case of the rgmii-id modes.
0 is disabled, even in this patch. The only thing this patch does is
change the behavior when the delays are not set. If the rx delays
should be 0, they should be defined as 0 in the device tree. There is
rgmii-rxid for a reason as well, but if they are setting the rx delay
to 0 with rgmii that implies this hardware is fundamentally broken.

Very Respectfully,
Peter Geis

>
> -- Sebastian
> _______________________________________________
> Linux-rockchip mailing list
> Linux-rockchip@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Andrew Lunn June 25, 2022, 6:50 a.m. UTC | #8
> The driver already sets the delays to 0 in case of the rgmii-id modes.
> 0 is disabled, even in this patch. The only thing this patch does is
> change the behavior when the delays are not set. If the rx delays
> should be 0, they should be defined as 0 in the device tree. There is
> rgmii-rxid for a reason as well, but if they are setting the rx delay
> to 0 with rgmii that implies this hardware is fundamentally broken.

Or the delay is implemented by longer tracks on the PCB. It happens
sometimes, but not very often.

	   Andrew
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index c469abc91fa1..56ccd4fbd6c0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -75,8 +75,16 @@  struct rk_priv_data {
 #define GRF_CLR_BIT(nr)	(BIT(nr+16))
 
 #define DELAY_ENABLE(soc, tx, rx) \
-	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
-	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
+	((((tx) >= 0) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
+	 (((rx) >= 0) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
+
+#define DELAY_ENABLE_BY_ID(soc, tx, rx, id) \
+	((((tx) >= 0) ? soc##_GMAC_TXCLK_DLY_ENABLE(id) : soc##_GMAC_TXCLK_DLY_DISABLE(id)) | \
+	 (((rx) >= 0) ? soc##_GMAC_RXCLK_DLY_ENABLE(id) : soc##_GMAC_RXCLK_DLY_DISABLE(id)))
+
+#define DELAY_VALUE(soc, tx, rx) \
+	((((tx) >= 0) ? soc##_GMAC_CLK_TX_DL_CFG(tx) : 0) | \
+	 (((rx) >= 0) ? soc##_GMAC_CLK_RX_DL_CFG(rx) : 0))
 
 #define PX30_GRF_GMAC_CON1		0x0904
 
@@ -179,8 +187,7 @@  static void rk3128_set_to_rgmii(struct rk_priv_data *bsp_priv,
 		     RK3128_GMAC_RMII_MODE_CLR);
 	regmap_write(bsp_priv->grf, RK3128_GRF_MAC_CON0,
 		     DELAY_ENABLE(RK3128, tx_delay, rx_delay) |
-		     RK3128_GMAC_CLK_RX_DL_CFG(rx_delay) |
-		     RK3128_GMAC_CLK_TX_DL_CFG(tx_delay));
+		     DELAY_VALUE(RK3128, tx_delay, rx_delay));
 }
 
 static void rk3128_set_to_rmii(struct rk_priv_data *bsp_priv)
@@ -296,8 +303,7 @@  static void rk3228_set_to_rgmii(struct rk_priv_data *bsp_priv,
 		     DELAY_ENABLE(RK3228, tx_delay, rx_delay));
 
 	regmap_write(bsp_priv->grf, RK3228_GRF_MAC_CON0,
-		     RK3228_GMAC_CLK_RX_DL_CFG(rx_delay) |
-		     RK3228_GMAC_CLK_TX_DL_CFG(tx_delay));
+		     DELAY_VALUE(RK3128, tx_delay, rx_delay));
 }
 
 static void rk3228_set_to_rmii(struct rk_priv_data *bsp_priv)
@@ -417,8 +423,7 @@  static void rk3288_set_to_rgmii(struct rk_priv_data *bsp_priv,
 		     RK3288_GMAC_RMII_MODE_CLR);
 	regmap_write(bsp_priv->grf, RK3288_GRF_SOC_CON3,
 		     DELAY_ENABLE(RK3288, tx_delay, rx_delay) |
-		     RK3288_GMAC_CLK_RX_DL_CFG(rx_delay) |
-		     RK3288_GMAC_CLK_TX_DL_CFG(tx_delay));
+		     DELAY_VALUE(RK3288, tx_delay, rx_delay));
 }
 
 static void rk3288_set_to_rmii(struct rk_priv_data *bsp_priv)
@@ -579,12 +584,10 @@  static void rk3328_set_to_rgmii(struct rk_priv_data *bsp_priv,
 	regmap_write(bsp_priv->grf, RK3328_GRF_MAC_CON1,
 		     RK3328_GMAC_PHY_INTF_SEL_RGMII |
 		     RK3328_GMAC_RMII_MODE_CLR |
-		     RK3328_GMAC_RXCLK_DLY_ENABLE |
-		     RK3328_GMAC_TXCLK_DLY_ENABLE);
+		     DELAY_ENABLE(RK3328, tx_delay, rx_delay));
 
 	regmap_write(bsp_priv->grf, RK3328_GRF_MAC_CON0,
-		     RK3328_GMAC_CLK_RX_DL_CFG(rx_delay) |
-		     RK3328_GMAC_CLK_TX_DL_CFG(tx_delay));
+		     DELAY_VALUE(RK3328, tx_delay, rx_delay));
 }
 
 static void rk3328_set_to_rmii(struct rk_priv_data *bsp_priv)
@@ -709,8 +712,7 @@  static void rk3366_set_to_rgmii(struct rk_priv_data *bsp_priv,
 		     RK3366_GMAC_RMII_MODE_CLR);
 	regmap_write(bsp_priv->grf, RK3366_GRF_SOC_CON7,
 		     DELAY_ENABLE(RK3366, tx_delay, rx_delay) |
-		     RK3366_GMAC_CLK_RX_DL_CFG(rx_delay) |
-		     RK3366_GMAC_CLK_TX_DL_CFG(tx_delay));
+		     DELAY_VALUE(RK3366, tx_delay, rx_delay));
 }
 
 static void rk3366_set_to_rmii(struct rk_priv_data *bsp_priv)
@@ -820,8 +822,7 @@  static void rk3368_set_to_rgmii(struct rk_priv_data *bsp_priv,
 		     RK3368_GMAC_RMII_MODE_CLR);
 	regmap_write(bsp_priv->grf, RK3368_GRF_SOC_CON16,
 		     DELAY_ENABLE(RK3368, tx_delay, rx_delay) |
-		     RK3368_GMAC_CLK_RX_DL_CFG(rx_delay) |
-		     RK3368_GMAC_CLK_TX_DL_CFG(tx_delay));
+		     DELAY_VALUE(RK3368, tx_delay, rx_delay));
 }
 
 static void rk3368_set_to_rmii(struct rk_priv_data *bsp_priv)
@@ -931,8 +932,7 @@  static void rk3399_set_to_rgmii(struct rk_priv_data *bsp_priv,
 		     RK3399_GMAC_RMII_MODE_CLR);
 	regmap_write(bsp_priv->grf, RK3399_GRF_SOC_CON6,
 		     DELAY_ENABLE(RK3399, tx_delay, rx_delay) |
-		     RK3399_GMAC_CLK_RX_DL_CFG(rx_delay) |
-		     RK3399_GMAC_CLK_TX_DL_CFG(tx_delay));
+		     DELAY_VALUE(RK3399, tx_delay, rx_delay));
 }
 
 static void rk3399_set_to_rmii(struct rk_priv_data *bsp_priv)
@@ -1037,13 +1037,11 @@  static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
 				     RK3568_GRF_GMAC0_CON1;
 
 	regmap_write(bsp_priv->grf, con0,
-		     RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
-		     RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
+		     DELAY_VALUE(RK3568, tx_delay, rx_delay));
 
 	regmap_write(bsp_priv->grf, con1,
 		     RK3568_GMAC_PHY_INTF_SEL_RGMII |
-		     RK3568_GMAC_RXCLK_DLY_ENABLE |
-		     RK3568_GMAC_TXCLK_DLY_ENABLE);
+		     DELAY_ENABLE(RK3568, tx_delay, rx_delay));
 }
 
 static void rk3568_set_to_rmii(struct rk_priv_data *bsp_priv)
@@ -1422,7 +1420,7 @@  static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
 
 	ret = of_property_read_u32(dev->of_node, "tx_delay", &value);
 	if (ret) {
-		bsp_priv->tx_delay = 0x30;
+		bsp_priv->tx_delay = -1;
 		dev_err(dev, "Can not read property: tx_delay.");
 		dev_err(dev, "set tx_delay to 0x%x\n",
 			bsp_priv->tx_delay);
@@ -1433,7 +1431,7 @@  static struct rk_priv_data *rk_gmac_setup(struct platform_device *pdev,
 
 	ret = of_property_read_u32(dev->of_node, "rx_delay", &value);
 	if (ret) {
-		bsp_priv->rx_delay = 0x10;
+		bsp_priv->rx_delay = -1;
 		dev_err(dev, "Can not read property: rx_delay.");
 		dev_err(dev, "set rx_delay to 0x%x\n",
 			bsp_priv->rx_delay);
@@ -1507,15 +1505,15 @@  static int rk_gmac_powerup(struct rk_priv_data *bsp_priv)
 		break;
 	case PHY_INTERFACE_MODE_RGMII_ID:
 		dev_info(dev, "init for RGMII_ID\n");
-		bsp_priv->ops->set_to_rgmii(bsp_priv, 0, 0);
+		bsp_priv->ops->set_to_rgmii(bsp_priv, -1, -1);
 		break;
 	case PHY_INTERFACE_MODE_RGMII_RXID:
 		dev_info(dev, "init for RGMII_RXID\n");
-		bsp_priv->ops->set_to_rgmii(bsp_priv, bsp_priv->tx_delay, 0);
+		bsp_priv->ops->set_to_rgmii(bsp_priv, bsp_priv->tx_delay, -1);
 		break;
 	case PHY_INTERFACE_MODE_RGMII_TXID:
 		dev_info(dev, "init for RGMII_TXID\n");
-		bsp_priv->ops->set_to_rgmii(bsp_priv, 0, bsp_priv->rx_delay);
+		bsp_priv->ops->set_to_rgmii(bsp_priv, -1, bsp_priv->rx_delay);
 		break;
 	case PHY_INTERFACE_MODE_RMII:
 		dev_info(dev, "init for RMII\n");