| Message ID | 20211121165647.26706-10-semen.protsenko@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: s3c2410: Add Exynos850 support | expand |
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 ? Guenter > + * 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; > > -- > 2.30.2 >
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. > Guenter > > > + * 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; > > > > -- > > 2.30.2 > >
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. 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? Thanks! > > Guenter
On Sun, 28 Nov 2021 at 19:56, Guenter Roeck <linux@roeck-us.net> wrote: > > 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. > Thank you, Guenter! > > 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;