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

[Anson Huang]

Hi, Guenter

Best Regards!
Anson Huang

> -----Original Message-----
> From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of Guenter
> Roeck
> Sent: 2019å3æ1æ 12:21
> To: Anson Huang <anson.huang@xxxxxxx>; catalin.marinas@xxxxxxx;
> will.deacon@xxxxxxx; wim@xxxxxxxxxxxxxxxxxx; shawnguo@xxxxxxxxxx;
> s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> Andy Gross <andy.gross@xxxxxxxxxx>; heiko@xxxxxxxxx;
> horms+renesas@xxxxxxxxxxxx; arnd@xxxxxxxx; olof@xxxxxxxxx;
> jagan@xxxxxxxxxxxxxxxxxxxx; bjorn.andersson@xxxxxxxxxx;
> enric.balletbo@xxxxxxxxxxxxx; marc.w.gonzalez@xxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> watchdog@xxxxxxxxxxxxxxx
> Cc: dl-linux-imx <linux-imx@xxxxxxx>
> Subject: Re: [PATCH V4 1/2] watchdog: imx_sc: Add i.MX system controller
> watchdog support
> 
> On 2/28/19 7:46 PM, Anson Huang wrote:
> > Hi, Guenter
> >
> > Best Regards!
> > Anson Huang
> >
> >> -----Original Message-----
> >> From: Guenter Roeck [mailto:groeck7@xxxxxxxxx] On Behalf Of Guenter
> >> Roeck
> >> Sent: 2019å2æ28æ 22:58
> >> To: Anson Huang <anson.huang@xxxxxxx>; catalin.marinas@xxxxxxx;
> >> will.deacon@xxxxxxx; wim@xxxxxxxxxxxxxxxxxx; shawnguo@xxxxxxxxxx;
> >> s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx;
> >> Andy Gross <andy.gross@xxxxxxxxxx>; heiko@xxxxxxxxx;
> >> horms+renesas@xxxxxxxxxxxx; arnd@xxxxxxxx; olof@xxxxxxxxx;
> >> jagan@xxxxxxxxxxxxxxxxxxxx; bjorn.andersson@xxxxxxxxxx;
> >> enric.balletbo@xxxxxxxxxxxxx; marc.w.gonzalez@xxxxxxx; linux-arm-
> >> kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-
> >> watchdog@xxxxxxxxxxxxxxx
> >> Cc: dl-linux-imx <linux-imx@xxxxxxx>
> >> 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@xxxxxxx>
> >>> ---
> >>> 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.

Many thanks for your detail info, I will add depends on " HAVE_ARM_SMCCC "
in V5 patch series.

Anson.

> 
> Guenter