Re: [net-next PATCH v4 06/13] net: dsa: qca8k: move rgmii delay detection to phylink mac_config

On Sun, Oct 10, 2021 at 03:28:39PM +0200, Ansuel Smith wrote:

quoted

I was actually going to say that since RGMII delays are runtime
invariants, you should move their entire programming to probe time, now
you move device tree parsing to runtime :-/

The main idea here was to move everything to mac config and scan the DT
node of the current port that is being configured.

If you insist on doing static configuration in a phylink callback, sure,
the comment was mostly about not accessing directly this struct dsa_port
member. It might change in the future, and the less refactoring required,
the better.

quoted

quoted

-{
-	struct device_node *port_dn;
-	phy_interface_t mode;
-	struct dsa_port *dp;
-	u32 val;
-
-	/* CPU port is already checked */
-	dp = dsa_to_port(priv->ds, 0);
-
-	port_dn = dp->dn;
-
-	/* Check if port 0 is set to the correct type */
-	of_get_phy_mode(port_dn, &mode);
-	if (mode != PHY_INTERFACE_MODE_RGMII_ID &&
-	    mode != PHY_INTERFACE_MODE_RGMII_RXID &&
-	    mode != PHY_INTERFACE_MODE_RGMII_TXID) {
-		return 0;
-	}
-
-	switch (mode) {
-	case PHY_INTERFACE_MODE_RGMII_ID:
-	case PHY_INTERFACE_MODE_RGMII_RXID:

Also, since you touch this area.
There have been tons of discussions on this topic, but I believe that
your interpretation of the RGMII delays is wrong.
Basically a MAC should not apply delays based on the phy-mode string (so
it should treat "rgmii" same as "rgmii-id"), but based on the value of
"rx-internal-delay-ps" and "tx-internal-delay-ps".
The phy-mode is for a PHY to use.

Ok so we can just drop the case and directly check for the
internal-delay-ps presence?

Yes, but please consider existing device trees for this driver. I see
qcom-ipq8064-rb3011.dts and imx6dl-yapp4-common.dtsi, and neither use
explicit rx-internal-delay-ps or tx-internal-delay-ps properties. So
changing the driver to look at just those and ignore "rgmii-id" will
break those device trees, which is not pleasant. What would work is to
search first for *-internal-delay-ps, and then revert to determining the
delays based on the phy-mode, for compatibility.