[PATCH v5 0/4] add device drivers for Siemens Industrial PCs

[PATCH v5 0/4] add device drivers for Siemens Industrial PCs

[Henning Schild]

changes since v4:
- make everything around GPIO memory usage more verbose
  - commit messages, FIXME in p1, cover-letter

changes since v3:

- fix io access width and region reservations
- fix style in p1

changes since v2:

- remove "simatic-ipc" prefix from LED names
- fix style issues found in v2, mainly LED driver
- fix OEM specific dmi code, and remove magic numbers
- more "simatic_ipc" name prefixing
- improved pmc quirk code using callbacks

changes since v1:

- fixed lots of style issues found in v1
  - (debug) printing
  - header ordering
- fixed license issues GPLv2 and SPDX in all files
- module_platform_driver instead of __init __exit
- wdt simplifications cleanup
- lots of fixes in wdt driver, all that was found in v1
- fixed dmi length in dmi helper
- changed LED names to allowed ones
- move led driver to simple/
- switched pmc_atom to dmi callback with global variable


This series adds support for watchdogs and leds of several x86 devices
from Siemens.

It is structured with a platform driver that mainly does identification
of the machines. It might trigger loading of the actual device drivers
by attaching devices to the platform bus.

The identification is vendor specific, parsing a special binary DMI
entry. The implementation of that platform identification is applied on
pmc_atom clock quirks in the final patch.

It is all structured in a way that we can easily add more devices and
more platform drivers later. Internally we have some more code for
hardware monitoring, more leds, watchdogs etc. This will follow some
day.

The LED as well as the watchdog drivers access GPIO memory directly.
Using pinctrl is not possible because the machines lack ACPI entries for
the pinctrl drivers. Updates to the ACPI tables are not expected. So we
can rule out a conflict where two drivers would try and access that GPIO
memory.
So we do not use those pins as "general purpose" but as "Siemens
purpose", after having identified the devices very clearly. 

Henning Schild (4):
  platform/x86: simatic-ipc: add main driver for Siemens devices
  leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  platform/x86: pmc_atom: improve critclk_systems matching for Siemens
    PCs

 drivers/leds/Kconfig                          |   3 +
 drivers/leds/Makefile                         |   3 +
 drivers/leds/simple/Kconfig                   |  11 +
 drivers/leds/simple/Makefile                  |   2 +
 drivers/leds/simple/simatic-ipc-leds.c        | 202 ++++++++++++++++
 drivers/platform/x86/Kconfig                  |  12 +
 drivers/platform/x86/Makefile                 |   3 +
 drivers/platform/x86/pmc_atom.c               |  54 +++--
 drivers/platform/x86/simatic-ipc.c            | 176 ++++++++++++++
 drivers/watchdog/Kconfig                      |  11 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/simatic-ipc-wdt.c            | 228 ++++++++++++++++++
 .../platform_data/x86/simatic-ipc-base.h      |  29 +++
 include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
 14 files changed, 786 insertions(+), 21 deletions(-)
 create mode 100644 drivers/leds/simple/Kconfig
 create mode 100644 drivers/leds/simple/Makefile
 create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
 create mode 100644 drivers/platform/x86/simatic-ipc.c
 create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
 create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
 create mode 100644 include/linux/platform_data/x86/simatic-ipc.h

-- 
2.32.0

