[PATCH v1 01/22] wdt: imx8qxp: add option to control external PMIC wdt via IMX8 SCU

[Sverdlin, Alexander]

Hi Stefan,

On Fri, 2024-11-08 at 08:19 +0100, Stefan Roese wrote:
> > control the watchdog in the PMIC through SCU.
> 
> A few more details would be good here (please see below).
> 
> > Signed-off-by: Andrej Valek <andrej.valek at siemens.com>
> > Signed-off-by: Heiko Schocher <hs at denx.de>
> > ---
> > 
> >    drivers/misc/imx8/scu_api.c    | 21 ++++++++++++++
> >    drivers/watchdog/Kconfig       |  7 +++++
> >    drivers/watchdog/Makefile      |  1 +
> >    drivers/watchdog/scu_wdt.c     | 52 ++++++++++++++++++++++++++++++++++
> >    include/firmware/imx/sci/rpc.h |  1 +
> >    include/firmware/imx/sci/sci.h |  1 +
> >    6 files changed, 83 insertions(+)
> >    create mode 100644 drivers/watchdog/scu_wdt.c

[]
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 0e45f0a0922..eb9e75abe48 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -428,6 +428,13 @@ config WDT_ARM_SMC
> >    	  Select this to enable Arm SMC watchdog timer. This watchdog will manage
> >    	  a watchdog based on ARM SMCCC communication.
> >    
> > +config WDT_IMX_SCU
> > +	bool "Enable external Watchdog Timer support for IMX"
> > +	depends on IMX8 && WDT
> > +	help
> > +	  Select this to enable the external IMX watchdog driver
> > +	  controlled via IMX8 SCU.
> 
> I don't know the details here, but what exactly is "external" here?
> If it's still inside the i.MX then external sounds a bit misleading.

In this case the WDT used in in PMIC chip, external to i.MX8 SoC, while
imx SCU firmware only acts as a proxy. Maybe something like s/external/PMIC/
would make more sense...

> > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> > index 0b107c008f7..e943b904bae 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -33,6 +33,7 @@ obj-$(CONFIG_WDT_DA9063) += da9063-wdt.o
> >    obj-$(CONFIG_WDT_DAVINCI) += davinci_wdt.o
> >    obj-$(CONFIG_WDT_FTWDT010) += ftwdt010_wdt.o
> >    obj-$(CONFIG_$(SPL_TPL_)WDT_GPIO) += gpio_wdt.o
> > +obj-$(CONFIG_WDT_IMX_SCU) += scu_wdt.o
> 
> Perhaps better to add some "imx" to the source file name?
> 

Or maybe even "siemens", as we will see below...

> > diff --git a/drivers/watchdog/scu_wdt.c b/drivers/watchdog/scu_wdt.c
> > new file mode 100644
> > index 00000000000..ad8766dc132
> > --- /dev/null
> > +++ b/drivers/watchdog/scu_wdt.c
> > @@ -0,0 +1,52 @@
> > +// SPDX-License-Identifier: GPL-2.0-or-later
> > +
> > +#include <dm.h>
> > +#include <wdt.h>
> > +#include <firmware/imx/sci/sci.h>
> > +
> > +/* watchdog commands */
> > +#define CMD_START_WDT 0x55
> > +#define CMD_STOP_WDT  0x45
> > +#define CMD_PING_WDT  0x35
> > +
> > +static int scu_wdt_start(struct udevice *dev, u64 timeout_ms, ulong flags)
> > +{
> > +	/* start external watchdog via Timer API */
> > +	return sc_timer_control_pmic_wdog(-1, CMD_START_WDT);
> > +}
> > +
> > +static int scu_wdt_stop(struct udevice *dev)
> > +{
> > +	/* stop external watchdog via Timer API */
> > +	return sc_timer_control_pmic_wdog(-1, CMD_STOP_WDT);
> > +}
> > +
> > +static int scu_wdt_reset(struct udevice *dev)
> > +{
> > +	return sc_timer_control_pmic_wdog(-1, CMD_PING_WDT);
> > +}
> > +
> > +static int scu_wdt_probe(struct udevice *dev)
> > +{
> > +	debug("%s(dev=%p)\n", __func__, dev);
> > +	return 0;
> > +}
> > +
> > +static const struct wdt_ops scu_wdt_ops = {
> > +	.reset		= scu_wdt_reset,
> > +	.start		= scu_wdt_start,
> > +	.stop		= scu_wdt_stop,
> > +};
> > +
> > +static const struct udevice_id scu_wdt_ids[] = {
> > +	{ .compatible = "siemens,scu-wdt" },
> 
> Really siemens specific? Is this also available in Linux btw?
> Looking into the Linux source, is this driver somehow related to
> this one:
> 
> drivers/watchdog/imx_sc_wdt.c ?

[]

In the rpc.h below new RPC func is being added (TIMER_FUNC_CTRL_PMIC_WDOG),
this one is exclusive to the SCU FW we build internally based on NXP
provided material. We neither have the full source code, nor will we be
able to re-distribute the source code for the firmware. So, yes, this
turns to be Siemens-specific driver. It does look similar to the imx_sc_wdt.c
from Linux only because it indeed uses the same RPC API, but actually
a new function added on top. The Linux driver uses the firmware WDT,
implemented in SCU FW. So our build of SCU FW has actually support for
two WDT variants and the new U-Boot driver provides the support for
our proprietary one. 

> > diff --git a/include/firmware/imx/sci/rpc.h b/include/firmware/imx/sci/rpc.h
> > index 28adec2a8e1..9470f9e7178 100644
> > --- a/include/firmware/imx/sci/rpc.h
> > +++ b/include/firmware/imx/sci/rpc.h
> > @@ -230,5 +230,6 @@ struct sc_rpc_msg_s {
> >    #define TIMER_FUNC_SET_SYSCTR_ALARM 16U /* Index for sc_timer_set_sysctr_alarm() RPC call */
> >    #define TIMER_FUNC_SET_SYSCTR_PERIODIC_ALARM 17U /* Index for sc_timer_set_sysctr_periodic_alarm() RPC call */
> >    #define TIMER_FUNC_CANCEL_SYSCTR_ALARM 18U /* Index for sc_timer_cancel_sysctr_alarm() RPC call */
> > +#define TIMER_FUNC_CTRL_PMIC_WDOG 20U  /*!< Index for sc_timer_ctrl_pmic_wdog() RPC call */
> >    
> >    #endif /* SC_RPC_H */
> > diff --git a/include/firmware/imx/sci/sci.h b/include/firmware/imx/sci/sci.h
> > index 7d8499f070a..b971b62836d 100644
> > --- a/include/firmware/imx/sci/sci.h
> > +++ b/include/firmware/imx/sci/sci.h
> > @@ -123,6 +123,7 @@ int sc_rm_set_master_sid(sc_ipc_t ipc, sc_rsrc_t resource, sc_rm_sid_t sid);
> >    
> >    /* Timer API */
> >    int sc_timer_set_wdog_window(sc_ipc_t ipc, sc_timer_wdog_time_t window);
> > +int sc_timer_control_pmic_wdog(sc_ipc_t ipc, u8 cmd);
> >    
> >    /* SECO API */
> >    int sc_seco_authenticate(sc_ipc_t ipc, sc_seco_auth_cmd_t cmd,

-- 
Alexander Sverdlin
Siemens AG
www.siemens.com