[PATCH] ipc4x7e_wdt: add support for SIMATIC IPC 2x7E

[Cedric Hombourger]

SIMATIC IPCs 227E/277E share the same external watchdog (SCH5347)
than the IPC 4x7Es we already support. Initialization of the
SAFE_EN_N line differs a bit and was therefore moved to separate
setup functions. Programming and enabling of the timeout is the
same and is now handled in set_timeout(). With this change, both
Linux and efibootguard would use the same chip and no longer
depend on the built-in iTCO watchdog.

Signed-off-by: Cedric Hombourger <Cedric_H...@mentor.com>
---
drivers/watchdog/ipc4x7e_wdt.c | 126 +++++++++++++++++++++------------
1 file changed, 81 insertions(+), 45 deletions(-)

diff --git a/drivers/watchdog/ipc4x7e_wdt.c b/drivers/watchdog/ipc4x7e_wdt.c
index 24faebe..12a9a87 100644
--- a/drivers/watchdog/ipc4x7e_wdt.c
+++ b/drivers/watchdog/ipc4x7e_wdt.c
@@ -25,6 +25,8 @@

#define SIMATIC_OEM_ENTRY_TYPE_BINARY 0xff

+#define SIMATIC_IPC227E 0x901
+#define SIMATIC_IPC277E 0x902
#define SIMATIC_IPC427E 0xa01
#define SIMATIC_IPC477E 0xa02

@@ -55,6 +57,8 @@ typedef struct {
/* drives SAFE_EN_N */
#define PAD_CFG_DW0_GPP_A_23 0x4b8
#define PAD_CFG_GPIOTXSTATE (1 << 0)
+#define GP_STATUS_REG_227E 0x404D
+#define SAFE_EN_N_227E 0x04

static UINT32 get_station_id(SMBIOS_STRUCTURE_POINTER oem_strct)
{
@@ -134,6 +138,70 @@ static UINT64 get_sbreg_rba(VOID)
return sbreg;
}

+/*
+ * created from linux/drivers/watchdog/simatic-ipc-wdt.c
+ * (submitted upstream but hasn't reached mainline)
+ */
+static VOID setup_2x7E(VOID)
+{
+ UINT16 resetbit;
+
+ /* enable SAFE_EN_N on GP_STATUS_REG_227E */
+ resetbit = inw(GP_STATUS_REG_227E);
+ resetbit &= ~SAFE_EN_N_227E;
+ outw(resetbit, GP_STATUS_REG_227E);
+}
+
+static VOID setup_4x7E(VOID)
+{
+ UINTN pad_cfg;
+
+ /*
+ * Drive SAFE_EN_N low to allow that watchdog can trigger a
+ * hard reset.
+ */
+ pad_cfg = get_sbreg_rba() + (GPIO_COMMUNITY0_PORT_ID << 16) +
+ PAD_CFG_DW0_GPP_A_23;
+ writel(readl(pad_cfg) & ~PAD_CFG_GPIOTXSTATE, pad_cfg);
+}
+
+static void set_timeout(UINTN timeout)
+{
+ UINT8 val;
+
+ if (timeout <= 2) {
+ val = 0 << SIMATIC_WD_SCALER_SHIFT;
+ } else if (timeout <= 4) {
+ val = 1 << SIMATIC_WD_SCALER_SHIFT;
+ } else if (timeout <= 6) {
+ val = 2 << SIMATIC_WD_SCALER_SHIFT;
+ } else if (timeout <= 8) {
+ val = 3 << SIMATIC_WD_SCALER_SHIFT;
+ } else if (timeout <= 16) {
+ val = 4 << SIMATIC_WD_SCALER_SHIFT;
+ } else if (timeout <= 32) {
+ val = 5 << SIMATIC_WD_SCALER_SHIFT;
+ } else if (timeout <= 48) {
+ val = 6 << SIMATIC_WD_SCALER_SHIFT;
+ } else {
+ val = 7 << SIMATIC_WD_SCALER_SHIFT;
+ }
+ val |= SIMATIC_WD_MACRO_MOD;
+ if (inb(SIMATIC_WD_ENABLE_REG) & SIMATIC_WD_TRIGGERED) {
+ Print(L"NOTE: Detected watchdog triggered reboot\n");
+ /* acknowledge, turning off the LED */
+ val |= SIMATIC_WD_TRIGGERED;
+ }
+ outb(val, SIMATIC_WD_ENABLE_REG);
+
+ /* Enable the watchdog after programming it, just to be safe. */
+ val |= SIMATIC_WD_ENABLE;
+ outb(val, SIMATIC_WD_ENABLE_REG);
+
+ /* Trigger the watchdog once. Now the clock ticks. */
+ inb(SIMATIC_WD_TRIGGER_REG);
+}
+
static EFI_STATUS __attribute__((constructor))
init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
UINTN timeout)
@@ -141,13 +209,6 @@ init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
SMBIOS_STRUCTURE_TABLE *smbios_table;
SMBIOS_STRUCTURE_POINTER smbios_struct;
EFI_STATUS status;
- UINTN pad_cfg;
- UINT8 val;
-
- if (!pci_io || pci_vendor_id != PCI_VENDOR_ID_INTEL ||
- pci_device_id != PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_LPC) {
- return EFI_UNSUPPORTED;
- }

