diff mbox series

[net-next,1/2] net: pcs: xpcs: enable skip xPCS soft reset

Message ID 20210809102229.933748-2-vee.khee.wong@linux.intel.com
State New
Headers show
Series Intel AlderLake-S 2.5Gbps link speed support | expand

Commit Message

Wong Vee Khee Aug. 9, 2021, 10:22 a.m. UTC 
From: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>

Unlike any other platforms, Intel AlderLake-S uses Synopsys SerDes where
all the SerDes PLL configurations are controlled by the xPCS at the BIOS
level. If the driver perform a xPCS soft reset on initialization, these
settings will be switched back to the power on reset values.

This changes the xpcs_create function to take in an additional argument
to check if the platform request to skip xPCS soft reset during device
initialization.

Cc: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
Signed-off-by: Wong Vee Khee <vee.khee.wong@linux.intel.com>
---
 drivers/net/dsa/sja1105/sja1105_mdio.c           |  2 +-
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c    |  4 +++-
 drivers/net/pcs/pcs-xpcs.c                       | 16 ++++++++++++----
 include/linux/pcs/pcs-xpcs.h                     |  3 ++-
 include/linux/stmmac.h                           |  1 +
 5 files changed, 19 insertions(+), 7 deletions(-)

Comments

Andrew Lunn Aug. 9, 2021, 1:35 p.m. UTC | #1 
On Mon, Aug 09, 2021 at 06:22:28PM +0800, Wong Vee Khee wrote:
> From: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
> 
> Unlike any other platforms, Intel AlderLake-S uses Synopsys SerDes where
> all the SerDes PLL configurations are controlled by the xPCS at the BIOS
> level. If the driver perform a xPCS soft reset on initialization, these
> settings will be switched back to the power on reset values.
> 
> This changes the xpcs_create function to take in an additional argument
> to check if the platform request to skip xPCS soft reset during device
> initialization.

Why not just call into the BIOS and ask it to configure the SERDES?
Isn't that what ACPI is all about, hiding the details from the OS? Or
did the BIOS writers not add a control method to do this?

    Andrew
