Re: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller watchdog support

[Guenter Roeck <linux@roeck-us.net>]

On 2/28/19 7:46 PM, Anson Huang wrote:
> Hi, Guenter
> 
> Best Regards!
> Anson Huang
> 
>> -----Original Message-----
>> From: Guenter Roeck [mailto:groeck7@gmail.com] On Behalf Of Guenter
>> Roeck
>> Sent: 2019年2月28日 22:58
>> To: Anson Huang <anson.huang@nxp.com>; catalin.marinas@arm.com;
>> will.deacon@arm.com; wim@linux-watchdog.org; shawnguo@kernel.org;
>> s.hauer@pengutronix.de; kernel@pengutronix.de; festevam@gmail.com;
>> Andy Gross <andy.gross@linaro.org>; heiko@sntech.de;
>> horms+renesas@verge.net.au; arnd@arndb.de; olof@lixom.net;
>> jagan@amarulasolutions.com; bjorn.andersson@linaro.org;
>> enric.balletbo@collabora.com; marc.w.gonzalez@free.fr; linux-arm-
>> kernel@lists.infradead.org; linux-kernel@vger.kernel.org; linux-
>> watchdog@vger.kernel.org
>> Cc: dl-linux-imx <linux-imx@nxp.com>
>> Subject: Re: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller
>> watchdog support
>>
>> On 2/27/19 11:59 PM, 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 watchdog etc..
>>>
>>> This patch adds i.MX system controller watchdog driver support,
>>> watchdog operation needs to be done in secure EL3 mode via
>>> ARM-Trusted-Firmware, using SMC call, CPU will trap into
>>> ARM-Trusted-Firmware and then it will request system controller to do
>>> watchdog operation via IPC.
>>>
>>> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
>>> ---
>>> Changes since V3:
>>>           - add ARM64 dependency for this driver;
>>>           - change SPDX license to GPL-2.0 to match module license;
>>>           - register platform device in driver instead of getting from dts;
>>> 	- return linux error code instead of system controller firmware error
>> code for watchdog operation
>>> 	  failed case.
>>> ---
>>>    drivers/watchdog/Kconfig      |  13 +++
>>>    drivers/watchdog/Makefile     |   1 +
>>>    drivers/watchdog/imx_sc_wdt.c | 201
>> ++++++++++++++++++++++++++++++++++++++++++
>>>    3 files changed, 215 insertions(+)
>>>    create mode 100644 drivers/watchdog/imx_sc_wdt.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
>>> 65c3c42..8c6575e 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -625,6 +625,19 @@ config IMX2_WDT
>>>    	  To compile this driver as a module, choose M here: the
>>>    	  module will be called imx2_wdt.
>>>
>>> +config IMX_SC_WDT
>>> +	tristate "IMX SC Watchdog"
>>> +	depends on (ARCH_MXC && ARM64) || COMPILE_TEST
>>> +	select WATCHDOG_CORE
>>> +	help
>>> +	  This is the driver for the system controller watchdog
>>> +	  on the NXP i.MX SoCs with system controller inside.
>>> +	  If you have one of these processors and wish to have
>>> +	  watchdog support enabled, say Y, otherwise say N.
>>> +
>>> +	  To compile this driver as a module, choose M here: the
>>> +	  module will be called imx_sc_wdt.
>>> +
>>
>> With this patch applied, alpha:allmodconfig and almost all other
>> allmodconfig/allyesconfig builds fail with:
>>
>> ERROR: "__arm_smccc_smc" [drivers/watchdog/imx_sc_wdt.ko] undefined!
>> scripts/Makefile.modpost:92: recipe for target '__modpost' failed
>> make[1]: *** [__modpost] Error 1
>> Makefile:1260: recipe for target 'modules' failed
>>
>> as I told you before would happen. For the future, please consider that
>> forcing me to "prove" such failures does take a significant amount of time,
>> which is not always readily available.
> 
> Sorry for wasting your time, it is just because I misunderstand your point, NOT
> that I did NOT force you to prove it.
> 
> I am a little confuse, since this is new to me about these configs, I looked into
> other drivers which also use arm_smccc_smc, they do NOT add special config
> dependency for the failure build cause as you said, can you be more detail about
> this build issue, I tried below build, but the failure I met is other, so could you please
> advise how to fix such dependency issue, adding dummy function looks like NOT
> a good way since this arm_smccc_smc() API is widely used in kernel, there should be
> some common solution for it, Thanks in advanced and appreciate for your time.
> 

I am quite sure that the other drivers calling arm_smccc_smc don't have
"|| COMPILE_TEST" as dependency, or they have a secondary dependency.

Randomly picking some

RTC_DRV_IMX_SC - no.
ARM_RK3399_DMC_DEVFREQ - no.
PHY_MVEBU_A3700_COMPHY - yes, but it also has "depends on HAVE_ARM_SMCCC" as
separate line, limiting its scope.

You can either drop "|| COMPILE_TEST" or add "depends on HAVE_ARM_SMCCC"
in a separate line. There may be other options, but I did not explore them.

Guenter