Linux-Watchdog Archive on lore.kernel.org
 help / color / 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; 26+ 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] 26+ 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-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, 0 replies; 26+ 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	[flat|nested] 26+ 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-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, 1 reply; 26+ 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	[flat|nested] 26+ 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
  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, 1 reply; 26+ 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	[flat|nested] 26+ 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; 26+ 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	[flat|nested] 26+ 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; 26+ 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] 26+ 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
  0 siblings, 1 reply; 26+ 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] 26+ 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
  0 siblings, 1 reply; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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
  0 siblings, 2 replies; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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
  5 siblings, 1 reply; 26+ 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] 26+ 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
  0 siblings, 0 replies; 26+ 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] 26+ 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
  0 siblings, 0 replies; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ 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; 26+ 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] 26+ messages in thread

end of thread, back to index

Thread overview: 26+ 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-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-04-12 15:15                     ` 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-04-12 15:35     ` Henning Schild
2021-04-12 16:06       ` Guenter Roeck
2021-04-12 16:17         ` 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

Linux-Watchdog Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-watchdog/0 linux-watchdog/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-watchdog linux-watchdog/ https://lore.kernel.org/linux-watchdog \
		linux-watchdog@vger.kernel.org
	public-inbox-index linux-watchdog

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-watchdog


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git