[PATCH 6/7] ARM: s3c64xx: Switch to generic watchdog driver reset

Thu Jul 30 07:45:15 EDT 2020

On Thu, Jul 30, 2020 at 01:34:38PM +0200, Tomasz Figa wrote:
> 2020年7月29日(水) 21:15 Krzysztof Kozlowski <krzk at kernel.org>:
> >
> > On Wed, Jul 29, 2020 at 07:33:33PM +0200, Tomasz Figa wrote:
> > > Hi Krzysztof,
> > >
> > > 2020年7月29日(水) 18:11 Krzysztof Kozlowski <krzk at kernel.org>:
> > > >
> > > > Similarly to commit f6361c6b3880 ("ARM: S3C24XX: remove separate restart
> > > > code"), the platform watchdog reset code can be removed in favor of
> > > > a generic watchdog driver which already handles reset.
> > > >
> > > > This allows removal of a bunch of machine code and fixes also W=1
> > > > compile warnings:
> > > >
> > > >     arch/arm/plat-samsung/watchdog-reset.c:29:6: warning: no previous prototype for 'samsung_wdt_reset' [-Wmissing-prototypes]
> > > >        29 | void samsung_wdt_reset(void)
> > > >           |      ^~~~~~~~~~~~~~~~~
> > > >     arch/arm/plat-samsung/watchdog-reset.c:69:13: warning: no previous prototype for 'samsung_wdt_reset_of_init' [-Wmissing-prototypes]
> > > >        69 | void __init samsung_wdt_reset_of_init(void)
> > > >           |             ^~~~~~~~~~~~~~~~~~~~~~~~~
> > > >     arch/arm/plat-samsung/watchdog-reset.c:89:13: warning: no previous prototype for 'samsung_wdt_reset_init' [-Wmissing-prototypes]
> > > >        89 | void __init samsung_wdt_reset_init(void __iomem *base)
> > > >
> > > > Signed-off-by: Krzysztof Kozlowski <krzk at kernel.org>
> > > > ---
> > > >  arch/arm/mach-s3c64xx/Kconfig           |  3 +-
> > > >  arch/arm/mach-s3c64xx/common.c          | 15 +---
> > > >  arch/arm/mach-s3c64xx/common.h          |  2 -
> > > >  arch/arm/mach-s3c64xx/mach-anw6410.c    |  1 -
> > > >  arch/arm/mach-s3c64xx/mach-crag6410.c   |  1 -
> > > >  arch/arm/mach-s3c64xx/mach-hmt.c        |  1 -
> > > >  arch/arm/mach-s3c64xx/mach-mini6410.c   |  1 -
> > > >  arch/arm/mach-s3c64xx/mach-ncp.c        |  1 -
> > > >  arch/arm/mach-s3c64xx/mach-real6410.c   |  1 -
> > > >  arch/arm/mach-s3c64xx/mach-s3c64xx-dt.c | 17 -----
> > > >  arch/arm/mach-s3c64xx/mach-smartq5.c    |  1 -
> > > >  arch/arm/mach-s3c64xx/mach-smartq7.c    |  1 -
> > > >  arch/arm/mach-s3c64xx/mach-smdk6400.c   |  1 -
> > > >  arch/arm/mach-s3c64xx/mach-smdk6410.c   |  1 -
> > > >  arch/arm/mach-s3c64xx/watchdog-reset.h  | 16 -----
> > > >  arch/arm/plat-samsung/Kconfig           |  6 --
> > > >  arch/arm/plat-samsung/Makefile          |  1 -
> > > >  arch/arm/plat-samsung/watchdog-reset.c  | 93 -------------------------
> > > >  18 files changed, 5 insertions(+), 158 deletions(-)
> > > >  delete mode 100644 arch/arm/mach-s3c64xx/watchdog-reset.h
> > > >  delete mode 100644 arch/arm/plat-samsung/watchdog-reset.c
> > > >
> > >
> > > Thanks for the patch! Please see my comments inline.
> > >
> > > > diff --git a/arch/arm/mach-s3c64xx/Kconfig b/arch/arm/mach-s3c64xx/Kconfig
> > > > index e208c2b48853..f3fcb570edf5 100644
> > > > --- a/arch/arm/mach-s3c64xx/Kconfig
> > > > +++ b/arch/arm/mach-s3c64xx/Kconfig
> > > > @@ -18,9 +18,10 @@ menuconfig ARCH_S3C64XX
> > > >         select PM_GENERIC_DOMAINS if PM
> > > >         select S3C_DEV_NAND if ATAGS
> > > >         select S3C_GPIO_TRACK if ATAGS
> > > > +       select S3C2410_WATCHDOG
> > > >         select SAMSUNG_ATAGS if ATAGS
> > > >         select SAMSUNG_WAKEMASK if PM
> > > > -       select SAMSUNG_WDT_RESET
> > > > +       select WATCHDOG
> > > >         help
> > > >           Samsung S3C64XX series based systems
> > > >
> > > > diff --git a/arch/arm/mach-s3c64xx/common.c b/arch/arm/mach-s3c64xx/common.c
> > > > index a655bf0c7802..42e96d196f61 100644
> > > > --- a/arch/arm/mach-s3c64xx/common.c
> > > > +++ b/arch/arm/mach-s3c64xx/common.c
> > > > @@ -50,7 +50,6 @@
> > > >
> > > >  #include "common.h"
> > > >  #include "irq-uart.h"
> > > > -#include "watchdog-reset.h"
> > > >
> > > >  /* External clock frequency */
> > > >  static unsigned long xtal_f __ro_after_init = 12000000;
> > > > @@ -232,10 +231,11 @@ void __init s3c64xx_init_irq(u32 vic0_valid, u32 vic1_valid)
> > > >         /*
> > > >          * FIXME: there is no better place to put this at the moment
> > > >          * (s3c64xx_clk_init needs ioremap and must happen before init_time
> > > > -        * samsung_wdt_reset_init needs clocks)
> > > > +        * samsung_wdt_reset_init needs clocks).  However
> > > > +        * samsung_wdt_reset_init() was removed in favor of watchdog driver
> > > > +        * so this should be revised.
> > >
> > > This leaves the comment referring to an inexistent function.
> >
> > Yes, I left it as a reference/reason. Although might be quite confusing
> > now...
> >
> > >
> > > I wonder if this being here is actually a problem at all. It's legacy
> > > code and probably there isn't much value in reshuffling it further.
> > > Rather than that, we would probably want to make sure that everything
> > > migrated to DT and just drop the board files.
> >
> > Maybe let's remove the FIXME and leave the clock init. Since all these
> > times no one fixed the FIXME, so now with limited hardware access I do
> > not expect any movements here.
> >
> 
> I think that would be fairly uncontroversial at this point.
> 
> > >
> > > >          */
> > > >         s3c64xx_clk_init(NULL, xtal_f, xusbxti_f, soc_is_s3c6400(), S3C_VA_SYS);
> > > > -       samsung_wdt_reset_init(S3C_VA_WATCHDOG);
> > > >
> > > >         printk(KERN_DEBUG "%s: initialising interrupts\n", __func__);
> > > >
> > > > @@ -429,12 +429,3 @@ static int __init s3c64xx_init_irq_eint(void)
> > > >         return 0;
> > > >  }
> > > >  arch_initcall(s3c64xx_init_irq_eint);
> > > > -
> > > > -void s3c64xx_restart(enum reboot_mode mode, const char *cmd)
> > > > -{
> > > > -       if (mode != REBOOT_SOFT)
> > > > -               samsung_wdt_reset();
> > > > -
> > > > -       /* if all else fails, or mode was for soft, jump to 0 */
> > > > -       soft_restart(0);
> > >
> > > Does this remove the soft reboot capability? I'm not sure how much of
> > > a problem that would be, though.
> >
> > 1. No one cared about it in f6361c6b3880 :)
> 
> That was a different SoC family, though.
> 
> > 2. Exynos does not have it
> > 3. Does soft_restart really work? It would be worth to keep it if more
> >    or less it was working.
> 
> Anyway, I guess there is no way to know other than just removing it.
> It can be always brought back if it turns out that someone needs it.
> Please just mention that in the commit message.
> 
> With that addressed:
> 
> Reviewed-by: Tomasz Figa <tomasz.figa at gmail.com>

Sure, I'll do it. Thanks for review.

Best regards,
Krzysztof