[PATCH net-next v7 0/7] Enable SGMII and 2500BASEX interface mode switching for Intel platforms

[PATCH net-next v7 0/7] Enable SGMII and 2500BASEX interface mode switching for Intel platforms

[Choong Yong Liang]

During the interface mode change, the 'phylink_major_config' function will
be triggered in phylink. The modification of the following functions will
support the switching between SGMII and 2500BASE-X interface modes for
the Intel platform:

- xpcs_switch_interface_mode: Re-initiates clause 37 auto-negotiation for
  the SGMII interface mode to perform auto-negotiation.
- mac_finish: Configures the SerDes according to the interface mode.

With the above changes, the code will work as follows during the interface
mode change. The PCS will reconfigure according to the pcs_neg_mode and the
selected interface mode. Then, the MAC driver will perform SerDes
configuration in 'mac_finish' based on the selected interface mode. During
the SerDes configuration, the selected interface mode will identify TSN
lane registers from FIA and then send IPC commands to the Power Management
Controller (PMC) through the PMC driver/API. The PMC will act as a proxy to
program the PLL registers.

Change log:
v1 -> v2: 
 - Add static to pmc_lpm_modes declaration
 - Add cur_link_an_mode to the kernel doc
 - Combine 2 commits i.e. "stmmac: intel: Separate driver_data of ADL-N
 from TGL" and "net: stmmac: Add 1G/2.5G auto-negotiation
 support for ADL-N" into 1 commit.

v2 -> v3:
 - Create `pmc_ipc.c` file for `intel_pmc_ipc()` function and 
 allocate the file in `arch/x86/platform/intel/` directory.
 - Update phylink's AN mode during phy interface change and 
 not exposing phylink's AN mode into phylib.
 
 v3 -> v4:
 - Introduce `allow_switch_interface` flag to have all ethtool 
 link modes that are supported and advertised will be published.
 - Introduce `mac_get_pcs_neg_mode` function that selects the PCS 
 negotiation mode according to the interface mode.
 - Remove pcs-xpcs.c changes and handle pcs during `mac_select_pcs`
 function
 - Configure SerDes base on the interface on `mac_finish` function.
 
 v4 -> v5:
 - remove 'allow_switch_interface' related patches.
 - remove 'mac_select_pcs' related patches.
 - add a soft reset according to XPCS datasheet for re-initiate Clause 37
 auto-negotiation when switching to SGMII interface mode.

v5 -> v6:
- Remove 'mac_get_pcs_neg_mode' related patches. 
  The pcs_neg_mode is properly handled by the
  'net: add negotiation of in-band capabilities' patch series:
  https://patchwork.kernel.org/project/netdevbpf/cover/Z08kCwxdkU4n2V6x@shell.armlinux.org.uk/
- Using act_link_an_mode to determine PHY, as cfg_link_an_mode was not
  updated for the 2500BASE-X interface mode, caused a failure to link up.
- Clean up and standardize the interface mode switch for xpcs.

v6 -> v7:
- Remove the "net: phylink: use act_link_an_mode to determine PHY" patch.
- Use pl->link_interface in phylink_expects_phy().
- Remove priv->plat->serdes_powerup in intel_tsn_lane_is_available() as it is
  always true.
- Refactor the code in intel_config_serdes().
- Rename intel_config_serdes() to intel_mac_finish() with an AN mode parameter.
- Define the magic number as "max_fia_regs".
- Store the pointer and the number of elements in the platform info structure.
- Move the arrays to the C file.

v1: https://patchwork.kernel.org/project/netdevbpf/cover/20230622041905.629430-1-yong.liang.choong@linux.intel.com/
v2: https://patchwork.kernel.org/project/netdevbpf/cover/20230804084527.2082302-1-yong.liang.choong@linux.intel.com/
v3: https://patchwork.kernel.org/project/netdevbpf/cover/20230921121946.3025771-1-yong.liang.choong@linux.intel.com/
v4: https://patchwork.kernel.org/project/netdevbpf/cover/20240129130253.1400707-1-yong.liang.choong@linux.intel.com/
v5: https://patchwork.kernel.org/project/netdevbpf/cover/20240215030500.3067426-1-yong.liang.choong@linux.intel.com/
v6: https://patchwork.kernel.org/project/netdevbpf/cover/20250204061020.1199124-1-yong.liang.choong@linux.intel.com/

Choong Yong Liang (6):
  net: phylink: use pl->link_interface in phylink_expects_phy()
  net: pcs: xpcs: re-initiate clause 37 Auto-negotiation
  stmmac: intel: configure SerDes according to the interface mode
  net: stmmac: configure SerDes on mac_finish
  stmmac: intel: interface switching support for EHL platform
  stmmac: intel: interface switching support for ADL-N platform

David E. Box (1):
  arch: x86: add IPC mailbox accessor function and add SoC register
    access

 MAINTAINERS                                   |   2 +
 arch/x86/Kconfig                              |   9 +
 arch/x86/platform/intel/Makefile              |   1 +
 arch/x86/platform/intel/pmc_ipc.c             |  75 ++++++
 drivers/net/ethernet/stmicro/stmmac/Kconfig   |   2 +
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 229 +++++++++++++++++-
 .../net/ethernet/stmicro/stmmac/dwmac-intel.h |  29 +++
 .../net/ethernet/stmicro/stmmac/stmmac_main.c |  13 +
 drivers/net/pcs/pcs-xpcs-wx.c                 |   4 +-
 drivers/net/pcs/pcs-xpcs.c                    |  60 ++++-
 drivers/net/phy/phylink.c                     |   2 +-
 .../linux/platform_data/x86/intel_pmc_ipc.h   |  34 +++
 include/linux/stmmac.h                        |   4 +
 13 files changed, 452 insertions(+), 12 deletions(-)
 create mode 100644 arch/x86/platform/intel/pmc_ipc.c
 create mode 100644 include/linux/platform_data/x86/intel_pmc_ipc.h

-- 
2.34.1

[PATCH net-next v7 1/7] net: phylink: use pl->link_interface in phylink_expects_phy()

[Choong Yong Liang]

The phylink_expects_phy() function allows MAC drivers to check if they are
expecting a PHY to attach. The checking condition in phylink_expects_phy()
aims to achieve the same result as the checking condition in
phylink_attach_phy().

However, the checking condition in phylink_expects_phy() uses
pl->link_config.interface, while phylink_attach_phy() uses
pl->link_interface.

Initially, both pl->link_interface and pl->link_config.interface are set
to SGMII, and pl->cfg_link_an_mode is set to MLO_AN_INBAND.

When the interface switches from SGMII to 2500BASE-X,
pl->link_config.interface is updated by phylink_major_config().
At this point, pl->cfg_link_an_mode remains MLO_AN_INBAND, and
pl->link_config.interface is set to 2500BASE-X.
Subsequently, when the STMMAC link goes down and comes up again,
it is blocked by phylink_expects_phy().

Since phylink_expects_phy() and phylink_attach_phy() aim to achieve the
same result, phylink_expects_phy() should check pl->link_interface,
which never changes, instead of pl->link_config.interface, which is
updated by phylink_major_config().

Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
---
 drivers/net/phy/phylink.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 214b62fba991..c173cd8db475 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -2046,7 +2046,7 @@ bool phylink_expects_phy(struct phylink *pl)
 {
 	if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
 	    (pl->cfg_link_an_mode == MLO_AN_INBAND &&
-	     phy_interface_mode_is_8023z(pl->link_config.interface)))
+	     phy_interface_mode_is_8023z(pl->link_interface)))
 		return false;
 	return true;
 }
