Re: [PATCH net-next v2 2/7] net: stmmac: mediatek: simplify TX/RX delay handling in mt8195_set_delay

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hey,

On 9/24/26 09:23, Louis-Alexis Eyraud wrote:
> The mt8195_set_delay function modifies at its beginning the TX and RX
> internal delay variables, located in the driver data, by dividing
> them by a constant (290) and restores their original values by
> multiplying them again at the function end. It is done in order to
> convert them into a step value, used by the hardware registers for
> setting these delays.
> 
> But this is rather pointless to modify the driver data for that, while
> it could be done locally in the function. The original delay values
> cannot be used anymore (if needed) during mt8195_set_delay processing.
> Finally, they are altered after the function call if they are not a
> multiple of 290.
> 
> So, simplify these delay variable handling by using local variables to
> convert them into the register value and use those in the write calls.
> Also, remove the two private conversion functions, that are not useful
> anymore and add definitions for MT8195 RX/TX delay maximum and divider
> values.
> 
> Signed-off-by: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

Looking at this, seems like the 2712 support could benefit from the same
cleanups you've done with the weird division / remultiplication.

That can be a separate cleanup though.

Maxime


> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-mediatek.c   | 77 ++++++++++------------
>  1 file changed, 36 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index 30ae0dba7fff..f7eb85110df0 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> @@ -63,6 +63,11 @@
>  #define MT8195_DLY_RMII_TXC_ENABLE	BIT(5)
>  #define MT8195_DLY_RMII_TXC_STAGES	GENMASK(4, 0)
>  
> +#define MT8195_DLY_RXC_STAGE_DIV	290 /* 290ps per stage */
> +#define MT8195_DLY_RXC_MAX		9280 /* 32 x 290ps */
> +#define MT8195_DLY_TXC_STAGE_DIV	290 /* 290ps per stage */
> +#define MT8195_DLY_TXC_MAX		9280 /* 32 x 290ps */
> +
>  struct mac_delay_struct {
>  	u32 tx_delay;
>  	u32 rx_delay;
> @@ -293,39 +298,27 @@ static int mt8195_set_interface(struct mediatek_dwmac_plat_data *plat,
>  	return 0;
>  }
>  
> -static void mt8195_delay_ps2stage(struct mediatek_dwmac_plat_data *plat)
> -{
> -	struct mac_delay_struct *mac_delay = &plat->mac_delay;
> -
> -	/* 290ps per stage */
> -	mac_delay->tx_delay /= 290;
> -	mac_delay->rx_delay /= 290;
> -}
> -
> -static void mt8195_delay_stage2ps(struct mediatek_dwmac_plat_data *plat)
> -{
> -	struct mac_delay_struct *mac_delay = &plat->mac_delay;
> -
> -	/* 290ps per stage */
> -	mac_delay->tx_delay *= 290;
> -	mac_delay->rx_delay *= 290;
> -}
> -
>  static int mt8195_set_delay(struct mediatek_dwmac_plat_data *plat)
>  {
>  	struct mac_delay_struct *mac_delay = &plat->mac_delay;
> -	u32 gtxc_delay_val = 0, delay_val = 0, rmii_delay_val = 0;
> -
> -	mt8195_delay_ps2stage(plat);
> +	u32 rx_delay_stage_val = mac_delay->rx_delay / MT8195_DLY_RXC_STAGE_DIV;
> +	u32 tx_delay_stage_val = mac_delay->tx_delay / MT8195_DLY_TXC_STAGE_DIV;
> +	u32 gtxc_delay_val = 0;
> +	u32 rmii_delay_val = 0;
> +	u32 delay_val = 0;
>  
>  	switch (plat->phy_mode) {
>  	case PHY_INTERFACE_MODE_MII:
> -		delay_val |= FIELD_PREP(MT8195_DLY_TXC_ENABLE, !!mac_delay->tx_delay);
> -		delay_val |= FIELD_PREP(MT8195_DLY_TXC_STAGES, mac_delay->tx_delay);
> +		delay_val |= FIELD_PREP(MT8195_DLY_TXC_ENABLE,
> +					!!tx_delay_stage_val);
> +		delay_val |= FIELD_PREP(MT8195_DLY_TXC_STAGES,
> +					tx_delay_stage_val);
>  		delay_val |= FIELD_PREP(MT8195_DLY_TXC_INV, mac_delay->tx_inv);
>  
> -		delay_val |= FIELD_PREP(MT8195_DLY_RXC_ENABLE, !!mac_delay->rx_delay);
> -		delay_val |= FIELD_PREP(MT8195_DLY_RXC_STAGES, mac_delay->rx_delay);
> +		delay_val |= FIELD_PREP(MT8195_DLY_RXC_ENABLE,
> +					!!rx_delay_stage_val);
> +		delay_val |= FIELD_PREP(MT8195_DLY_RXC_STAGES,
> +					rx_delay_stage_val);
>  		delay_val |= FIELD_PREP(MT8195_DLY_RXC_INV, mac_delay->rx_inv);
>  		break;
>  	case PHY_INTERFACE_MODE_RMII:
> @@ -336,16 +329,16 @@ static int mt8195_set_delay(struct mediatek_dwmac_plat_data *plat)
>  			 * The ingress timing can be adjusted by RMII_RXC delay macro circuit.
>  			 */
>  			rmii_delay_val |= FIELD_PREP(MT8195_DLY_RMII_TXC_ENABLE,
> -						     !!mac_delay->tx_delay);
> +						     !!tx_delay_stage_val);
>  			rmii_delay_val |= FIELD_PREP(MT8195_DLY_RMII_TXC_STAGES,
> -						     mac_delay->tx_delay);
> +						     tx_delay_stage_val);
>  			rmii_delay_val |= FIELD_PREP(MT8195_DLY_RMII_TXC_INV,
>  						     mac_delay->tx_inv);
>  
>  			rmii_delay_val |= FIELD_PREP(MT8195_DLY_RMII_RXC_ENABLE,
> -						     !!mac_delay->rx_delay);
> +						     !!rx_delay_stage_val);
>  			rmii_delay_val |= FIELD_PREP(MT8195_DLY_RMII_RXC_STAGES,
> -						     mac_delay->rx_delay);
> +						     rx_delay_stage_val);
>  			rmii_delay_val |= FIELD_PREP(MT8195_DLY_RMII_RXC_INV,
>  						     mac_delay->rx_inv);
>  		} else {
> @@ -361,9 +354,9 @@ static int mt8195_set_delay(struct mediatek_dwmac_plat_data *plat)
>  				 * by RXC delay macro circuit.
>  				 */
>  				delay_val |= FIELD_PREP(MT8195_DLY_RXC_ENABLE,
> -							!!mac_delay->rx_delay);
> +							!!rx_delay_stage_val);
>  				delay_val |= FIELD_PREP(MT8195_DLY_RXC_STAGES,
> -							mac_delay->rx_delay);
> +							rx_delay_stage_val);
>  				delay_val |= FIELD_PREP(MT8195_DLY_RXC_INV,
>  							mac_delay->rx_inv);
>  			} else {
> @@ -372,9 +365,9 @@ static int mt8195_set_delay(struct mediatek_dwmac_plat_data *plat)
>  				 * by TXC delay macro circuit.
>  				 */
>  				delay_val |= FIELD_PREP(MT8195_DLY_TXC_ENABLE,
> -							!!mac_delay->rx_delay);
> +							!!rx_delay_stage_val);
>  				delay_val |= FIELD_PREP(MT8195_DLY_TXC_STAGES,
> -							mac_delay->rx_delay);
> +							rx_delay_stage_val);
>  				delay_val |= FIELD_PREP(MT8195_DLY_TXC_INV,
>  							mac_delay->rx_inv);
>  			}
> @@ -384,12 +377,16 @@ static int mt8195_set_delay(struct mediatek_dwmac_plat_data *plat)
>  	case PHY_INTERFACE_MODE_RGMII_TXID:
>  	case PHY_INTERFACE_MODE_RGMII_RXID:
>  	case PHY_INTERFACE_MODE_RGMII_ID:
> -		gtxc_delay_val |= FIELD_PREP(MT8195_DLY_GTXC_ENABLE, !!mac_delay->tx_delay);
> -		gtxc_delay_val |= FIELD_PREP(MT8195_DLY_GTXC_STAGES, mac_delay->tx_delay);
> +		gtxc_delay_val |= FIELD_PREP(MT8195_DLY_GTXC_ENABLE,
> +					     !!tx_delay_stage_val);
> +		gtxc_delay_val |= FIELD_PREP(MT8195_DLY_GTXC_STAGES,
> +					     tx_delay_stage_val);
>  		gtxc_delay_val |= FIELD_PREP(MT8195_DLY_GTXC_INV, mac_delay->tx_inv);
>  
> -		delay_val |= FIELD_PREP(MT8195_DLY_RXC_ENABLE, !!mac_delay->rx_delay);
> -		delay_val |= FIELD_PREP(MT8195_DLY_RXC_STAGES, mac_delay->rx_delay);
> +		delay_val |= FIELD_PREP(MT8195_DLY_RXC_ENABLE,
> +					!!rx_delay_stage_val);
> +		delay_val |= FIELD_PREP(MT8195_DLY_RXC_STAGES,
> +					rx_delay_stage_val);
>  		delay_val |= FIELD_PREP(MT8195_DLY_RXC_INV, mac_delay->rx_inv);
>  
>  		break;
> @@ -408,8 +405,6 @@ static int mt8195_set_delay(struct mediatek_dwmac_plat_data *plat)
>  	regmap_write(plat->peri_regmap, MT8195_PERI_ETH_CTRL1, delay_val);
>  	regmap_write(plat->peri_regmap, MT8195_PERI_ETH_CTRL2, rmii_delay_val);
>  
> -	mt8195_delay_stage2ps(plat);
> -
>  	return 0;
>  }
>  
> @@ -418,8 +413,8 @@ static const struct mediatek_dwmac_variant mt8195_gmac_variant = {
>  	.dwmac_set_delay = mt8195_set_delay,
>  	.clk_list = mt8195_dwmac_clk_l,
>  	.num_clks = ARRAY_SIZE(mt8195_dwmac_clk_l),
> -	.rx_delay_max = 9280,
> -	.tx_delay_max = 9280,
> +	.rx_delay_max = MT8195_DLY_RXC_MAX,
> +	.tx_delay_max = MT8195_DLY_TXC_MAX,
>  	.dma_bit_mask = 35,
>  };
>  
>