[PATCH v5 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices

[Henning Schild]

This mainly implements detection of these devices and will allow
secondary drivers to work on such machines.

The identification is DMI-based with a vendor specific way to tell them
apart in a reliable way.

Drivers for LEDs and Watchdogs will follow to make use of that platform
detection.

There is also some code to allow secondary drivers to find GPIO memory,
that needs to be in place because the pinctrl drivers do not come up.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/platform/x86/Kconfig                  |  12 ++
 drivers/platform/x86/Makefile                 |   3 +
 drivers/platform/x86/simatic-ipc.c            | 176 ++++++++++++++++++
 .../platform_data/x86/simatic-ipc-base.h      |  29 +++
 include/linux/platform_data/x86/simatic-ipc.h |  72 +++++++
 5 files changed, 292 insertions(+)
 create mode 100644 drivers/platform/x86/simatic-ipc.c
 create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
 create mode 100644 include/linux/platform_data/x86/simatic-ipc.h

diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index 7400bc5da5be..a59f3bff5ae1 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1075,6 +1075,18 @@ config INTEL_SCU_IPC_UTIL
 	  low level access for debug work and updating the firmware. Say
 	  N unless you will be doing this on an Intel MID platform.
 
+config SIEMENS_SIMATIC_IPC
+	tristate "Siemens Simatic IPC Class driver"
+	depends on PCI
+	help
+	  This Simatic IPC class driver is the central of several drivers. It
+	  is mainly used for system identification, after which drivers in other
+	  classes will take care of driving specifics of those machines.
+	  i.e. LEDs and watchdog.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called simatic-ipc.
+
 endif # X86_PLATFORM_DEVICES
 
 config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 219478061683..310d1d2a0886 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -123,3 +123,6 @@ obj-$(CONFIG_INTEL_SCU_PLATFORM)	+= intel_scu_pltdrv.o
 obj-$(CONFIG_INTEL_SCU_WDT)		+= intel_scu_wdt.o
 obj-$(CONFIG_INTEL_SCU_IPC_UTIL)	+= intel_scu_ipcutil.o
 obj-$(CONFIG_PMC_ATOM)			+= pmc_atom.o
+
+# Siemens Simatic Industrial PCs
+obj-$(CONFIG_SIEMENS_SIMATIC_IPC)	+= simatic-ipc.o
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
new file mode 100644
index 000000000000..b599cda5ba3c
--- /dev/null
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -0,0 +1,176 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC platform driver
+ *
+ * Copyright (c) Siemens AG, 2018-2021
+ *
+ * Authors:
+ *  Henning Schild <henning.schild@siemens.com>
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/dmi.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_data/x86/simatic-ipc.h>
+#include <linux/platform_device.h>
+
+static struct platform_device *ipc_led_platform_device;
+static struct platform_device *ipc_wdt_platform_device;
+
+static const struct dmi_system_id simatic_ipc_whitelist[] = {
+	{
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
+		},
+	},
+	{}
+};
+
+static struct simatic_ipc_platform platform_data;
+
+static struct {
+	u32 station_id;
+	u8 led_mode;
+	u8 wdt_mode;
+} device_modes[] = {
+	{SIMATIC_IPC_IPC127E, SIMATIC_IPC_DEVICE_127E, SIMATIC_IPC_DEVICE_NONE},
+	{SIMATIC_IPC_IPC227D, SIMATIC_IPC_DEVICE_227D, SIMATIC_IPC_DEVICE_NONE},
+	{SIMATIC_IPC_IPC227E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_227E},
+	{SIMATIC_IPC_IPC277E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_227E},
+	{SIMATIC_IPC_IPC427D, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_NONE},
+	{SIMATIC_IPC_IPC427E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_427E},
+	{SIMATIC_IPC_IPC477E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_427E},
+};
+
+static int register_platform_devices(u32 station_id)
+{
+	u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
+	u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
+	int i;
+
+	platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
+
+	for (i = 0; i < ARRAY_SIZE(device_modes); i++) {
+		if (device_modes[i].station_id == station_id) {
+			ledmode = device_modes[i].led_mode;
+			wdtmode = device_modes[i].wdt_mode;
+			break;
+		}
+	}
+
+	if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
+		platform_data.devmode = ledmode;
+		ipc_led_platform_device =
+			platform_device_register_data(NULL,
+				KBUILD_MODNAME "_leds", PLATFORM_DEVID_NONE,
+				&platform_data,
+				sizeof(struct simatic_ipc_platform));
+		if (IS_ERR(ipc_led_platform_device))
+			return PTR_ERR(ipc_led_platform_device);
+
+		pr_debug("device=%s created\n",
+			 ipc_led_platform_device->name);
+	}
+
+	if (wdtmode != SIMATIC_IPC_DEVICE_NONE) {
+		platform_data.devmode = wdtmode;
+		ipc_wdt_platform_device =
+			platform_device_register_data(NULL,
+				KBUILD_MODNAME "_wdt", PLATFORM_DEVID_NONE,
+				&platform_data,
+				sizeof(struct simatic_ipc_platform));
+		if (IS_ERR(ipc_wdt_platform_device))
+			return PTR_ERR(ipc_wdt_platform_device);
+
+		pr_debug("device=%s created\n",
+			 ipc_wdt_platform_device->name);
+	}
+
+	if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
+	    wdtmode == SIMATIC_IPC_DEVICE_NONE) {
+		pr_warn("unsupported IPC detected, station id=%08x\n",
+			station_id);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+/* FIXME: this should eventually be done with generic P2SB discovery code
+ * the individual drivers for watchdogs and LEDs access memory that implements
+ * GPIO, but pinctrl will not come up because of missing ACPI entries
+ *
+ * While there is no conflict a cleaner solution would be to somehow bring up
+ * pinctrl even with these ACPI entries missing, and base the drivers on pinctrl.
+ * After which the following function could be dropped, together with the code
+ * poking the memory.
+ */
+/*
+ * Get membase address from PCI, used in leds and wdt module. Here we read
+ * the bar0. The final address calculation is done in the appropriate modules
+ */
+u32 simatic_ipc_get_membase0(unsigned int p2sb)
+{
+	struct pci_bus *bus;
+	u32 bar0 = 0;
+	/*
+	 * The GPIO memory is in bar0 of the hidden P2SB device.
+	 * Unhide the device to have a quick look at it, before we hide it
+	 * again.
+	 * Also grab the pci rescan lock so that device does not get discovered
+	 * and remapped while it is visible.
+	 * This code is inspired by drivers/mfd/lpc_ich.c
+	 */
+	bus = pci_find_bus(0, 0);
+	pci_lock_rescan_remove();
+	pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
+	pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
+
+	bar0 &= ~0xf;
+	pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
+	pci_unlock_rescan_remove();
+
+	return bar0;
+}
+EXPORT_SYMBOL(simatic_ipc_get_membase0);
+
+static int __init simatic_ipc_init_module(void)
+{
+	const struct dmi_system_id *match;
+	u32 station_id;
+	int err;
+
+	match = dmi_first_match(simatic_ipc_whitelist);
+	if (!match)
+		return 0;
+
+	err = dmi_walk(simatic_ipc_find_dmi_entry_helper, &station_id);
+
+	if (err || station_id == SIMATIC_IPC_INVALID_STATION_ID) {
+		pr_warn("DMI entry %d not found\n", SIMATIC_IPC_DMI_ENTRY_OEM);
+		return 0;
+	}
+
+	return register_platform_devices(station_id);
+}
+
+static void __exit simatic_ipc_exit_module(void)
+{
+	platform_device_unregister(ipc_led_platform_device);
+	ipc_led_platform_device = NULL;
+
+	platform_device_unregister(ipc_wdt_platform_device);
+	ipc_wdt_platform_device = NULL;
+}
+
+module_init(simatic_ipc_init_module);
+module_exit(simatic_ipc_exit_module);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
+MODULE_ALIAS("dmi:*:svnSIEMENSAG:*");
diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h
new file mode 100644
index 000000000000..62d2bc774067
--- /dev/null
+++ b/include/linux/platform_data/x86/simatic-ipc-base.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Siemens SIMATIC IPC drivers
+ *
+ * Copyright (c) Siemens AG, 2018-2021
+ *
+ * Authors:
+ *  Henning Schild <henning.schild@siemens.com>
+ *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
+ */
+
+#ifndef __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H
+#define __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H
+
+#include <linux/types.h>
+
+#define SIMATIC_IPC_DEVICE_NONE 0
+#define SIMATIC_IPC_DEVICE_227D 1
+#define SIMATIC_IPC_DEVICE_427E 2
+#define SIMATIC_IPC_DEVICE_127E 3
+#define SIMATIC_IPC_DEVICE_227E 4
+
+struct simatic_ipc_platform {
+	u8	devmode;
+};
+
+u32 simatic_ipc_get_membase0(unsigned int p2sb);
+
+#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H */
diff --git a/include/linux/platform_data/x86/simatic-ipc.h b/include/linux/platform_data/x86/simatic-ipc.h
new file mode 100644
index 000000000000..f3b76b39776b
--- /dev/null
+++ b/include/linux/platform_data/x86/simatic-ipc.h
@@ -0,0 +1,72 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Siemens SIMATIC IPC drivers
+ *
+ * Copyright (c) Siemens AG, 2018-2021
+ *
+ * Authors:
+ *  Henning Schild <henning.schild@siemens.com>
+ *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
+ */
+
+#ifndef __PLATFORM_DATA_X86_SIMATIC_IPC_H
+#define __PLATFORM_DATA_X86_SIMATIC_IPC_H
+
+#include <linux/dmi.h>
+#include <linux/platform_data/x86/simatic-ipc-base.h>
+
+#define SIMATIC_IPC_DMI_ENTRY_OEM	129
+/* binary type */
+#define SIMATIC_IPC_DMI_TYPE		0xff
+#define SIMATIC_IPC_DMI_GROUP		0x05
+#define SIMATIC_IPC_DMI_ENTRY		0x02
+#define SIMATIC_IPC_DMI_TID		0x02
+
+enum simatic_ipc_station_ids {
+	SIMATIC_IPC_INVALID_STATION_ID = 0,
+	SIMATIC_IPC_IPC227D = 0x00000501,
+	SIMATIC_IPC_IPC427D = 0x00000701,
+	SIMATIC_IPC_IPC227E = 0x00000901,
+	SIMATIC_IPC_IPC277E = 0x00000902,
+	SIMATIC_IPC_IPC427E = 0x00000A01,
+	SIMATIC_IPC_IPC477E = 0x00000A02,
+	SIMATIC_IPC_IPC127E = 0x00000D01,
+};
+
+static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
+{
+	struct {
+		u8	type;		/* type (0xff = binary) */
+		u8	len;		/* len of data entry */
+		u8	group;
+		u8	entry;
+		u8	tid;
+		__le32	station_id;	/* station id (LE) */
+	} __packed * data_entry = (void *)data + sizeof(struct dmi_header);
+
+	while ((u8 *)data_entry < data + max_len) {
+		if (data_entry->type == SIMATIC_IPC_DMI_TYPE &&
+		    data_entry->len == sizeof(*data_entry) &&
+		    data_entry->group == SIMATIC_IPC_DMI_GROUP &&
+		    data_entry->entry == SIMATIC_IPC_DMI_ENTRY &&
+		    data_entry->tid == SIMATIC_IPC_DMI_TID) {
+			return le32_to_cpu(data_entry->station_id);
+		}
+		data_entry = (void *)((u8 *)(data_entry) + data_entry->len);
+	}
+
+	return SIMATIC_IPC_INVALID_STATION_ID;
+}
+
+static inline void
+simatic_ipc_find_dmi_entry_helper(const struct dmi_header *dh, void *_data)
+{
+	u32 *id = _data;
+
+	if (dh->type != SIMATIC_IPC_DMI_ENTRY_OEM)
+		return;
+
+	*id = simatic_ipc_get_station_id((u8 *)dh, dh->length);
+}
+
+#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */
-- 
2.32.0

[PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Henning Schild]

This driver adds initial support for several devices from Siemens. It is
based on a platform driver introduced in an earlier commit.

One of the supported machines has GPIO connected LEDs, here we poke GPIO
memory directly because pinctrl does not come up.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/leds/Kconfig                   |   3 +
 drivers/leds/Makefile                  |   3 +
 drivers/leds/simple/Kconfig            |  11 ++
 drivers/leds/simple/Makefile           |   2 +
 drivers/leds/simple/simatic-ipc-leds.c | 202 +++++++++++++++++++++++++
 5 files changed, 221 insertions(+)
 create mode 100644 drivers/leds/simple/Kconfig
 create mode 100644 drivers/leds/simple/Makefile
 create mode 100644 drivers/leds/simple/simatic-ipc-leds.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index ed800f5da7d8..ac6688d7a3f4 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -879,4 +879,7 @@ source "drivers/leds/flash/Kconfig"
 comment "LED Triggers"
 source "drivers/leds/trigger/Kconfig"
 
+comment "Simple LED drivers"
+source "drivers/leds/simple/Kconfig"
+
 endif # NEW_LEDS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index c636ec069612..1a719caf14c0 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -105,3 +105,6 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= trigger/
 
 # LED Blink
 obj-y					+= blink/
+
+# Simple LED drivers
+obj-y					+= simple/
diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
new file mode 100644
index 000000000000..9f6a68336659
--- /dev/null
+++ b/drivers/leds/simple/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config LEDS_SIEMENS_SIMATIC_IPC
+	tristate "LED driver for Siemens Simatic IPCs"
+	depends on LEDS_CLASS
+	depends on SIEMENS_SIMATIC_IPC
+	help
+	  This option enables support for the LEDs of several Industrial PCs
+	  from Siemens.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called simatic-ipc-leds.
diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
new file mode 100644
index 000000000000..8481f1e9e360
--- /dev/null
+++ b/drivers/leds/simple/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o
diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
new file mode 100644
index 000000000000..ff2c96e73241
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for LEDs
+ *
+ * Copyright (c) Siemens AG, 2018-2021
+ *
+ * Authors:
+ *  Henning Schild <henning.schild@siemens.com>
+ *  Jan Kiszka <jan.kiszka@siemens.com>
+ *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
+ */
+
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_data/x86/simatic-ipc-base.h>
+#include <linux/platform_device.h>
+#include <linux/sizes.h>
+#include <linux/spinlock.h>
+
+#define SIMATIC_IPC_LED_PORT_BASE	0x404E
+
+struct simatic_ipc_led {
+	unsigned int value; /* mask for io and offset for mem */
+	char *name;
+	struct led_classdev cdev;
+};
+
+static struct simatic_ipc_led simatic_ipc_leds_io[] = {
+	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
+	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
+	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
+	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
+	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
+	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
+	{ }
+};
+
+/* the actual start will be discovered with PCI, 0 is a placeholder */
+struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
+
+static void *simatic_ipc_led_memory;
+
+static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
+	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
+	{0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
+	{0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
+	{0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
+	{0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
+	{0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
+	{ }
+};
+
+static struct resource simatic_ipc_led_io_res =
+	DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_2, KBUILD_MODNAME);
+
+static DEFINE_SPINLOCK(reg_lock);
+
+static inline struct simatic_ipc_led *cdev_to_led(struct led_classdev *led_cd)
+{
+	return container_of(led_cd, struct simatic_ipc_led, cdev);
+}
+
+static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
+				   enum led_brightness brightness)
+{
+	struct simatic_ipc_led *led = cdev_to_led(led_cd);
+	unsigned long flags;
+	unsigned int val;
+
+	spin_lock_irqsave(&reg_lock, flags);
+
+	val = inw(SIMATIC_IPC_LED_PORT_BASE);
+	if (brightness == LED_OFF)
+		outw(val | led->value, SIMATIC_IPC_LED_PORT_BASE);
+	else
+		outw(val & ~led->value, SIMATIC_IPC_LED_PORT_BASE);
+
+	spin_unlock_irqrestore(&reg_lock, flags);
+}
+
+static enum led_brightness simatic_ipc_led_get_io(struct led_classdev *led_cd)
+{
+	struct simatic_ipc_led *led = cdev_to_led(led_cd);
+
+	return inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF : led_cd->max_brightness;
+}
+
+static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
+				    enum led_brightness brightness)
+{
+	struct simatic_ipc_led *led = cdev_to_led(led_cd);
+
+	u32 *p;
+
+	p = simatic_ipc_led_memory + led->value;
+	*p = (*p & ~1) | (brightness == LED_OFF);
+}
+
+static enum led_brightness simatic_ipc_led_get_mem(struct led_classdev *led_cd)
+{
+	struct simatic_ipc_led *led = cdev_to_led(led_cd);
+
+	u32 *p;
+
+	p = simatic_ipc_led_memory + led->value;
+	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
+}
+
+static int simatic_ipc_leds_probe(struct platform_device *pdev)
+{
+	const struct simatic_ipc_platform *plat = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct simatic_ipc_led *ipcled;
+	struct led_classdev *cdev;
+	struct resource *res;
+	int err, type;
+	u32 *p;
+
+	switch (plat->devmode) {
+	case SIMATIC_IPC_DEVICE_227D:
+	case SIMATIC_IPC_DEVICE_427E:
+		res = &simatic_ipc_led_io_res;
+		ipcled = simatic_ipc_leds_io;
+		/* on 227D the two bytes work the other way araound */
+		if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
+			while (ipcled->value) {
+				ipcled->value = swab16(ipcled->value);
+				ipcled++;
+			}
+			ipcled = simatic_ipc_leds_io;
+		}
+		type = IORESOURCE_IO;
+		if (!devm_request_region(dev, res->start, resource_size(res), KBUILD_MODNAME)) {
+			dev_err(dev, "Unable to register IO resource at %pR\n", res);
+			return -EBUSY;
+		}
+		break;
+	case SIMATIC_IPC_DEVICE_127E:
+		res = &simatic_ipc_led_mem_res;
+		ipcled = simatic_ipc_leds_mem;
+		type = IORESOURCE_MEM;
+
+		/* get GPIO base from PCI */
+		res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
+		if (res->start == 0)
+			return -ENODEV;
+
+		/* do the final address calculation */
+		res->start = res->start + (0xC5 << 16);
+		res->end += res->start;
+
+		simatic_ipc_led_memory = devm_ioremap_resource(dev, res);
+		if (IS_ERR(simatic_ipc_led_memory))
+			return PTR_ERR(simatic_ipc_led_memory);
+
+		/* initialize power/watchdog LED */
+		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /* PM_WDT_OUT */
+		*p = (*p & ~1);
+		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /* PM_BIOS_BOOT_N */
+		*p = (*p | 1);
+
+		break;
+	default:
+		return -ENODEV;
+	}
+
+	while (ipcled->value) {
+		cdev = &ipcled->cdev;
+		if (type == IORESOURCE_MEM) {
+			cdev->brightness_set = simatic_ipc_led_set_mem;
+			cdev->brightness_get = simatic_ipc_led_get_mem;
+		} else {
+			cdev->brightness_set = simatic_ipc_led_set_io;
+			cdev->brightness_get = simatic_ipc_led_get_io;
+		}
+		cdev->max_brightness = LED_ON;
+		cdev->name = ipcled->name;
+
+		err = devm_led_classdev_register(dev, cdev);
+		if (err < 0)
+			return err;
+		ipcled++;
+	}
+
+	return 0;
+}
+
+static struct platform_driver simatic_ipc_led_driver = {
+	.probe = simatic_ipc_leds_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	}
+};
+
+module_platform_driver(simatic_ipc_led_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
-- 
2.32.0

[PATCH v5 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs

[Henning Schild]

This driver adds initial support for several devices from Siemens. It is
based on a platform driver introduced in an earlier commit.

One of the supported machines does access a GPIO pin to enable the
watchdog. Here we poke GPIO memory because pinctrl does not come up.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
---
 drivers/watchdog/Kconfig           |  11 ++
 drivers/watchdog/Makefile          |   1 +
 drivers/watchdog/simatic-ipc-wdt.c | 228 +++++++++++++++++++++++++++++
 3 files changed, 240 insertions(+)
 create mode 100644 drivers/watchdog/simatic-ipc-wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 9d222ba17ec6..1dc86eb1361a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1589,6 +1589,17 @@ config NIC7018_WDT
 	  To compile this driver as a module, choose M here: the module will be
 	  called nic7018_wdt.
 
+config SIEMENS_SIMATIC_IPC_WDT
+	tristate "Siemens Simatic IPC Watchdog"
+	depends on SIEMENS_SIMATIC_IPC
+	select WATCHDOG_CORE
+	help
+	  This driver adds support for several watchdogs found in Industrial
+	  PCs from Siemens.
+
+	  To compile this driver as a module, choose M here: the module will be
+	  called simatic-ipc-wdt.
+
 # M68K Architecture
 
 config M54xx_WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 2ee97064145b..31b931846e32 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -143,6 +143,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
 obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
 obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
 obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o
+obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o
 
 # M68K Architecture
 obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o
diff --git a/drivers/watchdog/simatic-ipc-wdt.c b/drivers/watchdog/simatic-ipc-wdt.c
new file mode 100644
index 000000000000..8bac793c63fb
--- /dev/null
+++ b/drivers/watchdog/simatic-ipc-wdt.c
@@ -0,0 +1,228 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for Watchdogs
+ *
+ * Copyright (c) Siemens AG, 2020-2021
+ *
+ * Authors:
+ *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_data/x86/simatic-ipc-base.h>
+#include <linux/platform_device.h>
+#include <linux/sizes.h>
+#include <linux/util_macros.h>
+#include <linux/watchdog.h>
+
+#define WD_ENABLE_IOADR			0x62
+#define WD_TRIGGER_IOADR		0x66
+#define GPIO_COMMUNITY0_PORT_ID		0xaf
+#define PAD_CFG_DW0_GPP_A_23		0x4b8
+#define SAFE_EN_N_427E			0x01
+#define SAFE_EN_N_227E			0x04
+#define WD_ENABLED			0x01
+#define WD_TRIGGERED			0x80
+#define WD_MACROMODE			0x02
+
+#define TIMEOUT_MIN	2
+#define TIMEOUT_DEF	64
+#define TIMEOUT_MAX	64
+
+#define GP_STATUS_REG_227E	0x404D	/* IO PORT for SAFE_EN_N on 227E */
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0000);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static struct resource gp_status_reg_227e_res =
+	DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1, KBUILD_MODNAME);
+
+static struct resource io_resource_enable =
+	DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1,
+			    KBUILD_MODNAME " WD_ENABLE_IOADR");
+
+static struct resource io_resource_trigger =
+	DEFINE_RES_IO_NAMED(WD_TRIGGER_IOADR, SZ_1,
+			    KBUILD_MODNAME " WD_TRIGGER_IOADR");
+
+/* the actual start will be discovered with pci, 0 is a placeholder */
+static struct resource mem_resource =
+	DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
+
+static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
+static void __iomem *wd_reset_base_addr;
+
+static int wd_start(struct watchdog_device *wdd)
+{
+	outb(inb(WD_ENABLE_IOADR) | WD_ENABLED, WD_ENABLE_IOADR);
+	return 0;
+}
+
+static int wd_stop(struct watchdog_device *wdd)
+{
+	outb(inb(WD_ENABLE_IOADR) & ~WD_ENABLED, WD_ENABLE_IOADR);
+	return 0;
+}
+
+static int wd_ping(struct watchdog_device *wdd)
+{
+	inb(WD_TRIGGER_IOADR);
+	return 0;
+}
+
+static int wd_set_timeout(struct watchdog_device *wdd, unsigned int t)
+{
+	int timeout_idx = find_closest(t, wd_timeout_table,
+				       ARRAY_SIZE(wd_timeout_table));
+
+	outb((inb(WD_ENABLE_IOADR) & 0xc7) | timeout_idx << 3, WD_ENABLE_IOADR);
+	wdd->timeout = wd_timeout_table[timeout_idx];
+	return 0;
+}
+
+static const struct watchdog_info wdt_ident = {
+	.options	= WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
+			  WDIOF_SETTIMEOUT,
+	.identity	= KBUILD_MODNAME,
+};
+
+static const struct watchdog_ops wdt_ops = {
+	.owner		= THIS_MODULE,
+	.start		= wd_start,
+	.stop		= wd_stop,
+	.ping		= wd_ping,
+	.set_timeout	= wd_set_timeout,
+};
+
+static void wd_secondary_enable(u32 wdtmode)
+{
+	u16 resetbit;
+
+	/* set safe_en_n so we are not just WDIOF_ALARMONLY */
+	if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
+		/* enable SAFE_EN_N on GP_STATUS_REG_227E */
+		resetbit = inb(GP_STATUS_REG_227E);
+		outb(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E);
+	} else {
+		/* enable SAFE_EN_N on PCH D1600 */
+		resetbit = ioread16(wd_reset_base_addr);
+		iowrite16(resetbit & ~SAFE_EN_N_427E, wd_reset_base_addr);
+	}
+}
+
+static int wd_setup(u32 wdtmode)
+{
+	unsigned int bootstatus = 0;
+	int timeout_idx;
+
+	timeout_idx = find_closest(TIMEOUT_DEF, wd_timeout_table,
+				   ARRAY_SIZE(wd_timeout_table));
+
+	if (inb(WD_ENABLE_IOADR) & WD_TRIGGERED)
+		bootstatus |= WDIOF_CARDRESET;
+
+	/* reset alarm bit, set macro mode, and set timeout */
+	outb(WD_TRIGGERED | WD_MACROMODE | timeout_idx << 3, WD_ENABLE_IOADR);
+
+	wd_secondary_enable(wdtmode);
+
+	return bootstatus;
+}
+
+static struct watchdog_device wdd_data = {
+	.info = &wdt_ident,
+	.ops = &wdt_ops,
+	.min_timeout = TIMEOUT_MIN,
+	.max_timeout = TIMEOUT_MAX
+};
+
+static int simatic_ipc_wdt_probe(struct platform_device *pdev)
+{
+	struct simatic_ipc_platform *plat = pdev->dev.platform_data;
+	struct device *dev = &pdev->dev;
+	struct resource *res;
+
+	switch (plat->devmode) {
+	case SIMATIC_IPC_DEVICE_227E:
+		if (!devm_request_region(dev, gp_status_reg_227e_res.start,
+					 resource_size(&gp_status_reg_227e_res),
+					 KBUILD_MODNAME)) {
+			dev_err(dev,
+				"Unable to register IO resource at %pR\n",
+				&gp_status_reg_227e_res);
+			return -EBUSY;
+		}
+		fallthrough;
+	case SIMATIC_IPC_DEVICE_427E:
+		wdd_data.parent = dev;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	if (!devm_request_region(dev, io_resource_enable.start,
+				 resource_size(&io_resource_enable),
+				 io_resource_enable.name)) {
+		dev_err(dev,
+			"Unable to register IO resource at %#x\n",
+			WD_ENABLE_IOADR);
+		return -EBUSY;
+	}
+
+	if (!devm_request_region(dev, io_resource_trigger.start,
+				 resource_size(&io_resource_trigger),
+				 io_resource_trigger.name)) {
+		dev_err(dev,
+			"Unable to register IO resource at %#x\n",
+			WD_TRIGGER_IOADR);
+		return -EBUSY;
+	}
+
+	if (plat->devmode == SIMATIC_IPC_DEVICE_427E) {
+		res = &mem_resource;
+
+		/* get GPIO base from PCI */
+		res->start = simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1));
+		if (res->start == 0)
+			return -ENODEV;
+
+		/* do the final address calculation */
+		res->start = res->start + (GPIO_COMMUNITY0_PORT_ID << 16) +
+			     PAD_CFG_DW0_GPP_A_23;
+		res->end += res->start;
+
+		wd_reset_base_addr = devm_ioremap_resource(dev, res);
+		if (IS_ERR(wd_reset_base_addr))
+			return PTR_ERR(wd_reset_base_addr);
+	}
+
+	wdd_data.bootstatus = wd_setup(plat->devmode);
+	if (wdd_data.bootstatus)
+		dev_warn(dev, "last reboot caused by watchdog reset\n");
+
+	watchdog_set_nowayout(&wdd_data, nowayout);
+	watchdog_stop_on_reboot(&wdd_data);
+	return devm_watchdog_register_device(dev, &wdd_data);
+}
+
+static struct platform_driver simatic_ipc_wdt_driver = {
+	.probe = simatic_ipc_wdt_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+module_platform_driver(simatic_ipc_wdt_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
-- 
2.32.0

[PATCH v5 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs

[Henning Schild]

Siemens industrial PCs unfortunately can not always be properly
identified the way we used to. An earlier commit introduced code that
allows proper identification without looking at DMI strings that could
differ based on product branding.
Switch over to that proper way and revert commits that used to collect
the machines based on unstable strings.

Fixes: 648e921888ad ("clk: x86: Stop marking clocks as CLK_IS_CRITICAL")
Fixes: e8796c6c69d1 ("platform/x86: pmc_atom: Add Siemens CONNECT ...")
Fixes: f110d252ae79 ("platform/x86: pmc_atom: Add Siemens SIMATIC ...")
Fixes: ad0d315b4d4e ("platform/x86: pmc_atom: Add Siemens SIMATIC ...")
Tested-by: Michael Haener <michael.haener@siemens.com>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/platform/x86/pmc_atom.c | 54 ++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index a9d2a4b98e57..a40fae6edc84 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -13,6 +13,7 @@
 #include <linux/io.h>
 #include <linux/platform_data/x86/clk-pmc-atom.h>
 #include <linux/platform_data/x86/pmc_atom.h>
+#include <linux/platform_data/x86/simatic-ipc.h>
 #include <linux/platform_device.h>
 #include <linux/pci.h>
 #include <linux/seq_file.h>
@@ -362,6 +363,30 @@ static void pmc_dbgfs_register(struct pmc_dev *pmc)
 }
 #endif /* CONFIG_DEBUG_FS */
 
+static bool pmc_clk_is_critical = true;
+
+static int dmi_callback(const struct dmi_system_id *d)
+{
+	pr_info("%s critclks quirk enabled\n", d->ident);
+
+	return 1;
+}
+
+static int dmi_callback_siemens(const struct dmi_system_id *d)
+{
+	u32 st_id;
+
+	if (dmi_walk(simatic_ipc_find_dmi_entry_helper, &st_id))
+		goto out;
+
+	if (st_id == SIMATIC_IPC_IPC227E || st_id == SIMATIC_IPC_IPC277E)
+		return dmi_callback(d);
+
+out:
+	pmc_clk_is_critical = false;
+	return 1;
+}
+
 /*
  * Some systems need one or more of their pmc_plt_clks to be
  * marked as critical.
@@ -370,6 +395,7 @@ static const struct dmi_system_id critclk_systems[] = {
 	{
 		/* pmc_plt_clk0 is used for an external HSIC USB HUB */
 		.ident = "MPL CEC1x",
+		.callback = dmi_callback,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "MPL AG"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "CEC10 Family"),
@@ -378,6 +404,7 @@ static const struct dmi_system_id critclk_systems[] = {
 	{
 		/* pmc_plt_clk0 - 3 are used for the 4 ethernet controllers */
 		.ident = "Lex 3I380D",
+		.callback = dmi_callback,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Lex BayTrail"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "3I380D"),
@@ -386,6 +413,7 @@ static const struct dmi_system_id critclk_systems[] = {
 	{
 		/* pmc_plt_clk* - are used for ethernet controllers */
 		.ident = "Lex 2I385SW",
+		.callback = dmi_callback,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Lex BayTrail"),
 			DMI_MATCH(DMI_PRODUCT_NAME, "2I385SW"),
@@ -394,30 +422,17 @@ static const struct dmi_system_id critclk_systems[] = {
 	{
 		/* pmc_plt_clk* - are used for ethernet controllers */
 		.ident = "Beckhoff Baytrail",
+		.callback = dmi_callback,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
 			DMI_MATCH(DMI_PRODUCT_FAMILY, "CBxx63"),
 		},
 	},
 	{
-		.ident = "SIMATIC IPC227E",
+		.ident = "SIEMENS AG",
+		.callback = dmi_callback_siemens,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "6ES7647-8B"),
-		},
-	},
-	{
-		.ident = "SIMATIC IPC277E",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "6AV7882-0"),
-		},
-	},
-	{
-		.ident = "CONNECT X300",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "A5E45074588"),
 		},
 	},
 
