| Message ID | 20220617071411.187542-5-francesco.dolcini@toradex.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | imx: thermal: Allow trip point configuration from DT | expand |
Hi Francesco, thanks for your patch. On 22-06-17, Francesco Dolcini wrote: > Allow over-writing critical and passive trip point for each > temperature grade from the device tree, by default the pre-existing > hard-coded trip points are used. > > This change enables configuring the system thermal characteristics into > the system-specific device tree instead of relying on global hard-coded > temperature thresholds that does not take into account the specific > system thermal design. > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > --- > v2: > - return immediately if no thermal node present in the dts > - use dev_info instead of dev_dbg if there is an invalid trip > - additional comment in case passive trip point is higher than critical > --- > drivers/thermal/imx_thermal.c | 58 +++++++++++++++++++++++++++++++++++ > 1 file changed, 58 insertions(+) > > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c > index 16663373b682..a964baf802fc 100644 > --- a/drivers/thermal/imx_thermal.c > +++ b/drivers/thermal/imx_thermal.c > @@ -17,6 +17,8 @@ > #include <linux/nvmem-consumer.h> > #include <linux/pm_runtime.h> > > +#include "thermal_core.h" > + > #define REG_SET 0x4 > #define REG_CLR 0x8 > #define REG_TOG 0xc > @@ -479,36 +481,92 @@ static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1) > return 0; > } > > +static void imx_init_temp_from_of(struct platform_device *pdev, const char *name) > +{ > + struct imx_thermal_data *data = platform_get_drvdata(pdev); > + struct device_node *thermal, *trips, *trip_point; > + > + thermal = of_get_child_by_name(pdev->dev.of_node, name); > + if (!thermal) > + return; > + > + trips = of_get_child_by_name(thermal, "trips"); > + > + for_each_child_of_node(trips, trip_point) { > + struct thermal_trip t; > + > + if (thermal_of_populate_trip(trip_point, &t)) > + continue; > + > + switch (t.type) { > + case THERMAL_TRIP_PASSIVE: > + data->temp_passive = t.temperature; > + break; > + case THERMAL_TRIP_CRITICAL: Should we check also the temp_critical and temp_passive not exceeding the temp_max? Sry. that it came not earlier in my mind. So system damage is avoided. Apart of this note, the patch is lgtm. Regards, Marco > + data->temp_critical = t.temperature; > + break; > + default: > + dev_info(&pdev->dev, "Ignoring trip type %d\n", t.type); > + break; > + } > + }; > + > + of_node_put(trips); > + of_node_put(thermal); > + > + if (data->temp_passive >= data->temp_critical) { > + dev_warn(&pdev->dev, > + "passive trip point must be lower than critical, fixing it up\n"); > + /* > + * In case of misconfiguration set passive temperature to > + * 5°C less than critical, this seems like a reasonable > + * default and the same is done when no thermal trips are > + * available in the device tree. > + */ > + data->temp_passive = data->temp_critical - (1000 * 5); > + } > +} > + > static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0) > { > struct imx_thermal_data *data = platform_get_drvdata(pdev); > + const char *thermal_node_name; > > /* The maximum die temp is specified by the Temperature Grade */ > switch ((ocotp_mem0 >> 6) & 0x3) { > case 0: /* Commercial (0 to 95 °C) */ > + thermal_node_name = "commercial-thermal"; > data->temp_grade = "Commercial"; > data->temp_max = 95000; > break; > case 1: /* Extended Commercial (-20 °C to 105 °C) */ > + thermal_node_name = "extended-commercial-thermal"; > data->temp_grade = "Extended Commercial"; > data->temp_max = 105000; > break; > case 2: /* Industrial (-40 °C to 105 °C) */ > + thermal_node_name = "industrial-thermal"; > data->temp_grade = "Industrial"; > data->temp_max = 105000; > break; > case 3: /* Automotive (-40 °C to 125 °C) */ > + thermal_node_name = "automotive-thermal"; > data->temp_grade = "Automotive"; > data->temp_max = 125000; > break; > } > > /* > + * Set defaults trips > + * > * Set the critical trip point at 5 °C under max > * Set the passive trip point at 10 °C under max (changeable via sysfs) > */ > data->temp_critical = data->temp_max - (1000 * 5); > data->temp_passive = data->temp_max - (1000 * 10); > + > + /* Override critical/passive temperature from devicetree */ > + imx_init_temp_from_of(pdev, thermal_node_name); > } > > static int imx_init_from_tempmon_data(struct platform_device *pdev) > -- > 2.25.1 > >
On Fri, Jun 17, 2022 at 10:40:17AM +0200, Marco Felsch wrote: > On 22-06-17, Francesco Dolcini wrote: > > Allow over-writing critical and passive trip point for each > > temperature grade from the device tree, by default the pre-existing > > hard-coded trip points are used. > > > > This change enables configuring the system thermal characteristics into > > the system-specific device tree instead of relying on global hard-coded > > temperature thresholds that does not take into account the specific > > system thermal design. > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > --- > > v2: > > - return immediately if no thermal node present in the dts > > - use dev_info instead of dev_dbg if there is an invalid trip > > - additional comment in case passive trip point is higher than critical > > --- > > drivers/thermal/imx_thermal.c | 58 +++++++++++++++++++++++++++++++++++ > > 1 file changed, 58 insertions(+) > > > > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c > > index 16663373b682..a964baf802fc 100644 > > --- a/drivers/thermal/imx_thermal.c > > +++ b/drivers/thermal/imx_thermal.c > > @@ -17,6 +17,8 @@ > > #include <linux/nvmem-consumer.h> > > #include <linux/pm_runtime.h> > > > > +#include "thermal_core.h" > > + > > #define REG_SET 0x4 > > #define REG_CLR 0x8 > > #define REG_TOG 0xc > > @@ -479,36 +481,92 @@ static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1) > > return 0; > > } > > > > +static void imx_init_temp_from_of(struct platform_device *pdev, const char *name) > > +{ > > + struct imx_thermal_data *data = platform_get_drvdata(pdev); > > + struct device_node *thermal, *trips, *trip_point; > > + > > + thermal = of_get_child_by_name(pdev->dev.of_node, name); > > + if (!thermal) > > + return; > > + > > + trips = of_get_child_by_name(thermal, "trips"); > > + > > + for_each_child_of_node(trips, trip_point) { > > + struct thermal_trip t; > > + > > + if (thermal_of_populate_trip(trip_point, &t)) > > + continue; > > + > > + switch (t.type) { > > + case THERMAL_TRIP_PASSIVE: > > + data->temp_passive = t.temperature; > > + break; > > + case THERMAL_TRIP_CRITICAL: > > Should we check also the temp_critical and temp_passive not exceeding > the temp_max? Sry. that it came not earlier in my mind. So system damage > is avoided. I would not add such kind of restriction in the code. I can think of multiple situations in which a system designer would prefer to take the chances of burning a silicon (or more likely just age it a little bit faster) than to just shut down the system. In the end whoever is building the system should be empowered to do something like that and it's no different from what it's already possible with thermal_of driver for example. In addition to that from a system debug prospective all the threshold (max, passive, critical) are already available in the kernel logs. Francesco
On 22-06-17, Francesco Dolcini wrote: > On Fri, Jun 17, 2022 at 10:40:17AM +0200, Marco Felsch wrote: > > On 22-06-17, Francesco Dolcini wrote: > > > Allow over-writing critical and passive trip point for each > > > temperature grade from the device tree, by default the pre-existing > > > hard-coded trip points are used. > > > > > > This change enables configuring the system thermal characteristics into > > > the system-specific device tree instead of relying on global hard-coded > > > temperature thresholds that does not take into account the specific > > > system thermal design. > > > > > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > > --- > > > v2: > > > - return immediately if no thermal node present in the dts > > > - use dev_info instead of dev_dbg if there is an invalid trip > > > - additional comment in case passive trip point is higher than critical > > > --- > > > drivers/thermal/imx_thermal.c | 58 +++++++++++++++++++++++++++++++++++ > > > 1 file changed, 58 insertions(+) > > > > > > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c > > > index 16663373b682..a964baf802fc 100644 > > > --- a/drivers/thermal/imx_thermal.c > > > +++ b/drivers/thermal/imx_thermal.c > > > @@ -17,6 +17,8 @@ > > > #include <linux/nvmem-consumer.h> > > > #include <linux/pm_runtime.h> > > > > > > +#include "thermal_core.h" > > > + > > > #define REG_SET 0x4 > > > #define REG_CLR 0x8 > > > #define REG_TOG 0xc > > > @@ -479,36 +481,92 @@ static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1) > > > return 0; > > > } > > > > > > +static void imx_init_temp_from_of(struct platform_device *pdev, const char *name) > > > +{ > > > + struct imx_thermal_data *data = platform_get_drvdata(pdev); > > > + struct device_node *thermal, *trips, *trip_point; > > > + > > > + thermal = of_get_child_by_name(pdev->dev.of_node, name); > > > + if (!thermal) > > > + return; > > > + > > > + trips = of_get_child_by_name(thermal, "trips"); > > > + > > > + for_each_child_of_node(trips, trip_point) { > > > + struct thermal_trip t; > > > + > > > + if (thermal_of_populate_trip(trip_point, &t)) > > > + continue; > > > + > > > + switch (t.type) { > > > + case THERMAL_TRIP_PASSIVE: > > > + data->temp_passive = t.temperature; > > > + break; > > > + case THERMAL_TRIP_CRITICAL: > > > > Should we check also the temp_critical and temp_passive not exceeding > > the temp_max? Sry. that it came not earlier in my mind. So system damage > > is avoided. > > I would not add such kind of restriction in the code. I can think of > multiple situations in which a system designer would prefer to take the > chances of burning a silicon (or more likely just age it a little bit > faster) than to just shut down the system. > > In the end whoever is building the system should be empowered to do > something like that and it's no different from what it's already possible > with thermal_of driver for example. > > In addition to that from a system debug prospective all the threshold > (max, passive, critical) are already available in the kernel logs. Okay, fine with me since you provided dt-snippets with the correct temperature. But we should really print a warning since this is a abnormal usage and the user should be warned. Regards, Marco > > Francesco > >
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index 16663373b682..a964baf802fc 100644 --- a/drivers/thermal/imx_thermal.c +++ b/drivers/thermal/imx_thermal.c @@ -17,6 +17,8 @@ #include <linux/nvmem-consumer.h> #include <linux/pm_runtime.h> +#include "thermal_core.h" + #define REG_SET 0x4 #define REG_CLR 0x8 #define REG_TOG 0xc @@ -479,36 +481,92 @@ static int imx_init_calib(struct platform_device *pdev, u32 ocotp_ana1) return 0; } +static void imx_init_temp_from_of(struct platform_device *pdev, const char *name) +{ + struct imx_thermal_data *data = platform_get_drvdata(pdev); + struct device_node *thermal, *trips, *trip_point; + + thermal = of_get_child_by_name(pdev->dev.of_node, name); + if (!thermal) + return; + + trips = of_get_child_by_name(thermal, "trips"); + + for_each_child_of_node(trips, trip_point) { + struct thermal_trip t; + + if (thermal_of_populate_trip(trip_point, &t)) + continue; + + switch (t.type) { + case THERMAL_TRIP_PASSIVE: + data->temp_passive = t.temperature; + break; + case THERMAL_TRIP_CRITICAL: + data->temp_critical = t.temperature; + break; + default: + dev_info(&pdev->dev, "Ignoring trip type %d\n", t.type); + break; + } + }; + + of_node_put(trips); + of_node_put(thermal); + + if (data->temp_passive >= data->temp_critical) { + dev_warn(&pdev->dev, + "passive trip point must be lower than critical, fixing it up\n"); + /* + * In case of misconfiguration set passive temperature to + * 5°C less than critical, this seems like a reasonable + * default and the same is done when no thermal trips are + * available in the device tree. + */ + data->temp_passive = data->temp_critical - (1000 * 5); + } +} + static void imx_init_temp_grade(struct platform_device *pdev, u32 ocotp_mem0) { struct imx_thermal_data *data = platform_get_drvdata(pdev); + const char *thermal_node_name; /* The maximum die temp is specified by the Temperature Grade */ switch ((ocotp_mem0 >> 6) & 0x3) { case 0: /* Commercial (0 to 95 °C) */ + thermal_node_name = "commercial-thermal"; data->temp_grade = "Commercial"; data->temp_max = 95000; break; case 1: /* Extended Commercial (-20 °C to 105 °C) */ + thermal_node_name = "extended-commercial-thermal"; data->temp_grade = "Extended Commercial"; data->temp_max = 105000; break; case 2: /* Industrial (-40 °C to 105 °C) */ + thermal_node_name = "industrial-thermal"; data->temp_grade = "Industrial"; data->temp_max = 105000; break; case 3: /* Automotive (-40 °C to 125 °C) */ + thermal_node_name = "automotive-thermal"; data->temp_grade = "Automotive"; data->temp_max = 125000; break; } /* + * Set defaults trips + * * Set the critical trip point at 5 °C under max * Set the passive trip point at 10 °C under max (changeable via sysfs) */ data->temp_critical = data->temp_max - (1000 * 5); data->temp_passive = data->temp_max - (1000 * 10); + + /* Override critical/passive temperature from devicetree */ + imx_init_temp_from_of(pdev, thermal_node_name); } static int imx_init_from_tempmon_data(struct platform_device *pdev)
Allow over-writing critical and passive trip point for each temperature grade from the device tree, by default the pre-existing hard-coded trip points are used. This change enables configuring the system thermal characteristics into the system-specific device tree instead of relying on global hard-coded temperature thresholds that does not take into account the specific system thermal design. Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> --- v2: - return immediately if no thermal node present in the dts - use dev_info instead of dev_dbg if there is an invalid trip - additional comment in case passive trip point is higher than critical --- drivers/thermal/imx_thermal.c | 58 +++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+)