[PATCH 0/2] opp: ti-opp-supply: Fixes

[PATCH 0/2] opp: ti-opp-supply: Fixes

[Keerthy]

The series brings in couple of fixes to the ti-opp-supply driver.
One of them updates u_volt_min dynamically and avoids hang due
to lesser static u_volt_min and the other fixes the supply in
_get_optimal_vdd_voltage.

Keerthy (2):
  power: opp: ti-opp-supply: Dynamically update u_volt_min
  power: opp: ti-opp-supply: Correct the supply in
    _get_optimal_vdd_voltage call

 drivers/opp/ti-opp-supply.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

-- 
1.9.1

[PATCH 1/2] opp: ti-opp-supply: Dynamically update u_volt_min

[Keerthy]

The voltage range (min, max) provided in the device tree is from
the data manual and is pretty big, catering to a wide range of devices.
On a i2c read/write failure the regulator_set_voltage_triplet function
falls back to set voltage between min and max. The min value from Device
Tree can be lesser than the optimal value and in that case that can lead
to a hang or crash. Hence set the u_volt_min dynamically to the optimal
voltage value.

Fixes: 9a835fa6e47 ("PM / OPP: Add ti-opp-supply driver")
Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/opp/ti-opp-supply.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c
index 9e5a9a3..29e08a4 100644
--- a/drivers/opp/ti-opp-supply.c
+++ b/drivers/opp/ti-opp-supply.c
@@ -290,6 +290,9 @@ static int ti_opp_supply_set_opp(struct dev_pm_set_opp_data *data)
 	vdd_uv = _get_optimal_vdd_voltage(dev, &opp_data,
 					  new_supply_vbb->u_volt);
 
+	if (new_supply_vdd->u_volt_min < vdd_uv)
+		new_supply_vdd->u_volt_min = vdd_uv;
+
 	/* Scaling up? Scale voltage before frequency */
 	if (freq > old_freq) {
 		ret = _opp_set_voltage(dev, new_supply_vdd, vdd_uv, vdd_reg,
-- 
1.9.1

[PATCH 2/2] opp: ti-opp-supply: Correct the supply in _get_optimal_vdd_voltage call

[Keerthy]

_get_optimal_vdd_voltage call provides new_supply_vbb->u_volt
as the reference voltage while it should be really new_supply_vdd->u_volt.

Fixes: 9a835fa6e47 ("PM / OPP: Add ti-opp-supply driver")
Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/opp/ti-opp-supply.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c
index 29e08a4..3f4fb4d 100644
--- a/drivers/opp/ti-opp-supply.c
+++ b/drivers/opp/ti-opp-supply.c
@@ -288,7 +288,7 @@ static int ti_opp_supply_set_opp(struct dev_pm_set_opp_data *data)
 	int ret;
 
 	vdd_uv = _get_optimal_vdd_voltage(dev, &opp_data,
-					  new_supply_vbb->u_volt);
+					  new_supply_vdd->u_volt);
 
 	if (new_supply_vdd->u_volt_min < vdd_uv)
 		new_supply_vdd->u_volt_min = vdd_uv;
-- 
1.9.1

Re: [PATCH 0/2] opp: ti-opp-supply: Fixes

[Viresh Kumar]

On 07-11-18, 10:04, Keerthy wrote:
> The series brings in couple of fixes to the ti-opp-supply driver.
> One of them updates u_volt_min dynamically and avoids hang due
> to lesser static u_volt_min and the other fixes the supply in
> _get_optimal_vdd_voltage.
> 
> Keerthy (2):
>   power: opp: ti-opp-supply: Dynamically update u_volt_min
>   power: opp: ti-opp-supply: Correct the supply in
>     _get_optimal_vdd_voltage call

@Dave: I would like to get an Ack from you on these before applying.

-- 
viresh

Re: [PATCH 1/2] opp: ti-opp-supply: Dynamically update u_volt_min

[Viresh Kumar]

On 07-11-18, 10:04, Keerthy wrote:
> The voltage range (min, max) provided in the device tree is from
> the data manual and is pretty big, catering to a wide range of devices.
> On a i2c read/write failure the regulator_set_voltage_triplet function
> falls back to set voltage between min and max. The min value from Device
> Tree can be lesser than the optimal value and in that case that can lead
> to a hang or crash. Hence set the u_volt_min dynamically to the optimal
> voltage value.

And why shouldn't we fix the DT for this ?

-- 
viresh

Re: [PATCH 1/2] opp: ti-opp-supply: Dynamically update u_volt_min

[J, KEERTHY]

On 11/8/2018 11:24 AM, Viresh Kumar wrote:
> On 07-11-18, 10:04, Keerthy wrote:
>> The voltage range (min, max) provided in the device tree is from
>> the data manual and is pretty big, catering to a wide range of devices.
>> On a i2c read/write failure the regulator_set_voltage_triplet function
>> falls back to set voltage between min and max. The min value from Device
>> Tree can be lesser than the optimal value and in that case that can lead
>> to a hang or crash. Hence set the u_volt_min dynamically to the optimal
>> voltage value.
> 
> And why shouldn't we fix the DT for this ?

The DT voltages do not cater to the broad range of devices. In some 
particular cases the DT min voltage is slightly lower the voltage at 
which the device cannot sustain a particular frequency in which case the 
device just silently hangs. So best thing to do is to actually read the 
device specific voltages dynamically which will guarantee a particular 
device sustaining a particular frequency at the optimal voltage.

> 

Re: [PATCH 2/2] opp: ti-opp-supply: Correct the supply in _get_optimal_vdd_voltage call

[Dave Gerlach]

On 11/06/2018 10:34 PM, Keerthy wrote:
> _get_optimal_vdd_voltage call provides new_supply_vbb->u_volt
> as the reference voltage while it should be really new_supply_vdd->u_volt.
> 
> Fixes: 9a835fa6e47 ("PM / OPP: Add ti-opp-supply driver")
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---

Acked-by: Dave Gerlach <d-gerlach@ti.com>

Currently all corresponding vbb and vdd values provided by the device tree match
which is why this does not fail, but this was a typo by me and vdd is the
correct value to actually use.

Regards,
Dave

>  drivers/opp/ti-opp-supply.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/opp/ti-opp-supply.c b/drivers/opp/ti-opp-supply.c
> index 29e08a4..3f4fb4d 100644
> --- a/drivers/opp/ti-opp-supply.c
> +++ b/drivers/opp/ti-opp-supply.c
> @@ -288,7 +288,7 @@ static int ti_opp_supply_set_opp(struct dev_pm_set_opp_data *data)
>  	int ret;
>  
>  	vdd_uv = _get_optimal_vdd_voltage(dev, &opp_data,
> -					  new_supply_vbb->u_volt);
> +					  new_supply_vdd->u_volt);
>  
>  	if (new_supply_vdd->u_volt_min < vdd_uv)
>  		new_supply_vdd->u_volt_min = vdd_uv;
> 

