lkml.org 
[lkml]   [2017]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [v4, 2/2] drivers/watchdog: ASPEED reference dev tree properties for config
From
Date


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);

\
 
 \ /
  Last update: 2017-07-10 17:02    [W:0.086 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site