[PATCH 0/3] w83627hf: Allow all configurations of BX-56A and 59A

[Tobias Schaffner]

This series adds the station id of the BX-56A and allows to run the
driver for all configurations of the BX-56A and BX-59A.

As the host bridge device id varies over the different configurations,
we only depend on the pci vendor and station id.

This has been tested on both the BX-56A and the BX-59A.

Tobias Schaffner (3):
w83627hf: Add station id of IPC BX-56A
w83627hf: Solely depend on pci vendor and station id
w83627hf: Cache simatic station id

drivers/watchdog/w83627hf_wdt.c | 28 ++++++++++++++++++----------
include/simatic.h | 1 +
2 files changed, 19 insertions(+), 10 deletions(-)

--
2.34.1

[Tobias Schaffner]

The simatic IPC BX-56A shares the same nct6116 superio chip as the
BX-59A. Allow efibootguard to configure the w83627hf watchdog for BX-56A
by adding its station id.

Signed-off-by: Tobias Schaffner <tobias.s...@siemens.com>
---
drivers/watchdog/w83627hf_wdt.c | 3 ++-
include/simatic.h | 1 +
2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
index df1da11..fd2950d 100644
--- a/drivers/watchdog/w83627hf_wdt.c
+++ b/drivers/watchdog/w83627hf_wdt.c
@@ -208,11 +208,12 @@ static EFI_STATUS init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
}

switch (simatic_station_id()) {
+ case SIMATIC_IPCBX_56A:
case SIMATIC_IPCBX_59A:
chip = wdt_find(0x2e);
if (chip < 0)
return EFI_UNSUPPORTED;
- INFO(L"Detected SIMATIC BX59A watchdog\n");
+ INFO(L"Detected SIMATIC BX5xA watchdog\n");
ret = w83627hf_init(chip);
if (ret < 0)
return EFI_UNSUPPORTED;
diff --git a/include/simatic.h b/include/simatic.h
index 724e79d..43c5515 100644
--- a/include/simatic.h
+++ b/include/simatic.h
@@ -25,6 +25,7 @@

#define SIMATIC_IPC427E 0x0a01
#define SIMATIC_IPC477E 0x0a02
+#define SIMATIC_IPCBX_56A 0x1201
#define SIMATIC_IPCBX_59A 0x1202

typedef struct {
--
2.34.1

[Tobias Schaffner]

Now that we solely depend on the vendor pci id the station id will be
compared for a large number of pci device ids. Cache the station id to
avoid retreiving it from SMBIOSTable every time.

Signed-off-by: Tobias Schaffner <tobias.s...@siemens.com>
---

drivers/watchdog/w83627hf_wdt.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
index 68cb42a..c0c35d2 100644
--- a/drivers/watchdog/w83627hf_wdt.c
+++ b/drivers/watchdog/w83627hf_wdt.c
@@ -39,6 +39,7 @@
#include <efi.h>
#include <efilib.h>
#include <pci/header.h>
+#include <stdbool.h>
#include <sys/io.h>
#include <mmio.h>
#include "simatic.h"
@@ -69,6 +70,8 @@ static int cr_wdt_control; /* WDT control register */
static int cr_wdt_csr; /* WDT control & status register */
static int wdt_cfg_enter = 0x87;/* key to unlock configuration space */
static int wdt_cfg_leave = 0xAA;/* key to lock configuration space */
+static UINT32 station_id = 0;
+static BOOLEAN station_id_cached = false;

static void superio_outb(int reg, int val)
{
@@ -192,6 +195,15 @@ static int wdt_set_time(unsigned int timeout)
return 0;
}

+static UINT32 cached_simatic_station_id(VOID)
+{
+ if (station_id_cached) return station_id;
+
+ station_id = simatic_station_id();
+ station_id_cached = true;
+ return station_id;
+}
+

static EFI_STATUS init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,

UINT16 __attribute__((unused)) pci_device_id,
UINTN timeout)
@@ -202,7 +214,7 @@ static EFI_STATUS init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
return EFI_UNSUPPORTED;
}

