diff mbox series

[V2] ARM: imx: imx8m: Adjust thermal trip points for Industrial parts

Message ID 20220507120440.24240-1-aford173@gmail.com
State Changes Requested
Delegated to: Stefano Babic
Headers show
Series [V2] ARM: imx: imx8m: Adjust thermal trip points for Industrial parts | expand

Commit Message

Adam Ford May 7, 2022, 12:04 p.m. UTC 
If the thermal sensor is enabled in U-Boot, adjust the cpu-thermal
trip points for industrial rated parts.  This should apply to 8MQ,
8MM, 8MN, and 8MP.

Signed-off-by: Adam Ford <aford173@gmail.com>
Reviewed-by: Tim Harvey <tharvey@gateworks.com>
---
V2:  Switch the check from looking for industrial or checkoing for
     anything but commerical.  This expands the trip point updates
     other grades as well.

Comments

Marek Vasut May 7, 2022, 10 p.m. UTC | #1 
On 5/7/22 14:04, Adam Ford wrote:
> If the thermal sensor is enabled in U-Boot, adjust the cpu-thermal
> trip points for industrial rated parts.  This should apply to 8MQ,
> 8MM, 8MN, and 8MP.
> 
> Signed-off-by: Adam Ford <aford173@gmail.com>
> Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> ---
> V2:  Switch the check from looking for industrial or checkoing for
>       anything but commerical.  This expands the trip point updates
>       other grades as well.

Are the industrial and automotive trip points identical ?
Adam Ford May 8, 2022, 2:59 a.m. UTC | #2 
On Sat, May 7, 2022 at 5:00 PM Marek Vasut <marex@denx.de> wrote:
>
> On 5/7/22 14:04, Adam Ford wrote:
> > If the thermal sensor is enabled in U-Boot, adjust the cpu-thermal
> > trip points for industrial rated parts.  This should apply to 8MQ,
> > 8MM, 8MN, and 8MP.
> >
> > Signed-off-by: Adam Ford <aford173@gmail.com>
> > Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> > V2:  Switch the check from looking for industrial or checkoing for
> >       anything but commerical.  This expands the trip point updates
> >       other grades as well.
>
> Are the industrial and automotive trip points identical ?

I do not know nor do I have any to test, but Francisco asked for the
change.  I would expect the corresponding values would be fetched with
get_cpu_temp_grade()

I will be on holiday in Italy for the next two weeks, so if there are
changes needed, I can address them when I get back.

adam
Marek Vasut May 8, 2022, 1:42 p.m. UTC | #3 
On 5/8/22 04:59, Adam Ford wrote:
> On Sat, May 7, 2022 at 5:00 PM Marek Vasut <marex@denx.de> wrote:
>>
>> On 5/7/22 14:04, Adam Ford wrote:
>>> If the thermal sensor is enabled in U-Boot, adjust the cpu-thermal
>>> trip points for industrial rated parts.  This should apply to 8MQ,
>>> 8MM, 8MN, and 8MP.
>>>
>>> Signed-off-by: Adam Ford <aford173@gmail.com>
>>> Reviewed-by: Tim Harvey <tharvey@gateworks.com>
>>> ---
>>> V2:  Switch the check from looking for industrial or checkoing for
>>>        anything but commerical.  This expands the trip point updates
>>>        other grades as well.
>>
>> Are the industrial and automotive trip points identical ?
> 
> I do not know nor do I have any to test, but Francisco asked for the
> change.  I would expect the corresponding values would be fetched with
> get_cpu_temp_grade()

Ah, I see. But then I have to wonder, why are we updating only 
non-commercial trip points ? Why not update them always ?
Francesco Dolcini May 8, 2022, 4:55 p.m. UTC | #4 
On Sun, May 08, 2022 at 03:42:31PM +0200, Marek Vasut wrote:
> On 5/8/22 04:59, Adam Ford wrote:
> > On Sat, May 7, 2022 at 5:00 PM Marek Vasut <marex@denx.de> wrote:
> > > 
> > > On 5/7/22 14:04, Adam Ford wrote:
> > > > If the thermal sensor is enabled in U-Boot, adjust the cpu-thermal
> > > > trip points for industrial rated parts.  This should apply to 8MQ,
> > > > 8MM, 8MN, and 8MP.
> > > > 
> > > > Signed-off-by: Adam Ford <aford173@gmail.com>
> > > > Reviewed-by: Tim Harvey <tharvey@gateworks.com>
> > > > ---
> > > > V2:  Switch the check from looking for industrial or checkoing for
> > > >        anything but commerical.  This expands the trip point updates
> > > >        other grades as well.
> > > 
> > > Are the industrial and automotive trip points identical ?
> > 
> > I do not know nor do I have any to test, but Francisco asked for the
> > change.  I would expect the corresponding values would be fetched with
> > get_cpu_temp_grade()
> 
> Ah, I see. But then I have to wonder, why are we updating only
> non-commercial trip points ? Why not update them always ?

I didn't think through it enough, I agree on updating the trip points
always.

Francesco
Andrejs Cainikovs May 9, 2022, 7:38 a.m. UTC | #5 
Hi Adam and all,

Thanks for your work. I was about to send a similar patch by myself, but 
I came up with slightly different code. I'm sending my version for 
downstream version of NXP (should apply with minor conflicts to 
upstream) at the end of this mail. Plus some code review.

On 07/05/2022 14:04, Adam Ford wrote:
> If the thermal sensor is enabled in U-Boot, adjust the cpu-thermal

Should be not only for `cpu-thermal`. See below.

> V2:  Switch the check from looking for industrial or checkoing for
>       anything but commerical.  This expands the trip point updates
>       other grades as well.

