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

[Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>]

Hi Maxime,

On Sun, 2026-10-04 at 15:43 +0200, Maxime Chevallier wrote:
> 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
> 
thanks for the review.

I'll add an additional patch to do the same cleanup for MT2712
in v3.

Regards,
Louis-Alexis

> 
> > ---
> >  .../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_s
> > tage_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_s
> > tage_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,
> >  };
> >  
> > 
>