[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <DM4PR11MB609568BF8AE50B4506C6A3CDCAAB9@DM4PR11MB6095.namprd11.prod.outlook.com>
Date: Mon, 13 Jun 2022 23:44:35 +0000
From: "Ong, Boon Leong" <boon.leong.ong@...el.com>
To: Russell King <linux@...linux.org.uk>
CC: Alexandre Torgue <alexandre.torgue@...s.st.com>,
Jose Abreu <Jose.Abreu@...opsys.com>,
Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>,
Paolo Abeni <pabeni@...hat.com>,
"David S . Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Vladimir Oltean <olteanv@...il.com>,
Vivien Didelot <vivien.didelot@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
"Maxime Coquelin" <mcoquelin.stm32@...il.com>,
Giuseppe Cavallaro <peppe.cavallaro@...com>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
"linux-stm32@...md-mailman.stormreply.com"
<linux-stm32@...md-mailman.stormreply.com>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"Riva, Emilio" <emilio.riva@...csson.com>
Subject: RE: [PATCH net-next v2 3/6] net: pcs: xpcs: add CL37 1000BASE-X AN
support
>> +static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
>> + struct phylink_link_state *state)
>> +{
>> + int lpa, adv;
>> + int ret;
>> +
>> + if (state->an_enabled) {
>> + /* Reset link state */
>> + state->link = false;
>> +
>> + lpa = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_LPA);
>> + if (lpa < 0 || lpa & LPA_RFAULT)
>> + return lpa;
>> +
>> + adv = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_ADVERTISE);
>> + if (adv < 0)
>> + return adv;
>> +
>> + if (lpa & ADVERTISE_1000XFULL &&
>> + adv & ADVERTISE_1000XFULL) {
>> + state->link = true;
>> + state->speed = SPEED_1000;
>> + state->duplex = DUPLEX_FULL;
>> + }
>
>phylink_mii_c22_pcs_decode_state() is your friend here, will implement
>this correctly, and will set lp_advertising correctly as well.
Thank for the suggestion. I will change it accordingly to use
phylink_mii_c22_pcs_decode_state()
>
>> +
>> + /* Clear CL37 AN complete status */
>> + ret = xpcs_write(xpcs, MDIO_MMD_VEND2,
>DW_VR_MII_AN_INTR_STS, 0);
>> + if (ret < 0)
>> + return ret;
>
>Why do you need to clear the interrupt status here? This function will
>be called from a work queue sometime later after an interrupt has fired.
>It will also be called at random times when userspace enquires what the
>link parameters are, so clearing the interrupt here can result in lost
>link changes.
Thanks for pointing it. Ya, it is unnecessary.
>
>> +static void xpcs_link_up_1000basex(struct dw_xpcs *xpcs, int speed,
>> + int duplex)
>> +{
>> + int val, ret;
>> +
>> + switch (speed) {
>> + case SPEED_1000:
>> + val = BMCR_SPEED1000;
>> + break;
>> + case SPEED_100:
>> + case SPEED_10:
>> + default:
>> + pr_err("%s: speed = %d\n", __func__, speed);
>> + return;
>> + }
>> +
>> + if (duplex == DUPLEX_FULL)
>> + val |= BMCR_FULLDPLX;
>> + else
>> + pr_err("%s: half duplex not supported\n", __func__);
>> +
>> + ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, val);
>> + if (ret)
>> + pr_err("%s: xpcs_write returned %pe\n", __func__,
>ERR_PTR(ret));
>
>Does this need to be done even when AN is enabled?
Thanks. Ya, it does not need. Will fix.
>
>--
>RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
>FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists