Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

On Mar 28, 2016, at 4:56 PM, Guenter Roeck  wrote:
> 
>> So taken from the timer offset 0x0208A000 I just have a generic counter 
>> register CPU0_APCS_GPT0_CNT at 0x8
>> 
>> What doc are you looking at?
>> 
> "Qualcomm Snapdragon 600 Processor APQ8064 Hardware Register Description"
> 
> It is available for download from the Qualcomm web site.
> 
> See chapter 12.10.3, "Watchdog timer registers". The register block is at
> 0x28882000. Registers are almost the same, except for the offset and the
> definition of the bits in the enable register.
> 
> LPASS is "Low Power Audio Subsystem". Maybe it has its own watchdog.

This block is here:

11.15 KPSS CPU0 Timer Registers (0x0208A000 CPU0_APCS_TMR_BASE)

-M

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

On Mar 28, 2016, at 4:56 PM, Guenter Roeck  wrote:
> 
>> So taken from the timer offset 0x0208A000 I just have a generic counter 
>> register CPU0_APCS_GPT0_CNT at 0x8
>> 
>> What doc are you looking at?
>> 
> "Qualcomm Snapdragon 600 Processor APQ8064 Hardware Register Description"
> 
> It is available for download from the Qualcomm web site.
> 
> See chapter 12.10.3, "Watchdog timer registers". The register block is at
> 0x28882000. Registers are almost the same, except for the offset and the
> definition of the bits in the enable register.
> 
> LPASS is "Low Power Audio Subsystem". Maybe it has its own watchdog.

This block is here:

11.15 KPSS CPU0 Timer Registers (0x0208A000 CPU0_APCS_TMR_BASE)

-M

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Guenter Roeck]

On Mon, Mar 28, 2016 at 03:40:58PM -0500, Matthew McClintock wrote:
> On Mar 28, 2016, at 1:13 PM, Guenter Roeck  wrote:
> > 
> >>> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
> >>> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
> >>> undefined.
> >> 
> >> I honestly don’t see anything at 0x8 for either blocks that looks like 
> >> this. For the new block bit 0 is enabling and bit 1 enabled interrupts.
> >> 
> > That is from the APQ8064 datasheet. 
> 
> So taken from the timer offset 0x0208A000 I just have a generic counter 
> register CPU0_APCS_GPT0_CNT at 0x8
> 
> What doc are you looking at?
> 
"Qualcomm Snapdragon 600 Processor APQ8064 Hardware Register Description"

It is available for download from the Qualcomm web site.

See chapter 12.10.3, "Watchdog timer registers". The register block is at
0x28882000. Registers are almost the same, except for the offset and the
definition of the bits in the enable register.

LPASS is "Low Power Audio Subsystem". Maybe it has its own watchdog.

Guenter

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Guenter Roeck]

On Mon, Mar 28, 2016 at 03:40:58PM -0500, Matthew McClintock wrote:
> On Mar 28, 2016, at 1:13 PM, Guenter Roeck  wrote:
> > 
> >>> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
> >>> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
> >>> undefined.
> >> 
> >> I honestly don’t see anything at 0x8 for either blocks that looks like 
> >> this. For the new block bit 0 is enabling and bit 1 enabled interrupts.
> >> 
> > That is from the APQ8064 datasheet. 
> 
> So taken from the timer offset 0x0208A000 I just have a generic counter 
> register CPU0_APCS_GPT0_CNT at 0x8
> 
> What doc are you looking at?
> 
"Qualcomm Snapdragon 600 Processor APQ8064 Hardware Register Description"

It is available for download from the Qualcomm web site.

See chapter 12.10.3, "Watchdog timer registers". The register block is at
0x28882000. Registers are almost the same, except for the offset and the
definition of the bits in the enable register.

LPASS is "Low Power Audio Subsystem". Maybe it has its own watchdog.

Guenter

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

On Mar 28, 2016, at 1:13 PM, Guenter Roeck  wrote:
> 
>>> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
>>> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
>>> undefined.
>> 
>> I honestly don’t see anything at 0x8 for either blocks that looks like this. 
>> For the new block bit 0 is enabling and bit 1 enabled interrupts.
>> 
> That is from the APQ8064 datasheet. 

So taken from the timer offset 0x0208A000 I just have a generic counter 
register CPU0_APCS_GPT0_CNT at 0x8

What doc are you looking at?

-M

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

