Re: [PATCH v2 09/11] net: pcs: add driver for MediaTek SGMII PCS

[Russell King (Oracle)]

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@xxxxxxxxxxxx>
> + * Author: Daniel Golle <daniel@xxxxxxxxxxxxxx>
> + *
> + */
> +#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!