| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <KI6LZR.QB3G3JWHXXOD@somainline.org>
Date: Fri, 18 Aug 2023 14:25:32 +0200
From: Martin Botka <martin.botka@...ainline.org>
To: Andre Przywara <andre.przywara@....com>
Cc: Vasily Khoruzhick <anarsoul@...il.com>,
Yangtao Li <tiny.windzz@...il.com>,
"Rafael J. Wysocki" <rafael@...nel.org>,
Daniel Lezcano <daniel.lezcano@...aro.org>,
Amit Kucheria <amitk@...nel.org>,
Zhang Rui <rui.zhang@...el.com>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Chen-Yu Tsai <wens@...e.org>,
Jernej Skrabec <jernej.skrabec@...il.com>,
Samuel Holland <samuel@...lland.org>, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-sunxi@...ts.linux.dev, linux-kernel@...r.kernel.org,
Alan Ma <tech@...u3d.com>,
Luke Harrison <bttuniversity@...u3d.com>,
Marijn Suijten <marijn.suijten@...ainline.org>,
AngeloGioacchino Del Regno
<angelogioacchino.delregno@...ainline.org>,
Konrad Dybcio <konrad.dybcio@...ainline.org>,
Martin Botka <martin@...u3d.com>
Subject: Re: [PATCH 2/3] thermal: sun8i: Add support for H616 THS controller
On Fri, Aug 18 2023 at 01:06:59 PM +01:00:00, Andre Przywara
<andre.przywara@....com> wrote:
> On Fri, 18 Aug 2023 12:54:48 +0200
> Martin Botka <martin.botka@...ainline.org> wrote:
>
> Hi Martin,
>
>> On Fri, Aug 18 2023 at 11:29:30 AM +01:00:00, Andre Przywara
>> <andre.przywara@....com> wrote:
>> > On Fri, 18 Aug 2023 10:43:17 +0200
>> > Martin Botka <martin.botka@...ainline.org> wrote:
>> >
>> > Hi Martin,
>> >
>> > many thanks for the time and effort for upstreaming this!
>> >
>> >> Add support for the thermal sensor found in H616 SoC
>> >> which slightly resembles the H6 thermal sensor
>> >> controller with few changes like 4 sensors.
>> >>
>> >> Signed-off-by: Martin Botka <martin.botka@...ainline.org>
>> >> ---
>> >> drivers/thermal/sun8i_thermal.c | 115
>> >> ++++++++++++++++++++++++++++++++++++++++
>> >> 1 file changed, 115 insertions(+)
>> >>
>> >> diff --git a/drivers/thermal/sun8i_thermal.c
>> >> b/drivers/thermal/sun8i_thermal.c
>> >> index 195f3c5d0b38..9d73e4313ad8 100644
>> >> --- a/drivers/thermal/sun8i_thermal.c
>> >> +++ b/drivers/thermal/sun8i_thermal.c
>> >> @@ -108,6 +108,12 @@ static int sun50i_h5_calc_temp(struct
>> >> ths_device *tmdev,
>> >> return -1590 * reg / 10 + 276000;
>> >> }
>> >>
>> >> +static int sun50i_h616_calc_temp(struct ths_device *tmdev,
>> >> + int id, int reg)
>> >> +{
>> >> + return (reg + tmdev->chip->offset) * tmdev->chip->scale;
>> >
>> > So if my school maths is not letting me down, this is the same as
>> the
>> > sun8i_ths_calc_temp() function, when using:
>> > scale = h616_scale * -10
>> > offset = h616_offset * h616_scale
>> >
>> > So we do not need this new function, when we use:
>> > + .offset = 263655,
>> > + .scale = 810,
>> > below, right?
>> That looks correct. Seems like my brain has let me down once again
>> hehe.
>
> Well, I guess you took the code from some BSP drop, right? And "same
> code
> written differently" is a common pattern in those vendor kernels,
> because
> they work on this "new SoC, new Linux kernel" premise, and miss out in
> factoring out the commonalities. Which on the other hand is something
> that
> Linux maintainers tend to push for.
More or less yes.
>
>> > Those values are not only positive, but also seem closer to the
>> other
>> > SoC's values.
>> > This of course requires some small adjustment in the calibrate
>> > function,
>> > to accommodate for the changed scale, but I leave this up as an
>> > exercise
>> > to the reader ;-)
>> >
>> > Martin, can you confirm that this works?
>> Will do :)
>> >
>> >> +}
>> >> +
>> >> static int sun8i_ths_get_temp(struct thermal_zone_device *tz,
>> int
>> >> *temp)
>> >> {
>> >> struct tsensor *s = thermal_zone_device_priv(tz);
>> >> @@ -278,6 +284,64 @@ static int sun50i_h6_ths_calibrate(struct
>> >> ths_device *tmdev,
>> >> return 0;
>> >> }
>> >>
>> >> +static int sun50i_h616_ths_calibrate(struct ths_device *tmdev,
>> >> + u16 *caldata, int callen)
>> >
>> > nit: alignment
>> ack
>> >
>> >> +{
>> >> + struct device *dev = tmdev->dev;
>> >> + int i, ft_temp;
>> >> +
>> >> + if (!caldata[0])
>> >> + return -EINVAL;
>> >> +
>> >> + /*
>> >> + * h616 efuse THS calibration data layout:
>> >> + *
>> >> + * 0 11 16 27 32 43 48 57
>> >> + * +----------+-----------+-----------+-----------+
>> >> + * | temp | |sensor0| |sensor1| |sensor2| |
>> >> + * +----------+-----------+-----------+-----------+
>> >> + * ^ ^ ^
>> >> + * | | |
>> >> + * | | sensor3[11:8]
>> >> + * | sensor3[7:4]
>> >> + * sensor3[3:0]
>> >> + *
>> >> + * The calibration data on the H616 is the ambient
>> temperature and
>> >> + * sensor values that are filled during the factory test
>> stage.
>> >> + *
>> >> + * The unit of stored FT temperature is 0.1 degreee celusis.
>> >
>> > nit: degree Celsius
>> .... I can't even legit excuse this typo (If it even can be called
>> typo). Got it tho.
>> >
>> >> + */
>> >> + ft_temp = caldata[0] & FT_TEMP_MASK;
>> >> +
>> >> + for (i = 0; i < tmdev->chip->sensor_num; i++) {
>> >> + int delta, cdata, offset, reg;
>> >> +
>> >> + if (i == 3)
>> >> + reg = (caldata[1] >> 12)
>> >> + | (caldata[2] >> 12 << 4)
>> >> + | (caldata[3] >> 12 << 8);
>> >
>> > Can you add parentheses around the (caldata[2|3] >> 12) part?
>> Makes
>> > it a
>> > bit more readable.
>> yep
>> >
>> >> + else
>> >> + reg = (int)caldata[i + 1] & TEMP_CALIB_MASK;
>> >> +
>> >> + delta = (ft_temp * 100 - tmdev->chip->calc_temp(tmdev, i,
>> reg))
>> >> + / tmdev->chip->scale;
>> >
>> > Looks a bit odd, can you write this as over two lines?
>> > delta = ft_temp ...;
>> > delta /= tmdev->chip_scale;
>> can do.
>> >
>> > (And this would be the place where you adjust the calculation to
>> use
>> > the
>> > new scale value).
>> yep. Tho it is bit of a spoiler for the reader ;)
>> >
>> >> + cdata = CALIBRATE_DEFAULT - delta;
>> >> + if (cdata & ~TEMP_CALIB_MASK) {
>> >> + dev_warn(dev, "sensor%d is not calibrated.\n", i);
>> >> +
>> >> + continue;
>> >> + }
>> >> +
>> >> + offset = (i % 2) * 16;
>> >> + regmap_update_bits(tmdev->regmap,
>> >> + SUN50I_H6_THS_TEMP_CALIB + (i / 2 * 4),
>> >> + 0xfff << offset,
>> >
>> > That should be TEMP_CALIB_MASK << offset, compare the H6 code.
>> Agreed. Missed this one. Ty.
>> >
>> >> + cdata << offset);
>> >> + }
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> static int sun8i_ths_calibrate(struct ths_device *tmdev)
>> >> {
>> >> struct nvmem_cell *calcell;
>> >> @@ -453,6 +517,43 @@ static int sun50i_h6_thermal_init(struct
>> >> ths_device *tmdev)
>> >> return 0;
>> >> }
>> >>
>> >> +static int sun50i_h616_thermal_init(struct ths_device *tmdev)
>> >> +{
>> >> + int val;
>> >> +
>> >> + /*
>> >> + * T_acq = 20us
>> >> + * clkin = 24MHz
>> >> + *
>> >> + * x = T_acq * clkin - 1
>> >> + * = 479
>> >> + */
>> >> + regmap_write(tmdev->regmap, SUN50I_THS_CTRL0,
>> >> + SUN8I_THS_CTRL0_T_ACQ0(47) |
>> SUN8I_THS_CTRL2_T_ACQ1(479));
>> >
>> > So this whole function is the same as the H6 (diff it!), except
>> this
>> > line.
>> > Which is actually also the same, just written differently (47 ==
>> > 0x2f).
>> > Can you please double check this, and if you agree, remove the
>> whole
>> > function and just use the H6 version?
>> They are not the same. Yes the 47 can be replaced with the unknown
>> value from H6.
>> What isnt the same is the CTRL at the end. CTRL0 in H6 and CTRL2 in
>> H616. A very
>> small change :)
>
> But the definitions of SUN8I_THS_CTRL2_T_ACQ1 and
> SUN50I_THS_CTRL0_T_ACQ
> are the same, aren't they?
oh my yea you are absolutely correct. I will test if they produce the
same bit pattern.
If so i will drop this function completely :)
> I wonder if we are missing one piece of info for the H6 here, so the
> "magic" 47 isn't actually magic, but there is some formula involving
> clocks, similar to the one that results in 479.
> Because then this would be more similar across the different SoCs:
> there
> are *two* timing values for each SoC, and they just happen to be the
> same
> for the H3 (20us), but are different for the H6 and H616 (one 2us, the
> other 20us). And then your H616 line would make sense for the H6 as
> well:
> 2us 20us
> SUN8I_THS_CTRL0_T_ACQ0(47) | SUN8I_THS_CTRL2_T_ACQ1(479));
>
> In any case I still believe the H6 line results in the very same bit
> pattern than your H616 line, but I am happy to stand corrected.
> In which case I would ask you to unify the code anyway, somehow, so
> you
> better hope I am right ;-)
>
> Cheers,
> Andre
>
>> >> + /* average over 4 samples */
>> >> + regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC,
>> >> + SUN50I_THS_FILTER_EN |
>> >> + SUN50I_THS_FILTER_TYPE(1));
>> >> + /*
>> >> + * clkin = 24MHz
>> >> + * filter_samples = 4
>> >> + * period = 0.25s
>> >> + *
>> >> + * x = period * clkin / 4096 / filter_samples - 1
>> >> + * = 365
>> >> + */
>> >> + regmap_write(tmdev->regmap, SUN50I_H6_THS_PC,
>> >> + SUN50I_H6_THS_PC_TEMP_PERIOD(365));
>> >> + /* enable sensor */
>> >> + val = GENMASK(tmdev->chip->sensor_num - 1, 0);
>> >> + regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val);
>> >> + /* thermal data interrupt enable */
>> >> + val = GENMASK(tmdev->chip->sensor_num - 1, 0);
>> >> + regmap_write(tmdev->regmap, SUN50I_H6_THS_DIC, val);
>> >> +
>> >> + return 0;
>> >> +}
>> >> +
>> >> static int sun8i_ths_register(struct ths_device *tmdev)
>> >> {
>> >> int i;
>> >> @@ -608,6 +709,19 @@ static const struct ths_thermal_chip
>> >> sun50i_h6_ths = {
>> >> .calc_temp = sun8i_ths_calc_temp,
>> >> };
>> >>
>> >> +static const struct ths_thermal_chip sun50i_h616_ths = {
>> >> + .sensor_num = 4,
>> >> + .has_bus_clk_reset = true,
>> >> + .ft_deviation = 8000,
>> >> + .offset = -3255,
>> >> + .scale = -81,
>> >> + .temp_data_base = SUN50I_H6_THS_TEMP_DATA,
>> >> + .calibrate = sun50i_h616_ths_calibrate,
>> >> + .init = sun50i_h616_thermal_init,
>> >> + .irq_ack = sun50i_h6_irq_ack,
>> >> + .calc_temp = sun50i_h616_calc_temp,
>> >> +};
>> >> +
>> >> static const struct of_device_id of_ths_match[] = {
>> >> { .compatible = "allwinner,sun8i-a83t-ths", .data =
>> >> &sun8i_a83t_ths },
>> >> { .compatible = "allwinner,sun8i-h3-ths", .data =
>> &sun8i_h3_ths },
>> >> @@ -616,6 +730,7 @@ static const struct of_device_id
>> of_ths_match[]
>> >> = {
>> >> { .compatible = "allwinner,sun50i-a100-ths", .data =
>> >> &sun50i_a100_ths },
>> >> { .compatible = "allwinner,sun50i-h5-ths", .data =
>> &sun50i_h5_ths
>> >> },
>> >> { .compatible = "allwinner,sun50i-h6-ths", .data =
>> &sun50i_h6_ths
>> >> },
>> >> + { .compatible = "allwinner,sun50i-h616-ths", .data =
>> >> &sun50i_h616_ths },
>> >> { /* sentinel */ },
>> >> };
>> >> MODULE_DEVICE_TABLE(of, of_ths_match);
>> >>
>> >
>>
>>
>
Powered by blists - more mailing lists