| Message ID | 20220615094804.388280-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, nice patch, only a few nits. On 22-06-15, 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> > --- > drivers/thermal/imx_thermal.c | 49 +++++++++++++++++++++++++++++++++++ > 1 file changed, 49 insertions(+) > > diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c > index 16663373b682..ef3e152b5ee2 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,83 @@ 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); here I would do: if (!thermal) return; since the thermal node is only available with your dt-changes in place. > + 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_dbg(&pdev->dev, "Ignoring trip type %d\n", t.type); ^ Maybe it is worth to use dev_info() since this never should happen and if it happen, it is a bug/misconfiguration/misusage. > + 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"); > + data->temp_passive = data->temp_critical - (1000 * 5); ^ Magic number? Maybe it would be worth a comment. Regards, Marco > + } > +} > + > 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 Wed, Jun 15, 2022 at 12:39:56PM +0200, Marco Felsch wrote: > On 22-06-15, Francesco Dolcini wrote: > > +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); > > here I would do: > > if (!thermal) > return; > > since the thermal node is only available with your dt-changes in place. I didn't do it since from my understanding both `of_get_child_by_name` and `for_each_child_of_node` just handle correctly NULL as an input parameter. Anyway, I agree that your suggested change would make crystal clear that this is optional, I'll do it. Francesco
diff --git a/drivers/thermal/imx_thermal.c b/drivers/thermal/imx_thermal.c index 16663373b682..ef3e152b5ee2 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,83 @@ 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); + 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_dbg(&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"); + 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> --- drivers/thermal/imx_thermal.c | 49 +++++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+)