diff mbox series

[1/2,v8] dt-bindings: watchdog: marvell GTI system watchdog driver

Message ID 20230526062626.1180-1-bbhushan2@marvell.com
State New
Headers show
Series [1/2,v8] dt-bindings: watchdog: marvell GTI system watchdog driver | expand

Commit Message

Bharat Bhushan May 26, 2023, 6:26 a.m. UTC 
Add binding documentation for the Marvell GTI system
watchdog driver.

Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
---
v8:
  - Compatible name as per soc name

 .../watchdog/marvell,octeontx2-wdt.yaml       | 73 +++++++++++++++++++
 1 file changed, 73 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml

Comments

Conor Dooley May 26, 2023, 7:37 p.m. UTC | #1 
Yo Bharat,

On Fri, May 26, 2023 at 11:56:25AM +0530, Bharat Bhushan wrote:
> Add binding documentation for the Marvell GTI system
> watchdog driver.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> ---
> v8:
>   - Compatible name as per soc name

I am sorry, but I do not understand this.

> 
>  .../watchdog/marvell,octeontx2-wdt.yaml       | 73 +++++++++++++++++++
>  1 file changed, 73 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
> 
> diff --git a/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
> new file mode 100644
> index 000000000000..3c642359d960
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
> @@ -0,0 +1,73 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/watchdog/marvell,octeontx2-wdt.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Global Timer (GTI) system watchdog
> +
> +allOf:
> +  - $ref: watchdog.yaml#

From v7:
Put allOf after maintainers:.

> +
> +maintainers:
> +  - Bharat Bhushan <bbhushan2@marvell.com>
> +
> +properties:
> +  compatible:
> +    enum:
> +      - marvell,cn9670-wdt
> +      - marvell,cn9880-wdt
> +      - marvell,cnf9535-wdt
> +      - marvell,cn10624-wdt
> +      - marvell,cn10308-wdt
> +      - marvell,cnf10518-wdt

