Linux-Watchdog Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] add device drivers for Siemens Industrial PCs
@ 2021-03-02 16:33 Henning Schild
  2021-03-02 16:33 ` [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
                   ` (4 more replies)
  0 siblings, 5 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-02 16:33 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

From: Henning Schild <henning.schild@siemens.com>

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.

But the idea here is to share early, and hopefully not fail early.

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                          |  11 +
 drivers/leds/Makefile                         |   1 +
 drivers/leds/simatic-ipc-leds.c               | 224 +++++++++++++
 drivers/platform/x86/Kconfig                  |   9 +
 drivers/platform/x86/Makefile                 |   3 +
 drivers/platform/x86/pmc_atom.c               |  39 +--
 drivers/platform/x86/simatic-ipc.c            | 166 ++++++++++
 drivers/watchdog/Kconfig                      |  11 +
 drivers/watchdog/Makefile                     |   1 +
 drivers/watchdog/simatic-ipc-wdt.c            | 305 ++++++++++++++++++
 .../platform_data/x86/simatic-ipc-base.h      |  33 ++
 include/linux/platform_data/x86/simatic-ipc.h |  68 ++++
 12 files changed, 853 insertions(+), 18 deletions(-)
 create mode 100644 drivers/leds/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] 46+ messages in thread

* [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-02 16:33 [PATCH 0/4] add device drivers for Siemens Industrial PCs Henning Schild
@ 2021-03-02 16:33 ` Henning Schild
  2021-03-04 10:11   ` Andy Shevchenko
  2021-03-04 14:03   ` Hans de Goede
  2021-03-02 16:33 ` [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-02 16:33 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

From: Henning Schild <henning.schild@siemens.com>

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

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

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

Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/platform/x86/Kconfig                  |   9 +
 drivers/platform/x86/Makefile                 |   3 +
 drivers/platform/x86/simatic-ipc.c            | 166 ++++++++++++++++++
 .../platform_data/x86/simatic-ipc-base.h      |  33 ++++
 include/linux/platform_data/x86/simatic-ipc.h |  68 +++++++
 5 files changed, 279 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..bb9e282d89cf 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1284,6 +1284,15 @@ 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 watchdogs
+
 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..6adcdac1a0d7
--- /dev/null
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -0,0 +1,166 @@
+// 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>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/x86/simatic-ipc.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)
+{
+	int i;
+	u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
+	u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
+
+	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(KBUILD_MODNAME ": 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(KBUILD_MODNAME ": device=%s created\n",
+			 ipc_wdt_platform_device->name);
+	}
+
+	if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
+	    wdtmode == SIMATIC_IPC_DEVICE_NONE) {
+		pr_warn(KBUILD_MODNAME
+			": unsupported IPC detected, station id=%08x\n",
+			station_id);
+		return -EINVAL;
+	} else {
+		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)
+{
+	u32 bar0 = 0;
+#ifdef CONFIG_PCI
+	struct pci_bus *bus;
+	/*
+	 * 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();
+#endif /* CONFIG_PCI */
+	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(KBUILD_MODNAME ": 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");
+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..d8e59eb8fc96
--- /dev/null
+++ b/include/linux/platform_data/x86/simatic-ipc-base.h
@@ -0,0 +1,33 @@
+/* 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>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H
+#define __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H
+
+#include <linux/pci.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..90bb0d7cf09a
--- /dev/null
+++ b/include/linux/platform_data/x86/simatic-ipc.h
@@ -0,0 +1,68 @@
+/* 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>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#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 simatic_ipc_station_ids {
+	SIMATIC_IPC_INVALID_STATION_ID = 0,
+	SIMATIC_IPC_IPC227D = 0x00000501,
+	SIMATIC_IPC_IPC427D = 0x00000701,
+	SIMATIC_IPC_IPC227E = 0x00000901,
+	SIMATIC_IPC_IPC277E = 0x00000902,
+	SIMATIC_IPC_IPC427E = 0x00000A01,
+	SIMATIC_IPC_IPC477E = 0x00000A02,
+	SIMATIC_IPC_IPC127E = 0x00000D01,
+};
+
+static inline u32 simatic_ipc_get_station_id(u8 *data)
+{
+	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;
+
+	/* 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 && 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 + sizeof(struct dmi_header));
+}
+
+#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */
-- 
2.26.2


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

* [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-02 16:33 [PATCH 0/4] add device drivers for Siemens Industrial PCs Henning Schild
  2021-03-02 16:33 ` [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
@ 2021-03-02 16:33 ` Henning Schild
  2021-03-02 20:54   ` Pavel Machek
  2021-03-02 16:33 ` [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-02 16:33 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

From: Henning Schild <henning.schild@siemens.com>

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

Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/leds/Kconfig            |  11 ++
 drivers/leds/Makefile           |   1 +
 drivers/leds/simatic-ipc-leds.c | 224 ++++++++++++++++++++++++++++++++
 3 files changed, 236 insertions(+)
 create mode 100644 drivers/leds/simatic-ipc-leds.c

diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b6742b4231bf..3ee6fc613a0a 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -928,6 +928,17 @@ config LEDS_ACER_A500
 	  This option enables support for the Power Button LED of
 	  Acer Iconia Tab A500.
 
+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.
+
 comment "Flash and Torch LED drivers"
 source "drivers/leds/flash/Kconfig"
 
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 2a698df9da57..c15e1e3c5958 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA)		+= leds-turris-omnia.o
 obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
 obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
 obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o
 
 # LED SPI Drivers
 obj-$(CONFIG_LEDS_CR0014114)		+= leds-cr0014114.o
diff --git a/drivers/leds/simatic-ipc-leds.c b/drivers/leds/simatic-ipc-leds.c
new file mode 100644
index 000000000000..760aef1d4ae1
--- /dev/null
+++ b/drivers/leds/simatic-ipc-leds.c
@@ -0,0 +1,224 @@
+// 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>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/ioport.h>
+#include <linux/sizes.h>
+#include <linux/leds.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/platform_data/x86/simatic-ipc-base.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:run-stop"},
+	{1 << 7,  "simatic-ipc:yellow:run-stop"},
+	{1 << 14, "simatic-ipc:red:error"},
+	{1 << 6,  "simatic-ipc:yellow:error"},
+	{1 << 13, "simatic-ipc:red:maint"},
+	{1 << 5,  "simatic-ipc:yellow:maint"},
+	{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:run-stop"},
+	{0x500 + 0x1A8, "simatic-ipc:green:run-stop"},
+	{0x500 + 0x1C8, "simatic-ipc:red:error"},
+	{0x500 + 0x1D0, "simatic-ipc:green:error"},
+	{0x500 + 0x1E0, "simatic-ipc:red:maint"},
+	{0x500 + 0x198, "simatic-ipc:green:maint"},
+	{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 device *dev = &pdev->dev;
+	struct led_classdev *cdev;
+	struct resource *res;
+	int err, type;
+	struct simatic_ipc_led *ipcled;
+	struct simatic_ipc_platform *plat;
+	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 (!simatic_ipc_led_memory)
+			return -ENOMEM;
+
+		/* 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,
+	},
+};
+
+static int __init simatic_ipc_led_init_module(void)
+{
+	return platform_driver_register(&led_driver);
+}
+
+static void simatic_ipc_led_exit_module(void)
+{
+	platform_driver_unregister(&led_driver);
+}
+
+module_init(simatic_ipc_led_init_module);
+module_exit(simatic_ipc_led_exit_module);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("Henning Schild <henning.schild@siemens.com>");
-- 
2.26.2


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

* [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-02 16:33 [PATCH 0/4] add device drivers for Siemens Industrial PCs Henning Schild
  2021-03-02 16:33 ` [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
  2021-03-02 16:33 ` [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
@ 2021-03-02 16:33 ` Henning Schild
  2021-03-02 18:38   ` Guenter Roeck
       [not found]   ` <CAHp75VdUXWDg-2o8fmeo7EoMtRLfCnFOw5ptwjXTv9fKMmHv2A@mail.gmail.com>
  2021-03-02 16:33 ` [PATCH 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
  2021-03-04 10:19 ` [PATCH 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
  4 siblings, 2 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-02 16:33 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

From: Henning Schild <henning.schild@siemens.com>

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

Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
Signed-off-by: Henning Schild <henning.schild@siemens.com>
---
 drivers/watchdog/Kconfig           |  11 ++
 drivers/watchdog/Makefile          |   1 +
 drivers/watchdog/simatic-ipc-wdt.c | 305 +++++++++++++++++++++++++++++
 3 files changed, 317 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..b5c8b7ceb404
--- /dev/null
+++ b/drivers/watchdog/simatic-ipc-wdt.c
@@ -0,0 +1,305 @@
+// 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>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/errno.h>
+#include <linux/watchdog.h>
+#include <linux/ioport.h>
+#include <linux/sizes.h>
+#include <linux/io.h>
+#include <linux/platform_data/x86/simatic-ipc-base.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 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 DEFINE_SPINLOCK(io_lock);	/* the lock for io operations */
+static struct watchdog_device wdd;
+
+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 get_timeout_idx(u32 timeout)
+{
+	int i;
+
+	i = ARRAY_SIZE(wd_timeout_table) - 1;
+	for (; i >= 0; i--) {
+		if (timeout >= wd_timeout_table[i])
+			break;
+	}
+
+	return i;
+}
+
+static int wd_start(struct watchdog_device *wdd)
+{
+	u8 regval;
+
+	spin_lock(&io_lock);
+
+	regval = inb(WD_ENABLE_IOADR);
+	regval |= WD_ENABLED;
+	outb(regval, WD_ENABLE_IOADR);
+
+	spin_unlock(&io_lock);
+
+	return 0;
+}
+
+static int wd_stop(struct watchdog_device *wdd)
+{
+	u8 regval;
+
+	spin_lock(&io_lock);
+
+	regval = inb(WD_ENABLE_IOADR);
+	regval &= ~WD_ENABLED;
+	outb(regval, WD_ENABLE_IOADR);
+
+	spin_unlock(&io_lock);
+
+	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)
+{
+	u8 regval;
+	int timeout_idx = get_timeout_idx(t);
+
+	spin_lock(&io_lock);
+
+	regval = inb(WD_ENABLE_IOADR) & 0xc7;
+	regval |= timeout_idx << 3;
+	outb(regval, WD_ENABLE_IOADR);
+
+	spin_unlock(&io_lock);
+	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_set_safe_en_n(u32 wdtmode, bool safe_en_n)
+{
+	u16 resetbit;
+
+	if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
+		/* enable SAFE_EN_N on GP_STATUS_REG_227E */
+		resetbit = inw(GP_STATUS_REG_227E);
+		if (safe_en_n)
+			resetbit &= ~SAFE_EN_N_227E;
+		else
+			resetbit |= SAFE_EN_N_227E;
+
+		outw(resetbit, GP_STATUS_REG_227E);
+	} else {
+		/* enable SAFE_EN_N on PCH D1600 */
+		resetbit = ioread16(wd_reset_base_addr);
+
+		if (safe_en_n)
+			resetbit &= ~SAFE_EN_N_427E;
+		else
+			resetbit |= SAFE_EN_N_427E;
+
+		iowrite16(resetbit, wd_reset_base_addr);
+	}
+}
+
+static int wd_setup(u32 wdtmode, bool safe_en_n)
+{
+	u8 regval;
+	int timeout_idx = 0;
+	bool alarm_active;
+
+	timeout_idx = get_timeout_idx(TIMEOUT_DEF);
+
+	wd_set_safe_en_n(wdtmode, safe_en_n);
+
+	/* read wd register to determine alarm state */
+	regval = inb(WD_ENABLE_IOADR);
+	if (regval & 0x80) {
+		pr_warn("Watchdog alarm active.\n");
+		regval = 0x82;	/* use only macro mode, reset alarm bit */
+		alarm_active = true;
+	} else {
+		regval = 0x02;	/* use only macro mode */
+		alarm_active = false;
+	}
+
+	regval |= timeout_idx << 3;
+	if (nowayout)
+		regval |= WD_ENABLED;
+
+	outb(regval, WD_ENABLE_IOADR);
+
+	return alarm_active;
+}
+
+static int simatic_ipc_wdt_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	int rc = 0;
+	struct simatic_ipc_platform *plat = pdev->dev.platform_data;
+	struct resource *res;
+
+	pr_debug(KBUILD_MODNAME ":%s(#%d) WDT mode: %d\n",
+		 __func__, __LINE__, plat->devmode);
+
+	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.info = &wdt_ident;
+		wdd.ops = &wdt_ops;
+		wdd.min_timeout = TIMEOUT_MIN;
+		wdd.max_timeout = TIMEOUT_MAX;
+		wdd.parent = NULL;
+		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 -ENOMEM;
+	}
+
+	wdd.bootstatus = wd_setup(plat->devmode, true);
+	if (wdd.bootstatus)
+		pr_warn(KBUILD_MODNAME ": last reboot caused by watchdog reset\n");
+
+	watchdog_set_nowayout(&wdd, nowayout);
+	watchdog_stop_on_reboot(&wdd);
+
+	rc = devm_watchdog_register_device(dev, &wdd);
+
+	if (rc == 0)
+		pr_debug("initialized. nowayout=%d\n",
+			 nowayout);
+
+	return rc;
+}
+
+static int simatic_ipc_wdt_remove(struct platform_device *pdev)
+{
+	struct simatic_ipc_platform *plat = pdev->dev.platform_data;
+
+	wd_setup(plat->devmode, false);
+	return 0;
+}
+
+static struct platform_driver wdt_driver = {
+	.probe = simatic_ipc_wdt_probe,
+	.remove = simatic_ipc_wdt_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+	},
+};
+
+static int __init simatic_ipc_wdt_init(void)
+{
+	return platform_driver_register(&wdt_driver);
+}
+
+static void __exit simatic_ipc_wdt_exit(void)
+{
+	platform_driver_unregister(&wdt_driver);
+}
+
+module_init(simatic_ipc_wdt_init);
+module_exit(simatic_ipc_wdt_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
-- 
2.26.2


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

* [PATCH 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs
  2021-03-02 16:33 [PATCH 0/4] add device drivers for Siemens Industrial PCs Henning Schild
                   ` (2 preceding siblings ...)
  2021-03-02 16:33 ` [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
@ 2021-03-02 16:33 ` Henning Schild
  2021-03-04  9:50   ` Andy Shevchenko
  2021-03-04 10:19 ` [PATCH 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
  4 siblings, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-02 16:33 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, Michael Haener

From: 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 | 39 ++++++++++++++++++---------------
 1 file changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/platform/x86/pmc_atom.c b/drivers/platform/x86/pmc_atom.c
index ca684ed760d1..03344f5502ad 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>
@@ -424,30 +425,31 @@ static const struct dmi_system_id critclk_systems[] = {
 		},
 	},
 	{
-		.ident = "SIMATIC IPC227E",
+		.ident = "SIEMENS AG",
 		.matches = {
 			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "6ES7647-8B"),
-		},
-	},
-	{
-		.ident = "SIMATIC IPC277E",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "6AV7882-0"),
-		},
-	},
-	{
-		.ident = "CONNECT X300",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
-			DMI_MATCH(DMI_PRODUCT_VERSION, "A5E45074588"),
 		},
 	},
 
 	{ /*sentinel*/ }
 };
 
+static int pmc_clk_is_critical(const struct dmi_system_id *d)
+{
+	int ret = true;
+	u32 station_id;
+
+	if (!strcmp(d->ident, "SIEMENS AG")) {
+		if (dmi_walk(simatic_ipc_find_dmi_entry_helper, &station_id))
+			ret = false;
+		else
+			ret = (station_id == SIMATIC_IPC_IPC227E ||
+			       station_id == SIMATIC_IPC_IPC277E);
+	}
+
+	return ret;
+}
+
 static int pmc_setup_clks(struct pci_dev *pdev, void __iomem *pmc_regmap,
 			  const struct pmc_data *pmc_data)
 {
@@ -462,8 +464,9 @@ 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);
+		clk_data->critical = pmc_clk_is_critical(d);
+		if (clk_data->critical)
+			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	[flat|nested] 46+ messages in thread

* Re: [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-02 16:33 ` [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
@ 2021-03-02 18:38   ` Guenter Roeck
  2021-03-03 14:21     ` Henning Schild
       [not found]   ` <CAHp75VdUXWDg-2o8fmeo7EoMtRLfCnFOw5ptwjXTv9fKMmHv2A@mail.gmail.com>
  1 sibling, 1 reply; 46+ messages in thread
From: Guenter Roeck @ 2021-03-02 18:38 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

On 3/2/21 8:33 AM, Henning Schild wrote:
> From: Henning Schild <henning.schild@siemens.com>
> 
> This driver adds initial support for several devices from Siemens. It is
> based on a platform driver introduced in an earlier commit.
> 
> Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> ---
>  drivers/watchdog/Kconfig           |  11 ++
>  drivers/watchdog/Makefile          |   1 +
>  drivers/watchdog/simatic-ipc-wdt.c | 305 +++++++++++++++++++++++++++++
>  3 files changed, 317 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..b5c8b7ceb404
> --- /dev/null
> +++ b/drivers/watchdog/simatic-ipc-wdt.c
> @@ -0,0 +1,305 @@
> +// 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>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

Covered by SPDX-License-Identifier

> + */
> +
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/errno.h>
> +#include <linux/watchdog.h>
> +#include <linux/ioport.h>
> +#include <linux/sizes.h>> +#include <linux/io.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>

Alphabetic order please

> +
> +#define WD_ENABLE_IOADR		0x62
> +#define WD_TRIGGER_IOADR	0x66
> +#define GPIO_COMMUNITY0_PORT_ID 0xaf
> +#define PAD_CFG_DW0_GPP_A_23	0x4b8

Please increase indentation and spare another tab

> +#define SAFE_EN_N_427E		0x01
> +#define SAFE_EN_N_227E		0x04
> +#define WD_ENABLED		0x01
> +
> +#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 DEFINE_SPINLOCK(io_lock);	/* the lock for io operations */
> +static struct watchdog_device wdd;
> +

Having two variables named 'wdd' is confusing. Please chose another name.

> +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 get_timeout_idx(u32 timeout)
> +{
> +	int i;
> +
> +	i = ARRAY_SIZE(wd_timeout_table) - 1;
> +	for (; i >= 0; i--) {
> +		if (timeout >= wd_timeout_table[i])
> +			break;
> +	}
> +
> +	return i;
> +}

Please add a comment explaining why you don't use find_closest().

> +
> +static int wd_start(struct watchdog_device *wdd)
> +{
> +	u8 regval;
> +
> +	spin_lock(&io_lock);
> +
The watchdog subsystem already provides locking
since the watchdog device can only be opened once.

Why is the additional lock needed ?

> +	regval = inb(WD_ENABLE_IOADR);
> +	regval |= WD_ENABLED;
> +	outb(regval, WD_ENABLE_IOADR);
> +
> +	spin_unlock(&io_lock);
> +
> +	return 0;
> +}
> +
> +static int wd_stop(struct watchdog_device *wdd)
> +{
> +	u8 regval;
> +
> +	spin_lock(&io_lock);
> +
> +	regval = inb(WD_ENABLE_IOADR);
> +	regval &= ~WD_ENABLED;
> +	outb(regval, WD_ENABLE_IOADR);
> +
> +	spin_unlock(&io_lock);
> +
> +	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)
> +{
> +	u8 regval;
> +	int timeout_idx = get_timeout_idx(t);
> +
> +	spin_lock(&io_lock);
> +
> +	regval = inb(WD_ENABLE_IOADR) & 0xc7;
> +	regval |= timeout_idx << 3;
> +	outb(regval, WD_ENABLE_IOADR);
> +
> +	spin_unlock(&io_lock);
> +	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_set_safe_en_n(u32 wdtmode, bool safe_en_n)
> +{
> +	u16 resetbit;
> +
> +	if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
> +		/* enable SAFE_EN_N on GP_STATUS_REG_227E */
> +		resetbit = inw(GP_STATUS_REG_227E);
> +		if (safe_en_n)
> +			resetbit &= ~SAFE_EN_N_227E;
> +		else
> +			resetbit |= SAFE_EN_N_227E;
> +
> +		outw(resetbit, GP_STATUS_REG_227E);
> +	} else {
> +		/* enable SAFE_EN_N on PCH D1600 */
> +		resetbit = ioread16(wd_reset_base_addr);
> +
> +		if (safe_en_n)
> +			resetbit &= ~SAFE_EN_N_427E;
> +		else
> +			resetbit |= SAFE_EN_N_427E;
> +
> +		iowrite16(resetbit, wd_reset_base_addr);
> +	}
> +}
> +
> +static int wd_setup(u32 wdtmode, bool safe_en_n)
> +{
> +	u8 regval;
> +	int timeout_idx = 0;

Unnecessary initialization

> +	bool alarm_active;
> +
> +	timeout_idx = get_timeout_idx(TIMEOUT_DEF);
> +
> +	wd_set_safe_en_n(wdtmode, safe_en_n);
> +
> +	/* read wd register to determine alarm state */
> +	regval = inb(WD_ENABLE_IOADR);
> +	if (regval & 0x80) {
> +		pr_warn("Watchdog alarm active.\n");

Why does that warrant a warning, and what does it mean ? The context suggests
that it means the previous reset was caused by the watchdog, but that is not
what the message says.

> +		regval = 0x82;	/* use only macro mode, reset alarm bit */
> +		alarm_active = true;
> +	} else {
> +		regval = 0x02;	/* use only macro mode */
> +		alarm_active = false;
> +	}

Would it hurt to just always write 0x82 ?
	alarm_active = regval & 0x80;
	regval = 0x82 | timeout_idx << 3;

would be much simpler. Or, if you prefer,
	alarm_active = !!(regval & 0x80);
	regval = 0x82 | timeout_idx << 3;

Actually, regval isn't even needed in that case.
	alarm_active = !!(regval & 0x80);
	outb(0x82 | timeout_idx << 3, WD_ENABLE_IOADR);


Either case, please use defines for the bits. WD_ENABLED is already
defined, thus the other bits should be set using defines as well.

> +
> +	regval |= timeout_idx << 3;
> +	if (nowayout)
> +		regval |= WD_ENABLED;

This is not the purpose of nowayout. nowayout prevents stopping
the watchdog after it has been started. It is not expected to start
the watchdog on boot.

> +
> +	outb(regval, WD_ENABLE_IOADR);
> +
> +	return alarm_active;
> +}
> +
> +static int simatic_ipc_wdt_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	int rc = 0;
> +	struct simatic_ipc_platform *plat = pdev->dev.platform_data;
> +	struct resource *res;
> +

Is it guaranteed that the device will always be instantiated only once ?
If so, how it it guaranteed ?

Because if there are ever multiple instances the various static variables
will cause major trouble (which is why it is always better to not use static
variables).

> +	pr_debug(KBUILD_MODNAME ":%s(#%d) WDT mode: %d\n",
> +		 __func__, __LINE__, plat->devmode);
> +

This is a platform device. Please use dev_ messages (dev_warn, dev_dbg etc)
throughout.

> +	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.info = &wdt_ident;
> +		wdd.ops = &wdt_ops;
> +		wdd.min_timeout = TIMEOUT_MIN;
> +		wdd.max_timeout = TIMEOUT_MAX;

Why not use static initialization ?

> +		wdd.parent = NULL;

parent should be the platform device.

> +		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 this is what prevents multiple registrations, it is too late: wdd
is already overwritten.

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

			return PTR_ERR(wd_reset_base_addr);

> +	}
> +
> +	wdd.bootstatus = wd_setup(plat->devmode, true);

bootstatus does not report a boolean. This translates to WDIOF_OVERHEAT
which is almost certainly wrong.

> +	if (wdd.bootstatus)
> +		pr_warn(KBUILD_MODNAME ": last reboot caused by watchdog reset\n");

Why two messages ?

> +
> +	watchdog_set_nowayout(&wdd, nowayout);
> +	watchdog_stop_on_reboot(&wdd);
> +
> +	rc = devm_watchdog_register_device(dev, &wdd);
> +
Extra empty line not needed

> +	if (rc == 0)
> +		pr_debug("initialized. nowayout=%d\n",
> +			 nowayout);

What is the value of this message (especially since there is no message
if there is an error) ?

> +
> +	return rc;
> +}
> +
> +static int simatic_ipc_wdt_remove(struct platform_device *pdev)
> +{
> +	struct simatic_ipc_platform *plat = pdev->dev.platform_data;
> +
> +	wd_setup(plat->devmode, false);

This warrants an explanation. What is the point of updating the timeout
here ? And what does SAFE_EN actually do ?

The watchdog is stopped on reboot, but this function won't
be called in that case, making this call even more questionable.
Please document what it does and why it is needed here (but not
when rebooting).

> +	return 0;
> +}
> +
> +static struct platform_driver wdt_driver = {
> +	.probe = simatic_ipc_wdt_probe,
> +	.remove = simatic_ipc_wdt_remove,
> +	.driver = {
> +		.name = KBUILD_MODNAME,
> +	},
> +};
> +
> +static int __init simatic_ipc_wdt_init(void)
> +{
> +	return platform_driver_register(&wdt_driver);
> +}
> +
> +static void __exit simatic_ipc_wdt_exit(void)
> +{
> +	platform_driver_unregister(&wdt_driver);
> +}
> +
> +module_init(simatic_ipc_wdt_init);
> +module_exit(simatic_ipc_wdt_exit);

Why not module_platform_driver() ?

> +
> +MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
> 


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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-02 16:33 ` [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
@ 2021-03-02 20:54   ` Pavel Machek
  2021-03-03 13:11     ` Henning Schild
  2021-03-03 17:37     ` Henning Schild
  0 siblings, 2 replies; 46+ messages in thread
From: Pavel Machek @ 2021-03-02 20:54 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, Hans de Goede


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

Hi!

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

Ok.

> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 2a698df9da57..c15e1e3c5958 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA)		+= leds-turris-omnia.o
>  obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
>  obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
>  obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+= simatic-ipc-leds.o

Let's put this into drivers/leds/simple. You'll have to create it.

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */

Remove?

> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> +	{1 << 15, "simatic-ipc:green:run-stop"},
> +	{1 << 7,  "simatic-ipc:yellow:run-stop"},
> +	{1 << 14, "simatic-ipc:red:error"},
> +	{1 << 6,  "simatic-ipc:yellow:error"},
> +	{1 << 13, "simatic-ipc:red:maint"},
> +	{1 << 5,  "simatic-ipc:yellow:maint"},
> +	{0, ""},
> +};

Please use names consistent with other systems, this is user
visible. If you have two-color power led, it should be
:green:power... See include/dt-bindings/leds/common.h .

Please avoid // comments in the code.

> +module_init(simatic_ipc_led_init_module);
> +module_exit(simatic_ipc_led_exit_module);

No need for such verbosity for functions that are static.

> +MODULE_LICENSE("GPL");

GPL v2?

Best regards,
								Pavel

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

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

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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-02 20:54   ` Pavel Machek
@ 2021-03-03 13:11     ` Henning Schild
  2021-03-03 19:31       ` Pavel Machek
  2021-03-03 17:37     ` Henning Schild
  1 sibling, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-03 13:11 UTC (permalink / raw)
  To: Pavel Machek
  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

Hi Pavel,

thanks for looking into this.

Am Tue, 2 Mar 2021 21:54:52 +0100
schrieb Pavel Machek <pavel@ucw.cz>:

> Hi!
> 
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.  
> 
> Ok.
> 
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 2a698df9da57..c15e1e3c5958 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA)		+=
> > leds-turris-omnia.o obj-$(CONFIG_LEDS_WM831X_STATUS)	+=
> > leds-wm831x-status.o obj-$(CONFIG_LEDS_WM8350)		+=
> > leds-wm8350.o obj-$(CONFIG_LEDS_WRAP)			+=
> > leds-wrap.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> > simatic-ipc-leds.o  
> 
> Let's put this into drivers/leds/simple. You'll have to create it.

Ok will do

> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundation.
> > + */  
> 
> Remove?

Sure, was found in wdt as well. Thx

> > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > +	{1 << 15, "simatic-ipc:green:run-stop"},
> > +	{1 << 7,  "simatic-ipc:yellow:run-stop"},
> > +	{1 << 14, "simatic-ipc:red:error"},
> > +	{1 << 6,  "simatic-ipc:yellow:error"},
> > +	{1 << 13, "simatic-ipc:red:maint"},
> > +	{1 << 5,  "simatic-ipc:yellow:maint"},
> > +	{0, ""},
> > +};  
> 
> Please use names consistent with other systems, this is user
> visible. If you have two-color power led, it should be
> :green:power... See include/dt-bindings/leds/common.h .

Well we wanted to pick names that are printed on the devices and would
like to stick to those. Has been a discussion ...
Can we have symlinks to have multiple names per LED?

How strong would you feel about us using our names?

> Please avoid // comments in the code.

Ok

> > +module_init(simatic_ipc_led_init_module);
> > +module_exit(simatic_ipc_led_exit_module);  
> 
> No need for such verbosity for functions that are static.
> 
> > +MODULE_LICENSE("GPL");  
> 
> GPL v2?

Will do.

Stay tuned for v2.

regards,
Henning


> Best regards,
> 								Pavel
> 


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

* Re: [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-02 18:38   ` Guenter Roeck
@ 2021-03-03 14:21     ` Henning Schild
  2021-03-03 14:49       ` Jan Kiszka
  2021-03-08 15:20       ` Henning Schild
  0 siblings, 2 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-03 14:21 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

Hi, 

thanks for the fast and thorough review!

Am Tue, 2 Mar 2021 10:38:19 -0800
schrieb Guenter Roeck <linux@roeck-us.net>:

> On 3/2/21 8:33 AM, Henning Schild wrote:
> > From: Henning Schild <henning.schild@siemens.com>
> > 
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.
> > 
> > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  drivers/watchdog/Kconfig           |  11 ++
> >  drivers/watchdog/Makefile          |   1 +
> >  drivers/watchdog/simatic-ipc-wdt.c | 305
> > +++++++++++++++++++++++++++++ 3 files changed, 317 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..b5c8b7ceb404
> > --- /dev/null
> > +++ b/drivers/watchdog/simatic-ipc-wdt.c
> > @@ -0,0 +1,305 @@
> > +// 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>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundation.  
> 
> Covered by SPDX-License-Identifier
> 
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/errno.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/ioport.h>
> > +#include <linux/sizes.h>> +#include <linux/io.h>
> > +#include <linux/platform_data/x86/simatic-ipc-base.h>  
> 
> Alphabetic order please
> 
> > +
> > +#define WD_ENABLE_IOADR		0x62
> > +#define WD_TRIGGER_IOADR	0x66
> > +#define GPIO_COMMUNITY0_PORT_ID 0xaf
> > +#define PAD_CFG_DW0_GPP_A_23	0x4b8  
> 
> Please increase indentation and spare another tab
> 
> > +#define SAFE_EN_N_427E		0x01
> > +#define SAFE_EN_N_227E		0x04
> > +#define WD_ENABLED		0x01
> > +
> > +#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 DEFINE_SPINLOCK(io_lock);	/* the lock for io
> > operations */ +static struct watchdog_device wdd;
> > +  
> 
> Having two variables named 'wdd' is confusing. Please chose another
> name.
> 
> > +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 get_timeout_idx(u32 timeout)
> > +{
> > +	int i;
> > +
> > +	i = ARRAY_SIZE(wd_timeout_table) - 1;
> > +	for (; i >= 0; i--) {
> > +		if (timeout >= wd_timeout_table[i])
> > +			break;
> > +	}
> > +
> > +	return i;
> > +}  
> 
> Please add a comment explaining why you don't use find_closest().

Will not be a comment but we will switch to using this, thanks for
pointing it out.

> > +
> > +static int wd_start(struct watchdog_device *wdd)
> > +{
> > +	u8 regval;
> > +
> > +	spin_lock(&io_lock);
> > +  
> The watchdog subsystem already provides locking
> since the watchdog device can only be opened once.
> 
> Why is the additional lock needed ?

We had this under internal review and somehow came to the conclusion
that we "might" need it. I think we will remove it or come back with
reasons.

> > +	regval = inb(WD_ENABLE_IOADR);
> > +	regval |= WD_ENABLED;
> > +	outb(regval, WD_ENABLE_IOADR);
> > +
> > +	spin_unlock(&io_lock);
> > +
> > +	return 0;
> > +}
> > +
> > +static int wd_stop(struct watchdog_device *wdd)
> > +{
> > +	u8 regval;
> > +
> > +	spin_lock(&io_lock);
> > +
> > +	regval = inb(WD_ENABLE_IOADR);
> > +	regval &= ~WD_ENABLED;
> > +	outb(regval, WD_ENABLE_IOADR);
> > +
> > +	spin_unlock(&io_lock);
> > +
> > +	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) +{
> > +	u8 regval;
> > +	int timeout_idx = get_timeout_idx(t);
> > +
> > +	spin_lock(&io_lock);
> > +
> > +	regval = inb(WD_ENABLE_IOADR) & 0xc7;
> > +	regval |= timeout_idx << 3;
> > +	outb(regval, WD_ENABLE_IOADR);
> > +
> > +	spin_unlock(&io_lock);
> > +	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_set_safe_en_n(u32 wdtmode, bool safe_en_n)
> > +{
> > +	u16 resetbit;
> > +
> > +	if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
> > +		/* enable SAFE_EN_N on GP_STATUS_REG_227E */
> > +		resetbit = inw(GP_STATUS_REG_227E);
> > +		if (safe_en_n)
> > +			resetbit &= ~SAFE_EN_N_227E;
> > +		else
> > +			resetbit |= SAFE_EN_N_227E;
> > +
> > +		outw(resetbit, GP_STATUS_REG_227E);
> > +	} else {
> > +		/* enable SAFE_EN_N on PCH D1600 */
> > +		resetbit = ioread16(wd_reset_base_addr);
> > +
> > +		if (safe_en_n)
> > +			resetbit &= ~SAFE_EN_N_427E;
> > +		else
> > +			resetbit |= SAFE_EN_N_427E;
> > +
> > +		iowrite16(resetbit, wd_reset_base_addr);
> > +	}
> > +}
> > +
> > +static int wd_setup(u32 wdtmode, bool safe_en_n)
> > +{
> > +	u8 regval;
> > +	int timeout_idx = 0;  
> 
> Unnecessary initialization
> 
> > +	bool alarm_active;
> > +
> > +	timeout_idx = get_timeout_idx(TIMEOUT_DEF);
> > +
> > +	wd_set_safe_en_n(wdtmode, safe_en_n);
> > +
> > +	/* read wd register to determine alarm state */
> > +	regval = inb(WD_ENABLE_IOADR);
> > +	if (regval & 0x80) {
> > +		pr_warn("Watchdog alarm active.\n");  
> 
> Why does that warrant a warning, and what does it mean ? The context
> suggests that it means the previous reset was caused by the watchdog,
> but that is not what the message says.
> 
> > +		regval = 0x82;	/* use only macro mode,
> > reset alarm bit */
> > +		alarm_active = true;
> > +	} else {
> > +		regval = 0x02;	/* use only macro mode */
> > +		alarm_active = false;
> > +	}  
> 
> Would it hurt to just always write 0x82 ?
> 	alarm_active = regval & 0x80;
> 	regval = 0x82 | timeout_idx << 3;
> 
> would be much simpler. Or, if you prefer,
> 	alarm_active = !!(regval & 0x80);
> 	regval = 0x82 | timeout_idx << 3;
> 
> Actually, regval isn't even needed in that case.
> 	alarm_active = !!(regval & 0x80);
> 	outb(0x82 | timeout_idx << 3, WD_ENABLE_IOADR);
> 
> 
> Either case, please use defines for the bits. WD_ENABLED is already
> defined, thus the other bits should be set using defines as well.
> 
> > +
> > +	regval |= timeout_idx << 3;
> > +	if (nowayout)
> > +		regval |= WD_ENABLED;  
> 
> This is not the purpose of nowayout. nowayout prevents stopping
> the watchdog after it has been started. It is not expected to start
> the watchdog on boot.

Thanks, that was misunderstood by the author, will fix.

> > +
> > +	outb(regval, WD_ENABLE_IOADR);
> > +
> > +	return alarm_active;
> > +}
> > +
> > +static int simatic_ipc_wdt_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev = &pdev->dev;
> > +	int rc = 0;
> > +	struct simatic_ipc_platform *plat =
> > pdev->dev.platform_data;
> > +	struct resource *res;
> > +  
> 
> Is it guaranteed that the device will always be instantiated only
> once ? If so, how it it guaranteed ?

I suppose if anything did register two platform devices on the platform
bus this might not be guaranteed. The assumption is that simatic-ipc
will only ever register one, and the machines always have 0-1 "Siemens
watchdogs" so at the moment there will never be a need for more than
one.

> Because if there are ever multiple instances the various static
> variables will cause major trouble (which is why it is always better
> to not use static variables).
> 
> > +	pr_debug(KBUILD_MODNAME ":%s(#%d) WDT mode: %d\n",
> > +		 __func__, __LINE__, plat->devmode);
> > +  
> 
> This is a platform device. Please use dev_ messages (dev_warn,
> dev_dbg etc) throughout.
> 
> > +	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.info = &wdt_ident;
> > +		wdd.ops = &wdt_ops;
> > +		wdd.min_timeout = TIMEOUT_MIN;
> > +		wdd.max_timeout = TIMEOUT_MAX;  
> 
> Why not use static initialization ?
> 
> > +		wdd.parent = NULL;  
> 
> parent should be the platform device.
> 
> > +		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 this is what prevents multiple registrations, it is too late: wdd
> is already overwritten.

As said, there will be no double-registration.

> > +
> > +	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 -ENOMEM;  
> 
> 			return PTR_ERR(wd_reset_base_addr);
> 
> > +	}
> > +
> > +	wdd.bootstatus = wd_setup(plat->devmode, true);  
> 
> bootstatus does not report a boolean. This translates to
> WDIOF_OVERHEAT which is almost certainly wrong.
> 
> > +	if (wdd.bootstatus)
> > +		pr_warn(KBUILD_MODNAME ": last reboot caused by
> > watchdog reset\n");  
> 
> Why two messages ?
> 
> > +
> > +	watchdog_set_nowayout(&wdd, nowayout);
> > +	watchdog_stop_on_reboot(&wdd);
> > +
> > +	rc = devm_watchdog_register_device(dev, &wdd);
> > +  
> Extra empty line not needed
> 
> > +	if (rc == 0)
> > +		pr_debug("initialized. nowayout=%d\n",
> > +			 nowayout);  
> 
> What is the value of this message (especially since there is no
> message if there is an error) ?
> 
> > +
> > +	return rc;
> > +}
> > +
> > +static int simatic_ipc_wdt_remove(struct platform_device *pdev)
> > +{
> > +	struct simatic_ipc_platform *plat =
> > pdev->dev.platform_data; +
> > +	wd_setup(plat->devmode, false);  
> 
> This warrants an explanation. What is the point of updating the
> timeout here ? And what does SAFE_EN actually do ?

The idea was that module unloading should disable the watchdog, but
that code will be removed and aligned with other watchdogs.

SAFE_EN is a second enable bit, if it is not set the watchdog will fire
into the void. Pretty pointless will keep that always armed or set it
always when toggling "enable".

All the other open points are pretty clear, and will all be dealt with
in v2.

regards,
Henning

> The watchdog is stopped on reboot, but this function won't
> be called in that case, making this call even more questionable.
> Please document what it does and why it is needed here (but not
> when rebooting).
> 
> > +	return 0;
> > +}
> > +
> > +static struct platform_driver wdt_driver = {
> > +	.probe = simatic_ipc_wdt_probe,
> > +	.remove = simatic_ipc_wdt_remove,
> > +	.driver = {
> > +		.name = KBUILD_MODNAME,
> > +	},
> > +};
> > +
> > +static int __init simatic_ipc_wdt_init(void)
> > +{
> > +	return platform_driver_register(&wdt_driver);
> > +}
> > +
> > +static void __exit simatic_ipc_wdt_exit(void)
> > +{
> > +	platform_driver_unregister(&wdt_driver);
> > +}
> > +
> > +module_init(simatic_ipc_wdt_init);
> > +module_exit(simatic_ipc_wdt_exit);  
> 
> Why not module_platform_driver() ?
> 
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
> >   
> 


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

* Re: [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-03 14:21     ` Henning Schild
@ 2021-03-03 14:49       ` Jan Kiszka
  2021-03-08 15:20       ` Henning Schild
  1 sibling, 0 replies; 46+ messages in thread
From: Jan Kiszka @ 2021-03-03 14:49 UTC (permalink / raw)
  To: Henning Schild, Guenter Roeck
  Cc: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog,
	Srikanth Krishnakar, Gerd Haeussler, Wim Van Sebroeck,
	Mark Gross, Hans de Goede, Pavel Machek

On 03.03.21 15:21, Henning Schild wrote:
> Hi, 
> 
> thanks for the fast and thorough review!
> 
> Am Tue, 2 Mar 2021 10:38:19 -0800
> schrieb Guenter Roeck <linux@roeck-us.net>:
> 
>> On 3/2/21 8:33 AM, Henning Schild wrote:
>>> From: Henning Schild <henning.schild@siemens.com>
>>>
>>> This driver adds initial support for several devices from Siemens.
>>> It is based on a platform driver introduced in an earlier commit.
>>>
>>> Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
>>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
>>> ---
>>>  drivers/watchdog/Kconfig           |  11 ++
>>>  drivers/watchdog/Makefile          |   1 +
>>>  drivers/watchdog/simatic-ipc-wdt.c | 305
>>> +++++++++++++++++++++++++++++ 3 files changed, 317 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..b5c8b7ceb404
>>> --- /dev/null
>>> +++ b/drivers/watchdog/simatic-ipc-wdt.c
>>> @@ -0,0 +1,305 @@
>>> +// 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>
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> modify
>>> + * it under the terms of the GNU General Public License version 2
>>> as
>>> + * published by the Free Software Foundation.  
>>
>> Covered by SPDX-License-Identifier
>>
>>> + */
>>> +
>>> +#include <linux/device.h>
>>> +#include <linux/init.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/errno.h>
>>> +#include <linux/watchdog.h>
>>> +#include <linux/ioport.h>
>>> +#include <linux/sizes.h>> +#include <linux/io.h>
>>> +#include <linux/platform_data/x86/simatic-ipc-base.h>  
>>
>> Alphabetic order please
>>
>>> +
>>> +#define WD_ENABLE_IOADR		0x62
>>> +#define WD_TRIGGER_IOADR	0x66
>>> +#define GPIO_COMMUNITY0_PORT_ID 0xaf
>>> +#define PAD_CFG_DW0_GPP_A_23	0x4b8  
>>
>> Please increase indentation and spare another tab
>>
>>> +#define SAFE_EN_N_427E		0x01
>>> +#define SAFE_EN_N_227E		0x04
>>> +#define WD_ENABLED		0x01
>>> +
>>> +#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 DEFINE_SPINLOCK(io_lock);	/* the lock for io
>>> operations */ +static struct watchdog_device wdd;
>>> +  
>>
>> Having two variables named 'wdd' is confusing. Please chose another
>> name.
>>
>>> +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 get_timeout_idx(u32 timeout)
>>> +{
>>> +	int i;
>>> +
>>> +	i = ARRAY_SIZE(wd_timeout_table) - 1;
>>> +	for (; i >= 0; i--) {
>>> +		if (timeout >= wd_timeout_table[i])
>>> +			break;
>>> +	}
>>> +
>>> +	return i;
>>> +}  
>>
>> Please add a comment explaining why you don't use find_closest().
> 
> Will not be a comment but we will switch to using this, thanks for
> pointing it out.
> 
>>> +
>>> +static int wd_start(struct watchdog_device *wdd)
>>> +{
>>> +	u8 regval;
>>> +
>>> +	spin_lock(&io_lock);
>>> +  
>> The watchdog subsystem already provides locking
>> since the watchdog device can only be opened once.
>>
>> Why is the additional lock needed ?
> 
> We had this under internal review and somehow came to the conclusion
> that we "might" need it. I think we will remove it or come back with
> reasons.

Probably my fault. I quickly scanned for locking usage in other watchdog
drivers and found plenty of those RMW-protecting locks. I didn't check
then what the actual locking model of the watchdog core is, and possibly
some of the other use cases need to protect register against other users
or multiple watchdog instances. But if the core locks
start/stop/set_timeout internally against each other, this is obviously
pointless without out inherently single-instance use case here.

Jan

-- 
Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-02 20:54   ` Pavel Machek
  2021-03-03 13:11     ` Henning Schild
@ 2021-03-03 17:37     ` Henning Schild
  2021-03-03 17:40       ` Pavel Machek
  1 sibling, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-03 17:37 UTC (permalink / raw)
  To: Pavel Machek
  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

Am Tue, 2 Mar 2021 21:54:52 +0100
schrieb Pavel Machek <pavel@ucw.cz>:

> Hi!
> 
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.  
> 
> Ok.
> 
> > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > index 2a698df9da57..c15e1e3c5958 100644
> > --- a/drivers/leds/Makefile
> > +++ b/drivers/leds/Makefile
> > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA)		+=
> > leds-turris-omnia.o obj-$(CONFIG_LEDS_WM831X_STATUS)	+=
> > leds-wm831x-status.o obj-$(CONFIG_LEDS_WM8350)		+=
> > leds-wm8350.o obj-$(CONFIG_LEDS_WRAP)			+=
> > leds-wrap.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> > simatic-ipc-leds.o  
> 
> Let's put this into drivers/leds/simple. You'll have to create it.

Can you please go into detail why? We plan to add more devices in the
future, which might in fact make this a little less simple. But we can
discuss that when the time is right and start with simple.

regards,
Henning

> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundation.
> > + */  
> 
> Remove?
> 
> > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > +	{1 << 15, "simatic-ipc:green:run-stop"},
> > +	{1 << 7,  "simatic-ipc:yellow:run-stop"},
> > +	{1 << 14, "simatic-ipc:red:error"},
> > +	{1 << 6,  "simatic-ipc:yellow:error"},
> > +	{1 << 13, "simatic-ipc:red:maint"},
> > +	{1 << 5,  "simatic-ipc:yellow:maint"},
> > +	{0, ""},
> > +};  
> 
> Please use names consistent with other systems, this is user
> visible. If you have two-color power led, it should be
> :green:power... See include/dt-bindings/leds/common.h .
> 
> Please avoid // comments in the code.
> 
> > +module_init(simatic_ipc_led_init_module);
> > +module_exit(simatic_ipc_led_exit_module);  
> 
> No need for such verbosity for functions that are static.
> 
> > +MODULE_LICENSE("GPL");  
> 
> GPL v2?
> 
> Best regards,
> 								Pavel
> 


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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-03 17:37     ` Henning Schild
@ 2021-03-03 17:40       ` Pavel Machek
  2021-03-03 18:49         ` Henning Schild
  0 siblings, 1 reply; 46+ messages in thread
From: Pavel Machek @ 2021-03-03 17:40 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, Hans de Goede


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

Hi!

> > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > index 2a698df9da57..c15e1e3c5958 100644
> > > --- a/drivers/leds/Makefile
> > > +++ b/drivers/leds/Makefile
> > > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA)		+=
> > > leds-turris-omnia.o obj-$(CONFIG_LEDS_WM831X_STATUS)	+=
> > > leds-wm831x-status.o obj-$(CONFIG_LEDS_WM8350)		+=
> > > leds-wm8350.o obj-$(CONFIG_LEDS_WRAP)			+=
> > > leds-wrap.o +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> > > simatic-ipc-leds.o  
> > 
> > Let's put this into drivers/leds/simple. You'll have to create it.
> 
> Can you please go into detail why? We plan to add more devices in the
> future, which might in fact make this a little less simple. But we can
> discuss that when the time is right and start with simple.

There's already way too many drivers in the directory, and your driver
is very different from drivers for camera flash (for example).

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-03 17:40       ` Pavel Machek
@ 2021-03-03 18:49         ` Henning Schild
  2021-03-03 19:27           ` Pavel Machek
  0 siblings, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-03 18:49 UTC (permalink / raw)
  To: Pavel Machek
  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

Am Wed, 3 Mar 2021 18:40:40 +0100
schrieb Pavel Machek <pavel@ucw.cz>:

> Hi!
> 
> > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > > index 2a698df9da57..c15e1e3c5958 100644
> > > > --- a/drivers/leds/Makefile
> > > > +++ b/drivers/leds/Makefile
> > > > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA)
> > > > 	+= leds-turris-omnia.o
> > > > obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
> > > > obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
> > > > obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
> > > > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> > > > simatic-ipc-leds.o    
> > > 
> > > Let's put this into drivers/leds/simple. You'll have to create
> > > it.  
> > 
> > Can you please go into detail why? We plan to add more devices in
> > the future, which might in fact make this a little less simple. But
> > we can discuss that when the time is right and start with simple.  
> 
> There's already way too many drivers in the directory, and your driver
> is very different from drivers for camera flash (for example).

Understood, the whole Makefile Kconfig thingy?

Henning

> Best regards,
> 								Pavel


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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-03 18:49         ` Henning Schild
@ 2021-03-03 19:27           ` Pavel Machek
  0 siblings, 0 replies; 46+ messages in thread
From: Pavel Machek @ 2021-03-03 19:27 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, Hans de Goede


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

On Wed 2021-03-03 19:49:56, Henning Schild wrote:
> Am Wed, 3 Mar 2021 18:40:40 +0100
> schrieb Pavel Machek <pavel@ucw.cz>:
> 
> > Hi!
> > 
> > > > > diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> > > > > index 2a698df9da57..c15e1e3c5958 100644
> > > > > --- a/drivers/leds/Makefile
> > > > > +++ b/drivers/leds/Makefile
> > > > > @@ -93,6 +93,7 @@ obj-$(CONFIG_LEDS_TURRIS_OMNIA)
> > > > > 	+= leds-turris-omnia.o
> > > > > obj-$(CONFIG_LEDS_WM831X_STATUS)	+= leds-wm831x-status.o
> > > > > obj-$(CONFIG_LEDS_WM8350)		+= leds-wm8350.o
> > > > > obj-$(CONFIG_LEDS_WRAP)			+= leds-wrap.o
> > > > > +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC)	+=
> > > > > simatic-ipc-leds.o    
> > > > 
> > > > Let's put this into drivers/leds/simple. You'll have to create
> > > > it.  
> > > 
> > > Can you please go into detail why? We plan to add more devices in
> > > the future, which might in fact make this a little less simple. But
> > > we can discuss that when the time is right and start with simple.  
> > 
> > There's already way too many drivers in the directory, and your driver
> > is very different from drivers for camera flash (for example).
> 
> Understood, the whole Makefile Kconfig thingy?

You'll need Makefile + Kconfig, yes. No need for CONFIG_LEDS_SIMPLE.

Best regards,
								Pavel

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-03 13:11     ` Henning Schild
@ 2021-03-03 19:31       ` Pavel Machek
  2021-03-03 20:48         ` Henning Schild
  0 siblings, 1 reply; 46+ messages in thread
From: Pavel Machek @ 2021-03-03 19:31 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, Hans de Goede


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

Hi!

> > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > +	{1 << 15, "simatic-ipc:green:run-stop"},
> > > +	{1 << 7,  "simatic-ipc:yellow:run-stop"},
> > > +	{1 << 14, "simatic-ipc:red:error"},
> > > +	{1 << 6,  "simatic-ipc:yellow:error"},
> > > +	{1 << 13, "simatic-ipc:red:maint"},
> > > +	{1 << 5,  "simatic-ipc:yellow:maint"},
> > > +	{0, ""},
> > > +};  
> > 
> > Please use names consistent with other systems, this is user
> > visible. If you have two-color power led, it should be
> > :green:power... See include/dt-bindings/leds/common.h .
> 
> Well we wanted to pick names that are printed on the devices and would
> like to stick to those. Has been a discussion ...
> Can we have symlinks to have multiple names per LED?

No symlinks. We plan to have command line tool to manipulate LEDs,
aliases might be possible there.

> How strong would you feel about us using our names?

Strongly. :-)

Do you have a picture how the leds look like?

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

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-03 19:31       ` Pavel Machek
@ 2021-03-03 20:48         ` Henning Schild
  2021-03-03 20:56           ` Henning Schild
  0 siblings, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-03 20:48 UTC (permalink / raw)
  To: Pavel Machek
  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

Am Wed, 3 Mar 2021 20:31:34 +0100
schrieb Pavel Machek <pavel@ucw.cz>:

> Hi!
> 
> > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > > +	{1 << 15, "simatic-ipc:green:run-stop"},
> > > > +	{1 << 7,  "simatic-ipc:yellow:run-stop"},
> > > > +	{1 << 14, "simatic-ipc:red:error"},
> > > > +	{1 << 6,  "simatic-ipc:yellow:error"},
> > > > +	{1 << 13, "simatic-ipc:red:maint"},
> > > > +	{1 << 5,  "simatic-ipc:yellow:maint"},
> > > > +	{0, ""},
> > > > +};    
> > > 
> > > Please use names consistent with other systems, this is user
> > > visible. If you have two-color power led, it should be
> > > :green:power... See include/dt-bindings/leds/common.h .  
> > 
> > Well we wanted to pick names that are printed on the devices and
> > would like to stick to those. Has been a discussion ...
> > Can we have symlinks to have multiple names per LED?  
> 
> No symlinks. We plan to have command line tool to manipulate LEDs,
> aliases might be possible there.

Sounds like a future plan. sysfs and "cat" "echo" are mighty tools and
"everything is a file" is the best idea ever. So i would say any
aliasing should live in the kernel, but that is just me. Tools will
just get out of sync, be missing in busybox or a random yocto ... or
whichever distro you like.
On the other hand you have "complexity should be userland" ... i do not
have the answer.

> > How strong would you feel about us using our names?  
> 
> Strongly. :-)

OK, will try to find a match where possible. 

> Do you have a picture how the leds look like?

I could even find chassis photos in our internal review but that would
be too much.

Our idea is probably the same as yours. We want the same names across
all devices. But we struggle with colors because on some boxes we have
red+green, while other offer yellow ... implemented in HW and messing
with red+green in some cases.

But so far we only looked at Siemens devices and thought we could get
our own "namespace".

To be honest i could not even tell how our names map on the known ones,
but we will do our best to find a match. They all are "high-level" so
"power" and other basic things are not exposed.

regards,
Henning
 
> Best regards,
> 							Pavel


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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-03 20:48         ` Henning Schild
@ 2021-03-03 20:56           ` Henning Schild
  2021-03-05 18:25             ` Henning Schild
  0 siblings, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-03 20:56 UTC (permalink / raw)
  To: Pavel Machek
  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

Am Wed, 3 Mar 2021 21:48:21 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Wed, 3 Mar 2021 20:31:34 +0100
> schrieb Pavel Machek <pavel@ucw.cz>:
> 
> > Hi!
> >   
> > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > > > +	{1 << 15, "simatic-ipc:green:run-stop"},
> > > > > +	{1 << 7,  "simatic-ipc:yellow:run-stop"},
> > > > > +	{1 << 14, "simatic-ipc:red:error"},
> > > > > +	{1 << 6,  "simatic-ipc:yellow:error"},
> > > > > +	{1 << 13, "simatic-ipc:red:maint"},
> > > > > +	{1 << 5,  "simatic-ipc:yellow:maint"},
> > > > > +	{0, ""},
> > > > > +};      
> > > > 
> > > > Please use names consistent with other systems, this is user
> > > > visible. If you have two-color power led, it should be
> > > > :green:power... See include/dt-bindings/leds/common.h .    
> > > 
> > > Well we wanted to pick names that are printed on the devices and
> > > would like to stick to those. Has been a discussion ...
> > > Can we have symlinks to have multiple names per LED?    
> > 
> > No symlinks. We plan to have command line tool to manipulate LEDs,
> > aliases might be possible there.  
> 
> Sounds like a future plan. sysfs and "cat" "echo" are mighty tools and
> "everything is a file" is the best idea ever. So i would say any
> aliasing should live in the kernel, but that is just me. Tools will
> just get out of sync, be missing in busybox or a random yocto ... or
> whichever distro you like.
> On the other hand you have "complexity should be userland" ... i do
> not have the answer.

My personal horror would be systemd-ledd or some dracut snipet for
initrd. But that would be a generic led class discussion ... that tool.

> > > How strong would you feel about us using our names?    
> > 
> > Strongly. :-)  
> 
> OK, will try to find a match where possible. 

Do we happen to have a description of the existing names, to find a fit
for ours? In the header you pointed out i only found names without
"meaning"

regards,
Henning

> 
> > Do you have a picture how the leds look like?  
> 
> I could even find chassis photos in our internal review but that would
> be too much.
> 
> Our idea is probably the same as yours. We want the same names across
> all devices. But we struggle with colors because on some boxes we have
> red+green, while other offer yellow ... implemented in HW and messing
> with red+green in some cases.
> 
> But so far we only looked at Siemens devices and thought we could get
> our own "namespace".
> 
> To be honest i could not even tell how our names map on the known
> ones, but we will do our best to find a match. They all are
> "high-level" so "power" and other basic things are not exposed.
> 
> regards,
> Henning
>  
> > Best regards,
> > 							Pavel  
> 


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

* Re: [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
       [not found]   ` <CAHp75VdUXWDg-2o8fmeo7EoMtRLfCnFOw5ptwjXTv9fKMmHv2A@mail.gmail.com>
@ 2021-03-04  9:00     ` Henning Schild
  0 siblings, 0 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-04  9:00 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: linux-kernel, linux-leds, platform-driver-x86, linux-watchdog,
	Srikanth Krishnakar, Jan Kiszka, Gerd Haeussler, Guenter Roeck,
	Wim Van Sebroeck, Mark Gross, Hans de Goede, Pavel Machek

Am Thu, 4 Mar 2021 10:27:07 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Tuesday, March 2, 2021, Henning Schild <henning.schild@siemens.com>
> wrote:
> 
> > From: Henning Schild <henning.schild@siemens.com>
> >
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.
> >
> >  
> 
> Is it ACPI based? Why it can not provide WDAT table instead?

Yes it is. We strongly urged the BIOS guys to look into that for future
products and BIOS updates for current products. But for existing
products they will not add "features". One fear is breaking Windows
where custom drivers have been the way they did it so far.

So we are dealing with legacy here, that is why. Hope that is not a
show-stopper.

Maybe we can carry ACPI quirk snippets instead, did not yet look into
this. And i am not sure which version would be easier to maintain and
more acceptable to the kernel.

regards,
Henning

> 
> > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > ---
> >  drivers/watchdog/Kconfig           |  11 ++
> >  drivers/watchdog/Makefile          |   1 +
> >  drivers/watchdog/simatic-ipc-wdt.c | 305
> > +++++++++++++++++++++++++++++ 3 files changed, 317 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..b5c8b7ceb404
> > --- /dev/null
> > +++ b/drivers/watchdog/simatic-ipc-wdt.c
> > @@ -0,0 +1,305 @@
> > +// 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>
> > + *
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/errno.h>
> > +#include <linux/watchdog.h>
> > +#include <linux/ioport.h>
> > +#include <linux/sizes.h>
> > +#include <linux/io.h>
> > +#include <linux/platform_data/x86/simatic-ipc-base.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 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 DEFINE_SPINLOCK(io_lock);       /* the lock for io
> > operations */ +static struct watchdog_device wdd;
> > +
> > +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 get_timeout_idx(u32 timeout)
> > +{
> > +       int i;
> > +
> > +       i = ARRAY_SIZE(wd_timeout_table) - 1;
> > +       for (; i >= 0; i--) {
> > +               if (timeout >= wd_timeout_table[i])
> > +                       break;
> > +       }
> > +
> > +       return i;
> > +}
> > +
> > +static int wd_start(struct watchdog_device *wdd)
> > +{
> > +       u8 regval;
> > +
> > +       spin_lock(&io_lock);
> > +
> > +       regval = inb(WD_ENABLE_IOADR);
> > +       regval |= WD_ENABLED;
> > +       outb(regval, WD_ENABLE_IOADR);
> > +
> > +       spin_unlock(&io_lock);
> > +
> > +       return 0;
> > +}
> > +
> > +static int wd_stop(struct watchdog_device *wdd)
> > +{
> > +       u8 regval;
> > +
> > +       spin_lock(&io_lock);
> > +
> > +       regval = inb(WD_ENABLE_IOADR);
> > +       regval &= ~WD_ENABLED;
> > +       outb(regval, WD_ENABLE_IOADR);
> > +
> > +       spin_unlock(&io_lock);
> > +
> > +       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) +{
> > +       u8 regval;
> > +       int timeout_idx = get_timeout_idx(t);
> > +
> > +       spin_lock(&io_lock);
> > +
> > +       regval = inb(WD_ENABLE_IOADR) & 0xc7;
> > +       regval |= timeout_idx << 3;
> > +       outb(regval, WD_ENABLE_IOADR);
> > +
> > +       spin_unlock(&io_lock);
> > +       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_set_safe_en_n(u32 wdtmode, bool safe_en_n)
> > +{
> > +       u16 resetbit;
> > +
> > +       if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
> > +               /* enable SAFE_EN_N on GP_STATUS_REG_227E */
> > +               resetbit = inw(GP_STATUS_REG_227E);
> > +               if (safe_en_n)
> > +                       resetbit &= ~SAFE_EN_N_227E;
> > +               else
> > +                       resetbit |= SAFE_EN_N_227E;
> > +
> > +               outw(resetbit, GP_STATUS_REG_227E);
> > +       } else {
> > +               /* enable SAFE_EN_N on PCH D1600 */
> > +               resetbit = ioread16(wd_reset_base_addr);
> > +
> > +               if (safe_en_n)
> > +                       resetbit &= ~SAFE_EN_N_427E;
> > +               else
> > +                       resetbit |= SAFE_EN_N_427E;
> > +
> > +               iowrite16(resetbit, wd_reset_base_addr);
> > +       }
> > +}
> > +
> > +static int wd_setup(u32 wdtmode, bool safe_en_n)
> > +{
> > +       u8 regval;
> > +       int timeout_idx = 0;
> > +       bool alarm_active;
> > +
> > +       timeout_idx = get_timeout_idx(TIMEOUT_DEF);
> > +
> > +       wd_set_safe_en_n(wdtmode, safe_en_n);
> > +
> > +       /* read wd register to determine alarm state */
> > +       regval = inb(WD_ENABLE_IOADR);
> > +       if (regval & 0x80) {
> > +               pr_warn("Watchdog alarm active.\n");
> > +               regval = 0x82;  /* use only macro mode, reset alarm
> > bit */
> > +               alarm_active = true;
> > +       } else {
> > +               regval = 0x02;  /* use only macro mode */
> > +               alarm_active = false;
> > +       }
> > +
> > +       regval |= timeout_idx << 3;
> > +       if (nowayout)
> > +               regval |= WD_ENABLED;
> > +
> > +       outb(regval, WD_ENABLE_IOADR);
> > +
> > +       return alarm_active;
> > +}
> > +
> > +static int simatic_ipc_wdt_probe(struct platform_device *pdev)
> > +{
> > +       struct device *dev = &pdev->dev;
> > +       int rc = 0;
> > +       struct simatic_ipc_platform *plat = pdev->dev.platform_data;
> > +       struct resource *res;
> > +
> > +       pr_debug(KBUILD_MODNAME ":%s(#%d) WDT mode: %d\n",
> > +                __func__, __LINE__, plat->devmode);
> > +
> > +       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.info = &wdt_ident;
> > +               wdd.ops = &wdt_ops;
> > +               wdd.min_timeout = TIMEOUT_MIN;
> > +               wdd.max_timeout = TIMEOUT_MAX;
> > +               wdd.parent = NULL;
> > +               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 -ENOMEM;
> > +       }
> > +
> > +       wdd.bootstatus = wd_setup(plat->devmode, true);
> > +       if (wdd.bootstatus)
> > +               pr_warn(KBUILD_MODNAME ": last reboot caused by
> > watchdog reset\n");
> > +
> > +       watchdog_set_nowayout(&wdd, nowayout);
> > +       watchdog_stop_on_reboot(&wdd);
> > +
> > +       rc = devm_watchdog_register_device(dev, &wdd);
> > +
> > +       if (rc == 0)
> > +               pr_debug("initialized. nowayout=%d\n",
> > +                        nowayout);
> > +
> > +       return rc;
> > +}
> > +
> > +static int simatic_ipc_wdt_remove(struct platform_device *pdev)
> > +{
> > +       struct simatic_ipc_platform *plat = pdev->dev.platform_data;
> > +
> > +       wd_setup(plat->devmode, false);
> > +       return 0;
> > +}
> > +
> > +static struct platform_driver wdt_driver = {
> > +       .probe = simatic_ipc_wdt_probe,
> > +       .remove = simatic_ipc_wdt_remove,
> > +       .driver = {
> > +               .name = KBUILD_MODNAME,
> > +       },
> > +};
> > +
> > +static int __init simatic_ipc_wdt_init(void)
> > +{
> > +       return platform_driver_register(&wdt_driver);
> > +}
> > +
> > +static void __exit simatic_ipc_wdt_exit(void)
> > +{
> > +       platform_driver_unregister(&wdt_driver);
> > +}
> > +
> > +module_init(simatic_ipc_wdt_init);
> > +module_exit(simatic_ipc_wdt_exit);
> > +
> > +MODULE_LICENSE("GPL");
> > +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
> > --
> > 2.26.2
> >
> >  
> 


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

* Re: [PATCH 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs
  2021-03-02 16:33 ` [PATCH 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
@ 2021-03-04  9:50   ` Andy Shevchenko
  0 siblings, 0 replies; 46+ messages in thread
From: Andy Shevchenko @ 2021-03-04  9:50 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, Michael Haener

On Thu, Mar 4, 2021 at 9:27 AM Henning Schild
<henning.schild@siemens.com> wrote:

> 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.

> +#include <linux/platform_data/x86/simatic-ipc.h>

> +static int pmc_clk_is_critical(const struct dmi_system_id *d)
> +{
> +       int ret = true;
> +       u32 station_id;
> +
> +       if (!strcmp(d->ident, "SIEMENS AG")) {
> +               if (dmi_walk(simatic_ipc_find_dmi_entry_helper, &station_id))
> +                       ret = false;
> +               else
> +                       ret = (station_id == SIMATIC_IPC_IPC227E ||
> +                              station_id == SIMATIC_IPC_IPC277E);
> +       }
> +
> +       return ret;

Much easier to rewrite it as

if (strcmp(...)) // BTW, do we have a dmi_* helper for that?
  return true;

if (dmi_walk)
  return false;

return station_id == || ...;

> +}

Maybe instead you can rewrite it as a callback in DMI table which
changes a (global, yeah) variable that you simply reassign...



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

...somewhere here?

Like
  clk_data->critical = global_var;
  if (...)
    pr_info();

It seems it will reduce burden on a callback by dropping strcmp() call.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-02 16:33 ` [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
@ 2021-03-04 10:11   ` Andy Shevchenko
  2021-03-04 13:47     ` Hans de Goede
  2021-03-04 19:42     ` Henning Schild
  2021-03-04 14:03   ` Hans de Goede
  1 sibling, 2 replies; 46+ messages in thread
From: Andy Shevchenko @ 2021-03-04 10:11 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 Thu, Mar 4, 2021 at 8:36 AM Henning Schild
<henning.schild@siemens.com> wrote:
>
> From: Henning Schild <henning.schild@siemens.com>
>
> This mainly implements detection of these devices and will allow
> secondary drivers to work on such machines.
>
> The identification is DMI-based with a vendor specific way to tell them
> apart in a reliable way.
>
> Drivers for LEDs and Watchdogs will follow to make use of that platform
> detection.

> Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> Signed-off-by: Henning Schild <henning.schild@siemens.com>

The order is wrong taking into account the From line in the body. So,
it's not clear who is the author, who is a co-developer, and who is
the committer (one person may utilize few roles).
Check for the rest of the series as well (basically this is the rule
of thumb to recheck entire code for the comment you have got at any
single place of it).

...

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

LEDs
watchdog. (missed period and singular form)

Module name?

>  endif # X86_PLATFORM_DEVICES

Not sure about the ordering of the section in Kconfig, but it is up to
maintainers.

...

> +# Siemens Simatic Industrial PCs
> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC)      += simatic-ipc.o

Ditto.

...

> + * Siemens SIMATIC IPC driver for LEDs

It seems to be used in patch 4 which is not LED related. Also, why is
it here if it's for LEDs?

...

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

Replace with SPDX

...

> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>

Ordered?

> +#include <linux/platform_data/x86/simatic-ipc.h>

...

> +static int register_platform_devices(u32 station_id)
> +{
> +       int i;
> +       u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> +       u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;

Reversed xmas tree order?

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

Strange indentation (second line).

> +               if (IS_ERR(ipc_led_platform_device))
> +                       return PTR_ERR(ipc_led_platform_device);

> +               pr_debug(KBUILD_MODNAME ": device=%s created\n",
> +                        ipc_led_platform_device->name);

Utilize pr_fmt() instead of adding prefixes like this.

> +       }

> +       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(KBUILD_MODNAME ": device=%s created\n",
> +                        ipc_wdt_platform_device->name);
> +       }

Same comments as above.

> +       if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
> +           wdtmode == SIMATIC_IPC_DEVICE_NONE) {
> +               pr_warn(KBUILD_MODNAME
> +                       ": unsupported IPC detected, station id=%08x\n",
> +                       station_id);

Ugly indentation. With pr_fmt() in use will be much better.

> +               return -EINVAL;

> +       } else {

Redundant.

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

bar -> BAR
Missed period.

> + */

> +

Unneeded blank line.

> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> +{
> +       u32 bar0 = 0;

> +#ifdef CONFIG_PCI

It's ugly besides the fact that you have a dependency.

> +       struct pci_bus *bus;

Missed blank line.

> +       /*
> +        * 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();
> +#endif /* CONFIG_PCI */
> +       return bar0;
> +}
> +EXPORT_SYMBOL(simatic_ipc_get_membase0);

Oy vey! I know what this is and let's do it differently. I have some
(relatively old) patch series I can send you privately for testing.

...

> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.

Not needed when you have SPDX.

...

> +#include <linux/pci.h>

Wrong header. Should be types.h

...

> +#include <linux/dmi.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>

Missed headers. You need to include ones that the code below is a
direct user of.

Like types.h

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 0/4] add device drivers for Siemens Industrial PCs
  2021-03-02 16:33 [PATCH 0/4] add device drivers for Siemens Industrial PCs Henning Schild
                   ` (3 preceding siblings ...)
  2021-03-02 16:33 ` [PATCH 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
@ 2021-03-04 10:19 ` Andy Shevchenko
  2021-03-04 10:20   ` Andy Shevchenko
  2021-03-04 19:14   ` Henning Schild
  4 siblings, 2 replies; 46+ messages in thread
From: Andy Shevchenko @ 2021-03-04 10:19 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 Thu, Mar 4, 2021 at 9:29 AM Henning Schild
<henning.schild@siemens.com> wrote:

> 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.
>
> But the idea here is to share early, and hopefully not fail early.

I have given a few comments here and there, so please check the entire
series and address them in _all_ similar locations. As I have noticed,
I have different approach about P2SB code, I have to give the series a
dust and see if you can utilize it.

I would like to be Cc'ed on the next version.


-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 0/4] add device drivers for Siemens Industrial PCs
  2021-03-04 10:19 ` [PATCH 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
@ 2021-03-04 10:20   ` Andy Shevchenko
  2021-03-04 19:26     ` Henning Schild
  2021-03-04 19:14   ` Henning Schild
  1 sibling, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2021-03-04 10:20 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 Thu, Mar 4, 2021 at 12:19 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Mar 4, 2021 at 9:29 AM Henning Schild
> <henning.schild@siemens.com> wrote:

> I have given a few comments here and there, so please check the entire
> series and address them in _all_ similar locations. As I have noticed,
> I have different approach about P2SB code, I have to give the series a
> dust and see if you can utilize it.
>
> I would like to be Cc'ed on the next version.

One more thing, is it Apollo Lake based?

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-04 10:11   ` Andy Shevchenko
@ 2021-03-04 13:47     ` Hans de Goede
  2021-03-05 15:42       ` Andy Shevchenko
  2021-03-04 19:42     ` Henning Schild
  1 sibling, 1 reply; 46+ messages in thread
From: Hans de Goede @ 2021-03-04 13:47 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, Pavel Machek

Hi,

On 3/4/21 11:11 AM, Andy Shevchenko wrote:
> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> <henning.schild@siemens.com> wrote:
>>
>> From: Henning Schild <henning.schild@siemens.com>
>>
>> This mainly implements detection of these devices and will allow
>> secondary drivers to work on such machines.
>>
>> The identification is DMI-based with a vendor specific way to tell them
>> apart in a reliable way.
>>
>> Drivers for LEDs and Watchdogs will follow to make use of that platform
>> detection.
> 
>> Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
>> Signed-off-by: Henning Schild <henning.schild@siemens.com>
> 
> The order is wrong taking into account the From line in the body. So,
> it's not clear who is the author, who is a co-developer, and who is
> the committer (one person may utilize few roles).
> Check for the rest of the series as well (basically this is the rule
> of thumb to recheck entire code for the comment you have got at any
> single place of it).
> 
> ...
> 
>> +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 watchdogs
> 
> LEDs
> watchdog. (missed period and singular form)
> 
> Module name?
> 
>>  endif # X86_PLATFORM_DEVICES
> 
> Not sure about the ordering of the section in Kconfig, but it is up to
> maintainers.
> 
> ...
> 
>> +# Siemens Simatic Industrial PCs
>> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC)      += simatic-ipc.o
> 
> Ditto.
> 
> ...
> 
>> + * Siemens SIMATIC IPC driver for LEDs
> 
> It seems to be used in patch 4 which is not LED related. Also, why is
> it here if it's for LEDs?
> 
> ...
> 
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
> 
> Replace with SPDX
> 
> ...
> 
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
> 
> Ordered?
> 
>> +#include <linux/platform_data/x86/simatic-ipc.h>
> 
> ...
> 
>> +static int register_platform_devices(u32 station_id)
>> +{
>> +       int i;
>> +       u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
>> +       u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> 
> Reversed xmas tree order?
> 
>> +       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));
> 
> Strange indentation (second line).
> 
>> +               if (IS_ERR(ipc_led_platform_device))
>> +                       return PTR_ERR(ipc_led_platform_device);
> 
>> +               pr_debug(KBUILD_MODNAME ": device=%s created\n",
>> +                        ipc_led_platform_device->name);
> 
> Utilize pr_fmt() instead of adding prefixes like this.
> 
>> +       }
> 
>> +       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(KBUILD_MODNAME ": device=%s created\n",
>> +                        ipc_wdt_platform_device->name);
>> +       }
> 
> Same comments as above.
> 
>> +       if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
>> +           wdtmode == SIMATIC_IPC_DEVICE_NONE) {
>> +               pr_warn(KBUILD_MODNAME
>> +                       ": unsupported IPC detected, station id=%08x\n",
>> +                       station_id);
> 
> Ugly indentation. With pr_fmt() in use will be much better.
> 
>> +               return -EINVAL;
> 
>> +       } else {
> 
> Redundant.
> 
>> +               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
> 
> bar -> BAR
> Missed period.
> 
>> + */
> 
>> +
> 
> Unneeded blank line.
> 
>> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
>> +{
>> +       u32 bar0 = 0;
> 
>> +#ifdef CONFIG_PCI
> 
> It's ugly besides the fact that you have a dependency.
> 
>> +       struct pci_bus *bus;
> 
> Missed blank line.
> 
>> +       /*
>> +        * 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();
>> +#endif /* CONFIG_PCI */
>> +       return bar0;
>> +}
>> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
> 
> Oy vey! I know what this is and let's do it differently. I have some
> (relatively old) patch series I can send you privately for testing.

This bit stood out the most to me too, it would be good if we can this fixed
in some cleaner work. So I'm curious how things will look with Andy's work
integrated.

Also I don't think this should be exported. Instead this (or its replacement)
should be used to get the address for an IOMEM resource to add the platform 
devices when they are instantiated. Then the platform-dev drivers can just
use the regular functions to get their resources instead of relying on this
module.

Regards,

Hans




> 
> ...
> 
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
> 
> Not needed when you have SPDX.
> 
> ...
> 
>> +#include <linux/pci.h>
> 
> Wrong header. Should be types.h
> 
> ...
> 
>> +#include <linux/dmi.h>
>> +#include <linux/platform_data/x86/simatic-ipc-base.h>
> 
> Missed headers. You need to include ones that the code below is a
> direct user of.
> 
> Like types.h
> 


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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-02 16:33 ` [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
  2021-03-04 10:11   ` Andy Shevchenko
@ 2021-03-04 14:03   ` Hans de Goede
  1 sibling, 0 replies; 46+ messages in thread
From: Hans de Goede @ 2021-03-04 14:03 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

Hi,

On 3/2/21 5:33 PM, Henning Schild wrote:

<snip>

> +static inline u32 simatic_ipc_get_station_id(u8 *data)
> +{
> +	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;
> +
> +	/* 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 && 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 + sizeof(struct dmi_header));
> +}

Please take dh->length into account here and make sure that you don't walk
past the end of the DMI tables during the parsing here.

Regards,

Hans


> +
> +#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */
> 


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

* Re: [PATCH 0/4] add device drivers for Siemens Industrial PCs
  2021-03-04 10:19 ` [PATCH 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
  2021-03-04 10:20   ` Andy Shevchenko
@ 2021-03-04 19:14   ` Henning Schild
  1 sibling, 0 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-04 19:14 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

Thanks Andy,

Am Thu, 4 Mar 2021 12:19:44 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Mar 4, 2021 at 9:29 AM Henning Schild
> <henning.schild@siemens.com> wrote:
> 
> > 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.
> >
> > But the idea here is to share early, and hopefully not fail early.  
> 
> I have given a few comments here and there, so please check the entire
> series and address them in _all_ similar locations. As I have noticed,
> I have different approach about P2SB code, I have to give the series a
> dust and see if you can utilize it.

You did find some things that others found as well. SPDX vs blabla,
header ordering, some other style.
Some things are already done and will be in v2.

Other findings are new, and we will look into them. The only thing that
did stick out is P2SB, also was a point in internal pre-review.
Let us see what you have, i can include a patch of yours into the q.
But i am kind of afraid once it is there, several existing users should
be touched with it, and this series would come later. Or this series
comes first and you come later and clean up our "mess". Not sure i
would want to take over all P2SB unhiders, but with you on board it
will work.

> I would like to be Cc'ed on the next version.

Sure thing.

regards,
Henning


> 


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

* Re: [PATCH 0/4] add device drivers for Siemens Industrial PCs
  2021-03-04 10:20   ` Andy Shevchenko
@ 2021-03-04 19:26     ` Henning Schild
  0 siblings, 0 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-04 19:26 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 Thu, 4 Mar 2021 12:20:22 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Mar 4, 2021 at 12:19 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Thu, Mar 4, 2021 at 9:29 AM Henning Schild
> > <henning.schild@siemens.com> wrote:  
> 
> > I have given a few comments here and there, so please check the
> > entire series and address them in _all_ similar locations. As I
> > have noticed, I have different approach about P2SB code, I have to
> > give the series a dust and see if you can utilize it.
> >
> > I would like to be Cc'ed on the next version.  
> 
> One more thing, is it Apollo Lake based?

Not sure i can answer that, or what you even refer to. The whole series
is about a range of devices, some even have sub-models with differing
CPUs and Lakes

regards,
Henning


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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-04 10:11   ` Andy Shevchenko
  2021-03-04 13:47     ` Hans de Goede
@ 2021-03-04 19:42     ` Henning Schild
  2021-03-05 15:46       ` Andy Shevchenko
  1 sibling, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-04 19:42 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 Thu, 4 Mar 2021 12:11:12 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> <henning.schild@siemens.com> wrote:
> >
> > From: Henning Schild <henning.schild@siemens.com>
> >
> > This mainly implements detection of these devices and will allow
> > secondary drivers to work on such machines.
> >
> > The identification is DMI-based with a vendor specific way to tell
> > them apart in a reliable way.
> >
> > Drivers for LEDs and Watchdogs will follow to make use of that
> > platform detection.  
> 
> > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> > Signed-off-by: Henning Schild <henning.schild@siemens.com>  
> 
> The order is wrong taking into account the From line in the body. So,
> it's not clear who is the author, who is a co-developer, and who is
> the committer (one person may utilize few roles).
> Check for the rest of the series as well (basically this is the rule
> of thumb to recheck entire code for the comment you have got at any
> single place of it).

For some code Gerd is the author, and i am the co-Author. We even have
Jan in the mix at places. At least in copyright headers.

I will remain the committer for the three of us. And since i do not
know exactly what the problem is i will add only my Signed-off to avoid
confusion.

Please speak up it that would be wrong as well and point me to the docs
i missed. Or tell me how it should be done. 

> ...
> 
> > +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 watchdogs  
> 
> LEDs
> watchdog. (missed period and singular form)
> 
> Module name?
> 
> >  endif # X86_PLATFORM_DEVICES  
> 
> Not sure about the ordering of the section in Kconfig, but it is up to
> maintainers.
> 
> ...
> 
> > +# Siemens Simatic Industrial PCs
> > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC)      += simatic-ipc.o  
> 
> Ditto.

I will check both again if a find a pattern, otherwise will wait for
maintainers to complain and hopefully suggest.

> ...
> 
> > + * Siemens SIMATIC IPC driver for LEDs  
> 
> It seems to be used in patch 4 which is not LED related. Also, why is
> it here if it's for LEDs?
> 
> ...
> 
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundation.  
> 
> Replace with SPDX
> 
> ...
> 
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/platform_device.h>  
> 
> Ordered?
> 
> > +#include <linux/platform_data/x86/simatic-ipc.h>  
> 
> ...
> 
> > +static int register_platform_devices(u32 station_id)
> > +{
> > +       int i;
> > +       u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> > +       u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;  
> 
> Reversed xmas tree order?

I do not get this, it is almost easter egg order time. Please explain.

> > +       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));  
> 
> Strange indentation (second line).
> 
> > +               if (IS_ERR(ipc_led_platform_device))
> > +                       return PTR_ERR(ipc_led_platform_device);  
> 
> > +               pr_debug(KBUILD_MODNAME ": device=%s created\n",
> > +                        ipc_led_platform_device->name);  
> 
> Utilize pr_fmt() instead of adding prefixes like this.
> 
> > +       }  
> 
> > +       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(KBUILD_MODNAME ": device=%s created\n",
> > +                        ipc_wdt_platform_device->name);
> > +       }  
> 
> Same comments as above.
> 
> > +       if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
> > +           wdtmode == SIMATIC_IPC_DEVICE_NONE) {
> > +               pr_warn(KBUILD_MODNAME
> > +                       ": unsupported IPC detected, station
> > id=%08x\n",
> > +                       station_id);  
> 
> Ugly indentation. With pr_fmt() in use will be much better.
> 
> > +               return -EINVAL;  
> 
> > +       } else {  
> 
> Redundant.
> 
> > +               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  
> 
> bar -> BAR
> Missed period.
> 
> > + */  
> 
> > +  
> 
> Unneeded blank line.
> 
> > +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> > +{
> > +       u32 bar0 = 0;  
> 
> > +#ifdef CONFIG_PCI  
> 
> It's ugly besides the fact that you have a dependency.

left over from out-of-tree, will be removed

rest is clear, Thanks!
Henning

> > +       struct pci_bus *bus;  
> 
> Missed blank line.
> 
> > +       /*
> > +        * 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();
> > +#endif /* CONFIG_PCI */
> > +       return bar0;
> > +}
> > +EXPORT_SYMBOL(simatic_ipc_get_membase0);  
> 
> Oy vey! I know what this is and let's do it differently. I have some
> (relatively old) patch series I can send you privately for testing.
> 
> ...
> 
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundation.  
> 
> Not needed when you have SPDX.
> 
> ...
> 
> > +#include <linux/pci.h>  
> 
> Wrong header. Should be types.h
> 
> ...
> 
> > +#include <linux/dmi.h>
> > +#include <linux/platform_data/x86/simatic-ipc-base.h>  
> 
> Missed headers. You need to include ones that the code below is a
> direct user of.
> 
> Like types.h
> 


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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-04 13:47     ` Hans de Goede
@ 2021-03-05 15:42       ` Andy Shevchenko
  2021-03-05 16:14         ` Hans de Goede
  2021-03-05 16:42         ` Henning Schild
  0 siblings, 2 replies; 46+ messages in thread
From: Andy Shevchenko @ 2021-03-05 15:42 UTC (permalink / raw)
  To: Hans de Goede
  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,
	Pavel Machek

On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 3/4/21 11:11 AM, Andy Shevchenko wrote:
> > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> > <henning.schild@siemens.com> wrote:

...

> >> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> >> +{
> >> +       u32 bar0 = 0;
> >
> >> +#ifdef CONFIG_PCI
> >
> > It's ugly besides the fact that you have a dependency.
> >
> >> +       struct pci_bus *bus;
> >
> > Missed blank line.
> >
> >> +       /*
> >> +        * 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();
> >> +#endif /* CONFIG_PCI */
> >> +       return bar0;
> >> +}
> >> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
> >
> > Oy vey! I know what this is and let's do it differently. I have some
> > (relatively old) patch series I can send you privately for testing.
>
> This bit stood out the most to me too, it would be good if we can this fixed
> in some cleaner work. So I'm curious how things will look with Andy's work
> integrated.
>
> Also I don't think this should be exported. Instead this (or its replacement)
> should be used to get the address for an IOMEM resource to add the platform
> devices when they are instantiated. Then the platform-dev drivers can just
> use the regular functions to get their resources instead of relying on this
> module.

I have published a WIP branch [1]. I have no means to test (I don't
know what hardware at hand I can use right now), but I made it compile
after 4 years of gathering dust...
Feel free to give any kind of comments or share your ideas on how it
can be improved (the above idea on IOMEM resource is interesting, but
devices are PCI, not sure how this can be done).

[1]: https://gitlab.com/andy-shev/next/-/tree/p2sb

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-04 19:42     ` Henning Schild
@ 2021-03-05 15:46       ` Andy Shevchenko
  2021-03-05 16:31         ` Henning Schild
  0 siblings, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2021-03-05 15:46 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 Thu, Mar 4, 2021 at 9:52 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Thu, 4 Mar 2021 12:11:12 +0200
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> > <henning.schild@siemens.com> wrote:

...

> > Check for the rest of the series as well (basically this is the rule
> > of thumb to recheck entire code for the comment you have got at any
> > single place of it).
>
> For some code Gerd is the author, and i am the co-Author. We even have
> Jan in the mix at places. At least in copyright headers.
>
> I will remain the committer for the three of us. And since i do not
> know exactly what the problem is i will add only my Signed-off to avoid
> confusion.
>
> Please speak up it that would be wrong as well and point me to the docs
> i missed. Or tell me how it should be done.

Then make sure that you have From line with the Author (`git commit
--amend --author="..."`) and add your Co-developed-by tag.

...

> > > +       int i;
> > > +       u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> > > +       u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> >
> > Reversed xmas tree order?
>
> I do not get this, it is almost easter egg order time. Please explain.

Longer lines
usually go
first.

See above :-)

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-05 15:42       ` Andy Shevchenko
@ 2021-03-05 16:14         ` Hans de Goede
  2021-03-05 16:25           ` Andy Shevchenko
  2021-03-05 16:42         ` Henning Schild
  1 sibling, 1 reply; 46+ messages in thread
From: Hans de Goede @ 2021-03-05 16:14 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,
	Pavel Machek

Hi,

On 3/5/21 4:42 PM, Andy Shevchenko wrote:
> On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 3/4/21 11:11 AM, Andy Shevchenko wrote:
>>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
>>> <henning.schild@siemens.com> wrote:
> 
> ...
> 
>>>> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
>>>> +{
>>>> +       u32 bar0 = 0;
>>>
>>>> +#ifdef CONFIG_PCI
>>>
>>> It's ugly besides the fact that you have a dependency.
>>>
>>>> +       struct pci_bus *bus;
>>>
>>> Missed blank line.
>>>
>>>> +       /*
>>>> +        * 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();
>>>> +#endif /* CONFIG_PCI */
>>>> +       return bar0;
>>>> +}
>>>> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
>>>
>>> Oy vey! I know what this is and let's do it differently. I have some
>>> (relatively old) patch series I can send you privately for testing.
>>
>> This bit stood out the most to me too, it would be good if we can this fixed
>> in some cleaner work. So I'm curious how things will look with Andy's work
>> integrated.
>>
>> Also I don't think this should be exported. Instead this (or its replacement)
>> should be used to get the address for an IOMEM resource to add the platform
>> devices when they are instantiated. Then the platform-dev drivers can just
>> use the regular functions to get their resources instead of relying on this
>> module.
> 
> I have published a WIP branch [1]. I have no means to test (I don't
> know what hardware at hand I can use right now), but I made it compile
> after 4 years of gathering dust...

So I took a quick look at the following 2 commits:

"platform/x86: p2sb: New Primary to Sideband bridge support library"
"mfd: lpc_ich: Switch to generic p2sb_bar()"

And this looks good to me, although compared to the code from this
patch-set you are missing the pci_lock_rescan_remove(); and
pci_unlock_rescan_remove(); calls.

> Feel free to give any kind of comments or share your ideas on how it
> can be improved (the above idea on IOMEM resource is interesting, but
> devices are PCI, not sure how this can be done).

The code added by this patch introduces a register_platform_devices()
function which creates a bunch of platform-devices; and then the
device-drivers for those call simatic_ipc_get_membase0() to get their
base-address.

My suggestion was to instead put the  simatic_ipc_get_membase0() call
inside the code instantiating the platform devices and to add the
base-address for that pdev as IOMEM resource to the instantiated
platform-devices.

I hope this helps to clarify what I was trying to say.

Regards,

Hans


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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-05 16:14         ` Hans de Goede
@ 2021-03-05 16:25           ` Andy Shevchenko
  2021-03-05 16:36             ` Hans de Goede
  2021-03-05 16:41             ` Andy Shevchenko
  0 siblings, 2 replies; 46+ messages in thread
From: Andy Shevchenko @ 2021-03-05 16:25 UTC (permalink / raw)
  To: Hans de Goede
  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,
	Pavel Machek

On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
> On 3/5/21 4:42 PM, Andy Shevchenko wrote:
> > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
> >> On 3/4/21 11:11 AM, Andy Shevchenko wrote:
> >>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> >>> <henning.schild@siemens.com> wrote:

...

> >>> Oy vey! I know what this is and let's do it differently. I have some
> >>> (relatively old) patch series I can send you privately for testing.
> >>
> >> This bit stood out the most to me too, it would be good if we can this fixed
> >> in some cleaner work. So I'm curious how things will look with Andy's work
> >> integrated.
> >>
> >> Also I don't think this should be exported. Instead this (or its replacement)
> >> should be used to get the address for an IOMEM resource to add the platform
> >> devices when they are instantiated. Then the platform-dev drivers can just
> >> use the regular functions to get their resources instead of relying on this
> >> module.
> >
> > I have published a WIP branch [1]. I have no means to test (I don't
> > know what hardware at hand I can use right now), but I made it compile
> > after 4 years of gathering dust...
>
> So I took a quick look at the following 2 commits:

(One of the latter commits moves the code to drivers/pci/pci-p2sb.c,
do you think it's better like that? The idea is to deduplicate
__pci_bus_read_base() call)

> "platform/x86: p2sb: New Primary to Sideband bridge support library"
> "mfd: lpc_ich: Switch to generic p2sb_bar()"
>
> And this looks good to me, although compared to the code from this
> patch-set you are missing the pci_lock_rescan_remove(); and
> pci_unlock_rescan_remove(); calls.

Oh, indeed.

> > Feel free to give any kind of comments or share your ideas on how it
> > can be improved (the above idea on IOMEM resource is interesting, but
> > devices are PCI, not sure how this can be done).
>
> The code added by this patch introduces a register_platform_devices()
> function which creates a bunch of platform-devices; and then the
> device-drivers for those call simatic_ipc_get_membase0() to get their
> base-address.

Sounds like an MFD approach...

> My suggestion was to instead put the  simatic_ipc_get_membase0() call
> inside the code instantiating the platform devices and to add the
> base-address for that pdev as IOMEM resource to the instantiated
> platform-devices.
>
> I hope this helps to clarify what I was trying to say.

Yes, thanks!

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-05 15:46       ` Andy Shevchenko
@ 2021-03-05 16:31         ` Henning Schild
  0 siblings, 0 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-05 16:31 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 Fri, 5 Mar 2021 17:46:08 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Mar 4, 2021 at 9:52 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Thu, 4 Mar 2021 12:11:12 +0200
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> 
> ...
> 
> > > Check for the rest of the series as well (basically this is the
> > > rule of thumb to recheck entire code for the comment you have got
> > > at any single place of it).  
> >
> > For some code Gerd is the author, and i am the co-Author. We even
> > have Jan in the mix at places. At least in copyright headers.
> >
> > I will remain the committer for the three of us. And since i do not
> > know exactly what the problem is i will add only my Signed-off to
> > avoid confusion.
> >
> > Please speak up it that would be wrong as well and point me to the
> > docs i missed. Or tell me how it should be done.  
> 
> Then make sure that you have From line with the Author (`git commit
> --amend --author="..."`) and add your Co-developed-by tag.
> 
> ...
> 
> > > > +       int i;
> > > > +       u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> > > > +       u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;  
> > >
> > > Reversed xmas tree order?  
> >
> > I do not get this, it is almost easter egg order time. Please
> > explain.  
> 
> Longer lines
> usually go
> first.

Henning

see
i

> See above :-)
> 


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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-05 16:25           ` Andy Shevchenko
@ 2021-03-05 16:36             ` Hans de Goede
  2021-03-05 16:41             ` Andy Shevchenko
  1 sibling, 0 replies; 46+ messages in thread
From: Hans de Goede @ 2021-03-05 16:36 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,
	Pavel Machek

Hi,

On 3/5/21 5:25 PM, Andy Shevchenko wrote:
> On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
>> On 3/5/21 4:42 PM, Andy Shevchenko wrote:
>>> On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com> wrote:
>>>> On 3/4/21 11:11 AM, Andy Shevchenko wrote:
>>>>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
>>>>> <henning.schild@siemens.com> wrote:
> 
> ...
> 
>>>>> Oy vey! I know what this is and let's do it differently. I have some
>>>>> (relatively old) patch series I can send you privately for testing.
>>>>
>>>> This bit stood out the most to me too, it would be good if we can this fixed
>>>> in some cleaner work. So I'm curious how things will look with Andy's work
>>>> integrated.
>>>>
>>>> Also I don't think this should be exported. Instead this (or its replacement)
>>>> should be used to get the address for an IOMEM resource to add the platform
>>>> devices when they are instantiated. Then the platform-dev drivers can just
>>>> use the regular functions to get their resources instead of relying on this
>>>> module.
>>>
>>> I have published a WIP branch [1]. I have no means to test (I don't
>>> know what hardware at hand I can use right now), but I made it compile
>>> after 4 years of gathering dust...
>>
>> So I took a quick look at the following 2 commits:
> 
> (One of the latter commits moves the code to drivers/pci/pci-p2sb.c,
> do you think it's better like that? The idea is to deduplicate
> __pci_bus_read_base() call)
> 
>> "platform/x86: p2sb: New Primary to Sideband bridge support library"
>> "mfd: lpc_ich: Switch to generic p2sb_bar()"
>>
>> And this looks good to me, although compared to the code from this
>> patch-set you are missing the pci_lock_rescan_remove(); and
>> pci_unlock_rescan_remove(); calls.
> 
> Oh, indeed.
> 
>>> Feel free to give any kind of comments or share your ideas on how it
>>> can be improved (the above idea on IOMEM resource is interesting, but
>>> devices are PCI, not sure how this can be done).
>>
>> The code added by this patch introduces a register_platform_devices()
>> function which creates a bunch of platform-devices; and then the
>> device-drivers for those call simatic_ipc_get_membase0() to get their
>> base-address.
> 
> Sounds like an MFD approach...

Yes except that there does not seem to be a clear parent for
these devices, so it is MFD-ish.

IOW I'm not sure this should be changed to use the MFD framework,
but I was thinking along those line myself too.

Henning did you look into using the MFD framework + MFD cell
descriptions to instantiate the platform-devices for you ?

Regards,

Hans


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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-05 16:25           ` Andy Shevchenko
  2021-03-05 16:36             ` Hans de Goede
@ 2021-03-05 16:41             ` Andy Shevchenko
  2021-03-05 16:47               ` Andy Shevchenko
  1 sibling, 1 reply; 46+ messages in thread
From: Andy Shevchenko @ 2021-03-05 16:41 UTC (permalink / raw)
  To: Hans de Goede
  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,
	Pavel Machek

On Fri, Mar 5, 2021 at 6:25 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > On 3/5/21 4:42 PM, Andy Shevchenko wrote:

...

> > So I took a quick look at the following 2 commits:
>
> (One of the latter commits moves the code to drivers/pci/pci-p2sb.c,
> do you think it's better like that? The idea is to deduplicate
> __pci_bus_read_base() call)
>
> > "platform/x86: p2sb: New Primary to Sideband bridge support library"
> > "mfd: lpc_ich: Switch to generic p2sb_bar()"
> >
> > And this looks good to me, although compared to the code from this
> > patch-set you are missing the pci_lock_rescan_remove(); and
> > pci_unlock_rescan_remove(); calls.
>
> Oh, indeed.

Correction here, I'm using pci_bus_sem in the latest version.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-05 15:42       ` Andy Shevchenko
  2021-03-05 16:14         ` Hans de Goede
@ 2021-03-05 16:42         ` Henning Schild
  2021-03-05 17:17           ` Andy Shevchenko
  1 sibling, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-05 16:42 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Hans de Goede, Linux Kernel Mailing List, Linux LED Subsystem,
	Platform Driver, linux-watchdog, Srikanth Krishnakar, Jan Kiszka,
	Gerd Haeussler, Guenter Roeck, Wim Van Sebroeck, Mark Gross,
	Pavel Machek

Am Fri, 5 Mar 2021 17:42:42 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com>
> wrote:
> > On 3/4/21 11:11 AM, Andy Shevchenko wrote:  
> > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> > > <henning.schild@siemens.com> wrote:  
> 
> ...
> 
> > >> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> > >> +{
> > >> +       u32 bar0 = 0;  
> > >  
> > >> +#ifdef CONFIG_PCI  
> > >
> > > It's ugly besides the fact that you have a dependency.
> > >  
> > >> +       struct pci_bus *bus;  
> > >
> > > Missed blank line.
> > >  
> > >> +       /*
> > >> +        * 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();
> > >> +#endif /* CONFIG_PCI */
> > >> +       return bar0;
> > >> +}
> > >> +EXPORT_SYMBOL(simatic_ipc_get_membase0);  
> > >
> > > Oy vey! I know what this is and let's do it differently. I have
> > > some (relatively old) patch series I can send you privately for
> > > testing.  
> >
> > This bit stood out the most to me too, it would be good if we can
> > this fixed in some cleaner work. So I'm curious how things will
> > look with Andy's work integrated.
> >
> > Also I don't think this should be exported. Instead this (or its
> > replacement) should be used to get the address for an IOMEM
> > resource to add the platform devices when they are instantiated.
> > Then the platform-dev drivers can just use the regular functions to
> > get their resources instead of relying on this module.  
> 
> I have published a WIP branch [1]. I have no means to test (I don't
> know what hardware at hand I can use right now), but I made it compile
> after 4 years of gathering dust...
> Feel free to give any kind of comments or share your ideas on how it
> can be improved (the above idea on IOMEM resource is interesting, but
> devices are PCI, not sure how this can be done).
> 
> [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb

That is a little weird, might be a good idea to RFC reply to the cover
letter of this one. To allow review and discussion in a central place.

Henning

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

* Re: [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices
  2021-03-05 16:41             ` Andy Shevchenko
@ 2021-03-05 16:47               ` Andy Shevchenko
  0 siblings, 0 replies; 46+ messages in thread
From: Andy Shevchenko @ 2021-03-05 16:47 UTC (permalink / raw)
  To: Hans de Goede
  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,
	Pavel Machek

On Fri, Mar 5, 2021 at 6:41 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Mar 5, 2021 at 6:25 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <hdegoede@redhat.com> wrote:
> > > On 3/5/21 4:42 PM, Andy Shevchenko wrote:
>
> ...
>
> > > So I took a quick look at the following 2 commits:
> >
> > (One of the latter commits moves the code to drivers/pci/pci-p2sb.c,
> > do you think it's better like that? The idea is to deduplicate
> > __pci_bus_read_base() call)
> >
> > > "platform/x86: p2sb: New Primary to Sideband bridge support library"
> > > "mfd: lpc_ich: Switch to generic p2sb_bar()"
> > >
> > > And this looks good to me, although compared to the code from this
> > > patch-set you are missing the pci_lock_rescan_remove(); and
> > > pci_unlock_rescan_remove(); calls.
> >
> > Oh, indeed.
>
> Correction here, I'm using pci_bus_sem in the latest version.

Okay, it seems that semaphore is not what I need and PCI rescan lock
is what is mandatory to take here.


-- 
With Best Regards,
Andy Shevchenko

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

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

On Fri, Mar 5, 2021 at 6:47 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Fri, 5 Mar 2021 17:42:42 +0200
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com>
> > wrote:

...

> > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
>
> That is a little weird, might be a good idea to RFC reply to the cover
> letter of this one. To allow review and discussion in a central place.

I'm now rebasing it to be more presentable.
If you can test this approach and it works for you, I'll send a formal
RFC series.

-- 
With Best Regards,
Andy Shevchenko

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

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

On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
>
> On Fri, Mar 5, 2021 at 6:47 PM Henning Schild
> <henning.schild@siemens.com> wrote:
> > Am Fri, 5 Mar 2021 17:42:42 +0200
> > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <hdegoede@redhat.com>
> > > wrote:
>
> ...
>
> > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
> >
> > That is a little weird, might be a good idea to RFC reply to the cover
> > letter of this one. To allow review and discussion in a central place.
>
> I'm now rebasing it to be more presentable.
> If you can test this approach and it works for you, I'll send a formal
> RFC series.

Okay, [1] now is in presentable shape, each patch with a proper commit
message and authorship, also all patches are compiled without issues.

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-03 20:56           ` Henning Schild
@ 2021-03-05 18:25             ` Henning Schild
  2021-03-06 12:54               ` Henning Schild
  2021-03-15 11:41               ` Pavel Machek
  0 siblings, 2 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-05 18:25 UTC (permalink / raw)
  To: Pavel Machek
  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

Am Wed, 3 Mar 2021 21:56:15 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Wed, 3 Mar 2021 21:48:21 +0100
> schrieb Henning Schild <henning.schild@siemens.com>:
> 
> > Am Wed, 3 Mar 2021 20:31:34 +0100
> > schrieb Pavel Machek <pavel@ucw.cz>:
> >   
> > > Hi!
> > >     
> > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > > > > +	{1 << 15, "simatic-ipc:green:run-stop"},
> > > > > > +	{1 << 7,  "simatic-ipc:yellow:run-stop"},
> > > > > > +	{1 << 14, "simatic-ipc:red:error"},
> > > > > > +	{1 << 6,  "simatic-ipc:yellow:error"},
> > > > > > +	{1 << 13, "simatic-ipc:red:maint"},
> > > > > > +	{1 << 5,  "simatic-ipc:yellow:maint"},
> > > > > > +	{0, ""},
> > > > > > +};        
> > > > > 
> > > > > Please use names consistent with other systems, this is user
> > > > > visible. If you have two-color power led, it should be
> > > > > :green:power... See include/dt-bindings/leds/common.h .      
> > > > 
> > > > Well we wanted to pick names that are printed on the devices and
> > > > would like to stick to those. Has been a discussion ...
> > > > Can we have symlinks to have multiple names per LED?      
> > > 
> > > No symlinks. We plan to have command line tool to manipulate LEDs,
> > > aliases might be possible there.    
> > 
> > Sounds like a future plan. sysfs and "cat" "echo" are mighty tools
> > and "everything is a file" is the best idea ever. So i would say any
> > aliasing should live in the kernel, but that is just me. Tools will
> > just get out of sync, be missing in busybox or a random yocto ... or
> > whichever distro you like.
> > On the other hand you have "complexity should be userland" ... i do
> > not have the answer.  
> 
> My personal horror would be systemd-ledd or some dracut snipet for
> initrd. But that would be a generic led class discussion ... that
> tool.
> 
> > > > How strong would you feel about us using our names?      
> > > 
> > > Strongly. :-)    
> > 
> > OK, will try to find a match where possible.   
> 
> Do we happen to have a description of the existing names, to find a
> fit for ours? In the header you pointed out i only found names without
> "meaning"

I had a closer look at the several LED_FUNCTION_ while i could probably
find a match for the names we had in mind ...

-       {1 << 14, "simatic-ipc:red:error"},
+       {1 << 14, "simatic-ipc:red:" LED_FUNCTION_FAULT },

I still do not understand what those mean. Going over the kernel
sources many have only one single grep-hit in the tree.
LED_FUNCTION_ not having a single one in drivers/leds
Others are found in one dts and in that header ... 2 hits in the tree,
maybe i should add my favorite strings ;)

LED_FUNCTION_FLASH vs LED_FUNCTION_TORCH ...? Sound like timing, not
function.

Let us say i match the three "error", "run-stop", "maint" to
LED_FUNCTION_*

I would have a really hard time finding matches for other LEDs i did
not even propose. One example being disks ... many of them, would i be
allowed to 

LED_FUNCTION_DISK "0"
LED_FUNCTION_DISK "1"
...

they would all have the same colors.

Maybe you explain the idea behind choosing only from that namespace? My
guess would be high-level software being able to toggle leds totally
indep of the device it runs on. Such software would have to do some
really nasty directory listing, name parsing, dealing with multiple
hits. Does such generic software already exist, maybe that would help
me understand my "mapping problems" ?

The current class encodes, color, function and name into "name".

Maybe i am all wrong and should go for

{1 << 14, "simatic-ipc-error:red:" LED_FUNCTION_STATUS }
{1 << 15, "simatic-ipc-run-stop:green:" LED_FUNCTION_STATUS}
{...    , "simatic-ipc-hdd0:red:" LED_FUNCTION_DISK }
{...    , "simatic-ipc-hdd1:red:" LED_FUNCTION_DISK }

so appending my wanted name to the name before the first :, and use
functions i "understand" after the second :

regards,
Henning


> regards,
> Henning
> 
> >   
> > > Do you have a picture how the leds look like?    
> > 
> > I could even find chassis photos in our internal review but that
> > would be too much.
> > 
> > Our idea is probably the same as yours. We want the same names
> > across all devices. But we struggle with colors because on some
> > boxes we have red+green, while other offer yellow ... implemented
> > in HW and messing with red+green in some cases.
> > 
> > But so far we only looked at Siemens devices and thought we could
> > get our own "namespace".
> > 
> > To be honest i could not even tell how our names map on the known
> > ones, but we will do our best to find a match. They all are
> > "high-level" so "power" and other basic things are not exposed.
> > 
> > regards,
> > Henning
> >    
> > > Best regards,
> > > 							Pavel    
> >   
> 


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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-05 18:25             ` Henning Schild
@ 2021-03-06 12:54               ` Henning Schild
  2021-03-06 13:06                 ` Henning Schild
  2021-03-15 11:41               ` Pavel Machek
  1 sibling, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-06 12:54 UTC (permalink / raw)
  To: Pavel Machek
  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

Am Fri, 5 Mar 2021 19:25:55 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Wed, 3 Mar 2021 21:56:15 +0100
> schrieb Henning Schild <henning.schild@siemens.com>:
> 
> > Am Wed, 3 Mar 2021 21:48:21 +0100
> > schrieb Henning Schild <henning.schild@siemens.com>:
> >   
> > > Am Wed, 3 Mar 2021 20:31:34 +0100
> > > schrieb Pavel Machek <pavel@ucw.cz>:
> > >     
> > > > Hi!
> > > >       
> > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > > > > > +	{1 << 15, "simatic-ipc:green:run-stop"},
> > > > > > > +	{1 << 7,  "simatic-ipc:yellow:run-stop"},
> > > > > > > +	{1 << 14, "simatic-ipc:red:error"},
> > > > > > > +	{1 << 6,  "simatic-ipc:yellow:error"},
> > > > > > > +	{1 << 13, "simatic-ipc:red:maint"},
> > > > > > > +	{1 << 5,  "simatic-ipc:yellow:maint"},
> > > > > > > +	{0, ""},
> > > > > > > +};          
> > > > > > 
> > > > > > Please use names consistent with other systems, this is user
> > > > > > visible. If you have two-color power led, it should be
> > > > > > :green:power... See include/dt-bindings/leds/common.h .
> > > > > >    
> > > > > 
> > > > > Well we wanted to pick names that are printed on the devices
> > > > > and would like to stick to those. Has been a discussion ...
> > > > > Can we have symlinks to have multiple names per LED?        
> > > > 
> > > > No symlinks. We plan to have command line tool to manipulate
> > > > LEDs, aliases might be possible there.      
> > > 
> > > Sounds like a future plan. sysfs and "cat" "echo" are mighty tools
> > > and "everything is a file" is the best idea ever. So i would say
> > > any aliasing should live in the kernel, but that is just me.
> > > Tools will just get out of sync, be missing in busybox or a
> > > random yocto ... or whichever distro you like.
> > > On the other hand you have "complexity should be userland" ... i
> > > do not have the answer.    
> > 
> > My personal horror would be systemd-ledd or some dracut snipet for
> > initrd. But that would be a generic led class discussion ... that
> > tool.
> >   
> > > > > How strong would you feel about us using our names?        
> > > > 
> > > > Strongly. :-)      
> > > 
> > > OK, will try to find a match where possible.     
> > 
> > Do we happen to have a description of the existing names, to find a
> > fit for ours? In the header you pointed out i only found names
> > without "meaning"  
> 
> I had a closer look at the several LED_FUNCTION_ while i could
> probably find a match for the names we had in mind ...
> 
> -       {1 << 14, "simatic-ipc:red:error"},
> +       {1 << 14, "simatic-ipc:red:" LED_FUNCTION_FAULT },
> 
> I still do not understand what those mean. Going over the kernel
> sources many have only one single grep-hit in the tree.
> LED_FUNCTION_ not having a single one in drivers/leds
> Others are found in one dts and in that header ... 2 hits in the tree,
> maybe i should add my favorite strings ;)
> 
> LED_FUNCTION_FLASH vs LED_FUNCTION_TORCH ...? Sound like timing, not
> function.
> 
> Let us say i match the three "error", "run-stop", "maint" to
> LED_FUNCTION_*
> 
> I would have a really hard time finding matches for other LEDs i did
> not even propose. One example being disks ... many of them, would i be
> allowed to 
> 
> LED_FUNCTION_DISK "0"
> LED_FUNCTION_DISK "1"
> ...
> 
> they would all have the same colors.
> 
> Maybe you explain the idea behind choosing only from that namespace?
> My guess would be high-level software being able to toggle leds
> totally indep of the device it runs on. Such software would have to
> do some really nasty directory listing, name parsing, dealing with
> multiple hits. Does such generic software already exist, maybe that
> would help me understand my "mapping problems" ?
> 
> The current class encodes, color, function and name into "name".
> 
> Maybe i am all wrong and should go for
> 
> {1 << 14, "simatic-ipc-error:red:" LED_FUNCTION_STATUS }
> {1 << 15, "simatic-ipc-run-stop:green:" LED_FUNCTION_STATUS}
> {...    , "simatic-ipc-hdd0:red:" LED_FUNCTION_DISK }
> {...    , "simatic-ipc-hdd1:red:" LED_FUNCTION_DISK }
> 
> so appending my wanted name to the name before the first :, and use
> functions i "understand" after the second :

Found the docs and the check script. It has been a while since i read
those docs.

But that script fails on bus=platform

quick workaround would be

        fi
+elif [ "$bus" = "platform" ]; then
+       true
 else
        echo "Unknown device type."
        exit 1

But i guess it would be nice to get some sort of platform information,
device vendor etc.

I see two options for pattern i could choose

"green:" LED_FUNCTION_STATUS "-0"
-> platform bus patch needed, no plaform information

simatic-ipc:green:" LED_FUNCTION_STATUS "-0"
-> platform bus patch needed, will fail with "Unknown devicename"

Without further advice i will choose the second for v2. That is also
what i.e. "tpacpi" on my laptop looks like.

I would also be happy to include a fix to that script. My suggestion
would be to allow bus=platform, in which case a "devicename" will be
required and is allowed to have any value.

regards,
Henning

> regards,
> Henning
> 
> 
> > regards,
> > Henning
> >   
> > >     
> > > > Do you have a picture how the leds look like?      
> > > 
> > > I could even find chassis photos in our internal review but that
> > > would be too much.
> > > 
> > > Our idea is probably the same as yours. We want the same names
> > > across all devices. But we struggle with colors because on some
> > > boxes we have red+green, while other offer yellow ... implemented
> > > in HW and messing with red+green in some cases.
> > > 
> > > But so far we only looked at Siemens devices and thought we could
> > > get our own "namespace".
> > > 
> > > To be honest i could not even tell how our names map on the known
> > > ones, but we will do our best to find a match. They all are
> > > "high-level" so "power" and other basic things are not exposed.
> > > 
> > > regards,
> > > Henning
> > >      
> > > > Best regards,
> > > > 							Pavel
> > > >    
> > >     
> >   
> 


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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-06 12:54               ` Henning Schild
@ 2021-03-06 13:06                 ` Henning Schild
  2021-03-15 11:49                   ` Pavel Machek
  0 siblings, 1 reply; 46+ messages in thread
From: Henning Schild @ 2021-03-06 13:06 UTC (permalink / raw)
  To: Pavel Machek
  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

Am Sat, 6 Mar 2021 13:54:53 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Am Fri, 5 Mar 2021 19:25:55 +0100
> schrieb Henning Schild <henning.schild@siemens.com>:
> 
> > Am Wed, 3 Mar 2021 21:56:15 +0100
> > schrieb Henning Schild <henning.schild@siemens.com>:
> >   
> > > Am Wed, 3 Mar 2021 21:48:21 +0100
> > > schrieb Henning Schild <henning.schild@siemens.com>:
> > >     
> > > > Am Wed, 3 Mar 2021 20:31:34 +0100
> > > > schrieb Pavel Machek <pavel@ucw.cz>:
> > > >       
> > > > > Hi!
> > > > >         
> > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > > > > > > +	{1 << 15, "simatic-ipc:green:run-stop"},
> > > > > > > > +	{1 << 7,  "simatic-ipc:yellow:run-stop"},
> > > > > > > > +	{1 << 14, "simatic-ipc:red:error"},
> > > > > > > > +	{1 << 6,  "simatic-ipc:yellow:error"},
> > > > > > > > +	{1 << 13, "simatic-ipc:red:maint"},
> > > > > > > > +	{1 << 5,  "simatic-ipc:yellow:maint"},
> > > > > > > > +	{0, ""},
> > > > > > > > +};            
> > > > > > > 
> > > > > > > Please use names consistent with other systems, this is
> > > > > > > user visible. If you have two-color power led, it should
> > > > > > > be :green:power... See include/dt-bindings/leds/common.h .
> > > > > > >      
> > > > > > 
> > > > > > Well we wanted to pick names that are printed on the devices
> > > > > > and would like to stick to those. Has been a discussion ...
> > > > > > Can we have symlinks to have multiple names per LED?
> > > > > >   
> > > > > 
> > > > > No symlinks. We plan to have command line tool to manipulate
> > > > > LEDs, aliases might be possible there.        
> > > > 
> > > > Sounds like a future plan. sysfs and "cat" "echo" are mighty
> > > > tools and "everything is a file" is the best idea ever. So i
> > > > would say any aliasing should live in the kernel, but that is
> > > > just me. Tools will just get out of sync, be missing in busybox
> > > > or a random yocto ... or whichever distro you like.
> > > > On the other hand you have "complexity should be userland" ... i
> > > > do not have the answer.      
> > > 
> > > My personal horror would be systemd-ledd or some dracut snipet for
> > > initrd. But that would be a generic led class discussion ... that
> > > tool.
> > >     
> > > > > > How strong would you feel about us using our names?
> > > > > >  
> > > > > 
> > > > > Strongly. :-)        
> > > > 
> > > > OK, will try to find a match where possible.       
> > > 
> > > Do we happen to have a description of the existing names, to find
> > > a fit for ours? In the header you pointed out i only found names
> > > without "meaning"    
> > 
> > I had a closer look at the several LED_FUNCTION_ while i could
> > probably find a match for the names we had in mind ...
> > 
> > -       {1 << 14, "simatic-ipc:red:error"},
> > +       {1 << 14, "simatic-ipc:red:" LED_FUNCTION_FAULT },
> > 
> > I still do not understand what those mean. Going over the kernel
> > sources many have only one single grep-hit in the tree.
> > LED_FUNCTION_ not having a single one in drivers/leds
> > Others are found in one dts and in that header ... 2 hits in the
> > tree, maybe i should add my favorite strings ;)
> > 
> > LED_FUNCTION_FLASH vs LED_FUNCTION_TORCH ...? Sound like timing, not
> > function.
> > 
> > Let us say i match the three "error", "run-stop", "maint" to
> > LED_FUNCTION_*
> > 
> > I would have a really hard time finding matches for other LEDs i did
> > not even propose. One example being disks ... many of them, would i
> > be allowed to 
> > 
> > LED_FUNCTION_DISK "0"
> > LED_FUNCTION_DISK "1"
> > ...
> > 
> > they would all have the same colors.
> > 
> > Maybe you explain the idea behind choosing only from that namespace?
> > My guess would be high-level software being able to toggle leds
> > totally indep of the device it runs on. Such software would have to
> > do some really nasty directory listing, name parsing, dealing with
> > multiple hits. Does such generic software already exist, maybe that
> > would help me understand my "mapping problems" ?
> > 
> > The current class encodes, color, function and name into "name".
> > 
> > Maybe i am all wrong and should go for
> > 
> > {1 << 14, "simatic-ipc-error:red:" LED_FUNCTION_STATUS }
> > {1 << 15, "simatic-ipc-run-stop:green:" LED_FUNCTION_STATUS}
> > {...    , "simatic-ipc-hdd0:red:" LED_FUNCTION_DISK }
> > {...    , "simatic-ipc-hdd1:red:" LED_FUNCTION_DISK }
> > 
> > so appending my wanted name to the name before the first :, and use
> > functions i "understand" after the second :  
> 
> Found the docs and the check script. It has been a while since i read
> those docs.
> 
> But that script fails on bus=platform
> 
> quick workaround would be
> 
>         fi
> +elif [ "$bus" = "platform" ]; then
> +       true
>  else
>         echo "Unknown device type."
>         exit 1
> 
> But i guess it would be nice to get some sort of platform information,
> device vendor etc.
> 
> I see two options for pattern i could choose
> 
> "green:" LED_FUNCTION_STATUS "-0"
> -> platform bus patch needed, no plaform information  
> 
> simatic-ipc:green:" LED_FUNCTION_STATUS "-0"
> -> platform bus patch needed, will fail with "Unknown devicename"  
> 
> Without further advice i will choose the second for v2. That is also
> what i.e. "tpacpi" on my laptop looks like.
> 
> I would also be happy to include a fix to that script. My suggestion
> would be to allow bus=platform, in which case a "devicename" will be
> required and is allowed to have any value.

