Messages in this thread | | | Date | Fri, 18 Aug 2023 14:25:32 +0200 | From | Martin Botka <> | 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@arm.com> wrote: > On Fri, 18 Aug 2023 12:54:48 +0200 > Martin Botka <martin.botka@somainline.org> wrote: > > Hi Martin, > >> On Fri, Aug 18 2023 at 11:29:30 AM +01:00:00, Andre Przywara >> <andre.przywara@arm.com> wrote: >> > On Fri, 18 Aug 2023 10:43:17 +0200 >> > Martin Botka <martin.botka@somainline.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@somainline.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); >> >> >> > >> >> >
| |