Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Andre Przywara]

On Wed, 13 Dec 2023 12:41:29 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

Hi Maksim,

> Hello, again!
> 
> On 28.11.2023 03:58, Andre Przywara wrote:
> > From: Martin Botka <martin.botka@somainline.org>
> >
> > Add support for the thermal sensor found in H616 SoCs, which slightly
> > resembles the H6 thermal sensor controller, with a few changes like
> > four sensors.
> > Also the registers readings are wrong, unless a bit in the first SYS_CFG
> > register cleared, so set needs_syscon to trigger that code
> >
> > Signed-off-by: Martin Botka <martin.botka@somainline.org>
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> > ---
> > drivers/thermal/sun8i_thermal.c | 73 +++++++++++++++++++++++++++++++++
> > 1 file changed, 73 insertions(+)
> >
> > diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
> > index 920e419ce7343..9a404fa9d76a9 100644
> > --- a/drivers/thermal/sun8i_thermal.c
> > +++ b/drivers/thermal/sun8i_thermal.c
> > @@ -280,6 +280,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)
> > +{
> > + 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 degree celsius.
> > + */
> > + ft_temp = caldata[0] & FT_TEMP_MASK;
> > +
> > + for (i = 0; i < tmdev->chip->sensor_num; i++) {
> > + int delta, cdata, offset, reg, temp;
> > +
> > + if (i == 3)
> > + reg = (caldata[1] >> 12)
> > + | ((caldata[2] >> 12) << 4)
> > + | ((caldata[3] >> 12) << 8);
> > + else
> > + reg = (int)caldata[i + 1] & TEMP_CALIB_MASK;
> > +
> > + temp = tmdev->chip->calc_temp(tmdev, i, reg);
> > + delta = ((temp - ft_temp * 100) * 10) / tmdev->chip->scale;
> > + 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),
> > + TEMP_CALIB_MASK << offset,
> > + cdata << offset);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > static int sun8i_ths_calibrate(struct ths_device *tmdev)
> > {
> > struct nvmem_cell *calcell;
> > @@ -659,6 +717,20 @@ 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,
> > + .needs_syscon = true,
> > + .ft_deviation = 8000,
> > + .offset = 263655,
> > + .scale = 810,  
> 
> Can I ask you, how did you get the offset and the scale values?
> 
> In the H616 user manual (3.10.3.4. THS Temperature Conversion Formula)
> we can find this formula:
> 
> T = (sensor_data - 3255)/(-12.401)
> 
> I calculated offset and scale values, and this is what I got:
> 
> scale = 1 / 12.401 = 806,38658173 ~ 806
> offset = 3255 / 12.401 = 262478,832352 ~ 262479
> 
> Let me assume that you got offset and scale values from Tina Linux
> Here for example
> https://github.com/Tina-Linux/linux-5.4/blob/ff6b1361f7041be75d4486559141e13134d164ef/drivers/thermal/sunxi_thermal.c#L373
> 
> In Tina Linux the scale is rounded to 81 (that equals to 810 in
> mainline driver).
> And looks like you used this rounded value to recalculate the offset:
> 3255 * 81 = 263655
> 
> I would be glad to know your thoughts about this.

Yes, this submission was based on some BSP code, from there we calculated
the values to match the formula used in mainline, so this would include
the rounding error, as your rightly figured.

I don't know if the manual is any more accurate than the BSP data, but
your calculations above make sense, and would explain the difference.

I will use your new values in the next version.

Thanks,
Andre

> > + .temp_data_base = SUN50I_H6_THS_TEMP_DATA,
> > + .calibrate = sun50i_h616_ths_calibrate,
> > + .init = sun50i_h6_thermal_init,
> > + .irq_ack = sun50i_h6_irq_ack,
> > + .calc_temp = sun8i_ths_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 },
> > @@ -667,6 +739,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);  
> 
> Best regards,
> Maksim
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 0/6] Add support for H616 Thermal system

[Andre Przywara]

Hi,

this is v3 of the series by Martin, now complemented by patches to fix
the problem with way-too-high temperatures reported on some boards. This
seems to be related to the firmware being run, because the vendor U-Boot
contains a hack avoiding this problem. Some investigation and digging
in BSP code later we identified that bit 16 in register 0x3000000
(SYS_CFG) needs to be cleared for the raw temperature register values to
contain reasonable values.
To achieve this, patch 1/6 exports this very register from the already
existing syscon device. Patch 4/6 then adds code to the thermal driver
to find the syscon device via a new DT property, and query its regmap to
clear bit 16 in there.

I am not fully convinced this is the best solution, but it works for me.
What leaves a bit of a bitter taste is that the SRAM driver (the one
exporting the regmap) also uses this register, to switch some SRAM C
region to the video engine (VE). Experiments show that only bit 0 in
this register is doing this job, so the current mask covering the 31
LSBs should probably be amended to only cover bit 0.
Another solution could be to model this bit as an SRAM switch, and let
the THS driver claim some (dummy?) SRAM region from the syscon/SRAM driver
directly. While this sounds cleaner to some degree, I don't think there
is really such a THS SRAM region, so it's not fully correct either.

