linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 0/4] add device drivers for Siemens Industrial PCs
@ 2021-03-29 17:49 Henning Schild
  2021-03-29 17:49 ` [PATCH v3 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
                   ` (5 more replies)
  0 siblings, 6 replies; 40+ messages in thread
From: Henning Schild @ 2021-03-29 17:49 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Henning Schild, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Andy Shevchenko, Enrico Weigelt

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.

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               |  57 +++--
 drivers/platform/x86/simatic-ipc.c            | 169 ++++++++++++++
 drivers/watchdog/Kconfig                      |  11 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/simatic-ipc-wdt.c            | 215 ++++++++++++++++++
 .../platform_data/x86/simatic-ipc-base.h      |  29 +++
 include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
 14 files changed, 769 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.26.3


^ permalink raw reply	[flat|nested] 40+ messages in thread

* [PATCH v3 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-29 17:49 [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
@ 2021-03-29 17:49 ` Henning Schild
  2021-11-26 12:39   ` Henning Schild
  2021-03-29 17:49 ` [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-03-29 17:49 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Henning Schild, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Andy Shevchenko, Enrico Weigelt

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.

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            | 169 ++++++++++++++++++
 .../platform_data/x86/simatic-ipc-base.h      |  29 +++
 include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++++
 5 files changed, 285 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 461ec61530eb..1eaa03d0d183 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1289,6 +1289,18 @@ config INTEL_TELEMETRY
 	  directly via debugfs files. Various tools may use
 	  this interface for SoC state monitoring.
 
+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 60d554073749..26cdebf2e701 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -138,3 +138,6 @@ obj-$(CONFIG_INTEL_TELEMETRY)		+= intel_telemetry_core.o \
 					   intel_telemetry_pltdrv.o \
 					   intel_telemetry_debugfs.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..52e8596bc63d
--- /dev/null
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -0,0 +1,169 @@
+// 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 */
+/*
+ * Get membase address from PCI, used in leds and wdt modul. 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.26.3


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-29 17:49 [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
  2021-03-29 17:49 ` [PATCH v3 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
@ 2021-03-29 17:49 ` Henning Schild
  2021-03-30 11:04   ` Andy Shevchenko
  2021-11-25 17:11   ` Henning Schild
  2021-03-29 17:49 ` [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
                   ` (3 subsequent siblings)
  5 siblings, 2 replies; 40+ messages in thread
From: Henning Schild @ 2021-03-29 17:49 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Henning Schild, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Andy Shevchenko, Enrico Weigelt

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

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 b6742b4231bf..5c8558a4fa60 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -937,4 +937,7 @@ source "drivers/leds/trigger/Kconfig"
 comment "LED Blink"
 source "drivers/leds/blink/Kconfig"
 
+comment "Simple LED drivers"
+source "drivers/leds/simple/Kconfig"
+
 endif # NEW_LEDS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 2a698df9da57..2de7fdd8d629 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -111,3 +111,6 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+= trigger/
 
 # LED Blink
 obj-$(CONFIG_LEDS_BLINK)                += 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..043edbf81b76
--- /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_1, 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.26.3


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-29 17:49 [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
  2021-03-29 17:49 ` [PATCH v3 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
  2021-03-29 17:49 ` [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
@ 2021-03-29 17:49 ` Henning Schild
  2021-04-01 16:15   ` Enrico Weigelt, metux IT consult
                     ` (2 more replies)
  2021-03-29 17:49 ` [PATCH v3 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
                   ` (2 subsequent siblings)
  5 siblings, 3 replies; 40+ messages in thread
From: Henning Schild @ 2021-03-29 17:49 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Henning Schild, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Andy Shevchenko, Enrico Weigelt

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

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 | 215 +++++++++++++++++++++++++++++
 3 files changed, 227 insertions(+)
 create mode 100644 drivers/watchdog/simatic-ipc-wdt.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 1fe0042a48d2..948497eb4bef 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1575,6 +1575,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 f3a6540e725e..7f5c73ec058c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -142,6 +142,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..e901718d05b9
--- /dev/null
+++ b/drivers/watchdog/simatic-ipc-wdt.c
@@ -0,0 +1,215 @@
+// 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 =
+	DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1,
+			    KBUILD_MODNAME " WD_ENABLE_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 = inw(GP_STATUS_REG_227E);
+		outw(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.start,
+				 resource_size(&io_resource),
+				 io_resource.name)) {
+		dev_err(dev,
+			"Unable to register IO resource at %#x\n",
+			WD_ENABLE_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.26.3


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* [PATCH v3 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs
  2021-03-29 17:49 [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
                   ` (2 preceding siblings ...)
  2021-03-29 17:49 ` [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
@ 2021-03-29 17:49 ` Henning Schild
  2021-03-29 18:00 ` [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
  2021-04-07 11:36 ` Hans de Goede
  5 siblings, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-03-29 17:49 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Henning Schild, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Andy Shevchenko, Enrico Weigelt, Michael Haener

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 | 57 +++++++++++++++++++++------------
 1 file changed, 36 insertions(+), 21 deletions(-)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index ca684ed760d1..9904fe6973df 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,6 +422,7 @@ static const struct dmi_system_id critclk_systems[] = {
 	{
 		/* pmc_plt_clk* - are used for ethernet controllers */
 		.ident = "Beckhoff CB3163",
+		.callback = dmi_callback,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
 			DMI_MATCH(DMI_BOARD_NAME, "CB3163"),
@@ -402,6 +431,7 @@ static const struct dmi_system_id critclk_systems[] = {
 	{
 		/* pmc_plt_clk* - are used for ethernet controllers */
 		.ident = "Beckhoff CB4063",
+		.callback = dmi_callback,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
 			DMI_MATCH(DMI_BOARD_NAME, "CB4063"),
@@ -410,6 +440,7 @@ static const struct dmi_system_id critclk_systems[] = {
 	{
 		/* pmc_plt_clk* - are used for ethernet controllers */
 		.ident = "Beckhoff CB6263",
+		.callback = dmi_callback,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
 			DMI_MATCH(DMI_BOARD_NAME, "CB6263"),
@@ -418,30 +449,17 @@ static const struct dmi_system_id critclk_systems[] = {
 	{
 		/* pmc_plt_clk* - are used for ethernet controllers */
 		.ident = "Beckhoff CB6363",
+		.callback = dmi_callback,
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "Beckhoff Automation"),
 			DMI_MATCH(DMI_BOARD_NAME, "CB6363"),
 		},
 	},
 	{
-		.ident = "SIMATIC IPC227E",
+		.callback = dmi_callback_siemens,
+		.ident = "SIEMENS AG",
 		.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"),
 		},
 	},
 
@@ -453,7 +471,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)
@@ -461,10 +478,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.26.3


^ permalink raw reply related	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 0/4] add device drivers for Siemens Industrial PCs
  2021-03-29 17:49 [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
                   ` (3 preceding siblings ...)
  2021-03-29 17:49 ` [PATCH v3 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
@ 2021-03-29 18:00 ` Henning Schild
  2021-04-07 11:36 ` Hans de Goede
  5 siblings, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-03-29 18:00 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek,
	Andy Shevchenko, Enrico Weigelt

Guys,

sorry for the delay. This one did in fact not change too much after all.

The biggest points that are still kind of open are the naming of the
LEDs. If what is proposed here is acceptable it is not open from my
side.

The other big point was "using a generic gpio" driver as a basis. My
current understanding of that point is, that such a driver does not yet
exist. Meaning an introduction of the abstractions can and probably
should wait for a second user. Without the second user it is just hard
to test and find the right abstraction, plus we will end up with more
code meaning more work for everyone.

regards,
Henning

Am Mon, 29 Mar 2021 19:49:24 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> 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.
> 
> 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               |  57 +++--
>  drivers/platform/x86/simatic-ipc.c            | 169 ++++++++++++++
>  drivers/watchdog/Kconfig                      |  11 +
>  drivers/watchdog/Makefile                     |   1 +
>  drivers/watchdog/simatic-ipc-wdt.c            | 215
> ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h      |
> 29 +++ include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
>  14 files changed, 769 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
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-29 17:49 ` [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
@ 2021-03-30 11:04   ` Andy Shevchenko
  2021-03-30 11:58     ` Henning Schild
  2021-11-26 13:28     ` Henning Schild
  2021-11-25 17:11   ` Henning Schild
  1 sibling, 2 replies; 40+ messages in thread
From: Andy Shevchenko @ 2021-03-30 11:04 UTC (permalink / raw)
  To: Henning Schild
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> This driver adds initial support for several devices from Siemens. It is
> based on a platform driver introduced in an earlier commit.

...

> +#define SIMATIC_IPC_LED_PORT_BASE      0x404E

> +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" },
> +       { }
> +};

> +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"},
> +       { }
> +};

It seems to me like poking GPIO controller registers directly. This is not good.
The question still remains: Can we simply register a GPIO (pin
control) driver and use an LED GPIO driver with an additional board
file that instantiates it?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-30 11:04   ` Andy Shevchenko
@ 2021-03-30 11:58     ` Henning Schild
  2021-03-30 12:15       ` Andy Shevchenko
  2021-11-26 13:28     ` Henning Schild
  1 sibling, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-03-30 11:58 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

Am Tue, 30 Mar 2021 14:04:35 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.  
> 
> ...
> 
> > +#define SIMATIC_IPC_LED_PORT_BASE      0x404E  
> 
> > +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" },
> > +       { }
> > +};  
> 
> > +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"},
> > +       { }
> > +};  
> 
> It seems to me like poking GPIO controller registers directly. This
> is not good. The question still remains: Can we simply register a
> GPIO (pin control) driver and use an LED GPIO driver with an
> additional board file that instantiates it?

I wrote about that in reply to the cover letter. My view is still that
it would be an abstraction with only one user, just causing work and
likely not ending up as generic as it might eventually have to be.

The region is reserved, not sure what the problem with the "poking" is.

Maybe i do not understand all the benefits of such a split at this
point in time. At the moment i only see work with hardly any benefit,
not just work for me but also for maintainers. I sure do not mean to be
ignorant. Maybe you go into details and convince me or we wait for other
peoples opinions on how to proceed, maybe there is a second user that i
am not aware of?
Until i am convinced otherwise i will try to argue that a
single-user-abstraction is needless work/code, and should be done only
when actually needed.

regards,
Henning

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-30 11:58     ` Henning Schild
@ 2021-03-30 12:15       ` Andy Shevchenko
  2021-03-30 12:30         ` Henning Schild
  0 siblings, 1 reply; 40+ messages in thread
From: Andy Shevchenko @ 2021-03-30 12:15 UTC (permalink / raw)
  To: Henning Schild
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Tue, 30 Mar 2021 14:04:35 +0300
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > <henning.schild@siemens.com> wrote:
> > >
> > > This driver adds initial support for several devices from Siemens.
> > > It is based on a platform driver introduced in an earlier commit.
> >
> > ...
> >
> > > +#define SIMATIC_IPC_LED_PORT_BASE      0x404E
> >
> > > +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" },
> > > +       { }
> > > +};
> >
> > > +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"},
> > > +       { }
> > > +};
> >
> > It seems to me like poking GPIO controller registers directly. This
> > is not good. The question still remains: Can we simply register a
> > GPIO (pin control) driver and use an LED GPIO driver with an
> > additional board file that instantiates it?
>
> I wrote about that in reply to the cover letter. My view is still that
> it would be an abstraction with only one user, just causing work and
> likely not ending up as generic as it might eventually have to be.
>
> The region is reserved, not sure what the problem with the "poking" is.


> Maybe i do not understand all the benefits of such a split at this
> point in time. At the moment i only see work with hardly any benefit,
> not just work for me but also for maintainers. I sure do not mean to be
> ignorant. Maybe you go into details and convince me or we wait for other
> peoples opinions on how to proceed, maybe there is a second user that i
> am not aware of?
> Until i am convinced otherwise i will try to argue that a
> single-user-abstraction is needless work/code, and should be done only
> when actually needed.

I have just read your messages (there is a cover letter and additional
email which was sent lately).

I would like to know what the CPU model number on that board is. Than
we can continue to see what possibilities we have here.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-30 12:15       ` Andy Shevchenko
@ 2021-03-30 12:30         ` Henning Schild
  2021-03-30 12:41           ` Andy Shevchenko
  0 siblings, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-03-30 12:30 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

Am Tue, 30 Mar 2021 15:15:16 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Tue, 30 Mar 2021 14:04:35 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> > > >
> > > > This driver adds initial support for several devices from
> > > > Siemens. It is based on a platform driver introduced in an
> > > > earlier commit.  
> > >
> > > ...
> > >  
> > > > +#define SIMATIC_IPC_LED_PORT_BASE      0x404E  
> > >  
> > > > +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" },
> > > > +       { }
> > > > +};  
> > >  
> > > > +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"},
> > > > +       { }
> > > > +};  
> > >
> > > It seems to me like poking GPIO controller registers directly.
> > > This is not good. The question still remains: Can we simply
> > > register a GPIO (pin control) driver and use an LED GPIO driver
> > > with an additional board file that instantiates it?  
> >
> > I wrote about that in reply to the cover letter. My view is still
> > that it would be an abstraction with only one user, just causing
> > work and likely not ending up as generic as it might eventually
> > have to be.
> >
> > The region is reserved, not sure what the problem with the "poking"
> > is.  
> 
> 
> > Maybe i do not understand all the benefits of such a split at this
> > point in time. At the moment i only see work with hardly any
> > benefit, not just work for me but also for maintainers. I sure do
> > not mean to be ignorant. Maybe you go into details and convince me
> > or we wait for other peoples opinions on how to proceed, maybe
> > there is a second user that i am not aware of?
> > Until i am convinced otherwise i will try to argue that a
> > single-user-abstraction is needless work/code, and should be done
> > only when actually needed.  
> 
> I have just read your messages (there is a cover letter and additional
> email which was sent lately).
> 
> I would like to know what the CPU model number on that board is. Than
> we can continue to see what possibilities we have here.

I guess we are talking about the one that uses memory mapped, that is
called an "IPC127E" and seems to have either Intel Atom E3940 or E3930
which seems to be Apollo Lake.

Henning

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-30 12:30         ` Henning Schild
@ 2021-03-30 12:41           ` Andy Shevchenko
  2021-03-30 15:23             ` Henning Schild
  0 siblings, 1 reply; 40+ messages in thread
From: Andy Shevchenko @ 2021-03-30 12:41 UTC (permalink / raw)
  To: Henning Schild
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Tue, 30 Mar 2021 15:15:16 +0300
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > <henning.schild@siemens.com> wrote:
> > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > <henning.schild@siemens.com> wrote:

> > > > > +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"},
> > > > > +       { }
> > > > > +};
> > > >
> > > > It seems to me like poking GPIO controller registers directly.
> > > > This is not good. The question still remains: Can we simply
> > > > register a GPIO (pin control) driver and use an LED GPIO driver
> > > > with an additional board file that instantiates it?
> > >
> > > I wrote about that in reply to the cover letter. My view is still
> > > that it would be an abstraction with only one user, just causing
> > > work and likely not ending up as generic as it might eventually
> > > have to be.
> > >
> > > The region is reserved, not sure what the problem with the "poking"
> > > is.
> >
> >
> > > Maybe i do not understand all the benefits of such a split at this
> > > point in time. At the moment i only see work with hardly any
> > > benefit, not just work for me but also for maintainers. I sure do
> > > not mean to be ignorant. Maybe you go into details and convince me
> > > or we wait for other peoples opinions on how to proceed, maybe
> > > there is a second user that i am not aware of?
> > > Until i am convinced otherwise i will try to argue that a
> > > single-user-abstraction is needless work/code, and should be done
> > > only when actually needed.
> >
> > I have just read your messages (there is a cover letter and additional
> > email which was sent lately).
> >
> > I would like to know what the CPU model number on that board is. Than
> > we can continue to see what possibilities we have here.
>
> I guess we are talking about the one that uses memory mapped, that is
> called an "IPC127E" and seems to have either Intel Atom E3940 or E3930
> which seems to be Apollo Lake.

Yep. And now the question, in my patch series you should have got the
apollolake-pinctrl driver loaded (if not, we have to investigate why
it's not being instantiated). This will give you a read GPIO driver.
So, you may use regular LED GPIO on top of it
(https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
I would like to understand why it can't be achieved.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-30 12:41           ` Andy Shevchenko
@ 2021-03-30 15:23             ` Henning Schild
  2021-03-31 15:40               ` Andy Shevchenko
  0 siblings, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-03-30 15:23 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

Am Tue, 30 Mar 2021 15:41:53 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Tue, 30 Mar 2021 15:15:16 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > <henning.schild@siemens.com> wrote:  
> 
> > > > > > +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"},
> > > > > > +       { }
> > > > > > +};  
> > > > >
> > > > > It seems to me like poking GPIO controller registers directly.
> > > > > This is not good. The question still remains: Can we simply
> > > > > register a GPIO (pin control) driver and use an LED GPIO
> > > > > driver with an additional board file that instantiates it?  
> > > >
> > > > I wrote about that in reply to the cover letter. My view is
> > > > still that it would be an abstraction with only one user, just
> > > > causing work and likely not ending up as generic as it might
> > > > eventually have to be.
> > > >
> > > > The region is reserved, not sure what the problem with the
> > > > "poking" is.  
> > >
> > >  
> > > > Maybe i do not understand all the benefits of such a split at
> > > > this point in time. At the moment i only see work with hardly
> > > > any benefit, not just work for me but also for maintainers. I
> > > > sure do not mean to be ignorant. Maybe you go into details and
> > > > convince me or we wait for other peoples opinions on how to
> > > > proceed, maybe there is a second user that i am not aware of?
> > > > Until i am convinced otherwise i will try to argue that a
> > > > single-user-abstraction is needless work/code, and should be
> > > > done only when actually needed.  
> > >
> > > I have just read your messages (there is a cover letter and
> > > additional email which was sent lately).
> > >
> > > I would like to know what the CPU model number on that board is.
> > > Than we can continue to see what possibilities we have here.  
> >
> > I guess we are talking about the one that uses memory mapped, that
> > is called an "IPC127E" and seems to have either Intel Atom E3940 or
> > E3930 which seems to be Apollo Lake.  
> 
> Yep. And now the question, in my patch series you should have got the
> apollolake-pinctrl driver loaded (if not, we have to investigate why
> it's not being instantiated). This will give you a read GPIO driver.

Ok, so there is the existing driver i asked about several times. Thanks
for pointing it out.

> So, you may use regular LED GPIO on top of it
> (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> I would like to understand why it can't be achieved.

Will have a look. Unfortunately this one box is missing in my personal
collection, but let us assume that one can be converted to that
existing driver.
I guess that will still mean the PIO-based part of the LED driver will
have to stay as is.

regards,
Henning

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-30 15:23             ` Henning Schild
@ 2021-03-31 15:40               ` Andy Shevchenko
  2021-04-01 10:44                 ` Henning Schild
  0 siblings, 1 reply; 40+ messages in thread
From: Andy Shevchenko @ 2021-03-31 15:40 UTC (permalink / raw)
  To: Henning Schild
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Tue, 30 Mar 2021 15:41:53 +0300
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > <henning.schild@siemens.com> wrote:
> > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > <henning.schild@siemens.com> wrote:
> > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > <henning.schild@siemens.com> wrote:
> >
> > > > > > > +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"},
> > > > > > > +       { }
> > > > > > > +};
> > > > > >
> > > > > > It seems to me like poking GPIO controller registers directly.
> > > > > > This is not good. The question still remains: Can we simply
> > > > > > register a GPIO (pin control) driver and use an LED GPIO
> > > > > > driver with an additional board file that instantiates it?
> > > > >
> > > > > I wrote about that in reply to the cover letter. My view is
> > > > > still that it would be an abstraction with only one user, just
> > > > > causing work and likely not ending up as generic as it might
> > > > > eventually have to be.
> > > > >
> > > > > The region is reserved, not sure what the problem with the
> > > > > "poking" is.
> > > >
> > > >
> > > > > Maybe i do not understand all the benefits of such a split at
> > > > > this point in time. At the moment i only see work with hardly
> > > > > any benefit, not just work for me but also for maintainers. I
> > > > > sure do not mean to be ignorant. Maybe you go into details and
> > > > > convince me or we wait for other peoples opinions on how to
> > > > > proceed, maybe there is a second user that i am not aware of?
> > > > > Until i am convinced otherwise i will try to argue that a
> > > > > single-user-abstraction is needless work/code, and should be
> > > > > done only when actually needed.
> > > >
> > > > I have just read your messages (there is a cover letter and
> > > > additional email which was sent lately).
> > > >
> > > > I would like to know what the CPU model number on that board is.
> > > > Than we can continue to see what possibilities we have here.
> > >
> > > I guess we are talking about the one that uses memory mapped, that
> > > is called an "IPC127E" and seems to have either Intel Atom E3940 or
> > > E3930 which seems to be Apollo Lake.
> >
> > Yep. And now the question, in my patch series you should have got the
> > apollolake-pinctrl driver loaded (if not, we have to investigate why
> > it's not being instantiated). This will give you a read GPIO driver.
>
> Ok, so there is the existing driver i asked about several times. Thanks
> for pointing it out.

If you remember, I asked you about the chip twice :-)
I assumed that we were talking about Apollo Lake and that's why I
insisted that the driver is in the kernel source tree.


> > So, you may use regular LED GPIO on top of it
> > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > I would like to understand why it can't be achieved.
>
> Will have a look. Unfortunately this one box is missing in my personal
> collection, but let us assume that one can be converted to that
> existing driver.

OK!

> I guess that will still mean the PIO-based part of the LED driver will
> have to stay as is.

Probably yes. I haven't looked into that part and I have no idea
what's going on on that platform(s).

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-31 15:40               ` Andy Shevchenko
@ 2021-04-01 10:44                 ` Henning Schild
  2021-04-01 11:04                   ` Andy Shevchenko
  0 siblings, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-04-01 10:44 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

Am Wed, 31 Mar 2021 18:40:23 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Tue, 30 Mar 2021 15:41:53 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > <henning.schild@siemens.com> wrote:  
> > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > <henning.schild@siemens.com> wrote:  
> > >  
> > > > > > > > +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"},
> > > > > > > > +       { }
> > > > > > > > +};  
> > > > > > >
> > > > > > > It seems to me like poking GPIO controller registers
> > > > > > > directly. This is not good. The question still remains:
> > > > > > > Can we simply register a GPIO (pin control) driver and
> > > > > > > use an LED GPIO driver with an additional board file that
> > > > > > > instantiates it?  
> > > > > >
> > > > > > I wrote about that in reply to the cover letter. My view is
> > > > > > still that it would be an abstraction with only one user,
> > > > > > just causing work and likely not ending up as generic as it
> > > > > > might eventually have to be.
> > > > > >
> > > > > > The region is reserved, not sure what the problem with the
> > > > > > "poking" is.  
> > > > >
> > > > >  
> > > > > > Maybe i do not understand all the benefits of such a split
> > > > > > at this point in time. At the moment i only see work with
> > > > > > hardly any benefit, not just work for me but also for
> > > > > > maintainers. I sure do not mean to be ignorant. Maybe you
> > > > > > go into details and convince me or we wait for other
> > > > > > peoples opinions on how to proceed, maybe there is a second
> > > > > > user that i am not aware of? Until i am convinced otherwise
> > > > > > i will try to argue that a single-user-abstraction is
> > > > > > needless work/code, and should be done only when actually
> > > > > > needed.  
> > > > >
> > > > > I have just read your messages (there is a cover letter and
> > > > > additional email which was sent lately).
> > > > >
> > > > > I would like to know what the CPU model number on that board
> > > > > is. Than we can continue to see what possibilities we have
> > > > > here.  
> > > >
> > > > I guess we are talking about the one that uses memory mapped,
> > > > that is called an "IPC127E" and seems to have either Intel Atom
> > > > E3940 or E3930 which seems to be Apollo Lake.  
> > >
> > > Yep. And now the question, in my patch series you should have got
> > > the apollolake-pinctrl driver loaded (if not, we have to
> > > investigate why it's not being instantiated). This will give you
> > > a read GPIO driver.  
> >
> > Ok, so there is the existing driver i asked about several times.
> > Thanks for pointing it out.  
> 
> If you remember, I asked you about the chip twice :-)
> I assumed that we were talking about Apollo Lake and that's why I
> insisted that the driver is in the kernel source tree.

Sorry, maybe i did not get the context of your question and which of
the machines you asked about. Now it is clear i guess.

> 
> > > So, you may use regular LED GPIO on top of it
> > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > > I would like to understand why it can't be achieved.  
> >
> > Will have a look. Unfortunately this one box is missing in my
> > personal collection, but let us assume that one can be converted to
> > that existing driver.  
> 
> OK!
> 
> > I guess that will still mean the PIO-based part of the LED driver
> > will have to stay as is.  
> 
> Probably yes. I haven't looked into that part and I have no idea
> what's going on on that platform(s).
> 

Which i guess means the series can be reviewed as if the mmio bits for
that apollo lake would not be in it, maybe i will even send a version
without that one box. We have others in the "backlog" might as well
delay that one if it helps sorting out a base-line.

regards,
Henning

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-04-01 10:44                 ` Henning Schild
@ 2021-04-01 11:04                   ` Andy Shevchenko
  2021-04-12 11:56                     ` Henning Schild
  2021-04-12 15:15                     ` Henning Schild
  0 siblings, 2 replies; 40+ messages in thread
From: Andy Shevchenko @ 2021-04-01 11:04 UTC (permalink / raw)
  To: Henning Schild
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

On Thu, Apr 1, 2021 at 1:44 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Wed, 31 Mar 2021 18:40:23 +0300
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
>
> > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> > <henning.schild@siemens.com> wrote:
> > > Am Tue, 30 Mar 2021 15:41:53 +0300
> > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > > <henning.schild@siemens.com> wrote:
> > > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > > <henning.schild@siemens.com> wrote:
> > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > > <henning.schild@siemens.com> wrote:
> > > >
> > > > > > > > > +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"},
> > > > > > > > > +       { }
> > > > > > > > > +};
> > > > > > > >
> > > > > > > > It seems to me like poking GPIO controller registers
> > > > > > > > directly. This is not good. The question still remains:
> > > > > > > > Can we simply register a GPIO (pin control) driver and
> > > > > > > > use an LED GPIO driver with an additional board file that
> > > > > > > > instantiates it?
> > > > > > >
> > > > > > > I wrote about that in reply to the cover letter. My view is
> > > > > > > still that it would be an abstraction with only one user,
> > > > > > > just causing work and likely not ending up as generic as it
> > > > > > > might eventually have to be.
> > > > > > >
> > > > > > > The region is reserved, not sure what the problem with the
> > > > > > > "poking" is.
> > > > > >
> > > > > >
> > > > > > > Maybe i do not understand all the benefits of such a split
> > > > > > > at this point in time. At the moment i only see work with
> > > > > > > hardly any benefit, not just work for me but also for
> > > > > > > maintainers. I sure do not mean to be ignorant. Maybe you
> > > > > > > go into details and convince me or we wait for other
> > > > > > > peoples opinions on how to proceed, maybe there is a second
> > > > > > > user that i am not aware of? Until i am convinced otherwise
> > > > > > > i will try to argue that a single-user-abstraction is
> > > > > > > needless work/code, and should be done only when actually
> > > > > > > needed.
> > > > > >
> > > > > > I have just read your messages (there is a cover letter and
> > > > > > additional email which was sent lately).
> > > > > >
> > > > > > I would like to know what the CPU model number on that board
> > > > > > is. Than we can continue to see what possibilities we have
> > > > > > here.
> > > > >
> > > > > I guess we are talking about the one that uses memory mapped,
> > > > > that is called an "IPC127E" and seems to have either Intel Atom
> > > > > E3940 or E3930 which seems to be Apollo Lake.
> > > >
> > > > Yep. And now the question, in my patch series you should have got
> > > > the apollolake-pinctrl driver loaded (if not, we have to
> > > > investigate why it's not being instantiated). This will give you
> > > > a read GPIO driver.
> > >
> > > Ok, so there is the existing driver i asked about several times.
> > > Thanks for pointing it out.
> >
> > If you remember, I asked you about the chip twice :-)
> > I assumed that we were talking about Apollo Lake and that's why I
> > insisted that the driver is in the kernel source tree.
>
> Sorry, maybe i did not get the context of your question and which of
> the machines you asked about. Now it is clear i guess.
>
> >
> > > > So, you may use regular LED GPIO on top of it
> > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > > > I would like to understand why it can't be achieved.
> > >
> > > Will have a look. Unfortunately this one box is missing in my
> > > personal collection, but let us assume that one can be converted to
> > > that existing driver.
> >
> > OK!
> >
> > > I guess that will still mean the PIO-based part of the LED driver
> > > will have to stay as is.
> >
> > Probably yes. I haven't looked into that part and I have no idea
> > what's going on on that platform(s).
> >
>
> Which i guess means the series can be reviewed as if the mmio bits for
> that apollo lake would not be in it, maybe i will even send a version
> without that one box. We have others in the "backlog" might as well
> delay that one if it helps sorting out a base-line.

It depends on the role of P2SB in this case.
Shouldn't you drop that completely out from this series?

Otherwise we have to understand what to do with it.
It seems the best approach can be to expose the P2SB device to Linux,
but we have to answer to Bjorn's request about region reservations.


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-29 17:49 ` [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
@ 2021-04-01 16:15   ` Enrico Weigelt, metux IT consult
  2021-04-06 14:52     ` Henning Schild
  2021-04-12 15:35     ` Henning Schild
  2021-11-25 17:08   ` Henning Schild
  2021-11-25 17:10   ` Henning Schild
  2 siblings, 2 replies; 40+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-04-01 16:15 UTC (permalink / raw)
  To: Henning Schild, linux-kernel, linux-leds, platform-driver-x86,
	linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek,
	Andy Shevchenko

On 29.03.21 19:49, Henning Schild wrote:

Hi,

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

Where does the wdt actually come from ?

Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty 
usual case.

Or some external chip ?

The code smells a bit like two entirely different wdt's that just have
some similarities. If that's the case, I'd rather split it into two
separate drivers and let the parent driver (board file) instantiate
the correct one.


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-04-01 16:15   ` Enrico Weigelt, metux IT consult
@ 2021-04-06 14:52     ` Henning Schild
  2021-04-07  8:53       ` Andy Shevchenko
  2021-04-12 15:35     ` Henning Schild
  1 sibling, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-04-06 14:52 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog,
	Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek,
	Andy Shevchenko

Am Thu, 1 Apr 2021 18:15:41 +0200
schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:

> On 29.03.21 19:49, Henning Schild wrote:
> 
> Hi,
> 
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.  
> 
> Where does the wdt actually come from ?
> 
> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty 
> usual case.
>
> Or some external chip ?

I guess external chip, but again we are talking about multiple
machines. And the manuals i read so far do not go into that sort of
detail. In fact on some of the machines you will have two watchdogs,
one from the SoC and that "special" one.
That has several reasons, probably not too important here. The HW guys
are adding another wd not just for fun, and it would be nice to have a
driver.
 
> The code smells a bit like two entirely different wdt's that just have
> some similarities. If that's the case, I'd rather split it into two
> separate drivers and let the parent driver (board file) instantiate
> the correct one.

Yes, it is two. Just like for the LEDs. One version PIO-based another
version gpio/p2sb/mmio based.
In fact the latter should very likely be based on that gpio pinctl,
whether it really needs to be a separate driver will have to be seen.
There are probably pros and cons for both options.

regards,
Henning

> 
> --mtx
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-04-06 14:52     ` Henning Schild
@ 2021-04-07  8:53       ` Andy Shevchenko
  2021-04-07 12:17         ` Guenter Roeck
  0 siblings, 1 reply; 40+ messages in thread
From: Andy Shevchenko @ 2021-04-07  8:53 UTC (permalink / raw)
  To: Henning Schild
  Cc: Enrico Weigelt, metux IT consult, Linux Kernel Mailing List,
	Linux LED Subsystem, Platform Driver, linux-watchdog,
	Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek

On Tue, Apr 6, 2021 at 5:56 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> Am Thu, 1 Apr 2021 18:15:41 +0200
> schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:
>
> > On 29.03.21 19:49, Henning Schild wrote:
> >
> > Hi,
> >
> > > This driver adds initial support for several devices from Siemens.
> > > It is based on a platform driver introduced in an earlier commit.
> >
> > Where does the wdt actually come from ?
> >
> > Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty
> > usual case.
> >
> > Or some external chip ?
>
> I guess external chip, but again we are talking about multiple
> machines. And the manuals i read so far do not go into that sort of
> detail. In fact on some of the machines you will have two watchdogs,
> one from the SoC and that "special" one.
> That has several reasons, probably not too important here. The HW guys
> are adding another wd not just for fun, and it would be nice to have a
> driver.
>
> > The code smells a bit like two entirely different wdt's that just have
> > some similarities. If that's the case, I'd rather split it into two
> > separate drivers and let the parent driver (board file) instantiate
> > the correct one.
>
> Yes, it is two. Just like for the LEDs. One version PIO-based another
> version gpio/p2sb/mmio based.

I tend to agree with Enrico that this should be two separate drivers.

> In fact the latter should very likely be based on that gpio pinctl,
> whether it really needs to be a separate driver will have to be seen.
> There are probably pros and cons for both options.


-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 0/4] add device drivers for Siemens Industrial PCs
  2021-03-29 17:49 [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
                   ` (4 preceding siblings ...)
  2021-03-29 18:00 ` [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
@ 2021-04-07 11:36 ` Hans de Goede
  2021-04-12 11:27   ` Henning Schild
  2021-07-12 11:35   ` Henning Schild
  5 siblings, 2 replies; 40+ messages in thread
From: Hans de Goede @ 2021-04-07 11:36 UTC (permalink / raw)
  To: Henning Schild, linux-kernel, linux-leds, platform-driver-x86,
	linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Pavel Machek, Andy Shevchenko,
	Enrico Weigelt

Hi,

On 3/29/21 7:49 PM, Henning Schild wrote:
> 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.

IT seems there still is significant discussion surrounding the LED and watchdog
drivers which use patch 1/4 as parent-driver.

I'm going to hold of on merging 1/4 and 4/4 until there is more consensus
surrounding this series.

Regards,

Hans


> 
> 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               |  57 +++--
>  drivers/platform/x86/simatic-ipc.c            | 169 ++++++++++++++
>  drivers/watchdog/Kconfig                      |  11 +
>  drivers/watchdog/Makefile                     |   1 +
>  drivers/watchdog/simatic-ipc-wdt.c            | 215 ++++++++++++++++++
>  .../platform_data/x86/simatic-ipc-base.h      |  29 +++
>  include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
>  14 files changed, 769 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
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-04-07  8:53       ` Andy Shevchenko
@ 2021-04-07 12:17         ` Guenter Roeck
  2021-11-26 13:18           ` Henning Schild
  0 siblings, 1 reply; 40+ messages in thread
From: Guenter Roeck @ 2021-04-07 12:17 UTC (permalink / raw)
  To: Andy Shevchenko, Henning Schild
  Cc: Enrico Weigelt, metux IT consult, Linux Kernel Mailing List,
	Linux LED Subsystem, Platform Driver, linux-watchdog,
	Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek

On 4/7/21 1:53 AM, Andy Shevchenko wrote:
> On Tue, Apr 6, 2021 at 5:56 PM Henning Schild
> <henning.schild@siemens.com> wrote:
>>
>> Am Thu, 1 Apr 2021 18:15:41 +0200
>> schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:
>>
>>> On 29.03.21 19:49, Henning Schild wrote:
>>>
>>> Hi,
>>>
>>>> This driver adds initial support for several devices from Siemens.
>>>> It is based on a platform driver introduced in an earlier commit.
>>>
>>> Where does the wdt actually come from ?
>>>
>>> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty
>>> usual case.
>>>
>>> Or some external chip ?
>>
>> I guess external chip, but again we are talking about multiple
>> machines. And the manuals i read so far do not go into that sort of
>> detail. In fact on some of the machines you will have two watchdogs,
>> one from the SoC and that "special" one.
>> That has several reasons, probably not too important here. The HW guys
>> are adding another wd not just for fun, and it would be nice to have a
>> driver.
>>
>>> The code smells a bit like two entirely different wdt's that just have
>>> some similarities. If that's the case, I'd rather split it into two
>>> separate drivers and let the parent driver (board file) instantiate
>>> the correct one.
>>
>> Yes, it is two. Just like for the LEDs. One version PIO-based another
>> version gpio/p2sb/mmio based.
> 
> I tend to agree with Enrico that this should be two separate drivers.
> 

Agreed.

Guenter

>> In fact the latter should very likely be based on that gpio pinctl,
>> whether it really needs to be a separate driver will have to be seen.
>> There are probably pros and cons for both options.
> 
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 0/4] add device drivers for Siemens Industrial PCs
  2021-04-07 11:36 ` Hans de Goede
@ 2021-04-12 11:27   ` Henning Schild
  2021-07-12 11:35   ` Henning Schild
  1 sibling, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-04-12 11:27 UTC (permalink / raw)
  To: Hans de Goede
  Cc: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog,
	Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Pavel Machek, Andy Shevchenko,
	Enrico Weigelt

Am Wed, 7 Apr 2021 13:36:40 +0200
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/29/21 7:49 PM, Henning Schild wrote:
> > 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.  
> 
> IT seems there still is significant discussion surrounding the LED
> and watchdog drivers which use patch 1/4 as parent-driver.
> 
> I'm going to hold of on merging 1/4 and 4/4 until there is more
> consensus surrounding this series.

Yes. Whithout 2 and 3, 1 would be way too big.

Henning

> Regards,
> 
> Hans
> 
> 
> > 
> > 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               |  57 +++--
> >  drivers/platform/x86/simatic-ipc.c            | 169 ++++++++++++++
> >  drivers/watchdog/Kconfig                      |  11 +
> >  drivers/watchdog/Makefile                     |   1 +
> >  drivers/watchdog/simatic-ipc-wdt.c            | 215
> > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h      |
> > 29 +++ include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
> >  14 files changed, 769 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 
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-04-01 11:04                   ` Andy Shevchenko
@ 2021-04-12 11:56                     ` Henning Schild
  2021-05-05 14:58                       ` Enrico Weigelt, metux IT consult
  2021-04-12 15:15                     ` Henning Schild
  1 sibling, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-04-12 11:56 UTC (permalink / raw)
  To: Andy Shevchenko, Enrico Weigelt
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek

Am Thu, 1 Apr 2021 14:04:51 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Apr 1, 2021 at 1:44 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > Am Wed, 31 Mar 2021 18:40:23 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> >  
> > > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> > > > Am Tue, 30 Mar 2021 15:41:53 +0300
> > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > > > <henning.schild@siemens.com> wrote:  
> > > > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > > > <henning.schild@siemens.com> wrote:  
> > > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > > > <henning.schild@siemens.com> wrote:  
> > > > >  
> > > > > > > > > > +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"},
> > > > > > > > > > +       { }
> > > > > > > > > > +};  
> > > > > > > > >
> > > > > > > > > It seems to me like poking GPIO controller registers
> > > > > > > > > directly. This is not good. The question still
> > > > > > > > > remains: Can we simply register a GPIO (pin control)
> > > > > > > > > driver and use an LED GPIO driver with an additional
> > > > > > > > > board file that instantiates it?  
> > > > > > > >
> > > > > > > > I wrote about that in reply to the cover letter. My
> > > > > > > > view is still that it would be an abstraction with only
> > > > > > > > one user, just causing work and likely not ending up as
> > > > > > > > generic as it might eventually have to be.
> > > > > > > >
> > > > > > > > The region is reserved, not sure what the problem with
> > > > > > > > the "poking" is.  
> > > > > > >
> > > > > > >  
> > > > > > > > Maybe i do not understand all the benefits of such a
> > > > > > > > split at this point in time. At the moment i only see
> > > > > > > > work with hardly any benefit, not just work for me but
> > > > > > > > also for maintainers. I sure do not mean to be
> > > > > > > > ignorant. Maybe you go into details and convince me or
> > > > > > > > we wait for other peoples opinions on how to proceed,
> > > > > > > > maybe there is a second user that i am not aware of?
> > > > > > > > Until i am convinced otherwise i will try to argue that
> > > > > > > > a single-user-abstraction is needless work/code, and
> > > > > > > > should be done only when actually needed.  
> > > > > > >
> > > > > > > I have just read your messages (there is a cover letter
> > > > > > > and additional email which was sent lately).
> > > > > > >
> > > > > > > I would like to know what the CPU model number on that
> > > > > > > board is. Than we can continue to see what possibilities
> > > > > > > we have here.  
> > > > > >
> > > > > > I guess we are talking about the one that uses memory
> > > > > > mapped, that is called an "IPC127E" and seems to have
> > > > > > either Intel Atom E3940 or E3930 which seems to be Apollo
> > > > > > Lake.  
> > > > >
> > > > > Yep. And now the question, in my patch series you should have
> > > > > got the apollolake-pinctrl driver loaded (if not, we have to
> > > > > investigate why it's not being instantiated). This will give
> > > > > you a read GPIO driver.  
> > > >
> > > > Ok, so there is the existing driver i asked about several times.
> > > > Thanks for pointing it out.  
> > >
> > > If you remember, I asked you about the chip twice :-)
> > > I assumed that we were talking about Apollo Lake and that's why I
> > > insisted that the driver is in the kernel source tree.  
> >
> > Sorry, maybe i did not get the context of your question and which of
> > the machines you asked about. Now it is clear i guess.
> >  
> > >  
> > > > > So, you may use regular LED GPIO on top of it
> > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > > > > I would like to understand why it can't be achieved.  
> > > >
> > > > Will have a look. Unfortunately this one box is missing in my
> > > > personal collection, but let us assume that one can be
> > > > converted to that existing driver.  
> > >
> > > OK!
> > >  
> > > > I guess that will still mean the PIO-based part of the LED
> > > > driver will have to stay as is.  
> > >
> > > Probably yes. I haven't looked into that part and I have no idea
> > > what's going on on that platform(s).
> > >  
> >
> > Which i guess means the series can be reviewed as if the mmio bits
> > for that apollo lake would not be in it, maybe i will even send a
> > version without that one box. We have others in the "backlog" might
> > as well delay that one if it helps sorting out a base-line.  
> 
> It depends on the role of P2SB in this case.
> Shouldn't you drop that completely out from this series?

We still have one such GPIO bit in one wdt, might be "sunrisepoint".
Still have to clarify if that can maybe be dropped for a first step.

> Otherwise we have to understand what to do with it.
> It seems the best approach can be to expose the P2SB device to Linux,
> but we have to answer to Bjorn's request about region reservations.

I now have such an apollolake to play with. Having your series applied
my LEDs driver fails to alloc that mmio memory, so far so correct.
Still have to figure out how to use those GPIOs.

I was hoping to find a gpiochip in sysfs and be able to export gpios to
userland.

Enrico, does "gpio_amd_fch" show up under /sys/class/gpio as a
gpiochip? Or how to interact with that driver before basing another one
on top?

I am afraid that pinctrl-broxton might be loaded but not working as
expected.

Henning

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-04-01 11:04                   ` Andy Shevchenko
  2021-04-12 11:56                     ` Henning Schild
@ 2021-04-12 15:15                     ` Henning Schild
  1 sibling, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-04-12 15:15 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

Am Thu, 1 Apr 2021 14:04:51 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Apr 1, 2021 at 1:44 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > Am Wed, 31 Mar 2021 18:40:23 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> >  
> > > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> > > > Am Tue, 30 Mar 2021 15:41:53 +0300
> > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > > > <henning.schild@siemens.com> wrote:  
> > > > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > > > <henning.schild@siemens.com> wrote:  
> > > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > > > <henning.schild@siemens.com> wrote:  
> > > > >  
> > > > > > > > > > +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"},
> > > > > > > > > > +       { }
> > > > > > > > > > +};  
> > > > > > > > >
> > > > > > > > > It seems to me like poking GPIO controller registers
> > > > > > > > > directly. This is not good. The question still
> > > > > > > > > remains: Can we simply register a GPIO (pin control)
> > > > > > > > > driver and use an LED GPIO driver with an additional
> > > > > > > > > board file that instantiates it?  
> > > > > > > >
> > > > > > > > I wrote about that in reply to the cover letter. My
> > > > > > > > view is still that it would be an abstraction with only
> > > > > > > > one user, just causing work and likely not ending up as
> > > > > > > > generic as it might eventually have to be.
> > > > > > > >
> > > > > > > > The region is reserved, not sure what the problem with
> > > > > > > > the "poking" is.  
> > > > > > >
> > > > > > >  
> > > > > > > > Maybe i do not understand all the benefits of such a
> > > > > > > > split at this point in time. At the moment i only see
> > > > > > > > work with hardly any benefit, not just work for me but
> > > > > > > > also for maintainers. I sure do not mean to be
> > > > > > > > ignorant. Maybe you go into details and convince me or
> > > > > > > > we wait for other peoples opinions on how to proceed,
> > > > > > > > maybe there is a second user that i am not aware of?
> > > > > > > > Until i am convinced otherwise i will try to argue that
> > > > > > > > a single-user-abstraction is needless work/code, and
> > > > > > > > should be done only when actually needed.  
> > > > > > >
> > > > > > > I have just read your messages (there is a cover letter
> > > > > > > and additional email which was sent lately).
> > > > > > >
> > > > > > > I would like to know what the CPU model number on that
> > > > > > > board is. Than we can continue to see what possibilities
> > > > > > > we have here.  
> > > > > >
> > > > > > I guess we are talking about the one that uses memory
> > > > > > mapped, that is called an "IPC127E" and seems to have
> > > > > > either Intel Atom E3940 or E3930 which seems to be Apollo
> > > > > > Lake.  
> > > > >
> > > > > Yep. And now the question, in my patch series you should have
> > > > > got the apollolake-pinctrl driver loaded (if not, we have to
> > > > > investigate why it's not being instantiated). This will give
> > > > > you a read GPIO driver.  
> > > >
> > > > Ok, so there is the existing driver i asked about several times.
> > > > Thanks for pointing it out.  
> > >
> > > If you remember, I asked you about the chip twice :-)
> > > I assumed that we were talking about Apollo Lake and that's why I
> > > insisted that the driver is in the kernel source tree.  
> >
> > Sorry, maybe i did not get the context of your question and which of
> > the machines you asked about. Now it is clear i guess.
> >  
> > >  
> > > > > So, you may use regular LED GPIO on top of it
> > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > > > > I would like to understand why it can't be achieved.  
> > > >
> > > > Will have a look. Unfortunately this one box is missing in my
> > > > personal collection, but let us assume that one can be
> > > > converted to that existing driver.  
> > >
> > > OK!
> > >  
> > > > I guess that will still mean the PIO-based part of the LED
> > > > driver will have to stay as is.  
> > >
> > > Probably yes. I haven't looked into that part and I have no idea
> > > what's going on on that platform(s).
> > >  
> >
> > Which i guess means the series can be reviewed as if the mmio bits
> > for that apollo lake would not be in it, maybe i will even send a
> > version without that one box. We have others in the "backlog" might
> > as well delay that one if it helps sorting out a base-line.  
> 
> It depends on the role of P2SB in this case.
> Shouldn't you drop that completely out from this series?

Unfortunately the WDT uses one P2SB-GPIO pin as well (for 1 of the two
machine types it supports). Dropping would mean loosing 1/5 machines in
LED and 2/4 in WDT. So i rather let this series sit until the P2SB
stuff is sorted out.
But that would just be my personal "preference". We could go "divide
and conquer", shrink the number of supported devices and drop all that
needs P2SB ... also a valid way, we have the platform-abstraction to
build upon ... and we would get p4 out of the way. 

Henning

> Otherwise we have to understand what to do with it.
> It seems the best approach can be to expose the P2SB device to Linux,
> but we have to answer to Bjorn's request about region reservations.
> 
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-04-01 16:15   ` Enrico Weigelt, metux IT consult
  2021-04-06 14:52     ` Henning Schild
@ 2021-04-12 15:35     ` Henning Schild
  2021-04-12 16:06       ` Guenter Roeck
  1 sibling, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-04-12 15:35 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult, Andy Shevchenko
  Cc: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog,
	Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek

Am Thu, 1 Apr 2021 18:15:41 +0200
schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:

> On 29.03.21 19:49, Henning Schild wrote:
> 
> Hi,
> 
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.  
> 
> Where does the wdt actually come from ?
> 
> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty 
> usual case.
> 
> Or some external chip ?
> 
> The code smells a bit like two entirely different wdt's that just have
> some similarities. If that's the case, I'd rather split it into two
> separate drivers and let the parent driver (board file) instantiate
> the correct one.

In fact they are the same watchdog device. The only difference is the
"secondary enable" which controls whether the watchdog causes a reboot
or just raises an alarm. The alarm feature is not even implemented in
the given driver, we just enable that secondary enable regardless.

In one range of devices (227E) that second enable is part of a
pio-based control register. On the other range (427E) it unfortunately
is a P2SB gpio, which gets us right into the discussion we have around
the LEDs.
With that i have my doubts that two drivers would be the way to go,
most likely not. 

Only that i have no clue which pinctrl driver should be used here. My
guess is "sunrisepoint" because the CPUs are "SkyLake" i.e. i5-6442EQ,
i3-6102E
And "grep INT344B /sys/firmware/acpi/tables/DSDT" matches. I booted a
kernel patched with the series from Andy but the "pinctrl-sunrisepoint"
does not seem to even claim the memory. Still trying to understand how
to make use of these pinctrl drivers they are in place but i lack
example users (drivers). If they should be available in sysfs, i might
be looking at the wrong place ... /sys/class/gpio/ does not list
anything

regards,
Henning



> 
> --mtx
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-04-12 15:35     ` Henning Schild
@ 2021-04-12 16:06       ` Guenter Roeck
  2021-04-12 16:17         ` Henning Schild
  0 siblings, 1 reply; 40+ messages in thread
From: Guenter Roeck @ 2021-04-12 16:06 UTC (permalink / raw)
  To: Henning Schild, Enrico Weigelt, metux IT consult, Andy Shevchenko
  Cc: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog,
	Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek

On 4/12/21 8:35 AM, Henning Schild wrote:
> Am Thu, 1 Apr 2021 18:15:41 +0200
> schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:
> 
>> On 29.03.21 19:49, Henning Schild wrote:
>>
>> Hi,
>>
>>> This driver adds initial support for several devices from Siemens.
>>> It is based on a platform driver introduced in an earlier commit.  
>>
>> Where does the wdt actually come from ?
>>
>> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty 
>> usual case.
>>
>> Or some external chip ?
>>
>> The code smells a bit like two entirely different wdt's that just have
>> some similarities. If that's the case, I'd rather split it into two
>> separate drivers and let the parent driver (board file) instantiate
>> the correct one.
> 
> In fact they are the same watchdog device. The only difference is the
> "secondary enable" which controls whether the watchdog causes a reboot
> or just raises an alarm. The alarm feature is not even implemented in
> the given driver, we just enable that secondary enable regardless.
> 

Confusing statement; I can't parse "we just enable that secondary enable
regardless". What secondary enable do you enable ?

The code says "set safe_en_n so we are not just WDIOF_ALARMONLY", which
suggests that it disables the alarm feature, and does make sense.

> In one range of devices (227E) that second enable is part of a
> pio-based control register. On the other range (427E) it unfortunately
> is a P2SB gpio, which gets us right into the discussion we have around
> the LEDs.
> With that i have my doubts that two drivers would be the way to go,
> most likely not. 
> 

Reading the code again, I agree. Still, you'll need to sort out how
to determine if the watchdog or the LED driver should be enabled,
and how to access the gpio port. The GPIO pin detection and use
for 427E is a bit awkward.

Thanks,
Guenter

> Only that i have no clue which pinctrl driver should be used here. My
> guess is "sunrisepoint" because the CPUs are "SkyLake" i.e. i5-6442EQ,
> i3-6102E
> And "grep INT344B /sys/firmware/acpi/tables/DSDT" matches. I booted a
> kernel patched with the series from Andy but the "pinctrl-sunrisepoint"
> does not seem to even claim the memory. Still trying to understand how
> to make use of these pinctrl drivers they are in place but i lack
> example users (drivers). If they should be available in sysfs, i might
> be looking at the wrong place ... /sys/class/gpio/ does not list
> anything
> 
> regards,
> Henning
> 
> 
> 
>>
>> --mtx
>>
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-04-12 16:06       ` Guenter Roeck
@ 2021-04-12 16:17         ` Henning Schild
  0 siblings, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-04-12 16:17 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Enrico Weigelt, metux IT consult, Andy Shevchenko, linux-kernel,
	linux-leds, platform-driver-x86, linux-watchdog,
	Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek

Am Mon, 12 Apr 2021 09:06:10 -0700
schrieb Guenter Roeck <linux@roeck-us.net>:

> On 4/12/21 8:35 AM, Henning Schild wrote:
> > Am Thu, 1 Apr 2021 18:15:41 +0200
> > schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:
> >   
> >> On 29.03.21 19:49, Henning Schild wrote:
> >>
> >> Hi,
> >>  
> >>> This driver adds initial support for several devices from Siemens.
> >>> It is based on a platform driver introduced in an earlier commit.
> >>>    
> >>
> >> Where does the wdt actually come from ?
> >>
> >> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a
> >> pretty usual case.
> >>
> >> Or some external chip ?
> >>
> >> The code smells a bit like two entirely different wdt's that just
> >> have some similarities. If that's the case, I'd rather split it
> >> into two separate drivers and let the parent driver (board file)
> >> instantiate the correct one.  
> > 
> > In fact they are the same watchdog device. The only difference is
> > the "secondary enable" which controls whether the watchdog causes a
> > reboot or just raises an alarm. The alarm feature is not even
> > implemented in the given driver, we just enable that secondary
> > enable regardless. 
> 
> Confusing statement; I can't parse "we just enable that secondary
> enable regardless". What secondary enable do you enable ?
>
> The code says "set safe_en_n so we are not just WDIOF_ALARMONLY",
> which suggests that it disables the alarm feature, and does make
> sense.

Yes go with the second statement. But the alarm is the default after
boot, and turning it off needs to be done with p2sb gpio on the 427.
 
> > In one range of devices (227E) that second enable is part of a
> > pio-based control register. On the other range (427E) it
> > unfortunately is a P2SB gpio, which gets us right into the
> > discussion we have around the LEDs.
> > With that i have my doubts that two drivers would be the way to go,
> > most likely not. 
> >   
> 
> Reading the code again, I agree. Still, you'll need to sort out how
> to determine if the watchdog or the LED driver should be enabled,
> and how to access the gpio port. The GPIO pin detection and use
> for 427E is a bit awkward.

Yes it is awkward, and that is exactly the discussion happening for the
LEDs. Using generic GPIO code, the mail was more to Andy as i am hoping
he might help me connect the dots here. On the other hand i wanted wdt
discussions in the wdt thread, and not talk about that one gpio-pin in
the LED thread.

regards,
Henning

> Thanks,
> Guenter
> 
> > Only that i have no clue which pinctrl driver should be used here.
> > My guess is "sunrisepoint" because the CPUs are "SkyLake" i.e.
> > i5-6442EQ, i3-6102E
> > And "grep INT344B /sys/firmware/acpi/tables/DSDT" matches. I booted
> > a kernel patched with the series from Andy but the
> > "pinctrl-sunrisepoint" does not seem to even claim the memory.
> > Still trying to understand how to make use of these pinctrl drivers
> > they are in place but i lack example users (drivers). If they
> > should be available in sysfs, i might be looking at the wrong place
> > ... /sys/class/gpio/ does not list anything
> > 
> > regards,
> > Henning
> > 
> > 
> >   
> >>
> >> --mtx
> >>  
> >   
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-04-12 11:56                     ` Henning Schild
@ 2021-05-05 14:58                       ` Enrico Weigelt, metux IT consult
  0 siblings, 0 replies; 40+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-05-05 14:58 UTC (permalink / raw)
  To: Henning Schild, Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek

On 12.04.21 13:56, Henning Schild wrote:

> Enrico, does "gpio_amd_fch" show up under /sys/class/gpio as a
> gpiochip? Or how to interact with that driver before basing another one
> on top?

It's not probed on its own, but explicitly by a board specific driver,
as it needs board specific data.

See drivers/platform/x86/pcengines-apuv2.c


--mtx

-- 
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
info@metux.net -- +49-151-27565287

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 0/4] add device drivers for Siemens Industrial PCs
  2021-04-07 11:36 ` Hans de Goede
  2021-04-12 11:27   ` Henning Schild
@ 2021-07-12 11:35   ` Henning Schild
  2021-07-12 12:09     ` Andy Shevchenko
  1 sibling, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-07-12 11:35 UTC (permalink / raw)
  To: Hans de Goede, Andy Shevchenko
  Cc: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog,
	Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Pavel Machek, Enrico Weigelt

This series is basically stuck because people rightfully want me to use
the GPIO subsystem for the LEDs and the watchdog bits that are
connected to GPIO.

Problem is that the GPIO subsystem does not initialize on the machines
in question. It is a combination of hidden P2SB and missing ACPI table
entries. The GPIO subsystem (intel pinctrl) needs either P2SB or ACPI do
come up ...

Andy proposed some patches for initializing the intel pinctrl stuff for
one of the machines by falling back to SoC detection in case there is
no ACPI or visible P2SB. While that works it would need to be done for
any Intel SoC to be consistent and discussions seem to go nowhere.

I would be willing to port over to "intel pintctl" and help with
testing, but not so much with actual coding. Andy is that moving at all?

Since my drivers do reserve the mmio regions properly and the intel
pinctrl will never come up anyways, i do not see a conflict merging my
proposed drivers in the current codebase. The wish to use the pinctrl
infrastructure can not be fulfilled if that infra is not in place. Once
intel pinctrl works, we can change those drivers to work with that.

I do not want to take shortcuts ... but also do not want to get stuck
here. So maybe one way to serialize the merge is to allow my changes
like proposed and rebase on intel pinctrl once that subsystem actually
initializes on these machines. We could even have two code paths ... if
region can not be reserved, try gpio ... or the other way around.

regards,
Henning

Am Wed, 7 Apr 2021 13:36:40 +0200
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/29/21 7:49 PM, Henning Schild wrote:
> > 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.  
> 
> IT seems there still is significant discussion surrounding the LED
> and watchdog drivers which use patch 1/4 as parent-driver.
> 
> I'm going to hold of on merging 1/4 and 4/4 until there is more
> consensus surrounding this series.
> 
> Regards,
> 
> Hans
> 
> 
> > 
> > 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               |  57 +++--
> >  drivers/platform/x86/simatic-ipc.c            | 169 ++++++++++++++
> >  drivers/watchdog/Kconfig                      |  11 +
> >  drivers/watchdog/Makefile                     |   1 +
> >  drivers/watchdog/simatic-ipc-wdt.c            | 215
> > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h      |
> > 29 +++ include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++
> >  14 files changed, 769 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 
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 0/4] add device drivers for Siemens Industrial PCs
  2021-07-12 11:35   ` Henning Schild
@ 2021-07-12 12:09     ` Andy Shevchenko
  2021-07-12 16:11       ` Henning Schild
  0 siblings, 1 reply; 40+ messages in thread
From: Andy Shevchenko @ 2021-07-12 12:09 UTC (permalink / raw)
  To: Henning Schild
  Cc: Hans de Goede, Linux Kernel Mailing List, Linux LED Subsystem,
	Platform Driver, linux-watchdog, Srikanth Krishnakar, Jan Kiszka,
	Gerd Haeussler, Guenter Roeck, Wim Van Sebroeck, Mark Gross,
	Pavel Machek, Enrico Weigelt

On Mon, Jul 12, 2021 at 2:35 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> This series is basically stuck because people rightfully want me to use
> the GPIO subsystem for the LEDs and the watchdog bits that are
> connected to GPIO.
>
> Problem is that the GPIO subsystem does not initialize on the machines
> in question. It is a combination of hidden P2SB and missing ACPI table
> entries. The GPIO subsystem (intel pinctrl) needs either P2SB or ACPI do
> come up ...
>
> Andy proposed some patches for initializing the intel pinctrl stuff for
> one of the machines by falling back to SoC detection in case there is
> no ACPI or visible P2SB. While that works it would need to be done for
> any Intel SoC to be consistent and discussions seem to go nowhere.
>
> I would be willing to port over to "intel pintctl" and help with
> testing, but not so much with actual coding. Andy is that moving at all?
>
> Since my drivers do reserve the mmio regions properly and the intel
> pinctrl will never come up anyways, i do not see a conflict merging my
> proposed drivers in the current codebase. The wish to use the pinctrl
> infrastructure can not be fulfilled if that infra is not in place. Once
> intel pinctrl works, we can change those drivers to work with that.
>
> I do not want to take shortcuts ... but also do not want to get stuck
> here. So maybe one way to serialize the merge is to allow my changes
> like proposed and rebase on intel pinctrl once that subsystem actually
> initializes on these machines. We could even have two code paths ... if
> region can not be reserved, try gpio ... or the other way around.

Bjorn suggested exercising the IORESOURCE_PCI_FIXED on top of the
early PCI quirk that unhides P2SB for the entire run time. But I have
had no time to actually patch the kernel this way. Have tried the
proposed approach on your side?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 0/4] add device drivers for Siemens Industrial PCs
  2021-07-12 12:09     ` Andy Shevchenko
@ 2021-07-12 16:11       ` Henning Schild
  0 siblings, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-07-12 16:11 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Hans de Goede, Linux Kernel Mailing List, Linux LED Subsystem,
	Platform Driver, linux-watchdog, Srikanth Krishnakar, Jan Kiszka,
	Gerd Haeussler, Guenter Roeck, Wim Van Sebroeck, Mark Gross,
	Pavel Machek, Enrico Weigelt

Am Mon, 12 Jul 2021 15:09:04 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Mon, Jul 12, 2021 at 2:35 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > This series is basically stuck because people rightfully want me to
> > use the GPIO subsystem for the LEDs and the watchdog bits that are
> > connected to GPIO.
> >
> > Problem is that the GPIO subsystem does not initialize on the
> > machines in question. It is a combination of hidden P2SB and
> > missing ACPI table entries. The GPIO subsystem (intel pinctrl)
> > needs either P2SB or ACPI do come up ...
> >
> > Andy proposed some patches for initializing the intel pinctrl stuff
> > for one of the machines by falling back to SoC detection in case
> > there is no ACPI or visible P2SB. While that works it would need to
> > be done for any Intel SoC to be consistent and discussions seem to
> > go nowhere.
> >
> > I would be willing to port over to "intel pintctl" and help with
> > testing, but not so much with actual coding. Andy is that moving at
> > all?
> >
> > Since my drivers do reserve the mmio regions properly and the intel
> > pinctrl will never come up anyways, i do not see a conflict merging
> > my proposed drivers in the current codebase. The wish to use the
> > pinctrl infrastructure can not be fulfilled if that infra is not in
> > place. Once intel pinctrl works, we can change those drivers to
> > work with that.
> >
> > I do not want to take shortcuts ... but also do not want to get
> > stuck here. So maybe one way to serialize the merge is to allow my
> > changes like proposed and rebase on intel pinctrl once that
> > subsystem actually initializes on these machines. We could even
> > have two code paths ... if region can not be reserved, try gpio ...
> > or the other way around.  
> 
> Bjorn suggested exercising the IORESOURCE_PCI_FIXED on top of the
> early PCI quirk that unhides P2SB for the entire run time. But I have
> had no time to actually patch the kernel this way. Have tried the
> proposed approach on your side?

Unhiding the P2SB (even if permanent and fixed) alone will not trigger
pinctrl to initialize. One would still need something along the lines
of "mfd: lpc_ich: Add support for pinctrl in non-ACPI system" for all
SoCs.

I guess it could be an improvement to your series, but i honestly do
not see all fitting together too soon. Since your p2sb series still
initializes the GPIO with two different names (depending on whether it
was PCI or ACPI) and only for one SoC, while this series would need two
... and a consistent solution many more.

Henning



^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-29 17:49 ` [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
  2021-04-01 16:15   ` Enrico Weigelt, metux IT consult
@ 2021-11-25 17:08   ` Henning Schild
  2021-11-25 17:10   ` Henning Schild
  2 siblings, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-11-25 17:08 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek,
	Andy Shevchenko, Enrico Weigelt

Am Mon, 29 Mar 2021 19:49:27 +0200
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.
> 
> 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 | 215
> +++++++++++++++++++++++++++++ 3 files changed, 227 insertions(+)
>  create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 1fe0042a48d2..948497eb4bef 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1575,6 +1575,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 f3a6540e725e..7f5c73ec058c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -142,6 +142,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..e901718d05b9
> --- /dev/null
> +++ b/drivers/watchdog/simatic-ipc-wdt.c
> @@ -0,0 +1,215 @@
> +// 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 =
> +	DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1,
> +			    KBUILD_MODNAME " WD_ENABLE_IOADR");

WD_TRIGGER_IOADR, SZ_1 missing here and in request_region part

Henning

> +/* 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 = inw(GP_STATUS_REG_227E);
> +		outw(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.start,
> +				 resource_size(&io_resource),
> +				 io_resource.name)) {
> +		dev_err(dev,
> +			"Unable to register IO resource at %#x\n",
> +			WD_ENABLE_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>");


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-29 17:49 ` [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
  2021-04-01 16:15   ` Enrico Weigelt, metux IT consult
  2021-11-25 17:08   ` Henning Schild
@ 2021-11-25 17:10   ` Henning Schild
  2 siblings, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-11-25 17:10 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek,
	Andy Shevchenko, Enrico Weigelt

Am Mon, 29 Mar 2021 19:49:27 +0200
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.
> 
> 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 | 215
> +++++++++++++++++++++++++++++ 3 files changed, 227 insertions(+)
>  create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 1fe0042a48d2..948497eb4bef 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1575,6 +1575,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 f3a6540e725e..7f5c73ec058c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -142,6 +142,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..e901718d05b9
> --- /dev/null
> +++ b/drivers/watchdog/simatic-ipc-wdt.c
> @@ -0,0 +1,215 @@
> +// 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 =
> +	DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1,
> +			    KBUILD_MODNAME " WD_ENABLE_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 = inw(GP_STATUS_REG_227E);
> +		outw(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E);

Should be inb/outb we just have an SZ_1 region.

Henning

> +	} 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.start,
> +				 resource_size(&io_resource),
> +				 io_resource.name)) {
> +		dev_err(dev,
> +			"Unable to register IO resource at %#x\n",
> +			WD_ENABLE_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>");


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-29 17:49 ` [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
  2021-03-30 11:04   ` Andy Shevchenko
@ 2021-11-25 17:11   ` Henning Schild
  1 sibling, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-11-25 17:11 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek,
	Andy Shevchenko, Enrico Weigelt

Am Mon, 29 Mar 2021 19:49:26 +0200
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.
> 
> 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 b6742b4231bf..5c8558a4fa60 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -937,4 +937,7 @@ source "drivers/leds/trigger/Kconfig"
>  comment "LED Blink"
>  source "drivers/leds/blink/Kconfig"
>  
> +comment "Simple LED drivers"
> +source "drivers/leds/simple/Kconfig"
> +
>  endif # NEW_LEDS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 2a698df9da57..2de7fdd8d629 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -111,3 +111,6 @@ obj-$(CONFIG_LEDS_TRIGGERS)		+=
> trigger/ 
>  # LED Blink
>  obj-$(CONFIG_LEDS_BLINK)                += 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..043edbf81b76
> --- /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_1,
> KBUILD_MODNAME);

Should be SZ_2

Henning

> +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>");


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-29 17:49 ` [PATCH v3 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
@ 2021-11-26 12:39   ` Henning Schild
  0 siblings, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-11-26 12:39 UTC (permalink / raw)
  To: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek,
	Andy Shevchenko, Enrico Weigelt

Am Mon, 29 Mar 2021 19:49:25 +0200
schrieb Henning Schild <henning.schild@siemens.com>:

> 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.
> 
> 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            | 169
> ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h      |
> 29 +++ include/linux/platform_data/x86/simatic-ipc.h |  72 ++++++++
>  5 files changed, 285 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 461ec61530eb..1eaa03d0d183 100644
> --- a/drivers/platform/x86/Kconfig
> +++ b/drivers/platform/x86/Kconfig
> @@ -1289,6 +1289,18 @@ config INTEL_TELEMETRY
>  	  directly via debugfs files. Various tools may use
>  	  this interface for SoC state monitoring.
>  
> +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 60d554073749..26cdebf2e701
> 100644 --- a/drivers/platform/x86/Makefile
> +++ b/drivers/platform/x86/Makefile
> @@ -138,3 +138,6 @@ obj-$(CONFIG_INTEL_TELEMETRY)		+=
> intel_telemetry_core.o \ intel_telemetry_pltdrv.o \
>  					   intel_telemetry_debugfs.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..52e8596bc63d
> --- /dev/null
> +++ b/drivers/platform/x86/simatic-ipc.c
> @@ -0,0 +1,169 @@
> +// 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;
> +}
> +
> +

extra newline, should be removed

> +/* FIXME: this should eventually be done with generic P2SB discovery
> code */ +/*
> + * Get membase address from PCI, used in leds and wdt modul. Here we
> read

/modul/module/

was pointed out already but forgotten about

Henning

> + * 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 */


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-04-07 12:17         ` Guenter Roeck
@ 2021-11-26 13:18           ` Henning Schild
  0 siblings, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-11-26 13:18 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Andy Shevchenko, Enrico Weigelt, metux IT consult,
	Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek

Am Wed, 7 Apr 2021 05:17:12 -0700
schrieb Guenter Roeck <linux@roeck-us.net>:

> On 4/7/21 1:53 AM, Andy Shevchenko wrote:
> > On Tue, Apr 6, 2021 at 5:56 PM Henning Schild
> > <henning.schild@siemens.com> wrote:  
> >>
> >> Am Thu, 1 Apr 2021 18:15:41 +0200
> >> schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:
> >>  
> >>> On 29.03.21 19:49, Henning Schild wrote:
> >>>
> >>> Hi,
> >>>  
> >>>> This driver adds initial support for several devices from
> >>>> Siemens. It is based on a platform driver introduced in an
> >>>> earlier commit.  
> >>>
> >>> Where does the wdt actually come from ?
> >>>
> >>> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a
> >>> pretty usual case.
> >>>
> >>> Or some external chip ?  
> >>
> >> I guess external chip, but again we are talking about multiple
> >> machines. And the manuals i read so far do not go into that sort of
> >> detail. In fact on some of the machines you will have two
> >> watchdogs, one from the SoC and that "special" one.
> >> That has several reasons, probably not too important here. The HW
> >> guys are adding another wd not just for fun, and it would be nice
> >> to have a driver.
> >>  
> >>> The code smells a bit like two entirely different wdt's that just
> >>> have some similarities. If that's the case, I'd rather split it
> >>> into two separate drivers and let the parent driver (board file)
> >>> instantiate the correct one.  
> >>
> >> Yes, it is two. Just like for the LEDs. One version PIO-based
> >> another version gpio/p2sb/mmio based.  
> > 
> > I tend to agree with Enrico that this should be two separate
> > drivers. 
> 
> Agreed.
> 
> Guenter

I will ignore the wish for a split in v4. Reason is that it would cause
a lot of duplication are spreading code over many files. like i.e. a
-common.c

Internally we have that driver in fact already support a few more
machines, which could call a split at some point. Or could also upset
people of the many CONFIG_ knobs and files as we keep pushing machine
support forward in the upstream drivers.

But i would like to discuss that in patch qs coming after a merge and
not split (maybe not yet).

Also splitting wdt and having leds in one file would be inconsistent.
So when there will be a split it should be on both ends. But please
allow me to postpone that.

regards,
Henning

> >> In fact the latter should very likely be based on that gpio pinctl,
> >> whether it really needs to be a separate driver will have to be
> >> seen. There are probably pros and cons for both options.  
> > 
> >   
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-30 11:04   ` Andy Shevchenko
  2021-03-30 11:58     ` Henning Schild
@ 2021-11-26 13:28     ` Henning Schild
  2021-11-26 14:02       ` Andy Shevchenko
  1 sibling, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-11-26 13:28 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

Am Tue, 30 Mar 2021 14:04:35 +0300
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.  
> 
> ...
> 
> > +#define SIMATIC_IPC_LED_PORT_BASE      0x404E  
> 
> > +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" },
> > +       { }
> > +};  
> 
> > +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"},
> > +       { }
> > +};  
> 
> It seems to me like poking GPIO controller registers directly. This
> is not good. The question still remains: Can we simply register a
> GPIO (pin control) driver and use an LED GPIO driver with an
> additional board file that instantiates it?

The short answer for v4 will be "No we can not!". The pinctrl drivers
do not currently probe on any of the devices and attempts to fix that
have failed or gut stuck. I tried to help out where i could and waited
for a long time.

Now my take is to turn the order around. We go in like that and will
happily switch to pinctrl if that ever comes up on the machines.
Meaning P2SB series on top of this, no more delays please.
We do use request_region so have a mutex in place. Meaning we really
only touch GPIO while pinctrl does not!
I see no issue here, waited for a long time and now expect to be
allowed to get merged first.

Henning

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-11-26 13:28     ` Henning Schild
@ 2021-11-26 14:02       ` Andy Shevchenko
  2021-11-26 14:44         ` Henning Schild
  0 siblings, 1 reply; 40+ messages in thread
From: Andy Shevchenko @ 2021-11-26 14:02 UTC (permalink / raw)
  To: Henning Schild
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

On Fri, Nov 26, 2021 at 3:28 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Tue, 30 Mar 2021 14:04:35 +0300
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > <henning.schild@siemens.com> wrote:

...

> > > +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"},
> > > +       { }
> > > +};
> >
> > It seems to me like poking GPIO controller registers directly. This
> > is not good. The question still remains: Can we simply register a
> > GPIO (pin control) driver and use an LED GPIO driver with an
> > additional board file that instantiates it?
>
> The short answer for v4 will be "No we can not!". The pinctrl drivers
> do not currently probe on any of the devices and attempts to fix that
> have failed or gut stuck. I tried to help out where i could and waited
> for a long time.

I see, unfortunately I have stuck with some other (more important
tasks) and can't fulfil this, but I still consider it's no go for
driver poking pin control registers directly. Lemme see if I can
prioritize this for next week.

> Now my take is to turn the order around. We go in like that and will
> happily switch to pinctrl if that ever comes up on the machines.
> Meaning P2SB series on top of this, no more delays please.

I don't want to slip bad code into the kernel where we can avoid that.

> We do use request_region so have a mutex in place. Meaning we really
> only touch GPIO while pinctrl does not!

I haven't got this. On Intel SoCs GPIO is a part of pin control
registers. You can't touch GPIO without touching pin control.

> I see no issue here, waited for a long time and now expect to be
> allowed to get merged first.

Okay, I have these questions / asks so far:
1) Can firmware be fixed in order to provide an ACPI table for the pin
control devices?
2) Can you share firmware (BIOS ROM file I suppose) that I may flash
on an Apollo Lake machine and see if I can reproduce the issue?
3) As may be a last resort, can you share (remotely) or even send to
us the device in question to try?

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-11-26 14:02       ` Andy Shevchenko
@ 2021-11-26 14:44         ` Henning Schild
  2021-11-26 14:59           ` Andy Shevchenko
  0 siblings, 1 reply; 40+ messages in thread
