Message-ID: <BYAPR14MB291837DB503BF967249B1F15E34FA@BYAPR14MB2918.namprd14.prod.outlook.com>
Date:   Sat, 3 Jun 2023 04:56:26 +0000
From:   Michal Smulski <michal.smulski@...a.com>
To:     Russell King <linux@...linux.org.uk>,
        Ioana Ciornei <ioana.ciornei@....com>
CC:     "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "olteanv@...il.com" <olteanv@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "edumazet@...gle.com" <edumazet@...gle.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "pabeni@...hat.com" <pabeni@...hat.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "simon.horman@...igine.com" <simon.horman@...igine.com>,
        "kabel@...nel.org" <kabel@...nel.org>
Subject: RE: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII
 mode for mv88e6393x

The two registers are not documented in Marvell spec for this switch and that is why my original patch did not have link status code.  I have followed suggestion by Andrew's and used other Marvell hardware that has USXGMII to see some of the same registers and same address would be accessible this switch (best effort attempt). 
These two registers appear to be read-only and likely are (1) what the switch is advertising on USXMII link and (2) what the switch received (link partner advertisement).
I am able to distinguish which is which because I can look at the other side's of USXGMII interfaces which has well defined USXGMII registers.

During the boot sequence these registers show the following:
1. On initial setup of the serdes link (inside the switch):
Status=0x00, lp_status=0xff
2. When irq link status triggers (mv88e6393x_serdes_irq_link_10g() ):
Status=0xd601, lp_status=0x9781
3. the driver then reports link
mv88e6085 0x0000000008b96000:02: Link is Up - 10Gbps/Full - flow control off

Please note that Link Partner' MDIO_USXGMII_LINK bit is always 1 which makes it useless for catching when the link is achieved. That is why I use switch's side link bit to note the link is set. However, I can certainly add this to the patch.

My board is based on LX2162a (a version of LX2160a). Specifically, USXMII interface is configured as follows:

LX2162a (DPMAC13 configured as USXGMII)  <---USXGMII --> 88E6191X (port10 configured as USXGMII).

In fact, I am using pcs-lynx driver for LX2162a side of the interface.

Michal


-----Original Message-----
From: Russell King <linux@...linux.org.uk> 
Sent: Friday, June 2, 2023 3:31 AM
To: msmulski2@...il.com; Ioana Ciornei <ioana.ciornei@....com>
Cc: andrew@...n.ch; f.fainelli@...il.com; olteanv@...il.com; davem@...emloft.net; edumazet@...gle.com; kuba@...nel.org; pabeni@...hat.com; netdev@...r.kernel.org; linux-kernel@...r.kernel.org; simon.horman@...igine.com; kabel@...nel.org; Michal Smulski <michal.smulski@...a.com>
Subject: Re: [PATCH net-next v6 1/1] net: dsa: mv88e6xxx: implement USXGMII mode for mv88e6393x

CAUTION: This email is originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.


On Thu, Jun 01, 2023 at 05:17:04PM -0700, msmulski2@...il.com wrote:
> +/* USXGMII registers for Marvell switch 88e639x are undocumented and 
> +this function is based
> + * on some educated guesses. It appears that there are no status bits 
> +related to
> + * autonegotiation complete or flow control.
> + */
> +static int mv88e639x_serdes_pcs_get_state_usxgmii(struct mv88e6xxx_chip *chip,
> +                                               int port, int lane,
> +                                               struct 
> +phylink_link_state *state) {
> +     u16 status, lp_status;
> +     int err;
> +
> +     err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> +                                 MV88E6390_USXGMII_PHY_STATUS, &status);
> +     if (err) {
> +             dev_err(chip->dev, "can't read Serdes USXGMII PHY status: %d\n", err);
> +             return err;
> +     }
> +     dev_dbg(chip->dev, "USXGMII PHY status: 0x%x\n", status);
> +
> +     state->link = !!(status & MDIO_USXGMII_LINK);
> +
> +     if (state->link) {
> +             err = mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> +                                         MV88E6390_USXGMII_LP_STATUS, 
> + &lp_status);

What's the difference between these two registers? Specifically, what I'm asking is why the USXGMII partner status seems to be split between two separate registers.

Note that I think phylink_decode_usxgmii_word() is probably missing a check for MDIO_USXGMII_LINK, based on how Cisco SGMII works (and USXGMII is pretty similar.)

MDIO_USXGMII_LINK indicates whether the attached PHY has link on the media side or not. It's entirely possible for the USXGMII link to be up (thus allowing us to receive the "LPA" from the PHY) but for the PHY to be reporting to us that it has no media side link.

So, I think phylink_decode_usxgmii_word() at least needs at the beginning this added:

        if (!(lpa & MDIO_USXGMII_LINK)) {
                state->link = false;
                return;
        }

The only user of this has been the Lynx PCS, so I'll add Ioana to this email as well to see whether there's any objection from Lynx PCS users to adding it.

--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Powered by blists - more mailing lists