Re: [PATCH v4 8/8] thermal: exynos: use set_trips

[Lukasz Luba]

On 11/2/23 10:35, Mateusz Majewski wrote:
> Hi,
>
> > > +        th &= ~(0xff << 0);
> > > +        th |= temp_to_code(data, temp) << 0;
> >   
> > This 2-line pattern repeats a few times. It looks like a nice cadidate
> > for an inline function which can abstract that. Something like:
> >   
> > val = update_temp_value(data, temp, threshold, LOW_TEMP_SHIFT)
> >   
> > Assisted with the macros {LOW|HIGH|CRIT}_TEMP_SHIFT, the code
> > would look less convoluted IMO.
> > (The old code with the multiplication for the shift value wasn't
> > cleaner nor faster).
>
> What would you think about something like this?
>
> static void exynos_tmu_update_temp(struct exynos_tmu_data *data, int reg_off,
>                                     int bit_off, u8 temp)
> {
>          u32 th;
>
>          th = readl(data->base + reg_off);
>          th &= ~(0xff << bit_off);
>          th |= temp_to_code(data, temp) << bit_off;
>          writel(th, data->base + reg_off);
> }
>
> And then, it would be used like this:
>
> static void exynos4412_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          exynos_tmu_update_temp(data, EXYNOS_THD_TEMP_RISE, 24, temp);
>          exynos_tmu_update_bit(data, EXYNOS_TMU_REG_CONTROL,
>                                EXYNOS_TMU_THERM_TRIP_EN_SHIFT, true);
> }

Yes, this looks good.

> Granted it's not as clear as if we had some macro like CRIT_TEMP_SHIFT, but
> we would need more than one variant anyway, as Exynos 5433 uses different
> values of reg_off, and the new function looks short and inviting IMHO.

Fair enough.

> > > -static void exynos7_tmu_set_trip_temp(struct exynos_tmu_data *data,
> > > -                                      int trip, u8 temp)
> > > +static void exynos7_tmu_update_temp(struct exynos_tmu_data *data, u8 temp,
> > > +                                    int idx, bool rise)
> > >     {
> > >             unsigned int reg_off, bit_off;
> > >             u32 th;
> > > +        void __iomem *reg;
> > >     
> > > -        reg_off = ((7 - trip) / 2) * 4;
> > > -        bit_off = ((8 - trip) % 2);
> > > +        reg_off = ((7 - idx) / 2) * 4;
> >   
> > Why can't we just have a set of defined register macros and pick one
> > in some small function?
> > A lot of operations here, also some assumption.
> >   
> > > +        bit_off = ((8 - idx) % 2);
> >   
> > So this can only be 0 or 1 and than it's used for the shift
> > multiplication. Also I don't know the history of older code and
> > if it was missed after some cleaning, but 'idx % 2' gives
> > equal values but w/o subtraction.
> >   
> > BTW, the code assumes the 'idx' values are under control somewhere else.
> > Is that because the DT make sure in the schema that the range cannot be
> > too big?
> > What are the possible values for 'idx'?
>
> In the old code, the values of trip (which is the same thing, I will
> change the name back from idx) were limited by the value of data->ntrip,
> which was always 8 (value is per SoC). In the new code, there are only three
> variants:
>
> static void exynos7_tmu_set_low_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          exynos7_tmu_update_temp(data, temp, 0, false);
> }
>
> static void exynos7_tmu_set_high_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          exynos7_tmu_update_temp(data, temp, 1, true);
> }
>
> static void exynos7_tmu_set_crit_temp(struct exynos_tmu_data *data, u8 temp)
> {
>          /*
>           * Like Exynos 4210, Exynos 7 does not seem to support critical temperature
>           * handling in hardware. Again, we still set a separate interrupt for it.
>           */
>          exynos7_tmu_update_temp(data, temp, 7, true);
> }
>
> To be fair, considering the values are constant like this, I should probably
> just do the calculations myself and then in code just call exynos_tmu_update_temp
> (from above) and exynos_tmu_update_bit, like on all other SoCs. I guess I were
> a bit too scared to touch Exynos 7 code...

Yes, anything that can be pre-calculated with nice comment, would be
more desired. I would suggest to not be afraid about touching that
Exynos 7 code.

> > > -        if (on) {
> > > -                for (i = 0; i < data->ntrip; i++) {
> > > -                        if (thermal_zone_get_trip(tz, i, &trip))
> > > -                                continue;
> > > -
> > > -                        interrupt_en |=
> > > -                                (1 << (EXYNOS_TMU_INTEN_RISE0_SHIFT + i * 4));
> > > -                }
> > > -
> > > -                if (data->soc != SOC_ARCH_EXYNOS4210)
> > > -                        interrupt_en |=
> > > -                                interrupt_en << EXYNOS_TMU_INTEN_FALL0_SHIFT;
> > > -
> > > +        if (on)
> > >                     con |= (1 << EXYNOS_TMU_CORE_EN_SHIFT);
> > > -        } else {
> > > +        else
> > >                     con &= ~(1 << EXYNOS_TMU_CORE_EN_SHIFT);
> >   
> > Please also consider the BIT() helper here and above...
>
> Will do, but should I do this in a separate patch in these cases? I don't touch
> the con lines otherwise, and this patch is already humongous.

That would definitely deserve an extra patch to address it. Please add
to the patch set.

Regards,
Lukasz