status = LibGetSystemConfigurationTable(&SMBIOSTableGuid,
(VOID **)&smbios_table);
@@ -161,50 +222,25 @@ init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id, UINT16 pci_device_id,
}

switch (get_station_id(smbios_struct)) {
+ case SIMATIC_IPC227E:
+ case SIMATIC_IPC277E:
+ Print(L"Detected SIMATIC IPC2x7E watchdog\n");
+
+ setup_2x7E();
+ set_timeout(timeout);
+ return EFI_SUCCESS;
+
case SIMATIC_IPC427E:
case SIMATIC_IPC477E:
Print(L"Detected SIMATIC IPC4x7E watchdog\n");

- /*
- * Drive SAFE_EN_N low to allow that watchdog can trigger a
- * hard reset.
- */
- pad_cfg = get_sbreg_rba() + (GPIO_COMMUNITY0_PORT_ID << 16) +
- PAD_CFG_DW0_GPP_A_23;
- writel(readl(pad_cfg) & ~PAD_CFG_GPIOTXSTATE, pad_cfg);
-
- if (timeout <= 2) {
- val = 0 << SIMATIC_WD_SCALER_SHIFT;
- } else if (timeout <= 4) {
- val = 1 << SIMATIC_WD_SCALER_SHIFT;
- } else if (timeout <= 6) {
- val = 2 << SIMATIC_WD_SCALER_SHIFT;
- } else if (timeout <= 8) {
- val = 3 << SIMATIC_WD_SCALER_SHIFT;
- } else if (timeout <= 16) {
- val = 4 << SIMATIC_WD_SCALER_SHIFT;
- } else if (timeout <= 32) {
- val = 5 << SIMATIC_WD_SCALER_SHIFT;
- } else if (timeout <= 48) {
- val = 6 << SIMATIC_WD_SCALER_SHIFT;
- } else {
- val = 7 << SIMATIC_WD_SCALER_SHIFT;
+ if (!pci_io || pci_vendor_id != PCI_VENDOR_ID_INTEL ||
+ pci_device_id != PCI_DEVICE_ID_INTEL_SUNRISEPOINT_H_LPC) {
+ return EFI_UNSUPPORTED;
}
- val |= SIMATIC_WD_MACRO_MOD;
- if (inb(SIMATIC_WD_ENABLE_REG) & SIMATIC_WD_TRIGGERED) {
- Print(L"NOTE: Detected watchdog triggered reboot\n");
- /* acknowledge, turning off the LED */
- val |= SIMATIC_WD_TRIGGERED;
- }
- outb(val, SIMATIC_WD_ENABLE_REG);
-
- /* Enable the watchdog after programming it, just to be safe. */
- val |= SIMATIC_WD_ENABLE;
- outb(val, SIMATIC_WD_ENABLE_REG);
-
- /* Trigger the watchdog once. Now the clock ticks. */
- inb(SIMATIC_WD_TRIGGER_REG);

+ setup_4x7E();
+ set_timeout(timeout);
return EFI_SUCCESS;

default:
--
2.30.2

[Jan Kiszka]

On 12.07.21 15:37, Cedric Hombourger wrote:
> SIMATIC IPCs 227E/277E share the same external watchdog (SCH5347)
> than the IPC 4x7Es we already support. Initialization of the
> SAFE_EN_N line differs a bit and was therefore moved to separate
> setup functions. Programming and enabling of the timeout is the
> same and is now handled in set_timeout(). With this change, both
> Linux and efibootguard would use the same chip and no longer
> depend on the built-in iTCO watchdog.

The 4x7E had a hard requirement to use platform watchdog rather than
iTCO - because the latter was unable to actually reset the device. This
limitation does not exist on the 2x7E series. Your commit message is
lacking a reasoning why a user should use that special watchdog, rather
than the working and widely supported iTCO?

Jan

--
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Cedric Hombourger]

Well I do not have a reason other than the Simatic IPC team "insisting"
on us using the external one via their simatic-ipc-wdt driver. We have
indeed found both watchdogs to be working just fine. However having
efibootguard initializing the built-in watchdog (iTCO) and Linux
blacklisting iTCO and using simatic-ipc-wdt (as instructed) is simply
asking for troubles (IMO).

If you believe we can push back and use the iTCO across the board, I am
all for it and both of us could talk to Florian S and maybe have his
team update the simatic-ipc-lsp documentation to make it clear that the
iTCO driver may safely be used on 227E (as we both found)

Let me know if you have other ideas or suggestions

Cedric

>
> Jan
>

[Jan Kiszka]

On 12.07.21 16:19, Cedric Hombourger wrote:
>
> On 7/12/2021 3:57 PM, Jan Kiszka wrote:
>> On 12.07.21 15:37, Cedric Hombourger wrote:
>>> SIMATIC IPCs 227E/277E share the same external watchdog (SCH5347)
>>> than the IPC 4x7Es we already support. Initialization of the
>>> SAFE_EN_N line differs a bit and was therefore moved to separate
>>> setup functions. Programming and enabling of the timeout is the
>>> same and is now handled in set_timeout(). With this change, both
>>> Linux and efibootguard would use the same chip and no longer
>>> depend on the built-in iTCO watchdog.
>> The 4x7E had a hard requirement to use platform watchdog rather than
>> iTCO - because the latter was unable to actually reset the device. This
>> limitation does not exist on the 2x7E series. Your commit message is
>> lacking a reasoning why a user should use that special watchdog, rather
>> than the working and widely supported iTCO?
>
> Well I do not have a reason other than the Simatic IPC team "insisting"
> on us using the external one via their simatic-ipc-wdt driver. We have
> indeed found both watchdogs to be working just fine. However having
> efibootguard initializing the built-in watchdog (iTCO) and Linux
> blacklisting iTCO and using simatic-ipc-wdt (as instructed) is simply
> asking for troubles (IMO).

That is true - but also not the problem of EBG but rather of the Linux
configuration (no need to prioritize the platform watchdog there as well).

>
> If you believe we can push back and use the iTCO across the board, I am
> all for it and both of us could talk to Florian S and maybe have his
> team update the simatic-ipc-lsp documentation to make it clear that the
> iTCO driver may safely be used on 227E (as we both found)
>
> Let me know if you have other ideas or suggestions

Already ping'ed Henning on this internally, now here as well, as he is
managing the IPC driver upstreaming. We have to tell the Linux community
the same story like here. If there is a technical reason to favor
platform over standard, we share that in EBG, obviously.

[Henning Schild]

Am Mon, 12 Jul 2021 16:33:15 +0200
schrieb Jan Kiszka <jan.k...@siemens.com>:

I would turn that around. The HW does have two watchdogs and both might
be armed once Linux takes over, so it needs drivers to do so.
How to model that in userland would be another fun-question, but
watchdogd and systemd support multiple watchdogs.

> >
> > If you believe we can push back and use the iTCO across the board,
> > I am all for it and both of us could talk to Florian S and maybe
> > have his team update the simatic-ipc-lsp documentation to make it
> > clear that the iTCO driver may safely be used on 227E (as we both
> > found)
> >
> > Let me know if you have other ideas or suggestions
>
> Already ping'ed Henning on this internally, now here as well, as he is
> managing the IPC driver upstreaming. We have to tell the Linux
> community the same story like here. If there is a technical reason to
> favor platform over standard, we share that in EBG, obviously.

I think the second watchdog is somehow more powerful. Our Industy PCs
have an nvram (not yet proposed upstream) and an interrupt (power going
to fail in 10ms - also not yet proposed upstream). The combination can
make a feature to sync (fast ... 10ms) while off-power. But the whole
story ... watchdog-wise ... only works with our watchdog.

I am not sure if/when we will be able to put all the pieces together in
the kernel. But the main story seems to be ... there is another
watchdog and we rather have a driver for it ... if just in case a
bootloader or the BIOS armed it and we need to drive/disable it.

Henning

> Jan
>

[Henning Schild]

Am Mon, 12 Jul 2021 22:05:50 +0200
schrieb Henning Schild <henning...@siemens.com>:

But as long as Linux can not drive or disable that Siemens watchdog, any
bootloader it probably better off arming the iTCO. Because that is
known to work and can be taken over by any kernel that supports it.

A bootloader arming a watchdog that is not Linux-upstream is asking for
reboot-loops.

Henning

> Henning
>
> > Jan
> >
>

[Jan Kiszka]

Fair enough for Linux. Here we are in control, though.

>>>
>>> If you believe we can push back and use the iTCO across the board,
>>> I am all for it and both of us could talk to Florian S and maybe
>>> have his team update the simatic-ipc-lsp documentation to make it
>>> clear that the iTCO driver may safely be used on 227E (as we both
>>> found)
>>>
>>> Let me know if you have other ideas or suggestions
>>
>> Already ping'ed Henning on this internally, now here as well, as he is
>> managing the IPC driver upstreaming. We have to tell the Linux
>> community the same story like here. If there is a technical reason to
>> favor platform over standard, we share that in EBG, obviously.
>
> I think the second watchdog is somehow more powerful. Our Industy PCs
> have an nvram (not yet proposed upstream) and an interrupt (power going
> to fail in 10ms - also not yet proposed upstream). The combination can
> make a feature to sync (fast ... 10ms) while off-power. But the whole
> story ... watchdog-wise ... only works with our watchdog.

How does the watchdog relate to any of those mentioned features?
Conceptually, they are unrelated - unless there are hardware-induced
internal dependencies.

Just looking at the platform watchdog functionality, I don't spot any
advantage over the iTCO. It lacks just like the latter no-way-out e.g.

>
> I am not sure if/when we will be able to put all the pieces together in
> the kernel. But the main story seems to be ... there is another
> watchdog and we rather have a driver for it ... if just in case a
> bootloader or the BIOS armed it and we need to drive/disable it.
>

As I said, here we are in control and need to decide which one to start.
EBG only supports one watchdog, thus needs to pick the best one for a
given device. On the 4x7E, the decision was simple because it was
between working and non-working. Here we have two times working, and I
would actually go for "standard first", specifically as long as there is
no upstream support for the platform watchdog.

[just reading your second reply, we are on the same page with this]

[Henning Schild]

Am Mon, 12 Jul 2021 22:47:05 +0200

I would guess that when the iTCO cuts power, no power loss interrupt
will be sent. If the power really fails or if the Siemens watchdogs
cuts power ... you would get one such interrupt.

> Just looking at the platform watchdog functionality, I don't spot any
> advantage over the iTCO. It lacks just like the latter no-way-out e.g.

We should ask our guys in the next call. I am just guessing here as
well.

> >
> > I am not sure if/when we will be able to put all the pieces
> > together in the kernel. But the main story seems to be ... there is
> > another watchdog and we rather have a driver for it ... if just in
> > case a bootloader or the BIOS armed it and we need to drive/disable
> > it.
>
> As I said, here we are in control and need to decide which one to
> start. EBG only supports one watchdog, thus needs to pick the best
> one for a given device. On the 4x7E, the decision was simple because
> it was between working and non-working. Here we have two times
> working, and I would actually go for "standard first", specifically
> as long as there is no upstream support for the platform watchdog.
>
> [just reading your second reply, we are on the same page with this]

Yes, take the iTCO and maybe switch if we ever find out why the other
one could be better. Even if the other one is better because it
supports the "power loss interrupt" ... Linux could use both or
transition from iTCO to Siemens in case it supports Siemens and wants
that interrupt or whatever else that Siemens watchdog would be better
at.

Henning

> Jan
>

[Jan Kiszka]

OK, let's clarify the functionality without colleagues. If it turns out
to be valuable, we would still need some control over which watchdog EBG
selects in the end in order to support setups where the booted OS does
not have support for the platform watchdog.