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

[Andre Przywara]

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 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.

> 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 will do as much testing now as possible, but I am not happy about that
situation.

Cheers,
Andre