@@ -429,7 +444,6 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
 {
 	struct platform_device *clkdev;
 	struct pmc_clk_data *clk_data;
-	const struct dmi_system_id *d = dmi_first_match(critclk_systems);
 
 	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
 	if (!clk_data)
@@ -437,10 +451,8 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
 
 	clk_data->base = pmc_regmap; /* offset is added by client */
 	clk_data->clks = pmc_data->clks;
-	if (d) {
-		clk_data->critical = true;
-		pr_info("%s critclks quirk enabled\n", d->ident);
-	}
+	if (dmi_check_system(critclk_systems))
+		clk_data->critical = pmc_clk_is_critical;
 
 	clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom",
 					       PLATFORM_DEVID_NONE,
-- 
2.32.0

Re: [PATCH v5 0/4] add device drivers for Siemens Industrial PCs

[Henning Schild]

This is now the outcome of the phone call i had with Andy concerning
the use of pinctrl. I could lay out why it is not "that easy" and why
we do access GPIO memory not in a "general purpose" way.

Yes we have thousands of devices out there with missing ACPI entries
so pinctrl does not come up. But our firmware guys will not add them,
because they strictly only address security fixes and not functional
changes. (stable ...)
Adding new ACPI entries could have unforeseeable side-effects on
devices already at the end-user, where many (also proprietary low-level
SW components (OS/hypervisor)) could be in use.

I hope in coming generations we will not forget such entries again, in
fact it is not even clear who forgot to put them in. Might even have
been Intel to begin with.

Henning

Am Mon, 13 Dec 2021 13:04:58 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> changes since v4:
> - make everything around GPIO memory usage more verbose
>   - commit messages, FIXME in p1, cover-letter
> 
> changes since v3:
> 
> - fix io access width and region reservations
> - fix style in p1
> 
> changes since v2:
> 
> - remove "simatic-ipc" prefix from LED names
> - fix style issues found in v2, mainly LED driver
> - fix OEM specific dmi code, and remove magic numbers
> - more "simatic_ipc" name prefixing
> - improved pmc quirk code using callbacks
> 
> changes since v1:
> 
> - fixed lots of style issues found in v1
>   - (debug) printing
>   - header ordering
> - fixed license issues GPLv2 and SPDX in all files
> - module_platform_driver instead of __init __exit
> - wdt simplifications cleanup
> - lots of fixes in wdt driver, all that was found in v1
> - fixed dmi length in dmi helper
> - changed LED names to allowed ones
> - move led driver to simple/
> - switched pmc_atom to dmi callback with global variable
> 
> 
> This series adds support for watchdogs and leds of several x86 devices
> from Siemens.
> 
> It is structured with a platform driver that mainly does
> identification of the machines. It might trigger loading of the
> actual device drivers by attaching devices to the platform bus.
> 
> The identification is vendor specific, parsing a special binary DMI
> entry. The implementation of that platform identification is applied
> on pmc_atom clock quirks in the final patch.
> 
> It is all structured in a way that we can easily add more devices and
> more platform drivers later. Internally we have some more code for
> hardware monitoring, more leds, watchdogs etc. This will follow some
> day.
> 
> The LED as well as the watchdog drivers access GPIO memory directly.
> Using pinctrl is not possible because the machines lack ACPI entries
> for the pinctrl drivers. Updates to the ACPI tables are not expected.
> So we can rule out a conflict where two drivers would try and access
> that GPIO memory.
> So we do not use those pins as "general purpose" but as "Siemens
> purpose", after having identified the devices very clearly. 
> 
> Henning Schild (4):
>   platform/x86: simatic-ipc: add main driver for Siemens devices
>   leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
>   watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
>   platform/x86: pmc_atom: improve critclk_systems matching for Siemens
>     PCs
> 
>  drivers/leds/Kconfig                          |   3 +
>  drivers/leds/Makefile                         |   3 +
>  drivers/leds/simple/Kconfig                   |  11 +
>  drivers/leds/simple/Makefile                  |   2 +
>  drivers/leds/simple/simatic-ipc-leds.c        | 202 ++++++++++++++++
>  drivers/platform/x86/Kconfig                  |  12 +
>  drivers/platform/x86/Makefile                 |   3 +
>  drivers/platform/x86/pmc_atom.c               |  54 +++--
>  drivers/platform/x86/simatic-ipc.c            | 176 ++++++++++++++
>  drivers/watchdog/Kconfig                      |  11 +
>  drivers/watchdog/Makefile                     |   1 +
>  drivers/watchdog/simatic-ipc-wdt.c            | 228
> ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h      |
> 29 +++ include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
>  14 files changed, 786 insertions(+), 21 deletions(-)
>  create mode 100644 drivers/leds/simple/Kconfig
>  create mode 100644 drivers/leds/simple/Makefile
>  create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
>  create mode 100644 drivers/platform/x86/simatic-ipc.c
>  create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
>  create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
>  create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
> 

Re: [PATCH v5 0/4] add device drivers for Siemens Industrial PCs

[Andy Shevchenko]

On Mon, Dec 13, 2021 at 2:21 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> This is now the outcome of the phone call i had with Andy concerning
> the use of pinctrl. I could lay out why it is not "that easy" and why
> we do access GPIO memory not in a "general purpose" way.
>
> Yes we have thousands of devices out there with missing ACPI entries
> so pinctrl does not come up. But our firmware guys will not add them,
> because they strictly only address security fixes and not functional
> changes. (stable ...)
> Adding new ACPI entries could have unforeseeable side-effects on
> devices already at the end-user, where many (also proprietary low-level
> SW components (OS/hypervisor)) could be in use.
>
> I hope in coming generations we will not forget such entries again, in
> fact it is not even clear who forgot to put them in. Might even have
> been Intel to begin with.

Yep and on top of that we agreed that whenever the better solution
comes out (meaning P2SB generic way of handling whichever it will be),
Siemens would help in testing and will move their driver forward to
support that solution. With all that said,

Acked-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Am Mon, 13 Dec 2021 13:04:58 +0100
> schrieb Henning Schild <henning.schild@siemens.com>:
>
> > changes since v4:
> > - make everything around GPIO memory usage more verbose
> >   - commit messages, FIXME in p1, cover-letter
> >
> > changes since v3:
> >
> > - fix io access width and region reservations
> > - fix style in p1
> >
> > changes since v2:
> >
> > - remove "simatic-ipc" prefix from LED names
> > - fix style issues found in v2, mainly LED driver
> > - fix OEM specific dmi code, and remove magic numbers
> > - more "simatic_ipc" name prefixing
> > - improved pmc quirk code using callbacks
> >
> > changes since v1:
> >
> > - fixed lots of style issues found in v1
> >   - (debug) printing
> >   - header ordering
> > - fixed license issues GPLv2 and SPDX in all files
> > - module_platform_driver instead of __init __exit
> > - wdt simplifications cleanup
> > - lots of fixes in wdt driver, all that was found in v1
> > - fixed dmi length in dmi helper
> > - changed LED names to allowed ones
> > - move led driver to simple/
> > - switched pmc_atom to dmi callback with global variable
> >
> >
> > This series adds support for watchdogs and leds of several x86 devices
> > from Siemens.
> >
> > It is structured with a platform driver that mainly does
> > identification of the machines. It might trigger loading of the
> > actual device drivers by attaching devices to the platform bus.
> >
> > The identification is vendor specific, parsing a special binary DMI
> > entry. The implementation of that platform identification is applied
> > on pmc_atom clock quirks in the final patch.
> >
> > It is all structured in a way that we can easily add more devices and
> > more platform drivers later. Internally we have some more code for
> > hardware monitoring, more leds, watchdogs etc. This will follow some
> > day.
> >
> > The LED as well as the watchdog drivers access GPIO memory directly.
> > Using pinctrl is not possible because the machines lack ACPI entries
> > for the pinctrl drivers. Updates to the ACPI tables are not expected.
> > So we can rule out a conflict where two drivers would try and access
> > that GPIO memory.
> > So we do not use those pins as "general purpose" but as "Siemens
> > purpose", after having identified the devices very clearly.
> >
> > Henning Schild (4):
> >   platform/x86: simatic-ipc: add main driver for Siemens devices
> >   leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
> >   watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
> >   platform/x86: pmc_atom: improve critclk_systems matching for Siemens
> >     PCs
> >
> >  drivers/leds/Kconfig                          |   3 +
> >  drivers/leds/Makefile                         |   3 +
> >  drivers/leds/simple/Kconfig                   |  11 +
> >  drivers/leds/simple/Makefile                  |   2 +
> >  drivers/leds/simple/simatic-ipc-leds.c        | 202 ++++++++++++++++
> >  drivers/platform/x86/Kconfig                  |  12 +
> >  drivers/platform/x86/Makefile                 |   3 +
> >  drivers/platform/x86/pmc_atom.c               |  54 +++--
> >  drivers/platform/x86/simatic-ipc.c            | 176 ++++++++++++++
> >  drivers/watchdog/Kconfig                      |  11 +
> >  drivers/watchdog/Makefile                     |   1 +
> >  drivers/watchdog/simatic-ipc-wdt.c            | 228
> > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h      |
> > 29 +++ include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
> >  14 files changed, 786 insertions(+), 21 deletions(-)
> >  create mode 100644 drivers/leds/simple/Kconfig
> >  create mode 100644 drivers/leds/simple/Makefile
> >  create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> >  create mode 100644 drivers/platform/x86/simatic-ipc.c
> >  create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> >  create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
> >  create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
> >
>


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 3922 bytes --]

