Re: [PATCH 17/21] watchdog: s3c2410_wdt: Add support for Google tensor SoCs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux