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
