Re: [PATCH net-next v7 4/7] stmmac: intel: configure SerDes according to the interface mode

[Ilpo Järvinen]

On Thu, 6 Feb 2025, Choong Yong Liang wrote:

> Intel platform will configure the SerDes through PMC API based on the
> provided interface mode.
> 
> This patch adds several new functions below:-
> - intel_tsn_lane_is_available(): This new function reads FIA lane
>   ownership registers and common lane registers through IPC commands
>   to know which lane the mGbE port is assigned to.
> - intel_mac_finish(): To configure the SerDes based on the assigned
>   lane and latest interface mode, it sends IPC command to the PMC through
>   PMC driver/API. The PMC acts as a proxy for R/W on behalf of the driver.
> - intel_set_reg_access(): Set the register access to the available TSN
>   interface.
> 
> Signed-off-by: Choong Yong Liang <yong.liang.choong@xxxxxxxxxxxxxxx>
> ---
>  drivers/net/ethernet/stmicro/stmmac/Kconfig   |   2 +
>  .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 146 +++++++++++++++++-
>  .../net/ethernet/stmicro/stmmac/dwmac-intel.h |  29 ++++
>  include/linux/stmmac.h                        |   4 +
>  4 files changed, 179 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/Kconfig b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> index 4cc85a36a1ab..25154b915b02 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/Kconfig
> +++ b/drivers/net/ethernet/stmicro/stmmac/Kconfig
> @@ -307,6 +307,8 @@ config DWMAC_INTEL
>  	default X86
>  	depends on X86 && STMMAC_ETH && PCI
>  	depends on COMMON_CLK
> +	depends on ACPI
> +	select INTEL_PMC_IPC
>  	help
>  	  This selects the Intel platform specific bus support for the
>  	  stmmac driver. This driver is used for Intel Quark/EHL/TGL.
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> index 48acba5eb178..837fd3fbaedb 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
> @@ -5,15 +5,30 @@
>  #include <linux/clk-provider.h>
>  #include <linux/pci.h>
>  #include <linux/dmi.h>
> +#include <linux/platform_data/x86/intel_pmc_ipc.h>
>  #include "dwmac-intel.h"
>  #include "dwmac4.h"
>  #include "stmmac.h"
>  #include "stmmac_ptp.h"
>  
> +struct pmc_serdes_regs {
> +	u8 index;
> +	u32 val;
> +};
> +
> +struct pmc_serdes_reg_info {
> +	const struct pmc_serdes_regs *regs;
> +	u8 num_regs;
> +};
> +
>  struct intel_priv_data {
>  	int mdio_adhoc_addr;	/* mdio address for serdes & etc */
>  	unsigned long crossts_adj;
>  	bool is_pse;
> +	const int *tsn_lane_regs;
> +	int max_tsn_lane_regs;
> +	struct pmc_serdes_reg_info pid_1g;
> +	struct pmc_serdes_reg_info pid_2p5g;
>  };
>  
>  /* This struct is used to associate PCI Function of MAC controller on a board,
> @@ -35,6 +50,42 @@ struct stmmac_pci_info {
>  	int (*setup)(struct pci_dev *pdev, struct plat_stmmacenet_data *plat);
>  };
>  
> +static const struct pmc_serdes_regs pid_modphy3_1g_regs[] = {
> +	{ PID_MODPHY3_B_MODPHY_PCR_LCPLL_DWORD0,	B_MODPHY_PCR_LCPLL_DWORD0_1G },
> +	{ PID_MODPHY3_N_MODPHY_PCR_LCPLL_DWORD2,	N_MODPHY_PCR_LCPLL_DWORD2_1G },
> +	{ PID_MODPHY3_N_MODPHY_PCR_LCPLL_DWORD7,	N_MODPHY_PCR_LCPLL_DWORD7_1G },
> +	{ PID_MODPHY3_N_MODPHY_PCR_LPPLL_DWORD10,	N_MODPHY_PCR_LPPLL_DWORD10_1G },
> +	{ PID_MODPHY3_N_MODPHY_PCR_CMN_ANA_DWORD30,	N_MODPHY_PCR_CMN_ANA_DWORD30_1G },
> +	{}
> +};
> +
> +static const struct pmc_serdes_regs pid_modphy3_2p5g_regs[] = {
> +	{ PID_MODPHY3_B_MODPHY_PCR_LCPLL_DWORD0,	B_MODPHY_PCR_LCPLL_DWORD0_2P5G },
> +	{ PID_MODPHY3_N_MODPHY_PCR_LCPLL_DWORD2,	N_MODPHY_PCR_LCPLL_DWORD2_2P5G },
> +	{ PID_MODPHY3_N_MODPHY_PCR_LCPLL_DWORD7,	N_MODPHY_PCR_LCPLL_DWORD7_2P5G },
> +	{ PID_MODPHY3_N_MODPHY_PCR_LPPLL_DWORD10,	N_MODPHY_PCR_LPPLL_DWORD10_2P5G },
> +	{ PID_MODPHY3_N_MODPHY_PCR_CMN_ANA_DWORD30,	N_MODPHY_PCR_CMN_ANA_DWORD30_2P5G },
> +	{}
> +};
> +
> +static const struct pmc_serdes_regs pid_modphy1_1g_regs[] = {
> +	{ PID_MODPHY1_B_MODPHY_PCR_LCPLL_DWORD0,	B_MODPHY_PCR_LCPLL_DWORD0_1G },
> +	{ PID_MODPHY1_N_MODPHY_PCR_LCPLL_DWORD2,	N_MODPHY_PCR_LCPLL_DWORD2_1G },
> +	{ PID_MODPHY1_N_MODPHY_PCR_LCPLL_DWORD7,	N_MODPHY_PCR_LCPLL_DWORD7_1G },
> +	{ PID_MODPHY1_N_MODPHY_PCR_LPPLL_DWORD10,	N_MODPHY_PCR_LPPLL_DWORD10_1G },
> +	{ PID_MODPHY1_N_MODPHY_PCR_CMN_ANA_DWORD30,	N_MODPHY_PCR_CMN_ANA_DWORD30_1G },
> +	{}
> +};
> +
> +static const struct pmc_serdes_regs pid_modphy1_2p5g_regs[] = {
> +	{ PID_MODPHY1_B_MODPHY_PCR_LCPLL_DWORD0,	B_MODPHY_PCR_LCPLL_DWORD0_2P5G },
> +	{ PID_MODPHY1_N_MODPHY_PCR_LCPLL_DWORD2,	N_MODPHY_PCR_LCPLL_DWORD2_2P5G },
> +	{ PID_MODPHY1_N_MODPHY_PCR_LCPLL_DWORD7,	N_MODPHY_PCR_LCPLL_DWORD7_2P5G },
> +	{ PID_MODPHY1_N_MODPHY_PCR_LPPLL_DWORD10,	N_MODPHY_PCR_LPPLL_DWORD10_2P5G },
> +	{ PID_MODPHY1_N_MODPHY_PCR_CMN_ANA_DWORD30,	N_MODPHY_PCR_CMN_ANA_DWORD30_2P5G },
> +	{}
> +};
> +
>  static int stmmac_pci_find_phy_addr(struct pci_dev *pdev,
>  				    const struct dmi_system_id *dmi_list)
>  {
> @@ -93,7 +144,7 @@ static int intel_serdes_powerup(struct net_device *ndev, void *priv_data)
>  	data &= ~SERDES_RATE_MASK;
>  	data &= ~SERDES_PCLK_MASK;
>  
> -	if (priv->plat->max_speed == 2500)
> +	if (priv->plat->phy_interface == PHY_INTERFACE_MODE_2500BASEX)
>  		data |= SERDES_RATE_PCIE_GEN2 << SERDES_RATE_PCIE_SHIFT |
>  			SERDES_PCLK_37p5MHZ << SERDES_PCLK_SHIFT;
>  	else
> @@ -415,6 +466,95 @@ static void intel_mgbe_pse_crossts_adj(struct intel_priv_data *intel_priv,
>  	}
>  }
>  
> +static int intel_tsn_lane_is_available(struct net_device *ndev,
> +				       struct intel_priv_data *intel_priv)
> +{
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	struct pmc_ipc_cmd tmp = {};
> +	u32 rbuf[4] = {};
> +	int ret = 0, i, j;
> +	const int max_fia_regs = 5;
> +
> +	tmp.cmd = IPC_SOC_REGISTER_ACCESS;
> +	tmp.sub_cmd = IPC_SOC_SUB_CMD_READ;
> +
> +	for (i = 0; i < max_fia_regs; i++) {

Usually, defines are used for true consts.

> +		tmp.wbuf[0] = R_PCH_FIA_15_PCR_LOS1_REG_BASE + i;
> +
> +		ret = intel_pmc_ipc(&tmp, rbuf);
> +		if (ret < 0) {
> +			netdev_info(priv->dev, "Failed to read from PMC.\n");
> +			return ret;
> +		}
> +
> +		for (j = 0; j <= intel_priv->max_tsn_lane_regs; j++)
> +			if ((rbuf[0] >>
> +				(4 * (intel_priv->tsn_lane_regs[j] % 8)) &
> +					B_PCH_FIA_PCR_L0O) == 0xB)
> +				return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int intel_set_reg_access(const struct pmc_serdes_regs *regs, int max_regs)
> +{
> +	int ret = 0, i;
> +
> +	for (i = 0; i < max_regs; i++) {
> +		struct pmc_ipc_cmd tmp = {};
> +		u32 buf[4] = {};
> +
> +		tmp.cmd = IPC_SOC_REGISTER_ACCESS;
> +		tmp.sub_cmd = IPC_SOC_SUB_CMD_WRITE;
> +		tmp.wbuf[0] = (u32)regs[i].index;
> +		tmp.wbuf[1] = regs[i].val;
> +
> +		ret = intel_pmc_ipc(&tmp, buf);
> +		if (ret < 0)
> +			return ret;
> +	}
> +
> +	return ret;
> +}
> +
> +static int intel_mac_finish(struct net_device *ndev,
> +			    void *intel_data,
> +			    unsigned int mode,
> +			    phy_interface_t interface)
> +{
> +	struct intel_priv_data *intel_priv = intel_data;
> +	struct stmmac_priv *priv = netdev_priv(ndev);
> +	const struct pmc_serdes_regs *regs;
> +	int max_regs = 0;
> +	int ret = 0;
> +
> +	ret = intel_tsn_lane_is_available(ndev, intel_priv);
> +	if (ret < 0) {
> +		netdev_info(priv->dev, "No TSN lane available to set the registers.\n");
> +		return ret;
> +	}
> +
> +	if (interface == PHY_INTERFACE_MODE_2500BASEX) {
> +		regs = intel_priv->pid_2p5g.regs;
> +		max_regs = intel_priv->pid_2p5g.num_regs;
> +	} else {
> +		regs = intel_priv->pid_1g.regs;
> +		max_regs = intel_priv->pid_1g.num_regs;
> +	}
> +
> +	ret = intel_set_reg_access(regs, max_regs);
> +	if (ret < 0)
> +		return ret;

This looks much cleaner now, thanks the update.

However, the intel_priv fields you introduced are not setup until patch 
6/7? Will this cause NULL ptr deref issues in between the two changes? By 
introducing the reg arrays in this patch but only use them after patch 6, 
you'll also get unused variable warnings out of them in between the 
changes which is unacceptable.

> +
> +	priv->plat->phy_interface = interface;
> +
> +	intel_serdes_powerdown(ndev, intel_priv);
> +	intel_serdes_powerup(ndev, intel_priv);
> +
> +	return ret;
> +}


-- 
 i.