[net-next v1] net: stmmac: starfive: Add serdes PHY init/deinit function

[net-next v1] net: stmmac: starfive: Add serdes PHY init/deinit function

[Minda Chen]

To support SGMII interface, add internal serdes PHY powerup/
powerdown function.

Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
---
 .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
index 2013d7477eb7..f5923f847100 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
@@ -9,6 +9,8 @@
 
 #include <linux/mod_devicetable.h>
 #include <linux/platform_device.h>
+#include <linux/phy.h>
+#include <linux/phy/phy.h>
 #include <linux/property.h>
 #include <linux/mfd/syscon.h>
 #include <linux/regmap.h>
@@ -28,6 +30,7 @@ struct starfive_dwmac_data {
 struct starfive_dwmac {
 	struct device *dev;
 	const struct starfive_dwmac_data *data;
+	struct phy *serdes_phy;
 };
 
 static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
@@ -80,6 +83,26 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
 	return 0;
 }
 
+static int starfive_dwmac_serdes_powerup(struct net_device *ndev, void *priv)
+{
+	struct starfive_dwmac *dwmac = priv;
+	int ret;
+
+	ret = phy_init(dwmac->serdes_phy);
+	if (ret)
+		return ret;
+
+	return phy_power_on(dwmac->serdes_phy);
+}
+
+static void starfive_dwmac_serdes_powerdown(struct net_device *ndev, void *priv)
+{
+	struct starfive_dwmac *dwmac = priv;
+
+	phy_power_off(dwmac->serdes_phy);
+	phy_exit(dwmac->serdes_phy);
+}
+
 static int starfive_dwmac_probe(struct platform_device *pdev)
 {
 	struct plat_stmmacenet_data *plat_dat;
@@ -102,6 +125,11 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
 	if (!dwmac)
 		return -ENOMEM;
 
+	dwmac->serdes_phy = devm_phy_optional_get(&pdev->dev, NULL);
+	if (IS_ERR(dwmac->serdes_phy))
+		return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->serdes_phy),
+				     "Failed to get serdes phy\n");
+
 	dwmac->data = device_get_match_data(&pdev->dev);
 
 	plat_dat->clk_tx_i = devm_clk_get_enabled(&pdev->dev, "tx");
@@ -132,6 +160,11 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
 	if (err)
 		return err;
 
+	if (dwmac->serdes_phy) {
+		plat_dat->serdes_powerup = starfive_dwmac_serdes_powerup;
+		plat_dat->serdes_powerdown  = starfive_dwmac_serdes_powerdown;
+	}
+
 	return stmmac_dvr_probe(&pdev->dev, plat_dat, &stmmac_res);
 }
 
-- 
2.17.1

Re: [net-next v1] net: stmmac: starfive: Add serdes PHY init/deinit function

[Paolo Abeni]

