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@xxxxxxxxxxxxxx>
>> ---
>> .../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@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/barebox
