[PATCH v3 7/8] thermal: exynos: split initialization of TMU and the thermal zone
Lukasz Luba
lukasz.luba at arm.com
Mon Oct 23 06:44:08 PDT 2023
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 at 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).
More information about the linux-arm-kernel
mailing list