Hi Guenter,
On Thu, 5 Oct 2023 at 19:58, Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
>
> On Thu, Oct 05, 2023 at 04:56:14PM +0100, Peter Griffin wrote:
> > This patch adds the compatibles and drvdata for the Google
> > gs101 & gs201 SoCs found in Pixel 6 and Pixel 7 phones. Similar
> > to Exynos850 it has two watchdog instances, one for each cluster
> > and has some control bits in PMU registers.
> >
> > The watchdog IP found in gs101 SoCs also supports a few
> > additional bits/features in the WTCON register which we add
> > support for and an additional register detailed below.
> >
> > dbgack-mask - Enables masking WDT interrupt and reset request
> > according to asserted DBGACK input
> >
> > windowed-mode - Enabled Windowed watchdog mode
> >
> > Windowed watchdog mode also has an additional register WTMINCNT.
> > If windowed watchdog is enabled and you reload WTCNT when the
> > value is greater than WTMINCNT, it prompts interrupt or reset
> > request as if the watchdog time has expired.
> >
> > Signed-off-by: Peter Griffin <peter.griffin@xxxxxxxxxx>
> > ---
> > drivers/watchdog/s3c2410_wdt.c | 116 +++++++++++++++++++++++++++++----
> > 1 file changed, 105 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
> > index 0b4bd883ff28..4c23c7e6a3f1 100644
> > --- a/drivers/watchdog/s3c2410_wdt.c
> > +++ b/drivers/watchdog/s3c2410_wdt.c
> > @@ -31,12 +31,14 @@
> > #define S3C2410_WTDAT 0x04
> > #define S3C2410_WTCNT 0x08
> > #define S3C2410_WTCLRINT 0x0c
> > -
> > +#define S3C2410_WTMINCNT 0x10
> > #define S3C2410_WTCNT_MAXCNT 0xffff
> >
> > -#define S3C2410_WTCON_RSTEN (1 << 0)
> > -#define S3C2410_WTCON_INTEN (1 << 2)
> > -#define S3C2410_WTCON_ENABLE (1 << 5)
> > +#define S3C2410_WTCON_RSTEN (1 << 0)
> > +#define S3C2410_WTCON_INTEN (1 << 2)
> > +#define S3C2410_WTCON_ENABLE (1 << 5)
> > +#define S3C2410_WTCON_DBGACK_MASK (1 << 16)
> > +#define S3C2410_WTCON_WINDOWED_WD (1 << 20)
> >
> > #define S3C2410_WTCON_DIV16 (0 << 3)
> > #define S3C2410_WTCON_DIV32 (1 << 3)
> > @@ -61,12 +63,16 @@
> > #define EXYNOS850_CLUSTER1_NONCPU_INT_EN 0x1644
> > #define EXYNOSAUTOV9_CLUSTER1_NONCPU_OUT 0x1520
> > #define EXYNOSAUTOV9_CLUSTER1_NONCPU_INT_EN 0x1544
> > -
> > #define EXYNOS850_CLUSTER0_WDTRESET_BIT 24
> > #define EXYNOS850_CLUSTER1_WDTRESET_BIT 23
> > #define EXYNOSAUTOV9_CLUSTER0_WDTRESET_BIT 25
> > #define EXYNOSAUTOV9_CLUSTER1_WDTRESET_BIT 24
> > -
> > +#define GS_CLUSTER0_NONCPU_OUT 0x1220
> > +#define GS_CLUSTER1_NONCPU_OUT 0x1420
> > +#define GS_CLUSTER0_NONCPU_INT_EN 0x1244
> > +#define GS_CLUSTER1_NONCPU_INT_EN 0x1444
> > +#define GS_CLUSTER2_NONCPU_INT_EN 0x1644
> > +#define GS_RST_STAT_REG_OFFSET 0x3B44
> > /**
> > * DOC: Quirk flags for different Samsung watchdog IP-cores
> > *
> > @@ -106,6 +112,8 @@
> > #define QUIRK_HAS_PMU_RST_STAT (1 << 2)
> > #define QUIRK_HAS_PMU_AUTO_DISABLE (1 << 3)
> > #define QUIRK_HAS_PMU_CNT_EN (1 << 4)
> > +#define QUIRK_HAS_DBGACK_BIT (1 << 5)
> > +#define QUIRK_HAS_WTMINCNT_REG (1 << 6)
> >
> > /* These quirks require that we have a PMU register map */
> > #define QUIRKS_HAVE_PMUREG \
> > @@ -263,6 +271,54 @@ static const struct s3c2410_wdt_variant drv_data_exynosautov9_cl1 = {
> > QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_CNT_EN,
> > };
> >
> > +static const struct s3c2410_wdt_variant drv_data_gs101_cl0 = {
> > + .mask_reset_reg = GS_CLUSTER0_NONCPU_INT_EN,
> > + .mask_bit = 2,
> > + .mask_reset_inv = true,
> > + .rst_stat_reg = GS_RST_STAT_REG_OFFSET,
> > + .rst_stat_bit = 0,
> > + .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT,
> > + .cnt_en_bit = 8,
> > + .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
> > + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> > +};
> > +
> > +static const struct s3c2410_wdt_variant drv_data_gs101_cl1 = {
> > + .mask_reset_reg = GS_CLUSTER1_NONCPU_INT_EN,
> > + .mask_bit = 2,
> > + .mask_reset_inv = true,
> > + .rst_stat_reg = GS_RST_STAT_REG_OFFSET,
> > + .rst_stat_bit = 1,
> > + .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT,
> > + .cnt_en_bit = 7,
> > + .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
> > + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> > +};
> > +
> > +static const struct s3c2410_wdt_variant drv_data_gs201_cl0 = {
> > + .mask_reset_reg = GS_CLUSTER0_NONCPU_INT_EN,
> > + .mask_bit = 2,
> > + .mask_reset_inv = true,
> > + .rst_stat_reg = GS_RST_STAT_REG_OFFSET,
> > + .rst_stat_bit = 0,
> > + .cnt_en_reg = GS_CLUSTER0_NONCPU_OUT,
> > + .cnt_en_bit = 8,
> > + .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
> > + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> > +};
> > +
> > +static const struct s3c2410_wdt_variant drv_data_gs201_cl1 = {
> > + .mask_reset_reg = GS_CLUSTER1_NONCPU_INT_EN,
> > + .mask_bit = 2,
> > + .mask_reset_inv = true,
> > + .rst_stat_reg = GS_RST_STAT_REG_OFFSET,
> > + .rst_stat_bit = 1,
> > + .cnt_en_reg = GS_CLUSTER1_NONCPU_OUT,
> > + .cnt_en_bit = 7,
> > + .quirks = QUIRK_HAS_PMU_RST_STAT | QUIRK_HAS_PMU_MASK_RESET | QUIRK_HAS_PMU_CNT_EN |
> > + QUIRK_HAS_WTCLRINT_REG | QUIRK_HAS_DBGACK_BIT | QUIRK_HAS_WTMINCNT_REG,
> > +};
> > +
> > static const struct of_device_id s3c2410_wdt_match[] = {
> > { .compatible = "samsung,s3c2410-wdt",
> > .data = &drv_data_s3c2410 },
> > @@ -278,6 +334,10 @@ static const struct of_device_id s3c2410_wdt_match[] = {
> > .data = &drv_data_exynos850_cl0 },
> > { .compatible = "samsung,exynosautov9-wdt",
> > .data = &drv_data_exynosautov9_cl0 },
> > + { .compatible = "google,gs101-wdt",
> > + .data = &drv_data_gs101_cl0 },
> > + { .compatible = "google,gs201-wdt",
> > + .data = &drv_data_gs201_cl0 },
> > {},
> > };
> > MODULE_DEVICE_TABLE(of, s3c2410_wdt_match);
> > @@ -375,6 +435,21 @@ static int s3c2410wdt_enable(struct s3c2410_wdt *wdt, bool en)
> > return 0;
> > }
> >
> > +static void s3c2410wdt_mask_dbgack(struct s3c2410_wdt *wdt, bool mask)
> > +{
> > + unsigned long wtcon;
> > +
> > + if (!(wdt->drv_data->quirks & QUIRK_HAS_DBGACK_BIT))
> > + return;
> > +
> > + wtcon = readl(wdt->reg_base + S3C2410_WTCON);
> > + if (mask)
> > + wtcon |= S3C2410_WTCON_DBGACK_MASK;
> > + else
> > + wtcon &= ~S3C2410_WTCON_DBGACK_MASK;
> > + writel(wtcon, wdt->reg_base + S3C2410_WTCON);
> > +}
> > +
> > static int s3c2410wdt_keepalive(struct watchdog_device *wdd)
> > {
> > struct s3c2410_wdt *wdt = watchdog_get_drvdata(wdd);
> > @@ -585,9 +660,11 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev, struct s3c2410_wdt *wdt)
> > }
> >
> > #ifdef CONFIG_OF
> > - /* Choose Exynos850/ExynosAutov9 driver data w.r.t. cluster index */
> > + /* Choose Exynos850/ExynosAutov9/gsx01 driver data w.r.t. cluster index */
> > if (variant == &drv_data_exynos850_cl0 ||
> > - variant == &drv_data_exynosautov9_cl0) {
> > + variant == &drv_data_exynosautov9_cl0 ||
> > + variant == &drv_data_gs101_cl0 ||
> > + variant == &drv_data_gs201_cl0) {
> > u32 index;
> > int err;
> >
> > @@ -600,9 +677,14 @@ s3c2410_get_wdt_drv_data(struct platform_device *pdev, struct s3c2410_wdt *wdt)
> > case 0:
> > break;
> > case 1:
> > - variant = (variant == &drv_data_exynos850_cl0) ?
> > - &drv_data_exynos850_cl1 :
> > - &drv_data_exynosautov9_cl1;
> > + if (variant == &drv_data_exynos850_cl0)
> > + variant = &drv_data_exynos850_cl1;
> > + else if (variant == &drv_data_exynosautov9_cl0)
> > + variant = &drv_data_exynosautov9_cl1;
> > + else if (variant == &drv_data_gs101_cl0)
> > + variant = &drv_data_gs101_cl1;
> > + else if (variant == &drv_data_gs201_cl0)
> > + variant = &drv_data_gs201_cl1;
> > break;
> > default:
> > return dev_err_probe(dev, -EINVAL, "wrong cluster index: %u\n", index);
> > @@ -700,6 +782,8 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > wdt->wdt_device.bootstatus = s3c2410wdt_get_bootstatus(wdt);
> > wdt->wdt_device.parent = dev;
> >
> > + s3c2410wdt_mask_dbgack(wdt, true);
> > +
> > /*
> > * If "tmr_atboot" param is non-zero, start the watchdog right now. Also
> > * set WDOG_HW_RUNNING bit, so that watchdog core can kick the watchdog.
> > @@ -712,6 +796,7 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > s3c2410wdt_start(&wdt->wdt_device);
> > set_bit(WDOG_HW_RUNNING, &wdt->wdt_device.status);
> > } else {
> > + dev_info(dev, "stopping watchdog timer\n");
>
> I am not inclined to accept patches adding such noise.
>
> > s3c2410wdt_stop(&wdt->wdt_device);
> > }
> >
> > @@ -738,6 +823,15 @@ static int s3c2410wdt_probe(struct platform_device *pdev)
> > (wtcon & S3C2410_WTCON_RSTEN) ? "en" : "dis",
> > (wtcon & S3C2410_WTCON_INTEN) ? "en" : "dis");
> >
> > + if (wdt->drv_data->quirks & QUIRK_HAS_DBGACK_BIT)
> > + dev_info(dev, "DBGACK %sabled\n",
> > + (wtcon & S3C2410_WTCON_DBGACK_MASK) ? "en" : "dis");
> > +
> > + if (wdt->drv_data->quirks & QUIRK_HAS_WTMINCNT_REG)
> > + dev_info(dev, "windowed watchdog %sabled, wtmincnt=%x\n",
> > + (wtcon & S3C2410_WTCON_WINDOWED_WD) ? "en" : "dis",
> > + readl(wdt->reg_base + S3C2410_WTMINCNT));
>
> ... and I really don't see its value.
Thanks for your review feedback. I will remove these dev_info prints in v2.
regards,
Peter.