-- 
2.34.1

[PATCH net-next v7 2/7] net: pcs: xpcs: re-initiate clause 37 Auto-negotiation

[Choong Yong Liang]

The xpcs_switch_interface_mode function was introduced to handle
interface switching.

According to the XPCS datasheet, a soft reset is required to initiate
Clause 37 auto-negotiation when the XPCS switches interface modes.

When the interface mode is set to 2500BASE-X, Clause 37 auto-negotiation
is turned off.

Subsequently, when the interface mode switches from 2500BASE-X to SGMII,
re-initiating Clause 37 auto-negotiation is required for the SGMII
interface mode to function properly.

Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
---
 drivers/net/pcs/pcs-xpcs-wx.c |  4 +--
 drivers/net/pcs/pcs-xpcs.c    | 60 ++++++++++++++++++++++++++++++++---
 2 files changed, 57 insertions(+), 7 deletions(-)

diff --git a/drivers/net/pcs/pcs-xpcs-wx.c b/drivers/net/pcs/pcs-xpcs-wx.c
index fc52f7aa5f59..f73ab04d09f0 100644
--- a/drivers/net/pcs/pcs-xpcs-wx.c
+++ b/drivers/net/pcs/pcs-xpcs-wx.c
@@ -172,11 +172,9 @@ int txgbe_xpcs_switch_mode(struct dw_xpcs *xpcs, phy_interface_t interface)
 		return 0;
 	}
 
-	if (xpcs->interface == interface && !txgbe_xpcs_mode_quirk(xpcs))
+	if (!txgbe_xpcs_mode_quirk(xpcs))
 		return 0;
 
-	xpcs->interface = interface;
-
 	ret = txgbe_pcs_poll_power_up(xpcs);
 	if (ret < 0)
 		return ret;
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 1faa37f0e7b9..fb3d1548a8e0 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -817,6 +817,58 @@ static int xpcs_config_2500basex(struct dw_xpcs *xpcs)
 			   BMCR_SPEED1000);
 }
 
+static int xpcs_switch_to_aneg_c37_sgmii(const struct dw_xpcs_compat *compat,
+					 struct dw_xpcs *xpcs,
+					 unsigned int neg_mode)
+{
+	bool an_c37_enabled;
+	int ret, mdio_ctrl;
+
+	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
+		mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
+		if (mdio_ctrl < 0)
+			return mdio_ctrl;
+
+		an_c37_enabled = mdio_ctrl & BMCR_ANENABLE;
+		if (!an_c37_enabled) {
+			//Perform soft reset to initiate C37 auto-negotiation
+			ret = xpcs_soft_reset(xpcs, compat);
+			if (ret)
+				return ret;
+		}
+	}
+	return 0;
+}
+
+static int xpcs_switch_interface_mode(const struct dw_xpcs_compat *compat,
+				      struct dw_xpcs *xpcs,
+				      phy_interface_t interface,
+				      unsigned int neg_mode)
+{
+	int ret = 0;
+
+	if (xpcs->interface != interface) {
+		if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
+			ret = txgbe_xpcs_switch_mode(xpcs, interface);
+		} else {
+			switch (compat->an_mode) {
+			case DW_AN_C37_SGMII:
+				ret = xpcs_switch_to_aneg_c37_sgmii(compat, xpcs, neg_mode);
+				break;
+			default:
+				break;
+			}
+		}
+
+		if (ret)
+			return ret;
+
+		xpcs->interface = interface;
+	}
+
+	return 0;
+}
+
 static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 			  const unsigned long *advertising,
 			  unsigned int neg_mode)
