[PATCH v1 0/4] net: stmmac: dwmac-rk: Fix phy regulator issues

[PATCH v1 0/4] net: stmmac: dwmac-rk: Fix phy regulator issues

[Romain Perier]

This series fixes few issues in dwmac-rk:

1. Voltage settings was hardcoded into the driver for the phy regulator.
   The driver now uses the default voltage settings found in the devicetree, which are
   applied throught the regulator framework.
2. The regulator name used to power on or power off the phy was put in the devicetree
   variable "phy_regulator", which is not standard and added a lot of code for nothing.
   The driver now uses the devicetree property "phy-supply" and the corresponding functions
   to manipulate this regulator.

The corresponding devicetree files are also updated. As, dwmac-rk was recently
pushed in the development tree, I don't care about devicetree backward compatibility
issues.

Romain Perier (4):
  net: stmmac: dwmac-rk: Don't set the regulator voltage for phy from
    the driver
  ARM: dts: Add regulator voltage settings for vcc_phy in
    rk3288-evb.dtsi
  net: stmmac: dwmac-rk: Use standard devicetree property for phy
    regulator
  ARM: dts: Convert gmac to phy-supply in rk3288-evb-rk808.dts

 arch/arm/boot/dts/rk3288-evb-rk808.dts         |  2 +-
 arch/arm/boot/dts/rk3288-evb.dtsi              |  2 +
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 62 ++++++++------------------
 3 files changed, 22 insertions(+), 44 deletions(-)

-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 1/4] net: stmmac: dwmac-rk: Don't set the regulator voltage for phy from the driver

[Romain Perier]

