[PATCH linux dev-4.10 v2 0/4] Witherspoon WDTRST1 enablement

[PATCH linux dev-4.10 v2 0/4] Witherspoon WDTRST1 enablement

[Andrew Jeffery]

Hello,

Earlier I sent a patch fixing the wdt1 pinctrl bindings so that we request the
WDTRST1 function:

  https://lists.ozlabs.org/pipermail/openbmc/2017-July/008373.html

This series builds on top of that patch (and contains it, hence v2), adding
new bindings to further configure the pin's behaviour through the watchdog
controller itself. This is necessary for correct behaviour of the latch
asserting the MAX31785 FAULT pin across a BMC reset.

Cheers,

Andrew

Andrew Jeffery (4):
  ARM: aspeed: Request WDTRST1 pinctrl function
  dt-bindings: watchdog: aspeed: External reset signal properties
  watchdog: aspeed: Support external signal drive and polarity bindings
  ARM: dts: aspeed: Witherspoon WDT1 external signal is push-pull

 .../devicetree/bindings/watchdog/aspeed-wdt.txt     |  7 +++++++
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts    |  4 ++++
 drivers/watchdog/aspeed_wdt.c                       | 21 +++++++++++++++++++++
 3 files changed, 32 insertions(+)

-- 
2.11.0

[PATCH linux dev-4.10 v2 1/4] ARM: aspeed: Request WDTRST1 pinctrl function

[Andrew Jeffery]

On Witherspoon, the Aspeed SoC's WDTRST1 pin is wired to the FAULT pin
on the MAX31785 fan controller, which drives the fans to 100% PWM duty
when asserted. The pulse generated by the watchdog is latched to ensure
the fans stay at full speed across the BMC reboot. The latch is reset
when the BMC transitions through the chassis-poweron systemd target.

The SoC's WDTRST1 pinctrl function needs to be requested for
aspeed,external-signal to be effective, otherwise the pin is not
associated with the watchdog controller.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
Cc: Matt Spinler <mspinler@linux.vnet.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index d9649013ee78..c28222c17d03 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -488,6 +488,9 @@
 &wdt1 {
 	aspeed,reset-type = "none";
 	aspeed,external-signal;
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_wdtrst1_default>;
 };
 
 &wdt2 {
-- 
2.11.0

[PATCH linux dev-4.10 v2 2/4] dt-bindings: watchdog: aspeed: External reset signal properties

[Andrew Jeffery]

For the AST2500 and compatible watchdog controllers the external reset
signal can be configured for push-pull or open-drain drive types, and in
the case of open-drain driving, active low or high.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
index f526b003b2e0..fa661319a080 100644
--- a/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
+++ b/Documentation/devicetree/bindings/watchdog/aspeed-wdt.txt
@@ -41,6 +41,13 @@ Optional properties:
 			specified no external signal is sent.
  - aspeed,alt-boot:    If property is present then boot from alternate block.
 
+Optional properties for AST2500-compatible watchdogs:
+ - aspeed,ext-push-pull: If aspeed,external-signal is present, set the pin's
+			 drive type to push-pull. The default is open-drain.
+ - aspeed,ext-active-high: If aspeed,external-signal is present and and the pin
+			   is configured as open-drain, then set the pulse
+			   polarity to active-high. The default is active-low.
+
 Example:
 
 	wdt1: watchdog@1e785000 {
-- 
2.11.0

[PATCH linux dev-4.10 v2 3/4] watchdog: aspeed: Support external signal drive and polarity bindings

[Andrew Jeffery]

These bindings are optional, and only relevant on the AST2500 and
compatible watchdog controllers.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 drivers/watchdog/aspeed_wdt.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
index d29f75e90bb6..9a07ef176054 100644
--- a/drivers/watchdog/aspeed_wdt.c
+++ b/drivers/watchdog/aspeed_wdt.c
@@ -42,6 +42,9 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
 #define   WDT_CTRL_WDT_INTR		BIT(2)
 #define   WDT_CTRL_RESET_SYSTEM		BIT(1)
 #define   WDT_CTRL_ENABLE		BIT(0)
+#define WDT_RESET_WIDTH		0x18
+#define   WDT_RESET_WIDTH_ACTIVE_HIGH	BIT(31)
+#define   WDT_RESET_WIDTH_PUSHPULL	BIT(30)
 
 #define WDT_RESTART_MAGIC	0x4755
 
@@ -204,6 +207,24 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
 		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
 	}
 
+	if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
+		u32 from = readl(wdt->base + WDT_RESET_WIDTH);
+		u32 to = from;
+
+		if (of_property_read_bool(np, "aspeed,ext-push-pull"))
+			to |= WDT_RESET_WIDTH_PUSHPULL;
+		else
+			to &= ~WDT_RESET_WIDTH_PUSHPULL;
+
+		if (of_property_read_bool(np, "aspeed,ext-active-high"))
+			to |= WDT_RESET_WIDTH_ACTIVE_HIGH;
+		else
+			to &= ~WDT_RESET_WIDTH_ACTIVE_HIGH;
+
+		if (from != to)
+			writel(to, wdt->base + WDT_RESET_WIDTH);
+	}
+
 	ret = watchdog_register_device(&wdt->wdd);
 	if (ret) {
 		dev_err(&pdev->dev, "failed to register\n");
-- 
2.11.0

Re: [PATCH linux dev-4.10 v2 3/4] watchdog: aspeed: Support external signal drive and polarity bindings

[Matt Spinler]

On 7/26/2017 11:08 AM, Andrew Jeffery wrote:
> These bindings are optional, and only relevant on the AST2500 and
> compatible watchdog controllers.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   drivers/watchdog/aspeed_wdt.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
>
> diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> index d29f75e90bb6..9a07ef176054 100644
> --- a/drivers/watchdog/aspeed_wdt.c
> +++ b/drivers/watchdog/aspeed_wdt.c
> @@ -42,6 +42,9 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
>   #define   WDT_CTRL_WDT_INTR		BIT(2)
>   #define   WDT_CTRL_RESET_SYSTEM		BIT(1)
>   #define   WDT_CTRL_ENABLE		BIT(0)
> +#define WDT_RESET_WIDTH		0x18
> +#define   WDT_RESET_WIDTH_ACTIVE_HIGH	BIT(31)
> +#define   WDT_RESET_WIDTH_PUSHPULL	BIT(30)
>
>   #define WDT_RESTART_MAGIC	0x4755
>
> @@ -204,6 +207,24 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
>   		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
>   	}
>
> +	if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
> +		u32 from = readl(wdt->base + WDT_RESET_WIDTH);
> +		u32 to = from;
> +
> +		if (of_property_read_bool(np, "aspeed,ext-push-pull"))
> +			to |= WDT_RESET_WIDTH_PUSHPULL;
> +		else
> +			to &= ~WDT_RESET_WIDTH_PUSHPULL;
 From the spec:

