From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bartlomiej Zolnierkiewicz Subject: Re: [PATCH 3/4] watchdog: s3c2410_wdt: parse watchdog dt node to read PMU registers adresses Date: Mon, 23 Sep 2013 19:03:10 +0200 Message-ID: <1918747.quF8QJoP6D@amdc1032> References: <1379414623-26329-1-git-send-email-l.krishna@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7Bit Return-path: In-reply-to: Sender: linux-samsung-soc-owner@vger.kernel.org To: Leela Krishna Amudala Cc: Doug Anderson , Tomasz Figa , linux-samsung-soc , Wim Van Sebroeck , Kukjin Kim , devicetree@vger.kernel.org, linux-watchdog@vger.kernel.org List-Id: devicetree@vger.kernel.org Hi, On Wednesday, September 18, 2013 12:20:31 PM Leela Krishna Amudala wrote: > Tomasz, > > On Wed, Sep 18, 2013 at 10:04 AM, Doug Anderson wrote: > > Tomasz, > > > > On Tue, Sep 17, 2013 at 6:30 AM, Tomasz Figa wrote: > >>> --- a/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt > >>> +++ b/Documentation/devicetree/bindings/watchdog/samsung-wdt.txt > >>> @@ -7,8 +7,20 @@ occurred. > >>> Required properties: > >>> - compatible : should be "samsung,s3c2410-wdt" > >> > >> Since the WDT block of Exynos 5420 needs some extra configuration in PMU > >> registers, it is no longer compatible with samsung.s3c2410-wdt. Please > >> introduce separate compatible ("samsung,exynos5420-wdt") and make the > >> driver handle the additional configuration only if running on a device with > >> this compatible value. > >> > >> I'd suggest introducing quirk system to the driver and adding a > >> NEEDS_PMU_CONFIG quirk selected by DT match entry with "samsung,exynos5420- > >> wdt" compatible. > >> > >>> - reg : base physical address of the controller and length of memory > >>> mapped - region. > >>> + region and the optional (addresses and length of memory mapped regions > >>> + of) PMU registers for masking/unmasking WDT. > >>> - interrupts : interrupt number to the cpu. > >>> > >>> Optional properties: > >>> - timeout-sec : contains the watchdog timeout in seconds. > >>> +- reset-mask-bit: bit number in the PMU registers to program mask/unmask > >>> WDT. + > >> > >> I believe this is mandatory on Exynos 5420 and unused on previous SoCs. It > >> should be handled depending on compatible value. > > > > I think at least 5250 needs something similar. I believe we got away > > with it in the past since other (non-WDT) code was tweaking with this > > bit, but that was a little bit gross. Leela Krishna can correct me if > > I'm wrong. > > > > Yes, 5250 also needs this reset-mask-bit, but not required by SoCs > other than Exynos5xxx. > Hence I kept it as optional parameter. > > I took care of this code such that it won't break on older SoCs. > > If you notice the code in probe function, I used the check condition > > if (!IS_ERR(wdt->pmu_disable_reg) && !IS_ERR(wdt->pmu_mask_reset_reg)) { > } > > i.e., if any of the PMU register address is not mentioned in the DT > node it simply skips reading reset-mask-bit > and continues execution (which may happen in older SoC DT node). > > ..also, in s3c2410wdt_mask_and_disable_reset() function, below > condition check is happening > > if (IS_ERR(wdt->pmu_disable_reg) || IS_ERR(wdt->pmu_mask_reset_reg) > || (wdt->pmu_mask_bit < 0)) > return; > > i.e., if any of the registers is not specified in DT node simply > return without programming > PMU registers(which is true in case of older SoCs). > > If you think this doesn't sounds good way of handling older SoCs. > I'll add new compatible string for Exynos5xxx and do like what you said. :) Yes, please re-do the code per Tomasz's suggestions. This would also allow you to check return values of devm_ioremap_resource() calls in the probe method and require them to succeed in order to register watchdog device (which is unfortunately not what happens currently). Best regards, -- Bartlomiej Zolnierkiewicz Samsung R&D Institute Poland Samsung Electronics