| Message ID | 20211121165647.26706-10-semen.protsenko@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: s3c2410: Add Exynos850 support | expand |
On 11/23/21 8:17 AM, Sam Protsenko wrote: > On Tue, 23 Nov 2021 at 18:06, Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Sun, Nov 21, 2021 at 06:56:44PM +0200, Sam Protsenko wrote: >>> Now that PMU enablement code was extended for new Exynos SoCs, it >>> doesn't look very cohesive and consistent anymore. Do a bit of renaming, >>> grouping and style changes, to make it look good again. While at it, add >>> quirks documentation as well. >>> >>> No functional change, just a refactoring commit. >>> >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >>> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >>> --- >>> Changes in v4: >>> - Added R-b tag by Guenter Roeck >>> >>> Changes in v3: >>> - Added quirks documentation >>> - Added R-b tag by Krzysztof Kozlowski >>> >>> Changes in v2: >>> - (none): it's a new patch >>> >>> drivers/watchdog/s3c2410_wdt.c | 83 ++++++++++++++++++++++++---------- >>> 1 file changed, 58 insertions(+), 25 deletions(-) >>> >>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >>> index ec341c876225..f211be8bf976 100644 >>> --- a/drivers/watchdog/s3c2410_wdt.c >>> +++ b/drivers/watchdog/s3c2410_wdt.c >>> @@ -56,17 +56,51 @@ >>> #define EXYNOS5_RST_STAT_REG_OFFSET 0x0404 >>> #define EXYNOS5_WDT_DISABLE_REG_OFFSET 0x0408 >>> #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET 0x040c >>> -#define QUIRK_HAS_PMU_CONFIG (1 << 0) >>> -#define QUIRK_HAS_RST_STAT (1 << 1) >>> -#define QUIRK_HAS_WTCLRINT_REG (1 << 2) >>> + >>> +/** >> >> 0-day complains: >> >> drivers/watchdog/s3c2410_wdt.c:94: warning: expecting prototype for Quirk flags for different Samsung watchdog IP(). Prototype was for QUIRK_HAS_WTCLRINT_REG() instead >> >> It doesn't seem to like the idea of documented bit masks. Not really sure >> what to do here. I am inclined to ignore it, but I don't want to get flooded >> by 0-day complaints until I retire either. Any idea ? >> > > Seems like 0-day thinks this kernel-doc comment is for the first > define only, and thus the comment has wrong format, or something like > that. I tried to follow the same style as GFP_KERNEL and others are > documented. > > Anyway, if you don't like 0-day complaints, can you please just > replace kernel-doc comment (/**) with regular comment (/*), by > removing one asterisk in the patch? Or I can re-send the patch > correspondingly -- then just let me know. > Oh, never mind. Let's just hope that 0-day stops complaining at some point. Guenter
On Wed, 24 Nov 2021 at 00:33, Guenter Roeck <linux@roeck-us.net> wrote: > > On 11/23/21 8:17 AM, Sam Protsenko wrote: > > On Tue, 23 Nov 2021 at 18:06, Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On Sun, Nov 21, 2021 at 06:56:44PM +0200, Sam Protsenko wrote: > >>> Now that PMU enablement code was extended for new Exynos SoCs, it > >>> doesn't look very cohesive and consistent anymore. Do a bit of renaming, > >>> grouping and style changes, to make it look good again. While at it, add > >>> quirks documentation as well. > >>> > >>> No functional change, just a refactoring commit. > >>> > >>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > >>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > >>> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > >>> --- > >>> Changes in v4: > >>> - Added R-b tag by Guenter Roeck > >>> > >>> Changes in v3: > >>> - Added quirks documentation > >>> - Added R-b tag by Krzysztof Kozlowski > >>> > >>> Changes in v2: > >>> - (none): it's a new patch > >>> > >>> drivers/watchdog/s3c2410_wdt.c | 83 ++++++++++++++++++++++++---------- > >>> 1 file changed, 58 insertions(+), 25 deletions(-) > >>> > >>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > >>> index ec341c876225..f211be8bf976 100644 > >>> --- a/drivers/watchdog/s3c2410_wdt.c > >>> +++ b/drivers/watchdog/s3c2410_wdt.c > >>> @@ -56,17 +56,51 @@ > >>> #define EXYNOS5_RST_STAT_REG_OFFSET 0x0404 > >>> #define EXYNOS5_WDT_DISABLE_REG_OFFSET 0x0408 > >>> #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET 0x040c > >>> -#define QUIRK_HAS_PMU_CONFIG (1 << 0) > >>> -#define QUIRK_HAS_RST_STAT (1 << 1) > >>> -#define QUIRK_HAS_WTCLRINT_REG (1 << 2) > >>> + > >>> +/** > >> > >> 0-day complains: > >> > >> drivers/watchdog/s3c2410_wdt.c:94: warning: expecting prototype for Quirk flags for different Samsung watchdog IP(). Prototype was for QUIRK_HAS_WTCLRINT_REG() instead > >> > >> It doesn't seem to like the idea of documented bit masks. Not really sure > >> what to do here. I am inclined to ignore it, but I don't want to get flooded > >> by 0-day complaints until I retire either. Any idea ? > >> > > > > Seems like 0-day thinks this kernel-doc comment is for the first > > define only, and thus the comment has wrong format, or something like > > that. I tried to follow the same style as GFP_KERNEL and others are > > documented. > > > > Anyway, if you don't like 0-day complaints, can you please just > > replace kernel-doc comment (/**) with regular comment (/*), by > > removing one asterisk in the patch? Or I can re-send the patch > > correspondingly -- then just let me know. > > > > Oh, never mind. Let's just hope that 0-day stops complaining at some point. > Just sent v5 for this patch, fixing that 0-day warning properly. Found info about it here: [1]. So to check that warning, apparently it's enough to run "make W=n" build, or dry-run for kernel-doc script like this: $ scripts/kernel-doc -v -none drivers/watchdog/s3c2410_wdt.c Anyway, please take v4 series + v5 for this patch. Hope that'll be all for 0-day swearing :) [1] https://github.com/torvalds/linux/blob/master/Documentation/doc-guide/kernel-doc.rst > Guenter
On 11/27/21 2:52 PM, Sam Protsenko wrote: > On Wed, 24 Nov 2021 at 01:30, Sam Protsenko <semen.protsenko@linaro.org> wrote: >> >> On Wed, 24 Nov 2021 at 00:33, Guenter Roeck <linux@roeck-us.net> wrote: >>> >>> On 11/23/21 8:17 AM, Sam Protsenko wrote: >>>> On Tue, 23 Nov 2021 at 18:06, Guenter Roeck <linux@roeck-us.net> wrote: >>>>> >>>>> On Sun, Nov 21, 2021 at 06:56:44PM +0200, Sam Protsenko wrote: >>>>>> Now that PMU enablement code was extended for new Exynos SoCs, it >>>>>> doesn't look very cohesive and consistent anymore. Do a bit of renaming, >>>>>> grouping and style changes, to make it look good again. While at it, add >>>>>> quirks documentation as well. >>>>>> >>>>>> No functional change, just a refactoring commit. >>>>>> >>>>>> Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> >>>>>> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> >>>>>> Reviewed-by: Guenter Roeck <linux@roeck-us.net> >>>>>> --- >>>>>> Changes in v4: >>>>>> - Added R-b tag by Guenter Roeck >>>>>> >>>>>> Changes in v3: >>>>>> - Added quirks documentation >>>>>> - Added R-b tag by Krzysztof Kozlowski >>>>>> >>>>>> Changes in v2: >>>>>> - (none): it's a new patch >>>>>> >>>>>> drivers/watchdog/s3c2410_wdt.c | 83 ++++++++++++++++++++++++---------- >>>>>> 1 file changed, 58 insertions(+), 25 deletions(-) >>>>>> >>>>>> diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c >>>>>> index ec341c876225..f211be8bf976 100644 >>>>>> --- a/drivers/watchdog/s3c2410_wdt.c >>>>>> +++ b/drivers/watchdog/s3c2410_wdt.c >>>>>> @@ -56,17 +56,51 @@ >>>>>> #define EXYNOS5_RST_STAT_REG_OFFSET 0x0404 >>>>>> #define EXYNOS5_WDT_DISABLE_REG_OFFSET 0x0408 >>>>>> #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET 0x040c >>>>>> -#define QUIRK_HAS_PMU_CONFIG (1 << 0) >>>>>> -#define QUIRK_HAS_RST_STAT (1 << 1) >>>>>> -#define QUIRK_HAS_WTCLRINT_REG (1 << 2) >>>>>> + >>>>>> +/** >>>>> >>>>> 0-day complains: >>>>> >>>>> drivers/watchdog/s3c2410_wdt.c:94: warning: expecting prototype for Quirk flags for different Samsung watchdog IP(). Prototype was for QUIRK_HAS_WTCLRINT_REG() instead >>>>> >>>>> It doesn't seem to like the idea of documented bit masks. Not really sure >>>>> what to do here. I am inclined to ignore it, but I don't want to get flooded >>>>> by 0-day complaints until I retire either. Any idea ? >>>>> >>>> >>>> Seems like 0-day thinks this kernel-doc comment is for the first >>>> define only, and thus the comment has wrong format, or something like >>>> that. I tried to follow the same style as GFP_KERNEL and others are >>>> documented. >>>> >>>> Anyway, if you don't like 0-day complaints, can you please just >>>> replace kernel-doc comment (/**) with regular comment (/*), by >>>> removing one asterisk in the patch? Or I can re-send the patch >>>> correspondingly -- then just let me know. >>>> >>> >>> Oh, never mind. Let's just hope that 0-day stops complaining at some point. >>> >> >> Just sent v5 for this patch, fixing that 0-day warning properly. Found >> info about it here: [1]. So to check that warning, apparently it's >> enough to run "make W=n" build, or dry-run for kernel-doc script like >> this: >> >> $ scripts/kernel-doc -v -none drivers/watchdog/s3c2410_wdt.c >> >> Anyway, please take v4 series + v5 for this patch. Hope that'll be all >> for 0-day swearing :) >> >> [1] https://github.com/torvalds/linux/blob/master/Documentation/doc-guide/kernel-doc.rst >> > > Hi Guenter, > > Can you please take this patch: > > [PATCH v4 12/12] watchdog: s3c2410: Add Exynos850 support > > and replace "Cleanup PMU related code" patch you already applied with this one: > > [PATCH v5] watchdog: s3c2410: Cleanup PMU related code > > I can see you already took most of WDT patches I sent, but those two > seem to be missing. > Upstream work is always "time permitting". Done now. > Also, I can't see my patches (which are already present in your > "watchdog-next" branch) in linux-next/master. Is that expected, or I'm > missing something? > My watchdog-next branch is for 0-day coverage only. It is not made available in linux-next. linux-next pulls watchdog related changes from the official watchdog repository at git://www.linux-watchdog.org/linux-watchdog-next.git#master Guenter
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c index ec341c876225..f211be8bf976 100644 --- a/drivers/watchdog/s3c2410_wdt.c +++ b/drivers/watchdog/s3c2410_wdt.c @@ -56,17 +56,51 @@ #define EXYNOS5_RST_STAT_REG_OFFSET 0x0404 #define EXYNOS5_WDT_DISABLE_REG_OFFSET 0x0408 #define EXYNOS5_WDT_MASK_RESET_REG_OFFSET 0x040c -#define QUIRK_HAS_PMU_CONFIG (1 << 0) -#define QUIRK_HAS_RST_STAT (1 << 1) -#define QUIRK_HAS_WTCLRINT_REG (1 << 2) + +/** + * Quirk flags for different Samsung watchdog IP-cores. + * + * This driver supports multiple Samsung SoCs, each of which might have + * different set of registers and features supported. As watchdog block + * sometimes requires modifying PMU registers for proper functioning, register + * differences in both watchdog and PMU IP-cores should be accounted for. Quirk + * flags described below serve the purpose of telling the driver about mentioned + * SoC traits, and can be specified in driver data for each particular supported + * device. + * + * %QUIRK_HAS_WTCLRINT_REG: Watchdog block has WTCLRINT register. It's used to + * clear the interrupt once the interrupt service routine is complete. It's + * write-only, writing any values to this register clears the interrupt, but + * reading is not permitted. + * + * %QUIRK_HAS_PMU_MASK_RESET: PMU block has the register for disabling/enabling + * WDT reset request. On old SoCs it's usually called MASK_WDT_RESET_REQUEST, + * new SoCs have CLUSTERx_NONCPU_INT_EN register, which 'mask_bit' value is + * inverted compared to the former one. + * + * %QUIRK_HAS_PMU_RST_STAT: PMU block has RST_STAT (reset status) register, + * which contains bits indicating the reason for most recent CPU reset. If + * present, driver will use this register to check if previous reboot was due to + * watchdog timer reset. + * + * %QUIRK_HAS_PMU_AUTO_DISABLE: PMU block has AUTOMATIC_WDT_RESET_DISABLE + * register. If 'mask_bit' bit is set, PMU will disable WDT reset when + * corresponding processor is in reset state. + * + * %QUIRK_HAS_PMU_CNT_EN: PMU block has some register (e.g. CLUSTERx_NONCPU_OUT) + * with "watchdog counter enable" bit. That bit should be set to make watchdog + * counter running. + */ +#define QUIRK_HAS_WTCLRINT_REG (1 << 0) +#define QUIRK_HAS_PMU_MASK_RESET (1 << 1) +#define QUIRK_HAS_PMU_RST_STAT (1 << 2) #define QUIRK_HAS_PMU_AUTO_DISABLE (1 << 3) #define QUIRK_HAS_PMU_CNT_EN (1 << 4) /* These quirks require that we have a PMU register map */ -#define QUIRKS_HAVE_PMUREG (QUIRK_HAS_PMU_CONFIG | \ - QUIRK_HAS_RST_STAT | \ - QUIRK_HAS_PMU_AUTO_DISABLE | \ - QUIRK_HAS_PMU_CNT_EN) +#define QUIRKS_HAVE_PMUREG \ + (QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_RST_STAT | \ + QUIRK_HAS_PMU_AUTO_DISABLE | QUIRK_HAS_PMU_CNT_EN) static bool nowayout = WATCHDOG_NOWAYOUT; static int tmr_margin; @@ -146,8 +180,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250 = { .mask_bit = 20, .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET, .rst_stat_bit = 20, - .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \ - | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE, + .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \ + QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE, }; static const struct s3c2410_wdt_variant drv_data_exynos5420 = { @@ -156,8 +190,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = { .mask_bit = 0, .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET, .rst_stat_bit = 9, - .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \ - | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE, + .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \ + QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE, }; static const struct s3c2410_wdt_variant drv_data_exynos7 = { @@ -166,8 +200,8 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = { .mask_bit = 23, .rst_stat_reg = EXYNOS5_RST_STAT_REG_OFFSET, .rst_stat_bit = 23, /* A57 WDTRESET */ - .quirks = QUIRK_HAS_PMU_CONFIG | QUIRK_HAS_RST_STAT \ - | QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE, + .quirks = QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_PMU_MASK_RESET | \ + QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_AUTO_DISABLE, }; static const struct of_device_id s3c2410_wdt_match[] = { @@ -253,24 +287,24 @@ static int s3c2410wdt_enable_counter(struct s3c2410_wdt *wdt, bool en) return ret; } -static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask) +static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en) { int ret; if (wdt->drv_data->quirks & QUIRK_HAS_PMU_AUTO_DISABLE) { - ret = s3c2410wdt_disable_wdt_reset(wdt, mask); + ret = s3c2410wdt_disable_wdt_reset(wdt, !en); if (ret < 0) return ret; } - if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CONFIG) { - ret = s3c2410wdt_mask_wdt_reset(wdt, mask); + if (wdt->drv_data->quirks & QUIRK_HAS_PMU_MASK_RESET) { + ret = s3c2410wdt_mask_wdt_reset(wdt, !en); if (ret < 0) return ret; } if (wdt->drv_data->quirks & QUIRK_HAS_PMU_CNT_EN) { - ret = s3c2410wdt_enable_counter(wdt, !mask); + ret = s3c2410wdt_enable_counter(wdt, en); if (ret < 0) return ret; } @@ -531,7 +565,7 @@ static inline unsigned int s3c2410wdt_get_bootstatus(struct s3c2410_wdt *wdt) unsigned int rst_stat; int ret; - if (!(wdt->drv_data->quirks & QUIRK_HAS_RST_STAT)) + if (!(wdt->drv_data->quirks & QUIRK_HAS_PMU_RST_STAT)) return 0; ret = regmap_read(wdt->pmureg, wdt->drv_data->rst_stat_reg, &rst_stat); @@ -672,7 +706,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev) if (ret) goto err_cpufreq; - ret = s3c2410wdt_mask_and_disable_reset(wdt, false); + ret = s3c2410wdt_enable(wdt, true); if (ret < 0) goto err_unregister; @@ -707,7 +741,7 @@ static int s3c2410wdt_remove(struct platform_device *dev) int ret; struct s3c2410_wdt *wdt = platform_get_drvdata(dev); - ret = s3c2410wdt_mask_and_disable_reset(wdt, true); + ret = s3c2410wdt_enable(wdt, false); if (ret < 0) return ret; @@ -724,8 +758,7 @@ static void s3c2410wdt_shutdown(struct platform_device *dev) { struct s3c2410_wdt *wdt = platform_get_drvdata(dev); - s3c2410wdt_mask_and_disable_reset(wdt, true); - + s3c2410wdt_enable(wdt, false); s3c2410wdt_stop(&wdt->wdt_device); } @@ -740,7 +773,7 @@ static int s3c2410wdt_suspend(struct device *dev) wdt->wtcon_save = readl(wdt->reg_base + S3C2410_WTCON); wdt->wtdat_save = readl(wdt->reg_base + S3C2410_WTDAT); - ret = s3c2410wdt_mask_and_disable_reset(wdt, true); + ret = s3c2410wdt_enable(wdt, false); if (ret < 0) return ret; @@ -760,7 +793,7 @@ static int s3c2410wdt_resume(struct device *dev) writel(wdt->wtdat_save, wdt->reg_base + S3C2410_WTCNT);/* Reset count */ writel(wdt->wtcon_save, wdt->reg_base + S3C2410_WTCON); - ret = s3c2410wdt_mask_and_disable_reset(wdt, false); + ret = s3c2410wdt_enable(wdt, true); if (ret < 0) return ret;