On Mar 28, 2016, at 1:13 PM, Guenter Roeck  wrote:
> 
>>> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
>>> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
>>> undefined.
>> 
>> I honestly don’t see anything at 0x8 for either blocks that looks like this. 
>> For the new block bit 0 is enabling and bit 1 enabled interrupts.
>> 
> That is from the APQ8064 datasheet. 

So taken from the timer offset 0x0208A000 I just have a generic counter 
register CPU0_APCS_GPT0_CNT at 0x8

What doc are you looking at?

-M

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Guenter Roeck]

On Mon, Mar 28, 2016 at 11:55:28AM -0500, Matthew McClintock wrote:
> 
> > On Mar 25, 2016, at 11:23 AM, Guenter Roeck  wrote:
> > 
> >> -#define WDT_RST   0x38
> >> -#define WDT_EN0x40
> >> -#define WDT_BITE_TIME 0x5C
> >> +enum wdt_reg {
> >> +  WDT_RST,
> >> +  WDT_EN,
> >> +  WDT_BITE_TIME,
> >> +};
> >> +
> >> +static const u32 reg_offset_data_apcs_tmr[] = {
> >> +  [WDT_RST] = 0x38,
> >> +  [WDT_EN] = 0x40,
> >> +  [WDT_BITE_TIME] = 0x5C,
> >> +};
> >> +
> >> +static const u32 reg_offset_data_kpss[] = {
> >> +  [WDT_RST] = 0x4,
> >> +  [WDT_EN] = 0x8,
> > 
> > Does this work ? In the datasheet I have in front of me (APQ8064), the 
> > watchdog
> > at this address uses different bits. At address 0x40 (eg 
> > GSS_A5_APCS_WDT0_EN),
> 
> 0x40 is acps_tmr, and looks fine.
> 
> > bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
> > LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
> > undefined.
> 
> I honestly don’t see anything at 0x8 for either blocks that looks like this. 
> For the new block bit 0 is enabling and bit 1 enabled interrupts.
> 
That is from the APQ8064 datasheet. 

> > Or does "qcom,kpss-standalone" refer to some other watchdog ?
> 
> APQ8064 would be the apcs_tmr block variant which is unchanged. MSM8916 as 
> well as IPQ4019 would use the new kpss variant.
> 
Unfortunately I don't have access to those datasheets.

> I went with block names I found internally here, but I will be the first to 
> admit I am terrible at names. The old block name for APQ was CPU0_ACPS_TMR 
> (where really the watchdog is a subset of a timer block), and on the IPQ4019 
> it’s called APCS_KPSS_WDT and it’s really just a watchdog block.
> 
> I kept the same driver because the register’s currently in use were 
> compatible. By the way, I tested this on an IPQ806x and IPQ4019 both new and 
> old blocks.
> 

The property name should probably be something like 'qcom,kpss-wdt'
(or 'qcom,kpss-watchdog' ?), possibly in addition to 'qcom,kpss-ipq4019-wdt'
and 'qcom,kpss-msm8916-wdt'.

Guenter

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Guenter Roeck]