Wong Vee Khee Aug. 10, 2021, 11:50 p.m. UTC | #2 
On Mon, Aug 09, 2021 at 02:06:26PM +0300, Vladimir Oltean wrote:
> Hi VK,
> 
> On Mon, Aug 09, 2021 at 06:22:28PM +0800, Wong Vee Khee wrote:
> > diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> > index 63fda3fc40aa..c7a3aa862079 100644
> > --- a/drivers/net/pcs/pcs-xpcs.c
> > +++ b/drivers/net/pcs/pcs-xpcs.c
> > @@ -1081,7 +1081,8 @@ static const struct phylink_pcs_ops xpcs_phylink_ops = {
> >  };
> >  
> >  struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> > -			    phy_interface_t interface)
> > +			    phy_interface_t interface,
> > +			    bool skip_reset)
> >  {
> >  	struct dw_xpcs *xpcs;
> >  	u32 xpcs_id;
> > @@ -1113,9 +1114,16 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> >  		xpcs->pcs.ops = &xpcs_phylink_ops;
> >  		xpcs->pcs.poll = true;
> >  
> > -		ret = xpcs_soft_reset(xpcs, compat);
> > -		if (ret)
> > -			goto out;
> > +		if (!skip_reset) {
> > +			dev_info(&xpcs->mdiodev->dev, "%s: xPCS soft reset\n",
> > +				 __func__);
> > +			ret = xpcs_soft_reset(xpcs, compat);
> > +			if (ret)
> > +				goto out;
> > +		} else {
> > +			dev_info(&xpcs->mdiodev->dev,
> > +				 "%s: skip xpcs soft reset\n", __func__);
> > +		}
> 
> I don't feel like the prints are really necessary.

Sounds good to me. I'll remove these.

> 
> >  
> >  		return xpcs;
> >  	}
> > diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
> > index add077a81b21..0c05a63f3446 100644
> > --- a/include/linux/pcs/pcs-xpcs.h
> > +++ b/include/linux/pcs/pcs-xpcs.h
> > @@ -36,7 +36,8 @@ void xpcs_validate(struct dw_xpcs *xpcs, unsigned long *supported,
> >  int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
> >  		    int enable);
> >  struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> > -			    phy_interface_t interface);
> > +			    phy_interface_t interface,
> > +			    bool xpcs_reset);
> >  void xpcs_destroy(struct dw_xpcs *xpcs);
> >  
> 
> How about exporting the reset functionality as a separate function, and
> the Intel Alder Lake stmmac shim just won't call it? Like this:
> 
> -----------------------------[ cut here ]-----------------------------
> diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
> index 19aea8fb76f6..5acf6742da4d 100644
> --- a/drivers/net/dsa/sja1105/sja1105_mdio.c
> +++ b/drivers/net/dsa/sja1105/sja1105_mdio.c
> @@ -437,13 +437,17 @@ static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
>  			goto out_pcs_free;
>  		}
>  
> -		xpcs = xpcs_create(mdiodev, priv->phy_mode[port]);
> +		xpcs = xpcs_create(mdiodev);
>  		if (IS_ERR(xpcs)) {
>  			rc = PTR_ERR(xpcs);
>  			goto out_pcs_free;
>  		}
>  
>  		priv->xpcs[port] = xpcs;
> +
> +		rc = xpcs_reset(xpcs, priv->phy_mode[port]);
> +		if (rc)
> +			goto out_pcs_free;
>  	}
>  
>  	priv->mdio_pcs = bus;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> index a5d150c5f3d8..81a145009488 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
> @@ -401,12 +401,15 @@ int stmmac_xpcs_setup(struct mii_bus *bus)
>  {
>  	struct net_device *ndev = bus->priv;
>  	struct mdio_device *mdiodev;
> +	bool skip_xpcs_soft_reset;
>  	struct stmmac_priv *priv;
>  	struct dw_xpcs *xpcs;
>  	int mode, addr;
> +	int err;
>  
>  	priv = netdev_priv(ndev);
>  	mode = priv->plat->phy_interface;
> +	skip_xpcs_soft_reset = priv->plat->skip_xpcs_soft_reset;
>  
>  	/* Try to probe the XPCS by scanning all addresses. */
>  	for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
> @@ -414,12 +417,21 @@ int stmmac_xpcs_setup(struct mii_bus *bus)
>  		if (IS_ERR(mdiodev))
>  			continue;
>  
> -		xpcs = xpcs_create(mdiodev, mode);
> +		xpcs = xpcs_create(mdiodev);
>  		if (IS_ERR_OR_NULL(xpcs)) {
>  			mdio_device_free(mdiodev);
>  			continue;
>  		}
>  
> +		if (!skip_xpcs_soft_reset) {
> +			err = xpcs_reset(xpcs, mode);
> +			if (err) {
> +				xpcs_destroy(xpcs);
> +				mdio_device_free(mdiodev);
> +				continue;
> +			}
> +		}
> +
>  		priv->hw->xpcs = xpcs;
>  		break;
>  	}
> diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
> index 63fda3fc40aa..2e721e57bee4 100644
> --- a/drivers/net/pcs/pcs-xpcs.c
> +++ b/drivers/net/pcs/pcs-xpcs.c
> @@ -248,6 +248,18 @@ static int xpcs_soft_reset(struct dw_xpcs *xpcs,
>  	return xpcs_poll_reset(xpcs, dev);
>  }
>  
> +int xpcs_reset(struct dw_xpcs *xpcs, phy_interface_t interface)
> +{
> +	const struct xpcs_compat *compat;
> +
> +	compat = xpcs_find_compat(xpcs->id, interface);
> +	if (!compat)
> +		return -ENODEV;
> +
> +	return xpcs_soft_reset(xpcs, compat);
> +}
> +EXPORT_SYMBOL_GPL(xpcs_reset);
> +
>  #define xpcs_warn(__xpcs, __state, __args...) \
>  ({ \
>  	if ((__state)->link) \
> @@ -1080,12 +1092,11 @@ static const struct phylink_pcs_ops xpcs_phylink_ops = {
>  	.pcs_link_up = xpcs_link_up,
>  };
>  
> -struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> -			    phy_interface_t interface)
> +struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev)
>  {
>  	struct dw_xpcs *xpcs;
>  	u32 xpcs_id;
> -	int i, ret;
> +	int i;
>  
>  	xpcs = kzalloc(sizeof(*xpcs), GFP_KERNEL);
>  	if (!xpcs)
> @@ -1097,35 +1108,18 @@ struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
>  
>  	for (i = 0; i < ARRAY_SIZE(xpcs_id_list); i++) {
>  		const struct xpcs_id *entry = &xpcs_id_list[i];
> -		const struct xpcs_compat *compat;
>  
>  		if ((xpcs_id & entry->mask) != entry->id)
>  			continue;
>  
>  		xpcs->id = entry;
> -
> -		compat = xpcs_find_compat(entry, interface);
> -		if (!compat) {
> -			ret = -ENODEV;
> -			goto out;
> -		}
> -
>  		xpcs->pcs.ops = &xpcs_phylink_ops;
>  		xpcs->pcs.poll = true;
>  
> -		ret = xpcs_soft_reset(xpcs, compat);
> -		if (ret)
> -			goto out;
> -
>  		return xpcs;
>  	}
>  
> -	ret = -ENODEV;
> -
> -out:
> -	kfree(xpcs);
> -
> -	return ERR_PTR(ret);
> +	return ERR_PTR(-ENODEV);
>  }
>  EXPORT_SYMBOL_GPL(xpcs_create);
>  
> diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
> index add077a81b21..d841f55f12cc 100644
> --- a/include/linux/pcs/pcs-xpcs.h
> +++ b/include/linux/pcs/pcs-xpcs.h
> @@ -35,8 +35,8 @@ void xpcs_validate(struct dw_xpcs *xpcs, unsigned long *supported,
>  		   struct phylink_link_state *state);
>  int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
>  		    int enable);
> -struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
> -			    phy_interface_t interface);
> +int xpcs_reset(struct dw_xpcs *xpcs, phy_interface_t interface);
> +struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev);
>  void xpcs_destroy(struct dw_xpcs *xpcs);
>  
>  #endif /* __LINUX_PCS_XPCS_H */
> diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
> index a6f03b36fc4f..0f901773c5e4 100644
> --- a/include/linux/stmmac.h
> +++ b/include/linux/stmmac.h
> @@ -268,5 +268,6 @@ struct plat_stmmacenet_data {
>  	int msi_rx_base_vec;
>  	int msi_tx_base_vec;
>  	bool use_phy_wol;
> +	bool skip_xpcs_soft_reset;
>  };
>  #endif
> -----------------------------[ cut here ]-----------------------------
> 
> I also gave this patch a run on sja1105 and it still works.

