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

[Christopher Bostic <cbostic@linux.vnet.ibm.com>]

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@linux.vnet.ibm.com>
>> ---
>> 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);