Problematic understanding of phy-mode in Rockchip DWMAC driver

Hi folks,

I was looking through the RGMII delay setup logic found in rk_gmac_powerup() of dwmac-rk.c, and found its behavior is strange,

	switch (bsp_priv->phy_iface) {
	case PHY_INTERFACE_MODE_RGMII:
		dev_info(dev, "init for RGMII\n");
		if (bsp_priv->ops->set_to_rgmii)
			bsp_priv->ops->set_to_rgmii(bsp_priv,
						    bsp_priv->tx_delay,
						    bsp_priv->rx_delay);
		break;
	case PHY_INTERFACE_MODE_RGMII_ID:
		dev_info(dev, "init for RGMII_ID\n");
		if (bsp_priv->ops->set_to_rgmii)
			bsp_priv->ops->set_to_rgmii(bsp_priv, 0, 0);
		break;
	case PHY_INTERFACE_MODE_RGMII_RXID:
		dev_info(dev, "init for RGMII_RXID\n");
		if (bsp_priv->ops->set_to_rgmii)
			bsp_priv->ops->set_to_rgmii(bsp_priv,
						    bsp_priv->tx_delay, 0);
		break;
	case PHY_INTERFACE_MODE_RGMII_TXID:
		dev_info(dev, "init for RGMII_TXID\n");
		if (bsp_priv->ops->set_to_rgmii)
			bsp_priv->ops->set_to_rgmii(bsp_priv,
						    0, bsp_priv->rx_delay);
		break;
	case PHY_INTERFACE_MODE_RMII:
		dev_info(dev, "init for RMII\n");
		if (bsp_priv->ops->set_to_rmii)
			bsp_priv->ops->set_to_rmii(bsp_priv);
		break;
	default:
		dev_err(dev, "NO interface defined!\n");
	}

I don't think dwmac-rk.c does any fixup for phy_interface, so the MAC driver always connects the PHY with unmodified phy-mode.

ethernet-controller.yaml states,

So let's assume this is true for Rockchip's DWMAC glue, then the driver's behavior could be summarized as,

phy-mode PCB delay Rockchip MAC delay PHY delay[1]
rgmii TX & RX TX & RX TX & RX
rgmii-id None None TX & RX
rgmii-rxid TX TX RX
rgmii-txid RX RX TX
[1]: if available

where only the "rgmii-id" case makes sense. Other cases come with 2ns delays added more than once, and would cause broken link.

It looks like dwmac-rk.c considers "phy-mode" to represent MAC's delay configuration. If so, the table would look like,

phy-mode PCB delay Rockchip MAC delay PHY delay[1]
rgmii N/A TX & RX None
rgmii-id N/A None TX & RX
rgmii-txid N/A TX RX
rgmii-rxid N/A RX TX

all cases have the necessary 2ns delay provided for both TX and RX signals, however on-PCB delays made by traces couldn't be described at all. This idea is also proved by comments in Rockchip devicetrees, for example, rk3576-roc-pc.dts,

	&gmac0 {
		...
		/* Use rgmii-rxid mode to disable rx delay inside Soc */
		phy-mode = "rgmii-rxid";
	};

It seems for Rockchip DWMAC driver, "phy-mode" doesn't follow the generic definition. Should we annotate in rockchip-dwmac.yaml to mention the different usage of the property, or update both the driver and devicetrees to align with ethernet-controller.yaml? The later would break compatibility with existing devicetrees since there are already 15 instances of "rgmii-rxid" found in arch/arm64/boot/dts/rockchip, but luckily most boards make use of "rgmii-id".

I only did a brief search in lore but find no related discussion, please point out if there has already been messages around it, thanks.

Best regards,
Yao Zi

dwmac-rk is very broken and is unfixable thanks to Rockchip not understanding the RGMII interface modes used by the kernel.

It is what it is, we can't change it without causing regressions. Please do not try to fix it.

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Rockchip have recently started adding support for a new version, and appear to of listened to what we have been saying. So it could be the next generation of chips get this correct.

Aspeed are in a similar position, and might also get their next generation correct.

But as Russell pointed out, we have to be very careful with any potential changes to this code. Despite it being wrong, boards work. We cannot fix it and cause regressions.

Andrew

_______________________________________________
Linux-rockchip mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-rockchip

Have you seen any proposed code from Rockchip for their new scheme?

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

There was a patch, including a rather odd formulae to convert register value to delay. I gave some feedback, but it has been silence afterwards.

Andrew

Searching lore's netdev archive doesn't seem to bring anything up.

RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

https://patch.msgid.link/[email protected]/

Hi all,

I am the author of this patch series. My understanding is that "rgmii-id" should be adopted first, and for any delays that may need to be introduced, they should be implemented in the PHY rather than in the GMAC, although I did try to do so.

Did I miss something?

Best, 
Chaoyi

Thanks for confirming. I understand concerns about regressions and agree keeping the driver/dts as-is is better.

However, should we mention the difference between "phy-mode" defined ethernet-controller.yaml and "phy-mode" understood by dwmac-rk driver in rockchip-dwmac.yaml to avoid confusion in the future?

Regards,
Yao Zi

I've been telling DT developers to set the correct phy-mode, 'rgmii-id', and the vendor properties for MAC delays to 0. That works, and it consistent with the documentation.

As far as i know, dwmac-rk only gets things wrong when you try to get the MAC to add the 2ns delays. And that is not a recommended configuration. So i don't think it is too important to document a not recommended configuration is broken.

Andrew