[PATCH v1 22/24] watchdog: Add reset support for OcteonTX / TX2

[Stefan Roese]

Hi Simon,

On 31.07.20 16:25, Stefan Roese wrote:
> Hi Simon,
> 
> On 28.07.20 21:01, Simon Glass wrote:
>> On Fri, 24 Jul 2020 at 04:09, Stefan Roese <sr at denx.de> wrote:
>>>
>>> From: Suneel Garapati <sgarapati at marvell.com>
>>>
>>> Adds support for Core 0 watchdog poke on OcteonTX and OcteonTX2
>>> platforms.
>>>
>>> Signed-off-by: Suneel Garapati <sgarapati at marvell.com>
>>> Signed-off-by: Stefan Roese <sr at denx.de>
>>>
>>> ---
>>>
>>> Changes in v1:
>>> - Change patch subject
>>> - Remove inclusion of common.h
>>> - Remove global wdt_dev as its unused
>>> - Remove #ifdef's
>>> - Remove optional fixed register access - only use address passed via
>>>    DT while probing
>>> - Use dev_remap_addr() instead of dev_read_addr_index()
>>>
>>>   drivers/watchdog/Kconfig        | 10 ++++++
>>>   drivers/watchdog/Makefile       |  1 +
>>>   drivers/watchdog/octeontx_wdt.c | 57 +++++++++++++++++++++++++++++++++
>>>   3 files changed, 68 insertions(+)
>>>   create mode 100644 drivers/watchdog/octeontx_wdt.c
>>
>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index bf06180cdd..981b33355d 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -139,6 +139,16 @@ config WDT_MTK
>>>            The watchdog timer is stopped when initialized.
>>>            It performs full SoC reset.
>>>
>>> +config WDT_OCTEONTX
>>> +       bool "OcteonTX core watchdog support"
>>> +       depends on WDT && (ARCH_OCTEONTX || ARCH_OCTEONTX2)
>>> +       default y if WDT && ARCH_OCTEONTX || ARCH_OCTEONTX2
>>> +       imply WATCHDOG
>>> +       help
>>> +         This enables OcteonTX watchdog driver, which can be
>>> +         found on OcteonTX/TX2 chipsets and inline with driver model.
>>> +         Only supports watchdog reset.
>>> +
>>>   config WDT_OMAP3
>>>          bool "TI OMAP watchdog timer support"
>>>          depends on WDT && ARCH_OMAP2PLUS
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 519bbd3a40..fbba0ca386 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -26,6 +26,7 @@ obj-$(CONFIG_WDT_CDNS) += cdns_wdt.o
>>>   obj-$(CONFIG_WDT_MPC8xx) += mpc8xx_wdt.o
>>>   obj-$(CONFIG_WDT_MT7621) += mt7621_wdt.o
>>>   obj-$(CONFIG_WDT_MTK) += mtk_wdt.o
>>> +obj-$(CONFIG_WDT_OCTEONTX) += octeontx_wdt.o
>>>   obj-$(CONFIG_WDT_OMAP3) += omap_wdt.o
>>>   obj-$(CONFIG_WDT_SP805) += sp805_wdt.o
>>>   obj-$(CONFIG_WDT_STM32MP) += stm32mp_wdt.o
>>> diff --git a/drivers/watchdog/octeontx_wdt.c 
>>> b/drivers/watchdog/octeontx_wdt.c
>>> new file mode 100644
>>> index 0000000000..a9c29ef26a
>>> --- /dev/null
>>> +++ b/drivers/watchdog/octeontx_wdt.c
>>> @@ -0,0 +1,57 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * Copyright (C) 2019 Marvell International Ltd.
>>> + *
>>> + * https://spdx.org/licenses
>>> + */
>>> +
>>> +#include <dm.h>
>>> +#include <errno.h>
>>> +#include <wdt.h>
>>> +#include <asm/io.h>
>>> +
>>> +DECLARE_GLOBAL_DATA_PTR;
>>> +
>>> +#define CORE0_POKE_OFFSET 0x50000
>>> +
>>> +struct octeontx_wdt {
>>> +       void __iomem *reg;
>>> +};
>>> +
>>> +static int octeontx_wdt_reset(struct udevice *dev)
>>> +{
>>> +       struct octeontx_wdt *priv = dev_get_priv(dev);
>>> +
>>> +       writeq(~0ULL, ((u64)priv->reg & ~0xfffffULL) | 
>>> CORE0_POKE_OFFSET);
>>
>> Do you actually need the mask? It seems odd that ->reg is part-way
>> through the register set.
> 
> Frankly, I was wondering the same thing. I will double-check before
> submitting the next patchset version.

The register address really needs some "tweaking" in these lower bits
so that it matches the one mentioned in the manual. I've changed the
code slightly, so that the poke register is calculated and stored in
probe() instead of doing it in each _reset() call.

Thanks,
Stefan