Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Sender: Guenter Roeck <groeck7@gmail.com>
Message-ID: <d88523a8-a240-915d-9ae7-54b2277e424a@roeck-us.net>
Date:   Tue, 3 May 2022 09:53:37 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.8.1
Content-Language: en-US
To:     "Hawkins, Nick" <nick.hawkins@hpe.com>,
        "Verdun, Jean-Marie" <verdun@hpe.com>,
        "joel@jms.id.au" <joel@jms.id.au>, "arnd@arndb.de" <arnd@arndb.de>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc:     Wim Van Sebroeck <wim@linux-watchdog.org>,
        "linux-watchdog@vger.kernel.org" <linux-watchdog@vger.kernel.org>
References: <20220502204050.88316-1-nick.hawkins@hpe.com>
 <20220502204050.88316-3-nick.hawkins@hpe.com>
 <c0262cf4-dfeb-c9a9-bcb2-24af006e6d4d@roeck-us.net>
 <PH0PR84MB1718D28F1846F54DB5766E6D88C09@PH0PR84MB1718.NAMPRD84.PROD.OUTLOOK.COM>
From:   Guenter Roeck <linux@roeck-us.net>
Subject: Re: [PATCH v6 3/8] watchdog: hpe-wdt: Introduce HPE GXP Watchdog
In-Reply-To: <PH0PR84MB1718D28F1846F54DB5766E6D88C09@PH0PR84MB1718.NAMPRD84.PROD.OUTLOOK.COM>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk

On 5/3/22 09:22, Hawkins, Nick wrote:
> On 5/2/22 13:40, nick.hawkins@hpe.com wrote:
>>> +#include <linux/of_address.h>
>>> +#include <linux/of_platform.h>
> 
>> Where are those of_ includes used ?
> 
> They were not used anymore with latest changes. Thank you for pointing this out. I will remember to check in the future for each new commit to double check this.
> 
>>> +#define WDT_MAX_TIMEOUT_MS	655000
> 
>> Shouldn't that be 655350 ?
> 
> Yes it should be. I will correct this.
> 
>>> +static int gxp_wdt_set_timeout(struct watchdog_device *wdd,
>>> +			       unsigned int timeout)
>>> +{
>>> +	struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
>>> +	u32 actual;
>>> +
>>> +	wdd->timeout = timeout;
>>> +	actual = min(timeout, wdd->max_hw_heartbeat_ms / 1000);
>>> +	writew(SECS_TO_WDOG_TICKS(actual), drvdata->base + GXP_WDT_CNT_OFS);
> 
>> First, the accuracy of actual is reduced to 1 second, then SECS_TO_WDOG_TICKS() multiplies the result with 100, meaning the actual accuracy is 10ms. Why not just use 10 ms ?
> 
>> 	actual = min(timeout * 100, wdd->max_hw_heartbeat_ms / 10);
>> 	writew(actual, drvdata->base + GXP_WDT_CNT_OFS);
> 
> I have replaced the mention code with what you recommended above.
> 
>>> +
>>> +static int gxp_restart(struct watchdog_device *wdd, unsigned long action,
>>> +		       void *data)
>>> +{
>>> +	struct gxp_wdt *drvdata = watchdog_get_drvdata(wdd);
>>> +
>>> +	writew(10, drvdata->base + GXP_WDT_CNT_OFS);
> 
>> Doesn't that translate to 100 ms timeout ? Why such a large reboot delay instead of writing 1 ?
> 
> This has been changed to 1.
> 
>>> +	gxp_wdt_enable_reload(drvdata);
>>> +	mdelay(100);
>>> +	return 0;
>>> +}
>>> +
>>> +static int gxp_wdt_probe(struct platform_device *pdev) {
>>> +	struct device *dev = &pdev->dev;
>>> +	struct gxp_wdt *drvdata;
>>> +	int err;
>>> +	u8 val;
>>> +
>>> +	drvdata = devm_kzalloc(dev, sizeof(struct gxp_wdt), GFP_KERNEL);
>>> +	if (!drvdata)
>>> +		return -ENOMEM;
>>> +
>>> +	drvdata->base = (void __iomem *)dev->platform_data;
> 
>> I'd personaly prefer if the address was passed as resource.
> 
> Just to clarify for my understanding are you asking that in the device structure I use the "void *platform_data" to pass "struct *resource"? If I am incorrect here can you elaborate on what you would like to be done? Based on feedback in review for the device tree; the watchdog is being created as a child to the timer. Therefore the conclusion reached was there should not be a gxp-wdt listed in the device tree files. I took this implementation based on what I found in ixp4xx_wdt.c.
> 

One bad deed tends to multiply.

No, I didn't ask to pass a struct resource as platform data.
That would be no different to the current code. Resources
can be added to a platform device using
platform_device_add_resources(), and the platform driver
can then use platform_get_resource() to use it. This
would make it independent of a "private" mechanism.

Thanks,
Guenter