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

[Andrew Lunn]

> > +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;

I would maybe base it on the value of tx-internal-delay-ps. If it is
2000, then add the delay here. But that has implications for the
interface mode value passed to the PHY.

https://elixir.bootlin.com/linux/v6.15/source/Documentation/devicetree/bindings/net/ethernet-controller.yaml#L287

We could keep mt8195 KISS, and reject all these properties, and let
the PHY add the delays.

	Andrew