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

[Daniel Lezcano]

On 22/02/2020 00:46, Anson Huang wrote:
> 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.

Thanks for the clarification.

>> [ ... ]
>>
>>> +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.

Ah, right. Everything is handled by "devm_"


>>> +		.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");
>

-- 
 <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