On 4/10/25 9:04 AM, Minda Chen wrote:
> To support SGMII interface, add internal serdes PHY powerup/
> powerdown function.
> 
> Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> ---
>  .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 33 +++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> index 2013d7477eb7..f5923f847100 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> @@ -9,6 +9,8 @@
>  
>  #include <linux/mod_devicetable.h>
>  #include <linux/platform_device.h>
> +#include <linux/phy.h>
> +#include <linux/phy/phy.h>
>  #include <linux/property.h>
>  #include <linux/mfd/syscon.h>
>  #include <linux/regmap.h>
> @@ -28,6 +30,7 @@ struct starfive_dwmac_data {
>  struct starfive_dwmac {
>  	struct device *dev;
>  	const struct starfive_dwmac_data *data;
> +	struct phy *serdes_phy;
>  };
>  
>  static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
> @@ -80,6 +83,26 @@ static int starfive_dwmac_set_mode(struct plat_stmmacenet_data *plat_dat)
>  	return 0;
>  }
>  
> +static int starfive_dwmac_serdes_powerup(struct net_device *ndev, void *priv)
> +{
> +	struct starfive_dwmac *dwmac = priv;
> +	int ret;
> +
> +	ret = phy_init(dwmac->serdes_phy);
> +	if (ret)
> +		return ret;

This is called also in case of PM suspend/resume. Do you need to keep
the init here, or should that moved at probe time only? Similar question
for phy_exit() below.

Thanks!

Paolo

Re: [net-next v1] net: stmmac: starfive: Add serdes PHY init/deinit function

[Andrew Lunn]

> +static int starfive_dwmac_serdes_powerup(struct net_device *ndev, void *priv)
> +{
> +	struct starfive_dwmac *dwmac = priv;
> +	int ret;
> +
> +	ret = phy_init(dwmac->serdes_phy);
> +	if (ret)
> +		return ret;
> +
> +	return phy_power_on(dwmac->serdes_phy);
> +}

static int qcom_ethqos_serdes_powerup(struct net_device *ndev, void *priv)
{
        struct qcom_ethqos *ethqos = priv;
        int ret;

        ret = phy_init(ethqos->serdes_phy);
        if (ret)
                return ret;

        ret = phy_power_on(ethqos->serdes_phy);
        if (ret)
                return ret;

        return phy_set_speed(ethqos->serdes_phy, ethqos->speed);
}

Similar?

> +static void starfive_dwmac_serdes_powerdown(struct net_device *ndev, void *priv)
> +{
> +	struct starfive_dwmac *dwmac = priv;
> +
> +	phy_power_off(dwmac->serdes_phy);
> +	phy_exit(dwmac->serdes_phy);
> +}

static void qcom_ethqos_serdes_powerdown(struct net_device *ndev, void *priv)
{
        struct qcom_ethqos *ethqos = priv;

        phy_power_off(ethqos->serdes_phy);
        phy_exit(ethqos->serdes_phy);
}

Pretty much cut & paste.

>  static int starfive_dwmac_probe(struct platform_device *pdev)
>  {
>  	struct plat_stmmacenet_data *plat_dat;
> @@ -102,6 +125,11 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
>  	if (!dwmac)
>  		return -ENOMEM;
>  
> +	dwmac->serdes_phy = devm_phy_optional_get(&pdev->dev, NULL);
> +	if (IS_ERR(dwmac->serdes_phy))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->serdes_phy),
> +				     "Failed to get serdes phy\n");
> +

        ethqos->serdes_phy = devm_phy_optional_get(dev, "serdes");
        if (IS_ERR(ethqos->serdes_phy))
                return dev_err_probe(dev, PTR_ERR(ethqos->serdes_phy),
                                     "Failed to get serdes phy\n");


>  	dwmac->data = device_get_match_data(&pdev->dev);
>  
>  	plat_dat->clk_tx_i = devm_clk_get_enabled(&pdev->dev, "tx");
> @@ -132,6 +160,11 @@ static int starfive_dwmac_probe(struct platform_device *pdev)
>  	if (err)
>  		return err;
>  
> +	if (dwmac->serdes_phy) {
> +		plat_dat->serdes_powerup = starfive_dwmac_serdes_powerup;
> +		plat_dat->serdes_powerdown  = starfive_dwmac_serdes_powerdown;
> +	}
> +

        if (ethqos->serdes_phy) {
                plat_dat->serdes_powerup = qcom_ethqos_serdes_powerup;
                plat_dat->serdes_powerdown  = qcom_ethqos_serdes_powerdown;
        }


I assume you have seen all the work Russell King has been doing
recently cleaning up all the copy/paste code between various glue
drivers. Please don't add to that mess. Please consider how you can
refactor the ethqos code to make is generic for any stmmac driver
which has a generic phy.

    Andrew

---
pw-bot: cr

Re: [net-next v1] net: stmmac: starfive: Add serdes PHY init/deinit function

[Minda Chen]

