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

[Lukasz Luba <lukasz.luba@arm.com>]

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).