Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753407AbbGaIlO (ORCPT ); Fri, 31 Jul 2015 04:41:14 -0400 Received: from mga02.intel.com ([134.134.136.20]:8386 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751526AbbGaIlI (ORCPT ); Fri, 31 Jul 2015 04:41:08 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,583,1432623600"; d="scan'208";a="775090014" Message-ID: <1438332064.29746.141.camel@linux.intel.com> Subject: Re: [PATCH v3 3/3] iTCO_wdt: Add support for TCO on Intel Sunrisepoint From: Andy Shevchenko To: Matt Fleming , Wim Van Sebroeck Cc: linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org, Mika Westerberg , Jean Delvare , Wolfram Sang , Lee Jones , Guenter Roeck , Matt Fleming Date: Fri, 31 Jul 2015 11:41:04 +0300 In-Reply-To: <1438293541-26131-4-git-send-email-matt@codeblueprint.co.uk> References: <1438293541-26131-1-git-send-email-matt@codeblueprint.co.uk> <1438293541-26131-4-git-send-email-matt@codeblueprint.co.uk> Organization: Intel Finland Oy Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.16.3-1 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5787 Lines: 211 On Thu, 2015-07-30 at 22:59 +0100, Matt Fleming wrote: > From: Matt Fleming > > The revision of the watchdog hardware in Sunrisepoint necessitates a > new > "version" inside the TCO watchdog driver because some of the register > layouts have changed. > > Also update the Kconfig entry to select both the LPC and SMBus > drivers > since the TCO device is on the SMBus in Sunrisepoint. Few minor comments below, though I'm okay with current version as well. > > Cc: Wim Van Sebroeck > Reviewed-by: Guenter Roeck > Cc: Jean Delvare > Signed-off-by: Matt Fleming > --- > > v3: > - Added Guenter's Review tag > > v2: > - Explicitly list TCO versions and their "no reboot" bits in the > switch > statement in no_reboot_bit() > - Use a switch/case statement when clearing status bits instead of > convoluted if/else branching > - Select both of the bus drivers instead of using depends > > drivers/watchdog/Kconfig | 3 ++- > drivers/watchdog/iTCO_wdt.c | 66 ++++++++++++++++++++++++++++------- > ---------- > 2 files changed, 43 insertions(+), 26 deletions(-) > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 241fafde42cb..55c4b5b0a317 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -797,7 +797,8 @@ config ITCO_WDT > tristate "Intel TCO Timer/Watchdog" > depends on (X86 || IA64) && PCI > select WATCHDOG_CORE > - select LPC_ICH > + select LPC_ICH if !EXPERT > + select I2C_I801 if !EXPERT > ---help--- > Hardware driver for the intel TCO timer based watchdog > devices. > These drivers are included in the Intel 82801 I/O > Controller > diff --git a/drivers/watchdog/iTCO_wdt.c > b/drivers/watchdog/iTCO_wdt.c > index a94401b2deca..7b8949d48029 100644 > --- a/drivers/watchdog/iTCO_wdt.c > +++ b/drivers/watchdog/iTCO_wdt.c > @@ -145,58 +145,67 @@ static inline unsigned int ticks_to_seconds(int > ticks) > return iTCO_wdt_private.iTCO_version == 3 ? ticks : (ticks * > 6) / 10; > } > > +static inline u32 no_reboot_bit(void) > +{ > + u32 enable_bit; > + > + switch (iTCO_wdt_private.iTCO_version) { > + case 3: > + enable_bit = 0x00000010; > + break; > + case 2: > + enable_bit = 0x00000020; > + break; > + case 4: > + case 1: > + default: > + enable_bit = 0x00000002; > + break; > + } > + > + return enable_bit; > +} > + > static void iTCO_wdt_set_NO_REBOOT_bit(void) > { > u32 val32; > > /* Set the NO_REBOOT bit: this disables reboots */ > - if (iTCO_wdt_private.iTCO_version == 3) { > - val32 = readl(iTCO_wdt_private.gcs_pmc); > - val32 |= 0x00000010; > - writel(val32, iTCO_wdt_private.gcs_pmc); > - } else if (iTCO_wdt_private.iTCO_version == 2) { > + if (iTCO_wdt_private.iTCO_version >= 2) { > val32 = readl(iTCO_wdt_private.gcs_pmc); > - val32 |= 0x00000020; > + val32 |= no_reboot_bit(); > writel(val32, iTCO_wdt_private.gcs_pmc); > } else if (iTCO_wdt_private.iTCO_version == 1) { > pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, > &val32); > - val32 |= 0x00000002; > + val32 |= no_reboot_bit(); > pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, > val32); > } > } > > static int iTCO_wdt_unset_NO_REBOOT_bit(void) > { > + u32 enable_bit = no_reboot_bit(); > int ret = 0; > - u32 val32; > + u32 val32 = 0; > > /* Unset the NO_REBOOT bit: this enables reboots */ > - if (iTCO_wdt_private.iTCO_version == 3) { > - val32 = readl(iTCO_wdt_private.gcs_pmc); > - val32 &= 0xffffffef; > - writel(val32, iTCO_wdt_private.gcs_pmc); > - > - val32 = readl(iTCO_wdt_private.gcs_pmc); > - if (val32 & 0x00000010) > - ret = -EIO; > - } else if (iTCO_wdt_private.iTCO_version == 2) { > + if (iTCO_wdt_private.iTCO_version >= 2) { > val32 = readl(iTCO_wdt_private.gcs_pmc); > - val32 &= 0xffffffdf; > + val32 &= ~enable_bit; > writel(val32, iTCO_wdt_private.gcs_pmc); > > val32 = readl(iTCO_wdt_private.gcs_pmc); > - if (val32 & 0x00000020) > - ret = -EIO; > } else if (iTCO_wdt_private.iTCO_version == 1) { > pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, > &val32); > - val32 &= 0xfffffffd; > + val32 &= ~enable_bit; > pci_write_config_dword(iTCO_wdt_private.pdev, 0xd4, > val32); > > pci_read_config_dword(iTCO_wdt_private.pdev, 0xd4, > &val32); > - if (val32 & 0x00000002) > - ret = -EIO; > } > > + if (val32 & enable_bit) > + ret = -EIO; > + > return ret; /* returns: 0 = OK, -EIO = Error */ What about removing ret at all? if (val32 & enable_bit) return -EIO; return 0; > } > > @@ -503,12 +512,19 @@ static int iTCO_wdt_probe(struct > platform_device *dev) > pdata->name, pdata->version, (u64)TCOBASE); > > /* Clear out the (probably old) status */ > - if (iTCO_wdt_private.iTCO_version == 3) { > + switch (iTCO_wdt_private.iTCO_version) { > + case 4: > + outw(0x0008, TCO1_STS); /* Clear the Time > Out Status bit */ > + outw(0x0002, TCO2_STS); /* Clear > SECOND_TO_STS bit */ > + break; > + case 3: > outl(0x20008, TCO1_STS); > - } else { > + break; > + default: Same idea here: put cases explicitly for all existing versions? case 2: case 1: default: > outw(0x0008, TCO1_STS); /* Clear the Time > Out Status bit */ > outw(0x0002, TCO2_STS); /* Clear > SECOND_TO_STS bit */ > outw(0x0004, TCO2_STS); /* Clear BOOT_STS > bit */ > + break; > } > > iTCO_wdt_watchdog_dev.bootstatus = 0; -- Andy Shevchenko Intel Finland Oy -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/