| Message ID | 20230713095127.1230109-1-huaqian.li@siemens.com |
|---|---|
| Headers | show |
| Series | Add support for WDIOF_CARDRESET on TI AM65x | expand |
On 13/07/2023 11:51, huaqian.li@siemens.com wrote: > From: Li Hua Qian <huaqian.li@siemens.com> > > TI RTI (Real Time Interrupt) Watchdog doesn't support to record the > watchdog cause. Add a reserved memory to know the last reboot was caused > by the watchdog card. In the reserved memory, some specific info will be > saved to indicate whether the watchdog reset was triggered in last > boot. > > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com> > --- > .../devicetree/bindings/watchdog/ti,rti-wdt.yaml | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml > index fc553211e42d..8c16fd3929ec 100644 > --- a/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml > +++ b/Documentation/devicetree/bindings/watchdog/ti,rti-wdt.yaml > @@ -34,6 +34,18 @@ properties: > power-domains: > maxItems: 1 > > + memory-region: > + maxItems: 1 > + description: > + Contains the watchdog reserved memory. It is optional. > + In the reserved memory, the specified values, which are > + PON_REASON_SOF_NUM(0xBBBBCCCC), PON_REASON_MAGIC_NUM(0xDDDDDDDD), > + and PON_REASON_EOF_NUM(0xCCCCBBBB), are pre-stored at the first > + 3 * 4 bytes to tell that last boot was caused by watchdog reset. > + Once the PON reason is captured by driver(rti_wdt.c), the driver > + is supposed to wipe the whole memory region. > + > + If there is going to be new version, only one blank line, not two. In any case: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On 7/13/23 02:51, huaqian.li@siemens.com wrote: > From: Li Hua Qian <huaqian.li@siemens.com> > > This patch adds the WDIOF_CARDRESET support for the platform watchdog > whose hardware does not support this feature, to know if the board > reboot is due to a watchdog reset. > > This is done via reserved memory(RAM), which indicates if specific > info saved, triggering the watchdog reset in last boot. > > Signed-off-by: Li Hua Qian <huaqian.li@siemens.com> > --- > drivers/watchdog/rti_wdt.c | 51 ++++++++++++++++++++++++++++++++++++++ > 1 file changed, 51 insertions(+) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index ce8f18e93aa9..b9435b972cb9 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -18,6 +18,8 @@ > #include <linux/pm_runtime.h> > #include <linux/types.h> > #include <linux/watchdog.h> > +#include <linux/of_address.h> > +#include <linux/of.h> > This driver so far managed to keep include files in alphabetic order. Please keep it that way. > #define DEFAULT_HEARTBEAT 60 > > @@ -52,6 +54,11 @@ > > #define DWDST BIT(1) > > +#define PON_REASON_SOF_NUM 0xBBBBCCCC > +#define PON_REASON_MAGIC_NUM 0xDDDDDDDD > +#define PON_REASON_EOF_NUM 0xCCCCBBBB > +#define RESERVED_MEM_MIN_SIZE 12 > + > static int heartbeat = DEFAULT_HEARTBEAT; > > /* > @@ -198,6 +205,11 @@ static int rti_wdt_probe(struct platform_device *pdev) > struct rti_wdt_device *wdt; > struct clk *clk; > u32 last_ping = 0; > + struct device_node *node; > + u32 reserved_mem_size; > + struct resource res; > + u32 *vaddr; > + u64 paddr; > > wdt = devm_kzalloc(dev, sizeof(*wdt), GFP_KERNEL); > if (!wdt) > @@ -284,6 +296,45 @@ static int rti_wdt_probe(struct platform_device *pdev) > } > } > > + node = of_parse_phandle(pdev->dev.of_node, "memory-region", 0); > + if (!node) { > + dev_dbg(dev, "No memory-region specified.\n"); If you really want that debug message, please keep the action part first. I personally think it is just noise; the devicetree can always be looked up if needed. > + } else { > + ret = of_address_to_resource(node, 0, &res); > + if (ret) { > + dev_err(dev, "No memory address assigned to the region.\n"); > + goto err_iomap; > + } > + > + /* > + * If reserved memory is defined for watchdog reset cause. > + * Readout the Power-on(PON) reason and pass to bootstatus. > + */ > + paddr = res.start; > + reserved_mem_size = res.end - (res.start - 1); Please use resource_size(). > + if (reserved_mem_size < RESERVED_MEM_MIN_SIZE) { > + dev_err(dev, "The size of reserved memory is too small.\n"); > + ret = -EINVAL; > + goto err_iomap; > + } > + > + vaddr = memremap(paddr, reserved_mem_size, MEMREMAP_WB); > + if (vaddr == NULL) { > + dev_err(dev, "Failed to map memory-region.\n"); > + ret = -ENOMEM; > + goto err_iomap; > + } > + > + if (vaddr[0] == PON_REASON_SOF_NUM && > + vaddr[1] == PON_REASON_MAGIC_NUM && > + vaddr[2] == PON_REASON_EOF_NUM) { > + dev_dbg(dev, "Watchdog reset cause detected.\n"); Isn't that a bit pointless ? That is obvious when reading the boot status. > + wdd->bootstatus |= WDIOF_CARDRESET; > + } > + memset(vaddr, 0, reserved_mem_size); > + memunmap(vaddr); > + } > + > watchdog_init_timeout(wdd, heartbeat, dev); > > ret = watchdog_register_device(wdd);
On 7/13/23 02:51, huaqian.li@siemens.com wrote: > From: Li Hua Qian <huaqian.li@siemens.com> > > The watchdog hardware of TI AM65X platform does not support > WDIOF_CARDRESET feature, add a reserved memory to save the watchdog > reset cause, to know if the board reboot is due to a watchdog reset. > One thing I keep wondering about: What prevents the Linux kernel from treating the special memory area like normal memory ? I would have expected some usage note, such as that the memory area must be reported as reserved to the kernel, but I don't see anything like that. Guenter > Changes in v3: > - Add memory-region back for the reserved memory, and remove reserved > memory from the watchdog IO address space. > - Add changelog. > - Link to v2: > https://lore.kernel.org/linux-watchdog/20230711091713.1113010-1-huaqian.li@siemens.com > > Changes in v2: > - Remove memory-region and memory-size properties, and bind the reserved > memory to watchdog IO address space. > - Remove the unnecessary rti_wdt_ioctl. > - Fix the mail list > - Link to v1: > https://lore.kernel.org/all/3137d87e56ef75ba0b8a923d407b2fecace6ccbd.camel@siemens.com/ > v1 had a wrong mail list at the beginning, and the mail thread was > messed up. > > Li Hua Qian (3): > dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET > arm64: dts: ti: Add reserved memory for watchdog > watchdog:rit_wdt: Add support for WDIOF_CARDRESET > > .../bindings/watchdog/ti,rti-wdt.yaml | 12 +++++ > .../boot/dts/ti/k3-am65-iot2050-common.dtsi | 10 ++++ > drivers/watchdog/rti_wdt.c | 51 +++++++++++++++++++ > 3 files changed, 73 insertions(+) >
On Thu, 2023-07-13 at 10:21 -0700, Guenter Roeck wrote: > On 7/13/23 02:51, huaqian.li@siemens.com wrote: > > From: Li Hua Qian <huaqian.li@siemens.com> > > > > The watchdog hardware of TI AM65X platform does not support > > WDIOF_CARDRESET feature, add a reserved memory to save the watchdog > > reset cause, to know if the board reboot is due to a watchdog > > reset. > > > > One thing I keep wondering about: What prevents the Linux kernel from > treating the special memory area like normal memory ? I would have > expected > some usage note, such as that the memory area must be reported as > reserved > to the kernel, but I don't see anything like that. > > Guenter Could you help to suggest how to handle it? I am not sure where is a good place to write the usage note. I am thinking to add it in v4 to DT binding. Best regards, Li Hua Qian > > > Changes in v3: > > - Add memory-region back for the reserved memory, and remove > > reserved > > memory from the watchdog IO address space. > > - Add changelog. > > - Link to v2: > > > > https://lore.kernel.org/linux-watchdog/20230711091713.1113010-1-huaqian.li@siemens.com > > > > Changes in v2: > > - Remove memory-region and memory-size properties, and bind the > > reserved > > memory to watchdog IO address space. > > - Remove the unnecessary rti_wdt_ioctl. > > - Fix the mail list > > - Link to v1: > > > > https://lore.kernel.org/all/3137d87e56ef75ba0b8a923d407b2fecace6ccbd.camel@siemens.com/ > > v1 had a wrong mail list at the beginning, and the mail thread > > was > > messed up. > > > > Li Hua Qian (3): > > dt-bindings: watchdog: ti,rti-wdt: Add support for > > WDIOF_CARDRESET > > arm64: dts: ti: Add reserved memory for watchdog > > watchdog:rit_wdt: Add support for WDIOF_CARDRESET > > > > .../bindings/watchdog/ti,rti-wdt.yaml | 12 +++++ > > .../boot/dts/ti/k3-am65-iot2050-common.dtsi | 10 ++++ > > drivers/watchdog/rti_wdt.c | 51 > > +++++++++++++++++++ > > 3 files changed, 73 insertions(+) > > >
From: Li Hua Qian <huaqian.li@siemens.com> The watchdog hardware of TI AM65X platform does not support WDIOF_CARDRESET feature, add a reserved memory to save the watchdog reset cause, to know if the board reboot is due to a watchdog reset. Changes in v3: - Add memory-region back for the reserved memory, and remove reserved memory from the watchdog IO address space. - Add changelog. - Link to v2: https://lore.kernel.org/linux-watchdog/20230711091713.1113010-1-huaqian.li@siemens.com Changes in v2: - Remove memory-region and memory-size properties, and bind the reserved memory to watchdog IO address space. - Remove the unnecessary rti_wdt_ioctl. - Fix the mail list - Link to v1: https://lore.kernel.org/all/3137d87e56ef75ba0b8a923d407b2fecace6ccbd.camel@siemens.com/ v1 had a wrong mail list at the beginning, and the mail thread was messed up. Li Hua Qian (3): dt-bindings: watchdog: ti,rti-wdt: Add support for WDIOF_CARDRESET arm64: dts: ti: Add reserved memory for watchdog watchdog:rit_wdt: Add support for WDIOF_CARDRESET .../bindings/watchdog/ti,rti-wdt.yaml | 12 +++++ .../boot/dts/ti/k3-am65-iot2050-common.dtsi | 10 ++++ drivers/watchdog/rti_wdt.c | 51 +++++++++++++++++++ 3 files changed, 73 insertions(+)