| Message ID | 20250306203858.1677595-2-jonas@kwiboo.se (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Use DELAY_ENABLE macro for RK3328, RK3566/RK3568 and RK3588 | expand |
Hello Jonas, On 2025-03-06 21:38, Jonas Karlman wrote: > Support for Rockchip RK3328 GMAC and addition of the DELAY_ENABLE macro > was merged in the same merge window. This resulted in RK3328 not being > converted to use the new DELAY_ENABLE macro. > > Change to use the DELAY_ENABLE macro to help disable MAC delay when > RGMII_ID/RXID/TXID is used. > > Fixes: eaf70ad14cbb ("net: stmmac: dwmac-rk: Add handling for > RGMII_ID/RXID/TXID") > Signed-off-by: Jonas Karlman <jonas@kwiboo.se> > --- > drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > index 003fa5cf42c3..297fa93e4a39 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c > @@ -593,8 +593,7 @@ 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) | Thanks for this patch... It's looking good to me, and good job spotting this issue! Please, feel free to include: Reviewed-by: Dragan Simic <dsimic@manjaro.org>
On Thu, Mar 06, 2025 at 08:38:52PM +0000, Jonas Karlman wrote: > Support for Rockchip RK3328 GMAC and addition of the DELAY_ENABLE macro > was merged in the same merge window. This resulted in RK3328 not being > converted to use the new DELAY_ENABLE macro. > > Change to use the DELAY_ENABLE macro to help disable MAC delay when > RGMII_ID/RXID/TXID is used. > > Fixes: eaf70ad14cbb ("net: stmmac: dwmac-rk: Add handling for RGMII_ID/RXID/TXID") Please add a description of the broken behaviour. How would i know i need this fix? What would i see? We also need to be careful with backwards compatibility. Is there the potential for double bugs cancelling each other out? A board which has the wrong phy-mode in DT, but because of this bug, the wrong register is written and it actually works because of reset defaults? Andrew --- pw-bot: cr
Hi Andrew, On 2025-03-06 23:25, Andrew Lunn wrote: > On Thu, Mar 06, 2025 at 08:38:52PM +0000, Jonas Karlman wrote: >> Support for Rockchip RK3328 GMAC and addition of the DELAY_ENABLE macro >> was merged in the same merge window. This resulted in RK3328 not being >> converted to use the new DELAY_ENABLE macro. >> >> Change to use the DELAY_ENABLE macro to help disable MAC delay when >> RGMII_ID/RXID/TXID is used. >> >> Fixes: eaf70ad14cbb ("net: stmmac: dwmac-rk: Add handling for RGMII_ID/RXID/TXID") > > Please add a description of the broken behaviour. How would i know i > need this fix? What would i see? Based on my layman testing I have not seen any real broken behaviour with current enablement of a zero rx/tx MAC delay for RGMII_ID/RXID/TXID. The driver ops is called with a rx/tx_delay=0 for RGMII_ID/RXID/TXID modes, what the MAC does with enable=true and rx/tx_delay=0 is unclear to me. > > We also need to be careful with backwards compatibility. Is there the > potential for double bugs cancelling each other out? A board which has > the wrong phy-mode in DT, but because of this bug, the wrong register > is written and it actually works because of reset defaults? To my knowledge this should have very limited effect, however I am no network expert and after doing very basic testing on several different rk3328/rk3566/rk3568/rk3588 I could not see any real affect with/without this change. The use of Fixes-tag was more to have a reference to the commit that first should have used the DELAY_ENABLE macro. Regards, Jonas > > Andrew > > --- > pw-bot: cr > > _______________________________________________ > Linux-rockchip mailing list > Linux-rockchip@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-rockchip
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 003fa5cf42c3..297fa93e4a39 100644 --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c @@ -593,8 +593,7 @@ 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) |
Support for Rockchip RK3328 GMAC and addition of the DELAY_ENABLE macro was merged in the same merge window. This resulted in RK3328 not being converted to use the new DELAY_ENABLE macro. Change to use the DELAY_ENABLE macro to help disable MAC delay when RGMII_ID/RXID/TXID is used. Fixes: eaf70ad14cbb ("net: stmmac: dwmac-rk: Add handling for RGMII_ID/RXID/TXID") Signed-off-by: Jonas Karlman <jonas@kwiboo.se> --- drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)