To set this bit to value ’1’, write bit[31:24] = 0xA8.
To set this bit to value ’0’, write bit[31:24] = 0x8A.

> +
> +		if (of_property_read_bool(np, "aspeed,ext-active-high"))
> +			to |= WDT_RESET_WIDTH_ACTIVE_HIGH;
> +		else
> +			to &= ~WDT_RESET_WIDTH_ACTIVE_HIGH;
> +
And again here:
To set this bit to value ’1’, write bit[31:24] = 0xA5.
To set this bit to value ’0’, write bit[31:24] = 0x5A.
For others value, this bit will keep old value without change.
> +		if (from != to)
> +			writel(to, wdt->base + WDT_RESET_WIDTH);
> +	}
> +
>   	ret = watchdog_register_device(&wdt->wdd);
>   	if (ret) {
>   		dev_err(&pdev->dev, "failed to register\n");

Re: [PATCH linux dev-4.10 v2 3/4] watchdog: aspeed: Support external signal drive and polarity bindings

[Andrew Jeffery]

On Thu, Jul 27, 2017, at 03:22, Matt Spinler wrote:
> 
> 
> On 7/26/2017 11:08 AM, Andrew Jeffery wrote:
> > These bindings are optional, and only relevant on the AST2500 and
> > compatible watchdog controllers.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >   drivers/watchdog/aspeed_wdt.c | 21 +++++++++++++++++++++
> >   1 file changed, 21 insertions(+)
> >
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index d29f75e90bb6..9a07ef176054 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -42,6 +42,9 @@ MODULE_DEVICE_TABLE(of, aspeed_wdt_of_table);
> >   #define   WDT_CTRL_WDT_INTR		BIT(2)
> >   #define   WDT_CTRL_RESET_SYSTEM		BIT(1)
> >   #define   WDT_CTRL_ENABLE		BIT(0)
> > +#define WDT_RESET_WIDTH		0x18
> > +#define   WDT_RESET_WIDTH_ACTIVE_HIGH	BIT(31)
> > +#define   WDT_RESET_WIDTH_PUSHPULL	BIT(30)
> >
> >   #define WDT_RESTART_MAGIC	0x4755
> >
> > @@ -204,6 +207,24 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >   		set_bit(WDOG_HW_RUNNING, &wdt->wdd.status);
> >   	}
> >
> > +	if (of_device_is_compatible(np, "aspeed,ast2500-wdt")) {
> > +		u32 from = readl(wdt->base + WDT_RESET_WIDTH);
> > +		u32 to = from;
> > +
> > +		if (of_property_read_bool(np, "aspeed,ext-push-pull"))
> > +			to |= WDT_RESET_WIDTH_PUSHPULL;
> > +		else
> > +			to &= ~WDT_RESET_WIDTH_PUSHPULL;
>  From the spec:
> 
> To set this bit to value ’1’, write bit[31:24] = 0xA8.
> To set this bit to value ’0’, write bit[31:24] = 0x8A.

Oof. Yeah, clearly I should have gone to bed earlier. I tested the patch
under qemu but forgot to model that behaviour.

> 
> > +
> > +		if (of_property_read_bool(np, "aspeed,ext-active-high"))
> > +			to |= WDT_RESET_WIDTH_ACTIVE_HIGH;
> > +		else
> > +			to &= ~WDT_RESET_WIDTH_ACTIVE_HIGH;
> > +
> And again here:
> To set this bit to value ’1’, write bit[31:24] = 0xA5.
> To set this bit to value ’0’, write bit[31:24] = 0x5A.
> For others value, this bit will keep old value without change.

As above.

Sorry about that.

Andrew

> > +		if (from != to)
> > +			writel(to, wdt->base + WDT_RESET_WIDTH);
> > +	}
> > +
> >   	ret = watchdog_register_device(&wdt->wdd);
> >   	if (ret) {
> >   		dev_err(&pdev->dev, "failed to register\n");
> 

[PATCH linux dev-4.10 v2 4/4] ARM: dts: aspeed: Witherspoon WDT1 external signal is push-pull

[Andrew Jeffery]

Correctly drive the latch asserting the FAULT pin on the MAX31785 when
the watchdog bites. This ensures the fan outputs are ramped to 100% PWM
duty.

Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index c28222c17d03..df36b87ba105 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -488,6 +488,7 @@
 &wdt1 {
 	aspeed,reset-type = "none";
 	aspeed,external-signal;
+	aspeed,ext-push-pull;
 
 	pinctrl-names = "default";
 	pinctrl-0 = <&pinctrl_wdtrst1_default>;
-- 
2.11.0

Re: [PATCH linux dev-4.10 v2 4/4] ARM: dts: aspeed: Witherspoon WDT1 external signal is push-pull

[Matt Spinler]

On 7/26/2017 11:08 AM, Andrew Jeffery wrote:
> Correctly drive the latch asserting the FAULT pin on the MAX31785 when
> the watchdog bites. This ensures the fan outputs are ramped to 100% PWM
> duty.
>
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> ---
>   arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> index c28222c17d03..df36b87ba105 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> @@ -488,6 +488,7 @@
>   &wdt1 {
>   	aspeed,reset-type = "none";
>   	aspeed,external-signal;
> +	aspeed,ext-push-pull;

We need active-high for sure.  It still works without push-pull, but not 
sure if that is totally electronically ideal or not as the HW designer 
requested for this to be push-pull.

>
>   	pinctrl-names = "default";
>   	pinctrl-0 = <&pinctrl_wdtrst1_default>;

Re: [PATCH linux dev-4.10 v2 4/4] ARM: dts: aspeed: Witherspoon WDT1 external signal is push-pull

[Andrew Jeffery]

On Thu, Jul 27, 2017, at 03:24, Matt Spinler wrote:
> 
> 
> On 7/26/2017 11:08 AM, Andrew Jeffery wrote:
> > Correctly drive the latch asserting the FAULT pin on the MAX31785 when
> > the watchdog bites. This ensures the fan outputs are ramped to 100% PWM
> > duty.
> >
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> > ---
> >   arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> > index c28222c17d03..df36b87ba105 100644
> > --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> > +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> > @@ -488,6 +488,7 @@
> >   &wdt1 {
> >   	aspeed,reset-type = "none";
> >   	aspeed,external-signal;
> > +	aspeed,ext-push-pull;
> 
> We need active-high for sure.  It still works without push-pull, but not 
> sure if that is totally electronically ideal or not as the HW designer 
> requested for this to be push-pull.

Right; given Jordan's statement, the wording of the documentation and
your testing the behaviour doesn't entirely make sense to me.

I might try to get it clarified with Aspeed.

Andrew

> 
> >
> >   	pinctrl-names = "default";
> >   	pinctrl-0 = <&pinctrl_wdtrst1_default>;
>