[PATCH v2 0/4] add coupled regulators for Exynos5422/5800

[PATCH v2 0/4] add coupled regulators for Exynos5422/5800

[Kamil Konieczny]

Hi,

The main purpose of this patch series is to add coupled regulators for
Exynos5422/5800 to keep constrain on voltage difference between vdd_arm
and vdd_int to be at most 300mV. In exynos-bus instead of using
regulator_set_voltage_tol() with default voltage tolerance it should be
used regulator_set_voltage_triplet() with volatege range, and this is
already present in opp/core.c code, so it can be reused. While at this,
move setting regulators into opp/core.

This patchset was tested on Odroid XU3.

The last patch depends on two previous.

Changes in v2:

- improve regulators enable/disable code in opp/core as suggested by
  Viresh Kumar
- add new patch for remove unused dt-bindings as suggested by Krzysztof
  Kozlowski

Regards,
Kamil

Kamil Konieczny (3):
  opp: core: add regulators enable and disable
  devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()
  dt-bindings: devfreq: exynos-bus: remove unused property

Marek Szyprowski (1):
  ARM: dts: exynos: add initial data for coupled regulators for
    Exynos5422/5800

 .../bindings/devfreq/exynos-bus.txt           |   2 -
 arch/arm/boot/dts/exynos5420.dtsi             |  34 ++--
 arch/arm/boot/dts/exynos5422-odroid-core.dtsi |   4 +
 arch/arm/boot/dts/exynos5800-peach-pi.dts     |   4 +
 arch/arm/boot/dts/exynos5800.dtsi             |  32 ++--
 drivers/devfreq/exynos-bus.c                  | 172 +++++++-----------
 drivers/opp/core.c                            |  18 +-
 7 files changed, 122 insertions(+), 144 deletions(-)

-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 1/4] opp: core: add regulators enable and disable

[Kamil Konieczny]

Add enable regulators to dev_pm_opp_set_regulators() and disable
regulators to dev_pm_opp_put_regulators(). This prepares for
converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().

Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
--
Changes in v2:

- move regulator enable and disable into loop

---
 drivers/opp/core.c | 18 +++++++++++++++---
 1 file changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/opp/core.c b/drivers/opp/core.c
index 0e7703fe733f..069c5cf8827e 100644
--- a/drivers/opp/core.c
+++ b/drivers/opp/core.c
@@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
 			goto free_regulators;
 		}
 
+		ret = regulator_enable(reg);
+		if (ret < 0)
+			goto disable;
+
 		opp_table->regulators[i] = reg;
 	}
 
@@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
 
 	return opp_table;
 
+disable:
+	regulator_put(reg);
+	--i;
+
 free_regulators:
-	while (i != 0)
-		regulator_put(opp_table->regulators[--i]);
+	for (; i >= 0; --i) {
+		regulator_disable(opp_table->regulators[i]);
+		regulator_put(opp_table->regulators[i]);
+	}
 
 	kfree(opp_table->regulators);
 	opp_table->regulators = NULL;
@@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
 	/* Make sure there are no concurrent readers while updating opp_table */
 	WARN_ON(!list_empty(&opp_table->opp_list));
 
-	for (i = opp_table->regulator_count - 1; i >= 0; i--)
+	for (i = opp_table->regulator_count - 1; i >= 0; i--) {
+		regulator_disable(opp_table->regulators[i]);
 		regulator_put(opp_table->regulators[i]);
+	}
 
 	_free_set_opp_data(opp_table);
 
-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] opp: core: add regulators enable and disable

[Chanwoo Choi]

Hi Kamil,

On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Add enable regulators to dev_pm_opp_set_regulators() and disable
> regulators to dev_pm_opp_put_regulators(). This prepares for
> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().

IMHO, it is not proper to mention the specific driver name.
If you explain the reason why enable the regulator before using it,
it is enough description.

> 
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> --
> Changes in v2:
> 
> - move regulator enable and disable into loop
> 
> ---
>  drivers/opp/core.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..069c5cf8827e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>  			goto free_regulators;
>  		}
>  
> +		ret = regulator_enable(reg);
> +		if (ret < 0)
> +			goto disable;
> +
>  		opp_table->regulators[i] = reg;
>  	}
>  
> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>  
>  	return opp_table;
>  
> +disable:
> +	regulator_put(reg);
> +	--i;
> +
>  free_regulators:
> -	while (i != 0)
> -		regulator_put(opp_table->regulators[--i]);
> +	for (; i >= 0; --i) {
> +		regulator_disable(opp_table->regulators[i]);
> +		regulator_put(opp_table->regulators[i]);
> +	}
>  
>  	kfree(opp_table->regulators);
>  	opp_table->regulators = NULL;
> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>  	/* Make sure there are no concurrent readers while updating opp_table */
>  	WARN_ON(!list_empty(&opp_table->opp_list));
>  
> -	for (i = opp_table->regulator_count - 1; i >= 0; i--)
> +	for (i = opp_table->regulator_count - 1; i >= 0; i--) {
> +		regulator_disable(opp_table->regulators[i]);
>  		regulator_put(opp_table->regulators[i]);
> +	}
>  
>  	_free_set_opp_data(opp_table);
>  
> 

I agree to enable the regulator before using it.
The bootloader might not enable the regulators
and the kernel need to enable regulator in order to increase
the reference count explicitly event if bootloader enables it.

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] opp: core: add regulators enable and disable

[Kamil Konieczny]

On 16.07.2019 06:03, Chanwoo Choi wrote:
> Hi Kamil,
> 
> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>> regulators to dev_pm_opp_put_regulators(). This prepares for
>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
> 
> IMHO, it is not proper to mention the specific driver name.
> If you explain the reason why enable the regulator before using it,
> it is enough description.
> 
>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>> --
>> Changes in v2:
>>
>> - move regulator enable and disable into loop
>>
>> ---
>>  drivers/opp/core.c | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..069c5cf8827e 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>  			goto free_regulators;
>>  		}
>>  
>> +		ret = regulator_enable(reg);
>> +		if (ret < 0)
>> +			goto disable;
>> +
>>  		opp_table->regulators[i] = reg;
>>  	}
>>  
>> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>  
>>  	return opp_table;
>>  
>> +disable:
>> +	regulator_put(reg);
>> +	--i;
>> +
>>  free_regulators:
>> -	while (i != 0)
>> -		regulator_put(opp_table->regulators[--i]);
>> +	for (; i >= 0; --i) {
>> +		regulator_disable(opp_table->regulators[i]);
>> +		regulator_put(opp_table->regulators[i]);
>> +	}
>>  
>>  	kfree(opp_table->regulators);
>>  	opp_table->regulators = NULL;
>> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>>  	/* Make sure there are no concurrent readers while updating opp_table */
>>  	WARN_ON(!list_empty(&opp_table->opp_list));
>>  
>> -	for (i = opp_table->regulator_count - 1; i >= 0; i--)
>> +	for (i = opp_table->regulator_count - 1; i >= 0; i--) {
>> +		regulator_disable(opp_table->regulators[i]);
>>  		regulator_put(opp_table->regulators[i]);
>> +	}
>>  
>>  	_free_set_opp_data(opp_table);
>>  
>>
> 
> I agree to enable the regulator before using it.
> The bootloader might not enable the regulators
> and the kernel need to enable regulator in order to increase
> the reference count explicitly event if bootloader enables it.
> 
> Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

Thank you, I will change commit description and send v3.

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] opp: core: add regulators enable and disable

[Viresh Kumar]

On 15-07-19, 14:04, Kamil Konieczny wrote:
> Add enable regulators to dev_pm_opp_set_regulators() and disable
> regulators to dev_pm_opp_put_regulators(). This prepares for
> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
> 
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> --
> Changes in v2:
> 
> - move regulator enable and disable into loop
> 
> ---
>  drivers/opp/core.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
> index 0e7703fe733f..069c5cf8827e 100644
> --- a/drivers/opp/core.c
> +++ b/drivers/opp/core.c
> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>  			goto free_regulators;
>  		}
>  
> +		ret = regulator_enable(reg);
> +		if (ret < 0)
> +			goto disable;

The name of this label is logically incorrect because we won't disable
the regulator from there but put it. Over that, I would rather prefer
to remove the label and add regulator_put() here itself.

> +
>  		opp_table->regulators[i] = reg;
>  	}
>  
> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>  
>  	return opp_table;
>  
> +disable:
> +	regulator_put(reg);
> +	--i;
> +
>  free_regulators:
> -	while (i != 0)
> -		regulator_put(opp_table->regulators[--i]);
> +	for (; i >= 0; --i) {
> +		regulator_disable(opp_table->regulators[i]);
> +		regulator_put(opp_table->regulators[i]);

This is incorrect as this will now try to put/disable the regulator
which we failed to acquire. As --i happens only after the loop has run
once. You can rather do:

	while (i--) {
		regulator_disable(opp_table->regulators[i]);
		regulator_put(opp_table->regulators[i]);
        }


> +	}
>  
>  	kfree(opp_table->regulators);
>  	opp_table->regulators = NULL;
> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>  	/* Make sure there are no concurrent readers while updating opp_table */
>  	WARN_ON(!list_empty(&opp_table->opp_list));
>  
> -	for (i = opp_table->regulator_count - 1; i >= 0; i--)
> +	for (i = opp_table->regulator_count - 1; i >= 0; i--) {
> +		regulator_disable(opp_table->regulators[i]);
>  		regulator_put(opp_table->regulators[i]);
> +	}
>  
>  	_free_set_opp_data(opp_table);
>  
> -- 
> 2.22.0

-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 1/4] opp: core: add regulators enable and disable

[Kamil Konieczny]

On 16.07.2019 12:05, Viresh Kumar wrote:
> On 15-07-19, 14:04, Kamil Konieczny wrote:
>> Add enable regulators to dev_pm_opp_set_regulators() and disable
>> regulators to dev_pm_opp_put_regulators(). This prepares for
>> converting exynos-bus devfreq driver to use dev_pm_opp_set_rate().
>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>> --
>> Changes in v2:
>>
>> - move regulator enable and disable into loop
>>
>> ---
>>  drivers/opp/core.c | 18 +++++++++++++++---
>>  1 file changed, 15 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/opp/core.c b/drivers/opp/core.c
>> index 0e7703fe733f..069c5cf8827e 100644
>> --- a/drivers/opp/core.c
>> +++ b/drivers/opp/core.c
>> @@ -1570,6 +1570,10 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>  			goto free_regulators;
>>  		}
>>  
>> +		ret = regulator_enable(reg);
>> +		if (ret < 0)
>> +			goto disable;
> 
> The name of this label is logically incorrect because we won't disable
> the regulator from there but put it. Over that, I would rather prefer
> to remove the label and add regulator_put() here itself.

I will change this and following according to your suggestions and will send v3.

>> +
>>  		opp_table->regulators[i] = reg;
>>  	}
>>  
>> @@ -1582,9 +1586,15 @@ struct opp_table *dev_pm_opp_set_regulators(struct device *dev,
>>  
>>  	return opp_table;
>>  
>> +disable:
>> +	regulator_put(reg);
>> +	--i;
>> +
>>  free_regulators:
>> -	while (i != 0)
>> -		regulator_put(opp_table->regulators[--i]);
>> +	for (; i >= 0; --i) {
>> +		regulator_disable(opp_table->regulators[i]);
>> +		regulator_put(opp_table->regulators[i]);
> 
> This is incorrect as this will now try to put/disable the regulator
> which we failed to acquire. As --i happens only after the loop has run
> once. You can rather do:
> 
> 	while (i--) {
> 		regulator_disable(opp_table->regulators[i]);
> 		regulator_put(opp_table->regulators[i]);
>         }
> 
> 
>> +	}
>>  
>>  	kfree(opp_table->regulators);
>>  	opp_table->regulators = NULL;
>> @@ -1610,8 +1620,10 @@ void dev_pm_opp_put_regulators(struct opp_table *opp_table)
>>  	/* Make sure there are no concurrent readers while updating opp_table */
>>  	WARN_ON(!list_empty(&opp_table->opp_list));
>>  
>> -	for (i = opp_table->regulator_count - 1; i >= 0; i--)
>> +	for (i = opp_table->regulator_count - 1; i >= 0; i--) {
>> +		regulator_disable(opp_table->regulators[i]);
>>  		regulator_put(opp_table->regulators[i]);
>> +	}
>>  
>>  	_free_set_opp_data(opp_table);
>>  
>> -- 
>> 2.22.0
> 

-- 
Best regards,
Kamil Konieczny
Samsung R&D Institute Poland


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()

[Kamil Konieczny]

Reuse opp core code for setting bus clock and voltage. As a side
effect this allow useage of coupled regulators feature (required
for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
uses regulator_set_voltage_triplet() for setting regulator voltage
while the old code used regulator_set_voltage_tol() with fixed
tolerance. This patch also removes no longer needed parsing of DT
property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
it).

Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
 1 file changed, 66 insertions(+), 106 deletions(-)

diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
index 486cc5b422f1..7fc4f76bd848 100644
--- a/drivers/devfreq/exynos-bus.c
+++ b/drivers/devfreq/exynos-bus.c
@@ -25,7 +25,6 @@
 #include <linux/slab.h>
 
 #define DEFAULT_SATURATION_RATIO	40
-#define DEFAULT_VOLTAGE_TOLERANCE	2
 
 struct exynos_bus {
 	struct device *dev;
@@ -37,9 +36,9 @@ struct exynos_bus {
 
 	unsigned long curr_freq;
 
-	struct regulator *regulator;
+	struct opp_table *opp_table;
+
 	struct clk *clk;
-	unsigned int voltage_tolerance;
 	unsigned int ratio;
 };
 
@@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
 {
 	struct exynos_bus *bus = dev_get_drvdata(dev);
 	struct dev_pm_opp *new_opp;
-	unsigned long old_freq, new_freq, new_volt, tol;
 	int ret = 0;
-
-	/* Get new opp-bus instance according to new bus clock */
+	/*
+	 * New frequency for bus may not be exactly matched to opp, adjust
+	 * *freq to correct value.
+	 */
 	new_opp = devfreq_recommended_opp(dev, freq, flags);
 	if (IS_ERR(new_opp)) {
 		dev_err(dev, "failed to get recommended opp instance\n");
 		return PTR_ERR(new_opp);
 	}
 
-	new_freq = dev_pm_opp_get_freq(new_opp);
-	new_volt = dev_pm_opp_get_voltage(new_opp);
 	dev_pm_opp_put(new_opp);
 
-	old_freq = bus->curr_freq;
-
-	if (old_freq == new_freq)
-		return 0;
-	tol = new_volt * bus->voltage_tolerance / 100;
-
 	/* Change voltage and frequency according to new OPP level */
 	mutex_lock(&bus->lock);
+	ret = dev_pm_opp_set_rate(dev, *freq);
+	if (!ret)
+		bus->curr_freq = *freq;
 
-	if (old_freq < new_freq) {
-		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
-		if (ret < 0) {
-			dev_err(bus->dev, "failed to set voltage\n");
-			goto out;
-		}
-	}
-
-	ret = clk_set_rate(bus->clk, new_freq);
-	if (ret < 0) {
-		dev_err(dev, "failed to change clock of bus\n");
-		clk_set_rate(bus->clk, old_freq);
-		goto out;
-	}
-
-	if (old_freq > new_freq) {
-		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
-		if (ret < 0) {
-			dev_err(bus->dev, "failed to set voltage\n");
-			goto out;
-		}
-	}
-	bus->curr_freq = new_freq;
-
-	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
-			old_freq, new_freq, clk_get_rate(bus->clk));
-out:
 	mutex_unlock(&bus->lock);
 
 	return ret;
@@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
 	if (ret < 0)
 		dev_warn(dev, "failed to disable the devfreq-event devices\n");
 
-	if (bus->regulator)
-		regulator_disable(bus->regulator);
+	if (bus->opp_table)
+		dev_pm_opp_put_regulators(bus->opp_table);
 
 	dev_pm_opp_of_remove_table(dev);
+
 	clk_disable_unprepare(bus->clk);
 }
 
@@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
 {
 	struct exynos_bus *bus = dev_get_drvdata(dev);
 	struct dev_pm_opp *new_opp;
-	unsigned long old_freq, new_freq;
-	int ret = 0;
+	int ret;
 
-	/* Get new opp-bus instance according to new bus clock */
+	/*
+	 * New frequency for bus may not be exactly matched to opp, adjust
+	 * *freq to correct value.
+	 */
 	new_opp = devfreq_recommended_opp(dev, freq, flags);
 	if (IS_ERR(new_opp)) {
 		dev_err(dev, "failed to get recommended opp instance\n");
 		return PTR_ERR(new_opp);
 	}
 
-	new_freq = dev_pm_opp_get_freq(new_opp);
 	dev_pm_opp_put(new_opp);
 
-	old_freq = bus->curr_freq;
-
-	if (old_freq == new_freq)
-		return 0;
-
 	/* Change the frequency according to new OPP level */
 	mutex_lock(&bus->lock);
+	ret = dev_pm_opp_set_rate(dev, *freq);
+	if (!ret)
+		bus->curr_freq = *freq;
 
-	ret = clk_set_rate(bus->clk, new_freq);
-	if (ret < 0) {
-		dev_err(dev, "failed to set the clock of bus\n");
-		goto out;
-	}
-
-	*freq = new_freq;
-	bus->curr_freq = new_freq;
-
-	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
-			old_freq, new_freq, clk_get_rate(bus->clk));
-out:
 	mutex_unlock(&bus->lock);
 
 	return ret;
@@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
 					struct exynos_bus *bus)
 {
 	struct device *dev = bus->dev;
-	int i, ret, count, size;
-
-	/* Get the regulator to provide each bus with the power */
-	bus->regulator = devm_regulator_get(dev, "vdd");
-	if (IS_ERR(bus->regulator)) {
-		dev_err(dev, "failed to get VDD regulator\n");
-		return PTR_ERR(bus->regulator);
-	}
-
-	ret = regulator_enable(bus->regulator);
-	if (ret < 0) {
-		dev_err(dev, "failed to enable VDD regulator\n");
-		return ret;
-	}
+	int i, count, size;
 
 	/*
 	 * Get the devfreq-event devices to get the current utilization of
@@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
 	count = devfreq_event_get_edev_count(dev);
 	if (count < 0) {
 		dev_err(dev, "failed to get the count of devfreq-event dev\n");
-		ret = count;
-		goto err_regulator;
+		return count;
 	}
+
 	bus->edev_count = count;
 
 	size = sizeof(*bus->edev) * count;
 	bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
-	if (!bus->edev) {
-		ret = -ENOMEM;
-		goto err_regulator;
-	}
+	if (!bus->edev)
+		return -ENOMEM;
 
 	for (i = 0; i < count; i++) {
 		bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
-		if (IS_ERR(bus->edev[i])) {
-			ret = -EPROBE_DEFER;
-			goto err_regulator;
-		}
+		if (IS_ERR(bus->edev[i]))
+			return -EPROBE_DEFER;
 	}
 
 	/*
@@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
 	if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
 		bus->ratio = DEFAULT_SATURATION_RATIO;
 
-	if (of_property_read_u32(np, "exynos,voltage-tolerance",
-					&bus->voltage_tolerance))
-		bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
-
 	return 0;
-
-err_regulator:
-	regulator_disable(bus->regulator);
-
-	return ret;
 }
 
 static int exynos_bus_parse_of(struct device_node *np,
-			      struct exynos_bus *bus)
+			      struct exynos_bus *bus, bool passive)
 {
 	struct device *dev = bus->dev;
+	struct opp_table *opp_table;
+	const char *vdd = "vdd";
 	struct dev_pm_opp *opp;
 	unsigned long rate;
 	int ret;
@@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
 		return ret;
 	}
 
+	if (!passive) {
+		opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
+		if (IS_ERR(opp_table)) {
+			ret = PTR_ERR(opp_table);
+			dev_err(dev, "failed to set regulators %d\n", ret);
+			goto err_clk;
+		}
+
+		bus->opp_table = opp_table;
+	}
+
 	/* Get the freq and voltage from OPP table to scale the bus freq */
 	ret = dev_pm_opp_of_add_table(dev);
 	if (ret < 0) {
 		dev_err(dev, "failed to get OPP table\n");
-		goto err_clk;
+		goto err_regulator;
 	}
 
 	rate = clk_get_rate(bus->clk);
@@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
 		ret = PTR_ERR(opp);
 		goto err_opp;
 	}
