Re: [PATCH net-next v3 7/9] net: stmmac: mediatek: use TX clock phase shift in RGMII mode with 1Gbps speed

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

Hi,

On 10/16/26 12:31, Louis-Alexis Eyraud wrote:
> Currently, if the devicetree of a board based on MT8195 (or compatible)
> configures the use of RGMII PHY mode and insert at MAC level additional
> delay on TX clock at MAC (with "mediatek,tx-delay-ps" vendor property
> presence), the driver inserts this delay by setting TX coarse delay
> bits (bit 0 to 5) of the Ethernet control 0 register from the
> peripheral configuration (pericfg) at probe time.
> 
> The HW delay macro circuit, that these bits enable, can be affected by
> external factors such as environment temperature or vcore voltage and
> thus applied delay value can change. For 10/100M link speed cases the
> variations are not problematic, but it can be for 1G link speed cases,
> that require much precise and stable timings (around 2ns).
> 
> The Ethernet control 0 register of MT8195 SoC has another bit (bit 22:
> MT8195_RGMII_TXC_PHASE_CTR) to enable the TX clock phase shift and
> insert a 2ns clock delay in a more consistence way, as it is not
> affected like the HW delay macro circuit under the same conditions.
> This only works though for 1G speed cases so it cannot be set
> unconditionally at probe time, thus needs to be set after link speed
> negotiation.
> 
> In order to allow the TX clock phase shift use in the driver, add in
> the variant platform data and probe sequence the support of
> fix_mac_speed callback, provided by dwmac core. This callback allows
> driver configuration update when the link speed has been negotiated.
> Implement it for MT8195 so that, when a delay needs to be inserted
> for TX clock (due to the devicetree configuration), it enables the
> TX clock phase shift in Ethernet control 0 register only and only if
> the PHY mode allows it and negotiated link speed is 1G, and fallback
> to the HW delay macro circuit otherwise.
> 
> Signed-off-by: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>
> ---
>  .../net/ethernet/stmicro/stmmac/dwmac-mediatek.c   | 62 ++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> index 7dd46cd5a49e..4b2d5280f703 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-mediatek.c
> @@ -104,6 +104,9 @@ struct mediatek_dwmac_variant {
>  				       u8 phy_intf_sel);
>  	int (*dwmac_set_delay)(struct mediatek_dwmac_plat_data *plat);
>  
> +	void (*dwmac_fix_mac_speed)(void *priv, phy_interface_t interface,
> +				    int speed, unsigned int mode);
> +
>  	/* clock ids to be requested */
>  	const char * const *clk_list;
>  	int num_clks;
> @@ -430,9 +433,65 @@ static int mt8195_set_delay(struct mediatek_dwmac_plat_data *plat)
>  	return 0;
>  }
>  
> +static void mt8195_fix_mac_speed(void *priv, phy_interface_t interface,
> +				 int speed, unsigned int mode)
> +{
> +	struct mediatek_dwmac_plat_data *priv_plat = priv;
> +	const struct mediatek_dwmac_variant *variant;
> +	struct mac_delay_struct *mac_delay;
> +	u32 tx_delay_stage_val, reg_offset;
> +	u32 reg_val = 0;
> +
> +	if (!priv_plat)
> +		return;
> +
> +	mac_delay = &priv_plat->mac_delay;
> +	variant = priv_plat->variant;
> +
> +	if (!mac_delay->tx_delay ||
> +	    (interface != PHY_INTERFACE_MODE_RGMII &&
> +	     interface != PHY_INTERFACE_MODE_RGMII_RXID))
> +		return;

I don't think this is right. PHY_INTERFACE_MODE_RGMII in that context comes
from the devicetree, and therefore means "RGMII delays are implemented through
traces on the PCB", the MAC isn't supposed to insert any. RXID means "TX delays
are on PCB, RX delays are internal).

I'd expect the condition here to be:

> +	if (!mac_delay->tx_delay ||
> +	    (interface != PHY_INTERFACE_MODE_RGMII_ID &&
> +	     interface != PHY_INTERFACE_MODE_RGMII_TXID))
> +		return;

Maxime