Typos: checkoing, commerical.

> +#include <imx_thermal.h>

Not needed if you drop temp grade check. See below.

> +#if defined(CONFIG_IMX_TMU)

I believe this is needed only if you do the temperature reading. 
get_cpu_temp_grade() does not require this. Please remove.

> +	nodeoff = fdt_path_offset(blob, "/thermal-zones/cpu-thermal/trips");

This is enough for all variants except 8MP, which has 2 temperature 
sensors: `cpu-thermal` and `soc-thermal`. Both needs to be updated with 
equal trips.

> +	/* Only update non-Commerical grade parts */
> +	if (get_cpu_temp_grade(&minc, &maxc) != TEMP_COMMERCIAL) {

As others said, I would remove this check, as I don't see any issues if 
code would set trips for a commercial grade as well. This implies 
removing the include I mentioned above.

My version which I was going to sent (after rebasing) is below. Feel 
free to modify/rework/adjust your patch with or without parts from my code.

 >> Are the industrial and automotive trip points identical ?
 >
 > I do not know nor do I have any to test...

After you're done with the new patchset I can test it for you. I have 
access to commercial and industrial variants.

Best regards,
Andrejs Cainikovs

---

diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c

index 
e93ecd28467ccc33ab35f0073840963eb3a01800..d4438b78887b91c600aaf22db47ef8985462ccd8 
100644

--- a/arch/arm/mach-imx/imx8m/soc.c

+++ b/arch/arm/mach-imx/imx8m/soc.c

@@ -1001,8 +1001,52 @@ static int disable_cpu_nodes(void *blob, u32 
disabled_cores)

  	return 0;

  }



+int fixup_thermal_trips(void *blob, const char *name)

+{

+	int minc, maxc;

+	int node, trip;

+	const char *type;

+	int temp, ret;

+

+	node = fdt_path_offset(blob, "/thermal-zones");

+	if (node < 0)

+		return node;

+

+	node = fdt_subnode_offset(blob, node, name);

+	if (node < 0)

+		return node;

+

+	node = fdt_subnode_offset(blob, node, "trips");

+	if (node < 0)

+		return node;

+

+	get_cpu_temp_grade(&minc, &maxc);

+

+	fdt_for_each_subnode(trip, blob, node) {

+		type = fdt_getprop(blob, trip, "type", NULL);

+

+		temp = 0;

+		if (!strcmp(type, "critical")) {

+			temp = 1000 * maxc;

+		} else if (!strcmp(type, "passive")) {

+			temp = 1000 * (maxc - 10);

+		}

+		if (temp) {

+			ret = fdt_setprop_u32(blob, trip, "temperature", temp);

+			if (ret) {

+				printf("Could not update thermal trip\n");

+				return ret;

+			}

+		}

+	}

+

+	return 0;

+}

+

  int ft_system_setup(void *blob, bd_t *bd)

  {

+	int ret;

+

  #ifdef CONFIG_IMX8MQ

  	int i = 0;

  	int rc;

@@ -1128,6 +1172,16 @@ usb_modify_speed:

  		disable_cpu_nodes(blob, 2);

  #endif



+	ret = fixup_thermal_trips(blob, "cpu-thermal");

+#ifdef CONFIG_IMX8MP

+	if (!ret)

+		ret = fixup_thermal_trips(blob, "soc-thermal");

+#endif

+	if (ret) {

+		printf("Failed to update thermal trip\n");

+		return ret;

+	}

+

  	return ft_add_optee_node(blob, bd);

  }

  #endif
Andrejs Cainikovs May 12, 2022, 10:12 a.m. UTC | #6 
Hi Adam,

Below is the final vesion of my patch after internal review.

Best regards,
Andrejs Cainikovs.
diff mbox series

Patch

diff --git a/arch/arm/mach-imx/imx8m/soc.c b/arch/arm/mach-imx/imx8m/soc.c
index 8e23e6da32..7175295c3c 100644
--- a/arch/arm/mach-imx/imx8m/soc.c
+++ b/arch/arm/mach-imx/imx8m/soc.c
@@ -30,6 +30,7 @@ 
 #include <fsl_wdog.h>
 #include <imx_sip.h>
 #include <linux/bitops.h>
+#include <imx_thermal.h>
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -1207,10 +1208,10 @@  static int cleanup_nodes_for_efi(void *blob)
 
 int ft_system_setup(void *blob, struct bd_info *bd)
 {
+	__maybe_unused int nodeoff;
 #ifdef CONFIG_IMX8MQ
 	int i = 0;
 	int rc;
-	int nodeoff;
 
 	if (get_boot_device() == USB_BOOT) {
 		disable_dcss_nodes(blob);
@@ -1346,6 +1347,24 @@  usb_modify_speed:
 		disable_cpu_nodes(blob, 2);
 #endif
 
+#if defined(CONFIG_IMX_TMU)
+	int minc, maxc, prop;
+
+	nodeoff = fdt_path_offset(blob, "/thermal-zones/cpu-thermal/trips");
+
+	/* Only update non-Commerical grade parts */
+	if (get_cpu_temp_grade(&minc, &maxc) != TEMP_COMMERCIAL) {
+		fdt_for_each_subnode(prop, blob, nodeoff) {
+			const char *type = fdt_getprop(blob, prop, "type", NULL);
+
+			if (type && (!strcmp("critical", type)))
+				fdt_setprop_u32(blob, prop, "temperature", maxc * 1000);
+			else if (type && (!strcmp("passive", type)))
+				fdt_setprop_u32(blob, prop, "temperature", (maxc - 10) * 1000);
+		}
+	}
+#endif
+
 	cleanup_nodes_for_efi(blob);
 	return 0;
 }