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

Andre Przywara andre.przywara at arm.com
Fri Nov 5 18:07:31 CET 2021


On Fri, 5 Nov 2021 10:12:11 -0600
Simon Glass <sjg at chromium.org> 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?

No, that's fine, I was just briefly confused about the Marvell part.

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

Understood.

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

To avoid churn, I am fine with keeping it in. If my testing reveals issue,
we can still revert later.

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

That's all true, but it would still be nice to get at least some ACK or
confirmation from the platform maintainers before merging patches that
affect that many boards.

Cheers,
Andre


More information about the U-Boot mailing list