As these settings can be directly expressed from devicetree for both fixed
regulators and pmic-integrated regulators, it is more standard to set them
from dts and let the regulator framework use the right voltage informations
when it is used in the driver.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 35f9b86..06e1637 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -303,7 +303,6 @@ static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
 	} else {
 		if (enable) {
 			if (!regulator_is_enabled(ldo)) {
-				regulator_set_voltage(ldo, 3300000, 3300000);
 				ret = regulator_enable(ldo);
 				if (ret != 0)
 					dev_err(dev, "%s: fail to enable %s\n",
-- 
1.9.1

[PATCH v1 2/4] ARM: dts: Add regulator voltage settings for vcc_phy in rk3288-evb.dtsi

[Romain Perier]

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/rk3288-evb.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/rk3288-evb.dtsi b/arch/arm/boot/dts/rk3288-evb.dtsi
index 489643c..1c08eb0 100644
--- a/arch/arm/boot/dts/rk3288-evb.dtsi
+++ b/arch/arm/boot/dts/rk3288-evb.dtsi
@@ -98,6 +98,8 @@
 		pinctrl-names = "default";
 		pinctrl-0 = <&eth_phy_pwr>;
 		regulator-name = "vcc_phy";
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
 		regulator-always-on;
 		regulator-boot-on;
 	};
-- 
1.9.1

[PATCH v1 3/4] net: stmmac: dwmac-rk: Use standard devicetree property for phy regulator

[Romain Perier]

Currently, dwmac-rk uses a custom propety "phy_regulator" to get the name of the
right regulator to use to power on or power off the phy. This commit converts the
driver to use phy-supply devicetree property and the corresponding API, it cleans
the code a bit and make it simpler to maintain.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 61 ++++++++------------------
 1 file changed, 19 insertions(+), 42 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
index 06e1637..8a8091c 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
@@ -32,7 +32,7 @@
 struct rk_priv_data {
 	struct platform_device *pdev;
 	int phy_iface;
-	char regulator[32];
+	struct regulator *regulator;
 
 	bool clk_enabled;
 	bool clock_input;
@@ -287,46 +287,25 @@ static int gmac_clk_enable(struct rk_priv_data *bsp_priv, bool enable)
 
 static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
 {
-	struct regulator *ldo;
-	char *ldostr = bsp_priv->regulator;
+	struct regulator *ldo = bsp_priv->regulator;
 	int ret;
 	struct device *dev = &bsp_priv->pdev->dev;
 
-	if (!ldostr) {
-		dev_err(dev, "%s: no ldo found\n", __func__);
+	if (!ldo) {
+		dev_err(dev, "%s: no regulator found\n", __func__);
 		return -1;
 	}
 
-	ldo = regulator_get(NULL, ldostr);
-	if (!ldo) {
-		dev_err(dev, "\n%s get ldo %s failed\n", __func__, ldostr);
+	if (enable) {
+		ret = regulator_enable(ldo);
+		if (ret)
+			dev_err(dev, "%s: fail to enable phy-supply\n",
+				__func__);
 	} else {
-		if (enable) {
-			if (!regulator_is_enabled(ldo)) {
-				ret = regulator_enable(ldo);
-				if (ret != 0)
-					dev_err(dev, "%s: fail to enable %s\n",
-						__func__, ldostr);
-				else
-					dev_info(dev, "turn on ldo done.\n");
-			} else {
-				dev_warn(dev, "%s is enabled before enable",
-					 ldostr);
-			}
-		} else {
-			if (regulator_is_enabled(ldo)) {
-				ret = regulator_disable(ldo);
-				if (ret != 0)
-					dev_err(dev, "%s: fail to disable %s\n",
-						__func__, ldostr);
-				else
-					dev_info(dev, "turn off ldo done.\n");
-			} else {
-				dev_warn(dev, "%s is disabled before disable",
-					 ldostr);
-			}
-		}
-		regulator_put(ldo);
+		ret = regulator_disable(ldo);
+		if (ret)
+			dev_err(dev, "%s: fail to disable phy-supply\n",
+				__func__);
 	}
 
 	return 0;
@@ -346,14 +325,12 @@ static void *rk_gmac_setup(struct platform_device *pdev)
 
 	bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
 
-	ret = of_property_read_string(dev->of_node, "phy_regulator", &strings);
-	if (ret) {
-		dev_warn(dev, "%s: Can not read property: phy_regulator.\n",
-			 __func__);
-	} else {
-		dev_info(dev, "%s: PHY power controlled by regulator(%s).\n",
-			 __func__, strings);
-		strcpy(bsp_priv->regulator, strings);
+	bsp_priv->regulator = devm_regulator_get_optional(dev, "phy");
+	if (IS_ERR(bsp_priv->regulator)) {
+		if (PTR_ERR(bsp_priv->regulator) == -EPROBE_DEFER)
+			return -EPROBE_DEFER;
+		dev_err(dev, "no regulator found\n");
+		bsp_priv->regulator = NULL;
 	}
 
 	ret = of_property_read_string(dev->of_node, "clock_in_out", &strings);
-- 
1.9.1

Re: [PATCH v1 3/4] net: stmmac: dwmac-rk: Use standard devicetree property for phy regulator

[Heiko Stübner]

Am Montag, 19. Januar 2015, 18:08:08 schrieb Romain Perier:
> Currently, dwmac-rk uses a custom propety "phy_regulator" to get the name of
> the right regulator to use to power on or power off the phy. This commit
> converts the driver to use phy-supply devicetree property and the
> corresponding API, it cleans the code a bit and make it simpler to
> maintain.
> 
> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>

I don't see updated binding documentation in here.

Secondly I think patch4 which changes the property in the evb-rk808 could be 
in here. The change is simple enough and would keep it bisectable without 
needing transistional patches.

And there is the one warning down below.


> ---
>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 61
> ++++++++------------------ 1 file changed, 19 insertions(+), 42
> deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 06e1637..8a8091c
> 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
> @@ -32,7 +32,7 @@
>  struct rk_priv_data {
>  	struct platform_device *pdev;
>  	int phy_iface;
> -	char regulator[32];
> +	struct regulator *regulator;
> 
>  	bool clk_enabled;
>  	bool clock_input;
> @@ -287,46 +287,25 @@ static int gmac_clk_enable(struct rk_priv_data
> *bsp_priv, bool enable)
> 
>  static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>  {
> -	struct regulator *ldo;
> -	char *ldostr = bsp_priv->regulator;
> +	struct regulator *ldo = bsp_priv->regulator;
>  	int ret;
>  	struct device *dev = &bsp_priv->pdev->dev;
> 
> -	if (!ldostr) {
> -		dev_err(dev, "%s: no ldo found\n", __func__);
> +	if (!ldo) {
> +		dev_err(dev, "%s: no regulator found\n", __func__);
>  		return -1;
>  	}
> 
> -	ldo = regulator_get(NULL, ldostr);
> -	if (!ldo) {
> -		dev_err(dev, "\n%s get ldo %s failed\n", __func__, ldostr);
> +	if (enable) {
> +		ret = regulator_enable(ldo);
> +		if (ret)
> +			dev_err(dev, "%s: fail to enable phy-supply\n",
> +				__func__);
>  	} else {
> -		if (enable) {
> -			if (!regulator_is_enabled(ldo)) {
> -				ret = regulator_enable(ldo);
> -				if (ret != 0)
> -					dev_err(dev, "%s: fail to enable %s\n",
> -						__func__, ldostr);
> -				else
> -					dev_info(dev, "turn on ldo done.\n");
> -			} else {
> -				dev_warn(dev, "%s is enabled before enable",
> -					 ldostr);
> -			}
> -		} else {
> -			if (regulator_is_enabled(ldo)) {
> -				ret = regulator_disable(ldo);
> -				if (ret != 0)
> -					dev_err(dev, "%s: fail to disable %s\n",
> -						__func__, ldostr);
> -				else
> -					dev_info(dev, "turn off ldo done.\n");
> -			} else {
> -				dev_warn(dev, "%s is disabled before disable",
> -					 ldostr);
> -			}
> -		}
> -		regulator_put(ldo);
> +		ret = regulator_disable(ldo);
> +		if (ret)
> +			dev_err(dev, "%s: fail to disable phy-supply\n",
> +				__func__);
>  	}
> 
>  	return 0;
> @@ -346,14 +325,12 @@ static void *rk_gmac_setup(struct platform_device
> *pdev)
> 
>  	bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
> 
> -	ret = of_property_read_string(dev->of_node, "phy_regulator", &strings);
> -	if (ret) {
> -		dev_warn(dev, "%s: Can not read property: phy_regulator.\n",
> -			 __func__);
> -	} else {
> -		dev_info(dev, "%s: PHY power controlled by regulator(%s).\n",
> -			 __func__, strings);
> -		strcpy(bsp_priv->regulator, strings);
> +	bsp_priv->regulator = devm_regulator_get_optional(dev, "phy");
> +	if (IS_ERR(bsp_priv->regulator)) {
> +		if (PTR_ERR(bsp_priv->regulator) == -EPROBE_DEFER)
> +			return -EPROBE_DEFER;

this should be return ERR_PTR(-EPROBE_DEFER) and produces a warning in its 
current state


> +		dev_err(dev, "no regulator found\n");
> +		bsp_priv->regulator = NULL;
>  	}
> 
>  	ret = of_property_read_string(dev->of_node, "clock_in_out", &strings);

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1 3/4] net: stmmac: dwmac-rk: Use standard devicetree property for phy regulator

[Romain Perier]

Hi,

Ok I will do these changes. However, the property "phy_regulator" was
not documented in
Documentation/devicetree/bindings/net/rockchip-dwmac.txt ... I can
document it anyway, it has probably been forgotten.

Thanks !

2015-01-19 21:12 GMT+01:00 Heiko Stübner <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>:
> Am Montag, 19. Januar 2015, 18:08:08 schrieb Romain Perier:
>> Currently, dwmac-rk uses a custom propety "phy_regulator" to get the name of
>> the right regulator to use to power on or power off the phy. This commit
>> converts the driver to use phy-supply devicetree property and the
>> corresponding API, it cleans the code a bit and make it simpler to
>> maintain.
>>
>> Signed-off-by: Romain Perier <romain.perier-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>
> I don't see updated binding documentation in here.
>
> Secondly I think patch4 which changes the property in the evb-rk808 could be
> in here. The change is simple enough and would keep it bisectable without
> needing transistional patches.
>
> And there is the one warning down below.
>
>
>> ---
>>  drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c | 61
>> ++++++++------------------ 1 file changed, 19 insertions(+), 42
>> deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c index 06e1637..8a8091c
>> 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-rk.c
>> @@ -32,7 +32,7 @@
>>  struct rk_priv_data {
>>       struct platform_device *pdev;
>>       int phy_iface;
>> -     char regulator[32];
>> +     struct regulator *regulator;
>>
>>       bool clk_enabled;
>>       bool clock_input;
>> @@ -287,46 +287,25 @@ static int gmac_clk_enable(struct rk_priv_data
>> *bsp_priv, bool enable)
>>
>>  static int phy_power_on(struct rk_priv_data *bsp_priv, bool enable)
>>  {
>> -     struct regulator *ldo;
>> -     char *ldostr = bsp_priv->regulator;
>> +     struct regulator *ldo = bsp_priv->regulator;
>>       int ret;
>>       struct device *dev = &bsp_priv->pdev->dev;
>>
>> -     if (!ldostr) {
>> -             dev_err(dev, "%s: no ldo found\n", __func__);
>> +     if (!ldo) {
>> +             dev_err(dev, "%s: no regulator found\n", __func__);
>>               return -1;
>>       }
>>
>> -     ldo = regulator_get(NULL, ldostr);
>> -     if (!ldo) {
>> -             dev_err(dev, "\n%s get ldo %s failed\n", __func__, ldostr);
>> +     if (enable) {
>> +             ret = regulator_enable(ldo);
>> +             if (ret)
>> +                     dev_err(dev, "%s: fail to enable phy-supply\n",
>> +                             __func__);
>>       } else {
>> -             if (enable) {
>> -                     if (!regulator_is_enabled(ldo)) {
>> -                             ret = regulator_enable(ldo);
>> -                             if (ret != 0)
>> -                                     dev_err(dev, "%s: fail to enable %s\n",
>> -                                             __func__, ldostr);
>> -                             else
>> -                                     dev_info(dev, "turn on ldo done.\n");
>> -                     } else {
>> -                             dev_warn(dev, "%s is enabled before enable",
>> -                                      ldostr);
>> -                     }
>> -             } else {
>> -                     if (regulator_is_enabled(ldo)) {
>> -                             ret = regulator_disable(ldo);
>> -                             if (ret != 0)
>> -                                     dev_err(dev, "%s: fail to disable %s\n",
>> -                                             __func__, ldostr);
>> -                             else
>> -                                     dev_info(dev, "turn off ldo done.\n");
>> -                     } else {
>> -                             dev_warn(dev, "%s is disabled before disable",
>> -                                      ldostr);
>> -                     }
>> -             }
>> -             regulator_put(ldo);
>> +             ret = regulator_disable(ldo);
>> +             if (ret)
>> +                     dev_err(dev, "%s: fail to disable phy-supply\n",
>> +                             __func__);
>>       }
>>
>>       return 0;
>> @@ -346,14 +325,12 @@ static void *rk_gmac_setup(struct platform_device
>> *pdev)
>>
>>       bsp_priv->phy_iface = of_get_phy_mode(dev->of_node);
>>
>> -     ret = of_property_read_string(dev->of_node, "phy_regulator", &strings);
>> -     if (ret) {
>> -             dev_warn(dev, "%s: Can not read property: phy_regulator.\n",
>> -                      __func__);
>> -     } else {
>> -             dev_info(dev, "%s: PHY power controlled by regulator(%s).\n",
>> -                      __func__, strings);
>> -             strcpy(bsp_priv->regulator, strings);
>> +     bsp_priv->regulator = devm_regulator_get_optional(dev, "phy");
>> +     if (IS_ERR(bsp_priv->regulator)) {
>> +             if (PTR_ERR(bsp_priv->regulator) == -EPROBE_DEFER)
>> +                     return -EPROBE_DEFER;
>
> this should be return ERR_PTR(-EPROBE_DEFER) and produces a warning in its
> current state
>
>
>> +             dev_err(dev, "no regulator found\n");
>> +             bsp_priv->regulator = NULL;
>>       }
>>
>>       ret = of_property_read_string(dev->of_node, "clock_in_out", &strings);
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v1 4/4] ARM: dts: Convert gmac to phy-supply in rk3288-evb-rk808.dts

[Romain Perier]

This replaces the property phy_regulator by the standard devicetree property
phy-supply and references the device_node vcc_phy from it.

Signed-off-by: Romain Perier <romain.perier@gmail.com>
---
 arch/arm/boot/dts/rk3288-evb-rk808.dts | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/rk3288-evb-rk808.dts b/arch/arm/boot/dts/rk3288-evb-rk808.dts
index 831a7aa..e1d3eeb 100644
--- a/arch/arm/boot/dts/rk3288-evb-rk808.dts
+++ b/arch/arm/boot/dts/rk3288-evb-rk808.dts
@@ -161,7 +161,7 @@
 };
 
 &gmac {
-	phy_regulator = "vcc_phy";
+	phy-supply = <&vcc_phy>;
 	phy-mode = "rgmii";
 	clock_in_out = "input";
 	snps,reset-gpio = <&gpio4 7 0>;
-- 
1.9.1

Re: [PATCH v1 0/4] net: stmmac: dwmac-rk: Fix phy regulator issues

[Heiko Stübner]

Hi Romain

Am Montag, 19. Januar 2015, 18:08:05 schrieb Romain Perier:
> This series fixes few issues in dwmac-rk:
> 
> 1. Voltage settings was hardcoded into the driver for the phy regulator.
>    The driver now uses the default voltage settings found in the devicetree,
> which are applied throught the regulator framework.
> 2. The regulator name used to power on or power off the phy was put in the
> devicetree variable "phy_regulator", which is not standard and added a lot
> of code for nothing. The driver now uses the devicetree property
> "phy-supply" and the corresponding functions to manipulate this regulator.
> 
> The corresponding devicetree files are also updated. As, dwmac-rk was
> recently pushed in the development tree, I don't care about devicetree
> backward compatibility issues.

This last sentence is slightly misleading :-) .

The actual fact is, that these new bindings for the rk3288 gmac have not been 
released with any official kernel release yet ... i.e. the will be released with 
3.20 in whatever form, so we don't _need_ to care about keeping compatibility 
still for the next 2.5 weeks or so.

@Dave: it would be good if this series (when fixed) could still go into the 
3.20 material so we don't get stuck with the non-standard regulator property.


As we'll probably need a v2 due to at the issue in patch3, could you also 
switch places of patch1 and 2, which would keep bisecatbility (i.e. regulator 
property before removing the voltage setting from the driver).


Otherwise this series:
Reviewed-by: Heiko Stuebner <heiko@sntech.de>
Tested-by: Heiko Stuebner <heiko@sntech.de>

Re: [PATCH v1 0/4] net: stmmac: dwmac-rk: Fix phy regulator issues

[Romain Perier]

Hi all,

2015-01-19 21:19 GMT+01:00 Heiko Stübner <heiko@sntech.de>:
> Hi Romain
>
> Am Montag, 19. Januar 2015, 18:08:05 schrieb Romain Perier:
>> This series fixes few issues in dwmac-rk:
>>
>> 1. Voltage settings was hardcoded into the driver for the phy regulator.
>>    The driver now uses the default voltage settings found in the devicetree,
>> which are applied throught the regulator framework.
>> 2. The regulator name used to power on or power off the phy was put in the
>> devicetree variable "phy_regulator", which is not standard and added a lot
>> of code for nothing. The driver now uses the devicetree property
>> "phy-supply" and the corresponding functions to manipulate this regulator.
>>
>> The corresponding devicetree files are also updated. As, dwmac-rk was
>> recently pushed in the development tree, I don't care about devicetree
>> backward compatibility issues.
>
> This last sentence is slightly misleading :-) .
>

Yes, I meant that I don't need to care about it, as you explain it well below.
Sorry for my misleading sentence ;) . I will fix it in my second serie.

> The actual fact is, that these new bindings for the rk3288 gmac have not been
> released with any official kernel release yet ... i.e. the will be released with
> 3.20 in whatever form, so we don't _need_ to care about keeping compatibility
> still for the next 2.5 weeks or so.
>
> @Dave: it would be good if this series (when fixed) could still go into the
> 3.20 material so we don't get stuck with the non-standard regulator property.
>
>
> As we'll probably need a v2 due to at the issue in patch3, could you also
> switch places of patch1 and 2, which would keep bisecatbility (i.e. regulator
> property before removing the voltage setting from the driver).

This sentence about re-ordering patches is for Dave or for me ?

Thanks,
Romain