I would appreciate any feedback on this, happy to implement the other
approach, if that's desired.

The rest of the patches is mostly unchanged from Martin's v2, just
updated and massaged the commit messages a bit. I also added patch 3/6
to document some so-far unknown register value.

Please have a look!

Cheers,
Andre

Changelog v2 .. v3:
- rebase on top of v6.7-rc3
- add patches to clear bit 16 in SYS_CFG register 0x3000000
- add syscon to the binding documentation
- add patch explaining the unknown control register value

Changelog v1 .. v2:
- Fix typos
- Remove h616 calc and init functions
- Use TEMP_CALIB_MASK insteaf of 0xffff
- Adjust calibration function to new offset and scale
- Add proper comment to bindings patch
- Split delta calculations to 2 lines
- Add parentheses around caldata[2|3] >> 12
- Negate bindings if for clocks

Andre Przywara (3):
  soc: sunxi: sram: export register 0 for THS on H616
  thermal: sun8i: explain unknown H6 register value
  thermal: sun8i: add syscon register access code

Martin Botka (3):
  dt-bindings: thermal: sun8i: Add H616 THS controller
  thermal: sun8i: add support for H616 THS controller
  arm64: dts: allwinner: h616: Add thermal sensor and zones

 .../thermal/allwinner,sun8i-a83t-ths.yaml     |  30 ++--
 .../arm64/boot/dts/allwinner/sun50i-h616.dtsi |  88 ++++++++++
 drivers/soc/sunxi/sunxi_sram.c                |   5 +
 drivers/thermal/sun8i_thermal.c               | 152 ++++++++++++++++--
 4 files changed, 252 insertions(+), 23 deletions(-)

-- 
2.35.8


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Andre Przywara]

From: Martin Botka <martin.botka@somainline.org>

Add support for the thermal sensor found in H616 SoCs, which slightly
resembles the H6 thermal sensor controller, with a few changes like
four sensors.
Also the registers readings are wrong, unless a bit in the first SYS_CFG
register cleared, so set needs_syscon to trigger that code

Signed-off-by: Martin Botka <martin.botka@somainline.org>
Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/thermal/sun8i_thermal.c | 73 +++++++++++++++++++++++++++++++++
 1 file changed, 73 insertions(+)

diff --git a/drivers/thermal/sun8i_thermal.c b/drivers/thermal/sun8i_thermal.c
index 920e419ce7343..9a404fa9d76a9 100644
--- a/drivers/thermal/sun8i_thermal.c
+++ b/drivers/thermal/sun8i_thermal.c
@@ -280,6 +280,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)
+{
+	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 degree celsius.
+	 */
+	ft_temp = caldata[0] & FT_TEMP_MASK;
+
+	for (i = 0; i < tmdev->chip->sensor_num; i++) {
+		int delta, cdata, offset, reg, temp;
+
+		if (i == 3)
+			reg = (caldata[1] >> 12)
+			      | ((caldata[2] >> 12) << 4)
+			      | ((caldata[3] >> 12) << 8);
+		else
+			reg = (int)caldata[i + 1] & TEMP_CALIB_MASK;
+
+		temp = tmdev->chip->calc_temp(tmdev, i, reg);
+		delta = ((temp - ft_temp * 100) * 10) / tmdev->chip->scale;
+		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),
+				   TEMP_CALIB_MASK << offset,
+				   cdata << offset);
+	}
+
+	return 0;
+}
+
 static int sun8i_ths_calibrate(struct ths_device *tmdev)
 {
 	struct nvmem_cell *calcell;
@@ -659,6 +717,20 @@ 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,
+	.needs_syscon = true,
+	.ft_deviation = 8000,
+	.offset = 263655,
+	.scale = 810,
+	.temp_data_base = SUN50I_H6_THS_TEMP_DATA,
+	.calibrate = sun50i_h616_ths_calibrate,
+	.init = sun50i_h6_thermal_init,
+	.irq_ack = sun50i_h6_irq_ack,
+	.calc_temp = sun8i_ths_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 },
@@ -667,6 +739,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);
-- 
2.35.8


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Maksim Kiselev]

Hi Martin, Andre.

May I inquire? Why do we need a separate sun50i_h616_ths_calibrate() 
function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?

At my glance the calculations in both functions are the same. We just 
need to handle a special case for the 4th sensor.

Best regards,
Maksim


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Andre Przywara]

On Sat, 9 Dec 2023 13:44:34 +0300
Maksim Kiselev <bigunclemax@gmail.com> wrote:

Hi Maksim,

> Hi Martin, Andre.
> 
> May I inquire? Why do we need a separate sun50i_h616_ths_calibrate() 
> function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?
> 
> At my glance the calculations in both functions are the same. We just 
> need to handle a special case for the 4th sensor.

