linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/4] add device drivers for Siemens Industrial PCs
@ 2021-03-15  9:57 Henning Schild
  2021-03-15  9:57 ` [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
                   ` (4 more replies)
  0 siblings, 5 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-15  9:57 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

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        | 210 +++++++++++++++++
 drivers/platform/x86/Kconfig                  |  12 +
 drivers/platform/x86/Makefile                 |   3 +
 drivers/platform/x86/pmc_atom.c               |  47 ++--
 drivers/platform/x86/simatic-ipc.c            | 168 ++++++++++++++
 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 |  66 ++++++
 14 files changed, 761 insertions(+), 20 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.2


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

* [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-15  9:57 [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Henning Schild
@ 2021-03-15  9:57 ` Henning Schild
  2021-03-15 10:31   ` Andy Shevchenko
  2021-03-15  9:57 ` [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-15  9:57 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

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            | 168 ++++++++++++++++++
 .../platform_data/x86/simatic-ipc-base.h      |  29 +++
 include/linux/platform_data/x86/simatic-ipc.h |  66 +++++++
 5 files changed, 278 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 ad4e630e73e2..44f8e82e1fd9 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1284,6 +1284,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..7c32c12ad32d
--- /dev/null
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -0,0 +1,168 @@
+// 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;
+}
+
+/*
+ * 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 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", 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..3af84a84f103
--- /dev/null
+++ b/include/linux/platform_data/x86/simatic-ipc.h
@@ -0,0 +1,66 @@
+/* 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 DMI_ENTRY_OEM	129
+
+enum 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)
+{
+	u32 station_id = SIMATIC_IPC_INVALID_STATION_ID;
+	int i;
+	struct {
+		u8	type;		/* type (0xff = binary) */
+		u8	len;		/* len of data entry */
+		u8	reserved[3];
+		u32	station_id;	/* station id (LE) */
+	} __packed
+	*data_entry = (void *)data + sizeof(struct dmi_header);
+
+	/* find 4th entry in OEM data */
+	for (i = 0; i < 3; i++)
+		data_entry = (void *)((u8 *)(data_entry) + data_entry->len);
+
+	/* decode station id */
+	if (data_entry && (u8 *)data_entry < data + max_len &&
+	    data_entry->type == 0xff && data_entry->len == 9)
+		station_id = le32_to_cpu(data_entry->station_id);
+
+	return station_id;
+}
+
+static inline void
+simatic_ipc_find_dmi_entry_helper(const struct dmi_header *dh, void *_data)
+{
+	u32 *id = _data;
+
+	if (dh->type != DMI_ENTRY_OEM)
+		return;
+
+	*id = simatic_ipc_get_station_id((u8 *)dh, dh->length);
+}
+
+#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */
-- 
2.26.2


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

* [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15  9:57 [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Henning Schild
  2021-03-15  9:57 ` [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
@ 2021-03-15  9:57 ` Henning Schild
  2021-03-15 10:48   ` Andy Shevchenko
  2021-03-18 10:25   ` Enrico Weigelt, metux IT consult
  2021-03-15  9:57 ` [PATCH v2 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
                   ` (2 subsequent siblings)
  4 siblings, 2 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-15  9:57 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

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 | 210 +++++++++++++++++++++++++
 5 files changed, 229 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..0f7e6320e10d
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -0,0 +1,210 @@
+// 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[32];
+	struct led_classdev cdev;
+};
+
+static struct simatic_ipc_led simatic_ipc_leds_io[] = {
+	{1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" },
+	{1 << 7,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" },
+	{1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
+	{1 << 6,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" },
+	{1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
+	{1 << 5,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" },
+	{0, ""},
+};
+
+/* 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, "simatic-ipc:red:" LED_FUNCTION_STATUS "-1"},
+	{0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1"},
+	{0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2"},
+	{0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS "-2"},
+	{0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3"},
+	{0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS "-3"},
+	{0, ""},
+};
+
+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 void simatic_ipc_led_set_io(struct led_classdev *led_cd,
+				   enum led_brightness brightness)
+{
+	struct simatic_ipc_led *led =
+		container_of(led_cd, struct simatic_ipc_led, cdev);
+	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 =
+		container_of(led_cd, struct simatic_ipc_led, cdev);
+
+	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 =
+		container_of(led_cd, struct simatic_ipc_led, cdev);
+
+	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 =
+		container_of(led_cd, struct simatic_ipc_led, cdev);
+
+	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)
+{
+	struct simatic_ipc_platform *plat;
+	struct device *dev = &pdev->dev;
+	struct simatic_ipc_led *ipcled;
+	struct led_classdev *cdev;
+	struct resource *res;
+	int err, type;
+	u32 *p;
+
+	plat = pdev->dev.platform_data;
+
+	switch (plat->devmode) {
+	case SIMATIC_IPC_DEVICE_227D:
+	case SIMATIC_IPC_DEVICE_427E:
+		res = &simatic_ipc_led_io_res;
+		ipcled = simatic_ipc_leds_io;
+		/* the 227D is high on while 427E is low on, invert the struct
+		 * we have
+		 */
+		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;
+		cdev->brightness_set = simatic_ipc_led_set_io;
+		cdev->brightness_get = simatic_ipc_led_get_io;
+		if (type == IORESOURCE_MEM) {
+			cdev->brightness_set = simatic_ipc_led_set_mem;
+			cdev->brightness_get = simatic_ipc_led_get_mem;
+		}
+		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 led_driver = {
+	.probe = simatic_ipc_leds_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+module_platform_driver(led_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
-- 
2.26.2


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

* [PATCH v2 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-15  9:57 [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Henning Schild
  2021-03-15  9:57 ` [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
  2021-03-15  9:57 ` [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
@ 2021-03-15  9:57 ` Henning Schild
  2021-03-15 15:10   ` Guenter Roeck
  2021-03-15  9:57 ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
  2021-03-15 10:55 ` [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
  4 siblings, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-15  9:57 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

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/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..f0f948968db3
--- /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 wdt_driver = {
+	.probe = simatic_ipc_wdt_probe,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+module_platform_driver(wdt_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
-- 
2.26.2


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

* [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs
  2021-03-15  9:57 [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Henning Schild
                   ` (2 preceding siblings ...)
  2021-03-15  9:57 ` [PATCH v2 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
@ 2021-03-15  9:57 ` Henning Schild
  2021-03-15 10:14   ` Henning Schild
  2021-03-15 13:25   ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs kernel test robot
  2021-03-15 10:55 ` [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
  4 siblings, 2 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-15  9:57 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, 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 | 47 +++++++++++++++++++--------------
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index ca684ed760d1..38542d547f29 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,23 @@ static void pmc_dbgfs_register(struct pmc_dev *pmc)
 }
 #endif /* CONFIG_DEBUG_FS */
 
+static bool pmc_clk_is_critical = true;
+
+static int siemens_clk_is_critical(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 1;
+
+out:
+	pmc_clk_is_critical = false;
+	return 1;
+}
+
 /*
  * Some systems need one or more of their pmc_plt_clks to be
  * marked as critical.
@@ -424,24 +442,10 @@ static const struct dmi_system_id critclk_systems[] = {
 		},
 	},
 	{
-		.ident = "SIMATIC IPC227E",
-		.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",
+		.callback = siemens_clk_is_critical,
+		.ident = "SIEMENS AG",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "A5E45074588"),
 		},
 	},
 
@@ -453,7 +457,7 @@ 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);
+	const struct dmi_system_id *d;
 
 	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
 	if (!clk_data)
@@ -461,9 +465,12 @@ 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;
+		if (clk_data->critical) {
+			d = dmi_first_match(critclk_systems);
+			pr_info("%s critclks quirk enabled\n", d->ident);
+		}
 	}
 
 	clkdev = platform_device_register_data(&pdev->dev, "clk-pmc-atom",
-- 
2.26.2


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

* Re: [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs
  2021-03-15  9:57 ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
@ 2021-03-15 10:14   ` Henning Schild
  2021-03-15 10:19     ` Hans de Goede
  2021-03-15 13:25   ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs kernel test robot
  1 sibling, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-15 10:14 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, Michael Haener

Am Mon, 15 Mar 2021 10:57:10 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> 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 | 47
> +++++++++++++++++++-------------- 1 file changed, 27 insertions(+),
> 20 deletions(-)
> 
> diff --git a/drivers/platform/x86/pmc_atom.c
> b/drivers/platform/x86/pmc_atom.c index ca684ed760d1..38542d547f29
> 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,23 @@ static void pmc_dbgfs_register(struct pmc_dev
> *pmc) }
>  #endif /* CONFIG_DEBUG_FS */
>  
> +static bool pmc_clk_is_critical = true;
> +
> +static int siemens_clk_is_critical(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 1;
> +
> +out:
> +	pmc_clk_is_critical = false;
> +	return 1;
> +}
> +
>  /*
>   * Some systems need one or more of their pmc_plt_clks to be
>   * marked as critical.
> @@ -424,24 +442,10 @@ static const struct dmi_system_id
> critclk_systems[] = { },
>  	},
>  	{
> -		.ident = "SIMATIC IPC227E",
> -		.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",
> +		.callback = siemens_clk_is_critical,
> +		.ident = "SIEMENS AG",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
> -			DMI_MATCH(DMI_PRODUCT_VERSION,
> "A5E45074588"), },
>  	},
>  
> @@ -453,7 +457,7 @@ 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);
> +	const struct dmi_system_id *d;
>  
>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>  	if (!clk_data)
> @@ -461,9 +465,12 @@ 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)) {

Had to switch to check_system to get the callback to work.

> +		clk_data->critical = pmc_clk_is_critical;
> +		if (clk_data->critical) {
> +			d = dmi_first_match(critclk_systems);
> +			pr_info("%s critclks quirk enabled\n",
> d->ident);

Now need a double match here just to print the ident. Not too happy
with that but proposing it like this to keep the ident printing.

I guess it could be improved by not printing the ident or having a
global variable and global callback to remember the ident to print
later. I would propose to not print the ident if the double-match does
not find traction.

Henning

> +		}
>  	}
>  
>  	clkdev = platform_device_register_data(&pdev->dev,
> "clk-pmc-atom",


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

* Re: [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs
  2021-03-15 10:14   ` Henning Schild
@ 2021-03-15 10:19     ` Hans de Goede
  2021-03-15 12:09       ` Henning Schild
  2021-03-15 14:58       ` [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries Henning Schild
  0 siblings, 2 replies; 43+ messages in thread
From: Hans de Goede @ 2021-03-15 10:19 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,
	Michael Haener

Hi,

On 3/15/21 11:14 AM, Henning Schild wrote:
> Am Mon, 15 Mar 2021 10:57:10 +0100
> schrieb Henning Schild <henning.schild@siemens.com>:
> 
>> 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 | 47
>> +++++++++++++++++++-------------- 1 file changed, 27 insertions(+),
>> 20 deletions(-)
>>
>> diff --git a/drivers/platform/x86/pmc_atom.c
>> b/drivers/platform/x86/pmc_atom.c index ca684ed760d1..38542d547f29
>> 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,23 @@ static void pmc_dbgfs_register(struct pmc_dev
>> *pmc) }
>>  #endif /* CONFIG_DEBUG_FS */
>>  
>> +static bool pmc_clk_is_critical = true;
>> +
>> +static int siemens_clk_is_critical(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 1;
>> +
>> +out:
>> +	pmc_clk_is_critical = false;
>> +	return 1;
>> +}
>> +
>>  /*
>>   * Some systems need one or more of their pmc_plt_clks to be
>>   * marked as critical.
>> @@ -424,24 +442,10 @@ static const struct dmi_system_id
>> critclk_systems[] = { },
>>  	},
>>  	{
>> -		.ident = "SIMATIC IPC227E",
>> -		.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",
>> +		.callback = siemens_clk_is_critical,
>> +		.ident = "SIEMENS AG",
>>  		.matches = {
>>  			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
>> -			DMI_MATCH(DMI_PRODUCT_VERSION,
>> "A5E45074588"), },
>>  	},
>>  
>> @@ -453,7 +457,7 @@ 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);
>> +	const struct dmi_system_id *d;
>>  
>>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>>  	if (!clk_data)
>> @@ -461,9 +465,12 @@ 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)) {
> 
> Had to switch to check_system to get the callback to work.
> 
>> +		clk_data->critical = pmc_clk_is_critical;
>> +		if (clk_data->critical) {
>> +			d = dmi_first_match(critclk_systems);
>> +			pr_info("%s critclks quirk enabled\n",
>> d->ident);
> 
> Now need a double match here just to print the ident. Not too happy
> with that but proposing it like this to keep the ident printing.
> 
> I guess it could be improved by not printing the ident or having a
> global variable and global callback to remember the ident to print
> later. I would propose to not print the ident if the double-match does
> not find traction.

IMHO it would be best to add another callback for the non Siemens
entries which just prints the ideent and returns 1 to avoid needsly
looping over the rest of the array.

And then just set the callback member of all the non Siemens entries
to this new callback. The space for the callback pointer is already
reserved in the struct anyways, so actually setting it does not take
up any space.

Regards,

Hans


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

* Re: [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-15  9:57 ` [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
@ 2021-03-15 10:31   ` Andy Shevchenko
  2021-03-15 16:30     ` Henning Schild
  2021-03-17 19:13     ` Henning Schild
  0 siblings, 2 replies; 43+ messages in thread
From: Andy Shevchenko @ 2021-03-15 10:31 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

On Mon, Mar 15, 2021 at 12:02 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> 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.

...

> +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;
> +}

Why not use MFD here?

...

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

No blank line here.

I would add FIXME or REVISIT here to point out that this should be
deduplicated in the future.

> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> +{
> +       struct pci_bus *bus;
> +       u32 bar0 = 0;
> +
> +       /*
> +        * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device

No, it's not a GPIO's bar. It's P2SB's one. GPIO resides in that bar somewhere.

> +        * 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 inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
> +{
> +       u32 station_id = SIMATIC_IPC_INVALID_STATION_ID;
> +       int i;

Reversed xmas tree order, please.

> +       struct {
> +               u8      type;           /* type (0xff = binary) */
> +               u8      len;            /* len of data entry */
> +               u8      reserved[3];
> +               u32     station_id;     /* station id (LE) */

> +       } __packed
> +       *data_entry = (void *)data + sizeof(struct dmi_header);

Can be one line.

> +       /* find 4th entry in OEM data */
> +       for (i = 0; i < 3; i++)

3 is magic!

> +               data_entry = (void *)((u8 *)(data_entry) + data_entry->len);
> +
> +       /* decode station id */
> +       if (data_entry && (u8 *)data_entry < data + max_len &&
> +           data_entry->type == 0xff && data_entry->len == 9)
> +               station_id = le32_to_cpu(data_entry->station_id);
> +
> +       return station_id;
> +}

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15  9:57 ` [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
@ 2021-03-15 10:48   ` Andy Shevchenko
  2021-03-15 11:19     ` Pavel Machek
                       ` (2 more replies)
  2021-03-18 10:25   ` Enrico Weigelt, metux IT consult
  1 sibling, 3 replies; 43+ messages in thread
From: Andy Shevchenko @ 2021-03-15 10:48 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

On Mon, Mar 15, 2021 at 11:57 AM 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.

...

> +struct simatic_ipc_led {
> +       unsigned int value; /* mask for io and offset for mem */

> +       char name[32];

Hmm... Dunno if LED framework defines its own constraints for the
length of the name.

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

Can you use BIT() macro here? And can it be sorted by the bit order?

> +       {0, ""},

{ } is enough (no comma for terminator lines in general, and no need
to show structure member assignments separately in particular).

> +};
> +
> +/* the actual start will be discovered with pci, 0 is a placeholder */

PCI

> +struct resource simatic_ipc_led_mem_res =
> +       DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);

One line?

...

> +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> +       {0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-1"},
> +       {0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1"},
> +       {0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2"},
> +       {0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS "-2"},
> +       {0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3"},
> +       {0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS "-3"},
> +       {0, ""},

As per above.

> +};

...

> +       struct simatic_ipc_led *led =
> +               container_of(led_cd, struct simatic_ipc_led, cdev);

One line?

...

> +       struct simatic_ipc_led *led =
> +               container_of(led_cd, struct simatic_ipc_led, cdev);

One line?

...

> +       struct simatic_ipc_led *led =
> +               container_of(led_cd, struct simatic_ipc_led, cdev);

Ditto.


Btw, usually for such cases we create an inline helper
... to_simatic_ipc_led(...)
{
  return container_of(...);
}

...

> +static int simatic_ipc_leds_probe(struct platform_device *pdev)
> +{
> +       struct simatic_ipc_platform *plat;

const?

> +       struct device *dev = &pdev->dev;
> +       struct simatic_ipc_led *ipcled;
> +       struct led_classdev *cdev;
> +       struct resource *res;
> +       int err, type;
> +       u32 *p;

> +       plat = pdev->dev.platform_data;

Can be done directly in the definition block.

> +       switch (plat->devmode) {
> +       case SIMATIC_IPC_DEVICE_227D:
> +       case SIMATIC_IPC_DEVICE_427E:
> +               res = &simatic_ipc_led_io_res;
> +               ipcled = simatic_ipc_leds_io;
> +               /* the 227D is high on while 427E is low on, invert the struct
> +                * we have
> +                */
> +               if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {

> +                       while (ipcled->value) {
> +                               ipcled->value = swab16(ipcled->value);
> +                               ipcled++;
> +                       }

This seems fishy. If you have a BE CPU it won't work the same way.
Better:
 a) to use cpu_to_le16 / be16
 b) create this as a helper that we may move to the generic header of byteorder.

But looking at the use of it, perhaps you rather need to redefine IO
accessors, i.e. ioread16()/iowrite16() vs. ioread16be()/iowrite16be().

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

Magic. As I told you this is an actual offseet in the P2SB's bar for
GPIO registers.
I have a question, why we can't provide a GPIO driver which is already
in the kernel and, with use of the patch series I sent, to convert
this all magic to GPIO LEDs as it's done for all normal cases?

> +               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;
> +       }

> +}

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 0/4] add device drivers for Siemens Industrial PCs
  2021-03-15  9:57 [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Henning Schild
                   ` (3 preceding siblings ...)
  2021-03-15  9:57 ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
@ 2021-03-15 10:55 ` Andy Shevchenko
  2021-03-15 16:08   ` Henning Schild
  2021-08-02  9:21   ` Henning Schild
  4 siblings, 2 replies; 43+ messages in thread
From: Andy Shevchenko @ 2021-03-15 10:55 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

On Mon, Mar 15, 2021 at 12:12 PM Henning Schild
<henning.schild@siemens.com> wrote:
>
> 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.

Thanks for an update!

I did review more thoughtfully the series and realized that you can
avoid that P2SB thingy and may the approach be much cleaner if you
register the real GPIO driver and convert your LEDs to be a GPIO LEDs.
Then you won't need any custom code for it (except some board file, or
what would be better to file _DSD in your ACPI tables.

--
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15 10:48   ` Andy Shevchenko
@ 2021-03-15 11:19     ` Pavel Machek
  2021-03-15 11:26       ` Andy Shevchenko
                         ` (3 more replies)
  2021-03-18 10:27     ` Enrico Weigelt, metux IT consult
  2021-03-26 16:33     ` Henning Schild
  2 siblings, 4 replies; 43+ messages in thread
From: Pavel Machek @ 2021-03-15 11:19 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Henning Schild, 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

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


> > +       struct led_classdev cdev;
> > +};
> > +
> > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > +       {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" },
> > +       {1 << 7,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" },
> > +       {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
> > +       {1 << 6,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" },
> > +       {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
> > +       {1 << 5,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" },
> 
> Can you use BIT() macro here? And can it be sorted by the bit order?

There's nothing wrong with << and this order is fine.

But I still don't like the naming. simantic-ipc: prefix is
useless. Having 6 status leds is not good, either.

> > +       struct simatic_ipc_led *led =
> > +               container_of(led_cd, struct simatic_ipc_led, cdev);
> 
> One line?

80 columns. It is fine as it is.

Best regards,

								Pavel
-- 
http://www.livejournal.com/~pavelmachek

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

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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15 11:19     ` Pavel Machek
@ 2021-03-15 11:26       ` Andy Shevchenko
  2021-03-15 12:40       ` Henning Schild
                         ` (2 subsequent siblings)
  3 siblings, 0 replies; 43+ messages in thread
From: Andy Shevchenko @ 2021-03-15 11:26 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Henning Schild, 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

On Mon, Mar 15, 2021 at 1:19 PM Pavel Machek <pavel@ucw.cz> wrote:

> > > +       struct simatic_ipc_led *led =
> > > +               container_of(led_cd, struct simatic_ipc_led, cdev);
> >
> > One line?
>
> 80 columns. It is fine as it is.

With the inline helper it will be possible to have this neat and short.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs
  2021-03-15 10:19     ` Hans de Goede
@ 2021-03-15 12:09       ` Henning Schild
  2021-03-15 14:58       ` [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries Henning Schild
  1 sibling, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-15 12:09 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,
	Michael Haener

Am Mon, 15 Mar 2021 11:19:24 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/15/21 11:14 AM, Henning Schild wrote:
> > Am Mon, 15 Mar 2021 10:57:10 +0100
> > schrieb Henning Schild <henning.schild@siemens.com>:
> >   
> >> 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 | 47
> >> +++++++++++++++++++-------------- 1 file changed, 27 insertions(+),
> >> 20 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/pmc_atom.c
> >> b/drivers/platform/x86/pmc_atom.c index ca684ed760d1..38542d547f29
> >> 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,23 @@ static void pmc_dbgfs_register(struct pmc_dev
> >> *pmc) }
> >>  #endif /* CONFIG_DEBUG_FS */
> >>  
> >> +static bool pmc_clk_is_critical = true;
> >> +
> >> +static int siemens_clk_is_critical(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 1;
> >> +
> >> +out:
> >> +	pmc_clk_is_critical = false;
> >> +	return 1;
> >> +}
> >> +
> >>  /*
> >>   * Some systems need one or more of their pmc_plt_clks to be
> >>   * marked as critical.
> >> @@ -424,24 +442,10 @@ static const struct dmi_system_id
> >> critclk_systems[] = { },
> >>  	},
> >>  	{
> >> -		.ident = "SIMATIC IPC227E",
> >> -		.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",
> >> +		.callback = siemens_clk_is_critical,
> >> +		.ident = "SIEMENS AG",
> >>  		.matches = {
> >>  			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
> >> -			DMI_MATCH(DMI_PRODUCT_VERSION,
> >> "A5E45074588"), },
> >>  	},
> >>  
> >> @@ -453,7 +457,7 @@ 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);
> >> +	const struct dmi_system_id *d;
> >>  
> >>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> >>  	if (!clk_data)
> >> @@ -461,9 +465,12 @@ 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)) {  
> > 
> > Had to switch to check_system to get the callback to work.
> >   
> >> +		clk_data->critical = pmc_clk_is_critical;
> >> +		if (clk_data->critical) {
> >> +			d = dmi_first_match(critclk_systems);
> >> +			pr_info("%s critclks quirk enabled\n",
> >> d->ident);  
> > 
> > Now need a double match here just to print the ident. Not too happy
> > with that but proposing it like this to keep the ident printing.
> > 
> > I guess it could be improved by not printing the ident or having a
> > global variable and global callback to remember the ident to print
> > later. I would propose to not print the ident if the double-match
> > does not find traction.  
> 
> IMHO it would be best to add another callback for the non Siemens
> entries which just prints the ideent and returns 1 to avoid needsly
> looping over the rest of the array.
> 
> And then just set the callback member of all the non Siemens entries
> to this new callback. The space for the callback pointer is already
> reserved in the struct anyways, so actually setting it does not take
> up any space.

Sounds good. I think i will make that another patch on top of the
series and send it in reply to "v2 4/4". Maybe squash it in a v3.

regards,
Henning

> Regards,
> 
> Hans
> 


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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15 11:19     ` Pavel Machek
  2021-03-15 11:26       ` Andy Shevchenko
@ 2021-03-15 12:40       ` Henning Schild
  2021-03-18 11:38       ` Enrico Weigelt, metux IT consult
  2021-03-27  9:56       ` Henning Schild
  3 siblings, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-15 12:40 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Andy Shevchenko, 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

Am Mon, 15 Mar 2021 12:19:15 +0100
schrieb Pavel Machek <pavel@ucw.cz>:

> > > +       struct led_classdev cdev;
> > > +};
> > > +
> > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > +       {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" },
> > > +       {1 << 7,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1"
> > > },
> > > +       {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
> > > +       {1 << 6,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2"
> > > },
> > > +       {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
> > > +       {1 << 5,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3"
> > > },  
> > 
> > Can you use BIT() macro here? And can it be sorted by the bit
> > order?  
> 
> There's nothing wrong with << and this order is fine.
> 
> But I still don't like the naming. simantic-ipc: prefix is
> useless. Having 6 status leds is not good, either.

You asked about a picture before, so here is one example

https://support.industry.siemens.com/cs/document/67235073/simatic-ipc427d?dti=0&pnid=16756&lc=en-WW

page 19 shows how the box looks like
page 135 it what the implementation is based on

Externally human visible are 3 "lights", which can be off, red, green,
yellow. Internally every single "light" has two leds and yellow is a
mix when red and green are on.
Unfortunately hw does not allow all 4 states for all 3 lights. Some
boxes implement "yellow" mixing in hw.

That is why the same name is used with two colors.

maybe those LEDs qualify for multi-color? 

"status" was the best name i found in the dt-bindings header

i guess i can be creative with the names and take ie "status", "fault",
and "indicator"

i will also drop that prefix "simatic-ipc" that was inspired by
"tpacpi", or should it be "platform:<color>:<name>"?

regards,
Henning

> > > +       struct simatic_ipc_led *led =
> > > +               container_of(led_cd, struct simatic_ipc_led,
> > > cdev);  
> > 
> > One line?  
> 
> 80 columns. It is fine as it is.
> 
> Best regards,
> 
> 								Pavel


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

* Re: [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs
  2021-03-15  9:57 ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
  2021-03-15 10:14   ` Henning Schild
@ 2021-03-15 13:25   ` kernel test robot
  1 sibling, 0 replies; 43+ messages in thread
From: kernel test robot @ 2021-03-15 13:25 UTC (permalink / raw)
  To: Henning Schild, linux-kernel, linux-leds, platform-driver-x86,
	linux-watchdog
  Cc: kbuild-all, Srikanth Krishnakar, Jan Kiszka, Henning Schild,
	Gerd Haeussler, Guenter Roeck, Wim Van Sebroeck

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

Hi Henning,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on pavel-linux-leds/for-next]
[also build test WARNING on tip/master linux/master linus/master v5.12-rc3 next-20210315]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Henning-Schild/add-device-drivers-for-Siemens-Industrial-PCs/20210315-181441
base:   git://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git for-next
config: x86_64-randconfig-s022-20210315 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.3-277-gc089cd2d-dirty
        # https://github.com/0day-ci/linux/commit/7b3ce3514b5f314b15ab6e2898104fa3e8e76d59
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Henning-Schild/add-device-drivers-for-Siemens-Industrial-PCs/20210315-181441
        git checkout 7b3ce3514b5f314b15ab6e2898104fa3e8e76d59
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


"sparse warnings: (new ones prefixed by >>)"
   drivers/platform/x86/pmc_atom.c: note: in included file:
>> include/linux/platform_data/x86/simatic-ipc.h:50:30: sparse: sparse: cast to restricted __le32

vim +50 include/linux/platform_data/x86/simatic-ipc.h

051f5729fd6fb4 Henning Schild 2021-03-15  30  
051f5729fd6fb4 Henning Schild 2021-03-15  31  static inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
051f5729fd6fb4 Henning Schild 2021-03-15  32  {
051f5729fd6fb4 Henning Schild 2021-03-15  33  	u32 station_id = SIMATIC_IPC_INVALID_STATION_ID;
051f5729fd6fb4 Henning Schild 2021-03-15  34  	int i;
051f5729fd6fb4 Henning Schild 2021-03-15  35  	struct {
051f5729fd6fb4 Henning Schild 2021-03-15  36  		u8	type;		/* type (0xff = binary) */
051f5729fd6fb4 Henning Schild 2021-03-15  37  		u8	len;		/* len of data entry */
051f5729fd6fb4 Henning Schild 2021-03-15  38  		u8	reserved[3];
051f5729fd6fb4 Henning Schild 2021-03-15  39  		u32	station_id;	/* station id (LE) */
051f5729fd6fb4 Henning Schild 2021-03-15  40  	} __packed
051f5729fd6fb4 Henning Schild 2021-03-15  41  	*data_entry = (void *)data + sizeof(struct dmi_header);
051f5729fd6fb4 Henning Schild 2021-03-15  42  
051f5729fd6fb4 Henning Schild 2021-03-15  43  	/* find 4th entry in OEM data */
051f5729fd6fb4 Henning Schild 2021-03-15  44  	for (i = 0; i < 3; i++)
051f5729fd6fb4 Henning Schild 2021-03-15  45  		data_entry = (void *)((u8 *)(data_entry) + data_entry->len);
051f5729fd6fb4 Henning Schild 2021-03-15  46  
051f5729fd6fb4 Henning Schild 2021-03-15  47  	/* decode station id */
051f5729fd6fb4 Henning Schild 2021-03-15  48  	if (data_entry && (u8 *)data_entry < data + max_len &&
051f5729fd6fb4 Henning Schild 2021-03-15  49  	    data_entry->type == 0xff && data_entry->len == 9)
051f5729fd6fb4 Henning Schild 2021-03-15 @50  		station_id = le32_to_cpu(data_entry->station_id);
051f5729fd6fb4 Henning Schild 2021-03-15  51  
051f5729fd6fb4 Henning Schild 2021-03-15  52  	return station_id;
051f5729fd6fb4 Henning Schild 2021-03-15  53  }
051f5729fd6fb4 Henning Schild 2021-03-15  54  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 33533 bytes --]

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

* [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries
  2021-03-15 10:19     ` Hans de Goede
  2021-03-15 12:09       ` Henning Schild
@ 2021-03-15 14:58       ` Henning Schild
  2021-03-15 16:31         ` Hans de Goede
  1 sibling, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-15 14:58 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

Introduce a global variable to remember the matching entry for later
printing. Also having a callback allows to stop matching after the first
hit.

Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/platform/x86/pmc_atom.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index 38542d547f29..d0f74856cd8b 100644
--- a/drivers/platform/x86/pmc_atom.c
+++ b/drivers/platform/x86/pmc_atom.c
@@ -364,8 +364,16 @@ static void pmc_dbgfs_register(struct pmc_dev *pmc)
 #endif /* CONFIG_DEBUG_FS */
 
 static bool pmc_clk_is_critical = true;
+static const struct dmi_system_id *dmi_critical;
 
-static int siemens_clk_is_critical(const struct dmi_system_id *d)
+static int dmi_callback(const struct dmi_system_id *d)
+{
+	dmi_critical = d;
+
+	return 1;
+}
+
+static int dmi_callback_siemens(const struct dmi_system_id *d)
 {
 	u32 st_id;
 
@@ -373,7 +381,7 @@ static int siemens_clk_is_critical(const struct dmi_system_id *d)
 		goto out;
 
 	if (st_id == SIMATIC_IPC_IPC227E || st_id == SIMATIC_IPC_IPC277E)
-		return 1;
+		return dmi_callback(d);
 
 out:
 	pmc_clk_is_critical = false;
@@ -388,6 +396,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"),
@@ -396,6 +405,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"),
@@ -404,6 +414,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"),
@@ -412,6 +423,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"),
@@ -420,6 +432,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"),
@@ -428,6 +441,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"),
@@ -436,13 +450,14 @@ 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"),
 		},
 	},
 	{
-		.callback = siemens_clk_is_critical,
+		.callback = dmi_callback_siemens,
 		.ident = "SIEMENS AG",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
@@ -457,7 +472,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;
 
 	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
 	if (!clk_data)
@@ -468,8 +482,8 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
 	if (dmi_check_system(critclk_systems)) {
 		clk_data->critical = pmc_clk_is_critical;
 		if (clk_data->critical) {
-			d = dmi_first_match(critclk_systems);
-			pr_info("%s critclks quirk enabled\n", d->ident);
+			pr_info("%s critclks quirk enabled\n",
+				dmi_critical->ident);
 		}
 	}
 
-- 
2.26.2


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

* Re: [PATCH v2 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-15  9:57 ` [PATCH v2 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
@ 2021-03-15 15:10   ` Guenter Roeck
  2021-03-29 16:28     ` Henning Schild
  0 siblings, 1 reply; 43+ messages in thread
From: Guenter Roeck @ 2021-03-15 15:10 UTC (permalink / raw)
  To: Henning Schild, linux-kernel, linux-leds, platform-driver-x86,
	linux-watchdog
  Cc: Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek,
	Andy Shevchenko

On 3/15/21 2:57 AM, Henning Schild wrote:
> This driver adds initial support for several devices from Siemens. It is
> based on a platform driver introduced in an earlier commit.
> 
> 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..f0f948968db3
> --- /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 wdt_driver = {
> +	.probe = simatic_ipc_wdt_probe,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +	},
> +};
> +
> +module_platform_driver(wdt_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
> 


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

* Re: [PATCH v2 0/4] add device drivers for Siemens Industrial PCs
  2021-03-15 10:55 ` [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
@ 2021-03-15 16:08   ` Henning Schild
  2021-08-02  9:21   ` Henning Schild
  1 sibling, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-15 16:08 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

Am Mon, 15 Mar 2021 12:55:13 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Mon, Mar 15, 2021 at 12:12 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > 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.  
> 
> Thanks for an update!
> 
> I did review more thoughtfully the series and realized that you can
> avoid that P2SB thingy and may the approach be much cleaner if you
> register the real GPIO driver and convert your LEDs to be a GPIO LEDs.
> Then you won't need any custom code for it (except some board file, or
> what would be better to file _DSD in your ACPI tables.

Thanks Andy. I will need to read through your comments and existing
code. Are you saying there already is a GPIO driver that i should
rather hook into ... given there is and will not be WDAT any time soon?
Can you please point it out to me, the driver and maybe an example.

If you are suggesting to write a generic GPIO driver, i would probably
say that this can hopefully wait until we have a second user and need
that level of abstraction.

regards,
Henning

> --
> With Best Regards,
> Andy Shevchenko


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

* Re: [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-15 10:31   ` Andy Shevchenko
@ 2021-03-15 16:30     ` Henning Schild
  2021-03-17 19:13     ` Henning Schild
  1 sibling, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-15 16: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

Am Mon, 15 Mar 2021 12:31:11 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Mon, Mar 15, 2021 at 12:02 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > 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.  
> 
> ...
> 
> > +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;
> > +}  
> 
> Why not use MFD here?
> 
> ...
> 
> > +/*
> > + * 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
> > + */  
> 
> No blank line here.
> 
> I would add FIXME or REVISIT here to point out that this should be
> deduplicated in the future.

Sure i forgot the mention that ordering problem of the two series here
again specifically. Was kind of assuming yours would maybe be first and
that code not being reviewed again ... 
The code is there to test and propose something "working" not something
i expect to be merged as is.

regards,
Henning

> > +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> > +{
> > +       struct pci_bus *bus;
> > +       u32 bar0 = 0;
> > +
> > +       /*
> > +        * The GPIO memory is bar0 of the hidden P2SB device.
> > Unhide the device  
> 
> No, it's not a GPIO's bar. It's P2SB's one. GPIO resides in that bar
> somewhere.
> 
> > +        * 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 inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
> > +{
> > +       u32 station_id = SIMATIC_IPC_INVALID_STATION_ID;
> > +       int i;  
> 
> Reversed xmas tree order, please.
> 
> > +       struct {
> > +               u8      type;           /* type (0xff = binary) */
> > +               u8      len;            /* len of data entry */
> > +               u8      reserved[3];
> > +               u32     station_id;     /* station id (LE) */  
> 
> > +       } __packed
> > +       *data_entry = (void *)data + sizeof(struct dmi_header);  
> 
> Can be one line.
> 
> > +       /* find 4th entry in OEM data */
> > +       for (i = 0; i < 3; i++)  
> 
> 3 is magic!
> 
> > +               data_entry = (void *)((u8 *)(data_entry) +
> > data_entry->len); +
> > +       /* decode station id */
> > +       if (data_entry && (u8 *)data_entry < data + max_len &&
> > +           data_entry->type == 0xff && data_entry->len == 9)
> > +               station_id = le32_to_cpu(data_entry->station_id);
> > +
> > +       return station_id;
> > +}  
> 


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

* Re: [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries
  2021-03-15 14:58       ` [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries Henning Schild
@ 2021-03-15 16:31         ` Hans de Goede
  2021-03-15 17:00           ` Henning Schild
  0 siblings, 1 reply; 43+ messages in thread
From: Hans de Goede @ 2021-03-15 16:31 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

Hi,

On 3/15/21 3:58 PM, Henning Schild wrote:
> Introduce a global variable to remember the matching entry for later
> printing. Also having a callback allows to stop matching after the first
> hit.
> 
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  drivers/platform/x86/pmc_atom.c | 26 ++++++++++++++++++++------
>  1 file changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
> index 38542d547f29..d0f74856cd8b 100644
> --- a/drivers/platform/x86/pmc_atom.c
> +++ b/drivers/platform/x86/pmc_atom.c
> @@ -364,8 +364,16 @@ static void pmc_dbgfs_register(struct pmc_dev *pmc)
>  #endif /* CONFIG_DEBUG_FS */
>  
>  static bool pmc_clk_is_critical = true;
> +static const struct dmi_system_id *dmi_critical;
>  
> -static int siemens_clk_is_critical(const struct dmi_system_id *d)
> +static int dmi_callback(const struct dmi_system_id *d)
> +{
> +	dmi_critical = d;

Don't introduce a global variable for this please. Instead just directly
print the ident of the matching dmi_system_id here.

Regards,

Hans


> +
> +	return 1;
> +}
> +
> +static int dmi_callback_siemens(const struct dmi_system_id *d)
>  {
>  	u32 st_id;
>  
> @@ -373,7 +381,7 @@ static int siemens_clk_is_critical(const struct dmi_system_id *d)
>  		goto out;
>  
>  	if (st_id == SIMATIC_IPC_IPC227E || st_id == SIMATIC_IPC_IPC277E)
> -		return 1;
> +		return dmi_callback(d);
>  
>  out:
>  	pmc_clk_is_critical = false;
> @@ -388,6 +396,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"),
> @@ -396,6 +405,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"),
> @@ -404,6 +414,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"),
> @@ -412,6 +423,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"),
> @@ -420,6 +432,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"),
> @@ -428,6 +441,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"),
> @@ -436,13 +450,14 @@ 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"),
>  		},
>  	},
>  	{
> -		.callback = siemens_clk_is_critical,
> +		.callback = dmi_callback_siemens,
>  		.ident = "SIEMENS AG",
>  		.matches = {
>  			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
> @@ -457,7 +472,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;
>  
>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>  	if (!clk_data)
> @@ -468,8 +482,8 @@ static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
>  	if (dmi_check_system(critclk_systems)) {
>  		clk_data->critical = pmc_clk_is_critical;
>  		if (clk_data->critical) {
> -			d = dmi_first_match(critclk_systems);
> -			pr_info("%s critclks quirk enabled\n", d->ident);
> +			pr_info("%s critclks quirk enabled\n",
> +				dmi_critical->ident);
>  		}
>  	}
>  
> 


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

* Re: [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries
  2021-03-15 16:31         ` Hans de Goede
@ 2021-03-15 17:00           ` Henning Schild
  2021-03-15 18:01             ` Hans de Goede
  0 siblings, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-15 17:00 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

Am Mon, 15 Mar 2021 17:31:49 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/15/21 3:58 PM, Henning Schild wrote:
> > Introduce a global variable to remember the matching entry for later
> > printing. Also having a callback allows to stop matching after the
> > first hit.
> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  drivers/platform/x86/pmc_atom.c | 26 ++++++++++++++++++++------
> >  1 file changed, 20 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/platform/x86/pmc_atom.c
> > b/drivers/platform/x86/pmc_atom.c index 38542d547f29..d0f74856cd8b
> > 100644 --- a/drivers/platform/x86/pmc_atom.c
> > +++ b/drivers/platform/x86/pmc_atom.c
> > @@ -364,8 +364,16 @@ static void pmc_dbgfs_register(struct pmc_dev
> > *pmc) #endif /* CONFIG_DEBUG_FS */
> >  
> >  static bool pmc_clk_is_critical = true;
> > +static const struct dmi_system_id *dmi_critical;
> >  
> > -static int siemens_clk_is_critical(const struct dmi_system_id *d)
> > +static int dmi_callback(const struct dmi_system_id *d)
> > +{
> > +	dmi_critical = d;  
> 
> Don't introduce a global variable for this please. Instead just
> directly print the ident of the matching dmi_system_id here.

Sorry, missed that part. Result looks nice and clean, thanks. I think i
will squash it into 4/4 in v3 and not follow up here for now.

Henning

> Regards,
> 
> Hans
> 
> 
> > +
> > +	return 1;
> > +}
> > +
> > +static int dmi_callback_siemens(const struct dmi_system_id *d)
> >  {
> >  	u32 st_id;
> >  
> > @@ -373,7 +381,7 @@ static int siemens_clk_is_critical(const struct
> > dmi_system_id *d) goto out;
> >  
> >  	if (st_id == SIMATIC_IPC_IPC227E || st_id ==
> > SIMATIC_IPC_IPC277E)
> > -		return 1;
> > +		return dmi_callback(d);
> >  
> >  out:
> >  	pmc_clk_is_critical = false;
> > @@ -388,6 +396,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"), @@ -396,6 +405,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"),
> > @@ -404,6 +414,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"),
> > @@ -412,6 +423,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"),
> > @@ -420,6 +432,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"),
> > @@ -428,6 +441,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"),
> > @@ -436,13 +450,14 @@ 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"),
> >  		},
> >  	},
> >  	{
> > -		.callback = siemens_clk_is_critical,
> > +		.callback = dmi_callback_siemens,
> >  		.ident = "SIEMENS AG",
> >  		.matches = {
> >  			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
> > @@ -457,7 +472,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;
> >  
> >  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> >  	if (!clk_data)
> > @@ -468,8 +482,8 @@ static int pmc_setup_clks(struct pci_dev *pdev,
> > void __iomem *pmc_regmap, if (dmi_check_system(critclk_systems)) {
> >  		clk_data->critical = pmc_clk_is_critical;
> >  		if (clk_data->critical) {
> > -			d = dmi_first_match(critclk_systems);
> > -			pr_info("%s critclks quirk enabled\n",
> > d->ident);
> > +			pr_info("%s critclks quirk enabled\n",
> > +				dmi_critical->ident);
> >  		}
> >  	}
> >  
> >   
> 


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

* Re: [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries
  2021-03-15 17:00           ` Henning Schild
@ 2021-03-15 18:01             ` Hans de Goede
  2021-03-16  5:47               ` Henning Schild
  0 siblings, 1 reply; 43+ messages in thread
From: Hans de Goede @ 2021-03-15 18:01 UTC (permalink / raw)
  To: Henning Schild
  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

Hi,

On 3/15/21 6:00 PM, Henning Schild wrote:
> Am Mon, 15 Mar 2021 17:31:49 +0100
> schrieb Hans de Goede <hdegoede@redhat.com>:
> 
>> Hi,
>>
>> On 3/15/21 3:58 PM, Henning Schild wrote:
>>> Introduce a global variable to remember the matching entry for later
>>> printing. Also having a callback allows to stop matching after the
>>> first hit.
>>>
>>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>>> ---
>>>  drivers/platform/x86/pmc_atom.c | 26 ++++++++++++++++++++------
>>>  1 file changed, 20 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/platform/x86/pmc_atom.c
>>> b/drivers/platform/x86/pmc_atom.c index 38542d547f29..d0f74856cd8b
>>> 100644 --- a/drivers/platform/x86/pmc_atom.c
>>> +++ b/drivers/platform/x86/pmc_atom.c
>>> @@ -364,8 +364,16 @@ static void pmc_dbgfs_register(struct pmc_dev
>>> *pmc) #endif /* CONFIG_DEBUG_FS */
>>>  
>>>  static bool pmc_clk_is_critical = true;
>>> +static const struct dmi_system_id *dmi_critical;
>>>  
>>> -static int siemens_clk_is_critical(const struct dmi_system_id *d)
>>> +static int dmi_callback(const struct dmi_system_id *d)
>>> +{
>>> +	dmi_critical = d;  
>>
>> Don't introduce a global variable for this please. Instead just
>> directly print the ident of the matching dmi_system_id here.
> 
> Sorry, missed that part. Result looks nice and clean, thanks. I think i
> will squash it into 4/4 in v3 and not follow up here for now.

Ack, that sounds good to me.

Regards,

Hans


>>> +
>>> +	return 1;
>>> +}
>>> +
>>> +static int dmi_callback_siemens(const struct dmi_system_id *d)
>>>  {
>>>  	u32 st_id;
>>>  
>>> @@ -373,7 +381,7 @@ static int siemens_clk_is_critical(const struct
>>> dmi_system_id *d) goto out;
>>>  
>>>  	if (st_id == SIMATIC_IPC_IPC227E || st_id ==
>>> SIMATIC_IPC_IPC277E)
>>> -		return 1;
>>> +		return dmi_callback(d);
>>>  
>>>  out:
>>>  	pmc_clk_is_critical = false;
>>> @@ -388,6 +396,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"), @@ -396,6 +405,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"),
>>> @@ -404,6 +414,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"),
>>> @@ -412,6 +423,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"),
>>> @@ -420,6 +432,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"),
>>> @@ -428,6 +441,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"),
>>> @@ -436,13 +450,14 @@ 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"),
>>>  		},
>>>  	},
>>>  	{
>>> -		.callback = siemens_clk_is_critical,
>>> +		.callback = dmi_callback_siemens,
>>>  		.ident = "SIEMENS AG",
>>>  		.matches = {
>>>  			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
>>> @@ -457,7 +472,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;
>>>  
>>>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>>>  	if (!clk_data)
>>> @@ -468,8 +482,8 @@ static int pmc_setup_clks(struct pci_dev *pdev,
>>> void __iomem *pmc_regmap, if (dmi_check_system(critclk_systems)) {
>>>  		clk_data->critical = pmc_clk_is_critical;
>>>  		if (clk_data->critical) {
>>> -			d = dmi_first_match(critclk_systems);
>>> -			pr_info("%s critclks quirk enabled\n",
>>> d->ident);
>>> +			pr_info("%s critclks quirk enabled\n",
>>> +				dmi_critical->ident);
>>>  		}
>>>  	}
>>>  
>>>   
>>
> 


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

* Re: [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries
  2021-03-15 18:01             ` Hans de Goede
@ 2021-03-16  5:47               ` Henning Schild
  2021-03-16  9:43                 ` Hans de Goede
  0 siblings, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-16  5:47 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

Hoi Hans,

on a slighly different but also related topic. Did you ever come across
SMSC SCH5347? Seems to be pretty similar to 56xx, only with spec non
public ... and probably less often in use
Maybe you happen to have code, or know the differences. We already have
it working with a modified copy of sch56xx but that is still rough and
i thought i ask before we potentially duplicate work.

groetjes,
Henning

Am Mon, 15 Mar 2021 19:01:13 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/15/21 6:00 PM, Henning Schild wrote:
> > Am Mon, 15 Mar 2021 17:31:49 +0100
> > schrieb Hans de Goede <hdegoede@redhat.com>:
> >   
> >> Hi,
> >>
> >> On 3/15/21 3:58 PM, Henning Schild wrote:  
> >>> Introduce a global variable to remember the matching entry for
> >>> later printing. Also having a callback allows to stop matching
> >>> after the first hit.
> >>>
> >>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> >>> ---
> >>>  drivers/platform/x86/pmc_atom.c | 26 ++++++++++++++++++++------
> >>>  1 file changed, 20 insertions(+), 6 deletions(-)
> >>>
> >>> diff --git a/drivers/platform/x86/pmc_atom.c
> >>> b/drivers/platform/x86/pmc_atom.c index 38542d547f29..d0f74856cd8b
> >>> 100644 --- a/drivers/platform/x86/pmc_atom.c
> >>> +++ b/drivers/platform/x86/pmc_atom.c
> >>> @@ -364,8 +364,16 @@ static void pmc_dbgfs_register(struct pmc_dev
> >>> *pmc) #endif /* CONFIG_DEBUG_FS */
> >>>  
> >>>  static bool pmc_clk_is_critical = true;
> >>> +static const struct dmi_system_id *dmi_critical;
> >>>  
> >>> -static int siemens_clk_is_critical(const struct dmi_system_id *d)
> >>> +static int dmi_callback(const struct dmi_system_id *d)
> >>> +{
> >>> +	dmi_critical = d;    
> >>
> >> Don't introduce a global variable for this please. Instead just
> >> directly print the ident of the matching dmi_system_id here.  
> > 
> > Sorry, missed that part. Result looks nice and clean, thanks. I
> > think i will squash it into 4/4 in v3 and not follow up here for
> > now.  
> 
> Ack, that sounds good to me.
> 
> Regards,
> 
> Hans
> 
> 
> >>> +
> >>> +	return 1;
> >>> +}
> >>> +
> >>> +static int dmi_callback_siemens(const struct dmi_system_id *d)
> >>>  {
> >>>  	u32 st_id;
> >>>  
> >>> @@ -373,7 +381,7 @@ static int siemens_clk_is_critical(const
> >>> struct dmi_system_id *d) goto out;
> >>>  
> >>>  	if (st_id == SIMATIC_IPC_IPC227E || st_id ==
> >>> SIMATIC_IPC_IPC277E)
> >>> -		return 1;
> >>> +		return dmi_callback(d);
> >>>  
> >>>  out:
> >>>  	pmc_clk_is_critical = false;
> >>> @@ -388,6 +396,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"), @@ -396,6 +405,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"),
> >>> @@ -404,6 +414,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"),
> >>> @@ -412,6 +423,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"),
> >>> @@ -420,6 +432,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"),
> >>> @@ -428,6 +441,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"),
> >>> @@ -436,13 +450,14 @@ 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"),
> >>>  		},
> >>>  	},
> >>>  	{
> >>> -		.callback = siemens_clk_is_critical,
> >>> +		.callback = dmi_callback_siemens,
> >>>  		.ident = "SIEMENS AG",
> >>>  		.matches = {
> >>>  			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
> >>> @@ -457,7 +472,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;
> >>>  
> >>>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
> >>>  	if (!clk_data)
> >>> @@ -468,8 +482,8 @@ static int pmc_setup_clks(struct pci_dev
> >>> *pdev, void __iomem *pmc_regmap, if
> >>> (dmi_check_system(critclk_systems)) { clk_data->critical =
> >>> pmc_clk_is_critical; if (clk_data->critical) {
> >>> -			d = dmi_first_match(critclk_systems);
> >>> -			pr_info("%s critclks quirk enabled\n",
> >>> d->ident);
> >>> +			pr_info("%s critclks quirk enabled\n",
> >>> +				dmi_critical->ident);
> >>>  		}
> >>>  	}
> >>>  
> >>>     
> >>  
> >   
> 


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

* Re: [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries
  2021-03-16  5:47               ` Henning Schild
@ 2021-03-16  9:43                 ` Hans de Goede
  0 siblings, 0 replies; 43+ messages in thread
From: Hans de Goede @ 2021-03-16  9:43 UTC (permalink / raw)
  To: Henning Schild
  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

Hi Henning,

On 3/16/21 6:47 AM, Henning Schild wrote:
> Hoi Hans,
> 
> on a slighly different but also related topic. Did you ever come across
> SMSC SCH5347? Seems to be pretty similar to 56xx, only with spec non
> public ... and probably less often in use
> Maybe you happen to have code, or know the differences. We already have
> it working with a modified copy of sch56xx but that is still rough and
> i thought i ask before we potentially duplicate work.

The history of FSC (Fujitsu-Siemens Computers) sensor support goes something
like this:

1. The university which I worked at had picked a FSC machine for the
replacement of all workstations, the FSCXXX sensor chip it had was i2c based,
so I could relatively easy reverse-engineer it and wrote a driver.

2. Around the time I stopped working for the university and started working
for Red Hat (2008) FSC contacted me and asked me if I wanted to help them
with officially supporting the FSC and later the SCH devices. They provided
test hardware and documentation at this time.

3. This continued for a while when FSC became just "Fujitsu" and things
moved from the FSC i2c based chips to the super-io based SCH chips. After
a while Fujitsu stopped contacting me about new chips and I stopped working
on this.

So ATM I'm not working no SCH56xx support and I've not worked on that for
quite some time now. So it is good to hear that you will be contributing
SCH5347 support to the kernel.

Regards,

Hans






> Am Mon, 15 Mar 2021 19:01:13 +0100
> schrieb Hans de Goede <hdegoede@redhat.com>:
> 
>> Hi,
>>
>> On 3/15/21 6:00 PM, Henning Schild wrote:
>>> Am Mon, 15 Mar 2021 17:31:49 +0100
>>> schrieb Hans de Goede <hdegoede@redhat.com>:
>>>   
>>>> Hi,
>>>>
>>>> On 3/15/21 3:58 PM, Henning Schild wrote:  
>>>>> Introduce a global variable to remember the matching entry for
>>>>> later printing. Also having a callback allows to stop matching
>>>>> after the first hit.
>>>>>
>>>>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>>>>> ---
>>>>>  drivers/platform/x86/pmc_atom.c | 26 ++++++++++++++++++++------
>>>>>  1 file changed, 20 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/drivers/platform/x86/pmc_atom.c
>>>>> b/drivers/platform/x86/pmc_atom.c index 38542d547f29..d0f74856cd8b
>>>>> 100644 --- a/drivers/platform/x86/pmc_atom.c
>>>>> +++ b/drivers/platform/x86/pmc_atom.c
>>>>> @@ -364,8 +364,16 @@ static void pmc_dbgfs_register(struct pmc_dev
>>>>> *pmc) #endif /* CONFIG_DEBUG_FS */
>>>>>  
>>>>>  static bool pmc_clk_is_critical = true;
>>>>> +static const struct dmi_system_id *dmi_critical;
>>>>>  
>>>>> -static int siemens_clk_is_critical(const struct dmi_system_id *d)
>>>>> +static int dmi_callback(const struct dmi_system_id *d)
>>>>> +{
>>>>> +	dmi_critical = d;    
>>>>
>>>> Don't introduce a global variable for this please. Instead just
>>>> directly print the ident of the matching dmi_system_id here.  
>>>
>>> Sorry, missed that part. Result looks nice and clean, thanks. I
>>> think i will squash it into 4/4 in v3 and not follow up here for
>>> now.  
>>
>> Ack, that sounds good to me.
>>
>> Regards,
>>
>> Hans
>>
>>
>>>>> +
>>>>> +	return 1;
>>>>> +}
>>>>> +
>>>>> +static int dmi_callback_siemens(const struct dmi_system_id *d)
>>>>>  {
>>>>>  	u32 st_id;
>>>>>  
>>>>> @@ -373,7 +381,7 @@ static int siemens_clk_is_critical(const
>>>>> struct dmi_system_id *d) goto out;
>>>>>  
>>>>>  	if (st_id == SIMATIC_IPC_IPC227E || st_id ==
>>>>> SIMATIC_IPC_IPC277E)
>>>>> -		return 1;
>>>>> +		return dmi_callback(d);
>>>>>  
>>>>>  out:
>>>>>  	pmc_clk_is_critical = false;
>>>>> @@ -388,6 +396,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"), @@ -396,6 +405,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"),
>>>>> @@ -404,6 +414,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"),
>>>>> @@ -412,6 +423,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"),
>>>>> @@ -420,6 +432,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"),
>>>>> @@ -428,6 +441,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"),
>>>>> @@ -436,13 +450,14 @@ 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"),
>>>>>  		},
>>>>>  	},
>>>>>  	{
>>>>> -		.callback = siemens_clk_is_critical,
>>>>> +		.callback = dmi_callback_siemens,
>>>>>  		.ident = "SIEMENS AG",
>>>>>  		.matches = {
>>>>>  			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
>>>>> @@ -457,7 +472,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;
>>>>>  
>>>>>  	clk_data = kzalloc(sizeof(*clk_data), GFP_KERNEL);
>>>>>  	if (!clk_data)
>>>>> @@ -468,8 +482,8 @@ static int pmc_setup_clks(struct pci_dev
>>>>> *pdev, void __iomem *pmc_regmap, if
>>>>> (dmi_check_system(critclk_systems)) { clk_data->critical =
>>>>> pmc_clk_is_critical; if (clk_data->critical) {
>>>>> -			d = dmi_first_match(critclk_systems);
>>>>> -			pr_info("%s critclks quirk enabled\n",
>>>>> d->ident);
>>>>> +			pr_info("%s critclks quirk enabled\n",
>>>>> +				dmi_critical->ident);
>>>>>  		}
>>>>>  	}
>>>>>  
>>>>>     
>>>>  
>>>   
>>
> 


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

* Re: [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-15 10:31   ` Andy Shevchenko
  2021-03-15 16:30     ` Henning Schild
@ 2021-03-17 19:13     ` Henning Schild
  2021-03-17 20:03       ` Hans de Goede
  1 sibling, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-17 19:13 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

Am Mon, 15 Mar 2021 12:31:11 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Mon, Mar 15, 2021 at 12:02 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > 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.  
> 
> ...
> 
> > +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;
> > +}  
> 
> Why not use MFD here?

Never had a close look at mfd to be honest. I might

With the custom dmi matching on 129 being part of the header, and the
p2sb unhide moving out as well ... that first driver ends up being not
too valuable indeed

It just identifies the box and tells subsequent drivers which one it
is, which watchdog and LED path to take. Moving the knowledge of which
box has which LED/watchdog into the respective drivers seems to be the
better way to go.

So we would end up with a LED and a watchdog driver both
MODULE_ALIAS("dmi:*:svnSIEMENSAG:*");
and doing the identification with the inline dmi from that header,
doing p2sb with the support to come ... possibly a "//TODO\ninline" in
the meantime.

So no "main platform" driver anymore, but still central platform
headers.

Not sure how this sounds, but i think making that change should be
possible. And that is what i will try and go for in v3.

regards,
Henning

> ...
> 
> > +/*
> > + * 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
> > + */  
> 
> No blank line here.
> 
> I would add FIXME or REVISIT here to point out that this should be
> deduplicated in the future.
> 
> > +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> > +{
> > +       struct pci_bus *bus;
> > +       u32 bar0 = 0;
> > +
> > +       /*
> > +        * The GPIO memory is bar0 of the hidden P2SB device.
> > Unhide the device  
> 
> No, it's not a GPIO's bar. It's P2SB's one. GPIO resides in that bar
> somewhere.
> 
> > +        * 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 inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
> > +{
> > +       u32 station_id = SIMATIC_IPC_INVALID_STATION_ID;
> > +       int i;  
> 
> Reversed xmas tree order, please.
> 
> > +       struct {
> > +               u8      type;           /* type (0xff = binary) */
> > +               u8      len;            /* len of data entry */
> > +               u8      reserved[3];
> > +               u32     station_id;     /* station id (LE) */  
> 
> > +       } __packed
> > +       *data_entry = (void *)data + sizeof(struct dmi_header);  
> 
> Can be one line.
> 
> > +       /* find 4th entry in OEM data */
> > +       for (i = 0; i < 3; i++)  
> 
> 3 is magic!
> 
> > +               data_entry = (void *)((u8 *)(data_entry) +
> > data_entry->len); +
> > +       /* decode station id */
> > +       if (data_entry && (u8 *)data_entry < data + max_len &&
> > +           data_entry->type == 0xff && data_entry->len == 9)
> > +               station_id = le32_to_cpu(data_entry->station_id);
> > +
> > +       return station_id;
> > +}  
> 


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

* Re: [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-17 19:13     ` Henning Schild
@ 2021-03-17 20:03       ` Hans de Goede
  2021-03-18 11:30         ` Enrico Weigelt, metux IT consult
  0 siblings, 1 reply; 43+ messages in thread
From: Hans de Goede @ 2021-03-17 20:03 UTC (permalink / raw)
  To: Henning Schild, Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Pavel Machek

Hi,

On 3/17/21 8:13 PM, Henning Schild wrote:
> Am Mon, 15 Mar 2021 12:31:11 +0200
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> 
>> On Mon, Mar 15, 2021 at 12:02 PM Henning Schild
>> <henning.schild@siemens.com> wrote:
>>>
>>> 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.  
>>
>> ...
>>
>>> +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;
>>> +}  
>>
>> Why not use MFD here?
> 
> Never had a close look at mfd to be honest. I might
> 
> With the custom dmi matching on 129 being part of the header, and the
> p2sb unhide moving out as well ... that first driver ends up being not
> too valuable indeed
> 
> It just identifies the box and tells subsequent drivers which one it
> is, which watchdog and LED path to take. Moving the knowledge of which
> box has which LED/watchdog into the respective drivers seems to be the
> better way to go.
> 
> So we would end up with a LED and a watchdog driver both
> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*");
> and doing the identification with the inline dmi from that header,
> doing p2sb with the support to come ... possibly a "//TODO\ninline" in
> the meantime.
> 
> So no "main platform" driver anymore, but still central platform
> headers.
> 
> Not sure how this sounds, but i think making that change should be
> possible. And that is what i will try and go for in v3.

Dropping the main drivers/platform/x86 driver sounds good to me,
I was already wondering a bit about its function since it just
instantiates devs to which the other ones bind to then instantiate
more devs (in the LED case).

Regards,

Hans


>> ...
>>
>>> +/*
>>> + * 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
>>> + */  
>>
>> No blank line here.
>>
>> I would add FIXME or REVISIT here to point out that this should be
>> deduplicated in the future.
>>
>>> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
>>> +{
>>> +       struct pci_bus *bus;
>>> +       u32 bar0 = 0;
>>> +
>>> +       /*
>>> +        * The GPIO memory is bar0 of the hidden P2SB device.
>>> Unhide the device  
>>
>> No, it's not a GPIO's bar. It's P2SB's one. GPIO resides in that bar
>> somewhere.
>>
>>> +        * 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 inline u32 simatic_ipc_get_station_id(u8 *data, int max_len)
>>> +{
>>> +       u32 station_id = SIMATIC_IPC_INVALID_STATION_ID;
>>> +       int i;  
>>
>> Reversed xmas tree order, please.
>>
>>> +       struct {
>>> +               u8      type;           /* type (0xff = binary) */
>>> +               u8      len;            /* len of data entry */
>>> +               u8      reserved[3];
>>> +               u32     station_id;     /* station id (LE) */  
>>
>>> +       } __packed
>>> +       *data_entry = (void *)data + sizeof(struct dmi_header);  
>>
>> Can be one line.
>>
>>> +       /* find 4th entry in OEM data */
>>> +       for (i = 0; i < 3; i++)  
>>
>> 3 is magic!
>>
>>> +               data_entry = (void *)((u8 *)(data_entry) +
>>> data_entry->len); +
>>> +       /* decode station id */
>>> +       if (data_entry && (u8 *)data_entry < data + max_len &&
>>> +           data_entry->type == 0xff && data_entry->len == 9)
>>> +               station_id = le32_to_cpu(data_entry->station_id);
>>> +
>>> +       return station_id;
>>> +}  
>>
> 


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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15  9:57 ` [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
  2021-03-15 10:48   ` Andy Shevchenko
@ 2021-03-18 10:25   ` Enrico Weigelt, metux IT consult
  2021-03-27 11:16     ` Henning Schild
  1 sibling, 1 reply; 43+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-03-18 10:25 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 15.03.21 10:57, Henning Schild wrote:

Hi,

> diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
> new file mode 100644
> index 000000000000..0f7e6320e10d
> --- /dev/null
> +++ b/drivers/leds/simple/simatic-ipc-leds.c
> @@ -0,0 +1,210 @@
> +// 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[32];
> +	struct led_classdev cdev;
> +};
> +
> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> +	{1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" },
> +	{1 << 7,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" },
> +	{1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
> +	{1 << 6,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" },
> +	{1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
> +	{1 << 5,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" },
> +	{0, ""},
> +};

Wouldn't it be better to name them like they're labeled on the device,
as shown on page #19 of the manual, or perhaps a little bit more
generic nameing (eg. power, status, error, maint) ?

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

hmm, could there *ever* be multiple instances of the driver ?

Wouldn't it be better to put this in the device priv data instead ?

> +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> +	{0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-1"},
> +	{0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1"},
> +	{0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2"},
> +	{0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS "-2"},
> +	{0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3"},
> +	{0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS "-3"},
> +	{0, ""},
> +};
> +
> +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);

Does this protect global data structures ? If not, I'd rather put it
into the device priv data instead.

BTW: doesn't have struct led_classdev already have a lock that
can be used ? Can multiple calls to led ops (within the same device)
at the same time happen at all, or does led core already serialize
that ?

> +static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
> +				   enum led_brightness brightness)
> +{
> +	struct simatic_ipc_led *led =
> +		container_of(led_cd, struct simatic_ipc_led, cdev);
> +	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);

Don't we already have an helper for setting or clearing bits in IO
registers (that already does the read + set/clear + write at once) ?

Does that really need to be protected by lock ?
(can happen multiple calls to that func from different threads happen
at all ?)

Is the port really *always* the same, so it really can be a const ?

<snip>

> +static int simatic_ipc_leds_probe(struct platform_device *pdev)
> +{
> +	struct simatic_ipc_platform *plat;
> +	struct device *dev = &pdev->dev;
> +	struct simatic_ipc_led *ipcled;
> +	struct led_classdev *cdev;
> +	struct resource *res;
> +	int err, type;
> +	u32 *p;
> +
> +	plat = pdev->dev.platform_data;

Maybe put this into swnode ?

IIRC, the consensus is not to introduce new platform data structs
anymore, instead legacy pdata to swnode some day.

> +	switch (plat->devmode) {
> +	case SIMATIC_IPC_DEVICE_227D:
> +	case SIMATIC_IPC_DEVICE_427E:
> +		res = &simatic_ipc_led_io_res;
> +		ipcled = simatic_ipc_leds_io;
> +		/* the 227D is high on while 427E is low on, invert the struct
> +		 * we have
> +		 */
> +		if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
> +			while (ipcled->value) {
> +				ipcled->value = swab16(ipcled->value);

Uff, better use explicit endian conversion macros (eg. be*_to_cpu()) for
that.

Also, I wouldn't change those global structs, instead put those data
into device priv data and make the global stuff const. You could also
use the same field for both port-io and mmap'ed variants. And adding
regmap to the equation, could use the same led ops for both. (IMHO,
the little bit of overhead by regmap shouldn't matter here)

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

Where does that device actually sit on ? Some generic card ? Some ASIC
or FPGA ?

It seems this driver is instantiated by another one, which already knows
what device we're actually dealing with (as it sets plat->devmode).
Why not letting that parent device also tell the io resource to this
driver ?

> +	while (ipcled->value) {
> +		cdev = &ipcled->cdev;
> +		cdev->brightness_set = simatic_ipc_led_set_io;
> +		cdev->brightness_get = simatic_ipc_led_get_io;
> +		if (type == IORESOURCE_MEM) {
> +			cdev->brightness_set = simatic_ipc_led_set_mem;
> +			cdev->brightness_get = simatic_ipc_led_get_mem;
> +		}

Why not if/else ?

> +		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 led_driver = {

Why not calling it simatic_ipc_led_driver ?

> +	.probe = simatic_ipc_leds_probe,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +	},
> +};
> +
> +module_platform_driver(led_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
> 


--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] 43+ messages in thread

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15 10:48   ` Andy Shevchenko
  2021-03-15 11:19     ` Pavel Machek
@ 2021-03-18 10:27     ` Enrico Weigelt, metux IT consult
  2021-03-18 12:40       ` Alexander Dahl
  2021-03-26 16:33     ` Henning Schild
  2 siblings, 1 reply; 43+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-03-18 10:27 UTC (permalink / raw)
  To: Andy Shevchenko, 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

On 15.03.21 11:48, Andy Shevchenko wrote:

Hi,

> I have a question, why we can't provide a GPIO driver which is already
> in the kernel and, with use of the patch series I sent, to convert
> this all magic to GPIO LEDs as it's done for all normal cases?

Do we alread have a generic led driver that for cases that just
set/clear bits in some mem/io location ? If not, that would be really
great to have.


--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] 43+ messages in thread

* Re: [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-17 20:03       ` Hans de Goede
@ 2021-03-18 11:30         ` Enrico Weigelt, metux IT consult
  2021-03-18 11:45           ` Hans de Goede
  0 siblings, 1 reply; 43+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-03-18 11:30 UTC (permalink / raw)
  To: Hans de Goede, Henning Schild, Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Pavel Machek

On 17.03.21 21:03, Hans de Goede wrote:

Hi,

>> It just identifies the box and tells subsequent drivers which one it
>> is, which watchdog and LED path to take. Moving the knowledge of which
>> box has which LED/watchdog into the respective drivers seems to be the
>> better way to go.
>>
>> So we would end up with a LED and a watchdog driver both
>> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*");

Uh, isn't that a bit too broad ? This basically implies that Siemens
will never produce boards with different configurations.

>> and doing the identification with the inline dmi from that header,
>> doing p2sb with the support to come ... possibly a "//TODO\ninline" in
>> the meantime.
>>
>> So no "main platform" driver anymore, but still central platform
>> headers.
>>
>> Not sure how this sounds, but i think making that change should be
>> possible. And that is what i will try and go for in v3.
> 
> Dropping the main drivers/platform/x86 driver sounds good to me,
> I was already wondering a bit about its function since it just
> instantiates devs to which the other ones bind to then instantiate
> more devs (in the LED case).

hmm, IMHO that depends on whether the individual sub-devices can be
more generic than just that specific machine. (@Hanning: could you
tell us more about that ?).

Another question is how they're actually probed .. only dmi or maybe
also pci dev ? (i've seen some refs to pci stuff in the led driver, but
missed the other code thats called here).

IMHO, if the whole thing lives on some PCI device (which can be probed
via pci ID), and that device has the knowledge, where the LED registers
actually are (eg. based on device ID, pci mmio mapping, ...) then there
should be some parent driver that instantiates the led devices (and
possibly other board specific stuff). That would be a clear separation,
modularization. In that case, maybe this LED driver could even be
replaced by some really generic "register-based-LED" driver, which just
needs to be fed with some parameters like register ranges, bitmasks, etc.

OTOH, if everything can be derived entirely from DMI match, w/o things
like pci mappings involved (IOW: behaves like directly wired to the
cpu's mem/io bus, no other "intelligent" bus involved), and it's all
really board specific logic (no generic led or gpio controllers
involved), then it might be better to have entirely separate drivers.


-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] 43+ messages in thread

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15 11:19     ` Pavel Machek
  2021-03-15 11:26       ` Andy Shevchenko
  2021-03-15 12:40       ` Henning Schild
@ 2021-03-18 11:38       ` Enrico Weigelt, metux IT consult
  2021-03-27  9:46         ` Henning Schild
  2021-03-27  9:56       ` Henning Schild
  3 siblings, 1 reply; 43+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-03-18 11:38 UTC (permalink / raw)
  To: Pavel Machek, Andy Shevchenko
  Cc: Henning Schild, 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

On 15.03.21 12:19, Pavel Machek wrote:

> But I still don't like the naming. simantic-ipc: prefix is
> useless. Having 6 status leds is not good, either.

Do we have some standard naming policy those kinds of LEDs ?

In this case, they seem to be assigned to certain specific functions (by 
physical labels on the box), so IMHO the LED names should reflect that
in some ways.

There're other cases (eg. apu board familiy) that just have several
front panel leds w/o any dedication, so we can just count them up.


--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] 43+ messages in thread

* Re: [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-18 11:30         ` Enrico Weigelt, metux IT consult
@ 2021-03-18 11:45           ` Hans de Goede
  2021-03-26  9:55             ` Henning Schild
  0 siblings, 1 reply; 43+ messages in thread
From: Hans de Goede @ 2021-03-18 11:45 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult, Henning Schild, Andy Shevchenko
  Cc: Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Pavel Machek

Hi,

On 3/18/21 12:30 PM, Enrico Weigelt, metux IT consult wrote:
> On 17.03.21 21:03, Hans de Goede wrote:
> 
> Hi,
> 
>>> It just identifies the box and tells subsequent drivers which one it
>>> is, which watchdog and LED path to take. Moving the knowledge of which
>>> box has which LED/watchdog into the respective drivers seems to be the
>>> better way to go.
>>>
>>> So we would end up with a LED and a watchdog driver both
>>> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*");
> 
> Uh, isn't that a bit too broad ? This basically implies that Siemens
> will never produce boards with different configurations.

There is a further check done in probe() based on some Siemens specific
DMI table entries.

>>> and doing the identification with the inline dmi from that header,
>>> doing p2sb with the support to come ... possibly a "//TODO\ninline" in
>>> the meantime.
>>>
>>> So no "main platform" driver anymore, but still central platform
>>> headers.
>>>
>>> Not sure how this sounds, but i think making that change should be
>>> possible. And that is what i will try and go for in v3.
>>
>> Dropping the main drivers/platform/x86 driver sounds good to me,
>> I was already wondering a bit about its function since it just
>> instantiates devs to which the other ones bind to then instantiate
>> more devs (in the LED case).
> 
> hmm, IMHO that depends on whether the individual sub-devices can be
> more generic than just that specific machine. (@Hanning: could you
> tell us more about that ?).
> 
> Another question is how they're actually probed .. only dmi or maybe
> also pci dev ? (i've seen some refs to pci stuff in the led driver, but
> missed the other code thats called here).
> 
> IMHO, if the whole thing lives on some PCI device (which can be probed
> via pci ID), and that device has the knowledge, where the LED registers
> actually are (eg. based on device ID, pci mmio mapping, ...) then there
> should be some parent driver that instantiates the led devices (and
> possibly other board specific stuff). That would be a clear separation,
> modularization. In that case, maybe this LED driver could even be
> replaced by some really generic "register-based-LED" driver, which just
> needs to be fed with some parameters like register ranges, bitmasks, etc.
> 
> OTOH, if everything can be derived entirely from DMI match, w/o things
> like pci mappings involved (IOW: behaves like directly wired to the
> cpu's mem/io bus, no other "intelligent" bus involved), and it's all
> really board specific logic (no generic led or gpio controllers
> involved), then it might be better to have entirely separate drivers.

FWIW I'm fine with either solution, and if we go the "parent driver"
route I'm happy to have that driver sit in drivers/platform/x86
(once all the discussions surrounding this are resolved).

My reply was because I noticed that the Led driver seemed to sort of
also act as a parent driver (last time I looked) and instantiated a
bunch of stuff, so then we have 2 parent(ish) drivers. If things stay
that way then having 2 levels of parent drivers seems a bit too much
to me, esp. if it can all be done cleanly in e.g. the LED driver.

But as said I'm fine either way as long as the code is reasonably
clean and dealing with this sort of platform specific warts happens
a lot in drivers/platform/x86 .

Regards,

Hans


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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-18 10:27     ` Enrico Weigelt, metux IT consult
@ 2021-03-18 12:40       ` Alexander Dahl
  2021-03-23 17:45         ` Henning Schild
  0 siblings, 1 reply; 43+ messages in thread
From: Alexander Dahl @ 2021-03-18 12:40 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult, Andy Shevchenko, 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

Hei hei,

> Enrico Weigelt, metux IT consult <lkml@metux.net> hat am 18.03.2021 11:27 geschrieben:
> 
>  
> On 15.03.21 11:48, Andy Shevchenko wrote:
> 
> Hi,
> 
> > I have a question, why we can't provide a GPIO driver which is already
> > in the kernel and, with use of the patch series I sent, to convert
> > this all magic to GPIO LEDs as it's done for all normal cases?
> 
> Do we alread have a generic led driver that for cases that just
> set/clear bits in some mem/io location ? If not, that would be really
> great to have.

Yes, there is. Look out for compatible "register-bit-led" in device tree. That's from driver in drivers/leds/leds-syscon.c and you can use it inside a syscon node in dts.

It assumes one bit per LED.

Greets
Alex

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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-18 12:40       ` Alexander Dahl
@ 2021-03-23 17:45         ` Henning Schild
  0 siblings, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-23 17:45 UTC (permalink / raw)
  To: Alexander Dahl
  Cc: Enrico Weigelt, metux IT consult, Andy Shevchenko,
	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, 18 Mar 2021 13:40:58 +0100
schrieb Alexander Dahl <ada@thorsis.com>:

> Hei hei,
> 
> > Enrico Weigelt, metux IT consult <lkml@metux.net> hat am 18.03.2021
> > 11:27 geschrieben:
> > 
> >  
> > On 15.03.21 11:48, Andy Shevchenko wrote:
> > 
> > Hi,
> >   
> > > I have a question, why we can't provide a GPIO driver which is
> > > already in the kernel and, with use of the patch series I sent,
> > > to convert this all magic to GPIO LEDs as it's done for all
> > > normal cases?  
> > 
> > Do we alread have a generic led driver that for cases that just
> > set/clear bits in some mem/io location ? If not, that would be
> > really great to have.  
> 
> Yes, there is. Look out for compatible "register-bit-led" in device
> tree. That's from driver in drivers/leds/leds-syscon.c and you can
> use it inside a syscon node in dts.
> 
> It assumes one bit per LED.

Sorry guys, i am lost here. Is there a driver i can base mine on, if so
which one? Maybe you can point me to a good example that is
conceptually similar.

As i already wrote in the reviews of v1, the ACPI tables will not
change on the machines in question. So there is a need for a driver.
Either one like i did propose or maybe something that patches ACPI or
loads device-tree snippets, again please point me to good examples.

We are talking about x86-only here.

Henning 

> Greets
> Alex


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

* Re: [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-18 11:45           ` Hans de Goede
@ 2021-03-26  9:55             ` Henning Schild
  2021-03-26 12:21               ` Hans de Goede
  0 siblings, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-26  9:55 UTC (permalink / raw)
  To: Hans de Goede
  Cc: Enrico Weigelt, metux IT consult, Andy Shevchenko,
	Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Pavel Machek

Am Thu, 18 Mar 2021 12:45:01 +0100
schrieb Hans de Goede <hdegoede@redhat.com>:

> Hi,
> 
> On 3/18/21 12:30 PM, Enrico Weigelt, metux IT consult wrote:
> > On 17.03.21 21:03, Hans de Goede wrote:
> > 
> > Hi,
> >   
> >>> It just identifies the box and tells subsequent drivers which one
> >>> it is, which watchdog and LED path to take. Moving the knowledge
> >>> of which box has which LED/watchdog into the respective drivers
> >>> seems to be the better way to go.
> >>>
> >>> So we would end up with a LED and a watchdog driver both
> >>> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*");  
> > 
> > Uh, isn't that a bit too broad ? This basically implies that Siemens
> > will never produce boards with different configurations.  
> 
> There is a further check done in probe() based on some Siemens
> specific DMI table entries.
> 
> >>> and doing the identification with the inline dmi from that header,
> >>> doing p2sb with the support to come ... possibly a
> >>> "//TODO\ninline" in the meantime.
> >>>
> >>> So no "main platform" driver anymore, but still central platform
> >>> headers.
> >>>
> >>> Not sure how this sounds, but i think making that change should be
> >>> possible. And that is what i will try and go for in v3.  
> >>
> >> Dropping the main drivers/platform/x86 driver sounds good to me,
> >> I was already wondering a bit about its function since it just
> >> instantiates devs to which the other ones bind to then instantiate
> >> more devs (in the LED case).  
> > 
> > hmm, IMHO that depends on whether the individual sub-devices can be
> > more generic than just that specific machine. (@Hanning: could you
> > tell us more about that ?).
> > 
> > Another question is how they're actually probed .. only dmi or maybe
> > also pci dev ? (i've seen some refs to pci stuff in the led driver,
> > but missed the other code thats called here).
> > 
> > IMHO, if the whole thing lives on some PCI device (which can be
> > probed via pci ID), and that device has the knowledge, where the
> > LED registers actually are (eg. based on device ID, pci mmio
> > mapping, ...) then there should be some parent driver that
> > instantiates the led devices (and possibly other board specific
> > stuff). That would be a clear separation, modularization. In that
> > case, maybe this LED driver could even be replaced by some really
> > generic "register-based-LED" driver, which just needs to be fed
> > with some parameters like register ranges, bitmasks, etc.
> > 
> > OTOH, if everything can be derived entirely from DMI match, w/o
> > things like pci mappings involved (IOW: behaves like directly wired
> > to the cpu's mem/io bus, no other "intelligent" bus involved), and
> > it's all really board specific logic (no generic led or gpio
> > controllers involved), then it might be better to have entirely
> > separate drivers.  

In fact it does dmi and not "common" but unfortunately vendor-specific.
On top it does pci, so it might be fair to call it "intelligent" and
keep it.

> FWIW I'm fine with either solution, and if we go the "parent driver"
> route I'm happy to have that driver sit in drivers/platform/x86
> (once all the discussions surrounding this are resolved).
> 
> My reply was because I noticed that the Led driver seemed to sort of
> also act as a parent driver (last time I looked) and instantiated
> a bunch of stuff, so then we have 2 parent(ish) drivers. If things
> stay that way then having 2 levels of parent drivers seems a bit too
> much to me, esp. if it can all be done cleanly in e.g. the LED driver.

One "leds" driver doing multiple leds seems to be a common pattern. So
that "1 parent N children" maybe does not count as parentish.

> But as said I'm fine either way as long as the code is reasonably
> clean and dealing with this sort of platform specific warts happens
> a lot in drivers/platform/x86 .

I thought about it again and also prefer the "parent driver" idea as it
is. That parent identifies the machine and depending on it, causes
device drivers to be loaded. At the moment LED and watchdog, but with
nvram, hwmon to come.

I will stick with "platform" instead of "mfd" because it is really a
machine having multiple devices. Not a device having multiple functions.

regards,
Henning

> Regards,
> 
> Hans
> 


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

* Re: [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-26  9:55             ` Henning Schild
@ 2021-03-26 12:21               ` Hans de Goede
  0 siblings, 0 replies; 43+ messages in thread
From: Hans de Goede @ 2021-03-26 12:21 UTC (permalink / raw)
  To: Henning Schild
  Cc: Enrico Weigelt, metux IT consult, Andy Shevchenko,
	Linux Kernel Mailing List, Linux LED Subsystem, Platform Driver,
	linux-watchdog, Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler,
	Guenter Roeck, Wim Van Sebroeck, Mark Gross, Pavel Machek

Hi,

On 3/26/21 10:55 AM, Henning Schild wrote:
> Am Thu, 18 Mar 2021 12:45:01 +0100
> schrieb Hans de Goede <hdegoede@redhat.com>:
> 
>> Hi,
>>
>> On 3/18/21 12:30 PM, Enrico Weigelt, metux IT consult wrote:
>>> On 17.03.21 21:03, Hans de Goede wrote:
>>>
>>> Hi,
>>>   
>>>>> It just identifies the box and tells subsequent drivers which one
>>>>> it is, which watchdog and LED path to take. Moving the knowledge
>>>>> of which box has which LED/watchdog into the respective drivers
>>>>> seems to be the better way to go.
>>>>>
>>>>> So we would end up with a LED and a watchdog driver both
>>>>> MODULE_ALIAS("dmi:*:svnSIEMENSAG:*");  
>>>
>>> Uh, isn't that a bit too broad ? This basically implies that Siemens
>>> will never produce boards with different configurations.  
>>
>> There is a further check done in probe() based on some Siemens
>> specific DMI table entries.
>>
>>>>> and doing the identification with the inline dmi from that header,
>>>>> doing p2sb with the support to come ... possibly a
>>>>> "//TODO\ninline" in the meantime.
>>>>>
>>>>> So no "main platform" driver anymore, but still central platform
>>>>> headers.
>>>>>
>>>>> Not sure how this sounds, but i think making that change should be
>>>>> possible. And that is what i will try and go for in v3.  
>>>>
>>>> Dropping the main drivers/platform/x86 driver sounds good to me,
>>>> I was already wondering a bit about its function since it just
>>>> instantiates devs to which the other ones bind to then instantiate
>>>> more devs (in the LED case).  
>>>
>>> hmm, IMHO that depends on whether the individual sub-devices can be
>>> more generic than just that specific machine. (@Hanning: could you
>>> tell us more about that ?).
>>>
>>> Another question is how they're actually probed .. only dmi or maybe
>>> also pci dev ? (i've seen some refs to pci stuff in the led driver,
>>> but missed the other code thats called here).
>>>
>>> IMHO, if the whole thing lives on some PCI device (which can be
>>> probed via pci ID), and that device has the knowledge, where the
>>> LED registers actually are (eg. based on device ID, pci mmio
>>> mapping, ...) then there should be some parent driver that
>>> instantiates the led devices (and possibly other board specific
>>> stuff). That would be a clear separation, modularization. In that
>>> case, maybe this LED driver could even be replaced by some really
>>> generic "register-based-LED" driver, which just needs to be fed
>>> with some parameters like register ranges, bitmasks, etc.
>>>
>>> OTOH, if everything can be derived entirely from DMI match, w/o
>>> things like pci mappings involved (IOW: behaves like directly wired
>>> to the cpu's mem/io bus, no other "intelligent" bus involved), and
>>> it's all really board specific logic (no generic led or gpio
>>> controllers involved), then it might be better to have entirely
>>> separate drivers.  
> 
> In fact it does dmi and not "common" but unfortunately vendor-specific.
> On top it does pci, so it might be fair to call it "intelligent" and
> keep it.
> 
>> FWIW I'm fine with either solution, and if we go the "parent driver"
>> route I'm happy to have that driver sit in drivers/platform/x86
>> (once all the discussions surrounding this are resolved).
>>
>> My reply was because I noticed that the Led driver seemed to sort of
>> also act as a parent driver (last time I looked) and instantiated
>> a bunch of stuff, so then we have 2 parent(ish) drivers. If things
>> stay that way then having 2 levels of parent drivers seems a bit too
>> much to me, esp. if it can all be done cleanly in e.g. the LED driver.
> 
> One "leds" driver doing multiple leds seems to be a common pattern. So
> that "1 parent N children" maybe does not count as parentish.
> 
>> But as said I'm fine either way as long as the code is reasonably
>> clean and dealing with this sort of platform specific warts happens
>> a lot in drivers/platform/x86 .
> 
> I thought about it again and also prefer the "parent driver" idea as it
> is. That parent identifies the machine and depending on it, causes
> device drivers to be loaded. At the moment LED and watchdog, but with
> nvram, hwmon to come.
> 
> I will stick with "platform" instead of "mfd" because it is really a
> machine having multiple devices. Not a device having multiple functions.

Ok, sticking with the current separate "platform" parent driver design
is fine by me.

Regards,

Hans


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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15 10:48   ` Andy Shevchenko
  2021-03-15 11:19     ` Pavel Machek
  2021-03-18 10:27     ` Enrico Weigelt, metux IT consult
@ 2021-03-26 16:33     ` Henning Schild
  2 siblings, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-26 16:33 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

Am Mon, 15 Mar 2021 12:48:19 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Mon, Mar 15, 2021 at 11:57 AM 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.  
> 
> ...
> 
> > +struct simatic_ipc_led {
> > +       unsigned int value; /* mask for io and offset for mem */  
> 
> > +       char name[32];  
> 
> Hmm... Dunno if LED framework defines its own constraints for the
> length of the name.
> 
> > +       struct led_classdev cdev;
> > +};
> > +
> > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > +       {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" },
> > +       {1 << 7,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" },
> > +       {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
> > +       {1 << 6,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" },
> > +       {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
> > +       {1 << 5,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" },
> >  
> 
> Can you use BIT() macro here? And can it be sorted by the bit order?
> 
> > +       {0, ""},  
> 
> { } is enough (no comma for terminator lines in general, and no need
> to show structure member assignments separately in particular).
> 
> > +};
> > +
> > +/* the actual start will be discovered with pci, 0 is a
> > placeholder */  
> 
> PCI
> 
> > +struct resource simatic_ipc_led_mem_res =
> > +       DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);  
> 
> One line?
> 
> ...
> 
> > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > +       {0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS
> > "-1"},
> > +       {0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS
> > "-1"},
> > +       {0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS
> > "-2"},
> > +       {0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS
> > "-2"},
> > +       {0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS
> > "-3"},
> > +       {0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS
> > "-3"},
> > +       {0, ""},  
> 
> As per above.
> 
> > +};  
> 
> ...
> 
> > +       struct simatic_ipc_led *led =
> > +               container_of(led_cd, struct simatic_ipc_led, cdev);
> >  
> 
> One line?
> 
> ...
> 
> > +       struct simatic_ipc_led *led =
> > +               container_of(led_cd, struct simatic_ipc_led, cdev);
> >  
> 
> One line?
> 
> ...
> 
> > +       struct simatic_ipc_led *led =
> > +               container_of(led_cd, struct simatic_ipc_led, cdev);
> >  
> 
> Ditto.
> 
> 
> Btw, usually for such cases we create an inline helper
> ... to_simatic_ipc_led(...)
> {
>   return container_of(...);
> }
> 
> ...
> 
> > +static int simatic_ipc_leds_probe(struct platform_device *pdev)
> > +{
> > +       struct simatic_ipc_platform *plat;  
> 
> const?
> 
> > +       struct device *dev = &pdev->dev;
> > +       struct simatic_ipc_led *ipcled;
> > +       struct led_classdev *cdev;
> > +       struct resource *res;
> > +       int err, type;
> > +       u32 *p;  
> 
> > +       plat = pdev->dev.platform_data;  
> 
> Can be done directly in the definition block.
> 
> > +       switch (plat->devmode) {
> > +       case SIMATIC_IPC_DEVICE_227D:
> > +       case SIMATIC_IPC_DEVICE_427E:
> > +               res = &simatic_ipc_led_io_res;
> > +               ipcled = simatic_ipc_leds_io;
> > +               /* the 227D is high on while 427E is low on, invert
> > the struct
> > +                * we have
> > +                */
> > +               if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {  
> 
> > +                       while (ipcled->value) {
> > +                               ipcled->value =
> > swab16(ipcled->value);
> > +                               ipcled++;
> > +                       }  
> 
> This seems fishy. If you have a BE CPU it won't work the same way.
> Better:
>  a) to use cpu_to_le16 / be16
>  b) create this as a helper that we may move to the generic header of
> byteorder.
> 
> But looking at the use of it, perhaps you rather need to redefine IO
> accessors, i.e. ioread16()/iowrite16() vs. ioread16be()/iowrite16be().

Got my hands on such a special-case device today. The comment is wrong
it talks about high and low, will fix that.
This one machine almost shares LED logic with some others. We have
those 6 bits spread over 2 consecutive bytes. For this one guy swapping
the two bytes is the shortest way to share the code.

I tried a few things, extra getters/setters, extra array defining bits
the other way around. It all ends up with way more code or conditions
in the getter/setter. So i think i will leave it like it is, clarify
that comment. And that swap16 is fine because we are on x86 only and are
basically swapping (1<<7 with 1<<15) ... where "<<" is already
endianessy. 

Henning

> > +                       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);  
> 
> Magic. As I told you this is an actual offseet in the P2SB's bar for
> GPIO registers.
> I have a question, why we can't provide a GPIO driver which is already
> in the kernel and, with use of the patch series I sent, to convert
> this all magic to GPIO LEDs as it's done for all normal cases?
> 
> > +               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;
> > +       }  
> 
> > +}  
> 


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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-18 11:38       ` Enrico Weigelt, metux IT consult
@ 2021-03-27  9:46         ` Henning Schild
  2021-04-01 16:20           ` Enrico Weigelt, metux IT consult
  0 siblings, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-27  9:46 UTC (permalink / raw)
  To: Enrico Weigelt, metux IT consult
  Cc: Pavel Machek, Andy Shevchenko, 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

Am Thu, 18 Mar 2021 12:38:53 +0100
schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:

> On 15.03.21 12:19, Pavel Machek wrote:
> 
> > But I still don't like the naming. simantic-ipc: prefix is
> > useless. Having 6 status leds is not good, either.  
> 
> Do we have some standard naming policy those kinds of LEDs ?

There is include/dt-bindings/leds/common.h with LED_FUNCTION_*

> In this case, they seem to be assigned to certain specific functions
> (by physical labels on the box), so IMHO the LED names should reflect
> that in some ways.

The choice for "status" was because of

>> /* Miscelleaus functions. Use functions above if you can. */

And those known names do not really come with an explanation of their
meaning. Names like "bluetooth" seem obvious, but "activity" or
"indicator" leave a lot of room for speculation.

The choice in numbers was inspired by labels on the box, which i wanted
to reflect in some way.

Henning

> There're other cases (eg. apu board familiy) that just have several
> front panel leds w/o any dedication, so we can just count them up.
> 
> 
> --mtx
> 


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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-15 11:19     ` Pavel Machek
                         ` (2 preceding siblings ...)
  2021-03-18 11:38       ` Enrico Weigelt, metux IT consult
@ 2021-03-27  9:56       ` Henning Schild
  3 siblings, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-27  9:56 UTC (permalink / raw)
  To: Pavel Machek
  Cc: Andy Shevchenko, 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

Am Mon, 15 Mar 2021 12:19:15 +0100
schrieb Pavel Machek <pavel@ucw.cz>:

> > > +       struct led_classdev cdev;
> > > +};
> > > +
> > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > +       {1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" },
> > > +       {1 << 7,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1"
> > > },
> > > +       {1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
> > > +       {1 << 6,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2"
> > > },
> > > +       {1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
> > > +       {1 << 5,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3"
> > > },  
> > 
> > Can you use BIT() macro here? And can it be sorted by the bit
> > order?  
> 
> There's nothing wrong with << and this order is fine.
> 
> But I still don't like the naming. simantic-ipc: prefix is
> useless. Having 6 status leds is not good, either.

With some of my questions still not being answered i will probably
remove that prefix entirely, not even use "platform:".

And i might stick with 6x "status". Since that allows reflecting the
labels on the machines, while using "above functions if you can"

regards,
Henning

> > > +       struct simatic_ipc_led *led =
> > > +               container_of(led_cd, struct simatic_ipc_led,
> > > cdev);  
> > 
> > One line?  
> 
> 80 columns. It is fine as it is.
> 
> Best regards,
> 
> 								Pavel


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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-18 10:25   ` Enrico Weigelt, metux IT consult
@ 2021-03-27 11:16     ` Henning Schild
  2021-03-27 11:41       ` Henning Schild
  0 siblings, 1 reply; 43+ messages in thread
From: Henning Schild @ 2021-03-27 11:16 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, 18 Mar 2021 11:25:10 +0100
schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:

> On 15.03.21 10:57, Henning Schild wrote:
> 
> Hi,
> 
> > diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> > b/drivers/leds/simple/simatic-ipc-leds.c new file mode 100644
> > index 000000000000..0f7e6320e10d
> > --- /dev/null
> > +++ b/drivers/leds/simple/simatic-ipc-leds.c
> > @@ -0,0 +1,210 @@
> > +// 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[32];
> > +	struct led_classdev cdev;
> > +};
> > +
> > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > +	{1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1" },
> > +	{1 << 7,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1" },
> > +	{1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
> > +	{1 << 6,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2" },
> > +	{1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
> > +	{1 << 5,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3" },
> > +	{0, ""},
> > +};  
> 
> Wouldn't it be better to name them like they're labeled on the device,
> as shown on page #19 of the manual, or perhaps a little bit more
> generic nameing (eg. power, status, error, maint) ?

That was proposed in v1 but modern LED drivers should stick to known
LED_FUNCTION_*
Here the numbers reflect what is on the device labels. We are talking
about roughly 10 boxes at the moment, not all having the same labels
... but all share 1, 2, 3 

At the end of the day the software view might be more important than
the labels. So that people can write generic "echo 1 > brightness"-code
that works across the full range, possibly even on other machines from
different vendors. I guess that is also why Pavel wants people to use
established names.

> > +/* 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;
>  > +  
> 
> hmm, could there *ever* be multiple instances of the driver ?

No, the platform bus makes sure there can only be one.
 
> Wouldn't it be better to put this in the device priv data instead ?

I guess no need for priv because of "highlander ... only 1"

> > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > +	{0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS
> > "-1"},
> > +	{0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS
> > "-1"},
> > +	{0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS
> > "-2"},
> > +	{0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS
> > "-2"},
> > +	{0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS
> > "-3"},
> > +	{0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS
> > "-3"},
> > +	{0, ""},
> > +};
> > +
> > +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);  
> 
> Does this protect global data structures ? If not, I'd rather put it
> into the device priv data instead.

highlander, no need for priv

It protects the setter, which needs to inw + outw and is not atomic

> BTW: doesn't have struct led_classdev already have a lock that
> can be used ? Can multiple calls to led ops (within the same device)
> at the same time happen at all, or does led core already serialize
> that ?

Not sure, i probably did check that in 4.19 at the time of writing.
Pavel did not complain so far. And other drivers have locks in their
setters.

> > +static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
> > +				   enum led_brightness brightness)
> > +{
> > +	struct simatic_ipc_led *led =
> > +		container_of(led_cd, struct simatic_ipc_led, cdev);
> > +	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);  
> 
> Don't we already have an helper for setting or clearing bits in IO
> registers (that already does the read + set/clear + write at once) ?

Did not find one

> Does that really need to be protected by lock ?
> (can happen multiple calls to that func from different threads happen
> at all ?)

i think so, see above

> Is the port really *always* the same, so it really can be a const ?

Yes, but making that ressource const would just cause casting in the
probe function, or a second pointer for the io case. In the mem-case
the static data gets written to.

> <snip>
> 
> > +static int simatic_ipc_leds_probe(struct platform_device *pdev)
> > +{
> > +	struct simatic_ipc_platform *plat;
> > +	struct device *dev = &pdev->dev;
> > +	struct simatic_ipc_led *ipcled;
> > +	struct led_classdev *cdev;
> > +	struct resource *res;
> > +	int err, type;
> > +	u32 *p;
> > +
> > +	plat = pdev->dev.platform_data;  
> 
> Maybe put this into swnode ?
> 
> IIRC, the consensus is not to introduce new platform data structs
> anymore, instead legacy pdata to swnode some day.
> 
> > +	switch (plat->devmode) {
> > +	case SIMATIC_IPC_DEVICE_227D:
> > +	case SIMATIC_IPC_DEVICE_427E:
> > +		res = &simatic_ipc_led_io_res;
> > +		ipcled = simatic_ipc_leds_io;
> > +		/* the 227D is high on while 427E is low on,
> > invert the struct
> > +		 * we have
> > +		 */
> > +		if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
> > +			while (ipcled->value) {
> > +				ipcled->value =
> > swab16(ipcled->value);  
> 
> Uff, better use explicit endian conversion macros (eg. be*_to_cpu())
> for that.
> 
> Also, I wouldn't change those global structs, instead put those data
> into device priv data and make the global stuff const. You could also
> use the same field for both port-io and mmap'ed variants. And adding
> regmap to the equation, could use the same led ops for both. (IMHO,
> the little bit of overhead by regmap shouldn't matter here)

You are the second person to point that out. (the swab) What is done
here is swapping two bytes, no endianess or anything and x86 only.

As for the const and the regmap or priv data. This driver will only
ever load once, if at all. There is really no need for abstractions,
additional memory use or cycles.

So i will ignore the complaints. Feel free to emphasize them if you do
not agree with me doing that.

> > +				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;  
> 
> Where does that device actually sit on ? Some generic card ? Some ASIC
> or FPGA ?

It is called P2SB and there some GPIO chip which i believe has no
generic driver to it yet.

> It seems this driver is instantiated by another one, which already
> knows what device we're actually dealing with (as it sets
> plat->devmode). Why not letting that parent device also tell the io
> resource to this driver ?
> 
> > +	while (ipcled->value) {
> > +		cdev = &ipcled->cdev;
> > +		cdev->brightness_set = simatic_ipc_led_set_io;
> > +		cdev->brightness_get = simatic_ipc_led_get_io;
> > +		if (type == IORESOURCE_MEM) {
> > +			cdev->brightness_set =
> > simatic_ipc_led_set_mem;
> > +			cdev->brightness_get =
> > simatic_ipc_led_get_mem;
> > +		}  
> 
> Why not if/else ?

Ok.

> > +		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 led_driver = {  
> 
> Why not calling it simatic_ipc_led_driver ?

Because it is a name with scope only in that file, but hey i will
change it. Also in wdt

regards,
Henning

> > +	.probe = simatic_ipc_leds_probe,
> > +	.driver = {
> > +		.name = KBUILD_MODNAME,
> > +	},
> > +};
> > +
> > +module_platform_driver(led_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> > +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
> >   
> 
> 
> --mtx
> 


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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-27 11:16     ` Henning Schild
@ 2021-03-27 11:41       ` Henning Schild
  0 siblings, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-27 11:41 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 Sat, 27 Mar 2021 12:16:23 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Thu, 18 Mar 2021 11:25:10 +0100
> schrieb "Enrico Weigelt, metux IT consult" <lkml@metux.net>:
> 
> > On 15.03.21 10:57, Henning Schild wrote:
> > 
> > Hi,
> >   
> > > diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> > > b/drivers/leds/simple/simatic-ipc-leds.c new file mode 100644
> > > index 000000000000..0f7e6320e10d
> > > --- /dev/null
> > > +++ b/drivers/leds/simple/simatic-ipc-leds.c
> > > @@ -0,0 +1,210 @@
> > > +// 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[32];
> > > +	struct led_classdev cdev;
> > > +};
> > > +
> > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > +	{1 << 15, "simatic-ipc:green:" LED_FUNCTION_STATUS "-1"
> > > },
> > > +	{1 << 7,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-1"
> > > },
> > > +	{1 << 14, "simatic-ipc:red:" LED_FUNCTION_STATUS "-2" },
> > > +	{1 << 6,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-2"
> > > },
> > > +	{1 << 13, "simatic-ipc:red:" LED_FUNCTION_STATUS "-3" },
> > > +	{1 << 5,  "simatic-ipc:yellow:" LED_FUNCTION_STATUS "-3"
> > > },
> > > +	{0, ""},
> > > +};    
> > 
> > Wouldn't it be better to name them like they're labeled on the
> > device, as shown on page #19 of the manual, or perhaps a little bit
> > more generic nameing (eg. power, status, error, maint) ?  
> 
> That was proposed in v1 but modern LED drivers should stick to known
> LED_FUNCTION_*
> Here the numbers reflect what is on the device labels. We are talking
> about roughly 10 boxes at the moment, not all having the same labels
> ... but all share 1, 2, 3 
> 
> At the end of the day the software view might be more important than
> the labels. So that people can write generic "echo 1 >
> brightness"-code that works across the full range, possibly even on
> other machines from different vendors. I guess that is also why Pavel
> wants people to use established names.
> 
> > > +/* 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;
> >  > +    
> > 
> > hmm, could there *ever* be multiple instances of the driver ?  
> 
> No, the platform bus makes sure there can only be one.
>  
> > Wouldn't it be better to put this in the device priv data instead ?
> >  
> 
> I guess no need for priv because of "highlander ... only 1"
> 
> > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > +	{0x500 + 0x1A0, "simatic-ipc:red:" LED_FUNCTION_STATUS
> > > "-1"},
> > > +	{0x500 + 0x1A8, "simatic-ipc:green:" LED_FUNCTION_STATUS
> > > "-1"},
> > > +	{0x500 + 0x1C8, "simatic-ipc:red:" LED_FUNCTION_STATUS
> > > "-2"},
> > > +	{0x500 + 0x1D0, "simatic-ipc:green:" LED_FUNCTION_STATUS
> > > "-2"},
> > > +	{0x500 + 0x1E0, "simatic-ipc:red:" LED_FUNCTION_STATUS
> > > "-3"},
> > > +	{0x500 + 0x198, "simatic-ipc:green:" LED_FUNCTION_STATUS
> > > "-3"},
> > > +	{0, ""},
> > > +};
> > > +
> > > +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);    
> > 
> > Does this protect global data structures ? If not, I'd rather put it
> > into the device priv data instead.  
> 
> highlander, no need for priv
> 
> It protects the setter, which needs to inw + outw and is not atomic
> 
> > BTW: doesn't have struct led_classdev already have a lock that
> > can be used ? Can multiple calls to led ops (within the same device)
> > at the same time happen at all, or does led core already serialize
> > that ?  
> 
> Not sure, i probably did check that in 4.19 at the time of writing.
> Pavel did not complain so far. And other drivers have locks in their
> setters.
> 
> > > +static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
> > > +				   enum led_brightness
> > > brightness) +{
> > > +	struct simatic_ipc_led *led =
> > > +		container_of(led_cd, struct simatic_ipc_led,
> > > cdev);
> > > +	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);    
> > 
> > Don't we already have an helper for setting or clearing bits in IO
> > registers (that already does the read + set/clear + write at once)
> > ?  
> 
> Did not find one
> 
> > Does that really need to be protected by lock ?
> > (can happen multiple calls to that func from different threads
> > happen at all ?)  
> 
> i think so, see above
> 
> > Is the port really *always* the same, so it really can be a const ?
> >  
> 
> Yes, but making that ressource const would just cause casting in the
> probe function, or a second pointer for the io case. In the mem-case
> the static data gets written to.
> 
> > <snip>
> >   
> > > +static int simatic_ipc_leds_probe(struct platform_device *pdev)
> > > +{
> > > +	struct simatic_ipc_platform *plat;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct simatic_ipc_led *ipcled;
> > > +	struct led_classdev *cdev;
> > > +	struct resource *res;
> > > +	int err, type;
> > > +	u32 *p;
> > > +
> > > +	plat = pdev->dev.platform_data;    
> > 
> > Maybe put this into swnode ?
> > 
> > IIRC, the consensus is not to introduce new platform data structs
> > anymore, instead legacy pdata to swnode some day.
> >   
> > > +	switch (plat->devmode) {
> > > +	case SIMATIC_IPC_DEVICE_227D:
> > > +	case SIMATIC_IPC_DEVICE_427E:
> > > +		res = &simatic_ipc_led_io_res;
> > > +		ipcled = simatic_ipc_leds_io;
> > > +		/* the 227D is high on while 427E is low on,
> > > invert the struct
> > > +		 * we have
> > > +		 */
> > > +		if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
> > > +			while (ipcled->value) {
> > > +				ipcled->value =
> > > swab16(ipcled->value);    
> > 
> > Uff, better use explicit endian conversion macros (eg. be*_to_cpu())
> > for that.
> > 
> > Also, I wouldn't change those global structs, instead put those data
> > into device priv data and make the global stuff const. You could
> > also use the same field for both port-io and mmap'ed variants. And
> > adding regmap to the equation, could use the same led ops for both.
> > (IMHO, the little bit of overhead by regmap shouldn't matter here)  
> 
> You are the second person to point that out. (the swab) What is done
> here is swapping two bytes, no endianess or anything and x86 only.
> 
> As for the const and the regmap or priv data. This driver will only
> ever load once, if at all. There is really no need for abstractions,
> additional memory use or cycles.
> 
> So i will ignore the complaints. Feel free to emphasize them if you do
> not agree with me doing that.

One more point is that the memory based PC has different colors. So the
led names can not be shared between io and mem. This is an
inconsistency between those machines.

mem offers individual control of red and green, where yellow becomes
visible when turning both on
io on the other hand exposes yellow and (green|red)

The driver currently exposes what hardware offers and nothing more. I
still have the idea to potentially add yellow for mem as a helper to
get the user-level view consistent. But not sure if that idea is any
good, even if it would probably be a patch on top.

Henning

> > > +				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;    
> > 
> > Where does that device actually sit on ? Some generic card ? Some
> > ASIC or FPGA ?  
> 
> It is called P2SB and there some GPIO chip which i believe has no
> generic driver to it yet.
> 
> > It seems this driver is instantiated by another one, which already
> > knows what device we're actually dealing with (as it sets
> > plat->devmode). Why not letting that parent device also tell the io
> > resource to this driver ?
> >   
> > > +	while (ipcled->value) {
> > > +		cdev = &ipcled->cdev;
> > > +		cdev->brightness_set = simatic_ipc_led_set_io;
> > > +		cdev->brightness_get = simatic_ipc_led_get_io;
> > > +		if (type == IORESOURCE_MEM) {
> > > +			cdev->brightness_set =
> > > simatic_ipc_led_set_mem;
> > > +			cdev->brightness_get =
> > > simatic_ipc_led_get_mem;
> > > +		}    
> > 
> > Why not if/else ?  
> 
> Ok.
> 
> > > +		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 led_driver = {    
> > 
> > Why not calling it simatic_ipc_led_driver ?  
> 
> Because it is a name with scope only in that file, but hey i will
> change it. Also in wdt
> 
> regards,
> Henning
> 
> > > +	.probe = simatic_ipc_leds_probe,
> > > +	.driver = {
> > > +		.name = KBUILD_MODNAME,
> > > +	},
> > > +};
> > > +
> > > +module_platform_driver(led_driver);
> > > +
> > > +MODULE_LICENSE("GPL v2");
> > > +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> > > +MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
> > >     
> > 
> > 
> > --mtx
> >   
> 


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

* Re: [PATCH v2 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-15 15:10   ` Guenter Roeck
@ 2021-03-29 16:28     ` Henning Schild
  0 siblings, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-03-29 16:28 UTC (permalink / raw)
  To: Guenter Roeck
  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,
	Andy Shevchenko

Am Mon, 15 Mar 2021 08:10:25 -0700
schrieb Guenter Roeck <linux@roeck-us.net>:

> On 3/15/21 2:57 AM, Henning Schild wrote:
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.
> > 
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>  
> 
> Reviewed-by: Guenter Roeck <linux@roeck-us.net>

I assume that means i should insert this into v3, will do. The diff
between v2 and v3 is only going to be a name change

-module_platform_driver(wdt_driver);
+module_platform_driver(simatic_ipc_wdt_driver);

Henning


> > ---
> >  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..f0f948968db3
> > --- /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 wdt_driver = {
> > +	.probe = simatic_ipc_wdt_probe,
> > +	.driver = {
> > +		.name = KBUILD_MODNAME,
> > +	},
> > +};
> > +
> > +module_platform_driver(wdt_driver);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
> >   
> 


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

* Re: [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-27  9:46         ` Henning Schild
@ 2021-04-01 16:20           ` Enrico Weigelt, metux IT consult
  0 siblings, 0 replies; 43+ messages in thread
From: Enrico Weigelt, metux IT consult @ 2021-04-01 16:20 UTC (permalink / raw)
  To: Henning Schild
  Cc: Pavel Machek, Andy Shevchenko, 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

On 27.03.21 10:46, Henning Schild wrote:

>> In this case, they seem to be assigned to certain specific functions
>> (by physical labels on the box), so IMHO the LED names should reflect
>> that in some ways.
> 
> The choice for "status" was because of
> 
>>> /* Miscelleaus functions. Use functions above if you can. */
> 
> And those known names do not really come with an explanation of their
> meaning. Names like "bluetooth" seem obvious, but "activity" or
> "indicator" leave a lot of room for speculation.

Maybe we should revise these and add more functions ?

Can you find out some more details, what these LEDs really had been
intented for ?

--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] 43+ messages in thread

* Re: [PATCH v2 0/4] add device drivers for Siemens Industrial PCs
  2021-03-15 10:55 ` [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
  2021-03-15 16:08   ` Henning Schild
@ 2021-08-02  9:21   ` Henning Schild
  1 sibling, 0 replies; 43+ messages in thread
From: Henning Schild @ 2021-08-02  9:21 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, Farooq, Muhammad Hamza

Am Mon, 15 Mar 2021 12:55:13 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Mon, Mar 15, 2021 at 12:12 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > 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.  
> 
> Thanks for an update!
> 
> I did review more thoughtfully the series and realized that you can
> avoid that P2SB thingy and may the approach be much cleaner if you
> register the real GPIO driver and convert your LEDs to be a GPIO LEDs.
> Then you won't need any custom code for it (except some board file, or
> what would be better to file _DSD in your ACPI tables.

For the next generation of these machines i managed to involve the BIOS
guys. Goal would be to describe as much as possible in a generic and
standard way in ACPI, to reduce cost on driver dev and maint in the
long run. Hopefully across OSs.

The first thing we wanted to look into is LEDs. The way they can be
described for leds-gpio does not seem to be standard but at least seems
generic. At the same time we contemplated whether to model the LEDs
using the multicolor class.

One thing that seems to speak against using multicolor seems to be
missing ACPI "support", while regular LEDs can be described in ACPI, it
does not seem like multicolor can. Or did we miss something?

regards,
Henning

> --
> With Best Regards,
> Andy Shevchenko


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

end of thread, other threads:[~2021-08-02  9:41 UTC | newest]

Thread overview: 43+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-15  9:57 [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Henning Schild
2021-03-15  9:57 ` [PATCH v2 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
2021-03-15 10:31   ` Andy Shevchenko
2021-03-15 16:30     ` Henning Schild
2021-03-17 19:13     ` Henning Schild
2021-03-17 20:03       ` Hans de Goede
2021-03-18 11:30         ` Enrico Weigelt, metux IT consult
2021-03-18 11:45           ` Hans de Goede
2021-03-26  9:55             ` Henning Schild
2021-03-26 12:21               ` Hans de Goede
2021-03-15  9:57 ` [PATCH v2 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
2021-03-15 10:48   ` Andy Shevchenko
2021-03-15 11:19     ` Pavel Machek
2021-03-15 11:26       ` Andy Shevchenko
2021-03-15 12:40       ` Henning Schild
2021-03-18 11:38       ` Enrico Weigelt, metux IT consult
2021-03-27  9:46         ` Henning Schild
2021-04-01 16:20           ` Enrico Weigelt, metux IT consult
2021-03-27  9:56       ` Henning Schild
2021-03-18 10:27     ` Enrico Weigelt, metux IT consult
2021-03-18 12:40       ` Alexander Dahl
2021-03-23 17:45         ` Henning Schild
2021-03-26 16:33     ` Henning Schild
2021-03-18 10:25   ` Enrico Weigelt, metux IT consult
2021-03-27 11:16     ` Henning Schild
2021-03-27 11:41       ` Henning Schild
2021-03-15  9:57 ` [PATCH v2 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
2021-03-15 15:10   ` Guenter Roeck
2021-03-29 16:28     ` Henning Schild
2021-03-15  9:57 ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
2021-03-15 10:14   ` Henning Schild
2021-03-15 10:19     ` Hans de Goede
2021-03-15 12:09       ` Henning Schild
2021-03-15 14:58       ` [PATCH] platform/x86: pmc_atom: use callback for all dmi quirk entries Henning Schild
2021-03-15 16:31         ` Hans de Goede
2021-03-15 17:00           ` Henning Schild
2021-03-15 18:01             ` Hans de Goede
2021-03-16  5:47               ` Henning Schild
2021-03-16  9:43                 ` Hans de Goede
2021-03-15 13:25   ` [PATCH v2 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs kernel test robot
2021-03-15 10:55 ` [PATCH v2 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
2021-03-15 16:08   ` Henning Schild
2021-08-02  9:21   ` Henning Schild

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