@@ -828,11 +880,11 @@ static int xpcs_do_config(struct dw_xpcs *xpcs, phy_interface_t interface,
 	if (!compat)
 		return -ENODEV;
 
-	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
-		ret = txgbe_xpcs_switch_mode(xpcs, interface);
-		if (ret)
-			return ret;
+	ret = xpcs_switch_interface_mode(compat, xpcs, interface, neg_mode);
+	if (ret)
+		return ret;
 
+	if (xpcs->info.pma == WX_TXGBE_XPCS_PMA_10G_ID) {
 		/* Wangxun devices need backplane CL37 AN enabled for
 		 * SGMII and 1000base-X
 		 */
-- 
2.34.1

Re: [PATCH net-next v7 2/7] net: pcs: xpcs: re-initiate clause 37 Auto-negotiation

[Russell King (Oracle)]

On Thu, Feb 06, 2025 at 09:18:54PM +0800, Choong Yong Liang wrote:
> The xpcs_switch_interface_mode function was introduced to handle
> interface switching.
> 
> According to the XPCS datasheet, a soft reset is required to initiate
> Clause 37 auto-negotiation when the XPCS switches interface modes.

Hmm. Given that description, taking it literally, claus 37
auto-negotiation is 1000BASE-X, not Cisco SGMII (which isn't an IEEE
802.3 standard.) Are you absolutely sure that this applies to Cisco
SGMII?

If the reset is required when switching to SGMII, should it be done
before or after configuring the XPCS for SGMII?

Also, if the reset is required, what happens if we're already using
SGMII, but AN has been disabled temporarily and is then re-enabled?
Is a reset required?

What about 1000BASE-X when AN is enabled or disabled and then switching
to SGMII?

> +static int xpcs_switch_to_aneg_c37_sgmii(const struct dw_xpcs_compat *compat,
> +					 struct dw_xpcs *xpcs,
> +					 unsigned int neg_mode)
> +{
> +	bool an_c37_enabled;
> +	int ret, mdio_ctrl;
> +
> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
> +		mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
> +		if (mdio_ctrl < 0)
> +			return mdio_ctrl;
> +
> +		an_c37_enabled = mdio_ctrl & BMCR_ANENABLE;
> +		if (!an_c37_enabled) {

I don't think that we need "an_c37_enabled" here, I think simply:

		if (!(mdio_ctrl & BMCR_ANENABLE)) {

would be sufficient.

> +			//Perform soft reset to initiate C37 auto-negotiation
> +			ret = xpcs_soft_reset(xpcs, compat);
> +			if (ret)
> +				return ret;
> +		}
> +	}
> +	return 0;

I'm also wondering (as above) whether this soft reset needs to happen
_after_ xpcs_config_aneg_c37_sgmii() has done its work - this function
temporarily disables AN while it's doing its work.

I'm also wondering whether AN being disabled is really a deciding
factor (e.g. when switching from 1000BASE-X AN-enabled to SGMII, is a
reset required?)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next v7 2/7] net: pcs: xpcs: re-initiate clause 37 Auto-negotiation

[Choong Yong Liang]

On 6/2/2025 11:30 pm, Russell King (Oracle) wrote:
> On Thu, Feb 06, 2025 at 09:18:54PM +0800, Choong Yong Liang wrote:
>> The xpcs_switch_interface_mode function was introduced to handle
>> interface switching.
>>
>> According to the XPCS datasheet, a soft reset is required to initiate
>> Clause 37 auto-negotiation when the XPCS switches interface modes.
> 
> Hmm. Given that description, taking it literally, claus 37
> auto-negotiation is 1000BASE-X, not Cisco SGMII (which isn't an IEEE
> 802.3 standard.) Are you absolutely sure that this applies to Cisco
> SGMII?
> 
Hi Russell,

Yes, you are correct that Clause 37 auto-negotiation is for 1000BASE-X. 
However, I do not believe it applies to Cisco SGMII. The XPCS implements 
Clause 37 auto-negotiation for both 1000BASE-X and SGMII.

> If the reset is required when switching to SGMII, should it be done
> before or after configuring the XPCS for SGMII?
> 
A soft reset is required before configuring the XPCS for SGMII. Based on 
the existing XPCS handling in the initial state, the xpcs_create() function 
will be called, and then xpcs->need_reset will be set to true. Later on, 
phylink_major_config() will call xpcs_pre_config() to perform the 
xpcs_soft_reset(), and then it will continue with xpcs_config().

I apologize for missing this patch: 
https://patchwork.kernel.org/project/netdevbpf/patch/E1svfMA-005ZI3-Va@rmk-PC.armlinux.org.uk/

I think I should move xpcs_switch_interface_mode() to xpcs_pre_config() and 
just update xpcs->need_reset instead of implementing my own method for 
calling xpcs_soft_reset().

> Also, if the reset is required, what happens if we're already using
> SGMII, but AN has been disabled temporarily and is then re-enabled?
> Is a reset required?
> 
Good point. I cannot find this scenario in the datasheet. Please allow me 
some time to test this scenario. I will update you with the results.

> What about 1000BASE-X when AN is enabled or disabled and then switching
> to SGMII?
> 
According to the datasheet, a soft reset is required.

>> +static int xpcs_switch_to_aneg_c37_sgmii(const struct dw_xpcs_compat *compat,
>> +					 struct dw_xpcs *xpcs,
>> +					 unsigned int neg_mode)
>> +{
>> +	bool an_c37_enabled;
>> +	int ret, mdio_ctrl;
>> +
>> +	if (neg_mode == PHYLINK_PCS_NEG_INBAND_ENABLED) {
>> +		mdio_ctrl = xpcs_read(xpcs, MDIO_MMD_VEND2, MII_BMCR);
>> +		if (mdio_ctrl < 0)
>> +			return mdio_ctrl;
>> +
>> +		an_c37_enabled = mdio_ctrl & BMCR_ANENABLE;
>> +		if (!an_c37_enabled) {
> 
> I don't think that we need "an_c37_enabled" here, I think simply:
> 
> 		if (!(mdio_ctrl & BMCR_ANENABLE)) {
> 
> would be sufficient.
> 
>> +			//Perform soft reset to initiate C37 auto-negotiation
>> +			ret = xpcs_soft_reset(xpcs, compat);
>> +			if (ret)
>> +				return ret;
>> +		}
>> +	}
>> +	return 0;
> 
> I'm also wondering (as above) whether this soft reset needs to happen
> _after_ xpcs_config_aneg_c37_sgmii() has done its work - this function
> temporarily disables AN while it's doing its work.
> 
Based on the programming sequence in the datasheet, it is not necessary to 
perform a soft reset after xpcs_config_aneg_c37_sgmii() has completed its work.

> I'm also wondering whether AN being disabled is really a deciding
> factor (e.g. when switching from 1000BASE-X AN-enabled to SGMII, is a
> reset required?)
> 

Thank you for pointing this out. The datasheet only mentions performing a 
soft reset when switching to the 1000BASE-X and SGMII interfaces, and it 
does not specify whether AN needs to be enabled or disabled. I thought 
adding a check would reduce the calls to the soft reset. However, I did not 
consider the scenario of switching from 1000BASE-X with AN enabled to SGMII 
with AN enabled. This scenario might cause regression. I will remove all 
the checks and just perform a soft reset when switching to the SGMII interface.

Re: [PATCH net-next v7 2/7] net: pcs: xpcs: re-initiate clause 37 Auto-negotiation

[Andrew Lunn]

> Good point. I cannot find this scenario in the datasheet. Please allow me
> some time to test this scenario. I will update you with the results.

By data sheet, do you mean documentation from Synopsis, or is this an
internal document? Assuming the hardware engineers have not hacked up
the Synopsis IP too much, the Synopsis documentation is probably the
most accurate you have.

> > What about 1000BASE-X when AN is enabled or disabled and then switching
> > to SGMII?
> > 
> According to the datasheet, a soft reset is required.

Do you know if this is specific to Intels integration of the Synopsis
IP, or this is part of the core licensed IP?

We need to understand when we need a quirk because intel did something
odd, or it is part of the licensed IP and should happen for all
devices using the IP.

	Andrew

Re: [PATCH net-next v7 2/7] net: pcs: xpcs: re-initiate clause 37 Auto-negotiation

[Choong Yong Liang]

On 7/2/2025 9:32 pm, Andrew Lunn wrote:
>> Good point. I cannot find this scenario in the datasheet. Please allow me
>> some time to test this scenario. I will update you with the results.
> 
> By data sheet, do you mean documentation from Synopsis, or is this an
> internal document? Assuming the hardware engineers have not hacked up
> the Synopsis IP too much, the Synopsis documentation is probably the
> most accurate you have.
> 
>>> What about 1000BASE-X when AN is enabled or disabled and then switching
>>> to SGMII?
>>>
>> According to the datasheet, a soft reset is required.
> 
> Do you know if this is specific to Intels integration of the Synopsis
> IP, or this is part of the core licensed IP?
> 
> We need to understand when we need a quirk because intel did something
> odd, or it is part of the licensed IP and should happen for all
> devices using the IP.
> 
> 	Andrew
> 

Hi Andrew,

Thank you for your questions. Just to clarify, when I mention the 
"datasheet," I am referring to the documentation from "Synopsis - 
DesignWare Cores Ethernet PCS" and not specific to Intel's documentation.

Re: [PATCH net-next v7 2/7] net: pcs: xpcs: re-initiate clause 37 Auto-negotiation

[Choong Yong Liang]

On 6/2/2025 11:30 pm, Russell King (Oracle) wrote:
> Also, if the reset is required, what happens if we're already using
> SGMII, but AN has been disabled temporarily and is then re-enabled?
> Is a reset required?
> 

Hi Russell,

When we use SGMII with XPCS, the AN is disabled and then re-enabled. This 
process does not involve any PCS configuration, so a reset is not required.

I tested it with stmmac (dwmac5) + xpcs + Marvell (88E1510) PHY, and it 
works fine when AN is temporarily disabled and then re-enabled without a reset.

[PATCH net-next v7 3/7] arch: x86: add IPC mailbox accessor function and add SoC register access

[Choong Yong Liang]

From: "David E. Box" <david.e.box@linux.intel.com>

- Exports intel_pmc_ipc() for host access to the PMC IPC mailbox
- Add support to use IPC command allows host to access SoC registers
through PMC firmware that are otherwise inaccessible to the host due to
security policies.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Signed-off-by: Chao Qin <chao.qin@intel.com>
Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
---
 MAINTAINERS                                   |  2 +
 arch/x86/Kconfig                              |  9 +++
 arch/x86/platform/intel/Makefile              |  1 +
 arch/x86/platform/intel/pmc_ipc.c             | 75 +++++++++++++++++++
 .../linux/platform_data/x86/intel_pmc_ipc.h   | 34 +++++++++
 5 files changed, 121 insertions(+)
 create mode 100644 arch/x86/platform/intel/pmc_ipc.c
 create mode 100644 include/linux/platform_data/x86/intel_pmc_ipc.h

diff --git a/MAINTAINERS b/MAINTAINERS
index d1086e53a317..7a0681f83449 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -11862,8 +11862,10 @@ M:	Rajneesh Bhardwaj <irenic.rajneesh@gmail.com>
 M:	David E Box <david.e.box@intel.com>
 L:	platform-driver-x86@vger.kernel.org
 S:	Maintained
+F:	arch/x86/platform/intel/pmc_ipc.c
 F:	Documentation/ABI/testing/sysfs-platform-intel-pmc
 F:	drivers/platform/x86/intel/pmc/
+F:	linux/platform_data/x86/intel_pmc_ipc.h
 
 INTEL PMIC GPIO DRIVERS
 M:	Andy Shevchenko <andy@kernel.org>
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 87198d957e2f..631c1f10776c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -688,6 +688,15 @@ config X86_AMD_PLATFORM_DEVICE
 	  I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
 	  implemented under PINCTRL subsystem.
 
+config INTEL_PMC_IPC
+	tristate "Intel Core SoC Power Management Controller IPC mailbox"
+	depends on ACPI
+	help
+	  This option enables sideband register access support for Intel SoC
+	  power management controller IPC mailbox.
+
+	  If you don't require the option or are in doubt, say N.
+
 config IOSF_MBI
 	tristate "Intel SoC IOSF Sideband support for SoC platforms"
 	depends on PCI
diff --git a/arch/x86/platform/intel/Makefile b/arch/x86/platform/intel/Makefile
index dbee3b00f9d0..2f1805556d0c 100644
--- a/arch/x86/platform/intel/Makefile
+++ b/arch/x86/platform/intel/Makefile
@@ -1,2 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_IOSF_MBI)			+= iosf_mbi.o
+obj-$(CONFIG_INTEL_PMC_IPC)		+= pmc_ipc.o
diff --git a/arch/x86/platform/intel/pmc_ipc.c b/arch/x86/platform/intel/pmc_ipc.c
new file mode 100644
index 000000000000..a96234982710
--- /dev/null
+++ b/arch/x86/platform/intel/pmc_ipc.c
@@ -0,0 +1,75 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel Core SoC Power Management Controller IPC mailbox
+ *
+ * Copyright (c) 2023, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Authors: Choong Yong Liang <yong.liang.choong@linux.intel.com>
+ *          David E. Box <david.e.box@linux.intel.com>
+ */
+#include <linux/module.h>
+#include <linux/acpi.h>
+#include <linux/platform_data/x86/intel_pmc_ipc.h>
+
+#define PMC_IPCS_PARAM_COUNT           7
+
+int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf)
+{
+	struct acpi_buffer buffer = { ACPI_ALLOCATE_BUFFER, NULL };
+	union acpi_object params[PMC_IPCS_PARAM_COUNT] = {
+		{.type = ACPI_TYPE_INTEGER,},
+		{.type = ACPI_TYPE_INTEGER,},
+		{.type = ACPI_TYPE_INTEGER,},
+		{.type = ACPI_TYPE_INTEGER,},
+		{.type = ACPI_TYPE_INTEGER,},
+		{.type = ACPI_TYPE_INTEGER,},
+		{.type = ACPI_TYPE_INTEGER,},
+	};
+	struct acpi_object_list arg_list = { PMC_IPCS_PARAM_COUNT, params };
+	union acpi_object *obj;
+	int status;
+
+	if (!ipc_cmd || !rbuf)
+		return -EINVAL;
+
+	/*
+	 * 0: IPC Command
+	 * 1: IPC Sub Command
+	 * 2: Size
+	 * 3-6: Write Buffer for offset
+	 */
+	params[0].integer.value = ipc_cmd->cmd;
+	params[1].integer.value = ipc_cmd->sub_cmd;
+	params[2].integer.value = ipc_cmd->size;
+	params[3].integer.value = ipc_cmd->wbuf[0];
+	params[4].integer.value = ipc_cmd->wbuf[1];
+	params[5].integer.value = ipc_cmd->wbuf[2];
+	params[6].integer.value = ipc_cmd->wbuf[3];
+
+	status = acpi_evaluate_object(NULL, "\\IPCS", &arg_list, &buffer);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	obj = buffer.pointer;
+	/* Check if the number of elements in package is 5 */
+	if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
+		const union acpi_object *objs = obj->package.elements;
+
+		if ((u8)objs[0].integer.value != 0)
+			return -EINVAL;
+
+		rbuf[0] = objs[1].integer.value;
+		rbuf[1] = objs[2].integer.value;
+		rbuf[2] = objs[3].integer.value;
+		rbuf[3] = objs[4].integer.value;
+	} else {
+		return -EINVAL;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(intel_pmc_ipc);
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Intel PMC IPC Mailbox accessor");
diff --git a/include/linux/platform_data/x86/intel_pmc_ipc.h b/include/linux/platform_data/x86/intel_pmc_ipc.h
new file mode 100644
index 000000000000..d47b89f873fc
--- /dev/null
+++ b/include/linux/platform_data/x86/intel_pmc_ipc.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Intel Core SoC Power Management Controller Header File
+ *
+ * Copyright (c) 2023, Intel Corporation.
+ * All Rights Reserved.
+ *
+ * Authors: Choong Yong Liang <yong.liang.choong@linux.intel.com>
+ *          David E. Box <david.e.box@linux.intel.com>
+ */
+#ifndef INTEL_PMC_IPC_H
+#define INTEL_PMC_IPC_H
+
+#define IPC_SOC_REGISTER_ACCESS			0xAA
+#define IPC_SOC_SUB_CMD_READ			0x00
+#define IPC_SOC_SUB_CMD_WRITE			0x01
+
+struct pmc_ipc_cmd {
+	u32 cmd;
+	u32 sub_cmd;
+	u32 size;
+	u32 wbuf[4];
+};
+
+/**
+ * intel_pmc_ipc() - PMC IPC Mailbox accessor
+ * @ipc_cmd:  struct pmc_ipc_cmd prepared with input to send
+ * @rbuf:     Allocated u32[4] array for returned IPC data
+ *
+ * Return: 0 on success. Non-zero on mailbox error
+ */
+int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf);
+
+#endif /* INTEL_PMC_IPC_H */
-- 
2.34.1

Re: [PATCH net-next v7 3/7] arch: x86: add IPC mailbox accessor function and add SoC register access

[Dave Hansen]

On 2/6/25 05:18, Choong Yong Liang wrote:
> 
> - Exports intel_pmc_ipc() for host access to the PMC IPC mailbox
> - Add support to use IPC command allows host to access SoC registers 
> through PMC firmware that are otherwise inaccessible to the host due
> to security policies.

I'm not quite parsing that second bullet as a complete sentence.

But that sounds scary! Why is the fact that they are "otherwise
inaccessible" relevant here?

...
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 87198d957e2f..631c1f10776c 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -688,6 +688,15 @@ config X86_AMD_PLATFORM_DEVICE
>  	  I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
>  	  implemented under PINCTRL subsystem.
>  
> +config INTEL_PMC_IPC
> +	tristate "Intel Core SoC Power Management Controller IPC mailbox"
> +	depends on ACPI
> +	help
> +	  This option enables sideband register access support for Intel SoC
> +	  power management controller IPC mailbox.
> +
> +	  If you don't require the option or are in doubt, say N.

Could we perhaps beef this up a bit to help users figure out if they
want to turn this on? Really the only word in the entire help text
that's useful is "Intel".

I'm not even sure we *want* to expose this to users. Can we just leave
it as:

	config INTEL_PMC_IPC
		def_tristate n
		depends on ACPI

so that it only gets enabled by the "select" in the other patches?

> + * Authors: Choong Yong Liang <yong.liang.choong@linux.intel.com>
> + *          David E. Box <david.e.box@linux.intel.com>

I'd probably just leave the authors bit out. It might have been useful
in the 90's, but that's what git is for today.

> +	obj = buffer.pointer;
> +	/* Check if the number of elements in package is 5 */
> +	if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count == 5) {
> +		const union acpi_object *objs = obj->package.elements;
> +

The comment there is just not super useful. It might be useful to say
*why* the number of elements needs to be 5.

> +EXPORT_SYMBOL(intel_pmc_ipc);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Intel PMC IPC Mailbox accessor");

Honestly, is this even worth being a module? How much code are we
talking about here?

> diff --git a/include/linux/platform_data/x86/intel_pmc_ipc.h b/include/linux/platform_data/x86/intel_pmc_ipc.h
> new file mode 100644
> index 000000000000..d47b89f873fc
> --- /dev/null
> +++ b/include/linux/platform_data/x86/intel_pmc_ipc.h
> @@ -0,0 +1,34 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Intel Core SoC Power Management Controller Header File
> + *
> + * Copyright (c) 2023, Intel Corporation.
> + * All Rights Reserved.
...

This copyright is a _bit_ funky. It's worth at least saying in the cover
letter that this patch has been sitting untouched for over a year, thus
the old copyright.

Or, if you've done actual work with it, I'd assume the copyright needs
to get updated.

> +struct pmc_ipc_cmd {
> +	u32 cmd;
> +	u32 sub_cmd;
> +	u32 size;
> +	u32 wbuf[4];
> +};
> +
> +/**
> + * intel_pmc_ipc() - PMC IPC Mailbox accessor
> + * @ipc_cmd:  struct pmc_ipc_cmd prepared with input to send

You probably don't need to restate the literal type of ipc_cmd.

> + * @rbuf:     Allocated u32[4] array for returned IPC data

The "Allocated" thing here threw me a bit. Does this mean it *must* be
"allocated" as in it comes from kmalloc()? Or can it be on the stack? Or
part of a static variable?

> + * Return: 0 on success. Non-zero on mailbox error
> + */
> +int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf);

Also, if it can *only* be u32[4], then the best way to declare it is:

struct pmc_ipc_rbuf {
	u32 buf[4];
};

and:

int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd,
		  struct pmc_ipc_rbuf rbuf *rbuf);

Then you don't need a comment saying that it must be a u32[4]. It's
implied in the structure.

Re: [PATCH net-next v7 3/7] arch: x86: add IPC mailbox accessor function and add SoC register access

[Andrew Lunn]

On Thu, Feb 06, 2025 at 08:46:24AM -0800, Dave Hansen wrote:
> On 2/6/25 05:18, Choong Yong Liang wrote:
> > 
> > - Exports intel_pmc_ipc() for host access to the PMC IPC mailbox
> > - Add support to use IPC command allows host to access SoC registers 
> > through PMC firmware that are otherwise inaccessible to the host due
> > to security policies.
> 
> I'm not quite parsing that second bullet as a complete sentence.
> 
> But that sounds scary! Why is the fact that they are "otherwise
> inaccessible" relevant here?

And i wounder what other interesting things can be accessed in this
indirect manor, which the security policy would not otherwise allow.

Somebody is going to have fun here. Or is already have fun here.

	 Andrew

Re: [PATCH net-next v7 3/7] arch: x86: add IPC mailbox accessor function and add SoC register access

[David E. Box]

On Thu, 2025-02-06 at 08:46 -0800, Dave Hansen wrote:
> On 2/6/25 05:18, Choong Yong Liang wrote:
> > 
> > - Exports intel_pmc_ipc() for host access to the PMC IPC mailbox
> > - Add support to use IPC command allows host to access SoC registers 
> > through PMC firmware that are otherwise inaccessible to the host due
> > to security policies.
> I'm not quite parsing that second bullet as a complete sentence.
> 
> But that sounds scary! Why is the fact that they are "otherwise
> inaccessible" relevant here?

