Hi Pavel,
On Sat, 25 Jan 2020 at 21:13, Pavel Machek pavel@denx.de wrote:
Hi!
[ Upstream commit c7861adbe37f576931650ad8ef805e0c47564b9a ]
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'.
Also, it changes compatible of mdio0 to "fsl,etsec2-mdio" to match mdio1 device.
It actually changes compatible of both devices.
+++ b/arch/arm/boot/dts/ls1021a.dtsi @@ -584,7 +584,7 @@ };
mdio0: mdio@2d24000 {
compatible = "gianfar";
compatible = "fsl,etsec2-mdio"; device_type = "mdio"; #address-cells = <1>; #size-cells = <0>;@@ -592,6 +592,15 @@ <0x0 0x2d10030 0x0 0x4>; };
mdio1: mdio@2d64000 {compatible = "fsl,etsec2-mdio";And they trigger different code in the driver:
.type = "mdio", .compatible = "gianfar", .data = &(struct fsl_pq_mdio_data) { ... .get_tbipa = get_gfar_tbipa_from_mdio, }, .compatible = "fsl,etsec2-mdio", .data = &(struct fsl_pq_mdio_data) { ... .get_tbipa = get_etsec_tbipa, },Are you sure that is good idea for both mainline and stable?
Thanks for spotting this.
What has happened is that [ Leo ] Li Yang suggested me to change the compatible in v1 of this patch here: https://patchwork.ozlabs.org/patch/1064015/
Not having any argument to oppose (and not much experience, to be frank) I complied and sent out a 2-patch v2 series: https://patchwork.ozlabs.org/patch/1084366/ https://patchwork.ozlabs.org/patch/1084365/
And Shawn squashed them when merging them, "to get it land as fix a bit easier".
Judging the code in more detail, you are indeed correct that the "gianfar" compatible was the right one for this hardware. The difference being the "get_tbipa" function which calculates the address of the TBIPA register automatically, if not explicitly specified. However, for ls1021a.dtsi, the TBIPA register _is_ explicitly specified via the second "reg" (<0x0 0x2d10030 0x0 0x4>), so the "get_tbipa" function is dead code for LS1021A. Therefore, luckily no harm was done.
I would suggest that this patch continues to be applied as-is to the stable kernels, just for the sake of not having divergent patches across branches, and I'll send a new one that turns the compatible back into "gianfar".
Best regards, Pavel -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Thanks, -Vladimir