Thanks for the suggestion. I tested it out on stmmac and it works.
I will use this as it looks neater. :)

Regards,
 VK
Wong Vee Khee Aug. 10, 2021, 11:55 p.m. UTC | #3 
Hi Andrew,
On Mon, Aug 09, 2021 at 03:35:09PM +0200, Andrew Lunn wrote:
> On Mon, Aug 09, 2021 at 06:22:28PM +0800, Wong Vee Khee wrote:
> > From: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>
> > 
> > Unlike any other platforms, Intel AlderLake-S uses Synopsys SerDes where
> > all the SerDes PLL configurations are controlled by the xPCS at the BIOS
> > level. If the driver perform a xPCS soft reset on initialization, these
> > settings will be switched back to the power on reset values.
> > 
> > This changes the xpcs_create function to take in an additional argument
> > to check if the platform request to skip xPCS soft reset during device
> > initialization.
> 
> Why not just call into the BIOS and ask it to configure the SERDES?
> Isn't that what ACPI is all about, hiding the details from the OS? Or
> did the BIOS writers not add a control method to do this?
> 
>     Andrew

BIOS does configured the SerDes. The problem here is that all the
configurations done by BIOS are being reset at xpcs_create().

We would want user of the pcs-xpcs module (stmmac, sja1105) to have
control whether or not we need to perform to the soft reset in the
xpcs_create() call.