static const struct of_device_id gti_wdt_of_match[] = {
       { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
       { .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
       { .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
       { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
       { .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
       { .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},

This is a fat hint that you should be using fallback compatibles here.
You even had a fallback setup in your last revision, but you seem to
have removed it alongside the removal of the wildcards. Why did you do
that?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    minItems: 1

From v7:
maxItems instead. You see it is different than above properties?

> +
> +  clock-names:
> +    minItems: 1

From v7:
Need to define names.

Cheers,
Conor.
Bharat Bhushan May 27, 2023, 2:53 p.m. UTC | #2 
Hi Conor,

Please see inline 

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Saturday, May 27, 2023 1:07 AM
> To: Bharat Bhushan <bbhushan2@marvell.com>
> Cc: wim@linux-watchdog.org; linux@roeck-us.net; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; linux-watchdog@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>
> Subject: [EXT] Re: [PATCH 1/2 v8] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> External Email
> 
> ----------------------------------------------------------------------
> Yo Bharat,
> 
> On Fri, May 26, 2023 at 11:56:25AM +0530, Bharat Bhushan wrote:
> > Add binding documentation for the Marvell GTI system watchdog driver.
> >
> > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> > ---
> > v8:
> >   - Compatible name as per soc name
> 
> I am sorry, but I do not understand this.

I mean to say replaced soc family name from compatible with actual soc names

> 
> >
> >  .../watchdog/marvell,octeontx2-wdt.yaml       | 73 +++++++++++++++++++
> >  1 file changed, 73 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
> >
> > diff --git
> > a/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yam
> > l
> > b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yam
> > l
> > new file mode 100644
> > index 000000000000..3c642359d960
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt
> > +++ .yaml
> > @@ -0,0 +1,73 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) %YAML 1.2
> > +---
> > +$id:
> > +http://devicetree.org/schemas/watchdog/marvell,octeontx2-wdt.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Marvell Global Timer (GTI) system watchdog
> > +
> > +allOf:
> > +  - $ref: watchdog.yaml#
> 
> From v7:
> Put allOf after maintainers:.

Thanks for pointing, I am sorry, missed almost all comments on v7. Will resolve this and below as well in next version

> 
> > +
> > +maintainers:
> > +  - Bharat Bhushan <bbhushan2@marvell.com>
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - marvell,cn9670-wdt
> > +      - marvell,cn9880-wdt
> > +      - marvell,cnf9535-wdt
> > +      - marvell,cn10624-wdt
> > +      - marvell,cn10308-wdt
> > +      - marvell,cnf10518-wdt
> 
> static const struct of_device_id gti_wdt_of_match[] = {
>        { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
>        { .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
>        { .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
>        { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
>        { .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
>        { .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},
> 
> This is a fat hint that you should be using fallback compatibles here.
> You even had a fallback setup in your last revision, but you seem to have
> removed it alongside the removal of the wildcards. Why did you do that?

Not sure I understand this comment, Compatible in last version was as below:

+ properties:
+ compatible:
+    oneOf:
+      - const: marvell,octeontx2-wdt
+      - items:
+          - enum:
+              - marvell,octeontx2-95xx-wdt
+              - marvell,octeontx2-96xx-wdt
+              - marvell,octeontx2-98xx-wdt
+          - const: marvell,octeontx2-wdt
+      - const: marvell,cn10k-wdt
+      - items:
+          - enum:
+              - marvell,cn10kx-wdt
+              - marvell,cnf10kx-wdt
+          - const: marvell,cn10k-wdt

By fallback do you mean " const: marvell,cn10k-wdt" and " const: marvell,octeontx2-wdt" ? If yes I removed because "octeontx2" and "cn10k" are soc family name and no a specific soc.

Thanks
-Bharat

> 
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  clocks:
> > +    minItems: 1
> 
> From v7:
> maxItems instead. You see it is different than above properties?
> 
> > +
> > +  clock-names:
> > +    minItems: 1
> 
> From v7:
> Need to define names.
> 
> Cheers,
> Conor.
Conor Dooley May 27, 2023, 4:10 p.m. UTC | #3 
On Sat, May 27, 2023 at 02:53:25PM +0000, Bharat Bhushan wrote:
> From: Conor Dooley <conor@kernel.org>
> > On Fri, May 26, 2023 at 11:56:25AM +0530, Bharat Bhushan wrote:

> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - marvell,cn9670-wdt
> > > +      - marvell,cn9880-wdt
> > > +      - marvell,cnf9535-wdt
> > > +      - marvell,cn10624-wdt
> > > +      - marvell,cn10308-wdt
> > > +      - marvell,cnf10518-wdt
> > 
> > static const struct of_device_id gti_wdt_of_match[] = {
> >        { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> >        { .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> >        { .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> >        { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
> >        { .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
> >        { .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},
> > 
> > This is a fat hint that you should be using fallback compatibles here.
> > You even had a fallback setup in your last revision, but you seem to have
> > removed it alongside the removal of the wildcards. Why did you do that?
> 
> Not sure I understand this comment, Compatible in last version was as below:
> 
> + properties:
> + compatible:
> +    oneOf:
> +      - const: marvell,octeontx2-wdt
> +      - items:
> +          - enum:
> +              - marvell,octeontx2-95xx-wdt
> +              - marvell,octeontx2-96xx-wdt
> +              - marvell,octeontx2-98xx-wdt
> +          - const: marvell,octeontx2-wdt
> +      - const: marvell,cn10k-wdt
> +      - items:
> +          - enum:
> +              - marvell,cn10kx-wdt
> +              - marvell,cnf10kx-wdt
> +          - const: marvell,cn10k-wdt
> 
> By fallback do you mean " const: marvell,cn10k-wdt" and
> "const: marvell,octeontx2-wdt" ? If yes I removed because "octeontx2"
> and "cn10k" are soc family name and no a specific soc.

No, I meant that you should permit
	compatible = "marvell,cn9880-wdt", "marvell,cn9670-wdt";
and
	compatible = "marvell,cnf9535-wdt", "marvell,cn9670-wdt";
and
	compatible = "marvell,cn9670-wdt";
so the driver only needs to contain
	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
instead of 
	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
	{ .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
	{ .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},

Note that using fallback compatibles is separate from using wildcards,
and I was not suggesting that you go back to wildcards ;)

Cheers,
Conor.
Bharat Bhushan May 29, 2023, 6:14 a.m. UTC | #4 
Please see inline

> -----Original Message-----
> From: Conor Dooley <conor@kernel.org>
> Sent: Saturday, May 27, 2023 9:40 PM
> To: Bharat Bhushan <bbhushan2@marvell.com>
> Cc: wim@linux-watchdog.org; linux@roeck-us.net; robh+dt@kernel.org;
> krzysztof.kozlowski+dt@linaro.org; linux-watchdog@vger.kernel.org;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>
> Subject: Re: [EXT] Re: [PATCH 1/2 v8] dt-bindings: watchdog: marvell GTI system
> watchdog driver
> 
> On Sat, May 27, 2023 at 02:53:25PM +0000, Bharat Bhushan wrote:
> > From: Conor Dooley <conor@kernel.org>
> > > On Fri, May 26, 2023 at 11:56:25AM +0530, Bharat Bhushan wrote:
> 
> > > > +properties:
> > > > +  compatible:
> > > > +    enum:
> > > > +      - marvell,cn9670-wdt
> > > > +      - marvell,cn9880-wdt
> > > > +      - marvell,cnf9535-wdt
> > > > +      - marvell,cn10624-wdt
> > > > +      - marvell,cn10308-wdt
> > > > +      - marvell,cnf10518-wdt
> > >
> > > static const struct of_device_id gti_wdt_of_match[] = {
> > >        { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> > >        { .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> > >        { .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> > >        { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
> > >        { .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
> > >        { .compatible = "marvell,cnf10518-wdt", .data =
> > > &match_data_cn10k},
> > >
> > > This is a fat hint that you should be using fallback compatibles here.
> > > You even had a fallback setup in your last revision, but you seem to
> > > have removed it alongside the removal of the wildcards. Why did you do
> that?
> >
> > Not sure I understand this comment, Compatible in last version was as below:
> >
> > + properties:
> > + compatible:
> > +    oneOf:
> > +      - const: marvell,octeontx2-wdt
> > +      - items:
> > +          - enum:
> > +              - marvell,octeontx2-95xx-wdt
> > +              - marvell,octeontx2-96xx-wdt
> > +              - marvell,octeontx2-98xx-wdt
> > +          - const: marvell,octeontx2-wdt
> > +      - const: marvell,cn10k-wdt
> > +      - items:
> > +          - enum:
> > +              - marvell,cn10kx-wdt
> > +              - marvell,cnf10kx-wdt
> > +          - const: marvell,cn10k-wdt
> >
> > By fallback do you mean " const: marvell,cn10k-wdt" and
> > "const: marvell,octeontx2-wdt" ? If yes I removed because "octeontx2"
> > and "cn10k" are soc family name and no a specific soc.
> 
> No, I meant that you should permit
> 	compatible = "marvell,cn9880-wdt", "marvell,cn9670-wdt"; and
> 	compatible = "marvell,cnf9535-wdt", "marvell,cn9670-wdt"; and
> 	compatible = "marvell,cn9670-wdt";
> so the driver only needs to contain
> 	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> instead of
> 	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> 	{ .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> 	{ .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> 
> Note that using fallback compatibles is separate from using wildcards, and I was
> not suggesting that you go back to wildcards ;)

Fallback you mentioned make code look simple. Is below representation correct for above mentioned fallback? 

properties:
  compatible:
    oneOf:
      - const: marvell,cn9670-wdt
      - items:
          - enum:
              - marvell,cn9880-wdt
              - marvell,cnf9535-wdt
          - const: marvell,cn9670-wdt
      - const: marvell,cn10624-wdt
      - items:
          - enum:
              - marvell,cn10308-wdt
              - marvell,cnf10518-wdt
          - const: marvell,cn10624-wdt


And driver will have
        { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
        { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},


Thanks
-Bharat

> 
> Cheers,
> Conor.
Conor Dooley May 29, 2023, 12:34 p.m. UTC | #5 
Hey Bharat,

On Mon, May 29, 2023 at 06:14:07AM +0000, Bharat Bhushan wrote:

> Fallback you mentioned make code look simple. Is below representation correct for above mentioned fallback? 
> 
> properties:
>   compatible:
>     oneOf:
>       - const: marvell,cn9670-wdt
>       - items:
>           - enum:
>               - marvell,cn9880-wdt
>               - marvell,cnf9535-wdt
>           - const: marvell,cn9670-wdt
>       - const: marvell,cn10624-wdt
>       - items:
>           - enum:
>               - marvell,cn10308-wdt
>               - marvell,cnf10518-wdt
>           - const: marvell,cn10624-wdt

Instead of having const: bits for each of the single-compatible ones, if
you are not going to add descriptions, I'd probably do:
properties:
  compatible:
    oneOf:
      - enum:
          - marvell,cn9670-wdt
          - marvell,cn10624-wdt

      - items:
          - enum:
              - marvell,cn9880-wdt
              - marvell,cnf9535-wdt
          - const: marvell,cn9670-wdt

      - items:
          - enum:
              - marvell,cn10308-wdt
              - marvell,cnf10518-wdt
          - const: marvell,cn10624-wdt

> And driver will have
>         { .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
>         { .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},

Otherwise, looks good to me. You should probably also rename the file to
match one of the compatibles contained in it.

Thanks,
Conor.
Guenter Roeck May 29, 2023, 1:38 p.m. UTC | #6 
On 5/25/23 23:26, Bharat Bhushan wrote:
> This patch add support for Marvell GTI watchdog.  Global timer
> unit (GTI) support hardware watchdog timer. Software programs
> watchdog timer to generate interrupt on first timeout, second
> timeout is configured to be ignored and system reboots on
> third timeout.
> 
> Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> ---
> v8:
>    - Compatible names as per soc names
>    - This driver run on ARM64 based architecture, Added 64BIT config
>      dependency to avoid compilation error related to readq/writeq on
>      32 as platform (Hexagon)
>   

In the future, please provide complete change logs.

>   drivers/watchdog/Kconfig           |  13 ++
>   drivers/watchdog/Makefile          |   1 +
>   drivers/watchdog/marvell_gti_wdt.c | 346 +++++++++++++++++++++++++++++
>   3 files changed, 360 insertions(+)
>   create mode 100644 drivers/watchdog/marvell_gti_wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index f22138709bf5..bbdc20f33dbf 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1779,6 +1779,19 @@ config OCTEON_WDT
>   	  from the first interrupt, it is then only poked when the
>   	  device is written.
>   
> +config MARVELL_GTI_WDT
> +	tristate "Marvell GTI Watchdog driver"
> +	depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
> +	default y

Does this watchdog exist on all Thunder processors ?
If not please drop.

> +	select WATCHDOG_CORE
> +	help
> +	 Marvell GTI hardware supports watchdog timer. First timeout
> +	 works as watchdog pretimeout and installed interrupt handler
> +	 will be called on first timeout. Hardware can generate interrupt
> +	 to SCP on second timeout but it is not enabled, So second
> +	 timeout is ignored. If device poke does not happen then system
> +	 will reboot on third timeout.
> +
>   config BCM2835_WDT
>   	tristate "Broadcom BCM2835 hardware watchdog"
>   	depends on ARCH_BCM2835 || (OF && COMPILE_TEST)
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index b4c4ccf2d703..a164cd161ef3 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
>   obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
>   obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
>   obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
> +obj-$(CONFIG_MARVELL_GTI_WDT) += marvell_gti_wdt.o
>   
>   # X86 (i386 + ia64 + x86_64) Architecture
>   obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o
> diff --git a/drivers/watchdog/marvell_gti_wdt.c b/drivers/watchdog/marvell_gti_wdt.c
> new file mode 100644
> index 000000000000..976bb9306115
> --- /dev/null
> +++ b/drivers/watchdog/marvell_gti_wdt.c
> @@ -0,0 +1,346 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Marvell GTI Watchdog driver
> + *
> + * Copyright (C) 2023 Marvell.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/interrupt.h>
> +#include <linux/of_platform.h>
> +#include <linux/platform_device.h>
> +#include <linux/watchdog.h>
> +#include <linux/clk.h>
> +
Alphabetic order, please. Also, include io.h since
you are using io functions (writeq).

> +/*
> + * Hardware supports following mode of operation:
> + * 1) Interrupt Only:
> + *    This will generate the interrupt to arm core whenever timeout happens.
> + *
> + * 2) Interrupt + del3t (Interrupt to firmware (SCP processor)).
> + *    This will generate interrupt to arm core on 1st timeout happens
> + *    This will generate interrupt to SCP processor on 2nd timeout happens
> + *
> + * 3) Interrupt + Interrupt to SCP processor (called delt3t) + reboot.
> + *    This will generate interrupt to arm core on 1st timeout happens
> + *    Will generate interrupt to SCP processor on 2nd timeout happens,
> + *    if interrupt is configured.
> + *    Reboot on 3rd timeout.
> + *
> + * Driver will use hardware in mode-3 above so that system can reboot in case
> + * a hardware hang. Also h/w is configured not to generate SCP interrupt, so
> + * effectively 2nd timeout is ignored within hardware.
> + *
> + * First timeout is effectively watchdog pretimeout.
> + */
> +
> +/* GTI CWD Watchdog (GTI_CWD_WDOG) Register */
> +#define GTI_CWD_WDOG(reg_offset)	(0x8 * reg_offset)

reg_offset in ()

> +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST	0x3
> +#define GTI_CWD_WDOG_MODE_MASK		GENMASK_ULL(1, 0)
> +#define GTI_CWD_WDOG_LEN_SHIFT		4
> +#define GTI_CWD_WDOG_LEN_MASK		GENMASK_ULL(19, 4)
> +#define GTI_CWD_WDOG_CNT_SHIFT		20
> +#define GTI_CWD_WDOG_CNT_MASK		GENMASK_ULL(43, 20)
> +
> +/* GTI CWD Watchdog Interrupt (GTI_CWD_INT) Register */
> +#define GTI_CWD_INT			0x200
> +#define GTI_CWD_INT_PENDING_STATUS(bit)	(1 << bit)
> +
> +/* GTI CWD Watchdog Interrupt Enable Clear (GTI_CWD_INT_ENA_CLR) Register */
> +#define GTI_CWD_INT_ENA_CLR		0x210
> +#define GTI_CWD_INT_ENA_CLR_VAL(bit)	(1 << bit)

Please use BIT().

> +
> +/* GTI CWD Watchdog Interrupt Enable Set (GTI_CWD_INT_ENA_SET) Register */
> +#define GTI_CWD_INT_ENA_SET		0x218
> +#define GTI_CWD_INT_ENA_SET_VAL(bit)	(1 << bit)
> +
> +/* GTI CWD Watchdog Poke (GTI_CWD_POKE) Registers */
> +#define GTI_CWD_POKE(reg_offset)	(0x10000 + 0x8 * reg_offset)
> +#define GTI_CWD_POKE_VAL		1
> +
> +struct gti_match_data {
> +	u32 gti_num_timers;
> +};
> +
> +static const struct gti_match_data match_data_octeontx2 = {

The driver depends on ARCH_THUNDER but not ARCH_THUNDER2.

> +	.gti_num_timers = 54,
> +};
> +
> +static const struct gti_match_data match_data_cn10k = {
> +	.gti_num_timers = 64,
> +};
> +
> +struct gti_wdt_priv {
> +	struct watchdog_device wdev;
> +	void __iomem *base;
> +	u32 clock_freq;
> +	struct clk *sclk;
> +	/* wdt_timer_idx used for timer to be used for system watchdog */
> +	u32 wdt_timer_idx;
> +	const struct gti_match_data *data;
> +};
> +
> +static irqreturn_t gti_wdt_interrupt(int irq, void *data)
> +{
> +	struct watchdog_device *wdev = data;
> +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> +	/* Clear Interrupt Pending Status */
> +	writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
> +	       priv->base + GTI_CWD_INT);
> +
> +	watchdog_notify_pretimeout(wdev);
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int gti_wdt_ping(struct watchdog_device *wdev)
> +{
> +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +
> +	writeq(GTI_CWD_POKE_VAL,
> +	       priv->base + GTI_CWD_POKE(priv->wdt_timer_idx));
> +
> +	return 0;
> +}
> +
> +static int gti_wdt_start(struct watchdog_device *wdev)
> +{
> +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	u64 regval;
> +
> +	if (!wdev->pretimeout)
> +		return -EINVAL;
> +
> +	set_bit(WDOG_HW_RUNNING, &wdev->status);
> +
> +	/* Clear any pending interrupt */
> +	writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
> +	       priv->base + GTI_CWD_INT);
> +
> +	/* Enable Interrupt */
> +	writeq(GTI_CWD_INT_ENA_SET_VAL(priv->wdt_timer_idx),
> +	       priv->base + GTI_CWD_INT_ENA_SET);
> +
> +	/* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
> +	regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> +	regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
> +	writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> +
> +	return 0;
> +}
> +
> +static int gti_wdt_stop(struct watchdog_device *wdev)
> +{
> +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	u64 regval;
> +
> +	/* Disable Interrupt */
> +	writeq(GTI_CWD_INT_ENA_CLR_VAL(priv->wdt_timer_idx),
> +	       priv->base + GTI_CWD_INT_ENA_CLR);
> +
> +	/* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */
> +	regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> +	regval &= ~GTI_CWD_WDOG_MODE_MASK;
> +	writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> +
> +	return 0;
> +}
> +
> +static int gti_wdt_settimeout(struct watchdog_device *wdev,
> +					unsigned int timeout)
> +{
> +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	u64 timeout_wdog, regval;
> +
> +	/* Update new timeout */
> +	wdev->timeout = timeout;
> +
> +	/* Pretimeout is 1/3 of timeout */
> +	wdev->pretimeout = timeout / 3;
> +
> +	/* Get clock cycles from pretimeout */
> +	timeout_wdog = (u64)priv->clock_freq * wdev->pretimeout;
> +
> +	/* Watchdog counts in 1024 cycle steps */
> +	timeout_wdog = timeout_wdog >> 10;
> +
> +	/* GTI_CWD_WDOG.CNT: reload counter is 16-bit */
> +	timeout_wdog = (timeout_wdog + 0xff) >> 8;
> +	if (timeout_wdog >= 0x10000)
> +		timeout_wdog = 0xffff;
> +
> +	/*
> +	 * GTI_CWD_WDOG.LEN is 24bit, lower 8-bits should be zero and
> +	 * upper 16-bits are same as GTI_CWD_WDOG.CNT
> +	 */
> +	regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> +	regval &= GTI_CWD_WDOG_MODE_MASK;
> +	regval |= (timeout_wdog << (GTI_CWD_WDOG_CNT_SHIFT + 8)) |
> +		   (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT);
> +	writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> +
> +	return 0;
> +}
> +
> +static int gti_wdt_set_pretimeout(struct watchdog_device *wdev,
> +					unsigned int timeout)
> +{
> +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> +	struct watchdog_device *wdog_dev = &priv->wdev;
> +
> +	/* pretimeout should 1/3 of max_timeout */
> +	if (timeout * 3 <= wdog_dev->max_timeout)
> +		return gti_wdt_settimeout(wdev, timeout * 3);
> +
> +	return -EINVAL;
> +}
> +
> +static void gti_clk_disable_unprepare(void *data)
> +{
> +	clk_disable_unprepare(data);
> +}
> +
> +static int gti_wdt_get_cntfrq(struct platform_device *pdev,
> +			      struct gti_wdt_priv *priv)
> +{
> +	int err;
> +
> +	priv->sclk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(priv->sclk))
> +		return PTR_ERR(priv->sclk);
> +
> +	err = clk_prepare_enable(priv->sclk);
> +	if (err)
> +		return err;
> +

Any reason for not using devm_clk_get_enabled() ?

> +	err = devm_add_action_or_reset(&pdev->dev,
> +				       gti_clk_disable_unprepare, priv->sclk);
> +	if (err)
> +		return err;
> +
> +	priv->clock_freq = clk_get_rate(priv->sclk);
> +	if (!priv->clock_freq)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static const struct watchdog_info gti_wdt_ident = {
> +	.identity = "Marvell GTI watchdog",
> +	.options = WDIOF_SETTIMEOUT | WDIOF_PRETIMEOUT | WDIOF_KEEPALIVEPING |
> +		   WDIOF_MAGICCLOSE | WDIOF_CARDRESET,
> +};
> +
> +static const struct watchdog_ops gti_wdt_ops = {
> +	.owner = THIS_MODULE,
> +	.start = gti_wdt_start,
> +	.stop = gti_wdt_stop,
> +	.ping = gti_wdt_ping,
> +	.set_timeout = gti_wdt_settimeout,
> +	.set_pretimeout = gti_wdt_set_pretimeout,
> +};
> +
> +static int gti_wdt_probe(struct platform_device *pdev)
> +{
> +	struct gti_wdt_priv *priv;
> +	struct device *dev = &pdev->dev;
> +	struct watchdog_device *wdog_dev;
> +	u64 max_pretimeout;
> +	u32 wdt_idx;
> +	int irq;
> +	int err;
> +
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(priv->base))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->base),
> +			      "reg property not valid/found\n");
> +
> +	err = gti_wdt_get_cntfrq(pdev, priv);
> +	if (err)
> +		return dev_err_probe(&pdev->dev, err,
> +				     "GTI clock frequency not valid/found");
> +
> +	priv->data = of_device_get_match_data(dev);
> +
> +	/* default use last timer for watchdog */
> +	priv->wdt_timer_idx = priv->data->gti_num_timers - 1;
> +
> +	err = of_property_read_u32(dev->of_node, "marvell,wdt-timer-index",
> +				   &wdt_idx);
> +	if (!err) {
> +		if (wdt_idx >= priv->data->gti_num_timers)
> +			return dev_err_probe(&pdev->dev, err,
> +				"GTI wdog timer index not valid");
> +
> +		priv->wdt_timer_idx = wdt_idx;
> +	}
> +
> +	wdog_dev = &priv->wdev;
> +	wdog_dev->info = &gti_wdt_ident,
> +	wdog_dev->ops = &gti_wdt_ops,
> +	wdog_dev->parent = dev;
> +	/*
> +	 * Watchdog counter is 24 bit where lower 8 bits are zeros
> +	 * This counter decrements every 1024 clock cycles.
> +	 */
> +	max_pretimeout = (GTI_CWD_WDOG_CNT_MASK >> GTI_CWD_WDOG_CNT_SHIFT);
> +	max_pretimeout &= ~0xFFUL;
> +	max_pretimeout = (max_pretimeout * 1024) / priv->clock_freq;
> +	wdog_dev->pretimeout = max_pretimeout;
> +
> +	/* Maximum timeout is 3 times the pretimeout */
> +	wdog_dev->max_timeout = max_pretimeout * 3;
> +	/* Minimum first timeout (pretimeout) is 1, so min_timeout as 3 */
> +	wdog_dev->min_timeout = 3;
> +	wdog_dev->timeout = wdog_dev->pretimeout;
> +
> +	watchdog_set_drvdata(wdog_dev, priv);
> +	platform_set_drvdata(pdev, priv);
> +	gti_wdt_settimeout(wdog_dev, wdog_dev->timeout);
> +	watchdog_stop_on_reboot(wdog_dev);
> +	watchdog_stop_on_unregister(wdog_dev);
> +
> +	err = devm_watchdog_register_device(dev, wdog_dev);
> +	if (err)
> +		return err;
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0)
> +		return dev_err_probe(&pdev->dev, irq, "IRQ resource not found\n");
> +
> +	err = devm_request_irq(dev, irq, gti_wdt_interrupt, 0,
> +			       pdev->name, &priv->wdev);
> +	if (err)
> +		return dev_err_probe(dev, err, "Failed to register interrupt handler\n");
> +
> +	dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev->timeout);
> +	return 0;
> +}
> +
> +static const struct of_device_id gti_wdt_of_match[] = {
> +	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> +	{ .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> +	{ .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> +	{ .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
> +	{ .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
> +	{ .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, gti_wdt_of_match);
> +
> +static struct platform_driver gti_wdt_driver = {
> +	.driver = {
> +		.name = "gti-wdt",
> +		.of_match_table = gti_wdt_of_match,
> +	},
> +	.probe = gti_wdt_probe,
> +};
> +module_platform_driver(gti_wdt_driver);
> +
> +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
> +MODULE_DESCRIPTION("Marvell GTI watchdog driver");
Bharat Bhushan May 30, 2023, 4:08 a.m. UTC | #7 
Please see inline

> -----Original Message-----
> From: Guenter Roeck <groeck7@gmail.com> On Behalf Of Guenter Roeck
> Sent: Monday, May 29, 2023 7:08 PM
> To: Bharat Bhushan <bbhushan2@marvell.com>; wim@linux-watchdog.org;
> robh+dt@kernel.org; krzysztof.kozlowski+dt@linaro.org; linux-
> watchdog@vger.kernel.org; devicetree@vger.kernel.org; linux-
> kernel@vger.kernel.org; Sunil Kovvuri Goutham <sgoutham@marvell.com>
> Subject: [EXT] Re: [PATCH 2/2 v8] Watchdog: Add marvell GTI watchdog driver
> 
> External Email
> 
> ----------------------------------------------------------------------
> On 5/25/23 23:26, Bharat Bhushan wrote:
> > This patch add support for Marvell GTI watchdog.  Global timer unit
> > (GTI) support hardware watchdog timer. Software programs watchdog
> > timer to generate interrupt on first timeout, second timeout is
> > configured to be ignored and system reboots on third timeout.
> >
> > Signed-off-by: Bharat Bhushan <bbhushan2@marvell.com>
> > ---
> > v8:
> >    - Compatible names as per soc names
> >    - This driver run on ARM64 based architecture, Added 64BIT config
> >      dependency to avoid compilation error related to readq/writeq on
> >      32 as platform (Hexagon)
> >
> 
> In the future, please provide complete change logs.
> 
> >   drivers/watchdog/Kconfig           |  13 ++
> >   drivers/watchdog/Makefile          |   1 +
> >   drivers/watchdog/marvell_gti_wdt.c | 346
> +++++++++++++++++++++++++++++
> >   3 files changed, 360 insertions(+)
> >   create mode 100644 drivers/watchdog/marvell_gti_wdt.c
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index
> > f22138709bf5..bbdc20f33dbf 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -1779,6 +1779,19 @@ config OCTEON_WDT
> >   	  from the first interrupt, it is then only poked when the
> >   	  device is written.
> >
> > +config MARVELL_GTI_WDT
> > +	tristate "Marvell GTI Watchdog driver"
> > +	depends on ARCH_THUNDER || (COMPILE_TEST && 64BIT)
> > +	default y
> 
> Does this watchdog exist on all Thunder processors ?
> If not please drop.

This driver is not for all thunder but Octeon tx2 and cn10k  series of processor (which are arm64 based). 

> 
> > +	select WATCHDOG_CORE
> > +	help
> > +	 Marvell GTI hardware supports watchdog timer. First timeout
> > +	 works as watchdog pretimeout and installed interrupt handler
> > +	 will be called on first timeout. Hardware can generate interrupt
> > +	 to SCP on second timeout but it is not enabled, So second
> > +	 timeout is ignored. If device poke does not happen then system
> > +	 will reboot on third timeout.
> > +
> >   config BCM2835_WDT
> >   	tristate "Broadcom BCM2835 hardware watchdog"
> >   	depends on ARCH_BCM2835 || (OF && COMPILE_TEST) diff --git
> > a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index
> > b4c4ccf2d703..a164cd161ef3 100644
> > --- a/drivers/watchdog/Makefile
> > +++ b/drivers/watchdog/Makefile
> > @@ -98,6 +98,7 @@ obj-$(CONFIG_VISCONTI_WATCHDOG) += visconti_wdt.o
> >   obj-$(CONFIG_MSC313E_WATCHDOG) += msc313e_wdt.o
> >   obj-$(CONFIG_APPLE_WATCHDOG) += apple_wdt.o
> >   obj-$(CONFIG_SUNPLUS_WATCHDOG) += sunplus_wdt.o
> > +obj-$(CONFIG_MARVELL_GTI_WDT) += marvell_gti_wdt.o
> >
> >   # X86 (i386 + ia64 + x86_64) Architecture
> >   obj-$(CONFIG_ACQUIRE_WDT) += acquirewdt.o diff --git
> > a/drivers/watchdog/marvell_gti_wdt.c
> > b/drivers/watchdog/marvell_gti_wdt.c
> > new file mode 100644
> > index 000000000000..976bb9306115
> > --- /dev/null
> > +++ b/drivers/watchdog/marvell_gti_wdt.c
> > @@ -0,0 +1,346 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell GTI Watchdog driver
> > + *
> > + * Copyright (C) 2023 Marvell.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/clk.h>
> > +
> Alphabetic order, please. Also, include io.h since you are using io functions
> (writeq).

Ok,

> 
> > +/*
> > + * Hardware supports following mode of operation:
> > + * 1) Interrupt Only:
> > + *    This will generate the interrupt to arm core whenever timeout happens.
> > + *
> > + * 2) Interrupt + del3t (Interrupt to firmware (SCP processor)).
> > + *    This will generate interrupt to arm core on 1st timeout happens
> > + *    This will generate interrupt to SCP processor on 2nd timeout happens
> > + *
> > + * 3) Interrupt + Interrupt to SCP processor (called delt3t) + reboot.
> > + *    This will generate interrupt to arm core on 1st timeout happens
> > + *    Will generate interrupt to SCP processor on 2nd timeout happens,
> > + *    if interrupt is configured.
> > + *    Reboot on 3rd timeout.
> > + *
> > + * Driver will use hardware in mode-3 above so that system can reboot
> > +in case
> > + * a hardware hang. Also h/w is configured not to generate SCP
> > +interrupt, so
> > + * effectively 2nd timeout is ignored within hardware.
> > + *
> > + * First timeout is effectively watchdog pretimeout.
> > + */
> > +
> > +/* GTI CWD Watchdog (GTI_CWD_WDOG) Register */
> > +#define GTI_CWD_WDOG(reg_offset)	(0x8 * reg_offset)
> 
> reg_offset in ()

Ok.

> 
> > +#define GTI_CWD_WDOG_MODE_INT_DEL3T_RST	0x3
> > +#define GTI_CWD_WDOG_MODE_MASK		GENMASK_ULL(1, 0)
> > +#define GTI_CWD_WDOG_LEN_SHIFT		4
> > +#define GTI_CWD_WDOG_LEN_MASK		GENMASK_ULL(19, 4)
> > +#define GTI_CWD_WDOG_CNT_SHIFT		20
> > +#define GTI_CWD_WDOG_CNT_MASK		GENMASK_ULL(43, 20)
> > +
> > +/* GTI CWD Watchdog Interrupt (GTI_CWD_INT) Register */
> > +#define GTI_CWD_INT			0x200
> > +#define GTI_CWD_INT_PENDING_STATUS(bit)	(1 << bit)
> > +
> > +/* GTI CWD Watchdog Interrupt Enable Clear (GTI_CWD_INT_ENA_CLR)
> Register */
> > +#define GTI_CWD_INT_ENA_CLR		0x210
> > +#define GTI_CWD_INT_ENA_CLR_VAL(bit)	(1 << bit)
> 
> Please use BIT().

Okay,

> 
> > +
> > +/* GTI CWD Watchdog Interrupt Enable Set (GTI_CWD_INT_ENA_SET)
> Register */
> > +#define GTI_CWD_INT_ENA_SET		0x218
> > +#define GTI_CWD_INT_ENA_SET_VAL(bit)	(1 << bit)
> > +
> > +/* GTI CWD Watchdog Poke (GTI_CWD_POKE) Registers */
> > +#define GTI_CWD_POKE(reg_offset)	(0x10000 + 0x8 * reg_offset)
> > +#define GTI_CWD_POKE_VAL		1
> > +
> > +struct gti_match_data {
> > +	u32 gti_num_timers;
> > +};
> > +
> > +static const struct gti_match_data match_data_octeontx2 = {
> 
> The driver depends on ARCH_THUNDER but not ARCH_THUNDER2.

ARCH_THUNDER2 if different series.

> 
> > +	.gti_num_timers = 54,
> > +};
> > +
> > +static const struct gti_match_data match_data_cn10k = {
> > +	.gti_num_timers = 64,
> > +};
> > +
> > +struct gti_wdt_priv {
> > +	struct watchdog_device wdev;
> > +	void __iomem *base;
> > +	u32 clock_freq;
> > +	struct clk *sclk;
> > +	/* wdt_timer_idx used for timer to be used for system watchdog */
> > +	u32 wdt_timer_idx;
> > +	const struct gti_match_data *data;
> > +};
> > +
> > +static irqreturn_t gti_wdt_interrupt(int irq, void *data) {
> > +	struct watchdog_device *wdev = data;
> > +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	/* Clear Interrupt Pending Status */
> > +	writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
> > +	       priv->base + GTI_CWD_INT);
> > +
> > +	watchdog_notify_pretimeout(wdev);
> > +
> > +	return IRQ_HANDLED;
> > +}
> > +
> > +static int gti_wdt_ping(struct watchdog_device *wdev) {
> > +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +
> > +	writeq(GTI_CWD_POKE_VAL,
> > +	       priv->base + GTI_CWD_POKE(priv->wdt_timer_idx));
> > +
> > +	return 0;
> > +}
> > +
> > +static int gti_wdt_start(struct watchdog_device *wdev) {
> > +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	u64 regval;
> > +
> > +	if (!wdev->pretimeout)
> > +		return -EINVAL;
> > +
> > +	set_bit(WDOG_HW_RUNNING, &wdev->status);
> > +
> > +	/* Clear any pending interrupt */
> > +	writeq(GTI_CWD_INT_PENDING_STATUS(priv->wdt_timer_idx),
> > +	       priv->base + GTI_CWD_INT);
> > +
> > +	/* Enable Interrupt */
> > +	writeq(GTI_CWD_INT_ENA_SET_VAL(priv->wdt_timer_idx),
> > +	       priv->base + GTI_CWD_INT_ENA_SET);
> > +
> > +	/* Set (Interrupt + SCP interrupt (DEL3T) + core domain reset) Mode */
> > +	regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> > +	regval |= GTI_CWD_WDOG_MODE_INT_DEL3T_RST;
> > +	writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> > +
> > +	return 0;
> > +}
> > +
> > +static int gti_wdt_stop(struct watchdog_device *wdev) {
> > +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	u64 regval;
> > +
> > +	/* Disable Interrupt */
> > +	writeq(GTI_CWD_INT_ENA_CLR_VAL(priv->wdt_timer_idx),
> > +	       priv->base + GTI_CWD_INT_ENA_CLR);
> > +
> > +	/* Set GTI_CWD_WDOG.Mode = 0 to stop the timer */
> > +	regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> > +	regval &= ~GTI_CWD_WDOG_MODE_MASK;
> > +	writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> > +
> > +	return 0;
> > +}
> > +
> > +static int gti_wdt_settimeout(struct watchdog_device *wdev,
> > +					unsigned int timeout)
> > +{
> > +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	u64 timeout_wdog, regval;
> > +
> > +	/* Update new timeout */
> > +	wdev->timeout = timeout;
> > +
> > +	/* Pretimeout is 1/3 of timeout */
> > +	wdev->pretimeout = timeout / 3;
> > +
> > +	/* Get clock cycles from pretimeout */
> > +	timeout_wdog = (u64)priv->clock_freq * wdev->pretimeout;
> > +
> > +	/* Watchdog counts in 1024 cycle steps */
> > +	timeout_wdog = timeout_wdog >> 10;
> > +
> > +	/* GTI_CWD_WDOG.CNT: reload counter is 16-bit */
> > +	timeout_wdog = (timeout_wdog + 0xff) >> 8;
> > +	if (timeout_wdog >= 0x10000)
> > +		timeout_wdog = 0xffff;
> > +
> > +	/*
> > +	 * GTI_CWD_WDOG.LEN is 24bit, lower 8-bits should be zero and
> > +	 * upper 16-bits are same as GTI_CWD_WDOG.CNT
> > +	 */
> > +	regval = readq(priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> > +	regval &= GTI_CWD_WDOG_MODE_MASK;
> > +	regval |= (timeout_wdog << (GTI_CWD_WDOG_CNT_SHIFT + 8)) |
> > +		   (timeout_wdog << GTI_CWD_WDOG_LEN_SHIFT);
> > +	writeq(regval, priv->base + GTI_CWD_WDOG(priv->wdt_timer_idx));
> > +
> > +	return 0;
> > +}
> > +
> > +static int gti_wdt_set_pretimeout(struct watchdog_device *wdev,
> > +					unsigned int timeout)
> > +{
> > +	struct gti_wdt_priv *priv = watchdog_get_drvdata(wdev);
> > +	struct watchdog_device *wdog_dev = &priv->wdev;
> > +
> > +	/* pretimeout should 1/3 of max_timeout */
> > +	if (timeout * 3 <= wdog_dev->max_timeout)
> > +		return gti_wdt_settimeout(wdev, timeout * 3);
> > +
> > +	return -EINVAL;
> > +}
> > +
> > +static void gti_clk_disable_unprepare(void *data) {
> > +	clk_disable_unprepare(data);
> > +}
> > +
> > +static int gti_wdt_get_cntfrq(struct platform_device *pdev,
> > +			      struct gti_wdt_priv *priv)
> > +{
> > +	int err;
> > +
> > +	priv->sclk = devm_clk_get(&pdev->dev, NULL);
> > +	if (IS_ERR(priv->sclk))
> > +		return PTR_ERR(priv->sclk);
> > +
> > +	err = clk_prepare_enable(priv->sclk);
> > +	if (err)
> > +		return err;
> > +
> 
> Any reason for not using devm_clk_get_enabled() ?

No reason, just missed it.

Thanks
-Bharat

> 
> > +	err = devm_add_action_or_reset(&pdev->dev,
> > +				       gti_clk_disable_unprepare, priv->sclk);
> > +	if (err)
> > +		return err;
> > +
> > +	priv->clock_freq = clk_get_rate(priv->sclk);
> > +	if (!priv->clock_freq)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static const struct watchdog_info gti_wdt_ident = {
> > +	.identity = "Marvell GTI watchdog",
> > +	.options = WDIOF_SETTIMEOUT | WDIOF_PRETIMEOUT |
> WDIOF_KEEPALIVEPING |
> > +		   WDIOF_MAGICCLOSE | WDIOF_CARDRESET, };
> > +
> > +static const struct watchdog_ops gti_wdt_ops = {
> > +	.owner = THIS_MODULE,
> > +	.start = gti_wdt_start,
> > +	.stop = gti_wdt_stop,
> > +	.ping = gti_wdt_ping,
> > +	.set_timeout = gti_wdt_settimeout,
> > +	.set_pretimeout = gti_wdt_set_pretimeout, };
> > +
> > +static int gti_wdt_probe(struct platform_device *pdev) {
> > +	struct gti_wdt_priv *priv;
> > +	struct device *dev = &pdev->dev;
> > +	struct watchdog_device *wdog_dev;
> > +	u64 max_pretimeout;
> > +	u32 wdt_idx;
> > +	int irq;
> > +	int err;
> > +
> > +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> > +	if (!priv)
> > +		return -ENOMEM;
> > +
> > +	priv->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(priv->base))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(priv->base),
> > +			      "reg property not valid/found\n");
> > +
> > +	err = gti_wdt_get_cntfrq(pdev, priv);
> > +	if (err)
> > +		return dev_err_probe(&pdev->dev, err,
> > +				     "GTI clock frequency not valid/found");
> > +
> > +	priv->data = of_device_get_match_data(dev);
> > +
> > +	/* default use last timer for watchdog */
> > +	priv->wdt_timer_idx = priv->data->gti_num_timers - 1;
> > +
> > +	err = of_property_read_u32(dev->of_node, "marvell,wdt-timer-index",
> > +				   &wdt_idx);
> > +	if (!err) {
> > +		if (wdt_idx >= priv->data->gti_num_timers)
> > +			return dev_err_probe(&pdev->dev, err,
> > +				"GTI wdog timer index not valid");
> > +
> > +		priv->wdt_timer_idx = wdt_idx;
> > +	}
> > +
> > +	wdog_dev = &priv->wdev;
> > +	wdog_dev->info = &gti_wdt_ident,
> > +	wdog_dev->ops = &gti_wdt_ops,
> > +	wdog_dev->parent = dev;
> > +	/*
> > +	 * Watchdog counter is 24 bit where lower 8 bits are zeros
> > +	 * This counter decrements every 1024 clock cycles.
> > +	 */
> > +	max_pretimeout = (GTI_CWD_WDOG_CNT_MASK >>
> GTI_CWD_WDOG_CNT_SHIFT);
> > +	max_pretimeout &= ~0xFFUL;
> > +	max_pretimeout = (max_pretimeout * 1024) / priv->clock_freq;
> > +	wdog_dev->pretimeout = max_pretimeout;
> > +
> > +	/* Maximum timeout is 3 times the pretimeout */
> > +	wdog_dev->max_timeout = max_pretimeout * 3;
> > +	/* Minimum first timeout (pretimeout) is 1, so min_timeout as 3 */
> > +	wdog_dev->min_timeout = 3;
> > +	wdog_dev->timeout = wdog_dev->pretimeout;
> > +
> > +	watchdog_set_drvdata(wdog_dev, priv);
> > +	platform_set_drvdata(pdev, priv);
> > +	gti_wdt_settimeout(wdog_dev, wdog_dev->timeout);
> > +	watchdog_stop_on_reboot(wdog_dev);
> > +	watchdog_stop_on_unregister(wdog_dev);
> > +
> > +	err = devm_watchdog_register_device(dev, wdog_dev);
> > +	if (err)
> > +		return err;
> > +
> > +	irq = platform_get_irq(pdev, 0);
> > +	if (irq < 0)
> > +		return dev_err_probe(&pdev->dev, irq, "IRQ resource not
> found\n");
> > +
> > +	err = devm_request_irq(dev, irq, gti_wdt_interrupt, 0,
> > +			       pdev->name, &priv->wdev);
> > +	if (err)
> > +		return dev_err_probe(dev, err, "Failed to register interrupt
> > +handler\n");
> > +
> > +	dev_info(dev, "Watchdog enabled (timeout=%d sec)\n", wdog_dev-
> >timeout);
> > +	return 0;
> > +}
> > +
> > +static const struct of_device_id gti_wdt_of_match[] = {
> > +	{ .compatible = "marvell,cn9670-wdt", .data = &match_data_octeontx2},
> > +	{ .compatible = "marvell,cn9880-wdt", .data = &match_data_octeontx2},
> > +	{ .compatible = "marvell,cnf9535-wdt", .data = &match_data_octeontx2},
> > +	{ .compatible = "marvell,cn10624-wdt", .data = &match_data_cn10k},
> > +	{ .compatible = "marvell,cn10308-wdt", .data = &match_data_cn10k},
> > +	{ .compatible = "marvell,cnf10518-wdt", .data = &match_data_cn10k},
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(of, gti_wdt_of_match);
> > +
> > +static struct platform_driver gti_wdt_driver = {
> > +	.driver = {
> > +		.name = "gti-wdt",
> > +		.of_match_table = gti_wdt_of_match,
> > +	},
> > +	.probe = gti_wdt_probe,
> > +};
> > +module_platform_driver(gti_wdt_driver);
> > +
> > +MODULE_AUTHOR("Bharat Bhushan <bbhushan2@marvell.com>");
> > +MODULE_DESCRIPTION("Marvell GTI watchdog driver");
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
new file mode 100644
index 000000000000..3c642359d960
--- /dev/null
+++ b/Documentation/devicetree/bindings/watchdog/marvell,octeontx2-wdt.yaml
@@ -0,0 +1,73 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/watchdog/marvell,octeontx2-wdt.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell Global Timer (GTI) system watchdog
+
+allOf:
+  - $ref: watchdog.yaml#
+
+maintainers:
+  - Bharat Bhushan <bbhushan2@marvell.com>
+
+properties:
+  compatible:
+    enum:
+      - marvell,cn9670-wdt
+      - marvell,cn9880-wdt
+      - marvell,cnf9535-wdt
+      - marvell,cn10624-wdt
+      - marvell,cn10308-wdt
+      - marvell,cnf10518-wdt
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    minItems: 1
+
+  clock-names:
+    minItems: 1
+
+  marvell,wdt-timer-index:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    minimum: 0
+    maximum: 63
+    description:
+      An SoC have many timers (up to 64), firmware can reserve one or more timer
+      for some other use case and configures one of the global timer as watchdog
+      timer. Firmware will update this field with the timer number configured
+      as watchdog timer.
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        watchdog@802000040000 {
+            compatible = "marvell,cn9670-wdt";
+            reg = <0x00008020 0x00040000 0x00000000 0x00020000>;
+            interrupts = <GIC_SPI 38 IRQ_TYPE_EDGE_RISING>;
+            clocks = <&sclk>;
+            clock-names = "ref_clk";
+            marvell,wdt-timer-index = <63>;
+        };
+    };
+
+...