| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZOT/aIelVomIzdZk@shell.armlinux.org.uk>
Date: Tue, 22 Aug 2023 19:33:12 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Daniel Golle <daniel@...rotopia.org>
Cc: Felix Fietkau <nbd@....name>, John Crispin <john@...ozen.org>,
Sean Wang <sean.wang@...iatek.com>,
Mark Lee <Mark-MC.Lee@...iatek.com>,
SkyLake Huang <SkyLake.Huang@...iatek.com>,
Bc-bocun Chen <bc-bocun.chen@...iatek.com>,
Henry Yen <Henry.Yen@...iatek.com>,
Lorenzo Bianconi <lorenzo@...nel.org>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Matthias Brugger <matthias.bgg@...il.com>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>,
linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
linux-arm-kernel@...ts.infradead.org,
linux-mediatek@...ts.infradead.org
Subject: Re: [PATCH RFC net-next] net: ethernet: mtk_eth_soc: add paths and
SerDes modes for MT7988
Hi,
I haven't fully reviewed this yet, but here's some initial comments.
On Tue, Aug 22, 2023 at 07:04:42PM +0100, Daniel Golle wrote:
> MT7988 comes with a built-in 2.5G PHY as well as SerDes lanes to
> connect external PHYs or transceivers in USXGMII, 10GBase-R, 5GBase-KR,
5Gbase-KR K normally means backplane mode.
> +/* Register to read PCS AN status */
> +#define RG_PCS_AN_STS0 0x81c
> +#define USXGMII_LPA_SPEED_MASK GENMASK(11, 9)
> +#define USXGMII_LPA_SPEED_10 0
> +#define USXGMII_LPA_SPEED_100 1
> +#define USXGMII_LPA_SPEED_1000 2
> +#define USXGMII_LPA_SPEED_10000 3
> +#define USXGMII_LPA_SPEED_2500 4
> +#define USXGMII_LPA_SPEED_5000 5
> +#define USXGMII_LPA_DUPLEX BIT(12)
> +#define USXGMII_LPA_LINK BIT(15)
Aren't these the same as the MDIO_USXGMII_xxx definitions that define
the UsxgmiiChannelInfo[15:0] bits?
> +static int mtk_usxgmii_pcs_config(struct phylink_pcs *pcs, unsigned int neg_mode,
> + phy_interface_t interface,
> + const unsigned long *advertising,
> + bool permit_pause_to_mac)
> +{
> + struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
> + struct mtk_eth *eth = mpcs->eth;
> + struct regmap *pextp = eth->regmap_pextp[mpcs->id];
> + unsigned int an_ctrl = 0, link_timer = 0, xfi_mode = 0, adapt_mode = 0;
> + bool mode_changed = false;
> +
> + if (!pextp)
> + return -ENODEV;
> +
> + if (interface == PHY_INTERFACE_MODE_USXGMII) {
> + an_ctrl = FIELD_PREP(USXGMII_AN_SYNC_CNT, 0x1FF) |
> + (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) ?
Using the new neg_mode here... but you haven't set pcs.neg_mode below
true.
> +static void mtk_usxgmii_pcs_get_state(struct phylink_pcs *pcs,
> + struct phylink_link_state *state)
> +{
> + struct mtk_usxgmii_pcs *mpcs = pcs_to_mtk_usxgmii_pcs(pcs);
> + struct mtk_eth *eth = mpcs->eth;
> + struct mtk_mac *mac = eth->mac[mtk_xgmii2mac_id(eth, mpcs->id)];
> + u32 val = 0;
> +
> + regmap_read(mpcs->regmap, RG_PCS_AN_CTRL0, &val);
> + if (FIELD_GET(USXGMII_AN_ENABLE, val)) {
> + /* Refresh LPA by inverting LPA_LATCH */
> + regmap_read(mpcs->regmap, RG_PCS_AN_STS0, &val);
> + regmap_update_bits(mpcs->regmap, RG_PCS_AN_STS0,
> + USXGMII_LPA_LATCH,
> + !(val & USXGMII_LPA_LATCH));
> +
> + regmap_read(mpcs->regmap, RG_PCS_AN_STS0, &val);
> +
> + state->interface = mpcs->interface;
> + state->link = FIELD_GET(USXGMII_LPA_LINK, val);
> + state->duplex = FIELD_GET(USXGMII_LPA_DUPLEX, val);
> +
> + switch (FIELD_GET(USXGMII_LPA_SPEED_MASK, val)) {
> + case USXGMII_LPA_SPEED_10:
> + state->speed = SPEED_10;
> + break;
> + case USXGMII_LPA_SPEED_100:
> + state->speed = SPEED_100;
> + break;
> + case USXGMII_LPA_SPEED_1000:
> + state->speed = SPEED_1000;
> + break;
> + case USXGMII_LPA_SPEED_2500:
> + state->speed = SPEED_2500;
> + break;
> + case USXGMII_LPA_SPEED_5000:
> + state->speed = SPEED_5000;
> + break;
> + case USXGMII_LPA_SPEED_10000:
> + state->speed = SPEED_10000;
> + break;
> + }
Maybe consider using phylink_decode_usxgmii_word() ?
> + } else {
> + val = mtk_r32(mac->hw, MTK_XGMAC_STS(mac->id));
> +
> + if (mac->id == MTK_GMAC2_ID)
> + val >>= 16;
> +
> + switch (FIELD_GET(MTK_USXGMII_PCS_MODE, val)) {
> + case 0:
> + state->speed = SPEED_10000;
> + break;
> + case 1:
> + state->speed = SPEED_5000;
> + break;
> + case 2:
> + state->speed = SPEED_2500;
> + break;
> + case 3:
> + state->speed = SPEED_1000;
> + break;
> + }
> +
> + state->interface = mpcs->interface;
> + state->link = FIELD_GET(MTK_USXGMII_PCS_LINK, val);
> + state->duplex = DUPLEX_FULL;
> + }
> +
> + if (state->link == 0)
> + mtk_usxgmii_pcs_config(pcs, MLO_AN_INBAND,
Passing old-style mode rather than neg_mode here.
> + state->interface, NULL, false);
It would be good to describe why you're reconfiguring the interface
here. You describe why in mtk_usxgmii_pcs_link_up...
> + eth->usxgmii_pcs[i] = devm_kzalloc(dev, sizeof(*eth->usxgmii_pcs), GFP_KERNEL);
> + if (!eth->usxgmii_pcs[i])
> + return -ENOMEM;
> +
> + eth->usxgmii_pcs[i]->id = i;
> + eth->usxgmii_pcs[i]->eth = eth;
> + eth->usxgmii_pcs[i]->regmap = syscon_node_to_regmap(np);
> + if (IS_ERR(eth->usxgmii_pcs[i]->regmap))
> + return PTR_ERR(eth->usxgmii_pcs[i]->regmap);
> +
> + eth->usxgmii_pcs[i]->pcs.ops = &mtk_usxgmii_pcs_ops;
> + eth->usxgmii_pcs[i]->pcs.poll = true;
> + eth->usxgmii_pcs[i]->interface = PHY_INTERFACE_MODE_NA;
All new PCS should set pcs.neg_mode true.
Thanks.
--
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