RE: [PATCH V15 RESEND 3/5] thermal: imx_sc: add i.MX system controller thermal support

From: Anson Huang
Date: Fri Feb 21 2020 - 18:46:42 EST

Hi, Daniel

> Subject: Re: [PATCH V15 RESEND 3/5] thermal: imx_sc: add i.MX system
> controller thermal support
>
> Hi Anson,
>
> sorry for the delay with this review, hopefully the upstreaming will be now a
> bit more smooth.

Thanks very much for reviewð

>
> Apart the comments below, the driver looks good to me.
>
> On Thu, Feb 20, 2020 at 09:10:26AM +0800, Anson Huang wrote:
> > i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
> > inside, the system controller is in charge of controlling power, clock
> > and thermal sensors etc..
> >
> > This patch adds i.MX system controller thermal driver support, Linux
> > kernel has to communicate with system controller via MU (message unit)
> > IPC to get each thermal sensor's temperature, it supports multiple
> > sensors which are passed from device tree, please see the binding doc
> > for details.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@xxxxxxx>
> > ---
> > No change.
> > ---
> > drivers/thermal/Kconfig | 11 +++
> > drivers/thermal/Makefile | 1 +
> > drivers/thermal/imx_sc_thermal.c | 142
> > +++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 154 insertions(+)
> > create mode 100644 drivers/thermal/imx_sc_thermal.c
> >
> > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig index
> > 5a05db5..d1cb8dc 100644
> > --- a/drivers/thermal/Kconfig
> > +++ b/drivers/thermal/Kconfig
> > @@ -251,6 +251,17 @@ config IMX_THERMAL
> > cpufreq is used as the cooling device to throttle CPUs when the
> > passive trip is crossed.
> >
> > +config IMX_SC_THERMAL
> > + tristate "Temperature sensor driver for NXP i.MX SoCs with System
> Controller"
> > + depends on ARCH_MXC && IMX_SCU
>
> IMX_SCU depends on IMX_MBOX which depends on ARCH_MXC. This
> dependency could be simplified.
>
> Also add the COMPILE_TEST option to improve compilation test coverage.

Will make it depends on IMX_SCU and COMPILE_TEST

>
> > + depends on OF
> > + help
> > + Support for Temperature Monitor (TEMPMON) found on NXP i.MX
> SoCs with
> > + system controller inside, Linux kernel has to communicate with
> system
> > + controller via MU (message unit) IPC to get temperature from
> thermal
> > + sensor. It supports one critical trip point and one
> > + passive trip point for each thermal sensor.
> > +
> > config MAX77620_THERMAL
> > tristate "Temperature sensor driver for Maxim MAX77620 PMIC"
> > depends on MFD_MAX77620
> > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile index
> > 9fb88e2..a11a6d8 100644
> > --- a/drivers/thermal/Makefile
> > +++ b/drivers/thermal/Makefile
> > @@ -43,6 +43,7 @@ obj-$(CONFIG_DB8500_THERMAL) +=
> db8500_thermal.o
> > obj-$(CONFIG_ARMADA_THERMAL) += armada_thermal.o
> > obj-$(CONFIG_TANGO_THERMAL) += tango_thermal.o
> > obj-$(CONFIG_IMX_THERMAL) += imx_thermal.o
> > +obj-$(CONFIG_IMX_SC_THERMAL) += imx_sc_thermal.o
> > obj-$(CONFIG_MAX77620_THERMAL) += max77620_thermal.o
> > obj-$(CONFIG_QORIQ_THERMAL) += qoriq_thermal.o
> > obj-$(CONFIG_DA9062_THERMAL) += da9062-thermal.o
> > diff --git a/drivers/thermal/imx_sc_thermal.c
> > b/drivers/thermal/imx_sc_thermal.c
> > new file mode 100644
> > index 0000000..d406ecb
> > --- /dev/null
> > +++ b/drivers/thermal/imx_sc_thermal.c
> > @@ -0,0 +1,142 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Copyright 2018-2019 NXP.
>
> *sigh* 2020 now ...

