[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

[thermal: thermal/next] thermal/drivers/rcar_gen3_thermal: Add support for hardware trip points

[thermal-bot for Niklas Söderlund]

The following commit has been merged into the thermal/next branch of thermal:

Commit-ID:     47cf09e0f4fc51200ecea15387bec3584562f55d
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git//47cf09e0f4fc51200ecea15387bec3584562f55d
Author:        Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
AuthorDate:    Wed, 04 Aug 2021 11:18:17 +02:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Sat, 14 Aug 2021 12:48:00 +02:00

thermal/drivers/rcar_gen3_thermal: Add support for hardware trip points

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>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20210804091818.2196806-2-niklas.soderlund+renesas@ragnatech.se
---
 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 fdf16aa..e495934 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;

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

[thermal: thermal/next] thermal/drivers/rcar_gen3_thermal: Store TSC id as unsigned int

[thermal-bot for Niklas Söderlund]

The following commit has been merged into the thermal/next branch of thermal:

Commit-ID:     d3a2328e741bf6e9e6bda750e0a63832fa365a74
Gitweb:        https://git.kernel.org/pub/scm/linux/kernel/git/thermal/linux.git//d3a2328e741bf6e9e6bda750e0a63832fa365a74
Author:        Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
AuthorDate:    Wed, 04 Aug 2021 11:18:18 +02:00
Committer:     Daniel Lezcano <daniel.lezcano@linaro.org>
CommitterDate: Sat, 14 Aug 2021 12:49:05 +02:00

thermal/drivers/rcar_gen3_thermal: Store TSC id as unsigned int

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>
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
Link: https://lore.kernel.org/r/20210804091818.2196806-3-niklas.soderlund+renesas@ragnatech.se
---
 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 e495934..85228d3 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;