On Mon, Mar 28, 2016 at 11:55:28AM -0500, Matthew McClintock wrote:
> 
> > On Mar 25, 2016, at 11:23 AM, Guenter Roeck  wrote:
> > 
> >> -#define WDT_RST   0x38
> >> -#define WDT_EN0x40
> >> -#define WDT_BITE_TIME 0x5C
> >> +enum wdt_reg {
> >> +  WDT_RST,
> >> +  WDT_EN,
> >> +  WDT_BITE_TIME,
> >> +};
> >> +
> >> +static const u32 reg_offset_data_apcs_tmr[] = {
> >> +  [WDT_RST] = 0x38,
> >> +  [WDT_EN] = 0x40,
> >> +  [WDT_BITE_TIME] = 0x5C,
> >> +};
> >> +
> >> +static const u32 reg_offset_data_kpss[] = {
> >> +  [WDT_RST] = 0x4,
> >> +  [WDT_EN] = 0x8,
> > 
> > Does this work ? In the datasheet I have in front of me (APQ8064), the 
> > watchdog
> > at this address uses different bits. At address 0x40 (eg 
> > GSS_A5_APCS_WDT0_EN),
> 
> 0x40 is acps_tmr, and looks fine.
> 
> > bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
> > LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
> > undefined.
> 
> I honestly don’t see anything at 0x8 for either blocks that looks like this. 
> For the new block bit 0 is enabling and bit 1 enabled interrupts.
> 
That is from the APQ8064 datasheet. 

> > Or does "qcom,kpss-standalone" refer to some other watchdog ?
> 
> APQ8064 would be the apcs_tmr block variant which is unchanged. MSM8916 as 
> well as IPQ4019 would use the new kpss variant.
> 
Unfortunately I don't have access to those datasheets.

> I went with block names I found internally here, but I will be the first to 
> admit I am terrible at names. The old block name for APQ was CPU0_ACPS_TMR 
> (where really the watchdog is a subset of a timer block), and on the IPQ4019 
> it’s called APCS_KPSS_WDT and it’s really just a watchdog block.
> 
> I kept the same driver because the register’s currently in use were 
> compatible. By the way, I tested this on an IPQ806x and IPQ4019 both new and 
> old blocks.
> 

The property name should probably be something like 'qcom,kpss-wdt'
(or 'qcom,kpss-watchdog' ?), possibly in addition to 'qcom,kpss-ipq4019-wdt'
and 'qcom,kpss-msm8916-wdt'.

Guenter

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

> On Mar 25, 2016, at 11:23 AM, Guenter Roeck  wrote:
> 
>> -#define WDT_RST 0x38
>> -#define WDT_EN  0x40
>> -#define WDT_BITE_TIME   0x5C
>> +enum wdt_reg {
>> +WDT_RST,
>> +WDT_EN,
>> +WDT_BITE_TIME,
>> +};
>> +
>> +static const u32 reg_offset_data_apcs_tmr[] = {
>> +[WDT_RST] = 0x38,
>> +[WDT_EN] = 0x40,
>> +[WDT_BITE_TIME] = 0x5C,
>> +};
>> +
>> +static const u32 reg_offset_data_kpss[] = {
>> +[WDT_RST] = 0x4,
>> +[WDT_EN] = 0x8,
> 
> Does this work ? In the datasheet I have in front of me (APQ8064), the 
> watchdog
> at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN),

0x40 is acps_tmr, and looks fine.

> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
> undefined.

I honestly don’t see anything at 0x8 for either blocks that looks like this. 
For the new block bit 0 is enabling and bit 1 enabled interrupts.

> Or does "qcom,kpss-standalone" refer to some other watchdog ?

APQ8064 would be the apcs_tmr block variant which is unchanged. MSM8916 as well 
as IPQ4019 would use the new kpss variant.

I went with block names I found internally here, but I will be the first to 
admit I am terrible at names. The old block name for APQ was CPU0_ACPS_TMR 
(where really the watchdog is a subset of a timer block), and on the IPQ4019 
it’s called APCS_KPSS_WDT and it’s really just a watchdog block.

I kept the same driver because the register’s currently in use were compatible. 
By the way, I tested this on an IPQ806x and IPQ4019 both new and old blocks.

Let me know if you need more details.

-M

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

> On Mar 25, 2016, at 11:23 AM, Guenter Roeck  wrote:
> 
>> -#define WDT_RST 0x38
>> -#define WDT_EN  0x40
>> -#define WDT_BITE_TIME   0x5C
>> +enum wdt_reg {
>> +WDT_RST,
>> +WDT_EN,
>> +WDT_BITE_TIME,
>> +};
>> +
>> +static const u32 reg_offset_data_apcs_tmr[] = {
>> +[WDT_RST] = 0x38,
>> +[WDT_EN] = 0x40,
>> +[WDT_BITE_TIME] = 0x5C,
>> +};
>> +
>> +static const u32 reg_offset_data_kpss[] = {
>> +[WDT_RST] = 0x4,
>> +[WDT_EN] = 0x8,
> 
> Does this work ? In the datasheet I have in front of me (APQ8064), the 
> watchdog
> at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN),

0x40 is acps_tmr, and looks fine.

> bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
> LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
> undefined.

I honestly don’t see anything at 0x8 for either blocks that looks like this. 
For the new block bit 0 is enabling and bit 1 enabled interrupts.

> Or does "qcom,kpss-standalone" refer to some other watchdog ?

APQ8064 would be the apcs_tmr block variant which is unchanged. MSM8916 as well 
as IPQ4019 would use the new kpss variant.

I went with block names I found internally here, but I will be the first to 
admit I am terrible at names. The old block name for APQ was CPU0_ACPS_TMR 
(where really the watchdog is a subset of a timer block), and on the IPQ4019 
it’s called APCS_KPSS_WDT and it’s really just a watchdog block.

I kept the same driver because the register’s currently in use were compatible. 
By the way, I tested this on an IPQ806x and IPQ4019 both new and old blocks.

Let me know if you need more details.

-M

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Guenter Roeck]

