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

[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@xxxxxxxxxxx>
> > > ---
> > > 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).