Re: [PATCH 3/7] watchdog: add stm32 watchdog and reset driver

[Ahmad Fatoum <a.fatoum@pengutronix.de>]

Hello,

On 4/6/19 10:24, Sascha Hauer wrote:
> On Mon, Jun 03, 2019 at 09:05:55PM +0200, Ahmad Fatoum wrote:
>> The driver supports setting watchdog timeout, system reset
>> and querying reset reason. Disabling watchdog isn't possible
>> in hardware, thus users should either enable it before boot
>> or have the poller take care of feeding it.
>>
>> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
>> ---
>>  .../mach-stm32mp/include/mach/reset-reason.h  |  28 ++
>>  drivers/watchdog/Kconfig                      |   8 +
>>  drivers/watchdog/Makefile                     |   1 +
>>  drivers/watchdog/stm32_wdt.c                  | 288 ++++++++++++++++++
>>  4 files changed, 325 insertions(+)
>>  create mode 100644 arch/arm/mach-stm32mp/include/mach/reset-reason.h
>>  create mode 100644 drivers/watchdog/stm32_wdt.c
>>
>> diff --git a/arch/arm/mach-stm32mp/include/mach/reset-reason.h b/arch/arm/mach-stm32mp/include/mach/reset-reason.h
>> new file mode 100644
>> index 000000000000..1165b347c31f
>> --- /dev/null
>> +++ b/arch/arm/mach-stm32mp/include/mach/reset-reason.h
>> @@ -0,0 +1,28 @@
>> +#ifndef __MACH_RESET_REASON_H__
>> +#define __MACH_RESET_REASON_H__
>> +
>> +#include <reset_source.h>
>> +
>> +#define RCC_RSTF_POR		BIT(0)
>> +#define RCC_RSTF_BOR		BIT(1)
>> +#define RCC_RSTF_PAD		BIT(2)
>> +#define RCC_RSTF_HCSS		BIT(3)
>> +#define RCC_RSTF_VCORE		BIT(4)
>> +
>> +#define RCC_RSTF_MPSYS		BIT(6)
>> +#define RCC_RSTF_MCSYS		BIT(7)
>> +#define RCC_RSTF_IWDG1		BIT(8)
>> +#define RCC_RSTF_IWDG2		BIT(9)
>> +
>> +#define RCC_RSTF_STDBY		BIT(11)
>> +#define RCC_RSTF_CSTDBY		BIT(12)
>> +#define RCC_RSTF_MPUP0		BIT(13)
>> +#define RCC_RSTF_MPUP1		BIT(14)
>> +
>> +struct stm32_reset_reason {
>> +	uint32_t mask;
>> +	enum reset_src_type type;
>> +	int instance;
>> +};
> 
> This is used in the driver only. Why is this in a header file?

The driver is applicable to both stm32 and stm32mp, but the reasons
differ in bit mask. A future mach-stm32 could reuse the driver,
but define its own reset-reason.h. What do you think?

> 
>> +
>> +#endif
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 04efb1a3c866..5a28b530099d 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -81,4 +81,12 @@ config RAVE_SP_WATCHDOG
>>  	depends on RAVE_SP_CORE
>>  	help
>>  	  Support for the watchdog on RAVE SP device.
>> +
>> +config STM32_IWDG_WATCHDOG
>> +        bool "Enable IWDG watchdog driver for STM32 processors"
> 
> spaces instead of tabs here.
> 
>> +	depends on ARCH_STM32MP
>> +	select MFD_SYSCON
>> +        help
>> +                Enable the STM32 watchdog (IWDG) driver. Enable support to
>> +                configure STM32's on-SoC watchdog.
>>  endif
>> +
>> +static int stm32_iwdg_probe(struct device_d *dev)
>> +{
>> +	struct stm32_iwdg_data *data;
>> +	struct stm32_iwdg *wd;
>> +	struct resource *res;
>> +	struct watchdog *wdd;
>> +	struct clk *clk;
>> +	int ret;
>> +
>> +	wd = xzalloc(sizeof(*wd));
>> +
>> +	ret = dev_get_drvdata(dev, (const void **)&data);
>> +	if (ret)
>> +		return -ENODEV;
>> +
>> +	res = dev_request_mem_resource(dev, 0);
>> +	if (IS_ERR(res)) {
>> +		dev_err(dev, "could not get timer memory region\n");
>> +		return PTR_ERR(res);
>> +	}
>> +	wd->iwdg_base = IOMEM(res->start);
>> +
>> +	clk = of_clk_get_by_name(dev->device_node, "lsi");
> 
> This function is only to be used when there's no struct device_d *
> available. clk_get() is what you want to use here.

ok

> 
>> +	if (IS_ERR(clk))
>> +		return PTR_ERR(clk);
>> +
>> +	ret = clk_enable(clk);
>> +	if (ret)
>> +		return ret;
>> +
>> +	wd->rate = clk_get_rate(clk);
>> +
>> +	if (data->has_pclk) {
>> +		clk = of_clk_get_by_name(dev->device_node, "pclk");
>> +		if (IS_ERR(clk))
>> +			return PTR_ERR(clk);
>> +
>> +		ret = clk_enable(clk);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	wdd              = &wd->wdd;
>> +	wdd->hwdev       = dev;
>> +	wdd->set_timeout = stm32_iwdg_set_timeout;
>> +	wdd->timeout_max = (RLR_MAX + 1) * data->max_prescaler * 1000;
>> +	wdd->timeout_max /= wd->rate * 1000;
>> +	wdd->timeout_cur = wdd->timeout_max;
> 
> Please don't do such alignment things. When somebody has to change
> something here he only has the choice of breaking the alignment or
> changing unrelated lines.

ok

> 
>> +
>> +	ret = stm32_iwdg_set_timeout(wdd, wdd->timeout_max);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to set initial watchdog timeout\n");
>> +		return ret;
>> +	}
>> +
>> +	ret = watchdog_register(wdd);
>> +	if (ret) {
>> +		dev_err(dev, "Failed to register watchdog device\n");
>> +		return ret;
>> +	}
>> +
>> +	wd->restart.name = "stm32-iwdg";
>> +	wd->restart.restart = stm32_iwdg_restart_handler;
>> +	wd->restart.priority = 200;
>> +
>> +	wd->rcc_regmap = syscon_regmap_lookup_by_compatible("st,stm32mp1-rcc");
>> +	if (IS_ERR(wd->rcc_regmap)) {
>> +		dev_warn(dev, "Cannot register restart handler\n");
>> +		goto end;
>> +	}
>> +
>> +	ret = restart_handler_register(&wd->restart);
>> +	if (ret) {
>> +		dev_warn(dev, "Cannot register restart handler\n");
>> +		goto end;
>> +	}
>> +
>> +	ret = stm32_set_reset_reason(wd->rcc_regmap);
>> +	if (ret) {
>> +		dev_warn(dev, "Cannot determine reset reason\n");
>> +		goto end;
>> +	}
> 
> Are the last three failures really failures? You skip the remaining
> steps but still you return with success which looks inconsistent.

Right, restart handler failure shouldn't affect setting the reset
reason. I'll correct this.

I would keep the warnings, because I think it's appropriate to inform the
user that functionality they expected to be available isn't.
e.g. dw_wdt does this, imx-wd doesn't.

> 
>> +end:
>> +	dev_info(dev, "probed\n");
>> +	return 0;
>> +}
> 
> Sascha
> 
> 

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox