RE: [PATCH] ARM: dts: ls1021: Fix SGMII PCS link remaining down after PHY disconnect

[Leo Li]

> -----Original Message-----
> From: Vladimir Oltean
> Sent: Monday, March 25, 2019 4:31 AM
> To: shawnguo@xxxxxxxxxx; Leo Li <leoyang.li@xxxxxxx>; Claudiu Manoil
> <claudiu.manoil@xxxxxxx>
> Cc: robh+dt@xxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> netdev@xxxxxxxxxxxxxxx; davem@xxxxxxxxxxxxx; Vladimir Oltean
> <vladimir.oltean@xxxxxxx>
> Subject: [PATCH] ARM: dts: ls1021: Fix SGMII PCS link remaining down after
> PHY disconnect
> 
> Each eTSEC MAC has its own TBI (SGMII) PCS and private MDIO bus.
> But due to a DTS oversight, both SGMII-compatible MACs of the LS1021 SoC
> are pointing towards the same internal PCS. Therefore nobody is controlling
> the internal PCS of eTSEC0.
> 
> Upon initial ndo_open, the SGMII link is ok by virtue of U-boot initialization.
> But upon an ifdown/ifup sequence, the code path from ndo_open ->
> init_phy -> gfar_configure_serdes does not get executed for the PCS of
> eTSEC0 (and is executed twice for MAC eTSEC1). So the SGMII link remains
> down for eTSEC0. On the LS1021A-TWR board, to signal this failure condition,
> the PHY driver keeps printing
> '803x_aneg_done: SGMII link is not ok'.
> 
> Fixes: 055223d4d22d ("ARM: dts: ls1021a: Enable the eTSEC ports on QDS
> and TWR")
> Signed-off-by: Vladimir Oltean <vladimir.oltean@xxxxxxx>
> ---
>  arch/arm/boot/dts/ls1021a-twr.dts | 9 ++++++++-
>  arch/arm/boot/dts/ls1021a.dtsi    | 9 +++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/dts/ls1021a-twr.dts
> b/arch/arm/boot/dts/ls1021a-twr.dts
> index 97e1fb7..9b1fe99 100644
> --- a/arch/arm/boot/dts/ls1021a-twr.dts
> +++ b/arch/arm/boot/dts/ls1021a-twr.dts
> @@ -145,7 +145,7 @@
>  };
> 
>  &enet0 {
> -	tbi-handle = <&tbi1>;
> +	tbi-handle = <&tbi0>;
>  	phy-handle = <&sgmii_phy2>;
>  	phy-connection-type = "sgmii";
>  	status = "okay";
> @@ -225,6 +225,13 @@
>  	sgmii_phy2: ethernet-phy@2 {
>  		reg = <0x2>;
>  	};
> +	tbi0: tbi-phy@1f {
> +		reg = <0x1f>;
> +		device_type = "tbi-phy";
> +	};
> +};
> +
> +&mdio1 {
>  	tbi1: tbi-phy@1f {
>  		reg = <0x1f>;
>  		device_type = "tbi-phy";
> diff --git a/arch/arm/boot/dts/ls1021a.dtsi
> b/arch/arm/boot/dts/ls1021a.dtsi index b4f2723..3a3d264 100644
> --- a/arch/arm/boot/dts/ls1021a.dtsi
> +++ b/arch/arm/boot/dts/ls1021a.dtsi
> @@ -709,6 +709,15 @@
>  			      <0x0 0x2d10030 0x0 0x4>;
>  		};
> 
> +		mdio1: mdio@2d64000 {
> +			compatible = "gianfar";

I know it is not your fault to use this compatible string as it already used this for mdio node in the device tree, but it is somewhat ambiguous to use the same compatible string as the ethernet controller.  And this dts is the only one in all kernel device trees using this compatible string for mdio node.  I would think it will be more clear to use "fsl,etsec2-mdio" or "fsl,etsec2-tbi".

> +			device_type = "mdio";
> +			#address-cells = <1>;
> +			#size-cells = <0>;
> +			reg = <0x0 0x2d64000 0x0 0x4000>,
> +			      <0x0 0x2d50030 0x0 0x4>;
> +		};
> +
>  		ptp_clock@2d10e00 {
>  			compatible = "fsl,etsec-ptp";
>  			reg = <0x0 0x2d10e00 0x0 0xb0>;
> --
> 2.7.4