Re: [PATCH 4.19 320/639] ARM: dts: ls1021: Fix SGMII PCS link remaining down after PHY disconnect

[Vladimir Oltean]

Hi Pavel,

On Sat, 25 Jan 2020 at 21:13, Pavel Machek <pavel@xxxxxxx> 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