| Message ID | 20210804091818.2196806-2-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Daniel Lezcano |
| Headers | show |
| Series | thermal: rcar_gen3_thermal: Add support for trip points | expand |
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.
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?
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
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.
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 --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;
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(-)