[EXT] Re: [PATCH] net: stmmac: dwmac-imx: pause the TXC clock in fixed-link

[Vladimir Oltean]

On Wed, Jul 26, 2023 at 05:29:30PM +0100, Russell King (Oracle) wrote:
> On Wed, Jul 26, 2023 at 04:10:11PM +0000, Shenwei Wang wrote:
> > > So, plat->phy_node will never ever be equal to your "dn" above.
> > > plat->phylink_node is the same as dwmac->dev->of_node above, and
> > > so plat->phylink_node will never be your "dn" above either.
> > >
> > > Those two together means that imx_dwmac_is_fixed_link() will _always_ return
> > > false, and thus most of the code you're adding is rather useless.
> > >
> > > It also means the code you're submitting probably hasn't been properly tested.
> > >
> > > Have you confirmed that imx_dwmac_is_fixed_link() will actually return true in
> > > your testing? Under what conditions did your testing reveal a true return value
> > > from this function?
> > >
> > 
> > We can't make that assumption. In my testing code, I had trace statements in that
> > section to indicate the code was actually executed. You may get some clues from the following DTS:
> > 
> > +&eqos {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&pinctrl_eqos_rgmii>;
> > +       phy-mode = "rgmii-rxid";
> > +       phy-handle = <&fixed0>;
> > +       status = "okay";
> > +
> > +       fixed0: fixed-link {
> > +               speed = <1000>;
> > +               full-duplex;
> > +       };
> 
> This is just totally botched up.
> 
> "fixed0" is _not_ a PHY, therefore you should NOT be referencing it
> in phy-handle. Please see the DT binding document:
> 
>   phy-handle:
>     $ref: /schemas/types.yaml#/definitions/phandle
>     description:
>       Specifies a reference to a node representing a PHY device.
> 
>   fixed-link:
>     oneOf:
>       - $ref: /schemas/types.yaml#/definitions/uint32-array
>         deprecated: true
> ...
>       - type: object
>         additionalProperties: false
>         properties:
>           speed:
> ...
> 
> As I said, fixed-link is _not_ a PHY, and thus phy-handle must *not*
> be used to point at it.
> 
> The mere presence of a node called "fixed-link" will make this "eqos"
> device use that fixed-link node, and the phy-handle will be ignored.
> 
> So sorry, but as far as your patch goes, it's a hard NAK from me
> right now until the DT description is actually correct.
> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
> 

Shenwei, the correct way to describe the link between the eQOS and the
SJA1105 port in imx93-11x11-evk-sja1105.dts is:

#include "imx93-11x11-evk.dts"

&eqos {
	/delete-property/ phy-handle;
	clk_csr = <5>;

	fixed-link {
		speed = <1000>;
		full-duplex;
	};

	mdio {
		/delete-property/ ethernet-phy at 1;

		/* TJA1102 */
		phy0: ethernet-phy at 8 {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0x8>;

			phy1: ethernet-phy at 9 {
				reg = <0x9>;
			};
		};

		/* TJA1102 */
		phy2: ethernet-phy at e {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0xe>;

			phy3: ethernet-phy at f {
				reg = <0xf>;
			};
		};
	};
};

&iomuxc {
	pinctrl_lpspi8: lpspi8grp {
		fsl,pins = <
			MX93_PAD_GPIO_IO12__GPIO2_IO12  0x3fe
			MX93_PAD_GPIO_IO13__LPSPI8_SIN  0x3fe
			MX93_PAD_GPIO_IO14__LPSPI8_SOUT 0x3fe
			MX93_PAD_GPIO_IO15__LPSPI8_SCK  0x3fe
		>;
	};
};

&lpspi8 {
	fsl,spi-num-chipselects = <1>;
	pinctrl-names = "default";
	pinctrl-0 = <&pinctrl_lpspi8>;
	cs-gpios = <&gpio2 12 GPIO_ACTIVE_LOW>;
	pinctrl-assert-gpios = <&pcal6524_b 3 GPIO_ACTIVE_LOW>;
	status = "okay";

	ethernet-switch at 0 {
		reg = <0x0>;
		compatible = "nxp,sja1105q";
		#address-cells = <1>;
		#size-cells = <0>;
		spi-max-frequency = <4000000>;
		spi-cpha;
		status = "okay";

		ethernet-ports {
			#address-cells = <1>;
			#size-cells = <0>;

			port at 0 {
				ethernet = <&eqos>;
				phy-mode = "rgmii-id";
				tx-internal-delay-ps = <2000>;
				rx-internal-delay-ps = <2000>;
				reg = <0>;

				fixed-link {
					speed = <1000>;
					full-duplex;
				};
			};

			port at 1 {
				phy-handle = <&phy0>;
				label = "swp1";
				phy-mode = "mii";
				reg = <1>;
			};

			port at 2 {
				label = "swp2";
				phy-handle = <&phy1>;
				phy-mode = "mii";
				reg = <2>;
			};

			port at 3 {
				label = "swp3";
				phy-handle = <&phy2>;
				phy-mode = "rmii";
				reg = <3>;
			};

			port at 4 {
				phy-handle = <&phy3>;
				label = "swp4";
				phy-mode = "rmii";
				reg = <4>;
			};
		};
	};
};

The "mdio" node under "eqos" is just the OF node of the MDIO bus.
It doesn't necessarily mean that the net_device corresponding to the
stmmac uses all those PHYs. It uses just the PHY that it has a
phy-handle to. It may not even have a phy-handle at all, just a
fixed-link, like above.