diff mbox series

[v3,7/8] thermal: exynos: split initialization of TMU and the thermal zone

Message ID 20231003111638.241542-8-m.majewski2@samsung.com (mailing list archive)
State New, archived
Headers show
Series Improve Exynos thermal driver | expand

Commit Message

Mateusz Majewski Oct. 3, 2023, 11:16 a.m. UTC
This will be needed in the future, as the thermal zone subsystem might
call our callbacks right after devm_thermal_of_zone_register. Currently
we just make get_temp return EAGAIN in such case, but this will not be
possible with state-modifying callbacks, for instance set_trips.

Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
---
v1 -> v2: We take clocks into account; exynos_tmu_initialize needs both
  clocks, as tmu_initialize might use the base_second registers. However,
  exynos_thermal_zone_configure only needs clk.

 drivers/thermal/samsung/exynos_tmu.c | 104 +++++++++++++++------------
 1 file changed, 60 insertions(+), 44 deletions(-)

Comments

Krzysztof Kozlowski Oct. 6, 2023, 1:41 p.m. UTC | #1
On 03/10/2023 13:16, Mateusz Majewski wrote:
> This will be needed in the future, as the thermal zone subsystem might
> call our callbacks right after devm_thermal_of_zone_register. Currently
> we just make get_temp return EAGAIN in such case, but this will not be
> possible with state-modifying callbacks, for instance set_trips.
> 
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof
Lukasz Luba Oct. 23, 2023, 12:59 p.m. UTC | #2
Hi Mateusz,