+
 	bus->curr_freq = dev_pm_opp_get_freq(opp);
 	dev_pm_opp_put(opp);
 
@@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
 
 err_opp:
 	dev_pm_opp_of_remove_table(dev);
+
+err_regulator:
+	if (bus->opp_table) {
+		dev_pm_opp_put_regulators(bus->opp_table);
+		bus->opp_table = NULL;
+	}
+
 err_clk:
 	clk_disable_unprepare(bus->clk);
 
@@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	struct exynos_bus *bus;
 	int ret, max_state;
 	unsigned long min_freq, max_freq;
+	bool passive = false;
 
 	if (!np) {
 		dev_err(dev, "failed to find devicetree node\n");
@@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
 	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
 	if (!bus)
 		return -ENOMEM;
+
 	mutex_init(&bus->lock);
 	bus->dev = &pdev->dev;
 	platform_set_drvdata(pdev, bus);
+	node = of_parse_phandle(dev->of_node, "devfreq", 0);
+	if (node) {
+		of_node_put(node);
+		passive = true;
+	}
 
 	/* Parse the device-tree to get the resource information */
-	ret = exynos_bus_parse_of(np, bus);
+	ret = exynos_bus_parse_of(np, bus, passive);
 	if (ret < 0)
 		return ret;
 
@@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
 		goto err;
 	}
 
-	node = of_parse_phandle(dev->of_node, "devfreq", 0);
-	if (node) {
-		of_node_put(node);
+	if (passive)
 		goto passive;
-	} else {
-		ret = exynos_bus_parent_parse_of(np, bus);
-	}
+
+	ret = exynos_bus_parent_parse_of(np, bus);
 
 	if (ret < 0)
 		goto err;
@@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
 
 err:
 	dev_pm_opp_of_remove_table(dev);
+	if (bus->opp_table) {
+		dev_pm_opp_put_regulators(bus->opp_table);
+		bus->opp_table = NULL;
+	}
+
 	clk_disable_unprepare(bus->clk);
 
 	return ret;
-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()

[Chanwoo Choi]

Hi Kamil,

Looks good to me. But, this patch has some issue.
I added the detailed reviews.

I recommend that you make the separate patches as following
in order to clarify the role of which apply the dev_pm_opp_* function.

First patch,
Need to consolidate the following two function into one function.
because the original exynos-bus.c has the problem that the regulator
of parent devfreq device have to be enabled before enabling the clock.
This issue did not happen because bootloader enables the bus-related
regulators before kernel booting.
- exynos_bus_parse_of()
- exynos_bus_parent_parse_of()

Second patch,
Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()


On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Reuse opp core code for setting bus clock and voltage. As a side
> effect this allow useage of coupled regulators feature (required
> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
> uses regulator_set_voltage_triplet() for setting regulator voltage
> while the old code used regulator_set_voltage_tol() with fixed
> tolerance. This patch also removes no longer needed parsing of DT
> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
> it).
> 
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>  1 file changed, 66 insertions(+), 106 deletions(-)
> 
> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
> index 486cc5b422f1..7fc4f76bd848 100644
> --- a/drivers/devfreq/exynos-bus.c
> +++ b/drivers/devfreq/exynos-bus.c
> @@ -25,7 +25,6 @@
>  #include <linux/slab.h>
>  
>  #define DEFAULT_SATURATION_RATIO	40
> -#define DEFAULT_VOLTAGE_TOLERANCE	2
>  
>  struct exynos_bus {
>  	struct device *dev;
> @@ -37,9 +36,9 @@ struct exynos_bus {
>  
>  	unsigned long curr_freq;
>  
> -	struct regulator *regulator;
> +	struct opp_table *opp_table;
> +
>  	struct clk *clk;
> -	unsigned int voltage_tolerance;
>  	unsigned int ratio;
>  };
>  
> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>  {
>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>  	struct dev_pm_opp *new_opp;
> -	unsigned long old_freq, new_freq, new_volt, tol;
>  	int ret = 0;
> -
> -	/* Get new opp-bus instance according to new bus clock */
> +	/*
> +	 * New frequency for bus may not be exactly matched to opp, adjust
> +	 * *freq to correct value.
> +	 */

You better to change this comment with following styles
to keep the consistency:

	/* Get correct frequency for bus ... */

>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(new_opp)) {
>  		dev_err(dev, "failed to get recommended opp instance\n");
>  		return PTR_ERR(new_opp);
>  	}
>  
> -	new_freq = dev_pm_opp_get_freq(new_opp);
> -	new_volt = dev_pm_opp_get_voltage(new_opp);
>  	dev_pm_opp_put(new_opp);
>  
> -	old_freq = bus->curr_freq;
> -
> -	if (old_freq == new_freq)
> -		return 0;
> -	tol = new_volt * bus->voltage_tolerance / 100;
> -
>  	/* Change voltage and frequency according to new OPP level */
>  	mutex_lock(&bus->lock);
> +	ret = dev_pm_opp_set_rate(dev, *freq);
> +	if (!ret)
> +		bus->curr_freq = *freq;

Have to print the error log if ret has minus error value.
Modify it as following:

	if (ret < 0) {
		dev_err(dev, "failed to set bus rate\n");
		goto err:
	}
	bus->curr_freq = *freq;

err:
	mutex_unlock(&bus->lock);
	
	return ret;

>  
> -	if (old_freq < new_freq) {
> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> -		if (ret < 0) {
> -			dev_err(bus->dev, "failed to set voltage\n");
> -			goto out;
> -		}
> -	}
> -
> -	ret = clk_set_rate(bus->clk, new_freq);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to change clock of bus\n");
> -		clk_set_rate(bus->clk, old_freq);
> -		goto out;
> -	}
> -
> -	if (old_freq > new_freq) {
> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
> -		if (ret < 0) {
> -			dev_err(bus->dev, "failed to set voltage\n");
> -			goto out;
> -		}
> -	}
> -	bus->curr_freq = new_freq;
> -
> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> -			old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
>  	mutex_unlock(&bus->lock);
>  
>  	return ret;
> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>  	if (ret < 0)
>  		dev_warn(dev, "failed to disable the devfreq-event devices\n");
>  
> -	if (bus->regulator)
> -		regulator_disable(bus->regulator);
> +	if (bus->opp_table)
> +		dev_pm_opp_put_regulators(bus->opp_table);

Have to disable regulator after disabling the clock
to prevent the h/w fault.

I think that you should call them with following sequence:

	clk_disable_unprepare(bus->clk);
	if (bus->opp_table)
		dev_pm_opp_put_regulators(bus->opp_table);
	dev_pm_opp_of_remove_table(dev);

>  
>  	dev_pm_opp_of_remove_table(dev);
> +
>  	clk_disable_unprepare(bus->clk);
>  }
>  
> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>  {
>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>  	struct dev_pm_opp *new_opp;
> -	unsigned long old_freq, new_freq;
> -	int ret = 0;
> +	int ret;
>  
> -	/* Get new opp-bus instance according to new bus clock */
> +	/*
> +	 * New frequency for bus may not be exactly matched to opp, adjust
> +	 * *freq to correct value.
> +	 */

You better to change this comment with following styles
to keep the consistency:

	/* Get correct frequency for bus ... */

>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>  	if (IS_ERR(new_opp)) {
>  		dev_err(dev, "failed to get recommended opp instance\n");
>  		return PTR_ERR(new_opp);
>  	}
>  
> -	new_freq = dev_pm_opp_get_freq(new_opp);
>  	dev_pm_opp_put(new_opp);
>  
> -	old_freq = bus->curr_freq;
> -
> -	if (old_freq == new_freq)
> -		return 0;
> -
>  	/* Change the frequency according to new OPP level */
>  	mutex_lock(&bus->lock);
> +	ret = dev_pm_opp_set_rate(dev, *freq);
> +	if (!ret)
> +		bus->curr_freq = *freq;

ditto. Have to print the error log, check above comment.

>  
> -	ret = clk_set_rate(bus->clk, new_freq);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to set the clock of bus\n");
> -		goto out;
> -	}
> -
> -	*freq = new_freq;
> -	bus->curr_freq = new_freq;
> -
> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
> -			old_freq, new_freq, clk_get_rate(bus->clk));
> -out:
>  	mutex_unlock(&bus->lock);
>  
>  	return ret;
> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>  					struct exynos_bus *bus)
>  {
>  	struct device *dev = bus->dev;
> -	int i, ret, count, size;
> -
> -	/* Get the regulator to provide each bus with the power */
> -	bus->regulator = devm_regulator_get(dev, "vdd");
> -	if (IS_ERR(bus->regulator)) {
> -		dev_err(dev, "failed to get VDD regulator\n");
> -		return PTR_ERR(bus->regulator);
> -	}
> -
> -	ret = regulator_enable(bus->regulator);
> -	if (ret < 0) {
> -		dev_err(dev, "failed to enable VDD regulator\n");
> -		return ret;
> -	}
> +	int i, count, size;
>  
>  	/*
>  	 * Get the devfreq-event devices to get the current utilization of
> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>  	count = devfreq_event_get_edev_count(dev);
>  	if (count < 0) {
>  		dev_err(dev, "failed to get the count of devfreq-event dev\n");
> -		ret = count;
> -		goto err_regulator;
> +		return count;
>  	}
> +
>  	bus->edev_count = count;
>  
>  	size = sizeof(*bus->edev) * count;
>  	bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
> -	if (!bus->edev) {
> -		ret = -ENOMEM;
> -		goto err_regulator;
> -	}
> +	if (!bus->edev)
> +		return -ENOMEM;
>  
>  	for (i = 0; i < count; i++) {
>  		bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
> -		if (IS_ERR(bus->edev[i])) {
> -			ret = -EPROBE_DEFER;
> -			goto err_regulator;
> -		}
> +		if (IS_ERR(bus->edev[i]))
> +			return -EPROBE_DEFER;
>  	}
>  
>  	/*
> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>  	if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>  		bus->ratio = DEFAULT_SATURATION_RATIO;
>  
> -	if (of_property_read_u32(np, "exynos,voltage-tolerance",
> -					&bus->voltage_tolerance))
> -		bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
> -
>  	return 0;
> -
> -err_regulator:
> -	regulator_disable(bus->regulator);
> -
> -	return ret;
>  }
>  
>  static int exynos_bus_parse_of(struct device_node *np,
> -			      struct exynos_bus *bus)
> +			      struct exynos_bus *bus, bool passive)
>  {
>  	struct device *dev = bus->dev;
> +	struct opp_table *opp_table;
> +	const char *vdd = "vdd";
>  	struct dev_pm_opp *opp;
>  	unsigned long rate;
>  	int ret;
> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>  		return ret;
>  	}
>  
> +	if (!passive) {
> +		opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
> +		if (IS_ERR(opp_table)) {
> +			ret = PTR_ERR(opp_table);
> +			dev_err(dev, "failed to set regulators %d\n", ret);
> +			goto err_clk;/
> +		}
> +
> +		bus->opp_table = opp_table;
> +	}

This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
because the regulator is only used by parent devfreq device.

> +
>  	/* Get the freq and voltage from OPP table to scale the bus freq */
>  	ret = dev_pm_opp_of_add_table(dev);
>  	if (ret < 0) {
>  		dev_err(dev, "failed to get OPP table\n");
> -		goto err_clk;
> +		goto err_regulator;
>  	}
>  
>  	rate = clk_get_rate(bus->clk);
> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>  		ret = PTR_ERR(opp);
>  		goto err_opp;
>  	}
> +
>  	bus->curr_freq = dev_pm_opp_get_freq(opp);
>  	dev_pm_opp_put(opp);
>  
> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>  
>  err_opp:
>  	dev_pm_opp_of_remove_table(dev);
> +
> +err_regulator:
> +	if (bus->opp_table) {
> +		dev_pm_opp_put_regulators(bus->opp_table);
> +		bus->opp_table = NULL;
> +	}

As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
after removing the opp_table by dev_pm_opp_of_remove_table().

> +
>  err_clk:
>  	clk_disable_unprepare(bus->clk);
>  
> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  	struct exynos_bus *bus;
>  	int ret, max_state;
>  	unsigned long min_freq, max_freq;
> +	bool passive = false;
>  
>  	if (!np) {
>  		dev_err(dev, "failed to find devicetree node\n");
> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>  	if (!bus)
>  		return -ENOMEM;
> +
>  	mutex_init(&bus->lock);
>  	bus->dev = &pdev->dev;
>  	platform_set_drvdata(pdev, bus);
> +	node = of_parse_phandle(dev->of_node, "devfreq", 0);
> +	if (node) {
> +		of_node_put(node);
> +		passive = true;
> +	}
>  
>  	/* Parse the device-tree to get the resource information */
> -	ret = exynos_bus_parse_of(np, bus);
> +	ret = exynos_bus_parse_of(np, bus, passive);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  		goto err;
>  	}
>  
> -	node = of_parse_phandle(dev->of_node, "devfreq", 0);
> -	if (node) {
> -		of_node_put(node);
> +	if (passive)
>  		goto passive;
> -	} else {
> -		ret = exynos_bus_parent_parse_of(np, bus);
> -	}
> +
> +	ret = exynos_bus_parent_parse_of(np, bus);
>  

Remove unneeded blank line.

>  	if (ret < 0)
>  		goto err;
> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>  
>  err:
>  	dev_pm_opp_of_remove_table(dev);
> +	if (bus->opp_table) {
> +		dev_pm_opp_put_regulators(bus->opp_table);
> +		bus->opp_table = NULL;
> +	}
> +

ditto.
Have to disable regulator after disabling the clock
to prevent the h/w fault.

I think that you should call them with following sequence:

	clk_disable_unprepare(bus->clk);
	if (bus->opp_table)
		dev_pm_opp_put_regulators(bus->opp_table);
	dev_pm_opp_of_remove_table(dev);

>  	clk_disable_unprepare(bus->clk);
>  
>  	return ret;
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()

[Bartlomiej Zolnierkiewicz]

Hi Chanwoo,

