Messages in this thread | | | Date | Mon, 23 Oct 2023 14:44:08 +0100 | Subject | Re: [PATCH v3 7/8] thermal: exynos: split initialization of TMU and the thermal zone | From | Lukasz Luba <> |
| |
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).
| |