From: Henning Schild @ 2021-11-26 14:44 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

Am Fri, 26 Nov 2021 16:02:48 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Fri, Nov 26, 2021 at 3:28 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Tue, 30 Mar 2021 14:04:35 +0300
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> 
> ...
> 
> > > > +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"},
> > > > +       { }
> > > > +};  
> > >
> > > It seems to me like poking GPIO controller registers directly.
> > > This is not good. The question still remains: Can we simply
> > > register a GPIO (pin control) driver and use an LED GPIO driver
> > > with an additional board file that instantiates it?  
> >
> > The short answer for v4 will be "No we can not!". The pinctrl
> > drivers do not currently probe on any of the devices and attempts
> > to fix that have failed or gut stuck. I tried to help out where i
> > could and waited for a long time.  
> 
> I see, unfortunately I have stuck with some other (more important
> tasks) and can't fulfil this, but I still consider it's no go for
> driver poking pin control registers directly. Lemme see if I can
> prioritize this for next week.

I just sent v4. And am sick of waiting on you. Sorry to be that clear
here. I want that order changed! If you still end up being fast,
perfect. But i want to be faster!

> > Now my take is to turn the order around. We go in like that and will
> > happily switch to pinctrl if that ever comes up on the machines.
> > Meaning P2SB series on top of this, no more delays please.  
> 
> I don't want to slip bad code into the kernel where we can avoid that.