Furthermore it might be good to catch that in the led core instead of
that script. Maybe warn() on dev registration when function/color/name
seem off. Could later become "return -EINVAL"

Henning

> regards,
> Henning
> 
> > regards,
> > Henning
> > 
> >   
> > > regards,
> > > Henning
> > >     
> > > >       
> > > > > Do you have a picture how the leds look like?        
> > > > 
> > > > I could even find chassis photos in our internal review but that
> > > > would be too much.
> > > > 
> > > > Our idea is probably the same as yours. We want the same names
> > > > across all devices. But we struggle with colors because on some
> > > > boxes we have red+green, while other offer yellow ...
> > > > implemented in HW and messing with red+green in some cases.
> > > > 
> > > > But so far we only looked at Siemens devices and thought we
> > > > could get our own "namespace".
> > > > 
> > > > To be honest i could not even tell how our names map on the
> > > > known ones, but we will do our best to find a match. They all
> > > > are "high-level" so "power" and other basic things are not
> > > > exposed.
> > > > 
> > > > regards,
> > > > Henning
> > > >        
> > > > > Best regards,
> > > > > 							Pavel
> > > > >      
> > > >       
> > >     
> >   
> 


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

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

Am Fri, 5 Mar 2021 19:44:57 +0200
schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:

> On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> >
> > On Fri, Mar 5, 2021 at 6:47 PM Henning Schild
> > <henning.schild@siemens.com> wrote:  
> > > Am Fri, 5 Mar 2021 17:42:42 +0200
> > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:  
> > > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede
> > > > <hdegoede@redhat.com> wrote:  
> >
> > ...
> >  
> > > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb  
> > >
> > > That is a little weird, might be a good idea to RFC reply to the
> > > cover letter of this one. To allow review and discussion in a
> > > central place.  
> >
> > I'm now rebasing it to be more presentable.
> > If you can test this approach and it works for you, I'll send a
> > formal RFC series.  
> 
> Okay, [1] now is in presentable shape, each patch with a proper commit
> message and authorship, also all patches are compiled without issues.

Thank you so much, i will base v2 on that and let you know how that
works.

regards,
Henning

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

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

On Mon, Mar 8, 2021 at 3:02 PM Henning Schild
<henning.schild@siemens.com> wrote:
> Am Fri, 5 Mar 2021 19:44:57 +0200
> schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko
> > <andy.shevchenko@gmail.com> wrote:
> > > On Fri, Mar 5, 2021 at 6:47 PM Henning Schild
> > > <henning.schild@siemens.com> wrote:
> > > > Am Fri, 5 Mar 2021 17:42:42 +0200
> > > > schrieb Andy Shevchenko <andy.shevchenko@gmail.com>:
> > > > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede
> > > > > <hdegoede@redhat.com> wrote:

...

> > > > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
> > > >
> > > > That is a little weird, might be a good idea to RFC reply to the
> > > > cover letter of this one. To allow review and discussion in a
> > > > central place.
> > >
> > > I'm now rebasing it to be more presentable.
> > > If you can test this approach and it works for you, I'll send a
> > > formal RFC series.
> >
> > Okay, [1] now is in presentable shape, each patch with a proper commit
> > message and authorship, also all patches are compiled without issues.
>
> Thank you so much, i will base v2 on that and let you know how that
> works.

I went ahead and submitted the series [2]. Feel free either to use the
last 7 patches from [1], or the series. In either case, if it works
for you I would expect the Tested-by tag given against _series_.
Thanks!
(Or comment there what is not working / needed for your case)