On Wed, Mar 23, 2016 at 05:05:02PM -0500, Matthew McClintock wrote:
> Commit 0dfd582e026a ("watchdog: qcom: use timer devicetree binding") moved
> to use the watchdog as a subset timer register block. Some devices have the
> watchdog completely standalone with slightly different register offsets as
> well so let's account for the differences here.
> 
> Signed-off-by: Matthew McClintock 
> ---
>  drivers/watchdog/qcom-wdt.c | 69 
> -
>  1 file changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 20563cc..e46f18d 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -19,17 +19,37 @@
>  #include 
>  #include 
>  
> -#define WDT_RST  0x38
> -#define WDT_EN   0x40
> -#define WDT_BITE_TIME0x5C
> +enum wdt_reg {
> + WDT_RST,
> + WDT_EN,
> + WDT_BITE_TIME,
> +};
> +
> +static const u32 reg_offset_data_apcs_tmr[] = {
> + [WDT_RST] = 0x38,
> + [WDT_EN] = 0x40,
> + [WDT_BITE_TIME] = 0x5C,
> +};
> +
> +static const u32 reg_offset_data_kpss[] = {
> + [WDT_RST] = 0x4,
> + [WDT_EN] = 0x8,

Does this work ? In the datasheet I have in front of me (APQ8064), the watchdog
at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN),
bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
undefined. Or does "qcom,kpss-standalone" refer to some other watchdog ?

Thanks,
Guenter