It is not bad code! That is unfair to say. It can be improved on and
that is what we have a FIXME line for. The worst code is code that is
not there ... devices without drivers!
That is bad, not i minor poke of parts of a resource no other driver
claimed!

> > We do use request_region so have a mutex in place. Meaning we really
> > only touch GPIO while pinctrl does not!  
> 
> I haven't got this. On Intel SoCs GPIO is a part of pin control
> registers. You can't touch GPIO without touching pin control.

i meant pin control, if it ever did probe it would reserve the region
and push our hack out, or the other way around ... no conflict!
To get both we just need a simple patch and switch to pinctrl, just
notify me once your stuff is ready and i will write that patch.
 
> > I see no issue here, waited for a long time and now expect to be
> > allowed to get merged first.  
> 
> Okay, I have these questions / asks so far:
> 1) Can firmware be fixed in order to provide an ACPI table for the pin
> control devices?

No. The firmware will only receive security but no feature updates ...

> 2) Can you share firmware (BIOS ROM file I suppose) that I may flash
> on an Apollo Lake machine and see if I can reproduce the issue?

I do not have access. But all you need is a firware with no ACPI entry
and P2SB hidden. Or simply patch out the two probe paths ;). 

> 3) As may be a last resort, can you share (remotely) or even send to
> us the device in question to try?

We are talking about multiple devices. Not just that one apollo lake on
which your patches kind of worked.

But showed some weirdness which could really become a problem if
someone decided to add an ACPI entry ..

It pin 42 name could be 
GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 42
or
GPIO_LOOKUP_IDX("INT3452:01", 42

I guess that conflict will have to be dealt with before your can switch
to probing pinctrl drivers based on cpu model and not only ACPI/P2SB any
longer.

regards,
Henning

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-11-26 14:44         ` Henning Schild
@ 2021-11-26 14:59           ` Andy Shevchenko
  2021-11-26 19:54             ` Henning Schild
  0 siblings, 1 reply; 40+ messages in thread
From: Andy Shevchenko @ 2021-11-26 14:59 UTC (permalink / raw)
  To: Henning Schild
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

On Fri, Nov 26, 2021 at 4:44 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Fri, 26 Nov 2021 16:02:48 +0200
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Fri, Nov 26, 2021 at 3:28 PM Henning Schild
> > <henning.schild@siemens.com> wrote:
> > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > <henning.schild@siemens.com> wrote:

...

> > > > > +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"},
> > > > > +       { }
> > > > > +};
> > > >
> > > > It seems to me like poking GPIO controller registers directly.
> > > > This is not good. The question still remains: Can we simply
> > > > register a GPIO (pin control) driver and use an LED GPIO driver
> > > > with an additional board file that instantiates it?
> > >
> > > The short answer for v4 will be "No we can not!". The pinctrl
> > > drivers do not currently probe on any of the devices and attempts
> > > to fix that have failed or gut stuck. I tried to help out where i
> > > could and waited for a long time.
> >
> > I see, unfortunately I have stuck with some other (more important
> > tasks) and can't fulfil this, but I still consider it's no go for
> > driver poking pin control registers directly. Lemme see if I can
> > prioritize this for next week.
>
> I just sent v4. And am sick of waiting on you. Sorry to be that clear
> here. I want that order changed! If you still end up being fast,
> perfect. But i want to be faster!

It's good that you are honest, honesty is what we missed a lot!

> > > Now my take is to turn the order around. We go in like that and will
> > > happily switch to pinctrl if that ever comes up on the machines.
> > > Meaning P2SB series on top of this, no more delays please.
> >
> > I don't want to slip bad code into the kernel where we can avoid that.
>
> It is not bad code! That is unfair to say. It can be improved on and
> that is what we have a FIXME line for. The worst code is code that is
> not there ... devices without drivers!

Okay, that's how you interpret the term "bad". Probably I had to use
something else to explain that it's racy with the very same case if
one adds an ACPI support to it.

> That is bad, not i minor poke of parts of a resource no other driver
> claimed!
>
> > > We do use request_region so have a mutex in place. Meaning we really
> > > only touch GPIO while pinctrl does not!
> >
> > I haven't got this. On Intel SoCs GPIO is a part of pin control
> > registers. You can't touch GPIO without touching pin control.
>
> i meant pin control, if it ever did probe it would reserve the region
> and push our hack out, or the other way around ... no conflict!
> To get both we just need a simple patch and switch to pinctrl, just
> notify me once your stuff is ready and i will write that patch.

While thinking more on it, the quickest solution here is to do a P2SB
game based on DMI strings in the board code for the platform
(somewhere under PDx86).

> > > I see no issue here, waited for a long time and now expect to be
> > > allowed to get merged first.
> >
> > Okay, I have these questions / asks so far:
> > 1) Can firmware be fixed in order to provide an ACPI table for the pin
> > control devices?
>
> No. The firmware will only receive security but no feature updates ...
>
> > 2) Can you share firmware (BIOS ROM file I suppose) that I may flash
> > on an Apollo Lake machine and see if I can reproduce the issue?
>
> I do not have access. But all you need is a firware with no ACPI entry
> and P2SB hidden. Or simply patch out the two probe paths ;).

Yes, probably that will work.

> > 3) As may be a last resort, can you share (remotely) or even send to
> > us the device in question to try?
>
> We are talking about multiple devices. Not just that one apollo lake on
> which your patches kind of worked.
>
> But showed some weirdness which could really become a problem if
> someone decided to add an ACPI entry ..

Then it should have different DMI strings or so, it won't be the
_same_ platform anymore.

> It pin 42 name could be
> GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 42
> or
> GPIO_LOOKUP_IDX("INT3452:01", 42

> I guess that conflict will have to be dealt with before your can switch
> to probing pinctrl drivers based on cpu model and not only ACPI/P2SB any
> longer.

Not gonna happen.

-- 
With Best Regards,
Andy Shevchenko

^ permalink raw reply	[flat|nested] 40+ messages in thread

* Re: [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-11-26 14:59           ` Andy Shevchenko
@ 2021-11-26 19:54             ` Henning Schild
  0 siblings, 0 replies; 40+ messages in thread
From: Henning Schild @ 2021-11-26 19:54 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Hans de Goede,
	Pavel Machek, Enrico Weigelt

Am Fri, 26 Nov 2021 16:59:54 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Fri, Nov 26, 2021 at 4:44 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Fri, 26 Nov 2021 16:02:48 +0200
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > On Fri, Nov 26, 2021 at 3:28 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > <henning.schild@siemens.com> wrote:  
> 
> ...
> 
> > > > > > +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"},
> > > > > > +       { }
> > > > > > +};  
> > > > >
> > > > > It seems to me like poking GPIO controller registers directly.
> > > > > This is not good. The question still remains: Can we simply
> > > > > register a GPIO (pin control) driver and use an LED GPIO
> > > > > driver with an additional board file that instantiates it?  
> > > >
> > > > The short answer for v4 will be "No we can not!". The pinctrl
> > > > drivers do not currently probe on any of the devices and
> > > > attempts to fix that have failed or gut stuck. I tried to help
> > > > out where i could and waited for a long time.  
> > >
> > > I see, unfortunately I have stuck with some other (more important
> > > tasks) and can't fulfil this, but I still consider it's no go for
> > > driver poking pin control registers directly. Lemme see if I can
> > > prioritize this for next week.  
> >
> > I just sent v4. And am sick of waiting on you. Sorry to be that
> > clear here. I want that order changed! If you still end up being
> > fast, perfect. But i want to be faster!  
> 
> It's good that you are honest, honesty is what we missed a lot!

I was always honest, hope that was not missed from my side ... let
alone a lot.

> > > > Now my take is to turn the order around. We go in like that and
> > > > will happily switch to pinctrl if that ever comes up on the
> > > > machines. Meaning P2SB series on top of this, no more delays
> > > > please.  
> > >
> > > I don't want to slip bad code into the kernel where we can avoid
> > > that.  
> >
> > It is not bad code! That is unfair to say. It can be improved on and
> > that is what we have a FIXME line for. The worst code is code that
> > is not there ... devices without drivers!  
> 
> Okay, that's how you interpret the term "bad". Probably I had to use
> something else to explain that it's racy with the very same case if
> one adds an ACPI support to it.

It is only racy when the firmware would change (which i am
unfortunately pretty sure it will not), or if pinctrl would probe
without P2SB or ACPI. (where you say "Not gonna happen.")

Or i could say "fortunately pretty sure" because that means pinctrl
will never probe, hence no race!

> > That is bad, not i minor poke of parts of a resource no other driver
> > claimed!
> >  
> > > > We do use request_region so have a mutex in place. Meaning we
> > > > really only touch GPIO while pinctrl does not!  
> > >
> > > I haven't got this. On Intel SoCs GPIO is a part of pin control
> > > registers. You can't touch GPIO without touching pin control.  
> >
> > i meant pin control, if it ever did probe it would reserve the
> > region and push our hack out, or the other way around ... no
> > conflict! To get both we just need a simple patch and switch to
> > pinctrl, just notify me once your stuff is ready and i will write
> > that patch.  
> 
> While thinking more on it, the quickest solution here is to do a P2SB
> game based on DMI strings in the board code for the platform
> (somewhere under PDx86).

Not sure what you suggest here. p1 does pretty fancy DMI to be really
sure to only match specific devices, and only then we do our own P2SB
base address discover and region reservation.

> > > > I see no issue here, waited for a long time and now expect to be
> > > > allowed to get merged first.  
> > >
> > > Okay, I have these questions / asks so far:
> > > 1) Can firmware be fixed in order to provide an ACPI table for
> > > the pin control devices?  
> >
> > No. The firmware will only receive security but no feature updates
> > ... 
> > > 2) Can you share firmware (BIOS ROM file I suppose) that I may
> > > flash on an Apollo Lake machine and see if I can reproduce the
> > > issue?  
> >
> > I do not have access. But all you need is a firware with no ACPI
> > entry and P2SB hidden. Or simply patch out the two probe paths ;).  
> 
> Yes, probably that will work.

I wonder how you would probe without the two with your "Not gonna
happen.". But maybe your patches will open my eyes and i have been
blind all the time.

> > > 3) As may be a last resort, can you share (remotely) or even send
> > > to us the device in question to try?  
> >
> > We are talking about multiple devices. Not just that one apollo
> > lake on which your patches kind of worked.
> >
> > But showed some weirdness which could really become a problem if
> > someone decided to add an ACPI entry ..  
> 
> Then it should have different DMI strings or so, it won't be the
> _same_ platform anymore.

There is different DMI in place. p1 introduces
"enum simatic_ipc_station_ids" with currently 7 different devices
matched with not a string but a "binary" behind
SIMATIC_IPC_DMI_ENTRY_OEM. The struct can be found in
simatic_ipc_get_station_id

Our strings could be custom, but that binary allows for real DMI
identifaction of those currently proposed 7 "platforms".

See p4 where i revert string-based DMI matching with
SIMATIC_IPC_DMI_ENTRY_OEM-based. Make sure to look at my answer to a
question in v4 p4 on that DMI topic.

regards,
Henning

> > It pin 42 name could be
> > GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 42
> > or
> > GPIO_LOOKUP_IDX("INT3452:01", 42  
> 
> > I guess that conflict will have to be dealt with before your can
> > switch to probing pinctrl drivers based on cpu model and not only
> > ACPI/P2SB any longer.  
> 
> Not gonna happen.
> 


^ permalink raw reply	[flat|nested] 40+ messages in thread

end of thread, other threads:[~2021-11-26 19:57 UTC | newest]

Thread overview: 40+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-29 17:49 [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
2021-03-29 17:49 ` [PATCH v3 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
2021-11-26 12:39   ` Henning Schild
2021-03-29 17:49 ` [PATCH v3 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
2021-03-30 11:04   ` Andy Shevchenko
2021-03-30 11:58     ` Henning Schild
2021-03-30 12:15       ` Andy Shevchenko
2021-03-30 12:30         ` Henning Schild
2021-03-30 12:41           ` Andy Shevchenko
2021-03-30 15:23             ` Henning Schild
2021-03-31 15:40               ` Andy Shevchenko
2021-04-01 10:44                 ` Henning Schild
2021-04-01 11:04                   ` Andy Shevchenko
2021-04-12 11:56                     ` Henning Schild
2021-05-05 14:58                       ` Enrico Weigelt, metux IT consult
2021-04-12 15:15                     ` Henning Schild
2021-11-26 13:28     ` Henning Schild
2021-11-26 14:02       ` Andy Shevchenko
2021-11-26 14:44         ` Henning Schild
2021-11-26 14:59           ` Andy Shevchenko
2021-11-26 19:54             ` Henning Schild
2021-11-25 17:11   ` Henning Schild
2021-03-29 17:49 ` [PATCH v3 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
2021-04-01 16:15   ` Enrico Weigelt, metux IT consult
2021-04-06 14:52     ` Henning Schild
2021-04-07  8:53       ` Andy Shevchenko
2021-04-07 12:17         ` Guenter Roeck
2021-11-26 13:18           ` Henning Schild
2021-04-12 15:35     ` Henning Schild
2021-04-12 16:06       ` Guenter Roeck
2021-04-12 16:17         ` Henning Schild
2021-11-25 17:08   ` Henning Schild
2021-11-25 17:10   ` Henning Schild
2021-03-29 17:49 ` [PATCH v3 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
2021-03-29 18:00 ` [PATCH v3 0/4] add device drivers for Siemens Industrial PCs Henning Schild
2021-04-07 11:36 ` Hans de Goede
2021-04-12 11:27   ` Henning Schild
2021-07-12 11:35   ` Henning Schild
2021-07-12 12:09     ` Andy Shevchenko
2021-07-12 16:11       ` Henning Schild

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).