| Message ID | 20250410070453.61178-1-minda.chen@starfivetech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,v1] net: stmmac: starfive: Add serdes PHY init/deinit function | expand |
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
> +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
> > 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.
> > > +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.
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); }
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(+)