* [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes @ 2020-02-12 22:49 Niklas Söderlund 2020-02-13 8:19 ` Yoshihiro Shimoda 2020-02-26 9:05 ` Kieran Bingham 0 siblings, 2 replies; 8+ messages in thread From: Niklas Söderlund @ 2020-02-12 22:49 UTC (permalink / raw) To: linux-pm, linux-renesas-soc; +Cc: Yoshihiro Shimoda, Niklas Söderlund The desired behavior of the driver is to generate an interrupt and call thermal_zone_device_update() as soon as the temperature have changed more then one degree. When the set_trips operation was implemented it was believed that the trip window set by the framework would move around the current temperature and the hysteresis value described in devicetree. The behavior of the framework is however to set a window based on the trip points described in devicetree. Remove the set_trips operation which was not used correctly and update the temperatures that triggers interrupts directly from the interrupt handler. Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> --- drivers/thermal/rcar_gen3_thermal.c | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c index 72877bdc072daaed..55d1736f532cdb33 100644 --- a/drivers/thermal/rcar_gen3_thermal.c +++ b/drivers/thermal/rcar_gen3_thermal.c @@ -81,8 +81,6 @@ struct rcar_gen3_thermal_tsc { void __iomem *base; struct thermal_zone_device *zone; struct equation_coefs coef; - int low; - int high; int tj_t; int id; /* thermal channel id */ }; @@ -204,12 +202,14 @@ static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc, return INT_FIXPT(val); } -static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) +static int rcar_gen3_thermal_update_range(struct rcar_gen3_thermal_tsc *tsc) { - struct rcar_gen3_thermal_tsc *tsc = devdata; + int temperature, low, high; - low = clamp_val(low, -40000, 120000); - high = clamp_val(high, -40000, 120000); + rcar_gen3_thermal_get_temp(tsc, &temperature); + + low = temperature - MCELSIUS(1); + high = temperature + MCELSIUS(1); rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1, rcar_gen3_thermal_mcelsius_to_temp(tsc, low)); @@ -217,15 +217,11 @@ static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2, rcar_gen3_thermal_mcelsius_to_temp(tsc, high)); - tsc->low = low; - tsc->high = high; - return 0; } static const 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 void rcar_thermal_irq_set(struct rcar_gen3_thermal_priv *priv, bool on) @@ -246,9 +242,11 @@ static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data) 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) + if (status) { + rcar_gen3_thermal_update_range(priv->tscs[i]); thermal_zone_device_update(priv->tscs[i]->zone, THERMAL_EVENT_UNSPECIFIED); + } } return IRQ_HANDLED; @@ -454,6 +452,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev) if (ret < 0) goto error_unregister; + rcar_gen3_thermal_update_range(tsc); + dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret); } @@ -492,7 +492,7 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev) struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i]; priv->thermal_init(tsc); - rcar_gen3_thermal_set_trips(tsc, tsc->low, tsc->high); + rcar_gen3_thermal_update_range(tsc); } rcar_thermal_irq_set(priv, true); -- 2.25.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* RE: [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes 2020-02-12 22:49 [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes Niklas Söderlund @ 2020-02-13 8:19 ` Yoshihiro Shimoda 2020-02-26 9:05 ` Kieran Bingham 1 sibling, 0 replies; 8+ messages in thread From: Yoshihiro Shimoda @ 2020-02-13 8:19 UTC (permalink / raw) To: Niklas Söderlund, linux-pm, linux-renesas-soc Hi Niklas-san, > From: Niklas Söderlund, Sent: Thursday, February 13, 2020 7:49 AM > > The desired behavior of the driver is to generate an interrupt and call > thermal_zone_device_update() as soon as the temperature have changed > more then one degree. > > When the set_trips operation was implemented it was believed that the > trip window set by the framework would move around the current > temperature and the hysteresis value described in devicetree. The > behavior of the framework is however to set a window based on the trip > points described in devicetree. > > Remove the set_trips operation which was not used correctly and update > the temperatures that triggers interrupts directly from the interrupt > handler. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Thank you for the patch! Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> Best regards, Yoshihiro Shimoda ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes 2020-02-12 22:49 [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes Niklas Söderlund 2020-02-13 8:19 ` Yoshihiro Shimoda @ 2020-02-26 9:05 ` Kieran Bingham 2020-02-26 11:24 ` Niklas Söderlund 1 sibling, 1 reply; 8+ messages in thread From: Kieran Bingham @ 2020-02-26 9:05 UTC (permalink / raw) To: Niklas Söderlund, linux-pm, linux-renesas-soc; +Cc: Yoshihiro Shimoda Hi Niklas, On 12/02/2020 22:49, Niklas Söderlund wrote: > The desired behavior of the driver is to generate an interrupt and call s/behavior/behaviour/ but that's me being English, so you can ignore that ... (at your peril ... :-D ) > thermal_zone_device_update() as soon as the temperature have changed > more then one degree. > > When the set_trips operation was implemented it was believed that the > trip window set by the framework would move around the current > temperature and the hysteresis value described in devicetree. The Should the hysteresis value described in devicetree be a part of the +-MCELCIUS(1) calculations? or is it determined that a one degree window each side is sufficient to contain such hysteresis of the readings? > behavior of the framework is however to set a window based on the trip > points described in devicetree. > > Remove the set_trips operation which was not used correctly and update > the temperatures that triggers interrupts directly from the interrupt > handler. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Sounds good to me. Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > --- > drivers/thermal/rcar_gen3_thermal.c | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) > > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c > index 72877bdc072daaed..55d1736f532cdb33 100644 > --- a/drivers/thermal/rcar_gen3_thermal.c > +++ b/drivers/thermal/rcar_gen3_thermal.c > @@ -81,8 +81,6 @@ struct rcar_gen3_thermal_tsc { > void __iomem *base; > struct thermal_zone_device *zone; > struct equation_coefs coef; > - int low; > - int high; > int tj_t; > int id; /* thermal channel id */ > }; > @@ -204,12 +202,14 @@ static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc, > return INT_FIXPT(val); > } > > -static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) > +static int rcar_gen3_thermal_update_range(struct rcar_gen3_thermal_tsc *tsc) > { > - struct rcar_gen3_thermal_tsc *tsc = devdata; > + int temperature, low, high; > > - low = clamp_val(low, -40000, 120000); > - high = clamp_val(high, -40000, 120000); > + rcar_gen3_thermal_get_temp(tsc, &temperature); > + > + low = temperature - MCELSIUS(1); > + high = temperature + MCELSIUS(1); > > rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1, > rcar_gen3_thermal_mcelsius_to_temp(tsc, low)); > @@ -217,15 +217,11 @@ static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) > rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2, > rcar_gen3_thermal_mcelsius_to_temp(tsc, high)); > > - tsc->low = low; > - tsc->high = high; > - > return 0; > } > > static const 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 void rcar_thermal_irq_set(struct rcar_gen3_thermal_priv *priv, bool on) > @@ -246,9 +242,11 @@ static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data) > 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) > + if (status) { > + rcar_gen3_thermal_update_range(priv->tscs[i]); > thermal_zone_device_update(priv->tscs[i]->zone, > THERMAL_EVENT_UNSPECIFIED); > + } > } > > return IRQ_HANDLED; > @@ -454,6 +452,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev) > if (ret < 0) > goto error_unregister; > > + rcar_gen3_thermal_update_range(tsc); > + > dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret); > } > > @@ -492,7 +492,7 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev) > struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i]; > > priv->thermal_init(tsc); > - rcar_gen3_thermal_set_trips(tsc, tsc->low, tsc->high); > + rcar_gen3_thermal_update_range(tsc); > } > > rcar_thermal_irq_set(priv, true); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes 2020-02-26 9:05 ` Kieran Bingham @ 2020-02-26 11:24 ` Niklas Söderlund 2020-02-26 11:35 ` Kieran Bingham 2020-02-26 11:41 ` Daniel Lezcano 0 siblings, 2 replies; 8+ messages in thread From: Niklas Söderlund @ 2020-02-26 11:24 UTC (permalink / raw) To: Kieran Bingham; +Cc: linux-pm, linux-renesas-soc, Yoshihiro Shimoda Hi Kieran, Thanks for your feedback. On 2020-02-26 09:05:22 +0000, Kieran Bingham wrote: > Hi Niklas, > > On 12/02/2020 22:49, Niklas Söderlund wrote: > > The desired behavior of the driver is to generate an interrupt and call > > s/behavior/behaviour/ but that's me being English, so you can ignore > that ... (at your peril ... :-D ) > I have a hard time as it is trying to spell the "other" version of the language I been exposed to since early years, throwing more 'u' at it will only add to my confusion ;-) > > thermal_zone_device_update() as soon as the temperature have changed > > more then one degree. > > > > When the set_trips operation was implemented it was believed that the > > trip window set by the framework would move around the current > > temperature and the hysteresis value described in devicetree. The > > Should the hysteresis value described in devicetree be a part of the > +-MCELCIUS(1) calculations? or is it determined that a one degree window > each side is sufficient to contain such hysteresis of the readings? The hardware testers wanted a fixed 1 degree window. > > > behavior of the framework is however to set a window based on the trip > > points described in devicetree. > > > > Remove the set_trips operation which was not used correctly and update > > the temperatures that triggers interrupts directly from the interrupt > > handler. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Sounds good to me. > > Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> Thanks! Unfortunately the patch is already merged so it will be hard to add your tag. > > > --- > > drivers/thermal/rcar_gen3_thermal.c | 24 ++++++++++++------------ > > 1 file changed, 12 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c > > index 72877bdc072daaed..55d1736f532cdb33 100644 > > --- a/drivers/thermal/rcar_gen3_thermal.c > > +++ b/drivers/thermal/rcar_gen3_thermal.c > > @@ -81,8 +81,6 @@ struct rcar_gen3_thermal_tsc { > > void __iomem *base; > > struct thermal_zone_device *zone; > > struct equation_coefs coef; > > - int low; > > - int high; > > int tj_t; > > int id; /* thermal channel id */ > > }; > > @@ -204,12 +202,14 @@ static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc, > > return INT_FIXPT(val); > > } > > > > -static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) > > +static int rcar_gen3_thermal_update_range(struct rcar_gen3_thermal_tsc *tsc) > > { > > - struct rcar_gen3_thermal_tsc *tsc = devdata; > > + int temperature, low, high; > > > > - low = clamp_val(low, -40000, 120000); > > - high = clamp_val(high, -40000, 120000); > > + rcar_gen3_thermal_get_temp(tsc, &temperature); > > + > > + low = temperature - MCELSIUS(1); > > + high = temperature + MCELSIUS(1); > > > > rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1, > > rcar_gen3_thermal_mcelsius_to_temp(tsc, low)); > > @@ -217,15 +217,11 @@ static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) > > rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2, > > rcar_gen3_thermal_mcelsius_to_temp(tsc, high)); > > > > - tsc->low = low; > > - tsc->high = high; > > - > > return 0; > > } > > > > static const 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 void rcar_thermal_irq_set(struct rcar_gen3_thermal_priv *priv, bool on) > > @@ -246,9 +242,11 @@ static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data) > > 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) > > + if (status) { > > + rcar_gen3_thermal_update_range(priv->tscs[i]); > > thermal_zone_device_update(priv->tscs[i]->zone, > > THERMAL_EVENT_UNSPECIFIED); > > + } > > } > > > > return IRQ_HANDLED; > > @@ -454,6 +452,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev) > > if (ret < 0) > > goto error_unregister; > > > > + rcar_gen3_thermal_update_range(tsc); > > + > > dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret); > > } > > > > @@ -492,7 +492,7 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev) > > struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i]; > > > > priv->thermal_init(tsc); > > - rcar_gen3_thermal_set_trips(tsc, tsc->low, tsc->high); > > + rcar_gen3_thermal_update_range(tsc); > > } > > > > rcar_thermal_irq_set(priv, true); > > > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes 2020-02-26 11:24 ` Niklas Söderlund @ 2020-02-26 11:35 ` Kieran Bingham 2020-02-26 11:55 ` Niklas Söderlund 2020-02-26 11:41 ` Daniel Lezcano 1 sibling, 1 reply; 8+ messages in thread From: Kieran Bingham @ 2020-02-26 11:35 UTC (permalink / raw) To: Niklas Söderlund; +Cc: linux-pm, linux-renesas-soc, Yoshihiro Shimoda Hi Niklas, On 26/02/2020 11:24, Niklas Söderlund wrote: > Hi Kieran, > > Thanks for your feedback. > > On 2020-02-26 09:05:22 +0000, Kieran Bingham wrote: >> Hi Niklas, >> >> On 12/02/2020 22:49, Niklas Söderlund wrote: >>> The desired behavior of the driver is to generate an interrupt and call >> >> s/behavior/behaviour/ but that's me being English, so you can ignore >> that ... (at your peril ... :-D ) >> > > I have a hard time as it is trying to spell the "other" version of the > language I been exposed to since early years, throwing more 'u' at it > will only add to my confusion ;-) > >>> thermal_zone_device_update() as soon as the temperature have changed >>> more then one degree. >>> >>> When the set_trips operation was implemented it was believed that the >>> trip window set by the framework would move around the current >>> temperature and the hysteresis value described in devicetree. The >> >> Should the hysteresis value described in devicetree be a part of the >> +-MCELCIUS(1) calculations? or is it determined that a one degree window >> each side is sufficient to contain such hysteresis of the readings? > > The hardware testers wanted a fixed 1 degree window. But my question is what is the value described in devicetree. Should it be involved in the calculation somewhere? I.e. if the devicetree specifies that the hysteresis of the hardware is (guessing) say "0.2 degree", does that mean that the 'window' must be at least that value. And picking on that purely as an example as I don't know the value or meaning of the one in DT, but therefore - if there was hardware that then had a 2 degree hysteresis, this fixed-1-degree window would have to be: + low = temperature - min(MCELSIUS(1), MCELSIUS(hysteresis_val); + high = temperature + min(MCELSIUS(1), MCELSIUS(hysteresis_val)); Or perhaps I'm just mis-understanding the reference of the hysteresis value anyway. I see there is still a 'trips' section in the thermal documentation example. Should that be removed? or is it handled by core? >>> behavior of the framework is however to set a window based on the trip >>> points described in devicetree. >>> >>> Remove the set_trips operation which was not used correctly and update >>> the temperatures that triggers interrupts directly from the interrupt >>> handler. >>> >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >> >> Sounds good to me. >> >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Thanks! Unfortunately the patch is already merged so it will be hard to > add your tag. No problem, I saw it on linux-renesas-soc and was curious ;-) -- Kieran > >> >>> --- >>> drivers/thermal/rcar_gen3_thermal.c | 24 ++++++++++++------------ >>> 1 file changed, 12 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c >>> index 72877bdc072daaed..55d1736f532cdb33 100644 >>> --- a/drivers/thermal/rcar_gen3_thermal.c >>> +++ b/drivers/thermal/rcar_gen3_thermal.c >>> @@ -81,8 +81,6 @@ struct rcar_gen3_thermal_tsc { >>> void __iomem *base; >>> struct thermal_zone_device *zone; >>> struct equation_coefs coef; >>> - int low; >>> - int high; >>> int tj_t; >>> int id; /* thermal channel id */ >>> }; >>> @@ -204,12 +202,14 @@ static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc, >>> return INT_FIXPT(val); >>> } >>> >>> -static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) >>> +static int rcar_gen3_thermal_update_range(struct rcar_gen3_thermal_tsc *tsc) >>> { >>> - struct rcar_gen3_thermal_tsc *tsc = devdata; >>> + int temperature, low, high; >>> >>> - low = clamp_val(low, -40000, 120000); >>> - high = clamp_val(high, -40000, 120000); >>> + rcar_gen3_thermal_get_temp(tsc, &temperature); >>> + >>> + low = temperature - MCELSIUS(1); >>> + high = temperature + MCELSIUS(1); >>> >>> rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1, >>> rcar_gen3_thermal_mcelsius_to_temp(tsc, low)); >>> @@ -217,15 +217,11 @@ static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) >>> rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2, >>> rcar_gen3_thermal_mcelsius_to_temp(tsc, high)); >>> >>> - tsc->low = low; >>> - tsc->high = high; >>> - >>> return 0; >>> } >>> >>> static const 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 void rcar_thermal_irq_set(struct rcar_gen3_thermal_priv *priv, bool on) >>> @@ -246,9 +242,11 @@ static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data) >>> 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) >>> + if (status) { >>> + rcar_gen3_thermal_update_range(priv->tscs[i]); >>> thermal_zone_device_update(priv->tscs[i]->zone, >>> THERMAL_EVENT_UNSPECIFIED); >>> + } >>> } >>> >>> return IRQ_HANDLED; >>> @@ -454,6 +452,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev) >>> if (ret < 0) >>> goto error_unregister; >>> >>> + rcar_gen3_thermal_update_range(tsc); >>> + >>> dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret); >>> } >>> >>> @@ -492,7 +492,7 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev) >>> struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i]; >>> >>> priv->thermal_init(tsc); >>> - rcar_gen3_thermal_set_trips(tsc, tsc->low, tsc->high); >>> + rcar_gen3_thermal_update_range(tsc); >>> } >>> >>> rcar_thermal_irq_set(priv, true); >>> >> > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes 2020-02-26 11:35 ` Kieran Bingham @ 2020-02-26 11:55 ` Niklas Söderlund 0 siblings, 0 replies; 8+ messages in thread From: Niklas Söderlund @ 2020-02-26 11:55 UTC (permalink / raw) To: Kieran Bingham; +Cc: linux-pm, linux-renesas-soc, Yoshihiro Shimoda Hi Kieran, Thanks for your feedback. On 2020-02-26 11:35:45 +0000, Kieran Bingham wrote: > Hi Niklas, > > On 26/02/2020 11:24, Niklas Söderlund wrote: > > Hi Kieran, > > > > Thanks for your feedback. > > > > On 2020-02-26 09:05:22 +0000, Kieran Bingham wrote: > >> Hi Niklas, > >> > >> On 12/02/2020 22:49, Niklas Söderlund wrote: > >>> The desired behavior of the driver is to generate an interrupt and call > >> > >> s/behavior/behaviour/ but that's me being English, so you can ignore > >> that ... (at your peril ... :-D ) > >> > > > > I have a hard time as it is trying to spell the "other" version of the > > language I been exposed to since early years, throwing more 'u' at it > > will only add to my confusion ;-) > > > >>> thermal_zone_device_update() as soon as the temperature have changed > >>> more then one degree. > >>> > >>> When the set_trips operation was implemented it was believed that the > >>> trip window set by the framework would move around the current > >>> temperature and the hysteresis value described in devicetree. The > >> > >> Should the hysteresis value described in devicetree be a part of the > >> +-MCELCIUS(1) calculations? or is it determined that a one degree window > >> each side is sufficient to contain such hysteresis of the readings? > > > > The hardware testers wanted a fixed 1 degree window. > > But my question is what is the value described in devicetree. > Should it be involved in the calculation somewhere? Not for the window used to trigger and IRQ, the hysteresis is used with the trip points. This change does not effect that part. This change adds an interrupt if the temperature suddenly spikes more then 1 degree in addition to the polling mode implemented in thermal core (described as polling-delay-passive, polling-delay) which reads the temperature periodically for the trip points. > > I.e. if the devicetree specifies that the hysteresis of the hardware is > (guessing) say "0.2 degree", does that mean that the 'window' must be at > least that value. > > And picking on that purely as an example as I don't know the value or > meaning of the one in DT, but therefore - if there was hardware that > then had a 2 degree hysteresis, this fixed-1-degree window would have to > be: > > + low = temperature - min(MCELSIUS(1), MCELSIUS(hysteresis_val); > + high = temperature + min(MCELSIUS(1), MCELSIUS(hysteresis_val)); > > > Or perhaps I'm just mis-understanding the reference of the hysteresis > value anyway. > > > I see there is still a 'trips' section in the thermal documentation example. > > Should that be removed? or is it handled by core? It's handled by core. The trip points define different operations that could happen if the temperature go above/bellow it. For the example in the documentation it says if the temperature go above 90 with a hysteresis of 2 we reached the critical stage and the SoC will be powered off (I think the action can be specified in user space). > > > > > >>> behavior of the framework is however to set a window based on the trip > >>> points described in devicetree. > >>> > >>> Remove the set_trips operation which was not used correctly and update > >>> the temperatures that triggers interrupts directly from the interrupt > >>> handler. > >>> > >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > >> > >> Sounds good to me. > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > > Thanks! Unfortunately the patch is already merged so it will be hard to > > add your tag. > > No problem, I saw it on linux-renesas-soc and was curious ;-) > -- > Kieran > > > > > >> > >>> --- > >>> drivers/thermal/rcar_gen3_thermal.c | 24 ++++++++++++------------ > >>> 1 file changed, 12 insertions(+), 12 deletions(-) > >>> > >>> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c > >>> index 72877bdc072daaed..55d1736f532cdb33 100644 > >>> --- a/drivers/thermal/rcar_gen3_thermal.c > >>> +++ b/drivers/thermal/rcar_gen3_thermal.c > >>> @@ -81,8 +81,6 @@ struct rcar_gen3_thermal_tsc { > >>> void __iomem *base; > >>> struct thermal_zone_device *zone; > >>> struct equation_coefs coef; > >>> - int low; > >>> - int high; > >>> int tj_t; > >>> int id; /* thermal channel id */ > >>> }; > >>> @@ -204,12 +202,14 @@ static int rcar_gen3_thermal_mcelsius_to_temp(struct rcar_gen3_thermal_tsc *tsc, > >>> return INT_FIXPT(val); > >>> } > >>> > >>> -static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) > >>> +static int rcar_gen3_thermal_update_range(struct rcar_gen3_thermal_tsc *tsc) > >>> { > >>> - struct rcar_gen3_thermal_tsc *tsc = devdata; > >>> + int temperature, low, high; > >>> > >>> - low = clamp_val(low, -40000, 120000); > >>> - high = clamp_val(high, -40000, 120000); > >>> + rcar_gen3_thermal_get_temp(tsc, &temperature); > >>> + > >>> + low = temperature - MCELSIUS(1); > >>> + high = temperature + MCELSIUS(1); > >>> > >>> rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP1, > >>> rcar_gen3_thermal_mcelsius_to_temp(tsc, low)); > >>> @@ -217,15 +217,11 @@ static int rcar_gen3_thermal_set_trips(void *devdata, int low, int high) > >>> rcar_gen3_thermal_write(tsc, REG_GEN3_IRQTEMP2, > >>> rcar_gen3_thermal_mcelsius_to_temp(tsc, high)); > >>> > >>> - tsc->low = low; > >>> - tsc->high = high; > >>> - > >>> return 0; > >>> } > >>> > >>> static const 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 void rcar_thermal_irq_set(struct rcar_gen3_thermal_priv *priv, bool on) > >>> @@ -246,9 +242,11 @@ static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data) > >>> 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) > >>> + if (status) { > >>> + rcar_gen3_thermal_update_range(priv->tscs[i]); > >>> thermal_zone_device_update(priv->tscs[i]->zone, > >>> THERMAL_EVENT_UNSPECIFIED); > >>> + } > >>> } > >>> > >>> return IRQ_HANDLED; > >>> @@ -454,6 +452,8 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev) > >>> if (ret < 0) > >>> goto error_unregister; > >>> > >>> + rcar_gen3_thermal_update_range(tsc); > >>> + > >>> dev_info(dev, "TSC%d: Loaded %d trip points\n", i, ret); > >>> } > >>> > >>> @@ -492,7 +492,7 @@ static int __maybe_unused rcar_gen3_thermal_resume(struct device *dev) > >>> struct rcar_gen3_thermal_tsc *tsc = priv->tscs[i]; > >>> > >>> priv->thermal_init(tsc); > >>> - rcar_gen3_thermal_set_trips(tsc, tsc->low, tsc->high); > >>> + rcar_gen3_thermal_update_range(tsc); > >>> } > >>> > >>> rcar_thermal_irq_set(priv, true); > >>> > >> > > > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes 2020-02-26 11:24 ` Niklas Söderlund 2020-02-26 11:35 ` Kieran Bingham @ 2020-02-26 11:41 ` Daniel Lezcano 2020-02-26 11:44 ` Niklas Söderlund 1 sibling, 1 reply; 8+ messages in thread From: Daniel Lezcano @ 2020-02-26 11:41 UTC (permalink / raw) To: Niklas Söderlund, Kieran Bingham Cc: linux-pm, linux-renesas-soc, Yoshihiro Shimoda Hi Niklas, On 26/02/2020 12:24, Niklas Söderlund wrote: > Hi Kieran, [ ... ] >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >> >> Sounds good to me. >> >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > Thanks! Unfortunately the patch is already merged so it will be hard to > add your tag. Where is it merged? I've it in my tree in the 'testing' branch about to move it to the linux-next branch. I can still add the reviewed-by. [ ... ] -- <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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes 2020-02-26 11:41 ` Daniel Lezcano @ 2020-02-26 11:44 ` Niklas Söderlund 0 siblings, 0 replies; 8+ messages in thread From: Niklas Söderlund @ 2020-02-26 11:44 UTC (permalink / raw) To: Daniel Lezcano Cc: Kieran Bingham, linux-pm, linux-renesas-soc, Yoshihiro Shimoda Hi Daniel, On 2020-02-26 12:41:39 +0100, Daniel Lezcano wrote: > > Hi Niklas, > > On 26/02/2020 12:24, Niklas Söderlund wrote: > > Hi Kieran, > > [ ... ] > > >>> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > >> > >> Sounds good to me. > >> > >> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com> > > > > Thanks! Unfortunately the patch is already merged so it will be hard to > > add your tag. > > Where is it merged? I've it in my tree in the 'testing' branch about to > move it to the linux-next branch. I can still add the reviewed-by. My bad I was looking at the wrong branch, thanks for pointing it out. > > [ ... ] > > > -- > <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 > -- Regards, Niklas Söderlund ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-02-26 11:55 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-12 22:49 [PATCH] thermal: rcar_gen3_thermal: Generate interrupt when temperature changes Niklas Söderlund 2020-02-13 8:19 ` Yoshihiro Shimoda 2020-02-26 9:05 ` Kieran Bingham 2020-02-26 11:24 ` Niklas Söderlund 2020-02-26 11:35 ` Kieran Bingham 2020-02-26 11:55 ` Niklas Söderlund 2020-02-26 11:41 ` Daniel Lezcano 2020-02-26 11:44 ` Niklas Söderlund
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).