On 10/3/23 12:16, Mateusz Majewski wrote:
> This will be needed in the future, as the thermal zone subsystem might
> call our callbacks right after devm_thermal_of_zone_register. Currently
> we just make get_temp return EAGAIN in such case, but this will not be
> possible with state-modifying callbacks, for instance set_trips.
> 
> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
> ---
> v1 -> v2: We take clocks into account; exynos_tmu_initialize needs both
>    clocks, as tmu_initialize might use the base_second registers. However,
>    exynos_thermal_zone_configure only needs clk.
> 
>   drivers/thermal/samsung/exynos_tmu.c | 104 +++++++++++++++------------
>   1 file changed, 60 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
> index 7138e001fa5a..343e27c61528 100644
> --- a/drivers/thermal/samsung/exynos_tmu.c
> +++ b/drivers/thermal/samsung/exynos_tmu.c
> @@ -251,25 +251,8 @@ static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
>   static int exynos_tmu_initialize(struct platform_device *pdev)
>   {
>   	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
> -	struct thermal_zone_device *tzd = data->tzd;
> -	int num_trips = thermal_zone_get_num_trips(tzd);
>   	unsigned int status;
> -	int ret = 0, temp;
> -
> -	ret = thermal_zone_get_crit_temp(tzd, &temp);
> -	if (ret && data->soc != SOC_ARCH_EXYNOS5433) { /* FIXME */
> -		dev_err(&pdev->dev,
> -			"No CRITICAL trip point defined in device tree!\n");
> -		goto out;
> -	}
> -
> -	if (num_trips > data->ntrip) {
> -		dev_info(&pdev->dev,
> -			 "More trip points than supported by this TMU.\n");
> -		dev_info(&pdev->dev,
> -			 "%d trip points should be configured in polling mode.\n",
> -			 num_trips - data->ntrip);
> -	}
> +	int ret = 0;
>   
>   	mutex_lock(&data->lock);
>   	clk_enable(data->clk);
> @@ -280,32 +263,63 @@ static int exynos_tmu_initialize(struct platform_device *pdev)
>   	if (!status) {
>   		ret = -EBUSY;
>   	} else {
> -		int i, ntrips =
> -			min_t(int, num_trips, data->ntrip);
> -
>   		data->tmu_initialize(pdev);
> -
> -		/* Write temperature code for rising and falling threshold */
> -		for (i = 0; i < ntrips; i++) {
> -
> -			struct thermal_trip trip;
> -
> -			ret = thermal_zone_get_trip(tzd, i, &trip);
> -			if (ret)
> -				goto err;
> -
> -			data->tmu_set_trip_temp(data, i, trip.temperature / MCELSIUS);
> -			data->tmu_set_trip_hyst(data, i, trip.temperature / MCELSIUS,
> -						trip.hysteresis / MCELSIUS);
> -		}
> -
>   		data->tmu_clear_irqs(data);
>   	}
> +
> +	mutex_unlock(&data->lock);
> +	clk_disable(data->clk);
> +	if (!IS_ERR(data->clk_sec))
> +		clk_disable(data->clk_sec);

In all other places the clock is strictly protected inside the critical
section, but not here. In this code in theory on SMP (especially with
big.LITTLE system with different speeds of CPUs) this could lead to
disabling the clocks just after other CPU acquired the mutex and enabled
them (in order to use the HW regs).

Please put those two clocks before the mutex_unlock() and in the
reverse order.

Regards,
Lukasz
Marek Szyprowski Oct. 23, 2023, 1:33 p.m. UTC | #3
On 23.10.2023 14:59, Lukasz Luba wrote:
> On 10/3/23 12:16, Mateusz Majewski wrote:
>> This will be needed in the future, as the thermal zone subsystem might
>> call our callbacks right after devm_thermal_of_zone_register. Currently
>> we just make get_temp return EAGAIN in such case, but this will not be
>> possible with state-modifying callbacks, for instance set_trips.
>>
>> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
>> ---
>> v1 -> v2: We take clocks into account; exynos_tmu_initialize needs both
>>    clocks, as tmu_initialize might use the base_second registers. 
>> However,
>>    exynos_thermal_zone_configure only needs clk.
>>
>>   drivers/thermal/samsung/exynos_tmu.c | 104 +++++++++++++++------------
>>   1 file changed, 60 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/thermal/samsung/exynos_tmu.c 
>> b/drivers/thermal/samsung/exynos_tmu.c
>> index 7138e001fa5a..343e27c61528 100644
>> --- a/drivers/thermal/samsung/exynos_tmu.c
>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>> @@ -251,25 +251,8 @@ static void sanitize_temp_error(struct 
>> exynos_tmu_data *data, u32 trim_info)
>>   static int exynos_tmu_initialize(struct platform_device *pdev)
>>   {
>>       struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>> -    struct thermal_zone_device *tzd = data->tzd;
>> -    int num_trips = thermal_zone_get_num_trips(tzd);
>>       unsigned int status;
>> -    int ret = 0, temp;
>> -
>> -    ret = thermal_zone_get_crit_temp(tzd, &temp);
>> -    if (ret && data->soc != SOC_ARCH_EXYNOS5433) { /* FIXME */
>> -        dev_err(&pdev->dev,
>> -            "No CRITICAL trip point defined in device tree!\n");
>> -        goto out;
>> -    }
>> -
>> -    if (num_trips > data->ntrip) {
>> -        dev_info(&pdev->dev,
>> -             "More trip points than supported by this TMU.\n");
>> -        dev_info(&pdev->dev,
>> -             "%d trip points should be configured in polling mode.\n",
>> -             num_trips - data->ntrip);
>> -    }
>> +    int ret = 0;
>>         mutex_lock(&data->lock);
>>       clk_enable(data->clk);
>> @@ -280,32 +263,63 @@ static int exynos_tmu_initialize(struct 
>> platform_device *pdev)
>>       if (!status) {
>>           ret = -EBUSY;
>>       } else {
>> -        int i, ntrips =
>> -            min_t(int, num_trips, data->ntrip);
>> -
>>           data->tmu_initialize(pdev);
>> -
>> -        /* Write temperature code for rising and falling threshold */
>> -        for (i = 0; i < ntrips; i++) {
>> -
>> -            struct thermal_trip trip;
>> -
>> -            ret = thermal_zone_get_trip(tzd, i, &trip);
>> -            if (ret)
>> -                goto err;
>> -
>> -            data->tmu_set_trip_temp(data, i, trip.temperature / 
>> MCELSIUS);
>> -            data->tmu_set_trip_hyst(data, i, trip.temperature / 
>> MCELSIUS,
>> -                        trip.hysteresis / MCELSIUS);
>> -        }
>> -
>>           data->tmu_clear_irqs(data);
>>       }
>> +
>> +    mutex_unlock(&data->lock);
>> +    clk_disable(data->clk);
>> +    if (!IS_ERR(data->clk_sec))
>> +        clk_disable(data->clk_sec);
>
> In all other places the clock is strictly protected inside the critical
> section, but not here. In this code in theory on SMP (especially with
> big.LITTLE system with different speeds of CPUs) this could lead to
> disabling the clocks just after other CPU acquired the mutex and enabled
> them (in order to use the HW regs).


Clocks have internal atomic counters, so it is safe to disable them 
after leaving critical section. The clock would still be enabled in the 
mentioned case.


> Please put those two clocks before the mutex_unlock() and in the
> reverse order.

Best regards
Lukasz Luba Oct. 23, 2023, 1:44 p.m. UTC | #4
On 10/23/23 14:33, Marek Szyprowski wrote:
> On 23.10.2023 14:59, Lukasz Luba wrote:
>> On 10/3/23 12:16, Mateusz Majewski wrote:
>>> This will be needed in the future, as the thermal zone subsystem might
>>> call our callbacks right after devm_thermal_of_zone_register. Currently
>>> we just make get_temp return EAGAIN in such case, but this will not be
>>> possible with state-modifying callbacks, for instance set_trips.
>>>
>>> Signed-off-by: Mateusz Majewski <m.majewski2@samsung.com>
>>> ---
>>> v1 -> v2: We take clocks into account; exynos_tmu_initialize needs both
>>>     clocks, as tmu_initialize might use the base_second registers.
>>> However,
>>>     exynos_thermal_zone_configure only needs clk.
>>>
>>>    drivers/thermal/samsung/exynos_tmu.c | 104 +++++++++++++++------------
>>>    1 file changed, 60 insertions(+), 44 deletions(-)
>>>
>>> diff --git a/drivers/thermal/samsung/exynos_tmu.c
>>> b/drivers/thermal/samsung/exynos_tmu.c
>>> index 7138e001fa5a..343e27c61528 100644
>>> --- a/drivers/thermal/samsung/exynos_tmu.c
>>> +++ b/drivers/thermal/samsung/exynos_tmu.c
>>> @@ -251,25 +251,8 @@ static void sanitize_temp_error(struct
>>> exynos_tmu_data *data, u32 trim_info)
>>>    static int exynos_tmu_initialize(struct platform_device *pdev)
>>>    {
>>>        struct exynos_tmu_data *data = platform_get_drvdata(pdev);
>>> -    struct thermal_zone_device *tzd = data->tzd;
>>> -    int num_trips = thermal_zone_get_num_trips(tzd);
>>>        unsigned int status;
>>> -    int ret = 0, temp;
>>> -
>>> -    ret = thermal_zone_get_crit_temp(tzd, &temp);
>>> -    if (ret && data->soc != SOC_ARCH_EXYNOS5433) { /* FIXME */
>>> -        dev_err(&pdev->dev,
>>> -            "No CRITICAL trip point defined in device tree!\n");
>>> -        goto out;
>>> -    }
>>> -
>>> -    if (num_trips > data->ntrip) {
>>> -        dev_info(&pdev->dev,
>>> -             "More trip points than supported by this TMU.\n");
>>> -        dev_info(&pdev->dev,
>>> -             "%d trip points should be configured in polling mode.\n",
>>> -             num_trips - data->ntrip);
>>> -    }
>>> +    int ret = 0;
>>>          mutex_lock(&data->lock);
>>>        clk_enable(data->clk);
>>> @@ -280,32 +263,63 @@ static int exynos_tmu_initialize(struct
>>> platform_device *pdev)
>>>        if (!status) {
>>>            ret = -EBUSY;
>>>        } else {
>>> -        int i, ntrips =
>>> -            min_t(int, num_trips, data->ntrip);
>>> -
>>>            data->tmu_initialize(pdev);
>>> -
>>> -        /* Write temperature code for rising and falling threshold */
>>> -        for (i = 0; i < ntrips; i++) {
>>> -
>>> -            struct thermal_trip trip;
>>> -
>>> -            ret = thermal_zone_get_trip(tzd, i, &trip);
>>> -            if (ret)
>>> -                goto err;
>>> -
>>> -            data->tmu_set_trip_temp(data, i, trip.temperature /
>>> MCELSIUS);
>>> -            data->tmu_set_trip_hyst(data, i, trip.temperature /
>>> MCELSIUS,
>>> -                        trip.hysteresis / MCELSIUS);
>>> -        }
>>> -
>>>            data->tmu_clear_irqs(data);
>>>        }
>>> +
>>> +    mutex_unlock(&data->lock);
>>> +    clk_disable(data->clk);
>>> +    if (!IS_ERR(data->clk_sec))
>>> +        clk_disable(data->clk_sec);
>>
>> In all other places the clock is strictly protected inside the critical
>> section, but not here. In this code in theory on SMP (especially with
>> big.LITTLE system with different speeds of CPUs) this could lead to
>> disabling the clocks just after other CPU acquired the mutex and enabled
>> them (in order to use the HW regs).
> 
> 
> Clocks have internal atomic counters, so it is safe to disable them
> after leaving critical section. The clock would still be enabled in the
> mentioned case.

Fair enough. So I would just put them there for code cleanup and
aliment with all other places (otherwise it looks odd).
diff mbox series

Patch

diff --git a/drivers/thermal/samsung/exynos_tmu.c b/drivers/thermal/samsung/exynos_tmu.c
index 7138e001fa5a..343e27c61528 100644
--- a/drivers/thermal/samsung/exynos_tmu.c
+++ b/drivers/thermal/samsung/exynos_tmu.c
@@ -251,25 +251,8 @@  static void sanitize_temp_error(struct exynos_tmu_data *data, u32 trim_info)
 static int exynos_tmu_initialize(struct platform_device *pdev)
 {
 	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
-	struct thermal_zone_device *tzd = data->tzd;
-	int num_trips = thermal_zone_get_num_trips(tzd);
 	unsigned int status;
-	int ret = 0, temp;
-
-	ret = thermal_zone_get_crit_temp(tzd, &temp);
-	if (ret && data->soc != SOC_ARCH_EXYNOS5433) { /* FIXME */
-		dev_err(&pdev->dev,
-			"No CRITICAL trip point defined in device tree!\n");
-		goto out;
-	}
-
-	if (num_trips > data->ntrip) {
-		dev_info(&pdev->dev,
-			 "More trip points than supported by this TMU.\n");
-		dev_info(&pdev->dev,
-			 "%d trip points should be configured in polling mode.\n",
-			 num_trips - data->ntrip);
-	}
+	int ret = 0;
 
 	mutex_lock(&data->lock);
 	clk_enable(data->clk);
@@ -280,32 +263,63 @@  static int exynos_tmu_initialize(struct platform_device *pdev)
 	if (!status) {
 		ret = -EBUSY;
 	} else {
-		int i, ntrips =
-			min_t(int, num_trips, data->ntrip);
-
 		data->tmu_initialize(pdev);
-
-		/* Write temperature code for rising and falling threshold */
-		for (i = 0; i < ntrips; i++) {
-
-			struct thermal_trip trip;
-
-			ret = thermal_zone_get_trip(tzd, i, &trip);
-			if (ret)
-				goto err;
-
-			data->tmu_set_trip_temp(data, i, trip.temperature / MCELSIUS);
-			data->tmu_set_trip_hyst(data, i, trip.temperature / MCELSIUS,
-						trip.hysteresis / MCELSIUS);
-		}
-
 		data->tmu_clear_irqs(data);
 	}
+
+	mutex_unlock(&data->lock);
+	clk_disable(data->clk);
+	if (!IS_ERR(data->clk_sec))
+		clk_disable(data->clk_sec);
+
+	return ret;
+}
+
+static int exynos_thermal_zone_configure(struct platform_device *pdev)
+{
+	struct exynos_tmu_data *data = platform_get_drvdata(pdev);
+	struct thermal_zone_device *tzd = data->tzd;
+	int i, num_trips = thermal_zone_get_num_trips(tzd);
+	int ret = 0, temp;
+
+	ret = thermal_zone_get_crit_temp(tzd, &temp);
+
+	if (ret && data->soc != SOC_ARCH_EXYNOS5433) { /* FIXME */
+		dev_err(&pdev->dev,
+			"No CRITICAL trip point defined in device tree!\n");
+		goto out;
+	}
+
+	mutex_lock(&data->lock);
+
+	if (num_trips > data->ntrip) {
+		dev_info(&pdev->dev,
+			 "More trip points than supported by this TMU.\n");
+		dev_info(&pdev->dev,
+			 "%d trip points should be configured in polling mode.\n",
+			 num_trips - data->ntrip);
+	}
+
+	clk_enable(data->clk);
+
+	num_trips = min_t(int, num_trips, data->ntrip);
+
+	/* Write temperature code for rising and falling threshold */
+	for (i = 0; i < num_trips; i++) {
+		struct thermal_trip trip;
+
+		ret = thermal_zone_get_trip(tzd, i, &trip);
+		if (ret)
+			goto err;
+
+		data->tmu_set_trip_temp(data, i, trip.temperature / MCELSIUS);
+		data->tmu_set_trip_hyst(data, i, trip.temperature / MCELSIUS,
+					trip.hysteresis / MCELSIUS);
+	}
+
 err:
 	clk_disable(data->clk);
 	mutex_unlock(&data->lock);
-	if (!IS_ERR(data->clk_sec))
-		clk_disable(data->clk_sec);
 out:
 	return ret;
 }
@@ -1044,10 +1058,12 @@  static int exynos_tmu_probe(struct platform_device *pdev)
 		break;
 	}
 
-	/*
-	 * data->tzd must be registered before calling exynos_tmu_initialize(),
-	 * requesting irq and calling exynos_tmu_control().
-	 */
+	ret = exynos_tmu_initialize(pdev);
+	if (ret) {
+		dev_err(&pdev->dev, "Failed to initialize TMU\n");
+		return ret;
+	}
+
 	data->tzd = devm_thermal_of_zone_register(&pdev->dev, 0, data,
 						  &exynos_sensor_ops);
 	if (IS_ERR(data->tzd)) {
@@ -1058,9 +1074,9 @@  static int exynos_tmu_probe(struct platform_device *pdev)
 		goto err_sclk;
 	}
 
-	ret = exynos_tmu_initialize(pdev);
+	ret = exynos_thermal_zone_configure(pdev);
 	if (ret) {
-		dev_err(&pdev->dev, "Failed to initialize TMU\n");
+		dev_err(&pdev->dev, "Failed to configure the thermal zone\n");
 		goto err_sclk;
 	}