| 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: <Y+Yhd2Hou/kZkU1o@shell.armlinux.org.uk>
Date: Fri, 10 Feb 2023 10:50:31 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Daniel Golle <daniel@...rotopia.org>
Cc: netdev@...r.kernel.org, linux-mediatek@...ts.infradead.org,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Heiner Kallweit <hkallweit1@...il.com>,
Lorenzo Bianconi <lorenzo@...nel.org>,
Mark Lee <Mark-MC.Lee@...iatek.com>,
John Crispin <john@...ozen.org>, Felix Fietkau <nbd@....name>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>,
Matthias Brugger <matthias.bgg@...il.com>,
DENG Qingfang <dqfext@...il.com>,
Landen Chao <Landen.Chao@...iatek.com>,
Sean Wang <sean.wang@...iatek.com>,
Paolo Abeni <pabeni@...hat.com>,
Jakub Kicinski <kuba@...nel.org>,
Eric Dumazet <edumazet@...gle.com>,
"David S. Miller" <davem@...emloft.net>,
Vladimir Oltean <olteanv@...il.com>,
Florian Fainelli <f.fainelli@...il.com>,
Andrew Lunn <andrew@...n.ch>,
Jianhui Zhao <zhaojh329@...il.com>,
Bjørn Mork <bjorn@...k.no>
Subject: Re: [PATCH v2 09/11] net: pcs: add driver for MediaTek SGMII PCS
On Tue, Feb 07, 2023 at 02:23:08PM +0000, Daniel Golle wrote:
> +config PCS_MTK_LYNXI
> + tristate
> + select PHYLINK
Does this need to select PHYLINK? If the user of this doesn't already
select phylink, then phylink_create() won't be called and thus trying
to use PCS_MTK_LYNXI becomes impossible. I know PCS_XPCS does, none of
the others do though.
> + select REGMAP
> + help
> + This module provides helpers to phylink for managing the LynxI PCS
> + which is part of MediaTek's SoC and Ethernet switch ICs.
> +
> config PCS_RZN1_MIIC
> tristate "Renesas RZ/N1 MII converter"
> depends on OF && (ARCH_RZN1 || COMPILE_TEST)
> diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
> index 4c780d8f2e98..9b9afd6b1c22 100644
> --- a/drivers/net/pcs/Makefile
> +++ b/drivers/net/pcs/Makefile
> @@ -5,5 +5,6 @@ pcs_xpcs-$(CONFIG_PCS_XPCS) := pcs-xpcs.o pcs-xpcs-nxp.o
>
> obj-$(CONFIG_PCS_XPCS) += pcs_xpcs.o
> obj-$(CONFIG_PCS_LYNX) += pcs-lynx.o
> +obj-$(CONFIG_PCS_MTK_LYNXI) += pcs-mtk-lynxi.o
> obj-$(CONFIG_PCS_RZN1_MIIC) += pcs-rzn1-miic.o
> obj-$(CONFIG_PCS_ALTERA_TSE) += pcs-altera-tse.o
> diff --git a/drivers/net/pcs/pcs-mtk-lynxi.c b/drivers/net/pcs/pcs-mtk-lynxi.c
> new file mode 100644
> index 000000000000..0100def53d45
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-mtk-lynxi.c
> @@ -0,0 +1,315 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2018-2019 MediaTek Inc.
> +/* A library for MediaTek SGMII circuit
> + *
> + * Author: Sean Wang <sean.wang@...iatek.com>
> + * Author: Daniel Golle <daniel@...rotopia.org>
> + *
> + */
> +#include <linux/mdio.h>
> +#include <linux/phylink.h>
> +#include <linux/pcs/pcs-mtk-lynxi.h>
> +#include <linux/of.h>
> +#include <linux/phylink.h>
> +#include <linux/regmap.h>
> +
> +/* SGMII subsystem config registers */
> +/* BMCR (low 16) BMSR (high 16) */
> +#define SGMSYS_PCS_CONTROL_1 0x0
> +#define SGMII_BMCR GENMASK(15, 0)
> +#define SGMII_BMSR GENMASK(31, 16)
> +#define SGMII_AN_RESTART BIT(9)
> +#define SGMII_ISOLATE BIT(10)
> +#define SGMII_AN_ENABLE BIT(12)
Not really a review comment but a question: would it be sensible to
define these as:
#define SGMII_AN_RESTART BMCR_ANRESTART
etc, since they follow the IEEE802.3 clause 22 register layout?
> +#define SGMII_LINK_STATYS BIT(18)
> +#define SGMII_AN_ABILITY BIT(19)
> +#define SGMII_AN_COMPLETE BIT(21)
These also correspond to BMSR bits (<<16).
> +#define SGMII_PCS_FAULT BIT(23)
> +#define SGMII_AN_EXPANSION_CLR BIT(30)
This define doesn't seem to be used.
> +
> +#define SGMSYS_PCS_DEVICE_ID 0x4
> +#define SGMII_LYNXI_DEV_ID 0x4d544950
> +
> +#define SGMSYS_PCS_ADVERTISE 0x8
> +#define SGMII_ADVERTISE GENMASK(15, 0)
> +#define SGMII_LPA GENMASK(31, 16)
> +
> +#define SGMSYS_PCS_SCRATCH 0x14
> +#define SGMII_DEV_VERSION GENMASK(31, 16)
> +
> +/* Register to programmable link timer, the unit in 2 * 8ns */
> +#define SGMSYS_PCS_LINK_TIMER 0x18
> +#define SGMII_LINK_TIMER_MASK GENMASK(19, 0)
> +#define SGMII_LINK_TIMER_DEFAULT (0x186a0 & SGMII_LINK_TIMER_MASK)
We no longer make use of SGMII_LINK_TIMER_DEFAULT, so this can be
removed.
> +
> +/* Register to control remote fault */
> +#define SGMSYS_SGMII_MODE 0x20
> +#define SGMII_IF_MODE_SGMII BIT(0)
> +#define SGMII_SPEED_DUPLEX_AN BIT(1)
> +#define SGMII_SPEED_MASK GENMASK(3, 2)
> +#define SGMII_SPEED_10 FIELD_PREP(SGMII_SPEED_MASK, 0)
> +#define SGMII_SPEED_100 FIELD_PREP(SGMII_SPEED_MASK, 1)
> +#define SGMII_SPEED_1000 FIELD_PREP(SGMII_SPEED_MASK, 2)
> +#define SGMII_DUPLEX_HALF BIT(4)
> +#define SGMII_REMOTE_FAULT_DIS BIT(8)
> +#define SGMII_CODE_SYNC_SET_VAL BIT(9)
> +#define SGMII_CODE_SYNC_SET_EN BIT(10)
> +#define SGMII_SEND_AN_ERROR_EN BIT(11)
These three don't appear to be used.
> +
> +/* Register to reset SGMII design */
> +#define SGMII_RESERVED_0 0x34
> +#define SGMII_SW_RESET BIT(0)
> +
> +/* Register to set SGMII speed, ANA RG_ Control Signals III */
> +#define SGMSYS_ANA_RG_CS3 0x2028
SGMSYS_ANA_RG_CS3 isn't used here, although its register bits below
are.
> +#define RG_PHY_SPEED_MASK (BIT(2) | BIT(3))
> +#define RG_PHY_SPEED_1_25G 0x0
> +#define RG_PHY_SPEED_3_125G BIT(2)
> +
...
> +struct mtk_pcs_lynxi {
> + struct regmap *regmap;
> + struct device *dev;
I can only find one place that this is written to in this patch, it
seems its otherwise never used. Do we need it?
Other than that, I don't see anything else to comment on that hasn't
been mentioned in previous patches. Thanks!
--
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