> + [WDT_BITE_TIME] = 0x14,
> +};
>  
>  struct qcom_wdt {
>   struct watchdog_device  wdd;
>   struct clk  *clk;
>   unsigned long   rate;
>   void __iomem*base;
> + const u32   *layout;
>  };
>  
> +static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)
> +{
> + return wdt->base + wdt->layout[reg];
> +}
> +
>  static inline
>  struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>  {
> @@ -40,10 +60,10 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
>  {
>   struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>  
> - writel(0, wdt->base + WDT_EN);
> - writel(1, wdt->base + WDT_RST);
> - writel(wdd->timeout * wdt->rate, wdt->base + WDT_BITE_TIME);
> - writel(1, wdt->base + WDT_EN);
> + writel(0, wdt_addr(wdt, WDT_EN));
> + writel(1, wdt_addr(wdt, WDT_RST));
> + writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
> + writel(1, wdt_addr(wdt, WDT_EN));
>   return 0;
>  }
>  
> @@ -51,7 +71,7 @@ static int qcom_wdt_stop(struct watchdog_device *wdd)
>  {
>   struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>  
> - writel(0, wdt->base + WDT_EN);
> + writel(0, wdt_addr(wdt, WDT_EN));
>   return 0;
>  }
>  
> @@ -59,7 +79,7 @@ static int qcom_wdt_ping(struct watchdog_device *wdd)
>  {
>   struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>  
> - writel(1, wdt->base + WDT_RST);
> + writel(1, wdt_addr(wdt, WDT_RST));
>   return 0;
>  }
>  
> @@ -82,10 +102,10 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, 
> unsigned long action,
>*/
>   timeout = 128 * wdt->rate / 1000;
>  
> - writel(0, wdt->base + WDT_EN);
> - writel(1, wdt->base + WDT_RST);
> - writel(timeout, wdt->base + WDT_BITE_TIME);
> - writel(1, wdt->base + WDT_EN);
> + writel(0, wdt_addr(wdt, WDT_EN));
> + writel(1, wdt_addr(wdt, WDT_RST));
> + writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
> + writel(1, wdt_addr(wdt, WDT_EN));
>  
>   /*
>* Actually make sure the above sequence hits hardware before sleeping.
> @@ -112,14 +132,29 @@ static const struct watchdog_info qcom_wdt_info = {
>   .identity   = KBUILD_MODNAME,
>  };
>  
> +static const struct of_device_id qcom_wdt_of_table[] = {
> + { .compatible = "qcom,kpss-timer", .data = reg_offset_data_apcs_tmr },
> + { .compatible = "qcom,scss-timer", .data = reg_offset_data_apcs_tmr },
> + { .compatible = "qcom,kpss-standalone", .data = _offset_data_kpss},
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
> +
>  static int qcom_wdt_probe(struct platform_device *pdev)
>  {
>   struct qcom_wdt *wdt;
>   struct resource *res;
>   struct device_node *np = pdev->dev.of_node;
> + const struct of_device_id *match;
>   u32 percpu_offset;
>   int ret;
>  
> + match = of_match_node(qcom_wdt_of_table, np);
> + if (!match) {
> + dev_err(>dev, "Unsupported QCOM WDT module\n");
> + return -ENODEV;
> + }
> +
>   wdt = devm_kzalloc(>dev, sizeof(*wdt), GFP_KERNEL);
>   if (!wdt)
>   return -ENOMEM;
> @@ -170,6 +205,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>   wdt->wdd.min_timeout = 1;
>   wdt->wdd.max_timeout = 0x1000U / wdt->rate;
>   wdt->wdd.parent = 

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Guenter Roeck]

On Wed, Mar 23, 2016 at 05:05:02PM -0500, Matthew McClintock wrote:
> Commit 0dfd582e026a ("watchdog: qcom: use timer devicetree binding") moved
> to use the watchdog as a subset timer register block. Some devices have the
> watchdog completely standalone with slightly different register offsets as
> well so let's account for the differences here.
> 
> Signed-off-by: Matthew McClintock 
> ---
>  drivers/watchdog/qcom-wdt.c | 69 
> -
>  1 file changed, 49 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
> index 20563cc..e46f18d 100644
> --- a/drivers/watchdog/qcom-wdt.c
> +++ b/drivers/watchdog/qcom-wdt.c
> @@ -19,17 +19,37 @@
>  #include 
>  #include 
>  
> -#define WDT_RST  0x38
> -#define WDT_EN   0x40
> -#define WDT_BITE_TIME0x5C
> +enum wdt_reg {
> + WDT_RST,
> + WDT_EN,
> + WDT_BITE_TIME,
> +};
> +
> +static const u32 reg_offset_data_apcs_tmr[] = {
> + [WDT_RST] = 0x38,
> + [WDT_EN] = 0x40,
> + [WDT_BITE_TIME] = 0x5C,
> +};
> +
> +static const u32 reg_offset_data_kpss[] = {
> + [WDT_RST] = 0x4,
> + [WDT_EN] = 0x8,

Does this work ? In the datasheet I have in front of me (APQ8064), the watchdog
at this address uses different bits. At address 0x40 (eg GSS_A5_APCS_WDT0_EN),
bit 0 is the enable bit, and bit 1 enables interrupts. At address 0x08 (eg
LPASS_QDSP6SS_WDOG_UNMASKED_INT_EN), bit 0 enables interrupts and bit 1 is
undefined. Or does "qcom,kpss-standalone" refer to some other watchdog ?

Thanks,
Guenter

> + [WDT_BITE_TIME] = 0x14,
> +};
>  
>  struct qcom_wdt {
>   struct watchdog_device  wdd;
>   struct clk  *clk;
>   unsigned long   rate;
>   void __iomem*base;
> + const u32   *layout;
>  };
>  
> +static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)
> +{
> + return wdt->base + wdt->layout[reg];
> +}
> +
>  static inline
>  struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
>  {
> @@ -40,10 +60,10 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
>  {
>   struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>  
> - writel(0, wdt->base + WDT_EN);
> - writel(1, wdt->base + WDT_RST);
> - writel(wdd->timeout * wdt->rate, wdt->base + WDT_BITE_TIME);
> - writel(1, wdt->base + WDT_EN);
> + writel(0, wdt_addr(wdt, WDT_EN));
> + writel(1, wdt_addr(wdt, WDT_RST));
> + writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
> + writel(1, wdt_addr(wdt, WDT_EN));
>   return 0;
>  }
>  
> @@ -51,7 +71,7 @@ static int qcom_wdt_stop(struct watchdog_device *wdd)
>  {
>   struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>  
> - writel(0, wdt->base + WDT_EN);
> + writel(0, wdt_addr(wdt, WDT_EN));
>   return 0;
>  }
>  
> @@ -59,7 +79,7 @@ static int qcom_wdt_ping(struct watchdog_device *wdd)
>  {
>   struct qcom_wdt *wdt = to_qcom_wdt(wdd);
>  
> - writel(1, wdt->base + WDT_RST);
> + writel(1, wdt_addr(wdt, WDT_RST));
>   return 0;
>  }
>  
> @@ -82,10 +102,10 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, 
> unsigned long action,
>*/
>   timeout = 128 * wdt->rate / 1000;
>  
> - writel(0, wdt->base + WDT_EN);
> - writel(1, wdt->base + WDT_RST);
> - writel(timeout, wdt->base + WDT_BITE_TIME);
> - writel(1, wdt->base + WDT_EN);
> + writel(0, wdt_addr(wdt, WDT_EN));
> + writel(1, wdt_addr(wdt, WDT_RST));
> + writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
> + writel(1, wdt_addr(wdt, WDT_EN));
>  
>   /*
>* Actually make sure the above sequence hits hardware before sleeping.
> @@ -112,14 +132,29 @@ static const struct watchdog_info qcom_wdt_info = {
>   .identity   = KBUILD_MODNAME,
>  };
>  
> +static const struct of_device_id qcom_wdt_of_table[] = {
> + { .compatible = "qcom,kpss-timer", .data = reg_offset_data_apcs_tmr },
> + { .compatible = "qcom,scss-timer", .data = reg_offset_data_apcs_tmr },
> + { .compatible = "qcom,kpss-standalone", .data = _offset_data_kpss},
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
> +
>  static int qcom_wdt_probe(struct platform_device *pdev)
>  {
>   struct qcom_wdt *wdt;
>   struct resource *res;
>   struct device_node *np = pdev->dev.of_node;
> + const struct of_device_id *match;
>   u32 percpu_offset;
>   int ret;
>  
> + match = of_match_node(qcom_wdt_of_table, np);
> + if (!match) {
> + dev_err(>dev, "Unsupported QCOM WDT module\n");
> + return -ENODEV;
> + }
> +
>   wdt = devm_kzalloc(>dev, sizeof(*wdt), GFP_KERNEL);
>   if (!wdt)
>   return -ENOMEM;
> @@ -170,6 +205,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
>   wdt->wdd.min_timeout = 1;
>   wdt->wdd.max_timeout = 0x1000U / wdt->rate;
>   wdt->wdd.parent = >dev;
> + wdt->layout = 

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Stephen Boyd]

On 03/23, Matthew McClintock wrote:
> @@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev)
>   return 0;
>  }
>  
> -static const struct of_device_id qcom_wdt_of_table[] = {
> - { .compatible = "qcom,kpss-timer" },
> - { .compatible = "qcom,scss-timer" },
> - { },
> -};
> -MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
> -

Leave this here and use of_device_get_match_data() in probe
instead.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Stephen Boyd]

On 03/23, Matthew McClintock wrote:
> @@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev)
>   return 0;
>  }
>  
> -static const struct of_device_id qcom_wdt_of_table[] = {
> - { .compatible = "qcom,kpss-timer" },
> - { .compatible = "qcom,scss-timer" },
> - { },
> -};
> -MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
> -

Leave this here and use of_device_get_match_data() in probe
instead.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

Commit 0dfd582e026a ("watchdog: qcom: use timer devicetree binding") moved
to use the watchdog as a subset timer register block. Some devices have the
watchdog completely standalone with slightly different register offsets as
well so let's account for the differences here.

Signed-off-by: Matthew McClintock 
---
 drivers/watchdog/qcom-wdt.c | 69 -
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 20563cc..e46f18d 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -19,17 +19,37 @@
 #include 
 #include 
 
-#define WDT_RST0x38
-#define WDT_EN 0x40
-#define WDT_BITE_TIME  0x5C
+enum wdt_reg {
+   WDT_RST,
+   WDT_EN,
+   WDT_BITE_TIME,
+};
+
+static const u32 reg_offset_data_apcs_tmr[] = {
+   [WDT_RST] = 0x38,
+   [WDT_EN] = 0x40,
+   [WDT_BITE_TIME] = 0x5C,
+};
+
+static const u32 reg_offset_data_kpss[] = {
+   [WDT_RST] = 0x4,
+   [WDT_EN] = 0x8,
+   [WDT_BITE_TIME] = 0x14,
+};
 
 struct qcom_wdt {
struct watchdog_device  wdd;
struct clk  *clk;
unsigned long   rate;
void __iomem*base;
+   const u32   *layout;
 };
 
+static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)
+{
+   return wdt->base + wdt->layout[reg];
+}
+
 static inline
 struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
 {
@@ -40,10 +60,10 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
 {
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
 
-   writel(0, wdt->base + WDT_EN);
-   writel(1, wdt->base + WDT_RST);
-   writel(wdd->timeout * wdt->rate, wdt->base + WDT_BITE_TIME);
-   writel(1, wdt->base + WDT_EN);
+   writel(0, wdt_addr(wdt, WDT_EN));
+   writel(1, wdt_addr(wdt, WDT_RST));
+   writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
+   writel(1, wdt_addr(wdt, WDT_EN));
return 0;
 }
 
@@ -51,7 +71,7 @@ static int qcom_wdt_stop(struct watchdog_device *wdd)
 {
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
 
-   writel(0, wdt->base + WDT_EN);
+   writel(0, wdt_addr(wdt, WDT_EN));
return 0;
 }
 
@@ -59,7 +79,7 @@ static int qcom_wdt_ping(struct watchdog_device *wdd)
 {
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
 
-   writel(1, wdt->base + WDT_RST);
+   writel(1, wdt_addr(wdt, WDT_RST));
return 0;
 }
 
@@ -82,10 +102,10 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, 
unsigned long action,
 */
timeout = 128 * wdt->rate / 1000;
 
-   writel(0, wdt->base + WDT_EN);
-   writel(1, wdt->base + WDT_RST);
-   writel(timeout, wdt->base + WDT_BITE_TIME);
-   writel(1, wdt->base + WDT_EN);
+   writel(0, wdt_addr(wdt, WDT_EN));
+   writel(1, wdt_addr(wdt, WDT_RST));
+   writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
+   writel(1, wdt_addr(wdt, WDT_EN));
 
/*
 * Actually make sure the above sequence hits hardware before sleeping.
@@ -112,14 +132,29 @@ static const struct watchdog_info qcom_wdt_info = {
.identity   = KBUILD_MODNAME,
 };
 
+static const struct of_device_id qcom_wdt_of_table[] = {
+   { .compatible = "qcom,kpss-timer", .data = reg_offset_data_apcs_tmr },
+   { .compatible = "qcom,scss-timer", .data = reg_offset_data_apcs_tmr },
+   { .compatible = "qcom,kpss-standalone", .data = _offset_data_kpss},
+   { },
+};
+MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
+
 static int qcom_wdt_probe(struct platform_device *pdev)
 {
struct qcom_wdt *wdt;
struct resource *res;
struct device_node *np = pdev->dev.of_node;
+   const struct of_device_id *match;
u32 percpu_offset;
int ret;
 
+   match = of_match_node(qcom_wdt_of_table, np);
+   if (!match) {
+   dev_err(>dev, "Unsupported QCOM WDT module\n");
+   return -ENODEV;
+   }
+
wdt = devm_kzalloc(>dev, sizeof(*wdt), GFP_KERNEL);
if (!wdt)
return -ENOMEM;
@@ -170,6 +205,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
wdt->wdd.min_timeout = 1;
wdt->wdd.max_timeout = 0x1000U / wdt->rate;
wdt->wdd.parent = >dev;
+   wdt->layout = match->data;
 
/*
 * If 'timeout-sec' unspecified in devicetree, assume a 30 second
@@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev)
return 0;
 }
 
-static const struct of_device_id qcom_wdt_of_table[] = {
-   { .compatible = "qcom,kpss-timer" },
-   { .compatible = "qcom,scss-timer" },
-   { },
-};
-MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
-
 static struct platform_driver qcom_watchdog_driver = {
.probe  = qcom_wdt_probe,
.remove = qcom_wdt_remove,
-- 
The Qualcomm Innovation Center, Inc. is a member of the 

[PATCH 07/17] watchdog: qcom: add option for standalone watchdog not in timer block

[Matthew McClintock]

Commit 0dfd582e026a ("watchdog: qcom: use timer devicetree binding") moved
to use the watchdog as a subset timer register block. Some devices have the
watchdog completely standalone with slightly different register offsets as
well so let's account for the differences here.

Signed-off-by: Matthew McClintock 
---
 drivers/watchdog/qcom-wdt.c | 69 -
 1 file changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/watchdog/qcom-wdt.c b/drivers/watchdog/qcom-wdt.c
index 20563cc..e46f18d 100644
--- a/drivers/watchdog/qcom-wdt.c
+++ b/drivers/watchdog/qcom-wdt.c
@@ -19,17 +19,37 @@
 #include 
 #include 
 
-#define WDT_RST0x38
-#define WDT_EN 0x40
-#define WDT_BITE_TIME  0x5C
+enum wdt_reg {
+   WDT_RST,
+   WDT_EN,
+   WDT_BITE_TIME,
+};
+
+static const u32 reg_offset_data_apcs_tmr[] = {
+   [WDT_RST] = 0x38,
+   [WDT_EN] = 0x40,
+   [WDT_BITE_TIME] = 0x5C,
+};
+
+static const u32 reg_offset_data_kpss[] = {
+   [WDT_RST] = 0x4,
+   [WDT_EN] = 0x8,
+   [WDT_BITE_TIME] = 0x14,
+};
 
 struct qcom_wdt {
struct watchdog_device  wdd;
struct clk  *clk;
unsigned long   rate;
void __iomem*base;
+   const u32   *layout;
 };
 
+static void __iomem *wdt_addr(struct qcom_wdt *wdt, enum wdt_reg reg)
+{
+   return wdt->base + wdt->layout[reg];
+}
+
 static inline
 struct qcom_wdt *to_qcom_wdt(struct watchdog_device *wdd)
 {
@@ -40,10 +60,10 @@ static int qcom_wdt_start(struct watchdog_device *wdd)
 {
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
 
-   writel(0, wdt->base + WDT_EN);
-   writel(1, wdt->base + WDT_RST);
-   writel(wdd->timeout * wdt->rate, wdt->base + WDT_BITE_TIME);
-   writel(1, wdt->base + WDT_EN);
+   writel(0, wdt_addr(wdt, WDT_EN));
+   writel(1, wdt_addr(wdt, WDT_RST));
+   writel(wdd->timeout * wdt->rate, wdt_addr(wdt, WDT_BITE_TIME));
+   writel(1, wdt_addr(wdt, WDT_EN));
return 0;
 }
 
@@ -51,7 +71,7 @@ static int qcom_wdt_stop(struct watchdog_device *wdd)
 {
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
 
-   writel(0, wdt->base + WDT_EN);
+   writel(0, wdt_addr(wdt, WDT_EN));
return 0;
 }
 
@@ -59,7 +79,7 @@ static int qcom_wdt_ping(struct watchdog_device *wdd)
 {
struct qcom_wdt *wdt = to_qcom_wdt(wdd);
 
-   writel(1, wdt->base + WDT_RST);
+   writel(1, wdt_addr(wdt, WDT_RST));
return 0;
 }
 
@@ -82,10 +102,10 @@ static int qcom_wdt_restart(struct watchdog_device *wdd, 
unsigned long action,
 */
timeout = 128 * wdt->rate / 1000;
 
-   writel(0, wdt->base + WDT_EN);
-   writel(1, wdt->base + WDT_RST);
-   writel(timeout, wdt->base + WDT_BITE_TIME);
-   writel(1, wdt->base + WDT_EN);
+   writel(0, wdt_addr(wdt, WDT_EN));
+   writel(1, wdt_addr(wdt, WDT_RST));
+   writel(timeout, wdt_addr(wdt, WDT_BITE_TIME));
+   writel(1, wdt_addr(wdt, WDT_EN));
 
/*
 * Actually make sure the above sequence hits hardware before sleeping.
@@ -112,14 +132,29 @@ static const struct watchdog_info qcom_wdt_info = {
.identity   = KBUILD_MODNAME,
 };
 
+static const struct of_device_id qcom_wdt_of_table[] = {
+   { .compatible = "qcom,kpss-timer", .data = reg_offset_data_apcs_tmr },
+   { .compatible = "qcom,scss-timer", .data = reg_offset_data_apcs_tmr },
+   { .compatible = "qcom,kpss-standalone", .data = _offset_data_kpss},
+   { },
+};
+MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
+
 static int qcom_wdt_probe(struct platform_device *pdev)
 {
struct qcom_wdt *wdt;
struct resource *res;
struct device_node *np = pdev->dev.of_node;
+   const struct of_device_id *match;
u32 percpu_offset;
int ret;
 
+   match = of_match_node(qcom_wdt_of_table, np);
+   if (!match) {
+   dev_err(>dev, "Unsupported QCOM WDT module\n");
+   return -ENODEV;
+   }
+
wdt = devm_kzalloc(>dev, sizeof(*wdt), GFP_KERNEL);
if (!wdt)
return -ENOMEM;
@@ -170,6 +205,7 @@ static int qcom_wdt_probe(struct platform_device *pdev)
wdt->wdd.min_timeout = 1;
wdt->wdd.max_timeout = 0x1000U / wdt->rate;
wdt->wdd.parent = >dev;
+   wdt->layout = match->data;
 
/*
 * If 'timeout-sec' unspecified in devicetree, assume a 30 second
@@ -202,13 +238,6 @@ static int qcom_wdt_remove(struct platform_device *pdev)
return 0;
 }
 
-static const struct of_device_id qcom_wdt_of_table[] = {
-   { .compatible = "qcom,kpss-timer" },
-   { .compatible = "qcom,scss-timer" },
-   { },
-};
-MODULE_DEVICE_TABLE(of, qcom_wdt_of_table);
-
 static struct platform_driver qcom_watchdog_driver = {
.probe  = qcom_wdt_probe,
.remove = qcom_wdt_remove,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a