The PMC IPC mailbox is a host interface to the PMC. Its purpose is to allow the
host to access certain areas of the PMC that are restricted from direct MMIO
access due to security policies. Other parts of the PMC are accessible via MMIO
(most of what the intel_pmc_core driver touches with is MMIO), so I think the
original statement was trying to explain why the mailbox is needed instead of
MMIO in this case. However, I agree that the mention of security policies is not
relevant to the change itself.

> ...
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index 87198d957e2f..631c1f10776c 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -688,6 +688,15 @@ config X86_AMD_PLATFORM_DEVICE
> >  	  I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
> >  	  implemented under PINCTRL subsystem.
> >  
> > +config INTEL_PMC_IPC
> > +	tristate "Intel Core SoC Power Management Controller IPC mailbox"
> > +	depends on ACPI
> > +	help
> > +	  This option enables sideband register access support for Intel
> > SoC
> > +	  power management controller IPC mailbox.
> > +
> > +	  If you don't require the option or are in doubt, say N.
> 
> Could we perhaps beef this up a bit to help users figure out if they
> want to turn this on? Really the only word in the entire help text
> that's useful is "Intel".
> 
> I'm not even sure we *want* to expose this to users. Can we just leave
> it as:
> 
> 	config INTEL_PMC_IPC
> 		def_tristate n
> 		depends on ACPI
> 
> so that it only gets enabled by the "select" in the other patches?

I agree with this change Choong. This can be selected by the driver that's using
it. There's no need to expose it to users.

> 
> > + * Authors: Choong Yong Liang <yong.liang.choong@linux.intel.com>
> > + *          David E. Box <david.e.box@linux.intel.com>
> 
> I'd probably just leave the authors bit out. It might have been useful
> in the 90's, but that's what git is for today.
> 
> > +	obj = buffer.pointer;
> > +	/* Check if the number of elements in package is 5 */
> > +	if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count ==
> > 5) {
> > +		const union acpi_object *objs = obj->package.elements;
> > +
> 
> The comment there is just not super useful. It might be useful to say
> *why* the number of elements needs to be 5.
> 
> > +EXPORT_SYMBOL(intel_pmc_ipc);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_DESCRIPTION("Intel PMC IPC Mailbox accessor");
> 
> Honestly, is this even worth being a module? How much code are we
> talking about here?

Yeah, this doesn't need to be a module either.

David

> 
> > diff --git a/include/linux/platform_data/x86/intel_pmc_ipc.h
> > b/include/linux/platform_data/x86/intel_pmc_ipc.h
> > new file mode 100644
> > index 000000000000..d47b89f873fc
> > --- /dev/null
> > +++ b/include/linux/platform_data/x86/intel_pmc_ipc.h
> > @@ -0,0 +1,34 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Intel Core SoC Power Management Controller Header File
> > + *
> > + * Copyright (c) 2023, Intel Corporation.
> > + * All Rights Reserved.
> ...
> 
> This copyright is a _bit_ funky. It's worth at least saying in the cover
> letter that this patch has been sitting untouched for over a year, thus
> the old copyright.
> 
> Or, if you've done actual work with it, I'd assume the copyright needs
> to get updated.
> 
> > +struct pmc_ipc_cmd {
> > +	u32 cmd;
> > +	u32 sub_cmd;
> > +	u32 size;
> > +	u32 wbuf[4];
> > +};
> > +
> > +/**
> > + * intel_pmc_ipc() - PMC IPC Mailbox accessor
> > + * @ipc_cmd:  struct pmc_ipc_cmd prepared with input to send
> 
> You probably don't need to restate the literal type of ipc_cmd.
> 
> > + * @rbuf:     Allocated u32[4] array for returned IPC data
> 
> The "Allocated" thing here threw me a bit. Does this mean it *must* be
> "allocated" as in it comes from kmalloc()? Or can it be on the stack? Or
> part of a static variable?
> 
> > + * Return: 0 on success. Non-zero on mailbox error
> > + */
> > +int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd, u32 *rbuf);
> 
> Also, if it can *only* be u32[4], then the best way to declare it is:
> 
> struct pmc_ipc_rbuf {
> 	u32 buf[4];
> };
> 
> and:
> 
> int intel_pmc_ipc(struct pmc_ipc_cmd *ipc_cmd,
> 		  struct pmc_ipc_rbuf rbuf *rbuf);
> 
> Then you don't need a comment saying that it must be a u32[4]. It's
> implied in the structure.