On 7/16/19 5:56 AM, Chanwoo Choi wrote:
> Hi Kamil,
> 
> Looks good to me. But, this patch has some issue.
> I added the detailed reviews.
> 
> I recommend that you make the separate patches as following
> in order to clarify the role of which apply the dev_pm_opp_* function.
> 
> First patch,
> Need to consolidate the following two function into one function.
> because the original exynos-bus.c has the problem that the regulator
> of parent devfreq device have to be enabled before enabling the clock.
> This issue did not happen because bootloader enables the bus-related
> regulators before kernel booting.
> - exynos_bus_parse_of()
> - exynos_bus_parent_parse_of()
> > Second patch,
> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
> 
> 
> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>> Reuse opp core code for setting bus clock and voltage. As a side
>> effect this allow useage of coupled regulators feature (required
>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>> uses regulator_set_voltage_triplet() for setting regulator voltage
>> while the old code used regulator_set_voltage_tol() with fixed
>> tolerance. This patch also removes no longer needed parsing of DT
>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>> it).
>>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>> ---
>>  drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>  1 file changed, 66 insertions(+), 106 deletions(-)
>>
>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>> index 486cc5b422f1..7fc4f76bd848 100644
>> --- a/drivers/devfreq/exynos-bus.c
>> +++ b/drivers/devfreq/exynos-bus.c
>> @@ -25,7 +25,6 @@
>>  #include <linux/slab.h>
>>  
>>  #define DEFAULT_SATURATION_RATIO	40
>> -#define DEFAULT_VOLTAGE_TOLERANCE	2
>>  
>>  struct exynos_bus {
>>  	struct device *dev;
>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>  
>>  	unsigned long curr_freq;
>>  
>> -	struct regulator *regulator;
>> +	struct opp_table *opp_table;
>> +
>>  	struct clk *clk;
>> -	unsigned int voltage_tolerance;
>>  	unsigned int ratio;
>>  };
>>  
>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>  {
>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>  	struct dev_pm_opp *new_opp;
>> -	unsigned long old_freq, new_freq, new_volt, tol;
>>  	int ret = 0;
>> -
>> -	/* Get new opp-bus instance according to new bus clock */
>> +	/*
>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>> +	 * *freq to correct value.
>> +	 */
> 
> You better to change this comment with following styles
> to keep the consistency:
> 
> 	/* Get correct frequency for bus ... */
> 
>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>  	if (IS_ERR(new_opp)) {
>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>  		return PTR_ERR(new_opp);
>>  	}
>>  
>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>> -	new_volt = dev_pm_opp_get_voltage(new_opp);
>>  	dev_pm_opp_put(new_opp);
>>  
>> -	old_freq = bus->curr_freq;
>> -
>> -	if (old_freq == new_freq)
>> -		return 0;
>> -	tol = new_volt * bus->voltage_tolerance / 100;
>> -
>>  	/* Change voltage and frequency according to new OPP level */
>>  	mutex_lock(&bus->lock);
>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>> +	if (!ret)
>> +		bus->curr_freq = *freq;
> 
> Have to print the error log if ret has minus error value.

dev_pm_opp_set_rate() should print the error message on all
errors so wouldn't printing the error log also here be superfluous?

[ Please also note that the other user of dev_pm_opp_set_rate()
  (cpufreq-dt cpufreq driver) doesn't do this. ]

> Modify it as following:
> 
> 	if (ret < 0) {
> 		dev_err(dev, "failed to set bus rate\n");
> 		goto err:
> 	}
> 	bus->curr_freq = *freq;
> 
> err:
> 	mutex_unlock(&bus->lock);
> 	
> 	return ret;
> 
>>  
>> -	if (old_freq < new_freq) {
>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>> -		if (ret < 0) {
>> -			dev_err(bus->dev, "failed to set voltage\n");
>> -			goto out;
>> -		}
>> -	}
>> -
>> -	ret = clk_set_rate(bus->clk, new_freq);
>> -	if (ret < 0) {
>> -		dev_err(dev, "failed to change clock of bus\n");
>> -		clk_set_rate(bus->clk, old_freq);
>> -		goto out;
>> -	}
>> -
>> -	if (old_freq > new_freq) {
>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>> -		if (ret < 0) {
>> -			dev_err(bus->dev, "failed to set voltage\n");
>> -			goto out;
>> -		}
>> -	}
>> -	bus->curr_freq = new_freq;
>> -
>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>> -out:
>>  	mutex_unlock(&bus->lock);
>>  
>>  	return ret;
>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>  	if (ret < 0)
>>  		dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>  
>> -	if (bus->regulator)
>> -		regulator_disable(bus->regulator);
>> +	if (bus->opp_table)
>> +		dev_pm_opp_put_regulators(bus->opp_table);
> 
> Have to disable regulator after disabling the clock
> to prevent the h/w fault.
> 
> I think that you should call them with following sequence:
> 
> 	clk_disable_unprepare(bus->clk);
> 	if (bus->opp_table)
> 		dev_pm_opp_put_regulators(bus->opp_table);
> 	dev_pm_opp_of_remove_table(dev);
> 
>>  
>>  	dev_pm_opp_of_remove_table(dev);
>> +
>>  	clk_disable_unprepare(bus->clk);
>>  }
>>  
>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>  {
>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>  	struct dev_pm_opp *new_opp;
>> -	unsigned long old_freq, new_freq;
>> -	int ret = 0;
>> +	int ret;
>>  
>> -	/* Get new opp-bus instance according to new bus clock */
>> +	/*
>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>> +	 * *freq to correct value.
>> +	 */
> 
> You better to change this comment with following styles
> to keep the consistency:
> 
> 	/* Get correct frequency for bus ... */
> 
>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>  	if (IS_ERR(new_opp)) {
>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>  		return PTR_ERR(new_opp);
>>  	}
>>  
>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>>  	dev_pm_opp_put(new_opp);
>>  
>> -	old_freq = bus->curr_freq;
>> -
>> -	if (old_freq == new_freq)
>> -		return 0;
>> -
>>  	/* Change the frequency according to new OPP level */
>>  	mutex_lock(&bus->lock);
>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>> +	if (!ret)
>> +		bus->curr_freq = *freq;
> 
> ditto. Have to print the error log, check above comment.
> 
>>  
>> -	ret = clk_set_rate(bus->clk, new_freq);
>> -	if (ret < 0) {
>> -		dev_err(dev, "failed to set the clock of bus\n");
>> -		goto out;
>> -	}
>> -
>> -	*freq = new_freq;
>> -	bus->curr_freq = new_freq;
>> -
>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>> -out:
>>  	mutex_unlock(&bus->lock);
>>  
>>  	return ret;
>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>  					struct exynos_bus *bus)
>>  {
>>  	struct device *dev = bus->dev;
>> -	int i, ret, count, size;
>> -
>> -	/* Get the regulator to provide each bus with the power */
>> -	bus->regulator = devm_regulator_get(dev, "vdd");
>> -	if (IS_ERR(bus->regulator)) {
>> -		dev_err(dev, "failed to get VDD regulator\n");
>> -		return PTR_ERR(bus->regulator);
>> -	}
>> -
>> -	ret = regulator_enable(bus->regulator);
>> -	if (ret < 0) {
>> -		dev_err(dev, "failed to enable VDD regulator\n");
>> -		return ret;
>> -	}
>> +	int i, count, size;
>>  
>>  	/*
>>  	 * Get the devfreq-event devices to get the current utilization of
>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>  	count = devfreq_event_get_edev_count(dev);
>>  	if (count < 0) {
>>  		dev_err(dev, "failed to get the count of devfreq-event dev\n");
>> -		ret = count;
>> -		goto err_regulator;
>> +		return count;
>>  	}
>> +
>>  	bus->edev_count = count;
>>  
>>  	size = sizeof(*bus->edev) * count;
>>  	bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>> -	if (!bus->edev) {
>> -		ret = -ENOMEM;
>> -		goto err_regulator;
>> -	}
>> +	if (!bus->edev)
>> +		return -ENOMEM;
>>  
>>  	for (i = 0; i < count; i++) {
>>  		bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>> -		if (IS_ERR(bus->edev[i])) {
>> -			ret = -EPROBE_DEFER;
>> -			goto err_regulator;
>> -		}
>> +		if (IS_ERR(bus->edev[i]))
>> +			return -EPROBE_DEFER;
>>  	}
>>  
>>  	/*
>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>  	if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>  		bus->ratio = DEFAULT_SATURATION_RATIO;
>>  
>> -	if (of_property_read_u32(np, "exynos,voltage-tolerance",
>> -					&bus->voltage_tolerance))
>> -		bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>> -
>>  	return 0;
>> -
>> -err_regulator:
>> -	regulator_disable(bus->regulator);
>> -
>> -	return ret;
>>  }
>>  
>>  static int exynos_bus_parse_of(struct device_node *np,
>> -			      struct exynos_bus *bus)
>> +			      struct exynos_bus *bus, bool passive)
>>  {
>>  	struct device *dev = bus->dev;
>> +	struct opp_table *opp_table;
>> +	const char *vdd = "vdd";
>>  	struct dev_pm_opp *opp;
>>  	unsigned long rate;
>>  	int ret;
>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>  		return ret;
>>  	}
>>  
>> +	if (!passive) {
>> +		opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>> +		if (IS_ERR(opp_table)) {
>> +			ret = PTR_ERR(opp_table);
>> +			dev_err(dev, "failed to set regulators %d\n", ret);
>> +			goto err_clk;/
>> +		}
>> +
>> +		bus->opp_table = opp_table;
>> +	}
> 
> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
> because the regulator is only used by parent devfreq device.

exynos_bus_parse_of() is called for all devfreq devices (including
parent) and (as you've noticed) the regulator should be enabled before
enabling clock (which is done in exynos_bus_parse_of()) so adding
extra argument to exynos_bus_parse_of() (like it is done currently in
the patch) makes it possible to do the setup correctly without the need
of merging both functions into one huge function (which would be more
difficult to follow than two simpler functions IMHO). Is that approach
acceptable or do you prefer one big function?

>> +
>>  	/* Get the freq and voltage from OPP table to scale the bus freq */
>>  	ret = dev_pm_opp_of_add_table(dev);
>>  	if (ret < 0) {
>>  		dev_err(dev, "failed to get OPP table\n");
>> -		goto err_clk;
>> +		goto err_regulator;
>>  	}
>>  
>>  	rate = clk_get_rate(bus->clk);
>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>  		ret = PTR_ERR(opp);
>>  		goto err_opp;
>>  	}
>> +
>>  	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>  	dev_pm_opp_put(opp);
>>  
>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>  
>>  err_opp:
>>  	dev_pm_opp_of_remove_table(dev);
>> +
>> +err_regulator:
>> +	if (bus->opp_table) {
>> +		dev_pm_opp_put_regulators(bus->opp_table);
>> +		bus->opp_table = NULL;
>> +	}
> 
> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
> after removing the opp_table by dev_pm_opp_of_remove_table().
> 
>> +
>>  err_clk:
>>  	clk_disable_unprepare(bus->clk);
>>  
>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>  	struct exynos_bus *bus;
>>  	int ret, max_state;
>>  	unsigned long min_freq, max_freq;
>> +	bool passive = false;
>>  
>>  	if (!np) {
>>  		dev_err(dev, "failed to find devicetree node\n");
>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>  	if (!bus)
>>  		return -ENOMEM;
>> +
>>  	mutex_init(&bus->lock);
>>  	bus->dev = &pdev->dev;
>>  	platform_set_drvdata(pdev, bus);
>> +	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>> +	if (node) {
>> +		of_node_put(node);
>> +		passive = true;
>> +	}
>>  
>>  	/* Parse the device-tree to get the resource information */
>> -	ret = exynos_bus_parse_of(np, bus);
>> +	ret = exynos_bus_parse_of(np, bus, passive);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>  		goto err;
>>  	}
>>  
>> -	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>> -	if (node) {
>> -		of_node_put(node);
>> +	if (passive)
>>  		goto passive;
>> -	} else {
>> -		ret = exynos_bus_parent_parse_of(np, bus);
>> -	}
>> +
>> +	ret = exynos_bus_parent_parse_of(np, bus);
>>  
> 
> Remove unneeded blank line.
> 
>>  	if (ret < 0)
>>  		goto err;
>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>  
>>  err:
>>  	dev_pm_opp_of_remove_table(dev);
>> +	if (bus->opp_table) {
>> +		dev_pm_opp_put_regulators(bus->opp_table);
>> +		bus->opp_table = NULL;
>> +	}
>> +
> 
> ditto.
> Have to disable regulator after disabling the clock
> to prevent the h/w fault.
> 
> I think that you should call them with following sequence:
> 
> 	clk_disable_unprepare(bus->clk);
> 	if (bus->opp_table)
> 		dev_pm_opp_put_regulators(bus->opp_table);
> 	dev_pm_opp_of_remove_table(dev);
> 
>>  	clk_disable_unprepare(bus->clk);
>>  
>>  	return ret;

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()

[Chanwoo Choi]

Hi Bartlomiej,

On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
> 
> Hi Chanwoo,
> 
> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>> Hi Kamil,
>>
>> Looks good to me. But, this patch has some issue.
>> I added the detailed reviews.
>>
>> I recommend that you make the separate patches as following
>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>
>> First patch,
>> Need to consolidate the following two function into one function.
>> because the original exynos-bus.c has the problem that the regulator
>> of parent devfreq device have to be enabled before enabling the clock.
>> This issue did not happen because bootloader enables the bus-related
>> regulators before kernel booting.
>> - exynos_bus_parse_of()
>> - exynos_bus_parent_parse_of()
>>> Second patch,
>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>
>>
>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>> Reuse opp core code for setting bus clock and voltage. As a side
>>> effect this allow useage of coupled regulators feature (required
>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>> while the old code used regulator_set_voltage_tol() with fixed
>>> tolerance. This patch also removes no longer needed parsing of DT
>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>> it).
>>>
>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>> ---
>>>  drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>  1 file changed, 66 insertions(+), 106 deletions(-)
>>>
>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>> index 486cc5b422f1..7fc4f76bd848 100644
>>> --- a/drivers/devfreq/exynos-bus.c
>>> +++ b/drivers/devfreq/exynos-bus.c
>>> @@ -25,7 +25,6 @@
>>>  #include <linux/slab.h>
>>>  
>>>  #define DEFAULT_SATURATION_RATIO	40
>>> -#define DEFAULT_VOLTAGE_TOLERANCE	2
>>>  
>>>  struct exynos_bus {
>>>  	struct device *dev;
>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>  
>>>  	unsigned long curr_freq;
>>>  
>>> -	struct regulator *regulator;
>>> +	struct opp_table *opp_table;
>>> +
>>>  	struct clk *clk;
>>> -	unsigned int voltage_tolerance;
>>>  	unsigned int ratio;
>>>  };
>>>  
>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>  {
>>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>>  	struct dev_pm_opp *new_opp;
>>> -	unsigned long old_freq, new_freq, new_volt, tol;
>>>  	int ret = 0;
>>> -
>>> -	/* Get new opp-bus instance according to new bus clock */
>>> +	/*
>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>> +	 * *freq to correct value.
>>> +	 */
>>
>> You better to change this comment with following styles
>> to keep the consistency:
>>
>> 	/* Get correct frequency for bus ... */
>>
>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>  	if (IS_ERR(new_opp)) {
>>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>>  		return PTR_ERR(new_opp);
>>>  	}
>>>  
>>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>>> -	new_volt = dev_pm_opp_get_voltage(new_opp);
>>>  	dev_pm_opp_put(new_opp);
>>>  
>>> -	old_freq = bus->curr_freq;
>>> -
>>> -	if (old_freq == new_freq)
>>> -		return 0;
>>> -	tol = new_volt * bus->voltage_tolerance / 100;
>>> -
>>>  	/* Change voltage and frequency according to new OPP level */
>>>  	mutex_lock(&bus->lock);
>>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>>> +	if (!ret)
>>> +		bus->curr_freq = *freq;
>>
>> Have to print the error log if ret has minus error value.
> 
> dev_pm_opp_set_rate() should print the error message on all
> errors so wouldn't printing the error log also here be superfluous?
> 
> [ Please also note that the other user of dev_pm_opp_set_rate()
>   (cpufreq-dt cpufreq driver) doesn't do this. ]

OK. Thanks for the explanation. 

> 
>> Modify it as following:
>>
>> 	if (ret < 0) {
>> 		dev_err(dev, "failed to set bus rate\n");
>> 		goto err:
>> 	}
>> 	bus->curr_freq = *freq;
>>
>> err:
>> 	mutex_unlock(&bus->lock);
>> 	
>> 	return ret;
>>
>>>  
>>> -	if (old_freq < new_freq) {
>>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>> -		if (ret < 0) {
>>> -			dev_err(bus->dev, "failed to set voltage\n");
>>> -			goto out;
>>> -		}
>>> -	}
>>> -
>>> -	ret = clk_set_rate(bus->clk, new_freq);
>>> -	if (ret < 0) {
>>> -		dev_err(dev, "failed to change clock of bus\n");
>>> -		clk_set_rate(bus->clk, old_freq);
>>> -		goto out;
>>> -	}
>>> -
>>> -	if (old_freq > new_freq) {
>>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>> -		if (ret < 0) {
>>> -			dev_err(bus->dev, "failed to set voltage\n");
>>> -			goto out;
>>> -		}
>>> -	}
>>> -	bus->curr_freq = new_freq;
>>> -
>>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>>> -out:
>>>  	mutex_unlock(&bus->lock);
>>>  
>>>  	return ret;
>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>  	if (ret < 0)
>>>  		dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>  
>>> -	if (bus->regulator)
>>> -		regulator_disable(bus->regulator);
>>> +	if (bus->opp_table)
>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>
>> Have to disable regulator after disabling the clock
>> to prevent the h/w fault.
>>
>> I think that you should call them with following sequence:
>>
>> 	clk_disable_unprepare(bus->clk);
>> 	if (bus->opp_table)
>> 		dev_pm_opp_put_regulators(bus->opp_table);
>> 	dev_pm_opp_of_remove_table(dev);
>>
>>>  
>>>  	dev_pm_opp_of_remove_table(dev);
>>> +
>>>  	clk_disable_unprepare(bus->clk);
>>>  }
>>>  
>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>  {
>>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>>  	struct dev_pm_opp *new_opp;
>>> -	unsigned long old_freq, new_freq;
>>> -	int ret = 0;
>>> +	int ret;
>>>  
>>> -	/* Get new opp-bus instance according to new bus clock */
>>> +	/*
>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>> +	 * *freq to correct value.
>>> +	 */
>>
>> You better to change this comment with following styles
>> to keep the consistency:
>>
>> 	/* Get correct frequency for bus ... */
>>
>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>  	if (IS_ERR(new_opp)) {
>>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>>  		return PTR_ERR(new_opp);
>>>  	}
>>>  
>>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>>>  	dev_pm_opp_put(new_opp);
>>>  
>>> -	old_freq = bus->curr_freq;
>>> -
>>> -	if (old_freq == new_freq)
>>> -		return 0;
>>> -
>>>  	/* Change the frequency according to new OPP level */
>>>  	mutex_lock(&bus->lock);
>>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>>> +	if (!ret)
>>> +		bus->curr_freq = *freq;
>>
>> ditto. Have to print the error log, check above comment.
>>
>>>  
>>> -	ret = clk_set_rate(bus->clk, new_freq);
>>> -	if (ret < 0) {
>>> -		dev_err(dev, "failed to set the clock of bus\n");
>>> -		goto out;
>>> -	}
>>> -
>>> -	*freq = new_freq;
>>> -	bus->curr_freq = new_freq;
>>> -
>>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>>> -out:
>>>  	mutex_unlock(&bus->lock);
>>>  
>>>  	return ret;
>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>  					struct exynos_bus *bus)
>>>  {
>>>  	struct device *dev = bus->dev;
>>> -	int i, ret, count, size;
>>> -
>>> -	/* Get the regulator to provide each bus with the power */
>>> -	bus->regulator = devm_regulator_get(dev, "vdd");
>>> -	if (IS_ERR(bus->regulator)) {
>>> -		dev_err(dev, "failed to get VDD regulator\n");
>>> -		return PTR_ERR(bus->regulator);
>>> -	}
>>> -
>>> -	ret = regulator_enable(bus->regulator);
>>> -	if (ret < 0) {
>>> -		dev_err(dev, "failed to enable VDD regulator\n");
>>> -		return ret;
>>> -	}
>>> +	int i, count, size;
>>>  
>>>  	/*
>>>  	 * Get the devfreq-event devices to get the current utilization of
>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>  	count = devfreq_event_get_edev_count(dev);
>>>  	if (count < 0) {
>>>  		dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>> -		ret = count;
>>> -		goto err_regulator;
>>> +		return count;
>>>  	}
>>> +
>>>  	bus->edev_count = count;
>>>  
>>>  	size = sizeof(*bus->edev) * count;
>>>  	bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>> -	if (!bus->edev) {
>>> -		ret = -ENOMEM;
>>> -		goto err_regulator;
>>> -	}
>>> +	if (!bus->edev)
>>> +		return -ENOMEM;
>>>  
>>>  	for (i = 0; i < count; i++) {
>>>  		bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>> -		if (IS_ERR(bus->edev[i])) {
>>> -			ret = -EPROBE_DEFER;
>>> -			goto err_regulator;
>>> -		}
>>> +		if (IS_ERR(bus->edev[i]))
>>> +			return -EPROBE_DEFER;
>>>  	}
>>>  
>>>  	/*
>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>  	if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>  		bus->ratio = DEFAULT_SATURATION_RATIO;
>>>  
>>> -	if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>> -					&bus->voltage_tolerance))
>>> -		bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>> -
>>>  	return 0;
>>> -
>>> -err_regulator:
>>> -	regulator_disable(bus->regulator);
>>> -
>>> -	return ret;
>>>  }
>>>  
>>>  static int exynos_bus_parse_of(struct device_node *np,
>>> -			      struct exynos_bus *bus)
>>> +			      struct exynos_bus *bus, bool passive)
>>>  {
>>>  	struct device *dev = bus->dev;
>>> +	struct opp_table *opp_table;
>>> +	const char *vdd = "vdd";
>>>  	struct dev_pm_opp *opp;
>>>  	unsigned long rate;
>>>  	int ret;
>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>  		return ret;
>>>  	}
>>>  
>>> +	if (!passive) {
>>> +		opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>> +		if (IS_ERR(opp_table)) {
>>> +			ret = PTR_ERR(opp_table);
>>> +			dev_err(dev, "failed to set regulators %d\n", ret);
>>> +			goto err_clk;/
>>> +		}
>>> +
>>> +		bus->opp_table = opp_table;
>>> +	}
>>
>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>> because the regulator is only used by parent devfreq device.
> 
> exynos_bus_parse_of() is called for all devfreq devices (including
> parent) and (as you've noticed) the regulator should be enabled before
> enabling clock (which is done in exynos_bus_parse_of()) so adding
> extra argument to exynos_bus_parse_of() (like it is done currently in
> the patch) 

I think that this patch has still the problem about call sequence
between clock and regulator as following:

273         ret = clk_prepare_enable(bus->clk);                                     
274         if (ret < 0) {                                                          
275                 dev_err(dev, "failed to get enable clock\n");                   
276                 return ret;                                                     
277         }                                                                       
278                                                                                 
279         if (!passive) {                                                         
280                 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);            
281                 if (IS_ERR(opp_table)) {                                        
282                         ret = PTR_ERR(opp_table);                               
283                         dev_err(dev, "failed to set regulators %d\n", ret);     
284                         goto err_clk;                                           
285                 }                                                               
286                                                                                 
287                 bus->opp_table = opp_table;                                     
288         }                   

makes it possible to do the setup correctly without the need
> of merging both functions into one huge function (which would be more
> difficult to follow than two simpler functions IMHO). Is that approach
> acceptable or do you prefer one big function?

Actually, I don't force to make one function for both
exynos_bus_parse_of() and exynos_bus_parent_parse_of().

If we just keep this code, dev_pm_opp_set_regulators()
should be handled in exynos_bus_parent_parse_of()
because only parent devfreq device controls the regulator.

In order to keep the two functions, maybe have to change
the call the sequence between exynos_bus_parse_of() and
exynos_bus_parent_parse_of().

Once again, I don't force any fixed method. I want to fix them
with correct way.

> 
>>> +
>>>  	/* Get the freq and voltage from OPP table to scale the bus freq */
>>>  	ret = dev_pm_opp_of_add_table(dev);
>>>  	if (ret < 0) {
>>>  		dev_err(dev, "failed to get OPP table\n");
>>> -		goto err_clk;
>>> +		goto err_regulator;
>>>  	}
>>>  
>>>  	rate = clk_get_rate(bus->clk);
>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>  		ret = PTR_ERR(opp);
>>>  		goto err_opp;
>>>  	}
>>> +
>>>  	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>  	dev_pm_opp_put(opp);
>>>  
>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>  
>>>  err_opp:
>>>  	dev_pm_opp_of_remove_table(dev);
>>> +
>>> +err_regulator:
>>> +	if (bus->opp_table) {
>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>> +		bus->opp_table = NULL;
>>> +	}
>>
>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>
>>> +
>>>  err_clk:
>>>  	clk_disable_unprepare(bus->clk);
>>>  
>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>  	struct exynos_bus *bus;
>>>  	int ret, max_state;
>>>  	unsigned long min_freq, max_freq;
>>> +	bool passive = false;
>>>  
>>>  	if (!np) {
>>>  		dev_err(dev, "failed to find devicetree node\n");
>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>  	if (!bus)
>>>  		return -ENOMEM;
>>> +
>>>  	mutex_init(&bus->lock);
>>>  	bus->dev = &pdev->dev;
>>>  	platform_set_drvdata(pdev, bus);
>>> +	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>> +	if (node) {
>>> +		of_node_put(node);
>>> +		passive = true;
>>> +	}
>>>  
>>>  	/* Parse the device-tree to get the resource information */
>>> -	ret = exynos_bus_parse_of(np, bus);
>>> +	ret = exynos_bus_parse_of(np, bus, passive);
>>>  	if (ret < 0)
>>>  		return ret;
>>>  
>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>  		goto err;
>>>  	}
>>>  
>>> -	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>> -	if (node) {
>>> -		of_node_put(node);
>>> +	if (passive)
>>>  		goto passive;
>>> -	} else {
>>> -		ret = exynos_bus_parent_parse_of(np, bus);
>>> -	}
>>> +
>>> +	ret = exynos_bus_parent_parse_of(np, bus);
>>>  
>>
>> Remove unneeded blank line.
>>
>>>  	if (ret < 0)
>>>  		goto err;
>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>  
>>>  err:
>>>  	dev_pm_opp_of_remove_table(dev);
>>> +	if (bus->opp_table) {
>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>> +		bus->opp_table = NULL;
>>> +	}
>>> +
>>
>> ditto.
>> Have to disable regulator after disabling the clock
>> to prevent the h/w fault.
>>
>> I think that you should call them with following sequence:
>>
>> 	clk_disable_unprepare(bus->clk);
>> 	if (bus->opp_table)
>> 		dev_pm_opp_put_regulators(bus->opp_table);
>> 	dev_pm_opp_of_remove_table(dev);
>>
>>>  	clk_disable_unprepare(bus->clk);
>>>  
>>>  	return ret;
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()

[Bartlomiej Zolnierkiewicz]

On 7/16/19 12:33 PM, Chanwoo Choi wrote:
> Hi Bartlomiej,
> 
> On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>>
>> Hi Chanwoo,
>>
>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>> Hi Kamil,
>>>
>>> Looks good to me. But, this patch has some issue.
>>> I added the detailed reviews.
>>>
>>> I recommend that you make the separate patches as following
>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>
>>> First patch,
>>> Need to consolidate the following two function into one function.
>>> because the original exynos-bus.c has the problem that the regulator
>>> of parent devfreq device have to be enabled before enabling the clock.
>>> This issue did not happen because bootloader enables the bus-related
>>> regulators before kernel booting.
>>> - exynos_bus_parse_of()
>>> - exynos_bus_parent_parse_of()
>>>> Second patch,
>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>
>>>
>>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>> effect this allow useage of coupled regulators feature (required
>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>> it).
>>>>
>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>> ---
>>>>  drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>>  1 file changed, 66 insertions(+), 106 deletions(-)
>>>>
>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>> index 486cc5b422f1..7fc4f76bd848 100644
>>>> --- a/drivers/devfreq/exynos-bus.c
>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>> @@ -25,7 +25,6 @@
>>>>  #include <linux/slab.h>
>>>>  
>>>>  #define DEFAULT_SATURATION_RATIO	40
>>>> -#define DEFAULT_VOLTAGE_TOLERANCE	2
>>>>  
>>>>  struct exynos_bus {
>>>>  	struct device *dev;
>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>  
>>>>  	unsigned long curr_freq;
>>>>  
>>>> -	struct regulator *regulator;
>>>> +	struct opp_table *opp_table;
>>>> +
>>>>  	struct clk *clk;
>>>> -	unsigned int voltage_tolerance;
>>>>  	unsigned int ratio;
>>>>  };
>>>>  
>>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>>  {
>>>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>  	struct dev_pm_opp *new_opp;
>>>> -	unsigned long old_freq, new_freq, new_volt, tol;
>>>>  	int ret = 0;
>>>> -
>>>> -	/* Get new opp-bus instance according to new bus clock */
>>>> +	/*
>>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>>> +	 * *freq to correct value.
>>>> +	 */
>>>
>>> You better to change this comment with following styles
>>> to keep the consistency:
>>>
>>> 	/* Get correct frequency for bus ... */
>>>
>>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>  	if (IS_ERR(new_opp)) {
>>>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>>>  		return PTR_ERR(new_opp);
>>>>  	}
>>>>  
>>>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>>>> -	new_volt = dev_pm_opp_get_voltage(new_opp);
>>>>  	dev_pm_opp_put(new_opp);
>>>>  
>>>> -	old_freq = bus->curr_freq;
>>>> -
>>>> -	if (old_freq == new_freq)
>>>> -		return 0;
>>>> -	tol = new_volt * bus->voltage_tolerance / 100;
>>>> -
>>>>  	/* Change voltage and frequency according to new OPP level */
>>>>  	mutex_lock(&bus->lock);
>>>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>>>> +	if (!ret)
>>>> +		bus->curr_freq = *freq;
>>>
>>> Have to print the error log if ret has minus error value.
>>
>> dev_pm_opp_set_rate() should print the error message on all
>> errors so wouldn't printing the error log also here be superfluous?
>>
>> [ Please also note that the other user of dev_pm_opp_set_rate()
>>   (cpufreq-dt cpufreq driver) doesn't do this. ]
> 
> OK. Thanks for the explanation. 
> 
>>
>>> Modify it as following:
>>>
>>> 	if (ret < 0) {
>>> 		dev_err(dev, "failed to set bus rate\n");
>>> 		goto err:
>>> 	}
>>> 	bus->curr_freq = *freq;
>>>
>>> err:
>>> 	mutex_unlock(&bus->lock);
>>> 	
>>> 	return ret;
>>>
>>>>  
>>>> -	if (old_freq < new_freq) {
>>>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>> -		if (ret < 0) {
>>>> -			dev_err(bus->dev, "failed to set voltage\n");
>>>> -			goto out;
>>>> -		}
>>>> -	}
>>>> -
>>>> -	ret = clk_set_rate(bus->clk, new_freq);
>>>> -	if (ret < 0) {
>>>> -		dev_err(dev, "failed to change clock of bus\n");
>>>> -		clk_set_rate(bus->clk, old_freq);
>>>> -		goto out;
>>>> -	}
>>>> -
>>>> -	if (old_freq > new_freq) {
>>>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>> -		if (ret < 0) {
>>>> -			dev_err(bus->dev, "failed to set voltage\n");
>>>> -			goto out;
>>>> -		}
>>>> -	}
>>>> -	bus->curr_freq = new_freq;
>>>> -
>>>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>>>> -out:
>>>>  	mutex_unlock(&bus->lock);
>>>>  
>>>>  	return ret;
>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>>  	if (ret < 0)
>>>>  		dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>  
>>>> -	if (bus->regulator)
>>>> -		regulator_disable(bus->regulator);
>>>> +	if (bus->opp_table)
>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>
>>> Have to disable regulator after disabling the clock
>>> to prevent the h/w fault.
>>>
>>> I think that you should call them with following sequence:
>>>
>>> 	clk_disable_unprepare(bus->clk);
>>> 	if (bus->opp_table)
>>> 		dev_pm_opp_put_regulators(bus->opp_table);
>>> 	dev_pm_opp_of_remove_table(dev);
>>>
>>>>  
>>>>  	dev_pm_opp_of_remove_table(dev);
>>>> +
>>>>  	clk_disable_unprepare(bus->clk);
>>>>  }
>>>>  
>>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>>  {
>>>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>  	struct dev_pm_opp *new_opp;
>>>> -	unsigned long old_freq, new_freq;
>>>> -	int ret = 0;
>>>> +	int ret;
>>>>  
>>>> -	/* Get new opp-bus instance according to new bus clock */
>>>> +	/*
>>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>>> +	 * *freq to correct value.
>>>> +	 */
>>>
>>> You better to change this comment with following styles
>>> to keep the consistency:
>>>
>>> 	/* Get correct frequency for bus ... */
>>>
>>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>  	if (IS_ERR(new_opp)) {
>>>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>>>  		return PTR_ERR(new_opp);
>>>>  	}
>>>>  
>>>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>>>>  	dev_pm_opp_put(new_opp);
>>>>  
>>>> -	old_freq = bus->curr_freq;
>>>> -
>>>> -	if (old_freq == new_freq)
>>>> -		return 0;
>>>> -
>>>>  	/* Change the frequency according to new OPP level */
>>>>  	mutex_lock(&bus->lock);
>>>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>>>> +	if (!ret)
>>>> +		bus->curr_freq = *freq;
>>>
>>> ditto. Have to print the error log, check above comment.
>>>
>>>>  
>>>> -	ret = clk_set_rate(bus->clk, new_freq);
>>>> -	if (ret < 0) {
>>>> -		dev_err(dev, "failed to set the clock of bus\n");
>>>> -		goto out;
>>>> -	}
>>>> -
>>>> -	*freq = new_freq;
>>>> -	bus->curr_freq = new_freq;
>>>> -
>>>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>>>> -out:
>>>>  	mutex_unlock(&bus->lock);
>>>>  
>>>>  	return ret;
>>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>  					struct exynos_bus *bus)
>>>>  {
>>>>  	struct device *dev = bus->dev;
>>>> -	int i, ret, count, size;
>>>> -
>>>> -	/* Get the regulator to provide each bus with the power */
>>>> -	bus->regulator = devm_regulator_get(dev, "vdd");
>>>> -	if (IS_ERR(bus->regulator)) {
>>>> -		dev_err(dev, "failed to get VDD regulator\n");
>>>> -		return PTR_ERR(bus->regulator);
>>>> -	}
>>>> -
>>>> -	ret = regulator_enable(bus->regulator);
>>>> -	if (ret < 0) {
>>>> -		dev_err(dev, "failed to enable VDD regulator\n");
>>>> -		return ret;
>>>> -	}
>>>> +	int i, count, size;
>>>>  
>>>>  	/*
>>>>  	 * Get the devfreq-event devices to get the current utilization of
>>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>  	count = devfreq_event_get_edev_count(dev);
>>>>  	if (count < 0) {
>>>>  		dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>> -		ret = count;
>>>> -		goto err_regulator;
>>>> +		return count;
>>>>  	}
>>>> +
>>>>  	bus->edev_count = count;
>>>>  
>>>>  	size = sizeof(*bus->edev) * count;
>>>>  	bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>> -	if (!bus->edev) {
>>>> -		ret = -ENOMEM;
>>>> -		goto err_regulator;
>>>> -	}
>>>> +	if (!bus->edev)
>>>> +		return -ENOMEM;
>>>>  
>>>>  	for (i = 0; i < count; i++) {
>>>>  		bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>> -		if (IS_ERR(bus->edev[i])) {
>>>> -			ret = -EPROBE_DEFER;
>>>> -			goto err_regulator;
>>>> -		}
>>>> +		if (IS_ERR(bus->edev[i]))
>>>> +			return -EPROBE_DEFER;
>>>>  	}
>>>>  
>>>>  	/*
>>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>  	if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>>  		bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>  
>>>> -	if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>> -					&bus->voltage_tolerance))
>>>> -		bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>> -
>>>>  	return 0;
>>>> -
>>>> -err_regulator:
>>>> -	regulator_disable(bus->regulator);
>>>> -
>>>> -	return ret;
>>>>  }
>>>>  
>>>>  static int exynos_bus_parse_of(struct device_node *np,
>>>> -			      struct exynos_bus *bus)
>>>> +			      struct exynos_bus *bus, bool passive)
>>>>  {
>>>>  	struct device *dev = bus->dev;
>>>> +	struct opp_table *opp_table;
>>>> +	const char *vdd = "vdd";
>>>>  	struct dev_pm_opp *opp;
>>>>  	unsigned long rate;
>>>>  	int ret;
>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>  		return ret;
>>>>  	}
>>>>  
>>>> +	if (!passive) {
>>>> +		opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>> +		if (IS_ERR(opp_table)) {
>>>> +			ret = PTR_ERR(opp_table);
>>>> +			dev_err(dev, "failed to set regulators %d\n", ret);
>>>> +			goto err_clk;/
>>>> +		}
>>>> +
>>>> +		bus->opp_table = opp_table;
>>>> +	}
>>>
>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>> because the regulator is only used by parent devfreq device.
>>
>> exynos_bus_parse_of() is called for all devfreq devices (including
>> parent) and (as you've noticed) the regulator should be enabled before
>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>> extra argument to exynos_bus_parse_of() (like it is done currently in
>> the patch) 
> 
> I think that this patch has still the problem about call sequence
> between clock and regulator as following:

Yes, this should be fixed (though the wrong sequence between regulator
and clock handling is not introduced by the patchset itself and is present
in the original driver code).

> 273         ret = clk_prepare_enable(bus->clk);                                     
> 274         if (ret < 0) {                                                          
> 275                 dev_err(dev, "failed to get enable clock\n");                   
> 276                 return ret;                                                     
> 277         }                                                                       
> 278                                                                                 
> 279         if (!passive) {                                                         
> 280                 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);            
> 281                 if (IS_ERR(opp_table)) {                                        
> 282                         ret = PTR_ERR(opp_table);                               
> 283                         dev_err(dev, "failed to set regulators %d\n", ret);     
> 284                         goto err_clk;                                           
> 285                 }                                                               
> 286                                                                                 
> 287                 bus->opp_table = opp_table;                                     
> 288         }                   
> 
> makes it possible to do the setup correctly without the need
>> of merging both functions into one huge function (which would be more
>> difficult to follow than two simpler functions IMHO). Is that approach
>> acceptable or do you prefer one big function?
> 
> Actually, I don't force to make one function for both
> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
> 
> If we just keep this code, dev_pm_opp_set_regulators()
> should be handled in exynos_bus_parent_parse_of()
> because only parent devfreq device controls the regulator.

Could your please explain rationale for this requirement (besides
function name)?

The patch adds 'bool passive' argument (which is set to false for
parent devfreq device and true for child devfreq device) to
exynos_bus_parse_of() (which is called for *all* devfreq devices
and is called before exynos_bus_parent_parse_of()) and there is
no hard requirement to call dev_pm_opp_set_regulators() in
exynos_bus_parent_parse_of() so after only changing the ordering
between regulator and clock handling the setup code should be
correct.

[ Please note that this patch moves parent/child detection before
  exynos_bus_parse_of() call. ]

> In order to keep the two functions, maybe have to change
> the call the sequence between exynos_bus_parse_of() and
> exynos_bus_parent_parse_of().

Doesn't seem to be needed, care to explain it more?

> Once again, I don't force any fixed method. I want to fix them
> with correct way.
> 
>>
>>>> +
>>>>  	/* Get the freq and voltage from OPP table to scale the bus freq */
>>>>  	ret = dev_pm_opp_of_add_table(dev);
>>>>  	if (ret < 0) {
>>>>  		dev_err(dev, "failed to get OPP table\n");
>>>> -		goto err_clk;
>>>> +		goto err_regulator;
>>>>  	}
>>>>  
>>>>  	rate = clk_get_rate(bus->clk);
>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>  		ret = PTR_ERR(opp);
>>>>  		goto err_opp;
>>>>  	}
>>>> +
>>>>  	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>  	dev_pm_opp_put(opp);
>>>>  
>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>  
>>>>  err_opp:
>>>>  	dev_pm_opp_of_remove_table(dev);
>>>> +
>>>> +err_regulator:
>>>> +	if (bus->opp_table) {
>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>> +		bus->opp_table = NULL;
>>>> +	}
>>>
>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>
>>>> +
>>>>  err_clk:
>>>>  	clk_disable_unprepare(bus->clk);
>>>>  
>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>  	struct exynos_bus *bus;
>>>>  	int ret, max_state;
>>>>  	unsigned long min_freq, max_freq;
>>>> +	bool passive = false;
>>>>  
>>>>  	if (!np) {
>>>>  		dev_err(dev, "failed to find devicetree node\n");
>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>>  	if (!bus)
>>>>  		return -ENOMEM;
>>>> +
>>>>  	mutex_init(&bus->lock);
>>>>  	bus->dev = &pdev->dev;
>>>>  	platform_set_drvdata(pdev, bus);
>>>> +	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>> +	if (node) {
>>>> +		of_node_put(node);
>>>> +		passive = true;
>>>> +	}
>>>>  
>>>>  	/* Parse the device-tree to get the resource information */
>>>> -	ret = exynos_bus_parse_of(np, bus);
>>>> +	ret = exynos_bus_parse_of(np, bus, passive);
>>>>  	if (ret < 0)
>>>>  		return ret;
>>>>  
>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>  		goto err;
>>>>  	}
>>>>  
>>>> -	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>> -	if (node) {
>>>> -		of_node_put(node);
>>>> +	if (passive)
>>>>  		goto passive;
>>>> -	} else {
>>>> -		ret = exynos_bus_parent_parse_of(np, bus);
>>>> -	}
>>>> +
>>>> +	ret = exynos_bus_parent_parse_of(np, bus);
>>>>  
>>>
>>> Remove unneeded blank line.
>>>
>>>>  	if (ret < 0)
>>>>  		goto err;
>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>  
>>>>  err:
>>>>  	dev_pm_opp_of_remove_table(dev);
>>>> +	if (bus->opp_table) {
>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>> +		bus->opp_table = NULL;
>>>> +	}
>>>> +
>>>
>>> ditto.
>>> Have to disable regulator after disabling the clock
>>> to prevent the h/w fault.
>>>
>>> I think that you should call them with following sequence:
>>>
>>> 	clk_disable_unprepare(bus->clk);
>>> 	if (bus->opp_table)
>>> 		dev_pm_opp_put_regulators(bus->opp_table);
>>> 	dev_pm_opp_of_remove_table(dev);
>>>
>>>>  	clk_disable_unprepare(bus->clk);
>>>>  
>>>>  	return ret;
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()

[Chanwoo Choi]

Hi,

On 19. 7. 16. 오후 7:59, Bartlomiej Zolnierkiewicz wrote:
> 
> On 7/16/19 12:33 PM, Chanwoo Choi wrote:
>> Hi Bartlomiej,
>>
>> On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>>>
>>> Hi Chanwoo,
>>>
>>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>>> Hi Kamil,
>>>>
>>>> Looks good to me. But, this patch has some issue.
>>>> I added the detailed reviews.
>>>>
>>>> I recommend that you make the separate patches as following
>>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>>
>>>> First patch,
>>>> Need to consolidate the following two function into one function.
>>>> because the original exynos-bus.c has the problem that the regulator
>>>> of parent devfreq device have to be enabled before enabling the clock.
>>>> This issue did not happen because bootloader enables the bus-related
>>>> regulators before kernel booting.
>>>> - exynos_bus_parse_of()
>>>> - exynos_bus_parent_parse_of()
>>>>> Second patch,
>>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>>
>>>>
>>>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>>> effect this allow useage of coupled regulators feature (required
>>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>>> it).
>>>>>
>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>>> ---
>>>>>  drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>>>  1 file changed, 66 insertions(+), 106 deletions(-)
>>>>>
>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>> index 486cc5b422f1..7fc4f76bd848 100644
>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>> @@ -25,7 +25,6 @@
>>>>>  #include <linux/slab.h>
>>>>>  
>>>>>  #define DEFAULT_SATURATION_RATIO	40
>>>>> -#define DEFAULT_VOLTAGE_TOLERANCE	2
>>>>>  
>>>>>  struct exynos_bus {
>>>>>  	struct device *dev;
>>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>>  
>>>>>  	unsigned long curr_freq;
>>>>>  
>>>>> -	struct regulator *regulator;
>>>>> +	struct opp_table *opp_table;
>>>>> +
>>>>>  	struct clk *clk;
>>>>> -	unsigned int voltage_tolerance;
>>>>>  	unsigned int ratio;
>>>>>  };
>>>>>  
>>>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>>>  {
>>>>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>>  	struct dev_pm_opp *new_opp;
>>>>> -	unsigned long old_freq, new_freq, new_volt, tol;
>>>>>  	int ret = 0;
>>>>> -
>>>>> -	/* Get new opp-bus instance according to new bus clock */
>>>>> +	/*
>>>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>>>> +	 * *freq to correct value.
>>>>> +	 */
>>>>
>>>> You better to change this comment with following styles
>>>> to keep the consistency:
>>>>
>>>> 	/* Get correct frequency for bus ... */
>>>>
>>>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>>  	if (IS_ERR(new_opp)) {
>>>>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>>>>  		return PTR_ERR(new_opp);
>>>>>  	}
>>>>>  
>>>>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>>>>> -	new_volt = dev_pm_opp_get_voltage(new_opp);
>>>>>  	dev_pm_opp_put(new_opp);
>>>>>  
>>>>> -	old_freq = bus->curr_freq;
>>>>> -
>>>>> -	if (old_freq == new_freq)
>>>>> -		return 0;
>>>>> -	tol = new_volt * bus->voltage_tolerance / 100;
>>>>> -
>>>>>  	/* Change voltage and frequency according to new OPP level */
>>>>>  	mutex_lock(&bus->lock);
>>>>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>>>>> +	if (!ret)
>>>>> +		bus->curr_freq = *freq;
>>>>
>>>> Have to print the error log if ret has minus error value.
>>>
>>> dev_pm_opp_set_rate() should print the error message on all
>>> errors so wouldn't printing the error log also here be superfluous?
>>>
>>> [ Please also note that the other user of dev_pm_opp_set_rate()
>>>   (cpufreq-dt cpufreq driver) doesn't do this. ]
>>
>> OK. Thanks for the explanation. 
>>
>>>
>>>> Modify it as following:
>>>>
>>>> 	if (ret < 0) {
>>>> 		dev_err(dev, "failed to set bus rate\n");
>>>> 		goto err:
>>>> 	}
>>>> 	bus->curr_freq = *freq;
>>>>
>>>> err:
>>>> 	mutex_unlock(&bus->lock);
>>>> 	
>>>> 	return ret;
>>>>
>>>>>  
>>>>> -	if (old_freq < new_freq) {
>>>>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>> -		if (ret < 0) {
>>>>> -			dev_err(bus->dev, "failed to set voltage\n");
>>>>> -			goto out;
>>>>> -		}
>>>>> -	}
>>>>> -
>>>>> -	ret = clk_set_rate(bus->clk, new_freq);
>>>>> -	if (ret < 0) {
>>>>> -		dev_err(dev, "failed to change clock of bus\n");
>>>>> -		clk_set_rate(bus->clk, old_freq);
>>>>> -		goto out;
>>>>> -	}
>>>>> -
>>>>> -	if (old_freq > new_freq) {
>>>>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>> -		if (ret < 0) {
>>>>> -			dev_err(bus->dev, "failed to set voltage\n");
>>>>> -			goto out;
>>>>> -		}
>>>>> -	}
>>>>> -	bus->curr_freq = new_freq;
>>>>> -
>>>>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>>>>> -out:
>>>>>  	mutex_unlock(&bus->lock);
>>>>>  
>>>>>  	return ret;
>>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>>>  	if (ret < 0)
>>>>>  		dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>>  
>>>>> -	if (bus->regulator)
>>>>> -		regulator_disable(bus->regulator);
>>>>> +	if (bus->opp_table)
>>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>>
>>>> Have to disable regulator after disabling the clock
>>>> to prevent the h/w fault.
>>>>
>>>> I think that you should call them with following sequence:
>>>>
>>>> 	clk_disable_unprepare(bus->clk);
>>>> 	if (bus->opp_table)
>>>> 		dev_pm_opp_put_regulators(bus->opp_table);
>>>> 	dev_pm_opp_of_remove_table(dev);
>>>>
>>>>>  
>>>>>  	dev_pm_opp_of_remove_table(dev);
>>>>> +
>>>>>  	clk_disable_unprepare(bus->clk);
>>>>>  }
>>>>>  
>>>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>>>  {
>>>>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>>  	struct dev_pm_opp *new_opp;
>>>>> -	unsigned long old_freq, new_freq;
>>>>> -	int ret = 0;
>>>>> +	int ret;
>>>>>  
>>>>> -	/* Get new opp-bus instance according to new bus clock */
>>>>> +	/*
>>>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>>>> +	 * *freq to correct value.
>>>>> +	 */
>>>>
>>>> You better to change this comment with following styles
>>>> to keep the consistency:
>>>>
>>>> 	/* Get correct frequency for bus ... */
>>>>
>>>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>>  	if (IS_ERR(new_opp)) {
>>>>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>>>>  		return PTR_ERR(new_opp);
>>>>>  	}
>>>>>  
>>>>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>>>>>  	dev_pm_opp_put(new_opp);
>>>>>  
>>>>> -	old_freq = bus->curr_freq;
>>>>> -
>>>>> -	if (old_freq == new_freq)
>>>>> -		return 0;
>>>>> -
>>>>>  	/* Change the frequency according to new OPP level */
>>>>>  	mutex_lock(&bus->lock);
>>>>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>>>>> +	if (!ret)
>>>>> +		bus->curr_freq = *freq;
>>>>
>>>> ditto. Have to print the error log, check above comment.
>>>>
>>>>>  
>>>>> -	ret = clk_set_rate(bus->clk, new_freq);
>>>>> -	if (ret < 0) {
>>>>> -		dev_err(dev, "failed to set the clock of bus\n");
>>>>> -		goto out;
>>>>> -	}
>>>>> -
>>>>> -	*freq = new_freq;
>>>>> -	bus->curr_freq = new_freq;
>>>>> -
>>>>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>>>>> -out:
>>>>>  	mutex_unlock(&bus->lock);
>>>>>  
>>>>>  	return ret;
>>>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>  					struct exynos_bus *bus)
>>>>>  {
>>>>>  	struct device *dev = bus->dev;
>>>>> -	int i, ret, count, size;
>>>>> -
>>>>> -	/* Get the regulator to provide each bus with the power */
>>>>> -	bus->regulator = devm_regulator_get(dev, "vdd");
>>>>> -	if (IS_ERR(bus->regulator)) {
>>>>> -		dev_err(dev, "failed to get VDD regulator\n");
>>>>> -		return PTR_ERR(bus->regulator);
>>>>> -	}
>>>>> -
>>>>> -	ret = regulator_enable(bus->regulator);
>>>>> -	if (ret < 0) {
>>>>> -		dev_err(dev, "failed to enable VDD regulator\n");
>>>>> -		return ret;
>>>>> -	}
>>>>> +	int i, count, size;
>>>>>  
>>>>>  	/*
>>>>>  	 * Get the devfreq-event devices to get the current utilization of
>>>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>  	count = devfreq_event_get_edev_count(dev);
>>>>>  	if (count < 0) {
>>>>>  		dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>>> -		ret = count;
>>>>> -		goto err_regulator;
>>>>> +		return count;
>>>>>  	}
>>>>> +
>>>>>  	bus->edev_count = count;
>>>>>  
>>>>>  	size = sizeof(*bus->edev) * count;
>>>>>  	bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>>> -	if (!bus->edev) {
>>>>> -		ret = -ENOMEM;
>>>>> -		goto err_regulator;
>>>>> -	}
>>>>> +	if (!bus->edev)
>>>>> +		return -ENOMEM;
>>>>>  
>>>>>  	for (i = 0; i < count; i++) {
>>>>>  		bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>>> -		if (IS_ERR(bus->edev[i])) {
>>>>> -			ret = -EPROBE_DEFER;
>>>>> -			goto err_regulator;
>>>>> -		}
>>>>> +		if (IS_ERR(bus->edev[i]))
>>>>> +			return -EPROBE_DEFER;
>>>>>  	}
>>>>>  
>>>>>  	/*
>>>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>  	if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>>>  		bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>>  
>>>>> -	if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>>> -					&bus->voltage_tolerance))
>>>>> -		bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>>> -
>>>>>  	return 0;
>>>>> -
>>>>> -err_regulator:
>>>>> -	regulator_disable(bus->regulator);
>>>>> -
>>>>> -	return ret;
>>>>>  }
>>>>>  
>>>>>  static int exynos_bus_parse_of(struct device_node *np,
>>>>> -			      struct exynos_bus *bus)
>>>>> +			      struct exynos_bus *bus, bool passive)
>>>>>  {
>>>>>  	struct device *dev = bus->dev;
>>>>> +	struct opp_table *opp_table;
>>>>> +	const char *vdd = "vdd";
>>>>>  	struct dev_pm_opp *opp;
>>>>>  	unsigned long rate;
>>>>>  	int ret;
>>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>  		return ret;
>>>>>  	}
>>>>>  
>>>>> +	if (!passive) {
>>>>> +		opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>>> +		if (IS_ERR(opp_table)) {
>>>>> +			ret = PTR_ERR(opp_table);
>>>>> +			dev_err(dev, "failed to set regulators %d\n", ret);
>>>>> +			goto err_clk;/
>>>>> +		}
>>>>> +
>>>>> +		bus->opp_table = opp_table;
>>>>> +	}
>>>>
>>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>>> because the regulator is only used by parent devfreq device.
>>>
>>> exynos_bus_parse_of() is called for all devfreq devices (including
>>> parent) and (as you've noticed) the regulator should be enabled before
>>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>>> extra argument to exynos_bus_parse_of() (like it is done currently in
>>> the patch) 
>>
>> I think that this patch has still the problem about call sequence
>> between clock and regulator as following:
> 
> Yes, this should be fixed (though the wrong sequence between regulator
> and clock handling is not introduced by the patchset itself and is present
> in the original driver code).
> 
>> 273         ret = clk_prepare_enable(bus->clk);                                     
>> 274         if (ret < 0) {                                                          
>> 275                 dev_err(dev, "failed to get enable clock\n");                   
>> 276                 return ret;                                                     
>> 277         }                                                                       
>> 278                                                                                 
>> 279         if (!passive) {                                                         
>> 280                 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);            
>> 281                 if (IS_ERR(opp_table)) {                                        
>> 282                         ret = PTR_ERR(opp_table);                               
>> 283                         dev_err(dev, "failed to set regulators %d\n", ret);     
>> 284                         goto err_clk;                                           
>> 285                 }                                                               
>> 286                                                                                 
>> 287                 bus->opp_table = opp_table;                                     
>> 288         }                   
>>
>> makes it possible to do the setup correctly without the need
>>> of merging both functions into one huge function (which would be more
>>> difficult to follow than two simpler functions IMHO). Is that approach
>>> acceptable or do you prefer one big function?
>>
>> Actually, I don't force to make one function for both
>> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
>>
>> If we just keep this code, dev_pm_opp_set_regulators()
>> should be handled in exynos_bus_parent_parse_of()
>> because only parent devfreq device controls the regulator.
> 
> Could your please explain rationale for this requirement (besides
> function name)?

OK. I hope to satisfy the following requirements:

1. Fix the sequence problem between clock and regulator for enabling them.
2. dev_pm_opp_set_regulator() have to be handled in exynos_bus_parent_parse_of()
   instead of exynos_bus_parse_of() for only parent devfreq device.
3. exynos_bus_parse_of() have to handle the only common properties
   of both parent devfreq device and passive devfreq device.

> 
> The patch adds 'bool passive' argument (which is set to false for
> parent devfreq device and true for child devfreq device) to
> exynos_bus_parse_of() (which is called for *all* devfreq devices

As I menteiond, exynos_bus_parse_of have to handle the only common
properties of both parent device and passive device. 

I gathered the properties for parent device into exynos_bus_parent_parse_of()
This way using 'bool passive' argument is not proper in exynos_bus_parse_of().


> and is called before exynos_bus_parent_parse_of()) and there is
> no hard requirement to call dev_pm_opp_set_regulators() in
> exynos_bus_parent_parse_of() so after only changing the ordering
> between regulator and clock handling the setup code should be
> correct.
> 
> [ Please note that this patch moves parent/child detection before
>   exynos_bus_parse_of() call. ]
> 
>> In order to keep the two functions, maybe have to change
>> the call the sequence between exynos_bus_parse_of() and
>> exynos_bus_parent_parse_of().
> 
> Doesn't seem to be needed, care to explain it more?

In order to fix the sequence problem between clock and regulator
with dev_pm_opp_set_regualtor() and want to keep two functions
(exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
have to change the call order as following and then modify
the exception handling code when error happen.

	node = of_parse_phandle(dev->of_node, "devfreq", 0);                    
	if (node) {                                                             
		of_node_put(node);                                              
		passive = true
	}

	if (!passive)	
		exynos_bus_parent_parse_of()
			dev_pm_opp_set_regulator

	exynos_bus_parse_of()

> 
>> Once again, I don't force any fixed method. I want to fix them
>> with correct way.
>>
>>>
>>>>> +
>>>>>  	/* Get the freq and voltage from OPP table to scale the bus freq */
>>>>>  	ret = dev_pm_opp_of_add_table(dev);
>>>>>  	if (ret < 0) {
>>>>>  		dev_err(dev, "failed to get OPP table\n");
>>>>> -		goto err_clk;
>>>>> +		goto err_regulator;
>>>>>  	}
>>>>>  
>>>>>  	rate = clk_get_rate(bus->clk);
>>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>  		ret = PTR_ERR(opp);
>>>>>  		goto err_opp;
>>>>>  	}
>>>>> +
>>>>>  	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>>  	dev_pm_opp_put(opp);
>>>>>  
>>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>  
>>>>>  err_opp:
>>>>>  	dev_pm_opp_of_remove_table(dev);
>>>>> +
>>>>> +err_regulator:
>>>>> +	if (bus->opp_table) {
>>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>>> +		bus->opp_table = NULL;
>>>>> +	}
>>>>
>>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>>
>>>>> +
>>>>>  err_clk:
>>>>>  	clk_disable_unprepare(bus->clk);
>>>>>  
>>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>  	struct exynos_bus *bus;
>>>>>  	int ret, max_state;
>>>>>  	unsigned long min_freq, max_freq;
>>>>> +	bool passive = false;
>>>>>  
>>>>>  	if (!np) {
>>>>>  		dev_err(dev, "failed to find devicetree node\n");
>>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>>>  	if (!bus)
>>>>>  		return -ENOMEM;
>>>>> +
>>>>>  	mutex_init(&bus->lock);
>>>>>  	bus->dev = &pdev->dev;
>>>>>  	platform_set_drvdata(pdev, bus);
>>>>> +	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>> +	if (node) {
>>>>> +		of_node_put(node);
>>>>> +		passive = true;
>>>>> +	}
>>>>>  
>>>>>  	/* Parse the device-tree to get the resource information */
>>>>> -	ret = exynos_bus_parse_of(np, bus);
>>>>> +	ret = exynos_bus_parse_of(np, bus, passive);
>>>>>  	if (ret < 0)
>>>>>  		return ret;
>>>>>  
>>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>  		goto err;
>>>>>  	}
>>>>>  
>>>>> -	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>> -	if (node) {
>>>>> -		of_node_put(node);
>>>>> +	if (passive)
>>>>>  		goto passive;
>>>>> -	} else {
>>>>> -		ret = exynos_bus_parent_parse_of(np, bus);
>>>>> -	}
>>>>> +
>>>>> +	ret = exynos_bus_parent_parse_of(np, bus);
>>>>>  
>>>>
>>>> Remove unneeded blank line.
>>>>
>>>>>  	if (ret < 0)
>>>>>  		goto err;
>>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>  
>>>>>  err:
>>>>>  	dev_pm_opp_of_remove_table(dev);
>>>>> +	if (bus->opp_table) {
>>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>>> +		bus->opp_table = NULL;
>>>>> +	}
>>>>> +
>>>>
>>>> ditto.
>>>> Have to disable regulator after disabling the clock
>>>> to prevent the h/w fault.
>>>>
>>>> I think that you should call them with following sequence:
>>>>
>>>> 	clk_disable_unprepare(bus->clk);
>>>> 	if (bus->opp_table)
>>>> 		dev_pm_opp_put_regulators(bus->opp_table);
>>>> 	dev_pm_opp_of_remove_table(dev);
>>>>
>>>>>  	clk_disable_unprepare(bus->clk);
>>>>>  
>>>>>  	return ret;
>>>
>>> Best regards,
>>> --
>>> Bartlomiej Zolnierkiewicz
>>> Samsung R&D Institute Poland
>>> Samsung Electronics
> 
> Best regards,
> --
> Bartlomiej Zolnierkiewicz
> Samsung R&D Institute Poland
> Samsung Electronics
> 
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()

[Bartlomiej Zolnierkiewicz]

On 7/16/19 1:26 PM, Chanwoo Choi wrote:
> Hi,
> 
> On 19. 7. 16. 오후 7:59, Bartlomiej Zolnierkiewicz wrote:
>>
>> On 7/16/19 12:33 PM, Chanwoo Choi wrote:
>>> Hi Bartlomiej,
>>>
>>> On 19. 7. 16. 오후 7:13, Bartlomiej Zolnierkiewicz wrote:
>>>>
>>>> Hi Chanwoo,
>>>>
>>>> On 7/16/19 5:56 AM, Chanwoo Choi wrote:
>>>>> Hi Kamil,
>>>>>
>>>>> Looks good to me. But, this patch has some issue.
>>>>> I added the detailed reviews.
>>>>>
>>>>> I recommend that you make the separate patches as following
>>>>> in order to clarify the role of which apply the dev_pm_opp_* function.
>>>>>
>>>>> First patch,
>>>>> Need to consolidate the following two function into one function.
>>>>> because the original exynos-bus.c has the problem that the regulator
>>>>> of parent devfreq device have to be enabled before enabling the clock.
>>>>> This issue did not happen because bootloader enables the bus-related
>>>>> regulators before kernel booting.
>>>>> - exynos_bus_parse_of()
>>>>> - exynos_bus_parent_parse_of()
>>>>>> Second patch,
>>>>> Apply dev_pm_opp_set_regulators() and dev_pm_opp_set_rate()
>>>>>
>>>>>
>>>>> On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
>>>>>> Reuse opp core code for setting bus clock and voltage. As a side
>>>>>> effect this allow useage of coupled regulators feature (required
>>>>>> for boards using Exynos5422/5800 SoCs) because dev_pm_opp_set_rate()
>>>>>> uses regulator_set_voltage_triplet() for setting regulator voltage
>>>>>> while the old code used regulator_set_voltage_tol() with fixed
>>>>>> tolerance. This patch also removes no longer needed parsing of DT
>>>>>> property "exynos,voltage-tolerance" (no Exynos devfreq DT node uses
>>>>>> it).
>>>>>>
>>>>>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
>>>>>> ---
>>>>>>  drivers/devfreq/exynos-bus.c | 172 ++++++++++++++---------------------
>>>>>>  1 file changed, 66 insertions(+), 106 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.c
>>>>>> index 486cc5b422f1..7fc4f76bd848 100644
>>>>>> --- a/drivers/devfreq/exynos-bus.c
>>>>>> +++ b/drivers/devfreq/exynos-bus.c
>>>>>> @@ -25,7 +25,6 @@
>>>>>>  #include <linux/slab.h>
>>>>>>  
>>>>>>  #define DEFAULT_SATURATION_RATIO	40
>>>>>> -#define DEFAULT_VOLTAGE_TOLERANCE	2
>>>>>>  
>>>>>>  struct exynos_bus {
>>>>>>  	struct device *dev;
>>>>>> @@ -37,9 +36,9 @@ struct exynos_bus {
>>>>>>  
>>>>>>  	unsigned long curr_freq;
>>>>>>  
>>>>>> -	struct regulator *regulator;
>>>>>> +	struct opp_table *opp_table;
>>>>>> +
>>>>>>  	struct clk *clk;
>>>>>> -	unsigned int voltage_tolerance;
>>>>>>  	unsigned int ratio;
>>>>>>  };
>>>>>>  
>>>>>> @@ -99,56 +98,25 @@ static int exynos_bus_target(struct device *dev, unsigned long *freq, u32 flags)
>>>>>>  {
>>>>>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>>>  	struct dev_pm_opp *new_opp;
>>>>>> -	unsigned long old_freq, new_freq, new_volt, tol;
>>>>>>  	int ret = 0;
>>>>>> -
>>>>>> -	/* Get new opp-bus instance according to new bus clock */
>>>>>> +	/*
>>>>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>>>>> +	 * *freq to correct value.
>>>>>> +	 */
>>>>>
>>>>> You better to change this comment with following styles
>>>>> to keep the consistency:
>>>>>
>>>>> 	/* Get correct frequency for bus ... */
>>>>>
>>>>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>>>  	if (IS_ERR(new_opp)) {
>>>>>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>>>>>  		return PTR_ERR(new_opp);
>>>>>>  	}
>>>>>>  
>>>>>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>>>>>> -	new_volt = dev_pm_opp_get_voltage(new_opp);
>>>>>>  	dev_pm_opp_put(new_opp);
>>>>>>  
>>>>>> -	old_freq = bus->curr_freq;
>>>>>> -
>>>>>> -	if (old_freq == new_freq)
>>>>>> -		return 0;
>>>>>> -	tol = new_volt * bus->voltage_tolerance / 100;
>>>>>> -
>>>>>>  	/* Change voltage and frequency according to new OPP level */
>>>>>>  	mutex_lock(&bus->lock);
>>>>>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>>>>>> +	if (!ret)
>>>>>> +		bus->curr_freq = *freq;
>>>>>
>>>>> Have to print the error log if ret has minus error value.
>>>>
>>>> dev_pm_opp_set_rate() should print the error message on all
>>>> errors so wouldn't printing the error log also here be superfluous?
>>>>
>>>> [ Please also note that the other user of dev_pm_opp_set_rate()
>>>>   (cpufreq-dt cpufreq driver) doesn't do this. ]
>>>
>>> OK. Thanks for the explanation. 
>>>
>>>>
>>>>> Modify it as following:
>>>>>
>>>>> 	if (ret < 0) {
>>>>> 		dev_err(dev, "failed to set bus rate\n");
>>>>> 		goto err:
>>>>> 	}
>>>>> 	bus->curr_freq = *freq;
>>>>>
>>>>> err:
>>>>> 	mutex_unlock(&bus->lock);
>>>>> 	
>>>>> 	return ret;
>>>>>
>>>>>>  
>>>>>> -	if (old_freq < new_freq) {
>>>>>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>>> -		if (ret < 0) {
>>>>>> -			dev_err(bus->dev, "failed to set voltage\n");
>>>>>> -			goto out;
>>>>>> -		}
>>>>>> -	}
>>>>>> -
>>>>>> -	ret = clk_set_rate(bus->clk, new_freq);
>>>>>> -	if (ret < 0) {
>>>>>> -		dev_err(dev, "failed to change clock of bus\n");
>>>>>> -		clk_set_rate(bus->clk, old_freq);
>>>>>> -		goto out;
>>>>>> -	}
>>>>>> -
>>>>>> -	if (old_freq > new_freq) {
>>>>>> -		ret = regulator_set_voltage_tol(bus->regulator, new_volt, tol);
>>>>>> -		if (ret < 0) {
>>>>>> -			dev_err(bus->dev, "failed to set voltage\n");
>>>>>> -			goto out;
>>>>>> -		}
>>>>>> -	}
>>>>>> -	bus->curr_freq = new_freq;
>>>>>> -
>>>>>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>>>>>> -out:
>>>>>>  	mutex_unlock(&bus->lock);
>>>>>>  
>>>>>>  	return ret;
>>>>>> @@ -194,10 +162,11 @@ static void exynos_bus_exit(struct device *dev)
>>>>>>  	if (ret < 0)
>>>>>>  		dev_warn(dev, "failed to disable the devfreq-event devices\n");
>>>>>>  
>>>>>> -	if (bus->regulator)
>>>>>> -		regulator_disable(bus->regulator);
>>>>>> +	if (bus->opp_table)
>>>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>>>
>>>>> Have to disable regulator after disabling the clock
>>>>> to prevent the h/w fault.
>>>>>
>>>>> I think that you should call them with following sequence:
>>>>>
>>>>> 	clk_disable_unprepare(bus->clk);
>>>>> 	if (bus->opp_table)
>>>>> 		dev_pm_opp_put_regulators(bus->opp_table);
>>>>> 	dev_pm_opp_of_remove_table(dev);
>>>>>
>>>>>>  
>>>>>>  	dev_pm_opp_of_remove_table(dev);
>>>>>> +
>>>>>>  	clk_disable_unprepare(bus->clk);
>>>>>>  }
>>>>>>  
>>>>>> @@ -209,39 +178,26 @@ static int exynos_bus_passive_target(struct device *dev, unsigned long *freq,
>>>>>>  {
>>>>>>  	struct exynos_bus *bus = dev_get_drvdata(dev);
>>>>>>  	struct dev_pm_opp *new_opp;
>>>>>> -	unsigned long old_freq, new_freq;
>>>>>> -	int ret = 0;
>>>>>> +	int ret;
>>>>>>  
>>>>>> -	/* Get new opp-bus instance according to new bus clock */
>>>>>> +	/*
>>>>>> +	 * New frequency for bus may not be exactly matched to opp, adjust
>>>>>> +	 * *freq to correct value.
>>>>>> +	 */
>>>>>
>>>>> You better to change this comment with following styles
>>>>> to keep the consistency:
>>>>>
>>>>> 	/* Get correct frequency for bus ... */
>>>>>
>>>>>>  	new_opp = devfreq_recommended_opp(dev, freq, flags);
>>>>>>  	if (IS_ERR(new_opp)) {
>>>>>>  		dev_err(dev, "failed to get recommended opp instance\n");
>>>>>>  		return PTR_ERR(new_opp);
>>>>>>  	}
>>>>>>  
>>>>>> -	new_freq = dev_pm_opp_get_freq(new_opp);
>>>>>>  	dev_pm_opp_put(new_opp);
>>>>>>  
>>>>>> -	old_freq = bus->curr_freq;
>>>>>> -
>>>>>> -	if (old_freq == new_freq)
>>>>>> -		return 0;
>>>>>> -
>>>>>>  	/* Change the frequency according to new OPP level */
>>>>>>  	mutex_lock(&bus->lock);
>>>>>> +	ret = dev_pm_opp_set_rate(dev, *freq);
>>>>>> +	if (!ret)
>>>>>> +		bus->curr_freq = *freq;
>>>>>
>>>>> ditto. Have to print the error log, check above comment.
>>>>>
>>>>>>  
>>>>>> -	ret = clk_set_rate(bus->clk, new_freq);
>>>>>> -	if (ret < 0) {
>>>>>> -		dev_err(dev, "failed to set the clock of bus\n");
>>>>>> -		goto out;
>>>>>> -	}
>>>>>> -
>>>>>> -	*freq = new_freq;
>>>>>> -	bus->curr_freq = new_freq;
>>>>>> -
>>>>>> -	dev_dbg(dev, "Set the frequency of bus (%luHz -> %luHz, %luHz)\n",
>>>>>> -			old_freq, new_freq, clk_get_rate(bus->clk));
>>>>>> -out:
>>>>>>  	mutex_unlock(&bus->lock);
>>>>>>  
>>>>>>  	return ret;
>>>>>> @@ -259,20 +215,7 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>>  					struct exynos_bus *bus)
>>>>>>  {
>>>>>>  	struct device *dev = bus->dev;
>>>>>> -	int i, ret, count, size;
>>>>>> -
>>>>>> -	/* Get the regulator to provide each bus with the power */
>>>>>> -	bus->regulator = devm_regulator_get(dev, "vdd");
>>>>>> -	if (IS_ERR(bus->regulator)) {
>>>>>> -		dev_err(dev, "failed to get VDD regulator\n");
>>>>>> -		return PTR_ERR(bus->regulator);
>>>>>> -	}
>>>>>> -
>>>>>> -	ret = regulator_enable(bus->regulator);
>>>>>> -	if (ret < 0) {
>>>>>> -		dev_err(dev, "failed to enable VDD regulator\n");
>>>>>> -		return ret;
>>>>>> -	}
>>>>>> +	int i, count, size;
>>>>>>  
>>>>>>  	/*
>>>>>>  	 * Get the devfreq-event devices to get the current utilization of
>>>>>> @@ -281,24 +224,20 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>>  	count = devfreq_event_get_edev_count(dev);
>>>>>>  	if (count < 0) {
>>>>>>  		dev_err(dev, "failed to get the count of devfreq-event dev\n");
>>>>>> -		ret = count;
>>>>>> -		goto err_regulator;
>>>>>> +		return count;
>>>>>>  	}
>>>>>> +
>>>>>>  	bus->edev_count = count;
>>>>>>  
>>>>>>  	size = sizeof(*bus->edev) * count;
>>>>>>  	bus->edev = devm_kzalloc(dev, size, GFP_KERNEL);
>>>>>> -	if (!bus->edev) {
>>>>>> -		ret = -ENOMEM;
>>>>>> -		goto err_regulator;
>>>>>> -	}
>>>>>> +	if (!bus->edev)
>>>>>> +		return -ENOMEM;
>>>>>>  
>>>>>>  	for (i = 0; i < count; i++) {
>>>>>>  		bus->edev[i] = devfreq_event_get_edev_by_phandle(dev, i);
>>>>>> -		if (IS_ERR(bus->edev[i])) {
>>>>>> -			ret = -EPROBE_DEFER;
>>>>>> -			goto err_regulator;
>>>>>> -		}
>>>>>> +		if (IS_ERR(bus->edev[i]))
>>>>>> +			return -EPROBE_DEFER;
>>>>>>  	}
>>>>>>  
>>>>>>  	/*
>>>>>> @@ -314,22 +253,15 @@ static int exynos_bus_parent_parse_of(struct device_node *np,
>>>>>>  	if (of_property_read_u32(np, "exynos,saturation-ratio", &bus->ratio))
>>>>>>  		bus->ratio = DEFAULT_SATURATION_RATIO;
>>>>>>  
>>>>>> -	if (of_property_read_u32(np, "exynos,voltage-tolerance",
>>>>>> -					&bus->voltage_tolerance))
>>>>>> -		bus->voltage_tolerance = DEFAULT_VOLTAGE_TOLERANCE;
>>>>>> -
>>>>>>  	return 0;
>>>>>> -
>>>>>> -err_regulator:
>>>>>> -	regulator_disable(bus->regulator);
>>>>>> -
>>>>>> -	return ret;
>>>>>>  }
>>>>>>  
>>>>>>  static int exynos_bus_parse_of(struct device_node *np,
>>>>>> -			      struct exynos_bus *bus)
>>>>>> +			      struct exynos_bus *bus, bool passive)
>>>>>>  {
>>>>>>  	struct device *dev = bus->dev;
>>>>>> +	struct opp_table *opp_table;
>>>>>> +	const char *vdd = "vdd";
>>>>>>  	struct dev_pm_opp *opp;
>>>>>>  	unsigned long rate;
>>>>>>  	int ret;
>>>>>> @@ -347,11 +279,22 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>  		return ret;
>>>>>>  	}
>>>>>>  
>>>>>> +	if (!passive) {
>>>>>> +		opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);
>>>>>> +		if (IS_ERR(opp_table)) {
>>>>>> +			ret = PTR_ERR(opp_table);
>>>>>> +			dev_err(dev, "failed to set regulators %d\n", ret);
>>>>>> +			goto err_clk;/
>>>>>> +		}
>>>>>> +
>>>>>> +		bus->opp_table = opp_table;
>>>>>> +	}
>>>>>
>>>>> This driver has exynos_bus_parent_parse_of() function for parent devfreq device.
>>>>> dev_pm_opp_set_regulators() have to be called in exynos_bus_parent_parse_of()
>>>>> because the regulator is only used by parent devfreq device.
>>>>
>>>> exynos_bus_parse_of() is called for all devfreq devices (including
>>>> parent) and (as you've noticed) the regulator should be enabled before
>>>> enabling clock (which is done in exynos_bus_parse_of()) so adding
>>>> extra argument to exynos_bus_parse_of() (like it is done currently in
>>>> the patch) 
>>>
>>> I think that this patch has still the problem about call sequence
>>> between clock and regulator as following:
>>
>> Yes, this should be fixed (though the wrong sequence between regulator
>> and clock handling is not introduced by the patchset itself and is present
>> in the original driver code).
>>
>>> 273         ret = clk_prepare_enable(bus->clk);                                     
>>> 274         if (ret < 0) {                                                          
>>> 275                 dev_err(dev, "failed to get enable clock\n");                   
>>> 276                 return ret;                                                     
>>> 277         }                                                                       
>>> 278                                                                                 
>>> 279         if (!passive) {                                                         
>>> 280                 opp_table = dev_pm_opp_set_regulators(dev, &vdd, 1);            
>>> 281                 if (IS_ERR(opp_table)) {                                        
>>> 282                         ret = PTR_ERR(opp_table);                               
>>> 283                         dev_err(dev, "failed to set regulators %d\n", ret);     
>>> 284                         goto err_clk;                                           
>>> 285                 }                                                               
>>> 286                                                                                 
>>> 287                 bus->opp_table = opp_table;                                     
>>> 288         }                   
>>>
>>> makes it possible to do the setup correctly without the need
>>>> of merging both functions into one huge function (which would be more
>>>> difficult to follow than two simpler functions IMHO). Is that approach
>>>> acceptable or do you prefer one big function?
>>>
>>> Actually, I don't force to make one function for both
>>> exynos_bus_parse_of() and exynos_bus_parent_parse_of().
>>>
>>> If we just keep this code, dev_pm_opp_set_regulators()
>>> should be handled in exynos_bus_parent_parse_of()
>>> because only parent devfreq device controls the regulator.
>>
>> Could your please explain rationale for this requirement (besides
>> function name)?
> 
> OK. I hope to satisfy the following requirements:
> 
> 1. Fix the sequence problem between clock and regulator for enabling them.
> 2. dev_pm_opp_set_regulator() have to be handled in exynos_bus_parent_parse_of()
>    instead of exynos_bus_parse_of() for only parent devfreq device.
> 3. exynos_bus_parse_of() have to handle the only common properties
>    of both parent devfreq device and passive devfreq device.
> 
>>
>> The patch adds 'bool passive' argument (which is set to false for
>> parent devfreq device and true for child devfreq device) to
>> exynos_bus_parse_of() (which is called for *all* devfreq devices
> 
> As I menteiond, exynos_bus_parse_of have to handle the only common
> properties of both parent device and passive device. 
> 
> I gathered the properties for parent device into exynos_bus_parent_parse_of()
> This way using 'bool passive' argument is not proper in exynos_bus_parse_of().
> 
> 
>> and is called before exynos_bus_parent_parse_of()) and there is
>> no hard requirement to call dev_pm_opp_set_regulators() in
>> exynos_bus_parent_parse_of() so after only changing the ordering
>> between regulator and clock handling the setup code should be
>> correct.
>>
>> [ Please note that this patch moves parent/child detection before
>>   exynos_bus_parse_of() call. ]
>>
>>> In order to keep the two functions, maybe have to change
>>> the call the sequence between exynos_bus_parse_of() and
>>> exynos_bus_parent_parse_of().
>>
>> Doesn't seem to be needed, care to explain it more?
> 
> In order to fix the sequence problem between clock and regulator
> with dev_pm_opp_set_regualtor() and want to keep two functions
> (exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
> have to change the call order as following and then modify
> the exception handling code when error happen.
> 
> 	node = of_parse_phandle(dev->of_node, "devfreq", 0);                    
> 	if (node) {                                                             
> 		of_node_put(node);                                              
> 		passive = true
> 	}
> 
> 	if (!passive)	
> 		exynos_bus_parent_parse_of()
> 			dev_pm_opp_set_regulator
> 
> 	exynos_bus_parse_of()

OK. This seems like a solution.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

>>
>>> Once again, I don't force any fixed method. I want to fix them
>>> with correct way.
>>>
>>>>
>>>>>> +
>>>>>>  	/* Get the freq and voltage from OPP table to scale the bus freq */
>>>>>>  	ret = dev_pm_opp_of_add_table(dev);
>>>>>>  	if (ret < 0) {
>>>>>>  		dev_err(dev, "failed to get OPP table\n");
>>>>>> -		goto err_clk;
>>>>>> +		goto err_regulator;
>>>>>>  	}
>>>>>>  
>>>>>>  	rate = clk_get_rate(bus->clk);
>>>>>> @@ -362,6 +305,7 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>  		ret = PTR_ERR(opp);
>>>>>>  		goto err_opp;
>>>>>>  	}
>>>>>> +
>>>>>>  	bus->curr_freq = dev_pm_opp_get_freq(opp);
>>>>>>  	dev_pm_opp_put(opp);
>>>>>>  
>>>>>> @@ -369,6 +313,13 @@ static int exynos_bus_parse_of(struct device_node *np,
>>>>>>  
>>>>>>  err_opp:
>>>>>>  	dev_pm_opp_of_remove_table(dev);
>>>>>> +
>>>>>> +err_regulator:
>>>>>> +	if (bus->opp_table) {
>>>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>>>> +		bus->opp_table = NULL;
>>>>>> +	}
>>>>>
>>>>> As I mentioned above, it it wrong to call dev_pm_opp_put_regulators()
>>>>> after removing the opp_table by dev_pm_opp_of_remove_table().
>>>>>
>>>>>> +
>>>>>>  err_clk:
>>>>>>  	clk_disable_unprepare(bus->clk);
>>>>>>  
>>>>>> @@ -386,6 +337,7 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>  	struct exynos_bus *bus;
>>>>>>  	int ret, max_state;
>>>>>>  	unsigned long min_freq, max_freq;
>>>>>> +	bool passive = false;
>>>>>>  
>>>>>>  	if (!np) {
>>>>>>  		dev_err(dev, "failed to find devicetree node\n");
>>>>>> @@ -395,12 +347,18 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>>>>>>  	if (!bus)
>>>>>>  		return -ENOMEM;
>>>>>> +
>>>>>>  	mutex_init(&bus->lock);
>>>>>>  	bus->dev = &pdev->dev;
>>>>>>  	platform_set_drvdata(pdev, bus);
>>>>>> +	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>>> +	if (node) {
>>>>>> +		of_node_put(node);
>>>>>> +		passive = true;
>>>>>> +	}
>>>>>>  
>>>>>>  	/* Parse the device-tree to get the resource information */
>>>>>> -	ret = exynos_bus_parse_of(np, bus);
>>>>>> +	ret = exynos_bus_parse_of(np, bus, passive);
>>>>>>  	if (ret < 0)
>>>>>>  		return ret;
>>>>>>  
>>>>>> @@ -410,13 +368,10 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>  		goto err;
>>>>>>  	}
>>>>>>  
>>>>>> -	node = of_parse_phandle(dev->of_node, "devfreq", 0);
>>>>>> -	if (node) {
>>>>>> -		of_node_put(node);
>>>>>> +	if (passive)
>>>>>>  		goto passive;
>>>>>> -	} else {
>>>>>> -		ret = exynos_bus_parent_parse_of(np, bus);
>>>>>> -	}
>>>>>> +
>>>>>> +	ret = exynos_bus_parent_parse_of(np, bus);
>>>>>>  
>>>>>
>>>>> Remove unneeded blank line.
>>>>>
>>>>>>  	if (ret < 0)
>>>>>>  		goto err;
>>>>>> @@ -509,6 +464,11 @@ static int exynos_bus_probe(struct platform_device *pdev)
>>>>>>  
>>>>>>  err:
>>>>>>  	dev_pm_opp_of_remove_table(dev);
>>>>>> +	if (bus->opp_table) {
>>>>>> +		dev_pm_opp_put_regulators(bus->opp_table);
>>>>>> +		bus->opp_table = NULL;
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> ditto.
>>>>> Have to disable regulator after disabling the clock
>>>>> to prevent the h/w fault.
>>>>>
>>>>> I think that you should call them with following sequence:
>>>>>
>>>>> 	clk_disable_unprepare(bus->clk);
>>>>> 	if (bus->opp_table)
>>>>> 		dev_pm_opp_put_regulators(bus->opp_table);
>>>>> 	dev_pm_opp_of_remove_table(dev);
>>>>>
>>>>>>  	clk_disable_unprepare(bus->clk);
>>>>>>  
>>>>>>  	return ret;
>>>>
>>>> Best regards,
>>>> --
>>>> Bartlomiej Zolnierkiewicz
>>>> Samsung R&D Institute Poland
>>>> Samsung Electronics
>>
>> Best regards,
>> --
>> Bartlomiej Zolnierkiewicz
>> Samsung R&D Institute Poland
>> Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 2/4] devfreq: exynos-bus: convert to use dev_pm_opp_set_rate()

[Bartlomiej Zolnierkiewicz]

On 7/16/19 1:39 PM, Bartlomiej Zolnierkiewicz wrote:
> 
> On 7/16/19 1:26 PM, Chanwoo Choi wrote:

[...]

>>> Doesn't seem to be needed, care to explain it more?
>>
>> In order to fix the sequence problem between clock and regulator
>> with dev_pm_opp_set_regualtor() and want to keep two functions
>> (exynos_bus_parent_parse_of() and exynos_bus_parse_of()),
>> have to change the call order as following and then modify
>> the exception handling code when error happen.
>>
>> 	node = of_parse_phandle(dev->of_node, "devfreq", 0);                    
>> 	if (node) {                                                             
>> 		of_node_put(node);                                              
>> 		passive = true
>> 	}
>>
>> 	if (!passive)	
>> 		exynos_bus_parent_parse_of()
>> 			dev_pm_opp_set_regulator
>>
>> 	exynos_bus_parse_of()
> 
> OK. This seems like a solution.

PS Thanks for explaining this in detail.

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 3/4] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800

[Kamil Konieczny]

Declare Exynos5422/5800 voltage ranges for opp points for big cpu core and
bus wcore and couple their voltage supllies as vdd_arm and vdd_int should
be in 300mV range.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 arch/arm/boot/dts/exynos5420.dtsi             | 34 +++++++++----------
 arch/arm/boot/dts/exynos5422-odroid-core.dtsi |  4 +++
 arch/arm/boot/dts/exynos5800-peach-pi.dts     |  4 +++
 arch/arm/boot/dts/exynos5800.dtsi             | 32 ++++++++---------
 4 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
index 5fb2326875dc..0cbf74750553 100644
--- a/arch/arm/boot/dts/exynos5420.dtsi
+++ b/arch/arm/boot/dts/exynos5420.dtsi
@@ -48,62 +48,62 @@
 			opp-shared;
 			opp-1800000000 {
 				opp-hz = /bits/ 64 <1800000000>;
-				opp-microvolt = <1250000>;
+				opp-microvolt = <1250000 1250000 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-1700000000 {
 				opp-hz = /bits/ 64 <1700000000>;
-				opp-microvolt = <1212500>;
+				opp-microvolt = <1212500 1212500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-1600000000 {
 				opp-hz = /bits/ 64 <1600000000>;
-				opp-microvolt = <1175000>;
+				opp-microvolt = <1175000 1175000 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-1500000000 {
 				opp-hz = /bits/ 64 <1500000000>;
-				opp-microvolt = <1137500>;
+				opp-microvolt = <1137500 1137500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-1400000000 {
 				opp-hz = /bits/ 64 <1400000000>;
-				opp-microvolt = <1112500>;
+				opp-microvolt = <1112500 1112500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-1300000000 {
 				opp-hz = /bits/ 64 <1300000000>;
-				opp-microvolt = <1062500>;
+				opp-microvolt = <1062500 1062500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-1200000000 {
 				opp-hz = /bits/ 64 <1200000000>;
-				opp-microvolt = <1037500>;
+				opp-microvolt = <1037500 1037500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-1100000000 {
 				opp-hz = /bits/ 64 <1100000000>;
-				opp-microvolt = <1012500>;
+				opp-microvolt = <1012500 1012500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-1000000000 {
 				opp-hz = /bits/ 64 <1000000000>;
-				opp-microvolt = < 987500>;
+				opp-microvolt = < 987500 987500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-900000000 {
 				opp-hz = /bits/ 64 <900000000>;
-				opp-microvolt = < 962500>;
+				opp-microvolt = < 962500 962500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-800000000 {
 				opp-hz = /bits/ 64 <800000000>;
-				opp-microvolt = < 937500>;
+				opp-microvolt = < 937500 937500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 			opp-700000000 {
 				opp-hz = /bits/ 64 <700000000>;
-				opp-microvolt = < 912500>;
+				opp-microvolt = < 912500 912500 1500000>;
 				clock-latency-ns = <140000>;
 			};
 		};
@@ -1100,23 +1100,23 @@
 
 			opp00 {
 				opp-hz = /bits/ 64 <84000000>;
-				opp-microvolt = <925000>;
+				opp-microvolt = <925000 925000 1400000>;
 			};
 			opp01 {
 				opp-hz = /bits/ 64 <111000000>;
-				opp-microvolt = <950000>;
+				opp-microvolt = <950000 950000 1400000>;
 			};
 			opp02 {
 				opp-hz = /bits/ 64 <222000000>;
-				opp-microvolt = <950000>;
+				opp-microvolt = <950000 950000 1400000>;
 			};
 			opp03 {
 				opp-hz = /bits/ 64 <333000000>;
-				opp-microvolt = <950000>;
+				opp-microvolt = <950000 950000 1400000>;
 			};
 			opp04 {
 				opp-hz = /bits/ 64 <400000000>;
-				opp-microvolt = <987500>;
+				opp-microvolt = <987500 987500 1400000>;
 			};
 		};
 
diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
index 25d95de15c9b..65d094256b54 100644
--- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
+++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
@@ -428,6 +428,8 @@
 				regulator-max-microvolt = <1500000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-coupled-with = <&buck3_reg>;
+				regulator-coupled-max-spread = <300000>;
 			};
 
 			buck3_reg: BUCK3 {
@@ -436,6 +438,8 @@
 				regulator-max-microvolt = <1400000>;
 				regulator-always-on;
 				regulator-boot-on;
+				regulator-coupled-with = <&buck2_reg>;
+				regulator-coupled-max-spread = <300000>;
 			};
 
 			buck4_reg: BUCK4 {
diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
index e0f470fe54c8..5c1e965ed7e9 100644
--- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
+++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
@@ -257,6 +257,8 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-coupled-with = <&buck3_reg>;
+				regulator-coupled-max-spread = <300000>;
 				regulator-state-mem {
 					regulator-off-in-suspend;
 				};
@@ -269,6 +271,8 @@
 				regulator-always-on;
 				regulator-boot-on;
 				regulator-ramp-delay = <12500>;
+				regulator-coupled-with = <&buck2_reg>;
+				regulator-coupled-max-spread = <300000>;
 				regulator-state-mem {
 					regulator-off-in-suspend;
 				};
diff --git a/arch/arm/boot/dts/exynos5800.dtsi b/arch/arm/boot/dts/exynos5800.dtsi
index 57d3b319fd65..2a74735d161c 100644
--- a/arch/arm/boot/dts/exynos5800.dtsi
+++ b/arch/arm/boot/dts/exynos5800.dtsi
@@ -22,61 +22,61 @@
 
 &cluster_a15_opp_table {
 	opp-1700000000 {
-		opp-microvolt = <1250000>;
+		opp-microvolt = <1250000 1250000 1500000>;
 	};
 	opp-1600000000 {
-		opp-microvolt = <1250000>;
+		opp-microvolt = <1250000 1250000 1500000>;
 	};
 	opp-1500000000 {
-		opp-microvolt = <1100000>;
+		opp-microvolt = <1100000 1100000 1500000>;
 	};
 	opp-1400000000 {
-		opp-microvolt = <1100000>;
+		opp-microvolt = <1100000 1100000 1500000>;
 	};
 	opp-1300000000 {
-		opp-microvolt = <1100000>;
+		opp-microvolt = <1100000 1100000 1500000>;
 	};
 	opp-1200000000 {
-		opp-microvolt = <1000000>;
+		opp-microvolt = <1000000 1000000 1500000>;
 	};
 	opp-1100000000 {
-		opp-microvolt = <1000000>;
+		opp-microvolt = <1000000 1000000 1500000>;
 	};
 	opp-1000000000 {
-		opp-microvolt = <1000000>;
+		opp-microvolt = <1000000 1000000 1500000>;
 	};
 	opp-900000000 {
-		opp-microvolt = <1000000>;
+		opp-microvolt = <1000000 1000000 1500000>;
 	};
 	opp-800000000 {
-		opp-microvolt = <900000>;
+		opp-microvolt = <900000 900000 1500000>;
 	};
 	opp-700000000 {
-		opp-microvolt = <900000>;
+		opp-microvolt = <900000 900000 1500000>;
 	};
 	opp-600000000 {
 		opp-hz = /bits/ 64 <600000000>;
-		opp-microvolt = <900000>;
+		opp-microvolt = <900000 900000 1500000>;
 		clock-latency-ns = <140000>;
 	};
 	opp-500000000 {
 		opp-hz = /bits/ 64 <500000000>;
-		opp-microvolt = <900000>;
+		opp-microvolt = <900000 900000 1500000>;
 		clock-latency-ns = <140000>;
 	};
 	opp-400000000 {
 		opp-hz = /bits/ 64 <400000000>;
-		opp-microvolt = <900000>;
+		opp-microvolt = <900000 900000 1500000>;
 		clock-latency-ns = <140000>;
 	};
 	opp-300000000 {
 		opp-hz = /bits/ 64 <300000000>;
-		opp-microvolt = <900000>;
+		opp-microvolt = <900000 900000 1500000>;
 		clock-latency-ns = <140000>;
 	};
 	opp-200000000 {
 		opp-hz = /bits/ 64 <200000000>;
-		opp-microvolt = <900000>;
+		opp-microvolt = <900000 900000 1500000>;
 		clock-latency-ns = <140000>;
 	};
 };
-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800

[Chanwoo Choi]

Hi,

On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Declare Exynos5422/5800 voltage ranges for opp points for big cpu core and
> bus wcore and couple their voltage supllies as vdd_arm and vdd_int should
> be in 300mV range.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  arch/arm/boot/dts/exynos5420.dtsi             | 34 +++++++++----------
>  arch/arm/boot/dts/exynos5422-odroid-core.dtsi |  4 +++
>  arch/arm/boot/dts/exynos5800-peach-pi.dts     |  4 +++
>  arch/arm/boot/dts/exynos5800.dtsi             | 32 ++++++++---------
>  4 files changed, 41 insertions(+), 33 deletions(-)

Reviewed-by: Chanwoo Choi <cw00.choi@samsung.com>

> 
> diff --git a/arch/arm/boot/dts/exynos5420.dtsi b/arch/arm/boot/dts/exynos5420.dtsi
> index 5fb2326875dc..0cbf74750553 100644
> --- a/arch/arm/boot/dts/exynos5420.dtsi
> +++ b/arch/arm/boot/dts/exynos5420.dtsi
> @@ -48,62 +48,62 @@
>  			opp-shared;
>  			opp-1800000000 {
>  				opp-hz = /bits/ 64 <1800000000>;
> -				opp-microvolt = <1250000>;
> +				opp-microvolt = <1250000 1250000 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-1700000000 {
>  				opp-hz = /bits/ 64 <1700000000>;
> -				opp-microvolt = <1212500>;
> +				opp-microvolt = <1212500 1212500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-1600000000 {
>  				opp-hz = /bits/ 64 <1600000000>;
> -				opp-microvolt = <1175000>;
> +				opp-microvolt = <1175000 1175000 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-1500000000 {
>  				opp-hz = /bits/ 64 <1500000000>;
> -				opp-microvolt = <1137500>;
> +				opp-microvolt = <1137500 1137500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-1400000000 {
>  				opp-hz = /bits/ 64 <1400000000>;
> -				opp-microvolt = <1112500>;
> +				opp-microvolt = <1112500 1112500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-1300000000 {
>  				opp-hz = /bits/ 64 <1300000000>;
> -				opp-microvolt = <1062500>;
> +				opp-microvolt = <1062500 1062500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-1200000000 {
>  				opp-hz = /bits/ 64 <1200000000>;
> -				opp-microvolt = <1037500>;
> +				opp-microvolt = <1037500 1037500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-1100000000 {
>  				opp-hz = /bits/ 64 <1100000000>;
> -				opp-microvolt = <1012500>;
> +				opp-microvolt = <1012500 1012500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-1000000000 {
>  				opp-hz = /bits/ 64 <1000000000>;
> -				opp-microvolt = < 987500>;
> +				opp-microvolt = < 987500 987500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-900000000 {
>  				opp-hz = /bits/ 64 <900000000>;
> -				opp-microvolt = < 962500>;
> +				opp-microvolt = < 962500 962500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-800000000 {
>  				opp-hz = /bits/ 64 <800000000>;
> -				opp-microvolt = < 937500>;
> +				opp-microvolt = < 937500 937500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  			opp-700000000 {
>  				opp-hz = /bits/ 64 <700000000>;
> -				opp-microvolt = < 912500>;
> +				opp-microvolt = < 912500 912500 1500000>;
>  				clock-latency-ns = <140000>;
>  			};
>  		};
> @@ -1100,23 +1100,23 @@
>  
>  			opp00 {
>  				opp-hz = /bits/ 64 <84000000>;
> -				opp-microvolt = <925000>;
> +				opp-microvolt = <925000 925000 1400000>;
>  			};
>  			opp01 {
>  				opp-hz = /bits/ 64 <111000000>;
> -				opp-microvolt = <950000>;
> +				opp-microvolt = <950000 950000 1400000>;
>  			};
>  			opp02 {
>  				opp-hz = /bits/ 64 <222000000>;
> -				opp-microvolt = <950000>;
> +				opp-microvolt = <950000 950000 1400000>;
>  			};
>  			opp03 {
>  				opp-hz = /bits/ 64 <333000000>;
> -				opp-microvolt = <950000>;
> +				opp-microvolt = <950000 950000 1400000>;
>  			};
>  			opp04 {
>  				opp-hz = /bits/ 64 <400000000>;
> -				opp-microvolt = <987500>;
> +				opp-microvolt = <987500 987500 1400000>;
>  			};
>  		};
>  
> diff --git a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> index 25d95de15c9b..65d094256b54 100644
> --- a/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> +++ b/arch/arm/boot/dts/exynos5422-odroid-core.dtsi
> @@ -428,6 +428,8 @@
>  				regulator-max-microvolt = <1500000>;
>  				regulator-always-on;
>  				regulator-boot-on;
> +				regulator-coupled-with = <&buck3_reg>;
> +				regulator-coupled-max-spread = <300000>;
>  			};
>  
>  			buck3_reg: BUCK3 {
> @@ -436,6 +438,8 @@
>  				regulator-max-microvolt = <1400000>;
>  				regulator-always-on;
>  				regulator-boot-on;
> +				regulator-coupled-with = <&buck2_reg>;
> +				regulator-coupled-max-spread = <300000>;
>  			};
>  
>  			buck4_reg: BUCK4 {
> diff --git a/arch/arm/boot/dts/exynos5800-peach-pi.dts b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> index e0f470fe54c8..5c1e965ed7e9 100644
> --- a/arch/arm/boot/dts/exynos5800-peach-pi.dts
> +++ b/arch/arm/boot/dts/exynos5800-peach-pi.dts
> @@ -257,6 +257,8 @@
>  				regulator-always-on;
>  				regulator-boot-on;
>  				regulator-ramp-delay = <12500>;
> +				regulator-coupled-with = <&buck3_reg>;
> +				regulator-coupled-max-spread = <300000>;
>  				regulator-state-mem {
>  					regulator-off-in-suspend;
>  				};
> @@ -269,6 +271,8 @@
>  				regulator-always-on;
>  				regulator-boot-on;
>  				regulator-ramp-delay = <12500>;
> +				regulator-coupled-with = <&buck2_reg>;
> +				regulator-coupled-max-spread = <300000>;
>  				regulator-state-mem {
>  					regulator-off-in-suspend;
>  				};
> diff --git a/arch/arm/boot/dts/exynos5800.dtsi b/arch/arm/boot/dts/exynos5800.dtsi
> index 57d3b319fd65..2a74735d161c 100644
> --- a/arch/arm/boot/dts/exynos5800.dtsi
> +++ b/arch/arm/boot/dts/exynos5800.dtsi
> @@ -22,61 +22,61 @@
>  
>  &cluster_a15_opp_table {
>  	opp-1700000000 {
> -		opp-microvolt = <1250000>;
> +		opp-microvolt = <1250000 1250000 1500000>;
>  	};
>  	opp-1600000000 {
> -		opp-microvolt = <1250000>;
> +		opp-microvolt = <1250000 1250000 1500000>;
>  	};
>  	opp-1500000000 {
> -		opp-microvolt = <1100000>;
> +		opp-microvolt = <1100000 1100000 1500000>;
>  	};
>  	opp-1400000000 {
> -		opp-microvolt = <1100000>;
> +		opp-microvolt = <1100000 1100000 1500000>;
>  	};
>  	opp-1300000000 {
> -		opp-microvolt = <1100000>;
> +		opp-microvolt = <1100000 1100000 1500000>;
>  	};
>  	opp-1200000000 {
> -		opp-microvolt = <1000000>;
> +		opp-microvolt = <1000000 1000000 1500000>;
>  	};
>  	opp-1100000000 {
> -		opp-microvolt = <1000000>;
> +		opp-microvolt = <1000000 1000000 1500000>;
>  	};
>  	opp-1000000000 {
> -		opp-microvolt = <1000000>;
> +		opp-microvolt = <1000000 1000000 1500000>;
>  	};
>  	opp-900000000 {
> -		opp-microvolt = <1000000>;
> +		opp-microvolt = <1000000 1000000 1500000>;
>  	};
>  	opp-800000000 {
> -		opp-microvolt = <900000>;
> +		opp-microvolt = <900000 900000 1500000>;
>  	};
>  	opp-700000000 {
> -		opp-microvolt = <900000>;
> +		opp-microvolt = <900000 900000 1500000>;
>  	};
>  	opp-600000000 {
>  		opp-hz = /bits/ 64 <600000000>;
> -		opp-microvolt = <900000>;
> +		opp-microvolt = <900000 900000 1500000>;
>  		clock-latency-ns = <140000>;
>  	};
>  	opp-500000000 {
>  		opp-hz = /bits/ 64 <500000000>;
> -		opp-microvolt = <900000>;
> +		opp-microvolt = <900000 900000 1500000>;
>  		clock-latency-ns = <140000>;
>  	};
>  	opp-400000000 {
>  		opp-hz = /bits/ 64 <400000000>;
> -		opp-microvolt = <900000>;
> +		opp-microvolt = <900000 900000 1500000>;
>  		clock-latency-ns = <140000>;
>  	};
>  	opp-300000000 {
>  		opp-hz = /bits/ 64 <300000000>;
> -		opp-microvolt = <900000>;
> +		opp-microvolt = <900000 900000 1500000>;
>  		clock-latency-ns = <140000>;
>  	};
>  	opp-200000000 {
>  		opp-hz = /bits/ 64 <200000000>;
> -		opp-microvolt = <900000>;
> +		opp-microvolt = <900000 900000 1500000>;
>  		clock-latency-ns = <140000>;
>  	};
>  };
> 


-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800

[Krzysztof Kozlowski]

On Mon, 15 Jul 2019 at 14:04, Kamil Konieczny
<k.konieczny@partner.samsung.com> wrote:
>
> Declare Exynos5422/5800 voltage ranges for opp points for big cpu core and
> bus wcore and couple their voltage supllies as vdd_arm and vdd_int should
> be in 300mV range.
>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>

This one was previously from Marek, now it is from you. Any changes here?

Best regards,
Krzysztof

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 3/4] ARM: dts: exynos: add initial data for coupled regulators for Exynos5422/5800

[Bartlomiej Zolnierkiewicz]

On 7/16/19 11:22 AM, Krzysztof Kozlowski wrote:
> On Mon, 15 Jul 2019 at 14:04, Kamil Konieczny
> <k.konieczny@partner.samsung.com> wrote:
>>
>> Declare Exynos5422/5800 voltage ranges for opp points for big cpu core and
>> bus wcore and couple their voltage supllies as vdd_arm and vdd_int should
>> be in 300mV range.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> 
> This one was previously from Marek, now it is from you. Any changes here?

Hmmm, it seems that "From:" tag somehow got lost in v2 compared to v1?

Also the note about adding patch description (which was the only update to
original Marek's patch IIRC) should be added, something like:

...
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
[k.konieczny: add missing patch description]
Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v2 4/4] dt-bindings: devfreq: exynos-bus: remove unused property

[Kamil Konieczny]

Remove unused DT property "exynos,voltage-tolerance".

Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
---
 Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 2 --
 1 file changed, 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
index f8e946471a58..e71f752cc18f 100644
--- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
+++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
@@ -50,8 +50,6 @@ Required properties only for passive bus device:
 Optional properties only for parent bus device:
 - exynos,saturation-ratio: the percentage value which is used to calibrate
 			the performance count against total cycle count.
-- exynos,voltage-tolerance: the percentage value for bus voltage tolerance
-			which is used to calculate the max voltage.
 
 Detailed correlation between sub-blocks and power line according to Exynos SoC:
 - In case of Exynos3250, there are two power line as following:
-- 
2.22.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2 4/4] dt-bindings: devfreq: exynos-bus: remove unused property

[Chanwoo Choi]

Hi,

On 19. 7. 15. 오후 9:04, Kamil Konieczny wrote:
> Remove unused DT property "exynos,voltage-tolerance".
> 
> Signed-off-by: Kamil Konieczny <k.konieczny@partner.samsung.com>
> ---
>  Documentation/devicetree/bindings/devfreq/exynos-bus.txt | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> index f8e946471a58..e71f752cc18f 100644
> --- a/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> +++ b/Documentation/devicetree/bindings/devfreq/exynos-bus.txt
> @@ -50,8 +50,6 @@ Required properties only for passive bus device:
>  Optional properties only for parent bus device:
>  - exynos,saturation-ratio: the percentage value which is used to calibrate
>  			the performance count against total cycle count.
> -- exynos,voltage-tolerance: the percentage value for bus voltage tolerance
> -			which is used to calculate the max voltage.
>  
>  Detailed correlation between sub-blocks and power line according to Exynos SoC:
>  - In case of Exynos3250, there are two power line as following:
> 

Acked-by: Chanwoo Choi <cw00.choi@samsung.com>

-- 
Best Regards,
Chanwoo Choi
Samsung Electronics

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel