| Message ID | 20220623162850.245608-2-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | RK3588 Ethernet Support | expand |
> @@ -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
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
> > 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
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
> 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
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
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
> 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 --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");