Re: [PATCH net-next v7 3/7] arch: x86: add IPC mailbox accessor function and add SoC register access

[Choong Yong Liang]

On 20/2/2025 1:01 am, David E. Box wrote:
> On Thu, 2025-02-06 at 08:46 -0800, Dave Hansen wrote:
>> On 2/6/25 05:18, Choong Yong Liang wrote:
>>>
>>> - Exports intel_pmc_ipc() for host access to the PMC IPC mailbox
>>> - Add support to use IPC command allows host to access SoC registers
>>> through PMC firmware that are otherwise inaccessible to the host due
>>> to security policies.
>> I'm not quite parsing that second bullet as a complete sentence.
>>
>> But that sounds scary! Why is the fact that they are "otherwise
>> inaccessible" relevant here?
> 
> The PMC IPC mailbox is a host interface to the PMC. Its purpose is to allow the
> host to access certain areas of the PMC that are restricted from direct MMIO
> access due to security policies. Other parts of the PMC are accessible via MMIO
> (most of what the intel_pmc_core driver touches with is MMIO), so I think the
> original statement was trying to explain why the mailbox is needed instead of
> MMIO in this case. However, I agree that the mention of security policies is not
> relevant to the change itself.
> 
>> ...
>>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>>> index 87198d957e2f..631c1f10776c 100644
>>> --- a/arch/x86/Kconfig
>>> +++ b/arch/x86/Kconfig
>>> @@ -688,6 +688,15 @@ config X86_AMD_PLATFORM_DEVICE
>>>   	  I2C and UART depend on COMMON_CLK to set clock. GPIO driver is
>>>   	  implemented under PINCTRL subsystem.
>>>   
>>> +config INTEL_PMC_IPC
>>> +	tristate "Intel Core SoC Power Management Controller IPC mailbox"
>>> +	depends on ACPI
>>> +	help
>>> +	  This option enables sideband register access support for Intel
>>> SoC
>>> +	  power management controller IPC mailbox.
>>> +
>>> +	  If you don't require the option or are in doubt, say N.
>>
>> Could we perhaps beef this up a bit to help users figure out if they
>> want to turn this on? Really the only word in the entire help text
>> that's useful is "Intel".
>>
>> I'm not even sure we *want* to expose this to users. Can we just leave
>> it as:
>>
>> 	config INTEL_PMC_IPC
>> 		def_tristate n
>> 		depends on ACPI
>>
>> so that it only gets enabled by the "select" in the other patches?
> 
> I agree with this change Choong. This can be selected by the driver that's using
> it. There's no need to expose it to users.
> 
>>
>>> + * Authors: Choong Yong Liang <yong.liang.choong@linux.intel.com>
>>> + *          David E. Box <david.e.box@linux.intel.com>
>>
>> I'd probably just leave the authors bit out. It might have been useful
>> in the 90's, but that's what git is for today.
>>
>>> +	obj = buffer.pointer;
>>> +	/* Check if the number of elements in package is 5 */
>>> +	if (obj && obj->type == ACPI_TYPE_PACKAGE && obj->package.count ==
>>> 5) {
>>> +		const union acpi_object *objs = obj->package.elements;
>>> +
>>
>> The comment there is just not super useful. It might be useful to say
>> *why* the number of elements needs to be 5.
>>
>>> +EXPORT_SYMBOL(intel_pmc_ipc);
>>> +
>>> +MODULE_LICENSE("GPL");
>>> +MODULE_DESCRIPTION("Intel PMC IPC Mailbox accessor");
>>
>> Honestly, is this even worth being a module? How much code are we
>> talking about here?
> 
> Yeah, this doesn't need to be a module either.
> 
> David
> 

Hi David,

Thank you for the confirmation.
Let's work together to address the comments.

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

[Choong Yong Liang]

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@linux.intel.com>
---
 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++) {
+		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;
+
+	priv->plat->phy_interface = interface;
+
+	intel_serdes_powerdown(ndev, intel_priv);
+	intel_serdes_powerup(ndev, intel_priv);
+
+	return ret;
+}
+
 static void common_default_data(struct plat_stmmacenet_data *plat)
 {
 	plat->clk_csr = 2;	/* clk_csr_i = 20-35MHz & MDC = clk_csr_i/16 */
@@ -650,7 +790,7 @@ static int ehl_sgmii_data(struct pci_dev *pdev,
 	plat->speed_mode_2500 = intel_speed_mode_2500;
 	plat->serdes_powerup = intel_serdes_powerup;
 	plat->serdes_powerdown = intel_serdes_powerdown;
-
+	plat->mac_finish = intel_mac_finish;
 	plat->clk_ptp_rate = 204800000;
 
 	return ehl_common_data(pdev, plat);
@@ -709,6 +849,7 @@ static int ehl_pse0_sgmii1g_data(struct pci_dev *pdev,
 	plat->speed_mode_2500 = intel_speed_mode_2500;
 	plat->serdes_powerup = intel_serdes_powerup;
 	plat->serdes_powerdown = intel_serdes_powerdown;
+	plat->mac_finish = intel_mac_finish;
 	return ehl_pse0_common_data(pdev, plat);
 }
 
@@ -750,6 +891,7 @@ static int ehl_pse1_sgmii1g_data(struct pci_dev *pdev,
 	plat->speed_mode_2500 = intel_speed_mode_2500;
 	plat->serdes_powerup = intel_serdes_powerup;
 	plat->serdes_powerdown = intel_serdes_powerdown;
+	plat->mac_finish = intel_mac_finish;
 	return ehl_pse1_common_data(pdev, plat);
 }
 
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.h b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.h
index 0a37987478c1..a12f8e65f89f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.h
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.h
@@ -50,4 +50,33 @@
 #define PCH_PTP_CLK_FREQ_19_2MHZ	(GMAC_GPO0)
 #define PCH_PTP_CLK_FREQ_200MHZ		(0)
 
+/* Modphy Register index */
+#define R_PCH_FIA_15_PCR_LOS1_REG_BASE			8
+#define R_PCH_FIA_15_PCR_LOS2_REG_BASE			9
+#define R_PCH_FIA_15_PCR_LOS3_REG_BASE			10
+#define R_PCH_FIA_15_PCR_LOS4_REG_BASE			11
+#define R_PCH_FIA_15_PCR_LOS5_REG_BASE			12
+#define B_PCH_FIA_PCR_L0O				GENMASK(3, 0)
+#define PID_MODPHY1_B_MODPHY_PCR_LCPLL_DWORD0		13
+#define PID_MODPHY1_N_MODPHY_PCR_LCPLL_DWORD2		14
+#define PID_MODPHY1_N_MODPHY_PCR_LCPLL_DWORD7		15
+#define PID_MODPHY1_N_MODPHY_PCR_LPPLL_DWORD10		16
+#define PID_MODPHY1_N_MODPHY_PCR_CMN_ANA_DWORD30	17
+#define PID_MODPHY3_B_MODPHY_PCR_LCPLL_DWORD0		18
+#define PID_MODPHY3_N_MODPHY_PCR_LCPLL_DWORD2		19
+#define PID_MODPHY3_N_MODPHY_PCR_LCPLL_DWORD7		20
+#define PID_MODPHY3_N_MODPHY_PCR_LPPLL_DWORD10		21
+#define PID_MODPHY3_N_MODPHY_PCR_CMN_ANA_DWORD30	22
+
+#define B_MODPHY_PCR_LCPLL_DWORD0_1G		0x46AAAA41
+#define N_MODPHY_PCR_LCPLL_DWORD2_1G		0x00000139
+#define N_MODPHY_PCR_LCPLL_DWORD7_1G		0x002A0003
+#define N_MODPHY_PCR_LPPLL_DWORD10_1G		0x00170008
+#define N_MODPHY_PCR_CMN_ANA_DWORD30_1G		0x0000D4AC
+#define B_MODPHY_PCR_LCPLL_DWORD0_2P5G		0x58555551
+#define N_MODPHY_PCR_LCPLL_DWORD2_2P5G		0x0000012D
+#define N_MODPHY_PCR_LCPLL_DWORD7_2P5G		0x001F0003
+#define N_MODPHY_PCR_LPPLL_DWORD10_2P5G		0x00170008
+#define N_MODPHY_PCR_CMN_ANA_DWORD30_2P5G	0x8200ACAC
+
 #endif /* __DWMAC_INTEL_H__ */
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index c9878a612e53..83131bf7a5ef 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -236,6 +236,10 @@ struct plat_stmmacenet_data {
 	int (*serdes_powerup)(struct net_device *ndev, void *priv);
 	void (*serdes_powerdown)(struct net_device *ndev, void *priv);
 	void (*speed_mode_2500)(struct net_device *ndev, void *priv);
+	int (*mac_finish)(struct net_device *ndev,
+			  void *priv,
+			  unsigned int mode,
+			  phy_interface_t interface);
 	void (*ptp_clk_freq_config)(struct stmmac_priv *priv);
 	int (*init)(struct platform_device *pdev, void *priv);
 	void (*exit)(struct platform_device *pdev, void *priv);
-- 
2.34.1

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@linux.intel.com>
> ---
>  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.

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

[Choong Yong Liang]

On 6/2/2025 9:31 pm, Ilpo Järvinen wrote:
>> +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.
> 
Hi Ilpo,
Thank you for your feedback. I used const int max_fia_regs = 5; to leverage 
type safety and scope control in modern C. However, I understand that 
#define is a common practice. Please let me know if you prefer I switch to 
#define for consistency.

>> +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.
> 
Thank you for pointing out the potential issues with the intel_priv fields. 
I will move the changes from patch 6 into this patch to avoid NULL pointer 
de-reference issues and unused variable warnings. This will ensure 
everything is properly set up and used within the same patch.

[PATCH net-next v7 5/7] net: stmmac: configure SerDes on mac_finish

[Choong Yong Liang]

SerDes will configure according to the provided interface mode after
finish a major reconfiguration of the interface mode.

Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d04543e5697b..1c2c83d17f5a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1124,6 +1124,18 @@ static int stmmac_mac_enable_tx_lpi(struct phylink_config *config, u32 timer,
 	return 0;
 }
 
+static int stmmac_mac_finish(struct phylink_config *config, unsigned int mode,
+			     phy_interface_t interface)
+{
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct stmmac_priv *priv = netdev_priv(ndev);
+
+	if (priv->plat->mac_finish)
+		priv->plat->mac_finish(ndev, priv->plat->bsp_priv, mode, interface);
+
+	return 0;
+}
+
 static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
 	.mac_get_caps = stmmac_mac_get_caps,
 	.mac_select_pcs = stmmac_mac_select_pcs,
@@ -1132,6 +1144,7 @@ static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
 	.mac_link_up = stmmac_mac_link_up,
 	.mac_disable_tx_lpi = stmmac_mac_disable_tx_lpi,
 	.mac_enable_tx_lpi = stmmac_mac_enable_tx_lpi,
+	.mac_finish = stmmac_mac_finish,
 };
 
 /**
-- 
2.34.1

[PATCH net-next v7 6/7] stmmac: intel: interface switching support for EHL platform

[Choong Yong Liang]

The intel_config_serdes function was provided to handle interface mode
changes for the EHL platform.

The Modphy register lane was provided to configure the serdes when
changing interface modes.

Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 31 ++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 837fd3fbaedb..e7f5d023eaf2 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -86,6 +86,8 @@ static const struct pmc_serdes_regs pid_modphy1_2p5g_regs[] = {
 	{}
 };
 
+static const int ehl_tsn_lane_regs[] = {7, 8, 9, 10, 11};
+
 static int stmmac_pci_find_phy_addr(struct pci_dev *pdev,
 				    const struct dmi_system_id *dmi_list)
 {
@@ -764,6 +766,8 @@ static int intel_mgbe_common_data(struct pci_dev *pdev,
 static int ehl_common_data(struct pci_dev *pdev,
 			   struct plat_stmmacenet_data *plat)
 {
+	struct intel_priv_data *intel_priv = plat->bsp_priv;
+
 	plat->rx_queues_to_use = 8;
 	plat->tx_queues_to_use = 8;
 	plat->flags |= STMMAC_FLAG_USE_PHY_WOL;
@@ -779,12 +783,17 @@ static int ehl_common_data(struct pci_dev *pdev,
 	plat->safety_feat_cfg->prtyen = 0;
 	plat->safety_feat_cfg->tmouten = 0;
 
+	intel_priv->tsn_lane_regs = ehl_tsn_lane_regs;
+	intel_priv->max_tsn_lane_regs = ARRAY_SIZE(ehl_tsn_lane_regs);
+
 	return intel_mgbe_common_data(pdev, plat);
 }
 
 static int ehl_sgmii_data(struct pci_dev *pdev,
 			  struct plat_stmmacenet_data *plat)
 {
+	struct intel_priv_data *intel_priv = plat->bsp_priv;
+
 	plat->bus_id = 1;
 	plat->phy_interface = PHY_INTERFACE_MODE_SGMII;
 	plat->speed_mode_2500 = intel_speed_mode_2500;
@@ -793,6 +802,11 @@ static int ehl_sgmii_data(struct pci_dev *pdev,
 	plat->mac_finish = intel_mac_finish;
 	plat->clk_ptp_rate = 204800000;
 
+	intel_priv->pid_1g.regs = pid_modphy3_1g_regs;
+	intel_priv->pid_1g.num_regs = ARRAY_SIZE(pid_modphy3_1g_regs);
+	intel_priv->pid_2p5g.regs = pid_modphy3_2p5g_regs;
+	intel_priv->pid_2p5g.num_regs = ARRAY_SIZE(pid_modphy3_2p5g_regs);
+
 	return ehl_common_data(pdev, plat);
 }
 
@@ -845,11 +859,18 @@ static struct stmmac_pci_info ehl_pse0_rgmii1g_info = {
 static int ehl_pse0_sgmii1g_data(struct pci_dev *pdev,
 				 struct plat_stmmacenet_data *plat)
 {
+	struct intel_priv_data *intel_priv = plat->bsp_priv;
+
 	plat->phy_interface = PHY_INTERFACE_MODE_SGMII;
-	plat->speed_mode_2500 = intel_speed_mode_2500;
 	plat->serdes_powerup = intel_serdes_powerup;
 	plat->serdes_powerdown = intel_serdes_powerdown;
 	plat->mac_finish = intel_mac_finish;
+
+	intel_priv->pid_1g.regs = pid_modphy1_1g_regs;
+	intel_priv->pid_1g.num_regs = ARRAY_SIZE(pid_modphy1_1g_regs);
+	intel_priv->pid_2p5g.regs = pid_modphy1_2p5g_regs;
+	intel_priv->pid_2p5g.num_regs = ARRAY_SIZE(pid_modphy1_2p5g_regs);
+
 	return ehl_pse0_common_data(pdev, plat);
 }
 
@@ -887,11 +908,19 @@ static struct stmmac_pci_info ehl_pse1_rgmii1g_info = {
 static int ehl_pse1_sgmii1g_data(struct pci_dev *pdev,
 				 struct plat_stmmacenet_data *plat)
 {
+	struct intel_priv_data *intel_priv = plat->bsp_priv;
+
 	plat->phy_interface = PHY_INTERFACE_MODE_SGMII;
 	plat->speed_mode_2500 = intel_speed_mode_2500;
 	plat->serdes_powerup = intel_serdes_powerup;
 	plat->serdes_powerdown = intel_serdes_powerdown;
 	plat->mac_finish = intel_mac_finish;
+
+	intel_priv->pid_1g.regs = pid_modphy1_1g_regs;
+	intel_priv->pid_1g.num_regs = ARRAY_SIZE(pid_modphy1_1g_regs);
+	intel_priv->pid_2p5g.regs = pid_modphy1_2p5g_regs;
+	intel_priv->pid_2p5g.num_regs = ARRAY_SIZE(pid_modphy1_2p5g_regs);
+
 	return ehl_pse1_common_data(pdev, plat);
 }
 
-- 
2.34.1

[PATCH net-next v7 7/7] stmmac: intel: interface switching support for ADL-N platform

[Choong Yong Liang]

The intel_config_serdes function was provided to handle interface mode
changes for the ADL-N platform.

The Modphy register lane was provided to configure the serdes when
changing interface modes.

Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com>
---
 .../net/ethernet/stmicro/stmmac/dwmac-intel.c | 52 ++++++++++++++++++-
 1 file changed, 51 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index e7f5d023eaf2..4958b9a32879 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -87,6 +87,7 @@ static const struct pmc_serdes_regs pid_modphy1_2p5g_regs[] = {
 };
 
 static const int ehl_tsn_lane_regs[] = {7, 8, 9, 10, 11};
+static const int adln_tsn_lane_regs[] = {6};
 
 static int stmmac_pci_find_phy_addr(struct pci_dev *pdev,
 				    const struct dmi_system_id *dmi_list)
@@ -1006,6 +1007,55 @@ static int adls_sgmii_phy1_data(struct pci_dev *pdev,
 static struct stmmac_pci_info adls_sgmii1g_phy1_info = {
 	.setup = adls_sgmii_phy1_data,
 };
+
+static int adln_common_data(struct pci_dev *pdev,
+			    struct plat_stmmacenet_data *plat)
+{
+	struct intel_priv_data *intel_priv = plat->bsp_priv;
+
+	plat->rx_queues_to_use = 6;
+	plat->tx_queues_to_use = 4;
+	plat->clk_ptp_rate = 204800000;
+
+	plat->safety_feat_cfg->tsoee = 1;
+	plat->safety_feat_cfg->mrxpee = 0;
+	plat->safety_feat_cfg->mestee = 1;
+	plat->safety_feat_cfg->mrxee = 1;
+	plat->safety_feat_cfg->mtxee = 1;
+	plat->safety_feat_cfg->epsi = 0;
+	plat->safety_feat_cfg->edpp = 0;
+	plat->safety_feat_cfg->prtyen = 0;
+	plat->safety_feat_cfg->tmouten = 0;
+
+	intel_priv->tsn_lane_regs = adln_tsn_lane_regs;
+	intel_priv->max_tsn_lane_regs = ARRAY_SIZE(adln_tsn_lane_regs);
+
+	return intel_mgbe_common_data(pdev, plat);
+}
+
+static int adln_sgmii_phy0_data(struct pci_dev *pdev,
+				struct plat_stmmacenet_data *plat)
+{
+	struct intel_priv_data *intel_priv = plat->bsp_priv;
+
+	plat->bus_id = 1;
+	plat->phy_interface = PHY_INTERFACE_MODE_SGMII;
+	plat->serdes_powerup = intel_serdes_powerup;
+	plat->serdes_powerdown = intel_serdes_powerdown;
+	plat->mac_finish = intel_mac_finish;
+
+	intel_priv->pid_1g.regs = pid_modphy1_1g_regs;
+	intel_priv->pid_1g.num_regs = ARRAY_SIZE(pid_modphy1_1g_regs);
+	intel_priv->pid_2p5g.regs = pid_modphy1_2p5g_regs;
+	intel_priv->pid_2p5g.num_regs = ARRAY_SIZE(pid_modphy1_2p5g_regs);
+
+	return adln_common_data(pdev, plat);
+}
+
+static struct stmmac_pci_info adln_sgmii1g_phy0_info = {
+	.setup = adln_sgmii_phy0_data,
+};
+
 static const struct stmmac_pci_func_data galileo_stmmac_func_data[] = {
 	{
 		.func = 6,
@@ -1388,7 +1438,7 @@ static const struct pci_device_id intel_eth_pci_id_table[] = {
 	{ PCI_DEVICE_DATA(INTEL, TGLH_SGMII1G_1, &tgl_sgmii1g_phy1_info) },
 	{ PCI_DEVICE_DATA(INTEL, ADLS_SGMII1G_0, &adls_sgmii1g_phy0_info) },
 	{ PCI_DEVICE_DATA(INTEL, ADLS_SGMII1G_1, &adls_sgmii1g_phy1_info) },
-	{ PCI_DEVICE_DATA(INTEL, ADLN_SGMII1G, &tgl_sgmii1g_phy0_info) },
+	{ PCI_DEVICE_DATA(INTEL, ADLN_SGMII1G, &adln_sgmii1g_phy0_info) },
 	{ PCI_DEVICE_DATA(INTEL, RPLP_SGMII1G, &tgl_sgmii1g_phy0_info) },
 	{}
 };
-- 
2.34.1