[PATCH v3 0/6] Improved sysreset/watchdog uclass integration

Fri Nov 5 21:38:50 CET 2021

On 11/5/21 20:17, Tom Rini wrote:
> On Fri, Nov 05, 2021 at 07:37:02PM +0100, Heinrich Schuchardt wrote:
>> On 11/5/21 17:12, Simon Glass wrote:
>>> Hi,
>>>
>>> On Fri, 5 Nov 2021 at 08:21, Tom Rini <trini at konsulko.com> wrote:
>>>>
>>>> On Fri, Nov 05, 2021 at 12:14:47PM +0100, Stefan Roese wrote:
>>>>> Hi Andre,
>>>>>
>>>>> Added Tom to Cc.
>>>>>
>>>>> On 05.11.21 11:04, Andre Przywara wrote:
>>>>>> On Thu, 4 Nov 2021 20:02:41 -0600
>>>>>> Simon Glass <sjg at chromium.org> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> On Thu, 4 Nov 2021 at 19:22, Stefan Roese <sr at denx.de> wrote:
>>>>>>>>
>>>>>>>> Hi Andre,
>>>>>>>>
>>>>>>>> On 05.11.21 00:11, Andre Przywara wrote:
>>>>>>>>> On Thu, 4 Nov 2021 11:37:57 +0100
>>>>>>>>> Stefan Roese <sr at denx.de> wrote:
>>>>>>>>>
>>>>>>>>> Hi Stefan,
>>>>>>>>>> On 04.11.21 04:55, Samuel Holland wrote:
>>>>>>>>>>> This series hooks up the watchdog uclass to automatically register
>>>>>>>>>>> watchdog devices for use with sysreset, doing a bit of minor cleanup
>>>>>>>>>>> along the way.
>>>>>>>>>>>
>>>>>>>>>>> The goal is for this to replace the sunxi board-level non-DM reset_cpu()
>>>>>>>>>>> function. I was surprised to find that the wdt_reboot driver requires
>>>>>>>>>>> its own undocumented device tree node, which references the watchdog
>>>>>>>>>>> device by phandle. This is problematic for us, because sunxi-u-boot.dtsi
>>>>>>>>>>> file covers 20 different SoCs with varying watchdog node phandle names.
>>>>>>>>>>> So it would have required adding a -u-boot.dtsi file for each board.
>>>>>>>>>>>
>>>>>>>>>>> Hooking things up automatically makes sense to me; this is what Linux
>>>>>>>>>>> does. However, I put the code behind a new option to avoid surprises for
>>>>>>>>>>> other platforms.
>>>>>>>>>>>
>>>>>>>>>>> Changes in v3:
>>>>>>>>>>>       - Move condition to wdt-uclass.c to fix build errors.
>>>>>>>>>>>       - Include watchdog name in error message.
>>>>>>>>>>>
>>>>>>>>>>> Changes in v2:
>>>>>>>>>>>       - Extend the "if SYSRESET" block to the end of the file.
>>>>>>>>>>>       - Also make gpio_reboot_probe function static.
>>>>>>>>>>>       - Rebase on top of 492ee6b8d0e7 (now handle all watchdogs).
>>>>>>>>>>>       - Added patches 5-6 as an example of how the new option will be used.
>>>>>>>>>>>
>>>>>>>>>>> Samuel Holland (6):
>>>>>>>>>>>        sysreset: Add uclass Kconfig dependency to drivers
>>>>>>>>>>>        sysreset: Mark driver probe functions as static
>>>>>>>>>>>        sysreset: watchdog: Move watchdog reference to plat data
>>>>>>>>>>>        watchdog: Automatically register device with sysreset
>>>>>>>>>>>        sunxi: Avoid duplicate reset_cpu with SYSRESET enabled
>>>>>>>>>>>        sunxi: Use sysreset framework for poweroff/reset
>>>>>>>>>>>
>>>>>>>>>>>       arch/arm/Kconfig                     |  3 +++
>>>>>>>>>>>       arch/arm/mach-sunxi/board.c          |  2 ++
>>>>>>>>>>>       drivers/sysreset/Kconfig             | 11 ++++++--
>>>>>>>>>>>       drivers/sysreset/sysreset_gpio.c     |  2 +-
>>>>>>>>>>>       drivers/sysreset/sysreset_resetctl.c |  2 +-
>>>>>>>>>>>       drivers/sysreset/sysreset_syscon.c   |  2 +-
>>>>>>>>>>>       drivers/sysreset/sysreset_watchdog.c | 40 ++++++++++++++++++++++------
>>>>>>>>>>>       drivers/watchdog/wdt-uclass.c        |  8 ++++++
>>>>>>>>>>>       include/sysreset.h                   | 10 +++++++
>>>>>>>>>>>       9 files changed, 67 insertions(+), 13 deletions(-)
>>>>>>>>>>
>>>>>>>>>> Applied to u-boot-marvell
>>>>>>>>>
>>>>>>>>> Mmmh, why u-boot-marvell,
>>>>>>>>
>>>>>>>> Because I'm handling watchdog related changed since a few years and we
>>>>>>>> did not create a specific subsystem repo for this and I'm usually
>>>>>>>> using my "marvell" one for this.
>>>>
>>>> And fwiw, there's a few other cases like this.  If it's too confusing,
>>>> maybe we should just roll out a few more repositories, I think it's
>>>> easier to do that now than pre-gitlab?
>>>>
>>>>>>>>> and why did this end up already in master?
>>>>>>>>> Isn't that material for the next merge window? After all this changes
>>>>>>>>> quite a bit, for a lot of boards, and I did not have a closer look at
>>>>>>>>> the sunxi parts yet.
>>>>>>>>
>>>>>>>> I was hesitating also a bit. But since this patchset is on the list in
>>>>>>>> v1 since over 2 months now (2021-08-21) I thought it was "ready" for
>>>>>>>> inclusion now. We are at -rc1 and I think we still have enough time to
>>>>>>>> fix any resulting problems in this release cycle.
>>>>>>
>>>>>> Why do we have the merge window then? This is clearly not a regression or
>>>>>> general fix.
>>>>>
>>>>> AFAIU, we are a bit less strict here in U-Boot. Patches that were posted
>>>>> before the merge-window and skipped the review process (most likely
>>>>> because of lack of time) are often still integrated in the early rcX
>>>>> cycles. At least this is how I handle it usually.
>>>>>
>>>>> Tom, is my understanding here correct?
>>>>
>>>> Yes.  We are not as strict as the kernel is about what can come in
>>>> between rc1 and rc2 (and to a certain degree, post rc2).  I leave things
>>>> up to the discretion of the custodians.  People tend of have less time
>>>> to handle U-Boot changes than other stuff, so I try and be flexible in
>>>> picking things up.
>>>>
>>>>>>> Yes I agree, that should be plenty of time for people to review it.
>>>>>>
>>>>>> Well, if there would be people to review the sunxi parts :-(
>>>>>> I am totally fine with the generic patches (as they have been reviewed),
>>>>>> but the sunxi integration is somewhat risky.
>>>>>> I was explicitly deprioritising that in my queue, as it really doesn't
>>>>>> change, add or fix anything, it's mere refactoring, from the user's point
>>>>>> of view.
>>>>>>
>>>>>>>> Do you see any specific issues?
>>>>>>
>>>>>> Patch 6/6 changes the config for all 157 Allwinner boards, so I think that
>>>>>> deserves at least some testing, *before* merging it.
>>>>>
>>>>> I expect that Samuel did some testing. But still, I agree that it
>>>>> would be much better, if these patches - especially the Allwinner parts
>>>>> got more extensive testing.
>>>>>
>>>>>> I will do as much testing now as possible, but I am not happy about that
>>>>>> situation.
>>>>>
>>>>> Understood. Should we revert patch 6/6 for now?
>>>>
>>>> FWIW, given Samuel has been doing a number of allwinner changes, I had
>>>> also assumed it was sufficiently tested, which is why I didn't raise a
>>>> further concern when I saw the widespread nature of the overall changes,
>>>> just figured it was a few more ready-to-go cleanups that weren't quite
>>>> picked up in time.  Please do speak up if you want me to revert the last
>>>> part.
>>>
>>> Also it is often true that people find problems by testing on master
>>> so applying it helps to shake the tree a bit.
>>>
>>> Regards,
>>> Simon
>>>
>>
>> We don't actually have a problem with this series but with a previous
>> watchdog patch. The culprit according to bisecting is:
>>
>> b147bd3607f8 ("sunxi: Enable watchdog timer support by default")
>>
>> When booting the OrangePi PC the watchdog triggers while Linux is booting,
>> ca. 16 s after leaving the UEFI subsystem. This matches WDT_MAX_TIMEOUT in
>> drivers/watchdog/sunxi_wdt.c.
>>
>> If I run
>>
>> => wdt dev watchdog at 1c20ca0
>> => wdt stop
>>
>> before the bootefi command booting succeeds.
>>
>> We don't disarm the watchdog and Linux does not do it for us in time.
>>
>> The UEFI specification requires that the default watchdog reset time is 300
>> s. We should never arm the Sunxi hardware watchdog except within the
>> watchdog reset driver.
>>
>> The solution is to disable CONFIG_WATCHDOG_AUTOSTART on SUNXI. See
>>
>> [PATCH 1/1] watchdog: don't autostart watchdog on Sunxi boards
>> https://lists.denx.de/pipermail/u-boot/2021-November/466318.html
> 
> This means we never did come up with a satisfactory to everyone solution
> to what UEFI thinks a watchdog should do, and what other types of
> deployment think a watchdog should do, yes?
> 

Dear Tom,

The issue is *not* UEFI specific.

A watchdog timeout of 16 seconds is too short for Linux to boot no 
matter whether you use the EFI stub or the legacy entry point.

I only referred to the UEFI specification as it indicates what can be 
considered as a reasonable timeout interval: 300 seconds.

Best regards

Heinrich