changes since v2:
- remove "simatic-ipc" prefix from LED names
- fix style issues found in v2, mainly LED driver
- fix OEM specific dmi code, and remove magic numbers
- more "simatic_ipc" name prefixing
- improved pmc quirk code using callbacks
changes since v1:
- fixed lots of style issues found in v1
- (debug) printing
- header ordering
- fixed license issues GPLv2 and SPDX in all files
- module_platform_driver instead of __init __exit
- wdt simplifications cleanup
- lots of fixes in wdt driver, all that was found in v1
- fixed dmi length in dmi helper
- changed LED names to allowed ones
- move led driver to simple/
- switched pmc_atom to dmi callback with global variable
--
This series adds support for watchdogs and leds of several x86 devices
from Siemens.
It is structured with a platform driver that mainly does identification
of the machines. It might trigger loading of the actual device drivers
by attaching devices to the platform bus.
The identification is vendor specific, parsing a special binary DMI
entry. The implementation of that platform identification is applied on
pmc_atom clock quirks in the final patch.
It is all structured in a way that we can easily add more devices and
more platform drivers later. Internally we have some more code for
hardware monitoring, more leds, watchdogs etc. This will follow some
day.
Henning Schild (4):
platform/x86: simatic-ipc: add main driver for Siemens devices
leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
platform/x86: pmc_atom: improve critclk_systems matching for Siemens
PCs
drivers/leds/Kconfig | 3 +
drivers/leds/Makefile | 3 +
drivers/leds/simple/Kconfig | 11 +
drivers/leds/simple/Makefile | 2 +
drivers/leds/simple/simatic-ipc-leds.c | 202 ++++++++++++++++
drivers/platform/x86/Kconfig | 12 +
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/pmc_atom.c | 57 +++--
drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++
drivers/watchdog/Kconfig | 11 +
drivers/watchdog/Makefile | 1 +
drivers/watchdog/simatic-ipc-wdt.c | 215 ++++++++++++++++++
.../platform_data/x86/simatic-ipc-base.h | 29 +++
include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++
14 files changed, 769 insertions(+), 21 deletions(-)
create mode 100644 drivers/leds/simple/Kconfig
create mode 100644 drivers/leds/simple/Makefile
create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
create mode 100644 drivers/platform/x86/simatic-ipc.c
create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
--
2.26.3
This driver adds initial support for several devices from Siemens. It is
based on a platform driver introduced in an earlier commit.
Signed-off-by: Henning Schild <[email protected]>
---
drivers/leds/Kconfig | 3 +
drivers/leds/Makefile | 3 +
drivers/leds/simple/Kconfig | 11 ++
drivers/leds/simple/Makefile | 2 +
drivers/leds/simple/simatic-ipc-leds.c | 202 +++++++++++++++++++++++++
5 files changed, 221 insertions(+)
create mode 100644 drivers/leds/simple/Kconfig
create mode 100644 drivers/leds/simple/Makefile
create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
index b6742b4231bf..5c8558a4fa60 100644
--- a/drivers/leds/Kconfig
+++ b/drivers/leds/Kconfig
@@ -937,4 +937,7 @@ source "drivers/leds/trigger/Kconfig"
comment "LED Blink"
source "drivers/leds/blink/Kconfig"
+comment "Simple LED drivers"
+source "drivers/leds/simple/Kconfig"
+
endif # NEW_LEDS
diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
index 2a698df9da57..2de7fdd8d629 100644
--- a/drivers/leds/Makefile
+++ b/drivers/leds/Makefile
@@ -111,3 +111,6 @@ obj-$(CONFIG_LEDS_TRIGGERS) += trigger/
# LED Blink
obj-$(CONFIG_LEDS_BLINK) += blink/
+
+# Simple LED drivers
+obj-y += simple/
diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
new file mode 100644
index 000000000000..9f6a68336659
--- /dev/null
+++ b/drivers/leds/simple/Kconfig
@@ -0,0 +1,11 @@
+# SPDX-License-Identifier: GPL-2.0-only
+config LEDS_SIEMENS_SIMATIC_IPC
+ tristate "LED driver for Siemens Simatic IPCs"
+ depends on LEDS_CLASS
+ depends on SIEMENS_SIMATIC_IPC
+ help
+ This option enables support for the LEDs of several Industrial PCs
+ from Siemens.
+
+ To compile this driver as a module, choose M here: the module
+ will be called simatic-ipc-leds.
diff --git a/drivers/leds/simple/Makefile b/drivers/leds/simple/Makefile
new file mode 100644
index 000000000000..8481f1e9e360
--- /dev/null
+++ b/drivers/leds/simple/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o
diff --git a/drivers/leds/simple/simatic-ipc-leds.c b/drivers/leds/simple/simatic-ipc-leds.c
new file mode 100644
index 000000000000..043edbf81b76
--- /dev/null
+++ b/drivers/leds/simple/simatic-ipc-leds.c
@@ -0,0 +1,202 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for LEDs
+ *
+ * Copyright (c) Siemens AG, 2018-2021
+ *
+ * Authors:
+ * Henning Schild <[email protected]>
+ * Jan Kiszka <[email protected]>
+ * Gerd Haeussler <[email protected]>
+ */
+
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/leds.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_data/x86/simatic-ipc-base.h>
+#include <linux/platform_device.h>
+#include <linux/sizes.h>
+#include <linux/spinlock.h>
+
+#define SIMATIC_IPC_LED_PORT_BASE 0x404E
+
+struct simatic_ipc_led {
+ unsigned int value; /* mask for io and offset for mem */
+ char *name;
+ struct led_classdev cdev;
+};
+
+static struct simatic_ipc_led simatic_ipc_leds_io[] = {
+ {1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
+ {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" },
+ {1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
+ {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" },
+ {1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
+ {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" },
+ { }
+};
+
+/* the actual start will be discovered with PCI, 0 is a placeholder */
+struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0, SZ_4K, KBUILD_MODNAME);
+
+static void *simatic_ipc_led_memory;
+
+static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
+ {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
+ {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
+ {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
+ {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
+ {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
+ {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
+ { }
+};
+
+static struct resource simatic_ipc_led_io_res =
+ DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_1, KBUILD_MODNAME);
+
+static DEFINE_SPINLOCK(reg_lock);
+
+static inline struct simatic_ipc_led *cdev_to_led(struct led_classdev *led_cd)
+{
+ return container_of(led_cd, struct simatic_ipc_led, cdev);
+}
+
+static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
+ enum led_brightness brightness)
+{
+ struct simatic_ipc_led *led = cdev_to_led(led_cd);
+ unsigned long flags;
+ unsigned int val;
+
+ spin_lock_irqsave(®_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(®_lock, flags);
+}
+
+static enum led_brightness simatic_ipc_led_get_io(struct led_classdev *led_cd)
+{
+ struct simatic_ipc_led *led = cdev_to_led(led_cd);
+
+ return inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF : led_cd->max_brightness;
+}
+
+static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
+ enum led_brightness brightness)
+{
+ struct simatic_ipc_led *led = cdev_to_led(led_cd);
+
+ u32 *p;
+
+ p = simatic_ipc_led_memory + led->value;
+ *p = (*p & ~1) | (brightness == LED_OFF);
+}
+
+static enum led_brightness simatic_ipc_led_get_mem(struct led_classdev *led_cd)
+{
+ struct simatic_ipc_led *led = cdev_to_led(led_cd);
+
+ u32 *p;
+
+ p = simatic_ipc_led_memory + led->value;
+ return (*p & 1) ? LED_OFF : led_cd->max_brightness;
+}
+
+static int simatic_ipc_leds_probe(struct platform_device *pdev)
+{
+ const struct simatic_ipc_platform *plat = pdev->dev.platform_data;
+ struct device *dev = &pdev->dev;
+ struct simatic_ipc_led *ipcled;
+ struct led_classdev *cdev;
+ struct resource *res;
+ int err, type;
+ u32 *p;
+
+ switch (plat->devmode) {
+ case SIMATIC_IPC_DEVICE_227D:
+ case SIMATIC_IPC_DEVICE_427E:
+ res = &simatic_ipc_led_io_res;
+ ipcled = simatic_ipc_leds_io;
+ /* on 227D the two bytes work the other way araound */
+ if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
+ while (ipcled->value) {
+ ipcled->value = swab16(ipcled->value);
+ ipcled++;
+ }
+ ipcled = simatic_ipc_leds_io;
+ }
+ type = IORESOURCE_IO;
+ if (!devm_request_region(dev, res->start, resource_size(res), KBUILD_MODNAME)) {
+ dev_err(dev, "Unable to register IO resource at %pR\n", res);
+ return -EBUSY;
+ }
+ break;
+ case SIMATIC_IPC_DEVICE_127E:
+ res = &simatic_ipc_led_mem_res;
+ ipcled = simatic_ipc_leds_mem;
+ type = IORESOURCE_MEM;
+
+ /* get GPIO base from PCI */
+ res->start = simatic_ipc_get_membase0(PCI_DEVFN(13, 0));
+ if (res->start == 0)
+ return -ENODEV;
+
+ /* do the final address calculation */
+ res->start = res->start + (0xC5 << 16);
+ res->end += res->start;
+
+ simatic_ipc_led_memory = devm_ioremap_resource(dev, res);
+ if (IS_ERR(simatic_ipc_led_memory))
+ return PTR_ERR(simatic_ipc_led_memory);
+
+ /* initialize power/watchdog LED */
+ p = simatic_ipc_led_memory + 0x500 + 0x1D8; /* PM_WDT_OUT */
+ *p = (*p & ~1);
+ p = simatic_ipc_led_memory + 0x500 + 0x1C0; /* PM_BIOS_BOOT_N */
+ *p = (*p | 1);
+
+ break;
+ default:
+ return -ENODEV;
+ }
+
+ while (ipcled->value) {
+ cdev = &ipcled->cdev;
+ if (type == IORESOURCE_MEM) {
+ cdev->brightness_set = simatic_ipc_led_set_mem;
+ cdev->brightness_get = simatic_ipc_led_get_mem;
+ } else {
+ cdev->brightness_set = simatic_ipc_led_set_io;
+ cdev->brightness_get = simatic_ipc_led_get_io;
+ }
+ cdev->max_brightness = LED_ON;
+ cdev->name = ipcled->name;
+
+ err = devm_led_classdev_register(dev, cdev);
+ if (err < 0)
+ return err;
+ ipcled++;
+ }
+
+ return 0;
+}
+
+static struct platform_driver simatic_ipc_led_driver = {
+ .probe = simatic_ipc_leds_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ }
+};
+
+module_platform_driver(simatic_ipc_led_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("Henning Schild <[email protected]>");
--
2.26.3
This driver adds initial support for several devices from Siemens. It is
based on a platform driver introduced in an earlier commit.
Signed-off-by: Henning Schild <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/Kconfig | 11 ++
drivers/watchdog/Makefile | 1 +
drivers/watchdog/simatic-ipc-wdt.c | 215 +++++++++++++++++++++++++++++
3 files changed, 227 insertions(+)
create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 1fe0042a48d2..948497eb4bef 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1575,6 +1575,17 @@ config NIC7018_WDT
To compile this driver as a module, choose M here: the module will be
called nic7018_wdt.
+config SIEMENS_SIMATIC_IPC_WDT
+ tristate "Siemens Simatic IPC Watchdog"
+ depends on SIEMENS_SIMATIC_IPC
+ select WATCHDOG_CORE
+ help
+ This driver adds support for several watchdogs found in Industrial
+ PCs from Siemens.
+
+ To compile this driver as a module, choose M here: the module will be
+ called simatic-ipc-wdt.
+
# M68K Architecture
config M54xx_WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index f3a6540e725e..7f5c73ec058c 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o
+obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o
# M68K Architecture
obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o
diff --git a/drivers/watchdog/simatic-ipc-wdt.c b/drivers/watchdog/simatic-ipc-wdt.c
new file mode 100644
index 000000000000..e901718d05b9
--- /dev/null
+++ b/drivers/watchdog/simatic-ipc-wdt.c
@@ -0,0 +1,215 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for Watchdogs
+ *
+ * Copyright (c) Siemens AG, 2020-2021
+ *
+ * Authors:
+ * Gerd Haeussler <[email protected]>
+ */
+
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/ioport.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/platform_data/x86/simatic-ipc-base.h>
+#include <linux/platform_device.h>
+#include <linux/sizes.h>
+#include <linux/util_macros.h>
+#include <linux/watchdog.h>
+
+#define WD_ENABLE_IOADR 0x62
+#define WD_TRIGGER_IOADR 0x66
+#define GPIO_COMMUNITY0_PORT_ID 0xaf
+#define PAD_CFG_DW0_GPP_A_23 0x4b8
+#define SAFE_EN_N_427E 0x01
+#define SAFE_EN_N_227E 0x04
+#define WD_ENABLED 0x01
+#define WD_TRIGGERED 0x80
+#define WD_MACROMODE 0x02
+
+#define TIMEOUT_MIN 2
+#define TIMEOUT_DEF 64
+#define TIMEOUT_MAX 64
+
+#define GP_STATUS_REG_227E 0x404D /* IO PORT for SAFE_EN_N on 227E */
+
+static bool nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, bool, 0000);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+ __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
+static struct resource gp_status_reg_227e_res =
+ DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1, KBUILD_MODNAME);
+
+static struct resource io_resource =
+ DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1,
+ KBUILD_MODNAME " WD_ENABLE_IOADR");
+
+/* the actual start will be discovered with pci, 0 is a placeholder */
+static struct resource mem_resource =
+ DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
+
+static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
+static void __iomem *wd_reset_base_addr;
+
+static int wd_start(struct watchdog_device *wdd)
+{
+ outb(inb(WD_ENABLE_IOADR) | WD_ENABLED, WD_ENABLE_IOADR);
+ return 0;
+}
+
+static int wd_stop(struct watchdog_device *wdd)
+{
+ outb(inb(WD_ENABLE_IOADR) & ~WD_ENABLED, WD_ENABLE_IOADR);
+ return 0;
+}
+
+static int wd_ping(struct watchdog_device *wdd)
+{
+ inb(WD_TRIGGER_IOADR);
+ return 0;
+}
+
+static int wd_set_timeout(struct watchdog_device *wdd, unsigned int t)
+{
+ int timeout_idx = find_closest(t, wd_timeout_table,
+ ARRAY_SIZE(wd_timeout_table));
+
+ outb((inb(WD_ENABLE_IOADR) & 0xc7) | timeout_idx << 3, WD_ENABLE_IOADR);
+ wdd->timeout = wd_timeout_table[timeout_idx];
+ return 0;
+}
+
+static const struct watchdog_info wdt_ident = {
+ .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
+ WDIOF_SETTIMEOUT,
+ .identity = KBUILD_MODNAME,
+};
+
+static const struct watchdog_ops wdt_ops = {
+ .owner = THIS_MODULE,
+ .start = wd_start,
+ .stop = wd_stop,
+ .ping = wd_ping,
+ .set_timeout = wd_set_timeout,
+};
+
+static void wd_secondary_enable(u32 wdtmode)
+{
+ u16 resetbit;
+
+ /* set safe_en_n so we are not just WDIOF_ALARMONLY */
+ if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
+ /* enable SAFE_EN_N on GP_STATUS_REG_227E */
+ resetbit = inw(GP_STATUS_REG_227E);
+ outw(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E);
+ } else {
+ /* enable SAFE_EN_N on PCH D1600 */
+ resetbit = ioread16(wd_reset_base_addr);
+ iowrite16(resetbit & ~SAFE_EN_N_427E, wd_reset_base_addr);
+ }
+}
+
+static int wd_setup(u32 wdtmode)
+{
+ unsigned int bootstatus = 0;
+ int timeout_idx;
+
+ timeout_idx = find_closest(TIMEOUT_DEF, wd_timeout_table,
+ ARRAY_SIZE(wd_timeout_table));
+
+ if (inb(WD_ENABLE_IOADR) & WD_TRIGGERED)
+ bootstatus |= WDIOF_CARDRESET;
+
+ /* reset alarm bit, set macro mode, and set timeout */
+ outb(WD_TRIGGERED | WD_MACROMODE | timeout_idx << 3, WD_ENABLE_IOADR);
+
+ wd_secondary_enable(wdtmode);
+
+ return bootstatus;
+}
+
+static struct watchdog_device wdd_data = {
+ .info = &wdt_ident,
+ .ops = &wdt_ops,
+ .min_timeout = TIMEOUT_MIN,
+ .max_timeout = TIMEOUT_MAX
+};
+
+static int simatic_ipc_wdt_probe(struct platform_device *pdev)
+{
+ struct simatic_ipc_platform *plat = pdev->dev.platform_data;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+
+ switch (plat->devmode) {
+ case SIMATIC_IPC_DEVICE_227E:
+ if (!devm_request_region(dev, gp_status_reg_227e_res.start,
+ resource_size(&gp_status_reg_227e_res),
+ KBUILD_MODNAME)) {
+ dev_err(dev,
+ "Unable to register IO resource at %pR\n",
+ &gp_status_reg_227e_res);
+ return -EBUSY;
+ }
+ fallthrough;
+ case SIMATIC_IPC_DEVICE_427E:
+ wdd_data.parent = dev;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ if (!devm_request_region(dev, io_resource.start,
+ resource_size(&io_resource),
+ io_resource.name)) {
+ dev_err(dev,
+ "Unable to register IO resource at %#x\n",
+ WD_ENABLE_IOADR);
+ return -EBUSY;
+ }
+
+ if (plat->devmode == SIMATIC_IPC_DEVICE_427E) {
+ res = &mem_resource;
+
+ /* get GPIO base from PCI */
+ res->start = simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1));
+ if (res->start == 0)
+ return -ENODEV;
+
+ /* do the final address calculation */
+ res->start = res->start + (GPIO_COMMUNITY0_PORT_ID << 16) +
+ PAD_CFG_DW0_GPP_A_23;
+ res->end += res->start;
+
+ wd_reset_base_addr = devm_ioremap_resource(dev, res);
+ if (IS_ERR(wd_reset_base_addr))
+ return PTR_ERR(wd_reset_base_addr);
+ }
+
+ wdd_data.bootstatus = wd_setup(plat->devmode);
+ if (wdd_data.bootstatus)
+ dev_warn(dev, "last reboot caused by watchdog reset\n");
+
+ watchdog_set_nowayout(&wdd_data, nowayout);
+ watchdog_stop_on_reboot(&wdd_data);
+ return devm_watchdog_register_device(dev, &wdd_data);
+}
+
+static struct platform_driver simatic_ipc_wdt_driver = {
+ .probe = simatic_ipc_wdt_probe,
+ .driver = {
+ .name = KBUILD_MODNAME,
+ },
+};
+
+module_platform_driver(simatic_ipc_wdt_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:" KBUILD_MODNAME);
+MODULE_AUTHOR("Gerd Haeussler <[email protected]>");
--
2.26.3
Guys,
sorry for the delay. This one did in fact not change too much after all.
The biggest points that are still kind of open are the naming of the
LEDs. If what is proposed here is acceptable it is not open from my
side.
The other big point was "using a generic gpio" driver as a basis. My
current understanding of that point is, that such a driver does not yet
exist. Meaning an introduction of the abstractions can and probably
should wait for a second user. Without the second user it is just hard
to test and find the right abstraction, plus we will end up with more
code meaning more work for everyone.
regards,
Henning
Am Mon, 29 Mar 2021 19:49:24 +0200
schrieb Henning Schild <[email protected]>:
> changes since v2:
>
> - remove "simatic-ipc" prefix from LED names
> - fix style issues found in v2, mainly LED driver
> - fix OEM specific dmi code, and remove magic numbers
> - more "simatic_ipc" name prefixing
> - improved pmc quirk code using callbacks
>
> changes since v1:
>
> - fixed lots of style issues found in v1
> - (debug) printing
> - header ordering
> - fixed license issues GPLv2 and SPDX in all files
> - module_platform_driver instead of __init __exit
> - wdt simplifications cleanup
> - lots of fixes in wdt driver, all that was found in v1
> - fixed dmi length in dmi helper
> - changed LED names to allowed ones
> - move led driver to simple/
> - switched pmc_atom to dmi callback with global variable
>
> --
>
> This series adds support for watchdogs and leds of several x86 devices
> from Siemens.
>
> It is structured with a platform driver that mainly does
> identification of the machines. It might trigger loading of the
> actual device drivers by attaching devices to the platform bus.
>
> The identification is vendor specific, parsing a special binary DMI
> entry. The implementation of that platform identification is applied
> on pmc_atom clock quirks in the final patch.
>
> It is all structured in a way that we can easily add more devices and
> more platform drivers later. Internally we have some more code for
> hardware monitoring, more leds, watchdogs etc. This will follow some
> day.
>
> Henning Schild (4):
> platform/x86: simatic-ipc: add main driver for Siemens devices
> leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
> watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
> platform/x86: pmc_atom: improve critclk_systems matching for Siemens
> PCs
>
> drivers/leds/Kconfig | 3 +
> drivers/leds/Makefile | 3 +
> drivers/leds/simple/Kconfig | 11 +
> drivers/leds/simple/Makefile | 2 +
> drivers/leds/simple/simatic-ipc-leds.c | 202 ++++++++++++++++
> drivers/platform/x86/Kconfig | 12 +
> drivers/platform/x86/Makefile | 3 +
> drivers/platform/x86/pmc_atom.c | 57 +++--
> drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++
> drivers/watchdog/Kconfig | 11 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/simatic-ipc-wdt.c | 215
> ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h |
> 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++
> 14 files changed, 769 insertions(+), 21 deletions(-)
> create mode 100644 drivers/leds/simple/Kconfig
> create mode 100644 drivers/leds/simple/Makefile
> create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> create mode 100644 drivers/platform/x86/simatic-ipc.c
> create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
> create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
>
On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
<[email protected]> wrote:
>
> This driver adds initial support for several devices from Siemens. It is
> based on a platform driver introduced in an earlier commit.
...
> +#define SIMATIC_IPC_LED_PORT_BASE 0x404E
> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" },
> + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" },
> + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" },
> + { }
> +};
> +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> + { }
> +};
It seems to me like poking GPIO controller registers directly. This is not good.
The question still remains: Can we simply register a GPIO (pin
control) driver and use an LED GPIO driver with an additional board
file that instantiates it?
--
With Best Regards,
Andy Shevchenko
Am Tue, 30 Mar 2021 14:04:35 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> <[email protected]> wrote:
> >
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.
>
> ...
>
> > +#define SIMATIC_IPC_LED_PORT_BASE 0x404E
>
> > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> > + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" },
> > + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> > + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" },
> > + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> > + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" },
> > + { }
> > +};
>
> > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > + { }
> > +};
>
> It seems to me like poking GPIO controller registers directly. This
> is not good. The question still remains: Can we simply register a
> GPIO (pin control) driver and use an LED GPIO driver with an
> additional board file that instantiates it?
I wrote about that in reply to the cover letter. My view is still that
it would be an abstraction with only one user, just causing work and
likely not ending up as generic as it might eventually have to be.
The region is reserved, not sure what the problem with the "poking" is.
Maybe i do not understand all the benefits of such a split at this
point in time. At the moment i only see work with hardly any benefit,
not just work for me but also for maintainers. I sure do not mean to be
ignorant. Maybe you go into details and convince me or we wait for other
peoples opinions on how to proceed, maybe there is a second user that i
am not aware of?
Until i am convinced otherwise i will try to argue that a
single-user-abstraction is needless work/code, and should be done only
when actually needed.
regards,
Henning
On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
<[email protected]> wrote:
> Am Tue, 30 Mar 2021 14:04:35 +0300
> schrieb Andy Shevchenko <[email protected]>:
> > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > <[email protected]> wrote:
> > >
> > > This driver adds initial support for several devices from Siemens.
> > > It is based on a platform driver introduced in an earlier commit.
> >
> > ...
> >
> > > +#define SIMATIC_IPC_LED_PORT_BASE 0x404E
> >
> > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> > > + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" },
> > > + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> > > + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" },
> > > + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> > > + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" },
> > > + { }
> > > +};
> >
> > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > + { }
> > > +};
> >
> > It seems to me like poking GPIO controller registers directly. This
> > is not good. The question still remains: Can we simply register a
> > GPIO (pin control) driver and use an LED GPIO driver with an
> > additional board file that instantiates it?
>
> I wrote about that in reply to the cover letter. My view is still that
> it would be an abstraction with only one user, just causing work and
> likely not ending up as generic as it might eventually have to be.
>
> The region is reserved, not sure what the problem with the "poking" is.
> Maybe i do not understand all the benefits of such a split at this
> point in time. At the moment i only see work with hardly any benefit,
> not just work for me but also for maintainers. I sure do not mean to be
> ignorant. Maybe you go into details and convince me or we wait for other
> peoples opinions on how to proceed, maybe there is a second user that i
> am not aware of?
> Until i am convinced otherwise i will try to argue that a
> single-user-abstraction is needless work/code, and should be done only
> when actually needed.
I have just read your messages (there is a cover letter and additional
email which was sent lately).
I would like to know what the CPU model number on that board is. Than
we can continue to see what possibilities we have here.
--
With Best Regards,
Andy Shevchenko
Am Tue, 30 Mar 2021 15:15:16 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> <[email protected]> wrote:
> > Am Tue, 30 Mar 2021 14:04:35 +0300
> > schrieb Andy Shevchenko <[email protected]>:
> > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > <[email protected]> wrote:
> > > >
> > > > This driver adds initial support for several devices from
> > > > Siemens. It is based on a platform driver introduced in an
> > > > earlier commit.
> > >
> > > ...
> > >
> > > > +#define SIMATIC_IPC_LED_PORT_BASE 0x404E
> > >
> > > > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > > > + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> > > > + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" },
> > > > + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> > > > + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" },
> > > > + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> > > > + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" },
> > > > + { }
> > > > +};
> > >
> > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > > + { }
> > > > +};
> > >
> > > It seems to me like poking GPIO controller registers directly.
> > > This is not good. The question still remains: Can we simply
> > > register a GPIO (pin control) driver and use an LED GPIO driver
> > > with an additional board file that instantiates it?
> >
> > I wrote about that in reply to the cover letter. My view is still
> > that it would be an abstraction with only one user, just causing
> > work and likely not ending up as generic as it might eventually
> > have to be.
> >
> > The region is reserved, not sure what the problem with the "poking"
> > is.
>
>
> > Maybe i do not understand all the benefits of such a split at this
> > point in time. At the moment i only see work with hardly any
> > benefit, not just work for me but also for maintainers. I sure do
> > not mean to be ignorant. Maybe you go into details and convince me
> > or we wait for other peoples opinions on how to proceed, maybe
> > there is a second user that i am not aware of?
> > Until i am convinced otherwise i will try to argue that a
> > single-user-abstraction is needless work/code, and should be done
> > only when actually needed.
>
> I have just read your messages (there is a cover letter and additional
> email which was sent lately).
>
> I would like to know what the CPU model number on that board is. Than
> we can continue to see what possibilities we have here.
I guess we are talking about the one that uses memory mapped, that is
called an "IPC127E" and seems to have either Intel Atom E3940 or E3930
which seems to be Apollo Lake.
Henning
On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
<[email protected]> wrote:
> Am Tue, 30 Mar 2021 15:15:16 +0300
> schrieb Andy Shevchenko <[email protected]>:
> > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > <[email protected]> wrote:
> > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > schrieb Andy Shevchenko <[email protected]>:
> > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > <[email protected]> wrote:
> > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > > > + { }
> > > > > +};
> > > >
> > > > It seems to me like poking GPIO controller registers directly.
> > > > This is not good. The question still remains: Can we simply
> > > > register a GPIO (pin control) driver and use an LED GPIO driver
> > > > with an additional board file that instantiates it?
> > >
> > > I wrote about that in reply to the cover letter. My view is still
> > > that it would be an abstraction with only one user, just causing
> > > work and likely not ending up as generic as it might eventually
> > > have to be.
> > >
> > > The region is reserved, not sure what the problem with the "poking"
> > > is.
> >
> >
> > > Maybe i do not understand all the benefits of such a split at this
> > > point in time. At the moment i only see work with hardly any
> > > benefit, not just work for me but also for maintainers. I sure do
> > > not mean to be ignorant. Maybe you go into details and convince me
> > > or we wait for other peoples opinions on how to proceed, maybe
> > > there is a second user that i am not aware of?
> > > Until i am convinced otherwise i will try to argue that a
> > > single-user-abstraction is needless work/code, and should be done
> > > only when actually needed.
> >
> > I have just read your messages (there is a cover letter and additional
> > email which was sent lately).
> >
> > I would like to know what the CPU model number on that board is. Than
> > we can continue to see what possibilities we have here.
>
> I guess we are talking about the one that uses memory mapped, that is
> called an "IPC127E" and seems to have either Intel Atom E3940 or E3930
> which seems to be Apollo Lake.
Yep. And now the question, in my patch series you should have got the
apollolake-pinctrl driver loaded (if not, we have to investigate why
it's not being instantiated). This will give you a read GPIO driver.
So, you may use regular LED GPIO on top of it
(https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
I would like to understand why it can't be achieved.
--
With Best Regards,
Andy Shevchenko
Am Tue, 30 Mar 2021 15:41:53 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> <[email protected]> wrote:
> > Am Tue, 30 Mar 2021 15:15:16 +0300
> > schrieb Andy Shevchenko <[email protected]>:
> > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > <[email protected]> wrote:
> > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > <[email protected]> wrote:
>
> > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > > > > + { }
> > > > > > +};
> > > > >
> > > > > It seems to me like poking GPIO controller registers directly.
> > > > > This is not good. The question still remains: Can we simply
> > > > > register a GPIO (pin control) driver and use an LED GPIO
> > > > > driver with an additional board file that instantiates it?
> > > >
> > > > I wrote about that in reply to the cover letter. My view is
> > > > still that it would be an abstraction with only one user, just
> > > > causing work and likely not ending up as generic as it might
> > > > eventually have to be.
> > > >
> > > > The region is reserved, not sure what the problem with the
> > > > "poking" is.
> > >
> > >
> > > > Maybe i do not understand all the benefits of such a split at
> > > > this point in time. At the moment i only see work with hardly
> > > > any benefit, not just work for me but also for maintainers. I
> > > > sure do not mean to be ignorant. Maybe you go into details and
> > > > convince me or we wait for other peoples opinions on how to
> > > > proceed, maybe there is a second user that i am not aware of?
> > > > Until i am convinced otherwise i will try to argue that a
> > > > single-user-abstraction is needless work/code, and should be
> > > > done only when actually needed.
> > >
> > > I have just read your messages (there is a cover letter and
> > > additional email which was sent lately).
> > >
> > > I would like to know what the CPU model number on that board is.
> > > Than we can continue to see what possibilities we have here.
> >
> > I guess we are talking about the one that uses memory mapped, that
> > is called an "IPC127E" and seems to have either Intel Atom E3940 or
> > E3930 which seems to be Apollo Lake.
>
> Yep. And now the question, in my patch series you should have got the
> apollolake-pinctrl driver loaded (if not, we have to investigate why
> it's not being instantiated). This will give you a read GPIO driver.
Ok, so there is the existing driver i asked about several times. Thanks
for pointing it out.
> So, you may use regular LED GPIO on top of it
> (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> I would like to understand why it can't be achieved.
Will have a look. Unfortunately this one box is missing in my personal
collection, but let us assume that one can be converted to that
existing driver.
I guess that will still mean the PIO-based part of the LED driver will
have to stay as is.
regards,
Henning
On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
<[email protected]> wrote:
> Am Tue, 30 Mar 2021 15:41:53 +0300
> schrieb Andy Shevchenko <[email protected]>:
> > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > <[email protected]> wrote:
> > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > schrieb Andy Shevchenko <[email protected]>:
> > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > <[email protected]> wrote:
> > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > <[email protected]> wrote:
> >
> > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > > > > > + { }
> > > > > > > +};
> > > > > >
> > > > > > It seems to me like poking GPIO controller registers directly.
> > > > > > This is not good. The question still remains: Can we simply
> > > > > > register a GPIO (pin control) driver and use an LED GPIO
> > > > > > driver with an additional board file that instantiates it?
> > > > >
> > > > > I wrote about that in reply to the cover letter. My view is
> > > > > still that it would be an abstraction with only one user, just
> > > > > causing work and likely not ending up as generic as it might
> > > > > eventually have to be.
> > > > >
> > > > > The region is reserved, not sure what the problem with the
> > > > > "poking" is.
> > > >
> > > >
> > > > > Maybe i do not understand all the benefits of such a split at
> > > > > this point in time. At the moment i only see work with hardly
> > > > > any benefit, not just work for me but also for maintainers. I
> > > > > sure do not mean to be ignorant. Maybe you go into details and
> > > > > convince me or we wait for other peoples opinions on how to
> > > > > proceed, maybe there is a second user that i am not aware of?
> > > > > Until i am convinced otherwise i will try to argue that a
> > > > > single-user-abstraction is needless work/code, and should be
> > > > > done only when actually needed.
> > > >
> > > > I have just read your messages (there is a cover letter and
> > > > additional email which was sent lately).
> > > >
> > > > I would like to know what the CPU model number on that board is.
> > > > Than we can continue to see what possibilities we have here.
> > >
> > > I guess we are talking about the one that uses memory mapped, that
> > > is called an "IPC127E" and seems to have either Intel Atom E3940 or
> > > E3930 which seems to be Apollo Lake.
> >
> > Yep. And now the question, in my patch series you should have got the
> > apollolake-pinctrl driver loaded (if not, we have to investigate why
> > it's not being instantiated). This will give you a read GPIO driver.
>
> Ok, so there is the existing driver i asked about several times. Thanks
> for pointing it out.
If you remember, I asked you about the chip twice :-)
I assumed that we were talking about Apollo Lake and that's why I
insisted that the driver is in the kernel source tree.
> > So, you may use regular LED GPIO on top of it
> > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > I would like to understand why it can't be achieved.
>
> Will have a look. Unfortunately this one box is missing in my personal
> collection, but let us assume that one can be converted to that
> existing driver.
OK!
> I guess that will still mean the PIO-based part of the LED driver will
> have to stay as is.
Probably yes. I haven't looked into that part and I have no idea
what's going on on that platform(s).
--
With Best Regards,
Andy Shevchenko
Am Wed, 31 Mar 2021 18:40:23 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> <[email protected]> wrote:
> > Am Tue, 30 Mar 2021 15:41:53 +0300
> > schrieb Andy Shevchenko <[email protected]>:
> > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > <[email protected]> wrote:
> > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > <[email protected]> wrote:
> > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > <[email protected]> wrote:
> > >
> > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] =
> > > > > > > > {
> > > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS
> > > > > > > > "-1"},
> > > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS
> > > > > > > > "-1"},
> > > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS
> > > > > > > > "-2"},
> > > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS
> > > > > > > > "-2"},
> > > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS
> > > > > > > > "-3"},
> > > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS
> > > > > > > > "-3"},
> > > > > > > > + { }
> > > > > > > > +};
> > > > > > >
> > > > > > > It seems to me like poking GPIO controller registers
> > > > > > > directly. This is not good. The question still remains:
> > > > > > > Can we simply register a GPIO (pin control) driver and
> > > > > > > use an LED GPIO driver with an additional board file that
> > > > > > > instantiates it?
> > > > > >
> > > > > > I wrote about that in reply to the cover letter. My view is
> > > > > > still that it would be an abstraction with only one user,
> > > > > > just causing work and likely not ending up as generic as it
> > > > > > might eventually have to be.
> > > > > >
> > > > > > The region is reserved, not sure what the problem with the
> > > > > > "poking" is.
> > > > >
> > > > >
> > > > > > Maybe i do not understand all the benefits of such a split
> > > > > > at this point in time. At the moment i only see work with
> > > > > > hardly any benefit, not just work for me but also for
> > > > > > maintainers. I sure do not mean to be ignorant. Maybe you
> > > > > > go into details and convince me or we wait for other
> > > > > > peoples opinions on how to proceed, maybe there is a second
> > > > > > user that i am not aware of? Until i am convinced otherwise
> > > > > > i will try to argue that a single-user-abstraction is
> > > > > > needless work/code, and should be done only when actually
> > > > > > needed.
> > > > >
> > > > > I have just read your messages (there is a cover letter and
> > > > > additional email which was sent lately).
> > > > >
> > > > > I would like to know what the CPU model number on that board
> > > > > is. Than we can continue to see what possibilities we have
> > > > > here.
> > > >
> > > > I guess we are talking about the one that uses memory mapped,
> > > > that is called an "IPC127E" and seems to have either Intel Atom
> > > > E3940 or E3930 which seems to be Apollo Lake.
> > >
> > > Yep. And now the question, in my patch series you should have got
> > > the apollolake-pinctrl driver loaded (if not, we have to
> > > investigate why it's not being instantiated). This will give you
> > > a read GPIO driver.
> >
> > Ok, so there is the existing driver i asked about several times.
> > Thanks for pointing it out.
>
> If you remember, I asked you about the chip twice :-)
> I assumed that we were talking about Apollo Lake and that's why I
> insisted that the driver is in the kernel source tree.
Sorry, maybe i did not get the context of your question and which of
the machines you asked about. Now it is clear i guess.
>
> > > So, you may use regular LED GPIO on top of it
> > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > > I would like to understand why it can't be achieved.
> >
> > Will have a look. Unfortunately this one box is missing in my
> > personal collection, but let us assume that one can be converted to
> > that existing driver.
>
> OK!
>
> > I guess that will still mean the PIO-based part of the LED driver
> > will have to stay as is.
>
> Probably yes. I haven't looked into that part and I have no idea
> what's going on on that platform(s).
>
Which i guess means the series can be reviewed as if the mmio bits for
that apollo lake would not be in it, maybe i will even send a version
without that one box. We have others in the "backlog" might as well
delay that one if it helps sorting out a base-line.
regards,
Henning
On Thu, Apr 1, 2021 at 1:44 PM Henning Schild
<[email protected]> wrote:
>
> Am Wed, 31 Mar 2021 18:40:23 +0300
> schrieb Andy Shevchenko <[email protected]>:
>
> > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> > <[email protected]> wrote:
> > > Am Tue, 30 Mar 2021 15:41:53 +0300
> > > schrieb Andy Shevchenko <[email protected]>:
> > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > > <[email protected]> wrote:
> > > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > > <[email protected]> wrote:
> > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > > <[email protected]> wrote:
> > > >
> > > > > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] =
> > > > > > > > > {
> > > > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS
> > > > > > > > > "-1"},
> > > > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS
> > > > > > > > > "-1"},
> > > > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS
> > > > > > > > > "-2"},
> > > > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS
> > > > > > > > > "-2"},
> > > > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS
> > > > > > > > > "-3"},
> > > > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS
> > > > > > > > > "-3"},
> > > > > > > > > + { }
> > > > > > > > > +};
> > > > > > > >
> > > > > > > > It seems to me like poking GPIO controller registers
> > > > > > > > directly. This is not good. The question still remains:
> > > > > > > > Can we simply register a GPIO (pin control) driver and
> > > > > > > > use an LED GPIO driver with an additional board file that
> > > > > > > > instantiates it?
> > > > > > >
> > > > > > > I wrote about that in reply to the cover letter. My view is
> > > > > > > still that it would be an abstraction with only one user,
> > > > > > > just causing work and likely not ending up as generic as it
> > > > > > > might eventually have to be.
> > > > > > >
> > > > > > > The region is reserved, not sure what the problem with the
> > > > > > > "poking" is.
> > > > > >
> > > > > >
> > > > > > > Maybe i do not understand all the benefits of such a split
> > > > > > > at this point in time. At the moment i only see work with
> > > > > > > hardly any benefit, not just work for me but also for
> > > > > > > maintainers. I sure do not mean to be ignorant. Maybe you
> > > > > > > go into details and convince me or we wait for other
> > > > > > > peoples opinions on how to proceed, maybe there is a second
> > > > > > > user that i am not aware of? Until i am convinced otherwise
> > > > > > > i will try to argue that a single-user-abstraction is
> > > > > > > needless work/code, and should be done only when actually
> > > > > > > needed.
> > > > > >
> > > > > > I have just read your messages (there is a cover letter and
> > > > > > additional email which was sent lately).
> > > > > >
> > > > > > I would like to know what the CPU model number on that board
> > > > > > is. Than we can continue to see what possibilities we have
> > > > > > here.
> > > > >
> > > > > I guess we are talking about the one that uses memory mapped,
> > > > > that is called an "IPC127E" and seems to have either Intel Atom
> > > > > E3940 or E3930 which seems to be Apollo Lake.
> > > >
> > > > Yep. And now the question, in my patch series you should have got
> > > > the apollolake-pinctrl driver loaded (if not, we have to
> > > > investigate why it's not being instantiated). This will give you
> > > > a read GPIO driver.
> > >
> > > Ok, so there is the existing driver i asked about several times.
> > > Thanks for pointing it out.
> >
> > If you remember, I asked you about the chip twice :-)
> > I assumed that we were talking about Apollo Lake and that's why I
> > insisted that the driver is in the kernel source tree.
>
> Sorry, maybe i did not get the context of your question and which of
> the machines you asked about. Now it is clear i guess.
>
> >
> > > > So, you may use regular LED GPIO on top of it
> > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > > > I would like to understand why it can't be achieved.
> > >
> > > Will have a look. Unfortunately this one box is missing in my
> > > personal collection, but let us assume that one can be converted to
> > > that existing driver.
> >
> > OK!
> >
> > > I guess that will still mean the PIO-based part of the LED driver
> > > will have to stay as is.
> >
> > Probably yes. I haven't looked into that part and I have no idea
> > what's going on on that platform(s).
> >
>
> Which i guess means the series can be reviewed as if the mmio bits for
> that apollo lake would not be in it, maybe i will even send a version
> without that one box. We have others in the "backlog" might as well
> delay that one if it helps sorting out a base-line.
It depends on the role of P2SB in this case.
Shouldn't you drop that completely out from this series?
Otherwise we have to understand what to do with it.
It seems the best approach can be to expose the P2SB device to Linux,
but we have to answer to Bjorn's request about region reservations.
--
With Best Regards,
Andy Shevchenko
On 29.03.21 19:49, Henning Schild wrote:
Hi,
> This driver adds initial support for several devices from Siemens. It is
> based on a platform driver introduced in an earlier commit.
Where does the wdt actually come from ?
Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty
usual case.
Or some external chip ?
The code smells a bit like two entirely different wdt's that just have
some similarities. If that's the case, I'd rather split it into two
separate drivers and let the parent driver (board file) instantiate
the correct one.
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287
Am Thu, 1 Apr 2021 18:15:41 +0200
schrieb "Enrico Weigelt, metux IT consult" <[email protected]>:
> On 29.03.21 19:49, Henning Schild wrote:
>
> Hi,
>
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.
>
> Where does the wdt actually come from ?
>
> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty
> usual case.
>
> Or some external chip ?
I guess external chip, but again we are talking about multiple
machines. And the manuals i read so far do not go into that sort of
detail. In fact on some of the machines you will have two watchdogs,
one from the SoC and that "special" one.
That has several reasons, probably not too important here. The HW guys
are adding another wd not just for fun, and it would be nice to have a
driver.
> The code smells a bit like two entirely different wdt's that just have
> some similarities. If that's the case, I'd rather split it into two
> separate drivers and let the parent driver (board file) instantiate
> the correct one.
Yes, it is two. Just like for the LEDs. One version PIO-based another
version gpio/p2sb/mmio based.
In fact the latter should very likely be based on that gpio pinctl,
whether it really needs to be a separate driver will have to be seen.
There are probably pros and cons for both options.
regards,
Henning
>
> --mtx
>
On Tue, Apr 6, 2021 at 5:56 PM Henning Schild
<[email protected]> wrote:
>
> Am Thu, 1 Apr 2021 18:15:41 +0200
> schrieb "Enrico Weigelt, metux IT consult" <[email protected]>:
>
> > On 29.03.21 19:49, Henning Schild wrote:
> >
> > Hi,
> >
> > > This driver adds initial support for several devices from Siemens.
> > > It is based on a platform driver introduced in an earlier commit.
> >
> > Where does the wdt actually come from ?
> >
> > Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty
> > usual case.
> >
> > Or some external chip ?
>
> I guess external chip, but again we are talking about multiple
> machines. And the manuals i read so far do not go into that sort of
> detail. In fact on some of the machines you will have two watchdogs,
> one from the SoC and that "special" one.
> That has several reasons, probably not too important here. The HW guys
> are adding another wd not just for fun, and it would be nice to have a
> driver.
>
> > The code smells a bit like two entirely different wdt's that just have
> > some similarities. If that's the case, I'd rather split it into two
> > separate drivers and let the parent driver (board file) instantiate
> > the correct one.
>
> Yes, it is two. Just like for the LEDs. One version PIO-based another
> version gpio/p2sb/mmio based.
I tend to agree with Enrico that this should be two separate drivers.
> In fact the latter should very likely be based on that gpio pinctl,
> whether it really needs to be a separate driver will have to be seen.
> There are probably pros and cons for both options.
--
With Best Regards,
Andy Shevchenko
Hi,
On 3/29/21 7:49 PM, Henning Schild wrote:
> changes since v2:
>
> - remove "simatic-ipc" prefix from LED names
> - fix style issues found in v2, mainly LED driver
> - fix OEM specific dmi code, and remove magic numbers
> - more "simatic_ipc" name prefixing
> - improved pmc quirk code using callbacks
>
> changes since v1:
>
> - fixed lots of style issues found in v1
> - (debug) printing
> - header ordering
> - fixed license issues GPLv2 and SPDX in all files
> - module_platform_driver instead of __init __exit
> - wdt simplifications cleanup
> - lots of fixes in wdt driver, all that was found in v1
> - fixed dmi length in dmi helper
> - changed LED names to allowed ones
> - move led driver to simple/
> - switched pmc_atom to dmi callback with global variable
>
> --
>
> This series adds support for watchdogs and leds of several x86 devices
> from Siemens.
>
> It is structured with a platform driver that mainly does identification
> of the machines. It might trigger loading of the actual device drivers
> by attaching devices to the platform bus.
>
> The identification is vendor specific, parsing a special binary DMI
> entry. The implementation of that platform identification is applied on
> pmc_atom clock quirks in the final patch.
>
> It is all structured in a way that we can easily add more devices and
> more platform drivers later. Internally we have some more code for
> hardware monitoring, more leds, watchdogs etc. This will follow some
> day.
IT seems there still is significant discussion surrounding the LED and watchdog
drivers which use patch 1/4 as parent-driver.
I'm going to hold of on merging 1/4 and 4/4 until there is more consensus
surrounding this series.
Regards,
Hans
>
> Henning Schild (4):
> platform/x86: simatic-ipc: add main driver for Siemens devices
> leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
> watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial PCs
> platform/x86: pmc_atom: improve critclk_systems matching for Siemens
> PCs
>
> drivers/leds/Kconfig | 3 +
> drivers/leds/Makefile | 3 +
> drivers/leds/simple/Kconfig | 11 +
> drivers/leds/simple/Makefile | 2 +
> drivers/leds/simple/simatic-ipc-leds.c | 202 ++++++++++++++++
> drivers/platform/x86/Kconfig | 12 +
> drivers/platform/x86/Makefile | 3 +
> drivers/platform/x86/pmc_atom.c | 57 +++--
> drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++
> drivers/watchdog/Kconfig | 11 +
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/simatic-ipc-wdt.c | 215 ++++++++++++++++++
> .../platform_data/x86/simatic-ipc-base.h | 29 +++
> include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++
> 14 files changed, 769 insertions(+), 21 deletions(-)
> create mode 100644 drivers/leds/simple/Kconfig
> create mode 100644 drivers/leds/simple/Makefile
> create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> create mode 100644 drivers/platform/x86/simatic-ipc.c
> create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
> create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
>
On 4/7/21 1:53 AM, Andy Shevchenko wrote:
> On Tue, Apr 6, 2021 at 5:56 PM Henning Schild
> <[email protected]> wrote:
>>
>> Am Thu, 1 Apr 2021 18:15:41 +0200
>> schrieb "Enrico Weigelt, metux IT consult" <[email protected]>:
>>
>>> On 29.03.21 19:49, Henning Schild wrote:
>>>
>>> Hi,
>>>
>>>> This driver adds initial support for several devices from Siemens.
>>>> It is based on a platform driver introduced in an earlier commit.
>>>
>>> Where does the wdt actually come from ?
>>>
>>> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty
>>> usual case.
>>>
>>> Or some external chip ?
>>
>> I guess external chip, but again we are talking about multiple
>> machines. And the manuals i read so far do not go into that sort of
>> detail. In fact on some of the machines you will have two watchdogs,
>> one from the SoC and that "special" one.
>> That has several reasons, probably not too important here. The HW guys
>> are adding another wd not just for fun, and it would be nice to have a
>> driver.
>>
>>> The code smells a bit like two entirely different wdt's that just have
>>> some similarities. If that's the case, I'd rather split it into two
>>> separate drivers and let the parent driver (board file) instantiate
>>> the correct one.
>>
>> Yes, it is two. Just like for the LEDs. One version PIO-based another
>> version gpio/p2sb/mmio based.
>
> I tend to agree with Enrico that this should be two separate drivers.
>
Agreed.
Guenter
>> In fact the latter should very likely be based on that gpio pinctl,
>> whether it really needs to be a separate driver will have to be seen.
>> There are probably pros and cons for both options.
>
>
Am Thu, 1 Apr 2021 14:04:51 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Thu, Apr 1, 2021 at 1:44 PM Henning Schild
> <[email protected]> wrote:
> >
> > Am Wed, 31 Mar 2021 18:40:23 +0300
> > schrieb Andy Shevchenko <[email protected]>:
> >
> > > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> > > <[email protected]> wrote:
> > > > Am Tue, 30 Mar 2021 15:41:53 +0300
> > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > > > <[email protected]> wrote:
> > > > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > > > <[email protected]> wrote:
> > > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > > > <[email protected]> wrote:
> > > > >
> > > > > > > > > > +static struct simatic_ipc_led
> > > > > > > > > > simatic_ipc_leds_mem[] = {
> > > > > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS
> > > > > > > > > > "-1"},
> > > > > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS
> > > > > > > > > > "-1"},
> > > > > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS
> > > > > > > > > > "-2"},
> > > > > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS
> > > > > > > > > > "-2"},
> > > > > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS
> > > > > > > > > > "-3"},
> > > > > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS
> > > > > > > > > > "-3"},
> > > > > > > > > > + { }
> > > > > > > > > > +};
> > > > > > > > >
> > > > > > > > > It seems to me like poking GPIO controller registers
> > > > > > > > > directly. This is not good. The question still
> > > > > > > > > remains: Can we simply register a GPIO (pin control)
> > > > > > > > > driver and use an LED GPIO driver with an additional
> > > > > > > > > board file that instantiates it?
> > > > > > > >
> > > > > > > > I wrote about that in reply to the cover letter. My
> > > > > > > > view is still that it would be an abstraction with only
> > > > > > > > one user, just causing work and likely not ending up as
> > > > > > > > generic as it might eventually have to be.
> > > > > > > >
> > > > > > > > The region is reserved, not sure what the problem with
> > > > > > > > the "poking" is.
> > > > > > >
> > > > > > >
> > > > > > > > Maybe i do not understand all the benefits of such a
> > > > > > > > split at this point in time. At the moment i only see
> > > > > > > > work with hardly any benefit, not just work for me but
> > > > > > > > also for maintainers. I sure do not mean to be
> > > > > > > > ignorant. Maybe you go into details and convince me or
> > > > > > > > we wait for other peoples opinions on how to proceed,
> > > > > > > > maybe there is a second user that i am not aware of?
> > > > > > > > Until i am convinced otherwise i will try to argue that
> > > > > > > > a single-user-abstraction is needless work/code, and
> > > > > > > > should be done only when actually needed.
> > > > > > >
> > > > > > > I have just read your messages (there is a cover letter
> > > > > > > and additional email which was sent lately).
> > > > > > >
> > > > > > > I would like to know what the CPU model number on that
> > > > > > > board is. Than we can continue to see what possibilities
> > > > > > > we have here.
> > > > > >
> > > > > > I guess we are talking about the one that uses memory
> > > > > > mapped, that is called an "IPC127E" and seems to have
> > > > > > either Intel Atom E3940 or E3930 which seems to be Apollo
> > > > > > Lake.
> > > > >
> > > > > Yep. And now the question, in my patch series you should have
> > > > > got the apollolake-pinctrl driver loaded (if not, we have to
> > > > > investigate why it's not being instantiated). This will give
> > > > > you a read GPIO driver.
> > > >
> > > > Ok, so there is the existing driver i asked about several times.
> > > > Thanks for pointing it out.
> > >
> > > If you remember, I asked you about the chip twice :-)
> > > I assumed that we were talking about Apollo Lake and that's why I
> > > insisted that the driver is in the kernel source tree.
> >
> > Sorry, maybe i did not get the context of your question and which of
> > the machines you asked about. Now it is clear i guess.
> >
> > >
> > > > > So, you may use regular LED GPIO on top of it
> > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > > > > I would like to understand why it can't be achieved.
> > > >
> > > > Will have a look. Unfortunately this one box is missing in my
> > > > personal collection, but let us assume that one can be
> > > > converted to that existing driver.
> > >
> > > OK!
> > >
> > > > I guess that will still mean the PIO-based part of the LED
> > > > driver will have to stay as is.
> > >
> > > Probably yes. I haven't looked into that part and I have no idea
> > > what's going on on that platform(s).
> > >
> >
> > Which i guess means the series can be reviewed as if the mmio bits
> > for that apollo lake would not be in it, maybe i will even send a
> > version without that one box. We have others in the "backlog" might
> > as well delay that one if it helps sorting out a base-line.
>
> It depends on the role of P2SB in this case.
> Shouldn't you drop that completely out from this series?
We still have one such GPIO bit in one wdt, might be "sunrisepoint".
Still have to clarify if that can maybe be dropped for a first step.
> Otherwise we have to understand what to do with it.
> It seems the best approach can be to expose the P2SB device to Linux,
> but we have to answer to Bjorn's request about region reservations.
I now have such an apollolake to play with. Having your series applied
my LEDs driver fails to alloc that mmio memory, so far so correct.
Still have to figure out how to use those GPIOs.
I was hoping to find a gpiochip in sysfs and be able to export gpios to
userland.
Enrico, does "gpio_amd_fch" show up under /sys/class/gpio as a
gpiochip? Or how to interact with that driver before basing another one
on top?
I am afraid that pinctrl-broxton might be loaded but not working as
expected.
Henning
Am Thu, 1 Apr 2021 18:15:41 +0200
schrieb "Enrico Weigelt, metux IT consult" <[email protected]>:
> On 29.03.21 19:49, Henning Schild wrote:
>
> Hi,
>
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.
>
> Where does the wdt actually come from ?
>
> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty
> usual case.
>
> Or some external chip ?
>
> The code smells a bit like two entirely different wdt's that just have
> some similarities. If that's the case, I'd rather split it into two
> separate drivers and let the parent driver (board file) instantiate
> the correct one.
In fact they are the same watchdog device. The only difference is the
"secondary enable" which controls whether the watchdog causes a reboot
or just raises an alarm. The alarm feature is not even implemented in
the given driver, we just enable that secondary enable regardless.
In one range of devices (227E) that second enable is part of a
pio-based control register. On the other range (427E) it unfortunately
is a P2SB gpio, which gets us right into the discussion we have around
the LEDs.
With that i have my doubts that two drivers would be the way to go,
most likely not.
Only that i have no clue which pinctrl driver should be used here. My
guess is "sunrisepoint" because the CPUs are "SkyLake" i.e. i5-6442EQ,
i3-6102E
And "grep INT344B /sys/firmware/acpi/tables/DSDT" matches. I booted a
kernel patched with the series from Andy but the "pinctrl-sunrisepoint"
does not seem to even claim the memory. Still trying to understand how
to make use of these pinctrl drivers they are in place but i lack
example users (drivers). If they should be available in sysfs, i might
be looking at the wrong place ... /sys/class/gpio/ does not list
anything
regards,
Henning
>
> --mtx
>
On 4/12/21 8:35 AM, Henning Schild wrote:
> Am Thu, 1 Apr 2021 18:15:41 +0200
> schrieb "Enrico Weigelt, metux IT consult" <[email protected]>:
>
>> On 29.03.21 19:49, Henning Schild wrote:
>>
>> Hi,
>>
>>> This driver adds initial support for several devices from Siemens.
>>> It is based on a platform driver introduced in an earlier commit.
>>
>> Where does the wdt actually come from ?
>>
>> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a pretty
>> usual case.
>>
>> Or some external chip ?
>>
>> The code smells a bit like two entirely different wdt's that just have
>> some similarities. If that's the case, I'd rather split it into two
>> separate drivers and let the parent driver (board file) instantiate
>> the correct one.
>
> In fact they are the same watchdog device. The only difference is the
> "secondary enable" which controls whether the watchdog causes a reboot
> or just raises an alarm. The alarm feature is not even implemented in
> the given driver, we just enable that secondary enable regardless.
>
Confusing statement; I can't parse "we just enable that secondary enable
regardless". What secondary enable do you enable ?
The code says "set safe_en_n so we are not just WDIOF_ALARMONLY", which
suggests that it disables the alarm feature, and does make sense.
> In one range of devices (227E) that second enable is part of a
> pio-based control register. On the other range (427E) it unfortunately
> is a P2SB gpio, which gets us right into the discussion we have around
> the LEDs.
> With that i have my doubts that two drivers would be the way to go,
> most likely not.
>
Reading the code again, I agree. Still, you'll need to sort out how
to determine if the watchdog or the LED driver should be enabled,
and how to access the gpio port. The GPIO pin detection and use
for 427E is a bit awkward.
Thanks,
Guenter
> Only that i have no clue which pinctrl driver should be used here. My
> guess is "sunrisepoint" because the CPUs are "SkyLake" i.e. i5-6442EQ,
> i3-6102E
> And "grep INT344B /sys/firmware/acpi/tables/DSDT" matches. I booted a
> kernel patched with the series from Andy but the "pinctrl-sunrisepoint"
> does not seem to even claim the memory. Still trying to understand how
> to make use of these pinctrl drivers they are in place but i lack
> example users (drivers). If they should be available in sysfs, i might
> be looking at the wrong place ... /sys/class/gpio/ does not list
> anything
>
> regards,
> Henning
>
>
>
>>
>> --mtx
>>
>
Am Mon, 12 Apr 2021 09:06:10 -0700
schrieb Guenter Roeck <[email protected]>:
> On 4/12/21 8:35 AM, Henning Schild wrote:
> > Am Thu, 1 Apr 2021 18:15:41 +0200
> > schrieb "Enrico Weigelt, metux IT consult" <[email protected]>:
> >
> >> On 29.03.21 19:49, Henning Schild wrote:
> >>
> >> Hi,
> >>
> >>> This driver adds initial support for several devices from Siemens.
> >>> It is based on a platform driver introduced in an earlier commit.
> >>>
> >>
> >> Where does the wdt actually come from ?
> >>
> >> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a
> >> pretty usual case.
> >>
> >> Or some external chip ?
> >>
> >> The code smells a bit like two entirely different wdt's that just
> >> have some similarities. If that's the case, I'd rather split it
> >> into two separate drivers and let the parent driver (board file)
> >> instantiate the correct one.
> >
> > In fact they are the same watchdog device. The only difference is
> > the "secondary enable" which controls whether the watchdog causes a
> > reboot or just raises an alarm. The alarm feature is not even
> > implemented in the given driver, we just enable that secondary
> > enable regardless.
>
> Confusing statement; I can't parse "we just enable that secondary
> enable regardless". What secondary enable do you enable ?
>
> The code says "set safe_en_n so we are not just WDIOF_ALARMONLY",
> which suggests that it disables the alarm feature, and does make
> sense.
Yes go with the second statement. But the alarm is the default after
boot, and turning it off needs to be done with p2sb gpio on the 427.
> > In one range of devices (227E) that second enable is part of a
> > pio-based control register. On the other range (427E) it
> > unfortunately is a P2SB gpio, which gets us right into the
> > discussion we have around the LEDs.
> > With that i have my doubts that two drivers would be the way to go,
> > most likely not.
> >
>
> Reading the code again, I agree. Still, you'll need to sort out how
> to determine if the watchdog or the LED driver should be enabled,
> and how to access the gpio port. The GPIO pin detection and use
> for 427E is a bit awkward.
Yes it is awkward, and that is exactly the discussion happening for the
LEDs. Using generic GPIO code, the mail was more to Andy as i am hoping
he might help me connect the dots here. On the other hand i wanted wdt
discussions in the wdt thread, and not talk about that one gpio-pin in
the LED thread.
regards,
Henning
> Thanks,
> Guenter
>
> > Only that i have no clue which pinctrl driver should be used here.
> > My guess is "sunrisepoint" because the CPUs are "SkyLake" i.e.
> > i5-6442EQ, i3-6102E
> > And "grep INT344B /sys/firmware/acpi/tables/DSDT" matches. I booted
> > a kernel patched with the series from Andy but the
> > "pinctrl-sunrisepoint" does not seem to even claim the memory.
> > Still trying to understand how to make use of these pinctrl drivers
> > they are in place but i lack example users (drivers). If they
> > should be available in sysfs, i might be looking at the wrong place
> > ... /sys/class/gpio/ does not list anything
> >
> > regards,
> > Henning
> >
> >
> >
> >>
> >> --mtx
> >>
> >
>
Am Wed, 7 Apr 2021 13:36:40 +0200
schrieb Hans de Goede <[email protected]>:
> Hi,
>
> On 3/29/21 7:49 PM, Henning Schild wrote:
> > changes since v2:
> >
> > - remove "simatic-ipc" prefix from LED names
> > - fix style issues found in v2, mainly LED driver
> > - fix OEM specific dmi code, and remove magic numbers
> > - more "simatic_ipc" name prefixing
> > - improved pmc quirk code using callbacks
> >
> > changes since v1:
> >
> > - fixed lots of style issues found in v1
> > - (debug) printing
> > - header ordering
> > - fixed license issues GPLv2 and SPDX in all files
> > - module_platform_driver instead of __init __exit
> > - wdt simplifications cleanup
> > - lots of fixes in wdt driver, all that was found in v1
> > - fixed dmi length in dmi helper
> > - changed LED names to allowed ones
> > - move led driver to simple/
> > - switched pmc_atom to dmi callback with global variable
> >
> > --
> >
> > This series adds support for watchdogs and leds of several x86
> > devices from Siemens.
> >
> > It is structured with a platform driver that mainly does
> > identification of the machines. It might trigger loading of the
> > actual device drivers by attaching devices to the platform bus.
> >
> > The identification is vendor specific, parsing a special binary DMI
> > entry. The implementation of that platform identification is
> > applied on pmc_atom clock quirks in the final patch.
> >
> > It is all structured in a way that we can easily add more devices
> > and more platform drivers later. Internally we have some more code
> > for hardware monitoring, more leds, watchdogs etc. This will follow
> > some day.
>
> IT seems there still is significant discussion surrounding the LED
> and watchdog drivers which use patch 1/4 as parent-driver.
>
> I'm going to hold of on merging 1/4 and 4/4 until there is more
> consensus surrounding this series.
Yes. Whithout 2 and 3, 1 would be way too big.
Henning
> Regards,
>
> Hans
>
>
> >
> > Henning Schild (4):
> > platform/x86: simatic-ipc: add main driver for Siemens devices
> > leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
> > watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial
> > PCs platform/x86: pmc_atom: improve critclk_systems matching for
> > Siemens PCs
> >
> > drivers/leds/Kconfig | 3 +
> > drivers/leds/Makefile | 3 +
> > drivers/leds/simple/Kconfig | 11 +
> > drivers/leds/simple/Makefile | 2 +
> > drivers/leds/simple/simatic-ipc-leds.c | 202
> > ++++++++++++++++ drivers/platform/x86/Kconfig |
> > 12 + drivers/platform/x86/Makefile | 3 +
> > drivers/platform/x86/pmc_atom.c | 57 +++--
> > drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++
> > drivers/watchdog/Kconfig | 11 +
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/simatic-ipc-wdt.c | 215
> > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h |
> > 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++
> > 14 files changed, 769 insertions(+), 21 deletions(-)
> > create mode 100644 drivers/leds/simple/Kconfig
> > create mode 100644 drivers/leds/simple/Makefile
> > create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> > create mode 100644 drivers/platform/x86/simatic-ipc.c
> > create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> > create mode 100644
> > include/linux/platform_data/x86/simatic-ipc-base.h create mode
> > 100644 include/linux/platform_data/x86/simatic-ipc.h
>
Am Thu, 1 Apr 2021 14:04:51 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Thu, Apr 1, 2021 at 1:44 PM Henning Schild
> <[email protected]> wrote:
> >
> > Am Wed, 31 Mar 2021 18:40:23 +0300
> > schrieb Andy Shevchenko <[email protected]>:
> >
> > > On Tue, Mar 30, 2021 at 6:33 PM Henning Schild
> > > <[email protected]> wrote:
> > > > Am Tue, 30 Mar 2021 15:41:53 +0300
> > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > On Tue, Mar 30, 2021 at 3:35 PM Henning Schild
> > > > > <[email protected]> wrote:
> > > > > > Am Tue, 30 Mar 2021 15:15:16 +0300
> > > > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > > > On Tue, Mar 30, 2021 at 2:58 PM Henning Schild
> > > > > > > <[email protected]> wrote:
> > > > > > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > > > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > > > > > <[email protected]> wrote:
> > > > >
> > > > > > > > > > +static struct simatic_ipc_led
> > > > > > > > > > simatic_ipc_leds_mem[] = {
> > > > > > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS
> > > > > > > > > > "-1"},
> > > > > > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS
> > > > > > > > > > "-1"},
> > > > > > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS
> > > > > > > > > > "-2"},
> > > > > > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS
> > > > > > > > > > "-2"},
> > > > > > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS
> > > > > > > > > > "-3"},
> > > > > > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS
> > > > > > > > > > "-3"},
> > > > > > > > > > + { }
> > > > > > > > > > +};
> > > > > > > > >
> > > > > > > > > It seems to me like poking GPIO controller registers
> > > > > > > > > directly. This is not good. The question still
> > > > > > > > > remains: Can we simply register a GPIO (pin control)
> > > > > > > > > driver and use an LED GPIO driver with an additional
> > > > > > > > > board file that instantiates it?
> > > > > > > >
> > > > > > > > I wrote about that in reply to the cover letter. My
> > > > > > > > view is still that it would be an abstraction with only
> > > > > > > > one user, just causing work and likely not ending up as
> > > > > > > > generic as it might eventually have to be.
> > > > > > > >
> > > > > > > > The region is reserved, not sure what the problem with
> > > > > > > > the "poking" is.
> > > > > > >
> > > > > > >
> > > > > > > > Maybe i do not understand all the benefits of such a
> > > > > > > > split at this point in time. At the moment i only see
> > > > > > > > work with hardly any benefit, not just work for me but
> > > > > > > > also for maintainers. I sure do not mean to be
> > > > > > > > ignorant. Maybe you go into details and convince me or
> > > > > > > > we wait for other peoples opinions on how to proceed,
> > > > > > > > maybe there is a second user that i am not aware of?
> > > > > > > > Until i am convinced otherwise i will try to argue that
> > > > > > > > a single-user-abstraction is needless work/code, and
> > > > > > > > should be done only when actually needed.
> > > > > > >
> > > > > > > I have just read your messages (there is a cover letter
> > > > > > > and additional email which was sent lately).
> > > > > > >
> > > > > > > I would like to know what the CPU model number on that
> > > > > > > board is. Than we can continue to see what possibilities
> > > > > > > we have here.
> > > > > >
> > > > > > I guess we are talking about the one that uses memory
> > > > > > mapped, that is called an "IPC127E" and seems to have
> > > > > > either Intel Atom E3940 or E3930 which seems to be Apollo
> > > > > > Lake.
> > > > >
> > > > > Yep. And now the question, in my patch series you should have
> > > > > got the apollolake-pinctrl driver loaded (if not, we have to
> > > > > investigate why it's not being instantiated). This will give
> > > > > you a read GPIO driver.
> > > >
> > > > Ok, so there is the existing driver i asked about several times.
> > > > Thanks for pointing it out.
> > >
> > > If you remember, I asked you about the chip twice :-)
> > > I assumed that we were talking about Apollo Lake and that's why I
> > > insisted that the driver is in the kernel source tree.
> >
> > Sorry, maybe i did not get the context of your question and which of
> > the machines you asked about. Now it is clear i guess.
> >
> > >
> > > > > So, you may use regular LED GPIO on top of it
> > > > > (https://elixir.bootlin.com/linux/latest/source/drivers/leds/leds-gpio.c).
> > > > > I would like to understand why it can't be achieved.
> > > >
> > > > Will have a look. Unfortunately this one box is missing in my
> > > > personal collection, but let us assume that one can be
> > > > converted to that existing driver.
> > >
> > > OK!
> > >
> > > > I guess that will still mean the PIO-based part of the LED
> > > > driver will have to stay as is.
> > >
> > > Probably yes. I haven't looked into that part and I have no idea
> > > what's going on on that platform(s).
> > >
> >
> > Which i guess means the series can be reviewed as if the mmio bits
> > for that apollo lake would not be in it, maybe i will even send a
> > version without that one box. We have others in the "backlog" might
> > as well delay that one if it helps sorting out a base-line.
>
> It depends on the role of P2SB in this case.
> Shouldn't you drop that completely out from this series?
Unfortunately the WDT uses one P2SB-GPIO pin as well (for 1 of the two
machine types it supports). Dropping would mean loosing 1/5 machines in
LED and 2/4 in WDT. So i rather let this series sit until the P2SB
stuff is sorted out.
But that would just be my personal "preference". We could go "divide
and conquer", shrink the number of supported devices and drop all that
needs P2SB ... also a valid way, we have the platform-abstraction to
build upon ... and we would get p4 out of the way.
Henning
> Otherwise we have to understand what to do with it.
> It seems the best approach can be to expose the P2SB device to Linux,
> but we have to answer to Bjorn's request about region reservations.
>
>
On 12.04.21 13:56, Henning Schild wrote:
> Enrico, does "gpio_amd_fch" show up under /sys/class/gpio as a
> gpiochip? Or how to interact with that driver before basing another one
> on top?
It's not probed on its own, but explicitly by a board specific driver,
as it needs board specific data.
See drivers/platform/x86/pcengines-apuv2.c
--mtx
--
---
Hinweis: unverschlüsselte E-Mails können leicht abgehört und manipuliert
werden ! Für eine vertrauliche Kommunikation senden Sie bitte ihren
GPG/PGP-Schlüssel zu.
---
Enrico Weigelt, metux IT consult
Free software and Linux embedded engineering
[email protected] -- +49-151-27565287
This series is basically stuck because people rightfully want me to use
the GPIO subsystem for the LEDs and the watchdog bits that are
connected to GPIO.
Problem is that the GPIO subsystem does not initialize on the machines
in question. It is a combination of hidden P2SB and missing ACPI table
entries. The GPIO subsystem (intel pinctrl) needs either P2SB or ACPI do
come up ...
Andy proposed some patches for initializing the intel pinctrl stuff for
one of the machines by falling back to SoC detection in case there is
no ACPI or visible P2SB. While that works it would need to be done for
any Intel SoC to be consistent and discussions seem to go nowhere.
I would be willing to port over to "intel pintctl" and help with
testing, but not so much with actual coding. Andy is that moving at all?
Since my drivers do reserve the mmio regions properly and the intel
pinctrl will never come up anyways, i do not see a conflict merging my
proposed drivers in the current codebase. The wish to use the pinctrl
infrastructure can not be fulfilled if that infra is not in place. Once
intel pinctrl works, we can change those drivers to work with that.
I do not want to take shortcuts ... but also do not want to get stuck
here. So maybe one way to serialize the merge is to allow my changes
like proposed and rebase on intel pinctrl once that subsystem actually
initializes on these machines. We could even have two code paths ... if
region can not be reserved, try gpio ... or the other way around.
regards,
Henning
Am Wed, 7 Apr 2021 13:36:40 +0200
schrieb Hans de Goede <[email protected]>:
> Hi,
>
> On 3/29/21 7:49 PM, Henning Schild wrote:
> > changes since v2:
> >
> > - remove "simatic-ipc" prefix from LED names
> > - fix style issues found in v2, mainly LED driver
> > - fix OEM specific dmi code, and remove magic numbers
> > - more "simatic_ipc" name prefixing
> > - improved pmc quirk code using callbacks
> >
> > changes since v1:
> >
> > - fixed lots of style issues found in v1
> > - (debug) printing
> > - header ordering
> > - fixed license issues GPLv2 and SPDX in all files
> > - module_platform_driver instead of __init __exit
> > - wdt simplifications cleanup
> > - lots of fixes in wdt driver, all that was found in v1
> > - fixed dmi length in dmi helper
> > - changed LED names to allowed ones
> > - move led driver to simple/
> > - switched pmc_atom to dmi callback with global variable
> >
> > --
> >
> > This series adds support for watchdogs and leds of several x86
> > devices from Siemens.
> >
> > It is structured with a platform driver that mainly does
> > identification of the machines. It might trigger loading of the
> > actual device drivers by attaching devices to the platform bus.
> >
> > The identification is vendor specific, parsing a special binary DMI
> > entry. The implementation of that platform identification is
> > applied on pmc_atom clock quirks in the final patch.
> >
> > It is all structured in a way that we can easily add more devices
> > and more platform drivers later. Internally we have some more code
> > for hardware monitoring, more leds, watchdogs etc. This will follow
> > some day.
>
> IT seems there still is significant discussion surrounding the LED
> and watchdog drivers which use patch 1/4 as parent-driver.
>
> I'm going to hold of on merging 1/4 and 4/4 until there is more
> consensus surrounding this series.
>
> Regards,
>
> Hans
>
>
> >
> > Henning Schild (4):
> > platform/x86: simatic-ipc: add main driver for Siemens devices
> > leds: simatic-ipc-leds: add new driver for Siemens Industial PCs
> > watchdog: simatic-ipc-wdt: add new driver for Siemens Industrial
> > PCs platform/x86: pmc_atom: improve critclk_systems matching for
> > Siemens PCs
> >
> > drivers/leds/Kconfig | 3 +
> > drivers/leds/Makefile | 3 +
> > drivers/leds/simple/Kconfig | 11 +
> > drivers/leds/simple/Makefile | 2 +
> > drivers/leds/simple/simatic-ipc-leds.c | 202
> > ++++++++++++++++ drivers/platform/x86/Kconfig |
> > 12 + drivers/platform/x86/Makefile | 3 +
> > drivers/platform/x86/pmc_atom.c | 57 +++--
> > drivers/platform/x86/simatic-ipc.c | 169 ++++++++++++++
> > drivers/watchdog/Kconfig | 11 +
> > drivers/watchdog/Makefile | 1 +
> > drivers/watchdog/simatic-ipc-wdt.c | 215
> > ++++++++++++++++++ .../platform_data/x86/simatic-ipc-base.h |
> > 29 +++ include/linux/platform_data/x86/simatic-ipc.h | 72 ++++++
> > 14 files changed, 769 insertions(+), 21 deletions(-)
> > create mode 100644 drivers/leds/simple/Kconfig
> > create mode 100644 drivers/leds/simple/Makefile
> > create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
> > create mode 100644 drivers/platform/x86/simatic-ipc.c
> > create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
> > create mode 100644
> > include/linux/platform_data/x86/simatic-ipc-base.h create mode
> > 100644 include/linux/platform_data/x86/simatic-ipc.h
>
On Mon, Jul 12, 2021 at 2:35 PM Henning Schild
<[email protected]> wrote:
>
> This series is basically stuck because people rightfully want me to use
> the GPIO subsystem for the LEDs and the watchdog bits that are
> connected to GPIO.
>
> Problem is that the GPIO subsystem does not initialize on the machines
> in question. It is a combination of hidden P2SB and missing ACPI table
> entries. The GPIO subsystem (intel pinctrl) needs either P2SB or ACPI do
> come up ...
>
> Andy proposed some patches for initializing the intel pinctrl stuff for
> one of the machines by falling back to SoC detection in case there is
> no ACPI or visible P2SB. While that works it would need to be done for
> any Intel SoC to be consistent and discussions seem to go nowhere.
>
> I would be willing to port over to "intel pintctl" and help with
> testing, but not so much with actual coding. Andy is that moving at all?
>
> Since my drivers do reserve the mmio regions properly and the intel
> pinctrl will never come up anyways, i do not see a conflict merging my
> proposed drivers in the current codebase. The wish to use the pinctrl
> infrastructure can not be fulfilled if that infra is not in place. Once
> intel pinctrl works, we can change those drivers to work with that.
>
> I do not want to take shortcuts ... but also do not want to get stuck
> here. So maybe one way to serialize the merge is to allow my changes
> like proposed and rebase on intel pinctrl once that subsystem actually
> initializes on these machines. We could even have two code paths ... if
> region can not be reserved, try gpio ... or the other way around.
Bjorn suggested exercising the IORESOURCE_PCI_FIXED on top of the
early PCI quirk that unhides P2SB for the entire run time. But I have
had no time to actually patch the kernel this way. Have tried the
proposed approach on your side?
--
With Best Regards,
Andy Shevchenko
Am Mon, 12 Jul 2021 15:09:04 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Mon, Jul 12, 2021 at 2:35 PM Henning Schild
> <[email protected]> wrote:
> >
> > This series is basically stuck because people rightfully want me to
> > use the GPIO subsystem for the LEDs and the watchdog bits that are
> > connected to GPIO.
> >
> > Problem is that the GPIO subsystem does not initialize on the
> > machines in question. It is a combination of hidden P2SB and
> > missing ACPI table entries. The GPIO subsystem (intel pinctrl)
> > needs either P2SB or ACPI do come up ...
> >
> > Andy proposed some patches for initializing the intel pinctrl stuff
> > for one of the machines by falling back to SoC detection in case
> > there is no ACPI or visible P2SB. While that works it would need to
> > be done for any Intel SoC to be consistent and discussions seem to
> > go nowhere.
> >
> > I would be willing to port over to "intel pintctl" and help with
> > testing, but not so much with actual coding. Andy is that moving at
> > all?
> >
> > Since my drivers do reserve the mmio regions properly and the intel
> > pinctrl will never come up anyways, i do not see a conflict merging
> > my proposed drivers in the current codebase. The wish to use the
> > pinctrl infrastructure can not be fulfilled if that infra is not in
> > place. Once intel pinctrl works, we can change those drivers to
> > work with that.
> >
> > I do not want to take shortcuts ... but also do not want to get
> > stuck here. So maybe one way to serialize the merge is to allow my
> > changes like proposed and rebase on intel pinctrl once that
> > subsystem actually initializes on these machines. We could even
> > have two code paths ... if region can not be reserved, try gpio ...
> > or the other way around.
>
> Bjorn suggested exercising the IORESOURCE_PCI_FIXED on top of the
> early PCI quirk that unhides P2SB for the entire run time. But I have
> had no time to actually patch the kernel this way. Have tried the
> proposed approach on your side?
Unhiding the P2SB (even if permanent and fixed) alone will not trigger
pinctrl to initialize. One would still need something along the lines
of "mfd: lpc_ich: Add support for pinctrl in non-ACPI system" for all
SoCs.
I guess it could be an improvement to your series, but i honestly do
not see all fitting together too soon. Since your p2sb series still
initializes the GPIO with two different names (depending on whether it
was PCI or ACPI) and only for one SoC, while this series would need two
... and a consistent solution many more.
Henning
Am Mon, 29 Mar 2021 19:49:27 +0200
schrieb Henning Schild <[email protected]>:
> This driver adds initial support for several devices from Siemens. It
> is based on a platform driver introduced in an earlier commit.
>
> Signed-off-by: Henning Schild <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/Kconfig | 11 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/simatic-ipc-wdt.c | 215
> +++++++++++++++++++++++++++++ 3 files changed, 227 insertions(+)
> create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 1fe0042a48d2..948497eb4bef 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1575,6 +1575,17 @@ config NIC7018_WDT
> To compile this driver as a module, choose M here: the
> module will be called nic7018_wdt.
>
> +config SIEMENS_SIMATIC_IPC_WDT
> + tristate "Siemens Simatic IPC Watchdog"
> + depends on SIEMENS_SIMATIC_IPC
> + select WATCHDOG_CORE
> + help
> + This driver adds support for several watchdogs found in
> Industrial
> + PCs from Siemens.
> +
> + To compile this driver as a module, choose M here: the
> module will be
> + called simatic-ipc-wdt.
> +
> # M68K Architecture
>
> config M54xx_WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f3a6540e725e..7f5c73ec058c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
> obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
> obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
> obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o
> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o
>
> # M68K Architecture
> obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o
> diff --git a/drivers/watchdog/simatic-ipc-wdt.c
> b/drivers/watchdog/simatic-ipc-wdt.c new file mode 100644
> index 000000000000..e901718d05b9
> --- /dev/null
> +++ b/drivers/watchdog/simatic-ipc-wdt.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Siemens SIMATIC IPC driver for Watchdogs
> + *
> + * Copyright (c) Siemens AG, 2020-2021
> + *
> + * Authors:
> + * Gerd Haeussler <[email protected]>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/util_macros.h>
> +#include <linux/watchdog.h>
> +
> +#define WD_ENABLE_IOADR 0x62
> +#define WD_TRIGGER_IOADR 0x66
> +#define GPIO_COMMUNITY0_PORT_ID 0xaf
> +#define PAD_CFG_DW0_GPP_A_23 0x4b8
> +#define SAFE_EN_N_427E 0x01
> +#define SAFE_EN_N_227E 0x04
> +#define WD_ENABLED 0x01
> +#define WD_TRIGGERED 0x80
> +#define WD_MACROMODE 0x02
> +
> +#define TIMEOUT_MIN 2
> +#define TIMEOUT_DEF 64
> +#define TIMEOUT_MAX 64
> +
> +#define GP_STATUS_REG_227E 0x404D /* IO PORT for
> SAFE_EN_N on 227E */ +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0000);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
> (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static struct resource gp_status_reg_227e_res =
> + DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1,
> KBUILD_MODNAME); +
> +static struct resource io_resource =
> + DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1,
> + KBUILD_MODNAME " WD_ENABLE_IOADR");
> +
> +/* the actual start will be discovered with pci, 0 is a placeholder
> */ +static struct resource mem_resource =
> + DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
> +
> +static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
> +static void __iomem *wd_reset_base_addr;
> +
> +static int wd_start(struct watchdog_device *wdd)
> +{
> + outb(inb(WD_ENABLE_IOADR) | WD_ENABLED, WD_ENABLE_IOADR);
> + return 0;
> +}
> +
> +static int wd_stop(struct watchdog_device *wdd)
> +{
> + outb(inb(WD_ENABLE_IOADR) & ~WD_ENABLED, WD_ENABLE_IOADR);
> + return 0;
> +}
> +
> +static int wd_ping(struct watchdog_device *wdd)
> +{
> + inb(WD_TRIGGER_IOADR);
> + return 0;
> +}
> +
> +static int wd_set_timeout(struct watchdog_device *wdd, unsigned int
> t) +{
> + int timeout_idx = find_closest(t, wd_timeout_table,
> + ARRAY_SIZE(wd_timeout_table));
> +
> + outb((inb(WD_ENABLE_IOADR) & 0xc7) | timeout_idx << 3,
> WD_ENABLE_IOADR);
> + wdd->timeout = wd_timeout_table[timeout_idx];
> + return 0;
> +}
> +
> +static const struct watchdog_info wdt_ident = {
> + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> + WDIOF_SETTIMEOUT,
> + .identity = KBUILD_MODNAME,
> +};
> +
> +static const struct watchdog_ops wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = wd_start,
> + .stop = wd_stop,
> + .ping = wd_ping,
> + .set_timeout = wd_set_timeout,
> +};
> +
> +static void wd_secondary_enable(u32 wdtmode)
> +{
> + u16 resetbit;
> +
> + /* set safe_en_n so we are not just WDIOF_ALARMONLY */
> + if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
> + /* enable SAFE_EN_N on GP_STATUS_REG_227E */
> + resetbit = inw(GP_STATUS_REG_227E);
> + outw(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E);
Should be inb/outb we just have an SZ_1 region.
Henning
> + } else {
> + /* enable SAFE_EN_N on PCH D1600 */
> + resetbit = ioread16(wd_reset_base_addr);
> + iowrite16(resetbit & ~SAFE_EN_N_427E,
> wd_reset_base_addr);
> + }
> +}
> +
> +static int wd_setup(u32 wdtmode)
> +{
> + unsigned int bootstatus = 0;
> + int timeout_idx;
> +
> + timeout_idx = find_closest(TIMEOUT_DEF, wd_timeout_table,
> + ARRAY_SIZE(wd_timeout_table));
> +
> + if (inb(WD_ENABLE_IOADR) & WD_TRIGGERED)
> + bootstatus |= WDIOF_CARDRESET;
> +
> + /* reset alarm bit, set macro mode, and set timeout */
> + outb(WD_TRIGGERED | WD_MACROMODE | timeout_idx << 3,
> WD_ENABLE_IOADR); +
> + wd_secondary_enable(wdtmode);
> +
> + return bootstatus;
> +}
> +
> +static struct watchdog_device wdd_data = {
> + .info = &wdt_ident,
> + .ops = &wdt_ops,
> + .min_timeout = TIMEOUT_MIN,
> + .max_timeout = TIMEOUT_MAX
> +};
> +
> +static int simatic_ipc_wdt_probe(struct platform_device *pdev)
> +{
> + struct simatic_ipc_platform *plat = pdev->dev.platform_data;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> +
> + switch (plat->devmode) {
> + case SIMATIC_IPC_DEVICE_227E:
> + if (!devm_request_region(dev,
> gp_status_reg_227e_res.start,
> +
> resource_size(&gp_status_reg_227e_res),
> + KBUILD_MODNAME)) {
> + dev_err(dev,
> + "Unable to register IO resource at
> %pR\n",
> + &gp_status_reg_227e_res);
> + return -EBUSY;
> + }
> + fallthrough;
> + case SIMATIC_IPC_DEVICE_427E:
> + wdd_data.parent = dev;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (!devm_request_region(dev, io_resource.start,
> + resource_size(&io_resource),
> + io_resource.name)) {
> + dev_err(dev,
> + "Unable to register IO resource at %#x\n",
> + WD_ENABLE_IOADR);
> + return -EBUSY;
> + }
> +
> + if (plat->devmode == SIMATIC_IPC_DEVICE_427E) {
> + res = &mem_resource;
> +
> + /* get GPIO base from PCI */
> + res->start =
> simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1));
> + if (res->start == 0)
> + return -ENODEV;
> +
> + /* do the final address calculation */
> + res->start = res->start + (GPIO_COMMUNITY0_PORT_ID
> << 16) +
> + PAD_CFG_DW0_GPP_A_23;
> + res->end += res->start;
> +
> + wd_reset_base_addr = devm_ioremap_resource(dev, res);
> + if (IS_ERR(wd_reset_base_addr))
> + return PTR_ERR(wd_reset_base_addr);
> + }
> +
> + wdd_data.bootstatus = wd_setup(plat->devmode);
> + if (wdd_data.bootstatus)
> + dev_warn(dev, "last reboot caused by watchdog
> reset\n"); +
> + watchdog_set_nowayout(&wdd_data, nowayout);
> + watchdog_stop_on_reboot(&wdd_data);
> + return devm_watchdog_register_device(dev, &wdd_data);
> +}
> +
> +static struct platform_driver simatic_ipc_wdt_driver = {
> + .probe = simatic_ipc_wdt_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + },
> +};
> +
> +module_platform_driver(simatic_ipc_wdt_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_AUTHOR("Gerd Haeussler <[email protected]>");
Am Mon, 29 Mar 2021 19:49:27 +0200
schrieb Henning Schild <[email protected]>:
> This driver adds initial support for several devices from Siemens. It
> is based on a platform driver introduced in an earlier commit.
>
> Signed-off-by: Henning Schild <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/Kconfig | 11 ++
> drivers/watchdog/Makefile | 1 +
> drivers/watchdog/simatic-ipc-wdt.c | 215
> +++++++++++++++++++++++++++++ 3 files changed, 227 insertions(+)
> create mode 100644 drivers/watchdog/simatic-ipc-wdt.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 1fe0042a48d2..948497eb4bef 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1575,6 +1575,17 @@ config NIC7018_WDT
> To compile this driver as a module, choose M here: the
> module will be called nic7018_wdt.
>
> +config SIEMENS_SIMATIC_IPC_WDT
> + tristate "Siemens Simatic IPC Watchdog"
> + depends on SIEMENS_SIMATIC_IPC
> + select WATCHDOG_CORE
> + help
> + This driver adds support for several watchdogs found in
> Industrial
> + PCs from Siemens.
> +
> + To compile this driver as a module, choose M here: the
> module will be
> + called simatic-ipc-wdt.
> +
> # M68K Architecture
>
> config M54xx_WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index f3a6540e725e..7f5c73ec058c 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -142,6 +142,7 @@ obj-$(CONFIG_NI903X_WDT) += ni903x_wdt.o
> obj-$(CONFIG_NIC7018_WDT) += nic7018_wdt.o
> obj-$(CONFIG_MLX_WDT) += mlx_wdt.o
> obj-$(CONFIG_KEEMBAY_WATCHDOG) += keembay_wdt.o
> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC_WDT) += simatic-ipc-wdt.o
>
> # M68K Architecture
> obj-$(CONFIG_M54xx_WATCHDOG) += m54xx_wdt.o
> diff --git a/drivers/watchdog/simatic-ipc-wdt.c
> b/drivers/watchdog/simatic-ipc-wdt.c new file mode 100644
> index 000000000000..e901718d05b9
> --- /dev/null
> +++ b/drivers/watchdog/simatic-ipc-wdt.c
> @@ -0,0 +1,215 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Siemens SIMATIC IPC driver for Watchdogs
> + *
> + * Copyright (c) Siemens AG, 2020-2021
> + *
> + * Authors:
> + * Gerd Haeussler <[email protected]>
> + */
> +
> +#include <linux/device.h>
> +#include <linux/errno.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/util_macros.h>
> +#include <linux/watchdog.h>
> +
> +#define WD_ENABLE_IOADR 0x62
> +#define WD_TRIGGER_IOADR 0x66
> +#define GPIO_COMMUNITY0_PORT_ID 0xaf
> +#define PAD_CFG_DW0_GPP_A_23 0x4b8
> +#define SAFE_EN_N_427E 0x01
> +#define SAFE_EN_N_227E 0x04
> +#define WD_ENABLED 0x01
> +#define WD_TRIGGERED 0x80
> +#define WD_MACROMODE 0x02
> +
> +#define TIMEOUT_MIN 2
> +#define TIMEOUT_DEF 64
> +#define TIMEOUT_MAX 64
> +
> +#define GP_STATUS_REG_227E 0x404D /* IO PORT for
> SAFE_EN_N on 227E */ +
> +static bool nowayout = WATCHDOG_NOWAYOUT;
> +module_param(nowayout, bool, 0000);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started
> (default="
> + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +
> +static struct resource gp_status_reg_227e_res =
> + DEFINE_RES_IO_NAMED(GP_STATUS_REG_227E, SZ_1,
> KBUILD_MODNAME); +
> +static struct resource io_resource =
> + DEFINE_RES_IO_NAMED(WD_ENABLE_IOADR, SZ_1,
> + KBUILD_MODNAME " WD_ENABLE_IOADR");
WD_TRIGGER_IOADR, SZ_1 missing here and in request_region part
Henning
> +/* the actual start will be discovered with pci, 0 is a placeholder
> */ +static struct resource mem_resource =
> + DEFINE_RES_MEM_NAMED(0, SZ_4, "WD_RESET_BASE_ADR");
> +
> +static u32 wd_timeout_table[] = {2, 4, 6, 8, 16, 32, 48, 64 };
> +static void __iomem *wd_reset_base_addr;
> +
> +static int wd_start(struct watchdog_device *wdd)
> +{
> + outb(inb(WD_ENABLE_IOADR) | WD_ENABLED, WD_ENABLE_IOADR);
> + return 0;
> +}
> +
> +static int wd_stop(struct watchdog_device *wdd)
> +{
> + outb(inb(WD_ENABLE_IOADR) & ~WD_ENABLED, WD_ENABLE_IOADR);
> + return 0;
> +}
> +
> +static int wd_ping(struct watchdog_device *wdd)
> +{
> + inb(WD_TRIGGER_IOADR);
> + return 0;
> +}
> +
> +static int wd_set_timeout(struct watchdog_device *wdd, unsigned int
> t) +{
> + int timeout_idx = find_closest(t, wd_timeout_table,
> + ARRAY_SIZE(wd_timeout_table));
> +
> + outb((inb(WD_ENABLE_IOADR) & 0xc7) | timeout_idx << 3,
> WD_ENABLE_IOADR);
> + wdd->timeout = wd_timeout_table[timeout_idx];
> + return 0;
> +}
> +
> +static const struct watchdog_info wdt_ident = {
> + .options = WDIOF_MAGICCLOSE | WDIOF_KEEPALIVEPING |
> + WDIOF_SETTIMEOUT,
> + .identity = KBUILD_MODNAME,
> +};
> +
> +static const struct watchdog_ops wdt_ops = {
> + .owner = THIS_MODULE,
> + .start = wd_start,
> + .stop = wd_stop,
> + .ping = wd_ping,
> + .set_timeout = wd_set_timeout,
> +};
> +
> +static void wd_secondary_enable(u32 wdtmode)
> +{
> + u16 resetbit;
> +
> + /* set safe_en_n so we are not just WDIOF_ALARMONLY */
> + if (wdtmode == SIMATIC_IPC_DEVICE_227E) {
> + /* enable SAFE_EN_N on GP_STATUS_REG_227E */
> + resetbit = inw(GP_STATUS_REG_227E);
> + outw(resetbit & ~SAFE_EN_N_227E, GP_STATUS_REG_227E);
> + } else {
> + /* enable SAFE_EN_N on PCH D1600 */
> + resetbit = ioread16(wd_reset_base_addr);
> + iowrite16(resetbit & ~SAFE_EN_N_427E,
> wd_reset_base_addr);
> + }
> +}
> +
> +static int wd_setup(u32 wdtmode)
> +{
> + unsigned int bootstatus = 0;
> + int timeout_idx;
> +
> + timeout_idx = find_closest(TIMEOUT_DEF, wd_timeout_table,
> + ARRAY_SIZE(wd_timeout_table));
> +
> + if (inb(WD_ENABLE_IOADR) & WD_TRIGGERED)
> + bootstatus |= WDIOF_CARDRESET;
> +
> + /* reset alarm bit, set macro mode, and set timeout */
> + outb(WD_TRIGGERED | WD_MACROMODE | timeout_idx << 3,
> WD_ENABLE_IOADR); +
> + wd_secondary_enable(wdtmode);
> +
> + return bootstatus;
> +}
> +
> +static struct watchdog_device wdd_data = {
> + .info = &wdt_ident,
> + .ops = &wdt_ops,
> + .min_timeout = TIMEOUT_MIN,
> + .max_timeout = TIMEOUT_MAX
> +};
> +
> +static int simatic_ipc_wdt_probe(struct platform_device *pdev)
> +{
> + struct simatic_ipc_platform *plat = pdev->dev.platform_data;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> +
> + switch (plat->devmode) {
> + case SIMATIC_IPC_DEVICE_227E:
> + if (!devm_request_region(dev,
> gp_status_reg_227e_res.start,
> +
> resource_size(&gp_status_reg_227e_res),
> + KBUILD_MODNAME)) {
> + dev_err(dev,
> + "Unable to register IO resource at
> %pR\n",
> + &gp_status_reg_227e_res);
> + return -EBUSY;
> + }
> + fallthrough;
> + case SIMATIC_IPC_DEVICE_427E:
> + wdd_data.parent = dev;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + if (!devm_request_region(dev, io_resource.start,
> + resource_size(&io_resource),
> + io_resource.name)) {
> + dev_err(dev,
> + "Unable to register IO resource at %#x\n",
> + WD_ENABLE_IOADR);
> + return -EBUSY;
> + }
> +
> + if (plat->devmode == SIMATIC_IPC_DEVICE_427E) {
> + res = &mem_resource;
> +
> + /* get GPIO base from PCI */
> + res->start =
> simatic_ipc_get_membase0(PCI_DEVFN(0x1f, 1));
> + if (res->start == 0)
> + return -ENODEV;
> +
> + /* do the final address calculation */
> + res->start = res->start + (GPIO_COMMUNITY0_PORT_ID
> << 16) +
> + PAD_CFG_DW0_GPP_A_23;
> + res->end += res->start;
> +
> + wd_reset_base_addr = devm_ioremap_resource(dev, res);
> + if (IS_ERR(wd_reset_base_addr))
> + return PTR_ERR(wd_reset_base_addr);
> + }
> +
> + wdd_data.bootstatus = wd_setup(plat->devmode);
> + if (wdd_data.bootstatus)
> + dev_warn(dev, "last reboot caused by watchdog
> reset\n"); +
> + watchdog_set_nowayout(&wdd_data, nowayout);
> + watchdog_stop_on_reboot(&wdd_data);
> + return devm_watchdog_register_device(dev, &wdd_data);
> +}
> +
> +static struct platform_driver simatic_ipc_wdt_driver = {
> + .probe = simatic_ipc_wdt_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + },
> +};
> +
> +module_platform_driver(simatic_ipc_wdt_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_AUTHOR("Gerd Haeussler <[email protected]>");
Am Mon, 29 Mar 2021 19:49:26 +0200
schrieb Henning Schild <[email protected]>:
> This driver adds initial support for several devices from Siemens. It
> is based on a platform driver introduced in an earlier commit.
>
> Signed-off-by: Henning Schild <[email protected]>
> ---
> drivers/leds/Kconfig | 3 +
> drivers/leds/Makefile | 3 +
> drivers/leds/simple/Kconfig | 11 ++
> drivers/leds/simple/Makefile | 2 +
> drivers/leds/simple/simatic-ipc-leds.c | 202
> +++++++++++++++++++++++++ 5 files changed, 221 insertions(+)
> create mode 100644 drivers/leds/simple/Kconfig
> create mode 100644 drivers/leds/simple/Makefile
> create mode 100644 drivers/leds/simple/simatic-ipc-leds.c
>
> diff --git a/drivers/leds/Kconfig b/drivers/leds/Kconfig
> index b6742b4231bf..5c8558a4fa60 100644
> --- a/drivers/leds/Kconfig
> +++ b/drivers/leds/Kconfig
> @@ -937,4 +937,7 @@ source "drivers/leds/trigger/Kconfig"
> comment "LED Blink"
> source "drivers/leds/blink/Kconfig"
>
> +comment "Simple LED drivers"
> +source "drivers/leds/simple/Kconfig"
> +
> endif # NEW_LEDS
> diff --git a/drivers/leds/Makefile b/drivers/leds/Makefile
> index 2a698df9da57..2de7fdd8d629 100644
> --- a/drivers/leds/Makefile
> +++ b/drivers/leds/Makefile
> @@ -111,3 +111,6 @@ obj-$(CONFIG_LEDS_TRIGGERS) +=
> trigger/
> # LED Blink
> obj-$(CONFIG_LEDS_BLINK) += blink/
> +
> +# Simple LED drivers
> +obj-y += simple/
> diff --git a/drivers/leds/simple/Kconfig b/drivers/leds/simple/Kconfig
> new file mode 100644
> index 000000000000..9f6a68336659
> --- /dev/null
> +++ b/drivers/leds/simple/Kconfig
> @@ -0,0 +1,11 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +config LEDS_SIEMENS_SIMATIC_IPC
> + tristate "LED driver for Siemens Simatic IPCs"
> + depends on LEDS_CLASS
> + depends on SIEMENS_SIMATIC_IPC
> + help
> + This option enables support for the LEDs of several
> Industrial PCs
> + from Siemens.
> +
> + To compile this driver as a module, choose M here: the
> module
> + will be called simatic-ipc-leds.
> diff --git a/drivers/leds/simple/Makefile
> b/drivers/leds/simple/Makefile new file mode 100644
> index 000000000000..8481f1e9e360
> --- /dev/null
> +++ b/drivers/leds/simple/Makefile
> @@ -0,0 +1,2 @@
> +# SPDX-License-Identifier: GPL-2.0
> +obj-$(CONFIG_LEDS_SIEMENS_SIMATIC_IPC) += simatic-ipc-leds.o
> diff --git a/drivers/leds/simple/simatic-ipc-leds.c
> b/drivers/leds/simple/simatic-ipc-leds.c new file mode 100644
> index 000000000000..043edbf81b76
> --- /dev/null
> +++ b/drivers/leds/simple/simatic-ipc-leds.c
> @@ -0,0 +1,202 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Siemens SIMATIC IPC driver for LEDs
> + *
> + * Copyright (c) Siemens AG, 2018-2021
> + *
> + * Authors:
> + * Henning Schild <[email protected]>
> + * Jan Kiszka <[email protected]>
> + * Gerd Haeussler <[email protected]>
> + */
> +
> +#include <linux/ioport.h>
> +#include <linux/kernel.h>
> +#include <linux/leds.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>
> +#include <linux/platform_device.h>
> +#include <linux/sizes.h>
> +#include <linux/spinlock.h>
> +
> +#define SIMATIC_IPC_LED_PORT_BASE 0x404E
> +
> +struct simatic_ipc_led {
> + unsigned int value; /* mask for io and offset for mem */
> + char *name;
> + struct led_classdev cdev;
> +};
> +
> +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" },
> + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" },
> + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" },
> + { }
> +};
> +
> +/* the actual start will be discovered with PCI, 0 is a placeholder
> */ +struct resource simatic_ipc_led_mem_res = DEFINE_RES_MEM_NAMED(0,
> SZ_4K, KBUILD_MODNAME); +
> +static void *simatic_ipc_led_memory;
> +
> +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> + { }
> +};
> +
> +static struct resource simatic_ipc_led_io_res =
> + DEFINE_RES_IO_NAMED(SIMATIC_IPC_LED_PORT_BASE, SZ_1,
> KBUILD_MODNAME);
Should be SZ_2
Henning
> +static DEFINE_SPINLOCK(reg_lock);
> +
> +static inline struct simatic_ipc_led *cdev_to_led(struct
> led_classdev *led_cd) +{
> + return container_of(led_cd, struct simatic_ipc_led, cdev);
> +}
> +
> +static void simatic_ipc_led_set_io(struct led_classdev *led_cd,
> + enum led_brightness brightness)
> +{
> + struct simatic_ipc_led *led = cdev_to_led(led_cd);
> + unsigned long flags;
> + unsigned int val;
> +
> + spin_lock_irqsave(®_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(®_lock, flags);
> +}
> +
> +static enum led_brightness simatic_ipc_led_get_io(struct
> led_classdev *led_cd) +{
> + struct simatic_ipc_led *led = cdev_to_led(led_cd);
> +
> + return inw(SIMATIC_IPC_LED_PORT_BASE) & led->value ? LED_OFF
> : led_cd->max_brightness; +}
> +
> +static void simatic_ipc_led_set_mem(struct led_classdev *led_cd,
> + enum led_brightness brightness)
> +{
> + struct simatic_ipc_led *led = cdev_to_led(led_cd);
> +
> + u32 *p;
> +
> + p = simatic_ipc_led_memory + led->value;
> + *p = (*p & ~1) | (brightness == LED_OFF);
> +}
> +
> +static enum led_brightness simatic_ipc_led_get_mem(struct
> led_classdev *led_cd) +{
> + struct simatic_ipc_led *led = cdev_to_led(led_cd);
> +
> + u32 *p;
> +
> + p = simatic_ipc_led_memory + led->value;
> + return (*p & 1) ? LED_OFF : led_cd->max_brightness;
> +}
> +
> +static int simatic_ipc_leds_probe(struct platform_device *pdev)
> +{
> + const struct simatic_ipc_platform *plat =
> pdev->dev.platform_data;
> + struct device *dev = &pdev->dev;
> + struct simatic_ipc_led *ipcled;
> + struct led_classdev *cdev;
> + struct resource *res;
> + int err, type;
> + u32 *p;
> +
> + switch (plat->devmode) {
> + case SIMATIC_IPC_DEVICE_227D:
> + case SIMATIC_IPC_DEVICE_427E:
> + res = &simatic_ipc_led_io_res;
> + ipcled = simatic_ipc_leds_io;
> + /* on 227D the two bytes work the other way araound
> */
> + if (plat->devmode == SIMATIC_IPC_DEVICE_227D) {
> + while (ipcled->value) {
> + ipcled->value =
> swab16(ipcled->value);
> + ipcled++;
> + }
> + ipcled = simatic_ipc_leds_io;
> + }
> + type = IORESOURCE_IO;
> + if (!devm_request_region(dev, res->start,
> resource_size(res), KBUILD_MODNAME)) {
> + dev_err(dev, "Unable to register IO resource
> at %pR\n", res);
> + return -EBUSY;
> + }
> + break;
> + case SIMATIC_IPC_DEVICE_127E:
> + res = &simatic_ipc_led_mem_res;
> + ipcled = simatic_ipc_leds_mem;
> + type = IORESOURCE_MEM;
> +
> + /* get GPIO base from PCI */
> + res->start = simatic_ipc_get_membase0(PCI_DEVFN(13,
> 0));
> + if (res->start == 0)
> + return -ENODEV;
> +
> + /* do the final address calculation */
> + res->start = res->start + (0xC5 << 16);
> + res->end += res->start;
> +
> + simatic_ipc_led_memory = devm_ioremap_resource(dev,
> res);
> + if (IS_ERR(simatic_ipc_led_memory))
> + return PTR_ERR(simatic_ipc_led_memory);
> +
> + /* initialize power/watchdog LED */
> + p = simatic_ipc_led_memory + 0x500 + 0x1D8; /*
> PM_WDT_OUT */
> + *p = (*p & ~1);
> + p = simatic_ipc_led_memory + 0x500 + 0x1C0; /*
> PM_BIOS_BOOT_N */
> + *p = (*p | 1);
> +
> + break;
> + default:
> + return -ENODEV;
> + }
> +
> + while (ipcled->value) {
> + cdev = &ipcled->cdev;
> + if (type == IORESOURCE_MEM) {
> + cdev->brightness_set =
> simatic_ipc_led_set_mem;
> + cdev->brightness_get =
> simatic_ipc_led_get_mem;
> + } else {
> + cdev->brightness_set =
> simatic_ipc_led_set_io;
> + cdev->brightness_get =
> simatic_ipc_led_get_io;
> + }
> + cdev->max_brightness = LED_ON;
> + cdev->name = ipcled->name;
> +
> + err = devm_led_classdev_register(dev, cdev);
> + if (err < 0)
> + return err;
> + ipcled++;
> + }
> +
> + return 0;
> +}
> +
> +static struct platform_driver simatic_ipc_led_driver = {
> + .probe = simatic_ipc_leds_probe,
> + .driver = {
> + .name = KBUILD_MODNAME,
> + }
> +};
> +
> +module_platform_driver(simatic_ipc_led_driver);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("platform:" KBUILD_MODNAME);
> +MODULE_AUTHOR("Henning Schild <[email protected]>");
Am Wed, 7 Apr 2021 05:17:12 -0700
schrieb Guenter Roeck <[email protected]>:
> On 4/7/21 1:53 AM, Andy Shevchenko wrote:
> > On Tue, Apr 6, 2021 at 5:56 PM Henning Schild
> > <[email protected]> wrote:
> >>
> >> Am Thu, 1 Apr 2021 18:15:41 +0200
> >> schrieb "Enrico Weigelt, metux IT consult" <[email protected]>:
> >>
> >>> On 29.03.21 19:49, Henning Schild wrote:
> >>>
> >>> Hi,
> >>>
> >>>> This driver adds initial support for several devices from
> >>>> Siemens. It is based on a platform driver introduced in an
> >>>> earlier commit.
> >>>
> >>> Where does the wdt actually come from ?
> >>>
> >>> Is it in the SoC ? (which SoC exactly). SoC-builtin wdt is a
> >>> pretty usual case.
> >>>
> >>> Or some external chip ?
> >>
> >> I guess external chip, but again we are talking about multiple
> >> machines. And the manuals i read so far do not go into that sort of
> >> detail. In fact on some of the machines you will have two
> >> watchdogs, one from the SoC and that "special" one.
> >> That has several reasons, probably not too important here. The HW
> >> guys are adding another wd not just for fun, and it would be nice
> >> to have a driver.
> >>
> >>> The code smells a bit like two entirely different wdt's that just
> >>> have some similarities. If that's the case, I'd rather split it
> >>> into two separate drivers and let the parent driver (board file)
> >>> instantiate the correct one.
> >>
> >> Yes, it is two. Just like for the LEDs. One version PIO-based
> >> another version gpio/p2sb/mmio based.
> >
> > I tend to agree with Enrico that this should be two separate
> > drivers.
>
> Agreed.
>
> Guenter
I will ignore the wish for a split in v4. Reason is that it would cause
a lot of duplication are spreading code over many files. like i.e. a
-common.c
Internally we have that driver in fact already support a few more
machines, which could call a split at some point. Or could also upset
people of the many CONFIG_ knobs and files as we keep pushing machine
support forward in the upstream drivers.
But i would like to discuss that in patch qs coming after a merge and
not split (maybe not yet).
Also splitting wdt and having leds in one file would be inconsistent.
So when there will be a split it should be on both ends. But please
allow me to postpone that.
regards,
Henning
> >> In fact the latter should very likely be based on that gpio pinctl,
> >> whether it really needs to be a separate driver will have to be
> >> seen. There are probably pros and cons for both options.
> >
> >
>
Am Tue, 30 Mar 2021 14:04:35 +0300
schrieb Andy Shevchenko <[email protected]>:
> On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> <[email protected]> wrote:
> >
> > This driver adds initial support for several devices from Siemens.
> > It is based on a platform driver introduced in an earlier commit.
>
> ...
>
> > +#define SIMATIC_IPC_LED_PORT_BASE 0x404E
>
> > +static struct simatic_ipc_led simatic_ipc_leds_io[] = {
> > + {1 << 15, "green:" LED_FUNCTION_STATUS "-1" },
> > + {1 << 7, "yellow:" LED_FUNCTION_STATUS "-1" },
> > + {1 << 14, "red:" LED_FUNCTION_STATUS "-2" },
> > + {1 << 6, "yellow:" LED_FUNCTION_STATUS "-2" },
> > + {1 << 13, "red:" LED_FUNCTION_STATUS "-3" },
> > + {1 << 5, "yellow:" LED_FUNCTION_STATUS "-3" },
> > + { }
> > +};
>
> > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > + { }
> > +};
>
> It seems to me like poking GPIO controller registers directly. This
> is not good. The question still remains: Can we simply register a
> GPIO (pin control) driver and use an LED GPIO driver with an
> additional board file that instantiates it?
The short answer for v4 will be "No we can not!". The pinctrl drivers
do not currently probe on any of the devices and attempts to fix that
have failed or gut stuck. I tried to help out where i could and waited
for a long time.
Now my take is to turn the order around. We go in like that and will
happily switch to pinctrl if that ever comes up on the machines.
Meaning P2SB series on top of this, no more delays please.
We do use request_region so have a mutex in place. Meaning we really
only touch GPIO while pinctrl does not!
I see no issue here, waited for a long time and now expect to be
allowed to get merged first.
Henning
On Fri, Nov 26, 2021 at 3:28 PM Henning Schild
<[email protected]> wrote:
> Am Tue, 30 Mar 2021 14:04:35 +0300
> schrieb Andy Shevchenko <[email protected]>:
> > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > <[email protected]> wrote:
...
> > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > + { }
> > > +};
> >
> > It seems to me like poking GPIO controller registers directly. This
> > is not good. The question still remains: Can we simply register a
> > GPIO (pin control) driver and use an LED GPIO driver with an
> > additional board file that instantiates it?
>
> The short answer for v4 will be "No we can not!". The pinctrl drivers
> do not currently probe on any of the devices and attempts to fix that
> have failed or gut stuck. I tried to help out where i could and waited
> for a long time.
I see, unfortunately I have stuck with some other (more important
tasks) and can't fulfil this, but I still consider it's no go for
driver poking pin control registers directly. Lemme see if I can
prioritize this for next week.
> Now my take is to turn the order around. We go in like that and will
> happily switch to pinctrl if that ever comes up on the machines.
> Meaning P2SB series on top of this, no more delays please.
I don't want to slip bad code into the kernel where we can avoid that.
> We do use request_region so have a mutex in place. Meaning we really
> only touch GPIO while pinctrl does not!
I haven't got this. On Intel SoCs GPIO is a part of pin control
registers. You can't touch GPIO without touching pin control.
> I see no issue here, waited for a long time and now expect to be
> allowed to get merged first.
Okay, I have these questions / asks so far:
1) Can firmware be fixed in order to provide an ACPI table for the pin
control devices?
2) Can you share firmware (BIOS ROM file I suppose) that I may flash
on an Apollo Lake machine and see if I can reproduce the issue?
3) As may be a last resort, can you share (remotely) or even send to
us the device in question to try?
--
With Best Regards,
Andy Shevchenko
Am Fri, 26 Nov 2021 16:02:48 +0200
schrieb Andy Shevchenko <[email protected]>:
> On Fri, Nov 26, 2021 at 3:28 PM Henning Schild
> <[email protected]> wrote:
> > Am Tue, 30 Mar 2021 14:04:35 +0300
> > schrieb Andy Shevchenko <[email protected]>:
> > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > <[email protected]> wrote:
>
> ...
>
> > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > > + { }
> > > > +};
> > >
> > > It seems to me like poking GPIO controller registers directly.
> > > This is not good. The question still remains: Can we simply
> > > register a GPIO (pin control) driver and use an LED GPIO driver
> > > with an additional board file that instantiates it?
> >
> > The short answer for v4 will be "No we can not!". The pinctrl
> > drivers do not currently probe on any of the devices and attempts
> > to fix that have failed or gut stuck. I tried to help out where i
> > could and waited for a long time.
>
> I see, unfortunately I have stuck with some other (more important
> tasks) and can't fulfil this, but I still consider it's no go for
> driver poking pin control registers directly. Lemme see if I can
> prioritize this for next week.
I just sent v4. And am sick of waiting on you. Sorry to be that clear
here. I want that order changed! If you still end up being fast,
perfect. But i want to be faster!
> > Now my take is to turn the order around. We go in like that and will
> > happily switch to pinctrl if that ever comes up on the machines.
> > Meaning P2SB series on top of this, no more delays please.
>
> I don't want to slip bad code into the kernel where we can avoid that.
It is not bad code! That is unfair to say. It can be improved on and
that is what we have a FIXME line for. The worst code is code that is
not there ... devices without drivers!
That is bad, not i minor poke of parts of a resource no other driver
claimed!
> > We do use request_region so have a mutex in place. Meaning we really
> > only touch GPIO while pinctrl does not!
>
> I haven't got this. On Intel SoCs GPIO is a part of pin control
> registers. You can't touch GPIO without touching pin control.
i meant pin control, if it ever did probe it would reserve the region
and push our hack out, or the other way around ... no conflict!
To get both we just need a simple patch and switch to pinctrl, just
notify me once your stuff is ready and i will write that patch.
> > I see no issue here, waited for a long time and now expect to be
> > allowed to get merged first.
>
> Okay, I have these questions / asks so far:
> 1) Can firmware be fixed in order to provide an ACPI table for the pin
> control devices?
No. The firmware will only receive security but no feature updates ...
> 2) Can you share firmware (BIOS ROM file I suppose) that I may flash
> on an Apollo Lake machine and see if I can reproduce the issue?
I do not have access. But all you need is a firware with no ACPI entry
and P2SB hidden. Or simply patch out the two probe paths ;).
> 3) As may be a last resort, can you share (remotely) or even send to
> us the device in question to try?
We are talking about multiple devices. Not just that one apollo lake on
which your patches kind of worked.
But showed some weirdness which could really become a problem if
someone decided to add an ACPI entry ..
It pin 42 name could be
GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 42
or
GPIO_LOOKUP_IDX("INT3452:01", 42
I guess that conflict will have to be dealt with before your can switch
to probing pinctrl drivers based on cpu model and not only ACPI/P2SB any
longer.
regards,
Henning
On Fri, Nov 26, 2021 at 4:44 PM Henning Schild
<[email protected]> wrote:
> Am Fri, 26 Nov 2021 16:02:48 +0200
> schrieb Andy Shevchenko <[email protected]>:
> > On Fri, Nov 26, 2021 at 3:28 PM Henning Schild
> > <[email protected]> wrote:
> > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > schrieb Andy Shevchenko <[email protected]>:
> > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > <[email protected]> wrote:
...
> > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > > > + { }
> > > > > +};
> > > >
> > > > It seems to me like poking GPIO controller registers directly.
> > > > This is not good. The question still remains: Can we simply
> > > > register a GPIO (pin control) driver and use an LED GPIO driver
> > > > with an additional board file that instantiates it?
> > >
> > > The short answer for v4 will be "No we can not!". The pinctrl
> > > drivers do not currently probe on any of the devices and attempts
> > > to fix that have failed or gut stuck. I tried to help out where i
> > > could and waited for a long time.
> >
> > I see, unfortunately I have stuck with some other (more important
> > tasks) and can't fulfil this, but I still consider it's no go for
> > driver poking pin control registers directly. Lemme see if I can
> > prioritize this for next week.
>
> I just sent v4. And am sick of waiting on you. Sorry to be that clear
> here. I want that order changed! If you still end up being fast,
> perfect. But i want to be faster!
It's good that you are honest, honesty is what we missed a lot!
> > > Now my take is to turn the order around. We go in like that and will
> > > happily switch to pinctrl if that ever comes up on the machines.
> > > Meaning P2SB series on top of this, no more delays please.
> >
> > I don't want to slip bad code into the kernel where we can avoid that.
>
> It is not bad code! That is unfair to say. It can be improved on and
> that is what we have a FIXME line for. The worst code is code that is
> not there ... devices without drivers!
Okay, that's how you interpret the term "bad". Probably I had to use
something else to explain that it's racy with the very same case if
one adds an ACPI support to it.
> That is bad, not i minor poke of parts of a resource no other driver
> claimed!
>
> > > We do use request_region so have a mutex in place. Meaning we really
> > > only touch GPIO while pinctrl does not!
> >
> > I haven't got this. On Intel SoCs GPIO is a part of pin control
> > registers. You can't touch GPIO without touching pin control.
>
> i meant pin control, if it ever did probe it would reserve the region
> and push our hack out, or the other way around ... no conflict!
> To get both we just need a simple patch and switch to pinctrl, just
> notify me once your stuff is ready and i will write that patch.
While thinking more on it, the quickest solution here is to do a P2SB
game based on DMI strings in the board code for the platform
(somewhere under PDx86).
> > > I see no issue here, waited for a long time and now expect to be
> > > allowed to get merged first.
> >
> > Okay, I have these questions / asks so far:
> > 1) Can firmware be fixed in order to provide an ACPI table for the pin
> > control devices?
>
> No. The firmware will only receive security but no feature updates ...
>
> > 2) Can you share firmware (BIOS ROM file I suppose) that I may flash
> > on an Apollo Lake machine and see if I can reproduce the issue?
>
> I do not have access. But all you need is a firware with no ACPI entry
> and P2SB hidden. Or simply patch out the two probe paths ;).
Yes, probably that will work.
> > 3) As may be a last resort, can you share (remotely) or even send to
> > us the device in question to try?
>
> We are talking about multiple devices. Not just that one apollo lake on
> which your patches kind of worked.
>
> But showed some weirdness which could really become a problem if
> someone decided to add an ACPI entry ..
Then it should have different DMI strings or so, it won't be the
_same_ platform anymore.
> It pin 42 name could be
> GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 42
> or
> GPIO_LOOKUP_IDX("INT3452:01", 42
> I guess that conflict will have to be dealt with before your can switch
> to probing pinctrl drivers based on cpu model and not only ACPI/P2SB any
> longer.
Not gonna happen.
--
With Best Regards,
Andy Shevchenko
Am Fri, 26 Nov 2021 16:59:54 +0200
schrieb Andy Shevchenko <[email protected]>:
> On Fri, Nov 26, 2021 at 4:44 PM Henning Schild
> <[email protected]> wrote:
> > Am Fri, 26 Nov 2021 16:02:48 +0200
> > schrieb Andy Shevchenko <[email protected]>:
> > > On Fri, Nov 26, 2021 at 3:28 PM Henning Schild
> > > <[email protected]> wrote:
> > > > Am Tue, 30 Mar 2021 14:04:35 +0300
> > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > On Mon, Mar 29, 2021 at 8:59 PM Henning Schild
> > > > > <[email protected]> wrote:
>
> ...
>
> > > > > > +static struct simatic_ipc_led simatic_ipc_leds_mem[] = {
> > > > > > + {0x500 + 0x1A0, "red:" LED_FUNCTION_STATUS "-1"},
> > > > > > + {0x500 + 0x1A8, "green:" LED_FUNCTION_STATUS "-1"},
> > > > > > + {0x500 + 0x1C8, "red:" LED_FUNCTION_STATUS "-2"},
> > > > > > + {0x500 + 0x1D0, "green:" LED_FUNCTION_STATUS "-2"},
> > > > > > + {0x500 + 0x1E0, "red:" LED_FUNCTION_STATUS "-3"},
> > > > > > + {0x500 + 0x198, "green:" LED_FUNCTION_STATUS "-3"},
> > > > > > + { }
> > > > > > +};
> > > > >
> > > > > It seems to me like poking GPIO controller registers directly.
> > > > > This is not good. The question still remains: Can we simply
> > > > > register a GPIO (pin control) driver and use an LED GPIO
> > > > > driver with an additional board file that instantiates it?
> > > >
> > > > The short answer for v4 will be "No we can not!". The pinctrl
> > > > drivers do not currently probe on any of the devices and
> > > > attempts to fix that have failed or gut stuck. I tried to help
> > > > out where i could and waited for a long time.
> > >
> > > I see, unfortunately I have stuck with some other (more important
> > > tasks) and can't fulfil this, but I still consider it's no go for
> > > driver poking pin control registers directly. Lemme see if I can
> > > prioritize this for next week.
> >
> > I just sent v4. And am sick of waiting on you. Sorry to be that
> > clear here. I want that order changed! If you still end up being
> > fast, perfect. But i want to be faster!
>
> It's good that you are honest, honesty is what we missed a lot!
I was always honest, hope that was not missed from my side ... let
alone a lot.
> > > > Now my take is to turn the order around. We go in like that and
> > > > will happily switch to pinctrl if that ever comes up on the
> > > > machines. Meaning P2SB series on top of this, no more delays
> > > > please.
> > >
> > > I don't want to slip bad code into the kernel where we can avoid
> > > that.
> >
> > It is not bad code! That is unfair to say. It can be improved on and
> > that is what we have a FIXME line for. The worst code is code that
> > is not there ... devices without drivers!
>
> Okay, that's how you interpret the term "bad". Probably I had to use
> something else to explain that it's racy with the very same case if
> one adds an ACPI support to it.
It is only racy when the firmware would change (which i am
unfortunately pretty sure it will not), or if pinctrl would probe
without P2SB or ACPI. (where you say "Not gonna happen.")
Or i could say "fortunately pretty sure" because that means pinctrl
will never probe, hence no race!
> > That is bad, not i minor poke of parts of a resource no other driver
> > claimed!
> >
> > > > We do use request_region so have a mutex in place. Meaning we
> > > > really only touch GPIO while pinctrl does not!
> > >
> > > I haven't got this. On Intel SoCs GPIO is a part of pin control
> > > registers. You can't touch GPIO without touching pin control.
> >
> > i meant pin control, if it ever did probe it would reserve the
> > region and push our hack out, or the other way around ... no
> > conflict! To get both we just need a simple patch and switch to
> > pinctrl, just notify me once your stuff is ready and i will write
> > that patch.
>
> While thinking more on it, the quickest solution here is to do a P2SB
> game based on DMI strings in the board code for the platform
> (somewhere under PDx86).
Not sure what you suggest here. p1 does pretty fancy DMI to be really
sure to only match specific devices, and only then we do our own P2SB
base address discover and region reservation.
> > > > I see no issue here, waited for a long time and now expect to be
> > > > allowed to get merged first.
> > >
> > > Okay, I have these questions / asks so far:
> > > 1) Can firmware be fixed in order to provide an ACPI table for
> > > the pin control devices?
> >
> > No. The firmware will only receive security but no feature updates
> > ...
> > > 2) Can you share firmware (BIOS ROM file I suppose) that I may
> > > flash on an Apollo Lake machine and see if I can reproduce the
> > > issue?
> >
> > I do not have access. But all you need is a firware with no ACPI
> > entry and P2SB hidden. Or simply patch out the two probe paths ;).
>
> Yes, probably that will work.
I wonder how you would probe without the two with your "Not gonna
happen.". But maybe your patches will open my eyes and i have been
blind all the time.
> > > 3) As may be a last resort, can you share (remotely) or even send
> > > to us the device in question to try?
> >
> > We are talking about multiple devices. Not just that one apollo
> > lake on which your patches kind of worked.
> >
> > But showed some weirdness which could really become a problem if
> > someone decided to add an ACPI entry ..
>
> Then it should have different DMI strings or so, it won't be the
> _same_ platform anymore.
There is different DMI in place. p1 introduces
"enum simatic_ipc_station_ids" with currently 7 different devices
matched with not a string but a "binary" behind
SIMATIC_IPC_DMI_ENTRY_OEM. The struct can be found in
simatic_ipc_get_station_id
Our strings could be custom, but that binary allows for real DMI
identifaction of those currently proposed 7 "platforms".
See p4 where i revert string-based DMI matching with
SIMATIC_IPC_DMI_ENTRY_OEM-based. Make sure to look at my answer to a
question in v4 p4 on that DMI topic.
regards,
Henning
> > It pin 42 name could be
> > GPIO_LOOKUP_IDX("apollolake-pinctrl.0", 42
> > or
> > GPIO_LOOKUP_IDX("INT3452:01", 42
>
> > I guess that conflict will have to be dealt with before your can
> > switch to probing pinctrl drivers based on cpu model and not only
> > ACPI/P2SB any longer.
>
> Not gonna happen.
>