Yes, should be 2018-2020

>
> [ ... ]
>
> > +static int imx_sc_thermal_get_temp(void *data, int *temp) {
> > + struct imx_sc_msg_misc_get_temp msg;
> > + struct imx_sc_rpc_msg *hdr = &msg.hdr;
> > + struct imx_sc_sensor *sensor = data;
> > + int ret;
> > +
> > + msg.data.req.resource_id = sensor->resource_id;
> > + msg.data.req.type = IMX_SC_C_TEMP;
> > +
> > + hdr->ver = IMX_SC_RPC_VERSION;
> > + hdr->svc = IMX_SC_RPC_SVC_MISC;
> > + hdr->func = IMX_SC_MISC_FUNC_GET_TEMP;
> > + hdr->size = 2;
>
> Can you explain this 'size' value?

The size means the SCU message size, including the header and the data, its unit
is word(4 bytes), in thermal get temperature message, the header takes 1 word and
the data takes another 1, so it is 2, we all pass the size in this way to SCU in i.MX8
SoCs, the SCU know how long message it will need to receive from AP.

>
> [ ... ]
>
> > +MODULE_DEVICE_TABLE(of, imx_sc_thermal_table);
> > +
> > +static struct platform_driver imx_sc_thermal_driver = {
> > + .probe = imx_sc_thermal_probe,
>
> The driver can be compiled as module but there is no 'remove' callback

As there is nothing needs to be done in .remove callback, so I skip it. But
I think I can add a blank .remove callback to make it more complete.

Thanks,
Anson

>
> > + .driver = {
> > + .name = "imx-sc-thermal",
> > + .of_match_table = imx_sc_thermal_table,
> > + },
> > +};
> > +module_platform_driver(imx_sc_thermal_driver);
> > +
> > +MODULE_AUTHOR("Anson Huang <Anson.Huang@xxxxxxx>");
> > +MODULE_DESCRIPTION("Thermal driver for NXP i.MX SoCs with system
> > +controller"); MODULE_LICENSE("GPL v2");
>
>
>
> --
>
>
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7Cd5ce55
> aaab4141f5f6e308d7b6ccfe72%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0
> %7C0%7C637178863864826002&amp;sdata=1XOKNrrIkU9zXkljOAcSxPcKAF4g
> jShBav%2FyGO612FM%3D&amp;reserved=0> Linaro.org â Open source
> software for ARM SoCs
>
> Follow Linaro:
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> facebook.com%2Fpages%2FLinaro&amp;data=02%7C01%7Canson.huang%4
> 0nxp.com%7Cd5ce55aaab4141f5f6e308d7b6ccfe72%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C637178863864826002&amp;sdata=gpzxYFcEV
> c4LsenqNFbMVS5Yvx8GLeqitoNB66bn8v4%3D&amp;reserved=0> Facebook |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Ftwitte
> r.com%2F%23!%2Flinaroorg&amp;data=02%7C01%7Canson.huang%40nxp.c
> om%7Cd5ce55aaab4141f5f6e308d7b6ccfe72%7C686ea1d3bc2b4c6fa92cd99c
> 5c301635%7C0%7C0%7C637178863864835997&amp;sdata=TXyn%2F1%2B2b
> sCKhCibdSjdlyMe7RqjC8LzVUh%2FrYOnNPs%3D&amp;reserved=0> Twitter |
> <https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.
> linaro.org%2Flinaro-
> blog%2F&amp;data=02%7C01%7Canson.huang%40nxp.com%7Cd5ce55aaab
> 4141f5f6e308d7b6ccfe72%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C
> 0%7C637178863864835997&amp;sdata=LCNFfsfYGB1T1IFNdh4dmjZJ8S4NIAv
> iPpihR%2FXaAYA%3D&amp;reserved=0> Blog