Hope that explained.

Regards,
 VK
Florian Fainelli Aug. 11, 2021, 8:36 a.m. UTC | #4 
On 8/10/2021 4:55 PM, Wong Vee Khee wrote:
> Hi Andrew,

> On Mon, Aug 09, 2021 at 03:35:09PM +0200, Andrew Lunn wrote:

>> On Mon, Aug 09, 2021 at 06:22:28PM +0800, Wong Vee Khee wrote:

>>> From: Michael Sit Wei Hong <michael.wei.hong.sit@intel.com>

>>>

>>> Unlike any other platforms, Intel AlderLake-S uses Synopsys SerDes where

>>> all the SerDes PLL configurations are controlled by the xPCS at the BIOS

>>> level. If the driver perform a xPCS soft reset on initialization, these

>>> settings will be switched back to the power on reset values.

>>>

>>> This changes the xpcs_create function to take in an additional argument

>>> to check if the platform request to skip xPCS soft reset during device

>>> initialization.

>>

>> Why not just call into the BIOS and ask it to configure the SERDES?

>> Isn't that what ACPI is all about, hiding the details from the OS? Or

>> did the BIOS writers not add a control method to do this?

>>

>>      Andrew

> 

> BIOS does configured the SerDes. The problem here is that all the

> configurations done by BIOS are being reset at xpcs_create().

> 

> We would want user of the pcs-xpcs module (stmmac, sja1105) to have

> control whether or not we need to perform to the soft reset in the

> xpcs_create() call.


I understood Andrew's response as suggesting to introduce the ability 
for xpcs_create() to make a BIOS call which would configure the SerDes 
after xpcs_soft_reset(). That way the current xpcs_create() signature 
would remain the same, but you could easily hook any post-reset 
initialization by making an appropriate BIOS call.
-- 
Florian
Andrew Lunn Aug. 11, 2021, 2:20 p.m. UTC | #5 
> > BIOS does configured the SerDes. The problem here is that all the

> > configurations done by BIOS are being reset at xpcs_create().

> > 

> > We would want user of the pcs-xpcs module (stmmac, sja1105) to have

> > control whether or not we need to perform to the soft reset in the

> > xpcs_create() call.

> 

> I understood Andrew's response as suggesting to introduce the ability for

> xpcs_create() to make a BIOS call which would configure the SerDes after

> xpcs_soft_reset().


Yes. Exactly. That is what ACPI is for, so we should use it for this.

     Andrew
Wong Vee Khee Aug. 26, 2021, 11:50 a.m. UTC | #6 
On Wed, Aug 11, 2021 at 04:20:56PM +0200, Andrew Lunn wrote:
> > > BIOS does configured the SerDes. The problem here is that all the

> > > configurations done by BIOS are being reset at xpcs_create().

> > > 

> > > We would want user of the pcs-xpcs module (stmmac, sja1105) to have

> > > control whether or not we need to perform to the soft reset in the

> > > xpcs_create() call.

> > 

> > I understood Andrew's response as suggesting to introduce the ability for

> > xpcs_create() to make a BIOS call which would configure the SerDes after

> > xpcs_soft_reset().

> 

> Yes. Exactly. That is what ACPI is for, so we should use it for this.

> 

>      Andrew


Thanks Florian for the explaination.

I have checked with the BIOS developers and they did not implmenet a
method to this at the kernel level.

Also, Intel AlderLake has both UEFI BIOS and Slim Bootloader which
make it least feasible to go for the ACPI method as per suggested.


Regards,
  VK
diff mbox series

Patch