- switch (simatic_station_id()) {
+ switch (cached_simatic_station_id()) {

case SIMATIC_IPCBX_56A:
case SIMATIC_IPCBX_59A:
chip = wdt_find(0x2e);

--
2.34.1

[Tobias Schaffner]

The host bridge device id varies over the different configurations of
the BX-56A and BX-59A. Only filter intel based devices and then depend
on the station id.

Signed-off-by: Tobias Schaffner <tobias.s...@siemens.com>
---

drivers/watchdog/w83627hf_wdt.c | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
index fd2950d..68cb42a 100644
--- a/drivers/watchdog/w83627hf_wdt.c
+++ b/drivers/watchdog/w83627hf_wdt.c
@@ -44,11 +44,6 @@
#include "simatic.h"
#include "utils.h"

-/* Use the host bridge device found on the BX-59A to limit probing to Intel
- * based machines that may be a BX59A; technically we do not use/need PCI
- * for this driver but only port I/Os */
-#define PCI_DEVICE_ID_INTEL_HOST_BRIDGE 0xa700
-
#define WDT_EFER (wdt_io+0) /* Extended Function Enable Registers */
#define WDT_EFIR (wdt_io+0) /* Extended Function Index Register
(same as EFER) */
@@ -198,12 +193,12 @@ static int wdt_set_time(unsigned int timeout)

}

static EFI_STATUS init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,

- UINT16 pci_device_id, UINTN timeout)
+ UINT16 __attribute__((unused)) pci_device_id,
+ UINTN timeout)
{
int chip, ret;

- if (!pci_io || pci_vendor_id != PCI_VENDOR_ID_INTEL ||
- pci_device_id != PCI_DEVICE_ID_INTEL_HOST_BRIDGE) {
+ if (!pci_io || pci_vendor_id != PCI_VENDOR_ID_INTEL) {
return EFI_UNSUPPORTED;
}

--
2.34.1

[Jan Kiszka]

On 27.03.24 09:19, Tobias Schaffner wrote:
> Now that we solely depend on the vendor pci id the station id will be
> compared for a large number of pci device ids. Cache the station id to
> avoid retreiving it from SMBIOSTable every time.
>
> Signed-off-by: Tobias Schaffner <tobias.s...@siemens.com>
> ---
> drivers/watchdog/w83627hf_wdt.c | 14 +++++++++++++-
> 1 file changed, 13 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/watchdog/w83627hf_wdt.c b/drivers/watchdog/w83627hf_wdt.c
> index 68cb42a..c0c35d2 100644
> --- a/drivers/watchdog/w83627hf_wdt.c
> +++ b/drivers/watchdog/w83627hf_wdt.c
> @@ -39,6 +39,7 @@
> #include <efi.h>
> #include <efilib.h>
> #include <pci/header.h>
> +#include <stdbool.h>

Not needed when you properly pair UEFI BOOLEAN with TRUE and FALSE.

> #include <sys/io.h>
> #include <mmio.h>
> #include "simatic.h"
> @@ -69,6 +70,8 @@ static int cr_wdt_control; /* WDT control register */
> static int cr_wdt_csr; /* WDT control & status register */
> static int wdt_cfg_enter = 0x87;/* key to unlock configuration space */
> static int wdt_cfg_leave = 0xAA;/* key to lock configuration space */
> +static UINT32 station_id = 0;

This is redundant: First, static variables are zero-initialized. Second,
station_id is only used when station_id_cached is true.

> +static BOOLEAN station_id_cached = false;

This initialization is also redundant.

>
> static void superio_outb(int reg, int val)
> {
> @@ -192,6 +195,15 @@ static int wdt_set_time(unsigned int timeout)
> return 0;
> }
>
> +static UINT32 cached_simatic_station_id(VOID)
> +{
> + if (station_id_cached) return station_id;

Please unfold into two line.

> +
> + station_id = simatic_station_id();
> + station_id_cached = true;
> + return station_id;
> +}
> +
> static EFI_STATUS init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
> UINT16 __attribute__((unused)) pci_device_id,
> UINTN timeout)
> @@ -202,7 +214,7 @@ static EFI_STATUS init(EFI_PCI_IO *pci_io, UINT16 pci_vendor_id,
> return EFI_UNSUPPORTED;
> }
>
> - switch (simatic_station_id()) {
> + switch (cached_simatic_station_id()) {

Please make the official interface caching by default. That would avoid
this change and make every user benefit.

> case SIMATIC_IPCBX_56A:
> case SIMATIC_IPCBX_59A:
> chip = wdt_find(0x2e);

Jan

--
Siemens AG, Technology
Linux Expert Center

[Jan Kiszka]

On 27.03.24 09:19, Tobias Schaffner wrote:

Needs README.md update as well. And some comments in w83627hf_wdt.c.

[Jan Kiszka]

Ok, comments are removed with patch 2, so you can leave them untouched.

[shrikant bobade]

Hello Tobias,

We have a BX-59A/BX-56A device, during integration using efibootguard v0.16 we have found the BX-56A showing unsupported watchdog for "w83627hf", can you please assist hear these patches will help us get watchdog integrated well with updated efibootguard? 

also please advise will these be integrated with upstream efibootguard?

Thanks

Shrikant Bobade

[Jan Kiszka]

On 14.05.24 08:38, shrikant bobade wrote:
> Hello Tobias,
>
> We have a BX-59A/BX-56A device, during integration using efibootguard
> v0.16 we have found the BX-56A showing unsupported watchdog for
> "w83627hf", can you please assist hear these patches will help us get
> watchdog integrated well with updated efibootguard? 
>
> also please advise will these be integrated with upstream efibootguard?
>

Tobias is on leave for a while. But you can help the integration with
picking my review comments and addressing them in a v2.