On Mon 2021-12-13 13:05:00, Henning Schild wrote:
> This driver adds initial support for several devices from Siemens. It is
> based on a platform driver introduced in an earlier commit.
> 
> One of the supported machines has GPIO connected LEDs, here we poke GPIO
> memory directly because pinctrl does not come up.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>

Acked-by: Pavel Machek <pavel@ucw.cz>

> index c636ec069612..1a719caf14c0 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -105,3 +105,6 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= trigger/
>  
>  # LED Blink
>  obj-y					+= blink/
> +
> +# Simple LED drivers
> +obj-y					+= simple/
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> new file mode 100644
> index 000000000000..9f6a68336659
> --- /dev/null
> +++ b/drivers/leds/simple/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config LEDS_SIEMENS_SIMATIC_IPC
> +	tristate "LED driver for Siemens Simatic IPCs"
> +	depends on LEDS_CLASS
> +	depends on SIEMENS_SIMATIC_IPC
> +	help
> +	  This option enables support for the LEDs of several Industrial PCs
> +	  from Siemens.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called simatic-ipc-leds.
> diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
> new file mode 100644
> index 000000000000..8481f1e9e360
> --- /dev/null
> +++ b/drivers/leds/simple/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o
> diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
> new file mode 100644
> index 000000000000..ff2c96e73241
> --- /dev/null
> +++ b/drivers/leds/simple/simatic-ipc-leds.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Siemens SIMATIC IPC driver for LEDs
> + *
> + * Copyright (c) Siemens AG, 2018-2021
> + *
> + * Authors:
> + *  Henning Schild <henning.schild@siemens.com>
> + *  Jan Kiszka <jan.kiszka@siemens.com>
> + *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> + */
> +
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/spinlock.h>
> +
> +#define SIMATIC_IPC_LED_PORT_BASE	0x404E
> +
> +struct simatic_ipc_led {
> +	unsigned int value; /* mask for io and offset for mem */
> +	char *name;
> +	struct led_classdev cdev;
> +};
> +
> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
> +	{ }
> +};
> +
> +/* the actual start will be discovered with PCI, 0 is a placeholder */
> +struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
> +
> +static void *simatic_ipc_led_memory;
> +
> +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> +	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> +	{0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> +	{0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> +	{0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> +	{0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> +	{0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> +	{ }
> +};

Would it be possible to get some better naming for leds? status-1 to
status-3 is not quite useful.

Best regards,
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Hans de Goede]

Hi,

On 12/15/21 21:18, Pavel Machek wrote:
> On Mon 2021-12-13 13:05:00, Henning Schild wrote:
>> This driver adds initial support for several devices from Siemens. It is
>> based on a platform driver introduced in an earlier commit.
>>
>> One of the supported machines has GPIO connected LEDs, here we poke GPIO
>> memory directly because pinctrl does not come up.
>>
>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> 
> Acked-by: Pavel Machek <pavel@ucw.cz>

I see that this patch #includes linux/platform_data/x86/simatic-ipc-base.h
which gets added by patch 1/4.

Pavel, can I take this patch upstream through the pdx86 tree (with you Ack
added)? Or shall I prepare an immutable branch with patch 1 for you to
merge ?

Regards,

Hans


> 
>> index c636ec069612..1a719caf14c0 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -105,3 +105,6 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= trigger/
>>  
>>  # LED Blink
>>  obj-y					+= blink/
>> +
>> +# Simple LED drivers
>> +obj-y					+= simple/
>> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
>> new file mode 100644
>> index 000000000000..9f6a68336659
>> --- /dev/null
>> +++ b/drivers/leds/simple/Kconfig
>> @@ -0,0 +1,11 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config LEDS_SIEMENS_SIMATIC_IPC
>> +	tristate "LED driver for Siemens Simatic IPCs"
>> +	depends on LEDS_CLASS
>> +	depends on SIEMENS_SIMATIC_IPC
>> +	help
>> +	  This option enables support for the LEDs of several Industrial PCs
>> +	  from Siemens.
>> +
>> +	  To compile this driver as a module, choose M here: the module
>> +	  will be called simatic-ipc-leds.
>> diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
>> new file mode 100644
>> index 000000000000..8481f1e9e360
>> --- /dev/null
>> +++ b/drivers/leds/simple/Makefile
>> @@ -0,0 +1,2 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o
>> diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
>> new file mode 100644
>> index 000000000000..ff2c96e73241
>> --- /dev/null
>> +++ b/drivers/leds/simple/simatic-ipc-leds.c
>> @@ -0,0 +1,202 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Siemens SIMATIC IPC driver for LEDs
>> + *
>> + * Copyright (c) Siemens AG, 2018-2021
>> + *
>> + * Authors:
>> + *  Henning Schild <henning.schild@siemens.com>
>> + *  Jan Kiszka <jan.kiszka@siemens.com>
>> + *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
>> + */
>> +
>> +#include <linux/ioport.h>
>> +#include <linux/kernel.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_data/x86/simatic-ipc-base.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sizes.h>
>> +#include <linux/spinlock.h>
>> +
>> +#define SIMATIC_IPC_LED_PORT_BASE	0x404E
>> +
>> +struct simatic_ipc_led {
>> +	unsigned int value; /* mask for io and offset for mem */
>> +	char *name;
>> +	struct led_classdev cdev;
>> +};
>> +
>> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
>> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
>> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
>> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
>> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
>> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
>> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
>> +	{ }
>> +};
>> +
>> +/* the actual start will be discovered with PCI, 0 is a placeholder */
>> +struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
>> +
>> +static void *simatic_ipc_led_memory;
>> +
>> +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
>> +	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
>> +	{0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
>> +	{0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
>> +	{0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
>> +	{0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
>> +	{0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
>> +	{ }
>> +};
> 
> Would it be possible to get some better naming for leds? status-1 to
> status-3 is not quite useful.
> 
> Best regards,
> 								Pavel
> 

Re: [PATCH v5 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs

[Hans de Goede]

Hi Wim, Guenter,

On 12/13/21 13:05, Henning Schild wrote:
> This driver adds initial support for several devices from Siemens. It is
> based on a platform driver introduced in an earlier commit.
> 
> One of the supported machines does access a GPIO pin to enable the
> watchdog. Here we poke GPIO memory because pinctrl does not come up.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

I see that this patch #includes linux/platform_data/x86/simatic-ipc-base.h
which gets added by patch 1/4.

Since this has already been reviewed, can I take this patch upstream
through the pdx86 tree? Or shall I prepare an immutable branch with
patch 1 for you to merge ?

Regards,

Hans



> ---
>  drivers/watchdog/Kconfig           |  11 ++
>  drivers/watchdog/Makefile          |   1 +
>  drivers/watchdog/simatic-ipc-wdt.c | 228 +++++++++++++++++++++++++++++
>  3 files changed, 240 insertions(+)
>  create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 9d222ba17ec6..1dc86eb1361a 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1589,6 +1589,17 @@ config NIC7018_WDT
>  	  To compile this driver as a module, choose M here: the module will be
>  	  called nic7018_wdt.
>  
> +config SIEMENS_SIMATIC_IPC_WDT
> +	tristate "Siemens Simatic IPC Watchdog"
> +	depends on SIEMENS_SIMATIC_IPC
> +	select WATCHDOG_CORE
> +	help
> +	  This driver adds support for several watchdogs found in Industrial
> +	  PCs from Siemens.
> +
> +	  To compile this driver as a module, choose M here: the module will be
> +	  called simatic-ipc-wdt.
> +
>  # M68K Architecture
>  
>  config M54xx_WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 2ee97064145b..31b931846e32 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -143,6 +143,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
>  obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
>  obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
>  obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o
> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o
>  
>  # M68K Architecture
>  obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o
> diff --git a/drivers/watchdog/simatic-ipc-wdt.c b/drivers/watchdog/simatic-ipc-wdt.c
> new file mode 100644
> index 000000000000..8bac793c63fb
> --- /dev/null
> +++ b/drivers/watchdog/simatic-ipc-wdt.c
> @@ -0,0 +1,228 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Siemens SIMATIC IPC driver for Watchdogs
> + *
> + * Copyright (c) Siemens AG, 2020-2021
> + *
> + * Authors:
> + *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/util_macros.h>
> +#include <linux/watchdog.h>
> +
> +#define WD_ENABLE_IOADR			0x62
> +#define WD_TRIGGER_IOADR		0x66
> +#define GPIO_COMMUNITY0_PORT_ID		0xaf
> +#define PAD_CFG_DW0_GPP_A_23		0x4b8
> +#define SAFE_EN_N_427E			0x01
> +#define SAFE_EN_N_227E			0x04
> +#define WD_ENABLED			0x01
> +#define WD_TRIGGERED			0x80
> +#define WD_MACROMODE			0x02
> +
> +#define TIMEOUT_MIN	2
> +#define TIMEOUT_DEF	64
> +#define TIMEOUT_MAX	64
> +
> +#define GP_STATUS_REG_227E	0x404D	/* IO PORT for SAFE_EN_N on 227E */
> +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0000);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static struct resource gp_status_reg_227e_res =
> +	DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1, KBUILD_MODNAME);
> +
> +static struct resource io_resource_enable =
> +	DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1,
> +			    KBUILD_MODNAME " WD_ENABLE_IOADR");
> +
> +static struct resource io_resource_trigger =
> +	DEFINE_RES_IO_NAMED(WD_TRIGGER_IOADR, SZ_1,
> +			    KBUILD_MODNAME " WD_TRIGGER_IOADR");
> +
> +/* the actual start will be discovered with pci, 0 is a placeholder */
> +static struct resource mem_resource =
> +	DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
> +
> +static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
> +static void __iomem *wd_reset_base_addr;
> +
> +static int wd_start(struct watchdog_device *wdd)
> +{
> +	outb(inb(WD_ENABLE_IOADR) | WD_ENABLED, WD_ENABLE_IOADR);
> +	return 0;
> +}
> +
> +static int wd_stop(struct watchdog_device *wdd)
> +{
> +	outb(inb(WD_ENABLE_IOADR) & ~WD_ENABLED, WD_ENABLE_IOADR);
> +	return 0;
> +}
> +
> +static int wd_ping(struct watchdog_device *wdd)
> +{
> +	inb(WD_TRIGGER_IOADR);
> +	return 0;
> +}
> +
> +static int wd_set_timeout(struct watchdog_device *wdd, unsigned int t)
> +{
> +	int timeout_idx = find_closest(t, wd_timeout_table,
> +				       ARRAY_SIZE(wd_timeout_table));
> +
> +	outb((inb(WD_ENABLE_IOADR) & 0xc7) | timeout_idx << 3, WD_ENABLE_IOADR);
> +	wdd->timeout = wd_timeout_table[timeout_idx];
> +	return 0;
> +}
> +
> +static const struct watchdog_info wdt_ident = {
> +	.options	= WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> +			  WDIOF_SETTIMEOUT,
> +	.identity	= KBUILD_MODNAME,
> +};
> +
> +static const struct watchdog_ops wdt_ops = {
> +	.owner		= THIS_MODULE,
> +	.start		= wd_start,
> +	.stop		= wd_stop,
> +	.ping		= wd_ping,
> +	.set_timeout	= wd_set_timeout,
> +};
> +
> +static void wd_secondary_enable(u32 wdtmode)
> +{
> +	u16 resetbit;
> +
> +	/* set safe_en_n so we are not just WDIOF_ALARMONLY */
> +	if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
> +		/* enable SAFE_EN_N on GP_STATUS_REG_227E */
> +		resetbit = inb(GP_STATUS_REG_227E);
> +		outb(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E);
> +	} else {
> +		/* enable SAFE_EN_N on PCH D1600 */
> +		resetbit = ioread16(wd_reset_base_addr);
> +		iowrite16(resetbit & ~SAFE_EN_N_427E, wd_reset_base_addr);
> +	}
> +}
> +
> +static int wd_setup(u32 wdtmode)
> +{
> +	unsigned int bootstatus = 0;
> +	int timeout_idx;
> +
> +	timeout_idx = find_closest(TIMEOUT_DEF, wd_timeout_table,
> +				   ARRAY_SIZE(wd_timeout_table));
> +
> +	if (inb(WD_ENABLE_IOADR) & WD_TRIGGERED)
> +		bootstatus |= WDIOF_CARDRESET;
> +
> +	/* reset alarm bit, set macro mode, and set timeout */
> +	outb(WD_TRIGGERED | WD_MACROMODE | timeout_idx << 3, WD_ENABLE_IOADR);
> +
> +	wd_secondary_enable(wdtmode);
> +
> +	return bootstatus;
> +}
> +
> +static struct watchdog_device wdd_data = {
> +	.info = &wdt_ident,
> +	.ops = &wdt_ops,
> +	.min_timeout = TIMEOUT_MIN,
> +	.max_timeout = TIMEOUT_MAX
> +};
> +
> +static int simatic_ipc_wdt_probe(struct platform_device *pdev)
> +{
> +	struct simatic_ipc_platform *plat = pdev->dev.platform_data;
> +	struct device *dev = &pdev->dev;
> +	struct resource *res;
> +
> +	switch (plat->devmode) {
> +	case SIMATIC_IPC_DEVICE_227E:
> +		if (!devm_request_region(dev, gp_status_reg_227e_res.start,
> +					 resource_size(&gp_status_reg_227e_res),
> +					 KBUILD_MODNAME)) {
> +			dev_err(dev,
> +				"Unable to register IO resource at %pR\n",
> +				&gp_status_reg_227e_res);
> +			return -EBUSY;
> +		}
> +		fallthrough;
> +	case SIMATIC_IPC_DEVICE_427E:
> +		wdd_data.parent = dev;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (!devm_request_region(dev, io_resource_enable.start,
> +				 resource_size(&io_resource_enable),
> +				 io_resource_enable.name)) {
> +		dev_err(dev,
> +			"Unable to register IO resource at %#x\n",
> +			WD_ENABLE_IOADR);
> +		return -EBUSY;
> +	}
> +
> +	if (!devm_request_region(dev, io_resource_trigger.start,
> +				 resource_size(&io_resource_trigger),
> +				 io_resource_trigger.name)) {
> +		dev_err(dev,
> +			"Unable to register IO resource at %#x\n",
> +			WD_TRIGGER_IOADR);
> +		return -EBUSY;
> +	}
> +
> +	if (plat->devmode == SIMATIC_IPC_DEVICE_427E) {
> +		res = &mem_resource;
> +
> +		/* get GPIO base from PCI */
> +		res->start = simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1));
> +		if (res->start == 0)
> +			return -ENODEV;
> +
> +		/* do the final address calculation */
> +		res->start = res->start + (GPIO_COMMUNITY0_PORT_ID << 16) +
> +			     PAD_CFG_DW0_GPP_A_23;
> +		res->end += res->start;
> +
> +		wd_reset_base_addr = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(wd_reset_base_addr))
> +			return PTR_ERR(wd_reset_base_addr);
> +	}
> +
> +	wdd_data.bootstatus = wd_setup(plat->devmode);
> +	if (wdd_data.bootstatus)
> +		dev_warn(dev, "last reboot caused by watchdog reset\n");
> +
> +	watchdog_set_nowayout(&wdd_data, nowayout);
> +	watchdog_stop_on_reboot(&wdd_data);
> +	return devm_watchdog_register_device(dev, &wdd_data);
> +}
> +
> +static struct platform_driver simatic_ipc_wdt_driver = {
> +	.probe = simatic_ipc_wdt_probe,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +	},
> +};
> +
> +module_platform_driver(simatic_ipc_wdt_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
> 

Re: [PATCH v5 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs

[Guenter Roeck]

On 12/15/21 12:56 PM, Hans de Goede wrote:
> Hi Wim, Guenter,
> 
> On 12/13/21 13:05, Henning Schild wrote:
>> This driver adds initial support for several devices from Siemens. It is
>> based on a platform driver introduced in an earlier commit.
>>
>> One of the supported machines does access a GPIO pin to enable the
>> watchdog. Here we poke GPIO memory because pinctrl does not come up.
>>
>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>> Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> 
> I see that this patch #includes linux/platform_data/x86/simatic-ipc-base.h
> which gets added by patch 1/4.
> 
> Since this has already been reviewed, can I take this patch upstream
> through the pdx86 tree? Or shall I prepare an immutable branch with

Please go ahead and take the entire series.

Thanks,
Guenter

> patch 1 for you to merge ?
> 
> Regards,
> 
> Hans
> 
> 
> 
>> ---
>>   drivers/watchdog/Kconfig           |  11 ++
>>   drivers/watchdog/Makefile          |   1 +
>>   drivers/watchdog/simatic-ipc-wdt.c | 228 +++++++++++++++++++++++++++++
>>   3 files changed, 240 insertions(+)
>>   create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 9d222ba17ec6..1dc86eb1361a 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1589,6 +1589,17 @@ config NIC7018_WDT
>>   	  To compile this driver as a module, choose M here: the module will be
>>   	  called nic7018_wdt.
>>   
>> +config SIEMENS_SIMATIC_IPC_WDT
>> +	tristate "Siemens Simatic IPC Watchdog"
>> +	depends on SIEMENS_SIMATIC_IPC
>> +	select WATCHDOG_CORE
>> +	help
>> +	  This driver adds support for several watchdogs found in Industrial
>> +	  PCs from Siemens.
>> +
>> +	  To compile this driver as a module, choose M here: the module will be
>> +	  called simatic-ipc-wdt.
>> +
>>   # M68K Architecture
>>   
>>   config M54xx_WATCHDOG
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 2ee97064145b..31b931846e32 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -143,6 +143,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
>>   obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
>>   obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
>>   obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o
>> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o
>>   
>>   # M68K Architecture
>>   obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o
>> diff --git a/drivers/watchdog/simatic-ipc-wdt.c b/drivers/watchdog/simatic-ipc-wdt.c
>> new file mode 100644
>> index 000000000000..8bac793c63fb
>> --- /dev/null
>> +++ b/drivers/watchdog/simatic-ipc-wdt.c
>> @@ -0,0 +1,228 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Siemens SIMATIC IPC driver for Watchdogs
>> + *
>> + * Copyright (c) Siemens AG, 2020-2021
>> + *
>> + * Authors:
>> + *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
>> + */
>> +
>> +#include <linux/device.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/io.h>
>> +#include <linux/ioport.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_data/x86/simatic-ipc-base.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sizes.h>
>> +#include <linux/util_macros.h>
>> +#include <linux/watchdog.h>
>> +
>> +#define WD_ENABLE_IOADR			0x62
>> +#define WD_TRIGGER_IOADR		0x66
>> +#define GPIO_COMMUNITY0_PORT_ID		0xaf
>> +#define PAD_CFG_DW0_GPP_A_23		0x4b8
>> +#define SAFE_EN_N_427E			0x01
>> +#define SAFE_EN_N_227E			0x04
>> +#define WD_ENABLED			0x01
>> +#define WD_TRIGGERED			0x80
>> +#define WD_MACROMODE			0x02
>> +
>> +#define TIMEOUT_MIN	2
>> +#define TIMEOUT_DEF	64
>> +#define TIMEOUT_MAX	64
>> +
>> +#define GP_STATUS_REG_227E	0x404D	/* IO PORT for SAFE_EN_N on 227E */
>> +
>> +static bool nowayout = WATCHDOG_NOWAYOUT;
>> +module_param(nowayout, bool, 0000);
>> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>> +		 __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
>> +
>> +static struct resource gp_status_reg_227e_res =
>> +	DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1, KBUILD_MODNAME);
>> +
>> +static struct resource io_resource_enable =
>> +	DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1,
>> +			    KBUILD_MODNAME " WD_ENABLE_IOADR");
>> +
>> +static struct resource io_resource_trigger =
>> +	DEFINE_RES_IO_NAMED(WD_TRIGGER_IOADR, SZ_1,
>> +			    KBUILD_MODNAME " WD_TRIGGER_IOADR");
>> +
>> +/* the actual start will be discovered with pci, 0 is a placeholder */
>> +static struct resource mem_resource =
>> +	DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
>> +
>> +static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
>> +static void __iomem *wd_reset_base_addr;
>> +
>> +static int wd_start(struct watchdog_device *wdd)
>> +{
>> +	outb(inb(WD_ENABLE_IOADR) | WD_ENABLED, WD_ENABLE_IOADR);
>> +	return 0;
>> +}
>> +
>> +static int wd_stop(struct watchdog_device *wdd)
>> +{
>> +	outb(inb(WD_ENABLE_IOADR) & ~WD_ENABLED, WD_ENABLE_IOADR);
>> +	return 0;
>> +}
>> +
>> +static int wd_ping(struct watchdog_device *wdd)
>> +{
>> +	inb(WD_TRIGGER_IOADR);
>> +	return 0;
>> +}
>> +
>> +static int wd_set_timeout(struct watchdog_device *wdd, unsigned int t)
>> +{
>> +	int timeout_idx = find_closest(t, wd_timeout_table,
>> +				       ARRAY_SIZE(wd_timeout_table));
>> +
>> +	outb((inb(WD_ENABLE_IOADR) & 0xc7) | timeout_idx << 3, WD_ENABLE_IOADR);
>> +	wdd->timeout = wd_timeout_table[timeout_idx];
>> +	return 0;
>> +}
>> +
>> +static const struct watchdog_info wdt_ident = {
>> +	.options	= WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
>> +			  WDIOF_SETTIMEOUT,
>> +	.identity	= KBUILD_MODNAME,
>> +};
>> +
>> +static const struct watchdog_ops wdt_ops = {
>> +	.owner		= THIS_MODULE,
>> +	.start		= wd_start,
>> +	.stop		= wd_stop,
>> +	.ping		= wd_ping,
>> +	.set_timeout	= wd_set_timeout,
>> +};
>> +
>> +static void wd_secondary_enable(u32 wdtmode)
>> +{
>> +	u16 resetbit;
>> +
>> +	/* set safe_en_n so we are not just WDIOF_ALARMONLY */
>> +	if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
>> +		/* enable SAFE_EN_N on GP_STATUS_REG_227E */
>> +		resetbit = inb(GP_STATUS_REG_227E);
>> +		outb(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E);
>> +	} else {
>> +		/* enable SAFE_EN_N on PCH D1600 */
>> +		resetbit = ioread16(wd_reset_base_addr);
>> +		iowrite16(resetbit & ~SAFE_EN_N_427E, wd_reset_base_addr);
>> +	}
>> +}
>> +
>> +static int wd_setup(u32 wdtmode)
>> +{
>> +	unsigned int bootstatus = 0;
>> +	int timeout_idx;
>> +
>> +	timeout_idx = find_closest(TIMEOUT_DEF, wd_timeout_table,
>> +				   ARRAY_SIZE(wd_timeout_table));
>> +
>> +	if (inb(WD_ENABLE_IOADR) & WD_TRIGGERED)
>> +		bootstatus |= WDIOF_CARDRESET;
>> +
>> +	/* reset alarm bit, set macro mode, and set timeout */
>> +	outb(WD_TRIGGERED | WD_MACROMODE | timeout_idx << 3, WD_ENABLE_IOADR);
>> +
>> +	wd_secondary_enable(wdtmode);
>> +
>> +	return bootstatus;
>> +}
>> +
>> +static struct watchdog_device wdd_data = {
>> +	.info = &wdt_ident,
>> +	.ops = &wdt_ops,
>> +	.min_timeout = TIMEOUT_MIN,
>> +	.max_timeout = TIMEOUT_MAX
>> +};
>> +
>> +static int simatic_ipc_wdt_probe(struct platform_device *pdev)
>> +{
>> +	struct simatic_ipc_platform *plat = pdev->dev.platform_data;
>> +	struct device *dev = &pdev->dev;
>> +	struct resource *res;
>> +
>> +	switch (plat->devmode) {
>> +	case SIMATIC_IPC_DEVICE_227E:
>> +		if (!devm_request_region(dev, gp_status_reg_227e_res.start,
>> +					 resource_size(&gp_status_reg_227e_res),
>> +					 KBUILD_MODNAME)) {
>> +			dev_err(dev,
>> +				"Unable to register IO resource at %pR\n",
>> +				&gp_status_reg_227e_res);
>> +			return -EBUSY;
>> +		}
>> +		fallthrough;
>> +	case SIMATIC_IPC_DEVICE_427E:
>> +		wdd_data.parent = dev;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	if (!devm_request_region(dev, io_resource_enable.start,
>> +				 resource_size(&io_resource_enable),
>> +				 io_resource_enable.name)) {
>> +		dev_err(dev,
>> +			"Unable to register IO resource at %#x\n",
>> +			WD_ENABLE_IOADR);
>> +		return -EBUSY;
>> +	}
>> +
>> +	if (!devm_request_region(dev, io_resource_trigger.start,
>> +				 resource_size(&io_resource_trigger),
>> +				 io_resource_trigger.name)) {
>> +		dev_err(dev,
>> +			"Unable to register IO resource at %#x\n",
>> +			WD_TRIGGER_IOADR);
>> +		return -EBUSY;
>> +	}
>> +
>> +	if (plat->devmode == SIMATIC_IPC_DEVICE_427E) {
>> +		res = &mem_resource;
>> +
>> +		/* get GPIO base from PCI */
>> +		res->start = simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1));
>> +		if (res->start == 0)
>> +			return -ENODEV;
>> +
>> +		/* do the final address calculation */
>> +		res->start = res->start + (GPIO_COMMUNITY0_PORT_ID << 16) +
>> +			     PAD_CFG_DW0_GPP_A_23;
>> +		res->end += res->start;
>> +
>> +		wd_reset_base_addr = devm_ioremap_resource(dev, res);
>> +		if (IS_ERR(wd_reset_base_addr))
>> +			return PTR_ERR(wd_reset_base_addr);
>> +	}
>> +
>> +	wdd_data.bootstatus = wd_setup(plat->devmode);
>> +	if (wdd_data.bootstatus)
>> +		dev_warn(dev, "last reboot caused by watchdog reset\n");
>> +
>> +	watchdog_set_nowayout(&wdd_data, nowayout);
>> +	watchdog_stop_on_reboot(&wdd_data);
>> +	return devm_watchdog_register_device(dev, &wdd_data);
>> +}
>> +
>> +static struct platform_driver simatic_ipc_wdt_driver = {
>> +	.probe = simatic_ipc_wdt_probe,
>> +	.driver = {
>> +		.name = KBUILD_MODNAME,
>> +	},
>> +};
>> +
>> +module_platform_driver(simatic_ipc_wdt_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
>> +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
>>
> 

Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1406 bytes --]

