lkml.org 
[lkml]   [2020]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 6/7] ARM: s3c64xx: Switch to generic watchdog driver reset
    On Wed, Jul 29, 2020 at 09:15:44PM +0200, Krzysztof Kozlowski wrote:
    > On Wed, Jul 29, 2020 at 07:33:33PM +0200, Tomasz Figa wrote:
    > > Hi Krzysztof,
    > >
    > > 2020年7月29日(水) 18:11 Krzysztof Kozlowski <krzk@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@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.
    >
    > >
    > > > */
    > > > 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 :)
    > 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.

    ... and one more reason:
    4. Look how much code I removed:
    5 insertions(+), 158 deletions(-)
    Isn't it nice? :)

    Best regards,
    Krzysztof

    \
     
     \ /
      Last update: 2020-07-29 21:26    [W:2.492 / U:0.944 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site