Message-ID: <807db1e6-fd51-0add-90af-1813a5d47fab@seco.com>
Date:   Tue, 4 Oct 2022 11:25:24 -0400
From:   Sean Anderson <sean.anderson@...o.com>
To:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Madalin Bucur <madalin.bucur@....com>,
        Camelia Alexandra Groza <camelia.groza@....com>,
        netdev@...r.kernel.org
Cc:     Russell King <linux@...linux.org.uk>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>,
        "linuxppc-dev @ lists . ozlabs . org" <linuxppc-dev@...ts.ozlabs.org>
Subject: Re: [PATCH net-next v6 5/9] net: fman: memac: Use lynx pcs driver



On 9/30/22 4:09 PM, Sean Anderson wrote:
> Although not stated in the datasheet, as far as I can tell PCS for mEMACs
> is a "Lynx." By reusing the existing driver, we can remove the PCS
> management code from the memac driver. This requires calling some PCS
> functions manually which phylink would usually do for us, but we will let
> it do that soon.
> 
> One problem is that we don't actually have a PCS for QSGMII. We pretend
> that each mEMAC's MDIO bus has four QSGMII PCSs, but this is not the case.
> Only the "base" mEMAC's MDIO bus has the four QSGMII PCSs. This is not an
> issue yet, because we never get the PCS state. However, it will be once the
> conversion to phylink is complete, since the links will appear to never
> come up. To get around this, we allow specifying multiple PCSs in pcsphy.
> This breaks backwards compatibility with old device trees, but only for
> QSGMII. IMO this is the only reasonable way to figure out what the actual
> QSGMII PCS is.
> 
> Additionally, we now also support a separate XFI PCS. This can allow the
> SerDes driver to set different addresses for the SGMII and XFI PCSs so they
> can be accessed at the same time.
> 
> Signed-off-by: Sean Anderson <sean.anderson@...o.com>
> ---
> 
> Changes in v6:
> - Fix 81-character line
> 
> Changes in v3:
> - Put the PCS mdiodev only after we are done with it (since the PCS
>   does not perform a get itself).
> 
> Changes in v2:
> - Move PCS_LYNX dependency to fman Kconfig
> 
>  drivers/net/ethernet/freescale/fman/Kconfig   |   3 +
>  .../net/ethernet/freescale/fman/fman_memac.c  | 258 +++++++-----------
>  2 files changed, 105 insertions(+), 156 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fman/Kconfig b/drivers/net/ethernet/freescale/fman/Kconfig
> index 48bf8088795d..8f5637db41dd 100644
> --- a/drivers/net/ethernet/freescale/fman/Kconfig
> +++ b/drivers/net/ethernet/freescale/fman/Kconfig
> @@ -4,6 +4,9 @@ config FSL_FMAN
>  	depends on FSL_SOC || ARCH_LAYERSCAPE || COMPILE_TEST
>  	select GENERIC_ALLOCATOR
>  	select PHYLIB
> +	select PHYLINK
> +	select PCS
> +	select PCS_LYNX
>  	select CRC32
>  	default n
>  	help
> diff --git a/drivers/net/ethernet/freescale/fman/fman_memac.c b/drivers/net/ethernet/freescale/fman/fman_memac.c
> index 56a29f505590..eeb71352603b 100644
> --- a/drivers/net/ethernet/freescale/fman/fman_memac.c
> +++ b/drivers/net/ethernet/freescale/fman/fman_memac.c
> @@ -11,43 +11,12 @@
>  
>  #include <linux/slab.h>
>  #include <linux/io.h>
> +#include <linux/pcs-lynx.h>
>  #include <linux/phy.h>
>  #include <linux/phy_fixed.h>
>  #include <linux/phy/phy.h>
>  #include <linux/of_mdio.h>
>  
> -/* PCS registers */
> -#define MDIO_SGMII_CR			0x00
> -#define MDIO_SGMII_DEV_ABIL_SGMII	0x04
> -#define MDIO_SGMII_LINK_TMR_L		0x12
> -#define MDIO_SGMII_LINK_TMR_H		0x13
> -#define MDIO_SGMII_IF_MODE		0x14
> -
> -/* SGMII Control defines */
> -#define SGMII_CR_AN_EN			0x1000
> -#define SGMII_CR_RESTART_AN		0x0200
> -#define SGMII_CR_FD			0x0100
> -#define SGMII_CR_SPEED_SEL1_1G		0x0040
> -#define SGMII_CR_DEF_VAL		(SGMII_CR_AN_EN | SGMII_CR_FD | \
> -					 SGMII_CR_SPEED_SEL1_1G)
> -
> -/* SGMII Device Ability for SGMII defines */
> -#define MDIO_SGMII_DEV_ABIL_SGMII_MODE	0x4001
> -#define MDIO_SGMII_DEV_ABIL_BASEX_MODE	0x01A0
> -
> -/* Link timer define */
> -#define LINK_TMR_L			0xa120
> -#define LINK_TMR_H			0x0007
> -#define LINK_TMR_L_BASEX		0xaf08
> -#define LINK_TMR_H_BASEX		0x002f
> -
> -/* SGMII IF Mode defines */
> -#define IF_MODE_USE_SGMII_AN		0x0002
> -#define IF_MODE_SGMII_EN		0x0001
> -#define IF_MODE_SGMII_SPEED_100M	0x0004
> -#define IF_MODE_SGMII_SPEED_1G		0x0008
> -#define IF_MODE_SGMII_DUPLEX_HALF	0x0010
> -
>  /* Num of additional exact match MAC adr regs */
>  #define MEMAC_NUM_OF_PADDRS 7
>  
> @@ -326,7 +295,9 @@ struct fman_mac {
>  	struct fman_rev_info fm_rev_info;
>  	bool basex_if;
>  	struct phy *serdes;
> -	struct phy_device *pcsphy;
> +	struct phylink_pcs *sgmii_pcs;
> +	struct phylink_pcs *qsgmii_pcs;
> +	struct phylink_pcs *xfi_pcs;
>  	bool allmulti_enabled;
>  };
>  
> @@ -487,91 +458,22 @@ static u32 get_mac_addr_hash_code(u64 eth_addr)
>  	return xor_val;
>  }
>  
> -static void setup_sgmii_internal_phy(struct fman_mac *memac,
> -				     struct fixed_phy_status *fixed_link)
> +static void setup_sgmii_internal(struct fman_mac *memac,
> +				 struct phylink_pcs *pcs,
> +				 struct fixed_phy_status *fixed_link)
>  {
> -	u16 tmp_reg16;
> -
> -	if (WARN_ON(!memac->pcsphy))
> -		return;
> -
> -	/* SGMII mode */
> -	tmp_reg16 = IF_MODE_SGMII_EN;
> -	if (!fixed_link)
> -		/* AN enable */
> -		tmp_reg16 |= IF_MODE_USE_SGMII_AN;
> -	else {
> -		switch (fixed_link->speed) {
> -		case 10:
> -			/* For 10M: IF_MODE[SPEED_10M] = 0 */
> -		break;
> -		case 100:
> -			tmp_reg16 |= IF_MODE_SGMII_SPEED_100M;
> -		break;
> -		case 1000:
> -		default:
> -			tmp_reg16 |= IF_MODE_SGMII_SPEED_1G;
> -		break;
> -		}
> -		if (!fixed_link->duplex)
> -			tmp_reg16 |= IF_MODE_SGMII_DUPLEX_HALF;
> -	}
> -	phy_write(memac->pcsphy, MDIO_SGMII_IF_MODE, tmp_reg16);
> -
> -	/* Device ability according to SGMII specification */
> -	tmp_reg16 = MDIO_SGMII_DEV_ABIL_SGMII_MODE;
> -	phy_write(memac->pcsphy, MDIO_SGMII_DEV_ABIL_SGMII, tmp_reg16);
> -
> -	/* Adjust link timer for SGMII  -
> -	 * According to Cisco SGMII specification the timer should be 1.6 ms.
> -	 * The link_timer register is configured in units of the clock.
> -	 * - When running as 1G SGMII, Serdes clock is 125 MHz, so
> -	 * unit = 1 / (125*10^6 Hz) = 8 ns.
> -	 * 1.6 ms in units of 8 ns = 1.6ms / 8ns = 2*10^5 = 0x30d40
> -	 * - When running as 2.5G SGMII, Serdes clock is 312.5 MHz, so
> -	 * unit = 1 / (312.5*10^6 Hz) = 3.2 ns.
> -	 * 1.6 ms in units of 3.2 ns = 1.6ms / 3.2ns = 5*10^5 = 0x7a120.
> -	 * Since link_timer value of 1G SGMII will be too short for 2.5 SGMII,
> -	 * we always set up here a value of 2.5 SGMII.
> -	 */
> -	phy_write(memac->pcsphy, MDIO_SGMII_LINK_TMR_H, LINK_TMR_H);
> -	phy_write(memac->pcsphy, MDIO_SGMII_LINK_TMR_L, LINK_TMR_L);
> -
> -	if (!fixed_link)
> -		/* Restart AN */
> -		tmp_reg16 = SGMII_CR_DEF_VAL | SGMII_CR_RESTART_AN;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
> +	phy_interface_t iface = memac->basex_if ? PHY_INTERFACE_MODE_1000BASEX :
> +				PHY_INTERFACE_MODE_SGMII;
> +	unsigned int mode = fixed_link ? MLO_AN_FIXED : MLO_AN_INBAND;
> +
> +	linkmode_set_pause(advertising, true, true);
> +	pcs->ops->pcs_config(pcs, mode, iface, advertising, true);
> +	if (fixed_link)
> +		pcs->ops->pcs_link_up(pcs, mode, iface, fixed_link->speed,
> +				      fixed_link->duplex);
>  	else
> -		/* AN disabled */
> -		tmp_reg16 = SGMII_CR_DEF_VAL & ~SGMII_CR_AN_EN;
> -	phy_write(memac->pcsphy, 0x0, tmp_reg16);
> -}
> -
> -static void setup_sgmii_internal_phy_base_x(struct fman_mac *memac)
> -{
> -	u16 tmp_reg16;
> -
> -	/* AN Device capability  */
> -	tmp_reg16 = MDIO_SGMII_DEV_ABIL_BASEX_MODE;
> -	phy_write(memac->pcsphy, MDIO_SGMII_DEV_ABIL_SGMII, tmp_reg16);
> -
> -	/* Adjust link timer for SGMII  -
> -	 * For Serdes 1000BaseX auto-negotiation the timer should be 10 ms.
> -	 * The link_timer register is configured in units of the clock.
> -	 * - When running as 1G SGMII, Serdes clock is 125 MHz, so
> -	 * unit = 1 / (125*10^6 Hz) = 8 ns.
> -	 * 10 ms in units of 8 ns = 10ms / 8ns = 1250000 = 0x1312d0
> -	 * - When running as 2.5G SGMII, Serdes clock is 312.5 MHz, so
> -	 * unit = 1 / (312.5*10^6 Hz) = 3.2 ns.
> -	 * 10 ms in units of 3.2 ns = 10ms / 3.2ns = 3125000 = 0x2faf08.
> -	 * Since link_timer value of 1G SGMII will be too short for 2.5 SGMII,
> -	 * we always set up here a value of 2.5 SGMII.
> -	 */
> -	phy_write(memac->pcsphy, MDIO_SGMII_LINK_TMR_H, LINK_TMR_H_BASEX);
> -	phy_write(memac->pcsphy, MDIO_SGMII_LINK_TMR_L, LINK_TMR_L_BASEX);
> -
> -	/* Restart AN */
> -	tmp_reg16 = SGMII_CR_DEF_VAL | SGMII_CR_RESTART_AN;
> -	phy_write(memac->pcsphy, 0x0, tmp_reg16);
> +		pcs->ops->pcs_an_restart(pcs);
>  }
>  
>  static int check_init_parameters(struct fman_mac *memac)
> @@ -983,7 +885,6 @@ static int memac_set_exception(struct fman_mac *memac,
>  static int memac_init(struct fman_mac *memac)
>  {
>  	struct memac_cfg *memac_drv_param;
> -	u8 i;
>  	enet_addr_t eth_addr;
>  	bool slow_10g_if = false;
>  	struct fixed_phy_status *fixed_link = NULL;
> @@ -1036,32 +937,10 @@ static int memac_init(struct fman_mac *memac)
>  		iowrite32be(reg32, &memac->regs->command_config);
>  	}
>  
> -	if (memac->phy_if == PHY_INTERFACE_MODE_SGMII) {
> -		/* Configure internal SGMII PHY */
> -		if (memac->basex_if)
> -			setup_sgmii_internal_phy_base_x(memac);
> -		else
> -			setup_sgmii_internal_phy(memac, fixed_link);
> -	} else if (memac->phy_if == PHY_INTERFACE_MODE_QSGMII) {
> -		/* Configure 4 internal SGMII PHYs */
> -		for (i = 0; i < 4; i++) {
> -			u8 qsmgii_phy_addr, phy_addr;
> -			/* QSGMII PHY address occupies 3 upper bits of 5-bit
> -			 * phy_address; the lower 2 bits are used to extend
> -			 * register address space and access each one of 4
> -			 * ports inside QSGMII.
> -			 */
> -			phy_addr = memac->pcsphy->mdio.addr;
> -			qsmgii_phy_addr = (u8)((phy_addr << 2) | i);
> -			memac->pcsphy->mdio.addr = qsmgii_phy_addr;
> -			if (memac->basex_if)
> -				setup_sgmii_internal_phy_base_x(memac);
> -			else
> -				setup_sgmii_internal_phy(memac, fixed_link);
> -
> -			memac->pcsphy->mdio.addr = phy_addr;
> -		}
> -	}
> +	if (memac->phy_if == PHY_INTERFACE_MODE_SGMII)
> +		setup_sgmii_internal(memac, memac->sgmii_pcs, fixed_link);
> +	else if (memac->phy_if == PHY_INTERFACE_MODE_QSGMII)
> +		setup_sgmii_internal(memac, memac->qsgmii_pcs, fixed_link);
>  
>  	/* Max Frame Length */
>  	err = fman_set_mac_max_frame(memac->fm, memac->mac_id,
> @@ -1097,12 +976,25 @@ static int memac_init(struct fman_mac *memac)
>  	return 0;
>  }
>  
> +static void pcs_put(struct phylink_pcs *pcs)
> +{
> +	struct mdio_device *mdiodev;
> +
> +	if (!pcs)

This should be IS_ERR_OR_NULL. Will fix for next spin.

--Sean

> +		return;
> +
> +	mdiodev = lynx_get_mdio_device(pcs);
> +	lynx_pcs_destroy(pcs);
> +	mdio_device_free(mdiodev);
> +}
> +
>  static int memac_free(struct fman_mac *memac)
>  {
>  	free_init_resources(memac);
>  
> -	if (memac->pcsphy)
> -		put_device(&memac->pcsphy->mdio.dev);
> +	pcs_put(memac->sgmii_pcs);
> +	pcs_put(memac->qsgmii_pcs);
> +	pcs_put(memac->xfi_pcs);
>  
>  	kfree(memac->memac_drv_param);
>  	kfree(memac);
> @@ -1153,12 +1045,31 @@ static struct fman_mac *memac_config(struct mac_device *mac_dev,
>  	return memac;
>  }
>  
> +static struct phylink_pcs *memac_pcs_create(struct device_node *mac_node,
> +					    int index)
> +{
> +	struct device_node *node;
> +	struct mdio_device *mdiodev = NULL;
> +	struct phylink_pcs *pcs;
> +
> +	node = of_parse_phandle(mac_node, "pcsphy-handle", index);
> +	if (node && of_device_is_available(node))
> +		mdiodev = of_mdio_find_device(node);
> +	of_node_put(node);
> +
> +	if (!mdiodev)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	pcs = lynx_pcs_create(mdiodev);
> +	return pcs;
> +}
> +
>  int memac_initialization(struct mac_device *mac_dev,
>  			 struct device_node *mac_node,
>  			 struct fman_mac_params *params)
>  {
>  	int			 err;
> -	struct device_node	*phy_node;
> +	struct phylink_pcs	*pcs;
>  	struct fixed_phy_status *fixed_link;
>  	struct fman_mac		*memac;
>  
> @@ -1188,23 +1099,58 @@ int memac_initialization(struct mac_device *mac_dev,
>  	memac = mac_dev->fman_mac;
>  	memac->memac_drv_param->max_frame_length = fman_get_max_frm();
>  	memac->memac_drv_param->reset_on_init = true;
> -	if (memac->phy_if == PHY_INTERFACE_MODE_SGMII ||
> -	    memac->phy_if == PHY_INTERFACE_MODE_QSGMII) {
> -		phy_node = of_parse_phandle(mac_node, "pcsphy-handle", 0);
> -		if (!phy_node) {
> -			pr_err("PCS PHY node is not available\n");
> -			err = -EINVAL;
> +
> +	err = of_property_match_string(mac_node, "pcs-names", "xfi");
> +	if (err >= 0) {
> +		memac->xfi_pcs = memac_pcs_create(mac_node, err);
> +		if (IS_ERR(memac->xfi_pcs)) {
> +			err = PTR_ERR(memac->xfi_pcs);
> +			dev_err_probe(mac_dev->dev, err, "missing xfi pcs\n");
>  			goto _return_fm_mac_free;
>  		}
> +	} else if (err != -EINVAL && err != -ENODATA) {
> +		goto _return_fm_mac_free;
> +	}
>  
> -		memac->pcsphy = of_phy_find_device(phy_node);
> -		if (!memac->pcsphy) {
> -			pr_err("of_phy_find_device (PCS PHY) failed\n");
> -			err = -EINVAL;
> +	err = of_property_match_string(mac_node, "pcs-names", "qsgmii");
> +	if (err >= 0) {
> +		memac->qsgmii_pcs = memac_pcs_create(mac_node, err);
> +		if (IS_ERR(memac->qsgmii_pcs)) {
> +			err = PTR_ERR(memac->qsgmii_pcs);
> +			dev_err_probe(mac_dev->dev, err,
> +				      "missing qsgmii pcs\n");
>  			goto _return_fm_mac_free;
>  		}
> +	} else if (err != -EINVAL && err != -ENODATA) {
> +		goto _return_fm_mac_free;
> +	}
> +
> +	/* For compatibility, if pcs-names is missing, we assume this phy is
> +	 * the first one in pcsphy-handle
> +	 */
> +	err = of_property_match_string(mac_node, "pcs-names", "sgmii");
> +	if (err == -EINVAL)
> +		pcs = memac_pcs_create(mac_node, 0);
> +	else if (err < 0)
> +		goto _return_fm_mac_free;
> +	else
> +		pcs = memac_pcs_create(mac_node, err);
> +
> +	if (!pcs) {
> +		dev_err(mac_dev->dev, "missing pcs\n");
> +		err = -ENOENT;
> +		goto _return_fm_mac_free;
>  	}
>  
> +	/* If err is set here, it means that pcs-names was missing above (and
> +	 * therefore that xfi_pcs cannot be set). If we are defaulting to
> +	 * XGMII, assume this is for XFI. Otherwise, assume it is for SGMII.
> +	 */
> +	if (err && mac_dev->phy_if == PHY_INTERFACE_MODE_XGMII)
> +		memac->xfi_pcs = pcs;
> +	else
> +		memac->sgmii_pcs = pcs;
> +
>  	memac->serdes = devm_of_phy_get(mac_dev->dev, mac_node, "serdes");
>  	err = PTR_ERR(memac->serdes);
>  	if (err == -ENODEV || err == -ENOSYS) {
> 

Powered by blists - more mailing lists