> 
> On 4/10/25 9:04 AM, Minda Chen wrote:
> > To support SGMII interface, add internal serdes PHY powerup/ powerdown
> > function.
> >
> > Signed-off-by: Minda Chen <minda.chen@starfivetech.com>
> > ---
> >  .../ethernet/stmicro/stmmac/dwmac-starfive.c  | 33
> > +++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> > b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> > index 2013d7477eb7..f5923f847100 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-starfive.c
> > @@ -9,6 +9,8 @@
> >
> >  #include <linux/mod_devicetable.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/phy.h>
> > +#include <linux/phy/phy.h>
> >  #include <linux/property.h>
> >  #include <linux/mfd/syscon.h>
> >  #include <linux/regmap.h>
> > @@ -28,6 +30,7 @@ struct starfive_dwmac_data {  struct starfive_dwmac
> > {
> >  	struct device *dev;
> >  	const struct starfive_dwmac_data *data;
> > +	struct phy *serdes_phy;
> >  };
> >
> >  static int starfive_dwmac_set_mode(struct plat_stmmacenet_data
> > *plat_dat) @@ -80,6 +83,26 @@ static int starfive_dwmac_set_mode(struct
> plat_stmmacenet_data *plat_dat)
> >  	return 0;
> >  }
> >
> > +static int starfive_dwmac_serdes_powerup(struct net_device *ndev,
> > +void *priv) {
> > +	struct starfive_dwmac *dwmac = priv;
> > +	int ret;
> > +
> > +	ret = phy_init(dwmac->serdes_phy);
> > +	if (ret)
> > +		return ret;
> 
> This is called also in case of PM suspend/resume. Do you need to keep the init
> here, or should that moved at probe time only? Similar question for phy_exit()
> below.
> 
> Thanks!
> 
> Paolo
Yes ,The serdes PHY code is simple.  But I am not test with PHY code
I will send next version with PHY code.

Re: [net-next v1] net: stmmac: starfive: Add serdes PHY init/deinit function

[Minda Chen]

> 
> > +static int starfive_dwmac_serdes_powerup(struct net_device *ndev,
> > +void *priv) {
> > +	struct starfive_dwmac *dwmac = priv;
> > +	int ret;
> > +
> > +	ret = phy_init(dwmac->serdes_phy);
> > +	if (ret)
> > +		return ret;
> > +
> > +	return phy_power_on(dwmac->serdes_phy); }
> 
> static int qcom_ethqos_serdes_powerup(struct net_device *ndev, void *priv) {
>         struct qcom_ethqos *ethqos = priv;
>         int ret;
> 
>         ret = phy_init(ethqos->serdes_phy);
>         if (ret)
>                 return ret;
> 
>         ret = phy_power_on(ethqos->serdes_phy);
>         if (ret)
>                 return ret;
> 
>         return phy_set_speed(ethqos->serdes_phy, ethqos->speed); }
> 
> Similar?
> 
> > +static void starfive_dwmac_serdes_powerdown(struct net_device *ndev,
> > +void *priv) {
> > +	struct starfive_dwmac *dwmac = priv;
> > +
> > +	phy_power_off(dwmac->serdes_phy);
> > +	phy_exit(dwmac->serdes_phy);
> > +}
> 
> static void qcom_ethqos_serdes_powerdown(struct net_device *ndev, void
> *priv) {
>         struct qcom_ethqos *ethqos = priv;
> 
>         phy_power_off(ethqos->serdes_phy);
>         phy_exit(ethqos->serdes_phy);
> }
> 
> Pretty much cut & paste.
> 
> >  static int starfive_dwmac_probe(struct platform_device *pdev)  {
> >  	struct plat_stmmacenet_data *plat_dat; @@ -102,6 +125,11 @@ static
> > int starfive_dwmac_probe(struct platform_device *pdev)
> >  	if (!dwmac)
> >  		return -ENOMEM;
> >
> > +	dwmac->serdes_phy = devm_phy_optional_get(&pdev->dev, NULL);
> > +	if (IS_ERR(dwmac->serdes_phy))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(dwmac->serdes_phy),
> > +				     "Failed to get serdes phy\n");
> > +
> 
>         ethqos->serdes_phy = devm_phy_optional_get(dev, "serdes");
>         if (IS_ERR(ethqos->serdes_phy))
>                 return dev_err_probe(dev, PTR_ERR(ethqos->serdes_phy),
>                                      "Failed to get serdes phy\n");
> 
> 
> >  	dwmac->data = device_get_match_data(&pdev->dev);
> >
> >  	plat_dat->clk_tx_i = devm_clk_get_enabled(&pdev->dev, "tx"); @@
> > -132,6 +160,11 @@ static int starfive_dwmac_probe(struct platform_device
> *pdev)
> >  	if (err)
> >  		return err;
> >
> > +	if (dwmac->serdes_phy) {
> > +		plat_dat->serdes_powerup = starfive_dwmac_serdes_powerup;
> > +		plat_dat->serdes_powerdown  =
> starfive_dwmac_serdes_powerdown;
> > +	}
> > +
> 
>         if (ethqos->serdes_phy) {
>                 plat_dat->serdes_powerup =
> qcom_ethqos_serdes_powerup;
>                 plat_dat->serdes_powerdown  =
> qcom_ethqos_serdes_powerdown;
>         }
> 
> 
> I assume you have seen all the work Russell King has been doing recently
> cleaning up all the copy/paste code between various glue drivers. Please don't
> add to that mess. Please consider how you can refactor the ethqos code to
> make is generic for any stmmac driver which has a generic phy.
> 
>     Andrew
> 
> ---
> pw-bot: cr
OK. I will move it to generic. And I will send the serdes PHY code in next version.