From mboxrd@z Thu Jan 1 00:00:00 1970 From: Krzysztof Kozlowski Subject: Re: [RFC PATCH] watchdog: s3c2410_wdt: Add max and min timeout values Date: Thu, 03 Mar 2016 11:30:55 +0900 Message-ID: <56D7A1DF.9030805@samsung.com> References: <1456850717-3670-1-git-send-email-javier@osg.samsung.com> <56D62979.5040009@samsung.com> <56D7233B.50907@osg.samsung.com> <56D7838D.4060602@samsung.com> <56D79E1D.3030302@osg.samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: In-reply-to: <56D79E1D.3030302-JPH+aEBZ4P+UEJcrhfAQsw@public.gmane.org> Sender: linux-watchdog-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Javier Martinez Canillas , linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org Cc: Guenter Roeck , Kukjin Kim , Wim Van Sebroeck , linux-samsung-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org, linux-watchdog-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Chanwoo Choi List-Id: linux-samsung-soc@vger.kernel.org On 03.03.2016 11:14, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 03/02/2016 09:21 PM, Krzysztof Kozlowski wrote: >> On 03.03.2016 02:30, Javier Martinez Canillas wrote: > > [snip] > >>>>> >>>>> + wdt->wdt_device.min_timeout = 1; >>>>> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); >>>> >>>> Can the frequency of clock change? E.g. with devfreq? No problem if it >>>> goes lower but if it gets higher than initial, then the problem will >>>> appear again. >>>> > > I think both cases are problematic since low scaling will meant that the > watchdog will support a bigger timeout than what was set as maximum (this > will be a regression) and going up will mean that the maximum timeout is > bigger than what the watchdog supports (the same issue without this patch). Yes, both cases are bad. >> The problem will be more severe if the watchdog got configured on 50 MHz >> and then devfreq bumps the clock to 100 MHz... >> > > So, what do you propose? We could for example set a maximum timeout on > probe > as $SUBJECT do and also update the maximum timeout again on the > .set_timeout > callback in case the clock rate changed. Or print warning and don't care... :) > I think that is kind of hacky > but I > can't think of another way to guard about the frequency being changed. First of all your patch does not introduce any kind of regression on its own. Since we are at the topic of watchdog I am just looking for doing this properly. We can merge the patches now and fix stuff later. Second, I think there won't be any issues with current mainline code (and devfreq). I don't care about out of tree code. Third, it would be good to confirm that watchdog clock really changes frequency with devfreq... BR, KRzysztof -- To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mailout3.w1.samsung.com ([210.118.77.13]:47816 "EHLO mailout3.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754801AbcCCCbF (ORCPT ); Wed, 2 Mar 2016 21:31:05 -0500 Subject: Re: [RFC PATCH] watchdog: s3c2410_wdt: Add max and min timeout values To: Javier Martinez Canillas , linux-kernel@vger.kernel.org References: <1456850717-3670-1-git-send-email-javier@osg.samsung.com> <56D62979.5040009@samsung.com> <56D7233B.50907@osg.samsung.com> <56D7838D.4060602@samsung.com> <56D79E1D.3030302@osg.samsung.com> Cc: Guenter Roeck , Kukjin Kim , Wim Van Sebroeck , linux-samsung-soc@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-watchdog@vger.kernel.org, Chanwoo Choi From: Krzysztof Kozlowski Message-id: <56D7A1DF.9030805@samsung.com> Date: Thu, 03 Mar 2016 11:30:55 +0900 MIME-version: 1.0 In-reply-to: <56D79E1D.3030302@osg.samsung.com> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org On 03.03.2016 11:14, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 03/02/2016 09:21 PM, Krzysztof Kozlowski wrote: >> On 03.03.2016 02:30, Javier Martinez Canillas wrote: > > [snip] > >>>>> >>>>> + wdt->wdt_device.min_timeout = 1; >>>>> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); >>>> >>>> Can the frequency of clock change? E.g. with devfreq? No problem if it >>>> goes lower but if it gets higher than initial, then the problem will >>>> appear again. >>>> > > I think both cases are problematic since low scaling will meant that the > watchdog will support a bigger timeout than what was set as maximum (this > will be a regression) and going up will mean that the maximum timeout is > bigger than what the watchdog supports (the same issue without this patch). Yes, both cases are bad. >> The problem will be more severe if the watchdog got configured on 50 MHz >> and then devfreq bumps the clock to 100 MHz... >> > > So, what do you propose? We could for example set a maximum timeout on > probe > as $SUBJECT do and also update the maximum timeout again on the > .set_timeout > callback in case the clock rate changed. Or print warning and don't care... :) > I think that is kind of hacky > but I > can't think of another way to guard about the frequency being changed. First of all your patch does not introduce any kind of regression on its own. Since we are at the topic of watchdog I am just looking for doing this properly. We can merge the patches now and fix stuff later. Second, I think there won't be any issues with current mainline code (and devfreq). I don't care about out of tree code. Third, it would be good to confirm that watchdog clock really changes frequency with devfreq... BR, KRzysztof From mboxrd@z Thu Jan 1 00:00:00 1970 From: k.kozlowski@samsung.com (Krzysztof Kozlowski) Date: Thu, 03 Mar 2016 11:30:55 +0900 Subject: [RFC PATCH] watchdog: s3c2410_wdt: Add max and min timeout values In-Reply-To: <56D79E1D.3030302@osg.samsung.com> References: <1456850717-3670-1-git-send-email-javier@osg.samsung.com> <56D62979.5040009@samsung.com> <56D7233B.50907@osg.samsung.com> <56D7838D.4060602@samsung.com> <56D79E1D.3030302@osg.samsung.com> Message-ID: <56D7A1DF.9030805@samsung.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 03.03.2016 11:14, Javier Martinez Canillas wrote: > Hello Krzysztof, > > On 03/02/2016 09:21 PM, Krzysztof Kozlowski wrote: >> On 03.03.2016 02:30, Javier Martinez Canillas wrote: > > [snip] > >>>>> >>>>> + wdt->wdt_device.min_timeout = 1; >>>>> + wdt->wdt_device.max_timeout = s3c2410wdt_max_timeout(wdt->clock); >>>> >>>> Can the frequency of clock change? E.g. with devfreq? No problem if it >>>> goes lower but if it gets higher than initial, then the problem will >>>> appear again. >>>> > > I think both cases are problematic since low scaling will meant that the > watchdog will support a bigger timeout than what was set as maximum (this > will be a regression) and going up will mean that the maximum timeout is > bigger than what the watchdog supports (the same issue without this patch). Yes, both cases are bad. >> The problem will be more severe if the watchdog got configured on 50 MHz >> and then devfreq bumps the clock to 100 MHz... >> > > So, what do you propose? We could for example set a maximum timeout on > probe > as $SUBJECT do and also update the maximum timeout again on the > .set_timeout > callback in case the clock rate changed. Or print warning and don't care... :) > I think that is kind of hacky > but I > can't think of another way to guard about the frequency being changed. First of all your patch does not introduce any kind of regression on its own. Since we are at the topic of watchdog I am just looking for doing this properly. We can merge the patches now and fix stuff later. Second, I think there won't be any issues with current mainline code (and devfreq). I don't care about out of tree code. Third, it would be good to confirm that watchdog clock really changes frequency with devfreq... BR, KRzysztof