Message-ID: <Y+JgkvmhiC8aL4hG@makrotopia.org>
Date:   Tue, 7 Feb 2023 14:30:42 +0000
From:   Daniel Golle <daniel@...rotopia.org>
To:     netdev@...r.kernel.org, linux-mediatek@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Russell King <linux@...linux.org.uk>,
        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>
Cc:     Jianhui Zhao <zhaojh329@...il.com>,
        Bjørn Mork <bjorn@...k.no>
Subject: Re: [PATCH v2 10/11] net: ethernet: mtk_eth_soc: switch to external
 PCS driver

On Tue, Feb 07, 2023 at 02:23:48PM +0000, Daniel Golle wrote:
> Now that we got a PCS driver, use it and remove the now redundant
> PCS code and it's header macros from the Ethernet driver.
> 
> Signed-off-by: Daniel Golle <daniel@...rotopia.org>
> ---
>  drivers/net/ethernet/mediatek/Kconfig       |   2 +
>  drivers/net/ethernet/mediatek/mtk_eth_soc.c |  13 +-
>  drivers/net/ethernet/mediatek/mtk_eth_soc.h |  80 +-------
>  drivers/net/ethernet/mediatek/mtk_sgmii.c   | 202 +++-----------------
>  4 files changed, 38 insertions(+), 259 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/Kconfig b/drivers/net/ethernet/mediatek/Kconfig
> index 97374fb3ee79..da0db417ab69 100644
> --- a/drivers/net/ethernet/mediatek/Kconfig
> +++ b/drivers/net/ethernet/mediatek/Kconfig
> @@ -19,6 +19,8 @@ config NET_MEDIATEK_SOC
>  	select DIMLIB
>  	select PAGE_POOL
>  	select PAGE_POOL_STATS
> +	select PCS_MTK_LYNXI
> +	select REGMAP_MMIO
>  	help
>  	  This driver supports the gigabit ethernet MACs in the
>  	  MediaTek SoC family.
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> index 97df77c7999e..54e85f54d7fc 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c
> @@ -4077,6 +4077,7 @@ static int mtk_unreg_dev(struct mtk_eth *eth)
>  
>  static int mtk_cleanup(struct mtk_eth *eth)
>  {
> +	mtk_sgmii_destroy(eth->sgmii);
>  	mtk_unreg_dev(eth);
>  	mtk_free_dev(eth);
>  	cancel_work_sync(&eth->pending_work);
> @@ -4580,6 +4581,7 @@ static int mtk_probe(struct platform_device *pdev)
>  		if (!eth->sgmii)
>  			return -ENOMEM;
>  
> +		eth->sgmii->dev = eth->dev;
>  		err = mtk_sgmii_init(eth->sgmii, pdev->dev.of_node,
>  				     eth->soc->ana_rgc3);
>  
> @@ -4592,14 +4594,17 @@ static int mtk_probe(struct platform_device *pdev)
>  							    "mediatek,pctl");
>  		if (IS_ERR(eth->pctl)) {
>  			dev_err(&pdev->dev, "no pctl regmap found\n");
> -			return PTR_ERR(eth->pctl);
> +			err = PTR_ERR(eth->pctl);
> +			goto err_destroy_sgmii;
>  		}
>  	}
>  
>  	if (MTK_HAS_CAPS(eth->soc->caps, MTK_NETSYS_V2)) {
>  		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -		if (!res)
> -			return -EINVAL;
> +		if (!res) {
> +			err = -EINVAL;
> +			goto err_destroy_sgmii;
> +		}
>  	}
>  
>  	if (eth->soc->offload_version) {
> @@ -4749,6 +4754,8 @@ static int mtk_probe(struct platform_device *pdev)
>  
>  	return 0;
>  
> +err_destroy_sgmii:
> +	mtk_sgmii_destroy(eth->sgmii);
>  err_deinit_ppe:
>  	mtk_ppe_deinit(eth);
>  	mtk_mdio_cleanup(eth);
> diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.h b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> index c2e0fd773cc2..a72748d80bba 100644
> --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.h
> @@ -510,65 +510,6 @@
>  #define ETHSYS_DMA_AG_MAP_QDMA	BIT(1)
>  #define ETHSYS_DMA_AG_MAP_PPE	BIT(2)
>  
> -/* 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)
> -#define SGMII_LINK_STATYS	BIT(18)
> -#define SGMII_AN_ABILITY	BIT(19)
> -#define SGMII_AN_COMPLETE	BIT(21)
> -#define SGMII_PCS_FAULT		BIT(23)
> -#define SGMII_AN_EXPANSION_CLR	BIT(30)
> -
> -#define SGMSYS_PCS_ADVERTISE	0x8
> -#define SGMII_ADVERTISE		GENMASK(15, 0)
> -#define SGMII_LPA		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)
> -
> -/* 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_IF_MODE_BIT5		BIT(5)
> -#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)
> -#define SGMII_IF_MODE_MASK		GENMASK(5, 1)
> -
> -/* 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
> -#define RG_PHY_SPEED_MASK	(BIT(2) | BIT(3))
> -#define RG_PHY_SPEED_1_25G	0x0
> -#define RG_PHY_SPEED_3_125G	BIT(2)
> -
> -/* Register to power up QPHY */
> -#define SGMSYS_QPHY_PWR_STATE_CTRL 0xe8
> -#define	SGMII_PHYA_PWD		BIT(4)
> -
> -/* Register to QPHY wrapper control */
> -#define SGMSYS_QPHY_WRAP_CTRL	0xec
> -#define SGMII_PN_SWAP_MASK	GENMASK(1, 0)
> -#define SGMII_PN_SWAP_TX_RX	(BIT(0) | BIT(1))
> -#define MTK_SGMII_FLAG_PN_SWAP	BIT(0)
> -
>  /* Infrasys subsystem config registers */
>  #define INFRA_MISC2            0x70c
>  #define CO_QPHY_SEL            BIT(0)
> @@ -1103,29 +1044,13 @@ struct mtk_soc_data {
>  /* currently no SoC has more than 2 macs */
>  #define MTK_MAX_DEVS			2
>  
> -/* struct mtk_pcs -    This structure holds each sgmii regmap and associated
> - *                     data
> - * @regmap:            The register map pointing at the range used to setup
> - *                     SGMII modes
> - * @ana_rgc3:          The offset refers to register ANA_RGC3 related to regmap
> - * @interface:         Currently configured interface mode
> - * @pcs:               Phylink PCS structure
> - * @flags:             Flags indicating hardware properties
> - */
> -struct mtk_pcs {
> -	struct regmap	*regmap;
> -	u32             ana_rgc3;
> -	phy_interface_t	interface;
> -	struct phylink_pcs pcs;
> -	u32		flags;
> -};
> -
>  /* struct mtk_sgmii -  This is the structure holding sgmii regmap and its
>   *                     characteristics
>   * @pcs                Array of individual PCS structures
>   */
>  struct mtk_sgmii {
> -	struct mtk_pcs	pcs[MTK_MAX_DEVS];
> +	struct phylink_pcs *pcs[MTK_MAX_DEVS];
> +	struct device *dev;
>  };
>  
>  /* struct mtk_eth -	This is the main datasructure for holding the state
> @@ -1353,6 +1278,7 @@ u32 mtk_r32(struct mtk_eth *eth, unsigned reg);
>  struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id);
>  int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *np,
>  		   u32 ana_rgc3);
> +void mtk_sgmii_destroy(struct mtk_sgmii *ss);
>  
>  int mtk_gmac_sgmii_path_setup(struct mtk_eth *eth, int mac_id);
>  int mtk_gmac_gephy_path_setup(struct mtk_eth *eth, int mac_id);
> diff --git a/drivers/net/ethernet/mediatek/mtk_sgmii.c b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> index 9c58006d1c33..d4b428e23cfc 100644
> --- a/drivers/net/ethernet/mediatek/mtk_sgmii.c
> +++ b/drivers/net/ethernet/mediatek/mtk_sgmii.c
> @@ -10,199 +10,35 @@
>  #include <linux/mfd/syscon.h>
>  #include <linux/of.h>
>  #include <linux/phylink.h>
> +#include <linux/pcs/pcs-mtk-lynxi.h>
>  #include <linux/regmap.h>
>  
>  #include "mtk_eth_soc.h"
>  
> -static struct mtk_pcs *pcs_to_mtk_pcs(struct phylink_pcs *pcs)
> -{
> -	return container_of(pcs, struct mtk_pcs, pcs);
> -}
> -
> -static void mtk_pcs_get_state(struct phylink_pcs *pcs,
> -			      struct phylink_link_state *state)
> -{
> -	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
> -	unsigned int bm, adv;
> -
> -	/* Read the BMSR and LPA */
> -	regmap_read(mpcs->regmap, SGMSYS_PCS_CONTROL_1, &bm);
> -	regmap_read(mpcs->regmap, SGMSYS_PCS_ADVERTISE, &adv);
> -
> -	phylink_mii_c22_pcs_decode_state(state, FIELD_GET(SGMII_BMSR, bm),
> -					 FIELD_GET(SGMII_LPA, adv));
> -}
> -
> -static int mtk_pcs_config(struct phylink_pcs *pcs, unsigned int mode,
> -			  phy_interface_t interface,
> -			  const unsigned long *advertising,
> -			  bool permit_pause_to_mac)
> -{
> -	bool mode_changed = false, changed, use_an;
> -	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
> -	unsigned int rgc3, sgm_mode, bmcr;
> -	int advertise, link_timer;
> -
> -	advertise = phylink_mii_c22_pcs_encode_advertisement(interface,
> -							     advertising);
> -	if (advertise < 0)
> -		return advertise;
> -
> -	/* Clearing IF_MODE_BIT0 switches the PCS to BASE-X mode, and
> -	 * we assume that fixes it's speed at bitrate = line rate (in
> -	 * other words, 1000Mbps or 2500Mbps).
> -	 */
> -	if (interface == PHY_INTERFACE_MODE_SGMII) {
> -		sgm_mode = SGMII_IF_MODE_SGMII;
> -		if (phylink_autoneg_inband(mode)) {
> -			sgm_mode |= SGMII_REMOTE_FAULT_DIS |
> -				    SGMII_SPEED_DUPLEX_AN;
> -			use_an = true;
> -		} else {
> -			use_an = false;
> -		}
> -	} else if (phylink_autoneg_inband(mode)) {
> -		/* 1000base-X or 2500base-X autoneg */
> -		sgm_mode = SGMII_REMOTE_FAULT_DIS;
> -		use_an = linkmode_test_bit(ETHTOOL_LINK_MODE_Autoneg_BIT,
> -					   advertising);
> -	} else {
> -		/* 1000base-X or 2500base-X without autoneg */
> -		sgm_mode = 0;
> -		use_an = false;
> -	}
> -
> -	if (use_an) {
> -		bmcr = SGMII_AN_ENABLE;
> -	} else {
> -		bmcr = 0;
> -	}
> -
> -	if (mpcs->interface != interface) {
> -		/* PHYA power down */
> -		regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL,
> -				   SGMII_PHYA_PWD, SGMII_PHYA_PWD);
> -
> -		/* Reset SGMII PCS state */
> -		regmap_update_bits(mpcs->regmap, SGMII_RESERVED_0,
> -				   SGMII_SW_RESET, SGMII_SW_RESET);
> -
> -		if (mpcs->flags & MTK_SGMII_FLAG_PN_SWAP)
> -			regmap_update_bits(mpcs->regmap, SGMSYS_QPHY_WRAP_CTRL,
> -					   SGMII_PN_SWAP_MASK,
> -					   SGMII_PN_SWAP_TX_RX);
> -
> -		if (interface == PHY_INTERFACE_MODE_2500BASEX)
> -			rgc3 = RG_PHY_SPEED_3_125G;
> -		else
> -			rgc3 = 0;
> -
> -		/* Configure the underlying interface speed */
> -		regmap_update_bits(mpcs->regmap, mpcs->ana_rgc3,
> -				   RG_PHY_SPEED_3_125G, rgc3);
> -
> -		/* Setup the link timer and QPHY power up inside SGMIISYS */
> -		link_timer = phylink_get_link_timer_ns(interface);
> -		if (link_timer < 0)
> -			return link_timer;
> -
> -		regmap_write(mpcs->regmap, SGMSYS_PCS_LINK_TIMER, link_timer / 2 / 8);
> -
> -		mpcs->interface = interface;
> -		mode_changed = true;
> -	}
> -
> -	/* Update the advertisement, noting whether it has changed */
> -	regmap_update_bits_check(mpcs->regmap, SGMSYS_PCS_ADVERTISE,
> -				 SGMII_ADVERTISE, advertise, &changed);
> -
> -	/* Update the sgmsys mode register */
> -	regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
> -			   SGMII_REMOTE_FAULT_DIS | SGMII_SPEED_DUPLEX_AN |
> -			   SGMII_IF_MODE_SGMII, sgm_mode);
> -
> -	/* Update the BMCR */
> -	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
> -			   SGMII_AN_ENABLE, bmcr);
> -
> -	/* Release PHYA power down state
> -	 * Only removing bit SGMII_PHYA_PWD isn't enough.
> -	 * There are cases when the SGMII_PHYA_PWD register contains 0x9 which
> -	 * prevents SGMII from working. The SGMII still shows link but no traffic
> -	 * can flow. Writing 0x0 to the PHYA_PWD register fix the issue. 0x0 was
> -	 * taken from a good working state of the SGMII interface.
> -	 * Unknown how much the QPHY needs but it is racy without a sleep.
> -	 * Tested on mt7622 & mt7986.
> -	 */
> -	usleep_range(50, 100);
> -	regmap_write(mpcs->regmap, SGMSYS_QPHY_PWR_STATE_CTRL, 0);
> -
> -	return changed || mode_changed;
> -}
> -
> -static void mtk_pcs_restart_an(struct phylink_pcs *pcs)
> -{
> -	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
> -
> -	regmap_update_bits(mpcs->regmap, SGMSYS_PCS_CONTROL_1,
> -			   SGMII_AN_RESTART, SGMII_AN_RESTART);
> -}
> -
> -static void mtk_pcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
> -			    phy_interface_t interface, int speed, int duplex)
> -{
> -	struct mtk_pcs *mpcs = pcs_to_mtk_pcs(pcs);
> -	unsigned int sgm_mode;
> -
> -	if (!phylink_autoneg_inband(mode)) {
> -		/* Force the speed and duplex setting */
> -		if (speed == SPEED_10)
> -			sgm_mode = SGMII_SPEED_10;
> -		else if (speed == SPEED_100)
> -			sgm_mode = SGMII_SPEED_100;
> -		else
> -			sgm_mode = SGMII_SPEED_1000;
> -
> -		if (duplex != DUPLEX_FULL)
> -			sgm_mode |= SGMII_DUPLEX_HALF;
> -
> -		regmap_update_bits(mpcs->regmap, SGMSYS_SGMII_MODE,
> -				   SGMII_DUPLEX_HALF | SGMII_SPEED_MASK,
> -				   sgm_mode);
> -	}
> -}
> -
> -static const struct phylink_pcs_ops mtk_pcs_ops = {
> -	.pcs_get_state = mtk_pcs_get_state,
> -	.pcs_config = mtk_pcs_config,
> -	.pcs_an_restart = mtk_pcs_restart_an,
> -	.pcs_link_up = mtk_pcs_link_up,
> -};
> -
>  int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
>  {
>  	struct device_node *np;
> +	struct regmap *regmap;
> +	u32 flags;
>  	int i;
>  
> -	for (i = 0; i < MTK_MAX_DEVS; i++) {
> +	for (i = 0; id < MTK_MAX_DEVS; i++) {

This change was unintentional and will break the build. I've fixed it
for my test builds, but forgot to commit it before sending out the series.
Please discard for now.

>  		np = of_parse_phandle(r, "mediatek,sgmiisys", i);
>  		if (!np)
>  			break;
>  
> -		ss->pcs[i].ana_rgc3 = ana_rgc3;
> -		ss->pcs[i].regmap = syscon_node_to_regmap(np);
> -
> -		ss->pcs[i].flags = 0;
> +		regmap = syscon_node_to_regmap(np);
> +		flags = 0;
>  		if (of_property_read_bool(np, "mediatek,pn_swap"))
> -			ss->pcs[i].flags |= MTK_SGMII_FLAG_PN_SWAP;
> +			flags |= MTK_SGMII_FLAG_PN_SWAP;
>  
>  		of_node_put(np);
> -		if (IS_ERR(ss->pcs[i].regmap))
> -			return PTR_ERR(ss->pcs[i].regmap);
>  
> -		ss->pcs[i].pcs.ops = &mtk_pcs_ops;
> -		ss->pcs[i].pcs.poll = true;
> -		ss->pcs[i].interface = PHY_INTERFACE_MODE_NA;
> +		if (IS_ERR(regmap))
> +			return PTR_ERR(regmap);
> +
> +		ss->pcs[i] = mtk_pcs_lynxi_create(ss->dev, regmap, ana_rgc3,
> +						  flags);
>  	}
>  
>  	return 0;
> @@ -210,8 +46,16 @@ int mtk_sgmii_init(struct mtk_sgmii *ss, struct device_node *r, u32 ana_rgc3)
>  
>  struct phylink_pcs *mtk_sgmii_select_pcs(struct mtk_sgmii *ss, int id)
>  {
> -	if (!ss->pcs[id].regmap)
> -		return NULL;
> +	return ss->pcs[id];
> +}
> +
> +void mtk_sgmii_destroy(struct mtk_sgmii *ss)
> +{
> +	int i;
> +
> +	if (!ss)
> +		return;
>  
> -	return &ss->pcs[id].pcs;
> +	for (i = 0; i < MTK_MAX_DEVS; i++)
> +		mtk_pcs_lynxi_destroy(ss->pcs[i]);
>  }
> -- 
> 2.39.1
> 
> 

Powered by blists - more mailing lists