| Message ID | 20230717164307.2868264-1-m.felsch@pengutronix.de |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [net-next,1/2] dt-bindings: net: snps,dwmac: add phy-supply support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On 17/07/2023 18:43, Marco Felsch wrote: > Document the common phy-supply property to be able to specify a phy > regulator. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- > Documentation/devicetree/bindings/net/snps,dwmac.yaml | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > index 363b3e3ea3a60..f66d1839cf561 100644 > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > @@ -159,6 +159,9 @@ properties: > can be passive (no SW requirement), and requires that the MAC operate > in a different mode than the PHY in order to function. > > + phy-supply: > + description: PHY regulator > + Isn't this property of the PHY? Why would the Ethernet controller play with a supply of a phy? Best regards, Krzysztof
On 23-07-17, Krzysztof Kozlowski wrote: > On 17/07/2023 18:43, Marco Felsch wrote: > > Document the common phy-supply property to be able to specify a phy > > regulator. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > --- > > Documentation/devicetree/bindings/net/snps,dwmac.yaml | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > index 363b3e3ea3a60..f66d1839cf561 100644 > > --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml > > @@ -159,6 +159,9 @@ properties: > > can be passive (no SW requirement), and requires that the MAC operate > > in a different mode than the PHY in order to function. > > > > + phy-supply: > > + description: PHY regulator > > + > > Isn't this property of the PHY? Why would the Ethernet controller play > with a supply of a phy? Because this is the current state. Please check the all other MACs handling the phy-supply (if supported). Some of them handling it under the mdio-node (not the phy-node) but most bindings do specify this on MAC level (e.g. FEC, DWMAC (suni, rk)). I agree that the phy sould handle this but this would be a lot more effort and since the dwmac-sun8i/rk bindings do support this on MAC level I would keep it that way. Regards, Marco
> +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 17/07/2023 18:57, Marco Felsch wrote: > On 23-07-17, Krzysztof Kozlowski wrote: >> On 17/07/2023 18:43, Marco Felsch wrote: >>> Document the common phy-supply property to be able to specify a phy >>> regulator. >>> >>> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> >>> --- >>> Documentation/devicetree/bindings/net/snps,dwmac.yaml | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> index 363b3e3ea3a60..f66d1839cf561 100644 >>> --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml >>> @@ -159,6 +159,9 @@ properties: >>> can be passive (no SW requirement), and requires that the MAC operate >>> in a different mode than the PHY in order to function. >>> >>> + phy-supply: >>> + description: PHY regulator >>> + >> >> Isn't this property of the PHY? Why would the Ethernet controller play >> with a supply of a phy? > > Because this is the current state. Please check the all other MACs > handling the phy-supply (if supported). Some of them handling it under > the mdio-node (not the phy-node) but most bindings do specify this on > MAC level (e.g. FEC, DWMAC (suni, rk)). > > I agree that the phy sould handle this but this would be a lot more > effort and since the dwmac-sun8i/rk bindings do support this on MAC > level I would keep it that way. Indeed phy bindings do not allow a supply. Best regards, Krzysztof
On 17/07/2023 18:43, Marco Felsch wrote: > Document the common phy-supply property to be able to specify a phy > regulator. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > --- Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
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/Documentation/devicetree/bindings/net/snps,dwmac.yaml b/Documentation/devicetree/bindings/net/snps,dwmac.yaml index 363b3e3ea3a60..f66d1839cf561 100644 --- a/Documentation/devicetree/bindings/net/snps,dwmac.yaml +++ b/Documentation/devicetree/bindings/net/snps,dwmac.yaml @@ -159,6 +159,9 @@ properties: can be passive (no SW requirement), and requires that the MAC operate in a different mode than the PHY in order to function. + phy-supply: + description: PHY regulator + snps,axi-config: $ref: /schemas/types.yaml#/definitions/phandle description:
Document the common phy-supply property to be able to specify a phy regulator. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- Documentation/devicetree/bindings/net/snps,dwmac.yaml | 3 +++ 1 file changed, 3 insertions(+)