[PATCH 0/3] Add support for H616 Thermal system

[PATCH 0/3] Add support for H616 Thermal system

[Martin Botka]

Hello,

This patch series adds support to thermal system
found in H616 SoC.

There are 4 thermal sensors in this SoC.
One for GPU, CPU, DRAM and VE.

Trips while unused for now until cpufreq is implemented
(WIP) are required by dt-bindings and thus included here.

Cheers,
Martin

---------------

Hello,
Im very much not sure if the trips should be included or not.
Since they are not optional part I decided to add them but
please let me know.

Cheers,
Martin

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
Martin Botka (3):
      dt-bindings: thermal: sun8i: Add binding for H616 THS controller
      thermal: sun8i: Add support for H616 THS controller
      arm64: dts: allwinner: h616: Add thermal sensor and thermal zones

 .../bindings/thermal/allwinner,sun8i-a83t-ths.yaml |   3 +
 arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi     |  87 ++++++++++++++++
 drivers/thermal/sun8i_thermal.c                    | 115 +++++++++++++++++++++
 3 files changed, 205 insertions(+)
---
base-commit: a25793039a9cd5ac67d38a86dd2eb414abb93aa6
change-id: 20230815-ths-h616-77b2db565249

Best regards,
-- 
Martin Botka <martin.botka@somainline.org>

[PATCH 1/3] dt-bindings: thermal: sun8i: Add binding for H616 THS controller

[Martin Botka]

Add binding for H616 THS controller.

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 .../devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml          | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
index fbd4212285e2..79692f8360f5 100644
--- a/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
+++ b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
@@ -20,6 +20,7 @@ properties:
       - allwinner,sun50i-a100-ths
       - allwinner,sun50i-h5-ths
       - allwinner,sun50i-h6-ths
+      - allwinner,sun50i-h616-ths
 
   clocks:
     minItems: 1
@@ -63,6 +64,7 @@ allOf:
             enum:
               - allwinner,sun50i-a100-ths
               - allwinner,sun50i-h6-ths
+              - allwinner,sun50i-h616-ths
 
     then:
       properties:
@@ -107,6 +109,7 @@ allOf:
               - allwinner,sun50i-a100-ths
               - allwinner,sun50i-h5-ths
               - allwinner,sun50i-h6-ths
+              - allwinner,sun50i-h616-ths
 
     then:
       required:

-- 
2.41.0

Re: [PATCH 1/3] dt-bindings: thermal: sun8i: Add binding for H616 THS controller

[Andre Przywara]

On Fri, 18 Aug 2023 10:43:16 +0200
Martin Botka <martin.botka@somainline.org> wrote:

> Add binding for H616 THS controller.

You could add:
This controller is similar to the H6, but covers four sensors and uses
slightly different calibration methods.

One minor question below:

> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---
>  .../devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml          | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> index fbd4212285e2..79692f8360f5 100644
> --- a/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> +++ b/Documentation/devicetree/bindings/thermal/allwinner,sun8i-a83t-ths.yaml
> @@ -20,6 +20,7 @@ properties:
>        - allwinner,sun50i-a100-ths
>        - allwinner,sun50i-h5-ths
>        - allwinner,sun50i-h6-ths
> +      - allwinner,sun50i-h616-ths
>  
>    clocks:
>      minItems: 1
> @@ -63,6 +64,7 @@ allOf:
>              enum:
>                - allwinner,sun50i-a100-ths
>                - allwinner,sun50i-h6-ths
> +              - allwinner,sun50i-h616-ths
>  
>      then:
>        properties:
> @@ -107,6 +109,7 @@ allOf:
>                - allwinner,sun50i-a100-ths
>                - allwinner,sun50i-h5-ths
>                - allwinner,sun50i-h6-ths
> +              - allwinner,sun50i-h616-ths

I wonder if this list can be negated. Because what this currently says is:
"Every controller except the A83T one requires clocks and resets."
Can we write this more explicitly?

Regardless:

Reviewed-by: Andre Przywara <andre.przywara@arm.com>

Cheers,
Andre


>  
>      then:
>        required:
> 

Re: [PATCH 1/3] dt-bindings: thermal: sun8i: Add binding for H616 THS controller

[Krzysztof Kozlowski]

On 18/08/2023 10:43, Martin Botka wrote:
> Add binding for H616 THS controller.
> 
> Signed-off-by: Martin Botka <martin.botka@somainline.org>
> ---

Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

[PATCH 2/3] thermal: sun8i: Add support for H616 THS controller

[Martin Botka]

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;
+}
+
 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)
