Re: [v4, 2/2] drivers/watchdog: ASPEED reference dev tree properties for config

[Christopher Bostic]

On 7/8/17 9:55 AM, Guenter Roeck wrote:
> On Thu, Jul 06, 2017 at 07:49:00PM -0500, Christopher Bostic wrote:
> > Reference the system device tree when configuring the watchdog
> > engines. If property 'aspeed,reset_type' is present then set
> > reset behavior based on the specified value.  This can be one of
> > three different mutually exclusive values
> >    * cpu - Reset CPU only on watchdog timeout
> >    * soc - Reset System on Chip
> >    * system - Full system reset
> >
> > No reset can also be specified by indicating:
> >    * none - No reset, assumes another watchdog is responsible for
> >             this.
> >
> > Add optional property 'aspeed,external-signal'. If present then
> > configure to generate external signal on watchdog timeout.
> >
> > Signed-off-by: Christopher Bostic <cbostic@xxxxxxxxxxxxxxxxxx>
> > ---
> > v4 - Change the three reset type parameters to a new property
> >       'aspeed,reset_type' and check assignment for one of four
> >       different values, cpu, soc, system, none
> > v3 - Invert the logic for system reset dev tree property to
> >       preserve backwards compatibility. If not specified the
> >       default is to configure for system reset
> >     - Add check for 'aspeed,no-soc-reset' property and only if
> >       not present is SOC reset to be configured.  This preserves
> >       backwards compatibility.
> > v2 - Change of_get_property() to of_property_read_bool()
> >     - Remove redundant check for NULL struct device_node pointer
> >     - Optional property names now start with prefix 'aspeed,'
> > ---
> >   drivers/watchdog/aspeed_wdt.c | 27 ++++++++++++++++++++++-----
> >   1 file changed, 22 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/watchdog/aspeed_wdt.c b/drivers/watchdog/aspeed_wdt.c
> > index 1c65258..3ca79565 100644
> > --- a/drivers/watchdog/aspeed_wdt.c
> > +++ b/drivers/watchdog/aspeed_wdt.c
> > @@ -36,6 +36,7 @@ struct aspeed_wdt {
> >   #define WDT_CTRL		0x0C
> >   #define   WDT_CTRL_RESET_MODE_SOC	(0x00 << 5)
> >   #define   WDT_CTRL_RESET_MODE_FULL_CHIP	(0x01 << 5)
> > +#define   WDT_CTRL_RESET_MODE_ARM_CPU	(0x10 << 5)
> >   #define   WDT_CTRL_1MHZ_CLK		BIT(4)
> >   #define   WDT_CTRL_WDT_EXT		BIT(3)
> >   #define   WDT_CTRL_WDT_INTR		BIT(2)
> > @@ -140,6 +141,8 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >   {
> >   	struct aspeed_wdt *wdt;
> >   	struct resource *res;
> > +	struct device_node *np;
> > +	const char *reset_type;
> >   	int ret;
> >   
> >   	wdt = devm_kzalloc(&pdev->dev, sizeof(*wdt), GFP_KERNEL);
> > @@ -164,14 +167,28 @@ static int aspeed_wdt_probe(struct platform_device *pdev)
> >   	wdt->wdd.timeout = WDT_DEFAULT_TIMEOUT;
> >   	watchdog_init_timeout(&wdt->wdd, 0, &pdev->dev);
> >   
> > +	wdt->ctrl = WDT_CTRL_1MHZ_CLK;
> > +
> >   	/*
> >   	 * Control reset on a per-device basis to ensure the
> > -	 * host is not affected by a BMC reboot, so only reset
> > -	 * the SOC and not the full chip
> > +	 * host is not affected by a BMC reboot
> >   	 */
> > -	wdt->ctrl = WDT_CTRL_RESET_MODE_SOC |
> > -		WDT_CTRL_1MHZ_CLK |
> > -		WDT_CTRL_RESET_SYSTEM;
> > +	np = pdev->dev.of_node;
> > +	ret = of_property_read_string(np, "aspeed,reset-type", &reset_type);
> > +	if (ret) {
> > +		wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
> > +	} else {
> > +		if (!strcmp(reset_type, "cpu"))
> > +			wdt->ctrl |= WDT_CTRL_RESET_MODE_ARM_CPU;
> > +		else if (!strcmp(reset_type, "soc"))
> > +			wdt->ctrl |= WDT_CTRL_RESET_MODE_SOC;
> > +		else if (!strcmp(reset_type, "system"))
> > +			wdt->ctrl |= WDT_CTRL_RESET_SYSTEM;
> This silently accepts 'aspeed,reset-type="junk"' as "none".
> I think it would be better to explicitly check for "none".

Will add that check.
> Also, if I remember the datasheet correctly, bit 1 (WDT_CTRL_RESET_SYSTEM)
> enables the reset in general, and bit 5/6 specify the reset type.
> With that in mind, I don't think the above will work as expected.
> I think it would have to be
>
> 	WDT_CTRL_RESET_MODE_FULL_CHIP | WDT_CTRL_RESET_SYSTEM
> 	WDT_CTRL_RESET_MODE_SOC | WDT_CTRL_RESET_SYSTEM
> 	WDT_CTRL_RESET_MODE_ARM_CPU | WDT_CTRL_RESET_SYSTEM
>
> To match the original code, the default should be
>
> 	WDT_CTRL_RESET_MODE_SOC | WDT_CTRL_RESET_SYSTEM

Yes, true.  Will make the change in line above to match original code.

> > +	}
> > +	if (of_property_read_bool(np, "aspeed,external-signal"))
> > +		wdt->ctrl |= WDT_CTRL_WDT_EXT;
> > +
> No plan to support the other new configuration flags ?

No plan to support the other config flags at this time.

Thanks,
Chris
> > +	writel(wdt->ctrl, wdt->base + WDT_CTRL);
> >   
> >   	if (readl(wdt->base + WDT_CTRL) & WDT_CTRL_ENABLE)  {
> >   		aspeed_wdt_start(&wdt->wdd);