On Wed 2021-12-15 21:53:56, Hans de Goede wrote:
> Hi,
> 
> On 12/15/21 21:18, Pavel Machek wrote:
> > On Mon 2021-12-13 13:05:00, Henning Schild wrote:
> >> This driver adds initial support for several devices from Siemens. It is
> >> based on a platform driver introduced in an earlier commit.
> >>
> >> One of the supported machines has GPIO connected LEDs, here we poke GPIO
> >> memory directly because pinctrl does not come up.
> >>
> >> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > 
> > Acked-by: Pavel Machek <pavel@ucw.cz>
> 
> I see that this patch #includes linux/platform_data/x86/simatic-ipc-base.h
> which gets added by patch 1/4.
> 
> Pavel, can I take this patch upstream through the pdx86 tree (with you Ack
> added)? Or shall I prepare an immutable branch with patch 1 for you to
> merge ?

Yes, you can.


> >> +
> >> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> >> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> >> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
> >> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> >> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
> >> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> >> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
> >> +	{ }
> >> +};

But I'd still like better naming than red:status-2.
								Pavel
-- 
http://www.livejournal.com/~pavelmachek

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Hans de Goede]

Hi,

On 12/19/21 17:49, Pavel Machek wrote:
> On Wed 2021-12-15 21:53:56, Hans de Goede wrote:
>> Hi,
>>
>> On 12/15/21 21:18, Pavel Machek wrote:
>>> On Mon 2021-12-13 13:05:00, Henning Schild wrote:
>>>> This driver adds initial support for several devices from Siemens. It is
>>>> based on a platform driver introduced in an earlier commit.
>>>>
>>>> One of the supported machines has GPIO connected LEDs, here we poke GPIO
>>>> memory directly because pinctrl does not come up.
>>>>
>>>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>>>
>>> Acked-by: Pavel Machek <pavel@ucw.cz>
>>
>> I see that this patch #includes linux/platform_data/x86/simatic-ipc-base.h
>> which gets added by patch 1/4.
>>
>> Pavel, can I take this patch upstream through the pdx86 tree (with you Ack
>> added)? Or shall I prepare an immutable branch with patch 1 for you to
>> merge ?
> 
> Yes, you can.
> 
> 
>>>> +
>>>> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
>>>> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
>>>> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
>>>> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
>>>> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
>>>> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
>>>> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
>>>> +	{ }
>>>> +};
> 
> But I'd still like better naming than red:status-2.

Hmm, if I merge this and no patch setting a better name is merged
before 5.17 final, then we are stuck with this.

OTOH I think that given the use of this driver, where AFAIK the
use of the LEDs in the field varies, I think this might be the
best name we can come up with.

Regards,

Hans

Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Henning Schild]

Am Sun, 19 Dec 2021 17:49:03 +0100
schrieb Pavel Machek <pavel@ucw.cz>:

> On Wed 2021-12-15 21:53:56, Hans de Goede wrote:
> > Hi,
> > 
> > On 12/15/21 21:18, Pavel Machek wrote:  
> > > On Mon 2021-12-13 13:05:00, Henning Schild wrote:  
> > >> This driver adds initial support for several devices from
> > >> Siemens. It is based on a platform driver introduced in an
> > >> earlier commit.
> > >>
> > >> One of the supported machines has GPIO connected LEDs, here we
> > >> poke GPIO memory directly because pinctrl does not come up.
> > >>
> > >> Signed-off-by: Henning Schild <henning.schild@siemens.com>  
> > > 
> > > Acked-by: Pavel Machek <pavel@ucw.cz>  
> > 
> > I see that this patch #includes
> > linux/platform_data/x86/simatic-ipc-base.h which gets added by
> > patch 1/4.
> > 
> > Pavel, can I take this patch upstream through the pdx86 tree (with
> > you Ack added)? Or shall I prepare an immutable branch with patch 1
> > for you to merge ?  
> 
> Yes, you can.
> 
> 
> > >> +
> > >> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > >> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> > >> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
> > >> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> > >> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
> > >> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> > >> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
> > >> +	{ }
> > >> +};  
> 
> But I'd still like better naming than red:status-2.

We had the name discussion already several times, and i have to admit i
am not too happy either.

But my impression was that this is an acceptable compromise. I am not
happy because the names lack scope, which i had in the first round with
"simatic-ipc:red:...".

Function is also a bit unclear, but with the numbers and the user
manual, or looking at the chassis it kind of adds up and should be
clear to users which is which.

But i agree with Hans that we should sort this out before merge. So
please say what makes you unhappy, maybe that can be fixed ... might
even make me happier about the names i feel i had to choose.

The LEDs are per definition of the manuals meant for users/applications
to signal whatever the use-case might want to signal. There are 3 of
them numbered 1-3 on the chassis, and next to the number can often (not
always) be found a string like "error", "maint", "run-stop"
So a function suggestion i would say.

I could envision to use "fault" or "alarm" instead of "status" for the
one labeled "error". And maybe "standby" for the one called "maint" but
i would really like to keep the numbers.

Which would look like

status-1
alarm-2
standby-3

But still i have to clue what those names stand for and choosing
and of those "undefined" names could just suggest things and break
expectations. Calling them all "status" is neutral ... 

Or can you explain the difference between "fault", "panic" and "alarm".
Ask 5 people and get at least 3 different expectations ... i guess.

Henning


> 								Pavel

Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Henning Schild]

Am Mon, 20 Dec 2021 08:53:55 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Sun, 19 Dec 2021 17:49:03 +0100
> schrieb Pavel Machek <pavel@ucw.cz>:
> 
> > On Wed 2021-12-15 21:53:56, Hans de Goede wrote:  
> > > Hi,
> > > 
> > > On 12/15/21 21:18, Pavel Machek wrote:    
> > > > On Mon 2021-12-13 13:05:00, Henning Schild wrote:    
> > > >> This driver adds initial support for several devices from
> > > >> Siemens. It is based on a platform driver introduced in an
> > > >> earlier commit.
> > > >>
> > > >> One of the supported machines has GPIO connected LEDs, here we
> > > >> poke GPIO memory directly because pinctrl does not come up.
> > > >>
> > > >> Signed-off-by: Henning Schild <henning.schild@siemens.com>    
> > > > 
> > > > Acked-by: Pavel Machek <pavel@ucw.cz>    
> > > 
> > > I see that this patch #includes
> > > linux/platform_data/x86/simatic-ipc-base.h which gets added by
> > > patch 1/4.
> > > 
> > > Pavel, can I take this patch upstream through the pdx86 tree (with
> > > you Ack added)? Or shall I prepare an immutable branch with patch
> > > 1 for you to merge ?    
> > 
> > Yes, you can.
> > 
> >   
> > > >> +
> > > >> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > >> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> > > >> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
> > > >> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> > > >> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
> > > >> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> > > >> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
> > > >> +	{ }
> > > >> +};    
> > 
> > But I'd still like better naming than red:status-2.  
> 
> We had the name discussion already several times, and i have to admit
> i am not too happy either.
> 
> But my impression was that this is an acceptable compromise. I am not
> happy because the names lack scope, which i had in the first round
> with "simatic-ipc:red:...".
> 
> Function is also a bit unclear, but with the numbers and the user
> manual, or looking at the chassis it kind of adds up and should be
> clear to users which is which.
> 
> But i agree with Hans that we should sort this out before merge. So
> please say what makes you unhappy, maybe that can be fixed ... might
> even make me happier about the names i feel i had to choose.
> 
> The LEDs are per definition of the manuals meant for
> users/applications to signal whatever the use-case might want to
> signal. There are 3 of them numbered 1-3 on the chassis, and next to
> the number can often (not always) be found a string like "error",
> "maint", "run-stop" So a function suggestion i would say.
> 
> I could envision to use "fault" or "alarm" instead of "status" for the
> one labeled "error". And maybe "standby" for the one called "maint"
> but i would really like to keep the numbers.
> 
> Which would look like
> 
> status-1
> alarm-2
> standby-3
> 
> But still i have to clue what those names stand for and choosing
> and of those "undefined" names could just suggest things and break
> expectations. Calling them all "status" is neutral ... 
> 
> Or can you explain the difference between "fault", "panic" and
> "alarm". Ask 5 people and get at least 3 different expectations ... i
> guess.

Long story short, i am also not happy but the current suggestion is the
most generic and least "expectation-creating" i could come up with.
While keeping a mapping between the name and the chassis/manual.

So i will stick with it, unless i get concrete suggestions on how to
improve.

The misc functions
https://elixir.bootlin.com/linux/latest/source/include/dt-bindings/leds/common.h#L63

do not seem usable. Without a set of conventions they are nothing but
"allowed but undefined strings". I could however introduce
FUNCTION_ERROR FUNCTION_MAINT FUNCTION_RUN_STOP ... add more such random
strings. Which would probably make me happy because it would create a
better mapping between the names and the chassis ... but it would
worsen the problem of "what are those misc functions anyway?"

Henning

> Henning
> 
> 
> > 								Pavel
> >  
> 

Re: [PATCH v5 0/4] add device drivers for Siemens Industrial PCs

[Hans de Goede]

Hi,

On 12/13/21 13:04, Henning Schild wrote:
> changes since v4:
> - make everything around GPIO memory usage more verbose
>   - commit messages, FIXME in p1, cover-letter
> 
> changes since v3:
> 
> - fix io access width and region reservations
> - fix style in p1
> 
> changes since v2:
> 
> - remove "simatic-ipc" prefix from LED names
> - fix style issues found in v2, mainly LED driver
> - fix OEM specific dmi code, and remove magic numbers
> - more "simatic_ipc" name prefixing
> - improved pmc quirk code using callbacks
> 
> changes since v1:
> 
> - fixed lots of style issues found in v1
>   - (debug) printing
>   - header ordering
> - fixed license issues GPLv2 and SPDX in all files
> - module_platform_driver instead of __init __exit
> - wdt simplifications cleanup
> - lots of fixes in wdt driver, all that was found in v1
> - fixed dmi length in dmi helper
> - changed LED names to allowed ones
> - move led driver to simple/
> - switched pmc_atom to dmi callback with global variable
> 
> 
> This series adds support for watchdogs and leds of several x86 devices
> from Siemens.
> 
> It is structured with a platform driver that mainly does identification
> of the machines. It might trigger loading of the actual device drivers
> by attaching devices to the platform bus.
> 
> The identification is vendor specific, parsing a special binary DMI
> entry. The implementation of that platform identification is applied on
> pmc_atom clock quirks in the final patch.
> 
> It is all structured in a way that we can easily add more devices and
> more platform drivers later. Internally we have some more code for
> hardware monitoring, more leds, watchdogs etc. This will follow some
> day.
> 
> The LED as well as the watchdog drivers access GPIO memory directly.
> Using pinctrl is not possible because the machines lack ACPI entries for
> the pinctrl drivers. Updates to the ACPI tables are not expected. So we
> can rule out a conflict where two drivers would try and access that GPIO
> memory.
> So we do not use those pins as "general purpose" but as "Siemens
> purpose", after having identified the devices very clearly. 
> 
> Henning Schild (4):
>   platform/x86: simatic-ipc: add main driver for Siemens devices
>   leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
>   watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
>   platform/x86: pmc_atom: improve critclk_systems matching for Siemens
>     PCs


Thank you for your patch-series, I've applied the series to my
review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans


> 
>  drivers/leds/Kconfig                          |   3 +
>  drivers/leds/Makefile                         |   3 +
>  drivers/leds/simple/Kconfig                   |  11 +
>  drivers/leds/simple/Makefile                  |   2 +
>  drivers/leds/simple/simatic-ipc-leds.c        | 202 ++++++++++++++++
>  drivers/platform/x86/Kconfig                  |  12 +
>  drivers/platform/x86/Makefile                 |   3 +
>  drivers/platform/x86/pmc_atom.c               |  54 +++--
>  drivers/platform/x86/simatic-ipc.c            | 176 ++++++++++++++
>  drivers/watchdog/Kconfig                      |  11 +
>  drivers/watchdog/Makefile                     |   1 +
>  drivers/watchdog/simatic-ipc-wdt.c            | 228 ++++++++++++++++++
>  .../platform_data/x86/simatic-ipc-base.h      |  29 +++
>  include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
>  14 files changed, 786 insertions(+), 21 deletions(-)
>  create mode 100644 drivers/leds/simple/Kconfig
>  create mode 100644 drivers/leds/simple/Makefile
>  create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
>  create mode 100644 drivers/platform/x86/simatic-ipc.c
>  create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
>  create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
>  create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
> 

Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Hans de Goede]

