Message ID | 20211107202943.8859-1-semen.protsenko@linaro.org |
---|---|
Headers | show |
Series | watchdog: s3c2410: Add Exynos850 support | expand |
On Sun, Nov 07, 2021 at 10:29:32PM +0200, Sam Protsenko wrote: > Exynos7 watchdog driver is clearly indicating that its dts node must > define syscon phandle property. That was probably forgotten, so add it. > > Signed-off-by: Sam Protsenko <semen.protsenko@linaro.org> > Fixes: 2b9366b66967 ("watchdog: s3c2410_wdt: Add support for Watchdog device on Exynos7") > Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> > Reviewed-by: Rob Herring <robh@kernel.org> Reviewed-by: Guenter Roeck <linux@roeck-us.net> > --- > Changes in v3: > - Added R-b tag by Rob Herring > > Changes in v2: > - Added R-b tag by Krzysztof Kozlowski > - Added "Fixes" tag > > Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > index 76cb9586ee00..93cd77a6e92c 100644 > --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.yaml > @@ -39,8 +39,8 @@ properties: > samsung,syscon-phandle: > $ref: /schemas/types.yaml#/definitions/phandle > description: > - Phandle to the PMU system controller node (in case of Exynos5250 > - and Exynos5420). > + Phandle to the PMU system controller node (in case of Exynos5250, > + Exynos5420 and Exynos7). > > required: > - compatible > @@ -58,6 +58,7 @@ allOf: > enum: > - samsung,exynos5250-wdt > - samsung,exynos5420-wdt > + - samsung,exynos7-wdt > then: > required: > - samsung,syscon-phandle > -- > 2.30.2 >
On Sun, Nov 07, 2021 at 10:29:36PM +0200, Sam Protsenko wrote: > On new Exynos chips (e.g. Exynos850 and Exynos9) the > AUTOMATIC_WDT_RESET_DISABLE register was removed, and its value can be > thought of as "always 0x0". Add correspondig quirk bit, so that the > driver can omit accessing it if it's not present. > > This commit doesn't bring any functional change to existing devices, but > merely provides an infrastructure for upcoming chips support. > > 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 v3: > - Aligned arguments with opening parentheses > - Added R-b tag by Krzysztof Kozlowski > > Changes in v2: > - Used quirks instead of callbacks for all added PMU registers > - Used BIT() macro > - Extracted splitting the s3c2410wdt_mask_and_disable_reset() function > to separate patch > - Extracted cleanup code to separate patch to minimize changes and > ease the review and porting > > drivers/watchdog/s3c2410_wdt.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 0845c05034a1..2cc4923a98a5 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -59,10 +59,12 @@ > #define QUIRK_HAS_PMU_CONFIG (1 << 0) > #define QUIRK_HAS_RST_STAT (1 << 1) > #define QUIRK_HAS_WTCLRINT_REG (1 << 2) > +#define QUIRK_HAS_PMU_AUTO_DISABLE (1 << 3) > > /* These quirks require that we have a PMU register map */ > #define QUIRKS_HAVE_PMUREG (QUIRK_HAS_PMU_CONFIG | \ > - QUIRK_HAS_RST_STAT) > + QUIRK_HAS_RST_STAT | \ > + QUIRK_HAS_PMU_AUTO_DISABLE) > > static bool nowayout = WATCHDOG_NOWAYOUT; > static int tmr_margin; > @@ -137,7 +139,7 @@ static const struct s3c2410_wdt_variant drv_data_exynos5250 = { > .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_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE, > }; > > static const struct s3c2410_wdt_variant drv_data_exynos5420 = { > @@ -147,7 +149,7 @@ static const struct s3c2410_wdt_variant drv_data_exynos5420 = { > .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_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE, > }; > > static const struct s3c2410_wdt_variant drv_data_exynos7 = { > @@ -157,7 +159,7 @@ static const struct s3c2410_wdt_variant drv_data_exynos7 = { > .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_WTCLRINT_REG | QUIRK_HAS_PMU_AUTO_DISABLE, > }; > > static const struct of_device_id s3c2410_wdt_match[] = { > @@ -213,11 +215,13 @@ static int s3c2410wdt_mask_and_disable_reset(struct s3c2410_wdt *wdt, bool mask) > if (mask) > val = mask_val; > > - ret = regmap_update_bits(wdt->pmureg, > - wdt->drv_data->disable_reg, > - mask_val, val); > - if (ret < 0) > - goto error; > + if (wdt->drv_data->quirks & QUIRK_HAS_PMU_AUTO_DISABLE) { > + ret = regmap_update_bits(wdt->pmureg, > + wdt->drv_data->disable_reg, mask_val, > + val); > + if (ret < 0) > + goto error; > + } > > ret = regmap_update_bits(wdt->pmureg, > wdt->drv_data->mask_reset_reg, > -- > 2.30.2 >
On Sun, Nov 07, 2021 at 10:29:38PM +0200, Sam Protsenko wrote: > On new Exynos chips (like Exynos850) the MASK_WDT_RESET_REQUEST register > is replaced with CLUSTERx_NONCPU_INT_EN, and its mask bit value meaning > was reversed: for new register the bit value "1" means "Interrupt > enabled", while for MASK_WDT_RESET_REQUEST register "1" means "Mask the > interrupt" (i.e. "Interrupt disabled"). > > Introduce "mask_reset_inv" boolean field in driver data structure; when > that field is "true", mask register handling function will invert the > value before setting it to the register. > > This commit doesn't bring any functional change to existing devices, but > merely provides an infrastructure for upcoming chips support. > > 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 v3: > - Added R-b tag by Krzysztof Kozlowski > > Changes in v2: > - (none): it's a new patch > > drivers/watchdog/s3c2410_wdt.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index 4ac0a30e835e..2a61b6ea5602 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -92,6 +92,7 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to > * timer reset functionality. > * @mask_reset_reg: Offset in pmureg for the register that masks the watchdog > * timer reset functionality. > + * @mask_reset_inv: If set, mask_reset_reg value will have inverted meaning. > * @mask_bit: Bit number for the watchdog timer in the disable register and the > * mask reset register. > * @rst_stat_reg: Offset in pmureg for the register that has the reset status. > @@ -103,6 +104,7 @@ MODULE_PARM_DESC(soft_noboot, "Watchdog action, set to 1 to ignore reboots, 0 to > struct s3c2410_wdt_variant { > int disable_reg; > int mask_reset_reg; > + bool mask_reset_inv; > int mask_bit; > int rst_stat_reg; > int rst_stat_bit; > @@ -219,7 +221,8 @@ static int s3c2410wdt_disable_wdt_reset(struct s3c2410_wdt *wdt, bool mask) > static int s3c2410wdt_mask_wdt_reset(struct s3c2410_wdt *wdt, bool mask) > { > const u32 mask_val = BIT(wdt->drv_data->mask_bit); > - const u32 val = mask ? mask_val : 0; > + const bool val_inv = wdt->drv_data->mask_reset_inv; > + const u32 val = (mask ^ val_inv) ? mask_val : 0; > int ret; > > ret = regmap_update_bits(wdt->pmureg, wdt->drv_data->mask_reset_reg, > -- > 2.30.2 >
On Sun, Nov 07, 2021 at 10:29:40PM +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 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) > + > +/** > + * 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 Sun, Nov 07, 2021 at 10:29:42PM +0200, Sam Protsenko wrote: > 'err' label in probe function is not really need, it just returns. > Remove it and replace all 'goto' statements with actual returns in > place. > > No functional change here, just a cleanup patch. > > 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 v3: > - Added R-b tag by Krzysztof Kozlowski > > Changes in v2: > - (none): it's a new patch > > drivers/watchdog/s3c2410_wdt.c | 13 ++++--------- > 1 file changed, 4 insertions(+), 9 deletions(-) > > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c > index f31bc765a8a5..96aa5d9c6ed4 100644 > --- a/drivers/watchdog/s3c2410_wdt.c > +++ b/drivers/watchdog/s3c2410_wdt.c > @@ -627,22 +627,18 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > wdt_irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0); > if (wdt_irq == NULL) { > dev_err(dev, "no irq resource specified\n"); > - ret = -ENOENT; > - goto err; > + return -ENOENT; > } > > /* get the memory region for the watchdog timer */ > wdt->reg_base = devm_platform_ioremap_resource(pdev, 0); > - if (IS_ERR(wdt->reg_base)) { > - ret = PTR_ERR(wdt->reg_base); > - goto err; > - } > + if (IS_ERR(wdt->reg_base)) > + return PTR_ERR(wdt->reg_base); > > wdt->bus_clk = devm_clk_get(dev, "watchdog"); > if (IS_ERR(wdt->bus_clk)) { > dev_err(dev, "failed to find bus clock\n"); > - ret = PTR_ERR(wdt->bus_clk); > - goto err; > + return PTR_ERR(wdt->bus_clk); > } > > ret = clk_prepare_enable(wdt->bus_clk); > @@ -757,7 +753,6 @@ static int s3c2410wdt_probe(struct platform_device *pdev) > err_bus_clk: > clk_disable_unprepare(wdt->bus_clk); > > - err: > return ret; > } > > -- > 2.30.2 >