[2]: https://lore.kernel.org/linux-pci/20210308122020.57071-1-andriy.shevchenko@linux.intel.com/T/#t

-- 
With Best Regards,
Andy Shevchenko

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

* Re: [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
  2021-03-03 14:21     ` Henning Schild
  2021-03-03 14:49       ` Jan Kiszka
@ 2021-03-08 15:20       ` Henning Schild
  1 sibling, 0 replies; 46+ messages in thread
From: Henning Schild @ 2021-03-08 15:20 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

Am Wed, 3 Mar 2021 15:21:05 +0100
schrieb Henning Schild <henning.schild@siemens.com>:

> Hi, 
> 
> thanks for the fast and thorough review!
> 
> Am Tue, 2 Mar 2021 10:38:19 -0800
> schrieb Guenter Roeck <linux@roeck-us.net>:
> 
> > On 3/2/21 8:33 AM, Henning Schild wrote:  
> > > From: Henning Schild <henning.schild@siemens.com>
> > > 
> > > This driver adds initial support for several devices from Siemens.
> > > It is based on a platform driver introduced in an earlier commit.
> > > 
> > > Signed-off-by: Gerd Haeussler <gerd.haeussler.ext@siemens.com>
> > > Signed-off-by: Henning Schild <henning.schild@siemens.com>
> > > ---
> > >  drivers/watchdog/Kconfig           |  11 ++
> > >  drivers/watchdog/Makefile          |   1 +
> > >  drivers/watchdog/simatic-ipc-wdt.c | 305
> > > +++++++++++++++++++++++++++++ 3 files changed, 317 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..b5c8b7ceb404
> > > --- /dev/null
> > > +++ b/drivers/watchdog/simatic-ipc-wdt.c
> > > @@ -0,0 +1,305 @@
> > > +// 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>
> > > + *
> > > + * This program is free software; you can redistribute it and/or
> > > modify
> > > + * it under the terms of the GNU General Public License version 2
> > > as
> > > + * published by the Free Software Foundation.    
> > 
> > Covered by SPDX-License-Identifier
> >   
> > > + */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/init.h>
> > > +#include <linux/kernel.h>
> > > +#include <linux/module.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/errno.h>
> > > +#include <linux/watchdog.h>
> > > +#include <linux/ioport.h>
> > > +#include <linux/sizes.h>> +#include <linux/io.h>
> > > +#include <linux/platform_data/x86/simatic-ipc-base.h>    
> > 
> > Alphabetic order please
> >   
> > > +
> > > +#define WD_ENABLE_IOADR		0x62
> > > +#define WD_TRIGGER_IOADR	0x66
> > > +#define GPIO_COMMUNITY0_PORT_ID 0xaf
> > > +#define PAD_CFG_DW0_GPP_A_23	0x4b8    
> > 
> > Please increase indentation and spare another tab
> >   
> > > +#define SAFE_EN_N_427E		0x01
> > > +#define SAFE_EN_N_227E		0x04
> > > +#define WD_ENABLED		0x01
> > > +
> > > +#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 DEFINE_SPINLOCK(io_lock);	/* the lock for io
> > > operations */ +static struct watchdog_device wdd;
> > > +    
> > 
> > Having two variables named 'wdd' is confusing. Please chose another
> > name.
> >   
> > > +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 get_timeout_idx(u32 timeout)
> > > +{
> > > +	int i;
> > > +
> > > +	i = ARRAY_SIZE(wd_timeout_table) - 1;
> > > +	for (; i >= 0; i--) {
> > > +		if (timeout >= wd_timeout_table[i])
> > > +			break;
> > > +	}
> > > +
> > > +	return i;
> > > +}    
> > 
> > Please add a comment explaining why you don't use find_closest().  
> 
> Will not be a comment but we will switch to using this, thanks for
> pointing it out.
> 
> > > +
> > > +static int wd_start(struct watchdog_device *wdd)
> > > +{
> > > +	u8 regval;
> > > +
> > > +	spin_lock(&io_lock);
> > > +    
> > The watchdog subsystem already provides locking
> > since the watchdog device can only be opened once.
> > 
> > Why is the additional lock needed ?  
> 
> We had this under internal review and somehow came to the conclusion
> that we "might" need it. I think we will remove it or come back with
> reasons.
> 
> > > +	regval = inb(WD_ENABLE_IOADR);
> > > +	regval |= WD_ENABLED;
> > > +	outb(regval, WD_ENABLE_IOADR);
> > > +
> > > +	spin_unlock(&io_lock);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int wd_stop(struct watchdog_device *wdd)
> > > +{
> > > +	u8 regval;
> > > +
> > > +	spin_lock(&io_lock);
> > > +
> > > +	regval = inb(WD_ENABLE_IOADR);
> > > +	regval &= ~WD_ENABLED;
> > > +	outb(regval, WD_ENABLE_IOADR);
> > > +
> > > +	spin_unlock(&io_lock);
> > > +
> > > +	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) +{
> > > +	u8 regval;
> > > +	int timeout_idx = get_timeout_idx(t);
> > > +
> > > +	spin_lock(&io_lock);
> > > +
> > > +	regval = inb(WD_ENABLE_IOADR) & 0xc7;
> > > +	regval |= timeout_idx << 3;
> > > +	outb(regval, WD_ENABLE_IOADR);
> > > +
> > > +	spin_unlock(&io_lock);
> > > +	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_set_safe_en_n(u32 wdtmode, bool safe_en_n)
> > > +{
> > > +	u16 resetbit;
> > > +
> > > +	if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
> > > +		/* enable SAFE_EN_N on GP_STATUS_REG_227E */
> > > +		resetbit = inw(GP_STATUS_REG_227E);
> > > +		if (safe_en_n)
> > > +			resetbit &= ~SAFE_EN_N_227E;
> > > +		else
> > > +			resetbit |= SAFE_EN_N_227E;
> > > +
> > > +		outw(resetbit, GP_STATUS_REG_227E);
> > > +	} else {
> > > +		/* enable SAFE_EN_N on PCH D1600 */
> > > +		resetbit = ioread16(wd_reset_base_addr);
> > > +
> > > +		if (safe_en_n)
> > > +			resetbit &= ~SAFE_EN_N_427E;
> > > +		else
> > > +			resetbit |= SAFE_EN_N_427E;
> > > +
> > > +		iowrite16(resetbit, wd_reset_base_addr);
> > > +	}
> > > +}
> > > +
> > > +static int wd_setup(u32 wdtmode, bool safe_en_n)
> > > +{
> > > +	u8 regval;
> > > +	int timeout_idx = 0;    
> > 
> > Unnecessary initialization
> >   
> > > +	bool alarm_active;
> > > +
> > > +	timeout_idx = get_timeout_idx(TIMEOUT_DEF);
> > > +
> > > +	wd_set_safe_en_n(wdtmode, safe_en_n);
> > > +
> > > +	/* read wd register to determine alarm state */
> > > +	regval = inb(WD_ENABLE_IOADR);
> > > +	if (regval & 0x80) {
> > > +		pr_warn("Watchdog alarm active.\n");    
> > 
> > Why does that warrant a warning, and what does it mean ? The context
> > suggests that it means the previous reset was caused by the
> > watchdog, but that is not what the message says.
> >   
> > > +		regval = 0x82;	/* use only macro mode,
> > > reset alarm bit */
> > > +		alarm_active = true;
> > > +	} else {
> > > +		regval = 0x02;	/* use only macro mode */
> > > +		alarm_active = false;
> > > +	}    
> > 
> > Would it hurt to just always write 0x82 ?
> > 	alarm_active = regval & 0x80;
> > 	regval = 0x82 | timeout_idx << 3;
> > 
> > would be much simpler. Or, if you prefer,
> > 	alarm_active = !!(regval & 0x80);
> > 	regval = 0x82 | timeout_idx << 3;
> > 
> > Actually, regval isn't even needed in that case.
> > 	alarm_active = !!(regval & 0x80);
> > 	outb(0x82 | timeout_idx << 3, WD_ENABLE_IOADR);
> > 
> > 
> > Either case, please use defines for the bits. WD_ENABLED is already
> > defined, thus the other bits should be set using defines as well.
> >   
> > > +
> > > +	regval |= timeout_idx << 3;
> > > +	if (nowayout)
> > > +		regval |= WD_ENABLED;    
> > 
> > This is not the purpose of nowayout. nowayout prevents stopping
> > the watchdog after it has been started. It is not expected to start
> > the watchdog on boot.  
> 
> Thanks, that was misunderstood by the author, will fix.
> 
> > > +
> > > +	outb(regval, WD_ENABLE_IOADR);
> > > +
> > > +	return alarm_active;
> > > +}
> > > +
> > > +static int simatic_ipc_wdt_probe(struct platform_device *pdev)
> > > +{
> > > +	struct device *dev = &pdev->dev;
> > > +	int rc = 0;
> > > +	struct simatic_ipc_platform *plat =
> > > pdev->dev.platform_data;
> > > +	struct resource *res;
> > > +    
> > 
> > Is it guaranteed that the device will always be instantiated only
> > once ? If so, how it it guaranteed ?  
> 
> I suppose if anything did register two platform devices on the
> platform bus this might not be guaranteed. The assumption is that
> simatic-ipc will only ever register one, and the machines always have
> 0-1 "Siemens watchdogs" so at the moment there will never be a need
> for more than one.

Turns out the platform bus would not even allow registering the same
device twice. So it is guaranteed but nobody every attempting it in the
first place, and it never working in the second.

Henning

> 
> > Because if there are ever multiple instances the various static
> > variables will cause major trouble (which is why it is always better
> > to not use static variables).
> >   
> > > +	pr_debug(KBUILD_MODNAME ":%s(#%d) WDT mode: %d\n",
> > > +		 __func__, __LINE__, plat->devmode);
> > > +    
> > 
> > This is a platform device. Please use dev_ messages (dev_warn,
> > dev_dbg etc) throughout.
> >   
> > > +	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.info = &wdt_ident;
> > > +		wdd.ops = &wdt_ops;
> > > +		wdd.min_timeout = TIMEOUT_MIN;
> > > +		wdd.max_timeout = TIMEOUT_MAX;    
> > 
> > Why not use static initialization ?
> >   
> > > +		wdd.parent = NULL;    
> > 
> > parent should be the platform device.
> >   
> > > +		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 this is what prevents multiple registrations, it is too late: wdd
> > is already overwritten.  
> 
> As said, there will be no double-registration.
> 
> > > +
> > > +	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 -ENOMEM;    
> > 
> > 			return PTR_ERR(wd_reset_base_addr);
> >   
> > > +	}
> > > +
> > > +	wdd.bootstatus = wd_setup(plat->devmode, true);    
> > 
> > bootstatus does not report a boolean. This translates to
> > WDIOF_OVERHEAT which is almost certainly wrong.
> >   
> > > +	if (wdd.bootstatus)
> > > +		pr_warn(KBUILD_MODNAME ": last reboot caused by
> > > watchdog reset\n");    
> > 
> > Why two messages ?
> >   
> > > +
> > > +	watchdog_set_nowayout(&wdd, nowayout);
> > > +	watchdog_stop_on_reboot(&wdd);
> > > +
> > > +	rc = devm_watchdog_register_device(dev, &wdd);
> > > +    
> > Extra empty line not needed
> >   
> > > +	if (rc == 0)
> > > +		pr_debug("initialized. nowayout=%d\n",
> > > +			 nowayout);    
> > 
> > What is the value of this message (especially since there is no
> > message if there is an error) ?
> >   
> > > +
> > > +	return rc;
> > > +}
> > > +
> > > +static int simatic_ipc_wdt_remove(struct platform_device *pdev)
> > > +{
> > > +	struct simatic_ipc_platform *plat =
> > > pdev->dev.platform_data; +
> > > +	wd_setup(plat->devmode, false);    
> > 
> > This warrants an explanation. What is the point of updating the
> > timeout here ? And what does SAFE_EN actually do ?  
> 
> The idea was that module unloading should disable the watchdog, but
> that code will be removed and aligned with other watchdogs.
> 
> SAFE_EN is a second enable bit, if it is not set the watchdog will
> fire into the void. Pretty pointless will keep that always armed or
> set it always when toggling "enable".
> 
> All the other open points are pretty clear, and will all be dealt with
> in v2.
> 
> regards,
> Henning
> 
> > The watchdog is stopped on reboot, but this function won't
> > be called in that case, making this call even more questionable.
> > Please document what it does and why it is needed here (but not
> > when rebooting).
> >   
> > > +	return 0;
> > > +}
> > > +
> > > +static struct platform_driver wdt_driver = {
> > > +	.probe = simatic_ipc_wdt_probe,
> > > +	.remove = simatic_ipc_wdt_remove,
> > > +	.driver = {
> > > +		.name = KBUILD_MODNAME,
> > > +	},
> > > +};
> > > +
> > > +static int __init simatic_ipc_wdt_init(void)
> > > +{
> > > +	return platform_driver_register(&wdt_driver);
> > > +}
> > > +
> > > +static void __exit simatic_ipc_wdt_exit(void)
> > > +{
> > > +	platform_driver_unregister(&wdt_driver);
> > > +}
> > > +
> > > +module_init(simatic_ipc_wdt_init);
> > > +module_exit(simatic_ipc_wdt_exit);    
> > 
> > Why not module_platform_driver() ?
> >   
> > > +
> > > +MODULE_LICENSE("GPL");
> > > +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> > > +MODULE_AUTHOR("Gerd Haeussler <gerd.haeussler.ext@siemens.com>");
> > >     
> >   
> 


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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-05 18:25             ` Henning Schild
  2021-03-06 12:54               ` Henning Schild
@ 2021-03-15 11:41               ` Pavel Machek
  1 sibling, 0 replies; 46+ messages in thread
From: Pavel Machek @ 2021-03-15 11:41 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, Hans de Goede


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

Hi!

> Maybe you explain the idea behind choosing only from that namespace? My
> guess would be high-level software being able to toggle leds totally
> indep of the device it runs on. Such software would have to do some
> really nasty directory listing, name parsing, dealing with multiple
> hits. Does such generic software already exist, maybe that would help
> me understand my "mapping problems" ?

It does not, but we want to have one... or at least not have current situation.

> The current class encodes, color, function and name into "name".
> 
> Maybe i am all wrong and should go for
> 
> {1 << 14, "simatic-ipc-error:red:" LED_FUNCTION_STATUS }
> {1 << 15, "simatic-ipc-run-stop:green:" LED_FUNCTION_STATUS}
> {...    , "simatic-ipc-hdd0:red:" LED_FUNCTION_DISK }
> {...    , "simatic-ipc-hdd1:red:" LED_FUNCTION_DISK }

Can you explain in plain english what the leds should do?

We don't want to have simatic-ipc- prefix there. tpacpi was really bad
example.

Best regards,
								Pavel

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

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

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

* Re: [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
  2021-03-06 13:06                 ` Henning Schild
@ 2021-03-15 11:49                   ` Pavel Machek
  0 siblings, 0 replies; 46+ messages in thread
From: Pavel Machek @ 2021-03-15 11:49 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, Hans de Goede


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

Hi!

> > I would also be happy to include a fix to that script. My suggestion
> > would be to allow bus=platform, in which case a "devicename" will be
> > required and is allowed to have any value.
> 
> Furthermore it might be good to catch that in the led core instead of
> that script. Maybe warn() on dev registration when function/color/name
> seem off. Could later become "return -EINVAL"

I'm not sure if we want to change _existing_ funny names.

Would document such as below be helpful?

Could you describe the LEDs you have in similar format?

Best regards,
								Pavel

-*- org -*-

It is somehow important to provide consistent interface to the
userland. LED devices have one problem there, and that is naming of
directories in /sys/class/leds. It would be nice if userland would
just know right "name" for given LED function, but situation got more
complex.

Anyway, if backwards compatibility is not an issue, new code should
use one of the "good" names from this list, and you should extend the
list where applicable.

Bad names are listed, too; in case you are writing application that
wants to use particular feature, you should probe for good name, first,
but then try the bad ones, too.

* Keyboards
  
Good: "input*:*:capslock"
Good: "input*:*:scrolllock"
Good: "input*:*:numlock"
Bad: "shift-key-light" (Motorola Droid 4, capslock)

Set of common keyboard LEDs, going back to PC AT or so.

Good: "platform::kbd_backlight"
Bad: "tpacpi::thinklight" (IBM/Lenovo Thinkpads)
Bad: "lp5523:kb{1,2,3,4,5,6}" (Nokia N900)

Frontlight/backlight of main keyboard.

Bad: "button-backlight" (Motorola Droid 4)

Some phones have touch buttons below screen; it is different from main
keyboard. And this is their backlight.

* Sound subsystem

Good: "platform:*:mute"
Good: "platform:*:micmute"

LEDs on notebook body, indicating that sound input / output is muted.

* System notification

Good: "status-led:{red,green,blue}" (Motorola Droid 4)
Bad: "lp5523:{r,g,b}" (Nokia N900)

Phones usually have multi-color status LED.

* Power management

Good: "platform:*:charging" (allwinner sun50i)

* Screen

Good: ":backlight" (Motorola Droid 4)


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

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

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

end of thread, back to index

Thread overview: 46+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-02 16:33 [PATCH 0/4] add device drivers for Siemens Industrial PCs Henning Schild
2021-03-02 16:33 ` [PATCH 1/4] platform/x86: simatic-ipc: add main driver for Siemens devices Henning Schild
2021-03-04 10:11   ` Andy Shevchenko
2021-03-04 13:47     ` Hans de Goede
2021-03-05 15:42       ` Andy Shevchenko
2021-03-05 16:14         ` Hans de Goede
2021-03-05 16:25           ` Andy Shevchenko
2021-03-05 16:36             ` Hans de Goede
2021-03-05 16:41             ` Andy Shevchenko
2021-03-05 16:47               ` Andy Shevchenko
2021-03-05 16:42         ` Henning Schild
2021-03-05 17:17           ` Andy Shevchenko
2021-03-05 17:44             ` Andy Shevchenko
2021-03-08 12:57               ` Henning Schild
2021-03-08 13:43                 ` Andy Shevchenko
2021-03-04 19:42     ` Henning Schild
2021-03-05 15:46       ` Andy Shevchenko
2021-03-05 16:31         ` Henning Schild
2021-03-04 14:03   ` Hans de Goede
2021-03-02 16:33 ` [PATCH 2/4] leds: simatic-ipc-leds: add new driver for Siemens Industial PCs Henning Schild
2021-03-02 20:54   ` Pavel Machek
2021-03-03 13:11     ` Henning Schild
2021-03-03 19:31       ` Pavel Machek
2021-03-03 20:48         ` Henning Schild
2021-03-03 20:56           ` Henning Schild
2021-03-05 18:25             ` Henning Schild
2021-03-06 12:54               ` Henning Schild
2021-03-06 13:06                 ` Henning Schild
2021-03-15 11:49                   ` Pavel Machek
2021-03-15 11:41               ` Pavel Machek
2021-03-03 17:37     ` Henning Schild
2021-03-03 17:40       ` Pavel Machek
2021-03-03 18:49         ` Henning Schild
2021-03-03 19:27           ` Pavel Machek
2021-03-02 16:33 ` [PATCH 3/4] watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs Henning Schild
2021-03-02 18:38   ` Guenter Roeck
2021-03-03 14:21     ` Henning Schild
2021-03-03 14:49       ` Jan Kiszka
2021-03-08 15:20       ` Henning Schild
     [not found]   ` <CAHp75VdUXWDg-2o8fmeo7EoMtRLfCnFOw5ptwjXTv9fKMmHv2A@mail.gmail.com>
2021-03-04  9:00     ` Henning Schild
2021-03-02 16:33 ` [PATCH 4/4] platform/x86: pmc_atom: improve critclk_systems matching for Siemens PCs Henning Schild
2021-03-04  9:50   ` Andy Shevchenko
2021-03-04 10:19 ` [PATCH 0/4] add device drivers for Siemens Industrial PCs Andy Shevchenko
2021-03-04 10:20   ` Andy Shevchenko
2021-03-04 19:26     ` Henning Schild
2021-03-04 19:14   ` Henning Schild

Linux-Watchdog Archive on lore.kernel.org

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

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

Example config snippet for mirrors

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


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