Hi,

On 12/20/21 09:14, Henning Schild wrote:
> Am Mon, 20 Dec 2021 08:53:55 +0100
> schrieb Henning Schild <henning.schild@siemens.com>:
> 
>> Am Sun, 19 Dec 2021 17:49:03 +0100
>> schrieb Pavel Machek <pavel@ucw.cz>:
>>
>>> On Wed 2021-12-15 21:53:56, Hans de Goede wrote:  
>>>> Hi,
>>>>
>>>> On 12/15/21 21:18, Pavel Machek wrote:    
>>>>> On Mon 2021-12-13 13:05:00, Henning Schild wrote:    
>>>>>> This driver adds initial support for several devices from
>>>>>> Siemens. It is based on a platform driver introduced in an
>>>>>> earlier commit.
>>>>>>
>>>>>> One of the supported machines has GPIO connected LEDs, here we
>>>>>> poke GPIO memory directly because pinctrl does not come up.
>>>>>>
>>>>>> Signed-off-by: Henning Schild <henning.schild@siemens.com>    
>>>>>
>>>>> Acked-by: Pavel Machek <pavel@ucw.cz>    
>>>>
>>>> I see that this patch #includes
>>>> linux/platform_data/x86/simatic-ipc-base.h which gets added by
>>>> patch 1/4.
>>>>
>>>> Pavel, can I take this patch upstream through the pdx86 tree (with
>>>> you Ack added)? Or shall I prepare an immutable branch with patch
>>>> 1 for you to merge ?    
>>>
>>> Yes, you can.
>>>
>>>   
>>>>>> +
>>>>>> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
>>>>>> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
>>>>>> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
>>>>>> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
>>>>>> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
>>>>>> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
>>>>>> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
>>>>>> +	{ }
>>>>>> +};    
>>>
>>> But I'd still like better naming than red:status-2.  
>>
>> We had the name discussion already several times, and i have to admit
>> i am not too happy either.
>>
>> But my impression was that this is an acceptable compromise. I am not
>> happy because the names lack scope, which i had in the first round
>> with "simatic-ipc:red:...".
>>
>> Function is also a bit unclear, but with the numbers and the user
>> manual, or looking at the chassis it kind of adds up and should be
>> clear to users which is which.
>>
>> But i agree with Hans that we should sort this out before merge. So
>> please say what makes you unhappy, maybe that can be fixed ... might
>> even make me happier about the names i feel i had to choose.
>>
>> The LEDs are per definition of the manuals meant for
>> users/applications to signal whatever the use-case might want to
>> signal. There are 3 of them numbered 1-3 on the chassis, and next to
>> the number can often (not always) be found a string like "error",
>> "maint", "run-stop" So a function suggestion i would say.
>>
>> I could envision to use "fault" or "alarm" instead of "status" for the
>> one labeled "error". And maybe "standby" for the one called "maint"
>> but i would really like to keep the numbers.
>>
>> Which would look like
>>
>> status-1
>> alarm-2
>> standby-3
>>
>> But still i have to clue what those names stand for and choosing
>> and of those "undefined" names could just suggest things and break
>> expectations. Calling them all "status" is neutral ... 
>>
>> Or can you explain the difference between "fault", "panic" and
>> "alarm". Ask 5 people and get at least 3 different expectations ... i
>> guess.
> 
> Long story short, i am also not happy but the current suggestion is the
> most generic and least "expectation-creating" i could come up with.
> While keeping a mapping between the name and the chassis/manual.
> 
> So i will stick with it, unless i get concrete suggestions on how to
> improve.

Ok, given the above I've gone ahead and merged this series.

I too believe the current status names are fine, but if someone disagrees,
they still have the entire 5.17 cycle to come up with something better.

Regards,

Hans

Re: [PATCH v5 0/4] add device drivers for Siemens Industrial PCs

[Henning Schild]

Am Thu, 23 Dec 2021 18:17:03 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 12/13/21 13:04, Henning Schild wrote:
> > changes since v4:
> > - make everything around GPIO memory usage more verbose
> >   - commit messages, FIXME in p1, cover-letter
> > 
> > changes since v3:
> > 
> > - fix io access width and region reservations
> > - fix style in p1
> > 
> > changes since v2:
> > 
> > - remove "simatic-ipc" prefix from LED names
> > - fix style issues found in v2, mainly LED driver
> > - fix OEM specific dmi code, and remove magic numbers
> > - more "simatic_ipc" name prefixing
> > - improved pmc quirk code using callbacks
> > 
> > changes since v1:
> > 
> > - fixed lots of style issues found in v1
> >   - (debug) printing
> >   - header ordering
> > - fixed license issues GPLv2 and SPDX in all files
> > - module_platform_driver instead of __init __exit
> > - wdt simplifications cleanup
> > - lots of fixes in wdt driver, all that was found in v1
> > - fixed dmi length in dmi helper
> > - changed LED names to allowed ones
> > - move led driver to simple/
> > - switched pmc_atom to dmi callback with global variable
> > 
> > 
> > This series adds support for watchdogs and leds of several x86
> > devices from Siemens.
> > 
> > It is structured with a platform driver that mainly does
> > identification of the machines. It might trigger loading of the
> > actual device drivers by attaching devices to the platform bus.
> > 
> > The identification is vendor specific, parsing a special binary DMI
> > entry. The implementation of that platform identification is
> > applied on pmc_atom clock quirks in the final patch.
> > 
> > It is all structured in a way that we can easily add more devices
> > and more platform drivers later. Internally we have some more code
> > for hardware monitoring, more leds, watchdogs etc. This will follow
> > some day.
> > 
> > The LED as well as the watchdog drivers access GPIO memory directly.
> > Using pinctrl is not possible because the machines lack ACPI
> > entries for the pinctrl drivers. Updates to the ACPI tables are not
> > expected. So we can rule out a conflict where two drivers would try
> > and access that GPIO memory.
> > So we do not use those pins as "general purpose" but as "Siemens
> > purpose", after having identified the devices very clearly. 
> > 
> > Henning Schild (4):
> >   platform/x86: simatic-ipc: add main driver for Siemens devices
> >   leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
> >   watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial
> > PCs platform/x86: pmc_atom: improve critclk_systems matching for
> > Siemens PCs  
> 
> 
> Thank you for your patch-series, I've applied the series to my
> review-hans branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans
> 
> Note it will show up in my review-hans branch once I've pushed my
> local branch there, which might take a while.
> 
> Once I've run some tests on this branch the patches there will be
> added to the platform-drivers-x86/for-next branch and eventually
> will be included in the pdx86 pull-request to Linus for the next
> merge-window.

Cool! Thanks all for the reviews and for allowing the drivers in. Stay
tuned for more.

regards,
Henning

> Regards,
> 
> Hans
> 
> 
> > 
> >  drivers/leds/Kconfig                          |   3 +
> >  drivers/leds/Makefile                         |   3 +
> >  drivers/leds/simple/Kconfig                   |  11 +
> >  drivers/leds/simple/Makefile                  |   2 +
> >  drivers/leds/simple/simatic-ipc-leds.c        | 202
> > ++++++++++++++++ drivers/platform/x86/Kconfig                  |
> > 12 + drivers/platform/x86/Makefile                 |   3 +
> >  drivers/platform/x86/pmc_atom.c               |  54 +++--
> >  drivers/platform/x86/simatic-ipc.c            | 176 ++++++++++++++
> >  drivers/watchdog/Kconfig                      |  11 +
> >  drivers/watchdog/Makefile                     |   1 +
> >  drivers/watchdog/simatic-ipc-wdt.c            | 228
> > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h      |
> > 29 +++ include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
> >  14 files changed, 786 insertions(+), 21 deletions(-)
> >  create mode 100644 drivers/leds/simple/Kconfig
> >  create mode 100644 drivers/leds/simple/Makefile
> >  create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> >  create mode 100644 drivers/platform/x86/simatic-ipc.c
> >  create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> >  create mode 100644
> > include/linux/platform_data/x86/simatic-ipc-base.h create mode
> > 100644 include/linux/platform_data/x86/simatic-ipc.h 
> 

Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Henning Schild]

Am Mon, 13 Dec 2021 13:05:00 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> This driver adds initial support for several devices from Siemens. It
> is based on a platform driver introduced in an earlier commit.
> 
> One of the supported machines has GPIO connected LEDs, here we poke
> GPIO memory directly because pinctrl does not come up.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  drivers/leds/Kconfig                   |   3 +
>  drivers/leds/Makefile                  |   3 +
>  drivers/leds/simple/Kconfig            |  11 ++
>  drivers/leds/simple/Makefile           |   2 +
>  drivers/leds/simple/simatic-ipc-leds.c | 202
> +++++++++++++++++++++++++ 5 files changed, 221 insertions(+)
>  create mode 100644 drivers/leds/simple/Kconfig
>  create mode 100644 drivers/leds/simple/Makefile
>  create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> 
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index ed800f5da7d8..ac6688d7a3f4 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -879,4 +879,7 @@ source "drivers/leds/flash/Kconfig"
>  comment "LED Triggers"
>  source "drivers/leds/trigger/Kconfig"
>  
> +comment "Simple LED drivers"
> +source "drivers/leds/simple/Kconfig"
> +
>  endif # NEW_LEDS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index c636ec069612..1a719caf14c0 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -105,3 +105,6 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+=
> trigger/ 
>  # LED Blink
>  obj-y					+= blink/
> +
> +# Simple LED drivers
> +obj-y					+= simple/
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> new file mode 100644
> index 000000000000..9f6a68336659
> --- /dev/null
> +++ b/drivers/leds/simple/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config LEDS_SIEMENS_SIMATIC_IPC
> +	tristate "LED driver for Siemens Simatic IPCs"
> +	depends on LEDS_CLASS
> +	depends on SIEMENS_SIMATIC_IPC

Trying to bring this into distros i think this one should rather be
"select SIEMENS_SIMATIC_IPC" and the same thought goes for wdt.

Users care about enabling a feature and maybe not too much about having
to enable infra, enabling SIEMENS_SIMATIC_IPC is just infra and useless
without LEDS_SIEMENS_SIMATIC_IPC and/or SIEMENS_SIMATIC_IPC_WDT.

So those two should probably "select" and not "depend".

I will send a patch, feel free to tell me that i am wrong and teach me
about how deps should work.

regards,
Henning

