[PATCH v2 0/2] thermal: rcar_gen3_thermal: Add support for trip points

[PATCH v2 0/2] thermal: rcar_gen3_thermal: Add support for trip points

[Niklas Söderlund]

Hi Daniel,

This small series adds support for the .set_trips() to the
rcar_gen3_thermal driver on the platforms that supports it.

Patch 1/2 adds the actual trip point support while patch 2/2 is a drive-by fix of a
datatype found while working on this feature.

The work is based on-top of v5.14-rc1 and tested on M3-N and V3U
without any regressions or other issues.

Niklas Söderlund (2):
  thermal: rcar_gen3_thermal: Add support for hardware trip points
  thermal: rcar_gen3_thermal: Store TSC id as unsigned int

 drivers/thermal/rcar_gen3_thermal.c | 110 ++++++++++++++++++++++++++--
 1 file changed, 103 insertions(+), 7 deletions(-)

-- 
2.32.0

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

[Niklas Söderlund]

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

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

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

[Daniel Lezcano]

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.



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Niklas Söderlund]

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?

-- 
Kind Regards,
Niklas Söderlund

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

[Daniel Lezcano]

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



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

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

[Niklas Söderlund]

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?

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

-- 
Kind Regards,
Niklas Söderlund

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

[Daniel Lezcano]

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



-- 
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[PATCH v2 2/2] thermal: rcar_gen3_thermal: Store TSC id as unsigned int

[Niklas Söderlund]

The TSC id and number of TSC ids should be stored as unsigned int as
they can't be negative. Fix the datatype of the loop counter 'i' and
rcar_gen3_thermal_tsc.id to reflect this.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
* Changes since v1
- Fix rcar_gen3_thermal_tsc.id and dev_info() as pointed out by Geert.
- Update commit message to better capture the new data members pointed
  out in v1.
---
 drivers/thermal/rcar_gen3_thermal.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index e49593437edeb3ca..85228d308dd35b19 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -84,7 +84,7 @@ struct rcar_gen3_thermal_tsc {
 	struct thermal_zone_device *zone;
 	struct equation_coefs coef;
 	int tj_t;
-	int id; /* thermal channel id */
+	unsigned int id; /* thermal channel id */
 };
 
 struct rcar_gen3_thermal_priv {
@@ -398,7 +398,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 	const int *ths_tj_1 = of_device_get_match_data(dev);
 	struct resource *res;
 	struct thermal_zone_device *zone;
-	int ret, i;
+	unsigned int i;
+	int ret;
 
 	/* default values if FUSEs are missing */
 	/* TODO: Read values from hardware on supported platforms */
@@ -467,7 +468,7 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		if (ret < 0)
 			goto error_unregister;
 
-		dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret);
+		dev_info(dev, "TSC%u: Loaded %d trip points\n", i, ret);
 	}
 
 	priv->num_tscs = i;
-- 
2.32.0