From: Krzysztof Kozlowski <krzk@kernel.org>
To: Tomasz Figa <tomasz.figa@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>,
Kukjin Kim <kgene@kernel.org>,
Vincent Sanders <vince@simtec.co.uk>,
Simtec Linux Team <linux@simtec.co.uk>,
Kyungmin Park <kyungmin.park@samsung.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Will Deacon <will@kernel.org>,
Sylwester Nawrocki <s.nawrocki@samsung.com>,
Chanwoo Choi <cw00.choi@samsung.com>,
Michael Turquette <mturquette@baylibre.com>,
Stephen Boyd <sboyd@kernel.org>,
Wim Van Sebroeck <wim@linux-watchdog.org>,
Guenter Roeck <linux@roeck-us.net>, Arnd Bergmann <arnd@arndb.de>,
Linus Walleij <linus.walleij@linaro.org>,
linux-kernel <linux-kernel@vger.kernel.org>,
linux-arm-kernel <linux-arm-kernel@lists.infradead.org>,
"moderated list:SAMSUNG SOC CLOCK DRIVERS"
<linux-samsung-soc@vger.kernel.org>,
linux-stm32@st-md-mailman.stormreply.com,
patches@opensource.cirrus.com,
"open list:COMMON CLK FRAMEWORK" <linux-clk@vger.kernel.org>,
linux-watchdog@vger.kernel.org,
Sergio Prado <sergio.prado@e-labworks.com>,
Marek Szyprowski <m.szyprowski@samsung.com>,
Sylwester Nawrocki <snawrocki@kernel.org>,
Cedric Roux <sed@free.fr>, Lihua Yao <ylhuajnu@outlook.com>
Subject: Re: [PATCH 6/7] ARM: s3c64xx: Switch to generic watchdog driver reset
Date: Wed, 29 Jul 2020 21:25:27 +0200 [thread overview]
Message-ID: <20200729192527.GA7351@kozik-lap> (raw)
In-Reply-To: <20200729191544.GB5822@kozik-lap>
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
next prev parent reply other threads:[~2020-07-29 19:25 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-29 16:09 [PATCH 0/7] ARM: samsung: Cleanup of various S3C bits Krzysztof Kozlowski
2020-07-29 16:09 ` [PATCH 1/7] clk: samsung: s3c64xx: Declare s3c64xx_clk_init() in shared header Krzysztof Kozlowski
2020-07-29 17:39 ` Tomasz Figa
2020-07-29 19:42 ` Stephen Boyd
2020-07-29 20:16 ` Krzysztof Kozlowski
2020-07-29 16:09 ` [PATCH 2/7] ARM: s3c64xx: Include header to fix -Wmissing-prototypes Krzysztof Kozlowski
2020-07-29 17:46 ` Tomasz Figa
2020-07-29 16:09 ` [PATCH 3/7] ARM: s3c: Remove plat-samsung/.../samsung-time.h Krzysztof Kozlowski
2020-07-29 17:49 ` Tomasz Figa
2020-07-29 19:02 ` Krzysztof Kozlowski
2020-07-29 16:09 ` [PATCH 4/7] ARM: samsung: Fix language typo Krzysztof Kozlowski
2020-07-29 16:09 ` [PATCH 5/7] ARM: samsung: Kill useless HAVE_S3C2410_WATCHDOG Krzysztof Kozlowski
2020-07-29 17:02 ` Guenter Roeck
2020-07-29 17:36 ` Tomasz Figa
2020-07-29 19:08 ` Krzysztof Kozlowski
2020-07-29 16:09 ` [PATCH 6/7] ARM: s3c64xx: Switch to generic watchdog driver reset Krzysztof Kozlowski
2020-07-29 17:33 ` Tomasz Figa
2020-07-29 19:15 ` Krzysztof Kozlowski
2020-07-29 19:25 ` Krzysztof Kozlowski [this message]
2020-07-30 11:34 ` Tomasz Figa
2020-07-30 11:45 ` Krzysztof Kozlowski
2020-07-29 16:09 ` [PATCH 7/7] ARM: s3c24xx: Fix missing system reset Krzysztof Kozlowski
2020-07-29 20:01 ` [PATCH 0/7] ARM: samsung: Cleanup of various S3C bits Arnd Bergmann
2020-07-29 20:15 ` Krzysztof Kozlowski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20200729192527.GA7351@kozik-lap \
--to=krzk@kernel.org \
--cc=arnd@arndb.de \
--cc=catalin.marinas@arm.com \
--cc=cw00.choi@samsung.com \
--cc=kgene@kernel.org \
--cc=kyungmin.park@samsung.com \
--cc=linus.walleij@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-clk@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-samsung-soc@vger.kernel.org \
--cc=linux-stm32@st-md-mailman.stormreply.com \
--cc=linux-watchdog@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=linux@roeck-us.net \
--cc=linux@simtec.co.uk \
--cc=m.szyprowski@samsung.com \
--cc=mturquette@baylibre.com \
--cc=patches@opensource.cirrus.com \
--cc=s.nawrocki@samsung.com \
--cc=sboyd@kernel.org \
--cc=sed@free.fr \
--cc=sergio.prado@e-labworks.com \
--cc=snawrocki@kernel.org \
--cc=tomasz.figa@gmail.com \
--cc=vince@simtec.co.uk \
--cc=will@kernel.org \
--cc=wim@linux-watchdog.org \
--cc=ylhuajnu@outlook.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).