| Message ID | 20230717164307.2868264-2-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next,1/2] dt-bindings: net: snps,dwmac: add phy-supply support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
> +static int stmmac_phy_power(struct platform_device *pdev, > + struct plat_stmmacenet_data *plat, > + bool enable) > +{ > + struct regulator *regulator = plat->phy_regulator; > + int ret = 0; > + > + if (regulator) { > + if (enable) > + ret = regulator_enable(regulator); > + else > + regulator_disable(regulator); > + } > + > + if (ret) > + dev_err(&pdev->dev, "Fail to enable regulator\n"); 'enable' is only correct 50% of the time. > @@ -742,6 +786,8 @@ static int __maybe_unused stmmac_pltfr_suspend(struct device *dev) > if (priv->plat->exit) > priv->plat->exit(pdev, priv->plat->bsp_priv); > > + stmmac_phy_power_off(pdev, priv->plat); > + What about WOL? You probably want to leave the PHY with power in that case. > @@ -757,6 +803,11 @@ static int __maybe_unused stmmac_pltfr_resume(struct device *dev) > struct net_device *ndev = dev_get_drvdata(dev); > struct stmmac_priv *priv = netdev_priv(ndev); > struct platform_device *pdev = to_platform_device(dev); > + int ret; > + > + ret = stmmac_phy_power_on(pdev, priv->plat); > + if (ret) > + return ret; And this needs to balance with _suspend when WOL is being used. Andrew
On Mon, Jul 17, 2023 at 06:43:07PM +0200, Marco Felsch wrote: > Add generic phy-supply handling support to control the phy regulator. > Use the common stmmac_platform code path so all drivers using > stmmac_probe_config_dt() and stmmac_pltfr_pm_ops can use it. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > .../ethernet/stmicro/stmmac/stmmac_platform.c | 51 +++++++++++++++++++ > include/linux/stmmac.h | 1 + > 2 files changed, 52 insertions(+) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > index eb0b2898daa3d..6193d42b53fb7 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > @@ -10,6 +10,7 @@ > > #include <linux/platform_device.h> > #include <linux/pm_runtime.h> > +#include <linux/regulator/consumer.h> > #include <linux/module.h> > #include <linux/io.h> > #include <linux/of.h> > @@ -423,6 +424,15 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > if (plat->interface < 0) > plat->interface = plat->phy_interface; > > + /* Optional regulator for PHY */ > + plat->phy_regulator = devm_regulator_get_optional(&pdev->dev, "phy"); > + if (IS_ERR(plat->phy_regulator)) { > + if (PTR_ERR(plat->phy_regulator) == -EPROBE_DEFER) > + return ERR_CAST(plat->phy_regulator); > + dev_info(&pdev->dev, "No regulator found\n"); > + plat->phy_regulator = NULL; > + } > + So this gets the regulator. When do you actually turn it on? Andrew
On 23-07-18, Andrew Lunn wrote: > On Mon, Jul 17, 2023 at 06:43:07PM +0200, Marco Felsch wrote: > > Add generic phy-supply handling support to control the phy regulator. > > Use the common stmmac_platform code path so all drivers using > > stmmac_probe_config_dt() and stmmac_pltfr_pm_ops can use it. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > .../ethernet/stmicro/stmmac/stmmac_platform.c | 51 +++++++++++++++++++ > > include/linux/stmmac.h | 1 + > > 2 files changed, 52 insertions(+) > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > index eb0b2898daa3d..6193d42b53fb7 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > @@ -10,6 +10,7 @@ > > > > #include <linux/platform_device.h> > > #include <linux/pm_runtime.h> > > +#include <linux/regulator/consumer.h> > > #include <linux/module.h> > > #include <linux/io.h> > > #include <linux/of.h> > > @@ -423,6 +424,15 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > > if (plat->interface < 0) > > plat->interface = plat->phy_interface; > > > > + /* Optional regulator for PHY */ > > + plat->phy_regulator = devm_regulator_get_optional(&pdev->dev, "phy"); > > + if (IS_ERR(plat->phy_regulator)) { > > + if (PTR_ERR(plat->phy_regulator) == -EPROBE_DEFER) > > + return ERR_CAST(plat->phy_regulator); > > + dev_info(&pdev->dev, "No regulator found\n"); > > + plat->phy_regulator = NULL; > > + } > > + > > So this gets the regulator. When do you actually turn it on? During the suspend/resume logic like the rockchip, sun8i platform integrations did. Regards, Marco > > Andrew >
On 23-07-18, Andrew Lunn wrote: > > +static int stmmac_phy_power(struct platform_device *pdev, > > + struct plat_stmmacenet_data *plat, > > + bool enable) > > +{ > > + struct regulator *regulator = plat->phy_regulator; > > + int ret = 0; > > + > > + if (regulator) { > > + if (enable) > > + ret = regulator_enable(regulator); > > + else > > + regulator_disable(regulator); > > + } > > + > > + if (ret) > > + dev_err(&pdev->dev, "Fail to enable regulator\n"); > > 'enable' is only correct 50% of the time. You mean to move it under the enable path. > > @@ -742,6 +786,8 @@ static int __maybe_unused stmmac_pltfr_suspend(struct device *dev) > > if (priv->plat->exit) > > priv->plat->exit(pdev, priv->plat->bsp_priv); > > > > + stmmac_phy_power_off(pdev, priv->plat); > > + > > What about WOL? You probably want to leave the PHY with power in that > case. Good point didn't consider WOL. Is there a way to check if WOL is enabled? Regards, Marco > > > @@ -757,6 +803,11 @@ static int __maybe_unused stmmac_pltfr_resume(struct device *dev) > > struct net_device *ndev = dev_get_drvdata(dev); > > struct stmmac_priv *priv = netdev_priv(ndev); > > struct platform_device *pdev = to_platform_device(dev); > > + int ret; > > + > > + ret = stmmac_phy_power_on(pdev, priv->plat); > > + if (ret) > > + return ret; > > And this needs to balance with _suspend when WOL is being used. > > Andrew >
On Tue, Jul 18, 2023 at 10:35:04AM +0200, Marco Felsch wrote: > On 23-07-18, Andrew Lunn wrote: > > On Mon, Jul 17, 2023 at 06:43:07PM +0200, Marco Felsch wrote: > > > Add generic phy-supply handling support to control the phy regulator. > > > Use the common stmmac_platform code path so all drivers using > > > stmmac_probe_config_dt() and stmmac_pltfr_pm_ops can use it. > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > --- > > > .../ethernet/stmicro/stmmac/stmmac_platform.c | 51 +++++++++++++++++++ > > > include/linux/stmmac.h | 1 + > > > 2 files changed, 52 insertions(+) > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > > index eb0b2898daa3d..6193d42b53fb7 100644 > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > > @@ -10,6 +10,7 @@ > > > > > > #include <linux/platform_device.h> > > > #include <linux/pm_runtime.h> > > > +#include <linux/regulator/consumer.h> > > > #include <linux/module.h> > > > #include <linux/io.h> > > > #include <linux/of.h> > > > @@ -423,6 +424,15 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > > > if (plat->interface < 0) > > > plat->interface = plat->phy_interface; > > > > > > + /* Optional regulator for PHY */ > > > + plat->phy_regulator = devm_regulator_get_optional(&pdev->dev, "phy"); > > > + if (IS_ERR(plat->phy_regulator)) { > > > + if (PTR_ERR(plat->phy_regulator) == -EPROBE_DEFER) > > > + return ERR_CAST(plat->phy_regulator); > > > + dev_info(&pdev->dev, "No regulator found\n"); > > > + plat->phy_regulator = NULL; > > > + } > > > + > > > > So this gets the regulator. When do you actually turn it on? > > During the suspend/resume logic like the rockchip, sun8i platform > integrations did. So you are assuming the boot loader has turned it on? You also might have a difference between the actual state, and what kernel thinks the state is, depending on how the regulator is implemented. It would be better to explicitly turn it on before registering the MDIO bus. Andrew
On Tue, Jul 18, 2023 at 10:38:41AM +0200, Marco Felsch wrote: > On 23-07-18, Andrew Lunn wrote: > > > +static int stmmac_phy_power(struct platform_device *pdev, > > > + struct plat_stmmacenet_data *plat, > > > + bool enable) > > > +{ > > > + struct regulator *regulator = plat->phy_regulator; > > > + int ret = 0; > > > + > > > + if (regulator) { > > > + if (enable) > > > + ret = regulator_enable(regulator); > > > + else > > > + regulator_disable(regulator); > > > + } > > > + > > > + if (ret) > > > + dev_err(&pdev->dev, "Fail to enable regulator\n"); > > > > 'enable' is only correct 50% of the time. > > You mean to move it under the enable path. Or don't use the word 'enable'. 'modify' ? > Good point didn't consider WOL. Is there a way to check if WOL is > enabled? Yes, plenty of MAC drivers do this. Look around. Andrew
On 23-07-18, Andrew Lunn wrote: > On Tue, Jul 18, 2023 at 10:38:41AM +0200, Marco Felsch wrote: > > On 23-07-18, Andrew Lunn wrote: > > > > +static int stmmac_phy_power(struct platform_device *pdev, > > > > + struct plat_stmmacenet_data *plat, > > > > + bool enable) > > > > +{ > > > > + struct regulator *regulator = plat->phy_regulator; > > > > + int ret = 0; > > > > + > > > > + if (regulator) { > > > > + if (enable) > > > > + ret = regulator_enable(regulator); > > > > + else > > > > + regulator_disable(regulator); > > > > + } > > > > + > > > > + if (ret) > > > > + dev_err(&pdev->dev, "Fail to enable regulator\n"); > > > > > > 'enable' is only correct 50% of the time. > > > > You mean to move it under the enable path. > > Or don't use the word 'enable'. 'modify' ? I changed it but kept the 'enable'. > > Good point didn't consider WOL. Is there a way to check if WOL is > > enabled? > > Yes, plenty of MAC drivers do this. Look around. Yep, checked the code and found the interesting parts :) Thanks for the hint. Regards, Marco
On 23-07-18, Andrew Lunn wrote: > On Tue, Jul 18, 2023 at 10:35:04AM +0200, Marco Felsch wrote: > > On 23-07-18, Andrew Lunn wrote: > > > On Mon, Jul 17, 2023 at 06:43:07PM +0200, Marco Felsch wrote: > > > > Add generic phy-supply handling support to control the phy regulator. > > > > Use the common stmmac_platform code path so all drivers using > > > > stmmac_probe_config_dt() and stmmac_pltfr_pm_ops can use it. > > > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > > > --- > > > > .../ethernet/stmicro/stmmac/stmmac_platform.c | 51 +++++++++++++++++++ > > > > include/linux/stmmac.h | 1 + > > > > 2 files changed, 52 insertions(+) > > > > > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > > > index eb0b2898daa3d..6193d42b53fb7 100644 > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c > > > > @@ -10,6 +10,7 @@ > > > > > > > > #include <linux/platform_device.h> > > > > #include <linux/pm_runtime.h> > > > > +#include <linux/regulator/consumer.h> > > > > #include <linux/module.h> > > > > #include <linux/io.h> > > > > #include <linux/of.h> > > > > @@ -423,6 +424,15 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) > > > > if (plat->interface < 0) > > > > plat->interface = plat->phy_interface; > > > > > > > > + /* Optional regulator for PHY */ > > > > + plat->phy_regulator = devm_regulator_get_optional(&pdev->dev, "phy"); > > > > + if (IS_ERR(plat->phy_regulator)) { > > > > + if (PTR_ERR(plat->phy_regulator) == -EPROBE_DEFER) > > > > + return ERR_CAST(plat->phy_regulator); > > > > + dev_info(&pdev->dev, "No regulator found\n"); > > > > + plat->phy_regulator = NULL; > > > > + } > > > > + > > > > > > So this gets the regulator. When do you actually turn it on? > > > > During the suspend/resume logic like the rockchip, sun8i platform > > integrations did. > > So you are assuming the boot loader has turned it on? > > You also might have a difference between the actual state, and what > kernel thinks the state is, depending on how the regulator is > implemented. > > It would be better to explicitly turn it on before registering the > MDIO bus. You're right, I changed this. Thanks for the hint. Regards, Marco
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c index eb0b2898daa3d..6193d42b53fb7 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c @@ -10,6 +10,7 @@ #include <linux/platform_device.h> #include <linux/pm_runtime.h> +#include <linux/regulator/consumer.h> #include <linux/module.h> #include <linux/io.h> #include <linux/of.h> @@ -423,6 +424,15 @@ stmmac_probe_config_dt(struct platform_device *pdev, u8 *mac) if (plat->interface < 0) plat->interface = plat->phy_interface; + /* Optional regulator for PHY */ + plat->phy_regulator = devm_regulator_get_optional(&pdev->dev, "phy"); + if (IS_ERR(plat->phy_regulator)) { + if (PTR_ERR(plat->phy_regulator) == -EPROBE_DEFER) + return ERR_CAST(plat->phy_regulator); + dev_info(&pdev->dev, "No regulator found\n"); + plat->phy_regulator = NULL; + } + /* Some wrapper drivers still rely on phy_node. Let's save it while * they are not converted to phylink. */ plat->phy_node = of_parse_phandle(np, "phy-handle", 0); @@ -659,6 +669,38 @@ void stmmac_remove_config_dt(struct platform_device *pdev, EXPORT_SYMBOL_GPL(stmmac_probe_config_dt); EXPORT_SYMBOL_GPL(stmmac_remove_config_dt); +static int stmmac_phy_power(struct platform_device *pdev, + struct plat_stmmacenet_data *plat, + bool enable) +{ + struct regulator *regulator = plat->phy_regulator; + int ret = 0; + + if (regulator) { + if (enable) + ret = regulator_enable(regulator); + else + regulator_disable(regulator); + } + + if (ret) + dev_err(&pdev->dev, "Fail to enable regulator\n"); + + return ret; +} + +static int stmmac_phy_power_on(struct platform_device *pdev, + struct plat_stmmacenet_data *plat) +{ + return stmmac_phy_power(pdev, plat, true); +} + +static void stmmac_phy_power_off(struct platform_device *pdev, + struct plat_stmmacenet_data *plat) +{ + stmmac_phy_power(pdev, plat, false); +} + int stmmac_get_platform_resources(struct platform_device *pdev, struct stmmac_resources *stmmac_res) { @@ -720,6 +762,8 @@ int stmmac_pltfr_remove(struct platform_device *pdev) stmmac_remove_config_dt(pdev, plat); + stmmac_phy_power_off(pdev, plat); + return 0; } EXPORT_SYMBOL_GPL(stmmac_pltfr_remove); @@ -742,6 +786,8 @@ static int __maybe_unused stmmac_pltfr_suspend(struct device *dev) if (priv->plat->exit) priv->plat->exit(pdev, priv->plat->bsp_priv); + stmmac_phy_power_off(pdev, priv->plat); + return ret; } @@ -757,6 +803,11 @@ static int __maybe_unused stmmac_pltfr_resume(struct device *dev) struct net_device *ndev = dev_get_drvdata(dev); struct stmmac_priv *priv = netdev_priv(ndev); struct platform_device *pdev = to_platform_device(dev); + int ret; + + ret = stmmac_phy_power_on(pdev, priv->plat); + if (ret) + return ret; if (priv->plat->init) priv->plat->init(pdev, priv->plat->bsp_priv); diff --git a/include/linux/stmmac.h b/include/linux/stmmac.h index 225751a8fd8e3..456bab4a3b362 100644 --- a/include/linux/stmmac.h +++ b/include/linux/stmmac.h @@ -209,6 +209,7 @@ struct plat_stmmacenet_data { int phy_addr; int interface; phy_interface_t phy_interface; + struct regulator *phy_regulator; struct stmmac_mdio_bus_data *mdio_bus_data; struct device_node *phy_node; struct device_node *phylink_node;
Add generic phy-supply handling support to control the phy regulator. Use the common stmmac_platform code path so all drivers using stmmac_probe_config_dt() and stmmac_pltfr_pm_ops can use it. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- .../ethernet/stmicro/stmmac/stmmac_platform.c | 51 +++++++++++++++++++ include/linux/stmmac.h | 1 + 2 files changed, 52 insertions(+)