[PATCH net-next 1/2] dt-bindings: net: snps,dwmac: add phy-supply support

[PATCH net-next 1/2] dt-bindings: net: snps,dwmac: add phy-supply support

[Marco Felsch]

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
+
   snps,axi-config:
     $ref: /schemas/types.yaml#/definitions/phandle
     description:
-- 
2.39.2

[PATCH net-next 2/2] net: stmmac: platform: add support for phy-supply

[Marco Felsch]

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;
+	}
+
 	/* 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;
-- 
2.39.2

Re: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: add phy-supply support

[Krzysztof Kozlowski]

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

Re: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: add phy-supply support

[Marco Felsch]

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

Re: [PATCH net-next 2/2] net: stmmac: platform: add support for phy-supply

[Andrew Lunn]

> +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

Re: [PATCH net-next 2/2] net: stmmac: platform: add support for phy-supply

[Andrew Lunn]

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

Re: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: add phy-supply support

[Krzysztof Kozlowski]

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

Re: [PATCH net-next 1/2] dt-bindings: net: snps,dwmac: add phy-supply support

[Krzysztof Kozlowski]

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

Re: [PATCH net-next 2/2] net: stmmac: platform: add support for phy-supply

[Marco Felsch]

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
> 

Re: [PATCH net-next 2/2] net: stmmac: platform: add support for phy-supply

[Marco Felsch]

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
> 

Re: [PATCH net-next 2/2] net: stmmac: platform: add support for phy-supply

[Andrew Lunn]

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

Re: [PATCH net-next 2/2] net: stmmac: platform: add support for phy-supply

[Andrew Lunn]

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

Re: [PATCH net-next 2/2] net: stmmac: platform: add support for phy-supply

[Marco Felsch]

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

Re: [PATCH net-next 2/2] net: stmmac: platform: add support for phy-supply

[Marco Felsch]

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