[PATCH 1/2] itco: itcoV3 internal timer is stored as seconds.

[Mathieu Alexandre-Tétreault]

Signed-off-by: Mathieu Tetreault <alexa...@amotus.ca>
---
drivers/watchdog/itco.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/watchdog/itco.c b/drivers/watchdog/itco.c
index b7b6e80..5a21a06 100644
--- a/drivers/watchdog/itco.c
+++ b/drivers/watchdog/itco.c
@@ -95,7 +95,7 @@ init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
return status;
}
value &= 0xfc00;
- value |= ((timeout * 10) / 6) & 0x3ff;
+ value |= timeout & 0x3ff;
status = uefi_call_wrapper(pci_io->Io.Write, 6, pci_io,
EfiPciIoWidthUint16,
EFI_PCI_IO_PASS_THROUGH_BAR,
--
2.18.1

[Mathieu Alexandre-Tétreault]

Signed-off-by: Mathieu Tetreault <alexa...@amotus.ca>
---

drivers/watchdog/itco.c | 122 +++++++++++++++++++++++++++++++---------
1 file changed, 95 insertions(+), 27 deletions(-)

diff --git a/drivers/watchdog/itco.c b/drivers/watchdog/itco.c
index 5a21a06..6e5c5be 100644
--- a/drivers/watchdog/itco.c
+++ b/drivers/watchdog/itco.c
@@ -18,6 +18,7 @@

#define PCI_DEVICE_ID_INTEL_BAYTRAIL 0x0f1c
#define PCI_DEVICE_ID_INTEL_WPT_LP 0x9cc3
+#define PCI_DEVICE_ID_INTEL_LPC_LP 0x8c4e

#define PMBASE_REG 0x40
# define PMBASE_ADDRMASK 0xff00
@@ -35,16 +36,104 @@
#define PMC_REG 0x08
# define PMC_NO_REBOOT (1 << 4)

+/* iTCOv2 defines */
+#define RCBABASE_REG 0xf0
+#define RCBABASE_ADDRMASK 0xffffc000
+#define GCS_REG 0x3410
+#define GCS_NO_REBOOT (1 << 5)
+
+static UINT32 get_no_reboot_bit(char version) {
+ return version > 2 ? PMC_NO_REBOOT : GCS_NO_REBOOT;
+}
+
+static UINT32 get_wdog_base_mask(char version) {
+ return version > 2 ? PMCBASE_ADDRMASK : RCBABASE_ADDRMASK;
+}
+
+static UINT32 get_wdog_base_addr_cfg(char version) {
+ return version > 2 ? PMCBASE_REG : RCBABASE_REG;
+}
+
+static UINT32 get_wdog_offset(char version) {
+ return version > 2 ? PMC_REG : GCS_REG;
+}
+
+static UINTN get_wdog_timeout(char version, UINTN seconds) {
+ return version > 2 ? seconds : ((seconds * 10) / 6);
+}
+
+static EFI_STATUS get_wdog_base(EFI_PCI_IO *pci_io, char version, UINT32 *base) {
+ return uefi_call_wrapper(pci_io->Pci.Read, 5, pci_io,
+ EfiPciIoWidthUint32,
+ get_wdog_base_addr_cfg(version), 1, base);
+}
+
+static EFI_STATUS read_wdog_register(EFI_PCI_IO *pci_io, UINT32 wdogbase, UINT32 wdogoffset, UINT32 *value) {
+ return uefi_call_wrapper(pci_io->Mem.Read, 6, pci_io,
+ EfiPciIoWidthUint32,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ wdogbase + wdogoffset, 1, value);
+}
+
+static EFI_STATUS write_wdog_register(EFI_PCI_IO *pci_io, UINT32 wdogbase, UINT32 wdogoffset, UINT32 *value) {
+ return uefi_call_wrapper(pci_io->Mem.Write, 6, pci_io,
+ EfiPciIoWidthUint32,
+ EFI_PCI_IO_PASS_THROUGH_BAR,
+ wdogbase + wdogoffset, 1, value);
+}
+
+static EFI_STATUS clear_wdog_no_reboot(EFI_PCI_IO *pci_io, char version) {
+ EFI_STATUS status;
+ UINT32 value, wdogbase, wdogoffset, wdogmask, norebootbit;
+
+ wdogmask = get_wdog_base_mask(version);
+ wdogoffset = get_wdog_offset(version);
+
+ /* Get PMCBASE (iTCOv3) or GCS address (iTCOv2) */
+ status = get_wdog_base(pci_io, version, &wdogbase);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+ wdogbase &= wdogmask;
+
+ /* Reading NO_REBOOT flag */
+ status = read_wdog_register(pci_io, wdogbase, wdogoffset, &value);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+
+ value &= ~get_no_reboot_bit(version);
+
+ /* Writing NO_REBOOT flag */
+ status = write_wdog_register(pci_io, wdogbase, wdogoffset, &value);
+ if (EFI_ERROR(status)) {
+ return status;
+ }
+
+ return status;
+}
+
static EFI_STATUS __attribute__((constructor))

init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,

UINTN timeout)
{
- UINT32 pmbase, tcobase, pmcbase, value;
+ UINT32 pmbase, tcobase, value;
+ char itcoversion;
EFI_STATUS status;

- if (!pci_io || pci_vendor_id != PCI_VENDOR_ID_INTEL ||
- (pci_device_id != PCI_DEVICE_ID_INTEL_BAYTRAIL &&
- pci_device_id != PCI_DEVICE_ID_INTEL_WPT_LP)) {
+ if (!pci_io || pci_vendor_id != PCI_VENDOR_ID_INTEL) {
+ return EFI_UNSUPPORTED;
+ }
+
+ /* Setting variables according to iTCO version */
+ if (pci_device_id == PCI_DEVICE_ID_INTEL_BAYTRAIL ||
+ pci_device_id == PCI_DEVICE_ID_INTEL_WPT_LP) {
+ /* Detected iTCO v3 */
+ itcoversion = 3;
+ } else if (pci_device_id == PCI_DEVICE_ID_INTEL_LPC_LP){
+ /* Detected iTCO v2*/
+ itcoversion = 2;
+ } else {
return EFI_UNSUPPORTED;
}

@@ -60,15 +149,6 @@ init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
pmbase &= PMBASE_ADDRMASK;
tcobase = (pmbase & PMBASE_ADDRMASK) + 0x60;

- /* Get PMCBASE address */
- status = uefi_call_wrapper(pci_io->Pci.Read, 5, pci_io,
- EfiPciIoWidthUint32, PMCBASE_REG,
- 1, &pmcbase);
- if (EFI_ERROR(status)) {
- return status;
- }
- pmcbase &= PMCBASE_ADDRMASK;
-
/* Enable TCO SMIs */
status = uefi_call_wrapper(pci_io->Io.Read, 6, pci_io,
EfiPciIoWidthUint32,
@@ -95,7 +175,7 @@ init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,

return status;
}
value &= 0xfc00;

- value |= timeout & 0x3ff;
+ value |= get_wdog_timeout(itcoversion, timeout) & 0x3ff;

status = uefi_call_wrapper(pci_io->Io.Write, 6, pci_io,
EfiPciIoWidthUint16,
EFI_PCI_IO_PASS_THROUGH_BAR,

@@ -114,19 +194,7 @@ init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
return status;
}

- /* Clear NO_REBOOT flag */
- status = uefi_call_wrapper(pci_io->Mem.Read, 6, pci_io,
- EfiPciIoWidthUint32,
- EFI_PCI_IO_PASS_THROUGH_BAR,
- pmcbase + PMC_REG, 1, &value);
- if (EFI_ERROR(status)) {
- return status;
- }
- value &= ~PMC_NO_REBOOT;
- status = uefi_call_wrapper(pci_io->Mem.Write, 6, pci_io,
- EfiPciIoWidthUint32,
- EFI_PCI_IO_PASS_THROUGH_BAR,
- pmcbase + PMC_REG, 1, &value);
+ status = clear_wdog_no_reboot(pci_io, itcoversion);
if (EFI_ERROR(status)) {
return status;
}
--
2.18.1

[Jan Kiszka]

Interesting - so we were waiting too long so far? Or my reference back then,
QEMU, is not emulating this accurately. I suppose you tested this against a real
v3 iTCO implementation, right?

Jan

--
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

[Mathieu Alexandre-Tétreault]

On 12/20/18 11:40 AM, Jan Kiszka wrote:
> On 17.12.18 17:06, Mathieu Alexandre-Tétreault wrote:
>> Signed-off-by: Mathieu Tetreault <alexa...@amotus.ca>
>> ---
>> drivers/watchdog/itco.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/itco.c b/drivers/watchdog/itco.c
>> index b7b6e80..5a21a06 100644
>> --- a/drivers/watchdog/itco.c
>> +++ b/drivers/watchdog/itco.c
>> @@ -95,7 +95,7 @@ init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
>> return status;
>> }
>> value &= 0xfc00;
>> - value |= ((timeout * 10) / 6) & 0x3ff;
>> + value |= timeout & 0x3ff;
>> status = uefi_call_wrapper(pci_io->Io.Write, 6, pci_io,
>> EfiPciIoWidthUint16,
>> EFI_PCI_IO_PASS_THROUGH_BAR,
>>
> Interesting - so we were waiting too long so far? Or my reference back then,
> QEMU, is not emulating this accurately. I suppose you tested this against a real
> v3 iTCO implementation, right?
>
> Jan
>

I indeed tested with a real iTCO v3 device ( baytrail to be exact). I
timed the watchdog using a stop watch.
The timer was started as soon as ebg was displaying the wdog message. We
timed 60sec +/- 3 seconds
which I assume was due to a lack of synchronism between the start of the
wdt and the start of my stop watch.

Mathieu

[Jan Kiszka]

On 17.12.18 17:07, Mathieu Alexandre-Tétreault wrote:

Some few words about what v2 vs. v3 means would be good. Or even something like
"this allows to use EBG on X".

> Signed-off-by: Mathieu Tetreault <alexa...@amotus.ca>
> ---
> drivers/watchdog/itco.c | 122 +++++++++++++++++++++++++++++++---------
> 1 file changed, 95 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/watchdog/itco.c b/drivers/watchdog/itco.c
> index 5a21a06..6e5c5be 100644
> --- a/drivers/watchdog/itco.c
> +++ b/drivers/watchdog/itco.c
> @@ -18,6 +18,7 @@
>
> #define PCI_DEVICE_ID_INTEL_BAYTRAIL 0x0f1c
> #define PCI_DEVICE_ID_INTEL_WPT_LP 0x9cc3
> +#define PCI_DEVICE_ID_INTEL_LPC_LP 0x8c4e
>
> #define PMBASE_REG 0x40
> # define PMBASE_ADDRMASK 0xff00
> @@ -35,16 +36,104 @@
> #define PMC_REG 0x08
> # define PMC_NO_REBOOT (1 << 4)
>
> +/* iTCOv2 defines */
> +#define RCBABASE_REG 0xf0
> +#define RCBABASE_ADDRMASK 0xffffc000
> +#define GCS_REG 0x3410
> +#define GCS_NO_REBOOT (1 << 5)
> +
> +static UINT32 get_no_reboot_bit(char version) {

int or rather UINT32 like elsewhere. Please no "char" for anything that is not a
character.

> + return version > 2 ? PMC_NO_REBOOT : GCS_NO_REBOOT;
> +}
> +
> +static UINT32 get_wdog_base_mask(char version) {
> + return version > 2 ? PMCBASE_ADDRMASK : RCBABASE_ADDRMASK;
> +}
> +
> +static UINT32 get_wdog_base_addr_cfg(char version) {
> + return version > 2 ? PMCBASE_REG : RCBABASE_REG;
> +}
> +
> +static UINT32 get_wdog_offset(char version) {
> + return version > 2 ? PMC_REG : GCS_REG;
> +}
> +
> +static UINTN get_wdog_timeout(char version, UINTN seconds) {
> + return version > 2 ? seconds : ((seconds * 10) / 6);
> +}
> +
> +static EFI_STATUS get_wdog_base(EFI_PCI_IO *pci_io, char version, UINT32 *base) {

Let's stay below 80 chars/line and wrap this, as well as the other long lines.

> + return uefi_call_wrapper(pci_io->Pci.Read, 5, pci_io,
> + EfiPciIoWidthUint32,
> + get_wdog_base_addr_cfg(version), 1, base);

Should be aligned with "pci_io->Pci.Read" from the first line.

Just minor remarks, looks good to me in general.

Thanks for addressing this!