[PATCH 0/3] Use DELAY_ENABLE macro for RK3328, RK3566/RK3568 and RK3588

[PATCH 0/3] Use DELAY_ENABLE macro for RK3328, RK3566/RK3568 and RK3588

[Jonas Karlman]

Almost all Rockchip GMAC variants use the DELAY_ENABLE macro to help
enable or disable use of MAC rx/tx delay. However, RK3328, RK3566/RK3568
and RK3588 GMAC driver does not.

Use of the DELAY_ENABLE macro help ensure the MAC rx/tx delay is
disabled, instead of being enabled and using a zero delay, when
RGMII_ID/RXID/TXID is used.

RK3328 driver was merged around the same time as when DELAY_ENABLE was
introduced so it is understandable why it was missed. Both RK3566/RK3568
and RK3588 support were introduced much later yet they also missed using
the DELAY_ENABLE macro (so did vendor kernel at that time).

This series fixes all these cases to unify how GMAC delay feature is
enabled or disabled across the different GMAC variants.

Jonas Karlman (3):
  net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3328
  net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3566/RK3568
  net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3588

 .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

-- 
2.48.1

[PATCH 1/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3328

[Jonas Karlman]

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

[PATCH 2/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3566/RK3568

[Jonas Karlman]

Support for Rockchip RK3566/RK3568 GMAC was added without use of the
DELAY_ENABLE macro to assist with enable/disable use of MAC rx/tx delay.

Change to use the DELAY_ENABLE macro to help disable MAC delay when
RGMII_ID/RXID/TXID is used. This also re-order to enable/disable before
the delay is written to match all other GMAC and vendor kernel.

Fixes: 3bb3d6b1c195 ("net: stmmac: Add RK3566/RK3568 SoC support")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 297fa93e4a39..37eb86e4e325 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -1049,14 +1049,13 @@ static void rk3568_set_to_rgmii(struct rk_priv_data *bsp_priv,
 	con1 = (bsp_priv->id == 1) ? RK3568_GRF_GMAC1_CON1 :
 				     RK3568_GRF_GMAC0_CON1;
 
+	regmap_write(bsp_priv->grf, con1,
+		     RK3568_GMAC_PHY_INTF_SEL_RGMII |
+		     DELAY_ENABLE(RK3568, tx_delay, rx_delay));
+
 	regmap_write(bsp_priv->grf, con0,
 		     RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
 		     RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
-
-	regmap_write(bsp_priv->grf, con1,
-		     RK3568_GMAC_PHY_INTF_SEL_RGMII |
-		     RK3568_GMAC_RXCLK_DLY_ENABLE |
-		     RK3568_GMAC_TXCLK_DLY_ENABLE);
 }
 
 static void rk3568_set_to_rmii(struct rk_priv_data *bsp_priv)
-- 
2.48.1

[PATCH 3/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3588

[Jonas Karlman]

Support for Rockchip RK3588 GMAC was added without use of the
DELAY_ENABLE macro to assist with enable/disable use of MAC rx/tx delay.

Change to use a variant of the DELAY_ENABLE macro to help disable MAC
delay when RGMII_ID/RXID/TXID is used.

Fixes: 2f2b60a0ec28 ("net: ethernet: stmmac: dwmac-rk: Add gmac support for rk3588")
Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 37eb86e4e325..79db81d68afd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -91,6 +91,10 @@ struct rk_priv_data {
 	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) | \
 	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
 
+#define DELAY_ENABLE_BY_ID(soc, tx, rx, id) \
+	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE(id) : soc##_GMAC_TXCLK_DLY_DISABLE(id)) | \
+	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE(id) : soc##_GMAC_RXCLK_DLY_DISABLE(id)))
+
 #define PX30_GRF_GMAC_CON1		0x0904
 
 /* PX30_GRF_GMAC_CON1 */
@@ -1322,8 +1326,7 @@ static void rk3588_set_to_rgmii(struct rk_priv_data *bsp_priv,
 		     RK3588_GMAC_CLK_RGMII_MODE(id));
 
 	regmap_write(bsp_priv->grf, RK3588_GRF_GMAC_CON7,
-		     RK3588_GMAC_RXCLK_DLY_ENABLE(id) |
-		     RK3588_GMAC_TXCLK_DLY_ENABLE(id));
+		     DELAY_ENABLE_BY_ID(RK3588, tx_delay, rx_delay, id));
 
 	regmap_write(bsp_priv->grf, offset_con,
 		     RK3588_GMAC_CLK_RX_DL_CFG(rx_delay) |
-- 
2.48.1

Re: [PATCH 1/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3328

[Dragan Simic]

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>

Re: [PATCH 2/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3566/RK3568

[Dragan Simic]

Hello Jonas,

On 2025-03-06 21:38, Jonas Karlman wrote:
> Support for Rockchip RK3566/RK3568 GMAC was added without use of the
> DELAY_ENABLE macro to assist with enable/disable use of MAC rx/tx 
> delay.
> 
> Change to use the DELAY_ENABLE macro to help disable MAC delay when
> RGMII_ID/RXID/TXID is used. This also re-order to enable/disable before

s/re-order/re-orders/

> the delay is written to match all other GMAC and vendor kernel.

s/GMAC/GMACs/

> Fixes: 3bb3d6b1c195 ("net: stmmac: Add RK3566/RK3568 SoC support")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 297fa93e4a39..37eb86e4e325 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -1049,14 +1049,13 @@ static void rk3568_set_to_rgmii(struct
> rk_priv_data *bsp_priv,
>  	con1 = (bsp_priv->id == 1) ? RK3568_GRF_GMAC1_CON1 :
>  				     RK3568_GRF_GMAC0_CON1;
> 
> +	regmap_write(bsp_priv->grf, con1,
> +		     RK3568_GMAC_PHY_INTF_SEL_RGMII |
> +		     DELAY_ENABLE(RK3568, tx_delay, rx_delay));
> +
>  	regmap_write(bsp_priv->grf, con0,
>  		     RK3568_GMAC_CLK_RX_DL_CFG(rx_delay) |
>  		     RK3568_GMAC_CLK_TX_DL_CFG(tx_delay));
> -
> -	regmap_write(bsp_priv->grf, con1,
> -		     RK3568_GMAC_PHY_INTF_SEL_RGMII |
> -		     RK3568_GMAC_RXCLK_DLY_ENABLE |
> -		     RK3568_GMAC_TXCLK_DLY_ENABLE);
>  }
> 
>  static void rk3568_set_to_rmii(struct rk_priv_data *bsp_priv)

Thanks for this patch...  It's looking good to me, and good job
spotting this issue!  Please, free to include:

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

Re: [PATCH 3/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3588

[Dragan Simic]

Hello Jonas,

On 2025-03-06 21:38, Jonas Karlman wrote:
> Support for Rockchip RK3588 GMAC was added without use of the
> DELAY_ENABLE macro to assist with enable/disable use of MAC rx/tx 
> delay.
> 
> Change to use a variant of the DELAY_ENABLE macro to help disable MAC
> delay when RGMII_ID/RXID/TXID is used.
> 
> Fixes: 2f2b60a0ec28 ("net: ethernet: stmmac: dwmac-rk: Add gmac
> support for rk3588")
> Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> index 37eb86e4e325..79db81d68afd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -91,6 +91,10 @@ struct rk_priv_data {
>  	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) 
> | \
>  	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
> 
> +#define DELAY_ENABLE_BY_ID(soc, tx, rx, id) \
> +	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE(id) : 
> soc##_GMAC_TXCLK_DLY_DISABLE(id)) | \
> +	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE(id) : 
> soc##_GMAC_RXCLK_DLY_DISABLE(id)))
> +
>  #define PX30_GRF_GMAC_CON1		0x0904
> 
>  /* PX30_GRF_GMAC_CON1 */
> @@ -1322,8 +1326,7 @@ static void rk3588_set_to_rgmii(struct
> rk_priv_data *bsp_priv,
>  		     RK3588_GMAC_CLK_RGMII_MODE(id));
> 
>  	regmap_write(bsp_priv->grf, RK3588_GRF_GMAC_CON7,
> -		     RK3588_GMAC_RXCLK_DLY_ENABLE(id) |
> -		     RK3588_GMAC_TXCLK_DLY_ENABLE(id));
> +		     DELAY_ENABLE_BY_ID(RK3588, tx_delay, rx_delay, id));
> 
>  	regmap_write(bsp_priv->grf, offset_con,
>  		     RK3588_GMAC_CLK_RX_DL_CFG(rx_delay) |

Thanks for this patch...  It's looking good to me, and good job
spotting this issue!  Please, free to include:

Reviewed-by: Dragan Simic <dsimic@manjaro.org>

Re: [PATCH 0/3] Use DELAY_ENABLE macro for RK3328, RK3566/RK3568 and RK3588

[Dragan Simic]

Hello Jonas,

On 2025-03-06 21:38, Jonas Karlman wrote:
> Almost all Rockchip GMAC variants use the DELAY_ENABLE macro to help
> enable or disable use of MAC rx/tx delay. However, RK3328, 
> RK3566/RK3568
> and RK3588 GMAC driver does not.
> 
> Use of the DELAY_ENABLE macro help ensure the MAC rx/tx delay is
> disabled, instead of being enabled and using a zero delay, when
> RGMII_ID/RXID/TXID is used.
> 
> RK3328 driver was merged around the same time as when DELAY_ENABLE was
> introduced so it is understandable why it was missed. Both 
> RK3566/RK3568
> and RK3588 support were introduced much later yet they also missed 
> using
> the DELAY_ENABLE macro (so did vendor kernel at that time).
> 
> This series fixes all these cases to unify how GMAC delay feature is
> enabled or disabled across the different GMAC variants.
> 
> Jonas Karlman (3):
>   net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3328
>   net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3566/RK3568
>   net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3588
> 
>  .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)

As far as I can tell, the RV1126 GMAC should also be converted to use
the DELAY_ENABLE macro, which the vendor kernel already does. [*]  
Perhaps
that could be performed in new patch 4/4 in this series?

BTW, it would be quite neat to introduce the DELAY_VALUE macro, which
makes the function calls a bit more compact. [*]

[*] 
https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-5.10/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

Re: [PATCH 0/3] Use DELAY_ENABLE macro for RK3328, RK3566/RK3568 and RK3588

[Jonas Karlman]

Hi Dragan,

On 2025-03-06 22:41, Dragan Simic wrote:
> Hello Jonas,
> 
> On 2025-03-06 21:38, Jonas Karlman wrote:
>> Almost all Rockchip GMAC variants use the DELAY_ENABLE macro to help
>> enable or disable use of MAC rx/tx delay. However, RK3328, 
>> RK3566/RK3568
>> and RK3588 GMAC driver does not.
>>
>> Use of the DELAY_ENABLE macro help ensure the MAC rx/tx delay is
>> disabled, instead of being enabled and using a zero delay, when
>> RGMII_ID/RXID/TXID is used.
>>
>> RK3328 driver was merged around the same time as when DELAY_ENABLE was
>> introduced so it is understandable why it was missed. Both 
>> RK3566/RK3568
>> and RK3588 support were introduced much later yet they also missed 
>> using
>> the DELAY_ENABLE macro (so did vendor kernel at that time).
>>
>> This series fixes all these cases to unify how GMAC delay feature is
>> enabled or disabled across the different GMAC variants.
>>
>> Jonas Karlman (3):
>>   net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3328
>>   net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3566/RK3568
>>   net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3588
>>
>>  .../net/ethernet/stmicro/stmmac/dwmac-rk.c    | 19 ++++++++++---------
>>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> As far as I can tell, the RV1126 GMAC should also be converted to use
> the DELAY_ENABLE macro, which the vendor kernel already does. [*]  
> Perhaps
> that could be performed in new patch 4/4 in this series?

Good catch, the RV1126 seem to use a slight different M0/M1 variant and
unfortunately I do not have access to any RV1126 board to do any runtime
testing. For RK3328, RK356x and RK3588 I could at least do runtime
testing.

I can add an untested patch for RV1126 in a v2 if needed :-)

> 
> BTW, it would be quite neat to introduce the DELAY_VALUE macro, which
> makes the function calls a bit more compact. [*]

Personally, I do not see a real need for the DELAY_VALUE macro, current
use of the soc_GMAC_CLK_yX_DL_CFG() macro is readable enough.

The ENABLE_DELAY at least helps remove an otherwise more complex enable
or disable conditional handling.

Regards,
Jonas

> 
> [*] 
> https://raw.githubusercontent.com/rockchip-linux/kernel/refs/heads/develop-5.10/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c

Re: [PATCH 1/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3328

[Andrew Lunn]

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

Re: [PATCH 1/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3328

[Jonas Karlman]

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

Re: [PATCH 3/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3588

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

Hi,

On Thu, Mar 06, 2025 at 10:33:23PM +0100, Dragan Simic wrote:
> Hello Jonas,
> 
> On 2025-03-06 21:38, Jonas Karlman wrote:
> > Support for Rockchip RK3588 GMAC was added without use of the
> > DELAY_ENABLE macro to assist with enable/disable use of MAC rx/tx delay.
> > 
> > Change to use a variant of the DELAY_ENABLE macro to help disable MAC
> > delay when RGMII_ID/RXID/TXID is used.
> > 
> > Fixes: 2f2b60a0ec28 ("net: ethernet: stmmac: dwmac-rk: Add gmac
> > support for rk3588")
> > Signed-off-by: Jonas Karlman <jonas@kwiboo.se>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 7 +++++--
> >  1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > index 37eb86e4e325..79db81d68afd 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> > @@ -91,6 +91,10 @@ struct rk_priv_data {
> >  	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE : soc##_GMAC_TXCLK_DLY_DISABLE) |
> > \
> >  	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE : soc##_GMAC_RXCLK_DLY_DISABLE))
> > 
> > +#define DELAY_ENABLE_BY_ID(soc, tx, rx, id) \
> > +	(((tx) ? soc##_GMAC_TXCLK_DLY_ENABLE(id) :
> > soc##_GMAC_TXCLK_DLY_DISABLE(id)) | \
> > +	 ((rx) ? soc##_GMAC_RXCLK_DLY_ENABLE(id) :
> > soc##_GMAC_RXCLK_DLY_DISABLE(id)))
> > +
> >  #define PX30_GRF_GMAC_CON1		0x0904
> > 
> >  /* PX30_GRF_GMAC_CON1 */
> > @@ -1322,8 +1326,7 @@ static void rk3588_set_to_rgmii(struct
> > rk_priv_data *bsp_priv,
> >  		     RK3588_GMAC_CLK_RGMII_MODE(id));
> > 
> >  	regmap_write(bsp_priv->grf, RK3588_GRF_GMAC_CON7,
> > -		     RK3588_GMAC_RXCLK_DLY_ENABLE(id) |
> > -		     RK3588_GMAC_TXCLK_DLY_ENABLE(id));
> > +		     DELAY_ENABLE_BY_ID(RK3588, tx_delay, rx_delay, id));
> > 
> >  	regmap_write(bsp_priv->grf, offset_con,
> >  		     RK3588_GMAC_CLK_RX_DL_CFG(rx_delay) |
> 
> Thanks for this patch...  It's looking good to me, and good job
> spotting this issue!  Please, free to include:
> 
> Reviewed-by: Dragan Simic <dsimic@manjaro.org>

Reviewed-by: Sebastian Reichel <sebastian.reichel@collabora.com>

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] net: stmmac: dwmac-rk: Use DELAY_ENABLE macro for RK3328

[Andrew Lunn]

On Fri, Mar 07, 2025 at 12:28:42AM +0100, Jonas Karlman wrote:
> 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.

https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

  It must either fix a real bug that bothers people or just add a device ID.

It does not sound like this meets the stable requirements. Plus there
is the potential for breakage. So i suggest you drop the Fixes: tag
and we merge this via net-next.

Please take a look at:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

    Andrew

---
pw-bot: cr