+{
+	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.
+	 */
+	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);
+		else
+			reg = (int)caldata[i + 1] & TEMP_CALIB_MASK;
+
+		delta = (ft_temp * 100 - tmdev->chip->calc_temp(tmdev, i, reg))
+			/ 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),
+				   0xfff << offset,
+				   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));
+	/* 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);

-- 
2.41.0

Re: [PATCH 2/3] thermal: sun8i: Add support for H616 THS controller

[Andre Przywara]

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?
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?

> +}
> +
>  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

> +{
> +	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

> +	 */
> +	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.

> +		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;

(And this would be the place where you adjust the calculation to use the
new scale value).

> +		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.

> +				   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?

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);
> 

Re: [PATCH 2/3] thermal: sun8i: Add support for H616 THS controller

[Martin Botka]

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.
> 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 :)

Cheers,
Martin
> 
> 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);
>> 
> 

Re: [PATCH 2/3] thermal: sun8i: Add support for H616 THS controller

[Andre Przywara]

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.

> > 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?
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);
> >>   
> >   
> 
> 

Re: [PATCH 2/3] thermal: sun8i: Add support for H616 THS controller

[Martin Botka]

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);
>>  >>
>>  >
>> 
>> 
> 

[PATCH 3/3] arm64: dts: allwinner: h616: Add thermal sensor and thermal zones

[Martin Botka]

There are 4 thermal sensors:
- CPU
- GPU
- VE
- DRAM

Add the thermal sensor configuration and thermal zones

Signed-off-by: Martin Botka <martin.botka@somainline.org>
---
 arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi | 87 ++++++++++++++++++++++++++
 1 file changed, 87 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
index d549d277d972..063db9634e5f 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h616.dtsi
@@ -9,6 +9,7 @@
 #include <dt-bindings/clock/sun6i-rtc.h>
 #include <dt-bindings/reset/sun50i-h616-ccu.h>
 #include <dt-bindings/reset/sun50i-h6-r-ccu.h>
+#include <dt-bindings/thermal/thermal.h>
 
 / {
 	interrupt-parent = <&gic>;
@@ -138,6 +139,10 @@ sid: efuse@3006000 {
 			reg = <0x03006000 0x1000>;
 			#address-cells = <1>;
 			#size-cells = <1>;
+
+			ths_calibration: thermal-sensor-calibration@14 {
+				reg = <0x14 0x8>;
+			};
 		};
 
 		watchdog: watchdog@30090a0 {
@@ -511,6 +516,18 @@ mdio0: mdio {
 			};
 		};
 
+		ths: thermal-sensor@5070400 {
+			compatible = "allwinner,sun50i-h616-ths";
+			reg = <0x05070400 0x400>;
+			interrupts = <GIC_SPI 19 IRQ_TYPE_LEVEL_HIGH>;
+			clocks = <&ccu CLK_BUS_THS>;
+			clock-names = "bus";
+			resets = <&ccu RST_BUS_THS>;
+			nvmem-cells = <&ths_calibration>;
+			nvmem-cell-names = "calibration";
+			#thermal-sensor-cells = <1>;
+		};
+
 		usbotg: usb@5100000 {
 			compatible = "allwinner,sun50i-h616-musb",
 				     "allwinner,sun8i-h3-musb";
@@ -755,4 +772,74 @@ r_rsb: rsb@7083000 {
 			#size-cells = <0>;
 		};
 	};
+
+	thermal-zones {
+		cpu-thermal {
+			polling-delay-passive = <500>;
+			polling-delay = <1000>;
+			thermal-sensors = <&ths 2>;
+			sustainable-power = <1000>;
+
+			trips {
+				cpu_threshold: cpu-trip-0 {
+					temperature = <60000>;
+					type = "passive";
+					hysteresis = <0>;
+				};
+				cpu_target: cpu-trip-1 {
+					temperature = <70000>;
+					type = "passive";
+					hysteresis = <0>;
+				};
+				cpu_critical: cpu-trip-2 {
+					temperature = <110000>;
+					type = "critical";
+					hysteresis = <0>;
+				};
+			};
+		};
+
+		gpu-thermal {
+			polling-delay-passive = <500>;
+			polling-delay = <1000>;
+			thermal-sensors = <&ths 0>;
+			sustainable-power = <1100>;
+
+			trips {
+				gpu_temp_critical: gpu-trip-0 {
+					temperature = <110000>;
+					type = "critical";
+					hysteresis = <0>;
+				};
+			};
+		};
+
+		ve-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&ths 1>;
+
+			trips {
+				ve_temp_critical: ve-trip-0 {
+					temperature = <110000>;
+					type = "critical";
+					hysteresis = <0>;
+				};
+			};
+		};
+
+		ddr-thermal {
+			polling-delay-passive = <0>;
+			polling-delay = <0>;
+			thermal-sensors = <&ths 3>;
+
+			trips {
+				ddr_temp_critical: ddr-trip-0 {
+					temperature = <110000>;
+					type = "critical";
+					hysteresis = <0>;
+				};
+			};
+		};
+	};
 };

-- 
2.41.0