From: Henning Schild <[email protected]>
This mainly implements detection of these devices and will allow
secondary drivers to work on such machines.
The identification is DMI-based with a vendor specific way to tell them
apart in a reliable way.
Drivers for LEDs and Watchdogs will follow to make use of that platform
detection.
Signed-off-by: Gerd Haeussler <[email protected]>
Signed-off-by: Henning Schild <[email protected]>
---
drivers/platform/x86/Kconfig | 9 +
drivers/platform/x86/Makefile | 3 +
drivers/platform/x86/simatic-ipc.c | 166 ++++++++++++++++++
.../platform_data/x86/simatic-ipc-base.h | 33 ++++
include/linux/platform_data/x86/simatic-ipc.h | 68 +++++++
5 files changed, 279 insertions(+)
create mode 100644 drivers/platform/x86/simatic-ipc.c
create mode 100644 include/linux/platform_data/x86/simatic-ipc-base.h
create mode 100644 include/linux/platform_data/x86/simatic-ipc.h
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig
index ad4e630e73e2..bb9e282d89cf 100644
--- a/drivers/platform/x86/Kconfig
+++ b/drivers/platform/x86/Kconfig
@@ -1284,6 +1284,15 @@ config INTEL_TELEMETRY
directly via debugfs files. Various tools may use
this interface for SoC state monitoring.
+config SIEMENS_SIMATIC_IPC
+ tristate "Siemens Simatic IPC Class driver"
+ depends on PCI
+ help
+ This Simatic IPC class driver is the central of several drivers. It
+ is mainly used for system identification, after which drivers in other
+ classes will take care of driving specifics of those machines.
+ i.e. leds and watchdogs
+
endif # X86_PLATFORM_DEVICES
config PMC_ATOM
diff --git a/drivers/platform/x86/Makefile b/drivers/platform/x86/Makefile
index 60d554073749..26cdebf2e701 100644
--- a/drivers/platform/x86/Makefile
+++ b/drivers/platform/x86/Makefile
@@ -138,3 +138,6 @@ obj-$(CONFIG_INTEL_TELEMETRY) += intel_telemetry_core.o \
intel_telemetry_pltdrv.o \
intel_telemetry_debugfs.o
obj-$(CONFIG_PMC_ATOM) += pmc_atom.o
+
+# Siemens Simatic Industrial PCs
+obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o
diff --git a/drivers/platform/x86/simatic-ipc.c b/drivers/platform/x86/simatic-ipc.c
new file mode 100644
index 000000000000..6adcdac1a0d7
--- /dev/null
+++ b/drivers/platform/x86/simatic-ipc.c
@@ -0,0 +1,166 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Siemens SIMATIC IPC driver for LEDs
+ *
+ * Copyright (c) Siemens AG, 2018-2021
+ *
+ * Authors:
+ * Henning Schild <[email protected]>
+ * Jan Kiszka <[email protected]>
+ * Gerd Haeussler <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/x86/simatic-ipc.h>
+
+static struct platform_device *ipc_led_platform_device;
+static struct platform_device *ipc_wdt_platform_device;
+
+static const struct dmi_system_id simatic_ipc_whitelist[] = {
+ {
+ .matches = {
+ DMI_MATCH(DMI_SYS_VENDOR, "SIEMENS AG"),
+ },
+ },
+ {}
+};
+
+static struct simatic_ipc_platform platform_data;
+
+static struct {
+ u32 station_id;
+ u8 led_mode;
+ u8 wdt_mode;
+} device_modes[] = {
+ { SIMATIC_IPC_IPC127E, SIMATIC_IPC_DEVICE_127E, SIMATIC_IPC_DEVICE_NONE},
+ { SIMATIC_IPC_IPC227D, SIMATIC_IPC_DEVICE_227D, SIMATIC_IPC_DEVICE_NONE},
+ { SIMATIC_IPC_IPC227E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_227E},
+ { SIMATIC_IPC_IPC277E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_227E},
+ { SIMATIC_IPC_IPC427D, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_NONE},
+ { SIMATIC_IPC_IPC427E, SIMATIC_IPC_DEVICE_427E, SIMATIC_IPC_DEVICE_427E},
+ { SIMATIC_IPC_IPC477E, SIMATIC_IPC_DEVICE_NONE, SIMATIC_IPC_DEVICE_427E},
+};
+
+static int register_platform_devices(u32 station_id)
+{
+ int i;
+ u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
+ u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
+
+ platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
+
+ for (i = 0; i < ARRAY_SIZE(device_modes); i++) {
+ if (device_modes[i].station_id == station_id) {
+ ledmode = device_modes[i].led_mode;
+ wdtmode = device_modes[i].wdt_mode;
+ break;
+ }
+ }
+
+ if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
+ platform_data.devmode = ledmode;
+ ipc_led_platform_device = platform_device_register_data
+ (NULL, KBUILD_MODNAME "_leds", PLATFORM_DEVID_NONE,
+ &platform_data, sizeof(struct simatic_ipc_platform));
+ if (IS_ERR(ipc_led_platform_device))
+ return PTR_ERR(ipc_led_platform_device);
+
+ pr_debug(KBUILD_MODNAME ": device=%s created\n",
+ ipc_led_platform_device->name);
+ }
+
+ if (wdtmode != SIMATIC_IPC_DEVICE_NONE) {
+ platform_data.devmode = wdtmode;
+ ipc_wdt_platform_device = platform_device_register_data
+ (NULL, KBUILD_MODNAME "_wdt", PLATFORM_DEVID_NONE,
+ &platform_data, sizeof(struct simatic_ipc_platform));
+ if (IS_ERR(ipc_wdt_platform_device))
+ return PTR_ERR(ipc_wdt_platform_device);
+
+ pr_debug(KBUILD_MODNAME ": device=%s created\n",
+ ipc_wdt_platform_device->name);
+ }
+
+ if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
+ wdtmode == SIMATIC_IPC_DEVICE_NONE) {
+ pr_warn(KBUILD_MODNAME
+ ": unsupported IPC detected, station id=%08x\n",
+ station_id);
+ return -EINVAL;
+ } else {
+ return 0;
+ }
+}
+
+/*
+ * Get membase address from PCI, used in leds and wdt modul. Here we read
+ * the bar0. The final address calculation is done in the appropriate modules
+ */
+
+u32 simatic_ipc_get_membase0(unsigned int p2sb)
+{
+ u32 bar0 = 0;
+#ifdef CONFIG_PCI
+ struct pci_bus *bus;
+ /*
+ * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device
+ * to have a quick look at it, before we hide it again.
+ * Also grab the pci rescan lock so that device does not get discovered
+ * and remapped while it is visible.
+ * This code is inspired by drivers/mfd/lpc_ich.c
+ */
+ bus = pci_find_bus(0, 0);
+ pci_lock_rescan_remove();
+ pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
+ pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
+
+ bar0 &= ~0xf;
+ pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
+ pci_unlock_rescan_remove();
+#endif /* CONFIG_PCI */
+ return bar0;
+}
+EXPORT_SYMBOL(simatic_ipc_get_membase0);
+
+static int __init simatic_ipc_init_module(void)
+{
+ const struct dmi_system_id *match;
+ u32 station_id;
+ int err;
+
+ match = dmi_first_match(simatic_ipc_whitelist);
+ if (!match)
+ return 0;
+
+ err = dmi_walk(simatic_ipc_find_dmi_entry_helper, &station_id);
+
+ if (err || station_id == SIMATIC_IPC_INVALID_STATION_ID) {
+ pr_warn(KBUILD_MODNAME ": DMI entry %d not found\n",
+ DMI_ENTRY_OEM);
+ return 0;
+ }
+
+ return register_platform_devices(station_id);
+}
+
+static void __exit simatic_ipc_exit_module(void)
+{
+ platform_device_unregister(ipc_led_platform_device);
+ ipc_led_platform_device = NULL;
+
+ platform_device_unregister(ipc_wdt_platform_device);
+ ipc_wdt_platform_device = NULL;
+}
+
+module_init(simatic_ipc_init_module);
+module_exit(simatic_ipc_exit_module);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Gerd Haeussler <[email protected]>");
+MODULE_ALIAS("dmi:*:svnSIEMENSAG:*");
diff --git a/include/linux/platform_data/x86/simatic-ipc-base.h b/include/linux/platform_data/x86/simatic-ipc-base.h
new file mode 100644
index 000000000000..d8e59eb8fc96
--- /dev/null
+++ b/include/linux/platform_data/x86/simatic-ipc-base.h
@@ -0,0 +1,33 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Siemens SIMATIC IPC drivers
+ *
+ * Copyright (c) Siemens AG, 2018-2021
+ *
+ * Authors:
+ * Henning Schild <[email protected]>
+ * Gerd Haeussler <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H
+#define __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H
+
+#include <linux/pci.h>
+
+#define SIMATIC_IPC_DEVICE_NONE 0
+#define SIMATIC_IPC_DEVICE_227D 1
+#define SIMATIC_IPC_DEVICE_427E 2
+#define SIMATIC_IPC_DEVICE_127E 3
+#define SIMATIC_IPC_DEVICE_227E 4
+
+struct simatic_ipc_platform {
+ u8 devmode;
+};
+
+u32 simatic_ipc_get_membase0(unsigned int p2sb);
+
+#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_BASE_H */
diff --git a/include/linux/platform_data/x86/simatic-ipc.h b/include/linux/platform_data/x86/simatic-ipc.h
new file mode 100644
index 000000000000..90bb0d7cf09a
--- /dev/null
+++ b/include/linux/platform_data/x86/simatic-ipc.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Siemens SIMATIC IPC drivers
+ *
+ * Copyright (c) Siemens AG, 2018-2021
+ *
+ * Authors:
+ * Henning Schild <[email protected]>
+ * Gerd Haeussler <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef __PLATFORM_DATA_X86_SIMATIC_IPC_H
+#define __PLATFORM_DATA_X86_SIMATIC_IPC_H
+
+#include <linux/dmi.h>
+#include <linux/platform_data/x86/simatic-ipc-base.h>
+
+#define DMI_ENTRY_OEM 129
+
+enum simatic_ipc_station_ids {
+ SIMATIC_IPC_INVALID_STATION_ID = 0,
+ SIMATIC_IPC_IPC227D = 0x00000501,
+ SIMATIC_IPC_IPC427D = 0x00000701,
+ SIMATIC_IPC_IPC227E = 0x00000901,
+ SIMATIC_IPC_IPC277E = 0x00000902,
+ SIMATIC_IPC_IPC427E = 0x00000A01,
+ SIMATIC_IPC_IPC477E = 0x00000A02,
+ SIMATIC_IPC_IPC127E = 0x00000D01,
+};
+
+static inline u32 simatic_ipc_get_station_id(u8 *data)
+{
+ u32 station_id = SIMATIC_IPC_INVALID_STATION_ID;
+ int i;
+ struct {
+ u8 type; /* type (0xff = binary) */
+ u8 len; /* len of data entry */
+ u8 reserved[3];
+ u32 station_id; /* station id (LE) */
+ } __packed * data_entry = (void *)data;
+
+ /* find 4th entry in OEM data */
+ for (i = 0; i < 3; i++)
+ data_entry = (void *)((u8 *)(data_entry) + data_entry->len);
+
+ /* decode station id */
+ if (data_entry && data_entry->type == 0xff && data_entry->len == 9)
+ station_id = le32_to_cpu(data_entry->station_id);
+
+ return station_id;
+}
+
+static inline void
+simatic_ipc_find_dmi_entry_helper(const struct dmi_header *dh, void *_data)
+{
+ u32 *id = _data;
+
+ if (dh->type != DMI_ENTRY_OEM)
+ return;
+
+ *id = simatic_ipc_get_station_id((u8 *)dh + sizeof(struct dmi_header));
+}
+
+#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */
--
2.26.2
On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
<[email protected]> wrote:
>
> From: Henning Schild <[email protected]>
>
> This mainly implements detection of these devices and will allow
> secondary drivers to work on such machines.
>
> The identification is DMI-based with a vendor specific way to tell them
> apart in a reliable way.
>
> Drivers for LEDs and Watchdogs will follow to make use of that platform
> detection.
> Signed-off-by: Gerd Haeussler <[email protected]>
> Signed-off-by: Henning Schild <[email protected]>
The order is wrong taking into account the From line in the body. So,
it's not clear who is the author, who is a co-developer, and who is
the committer (one person may utilize few roles).
Check for the rest of the series as well (basically this is the rule
of thumb to recheck entire code for the comment you have got at any
single place of it).
...
> +config SIEMENS_SIMATIC_IPC
> + tristate "Siemens Simatic IPC Class driver"
> + depends on PCI
> + help
> + This Simatic IPC class driver is the central of several drivers. It
> + is mainly used for system identification, after which drivers in other
> + classes will take care of driving specifics of those machines.
> + i.e. leds and watchdogs
LEDs
watchdog. (missed period and singular form)
Module name?
> endif # X86_PLATFORM_DEVICES
Not sure about the ordering of the section in Kconfig, but it is up to
maintainers.
...
> +# Siemens Simatic Industrial PCs
> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o
Ditto.
...
> + * Siemens SIMATIC IPC driver for LEDs
It seems to be used in patch 4 which is not LED related. Also, why is
it here if it's for LEDs?
...
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
Replace with SPDX
...
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
Ordered?
> +#include <linux/platform_data/x86/simatic-ipc.h>
...
> +static int register_platform_devices(u32 station_id)
> +{
> + int i;
> + u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
Reversed xmas tree order?
> + platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
> +
> + for (i = 0; i < ARRAY_SIZE(device_modes); i++) {
> + if (device_modes[i].station_id == station_id) {
> + ledmode = device_modes[i].led_mode;
> + wdtmode = device_modes[i].wdt_mode;
> + break;
> + }
> + }
> +
> + if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
> + platform_data.devmode = ledmode;
> + ipc_led_platform_device = platform_device_register_data
> + (NULL, KBUILD_MODNAME "_leds", PLATFORM_DEVID_NONE,
> + &platform_data, sizeof(struct simatic_ipc_platform));
Strange indentation (second line).
> + if (IS_ERR(ipc_led_platform_device))
> + return PTR_ERR(ipc_led_platform_device);
> + pr_debug(KBUILD_MODNAME ": device=%s created\n",
> + ipc_led_platform_device->name);
Utilize pr_fmt() instead of adding prefixes like this.
> + }
> + if (wdtmode != SIMATIC_IPC_DEVICE_NONE) {
> + platform_data.devmode = wdtmode;
> + ipc_wdt_platform_device = platform_device_register_data
> + (NULL, KBUILD_MODNAME "_wdt", PLATFORM_DEVID_NONE,
> + &platform_data, sizeof(struct simatic_ipc_platform));
> + if (IS_ERR(ipc_wdt_platform_device))
> + return PTR_ERR(ipc_wdt_platform_device);
> +
> + pr_debug(KBUILD_MODNAME ": device=%s created\n",
> + ipc_wdt_platform_device->name);
> + }
Same comments as above.
> + if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
> + wdtmode == SIMATIC_IPC_DEVICE_NONE) {
> + pr_warn(KBUILD_MODNAME
> + ": unsupported IPC detected, station id=%08x\n",
> + station_id);
Ugly indentation. With pr_fmt() in use will be much better.
> + return -EINVAL;
> + } else {
Redundant.
> + return 0;
> + }
> +}
...
> +/*
> + * Get membase address from PCI, used in leds and wdt modul. Here we read
> + * the bar0. The final address calculation is done in the appropriate modules
bar -> BAR
Missed period.
> + */
> +
Unneeded blank line.
> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> +{
> + u32 bar0 = 0;
> +#ifdef CONFIG_PCI
It's ugly besides the fact that you have a dependency.
> + struct pci_bus *bus;
Missed blank line.
> + /*
> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device
> + * to have a quick look at it, before we hide it again.
> + * Also grab the pci rescan lock so that device does not get discovered
> + * and remapped while it is visible.
> + * This code is inspired by drivers/mfd/lpc_ich.c
> + */
> + bus = pci_find_bus(0, 0);
> + pci_lock_rescan_remove();
> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
> + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
> +
> + bar0 &= ~0xf;
> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
> + pci_unlock_rescan_remove();
> +#endif /* CONFIG_PCI */
> + return bar0;
> +}
> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
Oy vey! I know what this is and let's do it differently. I have some
(relatively old) patch series I can send you privately for testing.
...
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
Not needed when you have SPDX.
...
> +#include <linux/pci.h>
Wrong header. Should be types.h
...
> +#include <linux/dmi.h>
> +#include <linux/platform_data/x86/simatic-ipc-base.h>
Missed headers. You need to include ones that the code below is a
direct user of.
Like types.h
--
With Best Regards,
Andy Shevchenko
Hi,
On 3/4/21 11:11 AM, Andy Shevchenko wrote:
> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> <[email protected]> wrote:
>>
>> From: Henning Schild <[email protected]>
>>
>> This mainly implements detection of these devices and will allow
>> secondary drivers to work on such machines.
>>
>> The identification is DMI-based with a vendor specific way to tell them
>> apart in a reliable way.
>>
>> Drivers for LEDs and Watchdogs will follow to make use of that platform
>> detection.
>
>> Signed-off-by: Gerd Haeussler <[email protected]>
>> Signed-off-by: Henning Schild <[email protected]>
>
> The order is wrong taking into account the From line in the body. So,
> it's not clear who is the author, who is a co-developer, and who is
> the committer (one person may utilize few roles).
> Check for the rest of the series as well (basically this is the rule
> of thumb to recheck entire code for the comment you have got at any
> single place of it).
>
> ...
>
>> +config SIEMENS_SIMATIC_IPC
>> + tristate "Siemens Simatic IPC Class driver"
>> + depends on PCI
>> + help
>> + This Simatic IPC class driver is the central of several drivers. It
>> + is mainly used for system identification, after which drivers in other
>> + classes will take care of driving specifics of those machines.
>> + i.e. leds and watchdogs
>
> LEDs
> watchdog. (missed period and singular form)
>
> Module name?
>
>> endif # X86_PLATFORM_DEVICES
>
> Not sure about the ordering of the section in Kconfig, but it is up to
> maintainers.
>
> ...
>
>> +# Siemens Simatic Industrial PCs
>> +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o
>
> Ditto.
>
> ...
>
>> + * Siemens SIMATIC IPC driver for LEDs
>
> It seems to be used in patch 4 which is not LED related. Also, why is
> it here if it's for LEDs?
>
> ...
>
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>
> Replace with SPDX
>
> ...
>
>> +#include <linux/module.h>
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>
> Ordered?
>
>> +#include <linux/platform_data/x86/simatic-ipc.h>
>
> ...
>
>> +static int register_platform_devices(u32 station_id)
>> +{
>> + int i;
>> + u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
>> + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
>
> Reversed xmas tree order?
>
>> + platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
>> +
>> + for (i = 0; i < ARRAY_SIZE(device_modes); i++) {
>> + if (device_modes[i].station_id == station_id) {
>> + ledmode = device_modes[i].led_mode;
>> + wdtmode = device_modes[i].wdt_mode;
>> + break;
>> + }
>> + }
>> +
>> + if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
>> + platform_data.devmode = ledmode;
>
>> + ipc_led_platform_device = platform_device_register_data
>> + (NULL, KBUILD_MODNAME "_leds", PLATFORM_DEVID_NONE,
>> + &platform_data, sizeof(struct simatic_ipc_platform));
>
> Strange indentation (second line).
>
>> + if (IS_ERR(ipc_led_platform_device))
>> + return PTR_ERR(ipc_led_platform_device);
>
>> + pr_debug(KBUILD_MODNAME ": device=%s created\n",
>> + ipc_led_platform_device->name);
>
> Utilize pr_fmt() instead of adding prefixes like this.
>
>> + }
>
>> + if (wdtmode != SIMATIC_IPC_DEVICE_NONE) {
>> + platform_data.devmode = wdtmode;
>> + ipc_wdt_platform_device = platform_device_register_data
>> + (NULL, KBUILD_MODNAME "_wdt", PLATFORM_DEVID_NONE,
>> + &platform_data, sizeof(struct simatic_ipc_platform));
>> + if (IS_ERR(ipc_wdt_platform_device))
>> + return PTR_ERR(ipc_wdt_platform_device);
>> +
>> + pr_debug(KBUILD_MODNAME ": device=%s created\n",
>> + ipc_wdt_platform_device->name);
>> + }
>
> Same comments as above.
>
>> + if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
>> + wdtmode == SIMATIC_IPC_DEVICE_NONE) {
>> + pr_warn(KBUILD_MODNAME
>> + ": unsupported IPC detected, station id=%08x\n",
>> + station_id);
>
> Ugly indentation. With pr_fmt() in use will be much better.
>
>> + return -EINVAL;
>
>> + } else {
>
> Redundant.
>
>> + return 0;
>> + }
>> +}
>
> ...
>
>> +/*
>> + * Get membase address from PCI, used in leds and wdt modul. Here we read
>> + * the bar0. The final address calculation is done in the appropriate modules
>
> bar -> BAR
> Missed period.
>
>> + */
>
>> +
>
> Unneeded blank line.
>
>> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
>> +{
>> + u32 bar0 = 0;
>
>> +#ifdef CONFIG_PCI
>
> It's ugly besides the fact that you have a dependency.
>
>> + struct pci_bus *bus;
>
> Missed blank line.
>
>> + /*
>> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device
>> + * to have a quick look at it, before we hide it again.
>> + * Also grab the pci rescan lock so that device does not get discovered
>> + * and remapped while it is visible.
>> + * This code is inspired by drivers/mfd/lpc_ich.c
>> + */
>> + bus = pci_find_bus(0, 0);
>> + pci_lock_rescan_remove();
>> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
>> + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
>> +
>> + bar0 &= ~0xf;
>> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
>> + pci_unlock_rescan_remove();
>> +#endif /* CONFIG_PCI */
>> + return bar0;
>> +}
>> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
>
> Oy vey! I know what this is and let's do it differently. I have some
> (relatively old) patch series I can send you privately for testing.
This bit stood out the most to me too, it would be good if we can this fixed
in some cleaner work. So I'm curious how things will look with Andy's work
integrated.
Also I don't think this should be exported. Instead this (or its replacement)
should be used to get the address for an IOMEM resource to add the platform
devices when they are instantiated. Then the platform-dev drivers can just
use the regular functions to get their resources instead of relying on this
module.
Regards,
Hans
>
> ...
>
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>
> Not needed when you have SPDX.
>
> ...
>
>> +#include <linux/pci.h>
>
> Wrong header. Should be types.h
>
> ...
>
>> +#include <linux/dmi.h>
>> +#include <linux/platform_data/x86/simatic-ipc-base.h>
>
> Missed headers. You need to include ones that the code below is a
> direct user of.
>
> Like types.h
>
Am Thu, 4 Mar 2021 12:11:12 +0200
schrieb Andy Shevchenko <[email protected]>:
> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> <[email protected]> wrote:
> >
> > From: Henning Schild <[email protected]>
> >
> > This mainly implements detection of these devices and will allow
> > secondary drivers to work on such machines.
> >
> > The identification is DMI-based with a vendor specific way to tell
> > them apart in a reliable way.
> >
> > Drivers for LEDs and Watchdogs will follow to make use of that
> > platform detection.
>
> > Signed-off-by: Gerd Haeussler <[email protected]>
> > Signed-off-by: Henning Schild <[email protected]>
>
> The order is wrong taking into account the From line in the body. So,
> it's not clear who is the author, who is a co-developer, and who is
> the committer (one person may utilize few roles).
> Check for the rest of the series as well (basically this is the rule
> of thumb to recheck entire code for the comment you have got at any
> single place of it).
For some code Gerd is the author, and i am the co-Author. We even have
Jan in the mix at places. At least in copyright headers.
I will remain the committer for the three of us. And since i do not
know exactly what the problem is i will add only my Signed-off to avoid
confusion.
Please speak up it that would be wrong as well and point me to the docs
i missed. Or tell me how it should be done.
> ...
>
> > +config SIEMENS_SIMATIC_IPC
> > + tristate "Siemens Simatic IPC Class driver"
> > + depends on PCI
> > + help
> > + This Simatic IPC class driver is the central of several
> > drivers. It
> > + is mainly used for system identification, after which
> > drivers in other
> > + classes will take care of driving specifics of those
> > machines.
> > + i.e. leds and watchdogs
>
> LEDs
> watchdog. (missed period and singular form)
>
> Module name?
>
> > endif # X86_PLATFORM_DEVICES
>
> Not sure about the ordering of the section in Kconfig, but it is up to
> maintainers.
>
> ...
>
> > +# Siemens Simatic Industrial PCs
> > +obj-$(CONFIG_SIEMENS_SIMATIC_IPC) += simatic-ipc.o
>
> Ditto.
I will check both again if a find a pattern, otherwise will wait for
maintainers to complain and hopefully suggest.
> ...
>
> > + * Siemens SIMATIC IPC driver for LEDs
>
> It seems to be used in patch 4 which is not LED related. Also, why is
> it here if it's for LEDs?
>
> ...
>
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundation.
>
> Replace with SPDX
>
> ...
>
> > +#include <linux/module.h>
> > +#include <linux/kernel.h>
> > +#include <linux/platform_device.h>
>
> Ordered?
>
> > +#include <linux/platform_data/x86/simatic-ipc.h>
>
> ...
>
> > +static int register_platform_devices(u32 station_id)
> > +{
> > + int i;
> > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
>
> Reversed xmas tree order?
I do not get this, it is almost easter egg order time. Please explain.
> > + platform_data.devmode = SIMATIC_IPC_DEVICE_NONE;
> > +
> > + for (i = 0; i < ARRAY_SIZE(device_modes); i++) {
> > + if (device_modes[i].station_id == station_id) {
> > + ledmode = device_modes[i].led_mode;
> > + wdtmode = device_modes[i].wdt_mode;
> > + break;
> > + }
> > + }
> > +
> > + if (ledmode != SIMATIC_IPC_DEVICE_NONE) {
> > + platform_data.devmode = ledmode;
>
> > + ipc_led_platform_device =
> > platform_device_register_data
> > + (NULL, KBUILD_MODNAME "_leds",
> > PLATFORM_DEVID_NONE,
> > + &platform_data, sizeof(struct
> > simatic_ipc_platform));
>
> Strange indentation (second line).
>
> > + if (IS_ERR(ipc_led_platform_device))
> > + return PTR_ERR(ipc_led_platform_device);
>
> > + pr_debug(KBUILD_MODNAME ": device=%s created\n",
> > + ipc_led_platform_device->name);
>
> Utilize pr_fmt() instead of adding prefixes like this.
>
> > + }
>
> > + if (wdtmode != SIMATIC_IPC_DEVICE_NONE) {
> > + platform_data.devmode = wdtmode;
> > + ipc_wdt_platform_device =
> > platform_device_register_data
> > + (NULL, KBUILD_MODNAME "_wdt",
> > PLATFORM_DEVID_NONE,
> > + &platform_data, sizeof(struct
> > simatic_ipc_platform));
> > + if (IS_ERR(ipc_wdt_platform_device))
> > + return PTR_ERR(ipc_wdt_platform_device);
> > +
> > + pr_debug(KBUILD_MODNAME ": device=%s created\n",
> > + ipc_wdt_platform_device->name);
> > + }
>
> Same comments as above.
>
> > + if (ledmode == SIMATIC_IPC_DEVICE_NONE &&
> > + wdtmode == SIMATIC_IPC_DEVICE_NONE) {
> > + pr_warn(KBUILD_MODNAME
> > + ": unsupported IPC detected, station
> > id=%08x\n",
> > + station_id);
>
> Ugly indentation. With pr_fmt() in use will be much better.
>
> > + return -EINVAL;
>
> > + } else {
>
> Redundant.
>
> > + return 0;
> > + }
> > +}
>
> ...
>
> > +/*
> > + * Get membase address from PCI, used in leds and wdt modul. Here
> > we read
> > + * the bar0. The final address calculation is done in the
> > appropriate modules
>
> bar -> BAR
> Missed period.
>
> > + */
>
> > +
>
> Unneeded blank line.
>
> > +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> > +{
> > + u32 bar0 = 0;
>
> > +#ifdef CONFIG_PCI
>
> It's ugly besides the fact that you have a dependency.
left over from out-of-tree, will be removed
rest is clear, Thanks!
Henning
> > + struct pci_bus *bus;
>
> Missed blank line.
>
> > + /*
> > + * The GPIO memory is bar0 of the hidden P2SB device.
> > Unhide the device
> > + * to have a quick look at it, before we hide it again.
> > + * Also grab the pci rescan lock so that device does not
> > get discovered
> > + * and remapped while it is visible.
> > + * This code is inspired by drivers/mfd/lpc_ich.c
> > + */
> > + bus = pci_find_bus(0, 0);
> > + pci_lock_rescan_remove();
> > + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
> > + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0,
> > &bar0); +
> > + bar0 &= ~0xf;
> > + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
> > + pci_unlock_rescan_remove();
> > +#endif /* CONFIG_PCI */
> > + return bar0;
> > +}
> > +EXPORT_SYMBOL(simatic_ipc_get_membase0);
>
> Oy vey! I know what this is and let's do it differently. I have some
> (relatively old) patch series I can send you privately for testing.
>
> ...
>
> > + * This program is free software; you can redistribute it and/or
> > modify
> > + * it under the terms of the GNU General Public License version 2
> > as
> > + * published by the Free Software Foundation.
>
> Not needed when you have SPDX.
>
> ...
>
> > +#include <linux/pci.h>
>
> Wrong header. Should be types.h
>
> ...
>
> > +#include <linux/dmi.h>
> > +#include <linux/platform_data/x86/simatic-ipc-base.h>
>
> Missed headers. You need to include ones that the code below is a
> direct user of.
>
> Like types.h
>
Hi,
On 3/2/21 5:33 PM, Henning Schild wrote:
<snip>
> +static inline u32 simatic_ipc_get_station_id(u8 *data)
> +{
> + u32 station_id = SIMATIC_IPC_INVALID_STATION_ID;
> + int i;
> + struct {
> + u8 type; /* type (0xff = binary) */
> + u8 len; /* len of data entry */
> + u8 reserved[3];
> + u32 station_id; /* station id (LE) */
> + } __packed * data_entry = (void *)data;
> +
> + /* find 4th entry in OEM data */
> + for (i = 0; i < 3; i++)
> + data_entry = (void *)((u8 *)(data_entry) + data_entry->len);
> +
> + /* decode station id */
> + if (data_entry && data_entry->type == 0xff && data_entry->len == 9)
> + station_id = le32_to_cpu(data_entry->station_id);
> +
> + return station_id;
> +}
> +
> +static inline void
> +simatic_ipc_find_dmi_entry_helper(const struct dmi_header *dh, void *_data)
> +{
> + u32 *id = _data;
> +
> + if (dh->type != DMI_ENTRY_OEM)
> + return;
> +
> + *id = simatic_ipc_get_station_id((u8 *)dh + sizeof(struct dmi_header));
> +}
Please take dh->length into account here and make sure that you don't walk
past the end of the DMI tables during the parsing here.
Regards,
Hans
> +
> +#endif /* __PLATFORM_DATA_X86_SIMATIC_IPC_H */
>
On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <[email protected]> wrote:
> On 3/4/21 11:11 AM, Andy Shevchenko wrote:
> > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> > <[email protected]> wrote:
...
> >> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> >> +{
> >> + u32 bar0 = 0;
> >
> >> +#ifdef CONFIG_PCI
> >
> > It's ugly besides the fact that you have a dependency.
> >
> >> + struct pci_bus *bus;
> >
> > Missed blank line.
> >
> >> + /*
> >> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device
> >> + * to have a quick look at it, before we hide it again.
> >> + * Also grab the pci rescan lock so that device does not get discovered
> >> + * and remapped while it is visible.
> >> + * This code is inspired by drivers/mfd/lpc_ich.c
> >> + */
> >> + bus = pci_find_bus(0, 0);
> >> + pci_lock_rescan_remove();
> >> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
> >> + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
> >> +
> >> + bar0 &= ~0xf;
> >> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
> >> + pci_unlock_rescan_remove();
> >> +#endif /* CONFIG_PCI */
> >> + return bar0;
> >> +}
> >> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
> >
> > Oy vey! I know what this is and let's do it differently. I have some
> > (relatively old) patch series I can send you privately for testing.
>
> This bit stood out the most to me too, it would be good if we can this fixed
> in some cleaner work. So I'm curious how things will look with Andy's work
> integrated.
>
> Also I don't think this should be exported. Instead this (or its replacement)
> should be used to get the address for an IOMEM resource to add the platform
> devices when they are instantiated. Then the platform-dev drivers can just
> use the regular functions to get their resources instead of relying on this
> module.
I have published a WIP branch [1]. I have no means to test (I don't
know what hardware at hand I can use right now), but I made it compile
after 4 years of gathering dust...
Feel free to give any kind of comments or share your ideas on how it
can be improved (the above idea on IOMEM resource is interesting, but
devices are PCI, not sure how this can be done).
[1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
--
With Best Regards,
Andy Shevchenko
On Thu, Mar 4, 2021 at 9:52 PM Henning Schild
<[email protected]> wrote:
> Am Thu, 4 Mar 2021 12:11:12 +0200
> schrieb Andy Shevchenko <[email protected]>:
> > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> > <[email protected]> wrote:
...
> > Check for the rest of the series as well (basically this is the rule
> > of thumb to recheck entire code for the comment you have got at any
> > single place of it).
>
> For some code Gerd is the author, and i am the co-Author. We even have
> Jan in the mix at places. At least in copyright headers.
>
> I will remain the committer for the three of us. And since i do not
> know exactly what the problem is i will add only my Signed-off to avoid
> confusion.
>
> Please speak up it that would be wrong as well and point me to the docs
> i missed. Or tell me how it should be done.
Then make sure that you have From line with the Author (`git commit
--amend --author="..."`) and add your Co-developed-by tag.
...
> > > + int i;
> > > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> > > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> >
> > Reversed xmas tree order?
>
> I do not get this, it is almost easter egg order time. Please explain.
Longer lines
usually go
first.
See above :-)
--
With Best Regards,
Andy Shevchenko
Hi,
On 3/5/21 4:42 PM, Andy Shevchenko wrote:
> On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <[email protected]> wrote:
>> On 3/4/21 11:11 AM, Andy Shevchenko wrote:
>>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
>>> <[email protected]> wrote:
>
> ...
>
>>>> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
>>>> +{
>>>> + u32 bar0 = 0;
>>>
>>>> +#ifdef CONFIG_PCI
>>>
>>> It's ugly besides the fact that you have a dependency.
>>>
>>>> + struct pci_bus *bus;
>>>
>>> Missed blank line.
>>>
>>>> + /*
>>>> + * The GPIO memory is bar0 of the hidden P2SB device. Unhide the device
>>>> + * to have a quick look at it, before we hide it again.
>>>> + * Also grab the pci rescan lock so that device does not get discovered
>>>> + * and remapped while it is visible.
>>>> + * This code is inspired by drivers/mfd/lpc_ich.c
>>>> + */
>>>> + bus = pci_find_bus(0, 0);
>>>> + pci_lock_rescan_remove();
>>>> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
>>>> + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0, &bar0);
>>>> +
>>>> + bar0 &= ~0xf;
>>>> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
>>>> + pci_unlock_rescan_remove();
>>>> +#endif /* CONFIG_PCI */
>>>> + return bar0;
>>>> +}
>>>> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
>>>
>>> Oy vey! I know what this is and let's do it differently. I have some
>>> (relatively old) patch series I can send you privately for testing.
>>
>> This bit stood out the most to me too, it would be good if we can this fixed
>> in some cleaner work. So I'm curious how things will look with Andy's work
>> integrated.
>>
>> Also I don't think this should be exported. Instead this (or its replacement)
>> should be used to get the address for an IOMEM resource to add the platform
>> devices when they are instantiated. Then the platform-dev drivers can just
>> use the regular functions to get their resources instead of relying on this
>> module.
>
> I have published a WIP branch [1]. I have no means to test (I don't
> know what hardware at hand I can use right now), but I made it compile
> after 4 years of gathering dust...
So I took a quick look at the following 2 commits:
"platform/x86: p2sb: New Primary to Sideband bridge support library"
"mfd: lpc_ich: Switch to generic p2sb_bar()"
And this looks good to me, although compared to the code from this
patch-set you are missing the pci_lock_rescan_remove(); and
pci_unlock_rescan_remove(); calls.
> Feel free to give any kind of comments or share your ideas on how it
> can be improved (the above idea on IOMEM resource is interesting, but
> devices are PCI, not sure how this can be done).
The code added by this patch introduces a register_platform_devices()
function which creates a bunch of platform-devices; and then the
device-drivers for those call simatic_ipc_get_membase0() to get their
base-address.
My suggestion was to instead put the simatic_ipc_get_membase0() call
inside the code instantiating the platform devices and to add the
base-address for that pdev as IOMEM resource to the instantiated
platform-devices.
I hope this helps to clarify what I was trying to say.
Regards,
Hans
On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <[email protected]> wrote:
> On 3/5/21 4:42 PM, Andy Shevchenko wrote:
> > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <[email protected]> wrote:
> >> On 3/4/21 11:11 AM, Andy Shevchenko wrote:
> >>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> >>> <[email protected]> wrote:
...
> >>> Oy vey! I know what this is and let's do it differently. I have some
> >>> (relatively old) patch series I can send you privately for testing.
> >>
> >> This bit stood out the most to me too, it would be good if we can this fixed
> >> in some cleaner work. So I'm curious how things will look with Andy's work
> >> integrated.
> >>
> >> Also I don't think this should be exported. Instead this (or its replacement)
> >> should be used to get the address for an IOMEM resource to add the platform
> >> devices when they are instantiated. Then the platform-dev drivers can just
> >> use the regular functions to get their resources instead of relying on this
> >> module.
> >
> > I have published a WIP branch [1]. I have no means to test (I don't
> > know what hardware at hand I can use right now), but I made it compile
> > after 4 years of gathering dust...
>
> So I took a quick look at the following 2 commits:
(One of the latter commits moves the code to drivers/pci/pci-p2sb.c,
do you think it's better like that? The idea is to deduplicate
__pci_bus_read_base() call)
> "platform/x86: p2sb: New Primary to Sideband bridge support library"
> "mfd: lpc_ich: Switch to generic p2sb_bar()"
>
> And this looks good to me, although compared to the code from this
> patch-set you are missing the pci_lock_rescan_remove(); and
> pci_unlock_rescan_remove(); calls.
Oh, indeed.
> > Feel free to give any kind of comments or share your ideas on how it
> > can be improved (the above idea on IOMEM resource is interesting, but
> > devices are PCI, not sure how this can be done).
>
> The code added by this patch introduces a register_platform_devices()
> function which creates a bunch of platform-devices; and then the
> device-drivers for those call simatic_ipc_get_membase0() to get their
> base-address.
Sounds like an MFD approach...
> My suggestion was to instead put the simatic_ipc_get_membase0() call
> inside the code instantiating the platform devices and to add the
> base-address for that pdev as IOMEM resource to the instantiated
> platform-devices.
>
> I hope this helps to clarify what I was trying to say.
Yes, thanks!
--
With Best Regards,
Andy Shevchenko
Hi,
On 3/5/21 5:25 PM, Andy Shevchenko wrote:
> On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <[email protected]> wrote:
>> On 3/5/21 4:42 PM, Andy Shevchenko wrote:
>>> On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <[email protected]> wrote:
>>>> On 3/4/21 11:11 AM, Andy Shevchenko wrote:
>>>>> On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
>>>>> <[email protected]> wrote:
>
> ...
>
>>>>> Oy vey! I know what this is and let's do it differently. I have some
>>>>> (relatively old) patch series I can send you privately for testing.
>>>>
>>>> This bit stood out the most to me too, it would be good if we can this fixed
>>>> in some cleaner work. So I'm curious how things will look with Andy's work
>>>> integrated.
>>>>
>>>> Also I don't think this should be exported. Instead this (or its replacement)
>>>> should be used to get the address for an IOMEM resource to add the platform
>>>> devices when they are instantiated. Then the platform-dev drivers can just
>>>> use the regular functions to get their resources instead of relying on this
>>>> module.
>>>
>>> I have published a WIP branch [1]. I have no means to test (I don't
>>> know what hardware at hand I can use right now), but I made it compile
>>> after 4 years of gathering dust...
>>
>> So I took a quick look at the following 2 commits:
>
> (One of the latter commits moves the code to drivers/pci/pci-p2sb.c,
> do you think it's better like that? The idea is to deduplicate
> __pci_bus_read_base() call)
>
>> "platform/x86: p2sb: New Primary to Sideband bridge support library"
>> "mfd: lpc_ich: Switch to generic p2sb_bar()"
>>
>> And this looks good to me, although compared to the code from this
>> patch-set you are missing the pci_lock_rescan_remove(); and
>> pci_unlock_rescan_remove(); calls.
>
> Oh, indeed.
>
>>> Feel free to give any kind of comments or share your ideas on how it
>>> can be improved (the above idea on IOMEM resource is interesting, but
>>> devices are PCI, not sure how this can be done).
>>
>> The code added by this patch introduces a register_platform_devices()
>> function which creates a bunch of platform-devices; and then the
>> device-drivers for those call simatic_ipc_get_membase0() to get their
>> base-address.
>
> Sounds like an MFD approach...
Yes except that there does not seem to be a clear parent for
these devices, so it is MFD-ish.
IOW I'm not sure this should be changed to use the MFD framework,
but I was thinking along those line myself too.
Henning did you look into using the MFD framework + MFD cell
descriptions to instantiate the platform-devices for you ?
Regards,
Hans
On Fri, Mar 5, 2021 at 6:25 PM Andy Shevchenko
<[email protected]> wrote:
> On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <[email protected]> wrote:
> > On 3/5/21 4:42 PM, Andy Shevchenko wrote:
...
> > So I took a quick look at the following 2 commits:
>
> (One of the latter commits moves the code to drivers/pci/pci-p2sb.c,
> do you think it's better like that? The idea is to deduplicate
> __pci_bus_read_base() call)
>
> > "platform/x86: p2sb: New Primary to Sideband bridge support library"
> > "mfd: lpc_ich: Switch to generic p2sb_bar()"
> >
> > And this looks good to me, although compared to the code from this
> > patch-set you are missing the pci_lock_rescan_remove(); and
> > pci_unlock_rescan_remove(); calls.
>
> Oh, indeed.
Correction here, I'm using pci_bus_sem in the latest version.
--
With Best Regards,
Andy Shevchenko
Am Fri, 5 Mar 2021 17:46:08 +0200
schrieb Andy Shevchenko <[email protected]>:
> On Thu, Mar 4, 2021 at 9:52 PM Henning Schild
> <[email protected]> wrote:
> > Am Thu, 4 Mar 2021 12:11:12 +0200
> > schrieb Andy Shevchenko <[email protected]>:
> > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> > > <[email protected]> wrote:
>
> ...
>
> > > Check for the rest of the series as well (basically this is the
> > > rule of thumb to recheck entire code for the comment you have got
> > > at any single place of it).
> >
> > For some code Gerd is the author, and i am the co-Author. We even
> > have Jan in the mix at places. At least in copyright headers.
> >
> > I will remain the committer for the three of us. And since i do not
> > know exactly what the problem is i will add only my Signed-off to
> > avoid confusion.
> >
> > Please speak up it that would be wrong as well and point me to the
> > docs i missed. Or tell me how it should be done.
>
> Then make sure that you have From line with the Author (`git commit
> --amend --author="..."`) and add your Co-developed-by tag.
>
> ...
>
> > > > + int i;
> > > > + u8 ledmode = SIMATIC_IPC_DEVICE_NONE;
> > > > + u8 wdtmode = SIMATIC_IPC_DEVICE_NONE;
> > >
> > > Reversed xmas tree order?
> >
> > I do not get this, it is almost easter egg order time. Please
> > explain.
>
> Longer lines
> usually go
> first.
Henning
see
i
> See above :-)
>
On Fri, Mar 5, 2021 at 6:41 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Mar 5, 2021 at 6:25 PM Andy Shevchenko
> <[email protected]> wrote:
> > On Fri, Mar 5, 2021 at 6:14 PM Hans de Goede <[email protected]> wrote:
> > > On 3/5/21 4:42 PM, Andy Shevchenko wrote:
>
> ...
>
> > > So I took a quick look at the following 2 commits:
> >
> > (One of the latter commits moves the code to drivers/pci/pci-p2sb.c,
> > do you think it's better like that? The idea is to deduplicate
> > __pci_bus_read_base() call)
> >
> > > "platform/x86: p2sb: New Primary to Sideband bridge support library"
> > > "mfd: lpc_ich: Switch to generic p2sb_bar()"
> > >
> > > And this looks good to me, although compared to the code from this
> > > patch-set you are missing the pci_lock_rescan_remove(); and
> > > pci_unlock_rescan_remove(); calls.
> >
> > Oh, indeed.
>
> Correction here, I'm using pci_bus_sem in the latest version.
Okay, it seems that semaphore is not what I need and PCI rescan lock
is what is mandatory to take here.
--
With Best Regards,
Andy Shevchenko
Am Fri, 5 Mar 2021 17:42:42 +0200
schrieb Andy Shevchenko <[email protected]>:
> On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <[email protected]>
> wrote:
> > On 3/4/21 11:11 AM, Andy Shevchenko wrote:
> > > On Thu, Mar 4, 2021 at 8:36 AM Henning Schild
> > > <[email protected]> wrote:
>
> ...
>
> > >> +u32 simatic_ipc_get_membase0(unsigned int p2sb)
> > >> +{
> > >> + u32 bar0 = 0;
> > >
> > >> +#ifdef CONFIG_PCI
> > >
> > > It's ugly besides the fact that you have a dependency.
> > >
> > >> + struct pci_bus *bus;
> > >
> > > Missed blank line.
> > >
> > >> + /*
> > >> + * The GPIO memory is bar0 of the hidden P2SB device.
> > >> Unhide the device
> > >> + * to have a quick look at it, before we hide it again.
> > >> + * Also grab the pci rescan lock so that device does not
> > >> get discovered
> > >> + * and remapped while it is visible.
> > >> + * This code is inspired by drivers/mfd/lpc_ich.c
> > >> + */
> > >> + bus = pci_find_bus(0, 0);
> > >> + pci_lock_rescan_remove();
> > >> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x0);
> > >> + pci_bus_read_config_dword(bus, p2sb, PCI_BASE_ADDRESS_0,
> > >> &bar0); +
> > >> + bar0 &= ~0xf;
> > >> + pci_bus_write_config_byte(bus, p2sb, 0xE1, 0x1);
> > >> + pci_unlock_rescan_remove();
> > >> +#endif /* CONFIG_PCI */
> > >> + return bar0;
> > >> +}
> > >> +EXPORT_SYMBOL(simatic_ipc_get_membase0);
> > >
> > > Oy vey! I know what this is and let's do it differently. I have
> > > some (relatively old) patch series I can send you privately for
> > > testing.
> >
> > This bit stood out the most to me too, it would be good if we can
> > this fixed in some cleaner work. So I'm curious how things will
> > look with Andy's work integrated.
> >
> > Also I don't think this should be exported. Instead this (or its
> > replacement) should be used to get the address for an IOMEM
> > resource to add the platform devices when they are instantiated.
> > Then the platform-dev drivers can just use the regular functions to
> > get their resources instead of relying on this module.
>
> I have published a WIP branch [1]. I have no means to test (I don't
> know what hardware at hand I can use right now), but I made it compile
> after 4 years of gathering dust...
> Feel free to give any kind of comments or share your ideas on how it
> can be improved (the above idea on IOMEM resource is interesting, but
> devices are PCI, not sure how this can be done).
>
> [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
That is a little weird, might be a good idea to RFC reply to the cover
letter of this one. To allow review and discussion in a central place.
Henning
On Fri, Mar 5, 2021 at 6:47 PM Henning Schild
<[email protected]> wrote:
> Am Fri, 5 Mar 2021 17:42:42 +0200
> schrieb Andy Shevchenko <[email protected]>:
> > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <[email protected]>
> > wrote:
...
> > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
>
> That is a little weird, might be a good idea to RFC reply to the cover
> letter of this one. To allow review and discussion in a central place.
I'm now rebasing it to be more presentable.
If you can test this approach and it works for you, I'll send a formal
RFC series.
--
With Best Regards,
Andy Shevchenko
On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko
<[email protected]> wrote:
>
> On Fri, Mar 5, 2021 at 6:47 PM Henning Schild
> <[email protected]> wrote:
> > Am Fri, 5 Mar 2021 17:42:42 +0200
> > schrieb Andy Shevchenko <[email protected]>:
> > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede <[email protected]>
> > > wrote:
>
> ...
>
> > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
> >
> > That is a little weird, might be a good idea to RFC reply to the cover
> > letter of this one. To allow review and discussion in a central place.
>
> I'm now rebasing it to be more presentable.
> If you can test this approach and it works for you, I'll send a formal
> RFC series.
Okay, [1] now is in presentable shape, each patch with a proper commit
message and authorship, also all patches are compiled without issues.
--
With Best Regards,
Andy Shevchenko
Am Fri, 5 Mar 2021 19:44:57 +0200
schrieb Andy Shevchenko <[email protected]>:
> On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko
> <[email protected]> wrote:
> >
> > On Fri, Mar 5, 2021 at 6:47 PM Henning Schild
> > <[email protected]> wrote:
> > > Am Fri, 5 Mar 2021 17:42:42 +0200
> > > schrieb Andy Shevchenko <[email protected]>:
> > > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede
> > > > <[email protected]> wrote:
> >
> > ...
> >
> > > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
> > >
> > > That is a little weird, might be a good idea to RFC reply to the
> > > cover letter of this one. To allow review and discussion in a
> > > central place.
> >
> > I'm now rebasing it to be more presentable.
> > If you can test this approach and it works for you, I'll send a
> > formal RFC series.
>
> Okay, [1] now is in presentable shape, each patch with a proper commit
> message and authorship, also all patches are compiled without issues.
Thank you so much, i will base v2 on that and let you know how that
works.
regards,
Henning
On Mon, Mar 8, 2021 at 3:02 PM Henning Schild
<[email protected]> wrote:
> Am Fri, 5 Mar 2021 19:44:57 +0200
> schrieb Andy Shevchenko <[email protected]>:
> > On Fri, Mar 5, 2021 at 7:17 PM Andy Shevchenko
> > <[email protected]> wrote:
> > > On Fri, Mar 5, 2021 at 6:47 PM Henning Schild
> > > <[email protected]> wrote:
> > > > Am Fri, 5 Mar 2021 17:42:42 +0200
> > > > schrieb Andy Shevchenko <[email protected]>:
> > > > > On Thu, Mar 4, 2021 at 3:47 PM Hans de Goede
> > > > > <[email protected]> wrote:
...
> > > > > [1]: https://gitlab.com/andy-shev/next/-/tree/p2sb
> > > >
> > > > That is a little weird, might be a good idea to RFC reply to the
> > > > cover letter of this one. To allow review and discussion in a
> > > > central place.
> > >
> > > I'm now rebasing it to be more presentable.
> > > If you can test this approach and it works for you, I'll send a
> > > formal RFC series.
> >
> > Okay, [1] now is in presentable shape, each patch with a proper commit
> > message and authorship, also all patches are compiled without issues.
>
> Thank you so much, i will base v2 on that and let you know how that
> works.
I went ahead and submitted the series [2]. Feel free either to use the
last 7 patches from [1], or the series. In either case, if it works
for you I would expect the Tested-by tag given against _series_.
Thanks!
(Or comment there what is not working / needed for your case)
[2]: https://lore.kernel.org/linux-pci/[email protected]/T/#t
--
With Best Regards,
Andy Shevchenko