Re: [PATCH 1/2] opp: ti-opp-supply: Dynamically update u_volt_min

[Dave Gerlach]

On 11/11/2018 09:15 PM, J, KEERTHY wrote:
> 
> 
> On 11/8/2018 11:24 AM, Viresh Kumar wrote:
>> On 07-11-18, 10:04, Keerthy wrote:
>>> The voltage range (min, max) provided in the device tree is from
>>> the data manual and is pretty big, catering to a wide range of devices.
>>> On a i2c read/write failure the regulator_set_voltage_triplet function
>>> falls back to set voltage between min and max. The min value from Device
>>> Tree can be lesser than the optimal value and in that case that can lead
>>> to a hang or crash. Hence set the u_volt_min dynamically to the optimal
>>> voltage value.
>>
>> And why shouldn't we fix the DT for this ?
> 
> The DT voltages do not cater to the broad range of devices. In some 
> particular cases the DT min voltage is slightly lower the voltage at 
> which the device cannot sustain a particular frequency in which case the 
> device just silently hangs. So best thing to do is to actually read the 
> device specific voltages dynamically which will guarantee a particular 
> device sustaining a particular frequency at the optimal voltage.
> 

Acked-by: Dave Gerlach <d-gerlach@ti.com>

>>

Re: [PATCH 0/2] opp: ti-opp-supply: Fixes

[Viresh Kumar]

On 07-11-18, 10:04, Keerthy wrote:
> The series brings in couple of fixes to the ti-opp-supply driver.
> One of them updates u_volt_min dynamically and avoids hang due
> to lesser static u_volt_min and the other fixes the supply in
> _get_optimal_vdd_voltage.

Applied both patches after manually adding below:

Cc: 4.16+ <stable@vger.kernel.org> # v4.16+

Thanks.

-- 
viresh