[11/11] ARM: dts: Add PCLK to the Aspeed watchdogs

Message ID 20170812184318.10144-12-linus.walleij@linaro.org
State New
Headers show
Series
  • watchdog: Consolidate FTWDT010 derivatives
Related show

Commit Message

Linus Walleij Aug. 12, 2017, 6:43 p.m.
This adds the PCLK clock to the Aspeed watchdog blocks.
I am not directly familiar with the Aspeed clocking, but
since the IP is derived from Faraday FTWDT010 it probably
has the ability to run the watchdog on the PCLK if
desired so to obtain the frequency from it, it needs to
be present in the device tree, and for completeness the
PCLK should also be referenced and enabled anyways.

Take this opportunity to add the "faraday,ftwdt010"
compatible as fallback to the watchdog IP blocks.

Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 arch/arm/boot/dts/aspeed-g4.dtsi |  7 +++++--
 arch/arm/boot/dts/aspeed-g5.dtsi | 12 +++++++++---
 2 files changed, 14 insertions(+), 5 deletions(-)

-- 
2.13.4

--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Linus Walleij Oct. 10, 2017, 8:09 p.m. | #1
On Sat, Aug 12, 2017 at 8:43 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> This adds the PCLK clock to the Aspeed watchdog blocks.

> I am not directly familiar with the Aspeed clocking, but

> since the IP is derived from Faraday FTWDT010 it probably

> has the ability to run the watchdog on the PCLK if

> desired so to obtain the frequency from it, it needs to

> be present in the device tree, and for completeness the

> PCLK should also be referenced and enabled anyways.

>

> Take this opportunity to add the "faraday,ftwdt010"

> compatible as fallback to the watchdog IP blocks.

>

> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>


Joel could you merge this through the Aspeed tree? I think
the compatible string is completely uncontroversial
(binding ACKed) to add and all should just work fine so we
can slap in "EXTCLK" later as well.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Jeffery Oct. 11, 2017, 3:48 a.m. | #2
On Sat, 2017-08-12 at 20:43 +0200, Linus Walleij wrote:
> This adds the PCLK clock to the Aspeed watchdog blocks.

> I am not directly familiar with the Aspeed clocking, but

> since the IP is derived from Faraday FTWDT010 it probably

> has the ability to run the watchdog on the PCLK if

> desired


This is true for the AST2400, but not the AST2500 where the only option
is EXTCLK (1MHz).

>  so to obtain the frequency from it, it needs to

> be present in the device tree, and for completeness the

> PCLK should also be referenced and enabled anyways.

> 

> Take this opportunity to add the "faraday,ftwdt010"

> compatible as fallback to the watchdog IP blocks.

> 

> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

> ---

>  arch/arm/boot/dts/aspeed-g4.dtsi |  7 +++++--

>  arch/arm/boot/dts/aspeed-g5.dtsi | 12 +++++++++---

>  2 files changed, 14 insertions(+), 5 deletions(-)

> 

> diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi

> index 8a04c7e2d818..23b100383c15 100644

> --- a/arch/arm/boot/dts/aspeed-g4.dtsi

> +++ b/arch/arm/boot/dts/aspeed-g4.dtsi

> @@ -895,16 +895,19 @@

> >  			};

>  

> > >  			wdt1: wdt@1e785000 {

> > -				compatible = "aspeed,ast2400-wdt";

> > +				compatible = "aspeed,ast2400-wdt", "faraday,ftwdt010";

> >  				reg = <0x1e785000 0x1c>;

> >  				interrupts = <27>;

> > +				clocks = <&clk_apb>;

> > +				clock-names = "PCLK";

> >  			};

>  

> > >  			wdt2: wdt@1e785020 {

> > -				compatible = "aspeed,ast2400-wdt";

> > +				compatible = "aspeed,ast2400-wdt", "faraday,ftwdt010";

> >  				reg = <0x1e785020 0x1c>;

> >  				interrupts = <27>;

> >  				clocks = <&clk_apb>;

> > +				clock-names = "PCLK";

> >  				status = "disabled";

> >  			};

>  

> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi

> index 9cffe347b828..2322d72cd8a9 100644

> --- a/arch/arm/boot/dts/aspeed-g5.dtsi

> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi

> @@ -1003,21 +1003,27 @@

>  

>  

> > >  			wdt1: wdt@1e785000 {

> > -				compatible = "aspeed,ast2500-wdt";

> > +				compatible = "aspeed,ast2500-wdt", "faraday,ftwdt010";

> >  				reg = <0x1e785000 0x20>;

> >  				interrupts = <27>;

> > +				clocks = <&clk_apb>;

> +				clock-names = "PCLK";


Given the comment above, shouldn't we be doing something like the
following instead for each of the watchdogs?

+				faraday,use-extclk;
+				clock-names = "EXTCLK";

Andrew

>  			};

>  

> > >  			wdt2: wdt@1e785020 {

> > -				compatible = "aspeed,ast2500-wdt";

> > +				compatible = "aspeed,ast2500-wdt", "faraday,ftwdt010";

> >  				reg = <0x1e785020 0x20>;

> >  				interrupts = <27>;

> > +				clocks = <&clk_apb>;

> > +				clock-names = "PCLK";

> >  				status = "disabled";

> >  			};

>  

> > >  			wdt3: wdt@1e785040 {

> > -				compatible = "aspeed,ast2500-wdt";

> > +				compatible = "aspeed,ast2500-wdt", "faraday,ftwdt010";

> >  				reg = <0x1e785040 0x20>;

> > +				clocks = <&clk_apb>;

> > +				clock-names = "PCLK";

> >  				status = "disabled";

> >  			};

>
Linus Walleij Oct. 11, 2017, 6:32 a.m. | #3
On Wed, Oct 11, 2017 at 5:48 AM, Andrew Jeffery <andrew@aj.id.au> wrote:
> On Sat, 2017-08-12 at 20:43 +0200, Linus Walleij wrote:

>> This adds the PCLK clock to the Aspeed watchdog blocks.

>> I am not directly familiar with the Aspeed clocking, but

>> since the IP is derived from Faraday FTWDT010 it probably

>> has the ability to run the watchdog on the PCLK if

>> desired

>

> This is true for the AST2400, but not the AST2500 where the only option

> is EXTCLK (1MHz).


The IP block/cell certainly has a PCLK even if it cannot be used
to drive the watchdog timer. It is necessary to interface the
SoC interconnect.

>> > +                           clocks = <&clk_apb>;

>> +                             clock-names = "PCLK";

>

> Given the comment above, shouldn't we be doing something like the

> following instead for each of the watchdogs?

>

> +                               faraday,use-extclk;

> +                               clock-names = "EXTCLK";


So that will be added too, later, when there is a 1MHz clock
to reference in the device tree. I guess after Joel's patches.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andrew Jeffery Oct. 11, 2017, 7:14 a.m. | #4
On Wed, 2017-10-11 at 08:32 +0200, Linus Walleij wrote:
> > On Wed, Oct 11, 2017 at 5:48 AM, Andrew Jeffery <andrew@aj.id.au> wrote:

> > On Sat, 2017-08-12 at 20:43 +0200, Linus Walleij wrote:

> > > This adds the PCLK clock to the Aspeed watchdog blocks.

> > > I am not directly familiar with the Aspeed clocking, but

> > > since the IP is derived from Faraday FTWDT010 it probably

> > > has the ability to run the watchdog on the PCLK if

> > > desired

> > 

> > This is true for the AST2400, but not the AST2500 where the only option

> > is EXTCLK (1MHz).

> 

> The IP block/cell certainly has a PCLK even if it cannot be used

> to drive the watchdog timer. It is necessary to interface the

> SoC interconnect.


Hah, yep, brain-fade.

Cheers,

Andrew

> 

> > > > +                           clocks = <&clk_apb>;

> > > 

> > > +                             clock-names = "PCLK";

> > 

> > Given the comment above, shouldn't we be doing something like the

> > following instead for each of the watchdogs?

> > 

> > +                               faraday,use-extclk;

> > +                               clock-names = "EXTCLK";

> 

> So that will be added too, later, when there is a 1MHz clock

> to reference in the device tree. I guess after Joel's patches.

> 

> Yours,

> Linus Walleij
Joel Stanley Oct. 12, 2017, 3:37 a.m. | #5
On Wed, Oct 11, 2017 at 4:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Sat, Aug 12, 2017 at 8:43 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

>

>> This adds the PCLK clock to the Aspeed watchdog blocks.

>> I am not directly familiar with the Aspeed clocking, but

>> since the IP is derived from Faraday FTWDT010 it probably

>> has the ability to run the watchdog on the PCLK if

>> desired so to obtain the frequency from it, it needs to

>> be present in the device tree, and for completeness the

>> PCLK should also be referenced and enabled anyways.

>>

>> Take this opportunity to add the "faraday,ftwdt010"

>> compatible as fallback to the watchdog IP blocks.

>>

>> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

>

> Joel could you merge this through the Aspeed tree? I think

> the compatible string is completely uncontroversial

> (binding ACKed) to add and all should just work fine so we

> can slap in "EXTCLK" later as well.


How do the clock-names work? I have been writing the aspeed clk driver
and updating the bindings without clock names, and instead using a
identifier in phandle to reference which clock the device wants.

eg:

 clocks = <&syscon 10>;

(I'm travelling over the next few weeks so my replies might be delayed)

Cheers,

Joel
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Oct. 12, 2017, 7:35 a.m. | #6
On Thu, Oct 12, 2017 at 5:37 AM, Joel Stanley <joel@jms.id.au> wrote:
> On Wed, Oct 11, 2017 at 4:09 AM, Linus Walleij <linus.walleij@linaro.org> wrote:


>> Joel could you merge this through the Aspeed tree? I think

>> the compatible string is completely uncontroversial

>> (binding ACKed) to add and all should just work fine so we

>> can slap in "EXTCLK" later as well.

>

> How do the clock-names work? I have been writing the aspeed clk driver

> and updating the bindings without clock names, and instead using a

> identifier in phandle to reference which clock the device wants.

>

> eg:

>

>  clocks = <&syscon 10>;


This works fine as long as there is just one clock.

clocks = <&syscon 10>, <&syscon 11>, <&syscon 12>;

becomes a problem, right?

clk_get() has this signature:
struct clk *clk_get(struct device *dev, const char *id);

So clk_get(dev, NULL); will return <&syscon 10>;

How to get the rest?

clocks = <&syscon 10>, <&syscon 11>, <&syscon 12>;
clock-names = "FOO", "BAR", "BAZ";

clk_get(dev, "BAR");

gets the second clock.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/arch/arm/boot/dts/aspeed-g4.dtsi b/arch/arm/boot/dts/aspeed-g4.dtsi
index 8a04c7e2d818..23b100383c15 100644
--- a/arch/arm/boot/dts/aspeed-g4.dtsi
+++ b/arch/arm/boot/dts/aspeed-g4.dtsi
@@ -895,16 +895,19 @@ 
 			};
 
 			wdt1: wdt@1e785000 {
-				compatible = "aspeed,ast2400-wdt";
+				compatible = "aspeed,ast2400-wdt", "faraday,ftwdt010";
 				reg = <0x1e785000 0x1c>;
 				interrupts = <27>;
+				clocks = <&clk_apb>;
+				clock-names = "PCLK";
 			};
 
 			wdt2: wdt@1e785020 {
-				compatible = "aspeed,ast2400-wdt";
+				compatible = "aspeed,ast2400-wdt", "faraday,ftwdt010";
 				reg = <0x1e785020 0x1c>;
 				interrupts = <27>;
 				clocks = <&clk_apb>;
+				clock-names = "PCLK";
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index 9cffe347b828..2322d72cd8a9 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -1003,21 +1003,27 @@ 
 
 
 			wdt1: wdt@1e785000 {
-				compatible = "aspeed,ast2500-wdt";
+				compatible = "aspeed,ast2500-wdt", "faraday,ftwdt010";
 				reg = <0x1e785000 0x20>;
 				interrupts = <27>;
+				clocks = <&clk_apb>;
+				clock-names = "PCLK";
 			};
 
 			wdt2: wdt@1e785020 {
-				compatible = "aspeed,ast2500-wdt";
+				compatible = "aspeed,ast2500-wdt", "faraday,ftwdt010";
 				reg = <0x1e785020 0x20>;
 				interrupts = <27>;
+				clocks = <&clk_apb>;
+				clock-names = "PCLK";
 				status = "disabled";
 			};
 
 			wdt3: wdt@1e785040 {
-				compatible = "aspeed,ast2500-wdt";
+				compatible = "aspeed,ast2500-wdt", "faraday,ftwdt010";
 				reg = <0x1e785040 0x20>;
+				clocks = <&clk_apb>;
+				clock-names = "PCLK";
 				status = "disabled";
 			};