Messages in this thread Patch in this message | | | Date | Fri, 10 Feb 2023 10:50:31 +0000 | From | "Russell King (Oracle)" <> | 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@mediatek.com> > + * Author: Daniel Golle <daniel@makrotopia.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!
| |