You seem to be right, they are indeed the same, just written slightly
differently. Do you already have any patches that unify that?
I don't know if Martin or I find time to do it this week, but we could
also optimise this later.

Cheers,
Andre

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Maxim Kiselev]

пн, 11 дек. 2023 г. в 02:48, Andre Przywara <andre.przywara@arm.com>:
>
> On Sat, 9 Dec 2023 13:44:34 +0300
> Maksim Kiselev <bigunclemax@gmail.com> wrote:
>
> Hi Maksim,
>
> > Hi Martin, Andre.
> >
> > May I inquire? Why do we need a separate sun50i_h616_ths_calibrate()
> > function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?
> >
> > At my glance the calculations in both functions are the same. We just
> > need to handle a special case for the 4th sensor.
>
> You seem to be right, they are indeed the same, just written slightly
> differently. Do you already have any patches that unify that?

No, I don't have any patches for that yet. But I can do it if you told
me where to
send the patch. Should I put it here as a reply?

> I don't know if Martin or I find time to do it this week, but we could
> also optimise this later.
>
> Cheers,
> Andre

Best regards,
Maksim

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Andre Przywara]

On Tue, 12 Dec 2023 21:09:45 +0300
Maxim Kiselev <bigunclemax@gmail.com> wrote:

Hi Maksim,

sorry for the delay.

> пн, 11 дек. 2023 г. в 02:48, Andre Przywara <andre.przywara@arm.com>:
> >
> > On Sat, 9 Dec 2023 13:44:34 +0300
> > Maksim Kiselev <bigunclemax@gmail.com> wrote:
> >
> > Hi Maksim,
> >  
> > > Hi Martin, Andre.
> > >
> > > May I inquire? Why do we need a separate sun50i_h616_ths_calibrate()
> > > function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?
> > >
> > > At my glance the calculations in both functions are the same. We just
> > > need to handle a special case for the 4th sensor.  
> >
> > You seem to be right, they are indeed the same, just written slightly
> > differently. Do you already have any patches that unify that?  
> 
> No, I don't have any patches for that yet. But I can do it if you told
> me where to
> send the patch. Should I put it here as a reply?

Can you make one patch on top of mainline, that prepares the existing
sun50i_h6_ths_calibrate() function to deal with four sensors? I would then
include this patch of yours in the next submission, and put Martin's H616
patch (now probably just the struct ths_thermal_chip) on top then.

Please send this patch just to the list (CC:ing people like Martin and
me), starting a new thread.

Many thanks,
Andre

> > I don't know if Martin or I find time to do it this week, but we could
> > also optimise this later.
> >
> > Cheers,
> > Andre  
> 
> Best regards,
> Maksim


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 5/6] thermal: sun8i: add support for H616 THS controller

[Maxim Kiselev]

Hi Andre,

чт, 14 дек. 2023 г. в 13:59, Andre Przywara <andre.przywara@arm.com>:
>
> On Tue, 12 Dec 2023 21:09:45 +0300
> Maxim Kiselev <bigunclemax@gmail.com> wrote:
>
> Hi Maksim,
>
> sorry for the delay.

Now it's my turn to apologize for the delay :)

>
> > пн, 11 дек. 2023 г. в 02:48, Andre Przywara <andre.przywara@arm.com>:
> > >
> > > On Sat, 9 Dec 2023 13:44:34 +0300
> > > Maksim Kiselev <bigunclemax@gmail.com> wrote:
> > >
> > > Hi Maksim,
> > >
> > > > Hi Martin, Andre.
> > > >
> > > > May I inquire? Why do we need a separate sun50i_h616_ths_calibrate()
> > > > function? Why can't we just extend an existing sun50i_h6_ths_calibrate()?
> > > >
> > > > At my glance the calculations in both functions are the same. We just
> > > > need to handle a special case for the 4th sensor.
> > >
> > > You seem to be right, they are indeed the same, just written slightly
> > > differently. Do you already have any patches that unify that?
> >
> > No, I don't have any patches for that yet. But I can do it if you told
> > me where to
> > send the patch. Should I put it here as a reply?
>
> Can you make one patch on top of mainline, that prepares the existing
> sun50i_h6_ths_calibrate() function to deal with four sensors? I would then
> include this patch of yours in the next submission, and put Martin's H616
> patch (now probably just the struct ths_thermal_chip) on top then.
>
> Please send this patch just to the list (CC:ing people like Martin and
> me), starting a new thread.

You can find my patch here:
https://lore.kernel.org/all/20231217133637.54773-1-bigunclemax@gmail.com/

Best regards,
Maksim

> Many thanks,
> Andre
>
> > > I don't know if Martin or I find time to do it this week, but we could
> > > also optimise this later.
> > >
> > > Cheers,
> > > Andre
> >
> > Best regards,
> > Maksim
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel