diff mbox series

[v2,1/2] thermal: rcar_gen3_thermal: Add support for hardware trip points

Message ID 20210804091818.2196806-2-niklas.soderlund+renesas@ragnatech.se
State Accepted
Commit 47cf09e0f4fc51200ecea15387bec3584562f55d
Headers show
Series thermal: rcar_gen3_thermal: Add support for trip points | expand

Commit Message

Niklas Söderlund Aug. 4, 2021, 9:18 a.m. UTC
All supported hardware except V3U is capable of generating interrupts
to the CPU when the temperature go below or above a set value. Use this
to implement support for the set_trip() feature of the thermal core on
supported hardware.

The V3U have its interrupts routed to the ECM module and therefore can
not be used to implement set_trip() as the driver can't be made aware of
when the interrupt triggers.

Each TSC is capable of tracking up-to three different temperatures while
only two are needed to implement the tracking of the thermal window.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Remove the 'have_irq' flag from the OF match data and auto-detect if
  interrupts are available using platform_get_irq_optional().
- Have a non-static thermal_zone_of_device_ops and clear the .set_trips
  if interrupts are unavailable.
---
 drivers/thermal/rcar_gen3_thermal.c | 103 ++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 4 deletions(-)

Comments

Daniel Lezcano July 6, 2022, 11:13 a.m. UTC | #1
Hi Niklas,


On 04/08/2021 11:18, Niklas Söderlund wrote:
> All supported hardware except V3U is capable of generating interrupts
> to the CPU when the temperature go below or above a set value. Use this
> to implement support for the set_trip() feature of the thermal core on
> supported hardware.
> 
> The V3U have its interrupts routed to the ECM module and therefore can
> not be used to implement set_trip() as the driver can't be made aware of
> when the interrupt triggers.
> 
> Each TSC is capable of tracking up-to three different temperatures while
> only two are needed to implement the tracking of the thermal window.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
> * Changes since v1
> - Remove the 'have_irq' flag from the OF match data and auto-detect if
>    interrupts are available using platform_get_irq_optional().
> - Have a non-static thermal_zone_of_device_ops and clear the .set_trips
>    if interrupts are unavailable.
> ---

[ ... ]

> @@ -401,8 +492,12 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
>   
>   	for (i = 0; i < priv->num_tscs; i++) {
>   		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
> +		struct thermal_zone_device *zone = tsc->zone;
>   
>   		priv->thermal_init(tsc);
> +		if (zone->ops->set_trips)
> +			rcar_gen3_thermal_set_trips(tsc, zone->prev_low_trip,
> +						    zone->prev_high_trip);
>   	}

While doing a cleanup I lately noticed this change and I've concerns 
about it:

  - it uses the thermal zone internals

  - is it really needed ?

At resume time we have:

thermal_pm_notify()
   --> PM_POST_RESTORE
     --> thermal_zone_device_update()
       --> thermal_zone_set_trips()

In addition, I believe this later call is consistent as it sets the trip 
point based on the last temperature update, while the 
rcar_gen3_thermal_resume() does not.

Was this function added on purpose because some there is an issue when 
resuming the board or just put there assuming it is doing the right thing ?

I would be happy if we can remove this portion of code because it is the 
only users of prev_*_trip I would like to replace by prev_trip id.
Daniel Lezcano July 7, 2022, 9:55 a.m. UTC | #2
Hi Niklas,


On 07/07/2022 11:51, Niklas Söderlund wrote:
> Hi Daniel,
> 
> On 2022-07-06 13:13:44 +0200, Daniel Lezcano wrote:
>>
>> Hi Niklas,
>>
>>
>> On 04/08/2021 11:18, Niklas Söderlund wrote:
>>> All supported hardware except V3U is capable of generating interrupts
>>> to the CPU when the temperature go below or above a set value. Use this
>>> to implement support for the set_trip() feature of the thermal core on
>>> supported hardware.
>>>
>>> The V3U have its interrupts routed to the ECM module and therefore can
>>> not be used to implement set_trip() as the driver can't be made aware of
>>> when the interrupt triggers.
>>>
>>> Each TSC is capable of tracking up-to three different temperatures while
>>> only two are needed to implement the tracking of the thermal window.
>>>
>>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>>> ---
>>> * Changes since v1
>>> - Remove the 'have_irq' flag from the OF match data and auto-detect if
>>>     interrupts are available using platform_get_irq_optional().
>>> - Have a non-static thermal_zone_of_device_ops and clear the .set_trips
>>>     if interrupts are unavailable.
>>> ---
>>
>> [ ... ]
>>
>>> @@ -401,8 +492,12 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
>>>    	for (i = 0; i < priv->num_tscs; i++) {
>>>    		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
>>> +		struct thermal_zone_device *zone = tsc->zone;
>>>    		priv->thermal_init(tsc);
>>> +		if (zone->ops->set_trips)
>>> +			rcar_gen3_thermal_set_trips(tsc, zone->prev_low_trip,
>>> +						    zone->prev_high_trip);
>>>    	}
>>
>> While doing a cleanup I lately noticed this change and I've concerns about
>> it:
>>
>>   - it uses the thermal zone internals
>>
>>   - is it really needed ?
>>
>> At resume time we have:
>>
>> thermal_pm_notify()
>>    --> PM_POST_RESTORE
>>      --> thermal_zone_device_update()
>>        --> thermal_zone_set_trips()
>>
>> In addition, I believe this later call is consistent as it sets the trip
>> point based on the last temperature update, while the
>> rcar_gen3_thermal_resume() does not.
>>
>> Was this function added on purpose because some there is an issue when
>> resuming the board or just put there assuming it is doing the right thing ?
>>
>> I would be happy if we can remove this portion of code because it is the
>> only users of prev_*_trip I would like to replace by prev_trip id.
> 
> 
> This looks like something that should never have been submitted
> upstream. The usage for this was to restore the trip points in the
> hardware registers *after* the hardware have been initialized. However
> as far as I can tell from the code this is already done by the thermal
> core so no need for the driver to deal with this.
> 
> I did a test on a Gen3 board (M3-N) with this code removed and the core
> appears to do the right thing so this code in the driver can be removed.
> Will you write up a patch as part of your cleanup work or would you
> prefer I do it?

Thanks for double checking and confirming. I've a patch removing this 
code, no need to send one. I'll submit it along with other changes 
around this. Perhaps, I'll try a revert before, it would make more sense.

Do you think the 'revert' should be backported ?

   -- Daniel
Daniel Lezcano July 7, 2022, 12:37 p.m. UTC | #3
On 07/07/2022 12:26, Niklas Söderlund wrote:
> Hi Daniel,
> 
> On 2022-07-07 11:55:55 +0200, Daniel Lezcano wrote:
> 
>> Thanks for double checking and confirming. I've a patch removing this code,
>> no need to send one. I'll submit it along with other changes around this.
>> Perhaps, I'll try a revert before, it would make more sense.
> 
> Thanks.
> 
> To be clear I don't think we should revert commit 47cf09e0f4fc5120
> ("thermal/drivers/rcar_gen3_thermal: Add support for hardware trip
> points"). Only remove the 4 lines it adds to rcar_gen3_thermal_resume()
> as they are redundant. Does this match your view of the revert?

Yes

>> Do you think the 'revert' should be backported ?
> 
> I have no strong opinion, I think it's a matter of risk :-)
> 
> There is no real harm in writing the trip points to hardware twice
> during resume. On the other hand if we *know* the thermal core in the
> backported kernel will always call set_trips() after the device is
> resumed, then there is no harm in removing it.

Yes, I agree.

Thanks for your feedback
diff mbox series

Patch

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index fdf16aa34eb470c7..e49593437edeb3ca 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -190,10 +190,64 @@  static int rcar_gen3_thermal_get_temp(void *devdata, int *temp)
 	return 0;
 }
 
-static const struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
+static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc,
+					      int mcelsius)
+{
+	int celsius, val;
+
+	celsius = DIV_ROUND_CLOSEST(mcelsius, 1000);
+	if (celsius <= INT_FIXPT(tsc->tj_t))
+		val = celsius * tsc->coef.a1 + tsc->coef.b1;
+	else
+		val = celsius * tsc->coef.a2 + tsc->coef.b2;
+
+	return INT_FIXPT(val);
+}
+
+static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high)
+{
+	struct rcar_gen3_thermal_tsc *tsc = devdata;
+	u32 irqmsk = 0;
+
+	if (low != -INT_MAX) {
+		irqmsk |= IRQ_TEMPD1;
+		rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1,
+					rcar_gen3_thermal_mcelsius_to_temp(tsc, low));
+	}
+
+	if (high != INT_MAX) {
+		irqmsk |= IRQ_TEMP2;
+		rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2,
+					rcar_gen3_thermal_mcelsius_to_temp(tsc, high));
+	}
+
+	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, irqmsk);
+
+	return 0;
+}
+
+static struct thermal_zone_of_device_ops rcar_gen3_tz_of_ops = {
 	.get_temp	= rcar_gen3_thermal_get_temp,
+	.set_trips	= rcar_gen3_thermal_set_trips,
 };
 
+static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data)
+{
+	struct rcar_gen3_thermal_priv *priv = data;
+	unsigned int i;
+	u32 status;
+
+	for (i = 0; i < priv->num_tscs; i++) {
+		status = rcar_gen3_thermal_read(priv->tscs[i], REG_GEN3_IRQSTR);
+		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQSTR, 0);
+		if (status)
+			thermal_zone_device_update(priv->tscs[i]->zone,
+						   THERMAL_EVENT_UNSPECIFIED);
+	}
+
+	return IRQ_HANDLED;
+}
+
 static const struct soc_device_attribute r8a7795es1[] = {
 	{ .soc_id = "r8a7795", .revision = "ES1.*" },
 	{ /* sentinel */ }
@@ -210,6 +264,9 @@  static void rcar_gen3_thermal_init_r8a7795es1(struct rcar_gen3_thermal_tsc *tsc)
 
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0x3F);
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
+	if (tsc->zone->ops->set_trips)
+		rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN,
+					IRQ_TEMPD1 | IRQ_TEMP2);
 
 	rcar_gen3_thermal_write(tsc, REG_GEN3_CTSR,
 				CTSR_PONM | CTSR_AOUT | CTSR_THBGR | CTSR_VMEN);
@@ -235,6 +292,9 @@  static void rcar_gen3_thermal_init(struct rcar_gen3_thermal_tsc *tsc)
 
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQCTL, 0);
 	rcar_gen3_thermal_write(tsc, REG_GEN3_IRQMSK, 0);
+	if (tsc->zone->ops->set_trips)
+		rcar_gen3_thermal_write(tsc, REG_GEN3_IRQEN,
+					IRQ_TEMPD1 | IRQ_TEMP2);
 
 	reg_val = rcar_gen3_thermal_read(tsc, REG_GEN3_THCTR);
 	reg_val |= THCTR_THSST;
@@ -303,6 +363,34 @@  static void rcar_gen3_hwmon_action(void *data)
 	thermal_remove_hwmon_sysfs(zone);
 }
 
+static int rcar_gen3_thermal_request_irqs(struct rcar_gen3_thermal_priv *priv,
+					  struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	unsigned int i;
+	char *irqname;
+	int ret, irq;
+
+	for (i = 0; i < 2; i++) {
+		irq = platform_get_irq_optional(pdev, i);
+		if (irq < 0)
+			return irq;
+
+		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
+					 dev_name(dev), i);
+		if (!irqname)
+			return -ENOMEM;
+
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						rcar_gen3_thermal_irq,
+						IRQF_ONESHOT, irqname, priv);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
 static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 {
 	struct rcar_gen3_thermal_priv *priv;
@@ -326,6 +414,9 @@  static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 
+	if (rcar_gen3_thermal_request_irqs(priv, pdev))
+		rcar_gen3_tz_of_ops.set_trips = NULL;
+
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
@@ -351,9 +442,6 @@  static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 
 		priv->tscs[i] = tsc;
 
-		priv->thermal_init(tsc);
-		rcar_gen3_thermal_calc_coefs(tsc, ptat, thcodes[i], *ths_tj_1);
-
 		zone = devm_thermal_zone_of_sensor_register(dev, i, tsc,
 							    &rcar_gen3_tz_of_ops);
 		if (IS_ERR(zone)) {
@@ -363,6 +451,9 @@  static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		}
 		tsc->zone = zone;
 
+		priv->thermal_init(tsc);
+		rcar_gen3_thermal_calc_coefs(tsc, ptat, thcodes[i], *ths_tj_1);
+
 		tsc->zone->tzp->no_hwmon = false;
 		ret = thermal_add_hwmon_sysfs(tsc->zone);
 		if (ret)
@@ -401,8 +492,12 @@  static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev)
 
 	for (i = 0; i < priv->num_tscs; i++) {
 		struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i];
+		struct thermal_zone_device *zone = tsc->zone;
 
 		priv->thermal_init(tsc);
+		if (zone->ops->set_trips)
+			rcar_gen3_thermal_set_trips(tsc, zone->prev_low_trip,
+						    zone->prev_high_trip);
 	}
 
 	return 0;