diff --git a/drivers/net/dsa/sja1105/sja1105_mdio.c b/drivers/net/dsa/sja1105/sja1105_mdio.c
index 19aea8fb76f6..73b43a5da68a 100644
--- a/drivers/net/dsa/sja1105/sja1105_mdio.c
+++ b/drivers/net/dsa/sja1105/sja1105_mdio.c
@@ -437,7 +437,7 @@  static int sja1105_mdiobus_pcs_register(struct sja1105_private *priv)
 			goto out_pcs_free;
 		}
 
-		xpcs = xpcs_create(mdiodev, priv->phy_mode[port]);
+		xpcs = xpcs_create(mdiodev, priv->phy_mode[port], false);
 		if (IS_ERR(xpcs)) {
 			rc = PTR_ERR(xpcs);
 			goto out_pcs_free;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index a5d150c5f3d8..803a4e61105b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -401,12 +401,14 @@  int stmmac_xpcs_setup(struct mii_bus *bus)
 {
 	struct net_device *ndev = bus->priv;
 	struct mdio_device *mdiodev;
+	bool skip_xpcs_soft_reset;
 	struct stmmac_priv *priv;
 	struct dw_xpcs *xpcs;
 	int mode, addr;
 
 	priv = netdev_priv(ndev);
 	mode = priv->plat->phy_interface;
+	skip_xpcs_soft_reset = priv->plat->skip_xpcs_soft_reset;
 
 	/* Try to probe the XPCS by scanning all addresses. */
 	for (addr = 0; addr < PHY_MAX_ADDR; addr++) {
@@ -414,7 +416,7 @@  int stmmac_xpcs_setup(struct mii_bus *bus)
 		if (IS_ERR(mdiodev))
 			continue;
 
-		xpcs = xpcs_create(mdiodev, mode);
+		xpcs = xpcs_create(mdiodev, mode, skip_xpcs_soft_reset);
 		if (IS_ERR_OR_NULL(xpcs)) {
 			mdio_device_free(mdiodev);
 			continue;
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 63fda3fc40aa..c7a3aa862079 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -1081,7 +1081,8 @@  static const struct phylink_pcs_ops xpcs_phylink_ops = {
 };
 
 struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
-			    phy_interface_t interface)
+			    phy_interface_t interface,
+			    bool skip_reset)
 {
 	struct dw_xpcs *xpcs;
 	u32 xpcs_id;
@@ -1113,9 +1114,16 @@  struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
 		xpcs->pcs.ops = &xpcs_phylink_ops;
 		xpcs->pcs.poll = true;
 
-		ret = xpcs_soft_reset(xpcs, compat);
-		if (ret)
-			goto out;
+		if (!skip_reset) {
+			dev_info(&xpcs->mdiodev->dev, "%s: xPCS soft reset\n",
+				 __func__);
+			ret = xpcs_soft_reset(xpcs, compat);
+			if (ret)
+				goto out;
+		} else {
+			dev_info(&xpcs->mdiodev->dev,
+				 "%s: skip xpcs soft reset\n", __func__);
+		}
 
 		return xpcs;
 	}
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index add077a81b21..0c05a63f3446 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -36,7 +36,8 @@  void xpcs_validate(struct dw_xpcs *xpcs, unsigned long *supported,
 int xpcs_config_eee(struct dw_xpcs *xpcs, int mult_fact_100ns,
 		    int enable);
 struct dw_xpcs *xpcs_create(struct mdio_device *mdiodev,
-			    phy_interface_t interface);
+			    phy_interface_t interface,
+			    bool xpcs_reset);
 void xpcs_destroy(struct dw_xpcs *xpcs);
 
 #endif /* __LINUX_PCS_XPCS_H */
diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h
index a6f03b36fc4f..0f901773c5e4 100644
--- a/include/linux/stmmac.h
+++ b/include/linux/stmmac.h
@@ -268,5 +268,6 @@  struct plat_stmmacenet_data {
 	int msi_rx_base_vec;
 	int msi_tx_base_vec;
 	bool use_phy_wol;
+	bool skip_xpcs_soft_reset;
 };
 #endif