> +	help
> +	  This option enables support for the LEDs of several
> Industrial PCs
> +	  from Siemens.
> +
> +	  To compile this driver as a module, choose M here: the
> module
> +	  will be called simatic-ipc-leds.
> diff --git a/drivers/leds/simple/Makefile
> b/drivers/leds/simple/Makefile new file mode 100644
> index 000000000000..8481f1e9e360
> --- /dev/null
> +++ b/drivers/leds/simple/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o
> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> b/drivers/leds/simple/simatic-ipc-leds.c new file mode 100644
> index 000000000000..ff2c96e73241
> --- /dev/null
> +++ b/drivers/leds/simple/simatic-ipc-leds.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Siemens SIMATIC IPC driver for LEDs
> + *
> + * Copyright (c) Siemens AG, 2018-2021
> + *
> + * Authors:
> + *  Henning Schild <henning.schild@siemens.com>
> + *  Jan Kiszka <jan.kiszka@siemens.com>
> + *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> + */
> +
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/spinlock.h>
> +
> +#define SIMATIC_IPC_LED_PORT_BASE	0x404E
> +
> +struct simatic_ipc_led {
> +	unsigned int value; /* mask for io and offset for mem */
> +	char *name;
> +	struct led_classdev cdev;
> +};
> +
> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
> +	{ }
> +};
> +
> +/* the actual start will be discovered with PCI, 0 is a placeholder
> */ +struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0,
> SZ_4K, KBUILD_MODNAME); +
> +static void *simatic_ipc_led_memory;
> +
> +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> +	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> +	{0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> +	{0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> +	{0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> +	{0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> +	{0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> +	{ }
> +};
> +
> +static struct resource simatic_ipc_led_io_res =
> +	DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_2,
> KBUILD_MODNAME); +
> +static DEFINE_SPINLOCK(reg_lock);
> +
> +static inline struct simatic_ipc_led *cdev_to_led(struct
> led_classdev *led_cd) +{
> +	return container_of(led_cd, struct simatic_ipc_led, cdev);
> +}
> +
> +static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
> +				   enum led_brightness brightness)
> +{
> +	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> +	unsigned long flags;
> +	unsigned int val;
> +
> +	spin_lock_irqsave(&reg_lock, flags);
> +
> +	val = inw(SIMATIC_IPC_LED_PORT_BASE);
> +	if (brightness == LED_OFF)
> +		outw(val | led->value, SIMATIC_IPC_LED_PORT_BASE);
> +	else
> +		outw(val & ~led->value, SIMATIC_IPC_LED_PORT_BASE);
> +
> +	spin_unlock_irqrestore(&reg_lock, flags);
> +}
> +
> +static enum led_brightness simatic_ipc_led_get_io(struct
> led_classdev *led_cd) +{
> +	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> +
> +	return inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF
> : led_cd->max_brightness; +}
> +
> +static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
> +				    enum led_brightness brightness)
> +{
> +	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> +
> +	u32 *p;
> +
> +	p = simatic_ipc_led_memory + led->value;
> +	*p = (*p & ~1) | (brightness == LED_OFF);
> +}
> +
> +static enum led_brightness simatic_ipc_led_get_mem(struct
> led_classdev *led_cd) +{
> +	struct simatic_ipc_led *led = cdev_to_led(led_cd);
> +
> +	u32 *p;
> +
> +	p = simatic_ipc_led_memory + led->value;
> +	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
> +}
> +
> +static int simatic_ipc_leds_probe(struct platform_device *pdev)
> +{
> +	const struct simatic_ipc_platform *plat =
> pdev->dev.platform_data;
> +	struct device *dev = &pdev->dev;
> +	struct simatic_ipc_led *ipcled;
> +	struct led_classdev *cdev;
> +	struct resource *res;
> +	int err, type;
> +	u32 *p;
> +
> +	switch (plat->devmode) {
> +	case SIMATIC_IPC_DEVICE_227D:
> +	case SIMATIC_IPC_DEVICE_427E:
> +		res = &simatic_ipc_led_io_res;
> +		ipcled = simatic_ipc_leds_io;
> +		/* on 227D the two bytes work the other way araound
> */
> +		if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
> +			while (ipcled->value) {
> +				ipcled->value =
> swab16(ipcled->value);
> +				ipcled++;
> +			}
> +			ipcled = simatic_ipc_leds_io;
> +		}
> +		type = IORESOURCE_IO;
> +		if (!devm_request_region(dev, res->start,
> resource_size(res), KBUILD_MODNAME)) {
> +			dev_err(dev, "Unable to register IO resource
> at %pR\n", res);
> +			return -EBUSY;
> +		}
> +		break;
> +	case SIMATIC_IPC_DEVICE_127E:
> +		res = &simatic_ipc_led_mem_res;
> +		ipcled = simatic_ipc_leds_mem;
> +		type = IORESOURCE_MEM;
> +
> +		/* get GPIO base from PCI */
> +		res->start = simatic_ipc_get_membase0(PCI_DEVFN(13,
> 0));
> +		if (res->start == 0)
> +			return -ENODEV;
> +
> +		/* do the final address calculation */
> +		res->start = res->start + (0xC5 << 16);
> +		res->end += res->start;
> +
> +		simatic_ipc_led_memory = devm_ioremap_resource(dev,
> res);
> +		if (IS_ERR(simatic_ipc_led_memory))
> +			return PTR_ERR(simatic_ipc_led_memory);
> +
> +		/* initialize power/watchdog LED */
> +		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> PM_WDT_OUT */
> +		*p = (*p & ~1);
> +		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> PM_BIOS_BOOT_N */
> +		*p = (*p | 1);
> +
> +		break;
> +	default:
> +		return -ENODEV;
> +	}
> +
> +	while (ipcled->value) {
> +		cdev = &ipcled->cdev;
> +		if (type == IORESOURCE_MEM) {
> +			cdev->brightness_set =
> simatic_ipc_led_set_mem;
> +			cdev->brightness_get =
> simatic_ipc_led_get_mem;
> +		} else {
> +			cdev->brightness_set =
> simatic_ipc_led_set_io;
> +			cdev->brightness_get =
> simatic_ipc_led_get_io;
> +		}
> +		cdev->max_brightness = LED_ON;
> +		cdev->name = ipcled->name;
> +
> +		err = devm_led_classdev_register(dev, cdev);
> +		if (err < 0)
> +			return err;
> +		ipcled++;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct platform_driver simatic_ipc_led_driver = {
> +	.probe = simatic_ipc_leds_probe,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +	}
> +};
> +
> +module_platform_driver(simatic_ipc_led_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");

Re: [PATCH v5 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs

[Guenter Roeck]

On 5/13/22 12:40, Henning Schild wrote:
> Am Mon, 13 Dec 2021 13:05:00 +0100
> schrieb Henning Schild <henning.schild@siemens.com>:
> 
>> This driver adds initial support for several devices from Siemens. It
>> is based on a platform driver introduced in an earlier commit.
>>
>> One of the supported machines has GPIO connected LEDs, here we poke
>> GPIO memory directly because pinctrl does not come up.
>>
>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>> ---
>>   drivers/leds/Kconfig                   |   3 +
>>   drivers/leds/Makefile                  |   3 +
>>   drivers/leds/simple/Kconfig            |  11 ++
>>   drivers/leds/simple/Makefile           |   2 +
>>   drivers/leds/simple/simatic-ipc-leds.c | 202
>> +++++++++++++++++++++++++ 5 files changed, 221 insertions(+)
>>   create mode 100644 drivers/leds/simple/Kconfig
>>   create mode 100644 drivers/leds/simple/Makefile
>>   create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
>>
>> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
>> index ed800f5da7d8..ac6688d7a3f4 100644
>> --- a/drivers/leds/Kconfig
>> +++ b/drivers/leds/Kconfig
>> @@ -879,4 +879,7 @@ source "drivers/leds/flash/Kconfig"
>>   comment "LED Triggers"
>>   source "drivers/leds/trigger/Kconfig"
>>   
>> +comment "Simple LED drivers"
>> +source "drivers/leds/simple/Kconfig"
>> +
>>   endif # NEW_LEDS
>> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
>> index c636ec069612..1a719caf14c0 100644
>> --- a/drivers/leds/Makefile
>> +++ b/drivers/leds/Makefile
>> @@ -105,3 +105,6 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+=
>> trigger/
>>   # LED Blink
>>   obj-y					+= blink/
>> +
>> +# Simple LED drivers
>> +obj-y					+= simple/
>> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
>> new file mode 100644
>> index 000000000000..9f6a68336659
>> --- /dev/null
>> +++ b/drivers/leds/simple/Kconfig
>> @@ -0,0 +1,11 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +config LEDS_SIEMENS_SIMATIC_IPC
>> +	tristate "LED driver for Siemens Simatic IPCs"
>> +	depends on LEDS_CLASS
>> +	depends on SIEMENS_SIMATIC_IPC
> 
> Trying to bring this into distros i think this one should rather be
> "select SIEMENS_SIMATIC_IPC" and the same thought goes for wdt.
> 
> Users care about enabling a feature and maybe not too much about having
> to enable infra, enabling SIEMENS_SIMATIC_IPC is just infra and useless
> without LEDS_SIEMENS_SIMATIC_IPC and/or SIEMENS_SIMATIC_IPC_WDT.
> 
> So those two should probably "select" and not "depend".
> 
> I will send a patch, feel free to tell me that i am wrong and teach me
> about how deps should work.
> 

I disagree. Normally drivers depend on the platform and don't select it.
It would make more sense to specify something like

	depends on SIEMENS_SIMATIC_IPC
	default SIEMENS_SIMATIC_IPC

In this context, the description for SIEMENS_SIMATIC_IPC is
a bit misleading. Something like "Platform driver for Simatic
Industrial PC" or just "Support for Simatic Industrial PC"
would be a better description.

Guenter

> regards,
> Henning
> 
>> +	help
>> +	  This option enables support for the LEDs of several
>> Industrial PCs
>> +	  from Siemens.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> module
>> +	  will be called simatic-ipc-leds.
>> diff --git a/drivers/leds/simple/Makefile
>> b/drivers/leds/simple/Makefile new file mode 100644
>> index 000000000000..8481f1e9e360
>> --- /dev/null
>> +++ b/drivers/leds/simple/Makefile
>> @@ -0,0 +1,2 @@
>> +# SPDX-License-Identifier: GPL-2.0
>> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o
>> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
>> b/drivers/leds/simple/simatic-ipc-leds.c new file mode 100644
>> index 000000000000..ff2c96e73241
>> --- /dev/null
>> +++ b/drivers/leds/simple/simatic-ipc-leds.c
>> @@ -0,0 +1,202 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Siemens SIMATIC IPC driver for LEDs
>> + *
>> + * Copyright (c) Siemens AG, 2018-2021
>> + *
>> + * Authors:
>> + *  Henning Schild <henning.schild@siemens.com>
>> + *  Jan Kiszka <jan.kiszka@siemens.com>
>> + *  Gerd Haeussler <gerd.haeussler.ext@siemens.com>
>> + */
>> +
>> +#include <linux/ioport.h>
>> +#include <linux/kernel.h>
>> +#include <linux/leds.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/platform_data/x86/simatic-ipc-base.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/sizes.h>
>> +#include <linux/spinlock.h>
>> +
>> +#define SIMATIC_IPC_LED_PORT_BASE	0x404E
>> +
>> +struct simatic_ipc_led {
>> +	unsigned int value; /* mask for io and offset for mem */
>> +	char *name;
>> +	struct led_classdev cdev;
>> +};
>> +
>> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
>> +	{1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
>> +	{1 << 7,  "yellow:" LED_FUNCTION_STATUS "-1" },
>> +	{1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
>> +	{1 << 6,  "yellow:" LED_FUNCTION_STATUS "-2" },
>> +	{1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
>> +	{1 << 5,  "yellow:" LED_FUNCTION_STATUS "-3" },
>> +	{ }
>> +};
>> +
>> +/* the actual start will be discovered with PCI, 0 is a placeholder
>> */ +struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0,
>> SZ_4K, KBUILD_MODNAME); +
>> +static void *simatic_ipc_led_memory;
>> +
>> +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
>> +	{0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
>> +	{0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
>> +	{0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
>> +	{0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
>> +	{0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
>> +	{0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
>> +	{ }
>> +};
>> +
>> +static struct resource simatic_ipc_led_io_res =
>> +	DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_2,
>> KBUILD_MODNAME); +
>> +static DEFINE_SPINLOCK(reg_lock);
>> +
>> +static inline struct simatic_ipc_led *cdev_to_led(struct
>> led_classdev *led_cd) +{
>> +	return container_of(led_cd, struct simatic_ipc_led, cdev);
>> +}
>> +
>> +static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
>> +				   enum led_brightness brightness)
>> +{
>> +	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>> +	unsigned long flags;
>> +	unsigned int val;
>> +
>> +	spin_lock_irqsave(&reg_lock, flags);
>> +
>> +	val = inw(SIMATIC_IPC_LED_PORT_BASE);
>> +	if (brightness == LED_OFF)
>> +		outw(val | led->value, SIMATIC_IPC_LED_PORT_BASE);
>> +	else
>> +		outw(val & ~led->value, SIMATIC_IPC_LED_PORT_BASE);
>> +
>> +	spin_unlock_irqrestore(&reg_lock, flags);
>> +}
>> +
>> +static enum led_brightness simatic_ipc_led_get_io(struct
>> led_classdev *led_cd) +{
>> +	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>> +
>> +	return inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF
>> : led_cd->max_brightness; +}
>> +
>> +static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
>> +				    enum led_brightness brightness)
>> +{
>> +	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>> +
>> +	u32 *p;
>> +
>> +	p = simatic_ipc_led_memory + led->value;
>> +	*p = (*p & ~1) | (brightness == LED_OFF);
>> +}
>> +
>> +static enum led_brightness simatic_ipc_led_get_mem(struct
>> led_classdev *led_cd) +{
>> +	struct simatic_ipc_led *led = cdev_to_led(led_cd);
>> +
>> +	u32 *p;
>> +
>> +	p = simatic_ipc_led_memory + led->value;
>> +	return (*p & 1) ? LED_OFF : led_cd->max_brightness;
>> +}
>> +
>> +static int simatic_ipc_leds_probe(struct platform_device *pdev)
>> +{
>> +	const struct simatic_ipc_platform *plat =
>> pdev->dev.platform_data;
>> +	struct device *dev = &pdev->dev;
>> +	struct simatic_ipc_led *ipcled;
>> +	struct led_classdev *cdev;
>> +	struct resource *res;
>> +	int err, type;
>> +	u32 *p;
>> +
>> +	switch (plat->devmode) {
>> +	case SIMATIC_IPC_DEVICE_227D:
>> +	case SIMATIC_IPC_DEVICE_427E:
>> +		res = &simatic_ipc_led_io_res;
>> +		ipcled = simatic_ipc_leds_io;
>> +		/* on 227D the two bytes work the other way araound
>> */
>> +		if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
>> +			while (ipcled->value) {
>> +				ipcled->value =
>> swab16(ipcled->value);
>> +				ipcled++;
>> +			}
>> +			ipcled = simatic_ipc_leds_io;
>> +		}
>> +		type = IORESOURCE_IO;
>> +		if (!devm_request_region(dev, res->start,
>> resource_size(res), KBUILD_MODNAME)) {
>> +			dev_err(dev, "Unable to register IO resource
>> at %pR\n", res);
>> +			return -EBUSY;
>> +		}
>> +		break;
>> +	case SIMATIC_IPC_DEVICE_127E:
>> +		res = &simatic_ipc_led_mem_res;
>> +		ipcled = simatic_ipc_leds_mem;
>> +		type = IORESOURCE_MEM;
>> +
>> +		/* get GPIO base from PCI */
>> +		res->start = simatic_ipc_get_membase0(PCI_DEVFN(13,
>> 0));
>> +		if (res->start == 0)
>> +			return -ENODEV;
>> +
>> +		/* do the final address calculation */
>> +		res->start = res->start + (0xC5 << 16);
>> +		res->end += res->start;
>> +
>> +		simatic_ipc_led_memory = devm_ioremap_resource(dev,
>> res);
>> +		if (IS_ERR(simatic_ipc_led_memory))
>> +			return PTR_ERR(simatic_ipc_led_memory);
>> +
>> +		/* initialize power/watchdog LED */
>> +		p = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
>> PM_WDT_OUT */
>> +		*p = (*p & ~1);
>> +		p = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
>> PM_BIOS_BOOT_N */
>> +		*p = (*p | 1);
>> +
>> +		break;
>> +	default:
>> +		return -ENODEV;
>> +	}
>> +
>> +	while (ipcled->value) {
>> +		cdev = &ipcled->cdev;
>> +		if (type == IORESOURCE_MEM) {
>> +			cdev->brightness_set =
>> simatic_ipc_led_set_mem;
>> +			cdev->brightness_get =
>> simatic_ipc_led_get_mem;
>> +		} else {
>> +			cdev->brightness_set =
>> simatic_ipc_led_set_io;
>> +			cdev->brightness_get =
>> simatic_ipc_led_get_io;
>> +		}
>> +		cdev->max_brightness = LED_ON;
>> +		cdev->name = ipcled->name;
>> +
>> +		err = devm_led_classdev_register(dev, cdev);
>> +		if (err < 0)
>> +			return err;
>> +		ipcled++;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static struct platform_driver simatic_ipc_led_driver = {
>> +	.probe = simatic_ipc_leds_probe,
>> +	.driver = {
>> +		.name = KBUILD_MODNAME,
>> +	}
>> +};
>> +
>> +module_platform_driver(simatic_ipc_led_driver);
>> +
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
>> +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
>